Skip to content

[MOL-22741][SX] Hardening pass 2 — main (v3-alpha) - #762

Open
shengxi-gt wants to merge 14 commits into
LifeSG:mainfrom
shengxi-gt:MOL-22741-main
Open

shengxi-gt wants to merge 14 commits into
LifeSG:mainfrom
shengxi-gt:MOL-22741-main

Conversation

@shengxi-gt

@shengxi-gt shengxi-gt commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing apis or functionality to change)
  • Documentation (change to documentation, comments or API descriptions)
  • Tests (improvements to unit tests or E2E tests)
  • Other (technical improvements, refactoring, or changes that don't fall into the above categories)

Description of changes

Commit Area Behaviour change
StyleHelper: allowlist layout declarations and resolve CSS escapes locationModalStyles, imageReviewModalStyles Yes, see below
e2e: restrict CDN proxy to asset paths and stop following redirects e2e Next.js app only e2e only
FileUpload, ESignature: only fetch prefilled http(s) fileUrl prefilled fileUrl Yes, see below
FileHelper: restrict sanitizeFileName to safe characters and strip traversal uploaded file names Yes, see below
RegexHelper: clarify length cap only bounds polynomial backtracking comments only No
FileHelper: document getType text fallback as a client-side check comments only No
Lint: format files left unformatted on main formatting only No
e2e: assert custom modal styles keep layout and strip disallowed declarations e2e tests, snapshots, review modal box test id No

⚠️ Behaviour changes

  1. locationModalStyles / imageReviewModalStyles — the previous sanitiser only stripped the literal text url( and
    @import, which CSS escapes (e.g. \75rl() and non-url() resource functions such as image-set() get past. It now
    keeps only layout declarations (padding*, margin*, width/height and their min-/max- variants,
    top/right/bottom/left, border-radius) after resolving CSS escapes. Anything else — other properties,
    @media and other rules, resource functions — is dropped with a console.warn. On main these values go through
    style.cssText, which never applied @media anyway. The sanitiser does not touch the DOM so it can be reused where
    styles are built during server-side rendering.
  2. Prefilled fileUrl (FileUpload and ESignature) — only http(s) urls (including relative urls) are fetched.
    Other schemes (javascript:, data:, file:, blob:) are not fetched and the field shows its existing error
    state ("Upload failed. Please try again." / "Failed to load."). ESignature fetches with credentials, so it is
    covered too. Restricting which hosts can be reached is left to the consuming app's CSP connect-src, as agreed in
    Hardening pass: input validation, origin checks, and default-safe field behaviour — main (v3-alpha) #739; the field docs now say so.
  3. Uploaded file names — sanitizeFileName now keeps letters, digits, space, _ - ( ) and inner dots; extensions
    keep letters and digits. Path separators, traversal sequences and markup/reserved characters are removed, repeated
    dots are collapsed and leading/trailing dots trimmed. Ordinary names (my photo (1).jpg, report.v2.final.pdf) and
    dotfiles without an extension (.env) are unchanged. Names with ! @ # $ % ^ & * ~ ' , ; = + [ ] { } lose those
    characters. This remains a client-side normalisation, not a security boundary: servers should store uploads under a
    generated id and escape the name wherever it is displayed (noted in the JSDoc).

Notes for reviewers

  • Regex length cap (ReDoS): schema regexes still run on the browser's native backtracking engine. The existing
    1000-character cap only bounds polynomial backtracking; nested quantifiers such as (a+)+$ can still backtrack
    exponentially well under it. Comments now say this, matching validation-schema-generator (MOL-22744): schema
    patterns must be trusted and avoid nested or overlapping quantifiers. Rejecting "risky" patterns at runtime was
    evaluated again and not adopted — it flags common safe patterns and makes those fields fail every value.
  • Possible follow-up: if ReDoS needs to be closed fully, schema regexes could move to a linear-time (RE2-style)
    engine — e.g. re2js (~61 KB gzipped, single maintainer, would need pinning/vendoring), or a small RE2-style package
    we maintain ourselves and share with validation-schema-generator if a third-party dependency isn't acceptable. Either
    drops lookaround/backreference support and should be adopted in both libraries together.
  • getType: text formats have no magic number, so their type falls back to the browser-declared mime type and
    the file name. Documented as a client-side convenience check rather than changed.
  • e2e CDN proxy: the segment allowlist includes @ for retina assets such as 400@2x.png.

Checklist

  • Changes follow the project guidelines in CONTRIBUTING.md
  • Looks good on mobile and tablet
  • Updated documentation
  • Added/updated unit tests

Test plan

  • Full Jest suite + eslint + tsc --noEmit pass on this branch.
  • New tests fail against the previous implementation (verified for the sanitiser, fileUrl and file-name changes).
  • Playwright e2e: full run 184/187 before the e2e commit; the 2 custom-styles snapshot diffs were expected (red
    background now stripped) and are updated with new assertions; the third (Location Field › Warning,
    route.fulfill: Fetch response has been disposed) is a timing issue in the test's CDN route helper.

Screenshots

N/A — no visual changes to default rendering.

Comment thread e2e/nextjs-app/app/cdn/[...path]/route.ts
Comment thread e2e/tests/components/fields/image-upload/image-upload.e2e.spec.ts Outdated
Comment thread src/utils/file-helper.ts Outdated
Comment thread src/components/fields/e-signature-field/e-signature-field.tsx
Comment thread src/stories/3-fields/file-upload/file-upload.stories.tsx Outdated
Comment thread src/__tests__/utils/style-helper.spec.ts
@qroll qroll added the type: bug Something isn't working label Oct 15, 2026
@qroll qroll added this to the v3.0.0-alpha.2 milestone Oct 15, 2026
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.

2 participants