bcr-pr-reviewer: validate module version directories before diffing (#2842)
## Summary
`runDiffModule()` interpolates version strings directly into the paths
it hands to `diff`:
```js
const previousVersion = metadata.versions[versionIndex - 1];
const diffArgs = ['--color=always', '-urN',
`modules/${moduleName}/${previousVersion}`,
`modules/${moduleName}/${versionName}`];
```
`previousVersion` is read out of the module's `metadata.json` at
`refs/pull/${prNumber}/head`, so its value comes from the PR being
reviewed and is chosen by whoever opened that PR. A `versions` entry
such as `"../../.."` makes `diff` descend outside the module directory,
and `diff -urN` prints the contents of the files it finds to stdout,
which the action logs.
`generate_module_diff.yml` in `bazel-central-registry` runs this on
`pull_request` for any PR touching `modules/**`, so reaching it needs
only an ordinary PR.
## Impact
Limited, and deliberately stated as such. That workflow uses the default
`github.token` on a `pull_request` trigger, so a fork PR gets a
read-only token and no repository secrets. What the bug gives is a read
of runner-local files into a public workflow log, not code execution and
not access to anything the PR author does not already have.
This is the same input, in the same workflow, that #2220 handled by
replacing `execSync` with `spawnSync`. That change removed the shell, so
the string can no longer be interpreted as a command, but it is still
used as a path. #2220 was filed as an ordinary PR on the reasoning that
this workflow's permissions are limited enough not to warrant a private
report, and this is strictly the milder of the two, so I have followed
the same route. Happy to move it if you would rather it went to the
security team.
## Fix
Add `moduleVersionDir(moduleName, version)`, which resolves the
candidate directory and returns `null` unless it sits directly inside
`modules/<moduleName>/`, and skip a module whose version fails that
test.
The check is a containment test on the resolved path rather than a
pattern match on the version string, so it does not need to know the
Bazel version grammar and does not reject versions containing dots,
dashes or plus signs. Output for legitimate input is byte-for-byte
unchanged.
## Test plan
- `node -c index.js`: parses cleanly.
- `npm test`: 7/7 pass (the 3 existing `getPrApprovers` tests plus 4 new
ones covering `../../..`, `..`, `../other_module/1.0.0`, `nested/1.0.0`,
`/etc`, `/etc/passwd`, `.` and `""`, and asserting that `0.9.0-rc.1..2`
and `1.0.0+build.5` are still accepted).
- Ran the unmodified action end to end against a stubbed GitHub API and
a fabricated PR, in a directory tree laid out like the runner's, in
three arms:
- legitimate `previousVersion` of `0.9.0`: `diff --color=always -urN
modules/testmod/0.9.0 modules/testmod/1.0.0`, normal output;
- `previousVersion` of `../../..`: `diff --color=always -urN
modules/testmod/../../.. modules/testmod/1.0.0`, and the contents of a
file placed outside the checkout appear in the action's output;
- `previousVersion` of `0.9.0-rc.1..2`: stays inside the module
directory, confirming that the dots are not what matters.
- Re-ran the identical three arms against the patched file: arm 1 output
unchanged, arm 3 unchanged, arm 2 now stops at `Refusing to diff module
testmod: a version does not name a directory inside modules/testmod/`
with no file contents in the log.2 files changed