fix(approval): collapse redundant path separators before sensitive-path detection

[Jiaaqiliu]

What does this PR do?

_normalize_command_for_detection folds absolute home prefixes, strips backslash escapes, and expands $IFS — but it never collapses redundant / runs or no-op /./ segments inside a path token. The sensitive-path fragments (_SSH_SENSITIVE_PATH, _HERMES_CONFIG_PATH, the shell-rc and dotfile targets) all anchor on exactly one / between components, so a spelling POSIX resolves to the very same file slips every terminal-side write rule.

FLAGGED    echo 'approvals: {mode: off}' > ~/.hermes/config.yaml
UNFLAGGED  echo 'approvals: {mode: off}' > ~//.hermes/config.yaml
UNFLAGGED  echo 'approvals: {mode: off}' > ~/.hermes//config.yaml
UNFLAGGED  echo k >> ~//.ssh/authorized_keys
UNFLAGGED  echo k >> ~/./.ssh/authorized_keys
UNFLAGGED  echo x >> ~//.bashrc
UNFLAGGED  echo x >  ~//.hermes/.env

This matters most for ~/.hermes/config.yaml, for the reason the comment above _HERMES_CONFIG_PATH already states: that file is the security policy (approvals.mode, yolo, the permanent-approval allowlist), _get_approval_mode() re-reads it live, and check_all_command_guards returns unconditionally once approval_mode == "off". A prompt-injected agent needs one terminal call to write approvals: {mode: off} with no prompt, after which every command in the session runs ungated.

The absolute spellings were never affected — _home_prefix_fold_regex joins components with [/\\]+, so /Users/alice//.hermes/config.yaml is caught today. That inconsistency is what surfaced the gap: this PR gives the ~ / $HOME / $HERMES_HOME forms the same tolerance.

Related Issue

