Add referrerPolicy fetch.ts #4944
Conversation
Set referrerPolicy to 'same-origin' rather default `strict-origin-when-cross-origin` The strict referrer break fiftyone app when behind reverse proxy
WalkthroughThe changes made in the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/packages/utilities/src/fetch.ts (2)
275-275: LGTM: Consistent referrer policy for event source.The addition of
referrerPolicy: 'same-origin'to the fetchEventSource call is consistent with the earlier change and ensures that all types of requests follow the same referrer policy. This change further contributes to addressing the issue mentioned in #4539.Consider extracting the 'same-origin' value to a constant at the top of the file for better maintainability:
+const REFERRER_POLICY = 'same-origin'; // Then use it in both places: -referrerPolicy: 'same-origin', +referrerPolicy: REFERRER_POLICY,This would make it easier to update the policy in the future if needed.
Line range hint
1-437: Summary: Referrer policy changes effectively address the PR objectives.The changes made to this file are focused and consistent, addressing the issue of missing Referer headers in POST requests (issue #4539). By setting
referrerPolicy: 'same-origin'for both regular fetch calls and event source requests, the implementation ensures a uniform approach to handling the referrer policy.These modifications should resolve the problem of requests being blocked by Azure App Service when the FiftyOne app is hosted behind a reverse proxy. However, it's important to thoroughly test the application in various hosting scenarios to ensure no unintended side effects arise from this more restrictive referrer policy.
Consider adding a configuration option to allow users to customize the referrer policy if needed for different hosting environments. This would provide flexibility while maintaining the desired default behavior.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/packages/utilities/src/fetch.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/utilities/src/fetch.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
| @@ -129,6 +129,7 @@ export const setFetchFunction = ( | |||
| mode: "cors", | |||
| body: body ? JSON.stringify(body) : null, | |||
| signal: controller.signal, | |||
| referrerPolicy: 'same-origin', | |||
There was a problem hiding this comment.
💡 Codebase verification
Duplicate referrerPolicy Found in fetch.ts.
The shell script output indicates that referrerPolicy: 'same-origin' is present twice in app/packages/utilities/src/fetch.ts. Please review to ensure that this duplication is intentional and remove any redundant instances to maintain code clarity and prevent potential conflicts.
🔗 Analysis chain
LGTM: Referrer policy change addresses the issue.
The addition of referrerPolicy: 'same-origin' aligns with the PR objectives and addresses the issue mentioned in #4539 where POST requests were missing the Referer header. This change should help when the app is hosted behind a generic reverse proxy.
To ensure this change doesn't introduce any unintended side effects, please verify that:
- The app functions correctly when hosted behind different types of reverse proxies.
- Cross-origin requests, if any, are not negatively impacted.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any other occurrences of referrerPolicy in the codebase
rg "referrerPolicy" --type ts
Length of output: 175
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4944 +/- ##
=========================================
Coverage 99.17% 99.18%
=========================================
Files 49 49
Lines 17785 17991 +206
=========================================
+ Hits 17639 17845 +206
Misses 146 146
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Set referrerPolicy to 'same-origin' rather default
strict-origin-when-cross-originWhat changes are proposed in this pull request?
Allow Fiftyone app running behind generic reverse proxy
How is this patch tested? If it is not, please explain why.
Tested on our Fiftyone instance running behind Azure reverse proxy
Release Notes
Fix #4539
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
referrerPolicyoption for improved privacy control.Bug Fixes