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

Stop accepting repeated, duplicate Authorization headers. #3186

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

branlwyd
Copy link
Member

@branlwyd branlwyd commented Jun 3, 2024

The issue that led to us accepting this behavior has been fixed. Given that this behavior is not RFC-compliant, we remove it from Janus.

The issue that led to us accepting this behavior has been fixed. Given
that this behavior is not RFC-compliant, we remove it from Janus.
@branlwyd branlwyd requested a review from a team as a code owner June 3, 2024 17:55
@branlwyd
Copy link
Member Author

branlwyd commented Jun 3, 2024

I might wait a few more days/another release before merging this. I'm not totally convinced we are out of the woods yet (though it is likely).

Closes #3163.

Copy link
Contributor

@tgeoghegan tgeoghegan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is sound and does what it's intended to. But WDYT about instead adding a configuration parameter for accepting duplicate headers, so that we could first verify that the issue is resolved, and then only yank support once we're confident? On the other hand, I suppose rolling Janus back/forward to/from a version that accepts duplicate headers is about as difficult as changing a configuration value.

@branlwyd
Copy link
Member Author

branlwyd commented Jun 3, 2024

I don't think this issue is worth a config value: in the long run, we don't want to support this behavior at all, we only do so now because doing so allowed us to quickly work around a production issue. A config option effectively saying "do something RFC noncompliant", which no one activates or wants to activate, is not something I'd like to keep around long-term.

I do think it's a good idea to wait a bit (one more release, in practice?) before we yank support for this, for the reason you mention. I'm also not totally confident we are out of the woods.

@inahga
Copy link
Contributor

inahga commented Jun 20, 2024

We're OK with yanking support now.

@inahga inahga merged commit 99775e1 into main Jun 20, 2024
9 checks passed
@inahga inahga deleted the bran/revert-dup-auth branch June 20, 2024 18:29
@branlwyd
Copy link
Member Author

Er, were we? I think we were waiting on seeing additional traffic in the affected deployment to be sure. That said, rolling this back (in the unlikely event it is needed to do so) is not so hard, so I think I'm OK letting this roll forward.

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.

None yet

4 participants