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 does not work when used with Basic Auth #343

Merged
merged 6 commits into from
Apr 16, 2024

Conversation

ms140569
Copy link
Contributor

@ms140569 ms140569 commented Apr 14, 2024

This PR fixes #336 when CORS is used together with Basic Auth.

The Authorization header is now added to the cors-allow-headers list to work with the Basic Auth.
Also, the server will allow OPTIONS requests without an Authorization header since it is not required according to the spec.

Test cases will follow in a separate PR.

Copy link

semanticdiff-com bot commented Apr 14, 2024

Review changes with SemanticDiff.

Analyzed 3 of 4 files.

Overall, the semantic diff is 23% smaller than the GitHub diff.

Filename Status
docs/content/features/cors.md Unsupported file format
✔️ src/cors.rs 23.1% smaller
✔️ src/handler.rs 4.17% smaller
✔️ src/settings/cli.rs Analyzed

Copy link
Collaborator

@joseluisq joseluisq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, the PR has what we need to fix the issue.
In addition, I made some minor improvements to the src/cors.rs as well as added/updated a few log entries and updated the CORS page mentioning the new allowed authorization header.

However, as discussed. It would be great to add the necessary tests for the features used together. So feel free to add them here or let me know if you want to do it in a separate PR.

@joseluisq joseluisq self-assigned this Apr 14, 2024
@joseluisq joseluisq added v2 v2 release bugfix This is PR fixes a bug labels Apr 14, 2024
Copy link
Collaborator

@joseluisq joseluisq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just please update the PR description and title accordingly. Also, link the issue that you are fixing.

@joseluisq joseluisq assigned ms140569 and unassigned joseluisq Apr 16, 2024
@joseluisq joseluisq changed the title Fix 336 cors basicauth Fix CORS does not work when used with Basic Auth Apr 16, 2024
Copy link
Collaborator

@joseluisq joseluisq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@joseluisq joseluisq merged commit 1c4fad2 into static-web-server:master Apr 16, 2024
24 checks passed
@joseluisq joseluisq added this to the v2.30.0 milestone Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This is PR fixes a bug v2 v2 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CORS not working when Basic Auth enabled
2 participants