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

Duplicate Authorization headers should not be ignored. #45699

Closed
issuefiler opened this issue Dec 1, 2022 · 12 comments · Fixed by #45982
Closed

Duplicate Authorization headers should not be ignored. #45699

issuefiler opened this issue Dec 1, 2022 · 12 comments · Fixed by #45982
Labels
feature request Issues that request new features to be added to Node.js. http Issues or PRs related to the http subsystem.

Comments

@issuefiler
Copy link

issuefiler commented Dec 1, 2022

The context

A sender can send multiple Authorization headers in a request, because the Authorization header’s definition, credentials, allows multiple auth-params to be recombined as a comma-separated list.

RFC 9110 — HTTP semantics

a sender MUST NOT generate multiple field lines with the same name in a message (whether in the headers or trailers) or append a field line when a field line of the same name already exists in the message, unless that field's definition allows multiple field line values to be recombined as a comma-separated list (i.e., at least one alternative of the field's definition allows a comma-separated list, such as an ABNF rule of #(values) defined in Section 5.6.1).

Authorization = credentials
credentials = auth-scheme [ 1*SP ( token68 / #auth-param ) ]

Node.js MAY join them together with , .

RFC 9110 — HTTP semantics

A recipient MAY combine multiple field lines within a field section that have the same field name into one field line, without changing the semantics of the message, by appending each subsequent field line value to the initial field line value in order, separated by a comma (",") and optional whitespace (OWS, defined in Section 5.6.3). For consistency, use comma SP.


Currently, Node.js ignores duplicate Authorization headers when it creates message.headers.

Node.js 19.2.0 documentation — HTTP

  • Duplicates of age, authorization, content-length, content-type, etag, expires, from, host, if-modified-since, if-unmodified-since, last-modified, location, max-forwards, proxy-authorization, referer, retry-after, server, or user-agent are discarded.
  • For all other headers, the values are joined together with , .

Suggestion

I suggest Node.js join the field line values of multiple Authorization headers in a request with , , instead of ignoring them, when it creates message.headers.

The related issue

#3591

@issuefiler issuefiler added the feature request Issues that request new features to be added to Node.js. label Dec 1, 2022
@aduh95
Copy link
Contributor

aduh95 commented Dec 1, 2022

Can you share a minimal repro, the expected result, and the actual result?

@issuefiler
Copy link
Author

Proof-of-concept

import Network from "node:net";
import HTTP from "node:http";

const server = HTTP.createServer(request => console.log(request.headers))
	.listen(_ => {
		const client = Network.createConnection(
			server.address().port,
			_ => client.write(
				"GET / HTTP/1.1\r\n"
				+ `Authorization: Digest username="ha"\r\n`
				+ `Authorization: realm="no"\r\n\r\n`
			)
		);
	});

The actual output

{ authorization: 'Digest username="ha"' }

The expected output

{ authorization: 'Digest username="ha", realm="no"' }

@aduh95.

@issuefiler
Copy link
Author

But now that I think of it, splitting the Authorization header field value like this (Authorization: realm="no") could be violation of the RFC, Authorization = credentials.

@aduh95
Copy link
Contributor

aduh95 commented Dec 1, 2022

@nodejs/http

@issuefiler
Copy link
Author

RFC 9110 — HTTP semantics

5.2. Field Lines and Combined Field Value

Field sections are composed of any number of “field lines,” each with a “field name” (see Section 5.1) identifying the field, and a “field line value” that conveys data for that instance of the field.

…When a field name is repeated within a section, its combined field value consists of the list of corresponding field line values within that section, concatenated in order, with each field line value separated by a comma.

For example, this section:

Example-Field: Foo, Bar
Example-Field: Baz

contains two field lines, both with the field name “Example-Field.” The first field line has a field line value of “Foo, Bar,” while the second field line value is “Baz.” The field value for “Example-Field” is the list “Foo, Bar, Baz.”

11.6.2. Authorization

…Its value consists of credentials containing the authentication information of the user agent for the realm of the resource being requested.

Authorization = credentials

I am not sure whether the “value” in the section §11.6.2 means a “(combined) field value” or “field line value.”

If it means the former,

Authorization: Digest username="ha"
Authorization: realm="no"

would be valid (meaning, Node.js should respect multiple occurrences of Authorization headers),

and if it means the latter, invalid (meaning, Node.js is fine as is).

@ShogunPanda
Copy link
Contributor

According to section https://www.rfc-editor.org/rfc/rfc9110#name-changes-from-rfc-7230, if there is no clear indication of "field line value" then it refers to combined value.

Now, AFAIK Node implements RFC 7230 and not RFC 9110, so I wonder whether we should implement this or not.

@nodejs/http @nodejs/tsc WDYT?

@bnoordhuis
Copy link
Member

One problem I see is there's a non-zero risk of breaking existing programs if we make this change.

@mcollina
Copy link
Member

mcollina commented Dec 4, 2022

If we want to support this it would be behind an option.

@marco-ippolito
Copy link
Member

marco-ippolito commented Dec 12, 2022

We can leave the default behaviour as it is and implement a flag in http.createServer() like allowDuplicateHeaders and combine them with ,. Maybe in a future release this can become the standard behaviour.
WDYT?

@issuefiler
Copy link
Author

That sounds good to me. The future documentation on that option would refer to RFC 9110.

@mcollina
Copy link
Member

As I said, I would recommend adding an option and possibly documenting it as experimental. I don't have much time to look into RFC 9110 and how it changed things.

Note that a lot of old clients and servers are very lenient on HTTP semantics, so they can expect whatever.

@marco-ippolito
Copy link
Member

I will open a PR

@addaleax addaleax added the http Issues or PRs related to the http subsystem. label Dec 26, 2022
nodejs-github-bot pushed a commit that referenced this issue Jan 3, 2023
PR-URL: #45982
Fixes: #45699
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
RafaelGSS pushed a commit to RafaelGSS/node that referenced this issue Jan 17, 2023
PR-URL: nodejs#45982
Fixes: nodejs#45699
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
RafaelGSS pushed a commit to RafaelGSS/node that referenced this issue Jan 17, 2023
PR-URL: nodejs#45982
Fixes: nodejs#45699
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
RafaelGSS pushed a commit that referenced this issue Jan 20, 2023
PR-URL: #45982
Backport-PR-URL: #46240
Fixes: #45699
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
juanarbol pushed a commit that referenced this issue Jan 26, 2023
PR-URL: #45982
Fixes: #45699
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
juanarbol pushed a commit that referenced this issue Jan 31, 2023
PR-URL: #45982
Fixes: #45699
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants