Skip to content

fix: close three account-takeover paths found in review - #284

Merged
aquie00t merged 3 commits into
mainfrom
fix/review-findings
Sep 6, 2026
Merged

fix: close three account-takeover paths found in review#284
aquie00t merged 3 commits into
mainfrom
fix/review-findings

Conversation

@aquie00t

@aquie00t aquie00t commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Fixes from the /code-review and security passes over eb3c92e..origin/main (the eight feature PRs merged this week). Three of these are account takeover; please merge before the mobile app starts calling this API.

The three that matter

1. A request body could name its own identity

Fastify's default AJV setting strips unknown properties only from schemas that declare additionalProperties: false — and a plain TypeBox object declares nothing, so an unknown key survives validation and arrives in request.body intact. Three handlers spread ...request.body over an identity taken from the session, and the spread came last, so the body won:

  • POST /devices with {"currentUserId": "<victim's uuid>"} registers the attacker's phone against the victim. From then on every one of the victim's notifications — who followed them, who replied, with the content ids in the deep-link payload and their unread count as the badge — is delivered to the attacker's device. The victim sees nothing: there is no read side on /devices, and their own registration is a separate row.
  • PATCH /articles/:id with {"userId": "<victim's uuid>"} rewrites any article on the platform under its real author's byline. UpdateArticleUseCase proves ownership by asking whether the supplied userId owns the article, so the injected value does not fail that check — it satisfies it.
  • POST /articles with {"authorId": …} files a draft owned by somebody else, which the above then publishes.

User ids are not secret; GET /profile/:username returns one.

Fixed two ways, because either alone is one refactor from being untrue: AJV now runs with removeAdditional: "all", and the three handlers name their fields instead of spreading over an identity. The article handlers were pre-existing rather than introduced this week — they are fixed here anyway, since the root cause is shared and the outcome is worse.

2. The reuse alarm revoked nothing

RefreshUseCase.execute runs its whole body inside prisma.$transaction. resolveRetry called revokeAllByUserId and then threw — so Prisma rolled the revocation back with the very error that reported it. The response said "All sessions revoked"; the database was untouched.

The outcome was exactly inverted: an attacker who holds a stolen refresh token rotates it once, the victim's own client later presents the token it still holds, and the victim is ejected with a 401 while the attacker's token keeps rotating for its full thirty days. The alarm could be tripped any number of times and never converge.

The transaction now reports a compromise and the revocation runs outside it, on its own connection. The e2e test asserted only the 401 body; it now opens a second session first and asserts that session is dead afterwards, which is the property that was actually missing.

3. OAuth state was not bound to a browser

The state proved that somebody had started a flow on this deployment, not that the browser presenting the callback was that somebody — which is the entire point of state. So the login-CSRF that #278's own comment claims to close still worked: start a flow signed in as yourself, capture the callback URL without following it, get a victim to load it, and the victim's browser finishes the flow and is silently signed in as you. Everything they then write — posts, drafts, direct messages — lands in an account you can read.

The state is now also set as a signed, httpOnly, SameSite=Lax cookie scoped to /api/v1/oauth, and the callback requires it to match before the cache is even consulted.

The rest

  • Rotation no longer alarms on a second consecutive retry. The retry path retired the successor without repointing the token the client still holds, so a client that lost two responses in a row was treated as a thief and signed out of every device. It is repointed now; revokedAt is deliberately left alone so repeated retries cannot slide the window forward.
  • An in-flight idempotency claim expires in two minutes, not a day. A process that died between the claim and the response left the key answering "still in progress" for twenty-four hours, for a write that never happened.
  • A failed Play notification releases its record. It was recorded before the work, so a failure afterwards made Pub/Sub's redelivery look like a duplicate and dropped it silently.
  • STRICT keys on the edge address. The app runs trustProxy: true, so request.ip is the left-hand end of a client-written X-Forwarded-For — a caller could hand themselves a fresh login bucket per request. It now prefers CF-Connecting-IP, which Cloudflare overwrites and which cannot be reached around, since the Render subdomain is disabled.
  • Three schedulers destroy their task on close instead of dropping the reference and firing against a disconnected Prisma client; deletion survives a failed subscription cancellation; a redirect target that already carries a query string is appended to with &; the Play push secret is compared in constant time; the purchase endpoint answers the badge question through the shared helper rather than a second !== null; and OAUTH_NATIVE_REDIRECT_ALLOWLIST gets back the sync: false a conflict resolution had dropped.

