Skip to content

fix(ci): keep failure text after quoted auth headers and redact more secret forms - #2613

Merged
freeqaz merged 9 commits into
mainfrom
lows-1016n
Oct 16, 2026
Merged

freeqaz merged 9 commits into
mainfrom
lows-1016n

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Follow-ups from an audit of #2605 and #2607 (findings 1556-1559, all low):

  • 1556 (test(ci): bound the activation diagnostic and redact escaped JSON secrets #2605): the Bearer/Basic/Token shape matched 8+ non-space characters, so it also took the closing quote of a quoted header. The quoted-value scan then found no closing quote and redacted to the end of the whole text. redactLogLine/redactFailure on request failed {"authorization":"Bearer …"}\nexpected status 503, got 200 returned only request failed {"authorization":"[redacted]: the assertion lines were gone. Nothing leaked, but the job-log and results failure text lost what it was for.
  • 1559 (before test(ci): bound the activation diagnostic and redact escaped JSON secrets #2605): the redactor still published an object or array value after a credential key ({"credentials":{"key":"…"}}, {"tokens":["…"]}), every word after the first in password: my secret phrase, a Bearer value under 8 characters, and values after sig=, pwd=, passphrase= and accessKey=. Environment values were already replaced, so the exposure was fixture or Pod-log secrets.
  • 1557 (test(ci): bound the activation diagnostic and redact escaped JSON secrets #2605): withinDiagnosticDeadline raced the diagnostic against its deadline but never cancelled it. Past the deadline, kubectl reads kept running (up to 20 s x 2 attempts each), the startup diagnostic kept writing to stderr during teardown, and the open children kept the test process alive.
  • 1558 (fix(configuration): refuse a stored model baseUrl at deploy that saves refuse #2607): the new deploy-time refusal was noted only inside the archived 2026-10-13 entry. Operators read entries dated after the commit they run, so they would miss it.

Change

  • scripts/ci/failure-redaction.mjs
    • Auth-scheme values stop at a quote, backslash or closing bracket. A value shorter than 8 characters is redacted when it holds a digit, so Bearer token expired stays readable.
    • A quoted value stays private through its closing delimiter, including across newlines. After a properly closed Authorization value, the next line of the failure stays readable.
    • After a credential key, a value that opens with { or [ is redacted through its matching close, including nested or multiline values. A bracket inside a quoted string does not end redaction.
    • After password, passwd, passphrase, pwd or authorization and a colon, every word up to the end of the phrase (newline, quote, ,, ;, }, ], &) is redacted. = stays one word, as in logfmt and query strings.
    • New keys: passphrase, pwd, access[_-]?key, sig, signature.
    • The scanner works on bounded input. On the final correction, 100 local runs each over roughly 16 KiB of nested brackets, an unclosed quote, or repeated structured assignments took 14–58 ms in total; this is a smoke check, not a latency guarantee.
  • tests/helpers/kubernetes-real.mjs: withinDiagnosticDeadline(diagnostic) now takes a function and runs it with an AbortSignal held in AsyncLocalStorage. kubectlFor passes that signal to execFile, so a read still running at the deadline is killed and a later one is refused before kubectl starts. A nested diagnostic also stops at the outer deadline. diagnosticDeadlinePassed() lets a diagnostic stop writing.
  • tests/integration/sandbox-driver-openshell-k3d-real.test.mjs: the three callers pass functions, and the startup diagnostic writes nothing to stderr after its deadline.
  • docs/guides/deploy/breaking-changes.md: a 2026-10-16: stored model provider settings are checked at deploy entry. The two oldest 2026-10-13 notices move to the existing breaking-changes-archive.md to stay under the word cap (2337 / 2018 words by wc). No new page and no docs/docs.json change.

Verification

  • New and changed tests fail on main: a quoted auth header keeps the failure text after it, the redactor removes structured, multi-word and short secret values, and the two new deadline tests (main's helper takes a promise and has no cancellation).
  • Mutant: without the signal passed to execFile, a failure diagnostic's kubectl reads stop at its deadline fails (the fake 30 s kubectl holds the read past the test timeout).
  • The deadline test uses a fake stalled kubectl on PATH (no cluster). It checks that the read rejects with AbortError, that a second read is refused without starting kubectl, and that the process is gone within 10 s.
  • At 2 CPUs: tests/conformance/kubernetes-real-kubectl.test.mjs 20/20. The redaction tests in tests/integration/ci-runner.test.mjs 20/20. The whole ci-runner.test.mjs (53/53) and qa-fixture-inputs.test.mjs (24/24) pass.
  • Negative rows stay readable: Bearer token expired, token expired, secretName: occ-gateway-password, tokenCount: 3, design=compact, SyntaxError: Unexpected token '}'.
  • Prettier and ESLint are clean on the touched files. No new test files; both test files are already in suites (checks-baseline-1 and checks-baseline-2).
  • Open PRs touching these files: git merge-tree against this head shows the same conflicts as against main. test(openshell): prove model, tool and gateway outages fail closed #2393 has the same single 14-line hunk in the k3d test. chore(openshell): bump pinned release to 0.1.4 #2440, fix(ci): retain nested failure causes in diagnostics #2002 and Draft implementation: managed outbound HTTPS for dedicated Gateways #2518 merge cleanly. Credential Proxy Implementation #2487's conflicts are in migration files only.
  • Internal review: see the comment below.

Risks

  • Low. The CI redactor redacts more: a multi-word phrase after password: in prose (invalid password: too short, got 3 keeps only , got 3), any pwd= value including a PWD= path, and a Token/Bearer word followed by a short value with a digit. These are on the safe side.
  • If a quoted or structured credential never closes, or its brackets disagree, redaction intentionally hides the remainder of the bounded input. This does not claim to identify every credential or to reconstruct values already split across independent log records.
  • withinDiagnosticDeadline now takes a function instead of a promise. All callers are in this PR.

Final redaction-boundary correction (afb00802)

A CI failure message is not necessarily one line of JSON. Review with invented values caught two failure boundaries in the initial PR: a newline in an open quoted value, and a closing bracket inside quoted text in a structured value. The fix preserves following diagnostics when the value actually ends. It does not claim arbitrary log text is safe to publish.

  • The real runner artifact regression failed on b8290043 and passed after the correction; it checks both the short result and the full diagnostics artifact. Local owning results: all 53 CI-runner cases passed, and separately all 22 kubectl-helper cases passed. Earlier sparse-checkout runs lacked fixture files; the complete files above are the relevant results, not a retroactive claim those invocations passed.
  • Independent summary and detailed-report probes: three invented tails visible before, zero after; all six closed-value examples retained their following diagnostic. ESLint, Prettier and diff checks passed. No actual credential, production log, cluster or external service was used.
  • CI and independent review on this exact head are separate requirements. Earlier hosted greens are for the parent.

Internal review notes and merge (78b644f9, 7cbb7628)

The internal review of b8290043 asked for these changes. They are merged with afb00802's rules, which are kept as pushed: an unclosed value stays private through the end of the bounded input, and the bracket scan uses a typed close stack with string handling.

  • A multi-word phrase after password: or authorization: ends at the next credential key, so that key's quoted value is still redacted (password: x token="…" published it). Stopping there also keeps the phrase scan linear; an earlier draft took about 280 ms on 16 KiB of repeated password: .
  • Text glued to a bracket value's close (token: [a]…) belongs to the value.
  • An auth-scheme value may open with a quote, which is kept (curl -H "Authorization: Bearer '…'"). A quote, backslash or closing bracket ends the value only when no more of the value follows, so \/ in a Basic value, a)b and map[Authorization:[Basic …]] are all handled.
  • An aborted kubectl read is never retried, whatever kubectl wrote before the kill.
  • Verification on 7cbb7628, at 2 CPUs: ci-runner.test.mjs 53/53; the redaction tests, including the runner diagnostics-artifact case, 20/20; kubernetes-real-kubectl.test.mjs 20/20 (on 78b644f9, unchanged since); qa-fixture-inputs.test.mjs 24/24. Prettier and ESLint are clean.
  • Linear-time probe on the same head: 27 adversarial 16 KiB inputs each take 1.5 ms or less per entry point.
  • 7e99351f closes the remaining synthetic regression from that corpus: an escaped quote or adjacent closing delimiters within an auth-shaped log value no longer exposes the suffix. This concerns diagnostic text, not a claim that the string was a valid HTTP header or that a real credential was exposed. Actual closing delimiters and the failure on the following line stay readable.
  • On this final correction the complete CI-runner file passed 53/53 locally (none skipped), plus ESLint, Prettier and diff checks. An additional synthetic check exercised 216 combinations of interior delimiters. I also inspected the prior hosted artifact: it really contained 53 passing runner tests on the parent, but that suite did not yet include these new cases. On this head, CI Required passed. I downloaded and SHA-256-verified its baseline result: the test merge included 7e99351f, all 53 CI-runner cases passed, and the baseline lane passed 1,677 tests with none skipped. Independent review is still required; this is test evidence, not an approval or a claim arbitrary logs are safe to publish.

Sibling-field boundary (adc11b99)

The review thread on 7e99351f was right: {"authorization":"Bearer …","status":503} still lost the rest of the failure. The closing quote and comma joined the value, and the quoted scan then ran to the end. A delimiter run now joins the value only before token text. New rows for the JSON and node-inspect sibling-field forms, each followed by an assertion line, fail on 7e99351f and pass here. At 2 CPUs: ci-runner.test.mjs 53/53, redaction tests 20/20. On the synthetic main-vs-head corpus nothing that main redacted is published, and the worst 16 KiB input takes 1.8 ms.

Recorded CI failure output on the sibling-field correction

I ran the real scripts/ci/run-tests.mjs and reporter twice, changing only the checkout from 7e99351f to adc11b99. The input was an invented canary, not an environment value that would be automatically masked. Each run exited 1 intentionally, after exercising JSON and inspected-object sibling fields, an escaped quote, a multiline quoted value, and a structured value containing a closing brace.

Actual JSON-case failure text read from the results artifact (the second line is part of the input assertion):

7e99351f:
{"authorization":"[redacted]

adc11b99:
{"authorization":"[redacted]","status":503}
Expected HTTP 503; observed HTTP 200

The invented value was absent from terminal output and all five messages in both result files on both revisions. On the corrected revision, I also asserted the following diagnostic survived in every short result and every detailed message. The detailed report intentionally suppresses a whole credential-bearing line where appropriate. This is evidence about these failure shapes, not a guarantee arbitrary logs are safe to publish; no CI rerun or real secret was needed.

…secret forms

An auth-scheme value no longer takes a closing quote, backslash or bracket, and an
unclosed quoted value ends with its line, so a quoted Bearer header no longer takes
the assertion lines after it (finding 1556). Object and array values after a
credential key are redacted to their close on the same line, a passphrase or
Authorization header after a colon takes every word, short scheme values with a
digit are redacted, and pwd, passphrase, accessKey, sig and signature are keys
(finding 1559).
withinDiagnosticDeadline now takes a function and runs it with an AbortSignal
in async-local storage. kubectlFor passes that signal to execFile, so a read
still running at the deadline is killed and a later one is refused, and the
startup diagnostic stops writing to stderr once the deadline passed. A nested
diagnostic also stops at the outer deadline (finding 1557). The unit test
uses a fake stalled kubectl on PATH; no cluster.
#2607 noted the deploy refusal only inside the archived 2026-10-13 entry, so
operators who read entries dated after their commit missed it (finding 1558).
Add a 2026-10-16 entry and move the two oldest notices to the existing archive
page to stay under the word cap.
execFile rejects an aborted read when it sends the kill, before the child is
reaped, so 3 of 20 runs saw the pid still alive. Poll until it is gone, within
10 s of the 30 s stall.
@freeqaz
freeqaz requested a review from a team as a code owner October 16, 2026 16:40
@clawsweeper

clawsweeper Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review in progress

ClawSweeper is reviewing this revision. This supersedes any previous blocked status.

View the workflow run.

@freeqaz-openai

freeqaz-openai commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

I found and fixed a redaction boundary regression in this PR, using only invented values. On b8290043, our real CI-runner regression could see the tail of a quoted value after a newline; main had hidden it. The structured scanner also mistook a bracket inside quoted text for the end of a value. afb00802 fixes both, while retaining the original improvement: the failure text after a complete Authorization header stays visible.

All 53 owning runner tests passed locally, including the result/diagnostics artifact check; all 22 kubectl helper tests passed separately. The PR description now describes the actual boundary and limitations. Please review the new head; my work is not an independent approval, and the earlier CI result does not cover this commit. No actual credential disclosure is inferred.

Follow-up: the author’s 7cbb7628 retained the multiline fix and called out an escaped-delimiter case in logged text. I reproduced that with invented input, then fixed it in 7e99351f: adjacent delimiters within a value stay redacted, actual closing punctuation and the following diagnosis stay visible. The new regression failed on the parent, and the corrected complete runner file passed 53/53 locally. Please review the final head, not the parent’s green checks; this is not my own approving review.

- A multi-word phrase stops at the next credential key, so that key's quoted
  value is still redacted (password: x token="..." published it). Stopping
  there also keeps the scan linear.
- A bracket value ignores brackets inside strings (plain and serialized
  JSON), and takes text glued to its close.
- An auth-scheme value may open with a quote, which is kept, and may hold an
  escaped "/".
- An aborted kubectl read is never retried.
Keeps the pushed privacy-first rules (an unclosed quoted value and a bracket
value run past newlines; a mismatched bracket suppresses the rest) with the
review notes: the bracket scan uses the typed close stack with string
handling from the pushed change, the next key still ends a phrase, text glued
to a close joins the value, and an auth-scheme value keeps a quote, backslash
or closing bracket that more of the value follows.
@freeqaz

freeqaz commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator Author

Free's clanker here. Internal review summary for this PR (reviewed head b8290043; fixes in 78b644f9, merged with afb00802 in 7cbb7628):

  • Medium, fixed: a multi-word password: phrase ran past a later key and hid its quoted value. The phrase now stops at the next credential key.
  • Medium, fixed: the bracket rule ended at a }/] inside a quoted string. Superseded by afb00802's string-aware typed close stack, plus text glued to the close.
  • Low, fixed: a quote right after the auth scheme (Bearer 'x') beat the shape. Values like \/ and a)b were cut short.
  • Low, accepted by afb00802's design: multi-line quoted values. An unclosed value now stays private to the end of the input.
  • Nits: an aborted kubectl read is no longer retried (fixed). Over-redaction of some prose (Token 1 of 3, authorization: forbidden ...) is on the safe side and accepted.
  • Checked fine: linear time (27 adversarial 16 KiB inputs, at most 1.5 ms each), deadline cancellation, nested deadlines, no leftover processes, all callers updated. The 2026-10-16 docs entry matches the deploy path in fix(configuration): refuse a stored model baseUrl at deploy that saves refuse #2607.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 16, 2026
Comment thread scripts/ci/failure-redaction.mjs Outdated
… field

A delimiter run joined the value whenever any value character followed, so
the closing quote in {"authorization":"Bearer ...","status":503} was taken
with the comma, and the quoted-value scan then hid the rest of the failure.
A run now joins the value only before token text (letters, digits, ._~+/=-),
which keeps escaped slashes, a)b and log-escaped quotes inside it.
@freeqaz
freeqaz temporarily deployed to integration-qa-pr October 16, 2026 17:39 — with GitHub Actions Inactive
@freeqaz
freeqaz temporarily deployed to integration-qa-pr October 16, 2026 17:39 — with GitHub Actions Inactive
@freeqaz
freeqaz temporarily deployed to integration-qa-pr October 16, 2026 17:39 — with GitHub Actions Inactive
@freeqaz-openai
freeqaz-openai requested a review from a team October 16, 2026 17:41
@clawsweeper clawsweeper Bot removed the clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. label Oct 16, 2026
@freeqaz
freeqaz merged commit 7b238b2 into main Oct 16, 2026
50 checks passed
@freeqaz
freeqaz deleted the lows-1016n branch October 16, 2026 17:52
@freeqaz-openai

Copy link
Copy Markdown
Collaborator

Closing the review record: the Claw status comment still says “review in progress,” but that job is terminal. Its final status was codex_or_content_failure, review exit 1; completion and publication were skipped, and the run lists zero review artifacts. This means no final Claw verdict from that run, not a pass, a finding against the change, or evidence that sensitive input was present.

GitHub records head adc11b99 merged as 7b238b24 at 17:52:48 UTC; the job ended at 17:53:10. That ordering does not establish why the review failed. The author’s posted source review and the tests above remain separate evidence. I haven’t rerun the bot or changed the closed PR.

This branch was previously deployed

1 inactive deployment
integration-qa-pr — adc11b99 Deployed Oct 16, 2026 by freeqaz via QA advisory (OpenShell) #1462
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants