BGDIINF_SB-2420: Fixed browser origin validation and CORS - #patch #47
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
The browser doesn't add the origin header when the request is made from the same origin. However it does always send the 'Sec-Fetch-Site' header telling if the request is from the same origin, cross origin or from user. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Sec-Fetch-Site. Unfortunately this 'Sec-Fetch-Site' header is not supported by Safari ! Moreover we cannot hack the web application to always set the Origin header as most browser don't allow it. So we need to check 2 headers: Sec-Fetch-Site and Origin with a fallback to the Referer for Safari. Although this is usually not an issue for shortlink as it is always used from a different origin, the code has been corrected for completeness and consistency with other services. Also to have the correct CORS header in case of same origin we need to add the correct allowed origin in CORS header which is the same as the request. In case of cross-site then we use the Origin header as allowed origin in CORS. In case where the Origin header is not allowed, we use the request domain as Access-Control-Allow-Origin header. In case of redirect, the `Access-Control-Allow-Origin` header was not set if the origin was not from the allowed origin. Now this header in case of redirect is always set to `*` which tell the client that all origins are allowed for the redirect request.
ltshb
force-pushed
the
bug-BGDIINF_SB-2420-origin-header
branch
from
d0efe5e to
d988ba5
Compare
Yes that is related, we have some issue with headers that we try to solve on the backend side |
pakb
approved these changes
Jun 9, 2022
| {'Sec-Fetch-Site': 'none'}, | ||
| {'Sec-Fetch-Site': 'cross-site'}, | ||
| ) | ||
| def test_get_shortlink_redirect_origin_allowed(self, headers): |
There was a problem hiding this comment.
would it be useful to also add a test to cover a redirect with wrong headers?
There was a problem hiding this comment.
redirect should always be allowed and in this test I think I cover almost all possible headers including regular allowed and also non allowed header for the create.
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.
The browser doesn't add the origin header when the request is made from the
same origin. However it does always send the 'Sec-Fetch-Site' header telling
if the request is from the same origin, cross origin or from user.
See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Sec-Fetch-Site.
Unfortunately this 'Sec-Fetch-Site' header is not supported by Safari !
Moreover we cannot hack the web application to always set the Origin header as
most browser don't allow it.
So we need to check 2 headers: Sec-Fetch-Site and Origin with a fallback to the
Referer for Safari.
Although this is usually not an issue for shortlink as it is always used from a
different origin, the code has been corrected for completeness and consistency
with other services.
Also to have the correct CORS header in case of same origin we need to add the
correct allowed origin in CORS header which is the same as the request. In case
of cross-site then we use the Origin header as allowed origin in CORS. In case
where the Origin header is not allowed, we use the request domain as
Access-Control-Allow-Origin header.
In case of redirect, the
Access-Control-Allow-Originheader was not setif the origin was not from the allowed origin. Now this header in case of
redirect is always set to
*which tell the client that all origins areallowed for the redirect request.