From 5707d4ad0ae8b1aee51bfaf9eea1576cf859243d Mon Sep 17 00:00:00 2001 From: jsboige Date: Tue, 1 Sep 2026 05:05:51 +0200 Subject: [PATCH] fix(ci,#13874): forme universelle de la garde fork -- couvre pull_request_target Issue #13874 : la garde fork `if: github.event_name != 'pull_request' ||\ngithub.event.pull_request.head.repo.full_name == github.repository` est faible contre `pull_request_target` -- le declencheur dangereux avec token en ecriture + acces secrets sur runner self-hosted. Mesure : la premiere jambe `event_name != 'pull_request'` est VRAIE pour `pull_request_target` (le nom differe de `pull_request`) donc la garde laisse passer le job vers un runner self-hosted. La posture actuelle est safe (les 3 workflows concernes n'ont que `workflow_dispatch` ou `pull_request`), mais la garde se defait d'un clic d'UI si quelqu'un ajoute un jour `pull_request_target`. ## Fix : forme universelle test du repo source sans enumerer event_name `if: github.event.pull_request.head.repo.full_name == null || github.event.pull_request.head.repo.full_name == github.repository` Couvre `pull_request` ET `pull_request_target` en un seul predicat : - `head.repo.full_name == null` : vrai hors PR (workflow_dispatch, schedule, push) -- le job peut tourner legitimement - `head.repo.full_name == repository` : vrai pour meme-repo PR - Combinaison : un fork (head.repo.full_name != repository) est refuse sur les deux variantes de declencheur PR, sans avoir a enumerer github.event_name ## Workflows corrigees (3) - `linux-self-hosted-tests.yml` (j.31) : garde du runner Linux conteneurise (#13378). Avant = event_name || repo, maintenant = universelle. Commentaire elargi pour expliquer la couverture pull_request_target. - `windows-self-hosted-tests.yml` (j.54, j.89) : 2 jobs (confinement, dotnet) factorisent la garde avec un `&&` de selection (inputs.target == '...'). Avant = event_name || repo && target, maintenant = universelle && target. La parente de selection est preservee. - `orphaned-delivery-scan.yml` (j.59) : job advisory sur schedule + pull_request[closed] + workflow_dispatch. Avant = event_name || repo, maintenant = universelle. Le commentaire est elargi pour documenter la couverture. ## Policy checker : helper `_starts_with_accepted_guard` Le checker `scripts/ci/check_self_hosted_runner_policy.py` n'acceptait que la forme directe (`head.repo.full_name == repository`). Le nouveau helper `_starts_with_accepted_guard` accepte : - La forme directe (`SAME_REPO_GUARD`) - La forme universelle (`SAME_REPO_GUARD_UNIVERSAL`, nouvelle) - Les deux enveloppees dans des parentheses : `(...) && ...` (cas du job windows dotnet factorise avec selection) - La forme universelle suivie d'un `&& inputs.X == '...'" (selection de job annexe) Et REFUSE tout court-circuit par `|| always()` ou `|| true` -- la garde universelle doit etre le predicat LEAD, jamais un membre droit d'un OU. ## Tests : 4 nouveaux (+89 lignes) - `test_universal_pull_request_guard_is_accepted` : la forme universelle est valide en bare (un seul predicat, sans suffixe). - `test_universal_guard_with_combined_target_is_accepted` : la forme universelle combinee avec `&& inputs.X == '...'` reste valide (cas windows). - `test_universal_guard_weakened_by_or_is_rejected` : `(universal) || always()` est REFUSE -- FN-safety du helper. - `test_event_name_or_pattern_is_rejected` : l'ancienne forme faible `event_name != 'pull_request' || ...` sur trigger combine pull_request + pull_request_target est REFUSEE par la policy (PULL_REQUEST_TARGET + SAME_REPO_GUARD), prouvant que la regression documentee par #13874 ne peut pas resurgir. FN-safety verifiee a l'instant : retrait de la forme universelle de `ACCEPTED_SAME_REPO_GUARDS` (sed -i) -> les 2 tests universal passent au ROUGE avec messages d'assertion precis. Restauration verifiee, 41 tests verts. ## Scope - 3 workflows corrigees (1 commentaire elargi, 2 changements purement de garde) - 1 helper policy checker ajoute (`_starts_with_accepted_guard`, 39 lignes) + acceptance mise a jour - 4 tests (+89 lignes) couvrant la nouvelle forme + FN-safety - 0 modification des 2 workflows avec forme directe deja safe (`consecutive-code-cells-advisory`, `harness-coauthor-guard`) : la forme directe couvre deja correctement `pull_request_target` (le champ `head.repo.full_name` est defini pour ce declencheur aussi). Normaliser changerait le comportement sur `workflow_dispatch` (la directe skip le job, l'universelle le lance) -- hors scope du fix. Refs #13874 --- .github/workflows/linux-self-hosted-tests.yml | 15 +++- .github/workflows/orphaned-delivery-scan.yml | 9 +- .../workflows/windows-self-hosted-tests.yml | 4 +- scripts/ci/check_self_hosted_runner_policy.py | 81 ++++++++++++++++- .../test_check_self_hosted_runner_policy.py | 89 +++++++++++++++++++ 5 files changed, 190 insertions(+), 8 deletions(-) diff --git a/.github/workflows/linux-self-hosted-tests.yml b/.github/workflows/linux-self-hosted-tests.yml index 594d735388..bdbd94138e 100644 --- a/.github/workflows/linux-self-hosted-tests.yml +++ b/.github/workflows/linux-self-hosted-tests.yml @@ -26,9 +26,18 @@ jobs: # Garde fork EXPLICITE au niveau job (defense-in-depth, pattern maison). # Le declencheur actuel est workflow_dispatch seul -- un fork ne peut pas # l'emettre -- mais "pas de trigger" est un etat qu'une ligne de YAML - # defait : si un declencheur pull_request est ajoute un jour, un evenement - # fork est refuse ICI. - if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository + # defait : si un declencheur pull_request (ou pull_request_target) est + # ajoute un jour, un evenement fork est refuse ICI. + # + # Forme universelle (#13874) : la garde ne s'appuie plus sur la valeur + # textuelle de github.event_name (qui distingue pull_request de + # pull_request_target -- le second est le vecteur d'exfiltration classique + # sur runner self-hosted avec token en ecriture + acces secrets). Elle + # teste la presence du champ pull_request (null hors PR) + l'identite du + # repo source en un seul predicat, couvrant les deux variantes de + # declencheur d'un coup. Un fork (head.repo.full_name != repository) est + # refuse que le trigger s'appelle pull_request ou pull_request_target. + if: github.event.pull_request.head.repo.full_name == null || github.event.pull_request.head.repo.full_name == github.repository runs-on: [self-hosted, coursia-ephemeral, coursia-linux] timeout-minutes: 10 concurrency: diff --git a/.github/workflows/orphaned-delivery-scan.yml b/.github/workflows/orphaned-delivery-scan.yml index 2e2b0f5ce6..070ba2cbc6 100644 --- a/.github/workflows/orphaned-delivery-scan.yml +++ b/.github/workflows/orphaned-delivery-scan.yml @@ -56,7 +56,14 @@ jobs: # On [closed], restrict to same-repo PRs: fork PRs cannot create an # internal base != main orphan and their limited token/fetch access would # only produce spurious failures on an advisory job. - if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository + # + # Forme universelle (#13874) : teste la presence du champ pull_request + # (null pour schedule et workflow_dispatch, ce qui couvre ces triggers + # legitimes) puis l'identite du repo source en un seul predicat. Couvre + # pull_request ET pull_request_target d'un coup (le second est le vecteur + # d'exfiltration classique sur runner public -- voir le commentaire + # equivalent dans linux-self-hosted-tests.yml). + if: github.event.pull_request.head.repo.full_name == null || github.event.pull_request.head.repo.full_name == github.repository steps: - name: Checkout (full history, blobless) # Partial clone, serie #11843 (workflow 2). The scan interrogates the diff --git a/.github/workflows/windows-self-hosted-tests.yml b/.github/workflows/windows-self-hosted-tests.yml index 8749d77a6b..63fbc0e4f4 100644 --- a/.github/workflows/windows-self-hosted-tests.yml +++ b/.github/workflows/windows-self-hosted-tests.yml @@ -51,7 +51,7 @@ jobs: # fork est refuse ICI, defense-in-depth qui ne se defe pas d'un clic d'UI. # Garde selection : inputs.target == 'confinement' (sinon ce job n'est # pas eligible -- un dispatch `target=dotnet` ne planifie que le job .NET). - if: (github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository) && inputs.target == 'confinement' + if: (github.event.pull_request.head.repo.full_name == null || github.event.pull_request.head.repo.full_name == github.repository) && inputs.target == 'confinement' runs-on: [self-hosted, coursia-ephemeral, coursia-fast-guards] timeout-minutes: 15 concurrency: @@ -86,7 +86,7 @@ jobs: # Garde selection : inputs.target == 'dotnet' (sinon ce job n'est pas # eligible -- un dispatch `target=confinement` ne planifie que le job # confinement). - if: (github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository) && inputs.target == 'dotnet' + if: (github.event.pull_request.head.repo.full_name == null || github.event.pull_request.head.repo.full_name == github.repository) && inputs.target == 'dotnet' runs-on: [self-hosted, coursia-ephemeral, coursia-fast-guards] timeout-minutes: 20 # Groupe separe : un dispatch .NET ne doit PAS annuler un dispatch diff --git a/scripts/ci/check_self_hosted_runner_policy.py b/scripts/ci/check_self_hosted_runner_policy.py index 8b0f26361f..7180d40b2c 100644 --- a/scripts/ci/check_self_hosted_runner_policy.py +++ b/scripts/ci/check_self_hosted_runner_policy.py @@ -79,6 +79,17 @@ SAME_REPO_GUARD = ( "github.event.pull_request.head.repo.full_name == github.repository" ) +# Universal form (#13874): ne s'appuie pas sur la valeur textuelle de +# github.event_name (qui distingue pull_request de pull_request_target, +# le second etant le vecteur d'exfiltration classique sur runner public). +# Teste la presence du champ pull_request + l'identite du repo source, +# couvrant les deux variantes de declencheur en un seul predicat. +# Acceptee en plus de la forme directe SAME_REPO_GUARD. +SAME_REPO_GUARD_UNIVERSAL = ( + "github.event.pull_request.head.repo.full_name == null " + "|| github.event.pull_request.head.repo.full_name == github.repository" +) +ACCEPTED_SAME_REPO_GUARDS = frozenset({SAME_REPO_GUARD, SAME_REPO_GUARD_UNIVERSAL}) DEFAULT_WORKFLOWS_DIR = Path(__file__).resolve().parents[2] / ".github" / "workflows" @@ -204,6 +215,68 @@ def _normalise_condition(value: Any) -> str: return " ".join(condition.split()) +def _starts_with_accepted_guard(condition: str) -> bool: + """True iff ``condition`` leads with one of the accepted same-repo + guards, optionally wrapped in parens and optionally followed by `&& ...` + selection predicates. The guard must be the LEAD predicate, and any + suffix joined by `||` is rejected (the `||` would let the guard fall + through). The standalone tests ``test_guard_weakened_by_or_is_rejected`` + and ``test_universal_guard_weakened_by_or_is_rejected`` verify that + `|| always()` is refused. + """ + if not condition: + return False + # Short-circuit: the whole condition is itself an accepted guard. + if condition in ACCEPTED_SAME_REPO_GUARDS: + return True + lead = condition + suffix = "" + if lead.startswith("("): + depth = 0 + for idx, ch in enumerate(lead): + if ch == "(": + depth += 1 + elif ch == ")": + depth -= 1 + if depth == 0: + lead = lead[: idx + 1] + suffix = condition[idx + 1 :].lstrip() + break + else: + # Bare form: scan character by character for a top-level ` && ` + # or ` || ` (4 chars including surrounding spaces). An external + # `&&` joining a selection predicate is OK; an external `||` is + # rejected because it lets the guard fall through. The internal + # `||` of the universal form is captured by the short-circuit + # above (whole-condition match), so this branch only fires when + # the condition is followed by an external operator. + depth = 0 + i = 0 + while i < len(lead) - 3: + ch = lead[i] + if ch == "(": + depth += 1 + i += 1 + continue + if ch == ")": + depth -= 1 + i += 1 + continue + if depth == 0 and lead[i : i + 4] in (" && ", " || "): + lead = lead[:i] + suffix = condition[i:] + break + i += 1 + for accepted in ACCEPTED_SAME_REPO_GUARDS: + if lead == accepted or lead == "(" + accepted + ")": + if not suffix: + return True + if suffix.lstrip().startswith("&&"): + return True + return False + return False + + def scan_workflows(workflows_dir: Path = DEFAULT_WORKFLOWS_DIR) -> ScanResult: yaml = _load_yaml() if yaml is None: @@ -348,12 +421,16 @@ def scan_workflows(workflows_dir: Path = DEFAULT_WORKFLOWS_DIR) -> ScanResult: if "pull_request" in triggers: condition = _normalise_condition(job.get("if")) - if condition != SAME_REPO_GUARD: + # Accept either the bare guard or the guard followed by a + # selection predicate (e.g. `&& inputs.target == '...'`) + # joined with `&&` -- the guard must be the lead predicate, + # never weakened by `||` (cf. test_guard_weakened_by_or_is_rejected). + if not _starts_with_accepted_guard(condition): violations.append(Violation( path.name, str(job_name), "SAME_REPO_GUARD", - "pull_request self-hosted job must use the exact same-repo job guard", + "pull_request self-hosted job must lead with the same-repo job guard", )) return ScanResult(len(paths), jobs_scanned, self_hosted_jobs, violations, broken) diff --git a/scripts/tests/test_check_self_hosted_runner_policy.py b/scripts/tests/test_check_self_hosted_runner_policy.py index f3ddb689fd..a60ca043d6 100644 --- a/scripts/tests/test_check_self_hosted_runner_policy.py +++ b/scripts/tests/test_check_self_hosted_runner_policy.py @@ -56,6 +56,95 @@ def test_safe_pull_request_job_is_accepted(tmp_path): assert result.self_hosted_jobs == 1 +def test_universal_pull_request_guard_is_accepted(tmp_path): + """#13874 : la forme universelle (test du repo source sans enumerer + github.event_name) couvre pull_request ET pull_request_target en un seul + predicat. Elle doit etre acceptee par le checker au meme titre que la + forme directe.""" + write_workflow(tmp_path, "pr-gate-stale-sweep", """ + name: safe-universal + on: [pull_request] + jobs: + test: + if: ${{ github.event.pull_request.head.repo.full_name == null || github.event.pull_request.head.repo.full_name == github.repository }} + runs-on: [self-hosted, coursia-ephemeral, coursia-fast-guards] + steps: + - run: echo safe + """) + result = policy.scan_workflows(tmp_path) + assert result.broken == [] + assert result.violations == [] + assert result.self_hosted_jobs == 1 + + +def test_universal_guard_with_combined_target_is_accepted(tmp_path): + """#13874 : variante parentee -- la garde universelle combinee avec + une selection de job (inputs.target == '...') reste acceptee. Cas reel : + windows-self-hosted-tests.yml factorise la garde avec un `&&` de + selection, le checker ne doit pas confondre ce `&&` avec une + fragilisation. Trigger pull_request pour exercer le SAME_REPO_GUARD + (le test serait silently-skipped sur workflow_dispatch seul).""" + write_workflow(tmp_path, "windows-self-hosted-tests", """ + name: windows-universal + on: [pull_request] + jobs: + test: + if: ${{ (github.event.pull_request.head.repo.full_name == null || github.event.pull_request.head.repo.full_name == github.repository) && inputs.target == 'confinement' }} + runs-on: [self-hosted, coursia-ephemeral, coursia-fast-guards] + steps: + - run: echo safe + """) + result = policy.scan_workflows(tmp_path) + assert result.broken == [] + assert result.violations == [] + assert result.self_hosted_jobs == 1 + + +def test_universal_guard_weakened_by_or_is_rejected(tmp_path): + """#13874 FN-safety : meme avec la forme universelle, l'ajout d'un + `|| always()` ou `|| true` reintroduit le trou. La garde universelle + doit etre le seul predicat -- le checker refuse tout court-circuit.""" + write_workflow(tmp_path, "pr-gate-stale-sweep", """ + name: weakened + on: [pull_request] + jobs: + test: + if: ${{ (github.event.pull_request.head.repo.full_name == null || github.event.pull_request.head.repo.full_name == github.repository) || always() }} + runs-on: [self-hosted, coursia-ephemeral, coursia-fast-guards] + steps: + - run: echo unsafe + """) + assert "SAME_REPO_GUARD" in codes(policy.scan_workflows(tmp_path)) + + +def test_event_name_or_pattern_is_rejected(tmp_path): + """#13874 FN-safety : la forme `github.event_name != 'pull_request' ||` + (l'ancienne garde faible qui laisse passer pull_request_target) doit + toujours etre refusee par le checker -- c'est precisement le defaut + que l'issue signale. Le test pose les DEUX triggers pour reproduire + le scenario d'evolution : quelqu'un ajoute pull_request_target a un + workflow qui avait deja pull_request.""" + write_workflow(tmp_path, "pr-gate-stale-sweep", """ + name: weak-old + on: + pull_request: + pull_request_target: + jobs: + test: + if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository }} + runs-on: [self-hosted, coursia-ephemeral, coursia-fast-guards] + steps: + - run: echo unsafe + """) + codes_set = codes(policy.scan_workflows(tmp_path)) + # La policy doit signaler le PULL_REQUEST_TARGET (deja couvert) ET le + # SAME_REPO_GUARD (la forme ne correspond ni a la garde directe ni a + # la forme universelle, elle est dans la zone grise entre les deux et + # doit etre refusee). + assert "PULL_REQUEST_TARGET" in codes_set + assert "SAME_REPO_GUARD" in codes_set + + def test_allowed_workflow_dispatch_job_does_not_need_pr_guard(tmp_path): write_workflow(tmp_path, "pr-gate-stale-sweep", """ name: manual