🔒 Aisle Security Analysis

We found 3 potential security issue(s) in this PR:

1. 🟠 CI harness writes artifacts to attacker-controlled symlinked outputDir (arbitrary file write outside workspace)

Description

The new scripts/check-gateway-watch-regression.mjs writes multiple artifact files under options.outputDir without protecting against symlink traversal.

This is dangerous in CI because the default output directory is inside the repository checkout (${cwd}/.local/gateway-watch-regression), and .local/ is only gitignored (not blocked from being committed). A malicious PR can commit a .local path element (or a deeper element) as a symlink pointing outside the workspace. Node's mkdirSync(..., {recursive:true}) and writeFileSync() will follow symlinks in path components, causing the script to create directories and overwrite files outside the repository.

Additionally, the script accepts --output-dir and resolves it to an absolute path (path.resolve(...)), enabling writes anywhere on disk if that flag is ever passed by CI (directly or indirectly).

Vulnerable behaviors:

• ensureDir(options.outputDir) creates directories without checking for symlinks.
• Many subsequent writeFileSync(path.join(outputDir, ...)) calls will follow symlinks and overwrite targets.

Vulnerable code:

const DEFAULTS = {
  outputDir: path.join(process.cwd(), ".local", "gateway-watch-regression"),
};

function ensureDir(dirPath) {
  fs.mkdirSync(dirPath, { recursive: true });
}

async function main() {
  const options = parseArgs(process.argv.slice(2));
  ensureDir(options.outputDir);
  ​// ... multiple writeFileSync() into options.outputDir
}

Impact in CI:

• A PR can introduce a symlinked .local/ (or nested path) so the job writes outside $GITHUB_WORKSPACE.
• This can overwrite files on the runner (within permissions), potentially affecting subsequent steps or leaking/planting data in other locations.

Note: .gitignore does not prevent committing ignored paths; git add -f .local can add a symlink to the repo.

Recommendation

Harden the output directory handling so writes cannot escape the workspace or traverse symlinks.

Recommended defense-in-depth (pick one approach, or combine):

1. Force output under a safe base directory and verify realpath containment

const workspace = fs.realpathSync(process.cwd());
const requested = path.resolve(options.outputDir);
const realOutParent = fs.realpathSync(path.dirname(requested));
const realOut = path.join(realOutParent, path.basename(requested));

if (!realOut.startsWith(workspace + path.sep)) {
  throw new Error(`Refusing to write outside workspace: ${realOut}`);
}

2. Reject symlinks in any path segment (walk from workspace to outputDir and lstatSync each segment; if isSymbolicLink(), abort).

3. Prefer a temp directory controlled by the runner (avoids repository-controlled path elements):

const outputDir = fs.mkdtempSync(path.join(os.tmpdir(), "gateway-watch-regression-"));

4. If --output-dir must exist, require it to be relative (no absolute paths) and still apply the realpath/symlink checks.

Also consider hardening CI artifact upload paths to avoid following repository-controlled symlinks when collecting artifacts.

2. 🔵 Unpinned GitHub Actions versions in new CI job (supply-chain risk)

Description

The newly added gateway-watch-regression job references third-party GitHub Actions by a floating tag (@​v6, @​v7) instead of pinning to an immutable commit SHA.

This creates a supply-chain risk because:

• GitHub Actions are fetched and executed from the referenced repository at runtime.
• Tags/major versions can be moved or a maintainer account/release process could be compromised.
• This workflow runs on push to main (not just PRs), where GITHUB_TOKEN and any configured secrets/permissions for the workflow would be available to the runner.

Vulnerable code (new job):

- name: Checkout
  uses: actions/checkout@​v6
...
- name: Upload gateway watch regression artifacts
  uses: actions/upload-artifact@​v7

Recommendation

Pin GitHub Actions to a full commit SHA (and optionally keep the major tag as a comment for readability).

Example:

- name: Checkout
  uses: actions/checkout@<FULL_40_CHAR_COMMIT_SHA> # v4.x.x

- name: Upload gateway watch regression artifacts
  uses: actions/upload-artifact@<FULL_40_CHAR_COMMIT_SHA> # v4.x.x

Additionally, consider setting explicit least-privilege workflow/job permissions, e.g.:

permissions:
  contents: read

(and add only the scopes needed for release jobs).

3. 🔵 Arbitrary process signaling via stale/tampered PID file in gateway watch regression harness

Description

The regression harness determines which process to terminate by reading a PID from a predictable file path (watch.pid) and then sending signals to that PID.

Because the PID file is not created atomically, not removed before use, and not validated to be freshly written by the spawned watch process, a pre-existing or tampered watch.pid can cause the harness to send SIGTERM/SIGKILL to an unintended process.

Impact:

• If options.outputDir is reused (e.g., local runs, or a CI workspace/cache that preserves .local/), an old watch.pid can be read before the new watch process overwrites it.
• PID reuse on the OS can cause the old PID value to refer to a completely different process.
• In the worst case this can terminate unrelated processes owned by the same user, causing CI/job disruption or local DoS.

Vulnerable code:

let watchPid = null;
for (let attempt = 0; attempt < 50; attempt += 1) {
  if (fs.existsSync(pidFilePath)) {
    watchPid = Number(fs.readFileSync(pidFilePath, "utf8").trim());
    break;
  }
  await sleep(100);
}
...
process.kill(watchPid, "SIGTERM");
...
process.kill(watchPid, "SIGKILL");

