thephpleague / uri-src

URI manipulation Library
https://uri.thephpleague.com
MIT License
30 stars 7 forks source link

Incorrect encoding of commas in query string #115

Closed GrahamCampbell closed 1 year ago

GrahamCampbell commented 1 year ago

Bug Report

(Fill in the relevant information below to help triage your issue.)

Information Description
Package league/uri
Version 7.1.0
PHP version 8.2.8
OS Platform Linux

Summary

Commas are not correctly encoded, in expansion. AWS CloudFront rejects the URLs generated by this package as invalid.

Standalone code, or other way to reproduce the problem

$template = new League\Uri\UriTemplate('https://example.com/{?list}');
$uri = $template->expandOrFail(['list' => ['a', 'b']]);
echo $uri->toString();

Expected result

https://example.com/?list=a%2Cb

Actual result

https://example.com/?list=a,b
nyamsprod commented 1 year ago

@GrahamCampbell thanks for the report. The package follows and successfully passes the uritemplate-tests independent test suite specifically

If I "fix" to the expected behaviour then my tests will fail the independent tests. So unless I am misreading the tests, I would say that AWS CloudFront is in the wrong 🤔.

Also according to RFC3986

URI producing applications should percent-encode data octets that correspond to characters in the reserved set unless these characters are specifically allowed by the URI scheme to represent data in that component. If a reserved character is found in a URI component and no delimiting role is known for that character, then it must be interpreted as representing the data octet corresponding to that character's encoding in US-ASCII.

In this case the , which is a reserved characters serve a specific role to allow delimiting variables so IMHO this is a second argument as to why in this specific scenario, it should not be URL encoded.

GrahamCampbell commented 1 year ago

OK, this is fair. It seems in practice, nobody implements this stuff the same, regardless of what the spec says, lol.