security: fix missing Secure attribute on cookies#1217
security: fix missing Secure attribute on cookies#1217
Conversation
- Remove unused `categories` prop from BulkArchiveTab - Add guards for empty thread.messages arrays - Add error handling with try/catch in archiveSelected - Remove dead code for 'more emails' section - Fix createMany return value to use result.count - Use Next.js Link instead of anchor tag in creator page - Type error boundary props properly with reset function 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Memoize headings in TableOfContents to prevent IntersectionObserver recreation - Remove nested Button in TryInboxZero, style Link directly - Update deprecated Image props (layout/objectFit) to Next.js 13+ syntax - Add rel="noopener" to external link in CaseStudyContent 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The marketing directory was accidentally converted from a submodule to a regular directory during a merge, causing 100+ spurious file changes in the PR. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Move category icon mapping logic to bulk-archive/categoryIcons.ts to reduce BulkArchiveCards.tsx file size. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughA bulk archiving feature is introduced with new pages, components, and API routes to manage email sender categorization and archiving by confidence level. Email provider abstraction replaces direct Gmail API usage. Cookie security is enhanced across the application with SameSite and Secure attributes. Changes
Sequence DiagramsequenceDiagram
participant User
participant BulkArchivePage as Server:<br/>BulkArchivePage
participant DB as Database
participant Client as Client:<br/>BulkArchiveContent
participant SWR as SWR Hook
participant API as API:<br/>/categorize/senders
participant Queue as Archive<br/>Queue
User->>BulkArchivePage: Visit bulk-archive page
BulkArchivePage->>BulkArchivePage: Verify account ownership
BulkArchivePage->>DB: Fetch senders with categories
BulkArchivePage->>DB: Fetch user categories with rules
DB-->>BulkArchivePage: Initial data
BulkArchivePage-->>Client: Render with initial data
User->>Client: Enable auto-categorization
Client->>Client: Set isEnabling = true
Client->>API: bulkCategorizeSendersAction(emailAccountId)
API->>API: Initialize missing default categories
API->>API: Queue categorization batch job
API-->>Client: Success
Client->>Client: Show toast: "Processing..."
par Polling for Progress
Client->>SWR: Subscribe to progress with 2s interval
loop While categorizing
SWR->>API: GET /progress
API-->>SWR: {totalItems, completedItems}
SWR-->>Client: Update progress
end
end
par User Actions
User->>Client: Select senders to archive
User->>Client: Click "Archive Selected"
Client->>Queue: addToArchiveSenderQueue(senders)
Queue->>Queue: Process archive jobs
end
API->>API: Categorization complete
SWR->>API: GET /progress (100% complete)
API-->>SWR: completedItems == totalItems
SWR-->>Client: Completion detected
Client->>Client: After 3s: disable categorizing
Client->>Client: Invoke onComplete callback
Client->>SWR: Mutate to refresh data
SWR->>API: GET /categorized
API->>DB: Fetch updated senders
DB-->>API: Updated data
API-->>SWR: New data
SWR-->>Client: Update UI
Client-->>User: Show updated sender list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
Add Secure and SameSite=Lax to app cookies and introduce email-account–scoped Bulk Archive and Quick Bulk Archive pages with polling and archiving actionsSet cookie attributes to 📍Where to StartStart with the categorize batch handler refactor in handle-batch.ts, then review the new polling endpoint in route.ts and the Bulk Archive page composition in page.tsx. 📊 Macroscope summarized 2db6dfb. 22 files reviewed, 23 issues evaluated, 16 issues filtered, 2 comments posted. View details |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| {data.threads.slice(0, 5).map((thread) => { | ||
| const firstMessage = thread.messages[0]; | ||
| if (!firstMessage) return null; | ||
| const subject = firstMessage.subject; |
There was a problem hiding this comment.
subject.length can throw if firstMessage.subject is null/undefined. Consider defaulting subject to an empty string (or guarding) before checking .length.
| const subject = firstMessage.subject; | |
| const subject = firstMessage.subject ?? ""; |
🚀 Want me to fix this? Reply ex: "fix it for me".
| <span className="shrink-0 text-xs text-muted-foreground"> | ||
| {formatShortDate(new Date(date))} | ||
| </span> |
There was a problem hiding this comment.
Consider guarding firstMessage.date before new Date(...)/formatShortDate. If missing/invalid, skip the timestamp or show a placeholder to avoid Invalid Date/NaN.
- <span className="shrink-0 text-xs text-muted-foreground">
- {formatShortDate(new Date(date))}
- </span>
+ {(() => {
+ if (!date) return null;
+ const d = new Date(date);
+ return isNaN(d.getTime()) ? null : (
+ <span className="shrink-0 text-xs text-muted-foreground">
+ {formatShortDate(d)}
+ </span>
+ );
+ })()}🚀 Want me to fix this? Reply ex: "fix it for me".
![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 24 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">
P1: Cookie injection vulnerability: URL parameter values are interpolated directly into cookie strings without encoding. An attacker could craft a malicious URL (e.g., `?utm_source=x; Domain=.evil.com`) to inject arbitrary cookie attributes. Use `encodeURIComponent()` to safely encode the values.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| 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.
P1: Cookie injection vulnerability: URL parameter values are interpolated directly into cookie strings without encoding. An attacker could craft a malicious URL (e.g., ?utm_source=x; Domain=.evil.com) to inject arbitrary cookie attributes. Use encodeURIComponent() to safely encode the values.
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>Cookie injection vulnerability: URL parameter values are interpolated directly into cookie strings without encoding. An attacker could craft a malicious URL (e.g., `?utm_source=x; Domain=.evil.com`) to inject arbitrary cookie attributes. Use `encodeURIComponent()` to safely encode the values.</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`; |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/app/api/user/categorize/senders/batch/handle-batch.ts (1)
44-53: Remove redundant database query and clarify security assumptions for this cron endpoint.This is a Upstash QStash cron endpoint (protected by
verifySignatureAppRoutersignature verification), not a user-facing API route, so it doesn't requirewithEmailAccountorwithAuthmiddleware. However, there are two issues:
Redundant query: Lines 44-53 fetch
emailAccount.accountwhich is already available fromvalidateUserAndAiAccessat line 38. Remove this duplicate query and reuse the data fromuserResult.validateUserAndAiAccess design weakness: This function queries by
emailAccountIdonly without user scoping. While safe here (cron endpoint from trusted Upstash), the function lacks built-in ownership validation and could pose IDOR risks if reused in other contexts (e.g., user-facing API routes). Consider updatingvalidateUserAndAiAccessto require and validateuserIdownership.
🤖 Fix all issues with AI Agents
In
@apps/web/app/(app)/[emailAccountId]/bulk-archive/AutoCategorizationSetup.tsx:
- Around line 27-51: enableFeature sets setIsBulkCategorizing(true) but never
clears it when there are zero uncategorized senders; update enableFeature (the
async callback) to call setIsBulkCategorizing(false) when
result?.data?.totalUncategorizedSenders is 0 or falsy (before or immediately
after showing the "No uncategorized senders found." toast), keeping the existing
error and finally handling intact so the UI doesn't remain stuck in the
categorizing state.
In @apps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx:
- Around line 23-27: Replace the direct document.cookie assignment in
setOnboardingReturnCookie with the centralized cookie utility used elsewhere
(the auth cookie helper), i.e., call the utility to set
CALENDAR_ONBOARDING_RETURN_COOKIE to encodeURIComponent(onboardingReturnPath)
and pass options for path:'/', maxAge:180, sameSite:'Lax', secure:true; update
references in ConnectCalendar.tsx to import and use that helper instead of
manipulating document.cookie directly.
In @apps/web/app/(app)/[emailAccountId]/quick-bulk-archive/BulkArchiveTab.tsx:
- Around line 583-586: The Link element rendering the email URL (href from
getEmailUrl(thread.id, userEmail, provider)) uses target="_blank" but is missing
rel="noopener noreferrer"; update the Link in BulkArchiveTab.tsx to include
rel="noopener noreferrer" alongside target="_blank" (matching the implementation
in BulkArchiveCards.tsx) to prevent the opened page from accessing
window.opener.
In @apps/web/app/utm.tsx:
- Around line 17-28: Replace the six direct document.cookie assignments in
utm.tsx with calls to the centralized cookie utility (e.g., setCookie) from the
auth-cookies module: for each key (utm_source, utm_medium, utm_campaign,
utm_term, affiliate, referral_code) call setCookie(name, value, { maxAge:
<seconds>, path: '/', sameSite: 'Lax', secure: true }) instead of using expires;
remove the expires usage and convert the expiration to max-age seconds to match
the rest of the PR; ensure you import the utility (setCookie) and only set
cookies when the corresponding variable (utmSource, utmMedium, utmCampaign,
utmTerm, affiliate, referralCode) is present.
In @apps/web/components/ui/sidebar.tsx:
- Around line 90-91: In the Sidebar component (sidebar.tsx) there are two
identical comments "// This sets the cookie to keep the sidebar state."
repeated; remove the duplicated line so only one instance of that comment
remains (preserve surrounding whitespace/indentation and any nearby comments or
code in the render/return block where the cookie-setting logic exists).
- Around line 92-94: The code is directly setting cookies via document.cookie in
the loop over sidebarNames (using sidebarNames, openState and
SIDEBAR_COOKIE_MAX_AGE); replace that direct manipulation with the centralized
cookie utility from auth-cookies.ts — call the utility’s setter (e.g., setCookie
or equivalent) for each sidebarName and pass the same cookie name/value and
attributes (path=/, max-age=SIDEBAR_COOKIE_MAX_AGE, SameSite=Lax, Secure) so the
persistence logic is centralized and document.cookie is removed from the
component.
In @apps/web/utils/auth-cookies.ts:
- Line 10: Replace the direct document.cookie assignment in auth-cookies.ts with
a call to a centralized cookie utility: create apps/web/utils/cookie-manager.ts
exposing setCookie(name,value,options), getCookie(name) and
deleteCookie(name,path) that encapsulate SameSite, Secure, path and max-age
logic, then import and call deleteCookie('auth_error') (or
setCookie('auth_error','',{ maxAge:0 })) from the existing function instead of
assigning document.cookie directly.
In @apps/web/utils/cookies.ts:
- Around line 11-13: The markOnboardingAsCompleted function currently sets
document.cookie max-age to Number.MAX_SAFE_INTEGER which is excessively large
and may cause browser/PDPR issues; change it to a reasonable long duration
(e.g., ONE_YEAR_SECONDS = 365 * 24 * 60 * 60 or TWO_YEARS_SECONDS = 2 * 365 * 24
* 60 * 60) and use that constant in the template for max-age instead of
Number.MAX_SAFE_INTEGER, keeping the cookie name parameter cookie and preserving
path/SameSite/Secure attributes.
- Around line 11-21: Replace the direct document.cookie assignments in
markOnboardingAsCompleted, setInvitationCookie, and clearInvitationCookie with
calls to the centralized cookie utility exported from auth-cookies.ts: import
the cookie helper and use its set and delete functions (or equivalent names
provided by auth-cookies.ts) to set the cookie name/value and options (path,
max-age for Number.MAX_SAFE_INTEGER and 7*24*60*60, SameSite=Lax, Secure) and to
clear the cookie using an expiration option; keep using the INVITATION_COOKIE
symbol and preserve the exact semantics for persistence and deletion.
🧹 Nitpick comments (9)
apps/web/app/(app)/early-access/page.tsx (1)
24-68: Consider removing commented-out code or using feature flags.The fragment correctly wraps multiple elements, but two Card components are commented out (lines 40-53 and 54-67). Commented code should typically be removed since version control maintains history. If these features are planned for gradual rollout, consider using feature flags with conditional rendering instead.
Alternative approach with feature flags
If these are experimental features for gradual rollout:
{isGoogleProvider(provider) && ( <> <Card> <CardHeader> <CardTitle>Sender Categories</CardTitle> <CardDescription> Sender Categories is a feature that allows you to categorize emails by sender, and take bulk actions or apply rules to them. </CardDescription> </CardHeader> <CardContent> <Button asChild> <Link href="/smart-categories">Sender Categories</Link> </Button> </CardContent> </Card> {useBulkArchiveEnabled() && ( <Card> <CardHeader> <CardTitle>Bulk Archive</CardTitle> <CardDescription> Archive emails from multiple senders at once, organized by category. </CardDescription> </CardHeader> <CardContent> <Button asChild> <Link href="/bulk-archive">Bulk Archive</Link> </Button> </CardContent> </Card> )} {useQuickBulkArchiveEnabled() && ( <Card> <CardHeader> <CardTitle>Quick Bulk Archive</CardTitle> <CardDescription> Quickly archive emails from multiple senders at once, grouped by AI confidence level. </CardDescription> </CardHeader> <CardContent> <Button asChild> <Link href="/quick-bulk-archive">Quick Bulk Archive</Link> </Button> </CardContent> </Card> )} </> )}Based on learnings, these features would automatically appear on the Early Access page through the
EarlyAccessFeaturescomponent when enabled.apps/web/utils/cookies.ts (1)
11-21: Inconsistent cookie expiration approaches.The file uses two different methods for setting cookie expiration:
markOnboardingAsCompletedandsetInvitationCookieusemax-age(lines 12, 16)clearInvitationCookieusesexpireswith epoch date (line 20)While both approaches work, using
max-age=0is the more modern and consistent approach for deleting cookies. Consider standardizing onmax-agethroughout.🔎 Standardize on max-age
export function clearInvitationCookie() { - document.cookie = `${INVITATION_COOKIE}=; path=/; expires=Thu, 01 Jan 1970 00:00:00 GMT; SameSite=Lax; Secure`; + document.cookie = `${INVITATION_COOKIE}=; path=/; max-age=0; SameSite=Lax; Secure`; }apps/web/components/BulkArchiveCards.tsx (2)
148-169: Consider parallelizing archive operations for better performance.The sequential
awaitinside the loop processes senders one at a time. If the archive queue can handle concurrent additions, this could be significantly faster withPromise.allor batched processing.🔎 Optional parallel approach
const archiveCategory = async (categoryName: string, e: React.MouseEvent) => { e.stopPropagation(); const senders = groupedEmails[categoryName] || []; const selectedToArchive = senders.filter( (s) => selectedSenders[s.address] !== false, ); try { - for (const sender of selectedToArchive) { - await addToArchiveSenderQueue({ - sender: sender.address, - emailAccountId, - }); - } + await Promise.all( + selectedToArchive.map((sender) => + addToArchiveSenderQueue({ + sender: sender.address, + emailAccountId, + }), + ), + ); setArchivedCategories((prev) => ({ ...prev, [categoryName]: true })); } catch (_error) { toastError({ description: "Failed to archive some senders. Please try again.", }); } };
528-531: Redundant slice operation.The
useThreadshook already limits results to 5 vialimit: 5. The.slice(0, 5)on line 531 is unnecessary.🔎 Proposed fix
- {data.threads.slice(0, 5).map((thread) => { + {data.threads.map((thread) => {apps/web/app/(app)/[emailAccountId]/quick-bulk-archive/BulkArchiveTab.tsx (2)
78-103: Double computation of archive candidates.
getArchiveCandidates(emailGroups)is called in the state initializer (line 83) and again inuseMemo(line 101). Consider storing the initial candidates or restructuring to avoid redundant computation.🔎 Proposed optimization
+const initialCandidates = useMemo( + () => getArchiveCandidates(emailGroups), + [emailGroups], +); const [selectedSenders, setSelectedSenders] = useState< Record<string, boolean> >(() => { // Pre-select high and medium confidence senders const initial: Record<string, boolean> = {}; - const candidates = getArchiveCandidates(emailGroups); - for (const candidate of candidates) { + for (const candidate of initialCandidates) { initial[candidate.address] = candidate.confidence === "high" || candidate.confidence === "medium"; } return initial; }); // ... -const candidates = useMemo( - () => getArchiveCandidates(emailGroups), - [emailGroups], -); +const candidates = initialCandidates;Note: This requires careful handling since
useMemoruns after state initialization. The current approach may be intentional to ensure state initializer has access to candidates. If so, this is acceptable as-is.
430-497: Consider extracting shared subcomponents to reduce duplication.
SenderRow,ArchiveStatus, andExpandedEmailsare nearly identical between this file andBulkArchiveCards.tsx. Consider extracting them into shared components under@/components/bulk-archive/to improve maintainability.Also applies to: 499-526, 528-619
apps/web/utils/bulk-archive/get-archive-candidates.ts (1)
36-40: Unnecessary type assertions.The
as ConfidenceLevelcasts are redundant since the string literals"high","medium", and"low"already satisfy theConfidenceLeveltype. TypeScript can infer these correctly.🔎 Proposed simplification
return { ...group, - confidence: "high" as ConfidenceLevel, + confidence: "high", reason: "Marketing / Promotional", }; } // ... similar for medium and lowAlso applies to: 50-54, 58-62
apps/web/app/(app)/[emailAccountId]/quick-bulk-archive/page.tsx (1)
49-61: Consider extracting data transformation logic.The
emailGroupstransformation (sort + map with category lookup) is duplicated between this file andBulkArchiveContent.tsx. Also, performing this computation inline within JSX reduces readability.🔎 Proposed extraction
+// Consider extracting to a shared utility, e.g., @/utils/bulk-archive/build-email-groups.ts +const emailGroups = sortBy(senders, (sender) => sender.category?.name).map( + (sender) => ({ + address: sender.email, + category: + categories.find((category) => category.id === sender.category?.id) || + null, + }), +); <ClientOnly> <BulkArchiveTab - emailGroups={sortBy(senders, (sender) => sender.category?.name).map( - (sender) => ({ - address: sender.email, - category: - categories.find( - (category) => category.id === sender.category?.id, - ) || null, - }), - )} + emailGroups={emailGroups} /> </ClientOnly>apps/web/app/api/user/categorize/senders/batch/handle-batch.ts (1)
95-131: Consider removing the commented-out code.The large commented block (lines 95-131) should be removed to improve code clarity. If this logic is needed in the future, it's preserved in version control history.
| const enableFeature = useCallback(async () => { | ||
| setIsEnabling(true); | ||
| setIsBulkCategorizing(true); | ||
|
|
||
| try { | ||
| const result = await bulkCategorizeSendersAction(emailAccountId); | ||
|
|
||
| if (result?.serverError) { | ||
| throw new Error(result.serverError); | ||
| } | ||
|
|
||
| toast.success( | ||
| result?.data?.totalUncategorizedSenders | ||
| ? `Categorizing ${result.data.totalUncategorizedSenders} senders... This may take a few minutes.` | ||
| : "No uncategorized senders found.", | ||
| ); | ||
| } catch (error) { | ||
| toast.error( | ||
| `Failed to enable feature: ${error instanceof Error ? error.message : "Unknown error"}`, | ||
| ); | ||
| setIsBulkCategorizing(false); | ||
| } finally { | ||
| setIsEnabling(false); | ||
| } | ||
| }, [emailAccountId, setIsBulkCategorizing]); |
There was a problem hiding this comment.
Clear the categorizing flag when no senders are found.
When totalUncategorizedSenders is 0, the success toast displays "No uncategorized senders found", but setIsBulkCategorizing(true) from Line 29 is never cleared. This leaves the UI in a perpetual "categorizing" state even though no work is being performed.
🔎 Proposed fix
try {
const result = await bulkCategorizeSendersAction(emailAccountId);
if (result?.serverError) {
throw new Error(result.serverError);
}
- toast.success(
- result?.data?.totalUncategorizedSenders
- ? `Categorizing ${result.data.totalUncategorizedSenders} senders... This may take a few minutes.`
- : "No uncategorized senders found.",
- );
+ if (result?.data?.totalUncategorizedSenders) {
+ toast.success(
+ `Categorizing ${result.data.totalUncategorizedSenders} senders... This may take a few minutes.`
+ );
+ } else {
+ toast.success("No uncategorized senders found.");
+ setIsBulkCategorizing(false);
+ }
} catch (error) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const enableFeature = useCallback(async () => { | |
| setIsEnabling(true); | |
| setIsBulkCategorizing(true); | |
| try { | |
| const result = await bulkCategorizeSendersAction(emailAccountId); | |
| if (result?.serverError) { | |
| throw new Error(result.serverError); | |
| } | |
| toast.success( | |
| result?.data?.totalUncategorizedSenders | |
| ? `Categorizing ${result.data.totalUncategorizedSenders} senders... This may take a few minutes.` | |
| : "No uncategorized senders found.", | |
| ); | |
| } catch (error) { | |
| toast.error( | |
| `Failed to enable feature: ${error instanceof Error ? error.message : "Unknown error"}`, | |
| ); | |
| setIsBulkCategorizing(false); | |
| } finally { | |
| setIsEnabling(false); | |
| } | |
| }, [emailAccountId, setIsBulkCategorizing]); | |
| const enableFeature = useCallback(async () => { | |
| setIsEnabling(true); | |
| setIsBulkCategorizing(true); | |
| try { | |
| const result = await bulkCategorizeSendersAction(emailAccountId); | |
| if (result?.serverError) { | |
| throw new Error(result.serverError); | |
| } | |
| if (result?.data?.totalUncategorizedSenders) { | |
| toast.success( | |
| `Categorizing ${result.data.totalUncategorizedSenders} senders... This may take a few minutes.` | |
| ); | |
| } else { | |
| toast.success("No uncategorized senders found."); | |
| setIsBulkCategorizing(false); | |
| } | |
| } catch (error) { | |
| toast.error( | |
| `Failed to enable feature: ${error instanceof Error ? error.message : "Unknown error"}`, | |
| ); | |
| setIsBulkCategorizing(false); | |
| } finally { | |
| setIsEnabling(false); | |
| } | |
| }, [emailAccountId, setIsBulkCategorizing]); |
🤖 Prompt for AI Agents
In @apps/web/app/(app)/[emailAccountId]/bulk-archive/AutoCategorizationSetup.tsx
around lines 27 - 51, enableFeature sets setIsBulkCategorizing(true) but never
clears it when there are zero uncategorized senders; update enableFeature (the
async callback) to call setIsBulkCategorizing(false) when
result?.data?.totalUncategorizedSenders is 0 or falsy (before or immediately
after showing the "No uncategorized senders found." toast), keeping the existing
error and finally handling intact so the UI doesn't remain stuck in the
categorizing state.
| 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.
🛠️ Refactor suggestion | 🟠 Major
Apply centralized cookie utility here as well.
The security attributes addition is good, but this is another instance of direct document.cookie assignment that should use the centralized cookie utility suggested in the review of apps/web/utils/auth-cookies.ts.
🔎 Refactor with cookie utility
+import { setCookie } from '@/utils/cookie-manager';
import { CALENDAR_ONBOARDING_RETURN_COOKIE } from "@/utils/calendar/constants";
const setOnboardingReturnCookie = () => {
if (onboardingReturnPath) {
- document.cookie = `${CALENDAR_ONBOARDING_RETURN_COOKIE}=${encodeURIComponent(onboardingReturnPath)}; path=/; max-age=180; SameSite=Lax; Secure`;
+ setCookie(CALENDAR_ONBOARDING_RETURN_COOKIE, onboardingReturnPath, {
+ maxAge: 180,
+ });
}
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx around
lines 23 - 27, Replace the direct document.cookie assignment in
setOnboardingReturnCookie with the centralized cookie utility used elsewhere
(the auth cookie helper), i.e., call the utility to set
CALENDAR_ONBOARDING_RETURN_COOKIE to encodeURIComponent(onboardingReturnPath)
and pass options for path:'/', maxAge:180, sameSite:'Lax', secure:true; update
references in ConnectCalendar.tsx to import and use that helper instead of
manipulating document.cookie directly.
| <Link | ||
| href={getEmailUrl(thread.id, userEmail, provider)} | ||
| target="_blank" | ||
| className="mr-2 flex flex-1 items-center gap-3 rounded-md px-2 py-2 transition-colors hover:bg-muted/50" |
There was a problem hiding this comment.
Add rel="noopener noreferrer" to external links.
Links with target="_blank" should include rel="noopener noreferrer" to prevent the opened page from accessing the opener via window.opener. This is present in BulkArchiveCards.tsx (line 547) but missing here.
🔎 Proposed fix
<Link
href={getEmailUrl(thread.id, userEmail, provider)}
target="_blank"
+ rel="noopener noreferrer"
className="mr-2 flex flex-1 items-center gap-3 rounded-md px-2 py-2 transition-colors hover:bg-muted/50"
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Link | |
| href={getEmailUrl(thread.id, userEmail, provider)} | |
| target="_blank" | |
| className="mr-2 flex flex-1 items-center gap-3 rounded-md px-2 py-2 transition-colors hover:bg-muted/50" | |
| <Link | |
| href={getEmailUrl(thread.id, userEmail, provider)} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| className="mr-2 flex flex-1 items-center gap-3 rounded-md px-2 py-2 transition-colors hover:bg-muted/50" | |
| > |
🤖 Prompt for AI Agents
In @apps/web/app/(app)/[emailAccountId]/quick-bulk-archive/BulkArchiveTab.tsx
around lines 583 - 586, The Link element rendering the email URL (href from
getEmailUrl(thread.id, userEmail, provider)) uses target="_blank" but is missing
rel="noopener noreferrer"; update the Link in BulkArchiveTab.tsx to include
rel="noopener noreferrer" alongside target="_blank" (matching the implementation
in BulkArchiveCards.tsx) to prevent the opened page from accessing
window.opener.
| 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=/`; | ||
| document.cookie = `utm_medium=${utmMedium}; expires=${expires}; path=/; SameSite=Lax; Secure`; | ||
| if (utmCampaign) | ||
| document.cookie = `utm_campaign=${utmCampaign}; expires=${expires}; path=/`; | ||
| document.cookie = `utm_campaign=${utmCampaign}; expires=${expires}; path=/; SameSite=Lax; Secure`; | ||
| if (utmTerm) | ||
| document.cookie = `utm_term=${utmTerm}; expires=${expires}; path=/`; | ||
| document.cookie = `utm_term=${utmTerm}; expires=${expires}; path=/; SameSite=Lax; Secure`; | ||
| if (affiliate) | ||
| document.cookie = `affiliate=${affiliate}; expires=${expires}; path=/`; | ||
| document.cookie = `affiliate=${affiliate}; expires=${expires}; path=/; SameSite=Lax; Secure`; | ||
| if (referralCode) | ||
| document.cookie = `referral_code=${referralCode}; expires=${expires}; path=/`; | ||
| document.cookie = `referral_code=${referralCode}; expires=${expires}; path=/; SameSite=Lax; Secure`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Multiple document.cookie violations - consolidate with cookie utility.
Six instances of direct document.cookie assignment violate the coding guideline. Additionally, this file uses expires for cookie expiration while other files in this PR use max-age, creating inconsistency.
Refactor to use the centralized cookie utility suggested in apps/web/utils/auth-cookies.ts, which will also standardize the expiration approach.
🔎 Refactor with cookie utility
+import { setCookie } from '@/utils/cookie-manager';
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();
+ const maxAge = 30 * 24 * 60 * 60; // 30 days in seconds
- 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`;
+ if (utmSource) setCookie('utm_source', utmSource, { maxAge });
+ if (utmMedium) setCookie('utm_medium', utmMedium, { maxAge });
+ if (utmCampaign) setCookie('utm_campaign', utmCampaign, { maxAge });
+ if (utmTerm) setCookie('utm_term', utmTerm, { maxAge });
+ if (affiliate) setCookie('affiliate', affiliate, { maxAge });
+ if (referralCode) setCookie('referral_code', referralCode, { maxAge });
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/web/app/utm.tsx around lines 17 - 28, Replace the six direct
document.cookie assignments in utm.tsx with calls to the centralized cookie
utility (e.g., setCookie) from the auth-cookies module: for each key
(utm_source, utm_medium, utm_campaign, utm_term, affiliate, referral_code) call
setCookie(name, value, { maxAge: <seconds>, path: '/', sameSite: 'Lax', secure:
true }) instead of using expires; remove the expires usage and convert the
expiration to max-age seconds to match the rest of the PR; ensure you import the
utility (setCookie) and only set cookies when the corresponding variable
(utmSource, utmMedium, utmCampaign, utmTerm, affiliate, referralCode) is
present.
| // This sets the cookie to keep the sidebar state. | ||
| // This sets the cookie to keep the sidebar state. |
There was a problem hiding this comment.
Remove duplicate comment.
Lines 90-91 contain an identical comment that appears twice. Remove one instance.
🔎 Proposed fix
}
// This sets the cookie to keep the sidebar state.
- // This sets the cookie to keep the sidebar state.
sidebarNames.forEach((sidebarName) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // This sets the cookie to keep the sidebar state. | |
| // This sets the cookie to keep the sidebar state. | |
| } | |
| // This sets the cookie to keep the sidebar state. | |
| sidebarNames.forEach((sidebarName) => { |
🤖 Prompt for AI Agents
In @apps/web/components/ui/sidebar.tsx around lines 90 - 91, In the Sidebar
component (sidebar.tsx) there are two identical comments "// This sets the
cookie to keep the sidebar state." repeated; remove the duplicated line so only
one instance of that comment remains (preserve surrounding
whitespace/indentation and any nearby comments or code in the render/return
block where the cookie-setting logic exists).
| 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.
🛠️ Refactor suggestion | 🟠 Major
Apply centralized cookie utility to sidebar state persistence.
This UI component directly manipulates cookies for state persistence, which violates the coding guideline. Refactor to use the centralized cookie utility suggested in apps/web/utils/auth-cookies.ts.
🔎 Refactor with cookie utility
+import { setCookie } from '@/utils/cookie-manager';
// 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,
+ });
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/web/components/ui/sidebar.tsx around lines 92 - 94, The code is
directly setting cookies via document.cookie in the loop over sidebarNames
(using sidebarNames, openState and SIDEBAR_COOKIE_MAX_AGE); replace that direct
manipulation with the centralized cookie utility from auth-cookies.ts — call the
utility’s setter (e.g., setCookie or equivalent) for each sidebarName and pass
the same cookie name/value and attributes (path=/,
max-age=SIDEBAR_COOKIE_MAX_AGE, SameSite=Lax, Secure) so the persistence logic
is centralized and document.cookie is removed from the component.
|
|
||
| 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.
🛠️ Refactor suggestion | 🟠 Major
Security improvement approved, but consider refactoring to a cookie utility.
The addition of SameSite=Lax; Secure attributes improves security by preventing cookie transmission over unencrypted HTTP and providing CSRF protection.
However, this violates the coding guideline: "Don't assign directly to document.cookie." Based on learnings, direct cookie manipulation should be avoided across the codebase. Consider creating a centralized cookie utility module (e.g., utils/cookie-manager.ts) with functions like setCookie(), getCookie(), and deleteCookie() that encapsulate the security attributes and provide a consistent API.
🔎 Suggested cookie utility pattern
Create a new file apps/web/utils/cookie-manager.ts:
export function setCookie(
name: string,
value: string,
options: {
maxAge?: number;
expires?: Date;
path?: string;
secure?: boolean;
sameSite?: 'Strict' | 'Lax' | 'None';
} = {}
) {
const {
maxAge,
expires,
path = '/',
secure = true,
sameSite = 'Lax',
} = options;
let cookie = `${name}=${encodeURIComponent(value)}`;
if (maxAge !== undefined) cookie += `; max-age=${maxAge}`;
if (expires) cookie += `; expires=${expires.toUTCString()}`;
cookie += `; path=${path}`;
cookie += `; SameSite=${sameSite}`;
if (secure) cookie += '; Secure';
document.cookie = cookie;
}
export function deleteCookie(name: string, path = '/') {
setCookie(name, '', { maxAge: 0, path });
}
export function getCookie(name: string): string | undefined {
return document.cookie
.split('; ')
.find(row => row.startsWith(`${name}=`))
?.split('=')
.slice(1)
.join('=');
}Then refactor this function:
+import { getCookie, deleteCookie } from '@/utils/cookie-manager';
+
export function getAndClearAuthErrorCookie(): string | undefined {
- const authErrorCookie = document.cookie
- .split("; ")
- .find((row) => row.startsWith("auth_error="))
- ?.split("=")
- .slice(1)
- .join("=");
+ const authErrorCookie = getCookie("auth_error");
if (authErrorCookie) {
- document.cookie = "auth_error=; path=/; max-age=0; SameSite=Lax; Secure";
+ deleteCookie("auth_error");
}
return authErrorCookie;
}Based on learnings, this pattern should be applied across all cookie operations in the codebase.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/web/utils/auth-cookies.ts at line 10, Replace the direct
document.cookie assignment in auth-cookies.ts with a call to a centralized
cookie utility: create apps/web/utils/cookie-manager.ts exposing
setCookie(name,value,options), getCookie(name) and deleteCookie(name,path) that
encapsulate SameSite, Secure, path and max-age logic, then import and call
deleteCookie('auth_error') (or setCookie('auth_error','',{ maxAge:0 })) from the
existing function instead of assigning document.cookie directly.
| 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`; | ||
| } |
There was a problem hiding this comment.
Question the use of Number.MAX_SAFE_INTEGER for cookie expiration.
Using Number.MAX_SAFE_INTEGER as the max-age creates an extremely long-lived cookie (approximately 285 million years). While the intent seems to be "permanent" storage, this approach has potential issues:
- Some browsers may handle such large values inconsistently
- Privacy regulations (GDPR, CCPA) may require reasonable cookie lifetimes
- Standard practice is to use a more reasonable long duration (e.g., 1-2 years)
Consider using a more conventional long duration like 365 * 24 * 60 * 60 (1 year) or 2 * 365 * 24 * 60 * 60 (2 years).
🔎 Suggested fix
export function markOnboardingAsCompleted(cookie: string) {
- document.cookie = `${cookie}=true; path=/; max-age=${Number.MAX_SAFE_INTEGER}; SameSite=Lax; Secure`;
+ document.cookie = `${cookie}=true; path=/; max-age=${365 * 24 * 60 * 60}; SameSite=Lax; Secure`;
}🤖 Prompt for AI Agents
In @apps/web/utils/cookies.ts around lines 11 - 13, The
markOnboardingAsCompleted function currently sets document.cookie max-age to
Number.MAX_SAFE_INTEGER which is excessively large and may cause browser/PDPR
issues; change it to a reasonable long duration (e.g., ONE_YEAR_SECONDS = 365 *
24 * 60 * 60 or TWO_YEARS_SECONDS = 2 * 365 * 24 * 60 * 60) and use that
constant in the template for max-age instead of Number.MAX_SAFE_INTEGER, keeping
the cookie name parameter cookie and preserving path/SameSite/Secure attributes.
| 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) { | ||
| 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`; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Apply centralized cookie utility to this file.
This file already serves as a cookie utilities module but still uses direct document.cookie assignments. These functions should be refactored to use (or become part of) the centralized cookie management utility suggested in apps/web/utils/auth-cookies.ts.
🤖 Prompt for AI Agents
In @apps/web/utils/cookies.ts around lines 11 - 21, Replace the direct
document.cookie assignments in markOnboardingAsCompleted, setInvitationCookie,
and clearInvitationCookie with calls to the centralized cookie utility exported
from auth-cookies.ts: import the cookie helper and use its set and delete
functions (or equivalent names provided by auth-cookies.ts) to set the cookie
name/value and options (path, max-age for Number.MAX_SAFE_INTEGER and
7*24*60*60, SameSite=Lax, Secure) and to clear the cookie using an expiration
option; keep using the INVITATION_COOKIE symbol and preserve the exact semantics
for persistence and deletion.
2 participants
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.
Summary by CodeRabbit
Release Notes
New Features
Security
✏️ Tip: You can customize this high-level summary in your review settings.
Generated description
Below is a concise technical summary of the changes proposed in this PR:
graph LR BulkArchivePage_("BulkArchivePage"):::added BulkArchiveContent_("BulkArchiveContent"):::added PRISMA_DB_("PRISMA/DB"):::modified BulkArchiveProgress_("BulkArchiveProgress"):::added AutoCategorizationSetup_("AutoCategorizationSetup"):::added BulkArchiveCards_("BulkArchiveCards"):::added GET_("GET"):::added getCategorizedSenders_("getCategorizedSenders"):::added BulkArchivePage_ -- "Loads senders/categories then renders BulkArchiveContent for client UI" --> BulkArchiveContent_ BulkArchivePage_ -- "Reads newsletter rows and category metadata via Prisma" --> PRISMA_DB_ BulkArchiveContent_ -- "Renders progress panel and refreshes when categorization completes" --> BulkArchiveProgress_ BulkArchiveContent_ -- "Includes setup to trigger AI categorization when none exist" --> AutoCategorizationSetup_ BulkArchiveContent_ -- "Passes grouped senders and categories to UI card component" --> BulkArchiveCards_ BulkArchiveContent_ -- "Polls categorized senders API to refresh senders and categories" --> GET_ GET_ -- "Concurrent fetch: newsletters and user categories before responding" --> getCategorizedSenders_ getCategorizedSenders_ -- "Queries newsletter table for senders with categoryId not null" --> PRISMA_DB_ classDef added stroke:#15AA7A classDef removed stroke:#CD5270 classDef modified stroke:#EDAC4C linkStyle default stroke:#CBD5E1,font-size:13pxEnhances application security by adding
SecureandSameSite=Laxattributes to all client-side cookies, and introduces new bulk email archiving features including AI-powered sender categorization and a user interface for managing archived senders.document.cookieoperations include theSecureandSameSite=Laxattributes, preventing transmission over unencrypted HTTP and mitigating CSRF risks.Modified files (5)
Latest Contributors(2)
Modified files (19)
Latest Contributors(0)