Repository navigation
feat(#1919): self-contained HTML diagram view, step 2 of 3 - #1925
Conversation
Plan steps 4 and 5. No user-visible change: no skill calls the renderer yet (step 3 of 3 adds --html). - view.html: one ASCII template with all CSS and JS inline. The CSP meta tag is the first child of head: default-src 'none', base-uri 'none', form-action 'none', and sha256 hashes for the one script and the one style. No style attributes. meta charset stays in the first 1,024 bytes. - render.py and `diagram.py html`: the model and layout go into one application/json block. `<`, `>` and `&` become JSON Unicode escapes, and a byte search test proves the block has none raw. All placeholders are filled in one pass. The title comes from the data block through document.title. - Interaction: node panel with source links (https only, each path segment encoded), downstream and upstream trace along the arrows, path highlight, pan, zoom, fit, substring find, keyboard, and a system, light and dark theme toggle. DOM built with textContent. - Tokens: --ay-* on #2F6DF6 with cool graphite neutrals, the system font stack, contrast tested at 4.5:1 or more in both themes. - .gitignore: __pycache__/. 65 Python tests and 10 graph-test groups pass. Built by Codex. Claude ran the wrappers and took headless Chrome screenshots in both themes. Refs #1919 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
atlas-apex
left a comment
There was a problem hiding this comment.
Security Review: PR #1925
Commit: ecb2e07e00f7526927e8a0c023b0d6343916d73c
Scope: Full
Summary
This PR adds view.html, render.py and the html subcommand of diagram.py. The renderer puts the model into one application/json block in one self-contained HTML file. I treated every model string as attacker-controlled. I read the template, the renderer, the CLI change and the model validator. I ran a hash and escape check on all eight goldens. I opened xss.html in headless Chrome with --dump-dom. CI is green for the head SHA.
Checklist Results
- Secrets & Credentials: Pass
- Injection Prevention: Pass
- XSS Prevention: Pass
- Auth & Authorisation: Not applicable
- Data Protection: Pass
- API Security: Not applicable
- Gate & Trust-Chain Integrity: Not applicable (no hook or settings change)
Assessment
- Script injection: Pass.
render.pymakes the data withjson.dumps(..., ensure_ascii=True). Then it changes<,>and&to<,>and&. Thus no string can close the data block or open an HTML comment in it. My check found no raw<,>or&in the data block of any golden.- The fill is one pass: one
re.subwith a function. The function output is not scanned again, andre.subdoes not read backslash escapes in it. The{{CSP}} and {{DATA}}label inplaceholders.htmlstays as literal data. No slot text occurs outside the data block in any golden.render.pyalso refuses a template where a slot does not occur exactly once. - The script builds the DOM only with
createElement,createElementNS,textContentandsetAttribute. All attribute names are fixed literals in the script. No model string sets an attribute name, an event handler or a style. The template has noinnerHTML,outerHTML,insertAdjacentHTML,document.writeorstyleattribute. document.titlegets a plain string assignment. In Chrome, the</title><script>title ofxss.diagram.jsonshows as text in<title>and<h1>. The</script><img onerror>label shows as text in<title>,aria-labeland<text>nodes.
- CSP: Pass.
- The CSP
metatag is the first child of<head>. The policy isdefault-src 'none'; base-uri 'none'; form-action 'none', plus onesha256hash forscript-srcand one forstyle-src. It has nounsafe-inline,unsafe-eval,data:,blob:or wildcard source. - I recomputed both hashes for each golden, and they match. In Chrome the script ran and drew the scene, so the browser accepted the script hash. The JSON block is not executable, so the policy does not need a hash for it.
- The hashed script and style contain no slot. Thus the fill cannot change the hashed bytes.
- The CSP
- Links: Pass.
- The view makes a link only when
web_urlstarts withhttps:, the parsed host is exactlygithub.comorgitlab.com, and the result starts withhttps:. Each path segment and the commit go throughencodeURIComponent. The link hastarget="_blank"andrel="noopener noreferrer". - The step 1 validator limits
web_urltohttps://, a host of letters, digits, dots and hyphens, an optional port and path segments from[A-Za-z0-9._~-]. Thusweb_urlcannot hold@,\,?,#or%, and appended text cannot change the host.javascript:anddata:cannot occur. Source paths with.,.., empty segments, a leading/, a drive letter,\or control characters fail validation.linemust be an integer.
- The view makes a link only when
- Find and other inputs: Pass. Find uses
toLowerCase()andincludes(). The view has noRegExp,eval,Function,setTimeoutorsetIntervalwith a string.localStoragegives only a theme value, and the view accepts it only from a fixed list. - Network: Pass. The template has no
fetch,XMLHttpRequest,WebSocket,EventSource,sendBeacon, image,@import,@font-faceor remote font. The fonts are system fonts.url(#arrow)is a reference in the same document, not a request.default-src 'none'blocks any other load. A click on a source link is a user navigation, not a view-time request.
Security Issues Found
None that meets the blocking bar.
Recommendations
Advisory only. None of these blocks the PR.
- LOW:
sources()callsnew URL(data.repo.web_url)withouttry. A value that the URL parser refuses would stop the panel for that item. The step 1 regex prevents this case today. Atrywould keep the view safe if the validator changes. - LOW: A
metaCSP cannot setframe-ancestorsorsandbox. This does not matter for a file opened fromfile://. If a team serves the file over HTTP, the server should sendframe-ancestors 'none'in a header. - INFO: The link host list has only
github.comandgitlab.com. A self-hosted forge gets plain text and no link. This is safe, and it is a product choice.
Verdict
APPROVED
Reviewed by Hakim (Security Auditor)
Reviewed commit: ecb2e07e00f7526927e8a0c023b0d6343916d73c
atlas-apex
left a comment
There was a problem hiding this comment.
APPROVED. Next action: the Hakim security review and the Nour design review (plan Q11) must run on this PR, and then a human gives the per-PR merge approval. I found no blocker. I found six advisory items. Item 1 is a step 4 evidence gap that the design gate needs.
Code Review: PR #1925
Commit: ecb2e07e00f7526927e8a0c023b0d6343916d73c
Scope: Full
Summary
This PR is step 2 of 3 for #1919 (plan steps 4 and 5). It adds the self-contained HTML view of a diagram model:
render.pyfillsview.htmlin one pass. It computes the CSP hashes and writes the model, the positions andmodel_sha256into oneapplication/jsonblock.diagram.py htmlwrites the result to stdout. Exit 0 means OK, 1 a validation fault, and 2 bad input.view.htmldraws the SVG and the panel with DOM calls only. It has the trace, path, pan, zoom, find, keyboard and theme features.- 8 golden HTML files, 2 new fixtures (
xss,placeholders), a Python suite and a node logic suite.
No skill calls the renderer yet, so this PR has no user-visible change.
Acceptance Criteria
The PR uses "Refs #1919" and no closing keyword. #1919 closes after step 3. I checked the #1919 criteria that this step owns, and the plan step 4 and 5 checks.
| Issue | Criterion | Status | Evidence |
|---|---|---|---|
| #1919 | The view uses the design system, light and dark, follows prefers-color-scheme, and has a toggle |
Met | view.html lines 8 to 31 (tokens, media query, data-theme), lines 424 to 437 (toggle, localStorage in try/catch). test_contrast passes. |
| #1919 | Click a node to open a panel: role, trust boundary or layer, source links at a pinned commit | Met | select(), details(), sources() and the "Group chain" heading in view.html lines 274 to 340. |
| #1919 | Trace downstream or upstream. Other nodes fade | Met | reachable() and trace() lines 141 to 156. highlight() adds .faded. The node suite checks arrow direction against an independent closure on 6 fixtures. |
| #1919 | Pick two nodes and highlight the path between them | Met | directedPath() and pathBetween() lines 157 to 167: forward(A) intersected with backward(B), then B to A. Shift-click and "Path to" call it. |
| #1919 | The file works offline, all code is inside, no network calls | Met | The CSP has default-src 'none'. The template has no :// except the SVG namespace (test). A Chrome net log for dfd.html holds no request that came from the file. |
| #1919 | --html on the three skills; HTML and Mermaid read one model; validation before output |
Not verifiable here | Step 3 owns the skill flags. The model_sha256 field and the HTML drift test (test_generated_html_drift_check) prepare it. |
| Plan step 4 | Every HTML smoke check passes | Met | See the rule table in Validation. |
| Plan step 4 | Screenshots of each fixture in light and dark go into the PR | Partly met (advisory, item 1) | The PR describes 2 local screenshots and attaches none. |
| Plan step 5 | Logic suite passes | Met | 10 groups pass locally. CI job hook tests (linux jq-1.7) passes the wrapper with CI=true. |
| Plan step 5 | Manual check in Chrome, Safari and Firefox from file://, zero requests |
Partly met (recorded gap) | The PR records Safari and Firefox as not run. I rendered 3 goldens in headless Chrome. I did not click through the interactions in a browser. |
Checklist Results
- Architecture & Design: ✅ Pass.
render.pyis a pure function over a validated model.diagram.pykeeps validation before output, so a failed validation writes no HTML (test_cli_exit_codes). - Code Quality: ✅ Pass.
render()checks each slot count and the script and style count on the template and raisesValueError.diagram.pymaps that to exit 2. No error is swallowed. - Testing: ✅ Pass. The goldens are not vacuous: 3 mutations each made the suites fail (see Validation). The suites ran on a real Python 3.9.6.
- Security: ✅ Pass for my scope. I checked the plan safety rules below. Hakim owns the full OWASP pass and is still required on this PR.
- Performance: ⚠ Advisory. Files are about 25 KB, and 109 KB for 300 nodes. The 300-node first-render timing was not measured (recorded gap). Fit on the 300-node golden shows a thin strip (item 4).
- PR Description & Glossary: ✅ Pass. Summary, Testing, Glossary, "Refs #1919", no closing keyword. Known gaps are recorded: Safari and Firefox, the 300-node timing, and the A10 scan gap. No private name and no outside project.
- Summary Bullet Narrative: ✅ Pass.
- Technical Decisions (AgDR): ✅ Pass. AgDR-0279 bullets 6 and 7 on
devdescribe this design. The PR adds no new library and no new decision. - Adopter Handbooks: ✅ Pass.
handbooks/general/commit-message-qualityapplies: one commit with a conventional title, no finding. The architecture handbooks do not apply (no layered domain code, no migration). No language handbook matches. The private layer has two advisory handbooks that apply. Neither has a finding. Its architecture handbook asks for a CSP in the presentation layer, and this view has one. - Fallow Static Analysis (JS/TS): N/A. The diff has a
.jstest file, but the fallow CLI is not installed on this host.
Issues Found
None that block.
Suggestions
- Step 4 screenshots (plan step 4 check, plan Q11). The PR says that the screenshots are local and describes 2 of them. The plan asks for each fixture in light and dark. Attach light and dark screenshots to the PR before the Nour design review. Any golden in
tests/golden/produces one with headless Chrome. This is evidence for the design gate, not a code fault, so it does not change my verdict. - Tab order.
drawEdges()runs beforedrawNodes(), and every edge hastabindex="0". So a keyboard user tabs through every edge before the first node. The plan says "Tab moves between nodes". Draw order puts edges under nodes on purpose. A fix that keeps the draw order: give edgestabindex="-1", or move the nodes first in focus order with a separate focus list. - Source-link builder has no automatic test.
sources()(lines 274 to 293) does thehttps:check, the github.com and gitlab.com forms, andencodeURIComponentper path segment. I read it and it is correct. The node suite runs only the code between theay:graphmarkers. Move the URL builder into that section and add cases for#,?,%and a space in a path, and for an unknown host. - Fit at 300 nodes. The 300-node golden has a layout of 120,012 by 1,328 units. Fit shows it as a thin strip, and the reader must zoom. The plan limits readability to about 80 nodes, and the layout comes from step 1. Consider a minimum Fit scale, or fit to the first band, in a later step.
- Regenerate line. The footer always shows
<generated.by> <project> --html --force. Plan advisory A3 says/dfddoes not use--force, and/feature-diagramtakes a slug, not the project. Step 3 must align this line with each skill's real syntax. - A10 static-analysis gap. The PR body records the gap. Plan advisory A10 also asks to record it in the AgDR-0279 Consequences and to update the comments in
codeql.ymlandsecurity-scan.yml. Do this in step 3 or file a follow-up.
Validation
All runs are on the PR head, exported by the GitHub tarball for ecb2e07e into a new, empty scratch folder. Wrappers ran under /bin/bash 3.2.57.
CI (one read): all required checks pass on ecb2e07e. hook tests (linux jq-1.7) prints PASS for test_diagram_html.sh and test_diagram_view_logic.sh.
Current local runs:
| Command | Result |
|---|---|
test_diagram_html.sh with Python 3.15.2 |
7 tests OK |
test_diagram_html.sh with /usr/bin/python3 3.9.6 |
7 tests OK. Goldens match byte for byte on 3.9. |
test_diagram_model.sh, test_diagram_mermaid.sh, test_diagram_layout.sh on Python 3.9.6 |
27, 19 and 12 tests OK. The total is 65, as the PR states. |
test_diagram_view_logic.sh with node v26 |
10 groups PASS |
Same wrapper, no node on PATH, CI=true |
FAIL message, exit 1 |
Same wrapper, no node on PATH, CI unset |
NOTE: line, exit 0. No line starts with SKIP (grep of the tests folder: 0 hits). |
| After all runs | No __pycache__ folder in the export |
Mutation probes (each in a separate copy):
- Remove the
&escape fromrender.py: 7 failures (6 goldens andtest_xss_and_single_pass). - Replace the one-pass fill with two sequential replaces, DATA first: 2 failures (the
placeholdersgolden andtest_xss_and_single_pass). - Reverse the adjacency in
reachable(): the node suite fails with an assertion error.
Browser (headless Chrome, file://, 1400 by 900):
c4-container.htmlandxss.htmldraw nodes, groups, edges and arrow markers. The header text comes from the script. So the script hash and the style hash in the CSP match the inline blocks. Chrome logged no console message.- In
xss.html, the dumped DOM after the script hasdocument.titleas the literal payload text, noimgelement, and exactly twoscriptelements (the JSON block and the main script). The crossing edge has the dashed violet style. - A net log for
dfd.htmlholds only browser background-service requests. No request has the file as its source. Basis: Observed for the log content. Inferred for attribution, because the net log does not tag each background request.
Plan and design-review rules:
| Rule | Status | Evidence |
|---|---|---|
CSP first child of <head>, default-src 'none'; base-uri 'none'; form-action 'none', correct hashes (B2, AgDR bullet 7) |
Met | view.html line 3. The test recomputes both hashes from the output. Chrome executes both blocks. |
<meta charset> in the first 1,024 bytes (N3) |
Met | Byte offset 280 in the 300-node golden. The test asserts it. |
JSON escapes for <, > and &, proven by a byte search (B1, AgDR bullet 6) |
Met | render.py line 36 to 37. The test runs raw.find on the encoded data block for each character, and checks that the three escapes exist in the XSS output. |
Title only through document.title (B2) |
Met | Fixed <title>Diagram</title>. view.html lines 438 to 439. The XSS title fixture and the Chrome DOM dump agree. |
| One-pass placeholder fill, count check on the template | Met | One regular expression and one replacement function. The count guard test covers 0 and 2 copies of each slot. |
No style= attribute (A9) |
Met | The test checks every parsed tag. A grep of the template finds no style write from JS. |
No innerHTML with model data |
Met | No innerHTML, outerHTML or insertAdjacent in the template. The test forbids innerHTML. |
https:-only links, path segments encoded (A9) |
Met (by reading, see item 3) | view.html lines 283 to 291. |
| Find is a substring match (A9) | Met | includes() on line 414. The test forbids RegExp(. |
| Trace follows arrows (Q3) | Met | Downstream uses outgoing. The node suite proves it on the diamond graph and the fixtures. |
| Contrast of at least 4.5:1 (A14) | Met | test_contrast checks 6 text colours on 3 backgrounds in both themes, plus white on #2F6DF6. It also asserts that the dark media-query tokens equal the explicit dark tokens. |
Node needed only when CI=true, never SKIP (Q9) |
Met | See the wrapper runs above. |
| Python 3.9, ASCII only | Met | Real 3.9.6 run. test_source_constraints checks ASCII and ast.parse(feature_version=(3, 9)) on every .py, .html, .sh and .js file in the library. |
.gitignore adds __pycache__/ |
Met | No tracked __pycache__ path exists in the repository, so the rule hides no tracked file. |
Limits: I did not test the click, shift-click, U, D, pan, zoom, find and theme interactions in a real browser. I did not run Safari or Firefox. I did not measure the 300-node render time. I did not repeat the Hakim OWASP pass.
Verdict
APPROVED
All plan step 4 and 5 safety rules and the Tariq rules B1, B2, N3, A9, A14 and Q9 are met, with test or browser evidence. The goldens equal the render.py output, and the mutation probes prove that the tests catch a change. The six suggestions are advisory under the severity bar. Item 1 is input for the design gate. The Hakim and Nour reviews are still required on this PR before the human merge approval.
🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: ecb2e07e00f7526927e8a0c023b0d6343916d73c
Summary
Step 2 of 3 for #1919 (plan steps 4 and 5): the self-contained HTML view of a diagram model. No user-visible change: no skill calls the renderer yet. Step 3 of 3 adds
--htmlto/dfd,/feature-diagramand/c4.python3 .claude/skills/_lib-diagram/diagram.py html MODEL [--repo-dir CLONE]writes one HTML file to stdout. Exit 0 means OK, 1 a validation fault, and 2 bad input or a generation fault.view.htmlis one ASCII template with all CSS and JS inline. It has two placeholders,{{CSP}}and{{DATA}}, and both are filled in one pass. Inserted text is never scanned again.<head>. It setsdefault-src 'none'; base-uri 'none'; form-action 'none', withsha256-hashes for the one script and the one style. So the browser blocks every network request.<meta charset>stays in the first 1,024 bytes.application/jsonblock.<,>and&become JSON Unicode escapes, and a byte-search test proves the block holds none of them raw.<title>Diagram</title>. The script setsdocument.titlefrom the data block. A fixture with an XSS payload in the title and the labels renders as text.textContent. There is nostyle=attribute and noinnerHTMLwith model data.https:URL, with each path segment encoded. Find is a substring match and never builds a RegExp from input.--ay-*set on#2F6DF6with cool graphite neutrals and the system font stack (owner decision Q4). A test computes contrast of at least 4.5:1 for the main text and accent pairs in both themes..gitignore:__pycache__/.Screenshots
Headless Chrome, 1400 by 900, from
file://:(The screenshots are local. The reviewer can produce them with
--headless=new --screenshoton any golden intests/golden/.)Testing
Run outside the sandbox:
nodeon these graphs: branching, cycles, parallel edges, group endpoints and disconnected nodes.nodeis required only whenCI=true.file://:Built by Codex. Claude ran the wrappers and the headless screenshots.
Refs #1919
Glossary
🤖 Generated with Claude Code