diff --git a/scratchpad/github-mcp-access-control-specification.md b/scratchpad/github-mcp-access-control-specification.md index cdf29b18d1f..856e46dbbc1 100644 --- a/scratchpad/github-mcp-access-control-specification.md +++ b/scratchpad/github-mcp-access-control-specification.md @@ -2099,6 +2099,12 @@ Additional blocked-user validation tests in `TestValidateGitHubGuardPolicy`: - **T-GH-059**: No `min-integrity` configured → all non-blocked items pass integrity check - **T-GH-060**: Integrity ordinal order: none < unapproved < approved < merged +#### 11.1.10 Combined P5+P6 Evaluation Tests + +- **T-GH-091**: When both `blocked-users` and `min-integrity` are configured, access is the conjunction of P5_NotBlocked AND P6_IntegrityMet; a non-blocked user with content at or above the threshold is allowed +- **T-GH-092**: Non-blocked user with content exceeding the configured threshold is allowed; P5 and P6 both pass +- **T-GH-093**: Blocked user whose content also fails the integrity threshold is denied with `-32005` (P5_NotBlocked fires before P6_IntegrityMet per §8.5 combined evaluation order), not `-32006` + ### 11.2 Compliance Checklist | Requirement | Test ID | Level | Status | @@ -2147,6 +2153,7 @@ The following fixture files in [`specs/github-mcp-access-control-compliance/`](. | [`role-deny.yaml`](../../specs/github-mcp-access-control-compliance/role-deny.yaml) | Role filter allows matching role; denies insufficient role | T-GH-019, T-GH-020, T-GH-023 | | [`private-repo-block.yaml`](../../specs/github-mcp-access-control-compliance/private-repo-block.yaml) | `private-repos: false` blocks private repo; allows public repo | T-GH-024, T-GH-025, T-GH-026 | | [`integrity-level-block.yaml`](../../specs/github-mcp-access-control-compliance/integrity-level-block.yaml) | `min-integrity` allows content at/above threshold; blocks content below | T-GH-051, T-GH-052, T-GH-054 | +| [`combined-blocked-integrity.yaml`](../../specs/github-mcp-access-control-compliance/combined-blocked-integrity.yaml) | Combined P5+P6: blocked user denied with `-32005` even when P6 would also fail; non-blocked user with sufficient integrity allowed | T-GH-091, T-GH-092, T-GH-093 | See [`specs/github-mcp-access-control-compliance/README.md`](../../specs/github-mcp-access-control-compliance/README.md) for fixture schema documentation and instructions for adding new scenarios. @@ -2697,6 +2704,34 @@ Cross-reference of `scratchpad/github-mcp-access-control-specification.md` and ` --- +## Sync Follow-ups + +This section lists the files that **MUST** be reviewed and updated whenever a normative section of this specification changes, especially §8.5 (combined evaluation order), §4.4 (field definitions), and §11 (compliance tests). + +### After Changing §8.5 Combined Evaluation Order + +When the relative order of P5_NotBlocked and P6_IntegrityMet (or any other guard predicates in §4.6.3) changes: + +1. **Gateway runtime implementation** — The production P1–P6 guard-chain evaluator lives in the gateway implementation (not in this repository). Locate the six-predicate evaluation loop in the gateway and update the ordering and first-failing-guard error-code selection logic accordingly. (The local formal model in `pkg/workflow/github_mcp_access_control_formal_test.go` — function `formalEvaluateAccess` — mirrors the spec ordering for test purposes only; update it in parallel.) + +2. **`specs/github-mcp-access-control-compliance/README.md`** — Update the Formal Model section (`ALLOW(r, c) ≜ …`) and the Behavioral Coverage Map table to reflect the new predicate order. Regenerate `TestFormal_BlockedUserSafetyProperty` and `TestFormal_ErrorCodeFirstFailingGuard` test cases to match. + +3. **`specs/github-mcp-access-control-compliance/combined-blocked-integrity.yaml`** — Review the `combined-blocked-integrity-C` scenario whose expected error code (`-32005`) depends on P5_NotBlocked firing before P6_IntegrityMet. Update the expected `error_code` if the predicate order changes. (Scenario D is unaffected: the user is blocked AND integrity is `merged` ≥ `approved` threshold, so P6 already passes regardless of evaluation order.) + +### After Adding or Removing §4.4 Extension Fields + +When a new access-control field is added (e.g., `trusted-users`, `approval-labels`) or an existing field is removed: + +1. **`pkg/workflow/tools_types.go`** — Add or remove the corresponding `GitHubToolConfig` struct field and YAML tag. + +2. **`pkg/workflow/tools_validation_github.go`** — Add validation logic for the new field. Remove validation for any removed field. + +3. **`pkg/workflow/github_mcp_access_control_formal_test.go`** — Add a predicate-mapped `TestFormal_*` test function for the new field; remove obsolete tests for removed fields. + +4. **`specs/github-mcp-access-control-compliance/README.md`** — Update the Behavioral Coverage Map table and the Fixture Files table. Regenerate or create the corresponding YAML fixture file. + +--- + ## References ### Normative References diff --git a/scratchpad/guard-policies-specification.md b/scratchpad/guard-policies-specification.md index 8bcb547fee1..e00c8617fd1 100644 --- a/scratchpad/guard-policies-specification.md +++ b/scratchpad/guard-policies-specification.md @@ -483,11 +483,11 @@ The key words in this section are to be interpreted as described in RFC 2119 (se A conforming implementation of the guard policies framework **MUST** satisfy all of the following normative requirements: -**GP-01**: Implementations MUST support the `allowed-repos` field on `GitHubToolConfig` and validate its value as either a string scalar (`"all"` or `"public"`) or an array of repository patterns. Implementations MUST reject any other type with a descriptive compilation error. +**GP-01**: Implementations MUST support the `allowed-repos` field on `GitHubToolConfig` and validate its value as either a string scalar (`"all"`, `"public"`, or the expression `"${{ github.repository }}"`) or a non-empty array of repository patterns. Implementations MUST reject any other string scalar or any other type with a descriptive compilation error. **GP-02**: Implementations MUST support the `min-integrity` field on `GitHubToolConfig` and validate its value as one of the enum strings `"none"`, `"unapproved"`, `"approved"`, or `"merged"`. Any other value MUST produce a descriptive compilation error. -**GP-03**: When `allowed-repos` is set to an array, implementations MUST validate that each element is a non-empty string matching one of the allowed pattern formats: exact (`owner/repo`), owner-wildcard (`owner/*`), or prefix-wildcard (`owner/prefix*`). Uppercase letters and wildcards in non-terminal positions MUST be rejected. +**GP-03**: When `allowed-repos` is set to an array, implementations MUST validate that each element is either (a) the exact expression string `"${{ github.repository }}"` (accepted as a dynamic self-repo reference) or (b) a non-empty string matching one of the allowed pattern formats: exact (`owner/repo`), owner-wildcard (`owner/*`), or prefix-wildcard (`owner/prefix*`). Uppercase letters and wildcards in non-terminal positions MUST be rejected. **GP-04**: Implementations MUST NOT permit an empty array as the value of `allowed-repos`. An empty allowlist MUST produce a compilation error indicating that an empty array is invalid. @@ -575,3 +575,41 @@ The deprecated `repos` field (YAML key: `repos`) is handled alongside `allowed-r **Migration command**: `gh aw fix` applies a codemod that replaces `repos:` with `allowed-repos:` in workflow frontmatter. The codemod is idempotent and safe to run multiple times. **Removal tracking**: The `repos` alias is tracked for removal. When it is removed, update `pkg/workflow/tools_types.go` (delete the `Repos` field), `pkg/workflow/mcp_github_config.go` (remove the fallback lookup), and `pkg/workflow/tools_validation_github.go` (adjust any `repos`-specific validation paths). Update doc-comments in `pkg/workflow/tools_types.go` to reference this spec version after the removal. + +--- + +## Sync Follow-ups + +This section lists the files that **MUST** be reviewed and updated whenever a normative section of this specification changes. Reviewers **SHALL** confirm each target is consistent with the updated spec before merging. + +### After Adding or Changing Normative Requirements (§Conformance) + +When requirements GP-01–GP-11 (or any later additions) change, update the following: + +1. **`pkg/workflow/schemas/mcp-gateway-config.schema.json`** AND **`docs/public/schemas/mcp-gateway-config.schema.json`** — These are the source and published copies of the gateway config schema (JSON Schema draft-07, using `definitions`). The `allowed-repos` and `min-integrity` fields are frontmatter keys that compile to the `guard-policies` object inside the gateway config; they are not top-level properties of `stdioServerConfig` or `httpServerConfig`. Verify that the `guard-policies` definition and its `allowed-repos`/`min-integrity` sub-fields in both copies reflect the updated GP-01 and GP-02 constraints (enum values, types, `required` constraints). Keep both copies in sync. + +2. **`pkg/workflow/tools_validation_github.go`** — Update `validateGitHubGuardPolicy()`, `validateReposScope()`, and `validateRepoPattern()` to enforce the revised constraints. Any new rejection rule in GP-01–GP-11 **MUST** have a corresponding validation call and error message in this file. + +3. **`pkg/workflow/mcp_github_config.go`** — Update `deriveSafeOutputsGuardPolicyFromGitHub()` to match any changes to GP-05 or GP-08 derivation rules. + +4. **`pkg/workflow/tools_types.go`** — Update `GitHubToolConfig`, `GitHubReposScope`, and `GitHubIntegrityLevel` type definitions and struct tags when field names, types, or constraints change. + +### After Changing the Safe-Outputs Derivation Rules (§5) + +When the derivation mapping in §5 changes (e.g., new pattern transformation rules): + +1. **`pkg/workflow/schemas/mcp-gateway-config.schema.json`** AND **`docs/public/schemas/mcp-gateway-config.schema.json`** — Ensure the `write-sink` accept-list field structure in both copies of the gateway config schema matches the new derivation output. + +2. **`pkg/workflow/mcp_github_config.go`** — Update `deriveSafeOutputsGuardPolicyFromGitHub()` and `normalizeGitHubRepositoryInReposScope()`. + +3. **`pkg/workflow/safeoutputs_guard_policy_test.go`** — Add or update test cases in `TestDeriveSafeOutputsGuardPolicyFromGitHub` to cover the new transformation rules. + +### After Changing Extension-Point Semantics (§6) + +When the extensibility model for future MCP servers (Jira, WorkIQ) changes: + +1. **`pkg/workflow/tools_types.go`** — Update `MCPServerConfig.GuardPolicies` and any server-specific policy types. + +2. **`pkg/workflow/schemas/mcp-gateway-config.schema.json`** AND **`docs/public/schemas/mcp-gateway-config.schema.json`** — Add server-specific guard-policy schema objects as new `definitions` entries (the schema is JSON Schema draft-07 and uses `definitions`, not `$defs`) and reference them from the relevant server config schemas. Update both copies. + +3. Document the new policy type in this specification under a new `### Entity:` subsection in [§Entities](#entities). diff --git a/specs/github-mcp-access-control-compliance/README.md b/specs/github-mcp-access-control-compliance/README.md index c5b7651b5d0..f1fbe4e2ba7 100644 --- a/specs/github-mcp-access-control-compliance/README.md +++ b/specs/github-mcp-access-control-compliance/README.md @@ -62,6 +62,7 @@ The denial code is selected by the first failing guard in the evaluation order a | `INV2_ErrorCode` | `TestFormal_ErrorCodeFirstFailingGuard` | Deny error code matches first failing guard; table covers each guard as first failure | | `SAFETY_BlockedUserAlwaysDenied` | `TestFormal_BlockedUserSafetyProperty` | Safety: blocked user always produces `-32005` when all earlier guards pass | | `SAFETY_NoSpuriousAllow` | `TestFormal_NoSpuriousAllowInvariant` | Safety: no allow decision when any guard fails | +| `P5_NotBlocked + P6_IntegrityMet` (combined) | `TestFormal_FixtureRunner` | P5 fires before P6 in evaluation order; blocked user denied with -32005 even when P6 would also fail; scenarios executed dynamically by the fixture runner | ## Fixture Files @@ -76,6 +77,7 @@ The denial code is selected by the first failing guard in the evaluation order a | `private-repo-block.yaml` | `private-repos: false` blocks access to private repository | T-GH-024, T-GH-025 | | `integrity-level-block.yaml` | `min-integrity: approved` blocks content below the threshold | T-GH-051, T-GH-052 | | `combined-filter-allow.yaml` | All access-control conditions must be jointly satisfied | T-GH-081, T-GH-082, T-GH-083 | +| `combined-blocked-integrity.yaml` | `blocked-users` (P5) and `min-integrity` (P6) evaluated simultaneously; P5 fires first when both fail | T-GH-091, T-GH-092, T-GH-093 | ## Fixture Schema diff --git a/specs/github-mcp-access-control-compliance/combined-blocked-integrity.yaml b/specs/github-mcp-access-control-compliance/combined-blocked-integrity.yaml new file mode 100644 index 00000000000..11a577ffb49 --- /dev/null +++ b/specs/github-mcp-access-control-compliance/combined-blocked-integrity.yaml @@ -0,0 +1,125 @@ +# Combined Blocked + Integrity Evaluation — Compliance Fixture +# Tests: T-GH-091, T-GH-092, T-GH-093 +# Spec: §8.5 Combined Evaluation Order, §8.2 Blocked-User Enforcement, §8.4 Minimum Integrity Enforcement + +fixture_id: "combined-blocked-integrity" +description: > + When both `blocked-users` and `min-integrity` are configured, the access decision must + be the conjunction of P5_NotBlocked AND P6_IntegrityMet (T-GH-091). A request from a + non-blocked user whose content is above the integrity threshold MUST be allowed (T-GH-092). + A request from a blocked user whose content also fails the integrity threshold MUST be + denied with the error code corresponding to the first failing predicate in evaluation + order — P5_NotBlocked (-32005) fires before P6_IntegrityMet (-32006) (T-GH-093). + +spec_refs: + - "§8.5 — combined evaluation order: P5_NotBlocked (blocked-user check) evaluated before P6_IntegrityMet (threshold check)" + - "§8.2 — blocked-users MUST deny access unconditionally for listed actors" + - "§8.4 — min-integrity allows content at or above the configured threshold" + - "§11.4 — T-GH-091, T-GH-092, T-GH-093: combined P5+P6 evaluation" + +scenarios: + # --- Scenario A: non-blocked user, integrity meets threshold → allowed --- + - scenario_id: "combined-blocked-integrity-A" + description: > + Non-blocked user with content integrity meeting the threshold is allowed. + Both P5_NotBlocked and P6_IntegrityMet pass. + input: + tool_config: + repos: + - "github/gh-aw" + min-integrity: "approved" + blocked-users: + - "bad-actor" + request: + repository: "github/gh-aw" + user_login: "good-contributor" + content_integrity: "approved" + expected: + decision: allow + error_code: null + reason: "" + + # --- Scenario B: non-blocked user, content exceeds threshold → allowed --- + - scenario_id: "combined-blocked-integrity-B" + description: > + Non-blocked user with content integrity exceeding the threshold is allowed. + P5_NotBlocked passes (user not blocked); P6_IntegrityMet passes (merged > approved). + input: + tool_config: + repos: + - "github/gh-aw" + min-integrity: "approved" + blocked-users: + - "bad-actor" + request: + repository: "github/gh-aw" + user_login: "good-contributor" + content_integrity: "merged" + expected: + decision: allow + error_code: null + reason: "" + + # --- Scenario C: blocked user, integrity below threshold → denied with P5 code --- + - scenario_id: "combined-blocked-integrity-C" + description: > + Blocked user whose content also fails the integrity threshold is denied. + P5_NotBlocked fires first (evaluation order §8.5), so error code is -32005 even + though P6_IntegrityMet would also have failed. + input: + tool_config: + repos: + - "github/gh-aw" + min-integrity: "approved" + blocked-users: + - "bad-actor" + request: + repository: "github/gh-aw" + user_login: "bad-actor" + content_integrity: "none" + expected: + decision: deny + error_code: -32005 + reason: "user is blocked" + + # --- Scenario D: blocked user, integrity meets threshold → denied with P5 code --- + - scenario_id: "combined-blocked-integrity-D" + description: > + Blocked user whose content integrity meets the threshold is still denied. + P5_NotBlocked blocks unconditionally regardless of integrity level. + input: + tool_config: + repos: + - "github/gh-aw" + min-integrity: "approved" + blocked-users: + - "bad-actor" + request: + repository: "github/gh-aw" + user_login: "bad-actor" + content_integrity: "merged" + expected: + decision: deny + error_code: -32005 + reason: "user is blocked" + + # --- Scenario E: non-blocked user, integrity below threshold → denied with P6 code --- + - scenario_id: "combined-blocked-integrity-E" + description: > + Non-blocked user whose content integrity is below the configured threshold is denied. + P5_NotBlocked passes; P6_IntegrityMet fails → error code -32006. + input: + tool_config: + repos: + - "github/gh-aw" + min-integrity: "approved" + blocked-users: + - "bad-actor" + request: + repository: "github/gh-aw" + user_login: "good-contributor" + content_integrity: "unapproved" + expected: + decision: deny + error_code: -32006 + reason: "content integrity below minimum required level" diff --git a/specs/security-architecture-spec-validation.md b/specs/security-architecture-spec-validation.md index c3197fef45d..c2a96fa684e 100644 --- a/specs/security-architecture-spec-validation.md +++ b/specs/security-architecture-spec-validation.md @@ -262,6 +262,22 @@ The July 2026 maintenance pass rechecked the documentation-only clarifications r --- +### 4b. PM-11 Formal Test Coverage Audit (2026-07-11) + +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. + +**Finding**: PM-11 is **NOT** directly covered by a formal test in `security_architecture_sg_formal_test.go`. + +- `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. + +--- + ### 5. Threat Detection Layer (Section 9 - TD-01, TD-04) **Specification Claim**: diff --git a/specs/security-architecture-spec.md b/specs/security-architecture-spec.md index aacb4e318f3..690ef4ccb5a 100644 --- a/specs/security-architecture-spec.md +++ b/specs/security-architecture-spec.md @@ -1768,6 +1768,42 @@ Use this checklist to verify that a compiled `.lock.yml` workflow file meets all --- +#### G.10 Formal Test Coverage Audit + +The following table maps each Appendix G checklist category against the formal tests in +`pkg/workflow/security_architecture_sg_formal_test.go`. Each cell may list zero, one, or +more test function names. Cells marked **covered** have at least one dedicated test; +cells marked **gap** lack direct formal test coverage; cells marked **partial** have +related coverage that does not fully satisfy the checklist item. + +| Checklist Category | Formal Test(s) | Coverage | +|---|---|---| +| **G.1** Action pinning | _(none)_ | **gap** — no formal test verifies that compiled `uses:` lines are SHA-pinned; relies on actionlint/poutine in CI | +| **G.2** Permission separation — agent job | `TestFormalSG02_AgentJobHasNoWritePermissions`, `TestFormalSG04_LeastPrivilegeBasePermissions` | covered | +| **G.2** Permission separation — safe_outputs job | `TestFormalStaged_HandlerRequiresNoWritePerms` | **partial** — verifies that a globally staged handler does not accumulate write grants via `ComputePermissionsForSafeOutputs`, but does not inspect the compiled `safe_outputs` job itself or verify that its `permissions:` block is limited to the configured operations | +| **G.3** Fork protection | `TestFormalBasicConformance_AllFourControls` | **partial** — verifies `safe_outputs` job presence, permission management, and compilation-time checks; does not inspect the activation job's `if:` expression or assert a repository-ID fork guard | +| **G.4** Input sanitization | `TestFormalSG01_InputSanitizationInvariant` | **partial** — verifies that a generic `run:` step with a `${{ }}` expression is rewritten and an `env:` block is injected; does not assert either required activation step or that compiled prompts consume `steps.sanitized.outputs.text` | +| **G.5** Threat detection | `TestFormalSG06_ThreatDetectionAuditArtifact`, `TestFormalThreatDetection_EnabledByDefault`, `TestFormalThreatDetection_ExplicitDisable` | **partial** — detection-job presence and configuration defaults are covered; `permissions: {}` on the detection job and the `needs.detection.outputs.success` gate on `safe_outputs` are not directly asserted | +| **G.6** RBAC — job topology (PM-10b) | `TestFormalJobTopology_PipelineOrderEnforced` (verifies `pre_activation → activation` needs dependency) | covered | +| **G.6** RBAC — membership check step (PM-11) | _(none)_ | **gap** — `TestFormalJobTopology_PipelineOrderEnforced` verifies job ordering but does NOT assert that the `pre_activation` job contains a `check_membership.cjs` step; no dedicated PM-11 formal test exists in this file | +| **G.7** AWF sandbox | `TestFormalSG05_SandboxIsolationPresence` | **partial** — verifies `isSandboxEnabled()` return value for configuration combinations (AWF type, disabled flag, firewall); does not compile a workflow or assert `install_awf_binary.sh` step presence or execution inside the AWF container | +| **G.8** Concurrency control | _(none)_ | **gap** — no formal test verifies `concurrency.group` is set or that `cancel-in-progress` matches workflow type | +| **G.9** Runtime validation — timestamp check | _(none)_ | **gap** — `TestFormalSG07_FailSecureOnSecurityError` tests write-permission rejection at compile time and is unrelated to the G.9 checklist items; neither the activation timestamp step nor conclusion-job behavior is asserted by any test in this file | + +**Summary of coverage gaps (as of 2026-07-11)**: + +- **G.1 Action pinning**: Formal test gap. CI tooling (actionlint, poutine) provides runtime coverage; consider adding a dedicated test that parses compiled lock file `uses:` patterns. +- **G.2 Permission separation — safe_outputs job**: Partial coverage. `TestFormalStaged_HandlerRequiresNoWritePerms` does not inspect the compiled `safe_outputs` job's own `permissions:` block. A dedicated test compiling a safe-outputs workflow and asserting its permission scope would close this gap. +- **G.3 Fork protection**: Partial coverage. `TestFormalBasicConformance_AllFourControls` does not inspect the activation job's `if:` expression or verify a repository-ID fork guard. A test asserting the exact `if:` condition on the compiled activation job is needed. +- **G.4 Input sanitization**: Partial coverage. `TestFormalSG01_InputSanitizationInvariant` covers expression rewriting but not the full three-item checklist (activation step presence; `steps.sanitized.outputs.text` consumption in compiled prompts). +- **G.5 Threat detection**: Partial coverage. Detection-job presence and defaults are covered. Tests asserting `permissions: {}` on the detection job and the `needs.detection.outputs.success` gate on `safe_outputs` are missing. +- **G.6 RBAC (PM-11) membership check step**: Formal test gap. `pre_activation` job topology is verified, but the presence of `check_membership.cjs` as a step inside that job is not asserted by any test in this file. A dedicated `TestFormalPM11_PreActivationContainsMembershipStep` test should be added. See: specs/security-architecture-spec-validation.md §4b (PM-11 note). +- **G.7 AWF sandbox**: Partial coverage. Configuration-logic checks are covered; lock-file-level assertions (`install_awf_binary.sh`, AWF container execution) are missing. +- **G.8 Concurrency control**: Formal test gap. Concurrency group format is validated only by manual review. Consider a test that compiles a PR-trigger workflow and asserts `concurrency.group` contains the PR number expression. +- **G.9 Runtime validation — timestamp check**: Formal test gap. No formal test verifies the activation timestamp step or conclusion-job behavior. + +--- + ### Appendix H: Security Best Practices #### BP-01: Always Use Sanitized Context