Uh oh!
There was an error while loading. Please reload this page.
fix: require confirmation before campaign unsubscribe - #416
Conversation
Entire-Checkpoint: 494da77ae083
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe unsubscribe page no longer mutates contact state during GET requests. It validates signed link parameters, loads contact details, renders confirmation or resubscription states, and submits explicit unsubscribe requests through a server action. Errors are mapped to public messages and preserved through redirects. The campaign service centralizes link verification, and unit tests cover valid and malformed query parameters. UI controls now expose pending and disabled states. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Deploying usesend with |
| Latest commit: | 6ac4be1 |
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2faf64ab.usesend.pages.dev |
| Branch Preview URL: | https://codex-fix-unsubscribe-get.usesend.pages.dev |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/server/service/campaign-service.ts (1)
564-571: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse constant-time comparison for hash verification
hash !== expectedHashis vulnerable to timing attacks. While practical exploitation over HTTP is difficult, usingcrypto.timingSafeEqualis the standard practice for cryptographic hash comparison and eliminates the attack surface entirely.🔐 Proposed fix
+import { createHash, timingSafeEqual } from "crypto"; // ... inside verifyUnsubscribeLink: - if (hash !== expectedHash) {+ if (+ hash.length !== expectedHash.length ||+ !timingSafeEqual(Buffer.from(hash), Buffer.from(expectedHash))+ ) { throw new Error("Invalid unsubscribe link"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/server/service/campaign-service.ts` around lines 564 - 571, Replace the direct comparison in the hash verification logic with crypto.timingSafeEqual, comparing equal-length byte representations of hash and expectedHash and handling length mismatches safely before comparison. Preserve the existing Invalid unsubscribe link error behavior.
🧹 Nitpick comments (1)
apps/web/src/app/unsubscribe/page.unit.test.ts (1)
1-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for error and already-unsubscribed paths
The tests correctly verify the core PR objective (GET does not trigger unsubscribe mutation). Consider adding cases for:
- Error path:
getContactFromUnsubscribeLinkrejects (e.g., invalid hash or contact not found) → page should renderMessageCardwithout callingunsubscribeContactFromLink.- Already unsubscribed: mock returns
subscribed: false→ page should renderReSubscribeinstead of the confirmation form.🧪 Suggested additional tests
it("renders error card when contact lookup fails",async()=>{campaignService.getContactFromUnsubscribeLink.mockRejectedValueOnce(newError("Invalid unsubscribe link"),);constpage=awaitUnsubscribePage({searchParams: Promise.resolve({id: "contact-campaign",hash: "bad-hash"}),});expect(page).toBeTruthy();expect(campaignService.unsubscribeContactFromLink).not.toHaveBeenCalled();});it("renders ReSubscribe when contact is already unsubscribed",async()=>{campaignService.getContactFromUnsubscribeLink.mockResolvedValueOnce({id: "contact-1",email: "person@example.com",subscribed: false,});constpage=awaitUnsubscribePage({searchParams: Promise.resolve({id: "contact-campaign",hash: "hash"}),});expect(page).toBeTruthy();expect(campaignService.unsubscribeContactFromLink).not.toHaveBeenCalled();});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/app/unsubscribe/page.unit.test.ts` around lines 1 - 45, Add test coverage in the unsubscribe page test suite for both fallback branches: mock getContactFromUnsubscribeLink to reject and assert the page renders without calling unsubscribeContactFromLink, then mock it to return subscribed: false and assert the already-unsubscribed ReSubscribe view renders without triggering the mutation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/web/src/server/service/campaign-service.ts`:
- Around line 564-571: Replace the direct comparison in the hash verification
logic with crypto.timingSafeEqual, comparing equal-length byte representations
of hash and expectedHash and handling length mismatches safely before
comparison. Preserve the existing Invalid unsubscribe link error behavior.
---
Nitpick comments:
In `@apps/web/src/app/unsubscribe/page.unit.test.ts`:
- Around line 1-45: Add test coverage in the unsubscribe page test suite for
both fallback branches: mock getContactFromUnsubscribeLink to reject and assert
the page renders without calling unsubscribeContactFromLink, then mock it to
return subscribed: false and assert the already-unsubscribed ReSubscribe view
renders without triggering the mutation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 19477f38-b94f-444b-ab97-a63d050baee2
📒 Files selected for processing (5)
apps/web/src/app/unsubscribe/page.tsxapps/web/src/app/unsubscribe/page.unit.test.tsapps/web/src/app/unsubscribe/re-subscribe.tsxapps/web/src/app/unsubscribe/unsubscribe-button.tsxapps/web/src/server/service/campaign-service.ts
Uh oh!
There was an error while loading. Please reload this page.
Summary
Root cause
The in-body campaign unsubscribe page called
unsubscribeContactFromLinkwhile rendering a GET request. Email security scanners prefetch links, so they could unsubscribe contacts and increment campaign analytics without a recipient action. The signed hash could not prevent this because scanners received the complete signed URL.Impact
Opening or scanning an in-body unsubscribe link is now read-only. Contacts are unsubscribed only after explicitly pressing Confirm unsubscribe.
Verification
pnpm --filter=web exec vitest run -c vitest.unit.config.ts src/app/unsubscribe/page.unit.test.ts— 2 tests passedgit diff --checkCloses#412
Summary by cubic
Make campaign unsubscribe links safe to open via GET by adding a confirmation step and moving the unsubscribe to a POST-backed server action. Prevents email scanners from auto-unsubscribing contacts.
UnsubscribeButtonusesuseFormStatusfor pending state.getContactFromUnsubscribeLinkand shared hash validation.Written for commit 6ac4be1. Summary will update on new commits.
Summary by CodeRabbit