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

[BUG] Overriding of "Access-Control-Expose-Headers" when #95

Open
rpep opened this issue Mar 2, 2023 · 3 comments
Open

[BUG] Overriding of "Access-Control-Expose-Headers" when #95

rpep opened this issue Mar 2, 2023 · 3 comments
Labels
bug Something isn't working

Comments

@rpep
Copy link

rpep commented Mar 2, 2023

Describe the bug
It is sometimes necessary to modify the Access-Control-Expose-Headers within the request cycle. As an example, a developer might need to set the Content-Disposition and Content-Type when returning a file from an API so that it has a name, for e.g.:

Content-Disposition: attachment; filename="myfile.csv"
Content-Type:  text/csv; charset=utf-8
Correlation-ID: b038c7a8662f4d21962c80ef894d0946
Access-Control-Expose-Headers: Content-Type Content-Disposition Correlation-ID

We came across a bug in production with the way Django-GUID implements the EXPOSE_HEADER setting, in that on the outgoing request processing, it overrides any setting set by the user in the request flow, because it replaces the already set Access-Control-Expose-Headers rather than being additive.

To Reproduce

  • Create a view in Django
  • Set "Access-Control-Expose-Headers" to some value within the view
  • Set Django GUID setting "EXPOSE_HEADER" to "True"
  • Make an API request to the view, look at the request, and see that the developer set value has been overwritten.

Full stack trace
N/A

@rpep rpep added the bug Something isn't working label Mar 2, 2023
@JonasKs
Copy link
Member

JonasKs commented Mar 2, 2023

We just removed this setting in asgi-correlation-id. See this discussion

I’d recommend turning the setting off, and configure these settings in your Django CORS middleware instead. 😊 PR to remove the setting and updating docs welcome, of course!

@rpep
Copy link
Author

rpep commented Mar 2, 2023

Ha, fair enough. Will put together that PR then and close the fix

@JonasKs
Copy link
Member

JonasKs commented Mar 2, 2023

Thank you 😊 I won't be able to look until Saturday/Sunday, but I'll make sure we'll get a release out soon enough. In the meantime, I'd recommend just fixing CORS manually and not enabling that setting 😊

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 a pull request may close this issue.

2 participants