From 16c646b11a35d02454910fa1a6b528b7d157e0b6 Mon Sep 17 00:00:00 2001 From: HackingGate Date: Fri, 16 Oct 2026 21:56:15 +0900 Subject: [PATCH 1/4] Complete remote inheritance, design ruling checks, and PyPI normalization --- .github/workflows/design-age.yml | 26 ++ AGENTS.md | 1 + CONTRIBUTING.md | 12 +- Cargo.lock | 234 ++++++++++++- Cargo.toml | 5 + README.md | 5 + REVIEW.md | 1 + deny.toml | 3 + docs/REFERENCE.md | 71 +++- policy/principles.toml | 6 + policy/upheld.toml | 4 + scripts/design_age.py | 162 +++++++++ src/config.rs | 146 ++++---- src/main.rs | 10 + src/remote_policy.rs | 196 +++++++++++ src/supply.rs | 147 +++++++- src/supply/pypi.rs | 293 ++++++++++++++++ .../guarddog/pypi-pandas-3.0.3-metadata.json | 92 +++++ tests/remote_policy_cli.rs | 329 ++++++++++++++++++ tests/scan_cli.rs | 47 +++ tests/test_design_age.py | 183 ++++++++++ 21 files changed, 1883 insertions(+), 90 deletions(-) create mode 100644 .github/workflows/design-age.yml create mode 100644 scripts/design_age.py create mode 100644 src/remote_policy.rs create mode 100644 src/supply/pypi.rs create mode 100644 tests/fixtures/guarddog/pypi-pandas-3.0.3-metadata.json create mode 100644 tests/remote_policy_cli.rs create mode 100644 tests/test_design_age.py diff --git a/.github/workflows/design-age.yml b/.github/workflows/design-age.yml new file mode 100644 index 0000000..e6a98c4 --- /dev/null +++ b/.github/workflows/design-age.yml @@ -0,0 +1,26 @@ +name: Design record age + +on: + issues: + types: [opened, reopened, labeled, unlabeled] + schedule: + - cron: '17 9 * * *' + workflow_dispatch: + +permissions: + contents: read + issues: read + +jobs: + ruling-window: + name: Open Design records have a ruling + runs-on: ubuntu-slim + steps: + - uses: actions/checkout@v7 + with: + persist-credentials: false + + - name: Check ruling labels and pending age + env: + GITHUB_TOKEN: ${{ github.token }} + run: python3 scripts/design_age.py diff --git a/AGENTS.md b/AGENTS.md index f46f10f..07f4f32 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -8,6 +8,7 @@ Do NOT re-enforce the static rules. They run on every change and they run first; repeating their findings costs a reviewer's attention and buys a second opinion nobody asked for. The rules already active here are: +- `adr-retired` - `catalog-reference-current` - `catalog-tests` - `catalog-validate` diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5742096..3d2d2b8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -83,6 +83,14 @@ one ruling label. The template files it with `ruling:pending`, and the ruling is recorded by replacing that label with `ruling:accepted`, `ruling:rejected` or `ruling:deferred`. +The read-only Design record workflow checks open records when issues open, +reopen or change labels, and daily. Each open record must have exactly one +recognized ruling label; `ruling:pending` is overdue at 30 days from creation. +Run `python3 scripts/design_age.py` with `GITHUB_REPOSITORY` and `GH_TOKEN` set +to check manually. Rule the record or mark it `ruling:deferred` and record a +revisit date in the issue. API failures fail the check rather than reporting +that records are ruled. + The repository receives only what runs, plus [REFERENCE.md](docs/REFERENCE.md), which stays the normative statement of what uphold does now. Tracked files carry no issue numbers, dates or status lines; @@ -90,7 +98,9 @@ those stay in the Design record that rules the change. Decisions live in Design record issues. [DESIGN.md](docs/DESIGN.md) explains the current architecture; [REFERENCE.md](docs/REFERENCE.md) defines its -behavior. Retired decision files remain available in Git history. +behavior. Retired decision files remain available in Git history. The `adr-retired` +path rule refuses every tracked file under `docs/adr`; file a Design record +for a new decision. ## Working on the engine diff --git a/Cargo.lock b/Cargo.lock index 4cb23c0..6c81f48 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -63,6 +63,12 @@ dependencies = [ "serde_core", ] +[[package]] +name = "bytes" +version = "1.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fc652a48c352aef3ea3aed32080501cf3ef6ed5da78602a020c991775b0aff04" + [[package]] name = "cc" version = "1.4.4" @@ -146,7 +152,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys", + "windows-sys 0.61.2", ] [[package]] @@ -232,6 +238,22 @@ version = "0.17.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ed5909b6e89a2db4456e54cd5f673791d7eca6732202bbf2a9cc504fe2f9b84a" +[[package]] +name = "http" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "918d3568bebf352712bc2ef3d46a8bcf1a75b373be6539de198e9105cbbf9ce0" +dependencies = [ + "bytes", + "itoa", +] + +[[package]] +name = "httparse" +version = "1.10.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6dbf3de79e51f3d586ab4cb9d5c3e2c14aa28ed23d180cf89b4df0454a69cc87" + [[package]] name = "ignore" version = "0.4.33" @@ -318,6 +340,18 @@ dependencies = [ "autocfg", ] +[[package]] +name = "once_cell" +version = "1.21.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9f7c3e4beb33f85d45ae3e3a1792185706c8e16d043238c593331cc7cd313b50" + +[[package]] +name = "percent-encoding" +version = "2.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9b4f627cb1b25917193a259e49bdad08f671f8d9708acfd5fe0a8c1455d87220" + [[package]] name = "phf" version = "0.11.3" @@ -374,6 +408,15 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "pyreq" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c6eb3ecbf06d7c20d3e767683ce199979ed7bfdb5ae421f0b6dcb9f391d3462f" +dependencies = [ + "regex", +] + [[package]] name = "quote" version = "1.0.47" @@ -442,6 +485,20 @@ version = "0.8.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d6f6ff9a378485b298a5286656da665ba74413d36db0979633275d2e708145d4" +[[package]] +name = "ring" +version = "0.17.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a4689e6c2294d81e88dc6261c768b63bc4fcdb852be6d1352498b114f61383b7" +dependencies = [ + "cc", + "cfg-if", + "getrandom", + "libc", + "untrusted", + "windows-sys 0.52.0", +] + [[package]] name = "rustix" version = "1.1.5" @@ -452,7 +509,42 @@ dependencies = [ "errno", "libc", "linux-raw-sys", - "windows-sys", + "windows-sys 0.61.2", +] + +[[package]] +name = "rustls" +version = "0.23.46" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "48e13bd8c0e9365c43cfa5c9e8f9ad49d3c8444926c9aac819e0e4dc503c8fdf" +dependencies = [ + "log", + "once_cell", + "ring", + "rustls-pki-types", + "rustls-webpki", + "subtle", + "zeroize", +] + +[[package]] +name = "rustls-pki-types" +version = "1.15.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2f4925028c7eb5d1fcdaf196971378ed9d2c1c4efc7dc5d011256f76c99c0a96" +dependencies = [ + "zeroize", +] + +[[package]] +name = "rustls-webpki" +version = "0.103.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f3c3cf1d8b1e7d4927e2d154c3fcb02979afb9939629c62cd9048d4f07b60ac2" +dependencies = [ + "ring", + "rustls-pki-types", + "untrusted", ] [[package]] @@ -582,6 +674,12 @@ version = "0.1.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2b2231b7c3057d5e4ad0156fb3dc807d900806020c5ffa3ee6ff2c8c76fb8520" +[[package]] +name = "subtle" +version = "2.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "13c2bddecc57b384dee18652358fb23172facb8a2c51ccc10d74c157bdea3292" + [[package]] name = "syn" version = "2.0.119" @@ -755,6 +853,12 @@ dependencies = [ "rand", ] +[[package]] +name = "untrusted" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8ecb6da28b8a351d773b68d5825ac39017e680750f980f3a1a85cd8dd28a47c1" + [[package]] name = "uphold" version = "1.31.0" @@ -765,6 +869,7 @@ dependencies = [ "grep-regex", "grep-searcher", "ignore", + "pyreq", "regex", "rustix", "serde", @@ -779,8 +884,43 @@ dependencies = [ "unicode-script", "unicode-security", "unicode_names2", + "ureq", ] +[[package]] +name = "ureq" +version = "3.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9a7ac20be9b7726e0bbdbf974c059676d9acb1cd414961f570a4e8231cacd7fc" +dependencies = [ + "base64", + "log", + "percent-encoding", + "rustls", + "rustls-pki-types", + "ureq-proto", + "utf8-zero", + "webpki-roots", +] + +[[package]] +name = "ureq-proto" +version = "0.6.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f86fd172ccca569e458f61b6bdd6220965a9ef36e672a6852953b51a0e1583be" +dependencies = [ + "base64", + "http", + "httparse", + "log", +] + +[[package]] +name = "utf8-zero" +version = "0.8.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b8c0a043c9540bae7c578c88f91dda8bd82e59ae27c21baca69c8b191aaf5a6e" + [[package]] name = "walkdir" version = "2.5.0" @@ -797,13 +937,22 @@ version = "0.11.1+wasi-snapshot-preview1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ccf3ec651a847eb01de73ccad15eb7d99f80485de043efb2f370cd654f4ea44b" +[[package]] +name = "webpki-roots" +version = "1.0.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7dcd9d09a39985f5344844e66b0c530a33843579125f23e21e9f0f220850f22a" +dependencies = [ + "rustls-pki-types", +] + [[package]] name = "winapi-util" version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys", + "windows-sys 0.61.2", ] [[package]] @@ -812,6 +961,15 @@ version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e70d0a4fe5" +[[package]] +name = "windows-sys" +version = "0.52.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "282be5f36a8ce781fad8c8ae18fa3f9beff57ec1b52cb3de0789201425d9a33d" +dependencies = [ + "windows-targets", +] + [[package]] name = "windows-sys" version = "0.61.2" @@ -821,6 +979,70 @@ dependencies = [ "windows-link", ] +[[package]] +name = "windows-targets" +version = "0.52.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9b724f72796e036ab90c1021d4780d4d3d648aca59e491e6b98e725b84e99973" +dependencies = [ + "windows_aarch64_gnullvm", + "windows_aarch64_msvc", + "windows_i686_gnu", + "windows_i686_gnullvm", + "windows_i686_msvc", + "windows_x86_64_gnu", + "windows_x86_64_gnullvm", + "windows_x86_64_msvc", +] + +[[package]] +name = "windows_aarch64_gnullvm" +version = "0.52.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32a4622180e7a0ec044bb555404c800bc9fd9ec262ec147edd5989ccd0c02cd3" + +[[package]] +name = "windows_aarch64_msvc" +version = "0.52.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "09ec2a7bb152e2252b53fa7803150007879548bc709c039df7627cabbd05d469" + +[[package]] +name = "windows_i686_gnu" +version = "0.52.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8e9b5ad5ab802e97eb8e295ac6720e509ee4c243f69d781394014ebfe8bbfa0b" + +[[package]] +name = "windows_i686_gnullvm" +version = "0.52.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0eee52d38c090b3caa76c563b86c3a4bd71ef1a819287c19d586d7334ae8ed66" + +[[package]] +name = "windows_i686_msvc" +version = "0.52.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "240948bc05c5e7c6dabba28bf89d89ffce3e303022809e73deaefe4f6ec56c66" + +[[package]] +name = "windows_x86_64_gnu" +version = "0.52.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "147a5c80aabfbf0c7d901cb5895d1de30ef2907eb21fbbab29ca94c5b08b1a78" + +[[package]] +name = "windows_x86_64_gnullvm" +version = "0.52.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "24d5b23dc417412679681396f2b49f3de8c1473deb516bd34410872eff51ed0d" + +[[package]] +name = "windows_x86_64_msvc" +version = "0.52.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "589f6da84c646204747d1270a2a5661ea66ed1cced2631d546fdfb155959f9ec" + [[package]] name = "winnow" version = "1.0.4" @@ -847,6 +1069,12 @@ dependencies = [ "syn 2.0.119", ] +[[package]] +name = "zeroize" +version = "1.9.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e13084392c5e4bc371903e2935a5eaeed24905a7511356b883835e18a78f6879" + [[package]] name = "zmij" version = "1.0.23" diff --git a/Cargo.toml b/Cargo.toml index 1a72ba6..115caf7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -112,6 +112,11 @@ tree-sitter-go = "0.25" # positional word after them. Where this reader and git's `split_cmdline` # disagree, `expand_alias` says which case and how it is closed. shell-words = "1" +# The exact-release PyPI metadata used to verify optional extras, with bounded +# HTTPS requests and no asynchronous runtime. +ureq = { version = "3.4.2", default-features = false, features = ["rustls"] } +# Parse PEP 508 requirements before accepting the closed optional-extra form. +pyreq = "0.2.0" [target.'cfg(unix)'.dependencies] # Git discovers hooks with access(X_OK), which checks the caller's permission diff --git a/README.md b/README.md index 8673212..6782052 100644 --- a/README.md +++ b/README.md @@ -62,6 +62,11 @@ sets supply, and the `.pre-commit-config.yaml` below at this binary's version has a policy, and leaves an existing hook config as it is, printing the block to add. The owner and the visibility are stated, never read from `origin`. +Share organization rules without copying them: `[inherit] remote` accepts a +GitHub `owner/name`, a full commit ID in `rev`, and a policy-file `path`. +Remote rules support the same overrides as bundled rules and show their +provenance in `uphold rules --effective`. See [remote inheritance](docs/REFERENCE.md). + **pre-commit / prek** — one manifest serves both, and no Rust toolchain is needed (`language: rust` bootstraps it). diff --git a/REVIEW.md b/REVIEW.md index f46f10f..07f4f32 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -8,6 +8,7 @@ Do NOT re-enforce the static rules. They run on every change and they run first; repeating their findings costs a reviewer's attention and buys a second opinion nobody asked for. The rules already active here are: +- `adr-retired` - `catalog-reference-current` - `catalog-tests` - `catalog-validate` diff --git a/deny.toml b/deny.toml index a49c4f2..135dddf 100644 --- a/deny.toml +++ b/deny.toml @@ -41,6 +41,9 @@ allow = [ "Apache-2.0 WITH LLVM-exception", "BSD-2-Clause", "BSD-3-Clause", + # TLS implementation and the Mozilla root certificate data carried by ureq. + "ISC", + "CDLA-Permissive-2.0", "MIT", "Unicode-3.0", "Unicode-DFS-2016", diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index 124d8a3..223b20e 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -839,6 +839,37 @@ and is a one-line diff that says so. `[set]` in a repository's own policy, or in an `inherit.paths` file, is refused — nothing there ships compiled in, so nothing there has the problem the ceiling exists for. +Organization rules may instead come from a GitHub repository at an immutable +commit. They are merged after `inherit.paths`, before the repository's own +rules, and use the same `[override.]`, shadowing and `disabled_rules` behavior: + +```toml +[inherit] +remote = [{ repo = "example/policy", rev = "0123456789abcdef0123456789abcdef01234567", path = "policy/org.toml" }] + +[override.org-rule.files] +exclude = ["fixtures/**"] +``` + +`repo` is `owner/name` on GitHub. `rev` must be a full 40-character commit ID; +a branch, abbreviated ID or tag is refused with exit 2 because it can move. +Resolve a release tag to its commit before recording the pin. Missing commits, +unreachable repositories, missing files, symlinks, directories, non-UTF-8 +content and invalid policy all fail with exit 2. Nothing falls back to the +repository's latest revision. Each load fetches into a fresh temporary bare +repository and removes it afterwards; network access and Git credentials for +private sources must therefore be available wherever the policy runs. Git's +normal credential helpers and URL rewrites apply. No checkout, submodules or +source repository hooks are run. + +Only rules are adopted. Remote files may not declare their own `[inherit]`, +`[override]` or `[[shim]]`: sources and shims remain explicit decisions in the +consumer's own policy. This deliberately leaves org-wide shim inheritance +unsupported, separately from the bundled-set constraint. `rules --effective` +shows each remote rule's repository, commit and path; `--json` includes them in +its `remote` object, preserved when an override adjusts the rule. An own rule +that replaces the inherited id has its own provenance instead. + `inherit.disabled_rules` drops an inherited rule, and naming an id nothing inherited defines is an error. `inherit.paths` merges extra policy files, repository-relative, after the bundled sets. A path must stay inside the @@ -3253,7 +3284,7 @@ itself calls clean. It trades a false negative for a false positive. So guarddog is run with `--output-format json` and its own `risks` list is counted. This is the one scanner whose findings are read here. The count is reported, never recomputed, with one named exception: a `metadata_mismatch` -risk made up only of npm's publish-time normalizations is printed as +risk made up only of known metadata normalizations is printed as tolerated rather than counted ([below](#what-npm-writes-at-publish-is-tolerated-by-name)). @@ -3397,19 +3428,41 @@ these is true: `: Manifest("` or the `"), package.json("` marker appears more than once is not read. -Two cases are not on the list: - -- `"install": "node-gyp rebuild"`, which npm adds where `binding.gyp` exists. An - install script runs at install time, so it is not the same risk as `start`. -- PyPI optional extras, which guarddog 3.2.0 compares against the required - dependencies. Its report keeps the names and drops the `extra ==` markers, - so the report alone cannot tell an extra from an injected dependency. Those - findings still need a waiver. +`"install": "node-gyp rebuild"`, which npm adds where `binding.gyp` exists, +is not on the list. An install script runs at install time, so it is not the +same risk as `start`. guarddog's report does not list the tarball's files, so the `server.js` that npm's step requires is not confirmed. guarddog also prints an absent value as `None`, the same way it prints a literal `"None"`. +### PyPI optional extras require release evidence + +guarddog compares every `requires_dist` name with the required dependencies +in `pyproject.toml`, including names declared only under optional extras. Its +report drops the markers, so uphold fetches the exact scanned release from +[PyPI's release JSON endpoint](https://docs.pypi.org/api/json/) only when the +entire difference consists of names present in PyPI metadata and absent from +`pyproject.toml`. It verifies the response's package and version, then parses +all `requires_dist` entries with `pyreq`. + +A name is tolerated only when every occurrence has a simple positive +`extra == "name"` marker (including the reversed equality). An occurrence +with no marker, a platform marker, a conjunction or an alternative keeps the +finding. This closed form covers the ordinary extras declarations without +evaluating conditions against the host. All reported names must qualify. Both copies of guarddog's +description must agree; malformed reports, unknown differences, missing +metadata or unparseable requirements retain the finding and remain eligible +for an exact-version waiver. + +The lookup uses HTTPS at `pypi.org`, refuses redirects, and is bounded to +15 seconds and 2 MiB per response. A failed lookup prints why the finding was +retained. A successful normalization prints `tolerated:` with the verified +names and release. No package-wide exemption or cross-run metadata cache is +created. This verifies registry metadata, not the artifact contents; PyPI +metadata can differ from the uploaded artifact. This does not repair guarddog's order-dependent choice of a wheel +root, so an unmatched PyPI metadata waiver is still not called removable. + ### Waiving a confirmed guarddog false positive Because the findings are read here, a finding somebody has looked at and judged diff --git a/policy/principles.toml b/policy/principles.toml index 846ba93..27c82fc 100644 --- a/policy/principles.toml +++ b/policy/principles.toml @@ -130,6 +130,12 @@ sets = [ allowed_scripts = ["Latin", "Han", "Hiragana", "Katakana"] files.include = ["tests", "src/guard/unicode.rs"] +[rule.adr-retired] +message = "file a Design record issue (label design); docs/adr is retired." +path_regexp = '^docs/adr/' +files.include = ["."] +files.glob = ["docs/adr/**"] + # `workflow-declares-permissions` used to stand here, as the one instance of the # `require_regexp` kind anywhere -- a rule kind with no instance is # indistinguishable from a rule kind that does not work. It is in diff --git a/policy/upheld.toml b/policy/upheld.toml index 493cb29..0a03042 100644 --- a/policy/upheld.toml +++ b/policy/upheld.toml @@ -12,6 +12,10 @@ # listed here. They live in prose, where a reader can weigh them, rather than in # a config file that would have no condition on which to raise them. +[[enforce]] +principle = "single-authoritative-source" +rule = "adr-retired" + [[enforce]] principle = "explicit-unknown" rule = "catalog-tests" diff --git a/scripts/design_age.py b/scripts/design_age.py new file mode 100644 index 0000000..3a1e3a5 --- /dev/null +++ b/scripts/design_age.py @@ -0,0 +1,162 @@ +#!/usr/bin/env python3 +"""Report open Design records that have stayed pending past the ruling window.""" + +from __future__ import annotations + +import json +import os +import re +import sys +from datetime import UTC, datetime, timedelta +from urllib.error import HTTPError, URLError +from urllib.parse import urlencode +from urllib.request import Request, urlopen + +RULINGS = {"ruling:pending", "ruling:accepted", "ruling:rejected", "ruling:deferred"} +DESIGN_LABEL = "design" +RULING_WINDOW_DAYS = 30 +PAGE_SIZE = 100 + + +def _label_names(issue: dict) -> list[str]: + labels = issue["labels"] + if not isinstance(labels, list): + raise ValueError("GitHub issue labels must be a list") + names = [] + for label in labels: + if isinstance(label, str): + names.append(label) + elif isinstance(label, dict) and isinstance(label.get("name"), str): + names.append(label["name"]) + else: + raise ValueError("GitHub issue contains a malformed label") + return names + + +def _created_at(issue: dict) -> datetime: + created = issue["created_at"] + if not isinstance(created, str): + raise ValueError("GitHub issue created_at must be a timestamp") + parsed = datetime.fromisoformat(created.replace("Z", "+00:00")) + if parsed.tzinfo is None or parsed.utcoffset() is None: + raise ValueError("GitHub issue created_at must include a timezone") + return parsed + + +def audit( + issues: list[dict], now: datetime, window_days: int = RULING_WINDOW_DAYS +) -> tuple[list[tuple[dict, int]], list[str]]: + """Return overdue pending records and malformed design-record labels.""" + overdue: list[tuple[dict, int]] = [] + errors: list[str] = [] + + for issue in issues: + if not isinstance(issue, dict): + raise ValueError("GitHub issues API returned a malformed issue") + labels = _label_names(issue) + if DESIGN_LABEL not in labels or issue.get("state", "open") != "open": + continue + # The issues endpoint also returns pull requests. They are not design + # records even if somebody copies the label onto one. + if "pull_request" in issue: + continue + rulings = [label for label in labels if label.startswith("ruling:")] + number = issue.get("number", "?") + title = issue.get("title", "(untitled)") + if len(rulings) != 1 or rulings[0] not in RULINGS: + errors.append( + f"#{number} {title}: expected exactly one of " + f"{', '.join(sorted(RULINGS))}; found " + f"{', '.join(rulings) if rulings else 'no ruling label'}." + ) + continue + if rulings[0] != "ruling:pending": + continue + age = now - _created_at(issue) + age_days = age.days + if age >= timedelta(days=window_days): + overdue.append((issue, age_days)) + + overdue.sort(key=lambda item: (item[0].get("number", 0), item[0].get("title", ""))) + return overdue, errors + + +def fetch_open_design_issues(repository: str, token: str) -> list[dict]: + if not re.fullmatch(r"[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+", repository) or any( + part in {".", ".."} for part in repository.split("/") + ): + raise ValueError("GITHUB_REPOSITORY must have the form owner/repository") + issues: list[dict] = [] + page = 1 + while True: + query = urlencode( + { + "state": "open", + "labels": DESIGN_LABEL, + "per_page": PAGE_SIZE, + "page": page, + } + ) + request = Request( + f"https://api.github.com/repos/{repository}/issues?{query}", + headers={ + "Accept": "application/vnd.github+json", + "Authorization": f"Bearer {token}", + "X-GitHub-Api-Version": "2022-11-28", + "User-Agent": "uphold-design-age-check", + }, + ) + with urlopen(request, timeout=20) as response: + batch = json.load(response) + if not isinstance(batch, list): + raise ValueError("GitHub issues API returned a non-list response") + for issue in batch: + if not isinstance(issue, dict) or DESIGN_LABEL not in _label_names(issue): + raise ValueError( + "GitHub issues API returned an item without the design label" + ) + issues.extend(batch) + if len(batch) < PAGE_SIZE: + return issues + page += 1 + + +def main() -> int: + repository = os.environ.get("GITHUB_REPOSITORY", "") + token = os.environ.get("GH_TOKEN") or os.environ.get("GITHUB_TOKEN") + if not token: + print("set GH_TOKEN or GITHUB_TOKEN to read Design records", file=sys.stderr) + return 2 + try: + issues = fetch_open_design_issues(repository, token) + overdue, errors = audit(issues, datetime.now(UTC)) + except ( + HTTPError, + URLError, + TimeoutError, + ValueError, + KeyError, + TypeError, + AttributeError, + json.JSONDecodeError, + ) as error: + print(f"could not audit Design records: {error}", file=sys.stderr) + return 2 + + if not overdue and not errors: + print(f"Design records are ruled within {RULING_WINDOW_DAYS} days.") + return 0 + for message in errors: + print(message, file=sys.stderr) + for issue, age_days in overdue: + print( + f"#{issue['number']} {issue['title']}: ruling:pending for " + f"{age_days} days (limit {RULING_WINDOW_DAYS}); rule it or label it " + "ruling:deferred.", + file=sys.stderr, + ) + return 1 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/src/config.rs b/src/config.rs index 2b6b567..603dcf0 100644 --- a/src/config.rs +++ b/src/config.rs @@ -370,6 +370,9 @@ pub(crate) struct Inherit { /// and before the repository's own rules, in the order written. #[serde(default)] pub paths: Vec, + /// Organization policy fetched at an immutable GitHub commit, after local paths. + #[serde(default)] + pub remote: Vec, /// Ids dropped from everything inherited. A repository's own rule of the /// same id overrides instead, which is the other half of the same choice. #[serde(default)] @@ -758,6 +761,8 @@ pub(crate) enum Origin { Set(String), /// Inherited from an `inherit.paths` entry, named by the path as written. Path(String), + /// A policy in another repository, with its immutable revision and path. + Remote(crate::remote_policy::Remote), } impl Origin { @@ -765,7 +770,7 @@ impl Origin { pub(crate) fn set(&self) -> Option<&str> { match self { Self::Set(name) => Some(name), - Self::Own | Self::Path(_) => None, + Self::Own | Self::Path(_) | Self::Remote(_) => None, } } } @@ -1787,6 +1792,67 @@ fn inherited_path(root: &Path, policy_path: &Path, relative: &str) -> Result Result> { + refuse_set_header(path, &parsed)?; + refuse_inherited_declaration(path, &parsed, "an inherited file")?; + refuse_inherited_override(path, &parsed, "an inherited file")?; + // Only `.rules` is merged from an inherited file, and a top-level line + // that would vanish is refused rather than dropped. A bundled set's + // default is adopted because the set is reviewed with the binary; an + // inherited file supplies rules, while the consumer owns its identity. + if parsed.private_owners_file.is_some() { + return Err(Fatal::at( + path, + format!( + "{} declares `private_owners_file` at its top level, and an inherited \ + file's top-level declarations are not adopted. Write the line at the \ + top of the consumer's policy file", + path.display() + ), + )); + } + // Refused rather than merged, and refused rather than ignored. Only + // `.rules` is merged below, so an inherited `[[shim]]` used to vanish -- + // and vanish in the worst possible way, because the `exec` rule that + // came with it survived, and `validate_shims` then reported that rule's + // `command.before` as naming a shim nobody declared. The author would be + // told to declare the shim they had in fact declared. Merging is the + // other available answer and it is not obviously right: a shim is the + // thing that stands in front of a command, and inheriting one silently + // puts a program in front of `git` on the strength of a path in an + // `[inherit]` line. Until that is a decision somebody makes on purpose, + // say so here. + if !parsed.shims.is_empty() { + return Err(Fatal::at( + path, + format!( + "{} declares {} `[[shim]]` table(s), and an inherited file's shims are \ + not adopted. A shim stands in front of a real command, which is not \ + something to acquire by inheriting a path. Move the `[[shim]]` into \ + the consumer's policy file", + path.display(), + parsed.shims.len() + ), + )); + } + if matches!(origin, Origin::Remote(_)) && parsed.inherit.is_some() { + return Err(Fatal::at( + path, + "an inherited file may not declare `[inherit]`; list each source in the repository's own policy", + )); + } + Ok(parsed + .rules + .into_values() + .map(|mut rule| { + rule.origin = origin.clone(); + rule + }) + .collect()) +} + /// Load a policy file, resolving `[inherit]` and validating the result. pub(crate) fn load(root: &Path, policy_path: &Path) -> Result { let text = read_to_string(policy_path)?; @@ -1871,53 +1937,21 @@ pub(crate) fn load_text(root: &Path, policy_path: &Path, text: &str) -> Result

