Commit c69a270
authored
fix team invitation email check + verification code TOCTOU (#1365)
## Summary
Two authorization fixes in the backend. Both are pre-existing in `dev`
and were found during a security audit of `apps/backend/src`.
### 1. Team invitation accept — email not validated
[`team-invitations/accept/verification-code-handler.tsx`](https://github.com/stack-auth/stack-auth/blob/dev/apps/backend/src/app/api/latest/team-invitations/accept/verification-code-handler.tsx)
destructured the invited email as `{}` and only used `data.team_id` +
the accepting `user`. Any signed-in user in the tenancy who possessed
the 45-char code could join the team as themselves — the invitation was
not actually bound to the email it was addressed to.
**Attack scenarios that work without this fix**
- Forwarded invitation email (shared inbox, assistant inbox,
auto-forward rules).
- Screenshot of the invitation link pasted into Slack / Notion.
- Insider with server-access reading the email outbox (`GET
/api/latest/emails/outbox` returns rendered `html` +
`variables.teamInvitationLink`).
- Stale invite still sitting in spam after the invitee forwarded it
elsewhere.
**Fix.** The accept handler now requires that the accepting user owns
the invited email as a *verified* contact channel on their account.
Matches the invariant already used by the "list invitations for me"
endpoint
([`team-invitations/crud.tsx:41-66`](https://github.com/stack-auth/stack-auth/blob/dev/apps/backend/src/app/api/latest/team-invitations/crud.tsx#L41-L66)).
Rejections return a new `TEAM_INVITATION_EMAIL_MISMATCH` (403) error.
### 2. Verification-code handler TOCTOU
[`route-handlers/verification-code-handler.tsx`](https://github.com/stack-auth/stack-auth/blob/dev/apps/backend/src/route-handlers/verification-code-handler.tsx)
had a classic read-then-write TOCTOU:
```ts
const verificationCode = await prisma.verificationCode.findUnique(...);
if (verificationCode.usedAt) throw new KnownErrors.VerificationCodeAlreadyUsed();
// ... validation ...
await prisma.verificationCode.update({ data: { usedAt: new Date() } }); // unconditional
return await options.handler(...);
```
Five concurrent requests with the same code all pass the `if (usedAt)`
gate, all mark the code used, all run the post-handler. For OTP sign-in
the handler calls `createAuthTokens` which writes a fresh
`projectUserRefreshToken` row per call — so **one OTP → N refresh
tokens**. `auth/sessions/current` only revokes by `id: refreshTokenId`
and there is no bulk-revoke for passwordless users (only password change
in
[`users/crud.tsx:1210`](https://github.com/stack-auth/stack-auth/blob/dev/apps/backend/src/app/api/latest/users/crud.tsx#L1210)
does `deleteMany`). A phished OTP therefore becomes a
session-persistence primitive.
**Fix.** Replace the unconditional `update` with a conditional
`updateMany({ where: { …, usedAt: null } })` executed before
`options.handler`; if `count === 0` the race was already lost and we
throw `VERIFICATION_CODE_ALREADY_USED` (409). This also benefits MFA
sign-in and passkey sign-in, which share the same handler.
## Changes
| File | Change |
|---|---|
| `team-invitations/accept/verification-code-handler.tsx` | Require
verified contact channel matching `method.email` |
| `route-handlers/verification-code-handler.tsx` | Atomic `updateMany`
claim gated on `usedAt: null` |
| `stack-shared/src/known-errors.tsx` | New
`TeamInvitationEmailMismatch` (403) |
| `e2e/.../team-invitations.test.ts` | Two new tests (mismatch + happy
path) |
| `e2e/.../auth/otp/sign-in.test.ts` | One new test: 5 parallel
redemptions of one OTP → 1× 200 + 4× 409 |
## Test plan
- [x] `pnpm test run
apps/e2e/tests/backend/endpoints/api/v1/team-invitations.test.ts` —
27/27 pass
- [x] `pnpm test run
apps/e2e/tests/backend/endpoints/api/v1/auth/otp/sign-in.test.ts` —
12/12 (+ 4 pre-existing `it.todo`)
- [x] `pnpm test run
apps/e2e/tests/backend/endpoints/api/v1/auth/password` — 33/33 (+ 7
pre-existing todos)
- [x] `pnpm test run
apps/e2e/tests/backend/endpoints/api/v1/contact-channels` — 24/24
- [x] `pnpm test run
apps/e2e/tests/backend/endpoints/api/v1/auth/passkey
apps/e2e/tests/backend/endpoints/api/v1/auth/mfa` — 16/16
- [x] `pnpm --filter @stackframe/backend typecheck` — clean
- [x] `pnpm --filter @stackframe/backend lint` + `pnpm --filter
@stackframe/stack-shared lint` — clean
## Notes
- The broader "plaintext credentials in DB + Sentry logs every header"
finding from the same audit is **not** in this PR — a scrubber for
`Sentry.setContext` request headers + unit tests is prepared on a local
stash and will go out as a separate PR.
- The team-invitation fix does not require any config change; fresh
signups via the OTP / password flows that set `primary_email_verified:
true` during creation already land the user with a verified channel
matching the invited email, so the happy path is unaffected.
### Follow-up review (Codex)
Addressed in follow-up commit `954cddb`:
- **Finding 1 (High)**: mismatched invite acceptance was consuming the
invitation before rejecting. Moved the email-ownership check into the
pre-claim `options.validate` hook so a wrong-email attempt leaves
`usedAt` untouched and the real recipient can still redeem. New test
asserts this end-to-end.
- **Finding 3 (Medium)**: invitation stored `body.email` raw but contact
channels are stored via `normalizeEmail`, so case-varied invites (e.g.
`Alice@Example.com`) wouldn't match a `alice@example.com` channel.
`send-code` now normalizes on storage and `accept` normalizes on compare
for back-compat with already-issued invites. New test covers the
mixed-case path.
- **Finding 2 (partial)**: added `expiresAt > now` to the atomic claim
predicate for the boundary case where a code expires between the read
and the claim. The reviewer's broader point about the `attemptCount`
rate-limit check being non-atomic with its own increment **pre-dates
this PR** (it reads the in-memory `verificationCode.attemptCount` from
line 150, not a fresh read) and exists independently of the `usedAt`
TOCTOU I'm fixing here. Tracking that as a separate follow-up so this PR
stays scoped to the two originally-flagged issues.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Invite acceptance now requires the invitee’s verified, normalized
(case‑insensitive) email; mismatches return HTTP 403
(TEAM_INVITATION_EMAIL_MISMATCH).
* Client APIs now surface the new email-mismatch error alongside
verification errors.
* **Bug Fixes**
* OTP verification codes are now guarded against parallel double‑redeem
so only one request succeeds.
* **Tests**
* Added E2E tests for invitation email validation, non‑consuming
rejection, case‑insensitive matching, and OTP concurrency.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->1 parent 9f79bfb commit c69a270
10 files changed
Lines changed: 264 additions & 18 deletions
File tree
- apps
- backend/src
- app/api/latest/team-invitations
- accept
- send-code
- route-handlers
- e2e/tests/backend/endpoints/api/v1
- auth/otp
- payments
- packages
- template/src/lib/stack-app/apps
- implementations
- interfaces
Lines changed: 29 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | | - | |
| 2 | + | |
3 | 3 | | |
4 | 4 | | |
5 | 5 | | |
| |||
68 | 68 | | |
69 | 69 | | |
70 | 70 | | |
71 | | - | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
72 | 99 | | |
73 | 100 | | |
74 | 101 | | |
| |||
Lines changed: 4 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
1 | 2 | | |
2 | 3 | | |
3 | 4 | | |
| |||
48 | 49 | | |
49 | 50 | | |
50 | 51 | | |
| 52 | + | |
| 53 | + | |
51 | 54 | | |
52 | 55 | | |
53 | 56 | | |
54 | 57 | | |
55 | 58 | | |
56 | 59 | | |
57 | | - | |
| 60 | + | |
58 | 61 | | |
59 | 62 | | |
60 | 63 | | |
| |||
Lines changed: 10 additions & 6 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
190 | 190 | | |
191 | 191 | | |
192 | 192 | | |
193 | | - | |
| 193 | + | |
| 194 | + | |
194 | 195 | | |
195 | | - | |
196 | | - | |
197 | | - | |
198 | | - | |
199 | | - | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
200 | 199 | | |
| 200 | + | |
| 201 | + | |
201 | 202 | | |
202 | 203 | | |
203 | 204 | | |
204 | 205 | | |
205 | 206 | | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
206 | 210 | | |
207 | 211 | | |
208 | 212 | | |
| |||
Lines changed: 27 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
382 | 382 | | |
383 | 383 | | |
384 | 384 | | |
| 385 | + | |
| 386 | + | |
| 387 | + | |
| 388 | + | |
| 389 | + | |
| 390 | + | |
| 391 | + | |
| 392 | + | |
| 393 | + | |
| 394 | + | |
| 395 | + | |
| 396 | + | |
| 397 | + | |
| 398 | + | |
| 399 | + | |
| 400 | + | |
| 401 | + | |
| 402 | + | |
| 403 | + | |
| 404 | + | |
| 405 | + | |
| 406 | + | |
| 407 | + | |
| 408 | + | |
| 409 | + | |
| 410 | + | |
| 411 | + | |
385 | 412 | | |
386 | 413 | | |
387 | 414 | | |
| |||
Lines changed: 4 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
386 | 386 | | |
387 | 387 | | |
388 | 388 | | |
389 | | - | |
| 389 | + | |
| 390 | + | |
| 391 | + | |
| 392 | + | |
390 | 393 | | |
391 | 394 | | |
392 | 395 | | |
| |||
Lines changed: 171 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1140 | 1140 | | |
1141 | 1141 | | |
1142 | 1142 | | |
| 1143 | + | |
| 1144 | + | |
| 1145 | + | |
| 1146 | + | |
| 1147 | + | |
| 1148 | + | |
| 1149 | + | |
| 1150 | + | |
| 1151 | + | |
| 1152 | + | |
| 1153 | + | |
| 1154 | + | |
| 1155 | + | |
| 1156 | + | |
| 1157 | + | |
| 1158 | + | |
| 1159 | + | |
| 1160 | + | |
| 1161 | + | |
| 1162 | + | |
| 1163 | + | |
| 1164 | + | |
| 1165 | + | |
| 1166 | + | |
| 1167 | + | |
| 1168 | + | |
| 1169 | + | |
| 1170 | + | |
| 1171 | + | |
| 1172 | + | |
| 1173 | + | |
| 1174 | + | |
| 1175 | + | |
| 1176 | + | |
| 1177 | + | |
| 1178 | + | |
| 1179 | + | |
| 1180 | + | |
| 1181 | + | |
| 1182 | + | |
| 1183 | + | |
| 1184 | + | |
| 1185 | + | |
| 1186 | + | |
| 1187 | + | |
| 1188 | + | |
| 1189 | + | |
| 1190 | + | |
| 1191 | + | |
| 1192 | + | |
| 1193 | + | |
| 1194 | + | |
| 1195 | + | |
| 1196 | + | |
| 1197 | + | |
| 1198 | + | |
| 1199 | + | |
| 1200 | + | |
| 1201 | + | |
| 1202 | + | |
| 1203 | + | |
| 1204 | + | |
| 1205 | + | |
| 1206 | + | |
| 1207 | + | |
| 1208 | + | |
| 1209 | + | |
| 1210 | + | |
| 1211 | + | |
| 1212 | + | |
| 1213 | + | |
| 1214 | + | |
| 1215 | + | |
| 1216 | + | |
| 1217 | + | |
| 1218 | + | |
| 1219 | + | |
| 1220 | + | |
| 1221 | + | |
| 1222 | + | |
| 1223 | + | |
| 1224 | + | |
| 1225 | + | |
| 1226 | + | |
| 1227 | + | |
| 1228 | + | |
| 1229 | + | |
| 1230 | + | |
| 1231 | + | |
| 1232 | + | |
| 1233 | + | |
| 1234 | + | |
| 1235 | + | |
| 1236 | + | |
| 1237 | + | |
| 1238 | + | |
| 1239 | + | |
| 1240 | + | |
| 1241 | + | |
| 1242 | + | |
| 1243 | + | |
| 1244 | + | |
| 1245 | + | |
| 1246 | + | |
| 1247 | + | |
| 1248 | + | |
| 1249 | + | |
| 1250 | + | |
| 1251 | + | |
| 1252 | + | |
| 1253 | + | |
| 1254 | + | |
| 1255 | + | |
| 1256 | + | |
| 1257 | + | |
| 1258 | + | |
| 1259 | + | |
| 1260 | + | |
| 1261 | + | |
| 1262 | + | |
| 1263 | + | |
| 1264 | + | |
| 1265 | + | |
| 1266 | + | |
| 1267 | + | |
| 1268 | + | |
| 1269 | + | |
| 1270 | + | |
| 1271 | + | |
| 1272 | + | |
| 1273 | + | |
| 1274 | + | |
| 1275 | + | |
| 1276 | + | |
| 1277 | + | |
| 1278 | + | |
| 1279 | + | |
| 1280 | + | |
| 1281 | + | |
| 1282 | + | |
| 1283 | + | |
| 1284 | + | |
| 1285 | + | |
| 1286 | + | |
| 1287 | + | |
| 1288 | + | |
| 1289 | + | |
| 1290 | + | |
| 1291 | + | |
| 1292 | + | |
| 1293 | + | |
| 1294 | + | |
| 1295 | + | |
| 1296 | + | |
| 1297 | + | |
| 1298 | + | |
| 1299 | + | |
| 1300 | + | |
| 1301 | + | |
| 1302 | + | |
| 1303 | + | |
| 1304 | + | |
| 1305 | + | |
| 1306 | + | |
| 1307 | + | |
| 1308 | + | |
| 1309 | + | |
| 1310 | + | |
| 1311 | + | |
| 1312 | + | |
| 1313 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1080 | 1080 | | |
1081 | 1081 | | |
1082 | 1082 | | |
1083 | | - | |
| 1083 | + | |
1084 | 1084 | | |
1085 | 1085 | | |
1086 | 1086 | | |
| |||
1097 | 1097 | | |
1098 | 1098 | | |
1099 | 1099 | | |
1100 | | - | |
| 1100 | + | |
1101 | 1101 | | |
1102 | 1102 | | |
1103 | 1103 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1132 | 1132 | | |
1133 | 1133 | | |
1134 | 1134 | | |
| 1135 | + | |
| 1136 | + | |
| 1137 | + | |
| 1138 | + | |
| 1139 | + | |
| 1140 | + | |
| 1141 | + | |
| 1142 | + | |
| 1143 | + | |
| 1144 | + | |
1135 | 1145 | | |
1136 | 1146 | | |
1137 | 1147 | | |
| |||
1942 | 1952 | | |
1943 | 1953 | | |
1944 | 1954 | | |
| 1955 | + | |
1945 | 1956 | | |
1946 | 1957 | | |
1947 | 1958 | | |
| |||
Lines changed: 3 additions & 3 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2540 | 2540 | | |
2541 | 2541 | | |
2542 | 2542 | | |
2543 | | - | |
| 2543 | + | |
2544 | 2544 | | |
2545 | 2545 | | |
2546 | 2546 | | |
2547 | 2547 | | |
2548 | 2548 | | |
2549 | 2549 | | |
2550 | 2550 | | |
2551 | | - | |
| 2551 | + | |
2552 | 2552 | | |
2553 | 2553 | | |
2554 | 2554 | | |
| |||
2562 | 2562 | | |
2563 | 2563 | | |
2564 | 2564 | | |
2565 | | - | |
| 2565 | + | |
2566 | 2566 | | |
2567 | 2567 | | |
2568 | 2568 | | |
| |||
Lines changed: 3 additions & 3 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
73 | 73 | | |
74 | 74 | | |
75 | 75 | | |
76 | | - | |
77 | | - | |
78 | | - | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
79 | 79 | | |
80 | 80 | | |
81 | 81 | | |
| |||
0 commit comments