Stop an exported API key reaching the dot -V child process in shape - #28
Open
gitkodak wants to merge 1 commit into
Open
Stop an exported API key reaching the dot -V child process in shape#28gitkodak wants to merge 1 commit into
gitkodak wants to merge 1 commit into
Conversation
External review of ccd1113 found unifi-map shape's Graphviz-version probe (_graphviz_version, called from cmd_shape) ran `dot -V` with the plain inherited environment rather than layout.py's scrubbed one. The two Graphviz child processes used to actually render a map have stripped an exported UNIFI_API_KEY since early on; this third one, added later just to report a version number, was missed -- the same "fixed the instance, not the class" failure this repo has hit before. shape is specifically the command meant to be safe to paste into a bug report. Promoted layout._child_env to a public child_env(): it now has two production callers in two modules, which makes it a shared primitive rather than one module's private detail (same reasoning that moved the capped-read helper into httpio.py). Added a regression test that runs a stand-in `dot` and confirms an exported key never reaches it, red-tested against the pre-fix code before applying the fix. Also fixed a second, lower-severity finding from the same review: the randomised-MAC detector added for KAN-129 parsed only the substring before a MAC's first colon, so a malformed id like "2" read as hex 0x02 and came back locally-administered by accident. Now requires the full six-octet shape before checking the bit -- relevant because Node.id is not trusted input, and a support file's fields are attacker-controlled by this project's own threat model. Tightened a SECURITY.md claim the same review flagged as overclaiming: resolving Graphviz to an absolute path once guards against a substitution *after* resolution, not against PATH itself. Updates AI_DISCLOSURE.md's test count (686 -> 694), caught by its own guard test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
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
2 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.
Summary
Response to an external review of ccd1113 (KAN-129), which found three things. Two are fixed here; the third is a documented, deliberate trade-off with no code defect, so no change was made for it.
Fixed — High: API key leak (
cli.py)unifi-map shape's Graphviz-version probe (_graphviz_version, called fromcmd_shape) randot -Vwith the plain inherited environment rather thanlayout.py's scrubbed one. The two Graphviz child processes used to actually render a map have stripped an exportedUNIFI_API_KEYsince early on; this third one, added later just to report a version number, was missed.shapeis specifically the command meant to be safe to paste into a bug report.Promoted
layout._child_envto a publicchild_env()— it now has two production callers in two modules, so it's a shared primitive rather than one module's private detail. Added a regression test that runs a stand-indotand confirms an exported key never reaches it; confirmed it fails red against the pre-fix code before applying the fix.Fixed — Low: malformed-MAC parsing (
diagnostics.py)The KAN-129 randomised-MAC detector parsed only the substring before a MAC's first colon, so a malformed id like
"2"read as hex0x02and came back locally-administered by accident. Now requires the full six-octet shape before checking the bit — relevant becauseNode.idisn't trusted input, and a support file's fields are attacker-controlled by this project's own threat model.Not changed — Medium: release-provenance timing
The review noted the provenance workflow attests artifacts already built locally rather than building from the tagged source in CI. Correct, but already explicitly documented as a deliberate trade-off (
RELEASING.mdkeeps the build local and manual on purpose; the workflow itself says it runs "after the fact"). No code or doc defect, so no change.Also tightened a
SECURITY.mdclaim the same review flagged as overclaiming: resolving Graphviz to an absolute path once guards against a substitution after resolution, not againstPATHitself.Test plan
make check(ruff format/lint + 694 tests) passes cleanmake docsproduces no further changesvalidatebefore this PR🤖 Generated with Claude Code