From 66f03078b883faba19f509939ff31a87199fa220 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 19 Oct 2026 21:13:47 -0400 Subject: [PATCH 1/2] ci: drop the third-party step after the deploy key and declare workflow permissions (#1968) - cron-vendor/cron-conda: replace tj-actions/verify-changed-files@v17 with a `git status --porcelain` step that writes the same outputs; the commit steps read the file list through `env:` instead of `${{ }}` in the script - create-release, CodeQL: top-level `permissions: {}`; every job keeps its own - deploy-pages: top-level `contents: read`; the push to gh-pages moves to a `publish-pages` job holding `contents: write`, which never runs on a PR - tests: pin no third-party action after the deploy key, the step's outputs against v17's, top-level permissions in every workflow, and the Pages split Refs #1968 --- .github/workflows/codeql-analysis.yml | 3 + .github/workflows/create-release.yml | 4 +- .github/workflows/cron-conda.yml | 18 +- .github/workflows/cron-vendor.yml | 28 +- .github/workflows/deploy-pages.yml | 51 +++- docs/source/contributing/workflows.rst | 8 +- .../test_conda_platform_pin_refresh_1851.py | 2 +- .../test_vendor_commits_new_modules_1685.py | 46 ++- tests/project/test_workflow_hardening_1968.py | 284 ++++++++++++++++++ util/vendor_code_changed.py | 9 +- 10 files changed, 427 insertions(+), 26 deletions(-) create mode 100644 tests/project/test_workflow_hardening_1968.py diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index e8b2e2c21..2714cda8b 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -9,6 +9,9 @@ on: schedule: - cron: '0 2 * * 6' +# #1968: no scopes by default. `analyze` declares the ones it needs. +permissions: {} + # Only pull-request runs may be superseded -- see the note in unit-tests.yml. A # ref-keyed group drops a *pending* run when a newer one joins it, which for a # security scan means a commit reaching main with no analysis on record. diff --git a/.github/workflows/create-release.yml b/.github/workflows/create-release.yml index 91a241f0c..e709bc0a7 100644 --- a/.github/workflows/create-release.yml +++ b/.github/workflows/create-release.yml @@ -7,8 +7,8 @@ on: workflows: [ "Vendor Update" ] types: - completed - #release: - # types: ['published'] +# #1968: no scopes for a job without `permissions:` of its own. Each job has one. +permissions: {} name: Create Release diff --git a/.github/workflows/cron-conda.yml b/.github/workflows/cron-conda.yml index 09a5d6fc7..767dc960a 100644 --- a/.github/workflows/cron-conda.yml +++ b/.github/workflows/cron-conda.yml @@ -192,9 +192,21 @@ jobs: python util/conda_vendor.py + # #1968: `git status` in place of tj-actions/verify-changed-files@v17, so + # no third-party code runs while the deploy key is loaded. The script and + # its outputs are cron-vendor.yml's, whose comment gives the rules. - name: Verify Changed files - uses: tj-actions/verify-changed-files@v17 id: verify-changed-files + run: | + listing="$(git -c core.quotepath=on status --porcelain --untracked-files=all --no-renames)" + printf '%s\n' "$listing" + changed="$(printf '%s\n' "$listing" | cut -c4- | LC_ALL=C sort -u | paste -sd ' ' -)" + if [ -n "$changed" ]; then + echo "files_changed=true" >> "$GITHUB_OUTPUT" + echo "changed_files=$changed" >> "$GITHUB_OUTPUT" + else + echo "files_changed=false" >> "$GITHUB_OUTPUT" + fi - name: Bump Version if: steps.verify-changed-files.outputs.files_changed == 'true' @@ -203,13 +215,15 @@ jobs: - name: Commit Changes if: steps.verify-changed-files.outputs.files_changed == 'true' + env: + CHANGED_FILES: ${{ steps.verify-changed-files.outputs.changed_files }} run: | git config --local user.email "github-actions[bot]@users.noreply.github.com" git config --local user.name "github-actions[bot]" git commit -am"Bumped build to $(cat conda/build) Updated conda distribution at $(date). The following files have been changed: - ${{ steps.verify-changed-files.outputs.changed_files }}" + $CHANGED_FILES" # Writes `pushed` and `commit`, which `CONDA_CHANGED` and `CONDA_COMMIT` # below hand to `conda-tag` and `conda-dist` (#1632). diff --git a/.github/workflows/cron-vendor.yml b/.github/workflows/cron-vendor.yml index 67b6ed764..c3075b692 100644 --- a/.github/workflows/cron-vendor.yml +++ b/.github/workflows/cron-vendor.yml @@ -297,9 +297,31 @@ jobs: echo 'The step that tests the regenerated tree failed or timed out, so nothing from this crawl is committed.' } >> "$GITHUB_STEP_SUMMARY" + # #1968: `git status` in place of tj-actions/verify-changed-files@v17, so + # no third-party code runs while the deploy key is loaded. The outputs are + # the action's with no `files:` input, which it never had here: + # `files_changed`, and `changed_files`, the paths joined by spaces and + # written only when there are some. Every changed tracked file counts, + # untracked files are listed one by one, and ignored files are left out. + # A staged change counts too, which the action's `git diff` missed; + # nothing before this step stages one. git C-quotes a path holding a + # space, a quote, a control character or a non-ASCII byte, so the list is + # one line. Such a path still splits on its space, and + # util/vendor_code_changed.py counts a name it cannot read as code. The + # steps below read the list through `env:`, so a path is never shell text. + # cron-conda.yml runs the same script. - name: Verify Changed files - uses: tj-actions/verify-changed-files@v17 id: verify-changed-files + run: | + listing="$(git -c core.quotepath=on status --porcelain --untracked-files=all --no-renames)" + printf '%s\n' "$listing" + changed="$(printf '%s\n' "$listing" | cut -c4- | LC_ALL=C sort -u | paste -sd ' ' -)" + if [ -n "$changed" ]; then + echo "files_changed=true" >> "$GITHUB_OUTPUT" + echo "changed_files=$changed" >> "$GITHUB_OUTPUT" + else + echo "files_changed=false" >> "$GITHUB_OUTPUT" + fi # #1674: only a code change cuts a beta. util/vendor_code_changed.py, # whose docstring gives the rule, writes `code_changed=false` when every @@ -332,6 +354,8 @@ jobs: if: >- steps.verify-changed-files.outputs.files_changed == 'true' && steps.classify.outputs.code_changed != 'false' + env: + CHANGED_FILES: ${{ steps.verify-changed-files.outputs.changed_files }} run: | git config --local user.email "github-actions[bot]@users.noreply.github.com" git config --local user.name "github-actions[bot]" @@ -345,7 +369,7 @@ jobs: git commit -am"Bumped version to $(python -c 'import pcapkit; print(pcapkit.__version__)') Updated vendor constant enumerations at $(date). The following files - have been changed: ${{ steps.verify-changed-files.outputs.changed_files }}" + have been changed: $CHANGED_FILES" # #1674: the same commit without the bump. Create Release still runs when # this workflow completes, as after a crawl that changed nothing, and sees diff --git a/.github/workflows/deploy-pages.yml b/.github/workflows/deploy-pages.yml index 8250c8833..a901a937c 100644 --- a/.github/workflows/deploy-pages.yml +++ b/.github/workflows/deploy-pages.yml @@ -21,7 +21,7 @@ on: branches: [main] permissions: - contents: write + contents: read # Deliberately still keyed on the ref, unlike the validation workflows. This one # publishes a *latest state* rather than a verdict on a commit, so coalescing is @@ -173,9 +173,56 @@ jobs: # disable Jekyll touch docs/build/html/.nojekyll + # `.nojekyll` is hidden, and upload-artifact leaves hidden files out unless + # told otherwise. + - name: Upload the built site + if: ${{ github.event_name != 'pull_request' }} + uses: actions/upload-artifact@v7 + with: + name: pages-html + path: docs/build/html + include-hidden-files: true + retention-days: 1 + + # #1968: the push to `gh-pages`, split out of `deploy-pages` so that the job + # running the tree's own code, a pull request's included, holds only the + # top-level `contents: read`. This job runs no project code, and never on a + # pull request. It needs `contents: write` to push `gh-pages`, and `actions: + # read` to download the site `deploy-pages` uploaded in this run. That job's + # result is checked explicitly, under `!cancelled()`: `unit-tests` or `verdict` + # always skips, and a skip anywhere in the chain skips a job left with the + # implicit `success()` (#1632). Its own group keeps one push to `gh-pages` at + # a time. + publish-pages: + name: Publish to gh-pages + needs: [ deploy-pages ] + if: >- + !cancelled() && needs.deploy-pages.result == 'success' && + github.event_name != 'pull_request' + concurrency: + group: publish-pages-${{ github.ref }} + cancel-in-progress: false + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + actions: read + contents: write + steps: + # The deploy action pushes from a checkout of this repository. + - name: Checkout 🛎️ + uses: actions/checkout@v7 + with: + persist-credentials: false + + - name: Download the built site + env: + GH_TOKEN: ${{ github.token }} + run: >- + gh run download "$GITHUB_RUN_ID" --repo "$GITHUB_REPOSITORY" --name pages-html + --dir docs/build/html + - name: Deploy 🚀 uses: JamesIves/github-pages-deploy-action@v4 - if: ${{ github.event_name != 'pull_request' }} with: token: ${{ secrets.GITHUB_TOKEN }} branch: gh-pages # The branch the action should deploy to. diff --git a/docs/source/contributing/workflows.rst b/docs/source/contributing/workflows.rst index 0a626b638..b0b24aae8 100644 --- a/docs/source/contributing/workflows.rst +++ b/docs/source/contributing/workflows.rst @@ -1013,9 +1013,13 @@ approval, in ``create-release.yml`` and ``cron-python-versions.yml``: `Python Versions`_) No other workflow in scope here declares one -- including -``deploy-pages.yml``, whose ``deploy-pages`` job pushes straight to the +``deploy-pages.yml``, whose ``publish-pages`` job pushes straight to the ``gh-pages`` branch via ``JamesIves/github-pages-deploy-action`` rather than -through an ``environment:``-gated deployment. Of these environments, only +through an ``environment:``-gated deployment. Its ``deploy-pages`` job builds +the site, a pull request's included, with ``contents: read``, and uploads it +for ``publish-pages``, which alone holds ``contents: write`` and never runs on +a pull request +(`#1968 `__). Of these environments, only ``github-release`` carries a required reviewer; :doc:`releasing` covers why one approval on that single environment is enough to gate the whole release graph, and is the page to read for the reasoning rather than diff --git a/tests/project/test_conda_platform_pin_refresh_1851.py b/tests/project/test_conda_platform_pin_refresh_1851.py index 155f29e61..14022c112 100644 --- a/tests/project/test_conda_platform_pin_refresh_1851.py +++ b/tests/project/test_conda_platform_pin_refresh_1851.py @@ -662,7 +662,7 @@ def test_the_refresh_runs_between_the_install_and_the_writer(self) -> None: refresh = self.index(lambda step: self.runs(step) == ['python util/conda_vendor.py --refresh-pins']) install = self.index(lambda step: 'python -m pip install -e .[all]' in self.runs(step)) writer = self.index(lambda step: 'python util/conda_vendor.py' in self.runs(step)) - changed = self.index(lambda step: str(step.get('uses', '')).startswith('tj-actions/verify-changed-files@')) + changed = self.index(lambda step: step.get('id') == 'verify-changed-files') self.assertLess(install, refresh) self.assertLess(refresh, writer) self.assertLess(writer, changed) diff --git a/tests/project/test_vendor_commits_new_modules_1685.py b/tests/project/test_vendor_commits_new_modules_1685.py index 08c42dc82..3e410de7a 100644 --- a/tests/project/test_vendor_commits_new_modules_1685.py +++ b/tests/project/test_vendor_commits_new_modules_1685.py @@ -3,13 +3,14 @@ ``Commit changes`` in :file:`.github/workflows/cron-vendor.yml` ran ``git commit -am``, which stages modified and deleted tracked files and never an untracked -one. ``tj-actions/verify-changed-files@v17`` reports untracked files too (the -``??`` entries of ``git status -u --porcelain``), so a crawl that generated a -module for a new registry bumped the version and left the module out of the -commit. Every later crawl found it untracked again, and bumped again. - -:class:`TestTheBumpCommitStep` runs that step's script in a throwaway repository -the way the runner runs it, with ``${{ }}`` substituted first. The second test +one. ``Verify Changed files`` reports untracked files too (the ``??`` entries of +``git status -u --porcelain``), so a crawl that generated a module for a new +registry bumped the version and left the module out of the commit. Every later +crawl found it untracked again, and bumped again. + +:class:`TestTheBumpCommitStep` runs that step's script, and ``Verify Changed +files``', in a throwaway repository the way the runner runs them, with ``${{ }}`` +substituted first. The second test classifies a new module with :file:`util/vendor_code_changed.py` and walks the steps, which shows ``Commit documentation changes``, still ``-a`` alone, never sees one. The harness is #1674's, from @@ -78,10 +79,11 @@ def setUp(self) -> None: self.commit('chore(const): the package marker', HUMAN) self.base = self.git('rev-parse', 'HEAD') - def reported(self) -> 'list[str]': - """What ``verify-changed-files@v17`` reports: its ``entrypoint.sh`` with no - ``files`` pattern, so ``git diff --diff-filter=ACMUXTR``, the ``??`` lines - of ``git status -u --porcelain`` and ``git ls-files --deleted``, de-duplicated. + def v17(self) -> 'list[str]': + """What ``tj-actions/verify-changed-files@v17``, which ``Verify Changed + files`` replaced (#1968), reported: its ``entrypoint.sh`` with no ``files`` + pattern, so ``git diff --diff-filter=ACMUXTR``, the ``??`` lines of ``git + status -u --porcelain`` and ``git ls-files --deleted``, de-duplicated. """ tracked = self.git('diff', '--diff-filter=ACMUXTR', '--name-only').splitlines() @@ -90,6 +92,28 @@ def reported(self) -> 'list[str]': deleted = self.git('ls-files', '--deleted').splitlines() return sorted(set(tracked + untracked + deleted) - {''}) + def reported(self) -> 'list[str]': + """``Verify Changed files``' ``changed_files``, run as the runner runs it and + split on spaces as its readers split it, after checking it agrees with + :meth:`v17` on this tree. + + """ + path = self.root / 'verify.sh' + path.write_text(block_scalar_after(step_block(self.block, 'Verify Changed files'), _RUN_LITERAL), + encoding='utf-8') + output = self.root / 'verify.output' + output.write_text('', encoding='utf-8') + result = subprocess.run(['bash', '-e', str(path)], cwd=self.repo, + env=dict(self.env, GITHUB_OUTPUT=str(output)), + capture_output=True, text=True, check=False, + timeout=scale_timeout(60)) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + outputs = dict(line.split('=', 1) for line in output.read_text(encoding='utf-8').splitlines()) + changed = outputs.get('changed_files', '').split() + self.assertEqual(outputs['files_changed'], 'true' if changed else 'false', outputs) + self.assertEqual(changed, self.v17()) + return changed + def classify(self, paths: 'list[str]') -> 'tuple[tuple[str, dict[str, object]], str]': """``Classify Changed files``' outcome and outputs, from the script itself, and what it printed. diff --git a/tests/project/test_workflow_hardening_1968.py b/tests/project/test_workflow_hardening_1968.py new file mode 100644 index 000000000..3084b07c8 --- /dev/null +++ b/tests/project/test_workflow_hardening_1968.py @@ -0,0 +1,284 @@ +# -*- coding: utf-8 -*- +"""#1968: no third-party code after the deploy key, and permissions stated in every workflow. + +* :class:`TestNoThirdPartyCodeAfterTheDeployKey`: ``webfactory/ssh-agent`` + exports the deploy key's agent socket to every later step of its job, so no + later step may run an action from outside ``actions/``, ``github/`` or this + repository. ``tj-actions/verify-changed-files`` did, in Vendor Update and Conda + Update. +* :class:`TestTheChangedFilesStep` runs the ``Verify Changed files`` script that + replaced it, in a throwaway repository, on paths a crawl could write. +* :class:`TestEveryWorkflowDeclaresPermissions`: a workflow without top-level + ``permissions:`` hands the repository default to any job without its own. + GitHub Pages builds a pull request's tree, so that build holds ``contents: + read`` and the push to ``gh-pages`` is a job of its own. + +Read with :func:`yaml.safe_load`, from the test extra. + +""" + +from __future__ import annotations + +import os +import pathlib +import subprocess +import tempfile +import unittest +from typing import TYPE_CHECKING + +from tests._support import scale_timeout +from tests.project.test_release_gates import STATUS, job_blocks, job_runs + +try: + import yaml +except ImportError: # pragma: no cover + yaml = None # type: ignore[assignment] + +if TYPE_CHECKING: + from typing import Any, Optional + +ROOT = pathlib.Path(__file__).resolve().parents[2] +WORKFLOWS = ROOT / '.github' / 'workflows' + +#: The action that loads the deploy key into an agent for the rest of the job. +DEPLOY_KEY_ACTION = 'webfactory/ssh-agent@' +#: Owners whose actions may run with the deploy key loaded: GitHub's own, and +#: ``./`` for an action in this repository. +FIRST_PARTY = ('actions/', 'github/', './') +#: The jobs that load the deploy key and then commit, which the step replaced +#: by #1968 ran in. +CHANGED_FILES_JOBS = (('cron-vendor.yml', 'vendor-update'), ('cron-conda.yml', 'conda-update')) +#: The step's id, which every reader of its outputs names. +STEP_ID = 'verify-changed-files' + + +def workflow(name: 'str') -> 'dict[Any, Any]': + return yaml.safe_load((WORKFLOWS / name).read_text(encoding='utf-8')) + + +def step(name: 'str', job: 'str') -> 'dict[str, Any]': + found = [entry for entry in workflow(name)['jobs'][job]['steps'] if entry.get('id') == STEP_ID] + if len(found) != 1: + raise AssertionError(f'{name}: `{job}` has {len(found)} steps with id {STEP_ID!r}') + return found[0] + + +@unittest.skipIf(yaml is None, 'PyYAML, from the test extra, is not installed') +class TestNoThirdPartyCodeAfterTheDeployKey(unittest.TestCase): + """Every step after ``webfactory/ssh-agent`` uses a first-party action, or none.""" + + def test_no_job_runs_a_third_party_action_after_the_key(self) -> None: + keyed = [] # type: list[tuple[str, str]] + for path in sorted(WORKFLOWS.glob('*.yml')): + for job_id, job in (workflow(path.name).get('jobs') or {}).items(): + steps = job.get('steps') or [] + loads = [index for index, entry in enumerate(steps) + if str(entry.get('uses', '')).startswith(DEPLOY_KEY_ACTION)] + if not loads: + continue + keyed.append((path.name, job_id)) + for entry in steps[loads[0] + 1:]: + uses = str(entry.get('uses', '')) + with self.subTest(workflow=path.name, job=job_id, step=entry.get('name') or uses): + self.assertTrue(not uses or uses.startswith(FIRST_PARTY), + f'{uses} runs with the deploy key loaded') + # Not vacuous: the jobs #1968 was about are among those that load the key. + for where in CHANGED_FILES_JOBS: + self.assertIn(where, keyed) + + def test_the_changed_files_step_is_a_script(self) -> None: + """The same script in both jobs, so neither can drift from the other.""" + scripts = set() + for name, job in CHANGED_FILES_JOBS: + entry = step(name, job) + with self.subTest(workflow=name): + self.assertNotIn('uses', entry) + self.assertEqual(entry['name'], 'Verify Changed files') + scripts.add(entry['run']) + self.assertEqual(len(scripts), 1) + + def test_the_list_reaches_a_script_only_through_env(self) -> None: + """The list is file names; spliced into a ``run:`` it would be shell text.""" + output = f'steps.{STEP_ID}.outputs.changed_files' + for name, job in CHANGED_FILES_JOBS: + readers = 0 + for entry in workflow(name)['jobs'][job]['steps']: + with self.subTest(workflow=name, step=entry.get('name')): + self.assertNotIn(output, str(entry.get('run', ''))) + readers += output in str(entry.get('env', {})) + with self.subTest(workflow=name): + self.assertGreater(readers, 0, f'nothing in {name} reads {output}') + + +@unittest.skipIf(yaml is None, 'PyYAML, from the test extra, is not installed') +class TestTheChangedFilesStep(unittest.TestCase): + """``Verify Changed files``' script, in a throwaway repository, as ``bash -e`` runs it.""" + + def setUp(self) -> None: + tmp = tempfile.TemporaryDirectory() + self.addCleanup(tmp.cleanup) + self.root = pathlib.Path(tmp.name) + self.repo = self.root / 'repo' + home = self.root / 'home' + home.mkdir() + (home / '.gitconfig').write_text( + '[init]\n\tdefaultBranch = main\n[commit]\n\tgpgsign = false\n' + '[maintenance]\n\tauto = false\n\tautoDetach = false\n' + '[gc]\n\tauto = 0\n\tautoDetach = false\n' + '[user]\n\tname = Jarry Shaw\n\temail = jarryshaw@icloud.com\n', encoding='utf-8') + self.env = {'PATH': os.environ.get('PATH', ''), 'HOME': str(home), + 'XDG_CONFIG_HOME': str(home / '.config'), 'GIT_CONFIG_NOSYSTEM': '1', + 'LC_ALL': 'C'} + self.git('init', '-q', str(self.repo), cwd=self.root) + for path in ('pcapkit/const/a.py', 'pcapkit/const/b.py', 'conda/requirements.txt'): + self.write(path, 'x = 1\n') + self.write('.gitignore', 'build/\n') + self.git('add', '.') + self.git('commit', '-qm', 'base') + + def git(self, *args: 'str', cwd: 'Optional[pathlib.Path]' = None) -> 'str': + result = subprocess.run(['git', *args], cwd=cwd or self.repo, env=self.env, + capture_output=True, text=True, check=False, timeout=scale_timeout(30)) + self.assertEqual(result.returncode, 0, result.stderr) + return result.stdout + + def write(self, path: 'str', text: 'str') -> None: + file = self.repo / path + file.parent.mkdir(parents=True, exist_ok=True) + file.write_text(text, encoding='utf-8') + + def run_step(self) -> 'dict[str, str]': + """The step's outputs, each ``GITHUB_OUTPUT`` line read as ``key=value``.""" + script = self.root / 'step.sh' + script.write_text(step(*CHANGED_FILES_JOBS[0])['run'], encoding='utf-8') + output = self.root / 'step.output' + output.write_text('', encoding='utf-8') + result = subprocess.run(['bash', '-e', str(script)], cwd=self.repo, + env=dict(self.env, GITHUB_OUTPUT=str(output)), + capture_output=True, text=True, check=False, timeout=scale_timeout(60)) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + outputs = {} # type: dict[str, str] + for line in output.read_text(encoding='utf-8').splitlines(): + key, sep, value = line.partition('=') + self.assertTrue(sep, f'{line!r} is not a `key=value` line') + self.assertNotIn(key, outputs) + outputs[key] = value + return outputs + + def test_nothing_changed(self) -> None: + self.write('build/out.txt', 'ignored\n') + self.assertEqual(self.run_step(), {'files_changed': 'false'}) + + def test_modified_untracked_and_deleted_files(self) -> None: + self.write('pcapkit/const/a.py', 'x = 2\n') + self.write('pcapkit/const/new/c.py', 'x = 3\n') + (self.repo / 'pcapkit/const/b.py').unlink() + self.write('build/out.txt', 'ignored\n') + self.assertEqual(self.run_step(), { + 'files_changed': 'true', + 'changed_files': 'pcapkit/const/a.py pcapkit/const/b.py pcapkit/const/new/c.py', + }) + + def test_a_staged_change_counts(self) -> None: + """``git commit -a`` carries it, so it is a change to commit.""" + self.write('conda/requirements.txt', 'y\n') + self.git('add', 'conda/requirements.txt') + self.assertEqual(self.run_step(), {'files_changed': 'true', 'changed_files': 'conda/requirements.txt'}) + + def test_awkward_names_stay_one_line_and_never_run(self) -> None: + """git C-quotes a space, a newline or a quote, so the output file stays + ``key=value`` lines, and a name holding ``$(...)`` is data. + + """ + names = ('pcapkit/const/a b.py', 'pcapkit/const/n\nl.py', 'pcapkit/const/q"t.py', + 'pcapkit/const/$(touch ran).py', 'pcapkit/const/`touch ran`.py') + for name in names: + self.write(name, 'x = 4\n') + outputs = self.run_step() + self.assertEqual(outputs['files_changed'], 'true') + self.assertEqual(set(outputs), {'files_changed', 'changed_files'}) + for quoted in ('"pcapkit/const/$(touch ran).py"', '"pcapkit/const/`touch ran`.py"', + '"pcapkit/const/a b.py"', '"pcapkit/const/n\\nl.py"', '"pcapkit/const/q\\"t.py"'): + self.assertIn(quoted, outputs['changed_files']) + self.assertFalse((self.repo / 'ran').exists()) + self.assertFalse((self.repo / 'pcapkit/const/ran').exists()) + + +@unittest.skipIf(yaml is None, 'PyYAML, from the test extra, is not installed') +class TestEveryWorkflowDeclaresPermissions(unittest.TestCase): + """A top-level ``permissions:`` in every workflow, and no write scope for a pull request's build.""" + + def test_every_workflow_declares_them(self) -> None: + paths = sorted(WORKFLOWS.glob('*.yml')) + self.assertTrue(paths) + for path in paths: + with self.subTest(workflow=path.name): + self.assertIn('permissions', workflow(path.name)) + + def test_where_every_job_declares_its_own_the_default_is_nothing(self) -> None: + """So the top-level block changes none of them.""" + for name in ('create-release.yml', 'codeql-analysis.yml'): + spec = workflow(name) + with self.subTest(workflow=name): + self.assertEqual(spec['permissions'], {}) + for job_id, job in spec['jobs'].items(): + with self.subTest(job=job_id): + self.assertIn('permissions', job) + + def test_a_pull_request_builds_the_docs_with_read_only_contents(self) -> None: + spec = workflow('deploy-pages.yml') + jobs = job_blocks((WORKFLOWS / 'deploy-pages.yml').read_text(encoding='utf-8')) + self.assertEqual(spec['permissions'], {'contents': 'read'}) + build, publish = spec['jobs']['deploy-pages'], spec['jobs']['publish-pages'] + self.assertNotIn('permissions', build) + self.assertEqual(publish['permissions'], {'actions': 'read', 'contents': 'write'}) + self.assertEqual(publish['needs'], ['deploy-pages']) + + # `(event, unit-tests, verdict, deploy-pages, whether publish-pages runs)`. + # One of the first two always skips, and a skip anywhere in the chain + # skips a job left with the implicit `success()` (#1632). + rows = ( + ('workflow_run', 'skipped', 'success', 'success', True), + ('schedule', 'success', 'skipped', 'success', True), + ('pull_request', 'skipped', 'skipped', 'success', False), + ('workflow_run', 'skipped', 'success', 'skipped', False), + ('workflow_run', 'skipped', 'success', 'failure', False), + ('schedule', 'success', 'skipped', 'cancelled', False), + ) + for event, gate, verdict, built, expected in rows: + context = {STATUS: {'cancelled': False}, 'github': {'event_name': event, 'event': {}}, 'needs': { + 'unit-tests': {'result': gate}, 'verdict': {'result': verdict}, + 'deploy-pages': {'result': built}, + }} # type: dict[str, object] + with self.subTest(event=event, built=built): + self.assertEqual(job_runs(jobs, 'publish-pages', context), expected) + + # Only the job that runs no project code holds the write token, and + # only it pushes. + for job_id, job in spec['jobs'].items(): + uses = [str(entry.get('uses', '')) for entry in job.get('steps') or []] + with self.subTest(job=job_id): + pushes = any(entry.startswith('JamesIves/github-pages-deploy-action@') for entry in uses) + self.assertEqual(pushes, job_id == 'publish-pages') + writes = 'write' in (job.get('permissions') or spec['permissions']).values() + self.assertEqual(writes, job_id == 'publish-pages') + self.assertFalse([entry for entry in publish['steps'] if 'pip install' in str(entry.get('run', ''))]) + + def test_the_built_site_reaches_the_publish_job_whole(self) -> None: + """``.nojekyll`` is a hidden file, which upload-artifact drops by default.""" + build = workflow('deploy-pages.yml')['jobs']['deploy-pages']['steps'] + upload = [entry for entry in build if str(entry.get('uses', '')).startswith('actions/upload-artifact@')] + self.assertEqual(len(upload), 1) + self.assertEqual(upload[0]['if'], "${{ github.event_name != 'pull_request' }}") + self.assertEqual(upload[0]['with']['path'], 'docs/build/html') + self.assertIs(upload[0]['with']['include-hidden-files'], True) + publish = workflow('deploy-pages.yml')['jobs']['publish-pages']['steps'] + download = next(entry for entry in publish if entry.get('name') == 'Download the built site') + self.assertIn(f"--name {upload[0]['with']['name']}", download['run']) + self.assertIn('--dir docs/build/html', download['run']) + deploy = next(entry for entry in publish if 'JamesIves' in str(entry.get('uses', ''))) + self.assertEqual(deploy['with']['folder'], 'docs/build/html') + + +if __name__ == '__main__': + unittest.main() diff --git a/util/vendor_code_changed.py b/util/vendor_code_changed.py index 615a3f426..9506141f8 100644 --- a/util/vendor_code_changed.py +++ b/util/vendor_code_changed.py @@ -2,7 +2,8 @@ """Whether a Vendor Update regeneration changed code, or only comments and docstrings. :file:`.github/workflows/cron-vendor.yml` runs ``pcapkit-vendor`` and isort, then -``tj-actions/verify-changed-files``. Without this script any changed file would +lists the changed files with ``git status`` in its ``Verify Changed files`` step. +Without this script any changed file would bump the version, and every bump starts a release. Of the bumps 1.5.0b5 to b12, all but b5 and b8 changed only comments or docstrings by this script's rule: b9 a draft becoming RFC 10049, b10 and b11 reference formatting (#1664). The owner's ruling @@ -25,15 +26,15 @@ imports and every other expression stay in the dump. Any other path is *code*: a new file, a deleted one, a non-Python file, a file outside the generated tree, a mode or type change, or a file that does not parse. So is a path that is -listed but cannot be read, such as one ``verify-changed-files`` quoted or -escaped, and one identical to ``HEAD``. Only a positive finding that a file +listed but cannot be read, such as one ``git status`` quoted, and one +identical to ``HEAD``. Only a positive finding that a file differs in comments or docstrings alone spares the bump. The source is parsed as bytes, so a coding cookie still decides how its string literals decode. Paths are the arguments, or else ``$CHANGED_FILES`` split on whitespace, the -separator ``verify-changed-files`` uses. Writes ``code_changed=true`` or +separator that step uses. Writes ``code_changed=true`` or ``code_changed=false`` to ``$GITHUB_OUTPUT``. Exits non-zero, writing nothing, only when it has no paths to judge, and the workflow then bumps. From 7a4a2cf719acbad27ff10002377e54249c5dbd8b Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 19 Oct 2026 21:27:36 -0400 Subject: [PATCH 2/2] ci(pages): never publish a build that main has moved past (#1968) - deploy-pages: each deploy names its commit in the gh-pages commit message, and `publish-pages` first reads the commit gh-pages last published; when this run's commit is behind it on main, the deploy is skipped with a notice. Anything it cannot read publishes, as before - tests: run the check against a stubbed `gh`, and pin that the deploy waits on it and writes the message it reads Refs #1968 --- .github/workflows/deploy-pages.yml | 32 ++++++- docs/source/contributing/workflows.rst | 4 +- tests/project/test_workflow_hardening_1968.py | 87 +++++++++++++++++++ 3 files changed, 121 insertions(+), 2 deletions(-) diff --git a/.github/workflows/deploy-pages.yml b/.github/workflows/deploy-pages.yml index a901a937c..59a6286fe 100644 --- a/.github/workflows/deploy-pages.yml +++ b/.github/workflows/deploy-pages.yml @@ -192,7 +192,7 @@ jobs: # result is checked explicitly, under `!cancelled()`: `unit-tests` or `verdict` # always skips, and a skip anywhere in the chain skips a job left with the # implicit `success()` (#1632). Its own group keeps one push to `gh-pages` at - # a time. + # a time, and its check below keeps an older build from replacing a newer one. publish-pages: name: Publish to gh-pages needs: [ deploy-pages ] @@ -221,10 +221,40 @@ jobs: gh run download "$GITHUB_RUN_ID" --repo "$GITHUB_REPOSITORY" --name pages-html --dir docs/build/html + # Builds finish in any order, and one group per job cannot order them: a + # job joining a group cancels the one pending there, so sharing the build's + # group would cancel the next build. So each deploy names its commit, and + # this step reads the commit `gh-pages` last published. Where that one is + # newer than this run's -- this run's commit is behind it on `main` -- the + # deploy is skipped. Anything else publishes, as before: no `gh-pages`, a + # head this workflow did not write, an API error, the same commit, or one + # that is not an ancestor. A head check alone would also skip when the + # newer commit's own run never publishes, a red one, leaving the docs + # older than this build. + - name: Check gh-pages holds no newer build + id: fresh + env: + GH_TOKEN: ${{ github.token }} + run: | + published="$(gh api "repos/$GITHUB_REPOSITORY/commits/gh-pages" --jq .commit.message | + sed -n 's/^Publish the documentation of \([0-9a-f]\{40\}\)$/\1/p' | head -n 1)" + if [ -n "$published" ] && [ "$published" != "$GITHUB_SHA" ]; then + status="$(gh api "repos/$GITHUB_REPOSITORY/compare/$published...$GITHUB_SHA" --jq .status || true)" + if [ "$status" = behind ]; then + echo "::notice title=Newer documentation already published::gh-pages holds the build of $published, which $GITHUB_SHA is behind on main, so this older build is not published" + echo "publish=false" >> "$GITHUB_OUTPUT" + exit 0 + fi + fi + echo "publish=true" >> "$GITHUB_OUTPUT" + - name: Deploy 🚀 + if: steps.fresh.outputs.publish == 'true' uses: JamesIves/github-pages-deploy-action@v4 with: token: ${{ secrets.GITHUB_TOKEN }} branch: gh-pages # The branch the action should deploy to. folder: docs/build/html # The folder the action should deploy. clean: true # Automatically remove deleted files from the deploy branch + # Read back by the step above; keep the two in step. + commit-message: Publish the documentation of ${{ github.sha }} diff --git a/docs/source/contributing/workflows.rst b/docs/source/contributing/workflows.rst index b0b24aae8..bfdac0b67 100644 --- a/docs/source/contributing/workflows.rst +++ b/docs/source/contributing/workflows.rst @@ -1018,7 +1018,9 @@ No other workflow in scope here declares one -- including through an ``environment:``-gated deployment. Its ``deploy-pages`` job builds the site, a pull request's included, with ``contents: read``, and uploads it for ``publish-pages``, which alone holds ``contents: write`` and never runs on -a pull request +a pull request. Its ``Check gh-pages holds no newer build`` step skips the +deploy when ``gh-pages`` already holds the build of a commit this run's commit +is behind, so a build that finishes late cannot replace newer documentation (`#1968 `__). Of these environments, only ``github-release`` carries a required reviewer; :doc:`releasing` covers why one approval on that single environment is enough to gate the diff --git a/tests/project/test_workflow_hardening_1968.py b/tests/project/test_workflow_hardening_1968.py index 3084b07c8..9bc4a74cf 100644 --- a/tests/project/test_workflow_hardening_1968.py +++ b/tests/project/test_workflow_hardening_1968.py @@ -264,6 +264,13 @@ def test_a_pull_request_builds_the_docs_with_read_only_contents(self) -> None: self.assertEqual(writes, job_id == 'publish-pages') self.assertFalse([entry for entry in publish['steps'] if 'pip install' in str(entry.get('run', ''))]) + def test_the_deploy_waits_on_the_freshness_check(self) -> None: + steps = workflow('deploy-pages.yml')['jobs']['publish-pages']['steps'] + ids = [entry.get('id') for entry in steps] + deploy = next(index for index, entry in enumerate(steps) if 'JamesIves' in str(entry.get('uses', ''))) + self.assertLess(ids.index('fresh'), deploy) + self.assertEqual(steps[deploy]['if'], "steps.fresh.outputs.publish == 'true'") + def test_the_built_site_reaches_the_publish_job_whole(self) -> None: """``.nojekyll`` is a hidden file, which upload-artifact drops by default.""" build = workflow('deploy-pages.yml')['jobs']['deploy-pages']['steps'] @@ -280,5 +287,85 @@ def test_the_built_site_reaches_the_publish_job_whole(self) -> None: self.assertEqual(deploy['with']['folder'], 'docs/build/html') +#: A ``gh`` that answers the two ``gh api`` calls the check makes from the +#: environment, and logs each call. ``$2`` is the endpoint. +GH_STUB = '''#!/bin/sh +printf '%s\\n' "$*" >> "$STUB_LOG" +case "$2" in + */commits/gh-pages) [ -z "$STUB_COMMITS_FAIL" ] || exit 1; printf '%s\\n' "$STUB_MESSAGE" ;; + */compare/*) [ -z "$STUB_COMPARE_FAIL" ] || exit 1; printf '%s\\n' "$STUB_STATUS" ;; + *) exit 2 ;; +esac +''' + +#: This run's commit, and the one ``gh-pages`` last published. +OURS = '1' * 40 +THEIRS = '2' * 40 + + +@unittest.skipIf(yaml is None, 'PyYAML, from the test extra, is not installed') +class TestPublishSkipsAnOlderBuild(unittest.TestCase): + """``Check gh-pages holds no newer build``, run as ``bash -e`` runs it, ``gh`` stubbed. + + Builds finish in any order, so an older run's publish can reach ``gh-pages`` + after a newer one's; the check is what stops it replacing the newer docs. + + """ + + def setUp(self) -> None: + tmp = tempfile.TemporaryDirectory() + self.addCleanup(tmp.cleanup) + self.root = pathlib.Path(tmp.name) + stub = self.root / 'bin' / 'gh' + stub.parent.mkdir() + stub.write_text(GH_STUB, encoding='utf-8') + stub.chmod(0o755) + self.steps = workflow('deploy-pages.yml')['jobs']['publish-pages']['steps'] + self.check = next(entry for entry in self.steps if entry.get('id') == 'fresh') + deploy = next(entry for entry in self.steps if 'JamesIves' in str(entry.get('uses', ''))) + #: What the deploy writes as the ``gh-pages`` commit for ``THEIRS``. + self.ours_message = deploy['with']['commit-message'].replace('${{ github.sha }}', THEIRS) + + def run_check(self, message: 'str', status: 'str' = '', **flags: 'str') -> 'tuple[dict[str, str], str, str]': + """The check's outputs, its stdout, and the ``gh`` calls it made.""" + script = self.root / 'check.sh' + script.write_text(self.check['run'], encoding='utf-8') + output, log = self.root / 'check.output', self.root / 'gh.log' + output.write_text('', encoding='utf-8') + log.write_text('', encoding='utf-8') + env = {'PATH': f'{self.root / "bin"}{os.pathsep}{os.environ.get("PATH", "")}', 'LC_ALL': 'C', + 'GITHUB_SHA': OURS, 'GITHUB_REPOSITORY': 'JarryShaw/PyPCAPKit', 'GITHUB_OUTPUT': str(output), + 'STUB_LOG': str(log), 'STUB_MESSAGE': message, 'STUB_STATUS': status, **flags} + result = subprocess.run(['bash', '-e', str(script)], cwd=self.root, env=env, capture_output=True, + text=True, check=False, timeout=scale_timeout(30)) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + outputs = dict(line.split('=', 1) for line in output.read_text(encoding='utf-8').splitlines()) + return outputs, result.stdout, log.read_text(encoding='utf-8') + + def test_a_newer_published_build_is_kept(self) -> None: + outputs, stdout, calls = self.run_check(self.ours_message, 'behind') + self.assertEqual(outputs, {'publish': 'false'}) + self.assertIn('::notice title=Newer documentation already published::', stdout) + self.assertIn(f'compare/{THEIRS}...{OURS}', calls) + + def test_an_older_or_unrelated_published_build_is_replaced(self) -> None: + for status in ('ahead', 'diverged', 'identical', ''): + with self.subTest(status=status): + self.assertEqual(self.run_check(self.ours_message, status)[0], {'publish': 'true'}) + + def test_a_rerun_of_the_published_commit_publishes(self) -> None: + message = self.ours_message.replace(THEIRS, OURS) + outputs, _, calls = self.run_check(message, 'behind') + self.assertEqual(outputs, {'publish': 'true'}) + self.assertNotIn('compare/', calls) + + def test_what_it_cannot_read_publishes_as_before(self) -> None: + """A head this workflow did not write, or an API error: the old behaviour.""" + default = f'Deploying to gh-pages from @ JarryShaw/PyPCAPKit@{THEIRS} \U0001f680' + self.assertEqual(self.run_check(default, 'behind')[0], {'publish': 'true'}) + self.assertEqual(self.run_check(self.ours_message, 'behind', STUB_COMMITS_FAIL='1')[0], {'publish': 'true'}) + self.assertEqual(self.run_check(self.ours_message, 'behind', STUB_COMPARE_FAIL='1')[0], {'publish': 'true'}) + + if __name__ == '__main__': unittest.main()