Conversation
SafeRequestLogging replaces the value of any --client_env variable whose name matches auth|pass|cookie|token|api_key|credential|secret before the command line is written to the server log. This closes three gaps, two of them by widening existing terms rather than adding new ones: - "api_key" becomes "key", covering signing keys and private keys held directly in the environment. rules_jvm_external's publisher reads PGP_SIGNING_KEY, an in-memory PGP private key; the MAVEN_PASSWORD next to it in the same command line is redacted and this is not, only because of how the two variables happen to be named. - "credential" becomes "cred", covering the CREDS and CRED abbreviations that appear when a username and password are bound to a single variable. This was requested in bazelbuild#30828 alongside "credential" and "secret", which landed in bazelbuild#31104. - DIRENV_DIFF is redacted by exact name. direnv sets it on every shell it manages, and its value is a base64url and zlib encoded copy of everything the .envrc exported, plus the previous values of anything it shadowed. A variable that this filter does redact by name is therefore written to the log anyway, one entry along, whenever direnv was the thing that set it. Both term changes are substrings of the terms they replace, so the pattern covers strictly more than before. This affects the server log only. The build event protocol carries client env values through redactArguments, which filters credential-bearing option names and not environment variable names, so it is unchanged by this.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Author
|
@googlebot signed |
rdark
marked this pull request as ready for review
September 19, 2026 18:57
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.
SafeRequestLoggingreplaces the value of any--client_envvariable whose namematches
auth|pass|cookie|token|api_key|credential|secretwith__private_value_removed__before the command line is written to$(output_base)/java.log. Three gaps in that list are easy to hit.There's a more general argument to be made around the wisdom of defaulting to
logging all environment variables on server start and then attempting to play
whack-a-mole to redact them, but we can leave that one for another day.
In the mean time here's some real-world examples fixed in this PR:
Keys named as keys.
rules_jvm_external's publisher reads its signingmaterial straight from the environment
(
MavenPublisher.java:126-129),so
bazel run //:my_artifact.publishlogs:A PGP private key goes to the log in full, while the Maven password beside it in
the same command line is redacted. The difference is only how the two variables
happen to be named, and the names are fixed by the ruleset, so it cannot be
avoided by renaming. The same shape applies to any deploy key, signing key or
storage key held in the environment.
Abbreviated credentials.
credwas requested in #30828 alongsidecredentialandsecret; those two landed in #31104 and this is the third. Itcovers
CREDSandCRED, which is what tends to appear when a username andpassword are bound to one variable.
Rather than lengthening the list, both of these widen a term already in it:
keysubsumesapi_key,credsubsumescredential. The pattern coversstrictly more than before while getting shorter, and the existing
MY_API_KEYand
COURSIER_CREDENTIALScases pass unchanged, which is the guard that thereplacements really do subsume what they replace.
keyas a substring will match variables holding nothing sensitive. That costsone unreadable value in a debug log, against a private key on disk — and in a
public issue tracker whenever someone attaches
java.logto a bug report. It isalso a cost this filter already pays: because of
authandpass,GIT_AUTHOR_NAME,GIT_AUTHOR_EMAIL,COMPASS_HOMEandBYPASS_CACHEare allredacted today.
DIRENV_DIFF. direnv sets this on every shell it manages, and its value isa base64url and zlib encoded copy of everything the
.envrcexported, plus theprevious values of anything it shadowed — direnv ships its own decoder at
test/show-direnv-diff.sh.Deliberately not included:
pwd, which would also have coveredPGP_SIGNING_PWDabove.PWDandOLDPWDare set by every POSIX shell, andblanking the working directory in a log people read to debug is a real cost,
unlike the substring false positives above. Worth being explicit too that this
reaches the server log only — the BEP carries
--client_envvalues throughredactArguments, which filters credential-bearing option names and not variablenames, so it is unaffected here.
Would this be worth backporting, along with #31104? Neither is in a release:
9.2.0 does not contain #31104, so a user on 9.x currently has none of
credential,secret,key,credor the direnv case. Released 9.x also neverprunes the log — the client writes
SimpleLogHandler.limitand.total_limitwhile that same version reads
rotate_limit_bytesandtotal_limit_bytes, soboth fall back to unlimited and exposure accumulates for the life of the output
base. Log pruning is fixed on master by c7584fd, which is in 10.0.0
pre-releases only.
Testing
Four cases added alongside the existing per-term ones:
PGP_SIGNING_KEY, a*_CREDSname,DIRENV_DIFF, and a negative case pinningPWD/OLDPWDasvisible so the decision above does not get quietly reversed later. Fixture
values are
notprinted, matching the file, so nothing resembles a realcredential.
All 13 existing assertions pass unchanged, including the negatives
(
ok=COOKIE,--package_path=./MY_PASSWORD/foo,--client_env=BROKEN).BepCommandLineLeakTestis unaffected and passes.