feat(mrf): step-token write-guard for MRF next-step submissions (S3)#9758
feat(mrf): step-token write-guard for MRF next-step submissions (S3)#9758kevin9foong wants to merge 7 commits into
Conversation
| (doc as any).__v, | ||
| ['submittedSteps'], | ||
| ) | ||
| const saveSpy = jest |
There was a problem hiding this comment.
The 409 path fires on a Mongoose VersionError from save(). Reproducing a genuine optimistic-concurrency race (two interleaved load+save cycles) is timing-dependent and flaky, so we mock save to reject once with a real VersionError — a boundary mock (the DB write) that deterministically exercises exactly the code we own: the VersionError → DatabaseConflictError conversion, plus an assert that mapRouteError yields 409.
843d48f to
609de4e
Compare
1e47a3a to
546b076
Compare
| ogpSuiteSso: 'ogp-suite-sso' as const, | ||
| enableIntranetSgidLogin: 'enable-intranet-sgid-login' as const, | ||
| enableMrfWebhooks: 'enable-mrf-webhooks' as const, | ||
| mrfStepWriteToken: 'mrf-step-write-token' as const, |
There was a problem hiding this comment.
note for reviewer: based on rollout strategy in #9740, we must wait for S11 where plumber is migrated over to use the step token to provide submission link before enabling this flag.
| // MRF step-token write-guard (S3). Wrapped under the form public key, so it | ||
| // is safe to expose to a form-secret-key holder (the admin) who unwraps it to | ||
| // resend reminders. Mirrors encryptedSubmissionSecretKey; never in a webhook. | ||
| encryptedStepToken?: string |
There was a problem hiding this comment.
note to reviewer: this means the admin can also progress the next step, since they have a way to receive
- encryptedStepToken + have formSecretKey.
This is the same as existing (where they have the encrypted submission secret key which can do both) and no regression. Out of scope.
0c6ea1b to
696ee91
Compare
| // magic-link `&token=` param, echoed as `stepToken` in the next-step PUT | ||
| // body. Undefined (tokenless / flag-off link) is dropped by JSON.stringify, | ||
| // keeping the body byte-identical to today. | ||
| stepToken?: string |
There was a problem hiding this comment.
note to reviewer: this is the same mechanism as submissionSecretKey which is auto put into the formData and only included for update submission (ie, step >=2)
| // mrf-step-write-token flag is on. stepToken is the RAW token (threaded into | ||
| // the next respondent's magic link, never persisted); the hash and wrapped | ||
| // copy are persisted on the row. | ||
| stepToken?: string |
There was a problem hiding this comment.
note to reviewer: safe to add it here since its the internals, following the pattern of submissionSecretKey, to handoff from encryptSubmission → MRF create/update service.
This is different from PublicMultirespondentSubmissionDto which is the public facing DTO which does not include these: stepTokenHash and encryptedStepToken
| | 'workflowStep' | ||
| | 'mrfVersion' | ||
| | 'submittedSteps' | ||
| | 'encryptedStepToken' |
There was a problem hiding this comment.
question: should we omit this for non-reminder flow? eg, for individual response decrypting?
edit: no - we do not need to since doing so would:
- require re-work of frontend reminder sending flow and introduce new endpoints, out of scope for now
- the previous behavior already exposes the submissionSecretKey which allows admin to advance next step. hence no regression is introduced and this can be tackled next time.
However, we opt to apply POLP (principle of least privilege) to strip encryptedStepToken from public DTO as it is cheap to do so.
19453fc to
0f4091e
Compare
| ogpSuiteSso: 'ogp-suite-sso' as const, | ||
| enableIntranetSgidLogin: 'enable-intranet-sgid-login' as const, | ||
| enableMrfWebhooks: 'enable-mrf-webhooks' as const, | ||
| mrfStepWriteToken: 'mrf-step-write-token' as const, |
There was a problem hiding this comment.
note for reviewer: what happens when the feature flag is turned on then off then on again?
will advancements work at each stage?
There was a problem hiding this comment.
The presence of the stepToken determines if the magic link includes the step token - this is ideal so that if the flag is turned off and on again, no email needs to be resent once the validation of the step token is toggled on again.
Including a step token even when the validation is off is safe, since it simply ignores it.
PR #9758 added the per-step write-guard token but left admin-side magic-link regeneration (reminders and the copy-response link) unable to recover the raw token, so regenerated links fail the guard when the flag is on. Record the decisions taken before implementation: place wrap/unwrap in the SDK (a shared home for FormSG frontend/backend and Plumber v4), keep hash/verify backend-only, and keep the flag-off path byte-identical. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| ? decrypted.responses | ||
| : adaptV3ToV4(decrypted.responses, { formFields }) | ||
|
|
||
| let stepToken: string | undefined = undefined |
There was a problem hiding this comment.
note to reviewer:
- this will be used by plumber and clients (eg, frontend) to decrypt the
encryptedStepToken.
note that no encrypt method is exposed in the sdk for now as no client needs to encrypt.
b829747 to
e83e5e3
Compare
8d971f5 to
18ac15c
Compare
Introduce a pure, I/O-free step-token module: generate (256-bit CSPRNG, base64url), hash (unsalted sha256 hex for server verification), verify (constant-time, length-guarded), and wrap (nacl sealed box to the form public key, mirroring encryptedSubmissionSecretKey so an admin can recover the raw token for reminders). This is the deep primitive behind the MRF step-token write-guard, which closes the workflow-forge hole where a form-secret-key holder could pass the decrypt-gate and advance a workflow without authorisation. Ref #9743
Add the two step-token fields to MultirespondentSubmissionSchema and the shared Zod base (which the backend interface extends), plus a new mrf-step-write-token feature flag (distinct from enable-mrf-webhooks, which stays scoped to generic delivery). Both fields are optional: legacy in-flight rows carry neither and pass their next step on the decrypt-gate alone (migration grace). The raw token is never a schema path. Ref #9743
1418a29 to
46e25af
Compare
validateMultirespondentSubmission now requires, IN ADDITION to the existing decrypt-gate, a step token whose sha256 matches the pending step's stepTokenHash. Absent/mismatched token when the row carries a hash -> StepTokenVerificationError (403, non-retryable, business-friendly message). Legacy rows with no hash pass on the decrypt-gate alone (migration grace). A lost concurrent-write race is mapped to a 409 (DatabaseConflictError) instead of a 5xx default. Ref #9743
Mint a fresh token for the next step in the same block that generates the per-step keypair: raw token for the magic link, sha256 hash + form-public-key-wrapped copy for the row. Create/update persistence stores stepTokenHash and encryptedStepToken; update rotates them on every advance, which upgrades a legacy in-flight row (no hash) so all subsequent steps are gated. getMultirespondentSubmissionEditPath now carries the raw token as `&token=` on the create, update, and reminder paths. The reminder controller resends the SAME token (no rotation) supplied by the admin, keeping the original link valid. Ref #9743
Once mrf-step-write-token is on, the backend write-guard rejects a next-step PUT whose step token does not match the pending step's hash, so the respondent's browser must supply it. Read `&token=` from the magic link exactly as `?key=` is read today, and thread it as `stepToken` through updateMultirespondentSubmission into the PUT body. A tokenless (flag-off) link leaves the token empty, so JSON.stringify drops `stepToken` and the body stays byte-identical to today. Ref #9743
The admin unwraps encryptedStepToken (exposed on the admin submission DTO for this purpose, but never shipped in a webhook payload) and posts it back as stepToken so the reminder link resends the same, unrotated token — the original link stays valid. Ref #9743
46e25af to
ffe0223
Compare
Problem
MRF workflow steps are only decrypt-gated today: the next-step submission
PUTsucceeds for anyone who can decrypt the step with the submission secret key. A webhook consumer holds the form secret key, so it can unwrap the submission secret key, pass the decrypt-gate, and forge or hijack a workflow advance. We need a per-step credential the server verifies on thePUT— one the server itself cannot mint or recover — to close that hole.Closes #9743. Part of PRD #9740 (MRF v4 Webhooks — Option A).
Solution
A per-step, full-entropy step token is minted alongside each step's submission keypair and required on the next-step
PUT, in addition to (never replacing) the existing decrypt-gate.sequenceDiagram participant R1 as Respondent (step N) participant FE as Frontend participant BE as Backend participant R2 as Respondent (step N+1) R1->>FE: Submit step N FE->>BE: PUT (decrypt-gate keypair) Note over BE: encryptSubmission mints for step N+1:<br/>rawStepToken = generate()<br/>stepTokenHash = hash(rawStepToken)<br/>encryptedStepToken = wrap(rawStepToken, formPublicKey) BE-->>FE: Row saved with stepTokenHash + encryptedStepToken BE->>R2: Magic link ?key=...&token=rawStepToken R2->>FE: Open link, fill step N+1 FE->>BE: PUT { ...body, stepToken: rawStepToken } Note over BE: validateMultirespondentSubmission:<br/>decrypt-gate check (existing)<br/>+ verify(stepToken, mrfSubmission.stepTokenHash) alt token valid or row has no stepTokenHash (legacy) BE-->>FE: 200 — step advances else token missing/mismatched BE-->>FE: 403 StepTokenVerificationError else concurrent write lost the race BE-->>FE: 409 DatabaseConflictError endBackend
step-token.ts) — a pure, I/O-free module:generate(256-bit base64url CSPRNG),hash(plain sha256, no salt — full entropy needs none),verify(timing-safe),wrap(encrypts to the form public key using the exactencryptedSubmissionSecretKeyscheme).stepTokenHashlets the server verify thePUT;encryptedStepToken(wrapped under the form public key) lets an admin recover the raw token to resend reminders. The server holds only the public key, so it can neither unwrap nor re-derive the token — that asymmetry is the security property, and is why the two fields cannot be collapsed into one.encryptSubmission(same block as the step keypair), threaded into the next respondent's magic link (&token=), and checked invalidateMultirespondentSubmission. Wrong/absent token when the row carries a hash →StepTokenVerificationError→ 403 (mapped fromSUBMISSION_MRF_STEP_TOKEN_INVALID, business-friendly message, non-retryable). A lost optimistic-concurrency race on the submitted-steps array → 409 viaDatabaseConflictError(never a 5xx default).encryptedStepTokenand posts it back asstepToken; the original link stays valid.encryptedStepTokenis exposed on the admin submission DTO for this, but is explicitly stripped from the public/webhook DTO to follow POLP.Frontend
The respondent's browser reads
&token=from the magic link exactly as it reads?key=today and echoes it asstepTokenon the next-stepPUT. A tokenless (flag-off) link omits the field, so the body is byte-identical to today. The reminder-resend flow on the admin side similarly readsencryptedStepTokenoff the submission and posts the recovered token when triggering a reminder.Feature flag & migration
All behind
mrf-step-write-token(new; distinct fromenable-mrf-webhooks, which stays scoped to generic delivery). Migration is grace-by-presence: the token is required only when the row already carriesstepTokenHash, so legacy in-flight (tokenless) links advance one step on the decrypt-gate alone and become gated on that advance (upgrade-on-advance, no backfill).flowchart LR A[Row created before flag<br/>no stepTokenHash] -->|submit step, decrypt-gate only| B[Advances<br/>row now gets stepTokenHash + encryptedStepToken] B -->|next submit requires matching stepToken| C{token valid?} C -->|yes| D[Advances] C -->|no / missing| E[403 StepTokenVerificationError]Alternatives considered
tweetnaclinstead of importing the SDK'sencryptMessage. That helper isn't publicly exported, and importing@opengovsg/formsg-sdkdrags in axios/webhooks — network I/O — which breaks the primitive's purity gate. The wrap/unwrap round-trip test proves byte-compatibility with the app's existingdecryptContent, so the 6-line inline mirror is safe.validateMultirespondentSubmissionrather than a new middleware. The write-guard is additive to the decrypt-gate, which already lives there with the loaded row andreq.bodyin scope and its ownmapRouteErrorwiring. A separate middleware would re-load the row and duplicate error handling for no benefit.Breaking Changes
No — backwards compatible. With
mrf-step-write-tokenoff, the respondent flow and all payloads are byte-identical to today; legacy tokenless links advance once on the decrypt-gate, then become gated. No backfill.Tests
Start with a 3 step workflow MRF form.
TC1: Happy-path advance with a token-bearing link
mrf-step-write-tokenon, submit a step; open the next respondent's magic link and confirm it carries both?key=and&token=.TC2: Forged advance is rejected
&token=value.PUTis rejected with 403 (not a 5xx), and the step does not advance.TC3: Reminder resends the same token
TC4: Migration grace (legacy in-flight link)
stepTokenHash), open its existing tokenless link and submit — it advances on the decrypt-gate alone.TC5: Flag-off parity
mrf-step-write-tokenoff, run a full MRF fill/advance — behaviour and links are exactly as today (no token required).