refactor: cleanup the code for CORS handling #1959
Conversation
harsimranmaan
changed the title
There was a problem hiding this comment.
Thank you, and sorry for the super late review - this got lost in my inbox. The changes look good to me, I think I also identified a test case which should reproduce #1754. Would you be open to continue working on that? I promise that my next review will be much faster ;)
| code: http.StatusNotImplemented, | ||
| header: http.Header{"Origin": {"http://client-app.example.com"}, "Authorization": {"Basic Zm9vLTU6YmFy"}}, | ||
| expectHeader: http.Header{"Access-Control-Allow-Credentials": []string{"true"}, "Access-Control-Allow-Origin": []string{"http://client-app.example.com"}, "Access-Control-Expose-Headers": []string{"Content-Type"}, "Vary": []string{"Origin"}}, | ||
| }, |
There was a problem hiding this comment.
I think the bug that was reported would be reproduced like this (checking against the origin domain, not the server one):
{
d: "should accept when basic auth client exists and origin is set at the client as well as the server",
prep: func() {
viper.Set("serve.public.cors.enabled", true)
viper.Set("serve.public.cors.allowed_origins", []string{"http://**.example.com"})
r.ClientManager().CreateClient(context.Background(), &client.Client{ID: "foo8", Secret: "bar", AllowedCORSOrigins: []string{"http://myapp.example.biz"}})
},
code: http.StatusNotImplemented,
header: http.Header{"Origin": {"http://myapp.example.biz"}, "Authorization": {"Basic Zm9vLTU6YmFy"}},
expectHeader: http.Header{"Access-Control-Allow-Credentials": []string{"true"}, "Access-Control-Allow-Origin": []string{"http://client-app.example.com"}, "Access-Control-Expose-Headers": []string{"Content-Type"}, "Vary": []string{"Origin"}},
},
There was a problem hiding this comment.
Lemme give it a try. I expect the Allow-Origin to be set to http://myapp.example.biz.
There was a problem hiding this comment.
@aeneasr I could not repro the defect. Added the new test-case. Also refactored auth headers(in test cases) for readability.
There was a problem hiding this comment.
Ok, in that case I don't think there's any more to do! And you are correct, the expected value was a copy/paste mistake on my end!
harsimranmaan
force-pushed
the
cors
branch
2 times, most recently
from
764a7bd to
7afe15d
Compare
|
Awesome 🎉 Thank you for your contribution! |
Related issue
#1754
Proposed changes
This change was originally intended to fix the issue described in the bug. However, I am not able to repro it as shown in the newly added test.
Checklist
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further comments
I removed many extra loops for validating the origin against the CORS allowlist. The viper config was shared across test cases causing dependency on the order of the test cases. It also hid some assumptions about the setup.