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

fix: cors headers not added to the response #2922 #2934

Merged

Conversation

kstasik
Copy link
Contributor

@kstasik kstasik commented Dec 6, 2022

CORS response headers not added to the response:

access-control-allow-credentials: true
access-control-allow-origin: ...
access-control-expose-headers: Content-Type, Set-Cookie

Related issue(s)

#2922

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

I just wanted to quickly fix the issue. I noticed that upgrading the docker image breaks CORS. Checked the changes before releases and reverted the cause of the problem.

BTW: I noticed that response headers are added by the middleware but they disappear somehow during the processing of the request:

[cors] 2022/12/06 22:12:48   Actual response added headers: map[Access-Control-Allow-Credentials:[true] Access-Control-Allow-Origin:[http://....] Access-Control-Expose-Headers:[Content-Type, Set-Cookie] Vary:[Origin]]

Maybe someone with larger knowledge about negroni and libraries knows a better fix for this issue (order of middlewares etc.).

@CLAassistant
Copy link

CLAassistant commented Dec 6, 2022

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Dec 6, 2022

Codecov Report

Merging #2934 (096d826) into master (fc7aa86) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2934   +/-   ##
=======================================
  Coverage   76.19%   76.19%           
=======================================
  Files         309      309           
  Lines       19031    19032    +1     
=======================================
+ Hits        14501    14502    +1     
  Misses       3405     3405           
  Partials     1125     1125           
Impacted Files Coverage Δ
cmd/daemon/serve.go 86.63% <100.00%> (+0.07%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@alnr
Copy link
Contributor

alnr commented Dec 7, 2022

Thanks!

@alnr alnr merged commit 1ed6839 into ory:master Dec 7, 2022
@vinckr
Copy link
Member

vinckr commented Dec 7, 2022

Hello @kstasik
Congrats on merging your first PR in Ory 🎉 !
Your contribution will soon be helping secure millions of identities around the globe 🌏.
As a small token of appreciation we send all our first time contributors a gift package to welcome them to the community.
Please drop me an email and I will forward you the form to claim your Ory swag!

@vladyslav2
Copy link

@alnr @vinckr can you guys update your docker images on dockerhub https://hub.docker.com/r/oryd/kratos/tags?
seems like latest still haven't updated yet

thank you!

@vinckr
Copy link
Member

vinckr commented Dec 20, 2022

@alnr
Copy link
Contributor

alnr commented Dec 20, 2022

A release for including the CORS fix has not yet been tagged. If you require this fix right now, consider building kratos from source. Should be straightforward 👍

@vladyslav2
Copy link

@alnr already did.
But it would be great to have it in the docker hub at some point

@alnr
Copy link
Contributor

alnr commented Jan 16, 2023

Kratos v0.11.1 is out now 👍

@vladyslav2
Copy link

thanks

peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this pull request Jun 30, 2023
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.

None yet

5 participants