fix(security): redact embedded credentials from upstream URLs in logs and errors#299
Open
e-jung wants to merge 1 commit into
Open
fix(security): redact embedded credentials from upstream URLs in logs and errors#299e-jung wants to merge 1 commit into
e-jung wants to merge 1 commit into
Conversation
Upstream origin URLs carrying embedded credentials (e.g. https://x-access-token:ghp_...@github.com/o/r.git, common with fine-grained PATs and CI checkouts) were persisted verbatim to the SQLite repos.upstream_url column, the daemon log, and the world-readable (0o644) step logs — including inside `git push <url>` error strings. Step logs are append-mode, so a leaked token accumulated across runs and was readable by any local user. Add git.RedactURL, which strips the userinfo before the first '@' from any URL-looking string (leaving local paths, scp-like SSH URLs, refs, and flags untouched), and route every persisted / logged / errored use of the upstream URL through it: - gate.Init stores and logs the redacted URL (the bare gate's own "origin" remote still carries the full credentialled URL). - The push step and CI auto-fix push resolve the credentialled URL from the worktree's "origin" remote at run time (resolveUpstreamURL) so the credential still reaches the git push/ls-remote argv while only the redacted form is logged. - git.Run and stepGitRun redact URL-looking args and stderr in their error strings; the step-failure write to the step log redacts too. - The Bitbucket "resolve repo" error no longer echoes the raw URL. Also create daemon, step, CLI, and wizard log files with 0o600 so a secret that reaches a log through any path is not world-readable. Tests: RedactURL table + leak guard; git.Run error redaction; gate DB redaction (and bare-gate origin keeps the credential); credential still reaches the push argv (pushes keep working); push step log + error never leak the token; step log file mode is 0o600.
Owner
|
Thanks for the contribution, @e-jung! One process note on this repo: PRs need to be raised through no-mistakes ( If working from a fork was the blocker, that's fixed as of v1.30.0 (#306). Per CONTRIBUTING.md: point I won't be merging PRs that bypass the gate going forward, but I'd genuinely love to land your work once it's re-raised. Thanks for understanding! 🙏 |
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
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.
Summary
Redacts embedded credentials (e.g.
https://x-access-token:ghp_…@github.com/o/r.git, common with fine-grained PATs and CI checkouts) from every site where the upstream URL is persisted, logged, or wrapped into an error. The credential still reaches thegit push/ls-remoteargv so pushes keep working. Also tightens daemon / step / CLI / wizard log files to0o600.The bug
gate.Initcaptured the raworiginURL viagit.GetRemoteURLand stored / logged it verbatim. The URL flowed to:slog.Info("gate initialized", …, "upstream", upstreamURL)→ daemon.log (internal/gate/gate.go)repos.upstream_urlcolumn (same)sctx.Log(fmt.Sprintf("pushing to %s …", upstream, ref))→ the step log (internal/pipeline/steps/push.go)git.Pushpassing the URL as a bare arg →git.Run's error format"git %s: %w: %s"(joined args) → the step-failure write to the step log on rejection (internal/git/git.go,internal/pipeline/executor.go)Step log files were created
0o644and opened in append mode, so the token was world-readable and accumulated across runs. The existingintent/redact.gosecret patterns were never applied to URLs or log paths. The samegit.Run-style error format instepGitRun(internal/pipeline/steps/common_exec.go) and the Bitbucket "resolve repo" error (ci_bitbucket.go) echoed the raw URL too.Repro
After this PR each of those reads
https://***@github.com/o/r.git; the credential no longer appears anywhere persisted or logged.The fix
git.RedactURL(internal/git/git.go): a single regex-based helper that replaces thescheme://userinfo@prefix withscheme://***@. It operates on arbitrary text (so it's safe on argv joined into an error string or on git stderr), and leaves local paths, scp-like SSH URLs (git@host:path), refs, and flags untouched.Redact at every persisted / logged / errored site, keep the credential only for the argv:
gate.Initstores andslogs the redacted URL. The bare gate's ownoriginremote still carries the full credentialled URL (unchanged), so worktrees carved from it inherit it via git's shared common config — verified empirically.resolveUpstreamURL(sctx)(common_git.go) recovers the credentialled URL from the worktree'soriginremote at run time, falling back to the DB record for old gates / worktrees withoutorigin. The push step (push.go) and CI auto-fix push path (ci_fix.go) call it for the actualgit push/ls-remoteargv, while logging only the redacted form.git.RunandstepGitRunredact URL-looking args and stderr in their error strings; the executor's step-failure write to the step log redacts defensively too.resolveBitbucketRepoRef's error no longer echoes the raw URL.Log file mode
0o600(audit #23): daemon log (selfexec.go), step logs (executor.go), CLI log (main.go), and wizard agent log (wizard.go). A secret that reaches a log through any path (e.g. an agent dumpingenv/~/.netrc) is no longer world-readable.0o600has no group/other bits, so the result is independent of the process umask.Backward compatibility: old DB rows that still hold a full credentialled URL keep working —
resolveUpstreamURLfalls back to the recorded URL when a worktree has noorigin. The redaction is a no-op for local-path and scp-like upstreams (all existing tests and the e2e harness), so there is no behavior change for non-credentialled repos.gate.InitDB + sloghttps://***@…(bare gateoriginkeeps the credential)originviaresolveUpstreamURLpushing to https://token@…/r.gitpushing to https://***@…/r.gitgit.Run/stepGitRunerrorgit push https://token@…/r.git …: <stderr>0o644(world-readable, append)0o600Verification
gofmt -l .clean ·go vet ./...clean ·go run ./cmd/genskill --checkup to date ·go build ./...OK ·go test -race ./...green (Go 1.26.4 — system Go 1.18 is too old).New tests pinning the behavior (the defining symptom is a silent credential write, so these exist to catch regressions):
TestRedactURL/TestRedactURLNeverLeaksToken(internal/git/redact_test.go) — table-driven over PAT / bare-token / no-userinfo / scp-like / local-path / URL-in-text / multi-URL / GitLab shapes, plus a leak guard.TestRunRedactsURLInError(internal/git/redact_test.go) — a failing git command carrying a credentialled URL arg never leaks the token in the error.TestInitRedactsCredentialURL(internal/gate/gate_test.go) —Initstores/returns the redacted URL and the bare gate'soriginkeeps the full credential (pushes still authenticate).TestResolveUpstreamURL_PreservesCredential+ fallback tests (internal/pipeline/steps/upstream_test.go) — the credential still reaches the push argv (pushes keep working); old credentialled DB rows fall back correctly.TestPushStepRedactsCredentialURL(internal/pipeline/steps/upstream_test.go) — the push step's user-visible log and returned error never contain the token.TestExecutor_StepLogFileModeIsRestrictive(internal/pipeline/executor_logging_test.go) — step log files are0o600.The e2e harness uses local-path upstreams (redaction is a no-op there) and asserts no log-file modes, so recorded journeys are unaffected.
Notes for review
push.goandci_fix.go, which the open fork-routing PR (fix(git): correct fork→parent push target and PR base #296 /Repo.PushURL()) also touches. The two fixes are orthogonal (credential redaction vs. fork→parent routing); if both land the maintainer will reconcileresolveUpstreamURLwithRepo.PushURL()so the credentialled fork URL is the one recovered from git config. No coordination needed for this PR to be correct on currentmain.originconfig and the bare gate'soriginconfig (both under the user's home). This PR removes it from everything no-mistakes writes (DB, logs, errors). Rotating any PAT that may have leaked into a pre-existing0o644log before upgrading is recommended.AI disclosure: Human-reviewed. The change was produced with AI assistance and reviewed by a human contributor before submission.