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: cors should be at header_filter phase #1915

Closed
JanLi-air opened this issue Jul 28, 2020 · 6 comments
Closed

bug: cors should be at header_filter phase #1915

JanLi-air opened this issue Jul 28, 2020 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@JanLi-air
Copy link

Issue description

the cors headers can be overridden by upstreams if it's put at rewrite phase.

Environment

  • apisix version (cmd: apisix version): 1.4
  • OS: Linux

Minimal test code / Steps to reproduce the issue

  1. Create a backend which override the cors headers, such as Access-Control-Allow-Origin
  2. Configure cors plugin to proxy to the upstream setup in step 1
  3. Request the uri configured

What's the actual result? (including assertion message & call stack if applicable)

the cors header will be determined by the upstream

What's the expected result?

the cors header should be determined by the cors plugin.

This is related to antoher issue: #1528

I don't think it's good practice to change the phase of cors plugin to make sure it works with the auth plugins.
The cors plugin worked in header_filter phase since it needs to rewrite those headers after apisix gets response from upstream. If we change it to be at the rewrite phase, it's possible the header got overridden by upstream or the header is discarded somewhere.

@membphis membphis added the bug Something isn't working label Jul 28, 2020
@membphis
Copy link
Member

membphis commented Jul 28, 2020

@ShiningRush do you have time to look at this issue?

@membphis
Copy link
Member

@JanLi-air many thx for your report this bug

@ShiningRush
Copy link
Contributor

It related with #1704, I will fix it in this week.
I think user should not concern at which phase cors plugin work, they just care whether the cors plugin overwrite upstream's header.
BTW: make sure cors plugin work fine with auth plugins is a better way, because the Browser just report the cors error when authenticated failed.
It is unfriendly to front-end engineer

@JanLi-air
Copy link
Author

if cors plugin needs to work with auth plugins, i think we can set header both at header_filter phase and after auth failure.
But since auth plugin will return before cors plugin, and we need to keep plugins independent with each other, we'd better write cors headers at both rewrite phase and header_filter phase.

@ShiningRush
Copy link
Contributor

Yes, you are right : )
The cors plugin will write header in rewrite phase or both rewrite and header_filter phase, it depend on is_overwrite_upstream.
But now here is a strange problem that it seems if we write header in rewrite phase and upstream also write header , client will get a duplicate headers, it confuse me.

@JanLi-air
Copy link
Author

it depends on how we write the header. because in http you can have multiple headers with identical name, so if you want to override you need to first delete the existing one.

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

No branches or pull requests

3 participants