Repository navigation
fix: switch SVG and MDX active-content gates to allowlists - #1730
Conversation
✅ Deploy Preview for endusers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
2cc7f42
ℹ️ Sentinel notice — informational; author is trusted (owner), merge is not blockedHive flagged this PR (author @mrbobbytables, head
Hive did not add the |
|
Heads-up: this rewrite still misses the cross-document ReproductionA page carrying: returns Why the allowlist does not catch itThe gate keeps the per-node check
Suggested fixGive the publish gate the whole-document pass the import-time escaper already has ( Since this PR is mid-rewrite of exactly that function, folding the second pass in here is likely cheaper than a follow-up that has to rebase over it. Details and completion criteria are in #1732. 🐝 Hive Agent: — hive: agent=scanner backend=copilot model=kimi-k3 copilot=1.0.88 |
6ba02dc
ℹ️ Sentinel notice — informational; author is trusted (owner), merge is not blockedHive flagged this PR (author @mrbobbytables, head
Hive did not add the |
![hivecommons-hive[bot]](https://avatars.githubusercontent.com/in/3568013?s=60&v=4){kind=small}
There was a problem hiding this comment.
Conclusion: the mechanics check out in everything I read, and I found no defect — but this is a security-posture change that touches SECURITY.md, so a maintainer should sign off rather than an agent.
What I verified against the branch (head 6ba02dc), reading the tree rather than just the diff:
scripts/lib/uri-safety.mjs:disallowedScheme()reads the scheme of the normalized value (/^([a-z][a-z0-9+.-]*):/) againstALLOWED_SCHEMES = {http, https, mailto};data:is admitted only forRASTER_DATA_TYPESand only when the caller opts in. SincenormalizeUrican only remove characters a browser keeps, mis-normalization over-rejects rather than misses — fail-closed holds.scripts/lib/svg-active-content.mjsattributeAllowed(): namespace declarations themselves are gated (if (uri === XMLNS_NS) return NAMESPACES.has(value)), so content cannot be rebound into MathML/XSLT/XML-Events or a future namespace. Unbound prefixes are well-formedness errors undersax.parser(true, { xmlns: true }).classifyTag()checkshrefvalues directly and runscssUrls()over every attribute value (presentation attributes takeurl());<style>blocks go throughdisallowedStyles(); a malformedurl()token is itself rejected ("not vouched for"). The strip-then-rescan contract in the importer is preserved.- All checks green at this head.
The maintainer question (one sentence): do you accept the allowlist breadth — e.g. the nine metadata namespaces allowed wholesale (descendants still checked), target on <a>, data: raster images — and the new SECURITY.md sentence that puts "a way to publish anything outside those lists" explicitly in scope? That sentence changes what reporters are invited to probe, which is a policy decision, not a code one.
— hive: agent=reviewer backend=copilot model=claude-fable-5 copilot=1.0.88
The SVG and MDX gates rejected known-dangerous names and schemes, so every name or scheme nobody had thought of was published. Invert the model: anything not explicitly allowed is rejected on import or stripped and then fails the rescan. - SVG: allowlists of elements, attributes, namespaces and URL schemes, measured from the architecture diagrams in the repo. - MDX: allowlists of mdast node types, inline elements, the one component and its attributes, and URL schemes (http, https, mailto, relative, fragment). - A fast-check property test with a fixed seed and run count, plus table-driven rows for the four historical denylist bypasses, replaces the per-bypass denylist tests. - CONTENT-SECURITY.md lists every allowlist; SECURITY.md points to it. Part of #1718 (item 3). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bob Killen <bkillen@linuxfoundation.org>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bob Killen <bkillen@linuxfoundation.org>
6ba02dc to
ba4148e
Compare
Summary
Part of #1718 (item 3), part 1: the active-content gates become allowlists.
This eliminates the recurring Hive finding class "a new name, scheme or
syntax slips past the SVG/MDX denylist" (#1515, #1672, #1695, #1716 and the
fixing PRs #1516, #1673, #1696, #1713). Each of those was a construct the
denylist had not named yet. Under an allowlist the default answer for an
unnamed construct is "reject", so the next such finding becomes a request to
widen a list rather than a bypass.
Threat-model change
Before: the gates enumerated dangerous constructs (script, event handlers,
javascript:/vbscript:/script-capabledata:, animation elements, agrowing set of URL-bearing attributes) and published everything else. Safety
depended on the list being complete for every current and future browser.
After: the gates enumerate what architecture diagrams and imported pages
actually use, and everything else fails:
scripts/lib/svg-active-content.mjs): every element, attribute,namespace declaration and URL (attribute values,
url()instyleattributes and
<style>text) is checked against the lists below. Onimport,
stripActiveContentremoves anything off-list and then re-scans;any residue fails the import with the existing
reportAndExit/Could not strip/Active content survivedcontract. Theasset validator reports off-list content as errors (
--fixstrips).Namespace declarations are themselves allowlisted, so nothing can be
moved into MathML, XSLT, XML Events or an unknown namespace. A malformed
CSS
url(...)token is rejected rather than skipped.scripts/lib/mdx-active-content.mjs,mdx-escape.mjs,architecture-content.mjs): every mdast node type, JSX element, JSXattribute and URL scheme is checked. A node type a future parser or
extension introduces is reported as
disallowed node type X; any MDXexpression or non-literal attribute expression stays rejected.
scripts/lib/uri-safety.mjs):activeScheme(denylist) isreplaced by
disallowedScheme(allowlist).The remote-subresource gate (
findRemoteReferences) no longer checkssrc/srcset/action/formactionitself: those attributes are off theallowlist, and both gates always run together in every caller (importer,
asset validator, profile/asset mirroring).
Allowlists
SVG elements in the SVG namespace (40):
a,circle,clipPath,defs,desc,ellipse,feBlend,feColorMatrix,feComposite,feDropShadow,feFlood,feGaussianBlur,feMerge,feMergeNode,feOffset,filter,foreignObject,g,image,line,linearGradient,marker,mask,metadata,path,pattern,polygon,polyline,radialGradient,rect,stop,style,svg,switch,symbol,text,textPath,title,tspan,use.SVG attributes, unprefixed (133):
alignment-baseline,aria-describedby,aria-hidden,aria-label,aria-labelledby,baseProfile,baseline-shift,class,clip,clip-path,clip-rule,clipPathUnits,color,color-interpolation,color-interpolation-filters,color-rendering,cx,cy,d,direction,display,dominant-baseline,dx,dy,enable-background,fill,fill-opacity,fill-rule,filter,filterUnits,flood-color,flood-opacity,font-family,font-size,font-size-adjust,font-stretch,font-style,font-variant,font-weight,fr,fx,fy,gradientTransform,gradientUnits,height,href,id,image-rendering,in,in2,isolation,k1,k2,k3,k4,lang,lengthAdjust,letter-spacing,lighting-color,marker-end,marker-mid,marker-start,markerHeight,markerUnits,markerWidth,mask,maskContentUnits,maskUnits,mix-blend-mode,mode,offset,opacity,operator,orient,overflow,paint-order,pathLength,patternContentUnits,patternTransform,patternUnits,pointer-events,points,preserveAspectRatio,primitiveUnits,r,refX,refY,requiredExtensions,requiredFeatures,result,role,rotate,rx,ry,shape-rendering,spreadMethod,startOffset,stdDeviation,stop-color,stop-opacity,stroke,stroke-dasharray,stroke-dashoffset,stroke-linecap,stroke-linejoin,stroke-miterlimit,stroke-opacity,stroke-width,style,systemLanguage,target,text-anchor,text-decoration,text-rendering,textLength,transform,transform-origin,type,unicode-bidi,values,vector-effect,version,viewBox,visibility,width,word-spacing,writing-mode,x,x1,x2,y,y1,y2.Root
<svg>only:content(draw.io stores its mxfile there;the validator warns and strips it under
--fix).Any element:
data-*attributes matching/^data-[a-z0-9._-]+$/(authorstorage with no platform behaviour; Illustrator
data-name, draw.iodata-cell-id).XHTML elements inside
<foreignObject>(21):a,b,br,code,div,em,font,hr,i,li,ol,p,pre,s,span,strike,strong,sub,sup,u,ul.XHTML attributes (12):
align,class,color,dir,face,href,id,lang,size,style,target,title.xlink:attributes:href,title.xml:attributes:lang,space.Namespaces that may be declared: SVG, XHTML, XLink, XML, and these editor
metadata namespaces (9), whose elements and prefixed attributes are
allowed because no browser gives them behaviour:
http://creativecommons.org/ns#,http://ns.adobe.com/AdobeIllustrator/10.0/,http://ns.adobe.com/Extensibility/1.0/,http://ns.adobe.com/Graphs/1.0/,http://ns.adobe.com/SaveForWeb/1.0/,http://purl.org/dc/elements/1.1/,http://sodipodi.sourceforge.net/DTD/sodipodi-0.dtd,http://www.inkscape.org/namespaces/inkscape,http://www.w3.org/1999/02/22-rdf-syntax-ns#.Unprefixed attributes on editor metadata elements (16):
bleed,bordercolor,borderopacity,bottomLeftOrigin,gridtolerance,guidetolerance,height,id,margin,objecttolerance,pagecolor,showgrid,units,width,x,y.SVG URL values (
href,xlink:href,url()): relative, fragment-only,http,https,mailto, anddata:with a raster typeimage/avif,image/bmp,image/gif,image/jpeg,image/png,image/webp. (Remotehttp(s)subresources are still rejected separately by the remote gate.)MDX node types (24 Markdown):
blockquote,break,code,definition,delete,emphasis,footnoteDefinition,footnoteReference,heading,image,imageReference,inlineCode,link,linkReference,list,listItem,paragraph,root,strong,table,tableCell,tableRow,text,thematicBreak; plusmdxJsxFlowElement,mdxJsxTextElement,mdxjsEsm(
mdxjsEsmonly as the exact generatedimport CNCFProjectCard from '@site/src/components/CNCFProjectCard';).MDX inline elements, with no attributes (13):
b,br,code,em,hr,i,kbd,p,small,strong,sub,sup,u.MDX component:
CNCFProjectCardwithdescription,href,logo,name,since,version, valuesstring literals only;
hrefandlogoare URL-checked andlogomustnot be remote.
MDX URL schemes (links, images, definitions,
href/logo): relative,fragment,
http,https,mailto. Remote images are rejected separately.Basis: the vocabulary measured from the tracked SVGs (172 architecture assets
plus logos), all of which pass unchanged, plus closely related presentational
names editors emit for the same drawings (for example
feDropShadow,textPath,switch,strike,font), as CONTENT-SECURITY.md states.Behaviour changes worth reviewing
xmlns(or HTML insideforeignObjectwithout the XHTMLnamespace) is now rejected: its elements are in no allowlisted namespace.
No tracked asset is affected; three test fixtures gained an
xmlns.denylist), and
data:images in MDX are rejected (the importer alreadyreduced them to alt text).
ftp:) to alt text instead of demoting it to a link.findActiveContent(MDX) gains an optional{ mdastExtensions }argument so the unknown-node-type path is exercised with a real parse.
Tests
tests/active-content-allowlist.test.mjs(fast-check, seed 1718, 300runs per property, ~0.7 s): SVG and MDX trees with arbitrary element,
attribute, namespace and scheme names. An independent
sax/mdast walkasserts that no output of
stripActiveContent, no body the MDX gateaccepts, and no
escapeMdxoutput contains anything off-list. The fourhistorical bypasses (rescan after strip, first-definition-wins,
mdx-code-block,action/formaction) are table rows in the samefile. I mutation-checked it by hand: dropping the element or attribute
allowlist check, the malformed-
url()check, thedata-*pattern,the JSX attribute check, the literal-expression check or either MDX
scheme check makes a property fail.
per-construct strip loops and "every construct at once", per-scheme and
per-attribute rejection tests (event handlers,
src/srcset/poster/action/formactionremote tests, entity and data-type tests), theactiveSchemetests and the denylist contract incontent-security-contract.test.mjs(now binds each documented allowlistto the exported set, with a probe per entry).
Verification
npm run test:unit:coverage:check: 2981 pass; source lines and regions100% (the changed
scripts/libmodules at 100%/100%).npm run validate:architectures(8 records) andnpm run validate:architecture-assets(172 assets; the one existingforeignObjectwarning) pass.npm run buildpasses.check:format,check:spelling,check:markdown,check:duplication(255 clones;
CLONE_CEILINGlowered from 264 to 255) andcheck:auditpass.
Out of scope: CSP / Permissions-Policy headers (item 3, part 2) and a CSS
property allowlist (CSS values are scanned for
url()schemes only; notedunder residual risks in CONTENT-SECURITY.md).
Checklist
CONTRIBUTING.md)
git commit -s)