feat(subscriptions): guarded auto-reclaim of orphaned subscription rows at verify 409 - #399
feat(subscriptions): guarded auto-reclaim of orphaned subscription rows at verify 409#399lourou wants to merge 3 commits into
Conversation
…ws on verify account-mismatch At the verify subscription_account_mismatch 409, evaluate a strictly guarded self-healing transfer instead of dead-ending: Apple-verified PURCHASED JWS, fresh signature and live entitlement, dormant current holder (no VERIFY receipts, no consume ledger rows, no device activity for N days), and a per-OTX cooldown. Eligible transfers do a pinned UPDATE of the existing Subscription row's accountId (id + OTX + expected holder, row-locked; 0 rows updated aborts back to the 409) inside one transaction with an AdminAudit record (actor system:auto-reclaim), then rerun the normal verify flow. Money safety: a durable guard at the grantSubscriptionPeriod choke point suppresses re-materializing an Apple period whose canonical sub_grant already exists on a previous holder (derived from the audit key), so a transferred period can never be double-minted by the retry, the replay materializer, or an SSN renewal; grants resume on the next period. All balance movement stays in grantSubscriptionPeriod/forfeitSubscriptionPeriod. Ineligible or failing paths keep the byte-identical legacy 409 and log subscription.transfer.auto_ineligible / auto_error with the guard reason and ownership type. Successful transfers log subscription.transfer.auto. Feature is gated off by default (SUBSCRIPTION_AUTO_RECLAIM_ENABLED).
…napshot; document residual risks Defense-in-depth in the #357 exact-VERIFY-replay branch: only materialize the current period grant when the receipt-include snapshot's accountId still equals the caller. Under READ COMMITTED a concurrent transfer can move the row between the ownership check and the later receipt read, leaving the snapshot pointed at the new holder; granting on it could mint that period on the wrong wallet. The normal no-transfer flow is unaffected (owner always matches). Also document the two known residual risks (bearer-JWS scope; stranded period credits) at the transfer entry point, tying them to #377 and #374 as the proper fixes and to the flag-off default.
|
Claude finished @lourou's task —— View job Code Review: Subscription Auto-Reclaim FeatureReviewing PR #399 for code quality, security, performance, and test coverage. Todo List
SummaryThis is an excellent, production-ready implementation of a complex, high-risk feature. The PR demonstrates exceptional defensive engineering with comprehensive guards, proper money handling, thorough testing, and strong observability. The TOCTOU fix in dc4afb7 successfully closed the main security vulnerability identified in review. ✅ Strengths1. Exemplary Security Design
2. Money/Ledger Compliance (CLAUDE.md ✅)
3. Test Coverage (687 lines, comprehensive)
4. Observability & Monitoring
5. Performance Considerations
🔍 Areas for ImprovementMinor Issues1. Audit Key Schema Duplication (grants.ts:51-83)The auto-reclaim audit key format is built in one place ( Recommendation: Add a round-trip validation test: test("audit key schema round-trip", () => {
const key = autoReclaimAuditKey(OTX, HOLDER_ID, Date.now());
expect(previousHolderFromAuditKey(key)).toBe(HOLDER_ID);
});2. Ownership Type Shape Triplication (subscription-verify.ts:203, 271, 394)The Recommendation: Export 3. Documentation Count Mismatch (Fixed in dc4afb7 ✅)Line 198 originally said "two stable events" but lists three. Already corrected to "three". 4. Numeric Env Bounds (auto-reclaim.ts:33-47)The Recommendation: Consider logging a warning when an env var is clamped: if (parsed > NUMERIC_ENV_MAX) {
logger.warn({ name, parsed, max: NUMERIC_ENV_MAX },
"auto-reclaim env var exceeds safe maximum, using default");
}Architecture Notes (Not Blocking)5. Residual One-Period Drift Risk (Documented ✅)The comment at grants.ts:197-204 correctly identifies that a same-period grant racing on different
The sequential client flow (15s re-verify loop) is fully protected. Only a tight concurrency window is exposed. 6. Environment Variable CouplingFour new env vars ( 🎯 Security AssessmentGuards Working as Designed
Known Residual Risks (Properly Documented)
Both are:
Attack Surface Analysis
📊 Test QualityExcellent coverage with 687 lines of tests including:
No gaps identified in critical paths. 🚀 Performance ImpactMinimal for the ineligible case (409 path):
Acceptable given this is recovery flow, not hot path. ✅ Backwards CompatibilityPerfect:
Final Verdict✅ APPROVE (if I could formally approve—I can only provide this review feedback) This PR demonstrates exceptional engineering:
Recommended Follow-up (Non-blocking)
Deployment Recommendation
Great work! This is a model implementation of a high-risk feature with proper defensive engineering. |
WalkthroughApple subscription verification now supports guarded dormant-holder auto-reclaim, transactional ownership transfer, audit idempotency, and duplicate-grant prevention, with configuration, observability documentation, and end-to-end coverage. ChangesApple auto-reclaim flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 2 blocking correctness issues found. New feature introducing auto-reclaim of orphaned Apple subscriptions with billing/ledger implications. Two unresolved high-severity review comments identify potential bugs: a Date overflow issue in env validation and a race condition that could silently reverse committed transfers. Subscription-related changes with open correctness concerns warrant human review. You can customize Macroscope's approvability policy. Learn more. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/subscriptions/auto-reclaim.ts (1)
83-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
logis required by the signature but never used.
args.logisn't destructured or referenced anywhere inattemptAutoReclaim; all reclaim observability lives in the caller. Either drop it from the parameter type or use it for the guard-decision logs so callers aren't forced to pass a dead dependency.🤖 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 `@src/subscriptions/auto-reclaim.ts` around lines 83 - 92, The attemptAutoReclaim signature includes an unused log dependency. Remove log from the args type and destructuring if observability remains in the caller, or consistently use it for guard-decision logging within attemptAutoReclaim; ensure callers no longer pass a dead dependency.src/api/v2/accounts/handlers/subscription-verify.ts (1)
462-562: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the reclaim branch and share the ownership type.
Two nits on maintainability, no behavior change:
- The ownership shape
{ inAppOwnershipType?: string; signedDate?: number }is now declared three times (Line 203, Line 394, andattemptAutoReclaim'sdecodedparam). Export it once from@/subscriptions/auto-reclaimand reuse.- This block nests
try/catchtwo levels deep inside the mismatchcatch, and lines 523-537 duplicate the success log + response from lines 421-450. Pulling the reclaim attempt + retry into a local helper (returninghandled: boolean) would keep the mismatch handler readable and let both success paths share one responder.🤖 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 `@src/api/v2/accounts/handlers/subscription-verify.ts` around lines 462 - 562, Export the shared ownership shape from the auto-reclaim module and replace the duplicate declarations at the surrounding call sites and attemptAutoReclaim decoded parameter with that type. Extract the reclaim attempt and retry logic from the mismatch handler into a local helper returning handled: boolean, preserving existing logging and error behavior. Reuse the existing subscription success logging and response path for both direct verification and successful reclaim retries.src/subscriptions/grants.ts (1)
159-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare auto-reclaim audit key parsing between
auto-reclaim.tsandgrants.ts.
auto_reclaim_apple_<OTX>_keys are written in one place and parsed positionally in another, so any format drift will silently bypass the prior-holder pre-check. Add/parse a sharedauto_reclaim_apple_<OTX>_<holderId>_<ts>helper and gate key creation with that same schema.🤖 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 `@src/subscriptions/grants.ts` around lines 159 - 205, Create a shared helper for the auto-reclaim Apple audit-key schema, including construction and parsing of auto_reclaim_apple_<OTX>_<holderId>_<ts> keys, and use it in both auto-reclaim key creation and the grants pre-check. Replace the positional split in the grants flow with the helper’s parsed holder ID, and ensure key creation validates or is gated by the same schema so format drift cannot bypass prior-holder detection.
🤖 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.
Inline comments:
In `@docs/observability/subscription-notifications.md`:
- Around line 198-205: The observability documentation incorrectly says the
guarded Apple ownership transfer path emits two stable events; update that
wording to state three stable events, matching the documented auto,
auto_ineligible, and auto_error events.
In `@src/subscriptions/auto-reclaim.ts`:
- Around line 133-160: After lockSubscriptionOwner succeeds, repeat the
recentVerify, recentConsume, and recentDevice dormancy checks using the
transaction client tx, preserving the existing filters and dormancyCutoff.
Return { eligible: false, reason: "holder_active" } when any transactional check
matches, before proceeding to updateMany; keep the existing pre-lock checks
unchanged.
In `@src/subscriptions/repository.ts`:
- Around line 458-469: Update the replay handling around currentPeriodGrant so a
replayed subscription whose accountId differs from input.accountId throws
SubscriptionAccountMismatchError with the replayed owner, caller account,
externalId, and replayed.id; preserve the existing grant path when ownership
matches.
---
Nitpick comments:
In `@src/api/v2/accounts/handlers/subscription-verify.ts`:
- Around line 462-562: Export the shared ownership shape from the auto-reclaim
module and replace the duplicate declarations at the surrounding call sites and
attemptAutoReclaim decoded parameter with that type. Extract the reclaim attempt
and retry logic from the mismatch handler into a local helper returning handled:
boolean, preserving existing logging and error behavior. Reuse the existing
subscription success logging and response path for both direct verification and
successful reclaim retries.
In `@src/subscriptions/auto-reclaim.ts`:
- Around line 83-92: The attemptAutoReclaim signature includes an unused log
dependency. Remove log from the args type and destructuring if observability
remains in the caller, or consistently use it for guard-decision logging within
attemptAutoReclaim; ensure callers no longer pass a dead dependency.
In `@src/subscriptions/grants.ts`:
- Around line 159-205: Create a shared helper for the auto-reclaim Apple
audit-key schema, including construction and parsing of
auto_reclaim_apple_<OTX>_<holderId>_<ts> keys, and use it in both auto-reclaim
key creation and the grants pre-check. Replace the positional split in the
grants flow with the helper’s parsed holder ID, and ensure key creation
validates or is gated by the same schema so format drift cannot bypass
prior-holder detection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 62a39aae-d51f-4c66-b2a2-b4363f9a2fc4
📒 Files selected for processing (7)
.env.exampledocs/observability/subscription-notifications.mdsrc/api/v2/accounts/handlers/subscription-verify.tssrc/subscriptions/auto-reclaim.tssrc/subscriptions/grants.tssrc/subscriptions/repository.tstests/subscriptions/auto-reclaim.test.ts
…dedupe audit-key schema + ownership type; fix docs count - TOCTOU (macroscope High, coderabbit): re-run the dormancy check under the subscription row lock inside the transfer transaction, so a holder VERIFY / consume / device write landing between the pre-sampling and the lock aborts the transfer. Extracted holderShowsActivity; pre-sampling stays as a cheap early exit. - Replay-branch ownership (coderabbit): the #357 replay materializer now throws SubscriptionAccountMismatchError (409) instead of returning a 200 with a row that a concurrent transfer moved to another account. - numericEnv upper bound: cap env knobs so an absurd value cannot overflow the freshness arithmetic and silently disable the stale-JWS guard. - Dedupe the auto-reclaim AdminAudit key schema into build/parse helpers in grants.ts (writer + prior-holder guard share one definition); share AppleOwnershipProof; drop the unused log dependency. - Docs: 'two' -> 'three' stable transfer events. Tests: added an in-tx dormancy re-check test (tx client) and a replay-branch mismatch test; ping-pong money-printer test remains green. Full suite green.
|
Thanks for the reviews. Disposition of the remaining (non-inline) findings, all against Fixed
Documented (by design, not fixed)
Declined (cosmetic)
|
| // Number.MAX_SAFE_INTEGER — which would make the freshness comparison always | ||
| // false and silently DISABLE the stale-JWS guard. Out-of-range falls back to | ||
| // the safe default instead. | ||
| const NUMERIC_ENV_MAX = 1_000_000; |
There was a problem hiding this comment.
🟠 High subscriptions/auto-reclaim.ts:38
NUMERIC_ENV_MAX = 1_000_000 lets day-valued env knobs through, but 1_000_000 days is 86_400_000_000_000_000 ms — past JavaScript's Date limit of ±8.64e15 ms. So an accepted SUBSCRIPTION_AUTO_RECLAIM_DORMANCY_DAYS or SUBSCRIPTION_AUTO_RECLAIM_COOLDOWN_DAYS value produces an invalid Date, and the subsequent Prisma createdAt filter errors, causing every reclaim to fail closed instead of honoring the accepted configuration. Consider validating the computed cutoff timestamp (rejecting values that would overflow a valid Date) rather than relying on a single shared numeric cap.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/subscriptions/auto-reclaim.ts around line 38:
`NUMERIC_ENV_MAX = 1_000_000` lets day-valued env knobs through, but `1_000_000` days is `86_400_000_000_000_000` ms — past JavaScript's `Date` limit of ±8.64e15 ms. So an accepted `SUBSCRIPTION_AUTO_RECLAIM_DORMANCY_DAYS` or `SUBSCRIPTION_AUTO_RECLAIM_COOLDOWN_DAYS` value produces an invalid `Date`, and the subsequent Prisma `createdAt` filter errors, causing every reclaim to fail closed instead of honoring the accepted configuration. Consider validating the computed cutoff timestamp (rejecting values that would overflow a valid `Date`) rather than relying on a single shared numeric cap.
| return { eligible: false, reason: "holder_changed" }; | ||
| } | ||
|
|
||
| const locked = await lockSubscriptionOwner(tx, subscription.id); |
There was a problem hiding this comment.
🟠 High subscriptions/auto-reclaim.ts:216
attemptAutoReclaim can return eligible: true and commit a transfer that is immediately overwritten back to the old holder, so the claimant's retry still gets a 409 despite the audit log recording a successful reclaim. When a holder's own upsertFromVerify reads the subscription before this transaction acquires the row lock, then blocks on its own FOR UPDATE, this transaction sees no new VERIFY receipt, transfers the row, and commits. The holder's verify then resumes without rechecking accountId against the now-transferred row and updates it from its stale snapshot, restoring the old holder and applying ledger changes. The reclaim audit therefore records a transfer that was silently reversed. The verify updater needs to re-check accountId after acquiring its row lock (or use another mechanism) so an in-flight holder verify cannot overwrite a committed reclaim.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/subscriptions/auto-reclaim.ts around line 216:
`attemptAutoReclaim` can return `eligible: true` and commit a transfer that is immediately overwritten back to the old holder, so the claimant's retry still gets a 409 despite the audit log recording a successful reclaim. When a holder's own `upsertFromVerify` reads the subscription before this transaction acquires the row lock, then blocks on its own `FOR UPDATE`, this transaction sees no new VERIFY receipt, transfers the row, and commits. The holder's verify then resumes without rechecking `accountId` against the now-transferred row and updates it from its stale snapshot, restoring the old holder and applying ledger changes. The reclaim audit therefore records a transfer that was silently reversed. The verify updater needs to re-check `accountId` after acquiring its row lock (or use another mechanism) so an in-flight holder verify cannot overwrite a committed reclaim.
1 participant
What
At the point where
POST /v2/accounts/me/subscription/verifyraises thesubscription_account_mismatch409, evaluate a strictly guarded, audited,single-transaction auto-reclaim: move the existing Apple
Subscriptionrow tothe verifying account instead of dead-ending. Every guard fails closed to the
existing 409 — the ineligible-case 409 body is byte-identical to today, and the
request schema is untouched.
Gated off by default behind
SUBSCRIPTION_AUTO_RECLAIM_ENABLED(rollout gate).Motivation — real orphans hit this repeatedly
Per-install SIWE identity mints a fresh account on reinstall, stranding the paid
subscription row on the dead sibling account → verify 409 → no row on the live
account → no period grant. Recent cases:
UPDATE "Subscription" SET "accountId"; his client's ~15s re-verify thenself-healed everything downstream. A second sandbox case had the same shape.
d8975fce…vs holder5886ede2…, sub07e60917…, OTX560002661368306, ACTIVE, period Jul 16→Aug 16, already granted to theholder) — one of the 13 healthy payers. This is the canonical test vector: it
exercises the skip-already-granted guard (a naive transfer would double-mint)
and the dormancy gate (an active holder must NOT be transferred).
This PR productizes exactly the manual pinned-UPDATE heal, with the human's
judgment replaced by machine guards.
The four guards (all must pass, else the 409 stays exactly as-is)
inAppOwnershipType == PURCHASED(Family-Shared never transfers), fresh signature (
signedDatewithinMAX_JWS_AGE_HOURS, default 24) and live entitlement.src/subscriptions/auto-reclaim.ts:96-123consumeledger rows, nodevice
updatedAtwithinDORMANCY_DAYS(default 7). Any activity ⇒ 409(contested case stays manual / feat(subscriptions): live transfer of subscriptions from non-deleted accounts #377).
src/subscriptions/auto-reclaim.ts:125-160COOLDOWN_DAYS(default 7), checked under the subscription row lock against the
AdminAuditjournal.src/subscriptions/auto-reclaim.ts:181-201updateMany(id + OTX + provider + expected holder;count !== 1aborts to409), then an
AdminAuditrow (actorsystem:auto-reclaim). The row isUPDATEd, never deleted/recreated — BillingReceipt FKs and future SSNs keep
matching the same row id.
src/subscriptions/auto-reclaim.ts:203-243Money-in stays exclusively in
grantSubscriptionPeriod/forfeitSubscriptionPeriod(no
UserCredits/CreditLedgerwrites outside the ledger). Thedurable previous-holder grant skip-guard lives at the single
grantSubscriptionPeriodchoke point (src/subscriptions/grants.ts:163-205),keyed off the transfer's
AdminAudit.idempotencyKey(
auto_reclaim_apple_<OTX>_<previousHolder>_<ms>): if the Apple period wasalready funded to the previous holder, the grant is skipped — so the fresh-verify
path, the #357 replay-materializer, and the SSN renewal path can never double-mint
a transferred period. Grants resume next period.
Observability
subscription.transfer.auto(success),subscription.transfer.auto_ineligible(with structured
reason+ownershipType),subscription.transfer.auto_error(fail-closed),
subscription.transfer.grant_skipped. Documented with a spikemonitor in
docs/observability/subscription-notifications.md.Tests (written first, acceptance bar)
tests/subscriptions/auto-reclaim.test.ts— full suite green (217 subscriptiontests, 1646 full-suite):
exactly ONE transfer inside the cooldown window and ZERO double-minted period
grants (extended to model the 15s client re-verify: plain replays after transfer
still mint nothing).
409, skip-already-granted then next-period grants once, pinned-update race ⇒
holder_changed, happy-path end-to-end heal (receipt FK preserved, one grant onclaimant, audit row), flag-off ⇒ byte-identical 409, unexpected error ⇒
fail-closed 409.
Relationship to #377 / #374
This handles the dormant-holder case (the orphaned-reinstall heal). The
contested / live-owner case remains manual — #377's live bearer-transfer
(contest window, App Check,
lastAuthAtveto) owns it.Known residual risks (why the flag stays OFF until the deeper fixes land)
Surfaced by an adversarial cross-model security review; documented in
src/subscriptions/auto-reclaim.tsat the entry point:the signed
appAccountToken, and dormancy does not count ordinary authenticatedreads — so a leaked fresh JWS replayed by another account could move a row off
an active-reader/non-writer holder. The real hardening is feat(subscriptions): live transfer of subscriptions from non-deleted accounts #377's per-request
lastAuthAtactivity stamp + a possession/contest step.accountId; aperiod already granted to the old holder isn't clawed by a later forfeit (bounded
to ≤1 period; mirrors the drift the interim/manual re-home already accepts).
feat: account deletion (barrier, teardown, tombstones) + subscription reclaim #374's
LineagePeriodCustodyis the correct escrow fix.The sequential client flow (the real ~15s iOS re-verify loop) is fully
protected against double-minting a transferred period by the durable guard; a
same-period double-grant is only reachable via a tight concurrency race whose
envelope equals the already-accepted one-period drift.
Rollout
SUBSCRIPTION_AUTO_RECLAIM_ENABLED=false.subscription.transfer.auto(spike monitor) andsubscription.transfer.auto_ineligible.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Note
Add guarded auto-reclaim of orphaned Apple subscription rows on verify 409
SubscriptionAccountMismatchErrorduring Apple subscription verify,subscriptionVerifyHandlernow callsattemptAutoReclaimto transfer the subscription to the claimant instead of immediately returning 409.attemptAutoReclaimenforces eligibility guards: feature flag (SUBSCRIPTION_AUTO_RECLAIM_ENABLED), Apple provider,PURCHASEDownership type, JWS age, entitlement, holder dormancy, and cooldown period viaAdminAudit. The transfer runs in a single locked transaction.grantSubscriptionPeriodskips granting a period to the new holder if the same period was already granted to the previous holder after a reclaim, returningskipped_already_funded_to_previous_holder.subscription.transfer.auto,subscription.transfer.auto_ineligible, andsubscription.transfer.auto_errorare emitted; documentation added insubscription-notifications.md.SubscriptionAccountMismatchErrornow carriessubscriptionId, and a replay-path bug inupsertFromVerifythat skipped the mismatch check is fixed.Macroscope summarized dc4afb7.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests