Skip to content

feat(hutch): auto-save the first article for new signups#943

Open
FagnerMartinsBrack wants to merge 1 commit into
mainfrom
feat/first-article-autosave
Open

feat(hutch): auto-save the first article for new signups#943
FagnerMartinsBrack wants to merge 1 commit into
mainfrom
feat/first-article-autosave

Conversation

@FagnerMartinsBrack

Copy link
Copy Markdown
Member

First-article auto-save for new signups

Hypothesis. A new user whose queue already contains the article that brought them to Readplace is far likelier to activate (read something) than one who lands on an empty queue. If we quietly save that article at signup, the queue is non-empty on first sight and the "there's nothing here" cold-start is gone.

Evidence (production). Only 4 of 35 new users ever read anything. 23 of 34 signups arrive without a pending save — they clicked a nav/homepage "Sign up" CTA, not the reader's "Save to My Queue" button — so the existing pending-save round-trip (which already carries a held save through signup) never fires for them. Those 23 are exactly the cohort this change targets: they were reading an article on /view immediately before signing up, but nothing captured it.

What this changes.

  • On an anonymous, non-bot /view open, set a short-lived httpOnly hutch_lastview cookie (2h, SameSite=Lax, Secure per origin) holding the validated article URL. It sits behind the same isbot gate that emits the existing view_opened event, so crawlers and prefetches never set it, and authenticated viewers don't either.
  • At signup success (all three paths: email free+trial, Google, Apple), if the post-auth redirect would be the default /queue (no explicit ?return=) and a valid hutch_lastview cookie is present, redirect to /queue?url=<article>&utm_source=signup-autosave instead. The existing /queue?url=… auto-submit save form then saves the article into the brand-new queue — reusing the stub-save + async-enrich pipeline, so the card appears immediately and fills in.
  • A single pure resolvePostSignupRedirect({ returnUrl, lastViewUrl }) is consulted at all three sites so they can't drift. It never auto-saves when an explicit ?return= is present (the /save round-trip already saves in that case — no double-save) or when the cookie is absent/tampered (URL re-validated with the same validateSaveableUrl used everywhere else).
  • The cookie is cleared on consumption and on POST /logout. For Apple's cross-site form_post callback (which drops first-party Lax cookies) the URL is tunneled inside the existing HMAC-signed state, exactly like pendingSaveId already is.

Estimated result. The 23/34 no-pending-save signups start with a 1-article queue instead of an empty one. If even a fraction of that cold-start friction is what suppresses the current 4/35 (~11%) read rate, moving first-article-in-queue to ~100% of eligible signups should lift first-week activation meaningfully. We'll read the actual lift from the funnel below rather than guess a number.

