Skip to content

Validate min_mask_length instead of silently disabling masking - #329

Merged
kkarrenn merged 1 commit into
google-github-actions:mainfrom
BarakSrour:validate-min-mask-length
Sep 2, 2026
Merged

kkarrenn merged 1 commit into
google-github-actions:mainfrom
BarakSrour:validate-min-mask-length

Conversation

@BarakSrour

Copy link
Copy Markdown
Contributor

Summary

min_mask_length is parsed with parseInt() and never validated. parseInt('') and
parseInt('abc') both return NaN, 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
and, with export_to_environment, to the job environment. No error is raised and no warning is
logged, 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.yml default only when the input key is absent
(actions/runner, ActionRunner.cs: if (!inputs.ContainsKey(key))). So a pass-through like:

- uses: 'google-github-actions/get-secretmanager-secrets@v3'
  with:
    min_mask_length: '${{ inputs.min_mask_length }}'

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.js against a loopback fake Secret Manager, with a four-line
secret:

min_mask_length before: ::add-mask:: lines after: ::add-mask:: lines
4 (the declared default) 3 3
'' 0 3
abc 0 action fails, nothing exported
1 4 4
999 0 0

The 1 and 999 rows are unchanged, so the threshold semantics, including deliberate suppression
via a high value, behave exactly as before. Only the unparseable values change.

The change

parseMinMaskLength() mirrors parseBoolean() in @google-github-actions/actions-utils: an empty
value 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.

0 is still honoured as an explicit threshold, so a parseInt(...) || 4 style fallback was
deliberately not used.

Test plan

  • npm run lint, npm run build and npm test all pass, with 10 new cases in tests/main.test.ts.
  • dist/ is rebuilt, and is byte-identical to a fresh npm run build of the committed source.
  • The integration workflow will fail here, since a fork has no access to the repository secrets.

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.
@BarakSrour
BarakSrour requested a review from a team as a code owner September 1, 2026 16:40
@kkarrenn
kkarrenn merged commit 42fe56c into google-github-actions:main Sep 2, 2026
34 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants