Repository navigation
[MOL-22741][SX] Hardening pass 2 — main (v3-alpha) - #762
Open
shengxi-gt wants to merge 14 commits into
Open
shengxi-gt wants to merge 14 commits into
shengxi-gt wants to merge 14 commits into
Conversation
added 8 commits
October 7, 2026 17:12
…ers and strip traversal
… disallowed declarations
qroll
reviewed
Oct 9, 2026
added 6 commits
October 15, 2026 16:31
…om modal style snapshots
qroll
approved these changes
Oct 15, 2026
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
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.
Type of changes
Description of changes
StyleHelper: allowlist layout declarations and resolve CSS escapeslocationModalStyles,imageReviewModalStylese2e: restrict CDN proxy to asset paths and stop following redirectsFileUpload, ESignature: only fetch prefilled http(s) fileUrlfileUrlFileHelper: restrict sanitizeFileName to safe characters and strip traversalRegexHelper: clarify length cap only bounds polynomial backtrackingFileHelper: document getType text fallback as a client-side checkLint: format files left unformatted on maine2e: assert custom modal styles keep layout and strip disallowed declarationslocationModalStyles/imageReviewModalStyles— the previous sanitiser only stripped the literal texturl(and@import, which CSS escapes (e.g.\75rl() and non-url()resource functions such asimage-set()get past. It nowkeeps only layout declarations (
padding*,margin*,width/heightand theirmin-/max-variants,top/right/bottom/left,border-radius) after resolving CSS escapes. Anything else — other properties,@mediaand other rules, resource functions — is dropped with aconsole.warn. On main these values go throughstyle.cssText, which never applied@mediaanyway. The sanitiser does not touch the DOM so it can be reused wherestyles are built during server-side rendering.
fileUrl(FileUpload and ESignature) — onlyhttp(s)urls (including relative urls) are fetched.Other schemes (
javascript:,data:,file:,blob:) are not fetched and the field shows its existing errorstate ("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 inHardening pass: input validation, origin checks, and default-safe field behaviour — main (v3-alpha) #739; the field docs now say so.
sanitizeFileNamenow keeps letters, digits, space,_ - ( )and inner dots; extensionskeep 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) anddotfiles without an extension (
.env) are unchanged. Names with! @ # $ % ^ & * ~ ' , ; = + [ ] { }lose thosecharacters. 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
1000-character cap only bounds polynomial backtracking; nested quantifiers such as
(a+)+$can still backtrackexponentially 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.
engine — e.g.
re2js(~61 KB gzipped, single maintainer, would need pinning/vendoring), or a small RE2-style packagewe 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 andthe file name. Documented as a client-side convenience check rather than changed.
@for retina assets such as400@2x.png.Checklist
Test plan
eslint+tsc --noEmitpass on this branch.fileUrland file-name changes).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.