gortiz commented on code in PR #19502: URL: https://github.com/apache/pinot/pull/19502#discussion_r3957258242
########## .github/scripts/pr_flow/main.py: ########## @@ -0,0 +1,519 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Maintain a signed PR-description diagram using public API data, never PR code.""" + +import base64 +import hashlib +import hmac +import http.client +import json +import math +import os +from pathlib import Path +import re +import sys +import time +from urllib.parse import urlencode + +from model import DEFAULT_MODEL, ModelError, QuotaError, generate, render + +REPOSITORY = "apache/pinot" +POLICY = "pinot-pr-flow-v1" +START = "<!-- pinot-pr-flow:start -->" +END = "<!-- pinot-pr-flow:end -->" +CHECKBOX = "- [ ] Regenerate PR flow" +META_RE = re.compile(r"<!-- pinot-pr-flow:meta ([A-Za-z0-9_-]+) -->") +SIGNATURE_RE = re.compile(r"\n<!-- pinot-pr-flow:signature ([a-f0-9]{64}) -->") +MAX_EVIDENCE_BYTES = 180_000 +MAX_PATCH_BYTES = 24_000 +MAX_PR_FILES = 3_000 +MAX_SCAN_PRS = 200 + + +class FlowError(RuntimeError): + """A safe, operator-facing failure; never contains response bodies or tokens.""" + + +def packed(value): + return json.dumps(value, sort_keys=True, ensure_ascii=False, separators=(",", ":")) + + +def digest(value): + return hashlib.sha256(packed(value).encode()).hexdigest() + + +def number(value): + if not re.fullmatch(r"[1-9][0-9]{0,19}", str(value)): + raise FlowError("PR number must be a positive integer") + return int(value) + + +def boolean(value): + if value not in ("", "true", "false", None): + raise FlowError("Boolean options must be true or false") + return value == "true" + + +def sha(value): + if not isinstance(value, str) or not re.fullmatch(r"[a-f0-9]{40}", value): + raise FlowError("GitHub returned an invalid revision") + return value + + +class GitHub: + """Bounded GitHub-only transport with no redirects, proxy use, or shell calls.""" + + def __init__(self, token): + if not token or any(c in token for c in "\r\n"): + raise FlowError("GITHUB_TOKEN is required") + self.token = token + self.calls = 0 + self.viewer = None + self.history_metadata = {} + + def request(self, path, payload=None, graphql=False): + self.calls += 1 + if self.calls > 800 or not (path.startswith("/repos/apache/pinot/") or graphql and path == "/graphql"): + raise FlowError("GitHub request is outside the bounded repository scope") + connection = http.client.HTTPSConnection("api.github.com", timeout=60) + try: + connection.request("POST" if graphql else "PATCH" if payload is not None else "GET", path, + body=packed(payload).encode() if payload is not None else None, + headers={"Authorization": "Bearer " + self.token, + "Accept": "application/vnd.github+json", + "Content-Type": "application/json", + "X-GitHub-Api-Version": "2022-11-28", + "User-Agent": "apache-pinot-pr-flow"}) + response = connection.getresponse() + if response.status not in (200, 201): + raise FlowError(f"GitHub HTTP {response.status}; see workflow permissions or API limits") + raw = response.read(8_000_001) + if len(raw) > 8_000_000: + raise FlowError("GitHub response exceeded the size limit") + return json.loads(raw) + except (OSError, http.client.HTTPException, ValueError, RecursionError): + raise FlowError("GitHub request failed; no response body was logged") from None + finally: + connection.close() + + def get(self, suffix): + return self.request("/repos/" + REPOSITORY + "/" + suffix) + + def pr(self, pr_number): + return self.get("pulls/" + str(number(pr_number))) + + def update(self, pr_number, body): + return self.request("/repos/" + REPOSITORY + "/pulls/" + str(number(pr_number)), {"body": body}) + + def history(self, pr_number): + query = """query($number:Int!) { + viewer { login } + repository(owner:"apache", name:"pinot") { + pullRequest(number:$number) { + createdAt + includesCreatedEdit + userContentEdits(first:20) { + nodes { id editedAt diff editor { login } } + } + } + } + }""" + response = self.request("/graphql", {"query": query, "variables": {"number": number(pr_number)}}, + graphql=True) + if response.get("errors"): + raise FlowError("PR edit history is unavailable; preserving the description") + self.viewer = response["data"]["viewer"]["login"] + if not isinstance(self.viewer, str) or not self.viewer: + raise FlowError("Cannot verify the publication identity") + pull = response["data"]["repository"]["pullRequest"] + history = pull["userContentEdits"] + nodes = history["nodes"] + if not isinstance(nodes, list) or any(not isinstance(item, dict) or not isinstance(item.get("id"), str) + or "diff" not in item + or item["diff"] is not None and not isinstance(item["diff"], str) + for item in nodes): + raise FlowError("PR edit history cannot be audited; preserving the description") + if (not isinstance(pull.get("createdAt"), str) or not pull["createdAt"] + or type(pull.get("includesCreatedEdit")) is not bool): + raise FlowError("PR edit history metadata is unavailable; preserving the description") + self.history_metadata[number(pr_number)] = { + "created_at": pull["createdAt"], "includes_created_edit": pull["includesCreatedEdit"]} + return nodes + + def pages(self, suffix, maximum): + items = [] + separator = "&" if "?" in suffix else "?" + for page in range(1, maximum // 100 + 2): + batch = self.get(f"{suffix}{separator}per_page=100&page={page}") + if not isinstance(batch, list) or len(batch) > 100: + raise FlowError("GitHub returned an invalid paginated inventory") + items.extend(batch) + if len(items) > maximum: + raise FlowError("GitHub inventory exceeds the configured limit") + if len(batch) < 100: + return items + raise FlowError("GitHub inventory was not completely paginated") + + +def bounds(body): + if START not in body and END not in body: + return None + if body.count(START) != 1 or body.count(END) != 1: + raise FlowError("Duplicate or incomplete PR flow markers; preserving the description") + start, end = body.index(START), body.index(END) + len(END) + if start >= end - len(END) or not body.startswith("\n\n", end): Review Comment: The signature and boundary checks here assume the body read back is byte-identical to what was written, with LF endings. That holds until someone edits the description through a client that CRLF-normalizes, which rewrites the whole body *including* the managed block. Two things then fail at once: - `body.startswith("\n\n", end)` sees `\r\n\r\n` and raises `Malformed PR flow boundaries` - `signature()` HMACs the block text, so the CRLF block no longer verifies Both fail closed, and per the README `force` does not bypass signature checks — so the only recovery is the manual "maintainer removes the managed block by hand" procedure, per PR. This is not hypothetical on this repo. 13 of the last 100 `apache/pinot` PR bodies already come back from the API containing `\r\n`: ```sh gh api "repos/apache/pinot/pulls?state=all&per_page=100" \ --jq '.[] | select((.body // "") | test("\r")) | .number' ``` To be precise about the mechanism, since it is narrower than "CRLF bodies break it": a PR whose author text is *already* CRLF is fine — `bounds()` returns `None`, the block is prepended as LF, and the signature only covers the block. The break is a CRLF-normalizing edit that happens *after* publication. The 13% figure just shows that editing path is in active use here, and "author edited the description" is exactly the case the `edited` signal exists to serve. The fix looks small: normalize once on ingest (`body.replace("\r\n", "\n")` in `section()` / `author_body()`) and sign the normalized form. Worth a matching fixture in `OwnershipTest` as well — every body in `test_main.py` currently uses `\n`, so the suite cannot see this today. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
