Skip to content

Conversation

TiARETiK
Copy link
Contributor

@TiARETiK TiARETiK commented Jan 16, 2025

There are two things to start with:

  • OPTIONS requests are not expected to have Origin header.
  • OPTIONS responses are not expected to have an Access-Control-Allow-Credentials headers.
  • No response may have both Access-Control-Allow-Credentials=true and Access-Control-Allow-Origin=*. (source)

Currently, the master branch's response to OPTIONS request contains an Access-Control-Allow-Origin= , which is interpreted incorrectly by most browsers. We can't fix it by simply checking for the presence of an Origin header in the request, due to the possibility of sending out both Access-Control-Allow-Credentials=true and Access-Control-Allow-Origin=*. The simple yet correct way to fix that behavior is to specifically check that the request being answered is not an OPTIONS one. Such a check is implemented in this pull request. There is also a test added that checks for incorrect Origins-Credentials behavior.

@TiARETiK TiARETiK marked this pull request as draft January 16, 2025 10:33
@TiARETiK TiARETiK force-pushed the origin-handling-fix branch 4 times, most recently from 7d12d5e to f333d40 Compare January 16, 2025 14:01
@TiARETiK TiARETiK force-pushed the origin-handling-fix branch from f333d40 to aa79927 Compare January 16, 2025 14:47
@TiARETiK TiARETiK marked this pull request as ready for review January 16, 2025 14:50
@TiARETiK
Copy link
Contributor Author

TiARETiK commented Feb 6, 2025

@gittiver is the current state of the pull request acceptable, or shall I do something more for it to be accepted?

@gittiver
Copy link
Member

gittiver commented Feb 7, 2025

I simply overlooked the review, sorry.

@gittiver gittiver merged commit 62d883b into CrowCpp:master Feb 7, 2025
11 checks passed
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.

2 participants