diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 86ea48b4..e8c8c124 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1013,24 +1013,56 @@ jobs: const writerRuleset = writerRulesets[0]; const writerIncludes = writerRuleset.conditions?.ref_name?.include || []; const writerExcludes = writerRuleset.conditions?.ref_name?.exclude || []; - // GitHub's read projection omits parameters when this update-rule - // exception is disabled. Accept only that omission or exact false; - // reject a present null/non-boolean field instead of weakening the gate. + // GitHub's read projection omits the entire parameters property + // when this exception is disabled. Accept only that exact omission + // or a one-field object containing exact false. function isStrictUpdateRule(rule) { if (rule?.type !== 'update') { return false; } - const updateAllowsFetchAndMerge = - rule.parameters?.update_allows_fetch_and_merge; + if (!Object.prototype.hasOwnProperty.call(rule, 'parameters')) { + return true; + } + const parameters = rule.parameters; + if ( + parameters === null || + typeof parameters !== 'object' || + Array.isArray(parameters) + ) { + return false; + } + const parameterKeys = Object.keys(parameters); return ( - updateAllowsFetchAndMerge === undefined || - updateAllowsFetchAndMerge === false + parameterKeys.length === 1 && + parameterKeys[0] === 'update_allows_fetch_and_merge' && + parameters.update_allows_fetch_and_merge === false + ); + } + function isStrictGraphQLUpdateRule(restRuleset, graphRuleset) { + const restRulesetID = Number(restRuleset?.id); + const graphRulesetID = Number(graphRuleset?.databaseId); + const graphRules = graphRuleset?.rules; + const graphRule = graphRules?.nodes?.[0]; + return ( + Number.isSafeInteger(restRulesetID) && + restRulesetID > 0 && + graphRulesetID === restRulesetID && + graphRuleset.name === restRuleset.name && + graphRuleset.enforcement === 'ACTIVE' && + graphRuleset.target === 'BRANCH' && + graphRules?.totalCount === 1 && + graphRules.nodes?.length === 1 && + graphRule?.type === 'UPDATE' && + graphRule.parameters?.__typename === 'UpdateParameters' && + graphRule.parameters.updateAllowsFetchAndMerge === false ); } if ( writerIncludes.length !== 1 || writerIncludes[0] !== 'refs/heads/main' || writerExcludes.length !== 0 || + typeof writerRuleset.node_id !== 'string' || + !writerRuleset.node_id || writerRuleset.rules?.length !== 1 || !isStrictUpdateRule(writerRuleset.rules[0]) || writerRuleset.current_user_can_bypass !== 'never' @@ -1040,6 +1072,37 @@ jobs: ); return; } + const {node: graphWriterRuleset} = await github.graphql( + `query ReviewerRouterWriterRule($rulesetID: ID!) { + node(id: $rulesetID) { + ... on RepositoryRuleset { + databaseId + name + enforcement + target + rules(first: 2) { + totalCount + nodes { + type + parameters { + __typename + ... on UpdateParameters { + updateAllowsFetchAndMerge + } + } + } + } + } + } + }`, + {rulesetID: writerRuleset.node_id}, + ); + if (!isStrictGraphQLUpdateRule(writerRuleset, graphWriterRuleset)) { + core.setFailed( + `${writerRulesetName} must expose one strict UPDATE rule with updateAllowsFetchAndMerge=false through GraphQL.`, + ); + return; + } const maxAttempts = 6; for (let attempt = 1; attempt <= maxAttempts; attempt += 1) { diff --git a/.github/workflows/reviewer-router.yml b/.github/workflows/reviewer-router.yml index 68df6c71..b4066afd 100644 --- a/.github/workflows/reviewer-router.yml +++ b/.github/workflows/reviewer-router.yml @@ -446,18 +446,48 @@ jobs: const writerRuleset = writerRulesets[0]; const writerIncludes = writerRuleset.conditions?.ref_name?.include || []; const writerExcludes = writerRuleset.conditions?.ref_name?.exclude || []; - // GitHub's read projection omits parameters when this update-rule - // exception is disabled. Accept only that omission or exact false; - // reject a present null/non-boolean field instead of weakening the gate. + // GitHub's read projection omits the entire parameters property + // when this exception is disabled. Accept only that exact omission + // or a one-field object containing exact false. function isStrictUpdateRule(rule) { if (rule?.type !== 'update') { return false; } - const updateAllowsFetchAndMerge = - rule.parameters?.update_allows_fetch_and_merge; + if (!Object.prototype.hasOwnProperty.call(rule, 'parameters')) { + return true; + } + const parameters = rule.parameters; + if ( + parameters === null || + typeof parameters !== 'object' || + Array.isArray(parameters) + ) { + return false; + } + const parameterKeys = Object.keys(parameters); return ( - updateAllowsFetchAndMerge === undefined || - updateAllowsFetchAndMerge === false + parameterKeys.length === 1 && + parameterKeys[0] === 'update_allows_fetch_and_merge' && + parameters.update_allows_fetch_and_merge === false + ); + } + function isStrictGraphQLUpdateRule(restRuleset, graphRuleset) { + const restRulesetID = Number(restRuleset?.id); + const graphRulesetID = Number(graphRuleset?.databaseId); + const graphRules = graphRuleset?.rules; + const graphRule = graphRules?.nodes?.[0]; + return ( + Number.isSafeInteger(restRulesetID) && + restRulesetID > 0 && + graphRulesetID === restRulesetID && + graphRuleset.name === restRuleset.name && + graphRuleset.enforcement === 'ACTIVE' && + graphRuleset.target === 'BRANCH' && + graphRules?.totalCount === 1 && + graphRules.nodes?.length === 1 && + graphRule?.type === 'UPDATE' && + graphRule.parameters?.__typename === 'UpdateParameters' && + graphRule.parameters.updateAllowsFetchAndMerge === false ); } if ( @@ -466,6 +496,8 @@ jobs: writerIncludes.length !== 1 || writerIncludes[0] !== 'refs/heads/main' || writerExcludes.length !== 0 || + typeof writerRuleset.node_id !== 'string' || + !writerRuleset.node_id || writerRuleset.rules?.length !== 1 || !isStrictUpdateRule(writerRuleset.rules[0]) || writerRuleset.current_user_can_bypass !== 'pull_requests_only' @@ -474,6 +506,36 @@ jobs: 'Reviewer Router App requires the exact repository-owned main writer rule with pull-request-only bypass.', ); } + const {node: graphWriterRuleset} = await github.graphql( + `query ReviewerRouterWriterRule($rulesetID: ID!) { + node(id: $rulesetID) { + ... on RepositoryRuleset { + databaseId + name + enforcement + target + rules(first: 2) { + totalCount + nodes { + type + parameters { + __typename + ... on UpdateParameters { + updateAllowsFetchAndMerge + } + } + } + } + } + } + }`, + {rulesetID: writerRuleset.node_id}, + ); + if (!isStrictGraphQLUpdateRule(writerRuleset, graphWriterRuleset)) { + throw new Error( + 'Reviewer Router App requires GraphQL to expose one strict UPDATE rule with updateAllowsFetchAndMerge=false.', + ); + } for (const ruleset of activeMainRulesets) { if (ruleset.id === writerRuleset.id) { continue; @@ -736,18 +798,48 @@ jobs: const writerRuleset = writerRulesets[0]; const writerIncludes = writerRuleset.conditions?.ref_name?.include || []; const writerExcludes = writerRuleset.conditions?.ref_name?.exclude || []; - // GitHub's read projection omits parameters when this update-rule - // exception is disabled. Accept only that omission or exact false; - // reject a present null/non-boolean field instead of weakening the gate. + // GitHub's read projection omits the entire parameters property + // when this exception is disabled. Accept only that exact omission + // or a one-field object containing exact false. function isStrictUpdateRule(rule) { if (rule?.type !== 'update') { return false; } - const updateAllowsFetchAndMerge = - rule.parameters?.update_allows_fetch_and_merge; + if (!Object.prototype.hasOwnProperty.call(rule, 'parameters')) { + return true; + } + const parameters = rule.parameters; + if ( + parameters === null || + typeof parameters !== 'object' || + Array.isArray(parameters) + ) { + return false; + } + const parameterKeys = Object.keys(parameters); return ( - updateAllowsFetchAndMerge === undefined || - updateAllowsFetchAndMerge === false + parameterKeys.length === 1 && + parameterKeys[0] === 'update_allows_fetch_and_merge' && + parameters.update_allows_fetch_and_merge === false + ); + } + function isStrictGraphQLUpdateRule(restRuleset, graphRuleset) { + const restRulesetID = Number(restRuleset?.id); + const graphRulesetID = Number(graphRuleset?.databaseId); + const graphRules = graphRuleset?.rules; + const graphRule = graphRules?.nodes?.[0]; + return ( + Number.isSafeInteger(restRulesetID) && + restRulesetID > 0 && + graphRulesetID === restRulesetID && + graphRuleset.name === restRuleset.name && + graphRuleset.enforcement === 'ACTIVE' && + graphRuleset.target === 'BRANCH' && + graphRules?.totalCount === 1 && + graphRules.nodes?.length === 1 && + graphRule?.type === 'UPDATE' && + graphRule.parameters?.__typename === 'UpdateParameters' && + graphRule.parameters.updateAllowsFetchAndMerge === false ); } if ( @@ -756,6 +848,8 @@ jobs: writerIncludes.length !== 1 || writerIncludes[0] !== 'refs/heads/main' || writerExcludes.length !== 0 || + typeof writerRuleset.node_id !== 'string' || + !writerRuleset.node_id || writerRuleset.rules?.length !== 1 || !isStrictUpdateRule(writerRuleset.rules[0]) || writerRuleset.current_user_can_bypass !== 'pull_requests_only' @@ -764,6 +858,36 @@ jobs: 'Reviewer Router App requires the exact repository-owned main writer rule with pull-request-only bypass.', ); } + const {node: graphWriterRuleset} = await github.graphql( + `query ReviewerRouterWriterRule($rulesetID: ID!) { + node(id: $rulesetID) { + ... on RepositoryRuleset { + databaseId + name + enforcement + target + rules(first: 2) { + totalCount + nodes { + type + parameters { + __typename + ... on UpdateParameters { + updateAllowsFetchAndMerge + } + } + } + } + } + } + }`, + {rulesetID: writerRuleset.node_id}, + ); + if (!isStrictGraphQLUpdateRule(writerRuleset, graphWriterRuleset)) { + throw new Error( + 'Reviewer Router App requires GraphQL to expose one strict UPDATE rule with updateAllowsFetchAndMerge=false.', + ); + } for (const ruleset of activeMainRulesets) { if (ruleset.id === writerRuleset.id) { continue; diff --git a/docs/automation.md b/docs/automation.md index e10868f8..a3636bf1 100644 --- a/docs/automation.md +++ b/docs/automation.md @@ -84,8 +84,10 @@ it: targeting only `refs/heads/main`, with exactly one `Restrict updates` rule (`update_allows_fetch_and_merge: false`). GitHub may project that strict value through the read APIs as `{type: "update"}` with `parameters` omitted; - consumers accept only that omission or an explicit boolean `false`, and - reject every present non-false value; + consumers accept only that exact omission or a one-field `parameters` object + containing explicit boolean `false`, and reject every other present shape or + value. They then bind the same ruleset node through GraphQL and require its + non-null `updateAllowsFetchAndMerge` value to be exactly `false`; - give that ruleset exactly two bypass actors: the Reviewer Router App as an `Integration` in `pull_request` mode, and `PeterGuy326` (ID `47820304`) in `always` mode for Formula publication and break-glass recovery; diff --git a/docs/ci-pr-gates.md b/docs/ci-pr-gates.md index 3a998255..6254e564 100644 --- a/docs/ci-pr-gates.md +++ b/docs/ci-pr-gates.md @@ -172,8 +172,11 @@ producer. Token minting or takeover fails closed without falling back to the required `Test` context live-verifies the exact `main-merge-writers` update rule. GitHub's read APIs may omit `parameters` for the strict `update_allows_fetch_and_merge: false` value, so the gate accepts only that -omission or an explicit boolean `false` and rejects every present non-false -value. It also requires its own built-in token to report +exact omission or a one-field `parameters` object containing explicit boolean +`false`; every other present shape or value fails closed. The gate then binds +the same ruleset node through GraphQL and requires its non-null +`updateAllowsFetchAndMerge` value to be exactly `false`. It also requires its +own built-in token to report `current_user_can_bypass: never`. The minted App separately requires `pull_requests_only` on that writer rule and `never` on every other active main ruleset before it can enable or reconcile auto-merge. GitHub hides the complete diff --git a/test/scripts/reviewer_router_ruleset_projection_test.go b/test/scripts/reviewer_router_ruleset_projection_test.go index 84aad8b1..2b9457af 100644 --- a/test/scripts/reviewer_router_ruleset_projection_test.go +++ b/test/scripts/reviewer_router_ruleset_projection_test.go @@ -17,11 +17,42 @@ const strictUpdateRuleHelper = `function isStrictUpdateRule(rule) { if (rule?.type !== 'update') { return false; } - const updateAllowsFetchAndMerge = - rule.parameters?.update_allows_fetch_and_merge; + if (!Object.prototype.hasOwnProperty.call(rule, 'parameters')) { + return true; + } + const parameters = rule.parameters; + if ( + parameters === null || + typeof parameters !== 'object' || + Array.isArray(parameters) + ) { + return false; + } + const parameterKeys = Object.keys(parameters); return ( - updateAllowsFetchAndMerge === undefined || - updateAllowsFetchAndMerge === false + parameterKeys.length === 1 && + parameterKeys[0] === 'update_allows_fetch_and_merge' && + parameters.update_allows_fetch_and_merge === false + ); +}` + +const strictGraphQLUpdateRuleHelper = `function isStrictGraphQLUpdateRule(restRuleset, graphRuleset) { + const restRulesetID = Number(restRuleset?.id); + const graphRulesetID = Number(graphRuleset?.databaseId); + const graphRules = graphRuleset?.rules; + const graphRule = graphRules?.nodes?.[0]; + return ( + Number.isSafeInteger(restRulesetID) && + restRulesetID > 0 && + graphRulesetID === restRulesetID && + graphRuleset.name === restRuleset.name && + graphRuleset.enforcement === 'ACTIVE' && + graphRuleset.target === 'BRANCH' && + graphRules?.totalCount === 1 && + graphRules.nodes?.length === 1 && + graphRule?.type === 'UPDATE' && + graphRule.parameters?.__typename === 'UpdateParameters' && + graphRule.parameters.updateAllowsFetchAndMerge === false ); }` @@ -63,6 +94,9 @@ func TestReviewerRouterAcceptsGitHubStrictUpdateReadProjection(t *testing.T) { if got := strings.Count(script, strictUpdateRuleHelper); got != 1 { t.Errorf("writer-ruleset script contains strict update helper %d times, want 1", got) } + if got := strings.Count(script, strictGraphQLUpdateRuleHelper); got != 1 { + t.Errorf("writer-ruleset script contains GraphQL strict update helper %d times, want 1", got) + } if strings.Contains( script, "parameters?.update_allows_fetch_and_merge !== false", @@ -72,21 +106,36 @@ func TestReviewerRouterAcceptsGitHubStrictUpdateReadProjection(t *testing.T) { if !strings.Contains(script, "!isStrictUpdateRule(writerRuleset.rules[0])") { t.Error("writer-ruleset script does not enforce the strict update helper") } + for _, marker := range []string{ + "query ReviewerRouterWriterRule($rulesetID: ID!)", + "rules(first: 2)", + "updateAllowsFetchAndMerge", + "{rulesetID: writerRuleset.node_id}", + "!isStrictGraphQLUpdateRule(writerRuleset, graphWriterRuleset)", + } { + if !strings.Contains(script, marker) { + t.Errorf("writer-ruleset script does not enforce GraphQL marker %q", marker) + } + } } if checked != 3 { t.Fatalf("writer-ruleset scripts checked = %d, want App enable, reconcile, and CI self-check", checked) } - verification := strictUpdateRuleHelper + ` + verification := strictUpdateRuleHelper + "\n" + strictGraphQLUpdateRuleHelper + ` const cases = [ ['omitted parameters', {type: 'update'}, true], - ['null parameters', {type: 'update', parameters: null}, true], - ['empty parameters', {type: 'update', parameters: {}}, true], ['explicit false', {type: 'update', parameters: {update_allows_fetch_and_merge: false}}, true], + ['null parameters', {type: 'update', parameters: null}, false], + ['empty parameters', {type: 'update', parameters: {}}, false], + ['boolean parameters', {type: 'update', parameters: false}, false], + ['string parameters', {type: 'update', parameters: 'malformed'}, false], + ['array parameters', {type: 'update', parameters: []}, false], ['explicit true', {type: 'update', parameters: {update_allows_fetch_and_merge: true}}, false], ['null field', {type: 'update', parameters: {update_allows_fetch_and_merge: null}}, false], ['string false', {type: 'update', parameters: {update_allows_fetch_and_merge: 'false'}}, false], ['numeric false', {type: 'update', parameters: {update_allows_fetch_and_merge: 0}}, false], + ['extra parameter', {type: 'update', parameters: {update_allows_fetch_and_merge: false, future_exception: false}}, false], ['wrong rule type', {type: 'creation'}, false], ['null rule', null, false], ]; @@ -96,6 +145,41 @@ for (const [name, rule, want] of cases) { throw new Error(name + ': got ' + got + ', want ' + want); } } + +const restRuleset = {id: 21363804, name: 'main-merge-writers'}; +const graphRule = { + type: 'UPDATE', + parameters: { + __typename: 'UpdateParameters', + updateAllowsFetchAndMerge: false, + }, +}; +const graphRuleset = { + databaseId: 21363804, + name: 'main-merge-writers', + enforcement: 'ACTIVE', + target: 'BRANCH', + rules: {totalCount: 1, nodes: [graphRule]}, +}; +const graphCases = [ + ['valid GraphQL projection', restRuleset, graphRuleset, true], + ['invalid REST id', {id: 0, name: 'main-merge-writers'}, graphRuleset, false], + ['mismatched GraphQL id', restRuleset, {...graphRuleset, databaseId: 7}, false], + ['mismatched name', restRuleset, {...graphRuleset, name: 'other'}, false], + ['inactive ruleset', restRuleset, {...graphRuleset, enforcement: 'EVALUATE'}, false], + ['wrong target', restRuleset, {...graphRuleset, target: 'TAG'}, false], + ['extra rule', restRuleset, {...graphRuleset, rules: {totalCount: 2, nodes: [graphRule, graphRule]}}, false], + ['wrong rule type', restRuleset, {...graphRuleset, rules: {totalCount: 1, nodes: [{...graphRule, type: 'CREATION'}]}}, false], + ['wrong parameter type', restRuleset, {...graphRuleset, rules: {totalCount: 1, nodes: [{...graphRule, parameters: {...graphRule.parameters, __typename: 'PullRequestParameters'}}]}}, false], + ['fetch and merge enabled', restRuleset, {...graphRuleset, rules: {totalCount: 1, nodes: [{...graphRule, parameters: {...graphRule.parameters, updateAllowsFetchAndMerge: true}}]}}, false], + ['missing GraphQL node', restRuleset, null, false], +]; +for (const [name, rest, graph, want] of graphCases) { + const got = isStrictGraphQLUpdateRule(rest, graph); + if (got !== want) { + throw new Error(name + ': got ' + got + ', want ' + want); + } +} ` command := exec.Command(node, "-e", verification) if output, runErr := command.CombinedOutput(); runErr != nil { diff --git a/test/scripts/reviewer_router_workflow_test.go b/test/scripts/reviewer_router_workflow_test.go index ba972fa2..0d947707 100644 --- a/test/scripts/reviewer_router_workflow_test.go +++ b/test/scripts/reviewer_router_workflow_test.go @@ -296,10 +296,22 @@ func TestReviewerRouterWorkflowContract(t *testing.T) { "writerRuleset.rules?.length !== 1", "function isStrictUpdateRule(rule)", "rule?.type !== 'update'", - "rule.parameters?.update_allows_fetch_and_merge", - "updateAllowsFetchAndMerge === undefined", - "updateAllowsFetchAndMerge === false", + "!Object.prototype.hasOwnProperty.call(rule, 'parameters')", + "parameters === null", + "typeof parameters !== 'object'", + "Array.isArray(parameters)", + "const parameterKeys = Object.keys(parameters)", + "parameterKeys.length === 1", + "parameterKeys[0] === 'update_allows_fetch_and_merge'", + "parameters.update_allows_fetch_and_merge === false", "!isStrictUpdateRule(writerRuleset.rules[0])", + "typeof writerRuleset.node_id !== 'string'", + "function isStrictGraphQLUpdateRule(restRuleset, graphRuleset)", + "graphRule.parameters.updateAllowsFetchAndMerge === false", + "query ReviewerRouterWriterRule($rulesetID: ID!)", + "... on UpdateParameters", + "{rulesetID: writerRuleset.node_id}", + "!isStrictGraphQLUpdateRule(writerRuleset, graphWriterRuleset)", "current_user_can_bypass !== 'pull_requests_only'", "ruleset.current_user_can_bypass !== 'never'", "Reviewer Router App must never bypass main ruleset", @@ -449,10 +461,22 @@ func TestReviewerRouterWorkflowContract(t *testing.T) { "writerRuleset.rules?.length !== 1", "function isStrictUpdateRule(rule)", "rule?.type !== 'update'", - "rule.parameters?.update_allows_fetch_and_merge", - "updateAllowsFetchAndMerge === undefined", - "updateAllowsFetchAndMerge === false", + "!Object.prototype.hasOwnProperty.call(rule, 'parameters')", + "parameters === null", + "typeof parameters !== 'object'", + "Array.isArray(parameters)", + "const parameterKeys = Object.keys(parameters)", + "parameterKeys.length === 1", + "parameterKeys[0] === 'update_allows_fetch_and_merge'", + "parameters.update_allows_fetch_and_merge === false", "!isStrictUpdateRule(writerRuleset.rules[0])", + "typeof writerRuleset.node_id !== 'string'", + "function isStrictGraphQLUpdateRule(restRuleset, graphRuleset)", + "graphRule.parameters.updateAllowsFetchAndMerge === false", + "query ReviewerRouterWriterRule($rulesetID: ID!)", + "... on UpdateParameters", + "{rulesetID: writerRuleset.node_id}", + "!isStrictGraphQLUpdateRule(writerRuleset, graphWriterRuleset)", "current_user_can_bypass !== 'pull_requests_only'", "ruleset.current_user_can_bypass !== 'never'", "Reviewer Router App must never bypass main ruleset", @@ -560,10 +584,22 @@ func TestCodeAdmissionEnforcesReviewerRouterWriterBoundary(t *testing.T) { "writerRuleset.rules?.length !== 1", "function isStrictUpdateRule(rule)", "rule?.type !== 'update'", - "rule.parameters?.update_allows_fetch_and_merge", - "updateAllowsFetchAndMerge === undefined", - "updateAllowsFetchAndMerge === false", + "!Object.prototype.hasOwnProperty.call(rule, 'parameters')", + "parameters === null", + "typeof parameters !== 'object'", + "Array.isArray(parameters)", + "const parameterKeys = Object.keys(parameters)", + "parameterKeys.length === 1", + "parameterKeys[0] === 'update_allows_fetch_and_merge'", + "parameters.update_allows_fetch_and_merge === false", "!isStrictUpdateRule(writerRuleset.rules[0])", + "typeof writerRuleset.node_id !== 'string'", + "function isStrictGraphQLUpdateRule(restRuleset, graphRuleset)", + "graphRule.parameters.updateAllowsFetchAndMerge === false", + "query ReviewerRouterWriterRule($rulesetID: ID!)", + "... on UpdateParameters", + "{rulesetID: writerRuleset.node_id}", + "!isStrictGraphQLUpdateRule(writerRuleset, graphWriterRuleset)", "writerRuleset.current_user_can_bypass !== 'never'", "deny this built-in Actions identity any bypass", "const skipWorkflowPattern =",