diff --git a/scratchpad/github-mcp-access-control-specification.md b/scratchpad/github-mcp-access-control-specification.md index 24425a65cff..d4f5079065b 100644 --- a/scratchpad/github-mcp-access-control-specification.md +++ b/scratchpad/github-mcp-access-control-specification.md @@ -1597,19 +1597,19 @@ ELSE: ### 8.5 Combined Evaluation Order -The complete integrity evaluation MUST occur in this order: +The complete integrity evaluation MUST occur in this order. Each step is labeled with the formal guard-predicate name used in §11 Compliance Testing and `pkg/workflow/github_mcp_access_control_formal_test.go` (`formalEvaluateAccess`) so that the two are unambiguously the same evaluation sequence: ```text -1. Author Check (blocked-users) - → If author is blocked: DENY immediately -2. Label Promotion (approval-labels) +1. Author Check — predicate P5_NotBlocked (blocked-users) + → If author is blocked: DENY immediately (error code -32005) +2. Label Promotion — integrity computation input to P6 (approval-labels), not a standalone gating predicate → Promote effective_integrity if a matching label is present -3. Threshold Check (min-integrity) - → If effective_integrity < min-integrity: DENY +3. Threshold Check — predicate P6_IntegrityMet (min-integrity) + → If effective_integrity < min-integrity: DENY (error code -32006) 4. All integrity checks passed: ALLOW ``` -This order ensures that blocked users can never be promoted by labels, and that label promotion is always considered before the threshold check. +This order ensures that blocked users can never be promoted by labels, and that label promotion is always considered before the threshold check. Concretely: P5_NotBlocked (step 1) is evaluated and MUST fire before P6_IntegrityMet (step 3); label promotion (step 2) only mutates the `effective_integrity` value consumed by P6_IntegrityMet and never overrides a P5_NotBlocked denial. Note that P5_NotBlocked and P6_IntegrityMet are themselves evaluated after the repository/role/visibility predicates P1–P4 in §4.5.3's guard evaluation order; §8.5 covers only the internal ordering of the two integrity-related predicates. --- @@ -2680,6 +2680,53 @@ GitHub API rate limits apply to: - Monitor rate limit consumption - Implement exponential backoff for rate limit errors +**Example: Exponential backoff for rate-limited permission/visibility queries** + +Consistent with the fail-closed requirement in §9.4 (Security Considerations), a `403`/`429` response during backoff MUST still deny the current access-control decision rather than substituting a stale cached "allow" result while a retry is pending. + +```text +function queryWithBackoff(request): + maxAttempts = 5 + baseDelayMs = 500 # initial delay + maxDelayMs = 30000 # cap to avoid unbounded waits + + for attempt in 1..maxAttempts: + response = performGitHubAPIRequest(request) + + # 429 is always a primary rate limit. A 403 with a rate-limit-related + # body/header (e.g. "secondary rate limit" or a "Retry-After" header) + # is GitHub's secondary rate limit and is retried the same way; a 403 + # WITHOUT rate-limit signals is a permission/authorization denial and + # MUST NOT be retried — return it immediately as a non-retryable error. + isRateLimited = response.status == 429 or + (response.status == 403 and isRateLimitSignal(response)) + + if isRateLimited: + if attempt == maxAttempts: + # Fail closed: no more retries — deny for this decision (§9.4) + return DENY_RATE_LIMITED + + retryAfter = response.headers["Retry-After"] # seconds, if present + if retryAfter is present: + delayMs = retryAfter * 1000 + else: + # Exponential backoff with jitter: base * 2^(attempt-1), capped + delayMs = min(baseDelayMs * (2 ** (attempt - 1)), maxDelayMs) + delayMs = delayMs + randomJitterMs(0, delayMs * 0.1) + + sleep(delayMs) + continue + + return response # success, or a non-rate-limit error (e.g. plain 403); caller handles normally + # Loop only reaches maxAttempts via the rate-limited branch above, which + # always returns DENY_RATE_LIMITED on the final attempt, so this point is unreachable. +``` + +**Notes**: +- Respect the `Retry-After` header when GitHub provides one; fall back to exponential backoff (base 500ms, cap 30s) with jitter otherwise. +- Treat `403` as retryable only when it carries GitHub's secondary-rate-limit signal (`Retry-After` header or a rate-limit message in the response body); an ordinary `403` permission denial MUST be returned immediately, not retried. +- Exhausting `maxAttempts` MUST result in a fail-closed denial for the current request, not an implicit allow. + #### C.5 Configuration Validation Timing **Compilation-Time Validation**: Catches most configuration errors before runtime diff --git a/specs/safe-output-outcome-evaluation.md b/specs/safe-output-outcome-evaluation.md index 3a9ba9c45e6..8a786a9970c 100644 --- a/specs/safe-output-outcome-evaluation.md +++ b/specs/safe-output-outcome-evaluation.md @@ -815,6 +815,45 @@ The table below specifies one conformance test row per safe-output type. Each ro | §29 | `missing_tool` | `pkg/cli/outcome_eval_test.go` | covered | | §30 | `replace_label` | `pkg/cli/outcome_eval_update_test.go`, `pkg/workflow/replace_label_formal_test.go` | covered | +### Structure: Safe-Output Section-to-Implementation Mapping + +Each numbered safe-output-type section above corresponds to a dedicated configuration/compilation implementation file under `pkg/workflow/`. This mapping is distinct from the Compliance test mapping above; it maps specification sections to the Go source that defines and compiles the output type (shared cross-cutting logic such as `safe_output_handlers.go` and `compiler_safe_outputs_job.go` is omitted for brevity since it applies to all types). + +| Section | Output type | Implementation file(s) | +|---|---|---| +| §1 | `create_pull_request` | `pkg/workflow/create_pull_request.go` | +| §2 | `create_issue` | `pkg/workflow/create_issue.go` | +| §3 | `add_comment` | `pkg/workflow/add_comment.go` | +| §4 | `add_labels` | `pkg/workflow/add_labels.go` | +| §5 | `add_reviewer` | `pkg/workflow/add_reviewer.go` | +| §6 | `update_issue` | `pkg/workflow/update_issue.go` | +| §7 | `update_pull_request` | `pkg/workflow/update_pull_request.go` | +| §8 | `close_issue` | `pkg/workflow/close_entity_helpers.go` | +| §9 | `close_pull_request` | `pkg/workflow/close_entity_helpers.go` | +| §10 | `close_discussion` | `pkg/workflow/close_entity_helpers.go` | +| §11 | `create_discussion` | `pkg/workflow/create_discussion.go` | +| §12 | `update_discussion` | `pkg/workflow/update_discussion.go` | +| §13 | `create_pull_request_review_comment` | `pkg/workflow/create_pr_review_comment.go` | +| §14 | `submit_pull_request_review` | `pkg/workflow/submit_pr_review.go` | +| §15 | `reply_to_pull_request_review_comment` | `pkg/workflow/reply_to_pr_review_comment.go` | +| §16 | `resolve_pull_request_review_thread` | `pkg/workflow/resolve_pr_review_thread.go` | +| §17 | `push_to_pull_request_branch` | `pkg/workflow/push_to_pull_request_branch.go`, `pkg/workflow/push_to_pull_request_branch_validation.go` | +| §18 | `mark_pull_request_as_ready_for_review` | `pkg/workflow/mark_pull_request_as_ready_for_review.go` | +| §19 | `assign_to_agent` | `pkg/workflow/assign_to_agent.go` | +| §20 | `dispatch_workflow` | `pkg/workflow/dispatch_workflow.go`, `pkg/workflow/dispatch_workflow_validation.go`, `pkg/workflow/dispatch_workflow_file_resolver.go` | +| §21 | `autofix_code_scanning_alert` | `pkg/workflow/autofix_code_scanning_alert.go` | +| §22 | `create_code_scanning_alert` | `pkg/workflow/create_code_scanning_alert.go` | +| §23 | `link_sub_issue` | `pkg/workflow/link_sub_issue.go` | +| §24 | `hide_comment` | `pkg/workflow/hide_comment.go` | +| §25 | `assign_milestone` | `pkg/workflow/assign_milestone.go` | +| §26 | `update_project` | `pkg/workflow/update_project.go` | +| §27 | `update_release` | `pkg/workflow/update_release.go` | +| §28 | `noop` | `pkg/workflow/noop.go` | +| §29 | `missing_tool` | `pkg/workflow/missing_issue_reporting.go` | +| §30 | `replace_label` | `pkg/workflow/replace_label.go` | + +Sync procedure: when a safe-output type's implementation file is renamed, split, or removed, update the corresponding row in this table in the same change that moves the code. + ### OTel Backend Unavailability When the OTLP exporter is unavailable (e.g., endpoint unreachable, network timeout, authentication failure) during outcome evaluation, the following safeguards **MUST** apply: diff --git a/specs/security-architecture-spec-summary.md b/specs/security-architecture-spec-summary.md index 7e7c6137960..f9e631e1233 100644 --- a/specs/security-architecture-spec-summary.md +++ b/specs/security-architecture-spec-summary.md @@ -264,6 +264,17 @@ Step-by-step checklist for verifying that a compiled `.lock.yml` file meets all ### Appendix H: Security Best Practices Six key best practices with "Don't" and "Do" examples. +**Safeguards — compile-time vs. runtime enforcement**: + +| Practice | Enforcement | Mechanism | +|---|---|---| +| BP-01 Sanitized context | Compile-time | Compiler rewrites `${{ github.event.* }}` expressions in `prompt:` to `${{ steps.sanitized.outputs.text }}` during compilation; unsanitized expressions never reach the generated `.lock.yml` | +| BP-02 Strict mode for production | Compile-time | `strict: true` causes the compiler to reject workflows with write permissions on the `agent` job or missing `safe-outputs:`; violations fail `gh aw compile` | +| BP-03 Specific domain allowlists | Runtime | The AWF network proxy/firewall enforces the configured `network.allowed` domain list against outbound requests during workflow execution; a wildcard (`"*"`) is accepted at compile time but only its effect is observed at runtime | +| BP-04 Pin actions to SHAs | Compile-time (advisory) / CI-time (enforced) | The compiler itself does not reject unpinned `uses:` references; SHA-pinning is enforced by CI tooling (`actionlint`, `poutine`, `zizmor`) run against compiled `.lock.yml` files, not by the compiler at `gh aw compile` time (see Appendix G.1 coverage gap) | +| BP-05 Enable threat detection | Compile-time (job generation) / Runtime (detection execution) | The compiler generates a `detection` job when `threat-detection.enabled` is not explicitly `false`; the actual AI-based/TruffleHog scan and the `needs.detection.outputs.success` gate on `safe_outputs` are evaluated at runtime | +| BP-06 Role-based access control | Runtime | `roles:` configures the `GH_AW_REQUIRED_ROLES` environment variable consumed by the `pre_activation` job's `check_membership.cjs` step, which queries GitHub's API for the triggering actor's role at workflow run time; the compiler does not verify roles ahead of time | + ## Target Audience - **Security Engineers**: Audit and verify security controls diff --git a/specs/security-architecture-spec-validation.md b/specs/security-architecture-spec-validation.md index 2dba6c457cf..0fac54db49b 100644 --- a/specs/security-architecture-spec-validation.md +++ b/specs/security-architecture-spec-validation.md @@ -580,7 +580,7 @@ concurrency: **Clarification**: `pre_activation` handles role-based access control before activation. This is an implementation detail that doesn't contradict the specification - it's an additional security layer. -**Recommendation**: Consider adding a note about role validation occurring in a separate pre-activation step. +**Status**: ✅ **CLOSED** — Appendix D of `specs/security-architecture-spec.md` (Example 5) now shows `pre_activation` as a separate job gating `activation` on role membership. ### 2. Detection Job Naming @@ -588,7 +588,7 @@ concurrency: **Clarification**: The `detection` job is the runtime manifestation of the threat detection layer described in Section 9. -**Recommendation**: Add example job structure showing `detection` as a separate job in Appendix D. +**Status**: ✅ **CLOSED** — Appendix D of `specs/security-architecture-spec.md` (Example 5) now shows `detection` as a separate job between `agent` and `safe_outputs`. ### 3. Conclusion Job @@ -596,7 +596,7 @@ concurrency: **Clarification**: The `conclusion` job is an implementation detail for workflow cleanup and summary generation. -**Recommendation**: Consider adding a note about optional cleanup/reporting jobs. +**Status**: ✅ **CLOSED** — Appendix D of `specs/security-architecture-spec.md` (Example 5) now shows `conclusion` as the terminal `always()` job for cleanup/status reporting. --- @@ -613,6 +613,14 @@ A conforming maintainer MUST re-run this validation when any of the following oc For each re-validation pass, reviewers MUST rerun the Detailed Validation procedure above, refresh the evidence-location table, update the Minor Discrepancies section, and revise the validation grade if any claim is no longer fully verified. +### Trigger #3 Decision Log + +**2026-08-13 — `repos` vs `allowed-repos` field-naming divergence (deferred, no re-validation needed)** + +`scratchpad/github-mcp-access-control-specification.md`'s Divergence Audit (§Sync Notes) flagged that §4.4.1 historically used `repos` as the field name while §4.1 and the guard-policies spec used `allowed-repos`. This was investigated as a potential trigger-#3 event (companion guard-policy spec revision). + +**Decision: Defer — no re-validation of this report required.** The companion spec's own Divergence Audit already marked the item **Resolved**: `allowed-repos` is the canonical frontmatter key, `repos` is a documented deprecated alias for backward compatibility (`pkg/workflow/mcp_github_config.go` lines ~322-335), and `pkg/workflow/tools_validation_github.go` validates both spellings identically via the shared `AllowedRepos` field. This is a terminology/alias clarification, not a change to guard-policy *behavior*, job structure, or security guarantees. It does not affect the §3.2 Security Guarantees or §9 Threat Detection claims in `specs/security-architecture-spec.md`, so the Specification Accuracy Summary table below requires no update. `go test ./pkg/workflow/ -run TestValidateGitHubGuardPolicy` already exercises both the `repos` alias and `allowed-repos` field name (see `pkg/workflow/tools_validation_test.go`) and passes. + ### Failure Escalation When a re-validation pass identifies a specification claim that is no longer verifiable against the implementation: diff --git a/specs/security-architecture-spec.md b/specs/security-architecture-spec.md index b6523368aac..793037b8f0e 100644 --- a/specs/security-architecture-spec.md +++ b/specs/security-architecture-spec.md @@ -1584,6 +1584,68 @@ safe-outputs: **Behavior**: All workflow runs complete in order, preventing incomplete operations. +#### Example 5: Pre-Activation, Detection, and Conclusion Job Structure + +The following excerpt (abridged from a compiled `.lock.yml`) shows the three implementation-detail jobs referenced in the Minor Discrepancies section of `specs/security-architecture-spec-validation.md`: `pre_activation` (role-based access control ahead of `activation`), `detection` (runtime manifestation of the Threat Detection Layer, Section 9), and `conclusion` (cleanup/summary reporting job that always runs). + +```yaml +jobs: + pre_activation: + if: > + (github.event_name != 'pull_request' || github.event.pull_request.head.repo.id == github.repository_id) + runs-on: ubuntu-slim + permissions: + contents: read + outputs: + activated: ${{ steps.check_membership.outputs.is_team_member == 'true' }} + steps: + - name: Check team membership for workflow + id: check_membership + uses: actions/github-script@ # vX.Y.Z + env: + GH_AW_REQUIRED_ROLES: "admin,maintainer,write" + + activation: + needs: [pre_activation] + # ... timestamp/lockdown validation (Section 11.1) ... + + agent: + needs: [activation] + permissions: + contents: read + # ... agentic execution (read-only) ... + + detection: + needs: [activation, agent] + if: always() && needs.agent.result != 'skipped' + runs-on: ubuntu-latest + permissions: + contents: read + outputs: + detection_conclusion: ${{ steps.detection_conclusion.outputs.conclusion }} + detection_success: ${{ steps.detection_conclusion.outputs.success }} + # ... AI threat-detection scan of agent output (Section 9.1) ... + + safe_outputs: + needs: [activation, agent, detection] + if: (!cancelled()) && needs.agent.result != 'skipped' && needs.detection.result == 'success' + permissions: + issues: write + pull-requests: write + # ... write operations gated on successful detection ... + + conclusion: + needs: [activation, agent, detection, safe_outputs] + if: always() && (needs.agent.result != 'skipped' || needs.activation.outputs.lockdown_check_failed == 'true') + permissions: + actions: read + issues: write + pull-requests: write + # ... run summary, cleanup, and failure reporting ... +``` + +**Behavior**: `pre_activation` gates `activation` on role membership; `detection` runs after `agent` and gates `safe_outputs` on a successful threat-detection conclusion; `conclusion` always runs last (subject to `always()`) to report status regardless of upstream success or failure. + ### Appendix E: Concurrency Control Examples #### Example 1: Pull Request Workflow with Cancellation