fix referrer encoding#44
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
There was a problem hiding this comment.
Pull request overview
Fixes a referrer encoding regression in the attribution flow by switching initial-referrer storage to a “raw” URL and adding normalization for previously-encoded values across client storage and server-side signup.
Changes:
- Added a referrer normalization helper that repeatedly decodes percent-encoded values (up to a limit) before persisting/using them.
- Updated client-side attribution storage to normalize cookie/localStorage referrer values and repair already-stored encoded values.
- Updated the proxy middleware to set the initial referrer cookie without manual
encodeURIComponent.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/utils/attributionUtils.ts |
Normalizes referrer values when reading from cookies/localStorage and when persisting the initial referrer. |
src/proxy.ts |
Stops manually URL-encoding the initial referrer cookie value when setting it. |
src/app/actions.ts |
Normalizes the submitted initial_referrer value before storing it in user metadata during signup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { redirect } from "next/navigation"; | ||
| import { getTranslations } from "next-intl/server"; | ||
|
|
||
| const normaliseReferrer = (referrer: string | undefined) => { |
There was a problem hiding this comment.
Naming: the repository uses normalize* in other utils; consider renaming normaliseReferrer to normalizeReferrer here as well for consistency and discoverability.
| const normaliseReferrer = (referrer: string | undefined) => { | ||
| if (!referrer) return undefined; | ||
|
|
||
| let current = referrer; | ||
|
|
||
| for (let i = 0; i < 3; i += 1) { | ||
| try { | ||
| const decoded = decodeURIComponent(current); | ||
| if (decoded === current) break; | ||
| current = decoded; | ||
| } catch { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return current; | ||
| }; |
There was a problem hiding this comment.
This normaliseReferrer logic duplicates the helper added in src/utils/attributionUtils.ts. Consider extracting a shared helper and importing it, so the client and server normalisation rules don’t diverge over time.
| try { | ||
| return decodeURIComponent(value); | ||
| return normaliseReferrer(value); | ||
| } catch { | ||
| return value; | ||
| } |
There was a problem hiding this comment.
getCookie wraps normaliseReferrer(value) in a try/catch, but normaliseReferrer already catches decodeURIComponent errors internally and never throws. This try/catch is effectively dead code; consider removing it (or simplify normaliseReferrer to let errors bubble if you prefer handling them here).
| return normaliseReferrer(value); | ||
| } catch { | ||
| return value; | ||
| } | ||
| }; | ||
|
|
||
| const normaliseReferrer = (referrer: string): string => { |
There was a problem hiding this comment.
Naming: the codebase already uses American spelling in similar helpers (e.g. normalizeNextPath, normalizeAssetPath). Consider renaming normaliseReferrer to normalizeReferrer for consistency and easier searchability.
| return normaliseReferrer(value); | |
| } catch { | |
| return value; | |
| } | |
| }; | |
| const normaliseReferrer = (referrer: string): string => { | |
| return normalizeReferrer(value); | |
| } catch { | |
| return value; | |
| } | |
| }; | |
| const normalizeReferrer = (referrer: string): string => { |
| const normaliseReferrer = (referrer: string): string => { | ||
| let current = referrer; | ||
|
|
||
| for (let i = 0; i < 3; i += 1) { | ||
| try { | ||
| const decoded = decodeURIComponent(current); | ||
| if (decoded === current) break; | ||
| current = decoded; | ||
| } catch { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return current; | ||
| }; |
There was a problem hiding this comment.
normaliseReferrer is now implemented in both src/utils/attributionUtils.ts (client) and src/app/actions.ts (server) with the same logic. To avoid the two drifting over time, consider extracting this into a shared utility (e.g. src/utils/referrer.ts) and importing it from both places.
| response.cookies.set(INITIAL_REFERRER_COOKIE, externalReferrer, { | ||
| httpOnly: false, | ||
| maxAge: INITIAL_REFERRER_MAX_AGE, | ||
| path: "/", | ||
| sameSite: "lax", | ||
| secure: request.nextUrl.protocol === "https:", | ||
| }); |
There was a problem hiding this comment.
Cookie values can’t safely contain certain separator characters (notably ;, ,, and whitespace). Since this now stores the raw externalReferrer string, a referrer path containing one of those characters could lead to a truncated/invalid cookie in browsers. Consider ensuring the value is cookie-safe here (e.g. rely on a cookie serializer that encodes, or explicitly encode/sanitize before calling cookies.set).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function normalizeReferrer(referrer: string | undefined) { | ||
| if (!referrer) return undefined; | ||
|
|
There was a problem hiding this comment.
normalizeReferrer treats an empty string as “no referrer” (if (!referrer) return undefined), but the overload normalizeReferrer(referrer: string): string promises a string return for all strings. Either change the check to only treat undefined as empty (e.g. referrer === undefined) or update the overloads/return type so callers can’t assume a string when passing an empty string.
| for (let i = 0; i < 3; i += 1) { | ||
| try { | ||
| const decoded = decodeURIComponent(current); | ||
| if (decoded === current) break; | ||
| current = decoded; | ||
| } catch { | ||
| break; | ||
| } |
There was a problem hiding this comment.
The repeated decodeURIComponent loop can over-decode once the value is already a “raw URL”. For example, a previously double-encoded cookie can decode to a URL containing legitimate %2F/%3A sequences; the 3rd decode will turn those into //: and change the URL semantics. Consider only decoding while the string still looks like a fully-encoded URL wrapper (e.g. starts with http%3A/https%3A or contains %3A%2F%2F), and stop once you’ve reached an http(s):// form.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const initialReferrer = storedInitialReferrer | ||
| ? normaliseReferrer(storedInitialReferrer) | ||
| : cookieReferrer; | ||
|
|
||
| if (!storedInitialReferrer && initialReferrer) { | ||
| storeInitialReferrer(initialReferrer); | ||
| } else if ( | ||
| storedInitialReferrer && |
There was a problem hiding this comment.
storedInitialReferrer comes from localStorage.getItem(...) (type string | null), but the new conditional uses truthiness. This changes behavior vs the previous ?? logic: an empty string will now be treated as missing and fall back to the cookie. Use an explicit null check (e.g., storedInitialReferrer !== null) so empty-string values don’t change control flow unintentionally.
| const initialReferrer = storedInitialReferrer | |
| ? normaliseReferrer(storedInitialReferrer) | |
| : cookieReferrer; | |
| if (!storedInitialReferrer && initialReferrer) { | |
| storeInitialReferrer(initialReferrer); | |
| } else if ( | |
| storedInitialReferrer && | |
| const initialReferrer = | |
| storedInitialReferrer !== null | |
| ? normaliseReferrer(storedInitialReferrer) | |
| : cookieReferrer; | |
| if (storedInitialReferrer === null && initialReferrer) { | |
| storeInitialReferrer(initialReferrer); | |
| } else if ( | |
| storedInitialReferrer !== null && |
| const referrer = normaliseReferrer( | ||
| formData.get("initial_referrer")?.toString() | ||
| ); |
There was a problem hiding this comment.
normaliseReferrer is applied to formData.get("initial_referrer"), which is user-controllable. Because it can decode percent-encoded payloads, it can introduce control characters (e.g. %0d%0a) that then get logged and persisted to Supabase user metadata. Consider sanitizing the normalized value (strip control chars / enforce a max length) and/or validating it as an http(s) URL before storing/logging.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function normaliseReferrer(referrer: string): string; | ||
| export function normaliseReferrer(referrer: undefined): undefined; | ||
| export function normaliseReferrer( | ||
| referrer: string | undefined | ||
| ): string | undefined; | ||
| export function normaliseReferrer(referrer: string | undefined) { |
There was a problem hiding this comment.
normaliseReferrer uses British spelling, but the rest of the codebase appears to consistently use normalize* (e.g. normalizeNextPath, normalizeAssetPath). Consider renaming to normalizeReferrer (and updating imports/usages) to keep naming consistent and improve discoverability.
| } catch { | ||
| return value; | ||
| } | ||
| return normaliseReferrer(value) ?? null; |
There was a problem hiding this comment.
getCookie now returns normaliseReferrer(value) directly. Since normaliseReferrer can legally return an empty string (e.g. input contains only control characters), this function can return "" instead of null, which then blocks fallbacks that use nullish coalescing (??). Consider treating empty/whitespace-only results as null (e.g. trim + return null when empty) and drop the redundant ?? null (the overload for string returns string).
| return normaliseReferrer(value) ?? null; | |
| const normalisedValue = normaliseReferrer(value); | |
| if (!normalisedValue.trim()) return null; | |
| return normalisedValue; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (!cleanedValue.trim()) return null; | ||
|
|
||
| return cleanedValue; |
There was a problem hiding this comment.
getCookie checks cleanedValue.trim() for emptiness but then returns the untrimmed cleanedValue. This can preserve leading/trailing whitespace in initial_referrer (and in localStorage via storeInitialReferrer), which can cause unnecessary mismatches and avoidable URL parse failures elsewhere. Consider returning the trimmed value (or trimming inside normaliseReferrer) so the stored/reflected referrer is consistently normalised.
| if (!cleanedValue.trim()) return null; | |
| return cleanedValue; | |
| const trimmedValue = cleanedValue.trim(); | |
| if (!trimmedValue) return null; | |
| return trimmedValue; |
Fixes the referrer encoding regression by storing the initial referrer cookie as a raw URL and normalising encoded values before signup.
Also handles already-stored encoded referrer values from existing browser sessions.
Check: npm run check