diff --git a/.github/workflows/public-intake.yml b/.github/workflows/public-intake.yml index 401ee3228..383e409ff 100644 --- a/.github/workflows/public-intake.yml +++ b/.github/workflows/public-intake.yml @@ -11,6 +11,11 @@ permissions: jobs: public-intake: name: public-intake + permissions: + contents: read + pull-requests: read + # Only for the neutral "not evaluated" check run. Submitted code is never executed. + checks: write # Skip metadata edits only for terminal merged PRs. Reopenable PRs must # still pass the live checker, including closure and head-drift checks. if: ${{ !(github.event.pull_request.state == 'closed' && toJSON(github.event.pull_request.merged) == 'true') }} @@ -28,4 +33,20 @@ jobs: - name: Verify intake from API bytes without checking out the PR env: GH_TOKEN: ${{ github.token }} - run: python3 -B -I tools/public_intake_check.py + PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + set +e + python3 -B -I tools/public_intake_check.py + code=$? + set -e + # Actions cannot mark this job neutral. Skipping the job can count as a + # passing required check, and a zero status would make public-intake + # green without package verification. Record a separate neutral check, + # then exit 78 so this job is failure, not success. A failed post + # still reaches that same nonzero status. + if [ "$code" -eq 78 ]; then + echo "not evaluated: merged before check" + python3 -B -I tools/public_intake_check.py --publish-not-evaluated-check || true + exit 78 + fi + exit "$code" diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index df6b94c82..1fd09dbd5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -111,7 +111,7 @@ HTML, SVG, archives, executable files, symlinks and submodules are refused here. For code or a notebook proposal, describe it in an issue; a maintainer can sponsor a separate engineering PR. Do not encode a refused file inside an allowed format. -`public-intake` runs trusted default-branch code with a read-only token. It reads +`public-intake` runs trusted default-branch code; the job's token is read-only for contents and pull requests, plus `checks: write` used solely to post the neutral 'not evaluated: merged before check' check run, and submitted code is never checked out or executed. It reads PR metadata and blobs through the GitHub API, checks complete path lists and source identities, and fails on missing information or a changing PR. It never checks out or executes submitted code. The credential scan catches a small documented diff --git a/tests/test_public_intake.py b/tests/test_public_intake.py index 4730501aa..f274f42a5 100644 --- a/tests/test_public_intake.py +++ b/tests/test_public_intake.py @@ -8,7 +8,9 @@ import io import os import struct +import subprocess import tempfile +import textwrap from unittest import mock from pathlib import Path import unicodedata @@ -360,6 +362,175 @@ def test_non_open_pr_and_wrong_base_are_refused(self): self.pr['base']['ref'] = 'other' self.reject('targets main') + def test_event_queued_before_merge_is_not_a_rejection(self): + # Observed on main (runs 37139813163, 37843755359): an event for the open PR + # ran seconds after the merge and failed with 'PR is not open'. + self.pr['state'] = 'closed' + self.pr['merged'] = True + result = self.run_check() + self.assertEqual(result['result'], 'SKIPPED_MERGED_BEFORE_CHECK') + self.assertEqual(result['lane'], 'not-evaluated') + self.assertEqual(self.api.calls, [f'/repos/{REPO}/pulls/7']) + + def test_merged_pr_with_different_head_or_unmerged_close_still_refused(self): + self.pr['state'] = 'closed' + self.pr['merged'] = False + self.reject('not open') + self.pr['merged'] = True + self.pr['head']['sha'] = 'e' * 40 + self.reject('not open') + self.pr['head']['sha'] = HEAD + self.pr['merged'] = 'true' # only the JSON boolean counts + self.reject('not open') + + def test_merged_before_check_exits_78_and_is_not_success(self): + self.pr['state'] = 'closed' + self.pr['merged'] = True + with tempfile.TemporaryDirectory() as folder: + event_file = Path(folder) / 'event.json' + event_file.write_text(json.dumps(self.event)) + env = {'GITHUB_EVENT_NAME': 'pull_request_target', 'GITHUB_REPOSITORY': REPO, 'GH_TOKEN': 'test-only'} + with mock.patch.dict(os.environ, env), mock.patch.object(intake, 'API', return_value=self.api), \ + mock.patch.object(intake, 'publish_not_evaluated_check') as publish, \ + contextlib.redirect_stdout(io.StringIO()) as output: + code = intake.main(['--event', str(event_file)]) + publish.assert_not_called() + self.assertEqual(code, 78) + self.assertEqual(code, intake.EXIT_NOT_EVALUATED) + self.assertNotEqual(code, 0) + payload = json.loads(output.getvalue()) + self.assertEqual(payload['result'], 'SKIPPED_MERGED_BEFORE_CHECK') + self.assertEqual(payload['lane'], 'not-evaluated') + self.assertNotIn('success', output.getvalue()) + + def test_closed_unmerged_still_exits_1(self): + self.pr['state'] = 'closed' + self.pr['merged'] = False + with tempfile.TemporaryDirectory() as folder: + event_file = Path(folder) / 'event.json' + event_file.write_text(json.dumps(self.event)) + env = {'GITHUB_EVENT_NAME': 'pull_request_target', 'GITHUB_REPOSITORY': REPO, 'GH_TOKEN': 'test-only'} + with mock.patch.dict(os.environ, env), mock.patch.object(intake, 'API', return_value=self.api), \ + contextlib.redirect_stdout(io.StringIO()) as output: + code = intake.main(['--event', str(event_file)]) + self.assertEqual(code, 1) + self.assertEqual(json.loads(output.getvalue())['reason'], 'PR is not open') + + def test_not_evaluated_check_is_neutral_and_never_success(self): + class Response: + def __init__(self, payload): + self.payload = payload + + def read(self, limit): + return self.payload[:limit] + + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + class Opener: + def __init__(self, payload): + self.payload = payload + self.requests = [] + + def open(self, request, timeout=30): + self.requests.append(request) + return Response(self.payload) + + body = intake.not_evaluated_check_body(HEAD) + self.assertEqual(body['conclusion'], 'neutral') + self.assertNotEqual(body['conclusion'], 'success') + self.assertEqual(body['name'], 'public-intake (not evaluated: merged before check)') + self.assertNotEqual(body['name'], 'public-intake') + self.assertEqual(body['output']['title'], 'not evaluated: merged before check') + neutral = json.dumps({'conclusion': 'neutral', 'name': intake.NOT_EVALUATED_CHECK_NAME}).encode() + opener = Opener(neutral) + recorded = intake.publish_not_evaluated_check(REPO, HEAD, 'test-only', opener) + self.assertEqual(recorded['conclusion'], 'neutral') + self.assertNotEqual(recorded['conclusion'], 'success') + request = opener.requests[0] + self.assertEqual(request.full_url, 'https://api.github.com/repos/d6g8k5htny-coder/main/check-runs') + self.assertEqual(request.get_method(), 'POST') + self.assertEqual(request.get_header('Authorization'), 'Bearer test-only') + sent = json.loads(request.data.decode()) + self.assertEqual(sent['conclusion'], 'neutral') + self.assertNotEqual(sent['conclusion'], 'success') + self.assertNotEqual(sent['name'], 'public-intake') + self.assertNotIn('test-only', request.data.decode()) + refused = Opener(json.dumps({'conclusion': 'success', 'name': intake.NOT_EVALUATED_CHECK_NAME}).encode()) + with self.assertRaisesRegex(ValueError, 'did not record neutral'): + intake.publish_not_evaluated_check(REPO, HEAD, 'test-only', refused) + self.assertEqual(json.loads(refused.requests[0].data.decode())['conclusion'], 'neutral') + untouched = Opener(neutral) + with self.assertRaisesRegex(ValueError, 'invalid head sha'): + intake.publish_not_evaluated_check(REPO, 'not-a-sha', 'test-only', untouched) + with self.assertRaisesRegex(ValueError, 'wrong repository'): + intake.publish_not_evaluated_check('other/repo', HEAD, 'test-only', untouched) + self.assertEqual(untouched.requests, []) + + def test_publish_flag_failure_is_not_a_successful_intake(self): + env = {'GITHUB_REPOSITORY': REPO, 'PR_HEAD_SHA': 'not-a-sha', 'GH_TOKEN': 'test-only-secret'} + with mock.patch.dict(os.environ, env), contextlib.redirect_stdout(io.StringIO()) as output: + code = intake.main(['--publish-not-evaluated-check']) + self.assertEqual(code, 1) + self.assertNotEqual(code, 0) + self.assertIn('NOT_EVALUATED_CHECK_UNPUBLISHED', output.getvalue()) + self.assertNotIn('test-only-secret', output.getvalue()) + self.assertNotIn('success', output.getvalue()) + + def test_workflow_maps_exit_78_to_neutral_and_never_success(self): + workflow = (Path(__file__).resolve().parents[1] / '.github/workflows/public-intake.yml').read_text() + self.assertIn('checks: write', workflow) + self.assertIn('\n name: public-intake\n', workflow) + self.assertNotIn('continue-on-error', workflow) + self.assertNotIn('exit 0', workflow) + lines = [] + capture = False + for line in workflow.splitlines(): + if line.strip() == 'set +e': + capture = True + if capture: + lines.append(line) + if capture and line.strip() == 'exit "$code"': + break + script = textwrap.dedent('\n'.join(lines) + '\n') + self.assertIn('exit 78', script) + self.assertNotIn('exit 0', script) + publish_at = script.index('--publish-not-evaluated-check') + exit_78 = script.index('\n exit 78\n') + self.assertLess(script.index('if [ "$code" -eq 78 ]; then'), publish_at) + self.assertLess(publish_at, exit_78) + self.assertLess(exit_78, script.index('exit "$code"')) + with tempfile.TemporaryDirectory() as folder: + bin_dir = Path(folder) / 'bin' + bin_dir.mkdir() + log = Path(folder) / 'calls' + stub = bin_dir / 'python3' + stub.write_text('#!/bin/sh\nprintf "%s\\n" "$*" >> "$STUB_LOG"\n' + 'if [ "$4" = "--publish-not-evaluated-check" ]; then exit 0; fi\n' + 'exit "$STUB_CODE"\n') + stub.chmod(0o755) + env = dict(os.environ) + env['PATH'] = str(bin_dir) + os.pathsep + env.get('PATH', '') + env['STUB_LOG'] = str(log) + for stub_code, expected in ((0, 0), (1, 1), (78, 78), (2, 2)): + with self.subTest(stub_code=stub_code): + log.write_text('') + result = subprocess.run(['bash', '-c', script], env={**env, 'STUB_CODE': str(stub_code)}, + capture_output=True, text=True) + self.assertEqual(result.returncode, expected) + calls = log.read_text().splitlines() + if stub_code == 78: + self.assertNotEqual(result.returncode, 0) + self.assertIn('not evaluated: merged before check', result.stdout) + self.assertEqual(calls, ['-B -I tools/public_intake_check.py', + '-B -I tools/public_intake_check.py --publish-not-evaluated-check']) + else: + self.assertEqual(calls, ['-B -I tools/public_intake_check.py']) + self.assertNotIn('--publish-not-evaluated-check', log.read_text()) + def test_cli_pass_and_failure_are_distinct_without_network(self): with tempfile.TemporaryDirectory() as folder: event_file = Path(folder) / 'event.json' diff --git a/tools/public_intake_check.py b/tools/public_intake_check.py index b96674d4d..b554aad22 100644 --- a/tools/public_intake_check.py +++ b/tools/public_intake_check.py @@ -3,6 +3,7 @@ No submitted file is checked out, imported, rendered, or executed. The limited credential-pattern scan is not a guarantee that a submission contains no secret. A pass means packaging checks passed; scientific effect remains NONE. +A merged-before-check run did not verify the package: it exits 78, not 0. """ from __future__ import annotations @@ -30,6 +31,9 @@ MAX_PNG_SIDE = 16384 MAX_PNG_PIXELS = 1 << 24 HEX40 = re.compile(r'[0-9a-f]{40}\Z') +# Distinct from rejection (1). Nonzero so the required public-intake job cannot succeed. +EXIT_NOT_EVALUATED = 78 +NOT_EVALUATED_CHECK_NAME = 'public-intake (not evaluated: merged before check)' HEX64 = re.compile(r'[0-9a-f]{64}\Z') REPOSITORY = re.compile(r'[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+\Z') SAFE_PATH = re.compile(r'[A-Za-z0-9_][A-Za-z0-9_. /-]*\Z') @@ -385,6 +389,18 @@ def check(event, api, repository): number = integer(event.get('number'), 1, 10000000, 'invalid PR number') pr = api.get(f'/repos/{repository}/pulls/{number}') require(pr.get('number') == number, 'PR number mismatch') + event_pr = event.get('pull_request') + if (isinstance(pr, dict) and pr.get('state') == 'closed' and pr.get('merged') is True + and isinstance(event_pr, dict) and isinstance(event_pr.get('head'), dict) + and isinstance(pr.get('head'), dict) and event_pr['head'].get('sha') == pr['head'].get('sha') + and isinstance(event_pr['head'].get('sha'), str) and HEX40.fullmatch(event_pr['head']['sha'])): + # Queued while the PR was open, executed after the merge. Package verification + # did not run. This does not prove an earlier successful intake of this head. + # main() returns EXIT_NOT_EVALUATED so the required job cannot record success. + # A closed but unmerged (reopenable) PR, or a head that differs from the event, + # still rejects. + return {'head': pr['head']['sha'], 'scientific_effect': 'NONE', 'lane': 'not-evaluated', + 'result': 'SKIPPED_MERGED_BEFORE_CHECK'} original = snapshot(pr, repository) head, base, head_repo, labels, count = original event_pr = event.get('pull_request') @@ -401,15 +417,71 @@ def check(event, api, repository): return result +def not_evaluated_check_body(head_sha): + """Neutral Checks API body. Not a successful public-intake result.""" + sha40(head_sha, 'invalid head sha') + require(NOT_EVALUATED_CHECK_NAME != 'public-intake', 'neutral check must not reuse the required check name') + body = { + 'name': NOT_EVALUATED_CHECK_NAME, + 'head_sha': head_sha, + 'status': 'completed', + 'conclusion': 'neutral', + 'output': { + 'title': 'not evaluated: merged before check', + 'summary': 'The pull request merged before public-intake verified the package. ' + 'Nothing was evaluated. This is not a successful intake result.', + }, + } + require(body['conclusion'] == 'neutral', 'not-evaluated check must stay neutral') + return body + + +def publish_not_evaluated_check(repository, head_sha, token, opener=None): + """POST one neutral check run. Refuses any recorded conclusion other than neutral.""" + require(repository == 'd6g8k5htny-coder/main', 'wrong repository') + require(isinstance(token, str) and token, 'read-only GitHub token missing') + body = not_evaluated_check_body(head_sha) + request = urllib.request.Request( + 'https://api.github.com/repos/' + repository + '/check-runs', + data=json.dumps(body).encode(), method='POST', headers={ + 'Authorization': 'Bearer ' + token, 'Accept': 'application/vnd.github+json', + 'Content-Type': 'application/json', 'X-GitHub-Api-Version': '2022-11-28', + 'User-Agent': 'public-intake-not-evaluated'}) + if opener is None: + opener = urllib.request.build_opener(NoRedirect()) + with opener.open(request, timeout=30) as response: + raw = response.read(MAX_RESPONSE_BYTES + 1) + require(len(raw) <= MAX_RESPONSE_BYTES, 'API response exceeds safe limit') + recorded = strict_json(raw) + require(isinstance(recorded, dict) and recorded.get('conclusion') == 'neutral', 'API did not record neutral') + require(recorded.get('name') == NOT_EVALUATED_CHECK_NAME, 'API check name mismatch') + return recorded + + def main(argv=None): parser = argparse.ArgumentParser(description=__doc__) parser.add_argument('--event', type=Path, default=Path(os.environ.get('GITHUB_EVENT_PATH', '/nonexistent-event'))) + parser.add_argument('--publish-not-evaluated-check', action='store_true') args = parser.parse_args(argv) + if args.publish_not_evaluated_check: + # Exit 0 here means only that the neutral check was recorded. The workflow + # still exits EXIT_NOT_EVALUATED, so the required job does not succeed. + try: + recorded = publish_not_evaluated_check( + os.environ.get('GITHUB_REPOSITORY', ''), os.environ.get('PR_HEAD_SHA', ''), os.environ.get('GH_TOKEN', '')) + print(json.dumps({'conclusion': recorded.get('conclusion'), 'name': recorded.get('name'), + 'result': 'NOT_EVALUATED_RECORDED'}, sort_keys=True)) + return 0 + except (ValueError, OSError, TypeError, KeyError, UnicodeError, RecursionError) as error: + print(json.dumps({'reason': str(error), 'result': 'NOT_EVALUATED_CHECK_UNPUBLISHED', 'scientific_effect': 'NONE'})) + return 1 try: require(os.environ.get('GITHUB_EVENT_NAME') == 'pull_request_target', 'CI requires trusted pull_request_target context') event = strict_json(args.event.read_bytes()) result = check(event, API(os.environ.get('GH_TOKEN', '')), os.environ.get('GITHUB_REPOSITORY', '')) print(json.dumps(result, sort_keys=True)) + if result.get('result') == 'SKIPPED_MERGED_BEFORE_CHECK': + return EXIT_NOT_EVALUATED return 0 except (ValueError, OSError, TypeError, KeyError, UnicodeError, RecursionError) as error: # Never print response bodies, submitted file contents, or credentials.