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

[bugfix] Sending Accept-Charset causes 406 NotAcceptable response #3013

Merged

Conversation

PanHNE
Copy link
Contributor

@PanHNE PanHNE commented Jul 6, 2023

No description provided.

@PanHNE PanHNE added the bug Something isn't working label Jul 6, 2023
@PanHNE PanHNE linked an issue Jul 6, 2023 that may be closed by this pull request
// mozilla says servers should ignore the Accept-Charset header: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept-Charset
val hasAcceptCharset = ctx.request.header(HeaderNames.AcceptCharset).nonEmpty

if (hasMatchingRepresentation || hasAcceptCharset) endpointHandler.onDecodeSuccess(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this fix ignores the accept-charset header, it seems to bypass content negotiation when this header is present?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. It checks to see if we have that header and if we have, we move on. I didn't have any other idea about it, maybe a hint for better solution?

Copy link
Member

Choose a reason for hiding this comment

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

sorry, I have no idea, I would have to go and try to fix the issue myself ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, so I will think and try another solution ;)

@PanHNE PanHNE marked this pull request as draft July 6, 2023 12:04
@PanHNE PanHNE marked this pull request as ready for review July 12, 2023 05:22
@PanHNE PanHNE requested review from adamw and kciesielski July 12, 2023 05:22
@adamw
Copy link
Member

adamw commented Jul 12, 2023

Sorry but it most probably still is the wrong solution. You are filtering out accept-charset, but in a different place.

The first thing that's lacking in the PR is a test, that would demonstrate the bug. Let's start with that, probably in ServerContentNegotiationTests, and we'll see how this can be fixed after the test is in place.

@PanHNE PanHNE force-pushed the 2994_Sending_Accept-Charset_causes_406_NotAcceptable_response branch from 1fa020a to ec944d9 Compare July 13, 2023 14:30
@adamw adamw merged commit c6df0a2 into master Jul 21, 2023
@mergify mergify bot deleted the 2994_Sending_Accept-Charset_causes_406_NotAcceptable_response branch July 21, 2023 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sending Accept-Charset causes 406 NotAcceptable response
2 participants