[SDK] Feature: Adds location to platform headers #7462
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant fetch.ts (getPlatformHeaders)
alt Browser environment
fetch.ts -> window: Access location.href
fetch.ts -> Caller: Return headers + x-sdk-location
else Non-browser environment
fetch.ts -> Caller: Return headers (unchanged)
end
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/thirdweb/src/utils/fetch.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.@(ts|tsx)`: Accept a typed 'props' object and export a named function (e.g...
**/*.@(ts|tsx): Accept a typed 'props' object and export a named function (e.g., export function MyComponent()).
Combine class names via 'cn', expose 'className' prop if useful.
Reuse core UI primitives; avoid re-implementing buttons, cards, modals.
Local state or effects live inside; data fetching happens in hooks.
Merge class names with 'cn' from '@/lib/utils' to keep conditional logic readable.
Stick to design-tokens: background ('bg-card'), borders ('border-border'), muted text ('text-muted-foreground') etc.
Use the 'container' class with a 'max-w-7xl' cap for page width consistency.
Spacing utilities ('px-', 'py-', 'gap-*') are preferred over custom margins.
Responsive helpers follow mobile-first ('max-sm', 'md', 'lg', 'xl').
Never hard-code colors – always go through Tailwind variables.
Tailwind CSS is the styling system – avoid inline styles or CSS modules.
Prefix files with 'import "server-only";' so they never end up in the client bundle (for server-only code).
📄 Source: CodeRabbit Inference Engine (.cursor/rules/dashboard.mdc)
List of files the instruction was applied to:
packages/thirdweb/src/utils/fetch.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Unit Tests
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: Lint Packages
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript)
| if (typeof window !== "undefined") { | ||
| previousPlatform.push(["x-sdk-location", window.location.href]); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
previousPlatform is cached once – subsequent navigations send a stale location.
Because getPlatformHeaders memoises previousPlatform, the location is captured only on the first call. In SPA contexts (Next.js, React Router, etc.) the user can navigate without a full reload, resulting in headers that no longer reflect the current page.
If accurate location is required per request, recalculate (or update) on every call instead of caching.
🤖 Prompt for AI Agents
In packages/thirdweb/src/utils/fetch.ts around lines 231 to 233, the
previousPlatform array is memoized and captures window.location.href only once,
causing stale location headers on SPA navigations. To fix this, remove the
caching of previousPlatform so that the location header is recalculated or
updated on every call to getPlatformHeaders, ensuring the current page URL is
always sent in the headers.
Potential PII/Security leakage by transmitting full URL (window.location.href).
href includes the full path, query string, and fragment. URLs often embed auth tokens, e-mails, wallet addresses, or other sensitive identifiers (e.g., ?jwt=…, #access_token=). Forwarding them verbatim in every request exposes that data to every thirdweb backend service, caching layers, logs, and potentially intermediaries.
-previousPlatform.push(["x-sdk-location", window.location.href]);
+// Safer: strip search params & hashes, or redact known sensitive keys
+const location = new URL(window.location.href);
+location.search = "";
+location.hash = "";
+previousPlatform.push(["x-sdk-location", location.toString()]);At minimum, consider:
• Stripping query / fragment components.
• Whitelisting allowed origins instead of sending the full string.
• Gating this behind an explicit opt-in flag.
🤖 Prompt for AI Agents
In packages/thirdweb/src/utils/fetch.ts around lines 231 to 233, avoid sending
the full URL from window.location.href as it may contain sensitive information
like tokens or personal data. Modify the code to strip out the query string and
fragment parts before pushing the URL, or alternatively only send the origin
(protocol + host). Consider adding an explicit opt-in flag to control whether
this location data is sent. This will prevent potential PII/security leakage in
requests.
size-limit report 📦
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7462 +/- ##
=======================================
Coverage 51.92% 51.92%
=======================================
Files 947 947
Lines 63932 63935 +3
Branches 4216 4218 +2
=======================================
+ Hits 33194 33197 +3
Misses 30632 30632
Partials 106 106
🚀 New features to boost your workflow:
|
| @@ -228,6 +228,10 @@ export function getPlatformHeaders() { | |||
| ...(bundleId ? { "x-bundle-id": bundleId } : {}), | |||
| }); | |||
|
|
|||
| if (typeof window !== "undefined") { | |||
| previousPlatform.push(["x-sdk-location", window.location.href]); | |||
There was a problem hiding this comment.
this feels a bit odd no? also make sure to handle location undefined as well for some platforms. RN actually can have a window but no location i think
There was a problem hiding this comment.
i think it fits better as a param to the tracking event rather than a header to all requests
There was a problem hiding this comment.
can we just add it to the trackPayEvent / trackConnectEvent for now?
Also see https://github.com/thirdweb-dev/client-analytics/pull/63
PR-Codex overview
This PR adds a check to ensure that the code runs only in a browser environment, allowing it to safely access
window.location.hrefand push its value to thepreviousPlatformarray.Detailed summary
windowis defined.window.location.href) is pushed to thepreviousPlatformarray with the key"x-sdk-location".Summary by CodeRabbit