fix(security): ADR task records stay inside the wiki root (GHSA-5w3v-7v7h-q9x4) - #941

Claude finished @cdeust's task in 2m 43s —— View job

PR #941 review (1 of 3): done

• Read the diff against origin/main and the code around it
• Ran the security, code quality, test coverage, documentation and performance reviews
• Posted the review as a top-level comment: fix(security): ADR task records stay inside the wiki root (GHSA-5w3v-7v7h-q9x4) #941 (comment)

The reviews found no Blockers. The containment fix is complete for an MCP caller:

• An explicit domain is reduced to [a-z0-9-].
• The writer resolves the path, which handles both .. and a symlinked domain directory, and refuses it before gated_files.write_text.

The review comment lists three Notes that need no action before merge:

• An explicit domain is now normalized differently from resolve_explicit_domain, so cortex_v2 becomes cortex-v2.
• _next_adr_number lists the domain directory before the containment check runs.
• No test runs the full handler with a traversal domain.

I traced the code but did not run the tests.

Verdict: approve

Review 1 of 3: PR #941 (GHSA-5w3v-7v7h-q9x4)

Five foreground reviews ran: security, code quality, test coverage, documentation accuracy and performance. None found a Blocker. Reviewed at 702bb62.

Security (traced, not run): the fix is complete for an MCP caller.

• domain_id_from_label (shared/project_ids.py:106) leaves only [a-z0-9-]. Any input that reduces to nothing becomes "unknown".
• The writer resolves the path before checking it (auto_task_record_writer.py:223-226), so .. and a symlinked adr/<domain> are both refused before gated_files.write_text.
• _atomic_write opens the temp file with O_NOFOLLOW, and os.replace replaces a symlink rather than writing through it.
• maybe_write_task_record has one caller (record_session_end.py:349). Every other adr/<domain> writer already slugifies the domain or calls contained_file.

Tests:

• The writer tests reach the new check: without it, the rglob and outside.iterdir() assertions fail.
• No existing test breaks. The only explicit domain passed to the handler is "nonexistent", which normalization leaves unchanged.
• Performance: two resolve() calls run once per session end, which costs nothing measurable.

Notes, no action before merge

• record_session_end.py:181: domain_id_from_label doesn't match resolve_explicit_domain (shared/domain_mapping.py:476), which remember/recall use. An explicit cortex_v2 now becomes cortex-v2, so the profile lookup at :275 misses and the session memories land under a different domain. Consider resolve_explicit_domain plus rejecting anything that isn't a single safe path segment, and raising an error instead of substituting "unknown".
• auto_task_record_writer.py:221: _next_adr_number lists adr/<domain> before the containment check. It only reads, and the error path leaks nothing, but checking first would close the read too. The existing safe_join/contained_file helpers could replace the hand-rolled check.
• Tests: no end-to-end handler({"domain": "../../../x"}) test, and status == "error" could also assert "escapes wiki root" in result["reason"].

Verdict: approve

Claude finished @cdeust's task in 1m 37s —— View job

PR #941 review (2 of 3): done

• Checked the earlier Blockers: review 1 had none
• Found the commits pushed after 702bb62: only 4a74a0a, which changes only a test file
• Ran the security, code quality, test coverage, documentation and performance reviews on 4a74a0a
• Run the containment tests and ruff: not done, because both commands need approval in this environment. I traced the code instead.
• Posted the review as a top-level comment: fix(security): ADR task records stay inside the wiki root (GHSA-5w3v-7v7h-q9x4) #941 (comment)

None of the reviews found a Blocker. The new assertions match what the writer actually returns, "ValueError: task record path escapes wiki root", and an explicit domain of .., ../.. or /// falls back to "unknown". Both tests would fail if the fix were removed.

The review comment lists two Notes that need no action before merge:

• ESCAPE has no leading underscore, unlike the file's other constant, _TRAVERSAL.
• From review 1, still open: _next_adr_number reads the domain directory before the containment check runs.

Verdict: approve

Review 2 of 3: PR 941 (GHSA-5w3v-7v7h-q9x4)

Earlier Blockers: review 1 (702bb62) had none.

Scope: the only commit since 702bb62 is 4a74a0a. It changes only tests_py/handlers/test_auto_task_record_writer_containment.py; no production code. All five foreground reviews ran on it: security, code quality, test coverage, documentation and performance. None found a Blocker.

