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.

actions/bcr-pr-reviewer/index.js

diff --git a/actions/bcr-pr-reviewer/index.js b/actions/bcr-pr-reviewer/index.js
index 5d04d59..776b2ee 100644
--- a/actions/bcr-pr-reviewer/index.js
+++ b/actions/bcr-pr-reviewer/index.js
@@ -1,5 +1,6 @@
 const { getInput, setFailed } = require('@actions/core');
 const { context, getOctokit } = require("@actions/github");
+const path = require('path');
 
 async function _processAllPrFiles(octokit, owner, repo, prNumber, fileProcessor) {
   // Fetch the PR's initial head commit SHA to ensure consistency.
@@ -928,6 +929,24 @@
   }
 }
 
+/**
+ * Returns the directory to diff for `version` of `moduleName`, or null if `version`
+ * does not name a directory directly inside `modules/<moduleName>/`.
+ *
+ * Version strings reaching this function are read from the metadata.json of the PR
+ * under review, so they are controlled by whoever opened the PR. Without this check
+ * a value such as "../../.." makes the caller run `diff` against a directory outside
+ * the module, and `diff`'s output is printed to the workflow log.
+ */
+function moduleVersionDir(moduleName, version) {
+  const moduleDir = path.resolve('modules', moduleName);
+  const versionDir = path.resolve(moduleDir, version);
+  if (path.dirname(versionDir) !== moduleDir) {
+    return null;
+  }
+  return `modules/${moduleName}/${version}`;
+}
+
 async function runDiffModule(octokit) {
   const prNumber = context.issue.number;
   if (!prNumber) {
@@ -978,9 +997,18 @@
       }
 
       const previousVersion = metadata.versions[versionIndex - 1];
+
+      const previousVersionDir = moduleVersionDir(moduleName, previousVersion);
+      const currentVersionDir = moduleVersionDir(moduleName, versionName);
+      if (previousVersionDir === null || currentVersionDir === null) {
+        console.error(`Refusing to diff module ${moduleName}: a version does not name a directory inside modules/${moduleName}/ (previous: '${previousVersion}', current: '${versionName}')`);
+        setFailed(`Failed to generate diff for module ${moduleName}@${versionName}`);
+        continue;
+      }
+
       console.log(`${groupStart}Generating diff for module ${moduleName}@${versionName} against version ${previousVersion}`);
 
-      const diffArgs = ['--color=always', '-urN', `modules/${moduleName}/${previousVersion}`, `modules/${moduleName}/${versionName}`];
+      const diffArgs = ['--color=always', '-urN', previousVersionDir, currentVersionDir];
       console.log(`Running command: diff ${diffArgs.join(' ')}`);
       const { spawnSync } = require('child_process');
 
@@ -1047,4 +1075,4 @@
   });
 }
 
-module.exports = { getPrApprovers };
+module.exports = { getPrApprovers, moduleVersionDir };

actions/bcr-pr-reviewer/index.test.js

diff --git a/actions/bcr-pr-reviewer/index.test.js b/actions/bcr-pr-reviewer/index.test.js
index 21332d9..d6e6e47 100644
--- a/actions/bcr-pr-reviewer/index.test.js
+++ b/actions/bcr-pr-reviewer/index.test.js
@@ -1,6 +1,6 @@
 const test = require('node:test');
 const assert = require('node:assert/strict');
-const { getPrApprovers } = require('./index.js');
+const { getPrApprovers, moduleVersionDir } = require('./index.js');
 
 function fakeOctokit({ commits, reviews }) {
   return {
@@ -63,3 +63,28 @@
 
   assert.equal(approvers.has('maintainer'), true);
 });
+
+test('moduleVersionDir: a normal version resolves to its directory under the module', () => {
+  assert.equal(moduleVersionDir('rules_cc', '1.2.3'), 'modules/rules_cc/1.2.3');
+});
+
+test('moduleVersionDir: versions with dots and dashes are not treated as traversal', () => {
+  // Only a "../" path segment escapes; dots inside a version are ordinary characters.
+  assert.equal(moduleVersionDir('rules_cc', '0.9.0-rc.1..2'), 'modules/rules_cc/0.9.0-rc.1..2');
+  assert.equal(moduleVersionDir('rules_cc', '1.0.0+build.5'), 'modules/rules_cc/1.0.0+build.5');
+});
+
+test('moduleVersionDir: a version escaping the module directory is rejected', () => {
+  // metadata.json is read from the PR head, so these are attacker-supplied values.
+  assert.equal(moduleVersionDir('rules_cc', '../../..'), null);
+  assert.equal(moduleVersionDir('rules_cc', '..'), null);
+  assert.equal(moduleVersionDir('rules_cc', '../other_module/1.0.0'), null);
+  assert.equal(moduleVersionDir('rules_cc', 'nested/1.0.0'), null);
+  assert.equal(moduleVersionDir('rules_cc', '/etc'), null);
+  assert.equal(moduleVersionDir('rules_cc', '/etc/passwd'), null);
+});
+
+test('moduleVersionDir: a version naming the module directory itself is rejected', () => {
+  assert.equal(moduleVersionDir('rules_cc', '.'), null);
+  assert.equal(moduleVersionDir('rules_cc', ''), null);
+});