fix(core): stop leaking secrets into debug output (GHSA-28pg-v3hp-9g7f) - #383
Merged
Merged
Conversation
`DEBUG=@argos-ci/core` is the documented way to diagnose an upload, and its output is pasted into public issues and written to CI logs. It printed the whole `process.env` snapshot while detecting the CI environment — `GITHUB_TOKEN`, the OIDC request token and any secret a project defines itself — while the upload path printed its parameters and the resolved config, both carrying the `ARGOS_TOKEN` repository token. Redact in the logger rather than at each call site: every argument is copied with its credential-looking properties replaced before it reaches the debug package, so no caller has to remember which of its fields is a secret. The environment snapshot keeps the values of the variables CI detection reads, minus the credentials among them, and reduces every other variable to its name — which is what a "why wasn't my CI detected?" report needs, and leaks nothing the project defined itself. The hand-written token stripping in `deploy` and `uploadMedia` is the logger's job now, so it goes away. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
🟡 Changes recommended
Prefix-based environment allowlisting can still expose arbitrary secrets, and object getters can make debug logging alter runtime behavior.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Centralizes debug-log credential redaction and sanitizes CI environment output.
Changes:
- Adds recursive secret and environment redaction.
- Applies redaction centrally in the debug logger.
- Adds regression tests and removes caller-side token stripping.
File summaries
| File | Description |
|---|---|
packages/core/src/redact.ts |
Implements redaction helpers. |
packages/core/src/redact.test.ts |
Tests redaction behavior. |
packages/core/src/debug.ts |
Redacts enabled debug output. |
packages/core/src/debug.test.ts |
Verifies logger-level token removal. |
packages/core/src/ci-environment/index.ts |
Sanitizes environment diagnostics. |
packages/core/src/deploy.ts |
Delegates parameter redaction to logger. |
packages/core/src/media.ts |
Delegates parameter redaction to logger. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Allowlisting CI variables by prefix still printed whatever a project names `GITHUB_*`, `CI_*` or `ARGOS_*` itself, secrets included. Rather than maintain an exact list of the variables the detectors read, drop the environment from the debug log altogether: when a service matches, the next line already logs the resolved CI environment — commit, branch, repository, pull request — which is what the output is read for. A "no CI service matched" line takes over what the dump was worth when nothing matched. `redactSecrets` stays, covering the other sink: the upload parameters and the resolved config, both carrying `ARGOS_TOKEN`. It now walks property descriptors instead of `Object.entries`, so turning the debug flag on never calls a caller's getter — `util.inspect` renders an accessor as `[Getter]` without invoking it, and a getter that throws no longer takes the upload down with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hing With the environment no longer logged, the only credential left reaching the debug output is the Argos token, and it gets there through three flat objects — not enough to justify a logger that walks every argument it is handed. Drop `redact.ts` and strip the token where it is logged, the way `deploy` and `uploadMedia` already did. The token is still worth seeing: "is that the token I think it is?" is a real question when an upload lands on the wrong project. `resolveArgosToken` now logs its first six characters — enough to recognize it, useless to anyone reading a public CI log. That is also the one place that knows which token the command ends up using, whether it came from the parameters or from the environment, so it is logged once there instead of being repeated by every object carrying it. Add a regression test driving the advisory's own PoC through `upload()`: the token never appears in the debug output, the masked prefix does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gregberge
changed the title
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.
Fixes GHSA-28pg-v3hp-9g7f.
DEBUG=@argos-ci/coreis the documented way to diagnose an upload, and its output is pasted into public issues and written to CI logs. Two sinks printed live credentials:getCiEnvironment()logged the wholeprocess.envsnapshot —GITHUB_TOKEN, the OIDC request token, and any secret a project defines itself;upload()logged its parameters and the resolved config, both carrying theARGOS_TOKENrepository token.Fix
The environment is no longer logged.
getCiEnvironment()logs that detection is running, andNo CI service matchedwhen it comes up empty; when a service does match, the next line already logs the resolvedciEnvironment— commit, branch, repository, pull request — which is what the output is read for. Nothing to allowlist, nothing to leak.The token is masked, and logged once.
resolveArgosToken()logs its first six characters (Authenticated with ARGOS_TOKEN (92d832…)) — enough to answer "is that the token I think it is?" when an upload lands on the wrong project, useless to anyone reading a public CI log. That is the one place that knows which token the command ends up using, whether it came from the parameters or the environment, so it is logged there instead of being repeated by every object that carries it:upload()strips it from the parameters and from the resolved config, the waydeploy()anduploadMedia()already did.Tests
src/upload.test.tsdrives the advisory's own PoC through the realupload(): with the namespace enabled, the token never appears in what the logger writes to stderr, and the masked prefix does. It fails onmain.🤖 Generated with Claude Code