Skip to content

security: fix missing Secure attribute on cookies #1218

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.

Sign up for GitHub

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

Merged
elie222 merged 5 commits into from
Jan 6, 2026
Merged

security: fix missing Secure attribute on cookies #1218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
491bd61
security: add Secure attribute to cookies
elie222 Jan 6, 2026
5a4be53
fix
elie222 Jan 6, 2026
c25a7b6
security: encode UTM cookie values to prevent injection
elie222 Jan 6, 2026
a9908ef
revert: remove encodeURIComponent from UTM cookies (readers don't dec…
elie222 Jan 6, 2026
eb27d59
security: encode UTM cookies on write, decode on read
elie222 Jan 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export function ConnectCalendar({

const setOnboardingReturnCookie = () => {
if (onboardingReturnPath) {
document.cookie = `${CALENDAR_ONBOARDING_RETURN_COOKIE}=${encodeURIComponent(onboardingReturnPath)}; path=/; max-age=180`;
document.cookie = `${CALENDAR_ONBOARDING_RETURN_COOKIE}=${encodeURIComponent(onboardingReturnPath)}; path=/; max-age=180; SameSite=Lax; Secure`;
}
Comment on lines 23 to 26
Copy link
Copy Markdown
Contributor

@baz-reviewer baz-reviewer bot Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Secure attribute prevents this cookie from being set over HTTP (like http://localhost:3000), breaking the onboarding return path that /calendars/page.tsx expects after OAuth.

Suggested change
const setOnboardingReturnCookie = () => {
if (onboardingReturnPath) {
document.cookie = `${CALENDAR_ONBOARDING_RETURN_COOKIE}=${encodeURIComponent(onboardingReturnPath)}; path=/; max-age=180`;
document.cookie = `${CALENDAR_ONBOARDING_RETURN_COOKIE}=${encodeURIComponent(onboardingReturnPath)}; path=/; max-age=180; SameSite=Lax; Secure`;
}
const setOnboardingReturnCookie = () => {
if (onboardingReturnPath) {
const isSecureContext = typeof window !== "undefined" && window.location.protocol === "https:";
document.cookie = `${CALENDAR_ONBOARDING_RETURN_COOKIE}=${encodeURIComponent(onboardingReturnPath)}; path=/; max-age=180; SameSite=Lax${isSecureContext ? "; Secure" : ""}`;
}
};

Finding types: AI Coding Guidelines Logical Bugs

Copy link
Copy Markdown
Owner Author

@elie222 elie222 Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modern browsers (Chrome 89+, Firefox 75+, Safari 15+) have a special exception: cookies with Secure are allowed on localhost even without HTTPS. This was specifically designed to not break local development. No change needed.

Copy link
Copy Markdown
Contributor

@baz-reviewer baz-reviewer bot Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — good point about modern browsers allowing Secure cookies on localhost for local dev; noted and I'll save this to memory after the PR is merged. If you want, I can update the review comment to reflect that exception so it doesn't prompt a change unnecessarily in dev environments.','thinking':'The feedback references a specific code snippet that sets a Secure cookie in ConnectCalendar.tsx, so this is specific code feedback rather than PR-level or general feedback. The user provides a factual correction about browser behavior for localhost Secure cookies, which is directly relevant to the previously commented code. Because this is specific code feedback, acknowledge it and indicate it will be saved to memory once the PR is merged.'}

};

Expand Down
21 changes: 15 additions & 6 deletions apps/web/app/(landing)/welcome/utms.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,24 @@ export function registerUtmTracking({
// See: https://nextjs.org/docs/app/api-reference/functions/after
export function extractUtmValues(cookies: ReadonlyRequestCookies): UtmValues {
return {
utmCampaign: cookies.get("utm_campaign")?.value,
utmMedium: cookies.get("utm_medium")?.value,
utmSource: cookies.get("utm_source")?.value,
utmTerm: cookies.get("utm_term")?.value,
affiliate: cookies.get("affiliate")?.value,
referralCode: cookies.get("referral_code")?.value,
utmCampaign: decodeCookieValue(cookies.get("utm_campaign")?.value),
utmMedium: decodeCookieValue(cookies.get("utm_medium")?.value),
utmSource: decodeCookieValue(cookies.get("utm_source")?.value),
utmTerm: decodeCookieValue(cookies.get("utm_term")?.value),
affiliate: decodeCookieValue(cookies.get("affiliate")?.value),
referralCode: decodeCookieValue(cookies.get("referral_code")?.value),
};
}

function decodeCookieValue(value: string | undefined): string | undefined {
if (!value) return undefined;
try {
return decodeURIComponent(value);
} catch {
return value;
}
}

export async function fetchUserAndStoreUtms(
userId: string,
utmValues: UtmValues,
Expand Down
12 changes: 6 additions & 6 deletions apps/web/app/utm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,17 @@ function setUtmCookies() {
const expires = new Date(Date.now() + 30 * 24 * 60 * 60 * 1000).toUTCString();

if (utmSource)
document.cookie = `utm_source=${utmSource}; expires=${expires}; path=/`;
document.cookie = `utm_source=${encodeURIComponent(utmSource)}; expires=${expires}; path=/; SameSite=Lax; Secure`;
if (utmMedium)
document.cookie = `utm_medium=${utmMedium}; expires=${expires}; path=/`;
document.cookie = `utm_medium=${encodeURIComponent(utmMedium)}; expires=${expires}; path=/; SameSite=Lax; Secure`;
if (utmCampaign)
document.cookie = `utm_campaign=${utmCampaign}; expires=${expires}; path=/`;
document.cookie = `utm_campaign=${encodeURIComponent(utmCampaign)}; expires=${expires}; path=/; SameSite=Lax; Secure`;
if (utmTerm)
document.cookie = `utm_term=${utmTerm}; expires=${expires}; path=/`;
document.cookie = `utm_term=${encodeURIComponent(utmTerm)}; expires=${expires}; path=/; SameSite=Lax; Secure`;
if (affiliate)
document.cookie = `affiliate=${affiliate}; expires=${expires}; path=/`;
document.cookie = `affiliate=${encodeURIComponent(affiliate)}; expires=${expires}; path=/; SameSite=Lax; Secure`;
if (referralCode)
document.cookie = `referral_code=${referralCode}; expires=${expires}; path=/`;
document.cookie = `referral_code=${encodeURIComponent(referralCode)}; expires=${expires}; path=/; SameSite=Lax; Secure`;
}

export function UTM() {
Expand Down
2 changes: 1 addition & 1 deletion apps/web/components/ui/sidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ const SidebarProvider = React.forwardRef<
// This sets the cookie to keep the sidebar state.
// This sets the cookie to keep the sidebar state.
sidebarNames.forEach((sidebarName) => {
document.cookie = `${sidebarName}:state=${openState.includes(sidebarName)}; path=/; max-age=${SIDEBAR_COOKIE_MAX_AGE}`;
document.cookie = `${sidebarName}:state=${openState.includes(sidebarName)}; path=/; max-age=${SIDEBAR_COOKIE_MAX_AGE}; SameSite=Lax; Secure`;
});
Comment on lines 90 to 94
Copy link
Copy Markdown
Contributor

@baz-reviewer baz-reviewer bot Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Secure flag breaks sidebar persistence in non-HTTPS environments (like next dev). Should we conditionally apply it based on protocol?

Suggested change
// This sets the cookie to keep the sidebar state.
// This sets the cookie to keep the sidebar state.
sidebarNames.forEach((sidebarName) => {
document.cookie = `${sidebarName}:state=${openState.includes(sidebarName)}; path=/; max-age=${SIDEBAR_COOKIE_MAX_AGE}`;
document.cookie = `${sidebarName}:state=${openState.includes(sidebarName)}; path=/; max-age=${SIDEBAR_COOKIE_MAX_AGE}; SameSite=Lax; Secure`;
});
sidebarNames.forEach((sidebarName) => {
const secureAttr = window.location.protocol === "https:" ? "; Secure" : "";
document.cookie = `${sidebarName}:state=${openState.includes(sidebarName)}; path=/; max-age=${SIDEBAR_COOKIE_MAX_AGE}; SameSite=Lax${secureAttr}`;
});

Finding types: Logical Bugs AI Coding Guidelines

Copy link
Copy Markdown
Owner Author

@elie222 elie222 Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above - Secure cookies work fine on localhost in modern browsers. No conditional logic needed.

Copy link
Copy Markdown
Contributor

@baz-reviewer baz-reviewer bot Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted — you're right that modern browsers accept Secure cookies on localhost, so no conditional logic is needed; I'll save this to memory after the PR is merged. Thanks for the clarification!" ,
"thinking": "The feedback references a specific code snippet in apps/web/components/ui/sidebar.tsx about setting the Secure attribute on cookies for localhost. The presence of commented code and a finding type indicates this is specific code feedback rather than PR-level or general feedback. The user explicitly disputes prior suggestion to add conditional logic, which is a concrete code-level correction and therefore appropriate to acknowledge and save to memory once the PR is merged."

},
[setOpenProp, open, sidebarNames],
Expand Down
2 changes: 1 addition & 1 deletion apps/web/utils/auth-cookies.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ export function getAndClearAuthErrorCookie(): string | undefined {
.join("=");

if (authErrorCookie) {
document.cookie = "auth_error=; path=/; max-age=0";
document.cookie = "auth_error=; path=/; max-age=0; SameSite=Lax; Secure";
Copy link
Copy Markdown
Contributor

@macroscopeapp macroscopeapp bot Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Secure here over HTTP can block deletion, leaving auth_error stale. Consider omitting Secure (or add it only on HTTPS) so the cookie clears in dev.

Suggested change
document.cookie = "auth_error=; path=/; max-age=0; SameSite=Lax; Secure";
document.cookie = "auth_error=; path=/; max-age=0; SameSite=Lax";

🚀 Want me to fix this? Reply ex: "fix it for me".

}
Comment on lines 9 to 11
Copy link
Copy Markdown
Contributor

@baz-reviewer baz-reviewer bot Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding Secure to the delete call won't clear the original non-secure auth_error cookie, leaving stale errors. Also breaks HTTP contexts (local dev)?

Suggested change
if (authErrorCookie) {
document.cookie = "auth_error=; path=/; max-age=0";
document.cookie = "auth_error=; path=/; max-age=0; SameSite=Lax; Secure";
}
if (authErrorCookie) {
document.cookie = "auth_error=; path=/; max-age=0";
document.cookie = "auth_error=; path=/; max-age=0; SameSite=Lax; Secure";
}

Finding types: Type Inconsistency Logical Bugs

Copy link
Copy Markdown
Owner Author

@elie222 elie222 Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When clearing a cookie, the Secure attribute doesn't affect cookie identification - cookies are matched by name + path + domain. The clear will work correctly.

Copy link
Copy Markdown
Contributor

@baz-reviewer baz-reviewer bot Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted — the point about the Secure attribute not affecting cookie matching is correct; I’ll save this to memory after the PR is merged. Thanks for the clarification! 🎯️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️


return authErrorCookie;
Expand Down
7 changes: 6 additions & 1 deletion apps/web/utils/auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,12 @@ export async function handleReferralOnSignUp({
return;
}

const referralCode = referralCookie.value;
let referralCode = referralCookie.value;
try {
referralCode = decodeURIComponent(referralCode);
} catch {
// Use original value if decoding fails
}
logger.info("Processing referral for new user", {
email,
referralCode,
Expand Down
6 changes: 3 additions & 3 deletions apps/web/utils/cookies.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@ export type LastEmailAccountCookieValue = {
};

export function markOnboardingAsCompleted(cookie: string) {
document.cookie = `${cookie}=true; path=/; max-age=${Number.MAX_SAFE_INTEGER}; SameSite=Lax`;
document.cookie = `${cookie}=true; path=/; max-age=${Number.MAX_SAFE_INTEGER}; SameSite=Lax; Secure`;
}

export function setInvitationCookie(invitationId: string) {
Comment on lines 11 to 15
Copy link
Copy Markdown
Contributor

@baz-reviewer baz-reviewer bot Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding Secure here (and in the matching blocks for setInvitationCookie/clearInvitationCookie and the auth-error cookie removal in apps/web/utils/auth-cookies.ts lines 9‑11) means these cookies cannot be set or cleared when the app runs over plain HTTP (e.g., next dev or client-side tests). Browsers simply ignore document.cookie writes in that scenario, so the onboarding and invitation cookies never reach the server and gating redirects always treat the user as un-onboarded; the invitation flow also cannot persist the ID over login locally. This breaks the predictable dev/test behavior referred to by apps/web/CLAUDE.md (Web App Development Review Guidelines). Can we set Secure only on HTTPS or drop it locally so the cookies exist in non-HTTPS environments?

Suggested change
export function markOnboardingAsCompleted(cookie: string) {
document.cookie = `${cookie}=true; path=/; max-age=${Number.MAX_SAFE_INTEGER}; SameSite=Lax`;
document.cookie = `${cookie}=true; path=/; max-age=${Number.MAX_SAFE_INTEGER}; SameSite=Lax; Secure`;
}
export function setInvitationCookie(invitationId: string) {
export function markOnboardingAsCompleted(cookie: string) {
const secureAttribute = window.location.protocol === "https:" ? "; Secure" : "";
document.cookie = `${cookie}=true; path=/; max-age=${Number.MAX_SAFE_INTEGER}; SameSite=Lax${secureAttribute}`;
}
export function setInvitationCookie(invitationId: string) {

Finding type: AI Coding Guidelines

Copy link
Copy Markdown
Owner Author

@elie222 elie222 Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modern browsers allow Secure cookies on localhost without HTTPS. This is a well-documented exception specifically for developer ergonomics. No conditional needed.

Copy link
Copy Markdown
Contributor

@baz-reviewer baz-reviewer bot Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted — this is specific code feedback about using Secure cookies on localhost; got it and I’ll save this to memory after the PR is merged. Thank you!','thinking':'The user referenced a specific code snippet (apps/web/utils/cookies.ts) and provided targeted feedback about the Secure cookie behavior on localhost, so this is feedback tied to a specific code finding. Therefore I should acknowledge it and state that I will save it to memory once the PR is merged.'}

document.cookie = `${INVITATION_COOKIE}=${invitationId}; path=/; max-age=${7 * 24 * 60 * 60}; SameSite=Lax`;
document.cookie = `${INVITATION_COOKIE}=${invitationId}; path=/; max-age=${7 * 24 * 60 * 60}; SameSite=Lax; Secure`;
}

export function clearInvitationCookie() {
document.cookie = `${INVITATION_COOKIE}=; path=/; expires=Thu, 01 Jan 1970 00:00:00 GMT; SameSite=Lax`;
document.cookie = `${INVITATION_COOKIE}=; path=/; expires=Thu, 01 Jan 1970 00:00:00 GMT; SameSite=Lax; Secure`;
}
Loading