security: fix missing Secure attribute on cookies#1218
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdded SameSite=Lax and Secure attributes to various cookies across the web app (onboarding, invitation, auth, UTM/tracking, and sidebar state). No control-flow, API, or public/exported signature changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Set Secure and SameSite=Lax on web cookies and URL-encode/decode UTM and referral values across onboarding, sidebar, and auth flowsAdd 📍Where to StartStart with cookie handling in Macroscope summarized eb27d59. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/web/app/utm.tsx">
<violation number="1" location="apps/web/app/utm.tsx:18">
P2: URL parameter values should be encoded with `encodeURIComponent()` before being set as cookie values. Without encoding, an attacker could craft a malicious URL (e.g., `?utm_source=foo; domain=.evil.com`) to inject arbitrary cookie attributes, potentially leading to cookie fixation or other issues.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
apps/web/app/utm.tsx
Outdated
|
|
||
| if (utmSource) | ||
| document.cookie = `utm_source=${utmSource}; expires=${expires}; path=/`; | ||
| document.cookie = `utm_source=${utmSource}; expires=${expires}; path=/; SameSite=Lax; Secure`; |
There was a problem hiding this comment.
P2: URL parameter values should be encoded with encodeURIComponent() before being set as cookie values. Without encoding, an attacker could craft a malicious URL (e.g., ?utm_source=foo; domain=.evil.com) to inject arbitrary cookie attributes, potentially leading to cookie fixation or other issues.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/app/utm.tsx, line 18:
<comment>URL parameter values should be encoded with `encodeURIComponent()` before being set as cookie values. Without encoding, an attacker could craft a malicious URL (e.g., `?utm_source=foo; domain=.evil.com`) to inject arbitrary cookie attributes, potentially leading to cookie fixation or other issues.</comment>
<file context>
@@ -15,17 +15,17 @@ function setUtmCookies() {
if (utmSource)
- document.cookie = `utm_source=${utmSource}; expires=${expires}; path=/`;
+ document.cookie = `utm_source=${utmSource}; expires=${expires}; path=/; SameSite=Lax; Secure`;
if (utmMedium)
- document.cookie = `utm_medium=${utmMedium}; expires=${expires}; path=/`;
</file context>
| document.cookie = `utm_source=${utmSource}; expires=${expires}; path=/; SameSite=Lax; Secure`; | |
| document.cookie = `utm_source=${encodeURIComponent(utmSource)}; expires=${expires}; path=/; SameSite=Lax; Secure`; |
✅ Addressed in 5a4be53
There was a problem hiding this comment.
Commit 5a4be53 addressed this comment. The diff shows that encodeURIComponent() has been applied to all cookie values (utm_source, utm_medium, utm_campaign, utm_term, affiliate, and referral_code), which prevents the cookie injection vulnerability described in the comment. This encoding ensures that special characters like semicolons in URL parameters cannot be used to inject arbitrary cookie attributes.
There was a problem hiding this comment.
Good catch! Added encodeURIComponent() to all UTM cookie values to prevent cookie attribute injection.
There was a problem hiding this comment.
Update: Also added decodeURIComponent() to the cookie readers in utms.tsx and auth.ts to properly decode the values.
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx (1)
23-27: Security improvements look good, but consider using a cookie utility helper.The addition of
SameSite=Lax; Secureattributes strengthens cookie security. However, directdocument.cookieassignments violate project guidelines. Consider creating a shared cookie utility function to ensure consistent security attributes and maintainability across the codebase.Based on learnings, the project standard is to avoid direct
document.cookieassignments.🔎 Example cookie utility helper
Create a helper in
utils/cookies.ts:export function setCookie( name: string, value: string, options: { path?: string; maxAge?: number; expires?: string; sameSite?: 'Lax' | 'Strict' | 'None'; secure?: boolean; } = {} ) { const { path = '/', sameSite = 'Lax', secure = true, ...rest } = options; let cookie = `${name}=${encodeURIComponent(value)}`; if (path) cookie += `; path=${path}`; if (rest.maxAge) cookie += `; max-age=${rest.maxAge}`; if (rest.expires) cookie += `; expires=${rest.expires}`; cookie += `; SameSite=${sameSite}`; if (secure) cookie += '; Secure'; document.cookie = cookie; }Then use it:
-document.cookie = `${CALENDAR_ONBOARDING_RETURN_COOKIE}=${encodeURIComponent(onboardingReturnPath)}; path=/; max-age=180; SameSite=Lax; Secure`; +setCookie(CALENDAR_ONBOARDING_RETURN_COOKIE, onboardingReturnPath, { maxAge: 180 });apps/web/app/utm.tsx (1)
5-29: Security improvements correct; refactor to eliminate repetition.The
SameSite=Lax; Secureattributes are properly applied to all UTM cookies. However, the six repetitivedocument.cookieassignments violate project guidelines and create maintenance burden.Based on learnings, avoid direct
document.cookieassignments.🔎 Refactored implementation using a helper
+import { setCookie } from '@/utils/cookies'; + function setUtmCookies() { const urlParams = new URLSearchParams(window.location.search); - const utmSource = urlParams.get("utm_source"); - const utmMedium = urlParams.get("utm_medium"); - const utmCampaign = urlParams.get("utm_campaign"); - const utmTerm = urlParams.get("utm_term"); - const affiliate = urlParams.get("aff_ref"); - const referralCode = urlParams.get("ref"); - - // expires in 30 days - const expires = new Date(Date.now() + 30 * 24 * 60 * 60 * 1000).toUTCString(); - - if (utmSource) - document.cookie = `utm_source=${utmSource}; expires=${expires}; path=/; SameSite=Lax; Secure`; - if (utmMedium) - document.cookie = `utm_medium=${utmMedium}; expires=${expires}; path=/; SameSite=Lax; Secure`; - if (utmCampaign) - document.cookie = `utm_campaign=${utmCampaign}; expires=${expires}; path=/; SameSite=Lax; Secure`; - if (utmTerm) - document.cookie = `utm_term=${utmTerm}; expires=${expires}; path=/; SameSite=Lax; Secure`; - if (affiliate) - document.cookie = `affiliate=${affiliate}; expires=${expires}; path=/; SameSite=Lax; Secure`; - if (referralCode) - document.cookie = `referral_code=${referralCode}; expires=${expires}; path=/; SameSite=Lax; Secure`; + + const cookieMap = { + utm_source: urlParams.get("utm_source"), + utm_medium: urlParams.get("utm_medium"), + utm_campaign: urlParams.get("utm_campaign"), + utm_term: urlParams.get("utm_term"), + affiliate: urlParams.get("aff_ref"), + referral_code: urlParams.get("ref"), + }; + + const expires = new Date(Date.now() + 30 * 24 * 60 * 60 * 1000).toUTCString(); + + Object.entries(cookieMap).forEach(([name, value]) => { + if (value) { + setCookie(name, value, { expires }); + } + }); }apps/web/utils/auth-cookies.ts (1)
1-14: AddclearCookiehelper to centralize cookie management.The security attributes are correctly applied. Since this is already a utility file, consider extracting a
clearCookiehelper function to ensure consistent security attributes and reduce directdocument.cookieusage throughout the codebase.Based on learnings, the project avoids direct
document.cookieassignments.🔎 Suggested refactor
+export function clearCookie(name: string, options: { path?: string } = {}) { + const { path = '/' } = options; + document.cookie = `${name}=; path=${path}; max-age=0; SameSite=Lax; Secure`; +} + export function getAndClearAuthErrorCookie(): string | undefined { const authErrorCookie = document.cookie .split("; ") .find((row) => row.startsWith("auth_error=")) ?.split("=") .slice(1) .join("="); if (authErrorCookie) { - document.cookie = "auth_error=; path=/; max-age=0; SameSite=Lax; Secure"; + clearCookie("auth_error"); } return authErrorCookie; }apps/web/utils/cookies.ts (1)
11-21: Excellent security hardening; extract base cookie helpers.The
Secureattribute additions are correct. As the central cookie utility file, this is the ideal location to provide low-levelsetCookieandclearCookiehelpers that other modules can import, ensuring consistent security attributes across the entire codebase.Based on learnings, avoid direct
document.cookieassignments in favor of utility functions.🔎 Refactor to provide reusable helpers
+export function setCookie( + name: string, + value: string, + options: { + path?: string; + maxAge?: number; + expires?: string; + sameSite?: 'Lax' | 'Strict' | 'None'; + secure?: boolean; + } = {} +) { + const { + path = '/', + sameSite = 'Lax', + secure = true, + ...rest + } = options; + + let cookie = `${name}=${encodeURIComponent(value)}`; + if (path) cookie += `; path=${path}`; + if (rest.maxAge !== undefined) cookie += `; max-age=${rest.maxAge}`; + if (rest.expires) cookie += `; expires=${rest.expires}`; + cookie += `; SameSite=${sameSite}`; + if (secure) cookie += '; Secure'; + + document.cookie = cookie; +} + +export function clearCookie(name: string, options: { path?: string } = {}) { + const { path = '/' } = options; + document.cookie = `${name}=; path=${path}; expires=Thu, 01 Jan 1970 00:00:00 GMT; SameSite=Lax; Secure`; +} + export function markOnboardingAsCompleted(cookie: string) { - document.cookie = `${cookie}=true; path=/; max-age=${Number.MAX_SAFE_INTEGER}; SameSite=Lax; Secure`; + setCookie(cookie, 'true', { maxAge: Number.MAX_SAFE_INTEGER }); } export function setInvitationCookie(invitationId: string) { - document.cookie = `${INVITATION_COOKIE}=${invitationId}; path=/; max-age=${7 * 24 * 60 * 60}; SameSite=Lax; Secure`; + setCookie(INVITATION_COOKIE, invitationId, { maxAge: 7 * 24 * 60 * 60 }); } export function clearInvitationCookie() { - document.cookie = `${INVITATION_COOKIE}=; path=/; expires=Thu, 01 Jan 1970 00:00:00 GMT; SameSite=Lax; Secure`; + clearCookie(INVITATION_COOKIE); }apps/web/components/ui/sidebar.tsx (1)
81-97: Security attributes correct; use cookie utility helper.The addition of
SameSite=Lax; Secureproperly secures sidebar state persistence. Consider importing asetCookiehelper fromutils/cookies.tsto maintain consistency with project guidelines.Based on learnings, avoid direct
document.cookieassignments.🔎 Use centralized cookie helper
+import { setCookie } from '@/utils/cookies'; + const setOpen = React.useCallback( (value: string[] | ((value: string[]) => string[])) => { const openState = typeof value === "function" ? value(open) : value; if (setOpenProp) { setOpenProp(openState); } else { _setOpen(openState); } // 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}; SameSite=Lax; Secure`; + setCookie( + `${sidebarName}:state`, + String(openState.includes(sidebarName)), + { maxAge: SIDEBAR_COOKIE_MAX_AGE } + ); }); }, [setOpenProp, open, sidebarNames], );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsxapps/web/app/utm.tsxapps/web/components/ui/sidebar.tsxapps/web/utils/auth-cookies.tsapps/web/utils/cookies.ts
🧰 Additional context used
📓 Path-based instructions (21)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/data-fetching.mdc)
**/*.{ts,tsx}: For API GET requests to server, use theswrpackage
Useresult?.serverErrorwithtoastErrorfrom@/components/Toastfor error handling in async operations
**/*.{ts,tsx}: Use wrapper functions for Gmail message operations (get, list, batch, etc.) from @/utils/gmail/message.ts instead of direct API calls
Use wrapper functions for Gmail thread operations from @/utils/gmail/thread.ts instead of direct API calls
Use wrapper functions for Gmail label operations from @/utils/gmail/label.ts instead of direct API calls
**/*.{ts,tsx}: For early access feature flags, create hooks using the naming conventionuse[FeatureName]Enabledthat return a boolean fromuseFeatureFlagEnabled("flag-key")
For A/B test variant flags, create hooks using the naming conventionuse[FeatureName]Variantthat define variant types, useuseFeatureFlagVariantKey()with type casting, and provide a default "control" fallback
Use kebab-case for PostHog feature flag keys (e.g.,inbox-cleaner,pricing-options-2)
Always define types for A/B test variant flags (e.g.,type PricingVariant = "control" | "variant-a" | "variant-b") and provide type safety through type casting
**/*.{ts,tsx}: Don't use primitive type aliases or misleading types
Don't use empty type parameters in type aliases and interfaces
Don't use this and super in static contexts
Don't use any or unknown as type constraints
Don't use the TypeScript directive @ts-ignore
Don't use TypeScript enums
Don't export imported variables
Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions
Don't use TypeScript namespaces
Don't use non-null assertions with the!postfix operator
Don't use parameter properties in class constructors
Don't use user-defined types
Useas constinstead of literal types and type annotations
Use eitherT[]orArray<T>consistently
Initialize each enum member value explicitly
Useexport typefor types
Use `impo...
Files:
apps/web/utils/auth-cookies.tsapps/web/utils/cookies.tsapps/web/components/ui/sidebar.tsxapps/web/app/utm.tsxapps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/prisma-enum-imports.mdc)
Always import Prisma enums from
@/generated/prisma/enumsinstead of@/generated/prisma/clientto avoid Next.js bundling errors in client componentsImport Prisma using the project's centralized utility:
import prisma from '@/utils/prisma'
Files:
apps/web/utils/auth-cookies.tsapps/web/utils/cookies.tsapps/web/components/ui/sidebar.tsxapps/web/app/utm.tsxapps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Import specific lodash functions rather than entire lodash library to minimize bundle size (e.g.,
import groupBy from 'lodash/groupBy')
apps/web/**/*.{ts,tsx}: Use TypeScript with strict null checks
Do not export types/interfaces that are only used within the same file. Export later if needed
Files:
apps/web/utils/auth-cookies.tsapps/web/utils/cookies.tsapps/web/components/ui/sidebar.tsxapps/web/app/utm.tsxapps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/security.mdc)
**/*.ts: ALL database queries MUST be scoped to the authenticated user/account by including user/account filtering in WHERE clauses to prevent unauthorized data access
Always validate that resources belong to the authenticated user before performing operations, using ownership checks in WHERE clauses or relationships
Always validate all input parameters for type, format, and length before using them in database queries
Use SafeError for error responses to prevent information disclosure. Generic error messages should not reveal internal IDs, logic, or resource ownership details
Only return necessary fields in API responses using Prisma'sselectoption. Never expose sensitive data such as password hashes, private keys, or system flags
Prevent Insecure Direct Object References (IDOR) by validating resource ownership before operations. AllfindUnique/findFirstcalls MUST include ownership filters
Prevent mass assignment vulnerabilities by explicitly whitelisting allowed fields in update operations instead of accepting all user-provided data
Prevent privilege escalation by never allowing users to modify system fields, ownership fields, or admin-only attributes through user input
AllfindManyqueries MUST be scoped to the user's data by including appropriate WHERE filters to prevent returning data from other users
Use Prisma relationships for access control by leveraging nested where clauses (e.g.,emailAccount: { id: emailAccountId }) to validate ownership
Files:
apps/web/utils/auth-cookies.tsapps/web/utils/cookies.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/ui-components.mdc)
**/*.{tsx,ts}: Use Shadcn UI and Tailwind for components and styling
Usenext/imagepackage for images
For API GET requests to server, use theswrpackage with hooks likeuseSWRto fetch data
For text inputs, use theInputcomponent withregisterPropsfor form integration and error handling
Files:
apps/web/utils/auth-cookies.tsapps/web/utils/cookies.tsapps/web/components/ui/sidebar.tsxapps/web/app/utm.tsxapps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx
**/*.{tsx,ts,css}
📄 CodeRabbit inference engine (.cursor/rules/ui-components.mdc)
Implement responsive design with Tailwind CSS using a mobile-first approach
Files:
apps/web/utils/auth-cookies.tsapps/web/utils/cookies.tsapps/web/components/ui/sidebar.tsxapps/web/app/utm.tsxapps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{js,jsx,ts,tsx}: Don't useaccessKeyattribute on any HTML element
Don't setaria-hidden="true"on focusable elements
Don't add ARIA roles, states, and properties to elements that don't support them
Don't use distracting elements like<marquee>or<blink>
Only use thescopeprop on<th>elements
Don't assign non-interactive ARIA roles to interactive HTML elements
Make sure label elements have text content and are associated with an input
Don't assign interactive ARIA roles to non-interactive HTML elements
Don't assigntabIndexto non-interactive HTML elements
Don't use positive integers fortabIndexproperty
Don't include "image", "picture", or "photo" in img alt prop
Don't use explicit role property that's the same as the implicit/default role
Make static elements with click handlers use a valid role attribute
Always include atitleelement for SVG elements
Give all elements requiring alt text meaningful information for screen readers
Make sure anchors have content that's accessible to screen readers
AssigntabIndexto non-interactive HTML elements witharia-activedescendant
Include all required ARIA attributes for elements with ARIA roles
Make sure ARIA properties are valid for the element's supported roles
Always include atypeattribute for button elements
Make elements with interactive roles and handlers focusable
Give heading elements content that's accessible to screen readers (not hidden witharia-hidden)
Always include alangattribute on the html element
Always include atitleattribute for iframe elements
AccompanyonClickwith at least one of:onKeyUp,onKeyDown, oronKeyPress
AccompanyonMouseOver/onMouseOutwithonFocus/onBlur
Include caption tracks for audio and video elements
Use semantic elements instead of role attributes in JSX
Make sure all anchors are valid and navigable
Ensure all ARIA properties (aria-*) are valid
Use valid, non-abstract ARIA roles for elements with ARIA roles
Use valid AR...
Files:
apps/web/utils/auth-cookies.tsapps/web/utils/cookies.tsapps/web/components/ui/sidebar.tsxapps/web/app/utm.tsxapps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx
!(pages/_document).{jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
Don't use the next/head module in pages/_document.js on Next.js projects
Files:
apps/web/utils/auth-cookies.tsapps/web/utils/cookies.tsapps/web/components/ui/sidebar.tsxapps/web/app/utm.tsxapps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/utilities.mdc)
**/*.{js,ts,jsx,tsx}: Use lodash utilities for common operations (arrays, objects, strings)
Import specific lodash functions to minimize bundle size (e.g.,import groupBy from 'lodash/groupBy')
Files:
apps/web/utils/auth-cookies.tsapps/web/utils/cookies.tsapps/web/components/ui/sidebar.tsxapps/web/app/utm.tsxapps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx
**/{utils,helpers,lib}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/logging.mdc)
Logger should be passed as a parameter to helper functions instead of creating their own logger instances
Files:
apps/web/utils/auth-cookies.tsapps/web/utils/cookies.ts
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/web/CLAUDE.md)
apps/web/**/*.{ts,tsx,js,jsx}: Use@/path aliases for imports from project root
Prefer self-documenting code over comments; use descriptive variable and function names instead of explaining intent with comments
Add helper functions to the bottom of files, not the top
All imports go at the top of files, no mid-file dynamic imports
Files:
apps/web/utils/auth-cookies.tsapps/web/utils/cookies.tsapps/web/components/ui/sidebar.tsxapps/web/app/utm.tsxapps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx
apps/web/**/*.{ts,tsx,js,jsx,json,css}
📄 CodeRabbit inference engine (apps/web/CLAUDE.md)
Format code with Prettier
Files:
apps/web/utils/auth-cookies.tsapps/web/utils/cookies.tsapps/web/components/ui/sidebar.tsxapps/web/app/utm.tsxapps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx
apps/web/**/*.{example,ts,json}
📄 CodeRabbit inference engine (apps/web/CLAUDE.md)
Add environment variables to
.env.example,env.ts, andturbo.json
Files:
apps/web/utils/auth-cookies.tsapps/web/utils/cookies.ts
apps/web/components/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/fullstack-workflow.mdc)
Use
LoadingContentcomponent to consistently handle loading and error states, passingloading,error, andchildrenpropsUse PascalCase for component file names (e.g.,
components/Button.tsx)
Files:
apps/web/components/ui/sidebar.tsx
**/{pages,routes,components}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/gmail-api.mdc)
Never call Gmail API directly from routes or components - always use wrapper functions from the utils folder
Files:
apps/web/components/ui/sidebar.tsx
apps/web/components/ui/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Shadcn UI components are located in
components/uidirectory
Files:
apps/web/components/ui/sidebar.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/ui-components.mdc)
**/*.tsx: Use theLoadingContentcomponent to handle loading states instead of manual loading state management
For text areas, use theInputcomponent withtype='text',autosizeTextareaprop set to true, andregisterPropsfor form integration
Files:
apps/web/components/ui/sidebar.tsxapps/web/app/utm.tsxapps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx
**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{jsx,tsx}: Don't use unnecessary fragments
Don't pass children as props
Don't use the return value of React.render
Make sure all dependencies are correctly specified in React hooks
Make sure all React hooks are called from the top level of component functions
Don't forget key props in iterators and collection literals
Don't define React components inside other components
Don't use event handlers on non-interactive elements
Don't assign to React component props
Don't use bothchildrenanddangerouslySetInnerHTMLprops on the same element
Don't use dangerous JSX props
Don't use Array index in keys
Don't insert comments as text nodes
Don't assign JSX properties multiple times
Don't add extra closing tags for components without children
Use<>...</>instead of<Fragment>...</Fragment>
Watch out for possible "wrong" semicolons inside JSX elements
Make sure void (self-closing) elements don't have children
Don't usetarget="_blank"withoutrel="noopener"
Don't use<img>elements in Next.js projects
Don't use<head>elements in Next.js projects
Files:
apps/web/components/ui/sidebar.tsxapps/web/app/utm.tsxapps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx
apps/web/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (apps/web/CLAUDE.md)
apps/web/**/*.{tsx,jsx}: Follow tailwindcss patterns with prettier-plugin-tailwindcss for class sorting
Prefer functional components with hooks in React
Use shadcn/ui components when available
Ensure responsive design with mobile-first approach in components
Follow consistent naming conventions using PascalCase for components
Use LoadingContent component for async data with loading and error states
Use React Hook Form with Zod validation for form handling
Useresult?.serverErrorwithtoastErrorandtoastSuccessfor error handling in forms
Files:
apps/web/components/ui/sidebar.tsxapps/web/app/utm.tsxapps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx
apps/web/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/web/CLAUDE.md)
Follow NextJS app router structure with (app) directory
Files:
apps/web/app/utm.tsxapps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx
apps/web/app/(app)/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/page-structure.mdc)
apps/web/app/(app)/**/*.{ts,tsx}: Components for the page are either put inpage.tsx, or in theapps/web/app/(app)/PAGE_NAMEfolder
If we're in a deeply nested component we will useswrto fetch via API
If you need to useonClickin a component, that component is a client component and file must start withuse client
Files:
apps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: elie222/inbox-zero PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-11-25T14:42:08.869Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Don't assign directly to document.cookie
📚 Learning: 2025-11-25T14:42:08.869Z
Learnt from: CR
Repo: elie222/inbox-zero PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-11-25T14:42:08.869Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Don't assign directly to document.cookie
Applied to files:
apps/web/utils/auth-cookies.tsapps/web/utils/cookies.tsapps/web/components/ui/sidebar.tsxapps/web/app/utm.tsxapps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Baz Reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: test
- GitHub Check: Analyze (javascript-typescript)
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| 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`; | ||
| } |
There was a problem hiding this comment.
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.
| 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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.'}
| // 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`; | ||
| }); |
There was a problem hiding this comment.
The Secure flag breaks sidebar persistence in non-HTTPS environments (like next dev). Should we conditionally apply it based on protocol?
| // 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
There was a problem hiding this comment.
Same as above - Secure cookies work fine on localhost in modern browsers. No conditional logic needed.
There was a problem hiding this comment.
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."
| if (authErrorCookie) { | ||
| document.cookie = "auth_error=; path=/; max-age=0"; | ||
| document.cookie = "auth_error=; path=/; max-age=0; SameSite=Lax; Secure"; | ||
| } |
There was a problem hiding this comment.
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)?
| 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
There was a problem hiding this comment.
When clearing a cookie, the Secure attribute doesn't affect cookie identification - cookies are matched by name + path + domain. The clear will work correctly.
There was a problem hiding this comment.
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! 🎯️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️️
| 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) { |
There was a problem hiding this comment.
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?
| 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
There was a problem hiding this comment.
Modern browsers allow Secure cookies on localhost without HTTPS. This is a well-documented exception specifically for developer ergonomics. No conditional needed.
There was a problem hiding this comment.
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.'}
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
|
||
| if (authErrorCookie) { | ||
| document.cookie = "auth_error=; path=/; max-age=0"; | ||
| document.cookie = "auth_error=; path=/; max-age=0; SameSite=Lax; Secure"; |
There was a problem hiding this comment.
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.
| 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".
1 participant
User description
security: Add Secure attribute to all document.cookie sets
This fix ensures all cookies set via the browser have the Secure attribute, preventing them from being transmitted over unencrypted HTTP.
Generated description
Below is a concise technical summary of the changes proposed in this PR:
graph LR extractUtmValues_("extractUtmValues"):::modified decodeCookieValue_("decodeCookieValue"):::added setUtmCookies_("setUtmCookies"):::modified ENCODE_URI_COMPONENT_("ENCODE_URI_COMPONENT"):::modified DOCUMENT_COOKIE_("DOCUMENT_COOKIE"):::modified setOnboardingReturnCookie_("setOnboardingReturnCookie"):::modified setInvitationCookie_("setInvitationCookie"):::modified markOnboardingAsCompleted_("markOnboardingAsCompleted"):::modified extractUtmValues_ -- "Decodes UTMs, safely fallbacks if decoding fails." --> decodeCookieValue_ setUtmCookies_ -- "Percent-encodes UTM/affiliate/referral values before storing." --> ENCODE_URI_COMPONENT_ setUtmCookies_ -- "Adds SameSite=Lax, Secure, and explicit expiration to cookies." --> DOCUMENT_COOKIE_ setOnboardingReturnCookie_ -- "Persists encoded return path with short max-age and security." --> DOCUMENT_COOKIE_ setInvitationCookie_ -- "Writes invitation cookie with seven-day max-age and security flags." --> DOCUMENT_COOKIE_ markOnboardingAsCompleted_ -- "Stores completion cookie with very long max-age and security." --> DOCUMENT_COOKIE_ classDef added stroke:#15AA7A classDef removed stroke:#CD5270 classDef modified stroke:#EDAC4C linkStyle default stroke:#CBD5E1,font-size:13pxEnhance web application security by adding
SecureandSameSite=Laxattributes to cookies set viadocument.cookiein components likeConnectCalendar,UTMtracking,Sidebarstate, and various utility functions. Improve cookie value handling by introducing robust decoding mechanisms inutms.tsxandauth.tsto prevent issues with malformed cookie data.Latest Contributors(2)