• Exact refusal pinned: the writer raises ValueError("task record path escapes wiki root") at auto_task_record_writer.py:226. The error boundary at :241 formats it as f"{type(exc).__name__}: {exc}", which is exactly ESCAPE.
  • The traversal test and the symlink test now pass only on this specific refusal. Before, any other error caught by that boundary (an OSError, say) would also have passed.
  • Without the containment check, the writer would succeed (ensure_private_dir creates the parent directory), so both assertions would fail.
• Fallback to "unknown": domain_id_from_label turns .., ../.. and /// into a single -, then strips it to "". _resolve_domain (record_session_end.py:181) then returns "unknown". The test would fail if either the normalization or the or "unknown" were removed.
• Craftsmanship gate: the source: rule only applies to numeric literals, so the string constant ESCAPE does not trigger it. The test file is 105 lines.
• Docs: the new comment, the test name and the CHANGELOG entry all match the code.
• Performance: no new I/O or fixtures.

I traced the code but did not run pytest or ruff: this environment needs approval for those commands.

Notes, no action before merge

• ESCAPE has no leading underscore, but the file's other constant, _TRAVERSAL, does.
• From review 1, still open: _next_adr_number lists the files in adr/<domain> before the containment check runs. It only reads.

Verdict: approve

Claude finished @cdeust's task in 1m 27s —— View job

PR #941 review (3 of 3): done

• Checked the earlier Blockers: reviews 1 and 2 found none
• Found the commits pushed after 4a74a0a: only 03bf3d8, which merges origin/main at 843fdd3. git diff 843fdd3 03bf3d8 is exactly the PR's original 4 files, so the merge was clean and left the fix as it was.
• Ran the five foreground reviews (security, code quality, test coverage, documentation, performance) on what the merge did to the PR files. None found a Blocker.
• Checked CI: the PR #941 check passed; Pipeline was still running when I posted.
• Posted the review as a top-level comment: fix(security): ADR task records stay inside the wiki root (GHSA-5w3v-7v7h-q9x4) #941 (comment)

Accepted: the whole PR, all 4 files. No file or commit has an open Blocker, so nothing needs a refinement PR. The review comment lists three Notes that need no action before merge.

I traced the code but did not run pytest or ruff.

Verdict: approve

Review 3 of 3: PR #941 (GHSA-5w3v-7v7h-q9x4)

Earlier Blockers: reviews 1 (702bb62) and 2 (4a74a0a) found none.

Scope: one commit landed after 4a74a0a: 03bf3d8, which merges origin/main (843fdd3). git diff 843fdd3 03bf3d8 is exactly the PR's original 4 files (+113/−2), so the merge was clean and changed nothing in the fix. I ran all five foreground reviews (security, code quality, test coverage, documentation, performance) on what the merge did to those files. None found a Blocker.

• Security: the fix is still in place.
  • record_session_end.py:181 returns domain_id_from_label(domain) or "unknown".
  • auto_task_record_writer.py:223-226 resolves the path and raises ValueError("task record path escapes wiki root") before gated_files.write_text.
  • host_patch.py came in from main and touches neither path.
• Code quality: no conflict markers or duplicated code. Both files have size and layer breaks that predate this PR, all already listed in .craftsmanship-baseline.json, so the gate is unchanged.
• Tests: the imports, the monkeypatched WIKI_ROOT / _git_commits_in_window and the ESCAPE string all still match the code. The tests that came in from main add no shared fixtures that would affect this test.
• Docs: the CHANGELOG entry for GHSA-5w3v appears once, reads correctly and matches the code. Main's GHSA-pw64 entry next to it is intact.
• Performance: no change. Each session end still does one pair of resolve() calls.

I traced all of this but did not run pytest or ruff. When I posted this, the PR #941 check had passed and Pipeline was still running.

Accepted: the whole PR, all 4 files: CHANGELOG.md, mcp_server/handlers/auto_task_record_writer.py, mcp_server/handlers/record_session_end.py, tests_py/handlers/test_auto_task_record_writer_containment.py. No file or commit has an open Blocker, so nothing needs a refinement PR.

Notes, no action before merge

• record_session_end.py:181: domain_id_from_label doesn't match resolve_explicit_domain, so an explicit cortex_v2 now becomes cortex-v2 (from review 1).
• auto_task_record_writer.py:221: _next_adr_number lists adr/<domain> before the containment check runs. It only reads.
• ESCAPE in the test file has no leading underscore, unlike the file's other constant, _TRAVERSAL.

Verdict: approve