benoitc / hackney

simple HTTP client in Erlang
Other
1.34k stars 427 forks source link

Fix urlencode bug #597

Closed alexandrejbr closed 4 years ago

alexandrejbr commented 4 years ago

The urlencode seems to be broken for a couple of characters. Seems like it's a typo, there a , instead of ;.

benoitc commented 4 years ago

why broken?

alexandrejbr commented 4 years ago

()!$ are being percent encoded. According to RFC3986 I believe they don't need to be and I believe that not percent encoding them may have been the intention of the author of the code, since C being $ and simultaneously ( or ) or ! is hard to fulfil.

benoitc commented 4 years ago

hrmm i will check. we need to be careful there as this code is there since a long time and that issue had never been raised before. which code author you talking btw?

On Sat 26 Oct 2019 at 08:11, Alexandre Rodrigues notifications@github.com wrote:

()!$ are being percent encoded. According to RFC3986 https://tools.ietf.org/html/rfc3986 I believe they don't need to be and I believe that not percent encoding them may have been the intention of the author of the code, since C being $ and simultaneously ( or ) or ! is hard to fulfil.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/benoitc/hackney/pull/597?email_source=notifications&email_token=AAADRISYFCNSDO46QE4HHC3QQPNSXA5CNFSM4JFD47MKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECKA5KI#issuecomment-546573993, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAADRIVOSZNYLJXCE4UKCVLQQPNSXANCNFSM4JFD47MA .

-- Sent from my Mobile

alexandrejbr commented 4 years ago

I'm trying to put myself in the shoes of the person who wrote this line of code. Seems hard to believe that that person wanted to have a clause that will never match. I don't know who wrote that line and it doesn't matter actually, I believe it's a typo. , is very close in the keyboard to ;.

It's a corner case, I only discovered it thanks to property-based tests.

benoitc commented 4 years ago

sorry i misread the change... This should only be one character change there anyway. Thanks for the fix