fix(cli): stop logging project env var values in the "started attempt" debug log - #4457
Conversation
…bug log
The managed run controller logged the raw `startRunAttempt` response body,
which carries `envVars` — the project environment variables injected into the
run — along with the trigger payload and run metadata. That went to the runner
container's stdout on every attempt, and to the webapp debug-log endpoint when
`TRIGGER_SEND_RUN_DEBUG_LOGS` is enabled.
The stdout path runs through `redact()`, but its deny-list matches whole
lowercased key names, so it filters `payload`/`metadata` and leaves every entry
inside the `envVars` map untouched: `DATABASE_URL` or `SUPABASE_SERVICE_ROLE_KEY`
match nothing in the list, and only values shaped like `tr_*`, `sk-*` or
`Bearer *` are caught by the value pattern. The debug-log HTTP sink applies no
redaction at all.
Log an explicit projection instead: run/snapshot identifiers, task, queue and
machine preset, plus the environment variable *names*. Names are the part with
debugging value ("did this var reach the runner?"); the values never are.
Because `envVars` keys are user-defined, no name-based deny-list can classify
them — so values are dropped wholesale rather than filtered. The remaining
fields are an allow-list, so a new field on the API response cannot silently
reintroduce a leak.
Same shape as triggerdotdev#4336, which fixed the equivalent leak in
`taskRunProcess.ts`.
Refs triggerdotdev#3566
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 5c98043 The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
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 |
|
Hi @enderyildirim, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| */ | ||
| export function startedAttemptLogProperties(start: WorkloadRunAttemptStartResponseBody) { | ||
| return { | ||
| runId: start.run.id, |
There was a problem hiding this comment.
🟡 Internal run identifier never appears in the attempt log despite being collected
The internal run identifier is added under a name that is later replaced (runId: start.run.id at packages/cli-v3/src/entryPoints/managed/executionLogging.ts:18) by the public run identifier, so that value never reaches the log and the entry shows the same identifier twice.
Impact: Operators reading the "started attempt" entry lose the internal run identifier the change intended to preserve, and see a duplicated value instead.
Property-name collision in the debug-log merge chain
RunExecution.sendDebugLog spreads the caller's properties and then unconditionally sets runId: this.runFriendlyId and snapshotId: this.currentSnapshotFriendlyId (packages/cli-v3/src/entryPoints/managed/execution.ts:1036-1043). ManagedRunLogger.sendDebugLog does the same again with runId (packages/cli-v3/src/entryPoints/managed/logger.ts:47-51). So the projection's runId (the internal DB id) is overwritten and only runFriendlyId survives — which equals the injected runId, making the two fields identical. The unit test passes because it calls startedAttemptLogProperties directly, bypassing both merges. Using a distinct key such as internalRunId would keep the value.
| runId: start.run.id, | |
| internalRunId: start.run.id, |
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe change adds ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Fixes the remaining leak site from #3566.
What I observed
On a self-hosted v4 install, each runner container writes the whole
startRunAttemptresponse body to stdout at the start of every attempt.WorkerApiRunAttemptStartResponseBodyisStartRunAttemptResult & { envVars: z.record(z.string()) }, so that object contains the full project environment variable map. Redacted excerpt of a real runner log:{"timestamp":"...","message":"started attempt","$name":"managed-run-logger","$level":"log", "start":{"envVars":{ "DATABASE_URL":"postgresql://user:REAL_PASSWORD_WAS_HERE@db:5432/app", "SOME_PROVIDER_API_KEY":"REAL_KEY_WAS_HERE", "APP_SIGNING_SECRET":"REAL_SECRET_WAS_HERE" }, ...}}Those values landed in every runner container's json log file, one entry per attempt.
Why the existing redaction doesn't catch it
SimpleStructuredLoggerpasses each line throughredact()frompackages/core/src/logger.ts, butfilterKeysmatches on the whole lowercased key name:envVarsisn't inDEFAULT_FILTERED_KEYS, so it's recursed into, and the user-defined names inside it (DATABASE_URL,SUPABASE_SERVICE_ROLE_KEY, …) match nothing in the list either. That leaves onlySECRET_VALUE_PATTERN, which catchestr_*,sk-*andBearer *— so a Postgres URL, a JWT-shaped service key, or a hex master key all pass through verbatim. Thepayloadandmetadatafields on the same object are filtered, which is why this is easy to miss.ManagedRunLogger.sendDebugLogalso forwards the same properties to the webapp debug-log endpoint viaflattenAttributes, with no redaction on that path. It's gated behindTRIGGER_SEND_RUN_DEBUG_LOGS(defaultfalse), so it only leaks for operators who turned that on — but when they do, unredacted.Why it matters for self-hosters
Runner stdout goes to whatever collects container logs (Loki, CloudWatch, Datadog, plain docker json files). Those pipelines are usually treated as low-sensitivity and are broadly readable, retained for weeks, and often shipped off-box. A task that needs any credential to do its job ends up publishing that credential there on every single attempt.
LOG_LEVEL/DEBUGdon't suppress it:sendDebugLogusesSimpleStructuredLogger.log, which isLogLevel.log = 0, the lowest value in the enum, so theif (this.level < LogLevel.log) returnguard can never fire.What I changed
execution.tsnow logs an explicit projection instead of the raw response body — newexecutionLogging.ts, mirroring thebuildWorkerLogging.tssplit added in #4420 so the payload is unit-testable:which yields run/snapshot identifiers, attempt number, task, queue, machine preset, and
envVarKeys— the environment variable names only.Two deliberate choices:
envVarskeys are user-defined, so no name-based deny-list can classify them — hence values are dropped wholesale rather than filtered. For the rest of the body an allow-list means a new field on the API response can't silently reintroduce a leak. This matches fix(cli): redact task run env values from debug log #4336, which fixed the equivalent leak intaskRunProcess.tsby loggingObject.keys(fullEnv).Since both sinks share one properties object, this fixes the stdout and webapp-endpoint halves together.
Scope
The other two sites named in #3566 are already handled on
main, so this PR leaves them alone:taskRunProcess.ts— fixed by fix(cli): redact task run env values from debug log #4336 (released in 4.5.7); logsenvKeysnow.controller.ts—...env.rawbecame...env.rawForLogging, which redactsTRIGGER_DEPLOYMENT_ID. The rest is the boundedRunnerEnvzod schema, so no project secrets reach it.One thing I noticed but did not touch, to keep this to a single issue: the debug-log HTTP sink in
ManagedRunLogger.sendDebugLogflattens the rawmergedProperties, so it doesn't get theredact()pass that the stdout sink gets fromSimpleStructuredLogger. Any othersendDebugLogcaller that passes something sensitive is therefore protected on stdout but not on the wire. Happy to open a follow-up if you'd like that sink to redact too.How I tested
packages/cli-v3/src/entryPoints/managed/executionLogging.test.ts(4 tests, passing): asserts the serialized properties contain neither an env var value nor the run payload, that the env var names survive, the exact field set, and theenvVars-absent case.oxlintandoxfmt --checkclean on the touched files.tsc -p tsconfig.src.json --noEmitreports the same 7 errors with and without this change (all from workspace packages I hadn't built), so no new type errors.snapshot.test.ts/taskRunProcess.test.tsin the same package: they need a built@trigger.dev/core, and building it needs Node 24, which I don't have locally. They fail identically on a clean checkout ofmain, so this change doesn't affect them, but I have not run the fullcli-v3suite.