fix: reject OTP generation when form is not public#9783
Conversation
The otp/generate endpoint issued OTPs (and sent the SMS/email) for forms that were closed or archived, since the handler only checked that the form exists. This let anyone trigger OTP sends against a closed form. Gate _handleGenerateOtp on FormService.isFormPublic, mirroring the feedback and issue controllers: private forms now map to 404 and archived forms to 410 in the verification route error mapper. Safe for MRF step-2+ respondents since submissions already require the form to be public via ensurePublicForm. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The route-level specs created fixture forms with the default Private status, so every otp/generate call now 404s under the new public-form check. Make the verifiable fixture form Public and add a route-level test for the 404-on-private behaviour. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| 'This form is no longer active, so OTPs can no longer be requested.', | ||
| statusCode: StatusCodes.NOT_FOUND, | ||
| } | ||
| case FormDeletedError: |
There was a problem hiding this comment.
issue(minor): it seems that the message for FormDeletedError is form is no longer active.
export class FormDeletedError extends ApplicationError { constructor(message = 'This form is no longer active') { super(message, undefined, ErrorCodes.FORM_DELETED) } }
should the error here be This form is no longer active, so OTPs cannot be requested instead? or shall we just use the default core error message?
| errorMessage: coreErrorMsg, | ||
| statusCode: StatusCodes.NOT_FOUND, | ||
| } | ||
| case PrivateFormError: |
There was a problem hiding this comment.
issue(minor): should we perhaps update the copy here to be consistent with other mappers?
suggestion: This form has been taken down, so OTPs cannot be requested.
other mappers:
case PrivateFormError: return { statusCode: StatusCodes.NOT_FOUND, errorMessage: 'This form has been taken down. For assistance, please contact the person who asked you to fill in this form.', }
kevin9foong
left a comment
There was a problem hiding this comment.
LGTM! Thanks for discovering this and it is quite scary that this was allowed previously haha. Just a few minor comments about the error messaging to use consistent terms with the rest of the app. Let's release this once those messages are updated!
|
Tick the box to add this pull request to the merge queue (same as
|
c7dd603 to
60c26ff
Compare
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem
The public
POST /api/v3/forms/:formId/fieldverifications/:transactionId/fields/:fieldId/otp/generateendpoint only checked that the form exists, not that it is public. Anyone with a form ID and a live transaction could keep requesting OTPs against a closed or archived form and get a 201 back — triggering real SMS and email sends for a form that can no longer accept submissions. This is both an abuse vector (unsolicited OTP sends, SMS cost) and inconsistent with the rest of the public API, which rejects non-public forms.Solution
Gate
_handleGenerateOtponFormService.isFormPublicimmediately after the form is retrieved, before any auth verification or OTP generation/sending. This mirrors the existing pattern in the feedback and issue controllers.Breaking Changes
Tests
Unit tests:
pnpm test src/app/modules/verification/__tests__/verification.controller.spec.tsinapps/backend(91 passing, includes new 404/410 cases).TC1: OTP generation blocked on a closed form
POST /api/v3/forms/:formId/fieldverifications/:transactionId/fields/:fieldId/otp/generatewith a valid{"answer": "<email>"}bodyTC2: OTP generation still works on a public form (regression)
TC3 MRF step 2 respondent on an open form (regression)
🤖 Generated with Claude Code