Recommendation

Ensure the PID file cannot be stale/tampered and that the PID corresponds to the process started by this run.

Minimum hardening (recommended):

• Delete any pre-existing PID file before spawning the watch.
• Wait for the PID file to be recreated, and validate contents are a positive integer.
• Optionally, write and verify a nonce (or check file mtime is after spawn time) to ensure freshness.

Example fix (delete + mtime freshness check):

const startTime = Date.now();
try { fs.unlinkSync(pidFilePath); } catch {}

​// spawn child...

for (let attempt = 0; attempt < 50; attempt += 1) {
  try {
    const st = fs.statSync(pidFilePath);
    if (st.mtimeMs >= startTime) {
      const pid = Number(fs.readFileSync(pidFilePath, "utf8").trim());
      if (Number.isInteger(pid) && pid > 0) {
        watchPid = pid;
        break;
      }
    }
  } catch {}
  await sleep(100);
}

Stronger alternative:

• Avoid PID files entirely and instead terminate the spawned process tree using process groups (e.g., spawn with { detached: true } and process.kill(-child.pid, signal) on POSIX), or use a library that can kill a process tree safely.

Analyzed PR: #49048 at commit e57f1b3

_{Last updated on: 2026-03-17T15:04:27Z}

Greptile Summary

This PR adds a CI regression harness (scripts/check-gateway-watch-regression.mjs) that guards against pnpm gateway:watch silently re-populating dist-runtime/ with a duplicate runtime graph after a clean pnpm build. The check snapshots dist/ and dist-runtime/ before and after a bounded watch run, verifies that file and byte counts do not exceed configurable thresholds, and confirms the gateway reaches its listening state within the measurement window. A new gateway-watch-regression CI job wires this harness into the existing pipeline with proper scoping guards, artifact uploads, and if: always() so diagnostic data is preserved on failure.

Issues found:

• keepLogs is declared in DEFAULTS but is never read or applied anywhere in the script — the option has no effect and should either be implemented (conditional log cleanup) or removed to avoid misleading future readers.
• The startup-readiness detection (chunkText.includes("[gateway] listening on ws://")) checks only the bytes in the current data event rather than the accumulated stdout buffer; a message fragmented across two consecutive chunks would cause a false "never reached listening state" failure. Checking stdout.includes(...) after accumulation would be more robust.
• startupReadyWarnMs (5 000 ms) pushes to the same failures array and calls process.exit(1) like the hard-fail threshold, making the "warn" label misleading. On slower CI runners this could trigger unexpected failures.

Confidence Score: 4/5

• Safe to merge — adds CI coverage only, no production code changes; minor script issues are non-blocking.
• The change is purely additive CI/infra scaffolding with no impact on production code paths. The three issues found are all style-level: dead keepLogs option, chunk-only startup detection (theoretical flakiness risk), and a misleadingly-named warn threshold that actually causes hard failures. None of these block the core purpose of the harness, and the PR evidence shows it passing cleanly on the author's machine. The slight risk of timing-induced CI flakiness on slower runners (5 000 ms soft-fail threshold) is the only realistic concern before merge.
• scripts/check-gateway-watch-regression.mjs — three minor issues: dead keepLogs field, chunk-only startup detection, and misleading warn-vs-fail threshold naming.

This is a comment left during a code review.
Path: scripts/check-gateway-watch-regression.mjs
Line: 16

Comment:
**`keepLogs` option is dead code**

`keepLogs` is declared in `DEFAULTS` and spread into `options` by `parseArgs`, but it is never read anywhere in the rest of the script — there is no conditional that checks `options.keepLogs` to decide whether to delete or retain the log files. Because the default is `true` and no cleanup path exists, logs are always kept regardless of what value this field holds. Either wire up the option (e.g., conditionally delete the watch log files after a successful run) or remove `keepLogs` from `DEFAULTS` to avoid misleading future maintainers.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: scripts/check-gateway-watch-regression.mjs
Line: 231-237

Comment:
**Startup detection checks the current chunk, not the accumulated buffer**

`chunkText.includes("[gateway] listening on ws://")` only inspects the bytes that arrived in the current `data` event. If the log line happens to be fragmented across two consecutive chunks (possible, though unlikely for a single `console.log` call over a pipe), `startupReadyMs` would remain `null` and the harness would fail with "never reached the gateway listening state". Checking the accumulated `stdout` string after appending is trivially more robust:

```suggestion
  child.stdout?.on("data", (chunk) => {
    const chunkText = String(chunk);
    stdout += chunkText;
    if (startupReadyMs === null && stdout.includes("[gateway] listening on ws://")) {
      startupReadyMs = Date.now() - startedAt;
    }
  });
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: scripts/check-gateway-watch-regression.mjs
Line: 379-383

Comment:
**"Warn" threshold causes a hard CI failure**

`startupReadyWarnMs` (5 000 ms) pushes into the same `failures` array as `startupReadyFailMs` (8 000 ms), so exceeding the "warn" threshold also calls `process.exit(1)` and fails the CI job. The name `warn` implies a softer outcome, but there is no way for the check to produce a non-zero-exit warning today. This is unlikely to be a problem on the current hardware, but could cause unexpected CI failures on slower runners in the future. Consider either renaming the field (e.g., `startupReadySoftFailMs`) to make the behavior explicit, or actually downgrading this branch to a non-fatal advisory (logged but not added to `failures`).

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: f7ba371}