Hardening pass: input validation, origin checks, and default-safe field behaviour — main (v3-alpha) - #735
Closed
shengxi-gt wants to merge 26 commits into
Closed
Hardening pass: input validation, origin checks, and default-safe field behaviour — main (v3-alpha)#735shengxi-gt wants to merge 26 commits into
shengxi-gt wants to merge 26 commits into
Conversation
added 21 commits
September 16, 2026 16:57
…b-pipeline.yml
Route github.head_ref, github.event.pull_request.title, and
github.event.head_commit.message through env: instead of inline
${{ }} interpolation in the run: block. ${{ }} expansion happens
before bash executes the script, so an attacker-controlled PR title
or branch name was previously substituted as literal script text and
executed as code. Mirrors the safe pattern already used one step
later in the same file for GITLAB_TOKEN/GITLAB_PAT.
…e misuse in Typography allowedAttributes: false means "allow ALL attributes on all tags", not "allow none" — the opposite of the intended restriction. This let sanitized rich text carry arbitrary attributes (e.g. event handlers, style) straight through. Replace with an explicit allowlist built on sanitize-html's own defaults plus the specific img attributes this component actually needs.
…e misuse in FilterCheckbox Same allowedAttributes: false misconfiguration as asgard-0002 (means "allow all attributes", not "allow none"). Replace with sanitize-html's own default allowlist for this label-rendering sink.
…e misuse in Popover Same allowedAttributes: false misconfiguration as asgard-0002/0003 (means "allow all attributes", not "allow none"). Replace with an explicit allowlist built on sanitize-html's own defaults plus the specific img attributes this component's hint content needs.
…me allowlist isValidUrl only checked that the URL parsed, not its scheme — javascript:, data:, and vbscript: URLs all parse successfully and were treated as "valid", letting a schema-configured button link execute arbitrary script on click. Restrict to http:, https:, mailto:, and tel: — the only schemes this field is meant to support.
…me field (hard default-on) useIframeMessage accepted postMessage events from any origin, so any page/frame on the client could forge SYNC/RESIZE/CHANGE/VALIDATION/ LOADED events into the parent form. Derive the expected origin from the iframe's own configured src and reject any message whose event.origin doesn't match. event.origin is set by the browser from the actual sender and cannot be spoofed by script. Enforced unconditionally (no opt-in schema flag) — every consumer gets this protection by default.
…g into RegExp boldResultsWithQuery compiled the raw user-typed search query directly into a RegExp used to highlight matches. An unescaped query containing regex metacharacters could throw (invalid pattern) or, with a crafted pattern, cause catastrophic backtracking (ReDoS) against the address list being rendered. Escape regex metacharacters before compiling and fail safe (return the list unhighlighted) if construction still throws.
Picks up upstream fixes for known sanitize-html CVEs identified by npm audit. Declared range (^2.8.1 -> ^2.17.7) still allows future non-breaking patch/minor updates.
…ndency chain sanitize-html 2.17.x depends on htmlparser2 v12+, which (along with dom-serializer/domhandler/domutils/domelementtype/entities) ships as ESM-only. The existing transformIgnorePatterns only carved out @lifesg/react-design-system and leaflet, so every test that imports sanitize-html (directly or via the shared Sanitize component) fails to even parse. Extend the exception list so babel-jest transpiles this dependency chain like project source.
… exposure network_mode: "host" plus binding the Playwright automation server to 0.0.0.0 exposed it on every interface of the host's own network namespace, unauthenticated, to anything else on that machine or LAN (host networking bypasses Docker's usual port isolation entirely). Drop network_mode: "host" so the container gets its own isolated network namespace, and publish the server's port only on the host's loopback interface via `ports: ["127.0.0.1:3010:3010"]`. The server itself still binds 0.0.0.0 inside the container - that's required for Docker's own NAT to forward anything to it at all; the actual exposure boundary is the host-side publish, not the in-container bind. (An earlier version of this fix tried binding 127.0.0.1 inside the container instead, which seemed like it would be even more locked-down, but actually running the e2e pipeline showed it made the server completely unreachable even from the host - loopback traffic never leaves a container's own network namespace, regardless of Docker networking mode or port publishing. `docker port` and a `curl` from the host confirmed the loopback-only publish here preserves connectivity while keeping the LAN/host-namespace exposure closed.) playwright.config.ts only ever connects via ws://127.0.0.1:3010/, which this repo's actual CI scripts (e2e-ci.sh/e2e-setup.sh) run through directly - not a theoretical path, verified end-to-end.
…n origin allowlist Prefilling a file-upload field via a schema/API-supplied fileUrl made the browser fetch that URL directly, carrying the user's ambient session credentials (cookies, auth headers via AxiosApiClient). A schema or API response that can influence fileUrl (e.g. a compromised backend response, or a misconfigured prefill source) could point it at an internal/third-party endpoint and exfiltrate the authenticated response — a client-side SSRF/credential-leak vector. Add a required allowedFileOrigins schema field. fileUrl is only fetched if its origin is in the allowlist; if allowedFileOrigins is unset, the fetch is skipped entirely, same as if no fileUrl were provided at all. Fail closed, not open: there's no principled permissive default here — this codebase has no basis for guessing which origins are safe for a given consumer's file storage backend, so "don't fetch until told it's safe" is the only default that's actually safe. This is a real breaking change for any consumer currently relying on unconfigured fileUrl prefetching. A downstream check across the 18 in-scope repos (see MOL-22453-impact-report.md) found none currently depend on this via the library's FileUpload field — one repo (facilities-admin-web) has a superficially similar fileUrl-fetch pattern, but it's built on the raw @lifesg/react-design-system FileUpload component directly with its own custom fetch logic, not this library's schema-driven field, so it's unaffected. The 9 out-of-scope BSG/RBS repos were not checked and should be verified by their own teams before upgrading.
…ata by default When a prefilled file had neither a dataURL nor a fetchable fileUrl, injectFile fell back to validating (and reporting the size of) the file purely from the caller-supplied uploadResponse object - mimeType/ext/fileSize the browser never independently verified. A compromised or misconfigured prefill source could claim any type/size for a file, bypassing fileType/fileExtension/maxSizeInKb validation entirely for that upload. Add an optional trustProvidedFileMetadata schema flag. Left unset (new default), such files are flagged as unverified and rejected with a generic upload error instead of being silently validated against unverified metadata. Set to true to restore the previous behavior for schemas/backends that intentionally rely on it. This is a deliberate breaking change to the default behavior. Checked both local clones and the wider GitLab org for consumers relying on the old default (uploadResponse-only prefill, no fileUrl/dataURL) and found none. Should still be called out explicitly in release notes. Running the real e2e suite against this change caught exactly the kind of consumer it would affect: the file-upload "Thumbnail" demo prefills a file via uploadResponse only, with no dataURL/fileUrl, and rendered as a rejected/unverified card instead of the expected thumbnail once this default flipped. Opted that demo into trustProvidedFileMetadata: true, since showing a trusted-metadata thumbnail is what it's actually demonstrating - verified the "Thumbnail" e2e test passes again after this change.
…nt-asserted, not a security boundary No code change. IOtpVerificationValue.state/additionalData are set by this component once its own client-side verify-OTP API call succeeds, but the submitted form payload is just JSON a client fully controls. A consuming backend that trusts state === "verified" at face value without independently re-verifying the OTP transaction server-side is trivially bypassable. Documents the existing trust boundary so integrators don't assume the frontend enforces it.
…-authored regex in Yup "matches" The "matches" validation rule compiled a schema-authored /pattern/ string and passed it straight to Yup's .matches(), which runs regex.search() against arbitrary-length form field input with no bound. A catastrophic-backtracking pattern (an easy, common authoring mistake, e.g. /^(a+)+$/) hangs the tab once a user types input that triggers it — no schema-author compromise needed, just a vulnerable pattern plus ordinary form input. Introduce a shared RegexHelper (parseMatchesPattern/safeTestRegex) that skips the regex test entirely once input exceeds a safe length bound (500 chars), and switch "matches" to a custom .test() built on it instead of .matches() (which has no hook for this). This reduces worst-case backtracking cost for the common cases (form field values, filenames, short keystroke buffers) but does not make a genuinely pathological pattern safe against every input length — a stronger fix (linting schema-authored patterns for catastrophic-backtracking shapes) is a further-out improvement, not attempted here. The same utility is applied to three sibling sinks using the same /pattern/ flags parsing convention in the following commits.
…ReDoS bound notMatches parsed its /pattern/flags string with no try/catch at all — a malformed regex string threw a raw, unhandled TypeError from matches[1] (regex.match() returns null on no match), crashing validation on bad input, not even malicious input. It also had no protection against a catastrophic-backtracking pattern being tested against arbitrary-length form field values, the same ReDoS class fixed for the "matches" rule in the previous commit (this is a second, previously-uncited sink for that same finding — the original report only pointed at yup/helper.ts's mapRules). Reuse RegexHelper from the previous commit: parse via parseMatchesPattern (fixes the crash), and reject an overly long value outright instead of testing it (fail-closed, matching this condition's inverted "does NOT match" semantics — unlike "matches", where skipping the test defaults to invalid, here it must default to invalid via an explicit length check rather than reusing safeTestRegex's return value directly).
…tern in ImageManager resolveMatchesPattern's try/catch only guards against a syntactically invalid regex; a syntactically valid catastrophic-backtracking pattern (e.g. /^(.+)+\.(jpg|png)$/ — an easy, common authoring mistake) compiled fine and hung later when tested against the uploaded file's name — attacker-controlled with no schema-author compromise needed, since a user picks their own filename on upload. Reuse RegexHelper from the earlier commits: parseMatchesPattern for parsing (same behavior, now shared) and safeTestRegex for the actual test, which skips testing an overly long filename and treats that as not matching (fail-closed, consistent with this being a "must match" pattern check).
…dField getRegex duplicated the same ad hoc "/pattern/flags" parsing already fixed via RegexHelper in the previous three commits. Switch to parseMatchesPattern for consistency (same crash-safety as before, no behavior change). Note on scope: unlike the other three sinks, the actual regex .test() against user keystrokes happens inside the design-system MaskedInput component this field hands maskRegex off to, not in this file — this codebase doesn't own that test call, so the safeTestRegex length bound from the earlier commits can't be applied here. A catastrophic-backtracking maskRegex pattern tested against live keystrokes remains a real, unresolved exposure; fixing it would require a change in @lifesg/react-design-system's MaskedInput itself.
…yles before applying as CSS locationModalStyles is a schema-authored CSS string assigned directly to an element's style.cssText. @import can load a remote stylesheet and url() can reference a remote resource — both create a network-side-channel (e.g. exfiltrating data via request timing/ querystring, or just confirming a victim rendered the form) purely from a string in the form schema, no script execution needed. Add a shared StyleHelper.sanitizeStyleString and strip both constructs before assignment. Not a full CSS parser/allowlist — the documented use case (padding/margin tweaks) needs neither construct, matching this finding's Low severity rating. Same fix applied to imageReviewModalStyles's identical sink in the next commit.
…lStyles before applying as CSS Same schema-authored cssText sink and same fix as asgard-0011's locationModalStyles in the previous commit — @import/url() in a CSS string assigned straight to style.cssText is a network-side-channel risk with no script execution needed. Reuses the shared StyleHelper.sanitizeStyleString from that commit.
… ReDoS mitigation
The earlier MaskedField commit in this series ("Use shared RegexHelper
for maskRegex parsing in MaskedField") only deduplicated pattern
parsing — it did not fix the ReDoS exposure, because the actual
regex.test() against live keystrokes happens inside
@lifesg/react-design-system's MaskedInput component, which this
codebase doesn't own. That commit's message says as much explicitly;
this finding was left open, not closed.
This codebase does control one thing MaskedInput receives: the
maxLength attribute, already derived from max/length Yup validation
rules when present. Extend that derivation to default maxLength to
RegexHelper.MAX_SAFE_PATTERN_INPUT_LENGTH (500) whenever maskRegex is
configured and no narrower max/length rule already applies. This caps
the keystroke buffer length at the DOM level before MaskedInput's
internal test ever runs, without needing an upstream design-system
change — the same mitigation strategy already used for the other 3
ReDoS sinks, applied here via the one lever this component actually
has.
Not a complete fix: a pathological maskRegex can still backtrack
catastrophically against inputs up to 500 characters. Genuinely
closing this requires either an upstream MaskedInput change or
authoring-time linting of maskRegex patterns — out of scope here.
…sion
ImageUpload's own "matches" filename-pattern validation is handled
per-file inside ImageManager (filenameMatches, tested against each
image's own name) — that part works correctly and already excludes
non-matching files from the submitted value.
But image-upload.tsx also forwarded the raw, unfiltered validation
array (including that same "matches" rule) into the generic Yup
mapRules pipeline for the field's array-level schema. mapRules had no
type guard on "matches" (unlike email/url/uuid, which are wrapped and
warn on a type mismatch) — it added a Yup .test() that runs against
the field's whole array value. Once any file actually uploads
successfully, that value is an array of file objects, and
RegExp.test() coerces it to a string ("[object Object]"-style) before
testing — which fails almost any real pattern, rejecting the array at
the form level and silently blocking the entire submission (SUBMIT_FN
never fires), regardless of how much time or how many retries pass.
This was already breaking any ImageUpload field configured with a
matches rule as soon as a real submission was attempted — not just
the mixed valid/invalid-file case that surfaced it. It predates
MOL-22453 entirely and is unrelated to the security patches, but was
found and is being fixed here because it deterministically blocks the
pre-push hook's test run for these branches.
Two-part fix:
- image-upload.tsx: stop forwarding "matches" into the generic array
schema at all, since ImageManager already owns that validation.
- yup/helper.ts: add a type guard to mapRules's "matches" case (warn
and skip for any non-string schema), matching the existing
email/url/uuid pattern — defense in depth against the same class of
bug on any other field, present or future.
thanhpt0105-gt
requested changes
Sep 17, 2026
| [[ $GITHUB_EVENT_NAME = "pull_request" ]] && BRANCH_NAME="$HEAD_REF" || BRANCH_NAME="$REF_NAME" | ||
|
|
||
| [[ $GITHUB_EVENT_NAME = "pull_request" ]] && COMMIT_MSG="${{ github.event.pull_request.title }}" || COMMIT_MSG=$(echo -e "${{ github.event.head_commit.message }}" | head -n 1) | ||
| [[ $GITHUB_EVENT_NAME = "pull_request" ]] && COMMIT_MSG="$PR_TITLE" || COMMIT_MSG=$(echo -e "$HEAD_COMMIT_MSG" | head -n 1) |
There was a problem hiding this comment.
https://github.com/LifeSG/react-icons/blob/main/.github/workflows/trigger-gitlab-pipeline.yml#L16-L22
Can follow this PR for the COMMIT_MSG
added 5 commits
September 17, 2026 11:43
…per review
Per PR review feedback, follow the same pattern already used in
LifeSG/react-icons' equivalent workflow: printf '%s\n' | awk
'NR>1{exit};1' instead of echo -e | head -n 1. Purely a robustness
nit, not a security fix — the injection vector this series' asgard-0001
commit closed (raw ${{ }} interpolation into the run: script) is
unaffected either way, since HEAD_COMMIT_MSG already flows through
env: before this line runs. echo -e interprets backslash escape
sequences literally present in the commit message text; printf '%s'
does not, which is the more correct behavior for opaque user-supplied
text and matches the org's existing convention.
…relative src
The asgard-0007 fix derived the allowed origin via new URL(src) with
no base URL. That throws for any relative or protocol-relative src
("/embedded/form", "//partner.example/form") — both completely
ordinary ways to configure a same-origin embed. On failure,
getTargetOriginFromSrc caught the error and returned null, and
useIframeMessage's guard was `if (allowedOrigin && event.origin !==
allowedOrigin) return;` — since null is falsy, the check never fired,
silently accepting postMessage events from any origin. That's the
exact vulnerability asgard-0007 was meant to close, reintroduced for
any iframe using a relative src.
Two-part fix:
- getTargetOriginFromSrc now resolves against the embedding page
(new URL(src, window.location.href)), so relative/protocol-relative
src values correctly derive a real origin instead of throwing. Also
rejects non-http(s) protocols (e.g. javascript:) explicitly rather
than deriving a nonsensical origin from them.
- useIframeMessage now distinguishes undefined (caller didn't request
an origin check — used by the child-side iframe-content demo pages,
which listen to their parent and don't enforce this) from null (an
origin check was required but couldn't be established). Only the
latter now fails closed: reject every message rather than accept
every message when no valid origin could be derived.
Found via external code review, not this codebase's own testing.
…h DOM attribute The earlier follow-up commit that defaulted maxLength to 500 when maskRegex is set only bounds what a user can type through the native input's own typing/paste handling. It does nothing to a value that arrives via defaultValues or a form reset: stateValue was set directly from the value prop with no length check, and handed straight to MaskedInput along with maskRegex — MaskedInput evaluates that regex against the value when rendering, regardless of what maxLength attribute happens to be sitting on the input. A long default value with a catastrophic maskRegex hangs on mount/reset with zero user interaction required, which is worse than the keystroke case the maxLength attribute actually addresses. Compute the same safe-length bound as a plain function so it's available synchronously (not just once the derivedAttributes effect has run), and apply it to stateValue itself, both on initial mount and whenever the value prop changes — not only to the DOM attribute. Found via external code review, not this codebase's own testing. Regression test added: reproduced the hang directly (process had to be killed after 15s with the fix removed) before confirming the fix resolves it.
…l limitation Every existing "should not hang" test for the 4 ReDoS sinks uses a 501+/600+ character malicious input — all that proves is the length short-circuit itself is fast, not that the bound protects against catastrophic backtracking. The classic pattern these fixes cite (/^(a+)+$/) already costs real, measurable time well under the 500-char threshold: independently measured at 20 chars/21ms, 25 chars/160-800ms, 30 chars/~5s. The suite had no test exercising that actual danger zone. Add one canonical test in regex-helper.spec.ts (the shared utility 3 of the 4 sinks call directly) using a 25-character near-match, calibrated to complete in under a second on this measurement while staying nowhere near the safe length bound — demonstrating the input is not short-circuited and the underlying regex actually runs, unlike every existing 501+/600+ char test. Generous 20s per-test timeout to tolerate normal measurement variance across CI hardware without flaking, not because a hang is expected. Found via external code review, not this codebase's own testing.
… values MaskedInput keeps its own internal raw-value state instead of always deriving it from the value prop. When maskRegex newly appears while that internal state still holds a long pre-existing value, it re-masks against its stale internal value, not whatever (already-clamped) value we pass on that render, reintroducing the catastrophic-backtracking hang regardless of our own clamping. Force a remount by keying on maskRegex so the fresh instance always initialises from the current clamped value, and derive the displayed value at render time rather than only in an effect that runs after the render has already committed. Also add an implicit max-length validation constraint when maskRegex is set with no explicit author max/length rule, so an oversized programmatic value (defaultValues, setValue, reset) is rejected with a field error instead of silently displaying truncated while the full, uncapped value remains in form state and gets submitted as-is.
Contributor
|
the sanitize-html bump is going to break jest and potentially downstream consumers see this comment for more info: #721 (comment) would suggest to leave it out of this ticket and tackle separately |
wenheloh
reviewed
Sep 18, 2026
Comment on lines
+140
to
+142
| // only messages from the origin derived from `src` are accepted; a child iframe that | ||
| // navigates to a different origin before posting back will need to do so via the | ||
| // original src origin's window, or the message is dropped |
Contributor
There was a problem hiding this comment.
reminder to add this potentially breaking change in changelog later, and maybe need TLDR like "if whatever you're serving using iframe and it navigated to another domain, it might break"
Contributor
There was a problem hiding this comment.
normally we will put the test file at the same directory of the target that we're testing, why is this test file here?
Contributor
There was a problem hiding this comment.
FDS and FEE have a separate __tests__ directory rather than co-locating test files 😬
shengxi-gt
changed the title
qroll
reviewed
Sep 18, 2026
Contributor
There was a problem hiding this comment.
FDS and FEE have a separate __tests__ directory rather than co-locating test files 😬
| expect(() => | ||
| boldResultsWithQuery([buildResult("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa!")], maliciousQuery) | ||
| ).not.toThrow(); | ||
| expect(Date.now() - start).toBeLessThan(1000); |
Contributor
There was a problem hiding this comment.
jest just seems to hang indefinitely on this test case, this time check doesn't seem useful
qroll
reviewed
Sep 18, 2026
Contributor
There was a problem hiding this comment.
would argue against this change, the value are provided by the client/server in the first place
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
Six changes alter default runtime behaviour unconditionally, no opt-out, for every existing schema as soon as this ships:
allowedFileOriginsis now required forFileUploadprefill. A prefilledfileUrlis no longer fetched unlessallowedFileOriginsis explicitly set. Checked all in-scope consumer repos; none currently rely on the old default.trustProvidedFileMetadataflag, defaultsfalse. Files with nodataURL/fileUrlare now flagged as unverified instead of silently accepted.IframepostMessageorigin check is now hard default-on. Messages from a mismatched origin are silently dropped.matches,notMatches, andfilenameMatchesrules.maxLengthnow defaults to 500 whenevermaskRegexis set and nomax/lengthrule already narrows it./pattern/flagsslashes 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): stricter
sanitize-htmlconfiguration may now strip attributes that previously passed through; the ButtonField URL scheme check may block non-standard schemes; custom modal style strings now have@import/url()stripped.Changes covered
sanitize-htmlconfiguration tightened in Typography, FilterCheckbox, PopoverpostMessagenow validates origin by defaultRegExpFileUploadprefill now requires an explicit origin allowliststatefield documented as client-asserted (no code change)maxLengthdefaults to 500 whenmaskRegexis set@importandurl()referencesmatchesrule no longer blocks valid submissions (pre-existing bug fix)Test plan
eslint+tsc --noEmitpass on this branch.