How to measure (CloudWatch Logs Insights, hutch log group /aws/lambda/hutch-handler). No new analytics event is added — attribution rides the existing pageview stream via the utm_source=signup-autosave marker on the post-signup /queue load (the save form's action is a fixed internal-tracking link, so the marker lands on the pageview, not on view_save_intent; each such pageview is 1:1 with an autosave trigger and joins to the resulting save by visitor_id).

fields @timestamp, visitor_id, path
| filter stream = "analytics" and event = "pageview"
| filter utm_source = "signup-autosave" and is_authenticated = 1
| stats count(*) as autosaves_triggered by bin(1d)

To confirm each autosave actually persisted, join the marker pageview to the follow-on save on the same visitor:

fields @timestamp, visitor_id, event, utm_source, surface, outcome
| filter stream = "analytics"
    and (
      (event = "pageview" and utm_source = "signup-autosave")
      or (event = "view_save_intent" and surface = "queue_save_bar" and outcome = "saved")
    )
| sort visitor_id, @timestamp
| stats
    count(*) as autosave_pageviews,
    sum(outcome = "saved") as saves_confirmed
  by visitor_id

Activation lift is then the read-rate (event = "article_read") among the signup-autosave visitor_id cohort vs. the historical 4/35 baseline.

Analytics note. This change introduces no new analytics event and no new dashboard widget, so the analytics-dashboard widget-count test is intentionally untouched. (If a reviewer prefers a dedicated first_article_autosaved event so the metric doesn't lean on a utm_source string, that would add one ANALYTICS_EVENTS entry and require bumping the dashboard widget-count assertion — called out here so the trade-off is explicit.)

https://claude.ai/code/session_01En2uUiiwQoRQKWenCnkPi9

@FagnerMartinsBrack

Copy link
Copy Markdown
Member Author

@claude Review this PR. Use your Read tool to read the local file .github/workflows/claude-PR-code-reviewer.md for instructions. The file is in the checked-out repository.

Context

@claude

claude Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Claude finished @FagnerMartinsBrack's task in 9m 58s —— View job


Reviewing PR #943 — first-article auto-save for new signups

  • Read review instructions
  • Gather context (git diff vs origin/main, read changed files)
  • Review new modules (last-view.ts, post-signup-redirect.ts)
  • Review auth page changes (email, Google, Apple)
  • Review view.page.ts cookie-set change
  • Check against CLAUDE.md guidelines & security concerns
  • Assess test coverage
  • Emit final review

Verdict: 0 High · 0 Medium. The change is well-constructed and mirrors the existing pending-save pattern precisely. I traced the full flow (anonymous /viewhutch_lastview cookie → signup → resolvePostSignupRedirect/queue?url=…&utm_source=signup-autosave → existing auto-submit save) and verified:

  • No open redirect / SSRF / XSS — the URL is validateSaveableUrl-normalised at set time and re-validated at redirect (post-signup-redirect.ts:17-22); the redirect target is always internal /queue, and the save POST re-validates again (rejects private-net / non-http(s)).
  • No double-save — autosave fires only when returnUrl === undefined; the pending-save /save round-trip always carries ?return=, so the two paths are mutually exclusive.
  • Bot/authed gating correct — cookie sits behind the same isbot gate as view_opened and only for req.userId === undefined (view.page.ts:242-244).
  • Cookie hygiene — httpOnly + Lax + Secure-per-origin + 2h TTL, consumed-and-cleared at signup and on POST /logout; Apple tunnels the URL through the HMAC-signed state (cross-site form_post drops Lax cookies), consistent with pendingSaveId.
  • OAuth — autosave only on genuinely new-user branches; existing-user/login and race branches keep parseReturnUrl (no autosave).
  • Test coverage — comprehensive route + unit tests across all three signup paths, plus tampered/absent-cookie and return-wins cases.

Detailed review posted below by the automation. A few low-priority notes included there.

Note: I could not execute the test suite here — the hutch:test target runs against compiled dist/ (--testMatch='**/dist/**/*.test.js'), which needs a full build first; CI runs the complete suite. Review is based on static analysis + full trace of the runtime paths.
· branch feat/first-article-autosave

@FagnerMartinsBrack

Copy link
Copy Markdown
Member Author

High Priority Issues (Must Fix)

None found.

I traced the complete runtime path (anonymous /viewhutch_lastview cookie → signup → resolvePostSignupRedirect/queue?url=…&utm_source=signup-autosave → existing auto-submit save POST) across all three signup paths and found no security or correctness defects:

  • URL is validated twice and can't open-redirect / SSRF. The cookie only ever holds validation.url from validateSaveableUrl (view.page.ts:157,243), and resolvePostSignupRedirect re-validates with the same validator before building the location (post-signup-redirect.ts:19-22), so a tampered hutch_lastview cannot reach the save. The redirect target is always the internal /queue; the save POST re-validates a third time (rejecting non-http(s) and private-network hosts).
  • No double-save. Autosave only fires when returnUrl === undefined. The pending-save /save flow always redirects through ?return=… (save.page.ts:63), so resolvePostSignupRedirect returns the return URL and skips autosave — the two paths are mutually exclusive.
  • Gating is correct. The cookie sits behind the same isbot gate as view_opened and only for req.userId === undefined (view.page.ts:230-244) — bots, prefetches, and authenticated viewers never set it. OAuth autosave is wired only into genuine new-user branches; existing-user/login and create-race branches keep parseReturnUrl (google-auth.page.ts:169,194,234 vs 217,273).
  • Cookie hygiene matches pending-save. httpOnly + SameSite=Lax + Secure-per-origin + 2h TTL, consumed-and-cleared at signup and on POST /logout (auth.page.ts:500). Apple tunnels the URL through the HMAC-signed state because the cross-site form_post drops Lax cookies, and clears the response cookie defensively — identical to how pendingSaveId is handled.
  • Backward compatible. With lastViewUrl absent, resolvePostSignupRedirect returns exactly parseReturnUrl({ return: returnUrl }), preserving prior redirect behavior.

Medium Priority Issues

None found.

Test coverage is comprehensive: unit tests for the pure resolver (return-wins, fallback, autosave, utm marker, and tampered not-a-url / javascript: / loopback cases), unit tests for the cookie module (read/consume/clear), plus route tests for the cookie-set gate (anon/authed/bot) and all three signup paths (autosave + clear, explicit-return-wins, no-cookie, tampered-cookie, trial branch, logout clear, Apple state-tunnel). CLAUDE.md conventions are respected — no process.env, no as, branded SaveableUrl, named-param objects, and the diff is Open/Closed-shaped (new modules + delegating call sites).

Low Priority Suggestions

1. The utm_source=signup-autosave marker is not 1:1-guaranteed for the funnel

Measurement rides a utm_source string on the post-signup /queue pageview rather than a discrete event. That string is not unique to this trigger by construction: if any future flow ever reuses utm_source=signup-autosave, or a user reloads/shares the post-signup /queue?url=…&utm_source=signup-autosave URL, the autosaves_triggered query counts it again. The PR already calls this trade-off out. If the metric proves load-bearing, a server-emitted first_article_autosaved event at the redirect decision point in resolvePostSignupRedirect's caller would be exactly 1:1 with the trigger and immune to URL reuse (at the cost of one ANALYTICS_EVENTS entry + the dashboard widget-count bump). No change requested now — just flagging the fragility.

2. A stale pending-save can co-attribute with an autosave

Autosave and the pending_save_id conversion tag can co-occur via a stale cookie: an abandoned /save leaves hutch_psid (1h TTL); if the visitor then reads a different article (setting hutch_lastview) and signs up via a nav CTA (no ?return=), the conversion event is tagged pending_save_id: A while article B is auto-saved. This is pre-existing behavior for pending_save_id (this PR doesn't change emitUserCreated) and is functionally harmless, but it's worth knowing when joining the autosave cohort to saves in the funnel.

3. readLastViewUrl defers value validation (consistent, no action needed)

Unlike readPendingSaveId, which zod-validates the UUID shape at read time, readLastViewUrl (last-view.ts:19-23) returns the raw string and leaves validation to resolvePostSignupRedirect. This is correct and intentional (the value is a free-form URL, not a fixed shape), and the deferred re-validation fully covers the tamper case — noting it only so the asymmetry with the sibling module reads as deliberate rather than an oversight.

@FagnerMartinsBrack

Copy link
Copy Markdown
Member Author

@claude Resolve conflicts. Use your Read tool to read the local file .github/workflows/claude-PR-conflict-fixer.md for instructions. The file is in the checked-out repository.

Context

@claude

claude Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Claude finished @FagnerMartinsBrack's task in 20m 42s —— View job


✅ Merge conflicts resolved

  • Read conflict-resolution instructions
  • Understand PR intent (full commit history)
  • Rebase onto origin/main and identify conflicts
  • Resolve conflicts (cross-referencing PR intent)
  • Complete rebase
  • Run pnpm check to verify — green
  • Force-push with --force-with-lease

Branch is now a clean descendant of the latest origin/main (3fa0019b) — mergeable, no conflicts.

What conflicted

One file: projects/hutch/src/runtime/web/auth/auth.page.ts (imports block).

Root cause: main (via release Sign in with Apple to all users, #933) removed the QuerystringFeatureToggle import and the featureToggle dependency from the auth routes — Apple sign-in is now always-on rather than toggle-gated. This PR added two new imports immediately adjacent to that now-deleted import line, so the two edits overlapped.

Resolution: kept the PR's new imports (consumeLastViewUrl/LAST_VIEW_COOKIE_NAME, resolvePostSignupRedirect) and honored main's removal of the unused QuerystringFeatureToggle import. Confirmed the whole merge is internally consistent:

  • initAuthRoutes no longer declares or receives featureToggleauth.page.ts, the server.ts call site, and auth.route.test.ts all agree.
  • main's other featureToggle usages (queue + inbox routers) are untouched.
  • No PR intent reverted — the resolvePostSignupRedirect wiring, hutch_lastview consume/clear, and logout cookie-clear all survive.

Second rebase (main advanced mid-fix)

While resolving, 3fa0019b ("hide the nav on scroll in the queue") landed on main and touched view/view.route.test.ts — a file this PR also edits. I re-rebased onto the new tip; it auto-merged cleanly (the PR's hutch_lastview test block and main's nav-scroll tests occupy different regions), and re-ran the full check on the new base.

Verification

Full pnpm nx run hutch:check passes on the final base:

  • Type-check (tsc), biome lint, knip, unused-css — all green
  • E2E — 8/8 passed
  • Coverage — functions 100%, statements 99.51%, branches 97.08%, lines 99.51% (all thresholds met)
  • 412 tests across the affected auth/view/last-view/post-signup suites pass
    · branch feat/first-article-autosave

@FagnerMartinsBrack

Copy link
Copy Markdown
Member Author

@claude Review this PR. Use your Read tool to read the local file .github/workflows/claude-PR-code-reviewer.md for instructions. The file is in the checked-out repository.

Context

@claude

claude Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Claude finished @FagnerMartinsBrack's task in 8m 4s —— View job


Reviewing PR #943 — first-article auto-save for new signups

  • Read review instructions
  • Gather context (git diff vs origin/main, read changed files)
  • Review new modules (last-view.ts, post-signup-redirect.ts)
  • Review auth page changes (email, Google, Apple)
  • Review view.page.ts cookie-set change
  • Trace the /queue?url= auto-submit save + utm_source attribution
  • Check against CLAUDE.md guidelines & security concerns
  • Assess test coverage
  • Emit final review

Verdict: 0 High · 0 Medium. Re-reviewed at the rebased tip (687a4454). I traced the full runtime chain end to end and confirmed the mechanism, not just the wiring:

  • Save actually fires, marker stays on the pageview. /queue?url=X&utm_source=signup-autosavefilterUrlsaveUrl: X → template renders value="{{saveUrl}}" + data-auto-submit, and AUTO_SUBMIT_SCRIPT calls requestSubmit() on a form whose action is the fixed track '/queue/save' source='queue' link (queue.page.ts:561,636, queue.template.html:20-21, queue.component.ts:171-189). So utm_source lands on the GET pageview only — the save POST body is just {url}. The PR's measurement claim holds.
  • URL is validated three times and can't open-redirect / SSRF / XSSvalidateSaveableUrl at view-set (view.page.ts:152,157,243), re-validated in resolvePostSignupRedirect (post-signup-redirect.ts:19-22), and again at the save POST (queue.page.ts:657). Redirect target is always the internal /queue; URLSearchParams encodes the value (no param injection); Handlebars escapes {{saveUrl}} and the scheme is http(s)-only (no javascript:).
  • No double-save — autosave fires only when returnUrl === undefined; the pending-save /save flow always carries ?return=. returnUrl comes from extractReturnUrl, so it's a /-prefixed path or undefined — never "", so the guard can't be fooled.
  • Gating / session ordering correct — cookie set behind the same isbot gate as view_opened and only for req.userId === undefined (view.page.ts:230-244); every signup branch sets the session cookie before the autosave redirect, so the /queue load is authenticated (email free+trial auth.page.ts:303-319,348-364, Google new-user google-auth.page.ts:216,272, race/existing branches keep parseReturnUrl).
  • Cookie hygiene mirrors pending-save — httpOnly + Lax + Secure-per-origin + 2h TTL, consumed-and-cleared at signup and on POST /logout; Apple tunnels the URL through the HMAC-signed state and defensively clears the response cookie.

Three low-priority notes in the detailed review below (Apple state-cookie size growth, the end-to-end persistence test boundary, and the utm_source measurement fragility already called out in the PR body). No blocking issues.

Note: I relied on static analysis + full runtime trace rather than re-running the suite — the conflict-fixer already ran pnpm nx run hutch:check green on this exact commit ~30 min ago, and hutch:test compiles to dist/ first (heavy), so re-running here adds little. CI covers the full suite.
· branch feat/first-article-autosave

@FagnerMartinsBrack

Copy link
Copy Markdown
Member Author

High Priority Issues (Must Fix)

None found.

I traced the complete runtime chain at the rebased tip (687a4454) — anonymous /viewhutch_lastview → signup → resolvePostSignupRedirect/queue?url=…&utm_source=signup-autosave → the existing auto-submit save — across all three signup paths, and found no security or correctness defect.

  • The save genuinely fires and the marker stays on the pageview. /queue?url=X becomes filterUrlsaveUrl: X (queue.page.ts:561,636); the template renders the save input value="{{saveUrl}}" with a data-auto-submit attribute (queue.template.html:20-21) and AUTO_SUBMIT_SCRIPT calls form.requestSubmit() (queue.component.ts:171-189). The form's action is the fixed track '/queue/save' source='queue' link and the POST body is only {url}, so utm_source=signup-autosave lands on the GET pageview and never on view_save_intent — the PR's attribution design holds.
  • URL validated three times; no open-redirect / SSRF / XSS. validateSaveableUrl at set time (view.page.ts:152,157,243), re-validated in resolvePostSignupRedirect (post-signup-redirect.ts:19-22), and again at the save POST (queue.page.ts:657). It rejects non-http(s) schemes (javascript:, data:, file:) and private-network/loopback hosts. The redirect target is always the internal /queue; URLSearchParams.toString() percent-encodes the value so no extra query params can be injected; Handlebars escapes {{saveUrl}}.
  • No double-save. Autosave fires only when returnUrl === undefined, and the pending-save /save round-trip always carries ?return=. returnUrl is derived from extractReturnUrl (auth.page.ts:244), so it is a /-prefixed path or undefined — never "" — meaning the !== undefined guard can't be fooled by an empty value.
  • Gating and session ordering are correct. The cookie sits behind the same isbot gate as view_opened and only for req.userId === undefined (view.page.ts:230-244). Every new-user branch sets the session cookie before the autosave redirect, so the follow-on /queue load is authenticated (email free+trial auth.page.ts:303-319,348-364; Google google-auth.page.ts:216,272); existing-user and create-race branches keep parseReturnUrl and never autosave.
  • Cookie hygiene mirrors pending-save exactly. httpOnly + SameSite=Lax + Secure-per-origin + 2h TTL, consumed-and-cleared at signup and on POST /logout; Apple tunnels the URL through the HMAC-signed state (Lax cookie is dropped on the cross-site form_post) and defensively clears the response cookie — identical to pendingSaveId.
  • Backward compatible. With lastViewUrl absent, resolvePostSignupRedirect returns exactly parseReturnUrl({ return: returnUrl }), preserving prior redirect behavior.

Medium Priority Issues

None found.

CLAUDE.md conventions are respected: named-param objects (setLastViewUrl, consumeLastViewUrl, resolvePostSignupRedirect), no process.env, no as, branded SaveableUrl used at every boundary, and comments explain why (TTL rationale, clear-on-consume rationale). The diff is Open/Closed-shaped — new pure modules plus delegating call sites — and test coverage is comprehensive: the pure resolver (return-wins / fallback / autosave / utm marker / tampered not-a-url, javascript:, loopback), the cookie module (read / consume / clear), the view-set gate (anon / authed / bot), and all three signup paths (autosave + clear, explicit-return-wins, no-cookie, tampered-cookie, trial branch, logout clear, Apple state-tunnel).

Low Priority Suggestions

1. Tunneling the article URL grows the Apple hutch_astate cookie toward the browser size cap

apple-auth.page.ts now adds lastViewUrl (a full article URL) to the signed hutch_astate payload alongside the already-tunneled returnUrl, attribution, visitorId, and pendingSaveId. validateSaveableUrl puts no length bound on the URL (it accepts any parseable http(s) URL, including 1–3 KB tracking-laden ones via parsed.toString()), and the state also carries the HMAC signature. A pathologically long article URL could push the serialized+signed hutch_astate toward the ~4 KB per-cookie browser limit; if it's exceeded the browser silently drops the cookie and Apple sign-in fails (nonce/state lost). This is unlikely in practice — the same URL already fit as the standalone hutch_lastview cookie, and the design already tunnels the URL-length returnUrl — but it's the one genuinely new consideration this PR adds to the state payload, and Apple sign-in was only just released to all users (#933). Consider skipping the lastViewUrl tunnel when it would push the payload past a safe threshold (autosave simply degrades to a plain /queue, which is the existing behavior). Fix this →

2. No single test asserts the end-to-end persistence of the autosave

The route tests assert the post-signup redirect Location is /queue?url=…&utm_source=signup-autosave, and the auto-submit prefill (data-auto-submit + requestSubmit) is covered separately in queue.listing.filters.route.test.ts. Both halves of the seam are tested, but no test stitches signup → /queue?url= → a persisted save row for the new user, because the final save is driven by client-side JS that supertest doesn't execute. This is consistent with the repo's integration-over-E2E strategy and the seam is well-defined, so it's acceptable — noting only that the persistence guarantee rests on the (separately-tested, pre-existing) auto-submit rather than on a single chain assertion.

3. Measurement rides a utm_source string rather than a discrete event

autosaves_triggered counts pageviews carrying utm_source=signup-autosave, which is not 1:1 with the trigger by construction: a reload, share, or bookmark of the post-signup /queue?url=…&utm_source=signup-autosave URL re-counts (and would idempotently re-save, so no data corruption — just inflated attribution). The PR body already calls this out. If the metric becomes load-bearing, a server-emitted first_article_autosaved event at the redirect decision point in resolvePostSignupRedirect's callers would be exactly 1:1 and immune to URL reuse, at the cost of one ANALYTICS_EVENTS entry plus the dashboard widget-count bump.

Only ~4 of 35 new users per month read anything, and 23 of 34 signups arrive
without a pending save (they clicked a nav/homepage signup CTA, not "Save on
the reader"). A new user who lands on an empty queue has nothing to do; one
whose queue already holds the article that brought them has an immediate reason
to come back.

On an anonymous /view open (behind the same isbot gate as view_opened), set a
2h httpOnly hutch_lastview cookie with the article URL. On signup success, when
the redirect would default to /queue (no explicit ?return=) and the cookie is
present and passes validateSaveableUrl, redirect to
/queue?url=<article>&utm_source=signup-autosave so the existing auto-submit save
form drops the first article into the new queue. A shared resolvePostSignupRedirect
helper keeps the three signup paths (email, Google, Apple) consistent; Apple
tunnels the URL through its signed state because its cross-site form_post callback
drops the Lax cookie. Cookie is cleared on consume and on logout; an explicit
?return= (the /save round-trip) always wins so nothing double-saves.

Claude-Session: https://claude.ai/code/session_01En2uUiiwQoRQKWenCnkPi9
@FagnerMartinsBrack

Copy link
Copy Markdown
Member Author

@claude Review this PR. Use your Read tool to read the local file .github/workflows/claude-PR-code-reviewer.md for instructions. The file is in the checked-out repository.

Context

@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @FagnerMartinsBrack's task in 6m 27s —— View job


Reviewing PR #943 — first-article auto-save for new signups

  • Read review instructions
  • Gather context (git diff vs origin/main, read changed files)
  • Review new modules (last-view.ts, post-signup-redirect.ts)
  • Review auth page changes (email, Google, Apple)
  • Review view.page.ts cookie-set change
  • Check against CLAUDE.md guidelines & security concerns
  • Assess test coverage
  • Emit final review

Verdict: 0 High · 0 Medium. Reviewed at the squashed tip (d7a8928c). I re-traced the full runtime chain end to end — anonymous /viewhutch_lastview → signup → resolvePostSignupRedirect/queue?url=…&utm_source=signup-autosave → the existing auto-submit save — across all three signup paths, and independently verified the mechanism, not just the wiring:

  • URL validated three times; no open-redirect / SSRF / XSS. validateSaveableUrl at set time (view.page.ts:152-157, only validation.url is stored), re-validated in resolvePostSignupRedirect (post-signup-redirect.ts:20-21), and again at the save POST. The validator rejects non-http(s) schemes and private/loopback hosts and normalises via parsed.toString(). Redirect target is always the internal /queue; URLSearchParams percent-encodes the value.
  • No double-save. Autosave fires only when returnUrl === undefined. Every caller derives returnUrl from extractReturnUrl (auth.page.ts, google-auth.page.ts:181, apple-auth.page.ts:230), which returns a /-prefixed path or undefined — never "" — so the guard can't be fooled, and the pending-save /save flow always carries ?return=, making the two paths mutually exclusive.
  • New-user gating is correct in OAuth. Autosave is wired only into genuine new-user branches (Google 208-225/275-291, Apple new-user branches); existing-user login and create-race branches keep parseReturnUrl (Google 193-203/233-243, Apple 225) and never autosave.
  • Correct set-gate + session ordering. The cookie is set only for anonymous (req.userId === undefined) non-bot views, behind the same isbot gate as view_opened and after the markdown early-return (view.page.ts:230-244). Each signup branch sets the session cookie on the same response as the 303, so the follow-on same-site /queue GET is authenticated and the save lands in the new user's queue.
  • Cookie hygiene mirrors pending-save exactly — httpOnly + SameSite=Lax + Secure-per-origin + 2h TTL, consumed-and-cleared at signup and on POST /logout; Apple tunnels the URL through the HMAC-signed state (Lax cookie dropped on the cross-site form_post) and defensively clears the response cookie.
  • utm_source marker can't contaminate attributioncreateClickAttributionMiddleware short-circuits when hutch_click already exists (always true by signup time), and attribution is emitted before the redirect.
  • Backward compatible — with lastViewUrl absent, resolvePostSignupRedirect returns exactly parseReturnUrl({ return: returnUrl }).

CLAUDE.md conventions are respected (no process.env, no as, branded SaveableUrl, named-param objects, why-focused comments, no c8 ignore added). Test coverage is comprehensive across the pure resolver, cookie module, view-set gate, and all three signup paths. Three low-priority notes below — none blocking.
· branch feat/first-article-autosave

@FagnerMartinsBrack

Copy link
Copy Markdown
Member Author

High Priority Issues (Must Fix)

None found.

I traced the complete runtime chain at the squashed tip (d7a8928c) — anonymous /viewhutch_lastview → signup → resolvePostSignupRedirect/queue?url=…&utm_source=signup-autosave → the existing auto-submit save — across all three signup paths, and found no security or correctness defect.

  • URL validated three times; no open-redirect / SSRF / XSS. The cookie only ever holds validation.url from validateSaveableUrl (view.page.ts:152,157,243); resolvePostSignupRedirect re-validates with the same validator (post-signup-redirect.ts:20-21); and the save POST validates a third time. validateSaveableUrl rejects non-http(s) schemes (javascript:, data:, file:) and private-network/loopback hosts, and normalises via parsed.toString() (saveable-url.ts:86-101). The redirect target is always the internal /queue; URLSearchParams.toString() percent-encodes the value so no extra params can be injected.
  • No double-save. Autosave fires only when returnUrl === undefined. Every call site derives returnUrl from extractReturnUrl (google-auth.page.ts:181, apple-auth.page.ts:230, and the email path), which returns a /-prefixed path or undefined — never "" — so the !== undefined guard is robust, and the pending-save /save round-trip always carries ?return=, making the two paths mutually exclusive.
  • New-user gating is correct. Autosave is wired only into genuine new-user branches (Google free 208-225 and trial 275-291; Apple new-user branches). Existing-user login and create-race branches keep parseReturnUrl (Google 193-203/233-243, Apple 225) and never autosave.
  • Set-gate + session ordering correct. The cookie is set only for anonymous (req.userId === undefined) non-bot views, behind the same isbot gate as view_opened and after the markdown early-return (view.page.ts:230-244), so bots, prefetches, markdown/LLM fetches, and authenticated viewers never set it. Each signup branch sets the session cookie on the same response as the 303, so the follow-on same-site /queue GET is authenticated and the save lands in the brand-new user's queue.
  • Cookie hygiene mirrors pending-save exactly — httpOnly + SameSite=Lax + Secure-per-origin + 2h TTL (last-view.ts via baseCookieOptions), consumed-and-cleared at signup and cleared on POST /logout; Apple tunnels the URL through the HMAC-signed state (the Lax cookie is dropped on the cross-site form_post) and defensively clears the response cookie.
  • No attribution contamination. createClickAttributionMiddleware short-circuits when a hutch_click cookie already exists (click-attribution.middleware.ts:67) — always true by the time a user reaches the post-signup /queue?…utm_source=signup-autosave GET — and attribution is emitted on emitUserCreated before the autosave redirect. The internal marker can never overwrite acquisition attribution.
  • Backward compatible. With lastViewUrl absent, resolvePostSignupRedirect returns exactly parseReturnUrl({ return: returnUrl }), preserving prior redirect behaviour.

Medium Priority Issues

None found.

CLAUDE.md conventions are respected: named-param objects (setLastViewUrl, consumeLastViewUrl, resolvePostSignupRedirect), no process.env (secureCookies threaded from the composition root at server.ts:427), no as, branded SaveableUrl at every boundary, and comments that explain why (TTL rationale, clear-on-consume rationale, cross-site clear rationale). The diff is Open/Closed-shaped — two new pure modules plus delegating call sites — with no c8 ignore added. Test coverage is comprehensive: the pure resolver (return-wins / fallback / autosave / utm marker / tampered not-a-url, javascript:, loopback), the cookie module (read / consume / clear), the view-set gate (anon / authed / bot), and all three signup paths (autosave + clear, explicit-return-wins, no-cookie, tampered-cookie, trial branch, logout clear, Apple state-tunnel).

Low Priority Suggestions

1. Tunneling the article URL grows the Apple hutch_astate signed cookie toward the 4 KB browser cap

apple-auth.page.ts:131 now adds lastViewUrl (a full article URL) to the signed hutch_astate payload alongside the already-tunneled returnUrl, attribution, visitorId, and pendingSaveId, plus the HMAC signature. validateSaveableUrl puts no length bound on the URL (it accepts any parseable http(s) URL, including 1–3 KB tracking-laden ones via parsed.toString()). A pathologically long article URL could push the serialized+signed state toward the ~4 KB per-cookie limit; if exceeded the browser silently drops the cookie and Apple sign-in fails (nonce/state lost). Low probability — the same URL already fit as the standalone hutch_lastview cookie, and the design already tunnels the URL-length returnUrl — but it is the one genuinely new field this PR adds to the state payload, and Apple sign-in was only just released to all users (#933). Consider skipping the lastViewUrl tunnel when it would push the payload past a safe threshold (autosave then degrades to a plain /queue, the existing behaviour). Fix this →

2. "Last-viewed" is not necessarily "the article that motivated signup"

setLastViewUrl overwrites the cookie on every anonymous non-bot /view (view.page.ts:242-244), so if a visitor lands on their motivating article A, clicks through to article B on the same site, then signs up via a nav CTA, it is B that auto-saves — not A. The hypothesis frames this as saving "the article that brought them to Readplace," which recency approximates but does not guarantee for multi-article browsing sessions. This is inherent to the last-view design and is a reasonable heuristic (recency is a fine proxy and the queue is non-empty either way) — noting only that the saved article may differ from the acquisition article when the cohort is analysed.

3. Measurement rides a utm_source string rather than a discrete event

autosaves_triggered counts pageviews carrying utm_source=signup-autosave, which is not 1:1 with the trigger by construction: a reload, share, or bookmark of the post-signup /queue?url=…&utm_source=signup-autosave URL re-counts (and would idempotently re-save, so no data corruption — just inflated attribution). The PR body already calls this out. If the metric becomes load-bearing, a server-emitted first_article_autosaved event at the resolvePostSignupRedirect decision point would be exactly 1:1 and immune to URL reuse, at the cost of one ANALYTICS_EVENTS entry plus the dashboard widget-count bump.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant