Fix issue with empty values within delimited authorization header

[ezekg]

Motivation / Background

This Pull Request has been created because certain delimited Authorization header values can be used to produce an ArgumentError, typically resulting in a 500 error response.

Detail

This Pull Request changes the token_and_options method to remove blank values, which cause the argument error. Typically, raw_params returns an array of tuples, but when an authorization header contains blank values, such as with Bearer foo,,bar (note the blank value, between the 2 , delimiters), the raw_params method will include non-tuple values. These values are then passed to Hash[], but that method only accepts sets of 1 or 2 arguments, not 0. Unfortunately, this ends up raising an ArgumentError: invalid number of elements (0 for 1..2).

Additional information

N/A

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[zzak]

Does it say anywhere in the spec what we should do with blank values? Throwing them away makes sense, but I wanted to double check.

[ezekg]

The spec does say that empty elements in a header value are valid:

Then the following are valid values for example-list (not including the double quotes, which are present for delimitation only):

"foo,bar"
"foo ,bar,"
"foo , ,bar,charlie"

But they're not supported with the current parsing code for the Authorization header, causing an error. And I can't think of a way to map empty values to the options hash, which is what these extra elements turn into. So I vote to throw the empty elements out for authenticate_with_http_token and friends. If the empty values are needed, the Authorization header can be read and parsed manually viarequest.headers['Authorization'].

We already cast missing values to nil, e.g. foo=, bar="" => { 'foo' => nil, 'bar' => '' }.

[ezekg]

@zzak I kind of changed my mind, so I refactored the code to retain empty header values, per the HTTP spec. I like this version better, but let me know if you'd like me to revert to the earlier solution using compact_blank.

[zzak]

I think sticking to the spec is the best path forward, let's wait to hear what other committers/core think 🙏 Thanks for digging into this further!

[jonathanhefner]

Does it say anywhere in the spec what we should do with blank values? Throwing them away makes sense, but I wanted to double check.

The spec does say that empty elements in a header value are valid:

However, the paragraph at the beginning of that section says:

Empty elements do not contribute to the count of elements present. A recipient MUST parse and ignore a reasonable number of empty list elements: enough to handle common mistakes by senders that merge values, but not so much that they could be used as a denial-of-service mechanism.

[ezekg]

All empty values (,,, , ,, , ,,etc.) are merged into the "" key, so I feel that is a reasonable compromise.

But let me know which direction we want to go in and I'll get the PR fixed up.

[jonathanhefner]

All empty values (,,, , ,, , ,,etc.) are merged into the "" key, so I feel that is a reasonable compromise.

Is there a benefit to including the values in the Hash, instead of ignoring them per the spec?

[ezekg]

Not that I can think of. I'll revert to my original changeset.

[jonathanhefner]

I pushed a commit to use reject!(&:empty?) instead of compact_blank. That way we avoid checking the blank? regexp on each string in the common case, and we avoid an array allocation. Also, I rebased to fix the build errors.

Thank you, @ezekg! ⚪ ⚫ ⚪