Code review batch 2: credential redaction, redirect/setup hardening, integration build tag#151
Merged
Conversation
…d redirects from leaking credentials Continues the code-review fixes (#124, #125, #134). #134 — the live integration suite now requires the `integration` build tag in addition to an auth env var, so `go test ./...` (including the GoReleaser pre-release hook) can no longer fire live CRUD when SYLLABLE_API_KEY happens to be set. spec-live-check.yml runs it with -tags integration; CONTRIBUTING updated. #124 — `--debug` now redacts sensitive keys (auth_values, password, token, *secret, hmac, …) in request bodies and form fields. The masked API-key header gave a false sense that --debug output was safe to paste; service credentials no longer print in cleartext. #125 — the client drops the Syllable-API-Key header on a cross-host redirect (Go's default policy only strips standard auth headers, not custom ones) and warns when a non-loopback base URL uses plaintext http. All http.Client instances share this redirect policy via newHTTPClient. New unit tests cover redaction, cross-host redirect stripping, and the insecure-URL warning; verified end-to-end that --debug prints "auth_values":"***REDACTED***". go build/vet/test -race green. Co-Authored-By: Claude Fable 5 <[email protected]>
…gin checks (#123) The browser-based `syllable setup` server bound to loopback but its /save handler authenticated nothing, so a malicious page open in the user's browser could POST a poisoned config (e.g. a base_url pointing at an attacker) and exfiltrate the API key on the next command. /save now requires: - a per-session random CSRF token (crypto/rand) embedded in the served page and returned as X-Syllable-CSRF — a cross-origin page can't read it, and the custom header forces a CORS preflight the server never approves; - a loopback Host header (defeats DNS rebinding); - a matching Origin when the browser supplies one. Verified end-to-end with an isolated HOME: requests with no token, a cross-origin Origin, or a rebound Host are rejected 403, while the legitimate same-origin flow still saves. New unit tests cover the guard, token generation, and page injection. go build/vet/test -race green. Co-Authored-By: Claude Fable 5 <[email protected]>
…#129) A second "Save & Exit" (double-click or browser retry) spawned a second goroutine that closed the already-closed `done` channel, panicking the process. Guard the close with sync.Once, and serialize the /save handler with a mutex so concurrent submits can't race on the shared real/masked config. go build/vet/test -race green. Co-Authored-By: Claude Fable 5 <[email protected]>
This was referenced Jun 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second implementation batch from the code review — credential-safety, test-safety, and setup-server hardening. 5 issues, three commits, all covered by new unit tests and verified end-to-end where noted.
go build,go vet, andgo test -raceare green.Credential safety
--debugnow redacts sensitive keys (auth_values,password,token,*secret,hmac, …) in request bodies and form fields. The masked API-key header had given a false sense that--debugoutput was safe to share; service credentials no longer print in cleartext. Verified end-to-end: the body prints"auth_values":"***REDACTED***"and the real secret never appears.Syllable-API-Keyheader on a cross-host redirect (Go's default policy only strips standard auth headers, not custom ones), and warns when a non-loopback base URL uses plaintexthttp. Allhttp.Clientinstances share this policy vianewHTTPClient.Setup-server hardening
syllable setupserver's/saveendpoint authenticated nothing, so a page open in the user's browser could POST a poisoned config and exfiltrate the key on the next command. It now requires a per-session CSRF token (returned asX-Syllable-CSRF), a loopbackHost, and a matchingOrigin. Verified end-to-end with an isolatedHOME: no-token / cross-origin / rebound-Host requests are rejected403; the legitimate flow still saves.sync.Once, and/saveis now serialized with a mutex so concurrent submits can't race on the shared config.Test safety
integrationbuild tag in addition to an auth env var, so a plaingo test ./...— including the GoReleaser pre-release hook — can no longer fire live CRUD whenSYLLABLE_API_KEYhappens to be set.spec-live-check.ymlruns it with-tags integration; CONTRIBUTING updated.New unit tests: body/field redaction, cross-host redirect stripping, insecure-URL warning, the setup CSRF guard, token generation, and page-token injection.
Closes #123, #124, #125, #129, #134.
🤖 Generated with Claude Code