fix: redact Semaphore API errors

[steipete]

Summary

• route every Semaphore non-2xx response through one client error helper
• redact the configured API token before storing/forming diagnostic text
• retain endpoint path, status code, and non-secret response context
• cover GET-with-headers, GET, POST, empty-secret handling, and real TLS reflection

Fixes #518

Verification

• go test -race ./internal/providers/semaphore
• go test -race ./internal/providers/semaphore -run 'TestAPIClient.*Redact' -count=1 -v
• go vet ./...
• go test -race ./...
• npm run format:check --prefix worker
• npm run lint --prefix worker
• npm run check --prefix worker
• npm test --prefix worker (646 tests)
• npm run build --prefix worker
• node scripts/build-docs-site.mjs
• repository autoreview: clean

Live proof

The focused integration starts a real loopback TLS server, verifies that the production client submitted Authorization: Token <dummy>, returns that header in a 401 response body, and asserts the resulting error contains Token [redacted] but not the dummy token:

=== RUN   TestAPIClientTLSRedactsReflectedToken
--- PASS: TestAPIClientTLSRedactsReflectedToken (0.01s)
PASS

The adjacent socket-free table executes every non-2xx construction path independently.

[clawsweeper]

Codex review: needs changes before merge. Reviewed June 21, 2026, 5:03 AM ET / 09:03 UTC.

Summary
The PR routes Semaphore non-2xx GET, GET-with-headers, and POST errors through a token-redacting helper, adds provider tests, updates Semaphore docs, and adds a changelog line.

Reproducibility: yes. source inspection gives a high-confidence path: a 2xx project-list response can provide a token-bearing next Link, and a later non-2xx getWithHeaders call formats that unchanged path through semaphoreAPIError.Error. I did not run tests because this review is read-only.

Review metrics: 3 noteworthy metrics.

• Production constructors: 3 routed through helper. All current Semaphore non-2xx body constructors now share the new redaction path.
• Stored error strings: 1 redacted, 1 unchanged. Body is redacted at construction time, but Path is still stored verbatim and formatted into the final error.
• Regression tests: 2 added. The tests prove body redaction through fake transport and TLS reflection but leave the response-controlled path case uncovered.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #518
Summary: This PR is the candidate fix for the open Semaphore API token diagnostic disclosure issue it references with closing syntax.

Members:

• canonical: [security] Semaphore API error bodies can disclose the configured provider token #518 - The issue describes the same Semaphore provider token disclosure in API error diagnostics and remains the canonical security-tracked item.

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦞 diamond lobster
Patch quality: 🧂 unranked krab
Result: blocked by patch quality or review findings.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

• Redact the stored Semaphore API error path with the same configured-token helper.
• [P2] Add a regression test where a token-bearing pagination Link drives a failing follow-up request and the final error omits the token.

Risk before merge

• [P1] If merged as-is, the linked issue may close while a response-controlled pagination Link path can still put the configured Semaphore token into user-visible diagnostics.

Maintainer options:

1. Redact path before merge (recommended)
   Apply the same configured-token redaction to the stored Semaphore API error path and cover the pagination Link reflection case before landing.
2. Accept body-only mitigation
   Maintainers could intentionally land the body redaction now, but should keep the linked security issue open for the remaining path diagnostic leak.

Copy recommended automerge instruction

@clawsweeper automerge

Special instructions:
Redact the Semaphore API error Path with the same configured-token helper before storing it, and add a regression test where a token-bearing pagination Link drives a failing follow-up request without leaking the token.

Next step before merge

• [P2] There is one narrow repairable blocker: redact the stored Semaphore API error path and add the missing pagination regression test.

Security
Needs attention: Needs attention: the diff narrows the body leak but leaves a concrete token leak through the stored Semaphore API error path.

Review findings

• [P2] Redact the stored response path too — internal/providers/semaphore/api.go:328

Review details

Best possible solution:

Redact the configured Semaphore token from both stored API error fields, then add a regression test for a token-bearing pagination Link followed by a failing request.

Do we have a high-confidence way to reproduce the issue?

Yes, source inspection gives a high-confidence path: a 2xx project-list response can provide a token-bearing next Link, and a later non-2xx getWithHeaders call formats that unchanged path through semaphoreAPIError.Error. I did not run tests because this review is read-only.

Is this the best way to solve the issue?

No, not yet. The provider-local helper is the right narrow shape, but it needs to redact the stored Path as well as Body before this is a complete fix for response-controlled Semaphore diagnostics.

Full review comments:

• [P2] Redact the stored response path too — internal/providers/semaphore/api.go:328
responseError now redacts only Body, but Path is still formatted by semaphoreAPIError.Error(). In resolveProjectID, that path can come from nextLinkPath parsing the API Link header; if a token-bearing next URL is followed by a non-2xx response, the configured token is still printed. Redact the path with the same helper and cover that pagination case.
  Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 15dd1dbb1d95.

Label changes

Label changes:

• add merge-risk: 🚨 security-boundary: The PR changes credential diagnostic handling but can still leave a configured Semaphore token visible through the stored error path.
• add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦞 diamond lobster and patch quality is 🧂 unranked krab.
• add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body includes terminal output for a live loopback TLS reflection test proving the body-redaction path, though the remaining path-redaction finding still needs a code fix.
• remove rating: 🐚 platinum hermit: Current PR rating is rating: 🧂 unranked krab, so this older rating label is no longer current.
• remove status: 👀 ready for maintainer look: Current PR status label is status: ⏳ waiting on author.

Label justifications:

• P2: This is a bounded provider-token diagnostic disclosure fix with a concrete remaining edge case and limited Semaphore-specific blast radius.
• merge-risk: 🚨 security-boundary: The PR changes credential diagnostic handling but can still leave a configured Semaphore token visible through the stored error path.
• rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦞 diamond lobster and patch quality is 🧂 unranked krab.
• status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body includes terminal output for a live loopback TLS reflection test proving the body-redaction path, though the remaining path-redaction finding still needs a code fix.
• proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes terminal output for a live loopback TLS reflection test proving the body-redaction path, though the remaining path-redaction finding still needs a code fix.

Evidence reviewed

Security concerns:

• [medium] Path redaction gap — internal/providers/semaphore/api.go:328
  A response-controlled pagination Link can become the next request path, and the new helper stores that path unchanged before it is formatted into CLI-visible diagnostics.
  Confidence: 0.88

Acceptance criteria:

• [P1] go test -race ./internal/providers/semaphore -run 'TestAPIClient.*Redact|TestResolveProjectID' -count=1.
• [P1] go test -race ./internal/providers/semaphore.

What I checked:

• Repository policy read: AGENTS.md was read in full; its provider-boundary and credential-handling guidance made the Semaphore diagnostic redaction path security-relevant for this review. (AGENTS.md:49, 15dd1dbb1d95)
• PR helper redacts only Body: At PR head, responseError stores Path unchanged while applying redactSemaphoreSecrets only to Body, and semaphoreAPIError.Error formats both fields into the final error. (internal/providers/semaphore/api.go:326, 1e246d5927c0)
• Pagination can make path response-controlled: resolveProjectID feeds the next request path from nextLinkPath, and nextLinkPath parses the API Link header before getWithHeaders later reports that path on non-2xx errors. (internal/providers/semaphore/api.go:271, 1e246d5927c0)
• Tests cover body reflection but not path reflection: The added tests cover GET-with-headers, GET, POST, empty-secret handling, and TLS body reflection, but they do not cover a token reflected through a pagination Link path. (internal/providers/semaphore/backend_test.go:231, 1e246d5927c0)
• Security docs require diagnostic redaction: The security guide says not to log environment values and to redact secret-looking strings in diagnostics, supporting the need to redact every diagnostic field that can contain the configured token. (docs/security.md:171, 15dd1dbb1d95)
• Canonical linked issue: The PR body uses closing syntax for [security] Semaphore API error bodies can disclose the configured provider token #518, which remains open and describes the same Semaphore token-disclosure problem.

Likely related people:

• loadez: GitHub commit metadata maps the original Semaphore provider commit to loadez, including the initial API client surface now being changed. (role: introduced behavior; confidence: medium; commits: 07619bc0206d; files: internal/providers/semaphore/api.go, internal/providers/semaphore/backend.go)
• vincentkoc: Recent Semaphore fixes and current blame for the API client area make vincentkoc a likely routing candidate for provider behavior review. (role: recent area contributor; confidence: high; commits: 341a4a047094, 3d412ee65d5e, 65037410323e; files: internal/providers/semaphore/api.go, internal/providers/semaphore/backend_test.go)
• steipete: Prior merged Semaphore hardening commits and current PR authorship show relevant recent area work beyond only proposing this branch. (role: recent hardening contributor; confidence: medium; commits: 840997883d73, a3e0e3eccd41, 1e246d5927c0; files: internal/providers/semaphore/api.go, internal/providers/semaphore/backend_test.go, docs/features/semaphore.md)

What the crustacean ranks mean

• 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
• 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
• 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
• 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
• 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
• 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
• 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1e246d5927

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Redact secrets before storing the error path

When resolving a project, path can come from the API's Link header via nextLinkPath; if a bad or reflective Semaphore response puts the configured token in that next URL and the follow-up request returns non-2xx, semaphoreAPIError.Error() prints this Path verbatim even though the body is redacted. Please run the same redaction over path before storing it so response-controlled pagination diagnostics cannot leak the token.

Useful? React with 👍 / 👎.

[steipete]

@clawsweeper re-review

Maintainer security review accepts the provider-local helper and coverage of all Semaphore non-2xx constructors; live TLS reflection proof is sufficient. The sole mechanical finding is declined: repository policy requires an Unreleased changelog entry for this maintainer-authored user-facing security fix.

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.