No existing issue found. I searched open and merged PRs for approval/sensitive-path/separator topics before writing this; the nearest neighbours (#78653 Windows separator normalization in file-tools, #71919 Windows path command boundaries, #10682 OS-agnostic sensitive path protection) address different layers and none collapses redundant separators in tools/approval.py.

Type of Change

• 🔒 Security fix

Changes Made

• tools/approval.py — in _normalize_command_for_detection, collapse /./ runs and repeated / inside path tokens, immediately after the home folds and before the backslash-escape strip.
• tests/tools/test_approval.py — new TestRedundantPathSeparators covering doubled separators, /./ segments (including repeated ones), and a false-positive guard.

Both substitutions use a lookbehind requiring a path character ([\w~.\-]), so URL schemes (https://), UNC roots (\\server) and integer division (10 // 3) are left alone.

Placement is deliberate: it runs after _rewrite_resolved_hermes_home / _rewrite_resolved_user_home so it also normalizes the ~-form those produce, and before the backslash strip for the same Windows reason documented there.

How to Test

Before the fix:

from tools.approval import detect_dangerous_command
detect_dangerous_command("echo 'approvals: {mode: off}' > ~//.hermes/config.yaml")
# (False, None, None)   <- no approval prompt

After:

# (True, 'sensitive_redirect', 'overwrite system file via redirection')

Regression suite:

pytest tests/tools/test_approval.py tests/tools/test_approval_windows.py tests/tools/test_approval_deny_rules.py -q

165 passed on my machine. One unrelated pre-existing failure, TestDetectDangerousRm::test_nonrecursive_verification_artifact_cleanup_is_not_dangerous, reproduces identically on unmodified main here — it is a macOS artifact (/tmp is a symlink to /private/tmp, so the canonical-target check in the temp-dir exemption does not match the mocked gettempdir), not a consequence of this change.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix
• I've run the relevant test suites and all pass (modulo the pre-existing macOS failure noted above)
• I've added tests for my changes

[andrexibiza]

Reviewed exact head fc18b956780366e195f37df47dffc9cb855ebb57 against its actual parent/merge base 74f99af470ae8ce47f0903cf431d106cecbd37f2 and current main 5dd15872a6878a19b9b5478b6968b38f48dd311f. I checked both changed files, the full normalization order, every sensitive-path fragment and write verb that consumes it, the file-tools sibling boundary, exact-head Actions state, existing review/comment state, and the related path-hardening/provenance chain.

The underlying defect is real and the ~//... / ~/./... cases added here are correctly targeted, but the implementation still leaves the same approval-policy write bypass open through the brace forms the existing detector explicitly supports.

Blocking: ${HOME} / ${HERMES_HOME} spellings are not normalized

Both new substitutions require the character immediately before the redundant separator to match [\w~.\-]:

re.sub(r'(?<=[\w~.\-])(?:/\.)+/', '/', command)
re.sub(r'(?<=[\w~.\-])//+', '/', command)

That works for $HOME//... because the preceding character is E, but not for ${HOME}//... because the preceding character is }. The same applies to /./ and to ${HERMES_HOME}.

Concrete residual reproductions on this exact head:

echo 'approvals: {mode: off}' > ${HOME}//.hermes/config.yaml
echo 'approvals: {mode: off}' > ${HOME}/./.hermes/config.yaml
echo 'approvals: {mode: off}' > ${HERMES_HOME}//config.yaml
echo 'approvals: {mode: off}' > ${HERMES_HOME}/./config.yaml
cat key >> ${HOME}//.ssh/authorized_keys

After the new normalization and lowercase step, those remain respectively ${home}//..., ${home}/./..., and ${hermes_home}//.... _HERMES_CONFIG_PATH, _SSH_SENSITIVE_PATH, _SHELL_RC_FILES, and _CREDENTIAL_FILES deliberately include ${home} / ${hermes_home} alternatives, but each requires exactly one /; therefore the tee/redirection/in-place/copy patterns still do not match these shell-equivalent targets.

This is not a neighboring theoretical bypass: it is the same separator-canonicalization class, on the same terminal surface, against the same files, using syntax the current patterns claim to cover. In the config.yaml cases it preserves the exact mode-off chain described in the PR body.

Required fix: either include the closing brace in the accepted predecessor set if this lookbehind design is retained, or normalize recognized path tokens/root expressions rather than relying on a generic preceding-character heuristic. Please add focused regressions for both ${HOME} and ${HERMES_HOME}, with both // and /./, including at least the Hermes config and SSH/shell-rc families.

Secondary test-contract note

The comment says URLs are left alone, but only the scheme delimiter is protected. In https://example.com//api/v1, the second // is preceded by m, so the normalizer collapses that URL path. The current test only proves the resulting command is not classified as dangerous; it does not prove the stated non-rewrite property. Because this string is detection-only rather than executed, I do not treat that as a separate blocker, but either narrow the normalization to actual path tokens or reword/pin the intended contract directly against _normalize_command_for_detection.

Interlocks / ownership / provenance

• This is complementary to #78653 and #10682, which operate in file_tools path resolution, and to #71919, which preserves Windows executable-path boundaries in this same approval normalizer. It does not duplicate them.
• The protected config.yaml topology originates in #14639 by @Subway2023 and was salvaged into main with contributor credit in 4e9d886d9d9391d096093071cd79ece7e543f3e0; this PR is a legitimate continuation of that terminal-side pairing rather than replacement work.
• Per SECURITY.md, the approval gate is an in-process heuristic, not an OS isolation boundary. This is still worthwhile hardening, but it should be described as closing this spelling class, not the adversarial-shell class generally.
• Current main is five commits ahead of the PR's actual merge base, but none of that drift touches tools/approval.py or tests/tools/test_approval.py; there is no current semantic merge-order conflict. Rebase/current-base verification is still appropriate before merge.

Verification state

There were no prior reviews, issue comments, or review threads on this PR, so this is not duplicate review residue. Exact-head CI has not executed: CI, Docker, and Nix all currently conclude action_required, and the combined status endpoint exposes no completed contexts. The author's reported focused result is 165 passing tests with one baseline macOS failure; the brace-form cases above are not in that matrix.

Re-review gate: close the brace-form residual class, add the focused regressions, and obtain executed exact-head CI.

[andrexibiza]

Reviewed exact head fc18b956780366e195f37df47dffc9cb855ebb57 against the current main tip 5dd15872a6878a19b9b5478b6968b38f48dd311f (merge base 74f99af470ae8ce47f0903cf431d106cecbd37f2; this head is currently 5 commits behind / 1 ahead). I inspected the complete diff, the full _normalize_command_for_detection ordering, every shared sensitive-path fragment and its redirect/tee/copy/in-place consumers, the new tests, exact-head Actions state, existing review/comment history, and the adjacent file-tool, Windows-normalization, hardline, and profile-config work.

The reported defect is real, and the patch correctly catches the simple ~//... and ~/./... spellings covered by the new tests. Two remaining forms still resolve to the protected resources while passing through this normalizer unchanged or incompletely, so I do not think the security class is closed yet.

Blocking 1: braced $HOME / $HERMES_HOME forms remain bypasses

The protected-path fragments deliberately accept both unbraced and braced variables: $home / ${home} and $hermes_home / ${hermes_home}. But both new substitutions require the slash run to be preceded by (?:[\w~.\-]). In a braced expansion, the character immediately before the slash is }, so neither substitution runs.

Concrete residuals at this head:

echo 'approvals: {mode: off}' > ${HOME}//.hermes/config.yaml
echo x > ${HOME}//.hermes/.env
echo k >> ${HOME}//.ssh/authorized_keys
echo x >> ${HOME}//.bashrc
echo 'approvals: {mode: off}' > ${HERMES_HOME}//config.yaml

The shell expands those variables and POSIX resolves the redundant separators to the same files as the canonical spellings, but the detector still sees the doubled separator and the exact-one-slash fragments do not match. The first and last commands therefore preserve the full approval-policy rewrite path this PR is intended to close.

Required fix: make separator/dot-segment normalization understand the braced-variable token boundary rather than relying only on a preceding path character, and add regression cases for all four supported forms ($HOME, ${HOME}, $HERMES_HOME, ${HERMES_HOME}), including at least config, .env, SSH, and shell-rc targets.

Blocking 2: the two passes are not closed under composition

The /./ pass runs before the repeated-slash pass. A mixed alias can therefore cause pass 2 to create a fresh /./ segment after pass 1 has already finished:

~//./.ssh/authorized_keys   ->   ~/./.ssh/authorized_keys
~//./.bashrc                ->   ~/./.bashrc

Both final strings still miss the exact sensitive fragments. The same issue survives the absolute-home fold: /home/alice//./.ssh/authorized_keys folds to the ~//./... shape and then ends at ~/./....

Required fix: normalize in an order/fixed point that cannot expose a new no-op segment after its pass has run (for example, collapse repeated separators before collapsing /./ runs, with tests for mixed and repeated compositions). Please pin at least ~//./.ssh/authorized_keys, ~//./.bashrc, and an absolute-home //./ spelling.

Other side of the shape / interlocks

• .. aliases such as ~/existing-dir/../.ssh/authorized_keys remain an adjacent canonical-path bypass class. I would track that separately rather than silently claiming full path identity here, because safe treatment needs to account for shell tokenization and symlink semantics.
• #60523 is complementary hardline promotion, not a duplicate. It reuses _HERMES_CONFIG_PATH / _HERMES_ENV_PATH, so these normalization gaps would bypass that stronger floor too; this normalizer needs to be correct before that policy becomes meaningful for all spellings.
• #79089 is complementary resource coverage for profile config/env paths and inherits the same alias problem. Once rebased, its ${HOME}/.hermes/profiles/... forms need the corrected normalizer.
• #71919 is adjacent work in this exact normalization function for Windows executable-path boundaries. There is no semantic supersession, but whichever branch rebases second must preserve both ordering constraints.
• #78653 and #10682 are the file-tool/sink side, not duplicates of this terminal-command fix. #78653 itself explicitly preserves #76247 as the original overlapping Windows file-tool implementation.
• The current terminal write family carries provenance from the #14639 line and the merged #38998 salvage of #36894; this PR is a shared-normalizer extension across those existing consumers, not a replacement for their command-specific coverage.

Verification state

• No prior reviews, comments, or review threads existed on this exact head, so this is not duplicate review residue.
• Exact-head GitHub Actions have not executed: CI, Docker, and Nix are all action_required, and the combined status endpoint has no completed contexts. The author's focused local result is useful but is not an upstream exact-head CI receipt.
• Current-main drift does not touch either changed file, but this head is five commits behind and will need a new commit for the blockers anyway.

Re-review gate: braced-variable normalization; composition-safe // + /./ normalization; focused regressions proving the residual commands above are detected by the real detect_dangerous_command; and executed exact-head CI.