From 6d6f83593dd3bc4badd03a3495120e14253b2d4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20M=2E=20Requena=20Plens?= Date: Mon, 26 Oct 2026 17:45:29 +0100 Subject: [PATCH] ci: adopt the Developer Certificate of Origin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The project now asks every commit of a pull request to carry a Signed-off-by trailer naming its author, which is how the author certifies the Developer Certificate of Origin 1.1 for it. This is the OpenSSF Best Practices criterion dco, which until now could only be answered as not met. cmd/audit_dco judges the commits reachable from -head and not from -base. It reads each one from git with the trailers git finds in its message, through git log's %(trailers) placeholder, so a sign-off counts exactly when git sees one. A commit passes when one of its Signed-off-by trailers names its author, name and email compared without regard to case. It fails with no trailer, or with trailers that name only other people or nobody. A record of git log's output ends only where its separator byte is followed by a newline, which no field git prints holds, so a message carrying the separator is read as git reads it; git log is also held to the hashes git rev-list lists for the same range, so a sign-off can never move onto another commit. Text an author chose is printed with its control characters replaced, so a subject cannot start a workflow command of its own. A merge of two parents that brings no change of its own, which git show --remerge-diff prints nothing for and which is what GitHub's Update branch button writes, is reported and not judged. Every other merge is judged like any other commit: one that resolves a conflict, adds or drops a change while merging, or joins more than two parents. A declared bot's commits are exempt in a pull request that bot opened, and only there: Dependabot is the one declaration, with the identity its commits carry and the login that opens its pull requests, and a commit claiming that identity in a pull request somebody else opened is judged like any other. The release workflow's version stamp is pushed to main and never sits in a pull request, so nothing judges it. The description is not judged. GitHub copies the Signed-off-by trailers of a pull request's commits into the squash commit, below the description and once each, as the squash commits of #1512, #1529 and Dependabot's pull requests show, so main's history carries them without a trailer in the description. make check-dco runs it on origin/main..HEAD by default. ci.yml gains a dco job that runs it on every pull request with the event's base and head commits, the login that opened it and annotations, and skips itself on a push to main; the checks job waits on it and counts that skip as a pass, as it does for the image build. The event of a stacked pull request names the head of the layer below as its base, so each layer's pull request judges that layer's commits and no others, which the em dash job's comment now says too. CONTRIBUTING.md gains a Developer Certificate of Origin section, with a checklist item and the review standard's mention, and the pull request template a checkbox. GOVERNANCE.md lists the sign-off among a contributor's responsibilities, and the site's about page says it in English and Spanish. The repository settings page records web commit sign-off, which GitHub applies to commits made in its web interface such as an applied suggestion and which is off today. The git-commit skill commits with -s. The command reference, the repository map, CLAUDE.md, with the rule and the package counted (309 in the module, 336 directories holding tracked Go files), and the Copilot instructions name it; the testing reference that counts its tests is regenerated once, at the top of the stack. The tests were written first. Against stubs of every function, with ci.yml and the Makefile as they stood before this commit, 39 of the 40 test functions fail, 118 tests and subtests in all; the one that passes is the empty log's, whose answer is the stub's. They now cover every statement, all 104 conditions and all 80 branches, and gremlins kills all 58 mutants. Refs #1269 Signed-off-by: José M. Requena Plens --- .github/copilot-instructions.md | 1 + .github/pull_request_template.md | 1 + .github/skills/git-commit/SKILL.md | 5 +- .github/workflows/ci.yml | 57 ++- CLAUDE.md | 10 +- CONTRIBUTING.md | 26 +- GOVERNANCE.md | 2 +- Makefile | 15 +- cmd/audit_dco/bots.go | 52 +++ cmd/audit_dco/bots_test.go | 81 ++++ cmd/audit_dco/doc.go | 80 ++++ cmd/audit_dco/git.go | 170 ++++++++ cmd/audit_dco/git_test.go | 522 ++++++++++++++++++++++++ cmd/audit_dco/main.go | 77 ++++ cmd/audit_dco/main_test.go | 457 +++++++++++++++++++++ cmd/audit_dco/report.go | 135 ++++++ cmd/audit_dco/report_test.go | 211 ++++++++++ cmd/audit_dco/signoff.go | 110 +++++ cmd/audit_dco/signoff_test.go | 261 ++++++++++++ docs/development/cmd-utilities.md | 38 ++ docs/development/repository-map.md | 1 + docs/development/repository-settings.md | 43 +- site/src/content/docs/about.mdx | 2 + site/src/content/docs/es/about.mdx | 2 + 24 files changed, 2342 insertions(+), 17 deletions(-) create mode 100644 cmd/audit_dco/bots.go create mode 100644 cmd/audit_dco/bots_test.go create mode 100644 cmd/audit_dco/doc.go create mode 100644 cmd/audit_dco/git.go create mode 100644 cmd/audit_dco/git_test.go create mode 100644 cmd/audit_dco/main.go create mode 100644 cmd/audit_dco/main_test.go create mode 100644 cmd/audit_dco/report.go create mode 100644 cmd/audit_dco/report_test.go create mode 100644 cmd/audit_dco/signoff.go create mode 100644 cmd/audit_dco/signoff_test.go diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 50191692f..63f0d9779 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -31,6 +31,7 @@ gitlab-mcp-server/ │ ├── audit_image_vulns/ # The image's Alpine packages against Trivy's vulnerability database before the push (make check-image-vulns) │ ├── reproduce_release/ # Release binaries rebuilt from the tag and compared byte for byte (make check-reproducible, make check-repeatable) │ ├── audit_site_headers/ # The documentation site's security headers, judged after each deploy (make check-site-headers) +│ ├── audit_dco/ # Every commit of a pull request signed off by its author, the Developer Certificate of Origin (make check-dco) │ ├── audit_surface_quality/ # MCP surface metadata + output quality (-view; was audit_tools + audit_output) │ ├── audit_supply_chain/ # Release-configuration invariants (SHA-pinned uses:, Dependabot cooldowns, SECURITY.md, installers) │ ├── audit_tokens/ # Token overhead; -footprint, --compare-schemas (was audit_meta_schema); cl100k_base tokenizer (tokens.go) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 06bd1d179..81ee1acde 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -66,6 +66,7 @@ What a review checks and what a merge requires are in [Code Review](https://gith - [ ] Contributor-facing change (a gate, a generator, a convention): `docs/development/` or `CLAUDE.md` updated - [ ] If introducing/removing a tool: tool reference regenerated (`make gen-tool-reference`), the site's counts refreshed (`make gen-site-stats`) and the fine-grained table regenerated (`make gen-action-grants`); generated pages are never edited by hand (`make update-all` runs every generator that needs no measurement, GitLab instance or paid run) - [ ] Commit messages follow [Conventional Commits](https://www.conventionalcommits.org/) +- [ ] Every commit is signed off by its author (`git commit -s`), which certifies the [Developer Certificate of Origin](https://github.com/jmrplens/gitlab-mcp-server/blob/main/CONTRIBUTING.md#developer-certificate-of-origin) for it; `make check-dco` lists any that is not ### Security diff --git a/.github/skills/git-commit/SKILL.md b/.github/skills/git-commit/SKILL.md index f230957cb..c927c4423 100644 --- a/.github/skills/git-commit/SKILL.md +++ b/.github/skills/git-commit/SKILL.md @@ -94,10 +94,10 @@ Analyze the diff to determine: ```bash # Single line -git commit -m "[scope]: " +git commit -s -m "[scope]: " # Multi-line with body/footer -git commit -m "$(cat <<'EOF' +git commit -s -m "$(cat <<'EOF' [scope]: @@ -114,6 +114,7 @@ EOF - Imperative mood: "fix bug" not "fixes bug" - Reference issues: `Closes #123`, `Refs #456` - Keep description under 72 characters +- Sign off every commit (`-s`), which certifies the [Developer Certificate of Origin](../../../CONTRIBUTING.md#developer-certificate-of-origin) for it; CI's `dco` job fails a pull request with a commit that lacks the sign-off, and `make check-dco` lists any before a push. A sign-off is the author's, not AI attribution ## Git Safety Protocol diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fa176f1be..ebdaf7212 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -16,8 +16,8 @@ # docker (×2) ─── docker-build # cross_platform (×3) ─── cross_platform_summary # generated · golangci-lint · govulncheck · markdown · site-lint · site-build -# hadolint · supply-chain · docs-audit · em-dash ─── checks -# └──► ci +# hadolint · supply-chain · docs-audit · em-dash · dco ─── checks +# └──► ci # # Every edge joins neighbouring columns, and that is deliberate. GitHub draws # a job with no parent and no child at the end of the first column, in one box @@ -1171,10 +1171,10 @@ jobs: fetch-depth: 0 - name: Check the lines this change adds env: - # On a pull request, the commit the branch grew from, which for a - # stacked pull request is the stack's base rather than the layer - # below, since GitHub tests a stacked layer as the stack merged into - # that base. On a push to main, the commit that was there before. + # On a pull request, the base commit its event names, which for a + # stacked pull request is the head of the layer below, so each + # layer is checked for the lines it adds itself. On a push to main, + # the commit that was there before. EM_DASH_BASE: ${{ github.event_name == 'pull_request' && github.event.pull_request.base.sha || github.event.before }} run: make check-em-dash # plan/ is ignored by git, so a file there is tracked only when it was @@ -1183,6 +1183,44 @@ jobs: - name: Check nothing under plan/ is tracked run: make check-plan-untracked + # The Developer Certificate of Origin: every commit a pull request brings + # carries a Signed-off-by trailer naming its author, which is how the author + # certifies the certificate for it (CONTRIBUTING.md, Developer Certificate of + # Origin). The commits judged are the ones between the base and the head the + # event names, the range the em dash job reads. A stacked pull request's + # event names the head of the layer below as the base, so each layer's + # pull request judges that layer's commits and no others. A merge commit + # that brings no change of its own is not judged, and Dependabot's commits + # are exempt in a pull request Dependabot opened, which is why the job is + # told who opened it. + # + # A push to main has nothing for it to judge, so the job skips itself there + # and the verdict below counts the skip as a pass: main receives the squash + # commit, into which GitHub copies the sign-offs of the pull request's + # commits, and the release workflow's version stamp, which never sits in a + # pull request. + dco: + name: "✍️ DCO sign-off" + if: github.event_name == 'pull_request' + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false + # The whole history, like the em dash job: the range runs from the + # pull request's base, which a shallow clone does not hold. + fetch-depth: 0 + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7 + with: + go-version-file: go.mod + - name: Check every commit is signed off by its author + env: + DCO_BASE: ${{ github.event.pull_request.base.sha }} + DCO_HEAD: ${{ github.event.pull_request.head.sha }} + DCO_OPENED_BY: ${{ github.event.pull_request.user.login }} + run: make check-dco DCO_FLAGS=-annotate + # One node for the jobs that depend on nothing. They are fanned in here # rather than straight into "ci" for the graph's sake, explained at the top # of the file: this node lands below the matrix summaries, because its @@ -1218,6 +1256,7 @@ jobs: - supply-chain - docs-audit - em-dash + - dco - coverage - compile - typecheck @@ -1236,9 +1275,9 @@ jobs: env: RESULTS: ${{ toJSON(needs) }} INFORMATIONAL: docs-audit - # The image build only runs for a pull request; on a push it is - # skipped by design rather than failed. - MAY_SKIP: docker-build + # The image build and the sign-off check only run for a pull + # request; on a push they are skipped by design rather than failed. + MAY_SKIP: docker-build dco run: bash .github/scripts/needs-verdict.sh # The other half of the em dash gate, and the half no later commit can # fix: this repository squash merges, so the pull request title and body diff --git a/CLAUDE.md b/CLAUDE.md index fa2199d01..763cdb188 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -23,6 +23,13 @@ > documents, or a skip-checks trailer), not even quoted in backticks: the squash commit of #1094 carried one, and > main ran no CI and the v3.1.0 tag on it no release. `make check-pr-description` > refuses it. +> +> **Every commit is signed off** (`git commit -s`): a `Signed-off-by:` trailer naming +> its author certifies the Developer Certificate of Origin, and the `dco` job of CI +> fails a pull request with a commit that lacks one. A sign-off is a person's, so it is +> not AI attribution and the rule above still holds. `make check-dco` judges the +> branch before a push, and [CONTRIBUTING](CONTRIBUTING.md#developer-certificate-of-origin) +> says what it certifies and how to sign off commits already made. **This file is loaded into every session, so it holds the rules and an index, and the long-form references live in `docs/development/`**, read when a task needs them: [Repository Map](docs/development/repository-map.md) (the annotated tree of commands and packages), [Environment Variables and Flags](docs/development/configuration.md), [Test Infrastructure](docs/development/test-infrastructure.md), [Request Paths and Fine-Grained Grants](docs/development/request-paths.md) (R-PATH, R-PAGE and R-GRANT), [Adding or Changing a Limit](docs/development/adding-a-limit.md), [Release Process](docs/development/release-process.md), [Tier Gating](docs/development/tier-gating.md), [HTTP Mode Invariants](docs/development/http-invariants.md) and [Resource Subscriptions](docs/development/resource-subscriptions.md). Keep it that way: what a new section adds here is the rule and a link, and its explanation goes to the page that owns the topic. Claude Code warns at every start once this file passes a size limit that scales with the model's context window and never falls below 40,000 characters (150,000 for current models in a one-million-token window). @@ -54,7 +61,7 @@ | MCP Icons | 51 icons (50 domain + brand mark), each a 3-entry `[]mcp.Icon`: one SVG (base64 data URI, `Sizes: ["any"]`, `currentColor`) plus light/dark 16×16 lossless WebP fallbacks (`Theme`-tagged, `cmd/gen_icon_webp`) for clients that reject SVG. The brand mark is the generated "fan-out" (`cmd/gen_brand` → `brandmark_gen.go`), original artwork replacing the former tanuki | | Source files (tools) | 788 non-test Go files under `internal/tools/` | | Test files (tools) | 488 test files under `internal/tools/` | -| Go packages | 308 in the module (`go list ./...`); 335 directories hold tracked `.go` files, the twenty-seven more being what `go list` leaves out: twenty-six `testdata` fixture packages (the two stand-in programs, the coverage audit's planted trees, the closed-server audit's planted module, the stand-in module the SDK route reader's tests read and the platform module the condition measurement's discovery lists) and `test/e2e/modeleval/internal/provider`, whose every file sits behind a build tag. 182 under `internal/tools/...` (the root package plus 181 sub-packages) | +| Go packages | 309 in the module (`go list ./...`); 336 directories hold tracked `.go` files, the twenty-seven more being what `go list` leaves out: twenty-six `testdata` fixture packages (the two stand-in programs, the coverage audit's planted trees, the closed-server audit's planted module, the stand-in module the SDK route reader's tests read and the platform module the condition measurement's discovery lists) and `test/e2e/modeleval/internal/provider`, whose every file sits behind a build tag. 182 under `internal/tools/...` (the root package plus 181 sub-packages) | ### Orbit live tests @@ -85,6 +92,7 @@ gitlab-mcp-server/ │ ├── audit_image_vulns/ # The image's Alpine packages against Trivy's database, before the push (make check-image-vulns) │ ├── reproduce_release/ # Release binaries rebuilt from the tag and compared byte for byte (make check-reproducible, check-repeatable) │ ├── audit_site_headers/ # The documentation site's security headers, after each deploy (make check-site-headers) +│ ├── audit_dco/ # Every commit of a pull request signed off by its author, the DCO (make check-dco) │ ├── godoc_tool/ # Go doc auditor and fixer (make check-godocs) │ ├── audit_md_escaping/ # GitLab-authored values reaching Markdown unescaped (make check-md-escaping) │ ├── audit_metrics/ # Tool, resource and prompt metrics diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e0839927a..a5f04f10a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -13,6 +13,7 @@ How the project makes decisions, who holds which role and what it depends on to - [Development Setup](#development-setup) - [Branch Naming](#branch-naming) - [Commit Messages](#commit-messages) +- [Developer Certificate of Origin](#developer-certificate-of-origin) - [Pull Requests](#pull-requests) - [Code Review](#code-review) - [Code Standards](#code-standards) @@ -132,6 +133,26 @@ test(branches): increase coverage to 90% docs(site): describe the wiki tools in English and Spanish ``` +## Developer Certificate of Origin + +Every commit in a pull request is signed off: its message ends with a `Signed-off-by:` trailer naming the commit's author, with the name and email the commit is authored under. The sign-off is how the author certifies the [Developer Certificate of Origin](https://developercertificate.org/) 1.1 for that commit, which in short says that the contribution is theirs to submit under the project's [MIT license](LICENSE): they wrote it, or it is based on earlier work under a compatible open source license that they may submit with their changes, or it was given to them by somebody who certified one of those and they have not changed it; and that the contribution and its sign-off are public and kept, and may be redistributed under the license. The certificate's own text is the one that applies. + +```bash +git commit -s -m "fix(config): handle empty GITLAB_URL gracefully" +``` + +`-s` (`--signoff`) writes the trailer from your git identity, so it names you when you are the author: + +```text +Signed-off-by: Jane Doe +``` + +To sign off commits already on your branch where they stand, rebase it on its own base and push it again: `git rebase --keep-base --signoff origin/main`, then `git push --force-with-lease`. A commit somebody else authored is signed off by that person. A sign-off is not a signature: signing commits with a GPG or SSH key is welcome, and it does not replace the trailer. + +`make check-dco` judges the commits of your branch against `origin/main` before you push, and the `✍️ DCO sign-off` job of CI judges every pull request the same way, from the base commit its event names to its head; for a stacked pull request that base is the head of the layer below, so each layer's pull request judges that layer's commits. It reads the trailer the way git does, and passes a commit when one of its `Signed-off-by` trailers names its author, the name and the email compared without regard to case. A merge commit that brings no change of its own, such as the one GitHub's Update branch button writes, is not judged; a merge that resolves a conflict or changes anything while merging is judged like any other commit, so conclude it with `git commit -s`. Dependabot's commits are exempt in the pull requests Dependabot opens, because an app cannot certify anything and what it commits is a dependency bump; the same commit in a pull request somebody else opened is judged like any other. The release workflow's version stamp reaches `main` without a pull request, so nothing judges it. The [command's reference](docs/development/cmd-utilities.md#audit_dco) says what else it refuses. + +GitHub signs off a commit made in its web interface, such as an applied review suggestion, when the repository requires sign-off for web commits; that setting is not on yet ([Repository Settings](docs/development/repository-settings.md#web-commit-sign-off)), so until it is, such a commit needs the trailer typed into its message. The squash commit that lands on `main` needs nothing more: GitHub copies the sign-offs of the pull request's commits into it, below the description, so the description carries no trailer of its own. + ## Pull Requests ### Before Submitting @@ -142,6 +163,7 @@ docs(site): describe the wiki tools in English and Spanish - [ ] New tools have tests, and total coverage stays at or above the 90% CI enforces - [ ] Documentation is updated where the change shows: the site page for user-facing behavior (English and Spanish), `docs/development/` for contributor-facing changes (see [Documentation](#documentation)) - [ ] Commit messages follow conventional commits +- [ ] Every commit is signed off by its author (`git commit -s`), which certifies the [Developer Certificate of Origin](#developer-certificate-of-origin); `make check-dco` says which are not ### PR Process @@ -176,11 +198,11 @@ Every change reaches `main` through a pull request, except the version stamp the - **The published surface.** Tool and action names, input and output schemas, annotations and descriptions are deliberate, and what a model reads names only actions and tools the server serves. - **Code standards.** The change follows [Code Standards](#code-standards), and any exception the linters allow is marked where it occurs with the reason. - **Documentation.** A change a user can see updates its site page in English and Spanish together, a change to how the repository works updates `docs/development/`, and every generated artifact is regenerated rather than edited (see [Documentation](#documentation)). -- **History.** Commit messages follow [Commit Messages](#commit-messages), and the description, which becomes the squash commit's message, carries no skip command and no block a review bot injected. +- **History.** Commit messages follow [Commit Messages](#commit-messages), every commit is signed off by its author as [Developer Certificate of Origin](#developer-certificate-of-origin) requires, and the description, which becomes the squash commit's message, carries no skip command and no block a review bot injected. ### What a change needs to be accepted -- The required `✅ CI` check passes. It aggregates the build, the tests on Linux, macOS and Windows, the 90% coverage floor, the linters and the repository's own gates, `make check-pr-description` among them. +- The required `✅ CI` check passes. It aggregates the build, the tests on Linux, macOS and Windows, the 90% coverage floor, the linters and the repository's own gates, `make check-pr-description` and the sign-off check among them. - Every review thread is resolved. The branch rules refuse the merge otherwise, except to the repository admin role, which is on their bypass list; the maintainer, who holds that role, merges a pull request only once this item and the one above hold. - A contributor's pull request completes the template's checklist, with any item that does not apply marked so. The maintainer's own pull requests carry the same information in the description's account of what was run. - The maintainer agrees the change belongs in the project. A correct change can still be declined when it falls outside the [roadmap](https://docs.jmrp.io/gitlab-mcp-server/roadmap/) or a recorded decision; the reason is given in the pull request. diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 042c5de51..8b76f386a 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -28,7 +28,7 @@ A disagreement is raised where it arises, in the issue or the pull request, with | Role | Held today by | Responsibilities | | ----------------- | ---------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Maintainer | José M. Requena Plens ([@jmrplens](https://github.com/jmrplens)) | Triage every issue; review every pull request a contributor opens; merge each pull request that meets Code review; cut each release (the written summary, the signed tag, and watching every channel publish); handle each vulnerability report as SECURITY.md sets out; enforce the Code of Conduct; keep dependencies current; hold the access and the accounts listed under [Continuity](#continuity) and keep their settings recorded | -| Contributor | Anyone who opens an issue, comments or sends a pull request | Follow CONTRIBUTING.md and the Code of Conduct; answer every review thread on their pull request, with a change or the reason for none; keep its checks passing | +| Contributor | Anyone who opens an issue, comments or sends a pull request | Follow CONTRIBUTING.md and the Code of Conduct; sign off every commit, which certifies the Developer Certificate of Origin for it; answer every review thread on their pull request, with a change or the reason for none; keep its checks passing | | Security reporter | Anyone who reports a vulnerability | Report it privately as SECURITY.md describes, and keep it private for as long as SECURITY.md sets out | Automation is not a role. Dependabot, the automated code reviewers and the workflows act within the permissions the repository gives them: their pull requests and review comments are read and answered like anyone's, and none of them merges a pull request. The one write to `main` that no pull request carries is the version stamp the release workflow pushes with a deploy key after a release, which [`docs/development/repository-settings.md`](docs/development/repository-settings.md#sc-01-keep-the-deploy-key-ruleset-bypass-declared) records. diff --git a/Makefile b/Makefile index 0e7730b5a..b9805117b 100644 --- a/Makefile +++ b/Makefile @@ -14,7 +14,7 @@ audit-discovery audit-discovery-check audit-e2e-gaps audit-e2e-coverage e2e-go-coverage check-e2e-static audit-gateway-chars check-gateway-chars check-test-file-names audit-test-subtests check-test-subtests check-supply-chain \ e2e-coverage-record e2e-coverage-record-ce e2e-coverage-record-ee e2e-coverage-record-render check-e2e-coverage-record check-e2e-coverage-page \ audit-md-escaping check-md-escaping \ - check-em-dash check-pr-description check-plan-untracked \ + check-em-dash check-pr-description check-plan-untracked check-dco \ audit-action-ids check-action-ids \ audit-dead-consts check-dead-consts \ audit-sdk-context check-sdk-context \ @@ -2600,6 +2600,19 @@ check-em-dash: check-pr-description: scripts/check-em-dash.sh description +## check-dco: fail when a commit of DCO_BASE..DCO_HEAD (origin/main..HEAD by +## default) carries no Signed-off-by trailer naming its author, the sign-off +## that certifies the Developer Certificate of Origin for it. A merge commit +## that brings no change of its own is not judged, and a declared bot's commits +## (cmd/audit_dco/bots.go) are exempt in a pull request that bot opened, which +## DCO_OPENED_BY names by its GitHub login. It reads git and nothing else, so run it before pushing; CI's dco job +## runs it on every pull request with the event's base and head, and +## DCO_FLAGS=-annotate. +DCO_BASE ?= origin/main +DCO_HEAD ?= HEAD +check-dco: + go run ./cmd/audit_dco/ -base "$(DCO_BASE)" -head "$(DCO_HEAD)" -opened-by "$(DCO_OPENED_BY)" $(DCO_FLAGS) + ## check-plan-untracked: fail when git tracks a file under plan/, a working area ## .gitignore excludes, so a file there is tracked only if it was force-added. ## It reads the index, so a file untracked with git rm --cached passes before diff --git a/cmd/audit_dco/bots.go b/cmd/audit_dco/bots.go new file mode 100644 index 000000000..d12fcab57 --- /dev/null +++ b/cmd/audit_dco/bots.go @@ -0,0 +1,52 @@ +package main + +import "strings" + +// declaredBot is an app whose commits are exempt from the sign-off in a pull +// request it opened: the author identity its commits carry, the GitHub login +// that opens its pull requests, and why it is exempt. +type declaredBot struct { + author identity + login string + reason string +} + +// declaredBots are the apps exempt from the sign-off. A declaration needs the +// identity and the login both, because the exemption holds only where they +// agree: a commit carrying the identity in a pull request opened by anybody +// else is judged like any other, since writing an app's name into a commit is +// something anybody can do and opening a pull request as the app is not. +// +// The release workflow's version stamp is not declared here: it is pushed to +// main after a release and never sits in a pull request, which is the only +// place this command reads commits. +var declaredBots = []declaredBot{ + { + author: identity{name: "dependabot[bot]", email: "49699333+dependabot[bot]@users.noreply.github.com"}, + login: "dependabot[bot]", + reason: "Dependabot is an app and cannot certify anything; what it commits is the version bump .github/dependabot.yml asks for, of dependencies licensed by their own authors", + }, +} + +// exemptingBot returns the declared bot that exempts a commit by author in a +// pull request openedBy opened, if there is one. The login is compared +// without regard to case, as GitHub compares logins. +func exemptingBot(author identity, openedBy string) (declaredBot, bool) { + for _, bot := range declaredBots { + if strings.EqualFold(bot.login, openedBy) && sameIdentity(bot.author, author) { + return bot, true + } + } + return declaredBot{}, false +} + +// claimedBot returns the declared bot whose identity author carries, whoever +// opened the pull request, if there is one. +func claimedBot(author identity) (declaredBot, bool) { + for _, bot := range declaredBots { + if sameIdentity(bot.author, author) { + return bot, true + } + } + return declaredBot{}, false +} diff --git a/cmd/audit_dco/bots_test.go b/cmd/audit_dco/bots_test.go new file mode 100644 index 000000000..667a7a456 --- /dev/null +++ b/cmd/audit_dco/bots_test.go @@ -0,0 +1,81 @@ +// bots_test.go covers the declared bots: what each declaration has to state, +// and that the exemption needs the bot's identity and its login together. +package main + +import ( + "strings" + "testing" +) + +// TestDeclaredBots_Declarations_StateAnIdentityALoginAndAReason pins the +// declarations: Dependabot alone, with the identity its commits carry and the +// login that opens its pull requests, and every declaration with an identity +// a trailer could name, an app's login and a reason. +func TestDeclaredBots_Declarations_StateAnIdentityALoginAndAReason(t *testing.T) { + if len(declaredBots) != 1 { + t.Fatalf("declaredBots holds %d bots, want Dependabot alone", len(declaredBots)) + } + if got := declaredBots[0]; got.author != dependabot || got.login != "dependabot[bot]" { + t.Errorf("declaredBots[0] = %v as %q, want %v as dependabot[bot]", got.author, got.login, dependabot) + } + for _, bot := range declaredBots { + t.Run(bot.login, func(t *testing.T) { + if parsed, ok := parseIdentity(bot.author.String()); !ok || parsed != bot.author { + t.Errorf("author %q does not read back as an identity: %#v, %v", bot.author, parsed, ok) + } + if !strings.HasSuffix(bot.login, "[bot]") { + t.Errorf("login %q is not an app's login, which GitHub ends with [bot]", bot.login) + } + if !strings.Contains(bot.reason, "cannot certify") { + t.Errorf("reason %q does not say why the bot cannot certify the DCO", bot.reason) + } + }) + } +} + +// TestExemptingBot_Commits_AreExemptOnlyWhereTheIdentityAndTheLoginAgree +// verifies that a commit is exempt when it carries a declared bot's identity +// in a pull request that bot opened, the login compared without regard to +// case as GitHub compares logins, and in no other case. +func TestExemptingBot_Commits_AreExemptOnlyWhereTheIdentityAndTheLoginAgree(t *testing.T) { + tests := []struct { + name string + author identity + openedBy string + want bool + }{ + {name: "bot in its own pull request", author: dependabot, openedBy: "dependabot[bot]", want: true}, + {name: "login in another case", author: dependabot, openedBy: "Dependabot[bot]", want: true}, + {name: "identity in another case", author: identity{name: "Dependabot[bot]", email: strings.ToUpper(dependabot.email)}, openedBy: "dependabot[bot]", want: true}, + {name: "bot in a person's pull request", author: dependabot, openedBy: "jmrplens"}, + {name: "bot with nobody named as the opener", author: dependabot}, + {name: "person in the bot's pull request", author: jane, openedBy: "dependabot[bot]"}, + {name: "bot's name with another email", author: identity{name: dependabot.name, email: "support@github.com"}, openedBy: "dependabot[bot]"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + bot, ok := exemptingBot(tc.author, tc.openedBy) + if ok != tc.want { + t.Errorf("exemptingBot(%v, %q) = %v, want %v", tc.author, tc.openedBy, ok, tc.want) + } + if ok && bot.login != "dependabot[bot]" { + t.Errorf("exemptingBot(%v, %q) = %q, want dependabot[bot]", tc.author, tc.openedBy, bot.login) + } + if !ok && bot != (declaredBot{}) { + t.Errorf("exemptingBot(%v, %q) = %+v with no exemption, want the zero declaration", tc.author, tc.openedBy, bot) + } + }) + } +} + +// TestClaimedBot_Authors_NameTheBotWhoseIdentityTheyCarry verifies that a +// commit carrying a declared bot's identity is told apart from a person's, +// whoever opened its pull request. +func TestClaimedBot_Authors_NameTheBotWhoseIdentityTheyCarry(t *testing.T) { + if bot, ok := claimedBot(dependabot); !ok || bot.login != "dependabot[bot]" { + t.Errorf("claimedBot(%v) = %q, %v; want dependabot[bot], true", dependabot, bot.login, ok) + } + if bot, ok := claimedBot(jane); ok || bot != (declaredBot{}) { + t.Errorf("claimedBot(%v) = %+v, %v; want the zero declaration, false", jane, bot, ok) + } +} diff --git a/cmd/audit_dco/doc.go b/cmd/audit_dco/doc.go new file mode 100644 index 000000000..7466c5891 --- /dev/null +++ b/cmd/audit_dco/doc.go @@ -0,0 +1,80 @@ +// Command audit_dco holds the commits of a branch to the Developer Certificate +// of Origin (https://developercertificate.org/): every commit it judges has to +// carry a Signed-off-by trailer naming its author, which is how the author +// certifies the certificate for that commit. +// +// audit_dco [-base ] [-head ] [-opened-by ] [-dir ] [-annotate] +// +// # What it judges +// +// The commits reachable from -head and not from -base, which are the ones a +// pull request brings: origin/main..HEAD by default, and in CI the pull +// request's base and head commits as the event names them. The event of a +// stacked pull request names the head of the layer below as its base, so each +// layer's pull request judges that layer's commits and no others, and a layer +// below is judged only by its own pull request's run of ci.yml. +// +// Each commit is read from git with the trailers git finds in its message +// (git log's %(trailers) placeholder, which applies the rules of git +// interpret-trailers), so a sign-off counts exactly when git, and every tool +// that reads trailers through git, finds one. A commit passes when one of its +// Signed-off-by trailers names its author, the name and the email each +// compared without regard to case. git commit --signoff writes that trailer +// with the committer's identity, which is the author's for a commit one makes +// oneself. +// +// A commit with no Signed-off-by trailer fails, and so does one whose trailers +// name only other people: a sign-off certifies the commit for the person it +// names, and the certificate is the author's to give. A trailer that is not a +// name followed by an email in angle brackets names nobody. +// +// git log prints the fields of each commit between separator bytes, and a +// message can carry them. A record ends only where its separator is followed +// by a newline, which no field git prints holds, so such a message is read as +// git reads it: the byte stays in its field, and a trailer it ends names +// nobody. The commits are also listed by git rev-list, whose hashes carry no +// text a commit's author chose, and a run in which the two lists differ stops +// rather than judging either, so a sign-off can never move onto another +// commit. +// +// # What it does not judge +// +// A merge of two parents that brings no change of its own is reported and not +// judged: its tree is the one git's own merge of its parents makes, which git +// show --remerge-diff prints no difference for, and the commits it brings in +// that the base does not hold are judged in their own right. GitHub's Update +// branch button writes one. Every other merge is judged like any other +// commit: one that resolves a conflict, adds or drops a change while merging, +// or joins more than two parents, which git does not remerge. +// +// The commits of a declared bot are exempt in a pull request that bot opened. +// A bot is an app, not a person, so it can certify nothing, and what it +// commits is generated. Each bot is declared in bots.go with the author +// identity its commits carry, the GitHub login that opens its pull requests +// and the reason it is exempt, and the exemption needs both to agree: a +// commit carrying a bot's identity in a pull request somebody else opened is +// judged like any other, so writing a bot's name into a commit exempts +// nothing. Today the one declared bot is Dependabot. A workflow that starts +// opening pull requests as another app fails here until that app is declared. +// +// The release workflow's version stamp is not declared, because it never sits +// in a pull request: the release job pushes it to main with its deploy key +// after a release, as GOVERNANCE.md describes, and the commits a pull request +// brings are the only ones this command reads. +// +// # The squash commit +// +// main gains one squash commit per pull request, whose message is the pull +// request description, and the description is not judged. GitHub copies the +// Signed-off-by trailers of the pull request's commits into the squash commit +// itself, once each, after the description: the squash commits of pull +// requests 1512 and 1529 carry the trailer their descriptions did not, and so +// do the squash commits of Dependabot's pull requests. A description that +// carried its own would repeat it. +// +// # Exit codes +// +// 0 when every judged commit is signed off by its author, or there is none to +// judge; 1 when a commit is not (said on stdout, with how to sign it off) or +// git could not answer (said on stderr); 2 for arguments it cannot use. +package main diff --git a/cmd/audit_dco/git.go b/cmd/audit_dco/git.go new file mode 100644 index 000000000..7f56f77f8 --- /dev/null +++ b/cmd/audit_dco/git.go @@ -0,0 +1,170 @@ +package main + +import ( + "bytes" + "context" + "fmt" + "os/exec" + "strings" +) + +// logFormat is what git log prints for each commit: the hash, the parents, +// the author's name and email, the subject, and the values of the +// Signed-off-by trailers git finds in the message, unfolded, a NUL between +// two fields and between two trailers, and a record separator after the +// commit, which git follows with the newline it ends every commit's output +// with. +const logFormat = "%H%x00%P%x00%an%x00%ae%x00%s%x00%(trailers:key=Signed-off-by,valueonly,unfold,separator=%x00)%x1e" + +// The separator logFormat prints between fields, the end of every record (the +// record separator logFormat prints and the newline git adds), and how many +// fields come before the trailers. A commit without trailers still has the +// field after them, empty. +// +// A record ends where the record separator is followed by a newline, and no +// field holds a newline: git keeps none in a name or an email, joins the +// lines of the subject with spaces, and unfolds each trailer onto one line. +// So a message carrying either separator byte shifts nothing: the byte stays +// in its field and is read as the value git reads. +const ( + fieldSeparator = "\x00" + recordEnd = "\x1e\n" + fixedFields = 5 +) + +// runGit runs git in dir and returns what it printed on stdout. A failure +// carries what git said on stderr, which is the only place it says why. +// +// The program is resolved to an absolute path first, which is how every other +// command here runs a tool it did not build (cmd/gen_orbit_record): the +// lookup is the one exec would do, decided here rather than by whatever PATH +// holds when the command starts. +func runGit(ctx context.Context, dir string, args ...string) (string, error) { + bin, err := exec.LookPath("git") + if err != nil { + return "", fmt.Errorf("find git: %w", err) + } + // #nosec G204 G702 -- the program is the absolute path just resolved, with arguments this command builds from resolved commit hashes and its own flags + cmd := exec.CommandContext(ctx, bin, args...) + cmd.Dir = dir + var stderr bytes.Buffer + cmd.Stderr = &stderr + out, err := cmd.Output() + if err != nil { + return "", fmt.Errorf("git %s: %w: %s", strings.Join(args, " "), err, strings.TrimSpace(stderr.String())) + } + return string(out), nil +} + +// resolveCommit returns the full hash of the commit rev names in dir. The +// caller refuses a revision git would read as an option before asking. +func resolveCommit(ctx context.Context, dir, rev string) (string, error) { + out, err := runGit(ctx, dir, "rev-parse", "--verify", rev+"^{commit}") + if err != nil { + return "", err + } + return strings.TrimSpace(out), nil +} + +// listCommits returns the commits reachable from head and not from base, +// oldest first, read from git log and held to the hashes git rev-list lists +// for the same range. +func listCommits(ctx context.Context, dir, base, head string) ([]commit, error) { + span := base + ".." + head + hashes, err := runGit(ctx, dir, "rev-list", "--reverse", span, "--") + if err != nil { + return nil, err + } + out, err := runGit(ctx, dir, "log", "--no-show-signature", "--reverse", "--format="+logFormat, span, "--") + if err != nil { + return nil, err + } + return readLog(strings.Fields(hashes), out) +} + +// readLog reads what git log printed in logFormat and holds it to the hashes +// git rev-list listed for the same range. The records cannot be split by a +// message (see recordEnd), so the second list is a guard on the reading +// itself: should git ever print a newline inside a field, the run stops +// rather than judging a record that belongs to no commit, or moving a +// sign-off onto another one. +func readLog(hashes []string, out string) ([]commit, error) { + commits, err := parseLog(out) + if err != nil { + return nil, err + } + if mismatch := matchHashes(hashes, commits); mismatch != nil { + return nil, mismatch + } + return commits, nil +} + +// parseLog reads the records git log printed in logFormat. Text after the +// last record end, and a record with no more fields than the ones before the +// trailers, are not what git prints for a commit, and stop the read. +func parseLog(out string) ([]commit, error) { + var commits []commit + for rest := out; rest != ""; { + record, after, ended := strings.Cut(rest, recordEnd) + if !ended { + return nil, fmt.Errorf("git log printed %q after its last record, where every record ends with a record separator and a newline", rest) + } + rest = after + fields := strings.Split(record, fieldSeparator) + if len(fields) <= fixedFields { + return nil, fmt.Errorf("git log printed a record of %d field(s) where every commit has at least %d: %q", len(fields), fixedFields+1, record) + } + var signoffs []string + for _, value := range fields[fixedFields:] { + if value != "" { + signoffs = append(signoffs, value) + } + } + commits = append(commits, commit{ + hash: fields[0], + parents: len(strings.Fields(fields[1])), + author: identity{name: fields[2], email: fields[3]}, + subject: fields[4], + signoffs: signoffs, + }) + } + return commits, nil +} + +// matchHashes holds the commits git log printed to the hashes git rev-list +// listed for the same range: as many, and the same ones in the same order. +func matchHashes(hashes []string, commits []commit) error { + if len(hashes) != len(commits) { + return fmt.Errorf("git rev-list lists %d commit(s) and git log printed %d, so the records were not read as git printed them", len(hashes), len(commits)) + } + for i, hash := range hashes { + if commits[i].hash != hash { + return fmt.Errorf("commit %d is %s to git rev-list and %s to git log, so the records were not read as git printed them", i+1, hash, commits[i].hash) + } + } + return nil +} + +// markCleanMerges marks each merge of two parents that brings no change of +// its own: one whose tree is the tree git's own merge of its parents makes, +// which git show --remerge-diff prints no difference for. That is the merge +// GitHub's Update branch button writes. A merge that resolves a conflict, +// changes anything while merging or drops a side's change prints one, and a +// merge of more than two parents is one git does not remerge; both stay +// unmarked, so they are judged like any other commit. Every setting that +// could make git print nothing for a change is overridden: an external diff +// program or a textconv filter the configuration names, a diff limited to the +// directory git runs in, and submodules left out of it. +func markCleanMerges(ctx context.Context, dir string, commits []commit) error { + for i := range commits { + if commits[i].parents != 2 { + continue + } + diff, err := runGit(ctx, dir, "show", "--no-show-signature", "--no-color", "--no-ext-diff", "--no-textconv", "--no-relative", "--ignore-submodules=none", "--remerge-diff", "--format=", commits[i].hash, "--") + if err != nil { + return err + } + commits[i].cleanMerge = diff == "" + } + return nil +} diff --git a/cmd/audit_dco/git_test.go b/cmd/audit_dco/git_test.go new file mode 100644 index 000000000..1db82f0db --- /dev/null +++ b/cmd/audit_dco/git_test.go @@ -0,0 +1,522 @@ +// git_test.go covers how a range is read from git: the records git log +// prints, the check against the hashes git rev-list lists, which merges bring +// no change of their own, and the answers of a real repository, git's own +// sign-off and git's errors included. +package main + +import ( + "bytes" + "context" + "os" + "os/exec" + "path/filepath" + "reflect" + "slices" + "strings" + "testing" +) + +// gitFixture runs git in dir as author on behalf of a test and returns what +// it printed on stdout, trimmed. It is isolated from the machine's and the +// user's configuration, so no signing key, hook or template of theirs reaches +// the fixture. +func gitFixture(t *testing.T, dir string, author identity, args ...string) string { + t.Helper() + + global := filepath.Join(t.TempDir(), "gitconfig") + if err := os.WriteFile(global, nil, 0o600); err != nil { + t.Fatal(err) + } + cmd := exec.CommandContext(context.Background(), "git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_CONFIG_GLOBAL="+global, "GIT_CONFIG_NOSYSTEM=1", + "GIT_AUTHOR_NAME="+author.name, "GIT_AUTHOR_EMAIL="+author.email, + "GIT_COMMITTER_NAME="+author.name, "GIT_COMMITTER_EMAIL="+author.email) + var stderr bytes.Buffer + cmd.Stderr = &stderr + out, err := cmd.Output() + if err != nil { + t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, stderr.String()) + } + return strings.TrimSpace(string(out)) +} + +// newRepository creates an empty repository in a temporary directory and +// returns it. +func newRepository(t *testing.T) string { + t.Helper() + + dir := t.TempDir() + gitFixture(t, dir, jane, "init", "--quiet") + return dir +} + +// commitAs makes an empty commit in dir as author, with message passed to git +// through a file and kept verbatim so that it reaches the commit byte for +// byte, and returns the commit. +func commitAs(t *testing.T, dir string, author identity, message string) string { + t.Helper() + + file := filepath.Join(t.TempDir(), "message") + if err := os.WriteFile(file, []byte(message), 0o600); err != nil { + t.Fatal(err) + } + gitFixture(t, dir, author, "commit", "--quiet", "--allow-empty", "--no-gpg-sign", "--cleanup=verbatim", "--file", file) + return gitFixture(t, dir, author, "rev-parse", "HEAD") +} + +// signedMessage is a commit message whose subject is followed by a +// Signed-off-by trailer naming who. +func signedMessage(subject string, who identity) string { + return subject + "\n\nSigned-off-by: " + who.String() + "\n" +} + +// commitFile writes content to name in dir, stages it and commits it as +// author with message, and returns the commit. +func commitFile(t *testing.T, dir string, author identity, name, content, message string) string { + t.Helper() + + if err := os.WriteFile(filepath.Join(dir, name), []byte(content), 0o600); err != nil { + t.Fatal(err) + } + gitFixture(t, dir, author, "add", "--", name) + return commitAs(t, dir, author, message) +} + +// mergeWithChange merges branch into the branch checked out in dir as author, +// adds a file of its own to the merge before committing it with message, and +// returns the merge, which therefore brings a change of its own. +func mergeWithChange(t *testing.T, dir string, author identity, branch, message string) string { + t.Helper() + + gitFixture(t, dir, author, "merge", "--quiet", "--no-ff", "--no-commit", branch) + if err := os.WriteFile(filepath.Join(dir, "payload.txt"), []byte("added while merging\n"), 0o600); err != nil { + t.Fatal(err) + } + gitFixture(t, dir, author, "add", "--", "payload.txt") + return commitAs(t, dir, author, message) +} + +// TestParseLog_Records_ReadEachCommitAndItsSignOffs verifies how the records +// git log prints are read: the hash, the parents counted, the author, the +// subject and every Signed-off-by value in order, with the newline git puts +// between records and the empty field of a commit without trailers ignored. +func TestParseLog_Records_ReadEachCommitAndItsSignOffs(t *testing.T) { + out := "aaa\x00ppp\x00Jane Doe\x00jane@example.org\x00feat: one\x00Jane Doe \x1e\n" + + "bbb\x00aaa ccc\x00John Roe\x00john@example.org\x00Merge\x00\x1e\n" + + "ccc\x00\x00Jane Doe\x00jane@example.org\x00root\x00John Roe \x00Jane Doe \x1e\n" + got, err := parseLog(out) + if err != nil { + t.Fatalf("parseLog() error = %v", err) + } + want := []commit{ + {hash: "aaa", parents: 1, author: jane, subject: "feat: one", signoffs: []string{"Jane Doe "}}, + {hash: "bbb", parents: 2, author: john, subject: "Merge"}, + {hash: "ccc", parents: 0, author: jane, subject: "root", signoffs: []string{"John Roe ", "Jane Doe "}}, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("parseLog() = %#v\nwant %#v", got, want) + } +} + +// TestParseLog_NoRecords_ReadsNoCommits verifies that an empty range, which +// git log prints nothing for, reads as no commits rather than an error. +func TestParseLog_NoRecords_ReadsNoCommits(t *testing.T) { + got, err := parseLog("") + if err != nil || len(got) != 0 { + t.Errorf("parseLog(\"\") = %#v, %v; want no commits and no error", got, err) + } +} + +// TestParseLog_SeparatorInAField_StaysInTheField verifies that the record +// separator inside a field, in the subject or at the end of the last trailer, +// is read as part of that field: a record ends only where the separator is +// followed by the newline git prints after every commit, and no field git +// prints holds a newline. +func TestParseLog_SeparatorInAField_StaysInTheField(t *testing.T) { + out := "aaa\x00ppp\x00Jane Doe\x00jane@example.org\x00feat: one\x1etwo\x00Jane Doe \x1e\x1e\n" + + "bbb\x00aaa\x00Jane Doe\x00jane@example.org\x00fix: three\x00\x1e\n" + got, err := parseLog(out) + if err != nil { + t.Fatalf("parseLog() error = %v", err) + } + want := []commit{ + {hash: "aaa", parents: 1, author: jane, subject: "feat: one\x1etwo", signoffs: []string{"Jane Doe \x1e"}}, + {hash: "bbb", parents: 1, author: jane, subject: "fix: three"}, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("parseLog() = %#v\nwant %#v", got, want) + } +} + +// TestParseLog_ShortRecord_IsRefused verifies that a record with fewer fields +// than every commit has stops the read instead of being taken as a commit. +// Five fields is one short, since the sixth always exists, empty for a commit +// without trailers, and an empty record is one field. +func TestParseLog_ShortRecord_IsRefused(t *testing.T) { + tests := []struct{ name, out, wantErr string }{ + {name: "five fields", out: "aaa\x00ppp\x00Jane Doe\x00jane@example.org\x00feat: one\x1e\n", wantErr: "a record of 5 field(s) where every commit has at least 6"}, + {name: "an empty record", out: "aaa\x00ppp\x00Jane Doe\x00jane@example.org\x00feat: one\x00\x1e\n\x1e\n", wantErr: "a record of 1 field(s) where every commit has at least 6"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, err := parseLog(tc.out) + if err == nil || got != nil { + t.Fatalf("parseLog(%q) = %#v, %v; want an error and no commits", tc.out, got, err) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Errorf("error = %q, want it to hold %q", err, tc.wantErr) + } + }) + } +} + +// TestParseLog_UnterminatedRecord_IsRefused verifies that text after the last +// record end, which git never prints since it ends every commit's record with +// a newline, stops the read rather than being taken as a record. +func TestParseLog_UnterminatedRecord_IsRefused(t *testing.T) { + out := "aaa\x00ppp\x00Jane Doe\x00jane@example.org\x00feat: one\x00\x1e\nbbb\x00aaa\x00Jane Doe\x00jane@example.org\x00fix: two\x00\x1e" + got, err := parseLog(out) + if err == nil || got != nil { + t.Fatalf("parseLog(%q) = %#v, %v; want an error and no commits", out, got, err) + } + if !strings.Contains(err.Error(), "where every record ends with a record separator and a newline") { + t.Errorf("error = %q, want it to say how a record ends", err) + } +} + +// TestReadLog_Output_IsHeldToTheHashesGitRevListListed verifies that what git +// log printed is read only when it parses and lists the hashes git rev-list +// listed for the same range: a record that does not parse, or a commit list +// that departs from the hashes, stops the read. +func TestReadLog_Output_IsHeldToTheHashesGitRevListListed(t *testing.T) { + out := "aaa\x00ppp\x00Jane Doe\x00jane@example.org\x00feat: one\x00Jane Doe \x1e\n" + t.Run("agreeing", func(t *testing.T) { + got, err := readLog([]string{"aaa"}, out) + want := []commit{{hash: "aaa", parents: 1, author: jane, subject: "feat: one", signoffs: []string{"Jane Doe "}}} + if err != nil || !reflect.DeepEqual(got, want) { + t.Errorf("readLog() = %#v, %v; want %#v and no error", got, err, want) + } + }) + tests := []struct { + name string + hashes []string + out string + wantErr string + }{ + {name: "a record that does not parse", hashes: []string{"aaa"}, out: "aaa\x1e\n", wantErr: "a record of 1 field(s)"}, + {name: "a commit rev-list did not list", hashes: []string{"bbb"}, out: out, wantErr: "commit 1 is bbb to git rev-list and aaa to git log"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, err := readLog(tc.hashes, tc.out) + if err == nil || got != nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Errorf("readLog() = %#v, %v; want no commits and an error holding %q", got, err, tc.wantErr) + } + }) + } +} + +// TestMatchHashes_Lists_AgreeOnlyWhenEveryHashMatchesInOrder verifies the +// check of the commits git log printed against the hashes git rev-list +// listed: the same hashes in the same order agree, and a missing, an extra +// or a different hash does not. +func TestMatchHashes_Lists_AgreeOnlyWhenEveryHashMatchesInOrder(t *testing.T) { + read := []commit{{hash: "aaa"}, {hash: "bbb"}} + tests := []struct { + name string + hashes []string + wantErr string + }{ + {name: "same hashes in order", hashes: []string{"aaa", "bbb"}}, + {name: "one more listed", hashes: []string{"aaa", "bbb", "ccc"}, wantErr: "git rev-list lists 3 commit(s) and git log printed 2"}, + {name: "one fewer listed", hashes: []string{"aaa"}, wantErr: "git rev-list lists 1 commit(s) and git log printed 2"}, + {name: "another hash", hashes: []string{"aaa", "ccc"}, wantErr: "commit 2 is ccc to git rev-list and bbb to git log"}, + {name: "another order", hashes: []string{"bbb", "aaa"}, wantErr: "commit 1 is bbb to git rev-list and aaa to git log"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := matchHashes(tc.hashes, read) + if tc.wantErr == "" { + if err != nil { + t.Errorf("matchHashes(%q) = %v, want nil", tc.hashes, err) + } + return + } + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Errorf("matchHashes(%q) = %v, want an error containing %q", tc.hashes, err, tc.wantErr) + } + }) + } +} + +// TestListCommits_Repository_ReadsTheRangeOldestFirst verifies the read of a +// real repository: the commits after base, oldest first, each with its +// author, its subject, its parents and its Signed-off-by values as git finds +// them, which includes the trailer git commit --signoff writes, a key written +// in lower case and a Signed-off-by line that shares the message's last +// paragraph with a line of prose, and leaves out a Signed-off-by line outside +// that paragraph. +func TestListCommits_Repository_ReadsTheRangeOldestFirst(t *testing.T) { + dir := newRepository(t) + base := commitAs(t, dir, jane, "root\n") + first := commitAs(t, dir, jane, signedMessage("feat: first", jane)) + gitFixture(t, dir, john, "commit", "--quiet", "--allow-empty", "--no-gpg-sign", "--signoff", "--message", "fix: second") + second := gitFixture(t, dir, john, "rev-parse", "HEAD") + third := commitAs(t, dir, jane, "docs: third\n\nsigned-off-by: Jane Doe \n") + fourth := commitAs(t, dir, jane, "chore: fourth\n\nSigned-off-by: Jane Doe is not a trailer here.\n\nThe last paragraph is prose.\n") + fifth := commitAs(t, dir, jane, "test: fifth\n\nSigned-off-by: Jane Doe \nand a line of prose in the same last paragraph\n") + + got, err := listCommits(context.Background(), dir, base, fifth) + if err != nil { + t.Fatalf("listCommits() error = %v", err) + } + want := []commit{ + {hash: first, parents: 1, author: jane, subject: "feat: first", signoffs: []string{"Jane Doe "}}, + {hash: second, parents: 1, author: john, subject: "fix: second", signoffs: []string{"John Roe "}}, + {hash: third, parents: 1, author: jane, subject: "docs: third", signoffs: []string{"Jane Doe "}}, + {hash: fourth, parents: 1, author: jane, subject: "chore: fourth"}, + {hash: fifth, parents: 1, author: jane, subject: "test: fifth", signoffs: []string{"Jane Doe "}}, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("listCommits() = %#v\nwant %#v", got, want) + } +} + +// TestListCommits_MergedBranch_CountsTheMergesParents verifies that a merge +// commit is read with both of its parents, and the commit of the merged +// branch with its own one. +func TestListCommits_MergedBranch_CountsTheMergesParents(t *testing.T) { + dir := newRepository(t) + base := commitAs(t, dir, jane, "root\n") + trunk := gitFixture(t, dir, jane, "symbolic-ref", "--short", "HEAD") + gitFixture(t, dir, jane, "checkout", "--quiet", "-b", "side") + side := commitAs(t, dir, jane, signedMessage("feat: side", jane)) + gitFixture(t, dir, jane, "checkout", "--quiet", trunk) + onTrunk := commitAs(t, dir, jane, signedMessage("feat: trunk", jane)) + gitFixture(t, dir, jane, "merge", "--quiet", "--no-ff", "--no-edit", "--no-gpg-sign", "side") + merged := gitFixture(t, dir, jane, "rev-parse", "HEAD") + + got, err := listCommits(context.Background(), dir, base, merged) + if err != nil { + t.Fatalf("listCommits() error = %v", err) + } + parents := map[string]int{} + for _, each := range got { + parents[each.hash] = each.parents + } + if want := map[string]int{side: 1, onTrunk: 1, merged: 2}; !reflect.DeepEqual(parents, want) { + t.Errorf("parents by commit = %v, want %v", parents, want) + } +} + +// TestListCommits_SeparatorInAMessage_IsReadAsGitReadsIt verifies that a +// message carrying the byte the read separates records with is read the way +// git reads it, every value in its own field and on its own commit: in the +// subject, inside a trailer value, at the end of the last one, and followed by +// trailers that would parse as a record of their own if the byte ended one. +func TestListCommits_SeparatorInAMessage_IsReadAsGitReadsIt(t *testing.T) { + dir := newRepository(t) + base := commitAs(t, dir, jane, "root\n") + message := "feat: one\x1etwo\n\nSigned-off-by: x\x1ey\n" + strings.Repeat("Signed-off-by: Jane Doe \n", 5) + + "Signed-off-by: John Roe \x1e\n" + head := commitAs(t, dir, john, message) + + got, err := listCommits(context.Background(), dir, base, head) + if err != nil { + t.Fatalf("listCommits() error = %v", err) + } + signoffs := append(append([]string{"x\x1ey"}, slices.Repeat([]string{"Jane Doe "}, 5)...), "John Roe \x1e") + want := []commit{{hash: head, parents: 1, author: john, subject: "feat: one\x1etwo", signoffs: signoffs}} + if !reflect.DeepEqual(got, want) { + t.Errorf("listCommits() = %#v\nwant %#v", got, want) + } +} + +// TestMarkCleanMerges_Merges_MarkOnlyTheOnesThatBringNoChange verifies which +// commits are marked as merges bringing no change of their own: a merge of +// two parents whose tree is the one git's own merge of them makes is, and a +// merge that adds a change while merging, one that keeps one side and drops +// the other's change, a merge of three parents, which git cannot remerge, and +// every commit that is not a merge are not. +func TestMarkCleanMerges_Merges_MarkOnlyTheOnesThatBringNoChange(t *testing.T) { + dir := newRepository(t) + base := commitAs(t, dir, jane, "root\n") + trunk := gitFixture(t, dir, jane, "symbolic-ref", "--short", "HEAD") + // commitNamed commits a file of its own on the branch checked out, and + // branch commits one on a new branch from the trunk and goes back to it. + commitNamed := func(name string) { + commitFile(t, dir, jane, name+".txt", name+"\n", signedMessage("feat: "+name, jane)) + } + branch := func(name string) { + gitFixture(t, dir, jane, "checkout", "--quiet", "-b", name) + commitNamed(name) + gitFixture(t, dir, jane, "checkout", "--quiet", trunk) + } + merge := func(args ...string) string { + gitFixture(t, dir, jane, append([]string{"merge", "--quiet", "--no-ff", "--no-edit", "--no-gpg-sign"}, args...)...) + return gitFixture(t, dir, jane, "rev-parse", "HEAD") + } + + branch("clean") + commitNamed("one") + clean := merge("clean") + branch("changed") + commitNamed("two") + changed := mergeWithChange(t, dir, jane, "changed", "Merge branch 'changed'\n") + branch("dropped") + commitNamed("three") + dropped := merge("--strategy=ours", "dropped") + branch("left") + branch("right") + commitNamed("four") + octopus := merge("left", "right") + + commits, err := listCommits(context.Background(), dir, base, octopus) + if err != nil { + t.Fatalf("listCommits() error = %v", err) + } + if err = markCleanMerges(context.Background(), dir, commits); err != nil { + t.Fatalf("markCleanMerges() error = %v", err) + } + marked := map[string]bool{} + parents := map[string]int{} + for _, each := range commits { + if each.cleanMerge { + marked[each.hash] = true + } + parents[each.hash] = each.parents + } + if want := map[string]bool{clean: true}; !reflect.DeepEqual(marked, want) { + t.Errorf("merges marked as bringing no change = %v, want %v alone", marked, clean) + } + if got := []int{parents[clean], parents[changed], parents[dropped], parents[octopus]}; !slices.Equal(got, []int{2, 2, 2, 3}) { + t.Errorf("parents of the clean, changed, dropped and octopus merges = %v, want [2 2 2 3]", got) + } +} + +// TestMarkCleanMerges_MissingTree_ReturnsWhatGitShowSaid verifies that a merge +// git cannot remerge, because the clone lacks a tree one of its parents +// records, fails with git's own reason, which names the tree, rather than +// being taken for a merge that brings a change, or none. +func TestMarkCleanMerges_MissingTree_ReturnsWhatGitShowSaid(t *testing.T) { + dir, base, head, tree := mergeMissingATree(t) + + commits, err := listCommits(context.Background(), dir, base, head) + if err != nil { + t.Fatalf("listCommits() error = %v", err) + } + err = markCleanMerges(context.Background(), dir, commits) + if err == nil || !strings.HasPrefix(err.Error(), "git show") || !strings.Contains(err.Error(), tree) { + t.Errorf("markCleanMerges() = %v, want git show's own reason naming the tree %s", err, tree) + } +} + +// mergeMissingATree is a repository whose root is followed by a merge of a +// branch that adds a file, with the tree of that branch's commit removed from +// the object store, and the root, the merge and the tree removed. git +// rev-list and git log read only commits, so they still answer; remerging +// needs the tree. +func mergeMissingATree(t *testing.T) (dir, base, head, tree string) { + t.Helper() + + dir = newRepository(t) + base = commitAs(t, dir, jane, "root\n") + trunk := gitFixture(t, dir, jane, "symbolic-ref", "--short", "HEAD") + gitFixture(t, dir, jane, "checkout", "--quiet", "-b", "side") + commitFile(t, dir, jane, "side.txt", "side\n", signedMessage("feat: side", jane)) + tree = gitFixture(t, dir, jane, "rev-parse", "HEAD^{tree}") + gitFixture(t, dir, jane, "checkout", "--quiet", trunk) + commitAs(t, dir, jane, signedMessage("feat: trunk", jane)) + gitFixture(t, dir, jane, "merge", "--quiet", "--no-ff", "--no-edit", "--no-gpg-sign", "side") + head = gitFixture(t, dir, jane, "rev-parse", "HEAD") + if err := os.Remove(filepath.Join(dir, ".git", "objects", tree[:2], tree[2:])); err != nil { + t.Fatal(err) + } + return dir, base, head, tree +} + +// TestListCommits_UnknownRevision_ReturnsWhatGitSaid verifies that a range +// git cannot read fails with git's own reason, from git rev-list first and +// then from git log. +func TestListCommits_UnknownRevision_ReturnsWhatGitSaid(t *testing.T) { + dir := newRepository(t) + head := commitAs(t, dir, jane, "root\n") + missing := strings.Repeat("0", 40) + + got, err := listCommits(context.Background(), dir, missing, head) + if err == nil || got != nil { + t.Fatalf("listCommits() = %#v, %v; want an error and no commits", got, err) + } + if !strings.HasPrefix(err.Error(), "git rev-list") || !strings.Contains(err.Error(), "fatal:") { + t.Errorf("error = %q, want git rev-list's own reason", err) + } +} + +// TestListCommits_LogFails_ReturnsWhatGitLogSaid verifies the second read's +// failure: a range git rev-list reads and git log does not. A repository +// whose configuration names an unknown log.showSignature value is one, since +// only git log reads that setting. +func TestListCommits_LogFails_ReturnsWhatGitLogSaid(t *testing.T) { + dir := newRepository(t) + base := commitAs(t, dir, jane, "root\n") + head := commitAs(t, dir, jane, signedMessage("feat: one", jane)) + gitFixture(t, dir, jane, "config", "log.showSignature", "maybe") + + got, err := listCommits(context.Background(), dir, base, head) + if err == nil || got != nil { + t.Fatalf("listCommits() = %#v, %v; want an error and no commits", got, err) + } + if !strings.HasPrefix(err.Error(), "git log") || !strings.Contains(err.Error(), "fatal:") { + t.Errorf("error = %q, want git log's own reason", err) + } +} + +// TestResolveCommit_Revisions_NameTheCommitOrSayWhyNot verifies that a +// revision resolves to the full hash of the commit it names, a tag included, +// and that one naming nothing fails with what git said. +func TestResolveCommit_Revisions_NameTheCommitOrSayWhyNot(t *testing.T) { + dir := newRepository(t) + head := commitAs(t, dir, jane, "root\n") + gitFixture(t, dir, jane, "tag", "--annotate", "--no-sign", "--message", "v1", "v1") + + for _, rev := range []string{"HEAD", "v1", head} { + t.Run(rev, func(t *testing.T) { + got, err := resolveCommit(context.Background(), dir, rev) + if err != nil || got != head { + t.Errorf("resolveCommit(%q) = %q, %v; want %q", rev, got, err, head) + } + }) + } + t.Run("unknown", func(t *testing.T) { + got, err := resolveCommit(context.Background(), dir, "no-such-branch") + if err == nil || got != "" || !strings.Contains(err.Error(), "git rev-parse") { + t.Errorf("resolveCommit(no-such-branch) = %q, %v; want git rev-parse's error", got, err) + } + }) +} + +// TestRunGit_Failure_CarriesWhatGitSaidOnStderr verifies that a failing git +// command is reported with its arguments and the reason git printed on +// stderr, which is the only place git says why. +func TestRunGit_Failure_CarriesWhatGitSaidOnStderr(t *testing.T) { + out, err := runGit(context.Background(), t.TempDir(), "rev-parse", "--verify", "HEAD") + if err == nil || out != "" { + t.Fatalf("runGit() = %q, %v; want an error outside a repository", out, err) + } + if !strings.Contains(err.Error(), "git rev-parse --verify HEAD") || !strings.Contains(strings.ToLower(err.Error()), "not a git repository") { + t.Errorf("error = %q, want the command and git's reason", err) + } +} + +// TestRunGit_NoGitOnPath_IsTheLookupsFailure verifies that git is found on +// PATH before it is run, and that a PATH holding none is reported as the +// lookup's failure rather than as a git that ran and refused. +func TestRunGit_NoGitOnPath_IsTheLookupsFailure(t *testing.T) { + t.Setenv("PATH", t.TempDir()) + + if _, err := runGit(context.Background(), t.TempDir(), "--version"); err == nil || !strings.HasPrefix(err.Error(), "find git: ") { + t.Errorf("runGit() with no git on PATH = %v, want the lookup's failure", err) + } +} diff --git a/cmd/audit_dco/main.go b/cmd/audit_dco/main.go new file mode 100644 index 000000000..2a5f68dd1 --- /dev/null +++ b/cmd/audit_dco/main.go @@ -0,0 +1,77 @@ +package main + +import ( + "context" + "flag" + "fmt" + "io" + "os" + "strings" +) + +// toolName is how every message this command prints names its source. +const toolName = "audit_dco" + +// osExit is a seam over os.Exit, so a test can observe the exit code main +// passes on without ending the test process. +var osExit = os.Exit + +func main() { + osExit(runMain(context.Background(), os.Args[1:], os.Stdout, os.Stderr)) +} + +// runMain parses the command line, reads the commits of the range from git +// with the merges among them that bring no change of their own, judges each +// and prints the report. It returns 2 for a usage error, 1 when a +// commit is not signed off by its author or git could not answer, and 0 +// otherwise. +func runMain(ctx context.Context, args []string, stdout, stderr io.Writer) int { + flags := flag.NewFlagSet(toolName, flag.ContinueOnError) + flags.SetOutput(stderr) + base := flags.String("base", "origin/main", "the commit the branch grew from; the commits reachable from it are not judged") + head := flags.String("head", "HEAD", "the commit at the tip of the branch") + openedBy := flags.String("opened-by", "", "the GitHub login that opened the pull request, which a declared bot's exemption needs") + dir := flags.String("dir", ".", "the repository to read") + annotate := flags.Bool("annotate", false, "print a GitHub Actions error command for every commit that fails") + if err := flags.Parse(args); err != nil { + return 2 + } + if flags.NArg() > 0 { + _, _ = fmt.Fprintf(stderr, "%s: takes no arguments, and was given %q; name the range with -base and -head\n", toolName, flags.Args()) + return 2 + } + revisions := []struct{ flag, rev string }{{"base", *base}, {"head", *head}} + for _, each := range revisions { + if each.rev == "" || strings.HasPrefix(each.rev, "-") { + _, _ = fmt.Fprintf(stderr, "%s: -%s %q is not a revision; name a commit, a branch or a tag\n", toolName, each.flag, each.rev) + return 2 + } + } + hashes := make([]string, 0, len(revisions)) + for _, each := range revisions { + hash, err := resolveCommit(ctx, *dir, each.rev) + if err != nil { + _, _ = fmt.Fprintf(stderr, "%s: -%s %s names no commit in %s (%v); a shallow clone may lack it, so fetch it or check out with fetch-depth: 0\n", toolName, each.flag, each.rev, *dir, err) + return 1 + } + hashes = append(hashes, hash) + } + span := fmt.Sprintf("%s..%s (%s..%s)", *base, *head, commit{hash: hashes[0]}.short(), commit{hash: hashes[1]}.short()) + read, err := listCommits(ctx, *dir, hashes[0], hashes[1]) + if err == nil { + err = markCleanMerges(ctx, *dir, read) + } + if err != nil { + _, _ = fmt.Fprintf(stderr, "%s: could not read the commits of %s: %v\n", toolName, span, err) + return 1 + } + verdicts := make([]verdict, 0, len(read)) + for _, each := range read { + verdicts = append(verdicts, judge(each, *openedBy)) + } + failed := report{span: span, base: *base, openedBy: *openedBy, verdicts: verdicts, annotate: *annotate}.write(stdout) + if failed { + return 1 + } + return 0 +} diff --git a/cmd/audit_dco/main_test.go b/cmd/audit_dco/main_test.go new file mode 100644 index 000000000..e44a88f48 --- /dev/null +++ b/cmd/audit_dco/main_test.go @@ -0,0 +1,457 @@ +// main_test.go covers the command end to end over real repositories: what it +// judges, what it prints and the status it exits with, the arguments it +// refuses, and the CI job and the make target that run it. +package main + +import ( + "bytes" + "context" + "os" + "slices" + "strings" + "testing" + + yaml "go.yaml.in/yaml/v3" +) + +// runCase is one repository and command line of the table below, and what the +// run must do with them. setup builds the repository and returns it and the +// arguments after -dir. +type runCase struct { + name string + setup func(t *testing.T) (dir string, args []string) + wantCode int + wantStdout []string + wantStderr string +} + +// check runs the case and reports every way the run departed from it: the +// exit code, each fragment stdout must hold, and stderr, which must be empty +// or hold wantStderr. +func (tc runCase) check(t *testing.T) { + t.Helper() + + dir, args := tc.setup(t) + args = append([]string{"-dir", dir}, args...) + var stdout, stderr bytes.Buffer + code := runMain(context.Background(), args, &stdout, &stderr) + if code != tc.wantCode { + t.Errorf("runMain(%q) = %d, want %d\nstdout:\n%s\nstderr:\n%s", args, code, tc.wantCode, stdout.String(), stderr.String()) + } + for _, want := range tc.wantStdout { + if !strings.Contains(stdout.String(), want) { + t.Errorf("stdout does not hold %q:\n%s", want, stdout.String()) + } + } + if tc.wantStderr == "" && stderr.Len() != 0 || !strings.Contains(stderr.String(), tc.wantStderr) { + t.Errorf("stderr = %q, want it to hold %q", stderr.String(), tc.wantStderr) + } +} + +// authored is a commit message and the identity it is committed as. +type authored struct { + author identity + message string +} + +// by pairs a commit message with its author for branch. +func by(author identity, message string) authored { return authored{author: author, message: message} } + +// branch is a repository whose root is followed by the given commits, and +// the root and the last commit. +func branch(t *testing.T, commits ...authored) (dir, base, head string) { + t.Helper() + + dir = newRepository(t) + base = commitAs(t, dir, jane, "root\n") + head = base + for _, each := range commits { + head = commitAs(t, dir, each.author, each.message) + } + return dir, base, head +} + +// sideAndTrunk is a repository whose root is followed by a signed commit on a +// branch named side and another on the trunk, which is checked out, and the +// root. The two commits add different files, so they merge without a +// conflict. +func sideAndTrunk(t *testing.T) (dir, base string) { + t.Helper() + + dir, base, _ = branch(t) + trunk := gitFixture(t, dir, jane, "symbolic-ref", "--short", "HEAD") + gitFixture(t, dir, jane, "checkout", "--quiet", "-b", "side") + commitFile(t, dir, jane, "side.txt", "side\n", signedMessage("feat: side", jane)) + gitFixture(t, dir, jane, "checkout", "--quiet", trunk) + commitFile(t, dir, jane, "trunk.txt", "trunk\n", signedMessage("feat: trunk", jane)) + return dir, base +} + +// dependabotSignOff is the sign-off Dependabot writes into its commits, which +// names an address other than the one its commits are authored with. +var dependabotSignOff = identity{name: "dependabot[bot]", email: "support@github.com"} + +// TestRunMain_Repositories_JudgeTheCommitsOfTheRange verifies the runs over +// real repositories: a branch whose every commit is signed off by its author +// passes, one with a commit that is not fails with the remedy naming the +// base, an empty range passes with nothing judged, an unsigned merge is not +// judged when it brings no change of its own and fails when it does, a +// sign-off git reads with the separator byte at its end names nobody, a +// Dependabot commit is exempt only in a pull request Dependabot opened, +// -annotate adds the workflow commands, and the defaults read +// origin/main..HEAD. +func TestRunMain_Repositories_JudgeTheCommitsOfTheRange(t *testing.T) { + tests := []runCase{ + { + name: "every commit signed off by its author", + setup: func(t *testing.T) (string, []string) { + t.Helper() + dir, base, head := branch(t, by(jane, signedMessage("feat: one", jane)), by(john, signedMessage("fix: two", john))) + return dir, []string{"-base", base, "-head", head} + }, + wantStdout: []string{"audit_dco: 2 commit(s) in ", " ok ", "feat: one", "fix: two", "audit_dco: 2 signed off by the author, 0 not, 0 exempt, 0 merge(s) not judged\n"}, + }, + { + name: "a commit without a sign-off", + setup: func(t *testing.T) (string, []string) { + t.Helper() + dir, base, head := branch(t, by(jane, signedMessage("feat: one", jane)), by(jane, "fix: two\n")) + return dir, []string{"-base", base, "-head", head} + }, + wantCode: 1, + wantStdout: []string{" FAIL ", "fix: two\n carries no Signed-off-by trailer", "1 signed off by the author, 1 not", "git rebase --keep-base --signoff "}, + }, + { + name: "a commit signed off by somebody else", + setup: func(t *testing.T) (string, []string) { + t.Helper() + dir, base, head := branch(t, by(john, signedMessage("docs: three", jane))) + return dir, []string{"-base", base, "-head", head} + }, + wantCode: 1, + wantStdout: []string{"is signed off by Jane Doe , and not by its author"}, + }, + { + name: "an empty range", + setup: func(t *testing.T) (string, []string) { + t.Helper() + dir, base, _ := branch(t) + return dir, []string{"-base", base, "-head", base} + }, + wantStdout: []string{"audit_dco: no commits in ", "so nothing to judge"}, + }, + { + name: "an unsigned merge that brings no change of its own", + setup: func(t *testing.T) (string, []string) { + t.Helper() + dir, base := sideAndTrunk(t) + gitFixture(t, dir, jane, "merge", "--quiet", "--no-ff", "--no-edit", "--no-gpg-sign", "side") + return dir, []string{"-base", base} + }, + wantStdout: []string{" merge ", "a merge commit that brings no change of its own, which is not judged", "2 signed off by the author, 0 not, 0 exempt, 1 merge(s) not judged"}, + }, + { + name: "an unsigned merge with a change of its own", + setup: func(t *testing.T) (string, []string) { + t.Helper() + dir, base := sideAndTrunk(t) + mergeWithChange(t, dir, jane, "side", "Merge branch 'side'\n") + return dir, []string{"-base", base} + }, + wantCode: 1, + wantStdout: []string{" FAIL ", "Merge branch 'side'\n carries no Signed-off-by trailer", "2 signed off by the author, 1 not, 0 exempt, 0 merge(s) not judged"}, + }, + { + name: "a sign-off the separator byte ends", + setup: func(t *testing.T) (string, []string) { + t.Helper() + dir, base, head := branch(t, by(jane, "feat: one\n\nSigned-off-by: Jane Doe \x1e\n")) + return dir, []string{"-base", base, "-head", head} + }, + wantCode: 1, + wantStdout: []string{"is signed off by Jane Doe �, and not by its author"}, + }, + { + name: "Dependabot's commit in a pull request Dependabot opened", + setup: func(t *testing.T) (string, []string) { + t.Helper() + dir, base, head := branch(t, by(dependabot, signedMessage("chore(deps): bump", dependabotSignOff))) + return dir, []string{"-base", base, "-head", head, "-opened-by", "dependabot[bot]"} + }, + wantStdout: []string{" exempt ", "0 signed off by the author, 0 not, 1 exempt"}, + }, + { + name: "Dependabot's commit in a pull request a person opened", + setup: func(t *testing.T) (string, []string) { + t.Helper() + dir, base, head := branch(t, by(dependabot, signedMessage("chore(deps): bump", dependabotSignOff))) + return dir, []string{"-base", base, "-head", head, "-opened-by", "jmrplens"} + }, + wantCode: 1, + wantStdout: []string{"carries the identity of dependabot[bot], whose commits are exempt only in a pull request dependabot[bot] opened, and jmrplens opened this one"}, + }, + { + name: "annotated", + setup: func(t *testing.T) (string, []string) { + t.Helper() + dir, base, head := branch(t, by(jane, "fix: two\n")) + return dir, []string{"-base", base, "-head", head, "-annotate"} + }, + wantCode: 1, + wantStdout: []string{"\n::error title=DCO sign-off::", " by Jane Doe carries no Signed-off-by trailer\n"}, + }, + { + name: "the defaults, origin/main..HEAD", + setup: func(t *testing.T) (string, []string) { + t.Helper() + dir, base, _ := branch(t, by(jane, signedMessage("feat: one", jane))) + gitFixture(t, dir, jane, "update-ref", "refs/remotes/origin/main", base) + return dir, nil + }, + wantStdout: []string{"audit_dco: 1 commit(s) in origin/main..HEAD (", " ok "}, + }, + { + name: "revisions named by name", + setup: func(t *testing.T) (string, []string) { + t.Helper() + dir, base, head := branch(t, by(jane, "fix: two\n")) + gitFixture(t, dir, jane, "branch", "layer-below", base) + return dir, []string{"-base", "layer-below", "-head", head[:12]} + }, + wantCode: 1, + wantStdout: []string{"in layer-below..", "git rebase --keep-base --signoff layer-below\n"}, + }, + } + for _, tc := range tests { + t.Run(tc.name, tc.check) + } +} + +// TestRunMain_UnreadableRange_FailsWithTheReasonOnStderr verifies the runs +// git cannot answer: a base or a head naming no commit, which also says what +// a shallow clone lacks, a directory that is not a repository, a range git +// log refuses to read, and a merge git cannot remerge. +func TestRunMain_UnreadableRange_FailsWithTheReasonOnStderr(t *testing.T) { + tests := []runCase{ + { + name: "base names no commit", + setup: func(t *testing.T) (string, []string) { + t.Helper() + dir, _, _ := branch(t) + return dir, []string{"-base", "no-such-branch"} + }, + wantCode: 1, + wantStderr: "audit_dco: -base no-such-branch names no commit in ", + }, + { + name: "base missing from a shallow clone", + setup: func(t *testing.T) (string, []string) { + t.Helper() + dir, _, _ := branch(t) + return dir, []string{"-base", strings.Repeat("0", 40)} + }, + wantCode: 1, + wantStderr: "fetch-depth: 0", + }, + { + name: "head names no commit", + setup: func(t *testing.T) (string, []string) { + t.Helper() + dir, base, _ := branch(t) + return dir, []string{"-base", base, "-head", "no-such-branch"} + }, + wantCode: 1, + wantStderr: "audit_dco: -head no-such-branch names no commit in ", + }, + { + name: "not a repository", + setup: func(t *testing.T) (string, []string) { + t.Helper() + return t.TempDir(), []string{"-base", "HEAD"} + }, + wantCode: 1, + wantStderr: "names no commit", + }, + { + name: "a range git log cannot read", + setup: func(t *testing.T) (string, []string) { + t.Helper() + dir, base, head := branch(t, by(jane, signedMessage("feat: one", jane))) + gitFixture(t, dir, jane, "config", "log.showSignature", "maybe") + return dir, []string{"-base", base, "-head", head} + }, + wantCode: 1, + wantStderr: "audit_dco: could not read the commits of ", + }, + { + name: "a merge git cannot remerge", + setup: func(t *testing.T) (string, []string) { + t.Helper() + dir, base, head, _ := mergeMissingATree(t) + return dir, []string{"-base", base, "-head", head} + }, + wantCode: 1, + wantStderr: "audit_dco: could not read the commits of ", + }, + } + for _, tc := range tests { + t.Run(tc.name, tc.check) + } +} + +// TestRunMain_Arguments_RefuseWhatTheyCannotUse verifies the usage errors, +// which exit 2 before git is asked anything: an unknown flag, a positional +// argument, and a revision that is empty or that git would read as an option. +func TestRunMain_Arguments_RefuseWhatTheyCannotUse(t *testing.T) { + tests := []struct { + name string + args []string + wantStderr string + }{ + {name: "unknown flag", args: []string{"-no-such-flag"}, wantStderr: "flag provided but not defined"}, + {name: "positional argument", args: []string{"main"}, wantStderr: `audit_dco: takes no arguments, and was given ["main"]; name the range with -base and -head`}, + {name: "empty base", args: []string{"-base", ""}, wantStderr: `audit_dco: -base "" is not a revision`}, + {name: "empty head", args: []string{"-head", ""}, wantStderr: `audit_dco: -head "" is not a revision`}, + {name: "base read as an option", args: []string{"-base", "--output=x"}, wantStderr: `audit_dco: -base "--output=x" is not a revision`}, + {name: "head read as an option", args: []string{"-head", "-n1"}, wantStderr: `audit_dco: -head "-n1" is not a revision`}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var stdout, stderr bytes.Buffer + args := append([]string{"-dir", t.TempDir()}, tc.args...) + if code := runMain(context.Background(), args, &stdout, &stderr); code != 2 { + t.Errorf("runMain(%q) = %d, want 2", args, code) + } + if !strings.Contains(stderr.String(), tc.wantStderr) || stdout.Len() != 0 { + t.Errorf("stdout = %q, stderr = %q; want nothing on stdout and %q on stderr", stdout.String(), stderr.String(), tc.wantStderr) + } + }) + } +} + +// TestMain_UnknownFlag_ExitsWithTheUsageStatus verifies that main hands the +// process arguments after its own name to runMain and passes the status on to +// the exit seam. +func TestMain_UnknownFlag_ExitsWithTheUsageStatus(t *testing.T) { + previousArgs, previousExit := os.Args, osExit + t.Cleanup(func() { os.Args, osExit = previousArgs, previousExit }) + got := -1 + os.Args = []string{"audit_dco", "-no-such-flag"} + osExit = func(code int) { got = code } + main() + if got != 2 { + t.Errorf("main() exit = %d, want 2", got) + } +} + +// ciJob is one job of .github/workflows/ci.yml as the contracts below read it. +type ciJob struct { + If string `yaml:"if"` + Needs []string `yaml:"needs"` + Permissions map[string]string `yaml:"permissions"` + Steps []struct { + Name string `yaml:"name"` + Uses string `yaml:"uses"` + With map[string]any `yaml:"with"` + Run string `yaml:"run"` + Env map[string]string `yaml:"env"` + } `yaml:"steps"` +} + +// loadCIJob reads one job of ci.yml, failing the test when the workflow does +// not have it. +func loadCIJob(t *testing.T, name string) ciJob { + t.Helper() + + data, err := os.ReadFile("../../.github/workflows/ci.yml") + if err != nil { + t.Fatal(err) + } + var workflow struct { + Jobs map[string]yaml.Node `yaml:"jobs"` + } + if err = yaml.Unmarshal(data, &workflow); err != nil { + t.Fatal(err) + } + node, ok := workflow.Jobs[name] + if !ok { + t.Fatalf("ci.yml has no %s job", name) + } + var job ciJob + if err = node.Decode(&job); err != nil { + t.Fatalf("ci.yml's %s job: %v", name, err) + } + return job +} + +// TestCIWorkflow_DCOJob_RunsTheCheckOnEveryPullRequest holds ci.yml's dco job +// to this command: it runs on every pull request and on nothing else, with +// the whole history checked out and no permission beyond the workflow's read +// token, and hands the make target the pull request's base and head commits +// and the login that opened it, with annotations. +func TestCIWorkflow_DCOJob_RunsTheCheckOnEveryPullRequest(t *testing.T) { + job := loadCIJob(t, "dco") + if job.If != "github.event_name == 'pull_request'" { + t.Errorf("dco job if = %q, want it to run on pull requests alone", job.If) + } + if len(job.Permissions) != 0 { + t.Errorf("dco job permissions = %v, want none beyond the workflow's", job.Permissions) + } + checkedOut, ran := false, false + for _, step := range job.Steps { + if strings.HasPrefix(step.Uses, "actions/checkout@") { + checkedOut = step.With["fetch-depth"] == 0 && step.With["persist-credentials"] == false + } + if step.Run == "make check-dco DCO_FLAGS=-annotate" { + ran = step.Env["DCO_BASE"] == "${{ github.event.pull_request.base.sha }}" && + step.Env["DCO_HEAD"] == "${{ github.event.pull_request.head.sha }}" && + step.Env["DCO_OPENED_BY"] == "${{ github.event.pull_request.user.login }}" + } + } + if !checkedOut { + t.Error("dco job does not check out the whole history without persisting the token") + } + if !ran { + t.Error("dco job does not run make check-dco DCO_FLAGS=-annotate with the pull request's base, head and opener") + } +} + +// TestCIWorkflow_ChecksJob_WaitsOnTheDCOJobAndPassesItsSkip holds the fan-in +// job to the dco job: it waits on it, and counts the skip a push to main +// makes as a pass. +func TestCIWorkflow_ChecksJob_WaitsOnTheDCOJobAndPassesItsSkip(t *testing.T) { + job := loadCIJob(t, "checks") + if !slices.Contains(job.Needs, "dco") { + t.Errorf("checks job needs %v, want dco among them", job.Needs) + } + maySkip := false + for _, step := range job.Steps { + maySkip = maySkip || (step.Name == "Decide" && slices.Contains(strings.Fields(step.Env["MAY_SKIP"]), "dco")) + } + if !maySkip { + t.Error("checks job's Decide step does not count a skipped dco job as a pass, and a push to main skips it") + } +} + +// TestMakefile_CheckDCO_HandsTheRangeAndTheOpenerToTheCommand holds the make +// target to the command line the CI job relies on, and to defaults that judge +// the branch against origin/main. +func TestMakefile_CheckDCO_HandsTheRangeAndTheOpenerToTheCommand(t *testing.T) { + data, err := os.ReadFile("../../Makefile") + if err != nil { + t.Fatal(err) + } + makefile := string(data) + tests := []struct{ name, want string }{ + {name: "base default", want: "\nDCO_BASE ?= origin/main\n"}, + {name: "head default", want: "\nDCO_HEAD ?= HEAD\n"}, + {name: "recipe", want: "\ncheck-dco:\n\tgo run ./cmd/audit_dco/ -base \"$(DCO_BASE)\" -head \"$(DCO_HEAD)\" -opened-by \"$(DCO_OPENED_BY)\" $(DCO_FLAGS)\n"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if !strings.Contains(makefile, tc.want) { + t.Errorf("Makefile does not hold %q", tc.want) + } + }) + } +} diff --git a/cmd/audit_dco/report.go b/cmd/audit_dco/report.go new file mode 100644 index 000000000..db7899fa4 --- /dev/null +++ b/cmd/audit_dco/report.go @@ -0,0 +1,135 @@ +package main + +import ( + "fmt" + "io" + "strings" + "unicode" +) + +// marks are the words a report line opens with, one per outcome. +var marks = map[outcome]string{ + signedOff: "ok", + unsigned: "FAIL", + signedByOthers: "FAIL", + exempt: "exempt", + merge: "merge", +} + +// remedy is what a failing run prints after the count, with the base the +// branch is rebased on in place of its one verb. +const remedy = ` +Each commit marked FAIL needs a Signed-off-by trailer naming its author, which +is how the author certifies the Developer Certificate of Origin +(https://developercertificate.org/) for it, as CONTRIBUTING.md sets out under +Developer Certificate of Origin. To sign off the commits you authored where +they stand, and push the branch again: + + git rebase --keep-base --signoff %s + git push --force-with-lease + +Then commit with git commit --signoff (-s). A commit somebody else authored is +signed off by that person. A rebase does not keep a merge commit, so a merge +marked FAIL, which brings a change of its own, is signed off with git commit +--amend --no-edit --signoff while it is the branch's last commit. In a stack, +run the rebase on the top layer with --update-refs as well, so the branch of +every layer below moves with it, and push each layer. +` + +// annotationTitle is the title of the workflow command a failing commit gets +// under -annotate. +const annotationTitle = "DCO sign-off" + +// report is what a run judged and how to word it: the range as the command +// line named it and as commits, the base the remedy rebases on, the login +// that opened the pull request, the verdicts in order, and whether to add a +// workflow command per failing commit. +type report struct { + span string + base string + openedBy string + verdicts []verdict + annotate bool +} + +// write prints the report and returns whether a commit failed: a line per +// commit with the reasons under it, the count, and for a failing run the +// remedy and, under annotate, one error command per failing commit. What a +// commit's author wrote is printed through printable, so none of it can end a +// line and start a workflow command of its own. +func (r report) write(w io.Writer) bool { + if len(r.verdicts) == 0 { + _, _ = fmt.Fprintf(w, "%s: no commits in %s, so nothing to judge\n", toolName, r.span) + return false + } + _, _ = fmt.Fprintf(w, "%s: %d commit(s) in %s\n", toolName, len(r.verdicts), r.span) + counts := map[outcome]int{} + for _, v := range r.verdicts { + counts[v.outcome]++ + _, _ = fmt.Fprintf(w, " %-7s %s %s %s\n", marks[v.outcome], v.commit.short(), printable(v.commit.author.String()), printable(v.commit.subject)) + for _, reason := range reasons(v, r.openedBy) { + _, _ = fmt.Fprintf(w, " %s\n", reason) + } + } + failing := counts[unsigned] + counts[signedByOthers] + _, _ = fmt.Fprintf(w, "%s: %d signed off by the author, %d not, %d exempt, %d merge(s) not judged\n", + toolName, counts[signedOff], failing, counts[exempt], counts[merge]) + if failing == 0 { + return false + } + _, _ = fmt.Fprintf(w, remedy, printable(r.base)) + if r.annotate { + for _, v := range r.verdicts { + if v.failed() { + message := v.commit.short() + " by " + printable(v.commit.author.String()) + " " + strings.Join(reasons(v, r.openedBy), "; ") + _, _ = fmt.Fprintf(w, "::error title=%s::%s\n", annotationTitle, escapeData(message)) + } + } + } + return true +} + +// reasons words why a commit reached its outcome, one line each: nothing for +// a commit signed off by its author, and for a failing commit carrying a +// declared bot's identity, why the bot's exemption did not hold. +func reasons(v verdict, openedBy string) []string { + var lines []string + switch v.outcome { + case unsigned: + lines = append(lines, "carries no Signed-off-by trailer") + case signedByOthers: + lines = append(lines, "is signed off by "+printable(strings.Join(v.commit.signoffs, "; "))+", and not by its author") + case exempt: + return []string{fmt.Sprintf("a commit of %[1]s, declared in bots.go, in a pull request %[1]s opened", v.bot.login)} + case merge: + return []string{"a merge commit that brings no change of its own, which is not judged"} + default: + return nil + } + if v.bot != nil { + opener := "no -opened-by names who opened this one" + if openedBy != "" { + opener = printable(openedBy) + " opened this one" + } + lines = append(lines, fmt.Sprintf("carries the identity of %[1]s, whose commits are exempt only in a pull request %[1]s opened, and %[2]s", v.bot.login, opener)) + } + return lines +} + +// printable replaces every control character of s with U+FFFD, so that text a +// commit's author chose can neither end a report line nor start one. +func printable(s string) string { + return strings.Map(func(r rune) rune { + if unicode.IsControl(r) { + return unicode.ReplacementChar + } + return r + }, s) +} + +// workflowData escapes the characters GitHub Actions decodes in the message of +// a workflow command. +var workflowData = strings.NewReplacer("%", "%25", "\r", "%0D", "\n", "%0A") + +// escapeData escapes s for the message of a workflow command. +func escapeData(s string) string { return workflowData.Replace(s) } diff --git a/cmd/audit_dco/report_test.go b/cmd/audit_dco/report_test.go new file mode 100644 index 000000000..71f58f86e --- /dev/null +++ b/cmd/audit_dco/report_test.go @@ -0,0 +1,211 @@ +// report_test.go covers what a run prints: one line per commit with the +// reason under each that is not simply signed off, the count, how to sign a +// failing commit off, the workflow commands of -annotate, and the escaping of +// what a commit's author chose to write. +package main + +import ( + "bytes" + "strings" + "testing" +) + +// hashOf is a forty-character hash made of one repeated digit, so a report +// line shows which commit it is. +func hashOf(digit string) string { return strings.Repeat(digit, 40) } + +// everyOutcome is one verdict of each outcome, a failing commit carrying a +// bot's identity included. +func everyOutcome() []verdict { + bot := declaredBots[0] + return []verdict{ + {commit: commit{hash: hashOf("1"), author: jane, subject: "feat: one"}, outcome: signedOff}, + {commit: commit{hash: hashOf("2"), author: jane, subject: "fix: two"}, outcome: unsigned}, + {commit: commit{hash: hashOf("3"), author: john, subject: "docs: three", signoffs: []string{"Jane Doe ", "Jane"}}, outcome: signedByOthers}, + {commit: commit{hash: hashOf("4"), author: dependabot, subject: "chore(deps): bump"}, outcome: exempt, bot: &bot}, + {commit: commit{hash: hashOf("5"), parents: 2, author: jane, subject: "Merge branch 'side'", cleanMerge: true}, outcome: merge}, + {commit: commit{hash: hashOf("6"), author: dependabot, subject: "chore(deps): copied"}, outcome: unsigned, bot: &bot}, + } +} + +// TestReport_Write_EveryOutcome_WordsEachCommitAndSaysHowToSignOff pins the +// whole report of a run with one commit of each outcome: the header, each +// commit's line and reasons, the count, and the remedy naming the base. +func TestReport_Write_EveryOutcome_WordsEachCommitAndSaysHowToSignOff(t *testing.T) { + var out bytes.Buffer + r := report{span: "origin/main..HEAD (aaaaaaaaaaaa..bbbbbbbbbbbb)", base: "origin/main", openedBy: "jmrplens", verdicts: everyOutcome()} + if !r.write(&out) { + t.Error("write() = false, want true for a run with failing commits") + } + want := `audit_dco: 6 commit(s) in origin/main..HEAD (aaaaaaaaaaaa..bbbbbbbbbbbb) + ok 111111111111 Jane Doe feat: one + FAIL 222222222222 Jane Doe fix: two + carries no Signed-off-by trailer + FAIL 333333333333 John Roe docs: three + is signed off by Jane Doe ; Jane, and not by its author + exempt 444444444444 dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> chore(deps): bump + a commit of dependabot[bot], declared in bots.go, in a pull request dependabot[bot] opened + merge 555555555555 Jane Doe Merge branch 'side' + a merge commit that brings no change of its own, which is not judged + FAIL 666666666666 dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> chore(deps): copied + carries no Signed-off-by trailer + carries the identity of dependabot[bot], whose commits are exempt only in a pull request dependabot[bot] opened, and jmrplens opened this one +audit_dco: 1 signed off by the author, 3 not, 1 exempt, 1 merge(s) not judged + +Each commit marked FAIL needs a Signed-off-by trailer naming its author, which +is how the author certifies the Developer Certificate of Origin +(https://developercertificate.org/) for it, as CONTRIBUTING.md sets out under +Developer Certificate of Origin. To sign off the commits you authored where +they stand, and push the branch again: + + git rebase --keep-base --signoff origin/main + git push --force-with-lease + +Then commit with git commit --signoff (-s). A commit somebody else authored is +signed off by that person. A rebase does not keep a merge commit, so a merge +marked FAIL, which brings a change of its own, is signed off with git commit +--amend --no-edit --signoff while it is the branch's last commit. In a stack, +run the rebase on the top layer with --update-refs as well, so the branch of +every layer below moves with it, and push each layer. +` + if got := out.String(); got != want { + t.Errorf("write() printed:\n%s\nwant:\n%s", got, want) + } +} + +// TestReport_Write_Annotate_AddsAnErrorCommandPerFailingCommit verifies that +// -annotate adds one GitHub Actions error command per failing commit, after +// the report, with every reason of the commit in it, and none for a commit +// that passes, is exempt or is a merge. +func TestReport_Write_Annotate_AddsAnErrorCommandPerFailingCommit(t *testing.T) { + var out bytes.Buffer + r := report{span: "a..b", base: "a", verdicts: everyOutcome(), annotate: true} + r.write(&out) + var commands []string + for line := range strings.SplitSeq(out.String(), "\n") { + if strings.HasPrefix(line, "::") { + commands = append(commands, line) + } + } + want := []string{ + "::error title=DCO sign-off::222222222222 by Jane Doe carries no Signed-off-by trailer", + "::error title=DCO sign-off::333333333333 by John Roe is signed off by Jane Doe ; Jane, and not by its author", + "::error title=DCO sign-off::666666666666 by dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> carries no Signed-off-by trailer; carries the identity of dependabot[bot], whose commits are exempt only in a pull request dependabot[bot] opened, and no -opened-by names who opened this one", + } + if strings.Join(commands, "\n") != strings.Join(want, "\n") { + t.Errorf("workflow commands:\n%s\nwant:\n%s", strings.Join(commands, "\n"), strings.Join(want, "\n")) + } + if !strings.HasSuffix(out.String(), want[len(want)-1]+"\n") { + t.Errorf("the workflow commands do not come last:\n%s", out.String()) + } +} + +// TestReport_Write_NoFailure_PrintsTheCountAndNoRemedy verifies that a run +// whose every commit holds says so with the count, and prints neither the +// remedy nor a workflow command even under -annotate. +func TestReport_Write_NoFailure_PrintsTheCountAndNoRemedy(t *testing.T) { + var out bytes.Buffer + verdicts := everyOutcome() + r := report{span: "a..b", base: "a", verdicts: []verdict{verdicts[0], verdicts[3], verdicts[4]}, annotate: true} + if r.write(&out) { + t.Error("write() = true, want false when no commit fails") + } + want := `audit_dco: 3 commit(s) in a..b + ok 111111111111 Jane Doe feat: one + exempt 444444444444 dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> chore(deps): bump + a commit of dependabot[bot], declared in bots.go, in a pull request dependabot[bot] opened + merge 555555555555 Jane Doe Merge branch 'side' + a merge commit that brings no change of its own, which is not judged +audit_dco: 1 signed off by the author, 0 not, 1 exempt, 1 merge(s) not judged +` + if got := out.String(); got != want { + t.Errorf("write() printed:\n%s\nwant:\n%s", got, want) + } +} + +// TestReport_Write_OnlySignedByOthers_CountsThemAsNotSignedOff verifies the +// count of a run whose one failing commit is signed off by somebody else, +// which the count adds to the commits with no trailer. +func TestReport_Write_OnlySignedByOthers_CountsThemAsNotSignedOff(t *testing.T) { + var out bytes.Buffer + r := report{span: "a..b", base: "a", verdicts: []verdict{everyOutcome()[2]}} + if !r.write(&out) { + t.Error("write() = false, want true") + } + if !strings.Contains(out.String(), "audit_dco: 0 signed off by the author, 1 not, 0 exempt, 0 merge(s) not judged\n") { + t.Errorf("write() printed:\n%s\nwant the count of one commit not signed off", out.String()) + } +} + +// TestReport_Write_NoCommits_SaysThereIsNothingToJudge verifies the report of +// an empty range: one line, and no failure. +func TestReport_Write_NoCommits_SaysThereIsNothingToJudge(t *testing.T) { + var out bytes.Buffer + if (report{span: "a..a"}).write(&out) { + t.Error("write() = true, want false for an empty range") + } + if got, want := out.String(), "audit_dco: no commits in a..a, so nothing to judge\n"; got != want { + t.Errorf("write() printed %q, want %q", got, want) + } +} + +// TestReport_Write_AuthorText_CannotStartALineOrBreakOne verifies that what a +// commit's author wrote, the subject, the identity, a trailer value and the +// opener, reaches the report with its control characters replaced, so none of +// it can end a line and start a workflow command of its own, and that the +// annotation escapes the percent sign GitHub decodes. +func TestReport_Write_AuthorText_CannotStartALineOrBreakOne(t *testing.T) { + bot := declaredBots[0] + crafted := commit{ + hash: hashOf("7"), + author: identity{name: "Jane\rDoe", email: "jane@example.org"}, + subject: "fix: 100%\n::warning::forged", + signoffs: []string{"John\x1bRoe 100% "}, + } + var out bytes.Buffer + r := report{ + span: "a..b", base: "a\nb", openedBy: "x\ny", annotate: true, + verdicts: []verdict{{commit: crafted, outcome: signedByOthers, bot: &bot}}, + } + r.write(&out) + for line := range strings.SplitSeq(out.String(), "\n") { + if strings.HasPrefix(line, "::") && !strings.HasPrefix(line, "::error title=DCO sign-off::") { + t.Errorf("a line starts a workflow command the report did not write: %q", line) + } + } + tests := []struct{ name, want string }{ + {name: "author and subject", want: "Jane\uFFFDDoe fix: 100%\uFFFD::warning::forged"}, + {name: "trailer value", want: "is signed off by John\uFFFDRoe 100% , and not by its author"}, + {name: "opener", want: "and x\uFFFDy opened this one"}, + {name: "base", want: "git rebase --keep-base --signoff a\uFFFDb"}, + {name: "annotation", want: "::error title=DCO sign-off::777777777777 by Jane\uFFFDDoe is signed off by John\uFFFDRoe 100%25 , and not by its author;"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if !strings.Contains(out.String(), tc.want) { + t.Errorf("the report does not hold %q:\n%s", tc.want, out.String()) + } + }) + } +} + +// TestPrintable_ControlCharacters_AreReplaced verifies that every control +// character, the line breaks, the escape character and DEL among them, is +// replaced, and that ordinary text, accents included, is kept as it is. +func TestPrintable_ControlCharacters_AreReplaced(t *testing.T) { + if got, want := printable("a\nb\rc\td\x1be\x7ff\x00g"), "a\uFFFDb\uFFFDc\uFFFDd\uFFFDe\uFFFDf\uFFFDg"; got != want { + t.Errorf("printable() = %q, want %q", got, want) + } + if got, want := printable("José M. Requena Plens "), "José M. Requena Plens "; got != want { + t.Errorf("printable() = %q, want %q", got, want) + } +} + +// TestEscapeData_WorkflowCommandMessage_EscapesWhatGitHubDecodes verifies the +// escaping GitHub Actions decodes in a workflow command's message: the +// percent sign, carriage return and line feed. +func TestEscapeData_WorkflowCommandMessage_EscapesWhatGitHubDecodes(t *testing.T) { + if got, want := escapeData("100% done\r\nnext"), "100%25 done%0D%0Anext"; got != want { + t.Errorf("escapeData() = %q, want %q", got, want) + } +} diff --git a/cmd/audit_dco/signoff.go b/cmd/audit_dco/signoff.go new file mode 100644 index 000000000..4ccf5dd98 --- /dev/null +++ b/cmd/audit_dco/signoff.go @@ -0,0 +1,110 @@ +package main + +import "strings" + +// identity is a person or an app as git records one: a name and an email. +type identity struct{ name, email string } + +// String writes the identity the way git and a Signed-off-by trailer do. +func (id identity) String() string { return id.name + " <" + id.email + ">" } + +// parseIdentity reads a trailer value as a name followed by an email in angle +// brackets that end the value, the space around each trimmed. It names nobody +// unless both are there, the name holds no closing bracket (it ends at the +// first opening one, and git keeps neither in a name, so such a name could +// never be an author's), and the email holds an at sign and no angle bracket +// or blank. +func parseIdentity(value string) (identity, bool) { + name, rest, _ := strings.Cut(strings.TrimSpace(value), "<") + email, closed := strings.CutSuffix(rest, ">") + name, email = strings.TrimSpace(name), strings.TrimSpace(email) + if !closed || name == "" || strings.ContainsRune(name, '>') || + !strings.Contains(email, "@") || strings.ContainsAny(email, "<> \t") { + return identity{}, false + } + return identity{name: name, email: email}, true +} + +// sameIdentity reports whether a and b name the same identity: the same name +// and the same email, each compared without regard to case, since neither is +// case-sensitive to the person it names and git commit --signoff copies both +// from the configuration that wrote the author. +func sameIdentity(a, b identity) bool { + return strings.EqualFold(a.name, b.name) && strings.EqualFold(a.email, b.email) +} + +// shortHash is how many characters of a hash the report prints. +const shortHash = 12 + +// commit is one commit of the range, as git records it: its hash, how many +// parents it has, its author, its subject, the values of its Signed-off-by +// trailers in the order they appear, and whether it is a merge of two +// parents that brings no change of its own (markCleanMerges decides). +type commit struct { + hash string + parents int + author identity + subject string + signoffs []string + cleanMerge bool +} + +// short is the commit's hash abbreviated for the report. +func (c commit) short() string { return c.hash[:min(len(c.hash), shortHash)] } + +// outcome is what judging one commit decided. +type outcome int + +// The outcomes. Only unsigned and signedByOthers fail a run. +const ( + // signedOff is a commit one of whose trailers names its author. + signedOff outcome = iota + // unsigned is a commit with no Signed-off-by trailer. + unsigned + // signedByOthers is a commit whose trailers name only other people. + signedByOthers + // exempt is a declared bot's commit in a pull request that bot opened. + exempt + // merge is a merge commit that brings no change of its own, which is + // not judged. + merge +) + +// verdict is one judged commit, its outcome, and the declared bot that +// exempts it or whose identity a failing commit carries. +type verdict struct { + commit commit + outcome outcome + bot *declaredBot +} + +// failed reports whether the verdict fails the run. +func (v verdict) failed() bool { return v.outcome == unsigned || v.outcome == signedByOthers } + +// judge decides one commit of a pull request openedBy opened. A merge that +// brings no change of its own is not judged, a declared bot's commit in a +// pull request that bot opened is exempt, and any other commit, any other +// merge included, passes only when a Signed-off-by trailer names its author. +// A failing commit that carries a declared bot's identity keeps the bot, so +// the report can say why the exemption did not hold. +func judge(c commit, openedBy string) verdict { + if c.cleanMerge { + return verdict{commit: c, outcome: merge} + } + if bot, ok := exemptingBot(c.author, openedBy); ok { + return verdict{commit: c, outcome: exempt, bot: &bot} + } + for _, value := range c.signoffs { + if signer, ok := parseIdentity(value); ok && sameIdentity(signer, c.author) { + return verdict{commit: c, outcome: signedOff} + } + } + failing := verdict{commit: c, outcome: unsigned} + if len(c.signoffs) > 0 { + failing.outcome = signedByOthers + } + if bot, ok := claimedBot(c.author); ok { + failing.bot = &bot + } + return failing +} diff --git a/cmd/audit_dco/signoff_test.go b/cmd/audit_dco/signoff_test.go new file mode 100644 index 000000000..4b5f50dd1 --- /dev/null +++ b/cmd/audit_dco/signoff_test.go @@ -0,0 +1,261 @@ +// signoff_test.go covers how a commit is judged: reading a trailer as an +// identity, comparing two identities, and the outcome each kind of commit +// reaches. +package main + +import "testing" + +// jane and john are the people the judging tests commit as. +var ( + jane = identity{name: "Jane Doe", email: "jane@example.org"} + john = identity{name: "John Roe", email: "john@example.org"} +) + +// dependabot is the identity Dependabot's commits carry, as declared. +var dependabot = identity{name: "dependabot[bot]", email: "49699333+dependabot[bot]@users.noreply.github.com"} + +// TestIdentity_String_WritesTheNameAndTheEmailInAngleBrackets verifies the +// form an identity is printed in, which is the one a trailer is written in. +func TestIdentity_String_WritesTheNameAndTheEmailInAngleBrackets(t *testing.T) { + if got, want := jane.String(), "Jane Doe "; got != want { + t.Errorf("String() = %q, want %q", got, want) + } +} + +// TestParseIdentity_Values_ReadANameAndAnEmailOrNobody verifies that a +// trailer value is an identity only when it is a name followed by an address +// in angle brackets, with the surrounding space trimmed, and that every other +// shape names nobody. +func TestParseIdentity_Values_ReadANameAndAnEmailOrNobody(t *testing.T) { + tests := []struct { + name string + value string + want identity + ok bool + }{ + {name: "name and email", value: "Jane Doe ", want: jane, ok: true}, + {name: "surrounding space trimmed", value: " Jane Doe ", want: jane, ok: true}, + {name: "space inside the brackets trimmed", value: "Jane Doe < jane@example.org >", want: jane, ok: true}, + {name: "non-ASCII name", value: "José M. Requena Plens ", want: identity{name: "José M. Requena Plens", email: "mail@jmrp.io"}, ok: true}, + {name: "name alone", value: "Jane Doe"}, + {name: "empty", value: ""}, + {name: "email alone", value: ""}, + {name: "blank name", value: " "}, + {name: "empty brackets", value: "Jane Doe <>"}, + {name: "unclosed bracket", value: "Jane Doe (work)"}, + {name: "no at sign", value: "Jane Doe "}, + {name: "space in the address", value: "Jane Doe "}, + {name: "tab in the address", value: "Jane Doe "}, + {name: "two addresses", value: "Jane Doe "}, + {name: "closing bracket in the name", value: "Jane > Doe "}, + {name: "bracket inside the address", value: "Jane Doe @example.org>"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, ok := parseIdentity(tc.value) + if ok != tc.ok || got != tc.want { + t.Errorf("parseIdentity(%q) = %#v, %v; want %#v, %v", tc.value, got, ok, tc.want, tc.ok) + } + }) + } +} + +// TestSameIdentity_Pairs_MatchTheNameAndTheEmailWithoutRegardToCase verifies +// that two identities are the same when both the name and the email agree, +// letters compared without regard to case, and differ when either does not. +func TestSameIdentity_Pairs_MatchTheNameAndTheEmailWithoutRegardToCase(t *testing.T) { + tests := []struct { + name string + a, b identity + want bool + }{ + {name: "identical", a: jane, b: jane, want: true}, + {name: "case differs", a: jane, b: identity{name: "JANE DOE", email: "Jane@Example.ORG"}, want: true}, + {name: "case differs outside ASCII", a: identity{name: "José", email: "j@x.org"}, b: identity{name: "JOSÉ", email: "j@x.org"}, want: true}, + {name: "name differs", a: jane, b: identity{name: "Jane Roe", email: jane.email}}, + {name: "email differs", a: jane, b: identity{name: jane.name, email: "jane@example.com"}}, + {name: "both differ", a: jane, b: john}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := sameIdentity(tc.a, tc.b); got != tc.want { + t.Errorf("sameIdentity(%v, %v) = %v, want %v", tc.a, tc.b, got, tc.want) + } + }) + } +} + +// TestCommit_Short_AbbreviatesTheHashToTwelveCharacters verifies the +// abbreviation the report prints, and that a hash shorter than that is +// printed whole rather than cut. +func TestCommit_Short_AbbreviatesTheHashToTwelveCharacters(t *testing.T) { + tests := []struct{ hash, want string }{ + {hash: "0123456789abcdef0123456789abcdef01234567", want: "0123456789ab"}, + {hash: "0123456789ab", want: "0123456789ab"}, + {hash: "0123", want: "0123"}, + } + for _, tc := range tests { + t.Run(tc.hash, func(t *testing.T) { + if got := (commit{hash: tc.hash}).short(); got != tc.want { + t.Errorf("short() = %q, want %q", got, tc.want) + } + }) + } +} + +// TestJudge_Commits_ReachTheOutcomeTheirSignOffsDecide verifies each outcome: +// a commit passes when a trailer names its author and fails when it has none +// or names only others; a merge that brings no change of its own is not +// judged, and any other merge is judged like any commit; a declared bot's +// commit is exempt only in a pull request that bot opened; and a failing +// commit that carries a bot's identity says which bot. +func TestJudge_Commits_ReachTheOutcomeTheirSignOffsDecide(t *testing.T) { + tests := []struct { + name string + commit commit + openedBy string + want outcome + wantBot bool + }{ + { + name: "signed off by its author", + commit: commit{parents: 1, author: jane, signoffs: []string{"Jane Doe "}}, + want: signedOff, + }, + { + name: "signed off by its author in another case", + commit: commit{parents: 1, author: jane, signoffs: []string{"jane doe "}}, + want: signedOff, + }, + { + name: "its author's sign-off after somebody else's", + commit: commit{parents: 1, author: jane, signoffs: []string{"John Roe ", "Jane Doe "}}, + want: signedOff, + }, + { + name: "root commit signed off", + commit: commit{parents: 0, author: jane, signoffs: []string{"Jane Doe "}}, + want: signedOff, + }, + { + name: "no trailer", + commit: commit{parents: 1, author: jane}, + want: unsigned, + }, + { + name: "signed off by somebody else", + commit: commit{parents: 1, author: jane, signoffs: []string{"John Roe "}}, + want: signedByOthers, + }, + { + name: "author's name with another email", + commit: commit{parents: 1, author: jane, signoffs: []string{"Jane Doe "}}, + want: signedByOthers, + }, + { + name: "trailer that names nobody", + commit: commit{parents: 1, author: jane, signoffs: []string{"Jane Doe"}}, + want: signedByOthers, + }, + { + name: "merge that brings no change, without a trailer", + commit: commit{parents: 2, author: jane, cleanMerge: true}, + want: merge, + }, + { + name: "merge with a change of its own, without a trailer", + commit: commit{parents: 2, author: jane}, + want: unsigned, + }, + { + name: "merge with a change of its own, signed off by its author", + commit: commit{parents: 2, author: jane, signoffs: []string{"Jane Doe "}}, + want: signedOff, + }, + { + name: "octopus merge without a trailer", + commit: commit{parents: 3, author: jane}, + want: unsigned, + }, + { + name: "Dependabot's merge that brings no change, in a pull request a person opened", + commit: commit{parents: 2, author: dependabot, cleanMerge: true}, + openedBy: "jmrplens", + want: merge, + }, + { + name: "Dependabot in a pull request Dependabot opened", + commit: commit{parents: 1, author: dependabot, signoffs: []string{"dependabot[bot] "}}, + openedBy: "dependabot[bot]", + want: exempt, + wantBot: true, + }, + { + name: "Dependabot's identity in a pull request a person opened", + commit: commit{parents: 1, author: dependabot}, + openedBy: "jmrplens", + want: unsigned, + wantBot: true, + }, + { + name: "Dependabot's own sign-off in a pull request a person opened", + commit: commit{parents: 1, author: dependabot, signoffs: []string{"dependabot[bot] "}}, + openedBy: "jmrplens", + want: signedByOthers, + wantBot: true, + }, + { + name: "a person's commit in a pull request Dependabot opened", + commit: commit{parents: 1, author: jane}, + openedBy: "dependabot[bot]", + want: unsigned, + }, + { + name: "a person's signed commit in a pull request Dependabot opened", + commit: commit{parents: 1, author: jane, signoffs: []string{"Jane Doe "}}, + openedBy: "dependabot[bot]", + want: signedOff, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := judge(tc.commit, tc.openedBy) + if got.outcome != tc.want { + t.Errorf("judge(%+v, %q).outcome = %d, want %d", tc.commit, tc.openedBy, got.outcome, tc.want) + } + if (got.bot != nil) != tc.wantBot { + t.Errorf("judge(%+v, %q).bot = %v, want a bot: %v", tc.commit, tc.openedBy, got.bot, tc.wantBot) + } + if got.bot != nil && got.bot.login != "dependabot[bot]" { + t.Errorf("judge(%+v, %q).bot = %q, want dependabot[bot]", tc.commit, tc.openedBy, got.bot.login) + } + if got.commit.author != tc.commit.author || got.commit.parents != tc.commit.parents { + t.Errorf("judge(%+v, %q).commit = %+v, want the commit judged", tc.commit, tc.openedBy, got.commit) + } + }) + } +} + +// TestVerdict_Failed_IsTrueForTheTwoOutcomesThatFailARun verifies that only a +// commit without its author's sign-off fails the run. +func TestVerdict_Failed_IsTrueForTheTwoOutcomesThatFailARun(t *testing.T) { + tests := []struct { + name string + outcome outcome + want bool + }{ + {name: "signed off", outcome: signedOff}, + {name: "unsigned", outcome: unsigned, want: true}, + {name: "signed by others", outcome: signedByOthers, want: true}, + {name: "exempt", outcome: exempt}, + {name: "merge", outcome: merge}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := (verdict{outcome: tc.outcome}).failed(); got != tc.want { + t.Errorf("failed() = %v, want %v", got, tc.want) + } + }) + } +} diff --git a/docs/development/cmd-utilities.md b/docs/development/cmd-utilities.md index 17e9ce44c..c92659327 100644 --- a/docs/development/cmd-utilities.md +++ b/docs/development/cmd-utilities.md @@ -47,6 +47,7 @@ Repository tooling is a Go command under `cmd/`, sharing what it needs through ` | `audit_image_vulns` | Release & supply-chain audits | Holds the image's Alpine packages to Trivy's vulnerability database, one Trivy report per platform, before the release pushes it: a fixable HIGH or CRITICAL advisory fails unless a declaration accepts it | `make check-image-vulns` | | `reproduce_release` | Release & supply-chain audits | Rebuilds every binary a release published from its tag and compares each with the published one byte for byte, or builds every release target of a tree twice and compares the two builds | `make check-reproducible`, `make check-repeatable` | | `audit_site_headers` | Release & supply-chain audits | Fetches the documentation site and judges the four security headers hardened_site names: losing one the site serves fails, one still waiting for the maintainer's Cloudflare rule is reported with the value to set | `make check-site-headers`, the `headers` job of `pages.yml` | +| `audit_dco` | Release & supply-chain audits | Holds every commit a pull request brings to the Developer Certificate of Origin: each needs a `Signed-off-by` trailer naming its author; a merge that brings no change of its own is not judged, and a declared bot's commits are exempt in the pull requests it opens | `make check-dco`, the `dco` job of `ci.yml` | | `gen_action_catalog_manifest` | Generators | Generates the ActionSpec group-builder manifest | `make gen-action-catalog-manifest` | | `gen_lhm_manifest` | Generators | Generates the tools/prompts/resources arrays in `lhm.plugin.json` (LobeHub Marketplace) | `make gen-lhm-manifest` | | `gen_llms` | Generators | Generates `llms.txt` and `llms-full.txt` | `make gen-llms` | @@ -1841,6 +1842,42 @@ For each judged page, ` answered ` and one line per header indented - `make check-site-headers`: judges the published site's root, or `SITE_URL` when given. It needs the network, so `make analyze` does not run it; the `headers` job of `pages.yml` runs it after every deploy, on the URL the deploy published. +### audit_dco + +Holds the commits of a branch to the [Developer Certificate of Origin](../../CONTRIBUTING.md#developer-certificate-of-origin): every commit it judges has to carry a `Signed-off-by` trailer naming its author, which is how the author certifies the certificate for that commit. The commits judged are the ones reachable from `-head` and not from `-base`, the ones a pull request brings. In CI they come from the event's base and head commits, the range the em dash job reads. The event of a stacked pull request names the head of the layer below as its base, so each layer's pull request judges that layer's commits and no others: a layer below is judged only by its own pull request, with the `ci.yml` that layer carries. + +Each commit is read from git with the trailers git finds in its message (`git log`'s `%(trailers)` placeholder, which applies the rules of `git interpret-trailers`), so a sign-off counts exactly when git finds one. git reads the message's last paragraph as trailers when every line of it is one, or when at least a quarter of its lines are and one of them is a trailer git writes itself, as `Signed-off-by` is, or one its configuration declares. So a `Signed-off-by` line outside that paragraph is not a trailer, nor is one in a last paragraph where fewer than a quarter of the lines are trailers, while one sharing the paragraph with a line of prose is, and so is a key written in lower case. A commit passes when one of its trailers names its author, the name and the email compared without regard to case. It fails with no `Signed-off-by` trailer, and when its trailers name only other people or nobody (a value that is not a name followed by an email in angle brackets). `git log` prints each commit's fields between separator bytes, which a message can carry too; a record ends only where its separator is followed by a newline, which no field git prints holds, so such a message is read as git reads it, the byte kept in its field (a trailer it ends names nobody). The commits are also listed by `git rev-list`, whose hashes carry nothing a commit's author chose, and a run in which the two lists differ stops instead of judging either, so a sign-off never moves onto another commit. What an author wrote is printed with its control characters replaced, so a subject cannot end a report line and start a workflow command. + +A merge of two parents that brings no change of its own is reported and not judged: its tree is the one git's own merge of its parents makes, which `git show --remerge-diff` prints no difference for, as for the merge GitHub's Update branch button writes, and the commits it brings in that the base does not hold are judged themselves. Every other merge is judged like any other commit: one that resolves a conflict, adds or drops a change while merging, or joins more than two parents, which git does not remerge. A merge git cannot remerge, because the clone lacks an object it needs, stops the run. The commits of a declared bot are exempt in a pull request that bot opened. Each bot is a row of `declaredBots` in `bots.go`, with the author identity its commits carry, the login that opens its pull requests and the reason, and the exemption needs the two to agree, so a commit carrying the bot's identity in a pull request somebody else opened is judged like any other. Dependabot is the one row. A workflow that starts opening pull requests as another app fails the check until that app is declared, with the test that pins the declarations (`TestDeclaredBots_Declarations_StateAnIdentityALoginAndAReason`). The release workflow's version stamp is not a row, since it is pushed to `main` and never sits in a pull request. + +The pull request description, which becomes the squash commit's message, is not judged: GitHub copies the `Signed-off-by` trailers of the pull request's commits into the squash commit, below the description and once each, as the squash commits of pull requests 1512 and 1529 and of Dependabot's pull requests show. + +#### Usage + +```bash +go run ./cmd/audit_dco/ +go run ./cmd/audit_dco/ -base origin/main -head HEAD +go run ./cmd/audit_dco/ -base -head -opened-by 'dependabot[bot]' -annotate +``` + +#### Flags + +| Flag | Type | Default | Description | +| ------------ | -------- | ------------- | -------------------------------------------------------------------------------------------------------------------------------------- | +| `-base` | `string` | `origin/main` | The commit the branch grew from; the commits reachable from it are not judged. An empty revision, or one starting with `-`, is refused | +| `-head` | `string` | `HEAD` | The commit at the tip of the branch. An empty revision, or one starting with `-`, is refused | +| `-opened-by` | `string` | | The GitHub login that opened the pull request, which a declared bot's exemption needs | +| `-dir` | `string` | `.` | The repository to read | +| `-annotate` | `bool` | `false` | Print a GitHub Actions error command for every commit that fails | + +#### Output + +`audit_dco: N commit(s) in .. (..)`, then one line per commit, oldest first, opening with `ok`, `FAIL`, `exempt` or `merge`, its abbreviated hash, its author and its subject, with the reason indented under every line but `ok`: no trailer, the sign-offs it carries instead of its author's, the bot that exempts it, or, for a failing commit carrying a bot's identity, why the exemption did not hold. Then the count, `N signed off by the author, N not, N exempt, N merge(s) not judged`, and for a failing run how to sign the commits off where they stand (`git rebase --keep-base --signoff ` and a push with `--force-with-lease`, `git commit --amend --no-edit --signoff` for a failing merge, which a rebase does not keep, and `--update-refs` for a stack), followed under `-annotate` by an `::error` command per failing commit. An empty range prints `no commits in , so nothing to judge`. Exit `0` when every judged commit is signed off by its author or there is none, `1` when one is not (on stdout) or git could not answer (on stderr, naming `fetch-depth: 0` for a base a shallow clone lacks, and git's own reason for a range or a merge it could not read), and `2` for arguments it cannot use. + +#### Make targets + +- `make check-dco`: judges `DCO_BASE..DCO_HEAD`, `origin/main..HEAD` by default, with `DCO_OPENED_BY` as `-opened-by` and `DCO_FLAGS` passed on. It reads git and nothing else, so it runs before a push; `make analyze` does not run it, since its answer depends on the branch rather than the tree. The `dco` job of `ci.yml` runs it on every pull request with the event's base and head commits, the login that opened it and `DCO_FLAGS=-annotate`, and skips itself on a push to `main`, which the `checks` job counts as a pass. + ## Generators ### gen_action_catalog_manifest @@ -2865,6 +2902,7 @@ The following utilities expose a verification mode (`--check` or `-check`, or an | `check-binary-vulns` | `audit_binary_vulns` | No advisory the declaration table does not accept affects a module any release binary links, and every declaration still matches a finding | Non-zero on an undeclared finding, a stale or malformed declaration, or a build or scan that could not be made | | `check-image-vulns` | `audit_image_vulns` | No fixable HIGH or CRITICAL advisory the declaration table does not accept affects an Alpine package of the image, and every declaration matches and is current (needs Docker, so not run in CI) | Non-zero on an undeclared finding, a stale, expired or malformed declaration, or a report or database it cannot vouch for | | `check-site-headers` | `audit_site_headers` | The documentation site still sends every security header declared required, with a value that is not permissive (needs the network, so not run in CI; `pages.yml` runs it after each deploy) | Non-zero on a required header missing or permissive, or a page that could not be judged; a pending header only warns | +| `check-dco` | `audit_dco` | Every commit of the branch, or in CI of the pull request, carries a `Signed-off-by` trailer naming its author (the `dco` job of `ci.yml`, on pull requests only) | Non-zero on a commit without its author's sign-off, or a range git could not read | | `check-test-goroutines` | `audit_test_goroutines` | No `testing.T` abort is made off the test goroutine | Non-zero if any abort site exists | | `check-test-subtests` | `audit_test_subtests` | No case loop asserts without a `t.Run` subtest | Non-zero if any site remains | | `check-closed-servers` | `audit_test_closed_servers -check` | No `httptest` server is closed while its address is still used later in the function, and every entry of its declaration table excuses one | Non-zero on a finding, a stale or unknown declaration, or a hidden file that imports `net/http/httptest` | diff --git a/docs/development/repository-map.md b/docs/development/repository-map.md index c27c2acbb..24ea8c785 100644 --- a/docs/development/repository-map.md +++ b/docs/development/repository-map.md @@ -28,6 +28,7 @@ gitlab-mcp-server/ │ ├── audit_binary_vulns/ # Builds every target `.goreleaser.yml` names (each `goos` crossed with each `goarch`, since the operating system decides the module set) and holds each binary to the Go vulnerability database at module grain, with govulncheck's scanner run in process at the version `go.mod` pins. That is the question every scanner reading a shipped binary or its SBOM asks, and the source scan (`make govulncheck`) answered a different one, which passed on 3.0.0 and 3.1.0 while each binary carried `golang.org/x/crypto` and was reported for GO-2026-5932. A finding fails unless `declarations.go` accepts it by advisory and module with a category and a reason, and a declaration no finding matches fails too; the table is empty (make check-binary-vulns, step 25 of `make analyze`, a step of CI's govulncheck job, and the release workflow's `binary-vulns` job, which stands in front of `docker` so it never runs after an image push) │ ├── audit_image_vulns/ # Holds the image's Alpine packages to Trivy's vulnerability database, from one Trivy report per platform of the two smoke images the release's docker job scans between the smoke test and the push: a fixable HIGH or CRITICAL advisory fails unless `declarations.go` accepts it under `fix-not-yet-adoptable` (a base image package, with the day the alpine bump can land) or `not-affected`, a declaration past its day, or one that accepts nothing in a run of both platforms, fails too, and a report or database it cannot vouch for is refused; the binary's findings are printed and left to check-binary-vulns. Standard library only, since the job holds the push (make check-image-vulns, needs Docker; the weekly `image-vulns.yml` scans `latest`) │ ├── audit_site_headers/ # Fetches the documentation site after a deploy and judges the four security headers the OpenSSF Best Practices criterion hardened_site names, read from the live responses since Cloudflare sets them all: losing one the site serves fails, and one still waiting for the maintainer's Cloudflare rule is reported with the value to set (make check-site-headers, the headers job of pages.yml) +│ ├── audit_dco/ # Holds every commit a pull request brings, between the base and the head its event names, to the Developer Certificate of Origin: each needs a Signed-off-by trailer naming its author, read as git reads trailers; a merge of two parents that brings no change of its own (git show --remerge-diff prints nothing for it) is not judged, every other merge is, and a declared bot's commits are exempt in a pull request that bot opened (make check-dco, the dco job of ci.yml) │ ├── reproduce_release/ # Rebuilds every binary a release published from a clone of its commit, with the configuration at that commit and the toolchain each published binary records, and compares each with the published one byte for byte, the universal darwin binary joined in the order the published one lists its slices; -twice builds every target of a tree twice, the second time with an empty build cache, offline and without git (make check-reproducible TAG=vX.Y.Z, the reproduce job of release.yml; make check-repeatable) │ ├── godoc_tool/ # Consolidated Go doc auditor + fixer (was audit_godocs + add_docs); `make check-godocs` gates package comments and exported symbols, and test functions stay a report (make audit-godocs-check) │ ├── audit_md_escaping/ # Fails when a Markdown formatter interpolates a GitLab-authored value into a table cell, heading, list item or link without EscapeMdTableCell/EscapeMdHeading/MdTitleLink, and when one lands inside a code fence the formatter wrote by hand rather than through MarkdownFencedBlock/MarkdownCodeFence; the fence context is decided by the writes before the hole rather than by its line, since a block opens on one line and closes on another; `Card.Markdown` and `CardTable.Row` are judged at the call site like `MarkdownTableRow`; `//gitlab:allow-unescaped : ` declares a value that needs none; the gate holds `internal/toolutil` to no unresolved value (make check-md-escaping). Two staged rules report without gating (`-contexts all,card,bool-time`, `make audit-md-escaping`): `card` lists every one-object card row written by hand instead of through `toolutil.Card`, and `bool-time` every flag or timestamp printed without BoolEmoji/FormatTime, excused by `//gitlab:allow-raw : ` diff --git a/docs/development/repository-settings.md b/docs/development/repository-settings.md index 8e47ced3b..abf2b8306 100644 --- a/docs/development/repository-settings.md +++ b/docs/development/repository-settings.md @@ -50,7 +50,10 @@ depends on, and it is recorded here because describes it and no file in the tree can show it. The documentation site's response headers are a Cloudflare setting, and the release does not depend on them either; they are here because the OpenSSF Best Practices answer for the -project's website rests on them and only the maintainer can set them. +project's website rests on them and only the maintainer can set them. The web +commit sign-off is not a release setting, and it is here because the sign-off +check every pull request runs depends on it for the commits GitHub makes in its +web interface. ## SC-04: enable immutable releases @@ -540,6 +543,43 @@ lost or turns permissive, and warns with the value to set while one is pending. A change in Cloudflare between deploys shows at the next deploy, or at once with `make check-site-headers`. +## Web commit sign-off + +**What to set.** Repository → Settings → General, **Require contributors to +sign off on web-based commits**, which the API calls +`web_commit_signoff_required`: + +```bash +gh api -X PATCH repos/jmrplens/gitlab-mcp-server -F web_commit_signoff_required=true +``` + +**Why.** Every commit a pull request brings has to carry a `Signed-off-by` +trailer naming its author, the sign-off of the +[Developer Certificate of Origin](../../CONTRIBUTING.md#developer-certificate-of-origin), +and the `✍️ DCO sign-off` job of `ci.yml` fails a pull request with a commit +that does not ([`audit_dco`](cmd-utilities.md#audit_dco)). A commit made in +GitHub's web interface, an applied review suggestion or a file edited in the +browser, cannot be made with `git commit --signoff`. With this setting on, +GitHub signs such a commit off with its author's identity, as GitHub's +documentation of the setting says, so it passes the job like one made with +`git commit -s`; without it, the trailer has to be typed into the commit +message by hand. The setting reaches only commits made in the web interface: +a commit made with git still needs `--signoff`. + +**Current state.** Not applied. Read on 2026-10-26, the repository answers +`web_commit_signoff_required: false`. + +**How to check.** + +```bash +gh api repos/jmrplens/gitlab-mcp-server --jq '{web_commit_signoff_required}' +``` + +It answers `true` once the setting is on. + +**What tells you it drifted.** The `✍️ DCO sign-off` job fails the first pull +request that carries a web commit without the trailer. + ## Checking all of it at once ```bash @@ -548,6 +588,7 @@ gh api repos/jmrplens/gitlab-mcp-server/rulesets --jq '.[] | "ruleset=\(.name) t gh api repos/jmrplens/gitlab-mcp-server/rulesets/16049728 --jq '.bypass_actors[] | "bypass=\(.actor_type)"' gh api repos/jmrplens/gitlab-mcp-server/rulesets/16049728 --jq '.rules[] | "main_rule=\(.type) approvals=\(.parameters.required_approving_review_count) threads=\(.parameters.required_review_thread_resolution) checks=\([.parameters.required_status_checks[]?.context] | join(","))"' gh api repos/jmrplens/gitlab-mcp-server --jq '"squash=\(.allow_squash_merge) merge_commit=\(.allow_merge_commit) rebase=\(.allow_rebase_merge)"' +gh api repos/jmrplens/gitlab-mcp-server --jq '"web_commit_signoff=\(.web_commit_signoff_required)"' gh api repos/jmrplens/gitlab-mcp-server/environments/release --jq '"release_policy=\(.deployment_branch_policy) admins_bypass=\(.can_admins_bypass)"' gh api repos/jmrplens/gitlab-mcp-server/environments/release/deployment-branch-policies --jq '.branch_policies[] | "release_ref=\(.type):\(.name)"' gh api repos/jmrplens/gitlab-mcp-server/environments/release-rehearsal/secrets --jq '"rehearsal_secrets=\(.total_count)"' diff --git a/site/src/content/docs/about.mdx b/site/src/content/docs/about.mdx index 9cef4b56b..99478ed4a 100644 --- a/site/src/content/docs/about.mdx +++ b/site/src/content/docs/about.mdx @@ -73,6 +73,8 @@ Bug reports and feature requests belong in [GitHub Issues](https://github.com/jm Contributions are welcome as pull requests on GitHub. [CONTRIBUTING.md](https://github.com/jmrplens/gitlab-mcp-server/blob/main/CONTRIBUTING.md) covers the development setup, branch naming, commit conventions and the review process, and the [Code of Conduct](https://github.com/jmrplens/gitlab-mcp-server/blob/main/CODE_OF_CONDUCT.md) ([Contributor Covenant 2.1](https://www.contributor-covenant.org/version/2/1/code_of_conduct/)) applies to every space the project uses. GitHub is the canonical repository; the read-only [mirror on GitLab.com](https://gitlab.com/jmrp/gitlab-mcp-server) exists for discoverability and takes no contributions. +Every commit in a pull request is signed off by its author with a `Signed-off-by` line (`git commit -s`), which certifies the [Developer Certificate of Origin](https://developercertificate.org/): that the author has the right to contribute the change under the project's MIT license. A check in CI refuses a pull request with a commit that is not signed off, and [CONTRIBUTING.md](https://github.com/jmrplens/gitlab-mcp-server/blob/main/CONTRIBUTING.md#developer-certificate-of-origin) says how to sign off commits already made. + To build the server from source, see [Build from source](/gitlab-mcp-server/install/binary/#build-from-source). diff --git a/site/src/content/docs/es/about.mdx b/site/src/content/docs/es/about.mdx index 84459acc9..c4e56717d 100644 --- a/site/src/content/docs/es/about.mdx +++ b/site/src/content/docs/es/about.mdx @@ -73,6 +73,8 @@ Los errores y las peticiones de funcionalidad van a [GitHub Issues](https://gith Las contribuciones son bienvenidas como pull requests en GitHub. [CONTRIBUTING.md](https://github.com/jmrplens/gitlab-mcp-server/blob/main/CONTRIBUTING.md) explica el entorno de desarrollo, el nombre de las ramas, las convenciones de commit y el proceso de revisión, y el [código de conducta](https://github.com/jmrplens/gitlab-mcp-server/blob/main/CODE_OF_CONDUCT.md) ([Contributor Covenant 2.1](https://www.contributor-covenant.org/version/2/1/code_of_conduct/)) rige en todos los espacios del proyecto. GitHub es el repositorio canónico; el [espejo de solo lectura en GitLab.com](https://gitlab.com/jmrp/gitlab-mcp-server) existe para que se encuentre y no admite contribuciones. +Cada commit de un pull request lleva una línea `Signed-off-by` con el nombre de su autor (`git commit -s`), con la que este certifica el [Developer Certificate of Origin](https://developercertificate.org/): que tiene derecho a aportar el cambio bajo la licencia MIT del proyecto. Una comprobación de CI rechaza el pull request que traiga un commit sin esa línea, y [CONTRIBUTING.md](https://github.com/jmrplens/gitlab-mcp-server/blob/main/CONTRIBUTING.md#developer-certificate-of-origin) explica cómo añadirla a los commits ya hechos. + Para compilar el servidor desde el código fuente, consulta [Compilar desde el código fuente](/gitlab-mcp-server/es/install/binary/#compilar-desde-el-código-fuente).