Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix encoding of client id and secret in HTTP Basic #171

Merged
merged 1 commit into from
Feb 8, 2025

Conversation

p2004a
Copy link
Contributor

@p2004a p2004a commented Feb 8, 2025

Per https://datatracker.ietf.org/doc/html/rfc6749#section-2.3.1 the parameters first need to be url encoded.

@evert
Copy link
Collaborator

evert commented Feb 8, 2025

Definitely missed that! Thank you

@evert evert merged commit 24ae1c5 into badgateway:main Feb 8, 2025
4 checks passed
@panva
Copy link

panva commented Feb 8, 2025

This implementation is unfortunately not correct.

Please see a reference of how it could be done properly.

@p2004a
Copy link
Contributor Author

p2004a commented Feb 8, 2025

I will open next PR to make it fully compliant.

@evert
Copy link
Collaborator

evert commented Feb 8, 2025

Reading the appendix, I fail to see what's wrong (but might be missing something) the implementation you shared encodes space as + and encodes more characters than encodeURIComponent, but that doesn't mean it's incorrect for x-www-form-urlencoded.

I'd love to see a source that supports this requirement, I just don't get that from the appendix alone. (it also feels a bit silly, only non-ascii and : really matters in this context). Without a stronger source, I rather stick with the implementation from this PR. Less is more!

@evert
Copy link
Collaborator

evert commented Feb 8, 2025

I appreciate the flag though @panva !

@p2004a
Copy link
Contributor Author

p2004a commented Feb 9, 2025

AFAIU the scheme of encoding as defined in the spec is fully deterministic, so I can imagine a hypothetical case of the authorization server doing a string comparison on encoded, not decoded values.

@evert
Copy link
Collaborator

evert commented Feb 9, 2025

It feels a tad theoretical, but would new URLSearchParams({v: input}).toString().slice(2) work?

@panva
Copy link

panva commented Feb 9, 2025

Out of the alphabet that needs escaping encodeURIComponent omits escaping - _ . ! ~ * ' ( ) and it uses %20 instead of + for (U20 SPACE).

Using encodeURIComponent produces incomplete and in the case of space also incorrect, x-www-form-urlencoded encoding.

@p2004a p2004a deleted the fix-basic-auth-encoding branch February 9, 2025 11:37
@p2004a
Copy link
Contributor Author

p2004a commented Feb 9, 2025

This is a mess IMHO...

The primary difference is that https://datatracker.ietf.org/doc/html/rfc6749#appendix-B explicitly says

the "application/x-www-form-urlencoded" media type was defined in Section 17.13.4 of [W3C.REC-html401-19991224]

which says (emphasis mine):

Control names and values are escaped. Space characters are replaced by '+', and then reserved characters are escaped as described in [RFC1738], section 2.2: Non-alphanumeric characters are replaced by '%HH', a percent sign and two hexadecimal digits representing the ASCII code of the character. Line breaks are represented as "CR LF" pairs (i.e., '%0D%0A').

Now, things like URLSearchParams percent-encode anything in the application/x-www-form-urlencoded percent-encode set as defined by living spec which also allows * - . _ to be not encoded.

So, effectively, to be the most strict interpretation requires encoding everything non-alphanumeric (and special treatment of space...)

@panva
Copy link

panva commented Feb 9, 2025

Yes it is a mess, which is why we're switching from

OAuth 2.0 Basic (MUST support) and Body (MAY, NOT RECOMMENDED)

to

OAuth 2.1 Body (MUST support) and Basic (MAY, NOT RECOMMENDED)

oauth-wg/oauth-v2-1#128

p2004a added a commit to p2004a/oauth2-client that referenced this pull request Feb 9, 2025
Amends 49d3241 per discussion in
badgateway#171 with the most
strict interpretation of required encoding for the HTTP Basic
Authentication user and password.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants