Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions scratchpad/github-mcp-access-control-specification.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down Expand Up @@ -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.

Expand Down Expand Up @@ -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
Expand Down
42 changes: 40 additions & 2 deletions scratchpad/guard-policies-specification.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -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).
2 changes: 2 additions & 0 deletions specs/github-mcp-access-control-compliance/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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

Expand Down
Original file line number Diff line number Diff line change
@@ -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"
Comment thread
pelikhan marked this conversation as resolved.

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"
16 changes: 16 additions & 0 deletions specs/security-architecture-spec-validation.md
Original file line number Diff line number Diff line change
Expand Up @@ -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**:
Expand Down
Loading
Loading