hooks/tests: the credential clone-URL guard test failed once with no hook change #501

New issue

Open

opened 2026-09-15 21:07:59 +00:00 by hexajon · 2 comments

hexajon commented

2026-09-15 21:07:59 +00:00

Owner

What happens

CI run 203 (b924051f) failed the hook-tests step:

FAIL [exp=block got=allow rc=0] clone cred url with | in password

The same test passed on run 212 (4c6f88cc). Between those commits only README.md and two files in docs/decisions/ changed, and nothing under hooks/ or scripts/ changed. So the hook or the test gives different results on the same code.

This guard stops a clone URL that carries a credential. A guard that sometimes allows is a security gap, and a flaky test hides it.

Ask

Find why the case clone cred url with | in password can return allow.
Fix the hook or the test so the result is the same on every run.
Prove it with repeated runs of the hook test suite.

Acceptance

The case blocks on every run.
The fix names the cause.

## What happens CI run 203 (b924051f) failed the `hook-tests` step: ``` FAIL [exp=block got=allow rc=0] clone cred url with | in password ``` The same test passed on run 212 (4c6f88cc). Between those commits only `README.md` and two files in `docs/decisions/` changed, and nothing under `hooks/` or `scripts/` changed. So the hook or the test gives different results on the same code. This guard stops a clone URL that carries a credential. A guard that sometimes allows is a security gap, and a flaky test hides it. ## Ask - Find why the case `clone cred url with | in password` can return allow. - Fix the hook or the test so the result is the same on every run. - Prove it with repeated runs of the hook test suite. ## Acceptance - The case blocks on every run. - The fix names the cause.

hexajon added the

bug

testing

security

labels

2026-09-15 21:07:59 +00:00

hexajon commented

2026-09-15 21:44:41 +00:00

Author

Owner

A second case, in a different hook. CI run 216 (push, 05eb2ac9) failed hook-tests in hooks/tests/check-git-commit.test.sh:

FAIL [coexist rc=0 commit=expblock/gotblock local=expblock/gotallow remote=expallow/gotallow] feature add && commit (both block their own surface)
      cmd=git\ add\ -A\ \&\&\ git\ commit\ -m\ \"wip\"

check-git-local.sh exited 0 with no deny for git add -A. The local full gate on the same commit passed hook-tests.

Both failures (run 203 and run 216) ran while the PR run of the same commit was still running on the runner (201: 20:47:56-20:55:41 against 203: 20:49:14-20:52:40; 214: from 21:27:13 against 216: 21:28:31-21:31:56). Most push runs overlap their PR run and pass, so overlap alone does not explain it.

One candidate to test, not yet proven: the hooks run under set -o pipefail and classify each form with echo "$stripped" | grep -qP .... If that pipeline ever returns non-zero on timing, the form stays empty and the hook falls through to allow.

A second case, in a different hook. CI run 216 (push, 05eb2ac9) failed `hook-tests` in `hooks/tests/check-git-commit.test.sh`: ``` FAIL [coexist rc=0 commit=expblock/gotblock local=expblock/gotallow remote=expallow/gotallow] feature add && commit (both block their own surface) cmd=git\ add\ -A\ \&\&\ git\ commit\ -m\ \"wip\" ``` `check-git-local.sh` exited 0 with no deny for `git add -A`. The local full gate on the same commit passed `hook-tests`. Both failures (run 203 and run 216) ran while the PR run of the same commit was still running on the runner (201: 20:47:56-20:55:41 against 203: 20:49:14-20:52:40; 214: from 21:27:13 against 216: 21:28:31-21:31:56). Most push runs overlap their PR run and pass, so overlap alone does not explain it. One candidate to test, not yet proven: the hooks run under `set -o pipefail` and classify each form with `echo "$stripped" | grep -qP ...`. If that pipeline ever returns non-zero on timing, the form stays empty and the hook falls through to allow.

hexajon commented

2026-09-24 14:35:30 +00:00

Author

Owner

Parked for now. What I tested, so the next attempt does not repeat it:

The pipefail candidate. If grep -q exits before echo finishes writing, echo gets SIGPIPE and the pipeline fails with 141. That would turn a match into "allow", both in the hooks and in the test's run_in, which does echo "$out" | grep -q '"decision": "block"' && got="block". I ran the exact pattern on a 650-byte block payload 45,000 times, idle and with 8 copies in parallel:
```
set -o pipefail
echo "$out" | grep -q '"decision": "block"' || miss=$((miss+1))
```
0 misses. A payload this small goes out in one write, before grep can read it, so SIGPIPE cannot happen here. This does not explain the failure.
A direct reproduction. I ran hooks/tests/check-git-remote.test.sh 400 times, 8 suites at a time on one host: 66,000 case results, 0 failures. clone cred url with | in password blocked on all 400 runs.
The hook scripts have no timeouts, clock reads, or shared temp files.

One difference is still untested: CI runs the suite inside its container image, so the bash, grep, and coreutils versions there may differ. The next step is the same repeated-load run inside that digest-pinned image, or the full logs of runs 203 and 216.

Parked for now. What I tested, so the next attempt does not repeat it: 1. The `pipefail` candidate. If `grep -q` exits before `echo` finishes writing, `echo` gets SIGPIPE and the pipeline fails with 141. That would turn a match into "allow", both in the hooks and in the test's `run_in`, which does `echo "$out" | grep -q '"decision": "block"' && got="block"`. I ran the exact pattern on a 650-byte block payload 45,000 times, idle and with 8 copies in parallel: ```bash set -o pipefail echo "$out" | grep -q '"decision": "block"' || miss=$((miss+1)) ``` 0 misses. A payload this small goes out in one `write`, before `grep` can read it, so SIGPIPE cannot happen here. This does not explain the failure. 2. A direct reproduction. I ran `hooks/tests/check-git-remote.test.sh` 400 times, 8 suites at a time on one host: 66,000 case results, 0 failures. `clone cred url with | in password` blocked on all 400 runs. 3. The hook scripts have no timeouts, clock reads, or shared temp files. One difference is still untested: CI runs the suite inside its container image, so the bash, grep, and coreutils versions there may differ. The next step is the same repeated-load run inside that digest-pinned image, or the full logs of runs 203 and 216.

hexajon referenced this issue

2026-10-13 22:23:04 +00:00

epic: code and supply-chain security hardening #759

hexajon added a new dependency

2026-10-13 22:23:14 +00:00

#759 epic: code and supply-chain security hardening