Result

= file.rules.keys().map(String::as_str).collect(); let disabled = &inherit.disabled_rules; + let inheritable: Vec = inherited.iter().map(|rule| rule.id.clone()).collect(); report_reshaped_shadows(&inherited, &file.rules); let mut rules: Vec = inherited @@ -2036,20 +2071,6 @@ pub(crate) fn load_text(root: &Path, policy_path: &Path, text: &str) -> Result

= { - let mut names = Vec::new(); - for name in &inherit.sets { - if let Some((_, bundled)) = BUNDLED.iter().find(|(bundled, _)| *bundled == name) { - names.extend(parse_bundled(name, bundled)?.rules.into_keys()); - } - } - for relative in &inherit.paths { - let path = inherited_path(root, policy_path, relative)?; - let extended = read_to_string(&path)?; - names.extend(parse(&path, &extended)?.rules.into_keys()); - } - names - }; for id in disabled { if !inheritable.contains(id) { return Err(Fatal::at( @@ -2133,6 +2154,7 @@ fn report_reshaped_shadows(inherited: &[Rule], own: &BTreeMap) { let from = match &rule.origin { Origin::Set(name) => format!("the bundled set {name:?}"), Origin::Path(path) => format!("the inherited file {path:?}"), + Origin::Remote(remote) => format!("the remote policy {}", remote.label()), Origin::Own => continue, }; eprintln!( diff --git a/src/main.rs b/src/main.rs index a2a2d21..6db6e06 100644 --- a/src/main.rs +++ b/src/main.rs @@ -95,6 +95,7 @@ mod pins; mod policy; mod probe; mod prose; +mod remote_policy; mod report; mod runner; mod scan; @@ -979,6 +980,8 @@ fn pruned(value: serde_json::Value) -> serde_json::Value { #[derive(serde::Serialize)] struct EffectiveRule<'rule> { id: &'rule str, + #[serde(skip_serializing_if = "Option::is_none")] + remote: Option<&'rule remote_policy::Remote>, git_hooks: &'rule [String], /// `git_hooks` alone cannot answer where a hookless rule runs, and a caller /// that has to guess guesses the scan -- which is how a claim on a rule @@ -1030,6 +1033,9 @@ fn effective_rules_command(as_json: bool) -> Result { } else { hooks.join(", ") }; + if let config::Origin::Remote(remote) = &rule.origin { + println!(" source: {}", remote.label()); + } // Which parts of an inherited rule this policy changed, so a // reworded message or a widened selection reads as local rather // than as the set's. @@ -1056,6 +1062,10 @@ fn effective_rules_command(as_json: bool) -> Result { .iter() .map(|rule| EffectiveRule { id: &rule.id, + remote: match &rule.origin { + config::Origin::Remote(remote) => Some(remote), + _ => None, + }, git_hooks: rule.hooks(), seams: rule.seams(), reach: (rule.reach() == config::Reach::Pinned).then_some(config::Reach::Pinned), diff --git a/src/remote_policy.rs b/src/remote_policy.rs new file mode 100644 index 0000000..af2060c --- /dev/null +++ b/src/remote_policy.rs @@ -0,0 +1,196 @@ +//! Rules from another repository are read as blobs, never checked out or run. +//! The commit is the pin: a branch or tag cannot substitute new policy under +//! the same configuration. Fetch failure never falls back to another revision. + +use std::path::{Component, Path, PathBuf}; +use std::sync::atomic::{AtomicUsize, Ordering}; + +use serde::{Deserialize, Serialize}; + +use crate::error::{Fatal, Result}; + +/// A consumer-selected policy blob, identified by GitHub repository, immutable +/// commit and path. The loader also uses this exact tuple as rule provenance. +#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +pub(crate) struct Remote { + /// GitHub repository in `owner/name` form. + pub repo: String, + /// Full commit object ID, never a movable branch or tag. + pub rev: String, + /// Regular policy blob relative to that commit's tree. + pub path: String, +} + +impl Remote { + /// Repository, commit and path as one diagnostic source name. + pub(crate) fn label(&self) -> String { + format!("{}@{}:{}", self.repo, self.rev, self.path) + } + + fn validate(&self, policy: &Path) -> Result<()> { + let parts: Vec<_> = self.repo.split('/').collect(); + if parts.len() != 2 + || parts.iter().any(|part| { + part.is_empty() + || matches!(*part, "." | "..") + || !part + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || b"-_.".contains(&byte)) + }) + { + return Err(Fatal::at( + policy, + "`inherit.remote.repo` must name a GitHub repository as `owner/name`", + )); + } + if self.rev.len() != 40 || !self.rev.bytes().all(|byte| byte.is_ascii_hexdigit()) { + return Err(Fatal::at( + policy, + "`inherit.remote.rev` must be a full 40-character commit ID; branches and tags can move, so resolve the intended revision to its immutable commit first", + )); + } + if self.path.is_empty() + || self.path.split('/').any(|part| matches!(part, "." | "..")) + || self.path.chars().any(char::is_control) + || Path::new(&self.path) + .components() + .any(|part| !matches!(part, Component::Normal(_))) + { + return Err(Fatal::at( + policy, + "`inherit.remote.path` must be a nonempty repository-relative file path without `.` or `..` components", + )); + } + Ok(()) + } + + /// Fetch and read this exact blob, or refuse without a substitute policy. + pub(crate) fn read(&self, policy: &Path) -> Result { + self.validate(policy)?; + let directory = Repository::new()?; + let fail = || { + Fatal::at( + policy, + format!( + "could not read remote policy {} at its pinned commit; nothing was inherited", + self.label() + ), + ) + }; + crate::git::run_elsewhere(&directory.0, &["init", "--bare", "--quiet", "--template="])?; + let url = format!("https://github.com/{}.git", self.repo); + // No checkout, submodule recursion, hooks or repository-supplied config. + // Git's normal credentials and URL rewrites remain available, including + // credentials for an organization's private policy repository. + if crate::git::try_run_elsewhere( + &directory.0, + &[ + "-c", + "fetch.recurseSubmodules=false", + "fetch", + "--quiet", + "--depth=1", + "--no-tags", + "--", + &url, + &self.rev, + ], + )? + .is_none() + { + return Err(fail()); + } + let resolved = crate::git::try_run_elsewhere( + &directory.0, + &["rev-parse", "--verify", "FETCH_HEAD^{commit}"], + )? + .ok_or_else(fail)?; + if !resolved.trim().eq_ignore_ascii_case(&self.rev) { + return Err(fail()); + } + // The checked mode, exact literal path and content must all name the + // same tree entry. Pathspec magic must not check one file and then + // read another, and `-z` prevents filename quoting from changing it. + let entry = crate::git::try_run_elsewhere( + &directory.0, + &[ + "--literal-pathspecs", + "ls-tree", + "-z", + &self.rev, + "--", + &self.path, + ], + )? + .ok_or_else(fail)?; + let record = entry + .strip_suffix('\0') + .filter(|record| !record.contains('\0')) + .ok_or_else(fail)?; + let (metadata, path) = record.split_once('\t').ok_or_else(fail)?; + let mut fields = metadata.split(' '); + let mode = fields.next().ok_or_else(fail)?; + let kind = fields.next().ok_or_else(fail)?; + let object = fields.next().ok_or_else(fail)?; + if fields.next().is_some() + || !matches!(mode, "100644" | "100755") + || kind != "blob" + || path != self.path + || object.len() != 40 + || !object.bytes().all(|byte| byte.is_ascii_hexdigit()) + { + return Err(fail()); + } + let mut command = crate::shim::inner_tool("git"); + command + .current_dir(&directory.0) + .args(["cat-file", "blob", object]); + let output = crate::git::elsewhere(&mut command) + .output() + .map_err(|_| fail())?; + if !output.status.success() { + return Err(fail()); + } + String::from_utf8(output.stdout).map_err(|_| { + Fatal::at( + policy, + format!("remote policy {} is not UTF-8", self.label()), + ) + }) + } +} + +/// An exclusive scratch object store. It is never shared by repositories or +/// reused after a fetch failure, and is removed on both success and failure. +struct Repository(PathBuf); + +impl Repository { + fn new() -> Result { + static NEXT: AtomicUsize = AtomicUsize::new(0); + loop { + let path = std::env::temp_dir().join(format!( + "uphold-remote-{}-{}", + std::process::id(), + NEXT.fetch_add(1, Ordering::Relaxed) + )); + let mut builder = std::fs::DirBuilder::new(); + #[cfg(unix)] + { + use std::os::unix::fs::DirBuilderExt as _; + builder.mode(0o700); + } + match builder.create(&path) { + Ok(()) => return Ok(Self(path)), + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => {} + Err(error) => return Err(Fatal::at(&path, error)), + } + } + } +} + +impl Drop for Repository { + fn drop(&mut self) { + std::fs::remove_dir_all(&self.0).ok(); + } +} diff --git a/src/supply.rs b/src/supply.rs index e74b83b..90f230b 100644 --- a/src/supply.rs +++ b/src/supply.rs @@ -44,7 +44,7 @@ //! run with `--output-format json` and its own `risks` list is counted. The //! count is reported, never recomputed; the choice is to parse it or to run it //! for nothing. One named exception: a `metadata_mismatch` risk whose every -//! difference is one npm writes at publish (`PUBLISH_NORMALIZATIONS`) is +//! difference is a known npm rewrite or a verified `PyPI` optional extra is //! printed as tolerated rather than counted. //! //! guarddog is also not handed what it cannot look up. A `uv export` spells a @@ -101,6 +101,7 @@ use std::process::Command; use crate::config::{Policy, Waiver}; use crate::error::{Exit, Fatal, Result, verdict}; +mod pypi; mod references; /// The zizmor policy run where the repository has none of its own. @@ -1248,8 +1249,8 @@ const GUARDDOG_RULES: [&str; 10] = [ "metadata_mismatch", ]; -/// A difference guarddog's npm `metadata_mismatch` reports that npm itself -/// writes at publish, tolerated rather than counted. +/// The closed list of metadata normalizations supported by the report reader. +/// `OptionalExtras` additionally requires release evidence from [`pypi`]. /// /// The rule compares the registry's manifest for a release with the /// `package.json` inside its tarball. `npm publish` normalizes the manifest it @@ -1275,20 +1276,21 @@ const GUARDDOG_RULES: [&str; 10] = [ /// and a waiver still applies to it. Not on the list: `"install": "node-gyp /// rebuild"`, which npm adds where `binding.gyp` exists; an install script runs /// at install time, which `start` does not, so it joins only with a case and a -/// test. Nor `PyPI`'s optional extras, which guarddog 3.2.0 compares with the -/// required dependencies: its report keeps the names and drops the `extra ==` -/// markers, so it cannot tell an extra from an injected dependency. +/// test. `PyPI` optional extras use separate release evidence in [`pypi`], +/// because the report itself drops the markers that identify an extra. #[derive(Clone, Copy)] enum Normalization { DefaultStart, BinPrefixStripped, + OptionalExtras, } /// Every normalization tolerated, in the order a difference is tried against /// them. -const PUBLISH_NORMALIZATIONS: [Normalization; 2] = [ +const PUBLISH_NORMALIZATIONS: [Normalization; 3] = [ Normalization::DefaultStart, Normalization::BinPrefixStripped, + Normalization::OptionalExtras, ]; impl Normalization { @@ -1297,6 +1299,7 @@ impl Normalization { match self { Self::DefaultStart => "npm default start script", Self::BinPrefixStripped => "npm strips a leading node_modules/.bin/", + Self::OptionalExtras => "PyPI optional extras verified against release requires_dist", } } @@ -1310,6 +1313,7 @@ impl Normalization { Self::BinPrefixStripped => ["./node_modules/.bin/", "node_modules/.bin/"] .iter() .any(|prefix| tarball.strip_prefix(prefix) == Some(registry)), + Self::OptionalExtras => false, } } } @@ -1353,6 +1357,36 @@ fn tolerated(description: &str) -> Option> { (!accepted.is_empty()).then_some(accepted) } +/// A second source is consulted only for the exact `PyPI` report shape it can +/// explain. Unavailable or mismatched evidence leaves the scanner's finding +/// intact, so optional normalization never makes the scan depend on a pass. +fn tolerated_metadata( + ecosystem: &str, + name: &str, + version: &str, + description: &str, + metadata: &mut impl FnMut(&str, &str) -> std::result::Result, +) -> Option> { + match ecosystem { + "npm" => tolerated(description), + "pypi" => { + let differences = pypi::differences(description)?; + match metadata(name, version) { + Ok(release) => pypi::confirms(&release, name, version, &differences) + .map(|accepted| vec![accepted]), + Err(reason) => { + println!( + " retained: metadata_mismatch on pypi:{name}@{version} -- \ + could not verify optional extras: {reason}" + ); + None + } + } + } + _ => None, + } +} + /// guarddog's floor: the "rules failed to run while scanning" text, the JSON /// report's `errors` / `results` / `risks` keys and exit 0 on a finding were /// all measured against guarddog 3.2.0. @@ -1502,7 +1536,7 @@ enum Read { /// it for nothing. `risks` is guarddog's own list of what it objected to and /// nothing is re-judged: the count is reported, not recomputed. /// -/// A risk that is only npm's own publish-time rewrites ([`tolerated`]) is +/// A risk covered by [`tolerated_metadata`] is /// printed as tolerated, naming each, and is not held against the run. /// /// A risk a waiver covers is printed as waived, with the waiver's reason, and @@ -1522,6 +1556,7 @@ fn guarddog_read( code: Option, stdout: &str, stderr: &str, + metadata: &mut impl FnMut(&str, &str) -> std::result::Result, ) -> Read { // Its own admission first, because it survives whatever the report is. if let Some(reason) = rules_that_did_not_run(stdout).or_else(|| rules_that_did_not_run(stderr)) @@ -1603,11 +1638,10 @@ fn guarddog_read( .and_then(serde_json::Value::as_str) .or_else(|| risk.get("name").and_then(serde_json::Value::as_str)) .unwrap_or(""); - // npm's own publish-time rewrites, named on the line that lets + // Known metadata normalizations, named on the line that lets // them through. The risk and the underlying result must describe // the same difference; anything else keeps the risk counted. - if ecosystem == "npm" - && rule == "metadata_mismatch" + if rule == "metadata_mismatch" && let Some(description) = risk .get("threat_description") .and_then(serde_json::Value::as_str) @@ -1615,7 +1649,8 @@ fn guarddog_read( .get("metadata_mismatch") .and_then(serde_json::Value::as_str) == Some(description) - && let Some(accepted) = tolerated(description) + && let Some(accepted) = + tolerated_metadata(ecosystem, name, version, description, metadata) { println!( " tolerated: {rule} on {ecosystem}:{name}@{version} at {at} -- {}", @@ -1817,6 +1852,7 @@ fn guarddog(root: &Path, scope: &Scope, policy: &Policy) -> Result

{ status.status.code(), &String::from_utf8_lossy(&status.stdout), &String::from_utf8_lossy(&status.stderr), + &mut pypi::fetch, ) { Read::Clean => { ledger.read.insert("pypi"); @@ -1895,6 +1931,7 @@ fn guarddog(root: &Path, scope: &Scope, policy: &Policy) -> Result
{ status.status.code(), &String::from_utf8_lossy(&status.stdout), &String::from_utf8_lossy(&status.stderr), + &mut pypi::fetch, ) { Read::Clean => { ledger.read.insert("npm"); @@ -2255,6 +2292,7 @@ mod tests { Some(0), report, "", + &mut |_, _| Err(String::from("no registry evidence in this fixture")), ); match read { Read::Risks(..) => Ok(true), @@ -2425,9 +2463,8 @@ mod tests { assert_eq!(objected("npm", &bare.to_string()), Ok(true)); } - /// The npm normalizations are npm's, and `PyPI`'s optional extras are not on - /// the list: guarddog's report drops the `extra ==` markers, so pandas is - /// still a finding, and so is an npm-shaped description in a `PyPI` report. + /// Without release evidence, the dropped `extra ==` markers cannot be + /// recovered from the report. An npm-shaped `PyPI` report also stays a finding. #[test] fn pypi_metadata_mismatch_is_not_tolerated() { assert_eq!(objected("pypi", PANDAS), Ok(true)); @@ -2435,6 +2472,86 @@ mod tests { assert_eq!(objected("pypi", &npm_shaped), Ok(true)); } + #[test] + fn pypi_extras_need_release_evidence_and_agreeing_reports() { + let mut report: serde_json::Value = serde_json::from_str(PANDAS).unwrap(); + let description = "Dependency mismatch between PyPI metadata and pyproject.toml:\n In PyPI metadata but NOT in pyproject.toml: pyarrow\n"; + report[0]["result"]["results"]["metadata_mismatch"] = description.into(); + report[0]["result"]["risks"][0]["threat_description"] = description.into(); + for (requirements, clean) in [ + (vec!["pyarrow>=13; extra == 'parquet'"], true), + (vec!["pyarrow>=13"], false), + (vec!["pyarrow; extra == 'parquet'", "pyarrow"], false), + ] { + let read = guarddog_read( + "project", + "pypi", + &BTreeSet::from([String::from("pandas")]), + &[], + &mut Ledger::default(), + Some(0), + &report.to_string(), + "", + &mut |name, version| { + assert_eq!((name, version), ("pandas", "3.0.3")); + Ok(serde_json::json!({"info": { + "name": name, "version": version, "requires_dist": requirements, + }})) + }, + ); + assert_eq!(matches!(read, Read::Clean), clean); + } + report[0]["result"]["results"]["metadata_mismatch"] = "different evidence".into(); + let mut fetched = false; + let read = guarddog_read( + "project", + "pypi", + &BTreeSet::new(), + &[], + &mut Ledger::default(), + Some(0), + &report.to_string(), + "", + &mut |_, _| { + fetched = true; + Err(String::from("unexpected lookup")) + }, + ); + assert!(matches!(read, Read::Risks(..))); + assert!(!fetched); + } + + #[test] + fn verified_pypi_extras_leave_unrelated_risks_counted() { + let mut report: serde_json::Value = serde_json::from_str(PANDAS).unwrap(); + for other_risk in [false, true] { + if other_risk { + report[0]["result"]["risks"] + .as_array_mut() + .unwrap() + .push(serde_json::json!({"threat_rule": "typosquatting"})); + } + let read = guarddog_read( + "project", + "pypi", + &BTreeSet::from([String::from("pandas")]), + &[], + &mut Ledger::default(), + Some(0), + &report.to_string(), + "", + &mut |_, _| { + Ok(serde_json::from_str(include_str!( + "../tests/fixtures/guarddog/pypi-pandas-3.0.3-metadata.json" + )) + .unwrap()) + }, + ); + assert_eq!(matches!(read, Read::Risks(..)), other_risk); + assert_eq!(matches!(read, Read::Clean), !other_risk); + } + } + /// guarddog unpacks under the cache directory, whatever `TMPDIR` says: the /// order its `metadata_mismatch` rule lists an unpacked wheel in is that /// filesystem's, so `TMPDIR` would otherwise be the verdict's input. diff --git a/src/supply/pypi.rs b/src/supply/pypi.rs new file mode 100644 index 0000000..e76668b --- /dev/null +++ b/src/supply/pypi.rs @@ -0,0 +1,293 @@ +//! Evidence for guarddog's comparison of optional extras with required dependencies. +//! +//! The report drops markers. Only the exact release's `requires_dist` can +//! establish that every occurrence of a reported name requires an extra. + +use std::collections::{BTreeMap, BTreeSet}; +use std::time::Duration; + +use pyreq::Requirement; +use serde_json::Value; + +/// The release endpoint, rather than the project endpoint, binds the evidence +/// to what guarddog scanned. `PyPI` documents it at . +fn release_url(name: &str, version: &str) -> Option { + if name.is_empty() { + return None; + } + let name = package_name(name)?; + // Versions are a single URL segment, never a caller-selected destination. + if !version.bytes().any(|byte| byte.is_ascii_digit()) + || !version + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || b".!+_-".contains(&byte)) + { + return None; + } + Some(format!("https://pypi.org/pypi/{name}/{version}/json")) +} + +/// Fetch bounded, exact-release registry evidence. Failure leaves the scanner's +/// risk available to the ordinary finding and waiver handling. +pub(super) fn fetch(name: &str, version: &str) -> Result { + let url = release_url(name, version).ok_or("invalid release identity")?; + let agent: ureq::Agent = ureq::Agent::config_builder() + .https_only(true) + .max_redirects(0) + .timeout_global(Some(Duration::from_secs(15))) + .build() + .into(); + read_response(agent.get(&url).call().map_err(|error| error.to_string())?) +} + +/// Redirect bodies and partial responses cannot supply release evidence even +/// when their JSON has the expected fields. +fn read_response(mut response: ureq::http::Response) -> Result { + if response.status() != 200 { + return Err(format!("unexpected PyPI HTTP status {}", response.status())); + } + let body = response + .body_mut() + .with_config() + .limit(2 * 1024 * 1024) + .read_to_string() + .map_err(|error| error.to_string())?; + serde_json::from_str(&body).map_err(|error| error.to_string()) +} + +/// Only the one-sided, names-only report has a meaning this evidence proves. +/// Unknown blocks, malformed names and duplicate normalized names stay findings. +pub(super) fn differences(description: &str) -> Option> { + let mut lines = description.lines(); + if lines.next()? != "Dependency mismatch between PyPI metadata and pyproject.toml:" { + return None; + } + let names = lines + .next()? + .strip_prefix(" In PyPI metadata but NOT in pyproject.toml: ")?; + if lines.next().is_some() { + return None; + } + let mut parsed = BTreeSet::new(); + for name in names.split(", ") { + if name.is_empty() || !parsed.insert(package_name(name)?) { + return None; + } + } + (!parsed.is_empty()).then_some(parsed) +} + +/// Names are parsed by the requirement reader, then constrained to the bare +/// name form so URL segments and report entries cannot carry extra syntax. +fn package_name(name: &str) -> Option { + let requirement = Requirement::parse(name).ok()?; + (requirement.name == name).then(|| super::references::normalise(name)) +} + +/// Confirm every reported name occurs only under a recognized optional-extra +/// marker in the named release. Any other occurrence prevents tolerance. +pub(super) fn confirms( + metadata: &Value, + name: &str, + version: &str, + differences: &BTreeSet, +) -> Option { + let info = metadata.get("info")?; + if package_name(info.get("name")?.as_str()?)? != package_name(name)? + || info.get("version")?.as_str()? != version + { + return None; + } + // The closed normalization recognizes a simple positive equality. More + // expressive markers stay findings; no host environment is guessed. + let extra_marker = regex::Regex::new( + r#"^(?:extra == "[A-Za-z0-9][A-Za-z0-9._-]*"|"[A-Za-z0-9][A-Za-z0-9._-]*" == extra)$"#, + ) + .ok()?; + let mut optional = BTreeMap::new(); + for raw in info.get("requires_dist")?.as_array()? { + let requirement = Requirement::parse(raw.as_str()?).ok()?; + let extra_only = requirement + .marker + .as_ref() + .is_some_and(|marker| extra_marker.is_match(&marker.to_string())); + optional + .entry(package_name(&requirement.name)?) + .and_modify(|all| *all &= extra_only) + .or_insert(extra_only); + } + if !differences + .iter() + .all(|dependency| optional.get(dependency) == Some(&true)) + { + return None; + } + Some(format!( + "{} ({})", + super::Normalization::OptionalExtras.name(), + differences + .iter() + .map(ToString::to_string) + .collect::>() + .join(", ") + )) +} + +#[cfg(test)] +mod tests { + use super::{confirms, differences, read_response, release_url}; + use serde_json::json; + use std::io::Write as _; + + const DESCRIPTION: &str = "Dependency mismatch between PyPI metadata and pyproject.toml:\n In PyPI metadata but NOT in pyproject.toml: pyarrow\n"; + + /// Exercise the production response reader over a local HTTP connection; + /// production URL selection always uses HTTPS at the fixed registry host. + fn response(status: u16, body: &str) -> Result { + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let address = listener.local_addr().unwrap(); + let body = body.to_owned(); + let server = std::thread::spawn(move || { + let (mut stream, _) = listener.accept().unwrap(); + write!(stream, "HTTP/1.1 {status} Fixture\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", body.len()).ok(); + }); + let agent: ureq::Agent = ureq::Agent::config_builder() + .max_redirects(0) + .http_status_as_error(false) + .timeout_global(Some(std::time::Duration::from_secs(5))) + .build() + .into(); + let result = read_response(agent.get(format!("http://{address}/")).call().unwrap()); + server.join().unwrap(); + result + } + + #[test] + fn only_a_complete_bounded_json_response_can_be_evidence() { + assert_eq!(response(200, "{}"), Ok(json!({}))); + for status in [206, 301, 302, 307, 404, 500] { + assert!(response(status, "{}").is_err(), "{status}"); + } + assert!(response(200, "not json").is_err()); + assert!(response(200, &" ".repeat(2 * 1024 * 1024 + 1)).is_err()); + } + + #[test] + fn the_recorded_pandas_report_is_explained_by_its_release_metadata() { + let report: serde_json::Value = serde_json::from_str(include_str!( + "../../tests/fixtures/guarddog/pypi-pandas-3.0.3.json" + )) + .unwrap(); + let metadata: serde_json::Value = serde_json::from_str(include_str!( + "../../tests/fixtures/guarddog/pypi-pandas-3.0.3-metadata.json" + )) + .unwrap(); + let names = differences( + report[0]["result"]["results"]["metadata_mismatch"] + .as_str() + .unwrap(), + ) + .unwrap(); + assert!(confirms(&metadata, "pandas", "3.0.3", &names).is_some()); + } + + #[test] + fn every_occurrence_must_require_a_positive_extra() { + let names = differences(DESCRIPTION).unwrap(); + for (requirements, accepted) in [ + (vec!["pyarrow>=13; extra == 'parquet'"], true), + ( + vec!["pyarrow; python_version >= '3.10' and extra == 'parquet'"], + false, + ), + (vec!["pyarrow; 'parquet' == extra"], true), + ( + vec!["pyarrow; extra == 'parquet'", "pyarrow; extra == 'test'"], + true, + ), + ( + vec![ + "pyarrow; extra == 'parquet'", + "pyarrow; sys_platform == 'win32'", + ], + false, + ), + ( + vec!["pyarrow; extra == 'parquet' or python_version < '3.10'"], + false, + ), + (vec!["pyarrow; extra != 'parquet'"], false), + (vec!["pyarrow"], false), + (vec!["another; extra == 'parquet'"], false), + (vec!["pyarrow; extra =="], false), + (vec![], false), + ] { + let metadata = json!({"info": {"name": "example", "version": "1.0", "requires_dist": requirements}}); + assert_eq!( + confirms(&metadata, "example", "1.0", &names).is_some(), + accepted, + "{metadata}" + ); + } + } + + #[test] + fn evidence_must_name_the_scanned_release_and_every_difference() { + let names = differences(DESCRIPTION).unwrap(); + for metadata in [ + json!({"info": {"name": "different", "version": "1.0", "requires_dist": ["pyarrow; extra == 'x'"]}}), + json!({"info": {"name": "example", "version": "2.0", "requires_dist": ["pyarrow; extra == 'x'"]}}), + json!({"info": {"name": "example", "version": "1.0", "requires_dist": null}}), + json!({"info": {"name": "example", "version": "1.0", "requires_dist": [42]}}), + json!({}), + ] { + assert!(confirms(&metadata, "example", "1.0", &names).is_none()); + } + let two = differences(&DESCRIPTION.replace("pyarrow\n", "pyarrow, injected\n")).unwrap(); + let metadata = json!({"info": {"name": "Example_Kit", "version": "1.0", "requires_dist": ["PyArrow; extra == 'x'"]}}); + assert!(confirms(&metadata, "example-kit", "1.0", &names).is_some()); + assert!(confirms(&metadata, "example-kit", "1.0", &two).is_none()); + } + + #[test] + fn unknown_or_ambiguous_report_shapes_are_not_candidates() { + for description in [ + DESCRIPTION.replace("pyarrow", ""), + DESCRIPTION.replace("pyarrow", "pyarrow, PyArrow"), + DESCRIPTION.replace("pyarrow", "pyarrow>=13"), + DESCRIPTION.replace( + "pyarrow", + "pyarrow, injected\n In pyproject.toml but NOT in PyPI metadata: removed", + ), + format!("{DESCRIPTION}unrecognized difference\n"), + format!("{DESCRIPTION}\n"), + ] { + assert!(differences(&description).is_none(), "{description}"); + } + } + + #[test] + fn release_identity_cannot_change_the_destination() { + assert_eq!( + release_url("Example_Kit", "1.0+local"), + Some(String::from( + "https://pypi.org/pypi/example-kit/1.0+local/json" + )) + ); + for name in ["../evil", "https://elsewhere", "x?y", "x#y", ""] { + assert!(release_url(name, "1.0").is_none()); + } + for version in [ + "../evil", + "1.0/anything", + "1.0?x", + "1.0#x", + "%2f", + ".", + "..", + "", + ] { + assert!(release_url("example", version).is_none()); + } + } +} diff --git a/tests/fixtures/guarddog/pypi-pandas-3.0.3-metadata.json b/tests/fixtures/guarddog/pypi-pandas-3.0.3-metadata.json new file mode 100644 index 0000000..5eb1a96 --- /dev/null +++ b/tests/fixtures/guarddog/pypi-pandas-3.0.3-metadata.json @@ -0,0 +1,92 @@ +{ + "info": { + "name": "pandas", + "version": "3.0.3", + "requires_dist": [ + "numpy>=1.26.0; python_version < \"3.14\"", + "numpy>=2.3.3; python_version >= \"3.14\"", + "python-dateutil>=2.8.2", + "tzdata; sys_platform == \"win32\"", + "tzdata; sys_platform == \"emscripten\"", + "hypothesis>=6.116.0; extra == \"test\"", + "pytest>=8.3.4; extra == \"test\"", + "pytest-xdist>=3.6.1; extra == \"test\"", + "pyarrow>=13.0.0; extra == \"pyarrow\"", + "bottleneck>=1.4.2; extra == \"performance\"", + "numba>=0.60.0; extra == \"performance\"", + "numexpr>=2.10.2; extra == \"performance\"", + "scipy>=1.14.1; extra == \"computation\"", + "xarray>=2024.10.0; extra == \"computation\"", + "fsspec>=2024.10.0; extra == \"fss\"", + "s3fs>=2024.10.0; extra == \"aws\"", + "gcsfs>=2024.10.0; extra == \"gcp\"", + "odfpy>=1.4.1; extra == \"excel\"", + "openpyxl>=3.1.5; extra == \"excel\"", + "python-calamine>=0.3.0; extra == \"excel\"", + "pyxlsb>=1.0.10; extra == \"excel\"", + "xlrd>=2.0.1; extra == \"excel\"", + "xlsxwriter>=3.2.0; extra == \"excel\"", + "pyarrow>=13.0.0; extra == \"parquet\"", + "pyarrow>=13.0.0; extra == \"feather\"", + "pyiceberg>=0.8.1; extra == \"iceberg\"", + "tables>=3.10.1; extra == \"hdf5\"", + "pyreadstat>=1.2.8; extra == \"spss\"", + "SQLAlchemy>=2.0.36; extra == \"postgresql\"", + "psycopg2>=2.9.10; extra == \"postgresql\"", + "adbc-driver-postgresql>=1.2.0; extra == \"postgresql\"", + "SQLAlchemy>=2.0.36; extra == \"mysql\"", + "pymysql>=1.1.1; extra == \"mysql\"", + "SQLAlchemy>=2.0.36; extra == \"sql-other\"", + "adbc-driver-postgresql>=1.2.0; extra == \"sql-other\"", + "adbc-driver-sqlite>=1.2.0; extra == \"sql-other\"", + "beautifulsoup4>=4.12.3; extra == \"html\"", + "html5lib>=1.1; extra == \"html\"", + "lxml>=5.3.0; extra == \"html\"", + "lxml>=5.3.0; extra == \"xml\"", + "matplotlib>=3.9.3; extra == \"plot\"", + "jinja2>=3.1.5; extra == \"output-formatting\"", + "tabulate>=0.9.0; extra == \"output-formatting\"", + "PyQt5>=5.15.9; extra == \"clipboard\"", + "qtpy>=2.4.2; extra == \"clipboard\"", + "zstandard>=0.23.0; extra == \"compression\"", + "pytz>=2020.1; extra == \"timezone\"", + "adbc-driver-postgresql>=1.2.0; extra == \"all\"", + "adbc-driver-sqlite>=1.2.0; extra == \"all\"", + "beautifulsoup4>=4.12.3; extra == \"all\"", + "bottleneck>=1.4.2; extra == \"all\"", + "fastparquet>=2024.11.0; extra == \"all\"", + "fsspec>=2024.10.0; extra == \"all\"", + "gcsfs>=2024.10.0; extra == \"all\"", + "html5lib>=1.1; extra == \"all\"", + "hypothesis>=6.116.0; extra == \"all\"", + "jinja2>=3.1.5; extra == \"all\"", + "lxml>=5.3.0; extra == \"all\"", + "matplotlib>=3.9.3; extra == \"all\"", + "numba>=0.60.0; extra == \"all\"", + "numexpr>=2.10.2; extra == \"all\"", + "odfpy>=1.4.1; extra == \"all\"", + "openpyxl>=3.1.5; extra == \"all\"", + "psycopg2>=2.9.10; extra == \"all\"", + "pyarrow>=13.0.0; extra == \"all\"", + "pyiceberg>=0.8.1; extra == \"all\"", + "pymysql>=1.1.1; extra == \"all\"", + "PyQt5>=5.15.9; extra == \"all\"", + "pyreadstat>=1.2.8; extra == \"all\"", + "pytest>=8.3.4; extra == \"all\"", + "pytest-xdist>=3.6.1; extra == \"all\"", + "python-calamine>=0.3.0; extra == \"all\"", + "pytz>=2020.1; extra == \"all\"", + "pyxlsb>=1.0.10; extra == \"all\"", + "qtpy>=2.4.2; extra == \"all\"", + "scipy>=1.14.1; extra == \"all\"", + "s3fs>=2024.10.0; extra == \"all\"", + "SQLAlchemy>=2.0.36; extra == \"all\"", + "tables>=3.10.1; extra == \"all\"", + "tabulate>=0.9.0; extra == \"all\"", + "xarray>=2024.10.0; extra == \"all\"", + "xlrd>=2.0.1; extra == \"all\"", + "xlsxwriter>=3.2.0; extra == \"all\"", + "zstandard>=0.23.0; extra == \"all\"" + ] + } +} diff --git a/tests/remote_policy_cli.rs b/tests/remote_policy_cli.rs new file mode 100644 index 0000000..3657e72 --- /dev/null +++ b/tests/remote_policy_cli.rs @@ -0,0 +1,329 @@ +//! Exercise the normal loader against real Git blobs, with network URLs +//! rewritten per child process to an isolated local policy repository. +#![expect( + clippy::let_underscore_must_use, + clippy::indexing_slicing, + clippy::tests_outside_test_module, + clippy::unwrap_used, + reason = "CLI fixtures report failures by assertions" +)] +mod support; + +use std::path::PathBuf; +use std::process::{Command, Output}; + +struct Fixture { + root: PathBuf, + source: PathBuf, + config: PathBuf, + rev: String, +} + +impl Fixture { + fn new(text: &str) -> Self { + let root = support::scratch("remote-policy"); + let source = root.join("source"); + std::fs::create_dir_all(source.join("policy")).unwrap(); + support::git(&source, &["init", "-q", "-b", "main"]); + std::fs::write(source.join("policy/org.toml"), text).unwrap(); + support::git(&source, &["add", "."]); + support::git(&source, &["commit", "-qm", "policy"]); + let answer = support::git_command(&source) + .args(["rev-parse", "HEAD"]) + .output() + .unwrap(); + let rev = String::from_utf8(answer.stdout).unwrap().trim().to_owned(); + let config = root.join("gitconfig"); + std::fs::write( + &config, + format!( + "[url \"file://{}\"]\n insteadOf = https://github.com/example/policy.git\n", + source.display() + ), + ) + .unwrap(); + std::fs::create_dir_all(root.join("consumer/policy")).unwrap(); + support::git(&root.join("consumer"), &["init", "-q", "-b", "main"]); + let fixture = Self { + root, + source, + config, + rev, + }; + fixture.policy(""); + fixture + } + + fn repin(&mut self) { + support::git(&self.source, &["add", "."]); + support::git(&self.source, &["commit", "-qm", "update"]); + let output = support::git_command(&self.source) + .args(["rev-parse", "HEAD"]) + .output() + .unwrap(); + String::from_utf8(output.stdout) + .unwrap() + .trim() + .clone_into(&mut self.rev); + self.policy(""); + } + + fn policy(&self, suffix: &str) { + self.with_remote("example/policy", &self.rev, "policy/org.toml", suffix); + } + + fn with_remote(&self, repo: &str, rev: &str, path: &str, suffix: &str) { + std::fs::write(self.root.join("consumer/policy/principles.toml"), format!( + "[inherit]\nremote = [{{ repo = {repo:?}, rev = {rev:?}, path = {path:?} }}]\n{suffix}" + )).unwrap(); + } + + fn run(&self, args: &[&str]) -> Output { + let mut command = Command::new(env!("CARGO_BIN_EXE_uphold")); + command + .args(args) + .current_dir(self.root.join("consumer")) + .env("GIT_CONFIG_GLOBAL", &self.config) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_TERMINAL_PROMPT", "0"); + support::without_git_environment(&mut command); + // Every load also exercises the environment exported to a Git hook. + command.env("GIT_DIR", self.root.join("consumer/.git")); + command.output().unwrap() + } +} + +const RULE: &str = "[rule.org-rule]\nregexp = 'ORG_BANNED'\nmessage = 'organization rule'\n[rule.org-rule.files]\nexclude = ['policy/**']\n"; + +#[test] +fn remote_rules_run_keep_provenance_and_accept_overrides() { + let fixture = Fixture::new(RULE); + fixture.policy("[override.org-rule]\nmessage = 'local wording'\n[override.org-rule.files]\nexclude = ['policy/**', 'skip.txt']\n"); + let output = fixture.run(&["rules", "--effective", "--json"]); + assert_eq!( + output.status.code(), + Some(0), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let rules: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + assert_eq!(rules[0]["remote"]["rev"], fixture.rev); + assert_eq!(rules[0]["remote"]["repo"], "example/policy"); + assert_eq!(rules[0]["remote"]["path"], "policy/org.toml"); + assert!( + rules[0]["overridden"] + .as_array() + .unwrap() + .contains(&serde_json::json!("message")) + ); + std::fs::write(fixture.root.join("consumer/skip.txt"), "ORG_BANNED").unwrap(); + support::git(&fixture.root.join("consumer"), &["add", "."]); + let clean = fixture.run(&["scan"]); + assert_eq!( + clean.status.code(), + Some(0), + "{}", + String::from_utf8_lossy(&clean.stderr) + ); + std::fs::write(fixture.root.join("consumer/hit.txt"), "ORG_BANNED").unwrap(); + support::git(&fixture.root.join("consumer"), &["add", "."]); + let refused = fixture.run(&["scan"]); + assert_eq!( + refused.status.code(), + Some(1), + "{}", + String::from_utf8_lossy(&refused.stderr) + ); + assert!( + format!( + "{}{}", + String::from_utf8_lossy(&refused.stdout), + String::from_utf8_lossy(&refused.stderr) + ) + .contains("local wording"), + "{refused:?}" + ); +} + +#[test] +fn disabled_and_own_rules_use_the_same_remote_snapshot() { + let fixture = Fixture::new(RULE); + fixture.policy("disabled_rules = ['org-rule']\n"); + let output = fixture.run(&["rules", "--effective", "--json"]); + assert_eq!( + output.status.code(), + Some(0), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!( + serde_json::from_slice::(&output.stdout).unwrap(), + serde_json::json!([]) + ); + fixture.policy( + "[rule.org-rule]\nregexp = 'LOCAL_BANNED'\nmessage = 'own rule'\nfiles.include = ['.']\nfiles.exclude = ['policy/**']\n", + ); + let own_output = fixture.run(&["rules", "--effective", "--json"]); + assert_eq!( + own_output.status.code(), + Some(0), + "{}", + String::from_utf8_lossy(&own_output.stderr) + ); + let rules: serde_json::Value = serde_json::from_slice(&own_output.stdout).unwrap(); + assert!(rules[0].get("remote").is_none()); +} + +#[test] +fn a_changed_source_cannot_change_the_pinned_policy() { + let fixture = Fixture::new(RULE); + std::fs::write( + fixture.source.join("policy/org.toml"), + "[rule.replacement]\nregexp = 'NEW'\nmessage = 'new rule'\n", + ) + .unwrap(); + support::git(&fixture.source, &["add", "."]); + support::git(&fixture.source, &["commit", "-qm", "change"]); + let output = fixture.run(&["rules", "--effective", "--json"]); + assert_eq!( + output.status.code(), + Some(0), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let rules: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + assert_eq!(rules[0]["id"], "org-rule"); +} + +#[test] +fn missing_commits_files_and_unreachable_repositories_fail_closed() { + let fixture = Fixture::new(RULE); + for (rev, path) in [ + ( + "1111111111111111111111111111111111111111", + "policy/org.toml", + ), + (fixture.rev.as_str(), "policy/missing.toml"), + ] { + fixture.with_remote("example/policy", rev, path, ""); + let output = fixture.run(&["rules", "--effective", "--json"]); + assert_eq!(output.status.code(), Some(2)); + assert_eq!(output.stdout, Vec::::new()); + assert!(String::from_utf8_lossy(&output.stderr).contains("pinned commit")); + } + fixture.policy(""); + std::fs::remove_dir_all(&fixture.source).unwrap(); + let output = fixture.run(&["rules", "--effective", "--json"]); + assert_eq!(output.status.code(), Some(2)); + assert_eq!(output.stdout, Vec::::new()); +} + +#[test] +fn mutable_pins_and_unsafe_paths_are_refused_before_network_access() { + let fixture = Fixture::new(RULE); + for (repo, rev, path, field) in [ + ("example/policy", "main", "policy/org.toml", "rev"), + ("example/policy", "v1", "policy/org.toml", "rev"), + ( + "https://example.test/policy", + fixture.rev.as_str(), + "policy/org.toml", + "repo", + ), + ( + "example/policy", + fixture.rev.as_str(), + "../org.toml", + "path", + ), + ("example/policy", fixture.rev.as_str(), "/org.toml", "path"), + ] { + fixture.with_remote(repo, rev, path, ""); + let output = fixture.run(&["rules", "--effective"]); + assert_eq!(output.status.code(), Some(2)); + assert!( + String::from_utf8_lossy(&output.stderr).contains(&format!("inherit.remote.{field}")) + ); + } +} + +#[test] +fn inherited_shims_and_transitive_inheritance_are_explicit_errors() { + for (text, evidence) in [ + ( + "[[shim]]\ncommand = 'gh'\nmatch = ['api:*']\n", + "shims are not adopted", + ), + ( + "[inherit]\nsets = ['process-residue']\n", + "may not declare `[inherit]`", + ), + ] { + let fixture = Fixture::new(text); + let output = fixture.run(&["rules", "--effective"]); + assert_eq!(output.status.code(), Some(2)); + assert!( + String::from_utf8_lossy(&output.stderr).contains(evidence), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + } +} + +#[test] +fn invalid_remote_content_is_never_a_partial_policy() { + for bytes in [b"[rule.broken".as_slice(), b"\xff".as_slice()] { + let mut fixture = Fixture::new(RULE); + std::fs::write(fixture.source.join("policy/org.toml"), bytes).unwrap(); + fixture.repin(); + let output = fixture.run(&["rules", "--effective", "--json"]); + assert_eq!(output.status.code(), Some(2)); + assert_eq!(output.stdout, Vec::::new()); + } +} + +#[cfg(unix)] +#[test] +fn a_remote_symlink_is_not_a_policy_blob() { + let mut fixture = Fixture::new(RULE); + std::os::unix::fs::symlink("org.toml", fixture.source.join("policy/link.toml")).unwrap(); + fixture.repin(); + fixture.with_remote("example/policy", &fixture.rev, "policy/link.toml", ""); + let output = fixture.run(&["rules", "--effective", "--json"]); + assert_eq!(output.status.code(), Some(2)); + assert_eq!(output.stdout, Vec::::new()); +} + +#[test] +fn a_magic_looking_remote_filename_is_read_literally() { + let mut fixture = Fixture::new(RULE); + std::fs::write(fixture.source.join("abc.toml"), RULE).unwrap(); + let literal = RULE.replace("org-rule", "literal-rule"); + std::fs::write(fixture.source.join(":(literal)abc.toml"), literal).unwrap(); + fixture.repin(); + fixture.with_remote("example/policy", &fixture.rev, ":(literal)abc.toml", ""); + let output = fixture.run(&["rules", "--effective", "--json"]); + assert_eq!( + output.status.code(), + Some(0), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let rules: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + assert_eq!(rules[0]["id"], "literal-rule"); +} + +#[cfg(unix)] +#[test] +fn pathspec_magic_cannot_use_a_regular_files_mode_to_read_a_symlink() { + let mut fixture = Fixture::new(RULE); + // Without literal pathspecs, ls-tree checks abc.toml's regular-file mode + // while rev:path reads this symlink's target bytes as valid TOML. + std::fs::write(fixture.source.join("abc.toml"), RULE).unwrap(); + std::os::unix::fs::symlink(RULE, fixture.source.join(":(literal)abc.toml")).unwrap(); + fixture.repin(); + fixture.with_remote("example/policy", &fixture.rev, ":(literal)abc.toml", ""); + let output = fixture.run(&["rules", "--effective", "--json"]); + assert_eq!(output.status.code(), Some(2)); + assert_eq!(output.stdout, Vec::::new()); +} diff --git a/tests/scan_cli.rs b/tests/scan_cli.rs index 9bf7d4d..e1aa8a9 100644 --- a/tests/scan_cli.rs +++ b/tests/scan_cli.rs @@ -175,6 +175,53 @@ fn a_path_rule_matches_the_path_and_not_the_content() { assert!(!text.contains("notes.md"), "{text}"); } +#[test] +fn retired_adr_paths_are_refused_by_the_repository_policy() { + let source: toml::Value = toml::from_str(include_str!("../policy/principles.toml")).unwrap(); + let mut policy = toml::Table::new(); + let mut rules = toml::Table::new(); + rules.insert( + "adr-retired".into(), + source + .get("rule") + .unwrap() + .get("adr-retired") + .unwrap() + .clone(), + ); + policy.insert("rule".into(), toml::Value::Table(rules)); + let root = workspace(); + write( + &root, + "policy/principles.toml", + &toml::to_string(&policy).unwrap(), + ); + write(&root, "docs/DESIGN.md", "Current architecture.\n"); + let empty = scan(&root); + assert_eq!(code(&empty), 0, "{}", stderr(&empty)); + for path in [ + "docs/adr/0001-old.md", + "docs/adr/0013-new.md", + "docs/adr/nested/record.md", + ] { + write(&root, path, "A decision.\n"); + let output = scan(&root); + assert_eq!(code(&output), 1, "{}", stderr(&output)); + assert!( + stderr(&output).contains("adr-retired"), + "{}", + stderr(&output) + ); + assert!(stderr(&output).contains(path), "{}", stderr(&output)); + assert!( + stderr(&output).contains("file a Design record"), + "{}", + stderr(&output) + ); + std::fs::remove_file(root.join(path)).unwrap(); + } +} + #[test] fn a_require_rule_fails_on_the_file_that_is_missing_the_pattern() { let root = workspace(); diff --git a/tests/test_design_age.py b/tests/test_design_age.py new file mode 100644 index 0000000..3dd9bac --- /dev/null +++ b/tests/test_design_age.py @@ -0,0 +1,183 @@ +"""The issue check shares this predicate between local runs and scheduled CI.""" + +from __future__ import annotations + +import io +import json +import sys +import unittest +from datetime import UTC, datetime, timedelta +from pathlib import Path +from unittest.mock import patch +from urllib.error import URLError + +ROOT = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(ROOT / "scripts")) + +import design_age # noqa: E402 + +NOW = datetime(2026, 10, 15, 12, 0, tzinfo=UTC) + + +def issue(number: int, ruling: str, age: timedelta, **extra: object) -> dict: + value = { + "number": number, + "title": f"Design record {number}", + "state": "open", + "created_at": (NOW - age).isoformat().replace("+00:00", "Z"), + "labels": [ + {"name": "design"}, + {"name": ruling}, + ], + } + value.update(extra) + return value + + +class DesignAge(unittest.TestCase): + def test_pending_is_due_at_thirty_days_but_not_one_second_before(self): + before = issue(1, "ruling:pending", timedelta(days=30) - timedelta(seconds=1)) + boundary = issue(2, "ruling:pending", timedelta(days=30)) + + overdue, errors = design_age.audit([before, boundary], NOW) + + self.assertEqual([record["number"] for record, _ in overdue], [2]) + self.assertEqual(overdue[0][1], 30) + self.assertEqual(errors, []) + + def test_each_final_ruling_closes_the_age_gate(self): + issues = [ + issue(number, f"ruling:{status}", timedelta(days=90)) + for number, status in enumerate(("accepted", "rejected", "deferred"), 1) + ] + + overdue, errors = design_age.audit(issues, NOW) + + self.assertEqual(overdue, []) + self.assertEqual(errors, []) + + def test_invalid_or_missing_ruling_label_is_reported_immediately(self): + missing = issue(1, "ruling:pending", timedelta(days=1)) + missing["labels"] = [{"name": "design"}] + multiple = issue(2, "ruling:pending", timedelta(days=1)) + multiple["labels"].append({"name": "ruling:accepted"}) + unknown = issue(3, "ruling:withdrawn", timedelta(days=1)) + + overdue, errors = design_age.audit([missing, multiple, unknown], NOW) + + self.assertEqual(overdue, []) + self.assertEqual(len(errors), 3) + self.assertIn("no ruling label", errors[0]) + self.assertIn("ruling:pending, ruling:accepted", errors[1]) + self.assertIn("ruling:withdrawn", errors[2]) + + def test_closed_non_design_and_pull_request_items_are_ignored(self): + closed = issue(1, "ruling:pending", timedelta(days=90), state="closed") + other = issue(2, "ruling:pending", timedelta(days=90)) + other["labels"] = [{"name": "ruling:pending"}] + pull_request = issue(3, "ruling:pending", timedelta(days=90), pull_request={}) + + overdue, errors = design_age.audit([closed, other, pull_request], NOW) + + self.assertEqual(overdue, []) + self.assertEqual(errors, []) + + def test_fetch_reads_a_second_page_before_returning(self): + first_page = [ + issue(number, "ruling:accepted", timedelta(days=1)) + for number in range(1, 101) + ] + second_page = [issue(101, "ruling:pending", timedelta(days=31))] + responses = [ + io.BytesIO(json.dumps(batch).encode()) + for batch in (first_page, second_page) + ] + + with patch.object(design_age, "urlopen", side_effect=responses) as open_url: + records = design_age.fetch_open_design_issues("owner/repo", "token") + + self.assertEqual(len(records), 101) + self.assertIn("page=1", open_url.call_args_list[0].args[0].full_url) + self.assertIn("page=2", open_url.call_args_list[1].args[0].full_url) + overdue, errors = design_age.audit(records, NOW) + self.assertEqual([record["number"] for record, _ in overdue], [101]) + self.assertEqual(errors, []) + + def test_main_reports_overdue_record_and_success_with_distinct_exit_codes(self): + for records, expected in [ + ([], 0), + ([issue(42, "ruling:pending", timedelta(days=31))], 1), + ]: + with ( + self.subTest(expected=expected), + patch.dict( + "os.environ", + {"GITHUB_REPOSITORY": "owner/repo", "GH_TOKEN": "token"}, + ), + patch.object( + design_age, "fetch_open_design_issues", return_value=records + ), + patch("sys.stdout", new_callable=io.StringIO) as stdout, + patch("sys.stderr", new_callable=io.StringIO) as stderr, + ): + self.assertEqual(design_age.main(), expected) + if expected: + self.assertIn("#42", stderr.getvalue()) + self.assertIn("ruling:deferred", stderr.getvalue()) + else: + self.assertIn("within 30 days", stdout.getvalue()) + + def test_fetch_refuses_invalid_repository_before_using_credentials(self): + with ( + patch.object(design_age, "urlopen") as open_url, + self.assertRaisesRegex(ValueError, "owner/repository"), + ): + design_age.fetch_open_design_issues("../elsewhere", "token") + open_url.assert_not_called() + + def test_fetch_rejects_api_response_without_the_requested_label(self): + response = io.BytesIO(json.dumps([{"labels": []}]).encode()) + with ( + patch.object(design_age, "urlopen", return_value=response), + self.assertRaisesRegex(ValueError, "without the design label"), + ): + design_age.fetch_open_design_issues("owner/repo", "token") + + def test_main_reports_api_read_failure_as_exit_two(self): + with ( + patch.dict( + "os.environ", + {"GITHUB_REPOSITORY": "owner/repo", "GITHUB_TOKEN": "token"}, + ), + patch.object( + design_age, "fetch_open_design_issues", side_effect=URLError("offline") + ), + patch("sys.stderr", new_callable=io.StringIO) as stderr, + ): + result = design_age.main() + + self.assertEqual(result, 2) + self.assertIn("could not audit Design records", stderr.getvalue()) + + def test_malformed_issue_response_fails_closed_without_a_traceback(self): + malformed = issue(1, "ruling:pending", timedelta(days=1)) + malformed["created_at"] = "2026-10-01T12:00:00" + with ( + patch.dict( + "os.environ", + {"GITHUB_REPOSITORY": "owner/repo", "GITHUB_TOKEN": "token"}, + ), + patch.object( + design_age, "fetch_open_design_issues", return_value=[malformed] + ), + patch("sys.stderr", new_callable=io.StringIO) as stderr, + ): + result = design_age.main() + + self.assertEqual(result, 2) + self.assertIn("must include a timezone", stderr.getvalue()) + self.assertNotIn("Traceback", stderr.getvalue()) + + +if __name__ == "__main__": + unittest.main() From e442980abc8fb0f25a06951cd333aae88b5d907d Mon Sep 17 00:00:00 2001 From: HackingGate Date: Fri, 16 Oct 2026 22:08:16 +0900 Subject: [PATCH 2/4] Keep source authority independent of issue-tracker state Remove design_age.py and test_design_age.py, their scheduled workflow, and the adr-retired policy introduced from a superseded issue checklist. Remove retired_adr_paths_are_refused_by_the_repository_policy with that rule. The versioned implementation, tests and current-state documentation define the delivered system. Design issues are optional proposal and discussion records; their labels and age cannot change a revision's behavior or verdict. Accepted contracts and maintenance rationale belong in the repository. --- .github/workflows/design-age.yml | 26 ----- AGENTS.md | 1 - CONTRIBUTING.md | 35 +++--- REVIEW.md | 1 - policy/principles.toml | 6 - policy/upheld.toml | 4 - scripts/design_age.py | 162 --------------------------- tests/scan_cli.rs | 47 -------- tests/test_design_age.py | 183 ------------------------------- 9 files changed, 16 insertions(+), 449 deletions(-) delete mode 100644 .github/workflows/design-age.yml delete mode 100644 scripts/design_age.py delete mode 100644 tests/test_design_age.py diff --git a/.github/workflows/design-age.yml b/.github/workflows/design-age.yml deleted file mode 100644 index e6a98c4..0000000 --- a/.github/workflows/design-age.yml +++ /dev/null @@ -1,26 +0,0 @@ -name: Design record age - -on: - issues: - types: [opened, reopened, labeled, unlabeled] - schedule: - - cron: '17 9 * * *' - workflow_dispatch: - -permissions: - contents: read - issues: read - -jobs: - ruling-window: - name: Open Design records have a ruling - runs-on: ubuntu-slim - steps: - - uses: actions/checkout@v7 - with: - persist-credentials: false - - - name: Check ruling labels and pending age - env: - GITHUB_TOKEN: ${{ github.token }} - run: python3 scripts/design_age.py diff --git a/AGENTS.md b/AGENTS.md index 07f4f32..f46f10f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -8,7 +8,6 @@ Do NOT re-enforce the static rules. They run on every change and they run first; repeating their findings costs a reviewer's attention and buys a second opinion nobody asked for. The rules already active here are: -- `adr-retired` - `catalog-reference-current` - `catalog-tests` - `catalog-validate` diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3d2d2b8..56db717 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -75,32 +75,29 @@ the policy file, and the two forms the note names are checked as well. See ## Where a decision goes -A change that needs a decision is a GitHub issue in the -[Design record](.github/ISSUE_TEMPLATE/design.md) shape before a pull request -exists: the decision in the body, and the research in a comment that carries -the version, date and evidence of each claim. A Design record carries exactly +A proposal that benefits from design discussion can use a GitHub issue in the +[Design record](.github/ISSUE_TEMPLATE/design.md) shape: the proposed decision +in the body, and the research in a comment that carries the version, date and +evidence of each claim. A Design record carries exactly one ruling label. The template files it with `ruling:pending`, and the ruling is recorded by replacing that label with `ruling:accepted`, `ruling:rejected` or `ruling:deferred`. -The read-only Design record workflow checks open records when issues open, -reopen or change labels, and daily. Each open record must have exactly one -recognized ruling label; `ruling:pending` is overdue at 30 days from creation. -Run `python3 scripts/design_age.py` with `GITHUB_REPOSITORY` and `GH_TOKEN` set -to check manually. Rule the record or mark it `ruling:deferred` and record a -revisit date in the issue. API failures fail the check rather than reporting -that records are ruled. - The repository receives only what runs, plus [REFERENCE.md](docs/REFERENCE.md), which stays the normative statement of what uphold does now. Tracked files carry no issue numbers, dates or status lines; -those stay in the Design record that rules the change. - -Decisions live in Design record issues. [DESIGN.md](docs/DESIGN.md) explains -the current architecture; [REFERENCE.md](docs/REFERENCE.md) defines its -behavior. Retired decision files remain available in Git history. The `adr-retired` -path rule refuses every tracked file under `docs/adr`; file a Design record -for a new decision. +those stay in the tracker and Git history. + +The versioned source tree is authoritative for the delivered system: its +implementation, tests, [DESIGN.md](docs/DESIGN.md) and +[REFERENCE.md](docs/REFERENCE.md) describe the accepted architecture and +behavior together. Design record issues hold proposals, discussion and +historical context. An accepted proposal changes the delivered system only +through a reviewed repository change; an issue's status or age does not change +what a revision implements or whether it passes its checks. Keep the accepted +contract and the rationale needed to maintain it in the repository so a reader +can understand the system without consulting the issue tracker. Retired +decision files remain available in Git history. ## Working on the engine diff --git a/REVIEW.md b/REVIEW.md index 07f4f32..f46f10f 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -8,7 +8,6 @@ Do NOT re-enforce the static rules. They run on every change and they run first; repeating their findings costs a reviewer's attention and buys a second opinion nobody asked for. The rules already active here are: -- `adr-retired` - `catalog-reference-current` - `catalog-tests` - `catalog-validate` diff --git a/policy/principles.toml b/policy/principles.toml index 27c82fc..846ba93 100644 --- a/policy/principles.toml +++ b/policy/principles.toml @@ -130,12 +130,6 @@ sets = [ allowed_scripts = ["Latin", "Han", "Hiragana", "Katakana"] files.include = ["tests", "src/guard/unicode.rs"] -[rule.adr-retired] -message = "file a Design record issue (label design); docs/adr is retired." -path_regexp = '^docs/adr/' -files.include = ["."] -files.glob = ["docs/adr/**"] - # `workflow-declares-permissions` used to stand here, as the one instance of the # `require_regexp` kind anywhere -- a rule kind with no instance is # indistinguishable from a rule kind that does not work. It is in diff --git a/policy/upheld.toml b/policy/upheld.toml index 0a03042..493cb29 100644 --- a/policy/upheld.toml +++ b/policy/upheld.toml @@ -12,10 +12,6 @@ # listed here. They live in prose, where a reader can weigh them, rather than in # a config file that would have no condition on which to raise them. -[[enforce]] -principle = "single-authoritative-source" -rule = "adr-retired" - [[enforce]] principle = "explicit-unknown" rule = "catalog-tests" diff --git a/scripts/design_age.py b/scripts/design_age.py deleted file mode 100644 index 3a1e3a5..0000000 --- a/scripts/design_age.py +++ /dev/null @@ -1,162 +0,0 @@ -#!/usr/bin/env python3 -"""Report open Design records that have stayed pending past the ruling window.""" - -from __future__ import annotations - -import json -import os -import re -import sys -from datetime import UTC, datetime, timedelta -from urllib.error import HTTPError, URLError -from urllib.parse import urlencode -from urllib.request import Request, urlopen - -RULINGS = {"ruling:pending", "ruling:accepted", "ruling:rejected", "ruling:deferred"} -DESIGN_LABEL = "design" -RULING_WINDOW_DAYS = 30 -PAGE_SIZE = 100 - - -def _label_names(issue: dict) -> list[str]: - labels = issue["labels"] - if not isinstance(labels, list): - raise ValueError("GitHub issue labels must be a list") - names = [] - for label in labels: - if isinstance(label, str): - names.append(label) - elif isinstance(label, dict) and isinstance(label.get("name"), str): - names.append(label["name"]) - else: - raise ValueError("GitHub issue contains a malformed label") - return names - - -def _created_at(issue: dict) -> datetime: - created = issue["created_at"] - if not isinstance(created, str): - raise ValueError("GitHub issue created_at must be a timestamp") - parsed = datetime.fromisoformat(created.replace("Z", "+00:00")) - if parsed.tzinfo is None or parsed.utcoffset() is None: - raise ValueError("GitHub issue created_at must include a timezone") - return parsed - - -def audit( - issues: list[dict], now: datetime, window_days: int = RULING_WINDOW_DAYS -) -> tuple[list[tuple[dict, int]], list[str]]: - """Return overdue pending records and malformed design-record labels.""" - overdue: list[tuple[dict, int]] = [] - errors: list[str] = [] - - for issue in issues: - if not isinstance(issue, dict): - raise ValueError("GitHub issues API returned a malformed issue") - labels = _label_names(issue) - if DESIGN_LABEL not in labels or issue.get("state", "open") != "open": - continue - # The issues endpoint also returns pull requests. They are not design - # records even if somebody copies the label onto one. - if "pull_request" in issue: - continue - rulings = [label for label in labels if label.startswith("ruling:")] - number = issue.get("number", "?") - title = issue.get("title", "(untitled)") - if len(rulings) != 1 or rulings[0] not in RULINGS: - errors.append( - f"#{number} {title}: expected exactly one of " - f"{', '.join(sorted(RULINGS))}; found " - f"{', '.join(rulings) if rulings else 'no ruling label'}." - ) - continue - if rulings[0] != "ruling:pending": - continue - age = now - _created_at(issue) - age_days = age.days - if age >= timedelta(days=window_days): - overdue.append((issue, age_days)) - - overdue.sort(key=lambda item: (item[0].get("number", 0), item[0].get("title", ""))) - return overdue, errors - - -def fetch_open_design_issues(repository: str, token: str) -> list[dict]: - if not re.fullmatch(r"[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+", repository) or any( - part in {".", ".."} for part in repository.split("/") - ): - raise ValueError("GITHUB_REPOSITORY must have the form owner/repository") - issues: list[dict] = [] - page = 1 - while True: - query = urlencode( - { - "state": "open", - "labels": DESIGN_LABEL, - "per_page": PAGE_SIZE, - "page": page, - } - ) - request = Request( - f"https://api.github.com/repos/{repository}/issues?{query}", - headers={ - "Accept": "application/vnd.github+json", - "Authorization": f"Bearer {token}", - "X-GitHub-Api-Version": "2022-11-28", - "User-Agent": "uphold-design-age-check", - }, - ) - with urlopen(request, timeout=20) as response: - batch = json.load(response) - if not isinstance(batch, list): - raise ValueError("GitHub issues API returned a non-list response") - for issue in batch: - if not isinstance(issue, dict) or DESIGN_LABEL not in _label_names(issue): - raise ValueError( - "GitHub issues API returned an item without the design label" - ) - issues.extend(batch) - if len(batch) < PAGE_SIZE: - return issues - page += 1 - - -def main() -> int: - repository = os.environ.get("GITHUB_REPOSITORY", "") - token = os.environ.get("GH_TOKEN") or os.environ.get("GITHUB_TOKEN") - if not token: - print("set GH_TOKEN or GITHUB_TOKEN to read Design records", file=sys.stderr) - return 2 - try: - issues = fetch_open_design_issues(repository, token) - overdue, errors = audit(issues, datetime.now(UTC)) - except ( - HTTPError, - URLError, - TimeoutError, - ValueError, - KeyError, - TypeError, - AttributeError, - json.JSONDecodeError, - ) as error: - print(f"could not audit Design records: {error}", file=sys.stderr) - return 2 - - if not overdue and not errors: - print(f"Design records are ruled within {RULING_WINDOW_DAYS} days.") - return 0 - for message in errors: - print(message, file=sys.stderr) - for issue, age_days in overdue: - print( - f"#{issue['number']} {issue['title']}: ruling:pending for " - f"{age_days} days (limit {RULING_WINDOW_DAYS}); rule it or label it " - "ruling:deferred.", - file=sys.stderr, - ) - return 1 - - -if __name__ == "__main__": - raise SystemExit(main()) diff --git a/tests/scan_cli.rs b/tests/scan_cli.rs index e1aa8a9..9bf7d4d 100644 --- a/tests/scan_cli.rs +++ b/tests/scan_cli.rs @@ -175,53 +175,6 @@ fn a_path_rule_matches_the_path_and_not_the_content() { assert!(!text.contains("notes.md"), "{text}"); } -#[test] -fn retired_adr_paths_are_refused_by_the_repository_policy() { - let source: toml::Value = toml::from_str(include_str!("../policy/principles.toml")).unwrap(); - let mut policy = toml::Table::new(); - let mut rules = toml::Table::new(); - rules.insert( - "adr-retired".into(), - source - .get("rule") - .unwrap() - .get("adr-retired") - .unwrap() - .clone(), - ); - policy.insert("rule".into(), toml::Value::Table(rules)); - let root = workspace(); - write( - &root, - "policy/principles.toml", - &toml::to_string(&policy).unwrap(), - ); - write(&root, "docs/DESIGN.md", "Current architecture.\n"); - let empty = scan(&root); - assert_eq!(code(&empty), 0, "{}", stderr(&empty)); - for path in [ - "docs/adr/0001-old.md", - "docs/adr/0013-new.md", - "docs/adr/nested/record.md", - ] { - write(&root, path, "A decision.\n"); - let output = scan(&root); - assert_eq!(code(&output), 1, "{}", stderr(&output)); - assert!( - stderr(&output).contains("adr-retired"), - "{}", - stderr(&output) - ); - assert!(stderr(&output).contains(path), "{}", stderr(&output)); - assert!( - stderr(&output).contains("file a Design record"), - "{}", - stderr(&output) - ); - std::fs::remove_file(root.join(path)).unwrap(); - } -} - #[test] fn a_require_rule_fails_on_the_file_that_is_missing_the_pattern() { let root = workspace(); diff --git a/tests/test_design_age.py b/tests/test_design_age.py deleted file mode 100644 index 3dd9bac..0000000 --- a/tests/test_design_age.py +++ /dev/null @@ -1,183 +0,0 @@ -"""The issue check shares this predicate between local runs and scheduled CI.""" - -from __future__ import annotations - -import io -import json -import sys -import unittest -from datetime import UTC, datetime, timedelta -from pathlib import Path -from unittest.mock import patch -from urllib.error import URLError - -ROOT = Path(__file__).resolve().parents[1] -sys.path.insert(0, str(ROOT / "scripts")) - -import design_age # noqa: E402 - -NOW = datetime(2026, 10, 15, 12, 0, tzinfo=UTC) - - -def issue(number: int, ruling: str, age: timedelta, **extra: object) -> dict: - value = { - "number": number, - "title": f"Design record {number}", - "state": "open", - "created_at": (NOW - age).isoformat().replace("+00:00", "Z"), - "labels": [ - {"name": "design"}, - {"name": ruling}, - ], - } - value.update(extra) - return value - - -class DesignAge(unittest.TestCase): - def test_pending_is_due_at_thirty_days_but_not_one_second_before(self): - before = issue(1, "ruling:pending", timedelta(days=30) - timedelta(seconds=1)) - boundary = issue(2, "ruling:pending", timedelta(days=30)) - - overdue, errors = design_age.audit([before, boundary], NOW) - - self.assertEqual([record["number"] for record, _ in overdue], [2]) - self.assertEqual(overdue[0][1], 30) - self.assertEqual(errors, []) - - def test_each_final_ruling_closes_the_age_gate(self): - issues = [ - issue(number, f"ruling:{status}", timedelta(days=90)) - for number, status in enumerate(("accepted", "rejected", "deferred"), 1) - ] - - overdue, errors = design_age.audit(issues, NOW) - - self.assertEqual(overdue, []) - self.assertEqual(errors, []) - - def test_invalid_or_missing_ruling_label_is_reported_immediately(self): - missing = issue(1, "ruling:pending", timedelta(days=1)) - missing["labels"] = [{"name": "design"}] - multiple = issue(2, "ruling:pending", timedelta(days=1)) - multiple["labels"].append({"name": "ruling:accepted"}) - unknown = issue(3, "ruling:withdrawn", timedelta(days=1)) - - overdue, errors = design_age.audit([missing, multiple, unknown], NOW) - - self.assertEqual(overdue, []) - self.assertEqual(len(errors), 3) - self.assertIn("no ruling label", errors[0]) - self.assertIn("ruling:pending, ruling:accepted", errors[1]) - self.assertIn("ruling:withdrawn", errors[2]) - - def test_closed_non_design_and_pull_request_items_are_ignored(self): - closed = issue(1, "ruling:pending", timedelta(days=90), state="closed") - other = issue(2, "ruling:pending", timedelta(days=90)) - other["labels"] = [{"name": "ruling:pending"}] - pull_request = issue(3, "ruling:pending", timedelta(days=90), pull_request={}) - - overdue, errors = design_age.audit([closed, other, pull_request], NOW) - - self.assertEqual(overdue, []) - self.assertEqual(errors, []) - - def test_fetch_reads_a_second_page_before_returning(self): - first_page = [ - issue(number, "ruling:accepted", timedelta(days=1)) - for number in range(1, 101) - ] - second_page = [issue(101, "ruling:pending", timedelta(days=31))] - responses = [ - io.BytesIO(json.dumps(batch).encode()) - for batch in (first_page, second_page) - ] - - with patch.object(design_age, "urlopen", side_effect=responses) as open_url: - records = design_age.fetch_open_design_issues("owner/repo", "token") - - self.assertEqual(len(records), 101) - self.assertIn("page=1", open_url.call_args_list[0].args[0].full_url) - self.assertIn("page=2", open_url.call_args_list[1].args[0].full_url) - overdue, errors = design_age.audit(records, NOW) - self.assertEqual([record["number"] for record, _ in overdue], [101]) - self.assertEqual(errors, []) - - def test_main_reports_overdue_record_and_success_with_distinct_exit_codes(self): - for records, expected in [ - ([], 0), - ([issue(42, "ruling:pending", timedelta(days=31))], 1), - ]: - with ( - self.subTest(expected=expected), - patch.dict( - "os.environ", - {"GITHUB_REPOSITORY": "owner/repo", "GH_TOKEN": "token"}, - ), - patch.object( - design_age, "fetch_open_design_issues", return_value=records - ), - patch("sys.stdout", new_callable=io.StringIO) as stdout, - patch("sys.stderr", new_callable=io.StringIO) as stderr, - ): - self.assertEqual(design_age.main(), expected) - if expected: - self.assertIn("#42", stderr.getvalue()) - self.assertIn("ruling:deferred", stderr.getvalue()) - else: - self.assertIn("within 30 days", stdout.getvalue()) - - def test_fetch_refuses_invalid_repository_before_using_credentials(self): - with ( - patch.object(design_age, "urlopen") as open_url, - self.assertRaisesRegex(ValueError, "owner/repository"), - ): - design_age.fetch_open_design_issues("../elsewhere", "token") - open_url.assert_not_called() - - def test_fetch_rejects_api_response_without_the_requested_label(self): - response = io.BytesIO(json.dumps([{"labels": []}]).encode()) - with ( - patch.object(design_age, "urlopen", return_value=response), - self.assertRaisesRegex(ValueError, "without the design label"), - ): - design_age.fetch_open_design_issues("owner/repo", "token") - - def test_main_reports_api_read_failure_as_exit_two(self): - with ( - patch.dict( - "os.environ", - {"GITHUB_REPOSITORY": "owner/repo", "GITHUB_TOKEN": "token"}, - ), - patch.object( - design_age, "fetch_open_design_issues", side_effect=URLError("offline") - ), - patch("sys.stderr", new_callable=io.StringIO) as stderr, - ): - result = design_age.main() - - self.assertEqual(result, 2) - self.assertIn("could not audit Design records", stderr.getvalue()) - - def test_malformed_issue_response_fails_closed_without_a_traceback(self): - malformed = issue(1, "ruling:pending", timedelta(days=1)) - malformed["created_at"] = "2026-10-01T12:00:00" - with ( - patch.dict( - "os.environ", - {"GITHUB_REPOSITORY": "owner/repo", "GITHUB_TOKEN": "token"}, - ), - patch.object( - design_age, "fetch_open_design_issues", return_value=[malformed] - ), - patch("sys.stderr", new_callable=io.StringIO) as stderr, - ): - result = design_age.main() - - self.assertEqual(result, 2) - self.assertIn("must include a timezone", stderr.getvalue()) - self.assertNotIn("Traceback", stderr.getvalue()) - - -if __name__ == "__main__": - unittest.main() From 798717139143ff42c90edd1df35ae68edb74d9bf Mon Sep 17 00:00:00 2001 From: HackingGate Date: Fri, 16 Oct 2026 22:10:01 +0900 Subject: [PATCH 3/4] Give remote policy fixtures their own Git identity --- tests/remote_policy_cli.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/remote_policy_cli.rs b/tests/remote_policy_cli.rs index 3657e72..4ecaf22 100644 --- a/tests/remote_policy_cli.rs +++ b/tests/remote_policy_cli.rs @@ -25,6 +25,8 @@ impl Fixture { let source = root.join("source"); std::fs::create_dir_all(source.join("policy")).unwrap(); support::git(&source, &["init", "-q", "-b", "main"]); + support::git(&source, &["config", "user.name", "Test"]); + support::git(&source, &["config", "user.email", "test@example.test"]); std::fs::write(source.join("policy/org.toml"), text).unwrap(); support::git(&source, &["add", "."]); support::git(&source, &["commit", "-qm", "policy"]); From b6ea7d39774373d93fb8608e83a6d5fe53faeb38 Mon Sep 17 00:00:00 2001 From: HackingGate Date: Fri, 16 Oct 2026 22:24:51 +0900 Subject: [PATCH 4/4] Keep remote policy fetches noninteractive and group rule provenance --- CONTRIBUTING.md | 7 ++-- docs/REFERENCE.md | 5 +-- src/main.rs | 6 ++-- src/remote_policy.rs | 23 +++++++++---- tests/remote_policy_cli.rs | 67 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 94 insertions(+), 14 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 56db717..a2d3686 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -83,9 +83,10 @@ one ruling label. The template files it with `ruling:pending`, and the ruling is recorded by replacing that label with `ruling:accepted`, `ruling:rejected` or `ruling:deferred`. -The repository receives only what runs, plus -[REFERENCE.md](docs/REFERENCE.md), which stays the normative statement of what -uphold does now. Tracked files carry no issue numbers, dates or status lines; +The repository carries runnable content and current-state documentation, +including [DESIGN.md](docs/DESIGN.md) for the architecture and +[REFERENCE.md](docs/REFERENCE.md) for normative behavior. +Tracked files carry no issue numbers, dates or status lines; those stay in the tracker and Git history. The versioned source tree is authoritative for the delivered system: its diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index 223b20e..16a05b0 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -859,8 +859,9 @@ content and invalid policy all fail with exit 2. Nothing falls back to the repository's latest revision. Each load fetches into a fresh temporary bare repository and removes it afterwards; network access and Git credentials for private sources must therefore be available wherever the policy runs. Git's -normal credential helpers and URL rewrites apply. No checkout, submodules or -source repository hooks are run. +normal credential helpers and URL rewrites apply. Terminal and askpass +credential prompts are disabled; HTTP transfers below one byte per second +for 30 seconds abort. No checkout, submodules or source repository hooks run. Only rules are adopted. Remote files may not declare their own `[inherit]`, `[override]` or `[[shim]]`: sources and shims remain explicit decisions in the diff --git a/src/main.rs b/src/main.rs index 6db6e06..dc31399 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1033,9 +1033,6 @@ fn effective_rules_command(as_json: bool) -> Result { } else { hooks.join(", ") }; - if let config::Origin::Remote(remote) = &rule.origin { - println!(" source: {}", remote.label()); - } // Which parts of an inherited rule this policy changed, so a // reworded message or a widened selection reads as local rather // than as the set's. @@ -1053,6 +1050,9 @@ fn effective_rules_command(as_json: bool) -> Result { rule.overridden.join(", ") ); } + if let config::Origin::Remote(remote) = &rule.origin { + println!(" source: {}", remote.label()); + } } return Ok(Exit::Clean); } diff --git a/src/remote_policy.rs b/src/remote_policy.rs index af2060c..c17391a 100644 --- a/src/remote_policy.rs +++ b/src/remote_policy.rs @@ -83,9 +83,10 @@ impl Remote { // No checkout, submodule recursion, hooks or repository-supplied config. // Git's normal credentials and URL rewrites remain available, including // credentials for an organization's private policy repository. - if crate::git::try_run_elsewhere( - &directory.0, - &[ + let mut fetch = crate::shim::inner_tool("git"); + fetch + .current_dir(&directory.0) + .args([ "-c", "fetch.recurseSubmodules=false", "fetch", @@ -95,9 +96,19 @@ impl Remote { "--", &url, &self.rev, - ], - )? - .is_none() + ]) + // Hook and agent invocations cannot answer credential prompts. + .env("GIT_TERMINAL_PROMPT", "0") + .env("GIT_ASKPASS", "") + .env("GIT_HTTP_LOW_SPEED_LIMIT", "1") + .env("GIT_HTTP_LOW_SPEED_TIME", "30") + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()); + if !crate::git::elsewhere(&mut fetch) + .status() + .map_err(|_| fail())? + .success() { return Err(fail()); } diff --git a/tests/remote_policy_cli.rs b/tests/remote_policy_cli.rs index 4ecaf22..fb742de 100644 --- a/tests/remote_policy_cli.rs +++ b/tests/remote_policy_cli.rs @@ -97,6 +97,61 @@ impl Fixture { const RULE: &str = "[rule.org-rule]\nregexp = 'ORG_BANNED'\nmessage = 'organization rule'\n[rule.org-rule.files]\nexclude = ['policy/**']\n"; +#[cfg(unix)] +#[test] +fn an_authentication_failure_never_launches_an_interactive_prompt() { + use std::io::{BufRead as _, Write as _}; + use std::os::unix::fs::PermissionsExt as _; + + let fixture = Fixture::new(RULE); + let askpass = fixture.root.join("askpass"); + std::fs::write( + &askpass, + "#!/bin/sh\nprintf asked > \"$0.called\"\nexit 1\n", + ) + .unwrap(); + std::fs::set_permissions(&askpass, std::fs::Permissions::from_mode(0o755)).unwrap(); + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let address = listener.local_addr().unwrap(); + std::fs::write(&fixture.config, format!( + "[url \"http://{address}/policy.git\"]\n insteadOf = https://github.com/example/policy.git\n" + )).unwrap(); + support::git( + &fixture.source, + &[ + "config", + "--file", + fixture.config.to_str().unwrap(), + "core.askPass", + askpass.to_str().unwrap(), + ], + ); + let server = std::thread::spawn(move || { + let (mut stream, _) = listener.accept().unwrap(); + stream + .set_read_timeout(Some(std::time::Duration::from_secs(5))) + .unwrap(); + let mut reader = std::io::BufReader::new(&stream); + let mut line = String::new(); + while reader.read_line(&mut line).unwrap() != 0 { + if line == "\r\n" { + break; + } + line.clear(); + } + stream.write_all(b"HTTP/1.1 401 Unauthorized\r\nWWW-Authenticate: Basic realm=\"fixture\"\r\nContent-Length: 0\r\nConnection: close\r\n\r\n").unwrap(); + }); + let output = fixture.run(&["rules", "--effective"]); + server.join().unwrap(); + assert_eq!( + output.status.code(), + Some(2), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + assert!(!fixture.root.join("askpass.called").exists()); +} + #[test] fn remote_rules_run_keep_provenance_and_accept_overrides() { let fixture = Fixture::new(RULE); @@ -118,6 +173,18 @@ fn remote_rules_run_keep_provenance_and_accept_overrides() { .unwrap() .contains(&serde_json::json!("message")) ); + let human = fixture.run(&["rules", "--effective"]); + assert_eq!(human.status.code(), Some(0)); + let listing = String::from_utf8(human.stdout).unwrap(); + let lines: Vec<_> = listing.lines().collect(); + let rule_line = lines + .iter() + .position(|line| line.starts_with(" org-rule (")) + .unwrap(); + assert_eq!( + lines.get(rule_line + 1).copied(), + Some(format!(" source: example/policy@{}:policy/org.toml", fixture.rev).as_str()) + ); std::fs::write(fixture.root.join("consumer/skip.txt"), "ORG_BANNED").unwrap(); support::git(&fixture.root.join("consumer"), &["add", "."]); let clean = fixture.run(&["scan"]);