Skip to content

Fix account takeover and privilege escalation paths in the API - #302

Closed
kaareal wants to merge 12 commits into
masterfrom
security/api-fixes
Closed

kaareal wants to merge 12 commits into
masterfrom
security/api-fixes

Conversation

@kaareal

@kaareal kaareal commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

A security review of the API turned up seven issues, verified individually before fixing. Each fix has a regression test. All 411 API tests pass.

What was wrong

# Issue Fix
1 roles had no writeAccess, so any signed-up user could PATCH /1/users/me with {"roles":[{"role":"superAdmin","scope":"global"}]} and become superAdmin. isTester was writable the same way, and it makes the login code a fixed value the response returns. Both moved into a $staff scope, writable by admins only.
2 /auth/password/update accepted any access token. Unsubscribe links in notification emails carry that same type, so a leaked unsubscribe URL could set the password and return a full session, with no MFA step. The route requires action === 'reset-password'.
3 enableTotp never set isMfa, so the recent-password check never ran and /auth/totp/login was a full login from email plus a 6-digit code. verifyTotp always requires a password verified in the last 5 minutes.
4 Signup never verified the email, and Google/Apple login signed into any account matching the address, keeping the password whoever registered first had set. That person kept access to everything the real owner later added. claimUnverifiedUser removes logins added before the email was proven.
5 fetchByParam passed ?include= straight through, and user email, phone and roles had no readAccess. GET /1/uploads/<id>?include=owner returned the uploader's contact details without a login; upload ids are public in image URLs. Those fields are readable by self and staff roles only.
6 The products routes only called authenticate(), so any user could change or delete any product, or add one to someone else's shop. Writes require products.write.
7 PATCH /1/docs and POST /1/docs/generate needed no login and were mounted everywhere, writing arbitrary paths into openapi.json, which /openapi.json serves. Mounted in development only.

Two more came out of reviewing the fixes themselves:

  • Marking an email verified without clearing the older logins let a squatter's passkey and sessions survive a password reset and skip the cleanup above. Password reset and invite accept now run the same cleanup.
  • /otp/send defaulted to email whatever identifier named the account, so an sms login emailed the code and never set phoneVerified. The channel now follows the identifier, and /otp/login verifies the channel the code was actually sent to rather than the field used to look the user up. /otp/send, /otp/login and /totp/login now require exactly one of email or phone, which findUser previously resolved by silently preferring phone.

Behaviour changes worth knowing

  • Someone who signed up with a password and never verified their email loses that password the first time they sign in with Google, Apple, or an email code. They recover it with a password reset, which goes to the inbox they have just proven they own.
  • TOTP login now fails for an account with no password, since it always asks for a recent password.
  • Login codes issued before this deploys have no channel recorded, so they verify nothing. They expire within the hour.
  • A client sending both an email and a phone to the code login routes now gets a 400. Nothing in this repo does.

Not fixed here

Six lower-confidence findings were left out, including replayable invite tokens, admins being able to grant themselves superAdmin, and staging config with a hardcoded JWT_SECRET and admin password. Happy to open a follow-up.

- User roles and isTester are writable only by admin/superAdmin; any user
  could previously grant themselves superAdmin via PATCH /1/users/me.
- User email, phone, roles and isTester are readable only by self and
  staff roles, so ?include=owner no longer leaks them.
- Product create/update/delete require products.write.
- /1/docs (unauthenticated writes to openapi.json) is mounted in
  development only.
- /auth/password/update only accepts access tokens minted for
  reset-password; unsubscribe-link tokens could previously reset the
  password and return a full session.
- TOTP login always requires a password verified in the last 5 minutes;
  enableTotp never set isMfa, so TOTP alone was a full login.
- Google/Apple login into an account whose email was never verified
  clears its existing authenticators and sessions, so a password set by
  whoever signed up first with that email stops working. Google sign-ups
  are now marked emailVerified.
Both prove ownership of the mailbox, so Google/Apple login no longer
treats these accounts as unverified and clears their credentials.
Marking the email verified without clearing credentials let a squatter's
passkey and sessions survive a reset and skip the later OAuth cleanup.
OTP login marked emailVerified based on the lookup field, so a code sent
to an attacker's phone could verify a squatted email and exempt it from
claimUnverifiedUser. Non-MFA email OTP login now also clears logins
added before verification, like password reset.
Both fields were optional and findUser silently preferred phone, so a
request could name one account and be resolved against another.
/otp/send defaulted to email even when the request named a phone, so an
sms login emailed the code and never verified the phone. Also fixes test
expectations: write-access violations return 401, and query params go in
the request helper.
# Conflicts:
#	services/api/src/routes/auth/apple.js
#	services/api/src/routes/auth/google.js
#	services/api/src/routes/auth/otp.js
#	services/api/src/routes/auth/password.js
#	services/api/src/routes/auth/totp.js
#	services/api/src/routes/auth/utils.js
#	services/api/src/routes/index.js
#	services/api/src/routes/invites.js
#	services/api/src/routes/products.js
#	services/api/src/routes/products.test.js
#	services/api/src/utils/auth/login.js
Reflects the code login schemas and the user model scope change.
@github-actions

Copy link
Copy Markdown

API Changes

Operations

  • Changed POST /1/auth/otp/send
    • Removed body.channel.default
  • Changed POST /1/products
    • Added x-permissions
  • Changed PATCH /1/products/:id
    • Added x-permissions
  • Changed DELETE /1/products/:id
    • Added x-permissions

@kaareal

kaareal commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator Author

Closing in favour of smaller PRs, one per fix:

Together they are identical to this PR's API source changes.

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