Tests

  • Unit, 1536 passing (5 new): the second consecutive retry being served, the grace window not sliding, the edge address winning over the proxied one, and deletion completing when the cancellation throws.
  • E2E: tests/e2e/security/mass-assignment.test.ts — an article is filed under the caller and not the id in the body, and an edit dressed in the victim's userId is refused. The OAuth suite now carries the state cookie the way a browser does, plus a case where the state is valid but the cookie is absent. The refresh reuse case asserts a sibling session dies.
  • CI sets REFRESH_ROTATION_GRACE_SECONDS=0, so the e2e exercises the alarm; the window itself is unit-tested, where a clock can be moved.
  • tsc -p tsconfig.build.json --noEmit, eslint, prettier --check clean.

Not fixed here

A device token can still be moved between accounts on the strength of the token alone. That is the design — it is how a phone handed to a second account stops delivering the first one's notifications — but a token is not a secret, and somebody who obtains one can both silence a victim's phone and choose what appears on its lock screen. Closing it properly needs a device-scoped installation id the app generates and stores in its keystore, so it is app-side work as much as API work. Added to the roadmap rather than rushed in here.

AI Asistan: Opus 5

aquie00t and others added 3 commits September 6, 2026 16:00
**Mass assignment.** Fastify's default AJV strips unknown properties only
from schemas that declare additionalProperties: false, and a plain TypeBox
object declares nothing - so an unknown key survives validation and reaches
the handler. Three handlers spread ...request.body over an identity taken
from the session, and the body won: POST /devices could register a phone
against another account and receive every one of that account's
notifications, and PATCH /articles/:id could rewrite anybody's article,
because UpdateArticleUseCase proves ownership by asking whether the supplied
userId owns it. AJV now runs with removeAdditional: "all", and the three
handlers name their fields instead of spreading over them.

**The reuse alarm revoked nothing.** RefreshUseCase runs inside a
transaction, and resolveRetry called revokeAllByUserId and then threw - so
Prisma rolled the revocation back with the error that reported it. A stolen
token that survived the grace window ejected the victim and kept working. The
transaction now reports a compromise and the revocation happens outside it.

**OAuth state was not bound to a browser.** The state proved that somebody
had started a flow, not that this browser had. An attacker could start one
with their own account and have a victim's browser finish it, signing the
victim into the attacker's account. The state is now also set as a signed,
httpOnly cookie and must match before the callback is honoured.

Also from the review: rotation no longer alarms on a second consecutive
retry (the chain is repointed, the window is not extended); an in-flight
idempotency claim expires in two minutes rather than a day, so a crashed
request does not block its key; a Play notification that fails after being
recorded releases the record so Pub/Sub can redeliver; STRICT rate limits key
on the edge address rather than a client-writable X-Forwarded-For; three
schedulers destroy their task on close; deletion survives a failed
cancellation; a redirect target that already carries a query string is
appended to correctly; the Play secret is compared in constant time; the
purchase endpoint answers the badge question through the shared helper; and
OAUTH_NATIVE_REDIRECT_ALLOWLIST gets the sync: false a conflict resolution
had dropped.
The state cookie means a callback must now carry it, the article body field
is `body` not `content`, and the rotation retry case cannot hold with the
grace window switched off in CI - that path is unit-tested, where a clock can
be moved.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@aquie00t
aquie00t merged commit 3654255 into main Sep 6, 2026
10 checks passed
@aquie00t
aquie00t deleted the fix/review-findings branch September 6, 2026 13:13
github-actions Bot pushed a commit that referenced this pull request Sep 6, 2026
## [1.27.1](v1.27.0...v1.27.1) (2026-09-06)

### Bug Fixes

* close three account-takeover paths found in review ([#284](#284)) ([3654255](3654255))
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown

🎉 This PR is included in version 1.27.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant