-
Notifications
You must be signed in to change notification settings - Fork 564
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
4.x Include scheme and port of origin and host in deciding whether to classify a request as CORS or not #8166
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oracle-contributor-agreement
bot
added
the
OCA Verified
All contributors have signed the Oracle Contributor Agreement.
label
Dec 19, 2023
ljnelson
reviewed
Jan 5, 2024
ljnelson
reviewed
Jan 5, 2024
ljnelson
previously approved these changes
Jan 5, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of questions but basically LGTM
…equest as CORS or not
…CORS request adapter and impls; update copyright years
tjquinno
force-pushed
the
4.x-cors-host
branch
from
January 5, 2024 06:17
843276d
to
bf72b31
Compare
ljnelson
previously approved these changes
Jan 5, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments
ljnelson
approved these changes
Jan 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Resolves #8088
Resolves #8093
Proper implementation of the CORS algorithm requires considering the scheme, node, and port of the origin (conveyed by the
Origin
header) and the host that was requested by the client (conveyed by theHost
header or theForwarded
header or theX-Forwarded-*
family of headers). A normal request in the CORS context is one that is not cross-origin--that is, for which the origin and the host share the same scheme, node, and port.The 4.x logic regressed from 3.x in not considering the scheme and port properly.
This PR does several things:
Adds an
isSecure
method to an adapter interface for requests.The adapter insulates the CORS logic from differences in and dependence on the SE and MP request implementations. The PR also includes the enhancements to the SE and MP adapters to implement this new method.
Corrects the CORS logic which checks to see if the incoming request is a normal request (that is, non-CORS).
The CORS code first checks for whether the request is normal and, if so, skips the CORS processing. This normal check failed to account properly for the scheme and port of the origin and host and therefore incorrectly identified requests that needed CORS processing as normal (non-CORS) requests.
Adds numerous tests.
The CORS code does considerable logging (when turned on) to help users understand the decisions the code makes. This PR also introduces a new private record which represents the result of the "is the request normal" check, containing not only the true/false result but also the effective origin and host locations (full scheme, node, and port) that the is-normal check computed to decide the true/false result. The logging code now uses these intermediate results rather than having to recompute them for logging.
Documentation
This is a bug fix. No doc impact.