Skip to content

Hardening pass: input validation, origin checks, and default-safe field behaviour — main (v3-alpha) - #739

Merged
qroll merged 23 commits into
LifeSG:mainfrom
shengxi-gt:clean/MOL-22453-main
Sep 22, 2026
Merged

qroll merged 23 commits into
LifeSG:mainfrom
shengxi-gt:clean/MOL-22453-main

Conversation

@shengxi-gt

@shengxi-gt shengxi-gt commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Hardening pass — main (v3-alpha)

Covers several hardening changes and default-safe behaviour updates across fields and shared utilities. One commit — review the diff directly since the changes span multiple independent areas with varying blast radius.

⚠️ Breaking changes

Four changes alter default runtime behaviour unconditionally, no opt-out, for every existing schema as soon as this ships:

  1. Iframe postMessage origin check is now hard default-on. Messages from a mismatched origin are silently dropped.
  2. 1000-character cap on schema-authored regex input. Inputs longer than 1000 chars are treated as non-matching regardless of the actual pattern result. Affects matches, notMatches, and filenameMatches rules.
  3. MaskedField maxLength now defaults to 1000 whenever maskRegex is set and no max/length rule already narrows it.
  4. Pattern strings not wrapped in /pattern/flags slashes are no longer silently skipped. They previously landed in a catch block and validation was skipped entirely (value always passed). The shared parsing helper now falls back to treating the whole string as a raw pattern and applies it — any schema relying on the old silent no-op will now get real validation applied.

Narrower breaking changes (only affect a consumer that relied on the specific permissive behaviour being fixed):

  • sanitize-html configuration: stricter attribute allowlist may now strip attributes that previously passed through unsanitized.
  • ButtonField URL scheme check: links using a non-standard scheme (e.g. custom app deep-links) will now be blocked.
  • locationModalStyles / imageReviewModalStyles: url() and @import are now stripped from these CSS strings before they are applied to the modal box. Any background image or external resource passed via these props will silently stop working. Verified against all known consumers — none currently use these props with url() or @import.

Changes covered

  • CI pipeline configuration hardened
  • sanitize-html configuration tightened in Typography, FilterCheckbox, Popover
  • ButtonField link URLs now restricted to http/https/tel/mailto schemes
  • Iframe postMessage now validates origin by default
  • Location-field search query now escaped before use in RegExp
  • OTP state field documented as client-asserted (no code change)
  • Schema regex inputs now capped at 1000 characters
  • MaskedField maxLength defaults to 1000 when maskRegex is set
  • Shared regex parsing helper introduced; unwrapped patterns now evaluated instead of silently skipped
  • Custom modal style strings now strip @import and url() references
  • locationModalStyles and imageReviewModalStyles prop descriptions updated in Storybook
  • ImageUpload matches rule no longer blocks valid submissions (pre-existing bug fix)
  • Playwright e2e dev-server configuration hardened

Test plan

  • Full Jest suite + eslint + tsc --noEmit pass on this branch.
  • Each change was individually verified at the time it was written.

@shengxi-gt

shengxi-gt commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

jest just seems to hang indefinitely on this test case, this time check doesn't seem useful

You were right that the old version hung — the query was passed directly to new RegExp() without escaping. In this PR we added escapeRegExp(query) before building the RegExp (line 12 of helper.ts), so (a+)+$ becomes the literal pattern \(a\+\)\+\$. No backtracking; the test completes in microseconds. We also moved the spec file to the location-modal/location-search/ subdirectory as part of the reorganisation.

@shengxi-gt

shengxi-gt commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

would argue against this change, the value are provided by the client/server in the first place

The concern isn't distrust of the originating server — it's that between the server delivering the prefilled payload and the browser submitting the form, the data lives in the page where a user (or MITM proxy) can modify it. The frontend can verify files it fetches (fileUrl) or encodes itself (dataURL), but for a prefilled file with neither field it has no independent signal to validate mimeType/ext/fileSize against. Defaulting trustProvidedFileMetadata: false means: we don't silently present unverifiable metadata as trusted. Consumers that control their own pipeline and accept that trade-off can opt in with true.

Comment thread src/context-providers/yup/helper.ts
Comment thread src/components/fields/image-upload/image-upload.tsx
@qroll

qroll commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

would argue against this change, the value are provided by the client/server in the first place

The concern isn't distrust of the originating server — it's that between the server delivering the prefilled payload and the browser submitting the form, the data lives in the page where a user (or MITM proxy) can modify it. The frontend can verify files it fetches (fileUrl) or encodes itself (dataURL), but for a prefilled file with neither field it has no independent signal to validate mimeType/ext/fileSize against. Defaulting trustProvidedFileMetadata: false means: we don't silently present unverifiable metadata as trusted. Consumers that control their own pipeline and accept that trade-off can opt in with true.

trustProvidedFileMetadata: if we're looking at prefilled data flow, fileUrl and dataURL also come from the same "server", what makes uploadResponse less trustworhty?

allowedFileOrigins: and then back to trusting fileUrl, shouldn't CSP connect-src help with restricting what origins are callable?

@qroll

qroll commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

jest just seems to hang indefinitely on this test case, this time check doesn't seem useful

You were right that the old version hung — the query was passed directly to new RegExp() without escaping. In this PR we added escapeRegExp(query) before building the RegExp (line 12 of helper.ts), so (a+)+$ becomes the literal pattern \(a\+\)\+\$. No backtracking; the test completes in microseconds. We also moved the spec file to the location-modal/location-search/ subdirectory as part of the reorganisation.

sorry what I meant was that without the fix, the test would hang instead of timing out. how can we make jest recover from this?

@shengxi-gt

shengxi-gt commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

would argue against this change, the value are provided by the client/server in the first place

The concern isn't distrust of the originating server — it's that between the server delivering the prefilled payload and the browser submitting the form, the data lives in the page where a user (or MITM proxy) can modify it. The frontend can verify files it fetches (fileUrl) or encodes itself (dataURL), but for a prefilled file with neither field it has no independent signal to validate mimeType/ext/fileSize against. Defaulting trustProvidedFileMetadata: false means: we don't silently present unverifiable metadata as trusted. Consumers that control their own pipeline and accept that trade-off can opt in with true.

trustProvidedFileMetadata: if we're looking at prefilled data flow, fileUrl and dataURL also come from the same "server", what makes uploadResponse less trustworhty?

allowedFileOrigins: and then back to trusting fileUrl, shouldn't CSP connect-src help with restricting what origins are callable?

trustProvidedFileMetadata: Actually now that i think about it more, validating this should always be server-side anyways as we should always regard the browser as an untrusted environment. So this feature actually just increases friction without really achieving what it was intended for. Hence will remove it.

allowedFileOrigins: Point taken, will add a note to use CSP connect-src instead of building redundancy. Will remove to reduce upgrade friction.

@shengxi-gt

Copy link
Copy Markdown
Contributor Author

sorry what I meant was that without the fix, the test would hang instead of timing out. how can we make jest recover from this?

we could try using vm.runInNewContext in this context for the test?

Comment thread src/utils/style-helper.ts
weili-govtech
weili-govtech previously approved these changes Sep 21, 2026

@qroll qroll left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the unit tests are stuck when I run them locally. do you encounter this too?

Comment thread src/stories/3-fields/location-field/location-field.stories.tsx Outdated
@qroll qroll added this to the v3.0.0-alpha.2 milestone Sep 21, 2026
@qroll qroll added the type: bug Something isn't working label Sep 21, 2026
@qroll
qroll merged commit d133d6a into LifeSG:main Sep 22, 2026
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants