fix(check-jsonschema): set nullglob and fail on empty glob expansion

[kyle-sexton]

Summary

.github/actions/check-jsonschema documents files as accepting shell globs and
deliberately leaves $FILES unquoted so they expand — but never set nullglob, so a
pattern matching nothing was passed through literally and crashed the tool with
OSError. That makes the documented glob support unsafe to use defensively (the
*.yml + *.yaml case from melodic-software/.github#29).

Both halves of the fix land together, because nullglob alone would convert a loud
crash into check-jsonschema being invoked with zero files — which exits 0 and becomes a
false-green CI lane, strictly worse than the bug:

1. shopt -s nullglob, scoped to the expansion only (shopt -u nullglob immediately
   after), capturing into a targets array.
2. A guard on the total expansion: if targets is empty the step fails with its own
   message.

The call site now passes "${targets[@]}" (already expanded) rather than re-expanding
$FILES, which also clears the pre-existing SC2086 on that line. The files input
description gained one sentence stating the new contract.

Two distinct failure paths

Input	Message
files empty / whitespace-only (pre-existing guard, unchanged)	::error::files is required.
files non-empty but expands to nothing (new)	::error::files expanded to no existing paths: <patterns>

Behavior deliberately not changed: a literal filename with no glob metacharacters is
untouched by nullglob, so a non-existent literal still reaches the tool and errors there
exactly as before.

Verification

1. Before/after against the real tool, in this repo

The pre-fix block reproduces the issue's crash on the defensive pattern; the fixed block
passes:

--- REAL: the #162 defensive pattern (.yml + .yaml, repo has no .yaml)  [FIXED BLOCK]
ok -- validation done
EXIT=0
--- REAL: same pattern against the PRE-FIX block (reproduces the bug)
  File ".../check_jsonschema/cli/param_types.py", line 146, in __init__
    if "r" in mode and not stat.S_ISFIFO(os.stat(filename).st_mode):
                                         ~~~~~~~^^^^^^^^^^
OSError: [WinError 123] The filename, directory name, or volume label syntax is incorrect: '.github/workflows/*.yaml'
EXIT=1

Live regression cases — the three real ci.yml call sites, run with the actual
check-jsonschema@0.37.4 against this worktree:

--- REAL: literal single file (ci.yml case 1)     files=.github/dependabot.yml
ok -- validation done
EXIT=0
--- REAL: workflows glob (ci.yml case 2)          files=.github/workflows/*.yml
ok -- validation done
EXIT=0
--- REAL: composite actions glob (ci.yml case 3)  files=.github/actions/*/action.yml
ok -- validation done
EXIT=0

2. Matrix over the modified shell logic

A throwaway harness extracts the Validate step's run: block verbatim from
action.yml (byte-diffed against the file to prove it is not a paraphrase), runs it under
the real set -euo pipefail, and stubs uvx with an arg-counter so the number of file
arguments actually handed to the tool is observable. Fixture tree: two .yml workflows,
zero .yaml.

--- CASE: literal single file
    files=[.github/workflows/ci.yml]
    uvx-stub: invoked with 1 file arg(s): .github/workflows/ci.yml
    EXIT=0

--- CASE: multiple space-separated literals
    files=[.github/workflows/ci.yml .github/workflows/release.yml]
    uvx-stub: invoked with 2 file arg(s): .github/workflows/ci.yml .github/workflows/release.yml
    EXIT=0

--- CASE: glob matching several files
    files=[.github/workflows/*.yml]
    uvx-stub: invoked with 2 file arg(s): .github/workflows/ci.yml .github/workflows/release.yml
    EXIT=0

--- CASE: glob matching nothing
    files=[.github/workflows/*.yaml]
    ::error::files expanded to no existing paths: .github/workflows/*.yaml
    EXIT=1

--- CASE: mix: matching + non-matching glob
    files=[.github/workflows/*.yml .github/workflows/*.yaml]
    uvx-stub: invoked with 2 file arg(s): .github/workflows/ci.yml .github/workflows/release.yml
    EXIT=0

--- CASE: all-empty expansion (2 dud globs)
    files=[.github/workflows/*.yaml *.toml]
    ::error::files expanded to no existing paths: .github/workflows/*.yaml *.toml
    EXIT=1

--- CASE: empty input (pre-existing guard)
    files=[]
    ::error::files is required.
    EXIT=1

--- CASE: whitespace-only input
    files=[   ]
    ::error::files is required.
    EXIT=1

--- CASE: literal missing file (unchanged)
    files=[.github/workflows/nope.yml]
    uvx-stub: invoked with 1 file arg(s): .github/workflows/nope.yml
    EXIT=0

3. The false-green is demonstrated, not asserted

Same harness, with only the empty-expansion guard stripped out (nullglob retained) —
i.e. what half 1 without half 2 would ship:

=== FALSE-GREEN CONTROL: nullglob WITHOUT the empty guard ===
    uvx-stub: invoked with 0 file arg(s): <none>
    EXIT=0  <-- exit 0 with 0 files = the false green half-2 prevents

4. Static analysis

ShellCheck 0.11.0 (the version this repo pins) against the extracted block, using the
repo's .shellcheckrc:

• Before: SC2086 (info) on the unquoted $FILES at the call site, plus two SC2154
  warnings.
• After: no SC2086, no SC2206. Only the same two SC2154 warnings
  (NO_CACHE, VERSION), which are artifacts of extracting the block out of its env:
  block — present identically before the change.

The # shellcheck disable=SC2206 directive was proven load-bearing by stripping it and
re-linting:

In - line 23:
targets=($FILES)
         ^----^ SC2206 (warning): Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.

Also clean: editorconfig-checker (exit 0), typos (exit 0), and this repo's own
check-jsonschema vendor.github-actions validation over .github/actions/*/action.yml
including the edited file (exit 0).

Blast radius

The composite is called by every repo in the constellation. Existing callers pass plain
space-separated literal file lists and globs; both paths are covered above and behave
identically to before. The only new failure mode is one that previously either crashed
with OSError or (after half 1 alone) would have silently passed.

Not verified here

actionlint does not lint composite action.yml files — passing the file explicitly makes
it parse it as a workflow and emit "jobs" section is missing / "on" section is missing.
This composite's run: block therefore has never been covered by the repo's actionlint
job; the standalone ShellCheck run above is the equivalent check. Not changed here — out of
scope for #162.

Closes #162

Related

• chore: fix drifted references and workflow schema gap .github#29 — the consumer hygiene sweep that surfaced this; it wants
  files: .github/workflows/*.yml .github/workflows/*.yaml, which this makes safe.
• .github/workflows/ci.yml — the three in-repo call sites (one literal, two globs) that
  serve as the live regression cases above.

🤖 Generated with Claude Code

https://claude.ai/code/session_01UmDSbhvfsnpL1DuTMjjJtP

[claude]

Claude finished @kyle-sexton's task in 1m 52s —— View job

---

PR Review

• Gather context and understand request
• Read changed file and diff
• Review for correctness, security, and CLAUDE.md alignment
• Post findings

---

Overall: LGTM. The fix is correct, the two halves are correctly coupled (nullglob alone would introduce the false-green the PR documents), and the verification section is unusually thorough. A few observations below.

---

Correctness

nullglob scoping (lines 76–79) is tight and correct. Each run: block runs in a fresh bash process (spawned by the Actions runner), so nullglob is always off at entry; shopt -u nullglob correctly restores that default. This is simpler than a save/restore pattern and appropriate here.

targets=($FILES) with SC2206 disabled (line 77–78) is the right call. mapfile or read -a would prevent glob expansion, which is the entire point of this construct. The inline disable is correctly scoped to one line and the companion comment explains why it's intentional rather than lazy.

Empty-expansion guard (lines 80–82) echoes ${FILES} (the original patterns) rather than ${targets[@]} (which is empty). That is exactly the right choice for debuggability — users see what they typed, not an empty string.

Call site (line 83) correctly uses "${targets[@]}", which passes each already-expanded path as a separate, properly-quoted argument to uvx. This is strictly safer than the old unquoted $FILES.

---

Security

No regressions introduced. The concern with targets=($FILES) is whether $FILES could trigger command execution via command substitution — it cannot. Bash performs only word-splitting and glob expansion during an unquoted variable assignment like this, not command substitution. A caller passing $(evil) in files: would see that string passed literally as a path argument, not executed.

The quoted "${targets[@]}" at the call site means even exotic filenames that somehow survived the expansion are passed as distinct argv tokens, not interpreted by the shell.

---

One minor observation

The PR correctly notes:

Behavior deliberately not changed: a literal filename with no glob metacharacters is untouched by nullglob, so a non-existent literal still reaches the tool and errors there exactly as before.

There is a narrow edge case not mentioned: a filename that contains glob metacharacters (*, ?, [) but is intended as a literal path. With nullglob, if no filesystem entry matches that pattern, the token is dropped and (if all tokens drop) the guard fires with files expanded to no existing paths. The old behavior would have passed it through to the tool and gotten an OSError. Both behaviors are wrong for this pathological case, but the new behavior's error message is arguably more informative. This is not a regression worth blocking on — .github/ paths don't contain glob metacharacters — and it mirrors standard shell semantics.

---

CLAUDE.md alignment

• No uses: lines were touched, so SHA-pinning is unchanged. ✓
• The # shellcheck disable=SC2206 is properly inline-scoped (not a file-wide suppression) and documented. ✓
• Input description update accurately describes the new contract. ✓

---

No blocking issues. The fix is sound, well-tested, and safe to merge.

[claude]

Claude finished @kyle-sexton's task in 2m 16s —— View job

---

Security Review

• Gather context and read changed files
• Analyze diff for security vulnerabilities
• Post findings

---

Scope: .github/actions/check-jsonschema/action.yml — the single file changed in this PR.

Overall verdict

The changes are net-positive from a security standpoint. The false-green path (nullglob alone, zero-file invocation exiting 0) that was explicitly guarded against is the most security-relevant behavior here — allowing validation to silently pass with no files checked is a subtle supply-chain risk, and this PR closes it.

No critical or important security issues were found. One suggestion-level finding follows.

---

SUGGESTION — Workflow command injection via echoed user input | Confidence: Low

Location: action.yml line 81

echo "::error::files expanded to no existing paths: ${FILES}"; exit 1

FILES is set from inputs.files via the env: block — the correct safe-injection pattern (no direct ${{ }} interpolation in the run: block). However, the value is then echoed verbatim to stdout, where the Actions runner scans every line for ::command:: workflow commands.

If a caller supplied a files: value containing a literal newline followed by a valid workflow command (e.g. ::add-mask::, ::save-state::, ::stop-commands::), the runner would process that second line as a command.

Practical exploitability is very low:

• The files: input is set by the workflow author in their committed YAML — a trusted party, not external/untrusted input like a PR title or issue body.
• Fork PRs cannot influence this value (CI uses the base branch's workflows).
• The highest-impact environment-mutation commands (::set-env::) were deprecated in 2020; the file-based replacements (GITHUB_ENV, GITHUB_OUTPUT) are not reachable this way.
• This composite action declares no outputs, so any injected ::set-output:: has no downstream consumer.

The pre-existing error path (echo '::error::files is required.') doesn't include user input, making this the one place where a new, if minor, injection surface was added.

A hardened alternative that removes the surface entirely:

echo "::error::files expanded to no existing paths:" >&2
echo "${FILES}" >&2
exit 1

Or, since only the first line carries the ::error:: command token, printing the pattern list on a separate line means only the command-token line is ever scanned:

printf '::error::files expanded to no existing paths:\n%s\n' "${FILES}"; exit 1

Either form is safe regardless of newlines in FILES.

---

No other security issues found. The use of env: for all inputs (not direct ${{ }} in run:), the explicit shopt scoping, the array quoting at the uvx call site, and the SHA-pinned uses: are all correct.