fix: harden security audit findings (path traversal, approval gate, parser) - #6
Conversation
`wtype` is read straight from a quest's watch.json and used to build the
checker script path `checkers/{wtype}.py`, which is then executed. A value
containing `/` or `..` could escape the checkers/ dir and run an arbitrary
executable. Gate the path on `[a-z0-9_]+` (the charset of every real watch
type); anything else routes to the existing misconfig verdict instead of an
exec.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
slack-send.py passed quest_id to quest_dir() without the traversal guard that log-event.py already applies, so both send paths validated inconsistently. Add the same guard (reject `/`, `.`, `..`, control chars) once at the single origin so both quest_dir() call sites are covered. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`start` was legal from both `pending_review` and `reviewed`, so the human-approval gate depended on the checker only marking reviewed items dispatchable rather than on the state machine itself. Drop the `pending_review -> start` transition so an unreviewed item can never move to `executing`; the gate is now structural. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A real Slack MCP message is `=== header ===` then `Message TS:` on adjacent lines. The parser accepted any TS line as a message start, so a `Message TS:` line an author typed into a body could be counted/mis-attributed. Require the TS to be immediately preceded by a header, which enforces the format contract and removes the orphan/lone-TS mis-count. A body that forges an adjacent header+TS pair remains inherent to the MCP's unescaped text format (documented inline); only structured MCP output closes that fully. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Unreferenced anywhere. It built a path from quest_id without the traversal guard the live paths apply, so removing it also drops an unvalidated-path helper that could be wired up unsafely later. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
guangmian-circle
left a comment
guangmian-circle
left a comment
There was a problem hiding this comment.
Approving changes, adjusted one fix and adding tests to it to cover future regression.
|
Pushed the YAAS v2 docs snapshot onto this branch ( Three files, docs only, no source changes. The publish replaces the whole tracked tree from the mirror, so I first adopted this branch's security hardening into the mirror and verified the resulting tree is identical to Local suite green: 46 suites, 29 differential goldens. |
97658a2 to
eb24a4d
Compare
eb24a4d to
b2a115f
Compare
|
Rewrote the branch above The branch is now the five security commits unchanged, plus one signed commit ( What it replaced:
Every commit on the branch now reads |
guangmian-circle
left a comment
guangmian-circle
left a comment
There was a problem hiding this comment.
Approving after making final changes based on the suggestions, as well as tightening up the README.
guangmian-circle
left a comment
guangmian-circle
left a comment
There was a problem hiding this comment.
Approving once more, after correcting logo path.
3 participants
Summary
Fixes the 5 items surfaced by a whole-codebase security audit of
yaas-triage. The audit found no high-confidence (>=8) exploitable vulnerabilities; these are the defense-in-depth / correctness hardening items below the exploitable bar. Each is a small, surgical change with tests kept green.watch.jsontypebefore it builds/execs the checker path ([a-z0-9_]+gate → misconfig verdict otherwise)tick.pylog-event.pyquest_id traversal guard toslack-send.pyso both send paths validate identicallysurfaces/slack-send.pypending_review -> startso the human-approval gate is structural, not dependent on the checkerapproval_state.pycheckers/slack_utils.py_find_watch_json(unvalidated-path helper, unreferenced)ledger/approval-helper.pyKnown residual (finding #4)
The Slack MCP returns messages as one flattened, unescaped text blob, so a body that forges an adjacent
=== header ===+Message TS:pair remains indistinguishable from a real message boundary. The parser change enforces the format contract and closes the orphan/lone-TS path, but full closure needs structured (per-message) MCP output — documented inline with aponytail:note. Impact is now further blunted by #3 (dispatch requires a structural review gate).Test plan
tick.pyvs goldens 29/0 (no golden edits)slack_utils, allapproval-*,watch-guard,log-event,slack-sendsuites greenslack-send.py --quest-id ../../etc→invalid quest idexit 1; real ids pass throughwtypeguard accepts every real watch type, rejects traversal/dotted/empty(The one full-suite failure,
path-references.test.sh, is entirely a local gitignored.ua/plugin cache — 0 non-.uarefs — not code and not from this branch.)🤖 Generated with Claude Code