Auto-redact high-entropy env var values and support forced redaction#497
Auto-redact high-entropy env var values and support forced redaction#497maschwenk wants to merge 11 commits intocloudflare:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 152f79b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 execStream leaks sensitive command in yielded start and complete events
The PR adds sensitive to ExecOptions (session.ts:171) and correctly redacts the command in exec's return value (session.ts:390-394) and in both exec/execStream log messages (session.ts:329-333, session.ts:449-453). However, execStream yields the raw command in its start event (line 498) and complete event (line 645) without checking options?.sensitive. Any caller using execStream with sensitive set would have the command leaked through these events, defeating the redaction feature.
Affected yield sites in execStream
Line 495-500 (start event):
yield {
type: 'start',
timestamp: new Date().toISOString(),
command, // raw, not redacted
pid
};Line 636-649 (complete event):
yield {
type: 'complete',
exitCode,
timestamp: new Date().toISOString(),
result: {
...
command, // raw, not redacted
...
}
};(Refers to lines 495-500)
Prompt for agents
In packages/sandbox-container/src/session.ts, the execStream method needs to redact the command in two yielded events, matching the pattern already used in exec (lines 390-394):
1. At lines 495-500, the start event should redact the command:
Replace `command,` with the same ternary used in exec:
command: options?.sensitive
? options.sensitive === 'redacted'
? '[REDACTED]'
: '[AUTO-REDACTED]'
: command,
2. At lines 636-649, the complete event's result.command should also be redacted:
Replace `command,` (line 645) with the same ternary:
command: options?.sensitive
? options.sensitive === 'redacted'
? '[REDACTED]'
: '[AUTO-REDACTED]'
: command,
Was this helpful? React with 👍 or 👎 to provide feedback.
commit: |
🐳 Docker Images Published
Usage: FROM cloudflare/sandbox:0.0.0-pr-497-152f79bVersion: 📦 Standalone BinaryFor arbitrary Dockerfiles: COPY --from=cloudflare/sandbox:0.0.0-pr-497-152f79b /container-server/sandbox /sandbox
ENTRYPOINT ["/sandbox"]Download via GitHub CLI: gh run download 23453714355 -n sandbox-binaryExtract from Docker: docker run --rm cloudflare/sandbox:0.0.0-pr-497-152f79b cat /container-server/sandbox > sandbox && chmod +x sandbox |
sandy-bonk
Bot
removed
the
ok-to-test
|
New commits pushed -- |
maschwenk
changed the title
maschwenk
changed the title
scuffi
left a comment
scuffi
left a comment
There was a problem hiding this comment.
I like the configurable approach so far! just a few comments/suggestions
sandy-bonk
Bot
removed
the
ok-to-test
|
New commits pushed -- |
| cwd?: string; | ||
| env?: Record<string, string | undefined>; | ||
| timeoutMs?: number; | ||
| redact?: RedactionMode; |
There was a problem hiding this comment.
Both this exec and the one above are public facing - can these follow the same boolean schema?
|
/bonk review this PR |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good idea to add automatic entropy-based redaction for env var values. The Shannon entropy approach is reasonable and the code is generally clean. A few issues to flag:
Bug: execStream leaks raw commands in yielded events
The exec method correctly redacts the command in its return value (line 383) and logs (line 329), but execStream yields the raw command in two places that are NOT redacted:
startevent atsession.ts:486—commandis emitted as-iscompleteevent atsession.ts:633—result.commandis emitted as-is
Any caller consuming the stream with sensitive/redact set would see the plaintext secret value in these events, defeating the redaction. Both should use the same getRedactionLabel(options?.redact) ?? command pattern already used in the exec path.
(Devin flagged this too — confirming it's a real issue.)
Missing changeset
This modifies the public API surface (setEnvVars signature on ISandbox and ExecutionSession) so it needs a changeset targeting @cloudflare/sandbox. Per project conventions, only reference @cloudflare/sandbox in the changeset (not internal packages).
| expect(isHighEntropy(value, entropy - 0.1)).toBe(true); | ||
| expect(isHighEntropy(value, entropy + 0.1)).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The tests cover calculateEntropy and isHighEntropy well, but shouldRedact and getRedactionLabel — the primary functions consumed by the rest of the codebase — have no unit tests. They contain branching logic worth covering:
shouldRedact(true, ...)→'forced'regardless of valueshouldRedact(false, ...)→undefinedregardless of valueshouldRedact(undefined, highEntropyValue)→'auto'shouldRedact(undefined, lowEntropyValue)→undefinedgetRedactionLabel('forced')→'[REDACTED]'getRedactionLabel('auto')→'[AUTO-REDACTED]'getRedactionLabel(undefined)→undefined
| setEnvVars(envVars: Record<string, string | undefined>): Promise<void>; | ||
| setEnvVars( | ||
| envVars: Record<string, string | undefined>, | ||
| options?: { /** @defaultValue 'auto' */ redact?: boolean } |
There was a problem hiding this comment.
The @defaultValue 'auto' annotation is misleading. The default value of redact is undefined (the parameter is optional and typed as boolean). The behavior when omitted is auto-detection via entropy, but 'auto' is a RedactionMode string, not a boolean — this could confuse consumers.
Consider something like:
| options?: { /** @defaultValue 'auto' */ redact?: boolean } | |
| options?: { /** When omitted, values are auto-redacted based on entropy. Set `true` to redact all, `false` to skip redaction entirely. */ redact?: boolean } |
| setEnvVars(envVars: Record<string, string | undefined>): Promise<void>; | ||
| setEnvVars( | ||
| envVars: Record<string, string | undefined>, | ||
| options?: { /** @defaultValue 'auto' */ redact?: boolean } |
There was a problem hiding this comment.
Same @defaultValue issue as the ExecutionSession interface above.
| options?: { /** @defaultValue 'auto' */ redact?: boolean } | |
| options?: { /** When omitted, values are auto-redacted based on entropy. Set `true` to redact all, `false` to skip redaction entirely. */ redact?: boolean } |
| for (const [key, value] of Object.entries(toSet)) { | ||
| const exportCommand = `export ${key}=${shellEscape(value)}`; | ||
| const result = await exec(exportCommand); | ||
| const mode = shouldRedact(options?.redact, value); |
There was a problem hiding this comment.
getRedactionLabel(mode) is called twice in the ternary below (lines 959-960). Since it's a pure function this is correct but reads awkwardly. Consider extracting once:
const label = getRedactionLabel(mode);
...
command: label ? `export ${key}=${label}` : exportCommand,| { redact: true } | ||
| ); | ||
|
|
||
| expect(result.success).toBe(true); |
There was a problem hiding this comment.
This test only asserts result.success — it doesn't verify any redaction behavior. It doesn't check that the command logged or returned is actually redacted, and it only tests redact: true (forced), not the auto-detection or redact: false paths.
Consider at minimum spying on the session's exec to verify the redact option is threaded through with the expected RedactionMode value.
| timeoutMs?: number; | ||
| env?: Record<string, string | undefined>; | ||
| cwd?: string; | ||
| redact?: RedactionMode; |
There was a problem hiding this comment.
Worth a brief doc comment here to clarify that redact is client-side only — the raw command is still sent to the container over HTTP (which is the correct design). Without this, someone might assume the container never sees the plaintext.
| redact?: RedactionMode; | |
| /** Controls log/callback redaction only; the command is still sent to the container unredacted. */ | |
| redact?: RedactionMode; |
| @@ -434,7 +438,7 @@ export class Session { | |||
| sessionId: this.id, | |||
There was a problem hiding this comment.
The log message here is redacted correctly, but note that the yielded start event (line 486) and complete event (line 633) in this same method still emit the raw command without redaction — see the review-level comment for details.
|
The review was submitted successfully. Here's a summary of what I flagged: Review posted on PR #497 with 7 inline comments and a top-level summary. Key findings:
|
sandy-bonk
Bot
removed
the
ok-to-test
|
New commits pushed -- |
|
/bonk review this pr |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
sandy-bonk
Bot
removed
the
ok-to-test
|
New commits pushed -- |
Add Shannon entropy-based detection to identify likely secrets (API keys, tokens) when setting environment variables, and redact them from logs, error details, and HTTP responses across both the SDK client and container runtime. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove the generic `sensitive` flag from ExecuteRequest and the HTTP execute path. Redaction now only applies within the env-var-specific code paths (session-manager setEnvVars and session.exec). Use [AUTO-REDACTED] instead of [REDACTED] so callers can distinguish entropy-detected redaction from explicit sensitive marking. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Allow callers to explicitly mark env var keys as sensitive via sensitiveKeys option. Explicit keys are logged as [REDACTED] while entropy-detected ones use [AUTO-REDACTED], making it clear whether redaction was caller-requested or automatic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace sensitiveKeys array with a simple { sensitive?: boolean } option.
When true, all values are logged as [REDACTED]. When false/omitted,
entropy auto-detection still applies with [AUTO-REDACTED].
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sandy-bonk
Bot
removed
the
ok-to-test
|
New commits pushed -- |
sandy-bonk
Bot
removed
the
ok-to-test
|
New commits pushed -- |
|
/bonk review this PR |
There was a problem hiding this comment.
Review
Nice feature — the Shannon entropy approach for auto-redacting likely secrets in setEnvVars logs is sensible, and the code is clean. The recent revisions addressed most earlier feedback (changeset added, shouldRedact/getRedactionLabel tests added, error details redacted, stream events intentionally left raw with test coverage documenting that decision, callback behavior documented with comments). A few remaining items:
Container-side setEnvVars is unreachable in production
SessionManager.setEnvVars() is defined and fully implemented with redaction logic, but no handler exposes it over HTTP. The SDK's Sandbox.setEnvVars() sends individual export commands through the generic /api/execute endpoint, bypassing SessionManager.setEnvVars() entirely. This means:
- The container-side
shouldRedact+getRedactionLabellogic insession-manager.ts:949-960is only exercised by tests, never in production. - The
redactoption threaded throughwithSessionand theexeccallback type signatures is dead code in practice.
This isn't necessarily wrong — the container-side implementation could be useful if a future handler exposes setEnvVars directly — but it's worth noting that redaction currently only happens at two layers: (1) the SDK's CommandClient log redaction, and (2) the container Session.exec() logger, which receives redact via the ExecuteRequest... except redact is NOT in ExecuteRequest. So container-side log redaction for env vars set via the SDK also doesn't happen.
The only real redaction path is CommandClient.execute() → logSuccess() in the DO layer. This is fine as a design choice (the DO controls what the user sees), but the PR description's claim of threading through the full path is misleading. Consider adding a comment or adjusting the description.
Changeset wording
Per project conventions, changeset descriptions appear in GitHub releases and should be user-focused. The current text is technically accurate but could better convey the benefit and how to use it.
Minor items
See inline comments.
| '@cloudflare/sandbox': patch | ||
| --- | ||
|
|
||
| Add functionality to redact environment variables and command values in logs. |
There was a problem hiding this comment.
Per project conventions, changeset descriptions are user-facing (they appear in GitHub releases). Suggest focusing on the benefit and how to use the feature:
| Add functionality to redact environment variables and command values in logs. | |
| Auto-redact high-entropy env var values in `setEnvVars` logs. Pass `redact: true` to force-redact all values, or `false` to skip auto-detection. |
| exportCommand, | ||
| this.defaultSession | ||
| this.defaultSession, | ||
| { redact: shouldRedact(options?.redact, value) } |
There was a problem hiding this comment.
The redact option passed here is a RedactionMode (e.g., 'auto' or 'forced'), which CommandClient.execute() uses only for its own logSuccess() call. The raw export KEY=VALUE command is still sent unredacted to the container's /api/execute endpoint (since redact is not part of ExecuteRequest), and the container's Session.exec() logs will show the raw command.
This means container-side logs are NOT redacted for env vars set through the SDK path. If that's intentional (DO-level redaction is sufficient), a brief comment here would prevent confusion:
| { redact: shouldRedact(options?.redact, value) } | |
| // redact controls DO-side log output only; the container receives the raw command | |
| const result = await this.client.commands.execute( | |
| exportCommand, | |
| this.defaultSession, | |
| { redact: shouldRedact(options?.redact, value) } | |
| ); |
| for (const key of toUnset) { | ||
| delete this.envVars[key]; | ||
| } | ||
| this.envVars = { ...this.envVars, ...toSet }; |
There was a problem hiding this comment.
Good improvement moving the in-memory cache update to after the container commands succeed. As noted in a previous review: if a partial failure occurs (first var succeeds, second throws), the successfully-set vars live in the shell but are NOT reflected in this.envVars. On sandbox sleep/wake, only pre-existing vars would be replayed. Pre-existing issue, not introduced here, but the comment on line 755-756 would be a natural place to mention this edge case as a TODO.
| const result = await exec(exportCommand); | ||
| const mode = shouldRedact(options?.redact, value); | ||
| const label = getRedactionLabel(mode); | ||
| const result = await exec(exportCommand, { redact: mode }); |
There was a problem hiding this comment.
SessionManager.setEnvVars() is not called from any HTTP handler — the SDK sends individual export commands through the generic /api/execute endpoint. This means the redact mode computed here and passed to session.exec() is only exercised by unit tests.
This is fine as forward-looking infrastructure, but worth a brief doc comment on the method noting it's not yet exposed via the container HTTP API.
| const result = await this.client.commands.execute( | ||
| exportCommand, | ||
| sessionId | ||
| sessionId, | ||
| { redact: shouldRedact(options?.redact, value) } |
There was a problem hiding this comment.
Same observation as the top-level setEnvVars — this session-scoped version also threads redact to CommandClient.execute() but the container-side logs still see the raw command. Consistent behavior, just noting the scope of redaction.
Also: unlike the top-level method which now validates env var names before executing, this session-scoped version validates at line 3314 but the error thrown will be a plain Error (from validateEnvVarNames), whereas failures from the container throw with result.stderr. Minor inconsistency — both paths work, but the error types differ for the same failure mode.
| env?: Record<string, string | undefined>; | ||
| /** Maximum execution time in milliseconds */ | ||
| timeoutMs?: number; | ||
| /** When set, logger contexts use redacted command labels. |
There was a problem hiding this comment.
The JSDoc here clearly documents that exec() results and execStream() events return raw commands, and tests verify this. This is the right approach — just noting that this design means callers who consume stream events must handle sensitive data themselves.
Nit: the comment would be slightly clearer if it mentioned that container-side logger output is redacted (not just "logger contexts"):
| /** When set, logger contexts use redacted command labels. | |
| /** When set, container-side logger output uses redacted command labels. | |
| * `exec()` results and `execStream()` events still return the raw command. | |
| * 'forced' = caller-requested (logged as [REDACTED]) | |
| * 'auto' = entropy-detected (logged as [AUTO-REDACTED]) */ |
| export function isHighEntropy(value: string, threshold: number = 4.0): boolean { | ||
| // Short values (< 8 chars) are unlikely to be secrets | ||
| if (value.length < 8) return false; | ||
| return calculateEntropy(value) > threshold; |
There was a problem hiding this comment.
Uses strict > rather than >=, meaning a value with entropy exactly equal to the threshold is NOT considered high-entropy. This is consistent with the JSDoc but worth being explicit about since edge-case behavior around thresholds is a common source of bugs in heuristic-based systems. If intentional (and it probably is — erring on the side of fewer false positives), no action needed.
|
The review was submitted successfully. Here's a summary of what I flagged: Review posted on PR #497 with 7 inline comments and a top-level summary. Key findings:
|
|
I'll be continuing to work on this in #526 to avoid bloating this PR even more. Thank you for raising @maschwenk! |
3 participants
Summary
Adds automatic entropy-based redaction of env var values in
setEnvVarslogs, with an option for callers to force redaction explicitly.RedactionModetype ('forced' | 'auto') and centralized helpers (resolveRedaction,redactLabel) in@repo/sharedentropy moduleisHighEntropy) to automatically identify likely secrets (API keys, tokens) — logged as[AUTO-REDACTED]{ redact?: RedactionMode }onsetEnvVars— when'forced', all values are logged as[REDACTED]regardless of entropyredactoption through the full env-var path:Sandbox.setEnvVars()→CommandClient.execute()→SessionManager.setEnvVars()→Session.exec()Closes