search

jshttp / basic-auth

Generic basic auth Authorization header field parser
MIT License
702 stars 86 forks source link

Disallow control characters in `user-id` and `password` to be RFC-compliant. #55

Open issuefiler opened 2 years ago

issuefiler commented 2 years ago

https://github.com/jshttp/basic-auth/blob/e8a29f94dc7c05b5858b08090386338af010ce49/index.js#L46 https://github.com/jshttp/basic-auth/blob/e8a29f94dc7c05b5858b08090386338af010ce49/index.js#L114-L115

Which, RFC 7617 forbids.

RFC 7617 — “Basic” HTTP Authentication Scheme

The user-id and password MUST NOT contain any control characters (see “CTL” in Appendix B.1 of RFC5234).

dougwilson commented 2 years ago

Hello, and thank you for your issue. You are correct regarding RFC 7617, although this module is written to RFC 2617, as this module was created prior to the existence of 7617.

It is possible that a future version of this module can update to instead follow RFC 7617 as a breaking change, though I don't see the exact purpose to make it more strict and break existing applications.

We could always add an option to the parser to choose with RFC to follow, would that be of interest to you?

dougwilson commented 2 years ago

I do see that TEXT is defined as follows, however:

       TEXT           = <any OCTET except CTLs,
                        but including LWS>

which looks like it also excludes certain CTL characters. I think we can release a 2.0 that adds this restriction. I need to think about if it should returned undefined or throw under this condition. Not sure if you have any thoughts on it or not.

issuefiler commented 2 years ago

That’s right: RFC 2617 forbids all control characters but linear whitespace, and its successor, RFC 7617, simply forbids all of them including linear whitespace. Hence the original title of this issue.

I was just reporting it to inform you, not expecting an immediate “fix.” With this notice, you can do as you please with your package.

Releasing a new version with this restriction sounds good, by the way.

issuefiler commented 2 years ago

If you don’t mind, may I change the title back?

issuefiler commented 2 years ago

And I believe it should return undefined, not throw, just like how invalid, malformed Authorization header field values are currently handled, so that it can be handled in the same manner (with 401 Unauthorized responses).

https://github.com/jshttp/basic-auth/blob/e8a29f94dc7c05b5858b08090386338af010ce49/index.js#L107-L112

RFC 9110 — HTTP semantics

11.4. Credentials

Upon receipt of a request for a protected resource that omits credentials, contains invalid credentials (e.g., a bad password) or partial credentials (e.g., when the authentication scheme requires more than one round trip), an origin server SHOULD send a 401 Unauthorized response that contains a WWW-Authenticate header field with at least one (possibly new) challenge applicable to the requested resource.

issuefiler commented 2 years ago

I also doubt the need for “an option for the parser to choose between the RFC 2617 and RFC 7617 modes,” because,

compared to RFC 7617, the characters that RFC 2617 additionally supports in user-id and password are:

which, innocent, sane users wouldn’t use in their usernames and passwords to sign in. Following the obsolete standard, RFC 2617, only benefits malicious attackers.

Internet Engineering Task Force (IETF)                        J. Reschke
Request for Comments: 7617                                    greenbytes
Obsoletes: 2617                                           September 2015
UlisesGascon commented 1 month ago

Sorry I accidentally closed this issue