diff --git a/actions/setup/js/evaluate_outcomes.cjs b/actions/setup/js/evaluate_outcomes.cjs index 677aa323050..ce1815e61f9 100644 --- a/actions/setup/js/evaluate_outcomes.cjs +++ b/actions/setup/js/evaluate_outcomes.cjs @@ -699,6 +699,168 @@ function evaluateClosePullRequest(item, defaultRepo, api = ghAPI, nowMs = Date.n return out; } +/** + * Evaluate `close_discussion`. + * @param {any} item + * @param {string} defaultRepo + * @param {(endpoint: string) => any} api + * @param {number} nowMs + * @returns {EvalResult} + */ +function evaluateCloseDiscussion(item, defaultRepo, api = ghAPI, nowMs = Date.now()) { + const repo = getItemRepo(item, defaultRepo); + const number = getItemNumber(item); + const timestamp = item.timestamp || ""; + /** @type {EvalResult} */ + const out = { + result: "unknown", + outcome_status: "unknown", + evidence_strength: "weak", + signal: "unknown", + detail: "", + resolution_sec: null, + pending_age_sec: null, + review_comments: null, + changed_files: null, + additions: null, + deletions: null, + reactions_total: null, + reactions_positive: null, + reactions_negative: null, + comments: null, + zero_touch: false, + }; + + if (!repo || !number) { + out.detail = "missing discussion reference"; + return out; + } + + const discussion = api(`repos/${repo}/discussions/${number}`); + if (!discussion || (typeof discussion.state !== "string" && typeof discussion.closed !== "boolean")) { + out.detail = "api error"; + setPendingAge(out, timestamp, nowMs); + return out; + } + + out.comments = typeof discussion.comments === "number" ? discussion.comments : null; + if (discussion.reactions && typeof discussion.reactions === "object") { + const summary = summarizeReactions(discussion.reactions); + out.reactions_total = summary.total; + out.reactions_positive = summary.positive; + out.reactions_negative = summary.negative; + } + + const isClosed = discussion.closed === true || String(discussion.state || "").toLowerCase() === "closed"; + if (isClosed) { + out.result = "accepted"; + out.outcome_status = "accepted"; + out.evidence_strength = "strong"; + out.signal = "closed"; + out.detail = "closed"; + if (discussion.created_at && discussion.closed_at) { + out.resolution_sec = secondsBetween(discussion.created_at, discussion.closed_at); + } + return out; + } + + out.result = "rejected"; + out.outcome_status = "rejected"; + out.evidence_strength = "strong"; + out.signal = "not_closed"; + out.detail = "not_closed"; + return out; +} + +/** + * Evaluate `create_discussion`. + * @param {any} item + * @param {string} defaultRepo + * @param {(endpoint: string) => any} api + * @param {number} nowMs + * @returns {EvalResult} + */ +function evaluateCreateDiscussion(item, defaultRepo, api = ghAPI, nowMs = Date.now()) { + const repo = getItemRepo(item, defaultRepo); + const number = getItemNumber(item); + const timestamp = item.timestamp || ""; + /** @type {EvalResult} */ + const out = { + result: "unknown", + outcome_status: "unknown", + evidence_strength: "weak", + signal: "unknown", + detail: "", + resolution_sec: null, + pending_age_sec: null, + review_comments: null, + changed_files: null, + additions: null, + deletions: null, + reactions_total: null, + reactions_positive: null, + reactions_negative: null, + comments: null, + zero_touch: false, + }; + + if (!repo || !number) { + out.detail = "missing discussion reference"; + return out; + } + + const discussion = api(`repos/${repo}/discussions/${number}`); + if (!discussion) { + out.detail = "api error"; + setPendingAge(out, timestamp, nowMs); + return out; + } + + out.comments = typeof discussion.comments === "number" ? discussion.comments : null; + if (discussion.reactions && typeof discussion.reactions === "object") { + const summary = summarizeReactions(discussion.reactions); + out.reactions_total = summary.total; + out.reactions_positive = summary.positive; + out.reactions_negative = summary.negative; + } + + const answered = discussion.answer_chosen_at != null || discussion.answer != null || discussion.answered === true; + if (answered) { + out.result = "accepted"; + out.outcome_status = "accepted"; + out.evidence_strength = "strong"; + out.signal = "answered"; + out.detail = "answered"; + return out; + } + + if (discussion.locked === true) { + out.result = "rejected"; + out.outcome_status = "rejected"; + out.evidence_strength = "strong"; + out.signal = "locked"; + out.detail = "locked"; + return out; + } + + if (typeof out.comments === "number" && out.comments > 0) { + out.result = "accepted"; + out.outcome_status = "accepted"; + out.evidence_strength = "medium"; + out.signal = "engaged"; + out.detail = "has replies"; + return out; + } + + out.result = "ignored"; + out.outcome_status = "ignored"; + out.evidence_strength = "medium"; + out.signal = "no_engagement"; + out.detail = "no replies"; + setPendingAge(out, timestamp, nowMs); + return out; +} + /** * Normalize legacy result/detail pairs into the shared outcome model. * @param {string} result @@ -797,7 +959,7 @@ function normalizeOutcome(result, detail) { function getItemNumber(item) { if (typeof item.number === "number" && Number.isFinite(item.number)) return item.number; const url = item.url || ""; - const issueMatch = url.match(/\/(?:issues|pull)\/(\d+)/); + const issueMatch = url.match(/\/(?:issues|pull|discussions)\/(\d+)/); if (issueMatch) return Number(issueMatch[1]); return null; } @@ -1370,6 +1532,12 @@ function evaluateItem(item, defaultRepo, apiOrOptions) { if (type === "close_pull_request") { return evaluateClosePullRequest(item, defaultRepo, ghAPIFn, nowMs); } + if (type === "close_discussion") { + return evaluateCloseDiscussion(item, defaultRepo, ghAPIFn, nowMs); + } + if (type === "create_discussion") { + return evaluateCreateDiscussion(item, defaultRepo, ghAPIFn, nowMs); + } if (type === "create_issue") { return evaluateCreateIssue(item, itemRepo, timestamp, out, ghAPIFn, nowMs); } @@ -2127,6 +2295,8 @@ module.exports = { evaluateAddLabels, evaluateCloseIssue, evaluateClosePullRequest, + evaluateCloseDiscussion, + evaluateCreateDiscussion, evaluateCreatePullRequestOutcome, evaluatePushToPullRequestBranchOutcome, normalizeOutcome, diff --git a/actions/setup/js/evaluate_outcomes.test.cjs b/actions/setup/js/evaluate_outcomes.test.cjs index 010f1b95bf1..a40df9002cf 100644 --- a/actions/setup/js/evaluate_outcomes.test.cjs +++ b/actions/setup/js/evaluate_outcomes.test.cjs @@ -1028,6 +1028,159 @@ describe("evaluate_outcomes create_pull_request evaluator", () => { }); }); +describe("evaluate_outcomes discussion evaluators", () => { + it("classifies closed discussions as accepted", () => { + const ghAPI = mockAPI({ + "repos/owner/repo/discussions/11": { + state: "closed", + closed: true, + comments: 2, + created_at: "2026-05-20T00:00:00Z", + closed_at: "2026-05-21T00:00:00Z", + }, + }); + + const result = evaluateItem( + { + type: "close_discussion", + repo: "owner/repo", + url: "https://github.com/owner/repo/discussions/11", + timestamp: "2026-05-20T00:00:00Z", + }, + "owner/repo", + { ghAPI } + ); + + expect(result.result).toBe("accepted"); + expect(result.detail).toBe("closed"); + expect(result.signal).toBe("closed"); + }); + + it("classifies reopened discussions as rejected", () => { + const ghAPI = mockAPI({ + "repos/owner/repo/discussions/12": { + state: "open", + closed: false, + comments: 1, + }, + }); + + const result = evaluateItem( + { + type: "close_discussion", + repo: "owner/repo", + url: "https://github.com/owner/repo/discussions/12", + timestamp: "2026-05-20T00:00:00Z", + }, + "owner/repo", + { ghAPI } + ); + + expect(result.result).toBe("rejected"); + expect(result.detail).toBe("not_closed"); + expect(result.signal).toBe("not_closed"); + }); + + it("classifies answered discussions as accepted", () => { + const ghAPI = mockAPI({ + "repos/owner/repo/discussions/13": { + state: "open", + comments: 0, + answer_chosen_at: "2026-05-21T00:00:00Z", + }, + }); + + const result = evaluateItem( + { + type: "create_discussion", + repo: "owner/repo", + url: "https://github.com/owner/repo/discussions/13", + timestamp: "2026-05-20T00:00:00Z", + }, + "owner/repo", + { ghAPI } + ); + + expect(result.result).toBe("accepted"); + expect(result.detail).toBe("answered"); + expect(result.signal).toBe("answered"); + }); + + it("classifies discussions with replies as accepted", () => { + const ghAPI = mockAPI({ + "repos/owner/repo/discussions/14": { + state: "open", + comments: 3, + }, + }); + + const result = evaluateItem( + { + type: "create_discussion", + repo: "owner/repo", + url: "https://github.com/owner/repo/discussions/14", + timestamp: "2026-05-20T00:00:00Z", + }, + "owner/repo", + { ghAPI } + ); + + expect(result.result).toBe("accepted"); + expect(result.detail).toBe("has replies"); + expect(result.signal).toBe("engaged"); + }); + + it("classifies locked discussions as rejected", () => { + const ghAPI = mockAPI({ + "repos/owner/repo/discussions/15": { + state: "open", + comments: 0, + locked: true, + }, + }); + + const result = evaluateItem( + { + type: "create_discussion", + repo: "owner/repo", + url: "https://github.com/owner/repo/discussions/15", + timestamp: "2026-05-20T00:00:00Z", + }, + "owner/repo", + { ghAPI } + ); + + expect(result.result).toBe("rejected"); + expect(result.detail).toBe("locked"); + expect(result.signal).toBe("locked"); + }); + + it("classifies unengaged discussions as ignored", () => { + const ghAPI = mockAPI({ + "repos/owner/repo/discussions/16": { + state: "open", + comments: 0, + locked: false, + }, + }); + + const result = evaluateItem( + { + type: "create_discussion", + repo: "owner/repo", + url: "https://github.com/owner/repo/discussions/16", + timestamp: "2026-05-20T00:00:00Z", + }, + "owner/repo", + { ghAPI } + ); + + expect(result.result).toBe("ignored"); + expect(result.detail).toBe("no replies"); + expect(result.signal).toBe("no_engagement"); + }); +}); + describe("evaluate_outcomes push_to_pull_request_branch evaluator", () => { beforeEach(() => { vi.useFakeTimers(); diff --git a/pkg/workflow/security_architecture_sg_formal_test.go b/pkg/workflow/security_architecture_sg_formal_test.go index 25d2834d672..3b2d96aac57 100644 --- a/pkg/workflow/security_architecture_sg_formal_test.go +++ b/pkg/workflow/security_architecture_sg_formal_test.go @@ -20,6 +20,7 @@ // ThreatDetectionOrDefault → TestFormalThreatDetection_EnabledByDefault // ThreatDetectionOrDefault → TestFormalThreatDetection_ExplicitDisable // IsContinueOnError → TestFormalThreatDetection_ContinueOnErrorDefault +// PM11_PreActivationMembership → TestFormalPM11_PreActivationContainsMembershipStep // StagedHandlerNoWritePerms → TestFormalStaged_HandlerRequiresNoWritePerms // IDTokenRequirement → TestFormalIDToken_OIDCVaultActionsRequireWriteScope // getPushFallbackAsPullRequest → TestFormalPushFallback_DefaultsToTrue @@ -445,6 +446,50 @@ func TestFormalThreatDetection_ContinueOnErrorDefault(t *testing.T) { "IsContinueOnError: must return false when ContinueOnError is explicitly set to false (blocking mode)") } +// TestFormalPM11_PreActivationContainsMembershipStep (PM11_PreActivationMembership) +// +// PM-11: Role checks MUST be performed at runtime using membership validation in +// the pre_activation job before activation may proceed. +func TestFormalPM11_PreActivationContainsMembershipStep(t *testing.T) { + md := `--- +name: pm11-membership-test +on: + pull_request: + types: [opened] + roles: + - write +engine: copilot +permissions: + contents: read +--- + +# Mission + +PM-11: verify the compiled pre_activation job contains the membership-check step. +` + tmpDir := t.TempDir() + mdPath := filepath.Join(tmpDir, "workflow.md") + require.NoError(t, os.WriteFile(mdPath, []byte(md), 0600)) + + compiler := NewCompiler(WithNoEmit(true)) + wd, err := compiler.ParseWorkflowFile(mdPath) + require.NoError(t, err, "PM-11: workflow with role-based access control must parse successfully") + + yamlOut, err := compiler.CompileToYAML(wd, mdPath) + require.NoError(t, err, "PM-11: workflow with role-based access control must compile successfully") + require.NotEmpty(t, yamlOut, "PM-11: compiled YAML must not be empty") + + preActivationSection := extractJobSection(yamlOut, string(constants.PreActivationJobName)) + require.NotEmpty(t, preActivationSection, + "PM-11: compiled workflow must contain the pre_activation job") + assert.Contains(t, preActivationSection, "id: "+string(constants.CheckMembershipStepID), + "PM-11: pre_activation job must contain the check_membership step ID") + assert.Contains(t, preActivationSection, "check_membership.cjs", + "PM-11: pre_activation job must invoke check_membership.cjs for runtime membership validation") + assert.Contains(t, preActivationSection, `GH_AW_REQUIRED_ROLES: "write"`, + "PM-11: pre_activation job must pass the configured role set to the membership check step") +} + // TestFormalStaged_HandlerRequiresNoWritePerms (StagedHandlerNoWritePerms) // // Invariant: staged safe-output handlers must not produce write-permission diff --git a/scratchpad/github-mcp-access-control-specification.md b/scratchpad/github-mcp-access-control-specification.md index 856e46dbbc1..50665922738 100644 --- a/scratchpad/github-mcp-access-control-specification.md +++ b/scratchpad/github-mcp-access-control-specification.md @@ -1803,7 +1803,11 @@ Implementations MUST log all access control decisions with the following informa - Permission queries count against GitHub API rate limits - Implementations SHOULD implement caching to reduce API calls -- Rate limit errors SHOULD be handled gracefully with retries +- On `403` or `429` responses that indicate GitHub rate limiting, implementations MUST fail closed for the current access-control decision and MUST NOT bypass repository, role, visibility, or integrity checks using stale "allow" results. +- On `403` or `429` rate-limit responses, implementations MUST either serve a still-valid cached authorization decision whose scope exactly matches the current repository/tool request or return a retryable denial/error; they MUST NOT silently downgrade enforcement. +- A cached authorization decision is still valid only if its TTL has not expired, it was computed for the same caller identity, repository target, tool/action, and required permission scope, and no invalidation trigger (for example token rotation, membership/role change, repository visibility change, or integrity-policy change) has been observed since it was stored. +- On `5xx` responses from the GitHub MCP server or the underlying GitHub API during an authorization check, implementations MUST treat the authorization state as indeterminate and deny the request until a successful re-check completes. +- Retry logic for `403`/`429` and `5xx` responses MUST use bounded exponential backoff and MUST NOT continue retrying past the workflow or request deadline. #### 9.4.3 Error Message Information Disclosure diff --git a/specs/safe-output-outcome-evaluation.md b/specs/safe-output-outcome-evaluation.md index 73351a2748e..d80b4985b39 100644 --- a/specs/safe-output-outcome-evaluation.md +++ b/specs/safe-output-outcome-evaluation.md @@ -128,14 +128,14 @@ Rows marked `evalGenericSticky` fallback are generic existence checks, not type- | `update_pull_request` | implemented | `pkg/workflow/safe_outputs_config.go`, `pkg/cli/outcome_eval_update.go` | `actions/setup/js/update_pull_request.cjs`, `actions/setup/js/evaluate_outcomes.cjs` | | `close_issue` | partial | `pkg/workflow/safe_outputs_dispatch.go`, `pkg/cli/outcome_eval.go` (`evalCloseSticky`) | `actions/setup/js/close_issue.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (`evaluateCloseIssue`) | | `close_pull_request` | partial | `pkg/workflow/safe_outputs_dispatch.go`, `pkg/cli/outcome_eval.go` (`evalCloseSticky`) | `actions/setup/js/close_pull_request.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (`evaluateClosePullRequest`) | -| `close_discussion` | partial | `pkg/workflow/safe_outputs_dispatch.go`, `pkg/cli/outcome_eval.go` (`evalCloseDiscussion`) | `actions/setup/js/close_discussion.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | -| `create_discussion` | partial | `pkg/workflow/safe_outputs_dispatch.go`, `pkg/cli/outcome_eval.go` (`evalCreateDiscussion`) | `actions/setup/js/create_discussion.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | +| `close_discussion` | implemented | `pkg/workflow/safe_outputs_dispatch.go`, `pkg/cli/outcome_eval.go` (`evalCloseDiscussion`) | `actions/setup/js/close_discussion.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (`evaluateCloseDiscussion`) | +| `create_discussion` | implemented | `pkg/workflow/safe_outputs_dispatch.go`, `pkg/cli/outcome_eval.go` (`evalCreateDiscussion`) | `actions/setup/js/create_discussion.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (`evaluateCreateDiscussion`) | | `update_discussion` | partial | `pkg/workflow/safe_outputs_config.go`, `pkg/cli/outcome_eval_workflow.go` (`evalUpdateDiscussion`) | `actions/setup/js/update_discussion.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | | `create_pull_request_review_comment` | partial | `pkg/workflow/safe_outputs_config.go`, `pkg/cli/outcome_eval.go` (`evalReviewComment`) | `actions/setup/js/create_pr_review_comment.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | | `submit_pull_request_review` | implemented | `pkg/workflow/safe_outputs_config.go`, `pkg/cli/outcome_eval_review.go` | `actions/setup/js/submit_pr_review.cjs`, `actions/setup/js/evaluate_outcomes.cjs` | | `reply_to_pull_request_review_comment` | not-started | `pkg/workflow/safe_outputs_config.go`, `pkg/cli/outcome_eval.go` (`evalGenericSticky` fallback) | `actions/setup/js/reply_to_pr_review_comment.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | | `resolve_pull_request_review_thread` | partial | `pkg/workflow/safe_outputs_config.go`, `pkg/cli/outcome_eval.go` (`evalResolveThread`) | `actions/setup/js/resolve_pr_review_thread.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | -| `push_to_pull_request_branch` | partial | `pkg/workflow/push_to_pull_request_branch_validation.go`, `pkg/cli/outcome_eval.go` (`evalPushToPRBranch`) | `actions/setup/js/push_to_pull_request_branch.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | +| `push_to_pull_request_branch` | implemented | `pkg/workflow/push_to_pull_request_branch_validation.go`, `pkg/cli/outcome_eval.go` (`evalPushToPRBranch`) | `actions/setup/js/push_to_pull_request_branch.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (`evaluatePushToPullRequestBranchOutcome`) | | `mark_pull_request_as_ready_for_review` | partial | `pkg/workflow/safe_outputs_config.go`, `pkg/cli/outcome_eval.go` (`evalMarkReady`) | `actions/setup/js/mark_pull_request_as_ready_for_review.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | | `assign_to_agent` | partial | `pkg/workflow/safe_outputs_dispatch.go`, `pkg/cli/outcome_eval.go` (`evalAssignToAgent`) | `actions/setup/js/assign_to_agent.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | | `dispatch_workflow` | partial | `pkg/workflow/dispatch_workflow.go`, `pkg/cli/outcome_eval_workflow.go` (`evalDispatchWorkflow`) | `actions/setup/js/dispatch_workflow.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | diff --git a/specs/security-architecture-spec-summary.md b/specs/security-architecture-spec-summary.md index 03f034f27a7..3ef87e49b76 100644 --- a/specs/security-architecture-spec-summary.md +++ b/specs/security-architecture-spec-summary.md @@ -172,6 +172,7 @@ JobTopologyOrder ≜ | `ThreatDetectionOrDefault` | `TestFormalThreatDetection_EnabledByDefault` | Threat detection auto-injected when safe-outputs configured without explicit disable | | `ThreatDetectionOrDefault` | `TestFormalThreatDetection_ExplicitDisable` | Threat detection returns nil when explicitly set to false | | `IsContinueOnError` | `TestFormalThreatDetection_ContinueOnErrorDefault` | IsContinueOnError defaults to true when field is nil | +| `PM11_PreActivationMembership` | `TestFormalPM11_PreActivationContainsMembershipStep` | `pre_activation` job contains the runtime `check_membership` gate when RBAC is enabled | | `StagedHandlerNoWritePerms` | `TestFormalStaged_HandlerRequiresNoWritePerms` | Staged safe-output handlers do not require write permissions | | `IDTokenRequirement` | `TestFormalIDToken_OIDCVaultActionsRequireWriteScope` | OIDC vault actions trigger id-token:write requirement | | `getPushFallbackAsPullRequest` | `TestFormalPushFallback_DefaultsToTrue` | Push fallback-as-pull-request defaults to true when config is nil | @@ -179,7 +180,7 @@ JobTopologyOrder ≜ ### Generated Test Suite -The 15 test functions above are implemented in +The 16 test functions above are implemented in `pkg/workflow/security_architecture_sg_formal_test.go` using the Go `testify` library. All tests carry the `//go:build !integration` tag so they run in the default unit-test suite without any special flags. @@ -195,7 +196,7 @@ Each test function: Run the full suite: ```sh -go test ./pkg/workflow/ -run 'TestFormalSG|TestFormalBasicConformance|TestFormalThreatDetection|TestFormalStaged|TestFormalIDToken|TestFormalPushFallback|TestFormalJobTopology' -v +go test ./pkg/workflow/ -run 'TestFormalSG|TestFormalBasicConformance|TestFormalThreatDetection|TestFormalPM11|TestFormalStaged|TestFormalIDToken|TestFormalPushFallback|TestFormalJobTopology' -v ``` ### Formal Requirements @@ -348,6 +349,7 @@ Summary version **1.0.0** corresponds to the minimum validated `.lock.yml` compi | Track conclusion job note from validation doc | ✅ Done (2026-07-06) | Documented the optional `conclusion` job as non-normative cleanup/reporting guidance | | Audit trusted-users runtime enforcement coverage | ✅ Done (2026-07-06) | Sections 8-9 now document runtime `trusted-users` enforcement scope directly in this spec summary (membership checks gate privileged runtime access) | | Add formal model and test suite for SG-01 through SG-07 | ✅ Done (2026-07-09) | Added "Formal Model" (TLA+/F*/Z3 invariants), "Behavioral Coverage Map" (15 predicates), and "Generated Test Suite" sections; 15 tests in `pkg/workflow/security_architecture_sg_formal_test.go` | +| Sync PM-11 formal coverage into behavioral coverage map | ✅ Done (2026-07-15) | Added `TestFormalPM11_PreActivationContainsMembershipStep` to the behavioral coverage map and generated suite notes; formal suite now tracks 16 tests in `pkg/workflow/security_architecture_sg_formal_test.go` | ## Versioning diff --git a/specs/security-architecture-spec-validation.md b/specs/security-architecture-spec-validation.md index c2a96fa684e..bc51f9590cd 100644 --- a/specs/security-architecture-spec-validation.md +++ b/specs/security-architecture-spec-validation.md @@ -9,7 +9,7 @@ ## Executive Summary -✅ **VALIDATION RESULT**: The specification accurately reflects the implementation in compiled `.lock.yml` files and JavaScript implementation (revalidated on 2026-07-06). +✅ **VALIDATION RESULT**: The specification accurately reflects the implementation in compiled `.lock.yml` files and JavaScript implementation (revalidated on 2026-07-15). All major security architecture claims in the specification have been verified against actual workflow implementations: - ✅ Job architecture (activation, agent, safe_outputs) @@ -17,6 +17,10 @@ All major security architecture claims in the specification have been verified a - ✅ Permission management (read-only agent jobs, write permissions in safe output jobs) - ✅ Fork protection (repository ID validation) - ✅ Role-based access control (pre_activation with membership checks) +- ✅ PM-11 formal coverage (`TestFormalPM11_PreActivationContainsMembershipStep`) +- ✅ Output isolation evidence for handler coverage, token precedence, and write-job separation +- ✅ Network isolation evidence for allowlist validation, protocol filtering, ecosystem expansion, and MCP/firewall wrapping +- ⚠️ Sandbox isolation evidence now documented from compiled AWF chroot/container invocations (representative lock-file evidence; direct runtime host-visibility proof remains partial) - ✅ Threat detection layer (detection job between agent and safe_outputs) - ✅ Action pinning to SHAs - ✅ Timestamp validation at runtime @@ -262,19 +266,17 @@ The July 2026 maintenance pass rechecked the documentation-only clarifications r --- -### 4b. PM-11 Formal Test Coverage Audit (2026-07-11) +### 4b. PM-11 Formal Test Coverage (2026-07-15) -An audit of `pkg/workflow/security_architecture_sg_formal_test.go` was performed to verify whether PM-11 (pre_activation membership validation) is covered by a dedicated formal test. +PM-11 (pre_activation membership validation) is now covered by a dedicated formal test in `pkg/workflow/security_architecture_sg_formal_test.go`. -**Finding**: PM-11 is **NOT** directly covered by a formal test in `security_architecture_sg_formal_test.go`. +- `TestFormalPM11_PreActivationContainsMembershipStep` compiles a real workflow with `on.roles` configured. +- The test extracts the compiled `pre_activation` job section and asserts the presence of: + - `id: check_membership` + - `check_membership.cjs` + - `GH_AW_REQUIRED_ROLES: "write"` -- `TestFormalSG04_LeastPrivilegeBasePermissions` covers SG-04 (least-privilege permissions baseline) but does not assert the presence or correctness of a membership-check step inside the `pre_activation` job. -- `TestFormalJobTopology_PipelineOrderEnforced` verifies that the `pre_activation → activation` needs dependency is present in the compiled YAML but does NOT assert that the `pre_activation` job contains the `check_membership.cjs` step required by PM-11. -- The runtime evidence for PM-11 (compiled `check_membership.cjs` step in `pkg/workflow/test-yaml-import.lock.yml`) is verified in §4 above from the lock-file artefact, but this is not a programmatic assertion. - -**Gap**: A dedicated formal test `TestFormalPM11_PreActivationContainsMembershipStep` should be added to `security_architecture_sg_formal_test.go`. The test should compile a real workflow with role-based access control enabled and assert that the compiled YAML contains a `check_membership` step inside the `pre_activation` job section. - -**Verification date**: 2026-07-11. Track gap via `specs/security-architecture-spec.md` Appendix G.10. +**Status**: ✅ **VERIFIED** — PM-11 now has programmatic formal coverage in addition to the lock-file evidence in §4 above. --- @@ -383,6 +385,46 @@ activation: --- +### 8a. Output Isolation Supplemental Evidence (T-OI-003 to T-OI-007) + +The compliance-matrix gaps for output isolation are now supported by direct implementation evidence: + +- **T-OI-003 / T-OI-004 — safe output type support and validation rules**: `actions/setup/js/collect_ndjson_output.test.cjs` exercises `create_discussion` schema handling (required fields, min/max handling, mixed-type batches), while `pkg/workflow/compiler_safe_outputs_steps.go` wires agent output download plus the single `Process Safe Outputs` dispatcher step that performs validation before any write action is applied. +- **T-OI-005 — token precedence**: `pkg/workflow/github_token.go` documents and implements the checkout/push token precedence chain, and `pkg/workflow/github_token_test.go` verifies the ordering (per-output PAT overrides checkout-scoped safe-output app token, which overrides safe-outputs level fallbacks). `pkg/workflow/compiler_safe_outputs_steps.go` then persists that resolved token into checkout credentials and the `GITHUB_TOKEN` environment for trusted safe-output handlers only. +- **T-OI-006 — token secret-expression handling**: `pkg/workflow/github_token.go` emits GitHub Actions secret expressions (`${{ secrets... }}`) instead of raw values for safe-output tokens, and `pkg/workflow/safe_outputs_validation.go` rejects configurations that would attempt workflow-scoped writes without a GitHub App. +- **T-OI-007 — write-operation isolation**: the compiled architecture in §1 still isolates write-capable processing to `safe_outputs`, after `activation`, `agent`, and `detection`, with read-only permissions preserved on the agent job and write permissions reserved for the safe-output job. + +**Status**: ✅ **VERIFIED** — dedicated evidence now exists for T-OI-003 through T-OI-007. + +--- + +### 8b. Network Isolation Supplemental Evidence (T-NI-001 to T-NI-009) + +The network-isolation claims now have concrete code and compiled-workflow evidence: + +- **T-NI-001 / T-NI-002 / T-NI-003**: `pkg/workflow/network_firewall_validation.go` validates firewall configuration, explicit allowed domains, ecosystem identifiers, and wildcard-domain patterns. +- **T-NI-004 / T-NI-005 / T-NI-006**: the same validator rejects invalid protocols and malformed wildcard patterns, while `actions/setup/js/sanitize_content_core.cjs` applies `sanitizeUrlProtocols()` and `sanitizeUrlDomains()` so runtime content sanitization and compiled allowlist semantics stay aligned. +- **T-NI-007**: §8 above already verifies AWF installation in compiled output. +- **T-NI-008**: representative compiled workflows such as `.github/workflows/portfolio-analyst.lock.yml` run the agent inside AWF with an explicit `--mcp-config "${RUNNER_TEMP}/gh-aw/mcp-config/mcp-servers.json"` argument and sandbox mounts rooted under `${RUNNER_TEMP}/gh-aw`, evidencing that MCP access is routed through the sandbox/firewall wrapper rather than direct host execution. +- **T-NI-009**: the `GH_AW_ALLOWED_DOMAINS` environment built in `pkg/workflow/compiler_safe_outputs_steps.go` feeds the same allowlist into runtime safe-output sanitization, closing the gap between network policy and content filtering. + +**Status**: ✅ **VERIFIED** — the validation report now includes evidence for T-NI-001 through T-NI-009. + +--- + +### 8c. Sandbox Isolation Supplemental Evidence (T-SI-001 to T-SI-007) + +Sandbox isolation was previously undocumented in this report; representative compiled-workflow evidence is now recorded: + +- **T-SI-001 / T-SI-004**: `actions/setup/js/patch_awf_chroot_config.cjs` injects a `chroot` block into `awf-config.json` with explicit `binariesSourcePath` and runtime identity (`user`, `uid`, `gid`, `home`). +- **T-SI-002 / T-SI-003 / T-SI-005**: `.github/workflows/portfolio-analyst.lock.yml` patches the AWF config when `DOCKER_HOST` points at a TCP daemon, then launches AWF with read-only mounts, `--env-all`, and explicit `--exclude-env` filters for sensitive tokens. The same compiled invocation passes the Docker endpoint via `--docker-host` rather than mounting `/var/run/docker.sock` into the sandbox. +- **T-SI-006**: the compiled AWF command mounts only the staged `${RUNNER_TEMP}/gh-aw` tool/config tree and passes MCP configuration through that sandboxed mount, rather than exposing arbitrary host state to the agent container. +- **T-SI-007**: the representative detection job uses a separate AWF invocation (including the same chroot patch path) even when host access is explicitly enabled for detection, showing that sandboxing and network policy remain composed controls rather than mutually exclusive modes. + +**Status**: ⚠️ **PARTIALLY EVIDENCED** — compiled lock-file and runtime-script evidence now cover all T-SI identifiers at least partially, but this report still lacks a direct runtime probe demonstrating host/socket invisibility from inside the sandbox. + +--- + ### 9. Output Validation (Section 5.4 - OI-06) **Specification Claim**: @@ -556,10 +598,10 @@ This section audits the compliance test matrix defined in `security-architecture | Test Category | Test IDs | Status | Notes | |---------------|----------|--------|-------| | Input Sanitization | T-IS-001 to T-IS-008 | ✅ EVIDENCED | Covered in §1a (IS-04 to IS-09); all sanitization functions verified in `sanitize_content_core.cjs` | -| Output Isolation | T-OI-001 to T-OI-007 | ⚠️ PARTIALLY EVIDENCED | OI-01 (job architecture) and OI-06 (output validation) verified; OI-02 (agent read-only) implicitly verified via permission blocks; T-OI-003 (safe output type support), T-OI-004 (output validation rules), T-OI-005 (token precedence), T-OI-006 (token secret expression), T-OI-007 (write operation isolation) lack dedicated evidence entries | -| Network Isolation | T-NI-001 to T-NI-009 | ⚠️ PARTIALLY EVIDENCED | AWF binary installation (T-NI-007) verified; T-NI-001 (network mode support), T-NI-002 (ecosystem expansion), T-NI-003 (domain matching), T-NI-004 (protocol filtering), T-NI-005 (invalid protocol), T-NI-006 (blocked domain precedence), T-NI-008 (MCP isolation), T-NI-009 (content sanitization integration) lack dedicated evidence entries | +| Output Isolation | T-OI-001 to T-OI-007 | ✅ EVIDENCED | OI-01/OI-06 remain verified, and §8a now adds dedicated evidence for T-OI-003 through T-OI-007 (type coverage, validation rules, token precedence, secret-expression handling, and write-job isolation) | +| Network Isolation | T-NI-001 to T-NI-009 | ✅ EVIDENCED | §8 and §8b now cover AWF installation, ecosystem/domain validation, protocol filtering, blocked-domain precedence, MCP sandbox routing, and allowlist-driven content sanitization | | Permission Management | T-PM-001 to T-PM-007 | ⚠️ PARTIALLY EVIDENCED | PM-01/PM-02 (permission defaults), PM-08 (fork protection), PM-10/PM-11 (RBAC) verified; T-PM-003 (strict mode), T-PM-005 (repository validation for `workflow_run`), T-PM-007 (token validation) lack dedicated evidence entries | -| Sandbox Isolation | T-SI-001 to T-SI-007 | ❌ GAP | No sandbox isolation evidence entries are present in this validation document; T-SI-001 through T-SI-007 (AWF chroot, Docker socket, `--env-all`, GOROOT, MCP container, network isolation independence) are not covered | +| Sandbox Isolation | T-SI-001 to T-SI-007 | ⚠️ PARTIALLY EVIDENCED | §8c adds compiled-workflow and runtime-script evidence for AWF chrooting, docker-host indirection, environment filtering, MCP/tool mounts, and composed sandbox/firewall operation; direct runtime host-visibility proof remains outstanding | | Threat Detection | T-TD-001 to T-TD-007 | ⚠️ PARTIALLY EVIDENCED | TD-01 (automatic threat detection) verified via `detection:` job; T-TD-002 (prompt injection), T-TD-003 (secret leaks), T-TD-004 (malicious patches), T-TD-005 (custom prompt), T-TD-006 (engine override), T-TD-007 (workflow failure on detection) lack dedicated evidence entries | | Compilation-Time Security | T-CS-001 to T-CS-006, T-SG07-001, T-SG07-002 | ⚠️ PARTIALLY EVIDENCED | CS-10 (action pinning, T-CS-005) verified; T-CS-001 (schema validation), T-CS-002 (expression safety), T-CS-003 (permission validation), T-CS-004 (network config validation), T-CS-006 (deprecated feature rejection), T-SG07-001 and T-SG07-002 (fail-secure behaviors) lack dedicated evidence entries | | Runtime Security | T-RS-001 to T-RS-011 | ⚠️ PARTIALLY EVIDENCED | RS-01/RS-02 (timestamp validation) and RS-16 to RS-22 (concurrency control) verified; T-RS-003 through T-RS-008 (repository validation for `workflow_run`, role validation, token validation, AWF/MCP network enforcement, output validation) lack dedicated evidence entries | @@ -569,14 +611,12 @@ This section audits the compliance test matrix defined in `security-architecture The following test categories require dedicated evidence entries to achieve full coverage in this validation document: -1. **Sandbox Isolation (T-SI-001 to T-SI-007)** — No evidence present. A re-validation pass MUST document how AWF chroot behavior, Docker socket visibility, and MCP container isolation are reflected in compiled workflow behavior. -2. **Output Isolation (T-OI-003 to T-OI-007)** — Partial evidence. Missing evidence for token precedence, secret expression validation, and write operation isolation. -3. **Network Isolation (T-NI-001 to T-NI-009)** — Partial evidence. Network mode, domain matching, protocol filtering, and MCP isolation behavior are unverified in this document. -4. **Threat Detection (T-TD-002 to T-TD-007)** — Partial evidence. Only TD-01 (job presence) verified; detection capability assertions unverified. -5. **Compilation-Time Security (T-CS-001 to T-CS-004, T-CS-006, T-SG07)** — Partial evidence. Only action pinning (T-CS-005) verified. -6. **Runtime Security (T-RS-003 to T-RS-008)** — Partial evidence. Only timestamp and concurrency verified. +1. **Sandbox Isolation (T-SI-001 to T-SI-007)** — Reduced from full gap to partial evidence in §8c. Remaining work is a direct runtime probe for host/socket visibility from inside the AWF sandbox. +2. **Threat Detection (T-TD-002 to T-TD-007)** — Partial evidence. Only TD-01 (job presence) verified; detection capability assertions remain unverified in this report. +3. **Compilation-Time Security (T-CS-001 to T-CS-004, T-CS-006, T-SG07)** — Partial evidence. Only action pinning (T-CS-005) verified. +4. **Runtime Security (T-RS-003 to T-RS-008)** — Partial evidence. Only timestamp and concurrency verified. -Maintainers SHOULD address gaps in order of risk: Sandbox Isolation and Network Isolation are the highest-priority gaps given their role in containing agent execution. +Maintainers SHOULD address remaining gaps in order of risk: the residual sandbox-runtime probe, then threat-detection capability evidence, then the remaining compilation-time and runtime security categories. ---