Validate min_mask_length instead of silently disabling masking - #329
Merged
kkarrenn merged 1 commit intoSep 2, 2026
Merged
Conversation
parseInt(getInput('min_mask_length')) returns NaN for an empty or
non-numeric value, and every `line.length >= NaN` comparison is false, so
the masking loop makes zero setSecret() calls while the secret is still
written to GITHUB_OUTPUT. No error or warning is raised, so a run with no
masking looks identical to a run with masking.
The empty value is reachable without a typo: the runner substitutes an
action.yml default only when the input key is absent, so a pass-through
such as `min_mask_length: ${{ inputs.min_mask_length }}` with an unset
caller input arrives as the empty string.
parseMinMaskLength() mirrors parseBoolean() from actions-utils. An empty
value returns the declared default of 4, and a value that is not a
non-negative integer raises an error rather than being ignored, which
matches the existing handling of an invalid encoding.
dargudear-google
approved these changes
Sep 2, 2026
kkarrenn
approved these changes
Sep 2, 2026
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
min_mask_lengthis parsed withparseInt()and never validated.parseInt('')andparseInt('abc')both returnNaN, and everyline.length >= NaNcomparison is false, so themasking loop makes zero
setSecret()calls while the secret is still written toGITHUB_OUTPUTand, with
export_to_environment, to the job environment. No error is raised and no warning islogged, so a run in which nothing was masked is indistinguishable from a run in which everything
was.
I reported this to Google privately first, and was told it is fine to discuss publicly in a PR.
Why an empty value is reachable without a typo
The runner applies an
action.ymldefault only when the input key is absent(
actions/runner,ActionRunner.cs:if (!inputs.ContainsKey(key))). So a pass-through like:supplies the empty string when the caller leaves its own input unset, rather than the documented
default of
4, while reading as a no-op.Measured behaviour
Driving the built
dist/main/index.jsagainst a loopback fake Secret Manager, with a four-linesecret:
min_mask_length::add-mask::lines::add-mask::lines4(the declared default)''abc1999The
1and999rows are unchanged, so the threshold semantics, including deliberate suppressionvia a high value, behave exactly as before. Only the unparseable values change.
The change
parseMinMaskLength()mirrorsparseBoolean()in@google-github-actions/actions-utils: an emptyvalue returns the declared default, and a value that is not a non-negative integer raises an error
rather than being ignored. That is also consistent with the current handling of an invalid
encoding, which already aborts the action.0is still honoured as an explicit threshold, so aparseInt(...) || 4style fallback wasdeliberately not used.
Test plan
npm run lint,npm run buildandnpm testall pass, with 10 new cases intests/main.test.ts.dist/is rebuilt, and is byte-identical to a freshnpm run buildof the committed source.