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

For proxied responses, we have been combining headers which we need to dedupe #189

Merged
merged 3 commits into from
Oct 31, 2023

Conversation

JymDyerIBI
Copy link
Contributor

Of the duplicates, "Access-Control-Allow-Origin" is the most problematic because browsers stop on that as an error.

Checklist

  • Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
  • Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
  • The description lists all applicable issues this PR seeks to resolve
  • The description lists any configuration setting(s) that differ from the default settings
  • All tests and CI builds passing

Description

When we proxy a POST request through the middleware we have request headers from the middleware's Spark Framework and we update them with the request headers from the GraphQL endpoint being proxied to. Unfortunately the com.sparkjava.spark.Response#header() method doesn't let you update a header, it creates duplicate headers. So I added a filter to prevent that.

Apparently the RFC for response headers is unclear (and thus undefined) on the legality of duplicate headers, but browsers specifically fail on duplicate Access-Control-Allow-Origin headers.

…e duplicates ("Access-Control-Allow-Origin" is the most problematic). So dedupe them.
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

Good finding!

Copy link
Contributor

@leonardehrenfried leonardehrenfried left a comment

Choose a reason for hiding this comment

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

This code looks fine but I can also make the CORS headers optional in OTP itself (but will increase the risk of a misconfiguration breaking a deployment.)

@JymDyerIBI JymDyerIBI merged commit 1abd925 into dev Oct 31, 2023
2 checks passed
@binh-dam-ibigroup binh-dam-ibigroup deleted the dedupe-http-response-headers branch October 31, 2023 21:05
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.

3 participants