diff --git a/README.md b/README.md index a2e5bbe..44db228 100644 --- a/README.md +++ b/README.md @@ -51,7 +51,7 @@ Set `SLACK_WEBHOOK_URL` and Shadow posts a one-line summary to a Slack channel o ### What Shadow does NOT do - **Auto-close PRs.** Shadow only comments and labels. You decide what merges. -- **Auto-merge or auto-approve.** Same. +- **Auto-merge.** Shadow never merges; you decide what merges. (Auto-*approve* is available as an opt-in, off by default — see [Auto-approval](#auto-approval).) - **Cross-repo refactors.** Shadow scopes to one repo at a time. - **Check out PR head code.** The workflow uses `pull_request_target` and reads only the base-branch checkout, defending against the [pwn-request attack](https://securitylab.github.com/research/github-actions-preventing-pwn-requests/) where untrusted PR code runs in privileged context. @@ -122,15 +122,16 @@ codebase: src_dir: src/ # required — your primary source directory ``` -### 3. AWS role + guardrail — three repo secrets +### 3. AWS role + guardrail — repo secrets -[Click the Launch Stack button](#aws-setup) (one click), or [follow the manual setup](#manual-setup-alternative). The stack emits three outputs to paste into repo secrets (Settings → Secrets and variables → Actions): +[Click the Launch Stack button](#aws-setup) (one click), or [follow the manual setup](#manual-setup-alternative). Paste these stack outputs into repo secrets (Settings → Secrets and variables → Actions): | Stack output | Repo secret | Required? | |---|---|---| | `ShadowRoleArn` (`arn:aws:iam::...:role/shadow-bot-ci`) | `AWS_ROLE_ARN` | Always | | `GuardrailId` | `GUARDRAIL_ID` | Production runs (default `ProvisionGuardrail=true`) | | `GuardrailVersion` | `GUARDRAIL_VERSION` | Production runs | +| `MetricsRoleArn` | `AWS_METRICS_ROLE_ARN` | Only with [auto-approval](#auto-approval) enabled | A production run (`DRY_RUN=false`) with `GUARDRAIL_ID` unset is refused at load time — Shadow won't run without prompt-injection defense. To opt out (e.g. a custom guardrail, or a dry-run-only fork), pass `require_guardrail: 'false'` under the caller's `with:` block. See [Security model](#security-model). @@ -291,8 +292,9 @@ The button opens AWS Console with [`infrastructure/shadow-iam-stack.yaml`](infra | **ShadowWorkflowRef** | `*` for quick start, a `refs/tags/v1.x` release tag (e.g. `refs/tags/v1.8`), or a 40-char SHA to pin trust to one audited revision | | **BedrockRegion** | Where Bedrock will be invoked. `us-east-1` / `us-west-2` / `us-east-2` are the validated combinations; other regions work if both Opus 4.8 and Haiku 4.5 are available there ([model-region matrix](https://docs.aws.amazon.com/bedrock/latest/userguide/models-regions.html)). The region you pick here must match where you enable model access in the next step. | | **ExistingOidcProviderArn** | Leave blank if your account has no GitHub OIDC provider yet. **If your account already uses GitHub Actions OIDC, paste the existing provider ARN** (`aws iam list-open-id-connect-providers`). Leaving blank when one exists fails with `EntityAlreadyExists`. | -| **MonthlyBudgetLimit** + **BudgetEmailAddress** | Optional. `MonthlyBudgetLimit > 0` + an email enables an AWS Budget emailing at 80% / 100% of the cap (`0` skips the Budget). **Supplying `BudgetEmailAddress` also provisions two behavioral-anomaly alarms** (escalation/invocation spikes — see [Cost protection](#cost-protection)) regardless of the limit; blank skips both. Confirm the SNS subscription email or alerts won't arrive. | +| **MonthlyBudgetLimit** + **BudgetEmailAddress** | Optional. `MonthlyBudgetLimit > 0` + an email enables an AWS Budget emailing at 80% / 100% of the cap (`0` skips the Budget). **Supplying `BudgetEmailAddress` also provisions the behavioral-anomaly alarms** (see [Cost protection](#cost-protection)) regardless of the limit; blank skips them. Confirm the SNS subscription email or alerts won't arrive. | | **EscalationSpikeThreshold** / **InvocationSpikeThreshold** | Fleet-wide per-hour alarm thresholds (defaults `25` / `100`). Only used when `BudgetEmailAddress` is set. Raise them above your normal hourly volume to avoid false pages. | +| **ApprovalSpikeThreshold** / **HighRiskApprovalThreshold** | Auto-approval alarm thresholds (defaults `5` / `0`). Only relevant with auto-approval enabled (see [Auto-approval](#auto-approval)). `HighRiskApprovalThreshold: 0` pages on the first auto-approval of a first-time contributor or large diff; raise it if you get many such PRs. | | **ProvisionGuardrail** | Default `true`. Provisions a Bedrock Guardrail with prompt-attack defense + PII blocks (see [Security model](#security-model)). Set to `false` only if you maintain a custom guardrail and want to point Shadow at it via the `GUARDRAIL_ID`/`GUARDRAIL_VERSION` secrets. | The stack creates the OIDC provider (if needed), an IAM role with the canonical `job_workflow_ref`-pinned trust policy, a Bedrock-invoke permission scoped to Anthropic models only, AND (by default) a Bedrock Guardrail with prompt-attack + PII filters. After deploy, copy these outputs into repo secrets: @@ -372,6 +374,16 @@ If you prefer not to run CloudFormation: --- +## Auto-approval + +Off by default. Shadow is comment-only until a maintainer sets the repo/org variable `SHADOW_AUTO_APPROVE=true`, because a bot approval can satisfy branch protection's required review — so enabling it means a clean PR can be approved without a human approval on the path. Enable it only where that trade-off is acceptable. + +To enable: copy [`examples/auto-approve.yml`](examples/auto-approve.yml) to `.github/workflows/`, set the CI workflow name in the two marked places, and set `SHADOW_AUTO_APPROVE=true`. It approves a PR only when Shadow's review is clean (``), CI passed for that exact SHA, and the PR touches no guarded path (`.github/`, `.shadow.yml`). It never merges. + +The workflow also publishes `ApprovalGranted` / `HighRiskApproval` metrics so the approval-spike and high-risk-approval alarms fire. This runs in a separate OIDC-only job that assumes a dedicated `PutMetricData`-only role (the stack's `MetricsRoleArn` output → set as the `AWS_METRICS_ROLE_ARN` secret) — so no job holds both GitHub-write and AWS, and the approval workflow never gets the engine's Bedrock/Secrets/S3 role. High-risk = first-time contributor or large diff (>500 lines or >20 files). The emit job's `aws-region` must match your `BedrockRegion`, or those regional alarms never see the data. + +--- + ## Security model You're letting a bot read your repo and post on your behalf. Here's the trust boundary. @@ -424,7 +436,7 @@ The per-PR levers (under [What it costs](#what-it-costs)) bound a single review. - **Per-(repo, item) hourly rate limit** (`BOT_MAX_RUNS_PER_HOUR`, default `20`). Caps how many times a single PR or issue can trigger Shadow per rolling hour. Beyond the limit, the bot ESCALATES with a `:rate-limited` label instead of running the agent pipeline. Defends against an adversary closing/reopening or editing a PR title in a loop. Set to `0` to disable. **Issue/issue_comment events** require `run-name: "Shadow #${{ github.event.issue.number || ... }}"` in your caller workflow so the rate-limit gate can match prior runs (see [`examples/caller-workflow.yml`](examples/caller-workflow.yml)). - **Pre-flight diff/file caps** (`BOT_MAX_DIFF_FOR_REVIEW_CHARS`, `BOT_MAX_FILES_FOR_REVIEW`, defaults `100000` / `50`). A 50-file PR makes the Investigator read 5+ files, the Critic re-reads, the Reporter formats — costs multiply. Diff or file count above the cap → ESCALATE before any Bedrock call. Pre-flight escalation is ~$0; a runaway pipeline on a giant PR is $5+. - **AWS Budgets opt-in via CFN** (`MonthlyBudgetLimit` parameter on `shadow-iam-stack.yaml`). Set a positive USD amount + a `BudgetEmailAddress` and the stack creates an `AWS::Budgets::Budget` filtered to Amazon Bedrock spend, with email alerts at 80% and 100%. `0` skips Budget creation (default — AWS Budgets bills $0.02/budget/day, so opt-in only). Email-only today; auto-shutdown via `SHADOW_DISABLED` is a planned upgrade. -- **Behavioral-anomaly alarms via CFN** (provisioned when `BudgetEmailAddress` is set). Two CloudWatch alarms aggregate the `Shadow` namespace fleet-wide via a Metrics Insights query (`SELECT SUM(...) FROM "Shadow"`): an **escalation spike** (`EscalationSpikeThreshold`, default `25`/hr — flags prompt-injection/abuse bursts or systemic failures) and an **invocation spike** (`InvocationSpikeThreshold`, default `100`/hr — flags public-trigger floods driving Bedrock spend, faster than the monthly Budget). Both notify the `AlarmTopicArn` SNS topic (`BudgetEmailAddress` auto-subscribed — confirm the email). Detection latency is up to 1 hour; tune thresholds to your fleet volume to avoid false pages. **Deploy the stack in the same region as your `aws_region`/`BedrockRegion`** — CloudWatch alarms are regional and see only metrics emitted in their own region. The alarms depend on Shadow's metric emission, so they are blind if `SHADOW_CLOUDWATCH_DISABLED=true` (they sit green, not red — absence of data isn't a breach). These are Metrics Insights query alarms, billed per alarm plus the metrics each query scans (which grows with your repo/reason cardinality) — small but not flat; see [CloudWatch pricing](https://aws.amazon.com/cloudwatch/pricing/). +- **Behavioral-anomaly alarms via CFN** (provisioned when `BudgetEmailAddress` is set). Four CloudWatch alarms aggregate the `Shadow` namespace fleet-wide via a Metrics Insights query (`SELECT SUM(...) FROM "Shadow"`): an **escalation spike** (`EscalationSpikeThreshold`, default `25`/hr — flags prompt-injection/abuse bursts or systemic failures), an **invocation spike** (`InvocationSpikeThreshold`, default `100`/hr — flags public-trigger floods driving Bedrock spend, faster than the monthly Budget), an **approval spike** (`ApprovalSpikeThreshold`, default `5`/hr — flags a burst of auto-approvals), and a **high-risk approval** alarm (`HighRiskApprovalThreshold`, default `0` — pages on the first auto-approval of a first-time contributor or large diff). The last two only receive data where auto-approval is enabled (see [Auto-approval](#auto-approval)). All notify the `AlarmTopicArn` SNS topic (`BudgetEmailAddress` auto-subscribed — confirm the email). Detection latency is up to 1 hour; tune thresholds to your fleet volume to avoid false pages. **Deploy the stack in the same region as your `aws_region`/`BedrockRegion`** — CloudWatch alarms are regional and see only metrics emitted in their own region; the auto-approve workflow's emit job must use that same region. The alarms depend on Shadow's metric emission, so they are blind if `SHADOW_CLOUDWATCH_DISABLED=true` (they sit green, not red — absence of data isn't a breach). These are Metrics Insights query alarms, billed per alarm plus the metrics each query scans (which grows with your repo/reason cardinality) — small but not flat; see [CloudWatch pricing](https://aws.amazon.com/cloudwatch/pricing/). --- @@ -466,7 +478,7 @@ If you previously tried to set `SHADOW_DISABLED` as a Secret rather than a Varia **Implemented** (shipped, covered by tests + CI): - BYO-AWS reusable workflow with two-job security split (`analyze` / `act`) -- One-click CloudFormation Launch Stack for IAM, OIDC trust, AWS Budget, optional behavioral-anomaly CloudWatch alarms (escalation/invocation spikes), **and a default Bedrock Guardrail** with prompt-attack + PII filters (set `ProvisionGuardrail=false` to skip) +- One-click CloudFormation Launch Stack for IAM, OIDC trust, AWS Budget, optional behavioral-anomaly CloudWatch alarms (escalation, invocation, approval, and high-risk-approval spikes), **and a default Bedrock Guardrail** with prompt-attack + PII filters (set `ProvisionGuardrail=false` to skip) - `shadow doctor` preflight CLI (verifies role, Bedrock access, prompts) - Audit trail in artifact: prompt-hash provenance, security-events histogram, SHA-256 integrity stamp bound to `(repo, run_id, pr_number)` - Refutation Trail rendered into posted comments (`
` block per finding) diff --git a/examples/auto-approve.yml b/examples/auto-approve.yml new file mode 100644 index 0000000..5b88e04 --- /dev/null +++ b/examples/auto-approve.yml @@ -0,0 +1,179 @@ +# Optional: auto-approve a PR once Shadow's review is clean AND CI passed. +# OFF unless repo/org var SHADOW_AUTO_APPROVE=true (a bot approval can satisfy +# branch protection's required review, so it stays opt-in). The split into +# approve + emit jobs keeps Shadow's invariant that no one job holds both +# GitHub-write and AWS access. +# Replace "Java CI with Maven" with your CI workflow name (two places below). +name: Auto-Approve Clean PRs + +on: + workflow_run: + workflows: ["Java CI with Maven", "Shadow"] + types: [completed] + +permissions: + pull-requests: write + actions: read + +jobs: + approve: + runs-on: ubuntu-latest + if: vars.SHADOW_AUTO_APPROVE == 'true' && (github.event.workflow_run.event == 'pull_request' || github.event.workflow_run.event == 'pull_request_target') + timeout-minutes: 2 + outputs: + approved: ${{ steps.gate.outputs.approved }} + highrisk: ${{ steps.gate.outputs.highrisk }} + steps: + - name: Find PR and check both conditions + id: gate + env: + SHADOW_BOT_NAME: ${{ vars.SHADOW_BOT_NAME }} + SHADOW_BOT_LOGIN: ${{ vars.SHADOW_BOT_LOGIN }} + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + const sha = context.payload.workflow_run.head_sha; + const owner = context.repo.owner; + const repo = context.repo.repo; + + // Find the PR for this SHA + let prNumber = null; + const prs = context.payload.workflow_run.pull_requests; + if (prs && prs.length > 0) { + prNumber = prs[0].number; + } else { + const {data: searchResult} = await github.rest.pulls.list({ + owner, repo, state: 'open', sort: 'updated', direction: 'desc', per_page: 30 + }); + const match = searchResult.find(pr => pr.head.sha === sha); + if (match) { + prNumber = match.number; + } + } + + if (!prNumber) { + core.info(`No open PR found for SHA ${sha}, skipping`); + return; + } + + core.info(`Found PR #${prNumber} for SHA ${sha}`); + + // Verify the PR head SHA still matches (no new push since trigger) + const {data: pr} = await github.rest.pulls.get({ + owner, repo, pull_number: prNumber + }); + if (pr.head.sha !== sha) { + core.info(`PR head ${pr.head.sha} differs from trigger SHA ${sha} — new push arrived, skipping`); + return; + } + + // Condition 0: a PR that edits the bot's own guardrails could weaken + // the gate it is approved by, so require a human. Check both filename + // and previous_filename, else renaming a guarded file out of the way + // slips past. + const files = await github.paginate(github.rest.pulls.listFiles, { + owner, repo, pull_number: prNumber, per_page: 100 + }); + const isGuarded = p => !!p && (p.startsWith('.github/') || p === '.shadow.yml'); + const guarded = files.find(f => isGuarded(f.filename) || isGuarded(f.previous_filename)); + if (guarded) { + core.info(`PR touches guarded path ${guarded.filename} — requires human review, skipping`); + return; + } + + // Condition 1 + const {data: workflowRuns} = await github.rest.actions.listWorkflowRunsForRepo({ + owner, repo, head_sha: sha, status: 'completed' + }); + const ciRun = workflowRuns.workflow_runs.find(r => + r.name === 'Java CI with Maven' && r.conclusion === 'success' + ); + if (!ciRun) { + core.info(`CI has not passed for SHA ${sha}, skipping`); + return; + } + + // Condition 2. Paginate: reviews come oldest-first 30/page, so an + // unpaginated read would miss the latest review on a busy PR. + const reviews = await github.paginate(github.rest.pulls.listReviews, { + owner, repo, pull_number: prNumber, per_page: 100 + }); + + // Override the vars if you set a custom bot.name / bot.github_actor. + const BOT_LOGIN = process.env.SHADOW_BOT_LOGIN || 'github-actions[bot]'; + const CLEAN_MARKER = ``; + const botReviews = reviews.filter(r => r.user && r.user.login === BOT_LOGIN); + + const latestBot = botReviews + .sort((a, b) => new Date(b.submitted_at) - new Date(a.submitted_at))[0]; + + if (!latestBot || !(latestBot.body || '').includes(CLEAN_MARKER) || latestBot.commit_id !== sha) { + core.info('Bot has not posted a clean review for this SHA, skipping'); + return; + } + + // DISMISSED = a human revoked the approval on this SHA; never re-approve. + const priorOnSha = botReviews.filter(r => + r.commit_id === sha && (r.state === 'APPROVED' || r.state === 'DISMISSED') + ); + if (priorOnSha.length > 0) { + core.info('Bot already approved or was dismissed on this SHA, skipping'); + return; + } + + // NONE = no prior repo association, so treat it like a first-timer. + const firstTime = ['FIRST_TIME_CONTRIBUTOR', 'FIRST_TIMER', 'NONE'] + .includes(pr.author_association); + const largeDiff = (pr.additions + pr.deletions) > 500 || pr.changed_files > 20; + const highRisk = firstTime || largeDiff; + + core.info(`Approving PR #${prNumber}: bot review clean + CI passed for SHA ${sha}` + + (highRisk ? ' [high-risk: first-time contributor or large diff]' : '')); + await github.rest.pulls.createReview({ + owner, repo, pull_number: prNumber, + event: 'APPROVE', + body: `No issues found and CI is passing. Auto-approved.\n\n---\n*Generated by AI — human merge required.*` + }); + + core.setOutput('approved', 'true'); + core.setOutput('highrisk', highRisk ? 'true' : 'false'); + + # id-token only, no pull-requests: publishes the approval metric for the + # CloudWatch alarms without holding both GitHub-write and AWS in one job. + emit: + needs: approve + if: needs.approve.outputs.approved == 'true' + runs-on: ubuntu-latest + timeout-minutes: 2 + permissions: + id-token: write + steps: + - name: Configure AWS credentials (OIDC) + id: creds + continue-on-error: true # no AWS_METRICS_ROLE_ARN => skip metric, don't fail + uses: aws-actions/configure-aws-credentials@cabfdba3510de1431bac9dba27511d97497fc100 # v5 + with: + # PutMetricData-only role (MetricsRoleArn stack output), NOT the full + # AWS_ROLE_ARN — this workflow only publishes a metric. + role-to-assume: ${{ secrets.AWS_METRICS_ROLE_ARN }} + role-session-name: shadow-approve-${{ github.run_id }}-${{ github.run_attempt }} + # Must match your CFN BedrockRegion, or the alarms (which are regional) + # never see these metrics. Change here if you deploy outside us-east-1. + aws-region: us-east-1 + - name: Emit approval metric + if: steps.creds.outcome == 'success' + continue-on-error: true # a dropped datapoint must not redden an approved run + env: + REPO: ${{ github.repository }} + HIGHRISK: ${{ needs.approve.outputs.highrisk }} + run: | + # `|| true` per call + HighRisk first: a throttle on one metric must + # not drop the other, least of all the first-high-risk-approval signal. + if [ "$HIGHRISK" = "true" ]; then + aws cloudwatch put-metric-data \ + --namespace Shadow \ + --metric-data "MetricName=HighRiskApproval,Value=1,Unit=Count,Dimensions=[{Name=Repository,Value=${REPO}},{Name=Pipeline,Value=agentic}]" || true + fi + aws cloudwatch put-metric-data \ + --namespace Shadow \ + --metric-data "MetricName=ApprovalGranted,Value=1,Unit=Count,Dimensions=[{Name=Repository,Value=${REPO}},{Name=Pipeline,Value=agentic}]" || true diff --git a/infrastructure/shadow-iam-stack.yaml b/infrastructure/shadow-iam-stack.yaml index 585602b..8417874 100644 --- a/infrastructure/shadow-iam-stack.yaml +++ b/infrastructure/shadow-iam-stack.yaml @@ -16,7 +16,7 @@ Metadata: - Label: { default: "Bedrock region" } Parameters: [BedrockRegion] - Label: { default: "Cost protection & anomaly alarms (optional)" } - Parameters: [MonthlyBudgetLimit, BudgetEmailAddress, EscalationSpikeThreshold, InvocationSpikeThreshold] + Parameters: [MonthlyBudgetLimit, BudgetEmailAddress, EscalationSpikeThreshold, InvocationSpikeThreshold, ApprovalSpikeThreshold, HighRiskApprovalThreshold] - Label: { default: "Prompt-injection guardrail" } Parameters: [ProvisionGuardrail] ParameterLabels: @@ -29,6 +29,8 @@ Metadata: BudgetEmailAddress: { default: "Email for budget + anomaly-alarm notifications (also enables the alarms)" } EscalationSpikeThreshold: { default: "Escalations/hour that trips the anomaly alarm (default 25)" } InvocationSpikeThreshold: { default: "Invocations/hour that trips the anomaly alarm (default 100)" } + ApprovalSpikeThreshold: { default: "Auto-approvals/hour that trip the approval-spike alarm (default 5)" } + HighRiskApprovalThreshold: { default: "High-risk auto-approvals/hour before alerting (default 0 = first one)" } ProvisionGuardrail: { default: "Provision a Bedrock Guardrail (recommended; default ON)" } Parameters: @@ -131,6 +133,24 @@ Parameters: Default: 100 MinValue: 1 + ApprovalSpikeThreshold: + Type: Number + Description: >- + Fleet-wide auto-approvals/hour above which the approval-spike alarm fires. + Only meaningful where SHADOW_AUTO_APPROVE is enabled. Auto-approval is + opt-in and rare on most repos, so the default is deliberately low. + Default: 5 + MinValue: 1 + + HighRiskApprovalThreshold: + Type: Number + Description: >- + Auto-approvals of a high-risk PR (first-time contributor or large diff) + per hour above which the high-risk alarm fires. Default 0 alerts on the + first one, since a single such approval is worth a human look. + Default: 0 + MinValue: 0 + ProvisionGuardrail: Type: String Description: >- @@ -358,6 +378,48 @@ Resources: - { Key: ManagedBy, Value: shadow-iam-stack } - { Key: Component, Value: shadow-bot } + ShadowMetricsRole: + # Assumed by the adopter's auto-approve.yml emit job. Least-privilege: only + # PutMetricData, so the approval workflow (which is a distinct trust surface + # from shadow-review.yml) never gets Bedrock/Secrets/S3. Trust is pinned to + # auto-approve.yml so no other workflow can assume it. + Type: AWS::IAM::Role + Properties: + RoleName: shadow-bot-metrics + MaxSessionDuration: 3600 + AssumeRolePolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Principal: + Federated: !If + - CreateOidcProvider + - !Ref GitHubOidcProvider + - !Ref ExistingOidcProviderArn + Action: sts:AssumeRoleWithWebIdentity + Condition: + StringEquals: + "token.actions.githubusercontent.com:aud": sts.amazonaws.com + StringLike: + "token.actions.githubusercontent.com:sub": + !Sub "repo:${GitHubOrg}/${GitHubRepo}:*" + "token.actions.githubusercontent.com:job_workflow_ref": + !Sub "${GitHubOrg}/${GitHubRepo}/.github/workflows/auto-approve.yml@*" + Policies: + - PolicyName: CloudWatchMetrics + PolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Action: cloudwatch:PutMetricData + Resource: "*" + Condition: + StringEquals: + "cloudwatch:namespace": Shadow + Tags: + - { Key: ManagedBy, Value: shadow-iam-stack } + - { Key: Component, Value: shadow-bot } + ShadowMonthlyBudget: # Optional fleet-wide cost protection. The per-PR caps # (BOT_INVESTIGATOR_MAX_TOOL_CALLS, BOT_MAX_DIFF_FOR_REVIEW_CHARS) bound a @@ -460,6 +522,54 @@ Resources: AlarmActions: - !Ref ShadowAlarmTopic + ShadowApprovalSpikeAlarm: + # A bot APPROVE can satisfy required review, so an approval burst is the + # highest-signal abuse indicator (invocation spike is only a proxy). + # ApprovalGranted comes from the caller's auto-approve.yml, same namespace. + Type: AWS::CloudWatch::Alarm + Condition: CreateAlarms + Properties: + AlarmName: !Sub "shadow-bot-approval-spike-${GitHubOrg}" + AlarmDescription: >- + Fleet-wide auto-approvals exceeded the threshold in a 1-hour window. + Each is a bot APPROVE that can satisfy required review — inspect the + approved PRs before assuming benign. + EvaluationPeriods: 1 + Threshold: !Ref ApprovalSpikeThreshold + ComparisonOperator: GreaterThanThreshold + TreatMissingData: notBreaching + Metrics: + - Id: appr_total + Label: ApprovalsPerHourFleetWide + Expression: 'SELECT SUM("ApprovalGranted") FROM "Shadow"' + Period: 3600 + ReturnData: true + AlarmActions: + - !Ref ShadowAlarmTopic + + ShadowHighRiskApprovalAlarm: + # A single auto-approval of a first-time contributor or large diff is worth + # a look on its own; the default threshold 0 pages on the first one. + Type: AWS::CloudWatch::Alarm + Condition: CreateAlarms + Properties: + AlarmName: !Sub "shadow-bot-highrisk-approval-${GitHubOrg}" + AlarmDescription: >- + A high-risk PR (first-time contributor or large diff) was auto-approved. + Review the PR and confirm the bot's clean verdict was correct. + EvaluationPeriods: 1 + Threshold: !Ref HighRiskApprovalThreshold + ComparisonOperator: GreaterThanThreshold + TreatMissingData: notBreaching + Metrics: + - Id: hr_total + Label: HighRiskApprovalsPerHourFleetWide + Expression: 'SELECT SUM("HighRiskApproval") FROM "Shadow"' + Period: 3600 + ReturnData: true + AlarmActions: + - !Ref ShadowAlarmTopic + Outputs: ShadowRoleArn: Description: >- @@ -468,6 +578,12 @@ Outputs: repository secret. Value: !GetAtt ShadowBotRole.Arn + MetricsRoleArn: + Description: >- + Paste as the `AWS_METRICS_ROLE_ARN` repository secret. Only the + auto-approve.yml emit job uses it; PutMetricData-only. + Value: !GetAtt ShadowMetricsRole.Arn + OidcProviderArn: Description: GitHub Actions OIDC provider ARN (created or reused). Value: !If diff --git a/src/scripts/shadow/bedrock_client.py b/src/scripts/shadow/bedrock_client.py index 212fd1b..601a32e 100644 --- a/src/scripts/shadow/bedrock_client.py +++ b/src/scripts/shadow/bedrock_client.py @@ -27,23 +27,12 @@ def _is_throttling_error(exc): return False return code == "ThrottlingException" or code == "TooManyRequestsException" -# Models that reject sampling params (temperature, top_p, top_k) in -# inferenceConfig — Opus 4.7 returns HTTP 400 if any are present. This -# codebase only ever sends `temperature` (top_p / top_k are never set in -# any call path), so the helper only strips `temperature`; if a future -# caller starts setting top_p / top_k, the helper signature and the strip -# branch must be extended in lockstep. -# Model families that reject sampling params (temperature) on Bedrock Converse: -# Opus 4.7/4.8/4.9 and single-digit Opus/Sonnet majors >= 5. Every alternative -# is boundary-anchored with (?!\d) so it matches the whole version token and -# never substring-matches a larger number or a date suffix — `opus-4-70`, -# `opus-50`, `sonnet-20240229` (Claude 3, which accepts temperature) do NOT -# match. Kept to single-digit majors deliberately: a hypothetical two-digit -# major (opus-10) is not enumerated here, but this is a best-effort fast path, -# NOT the only safeguard — converse() retries without sampling params if a model -# reports the param rejected (see _converse_with_retry), so any family missing -# here still degrades gracefully (one extra call + a warning) instead of -# hard-failing. Add new families here to skip that retry. +# Model families that reject `temperature` on Bedrock Converse (Opus 4.7+ and +# single-digit Opus/Sonnet majors >= 5). Each alternative is boundary-anchored +# with (?!\d) so it can't substring-match a larger number or date suffix +# (`opus-4-70`, `sonnet-20240229`). This is only a fast path: _converse_with_retry +# retries without sampling params if a model reports it rejected, so a family +# missing here still degrades gracefully rather than hard-failing. _NO_SAMPLING_PARAMS_PATTERN = re.compile( r"opus-4-[7-9](?!\d)" r"|opus-[5-9](?!\d)" diff --git a/src/scripts/shadow/cloudwatch.py b/src/scripts/shadow/cloudwatch.py index abf6557..47bcf34 100644 --- a/src/scripts/shadow/cloudwatch.py +++ b/src/scripts/shadow/cloudwatch.py @@ -23,25 +23,13 @@ def emit_metrics(*, repository, metrics, action, pipeline="agentic", region=None, reason=None, base_metrics=True, post_failure_count=0, slack_failure_count=0): - """Push a small batch of metrics to CloudWatch. - - `metrics` is the artifact's `metrics` dict (post-_finalize_metrics). - `reason` (optional) is the artifact's reason field — attached as a - `Reason` dimension so operator dashboards can group SKIP/ESCALATE - counts by cause. The Reason value is keyed on the prefix before the - first `:` to bound dimension cardinality: every (model_id, exception) - combo would otherwise mint a separate stream (~$0.30/month each). - `base_metrics=False` lets the act() post-failure path emit only the - PostFailures / SlackDeliveryFailures metrics without re-counting - Invocations against analyze's. - - `slack_failure_count` (act-time) emits a SlackDeliveryFailures metric so a - Slack-only team sees their escalation channel go dark — the Slack surface - was otherwise the one integration with no failure metric, unlike GitHub - posts (PostFailures). - - Returns (ok, reason). Caller logs the reason but never fails on it — - metrics are observability, not a posting prerequisite.""" + """Push a small batch of metrics to CloudWatch. Returns (ok, reason); + callers never fail on it — metrics are observability, not a prerequisite. + + `reason` is keyed on the prefix before the first `:` so a (model, exception) + reason can't mint a fresh dimension stream per combo. `base_metrics=False` + lets act()'s post-failure path emit only PostFailures/SlackDeliveryFailures + without double-counting Invocations.""" if os.getenv("SHADOW_CLOUDWATCH_DISABLED", "").lower() == "true": return False, "disabled by env" namespace = os.getenv("SHADOW_CLOUDWATCH_NAMESPACE", _DEFAULT_NAMESPACE) diff --git a/src/scripts/shadow/config.py b/src/scripts/shadow/config.py index 504ad35..86c936b 100644 --- a/src/scripts/shadow/config.py +++ b/src/scripts/shadow/config.py @@ -39,13 +39,8 @@ def __init__(self): shadow_config.get(yml, "models", "investigator"), _DEFAULT_MODEL, ), _DEFAULT_MODEL) - # Reporter is JSON-formatting only; Haiku handles structured output - # well enough AND is the model the issue path REQUIRES (Opus 4.7/4.8 - # reject outputConfig.textFormat over Bedrock today). Adopters who - # override BEDROCK_MODEL_ID (Investigator) without setting Reporter - # silently get Haiku here — log loud at INFO so the asymmetry is - # visible in workflow logs rather than discovered via "why does my - # Reporter look weaker than my Investigator". + # Overriding only BEDROCK_MODEL_ID leaves Reporter on the Haiku default; + # the INFO log below makes that asymmetry visible in workflow logs. self.reporter_model_id = _scrub_model_id(shadow_config.env_or( "BEDROCK_REPORTER_MODEL_ID", shadow_config.get(yml, "models", "reporter"), @@ -58,14 +53,9 @@ def __init__(self): "BEDROCK_REPORTER_MODEL_ID to override.", self.reporter_model_id, self.bedrock_model_id, ) - # Model for the issue-triage / issue-respond / followup answers. - # Defaults to the reporter model (Haiku) so behavior is unchanged, but - # is a distinct knob: issue answers are customer-facing prose that - # benefit from a stronger model (bench: Sonnet 4.6 >> Haiku on accuracy - # + hallucination), whereas the PR reporter only serializes the Critic's - # verdicts into JSON and stays cheap. Must still support structured - # output over Bedrock (Haiku 4.5 / Sonnet 4.6 do; Opus 4.8 / Sonnet 5 - # do NOT), same constraint as the reporter. + # Distinct knob from reporter (defaults to it): issue answers are + # customer-facing prose worth a stronger model, but must still support + # structured output over Bedrock (Haiku 4.5 / Sonnet 4.6 do; Opus does not). self.issue_model_id = _scrub_model_id(shadow_config.env_or( "BEDROCK_ISSUE_MODEL_ID", shadow_config.get(yml, "models", "issue"), @@ -92,16 +82,9 @@ def __init__(self): self.dry_run = os.getenv("DRY_RUN", "false").lower() == "true" - # Guardrail-required mode. Defaults ON: a production run (DRY_RUN=false) - # without GUARDRAIL_ID set is a misconfiguration — the CFN Launch Stack - # provisions one by default and outputs the ID; if the adopter skipped - # pasting it into a secret, refuse rather than silently run with the - # local sanitizer alone. DRY_RUN=true bypasses the check so adopters - # can still validate end-to-end without a guardrail in the dry path. - # Override BOT_REQUIRE_GUARDRAIL=false to point at a custom hand-built - # guardrail provisioned outside the CFN stack and not yet wired in, - # OR to deliberately accept the local-sanitizer-only stance for a - # specific deployment (not recommended). + # Default ON: a production run (DRY_RUN=false) without GUARDRAIL_ID + # refuses rather than run sanitizer-only. DRY_RUN bypasses; + # BOT_REQUIRE_GUARDRAIL=false opts out for a custom/hand-built guardrail. self.require_guardrail = os.getenv( "BOT_REQUIRE_GUARDRAIL", "true", ).strip().lower() not in ("0", "false", "no", "off") @@ -258,14 +241,10 @@ def _require(name): def _int_env(name, default): - """Garbage env values fall back to default rather than killing analyze() - before any artifact is written. Negative values also fall back to default - with a named warning — every _int_env caller is a positive cost/wall-clock - cap whose downstream consumer either gates on `> 0` (max_diff_for_review, - max_files_for_review at main.py:1152-1153) or treats negative values as - immediate-exhaustion (tool-call / turn / budget caps), so a `-1` typo - would silently disable pre-flight defenses or short-circuit the agent - pipeline.""" + """Unparseable or negative env values fall back to default (never crash + analyze() before an artifact is written). Negatives fall back rather than + pass through because every caller is a positive cost/wall-clock cap where a + `-1` would disable a pre-flight defense or short-circuit the pipeline.""" raw = os.getenv(name) try: n = int(raw) if raw else default @@ -293,15 +272,10 @@ def _scrub_codeblock_token(val): def _scrub_attribution(val): - """One-line footer attribution, appended to posted comments outside the - sanitize() boundary — so it must be self-scrubbing to the same standard the - sanitizer enforces on model output, or adopter config becomes a way to slip - unsanitized text past it. Non-string/empty yields "". - - Removes: newlines/backticks/`` (would break the one-line footer or - the adjacent clean-marker); any sanitizer injection marker (so config can't - bypass the marker block bot output is held to); and neutralizes `@`/`#` so a - stray value can't fire GitHub auto-mentions or issue-refs. Length-capped.""" + """Appended to comment footers OUTSIDE the sanitize() boundary, so it must + self-scrub to the sanitizer's standard or adopter config becomes a bypass. + Strips newlines/backticks/comment delimiters + injection markers, and + zero-width-breaks `@`/`#` so a typo can't fire mentions/issue-refs.""" if not isinstance(val, str): return "" val = val.replace("\r", " ").replace("\n", " ").replace("`", "") @@ -357,25 +331,12 @@ def _parse_allowed_labels(env_val, yaml_val, default): def _int_in_range_or_default(val, default, *, name): - """Yaml int, env str, or wrong type → default. Cap range [0, 100] so a - yaml typo can't set the bound absurdly high. `0` is valid and means - "no limit applied" (callers decide per-field semantics — e.g., - max_bot_replies=0 means escalate-on-first-followup, max_runs_per_hour=0 - means disable the rate limit). Garbage strings (`"--5"`, `"++5"`, - `"5 abc"`) fall back to default rather than crash Config(). - - Negative parsed ints fall back to default (with warning) rather than - clamping to 0 — clamping a negative typo to 0 silently flips - rate-limit/reply semantics into "disabled" / "escalate-on-first-reply", - which is the exact opposite of what an operator typing -1 intended. - Out-of-range positive ints clamp to 100 with warning so an operator - who set BOT_MAX_RUNS_PER_HOUR=200 hoping to double the cap doesn't - silently get the default (20) — the original rate-limit-tuning footgun. - Both diagnostics name the env var so the misconfig is greppable in - workflow logs. - - `name` is keyword-only and required so a future caller can't omit it - and silently re-introduce the no-warning footgun.""" + """int/str/wrong-type → default, clamped to [0, 100]. `0` is valid ("no + limit"; per-field meaning). Negatives fall back to default rather than + clamp to 0, which would flip rate-limit/reply semantics to "disabled" — + the opposite of what `-1` intends. Positives above 100 clamp to 100 (not + default) so BOT_MAX_RUNS_PER_HOUR=200 doesn't silently revert to 20. + `name` is required (keyword-only) so a caller can't drop the warning.""" if isinstance(val, bool): logger.warning( "%s=%r is a bool, not an int; using default %d", name, val, default, @@ -428,21 +389,13 @@ def _scrub_model_id(val, default): def _scrub_marker_token(val, default): - """Embedded in ``; reject HTML-comment-breaking chars. - Collapses runs of `-`/`_` to a single char so adjacent dashes can't break - out of the `` envelope (HTML forbids `--` inside a comment - body — `` would close the comment early). - - Also rejects a small, hardcoded list of token-suffix patterns whose - rendered marker (``) would trip the sanitizer's - injection-marker list (e.g., bot_name='system' renders - ``; the `system:` substring matches sanitizer's - `system:` marker → every clean review nulls to ESCALATE without a - marker). The reject-list is intentionally NOT imported from - sanitizer._INJECTION_MARKERS — that list grows as new injection classes - are discovered, and each addition must NOT silently invalidate adopters' - deployed bot_name. Keep this list narrow and bump it explicitly with a - CHANGELOG entry when a new bot_name pattern needs blocking.""" + """Embedded in ``. Rejects HTML-comment-breaking chars + and collapses `--`/`__` runs (HTML forbids `--` inside a comment, so + `shadow--evil` would close the envelope early). Also rejects names whose + rendered marker would substring-match a sanitizer injection marker (only + `system` does today) — NOT imported from sanitizer._INJECTION_MARKERS, + since that list grows and must not retroactively invalidate a deployed + bot_name.""" if not isinstance(val, str): return default val = "".join(c for c in val if c.isalnum() or c in "-_") @@ -452,11 +405,6 @@ def _scrub_marker_token(val, default): val = val[:32] if not val: return default - # Hardcoded reject-list. The rendered marker shape is `:clean -->`, so - # a bot_name ending in any of these tokens produces `:clean` which - # would substring-match sanitizer._INJECTION_MARKERS. Today only `system` - # actually trips a marker; the others are reserved for future markers - # added in lockstep with a CHANGELOG entry. _REJECTED_NAME_SUFFIXES = ("system",) lower_val = val.lower() for suffix in _REJECTED_NAME_SUFFIXES: diff --git a/src/scripts/shadow/github_client.py b/src/scripts/shadow/github_client.py index 9631399..bbe51f7 100644 --- a/src/scripts/shadow/github_client.py +++ b/src/scripts/shadow/github_client.py @@ -271,13 +271,10 @@ def count_recent_workflow_runs_for_item(self, item_number, - Same item (issue/issue_comment): name OR display_title contains `#` - Exclude current run: run.id != GITHUB_RUN_ID - The pull_requests array fixes the prior `head_branch == pull/` - check, which never matched real workflow_run records (head_branch is - the source branch name, not a refs/pull/... synthetic). The name+id - filter covers issue_comment events — but ONLY when the adopter's - caller workflow embeds the item number via `run-name:` (the reusable + The issue/issue_comment match works ONLY when the adopter's caller + workflow embeds the item number via `run-name:` (the reusable workflow's own run-name is ignored under workflow_call). See - `examples/caller-workflow.yml` for the canonical template. + `examples/caller-workflow.yml`. """ try: since = (datetime.datetime.now(datetime.timezone.utc) @@ -415,15 +412,10 @@ def post_pr_review(self, number, summary, inline_comments, event="COMMENT", ] payload = {"body": body, "event": event} - # Pin the review to the exact commit that was analyzed. Without commit_id, - # GitHub stamps the review with the PR's current head at post time — so a - # push landing between analyze and act would attach a review (computed - # against the old code) to the new head. Downstream automation keying on - # review.commit_id (e.g. an auto-approve gate that requires the review's - # commit to equal the tip) then behaves on a mismatched pairing. Sending - # the analyzed SHA keeps the review honestly bound to what it reviewed; - # if head has moved on, such a gate correctly declines rather than - # approving stale analysis. + # Pin the review to the analyzed commit. Without commit_id GitHub stamps + # it with the current head at post time, so a push between analyze and + # act would bind old-code analysis to the new head — and an auto-approve + # gate keying on review.commit_id == tip would then act on a mismatch. if commit_id: payload["commit_id"] = commit_id if valid_comments: diff --git a/src/scripts/shadow/main.py b/src/scripts/shadow/main.py index ece8076..25c6c81 100644 --- a/src/scripts/shadow/main.py +++ b/src/scripts/shadow/main.py @@ -168,15 +168,9 @@ def analyze(): _write_artifact({"action": "SKIP", "reason": "fetch_failed"}) return - # The model id stamped on this item's artifacts — and interpolated into the - # user-facing "Generated by AI (model: …)" footer act() posts. The - # issue/followup surface invokes the reporter model (Haiku; Opus 4.7/4.8 reject - # structured output over Bedrock), the PR pipeline invokes the investigator - # model. Name the model that actually runs on THIS surface so the footer is - # truthful. Computed once here so the many early-exit artifacts below can't - # drift onto the wrong model. (The PR pipeline's own artifacts set this - # internally in _write_artifact_pipeline; a multi-model PR review naming - # its Opus investigator is deliberate.) + # Model that actually runs on THIS surface (PR→investigator, issue→issue + # model), so the "Generated by AI (model: …)" footer is truthful. Computed + # once so the early-exit artifacts below can't drift onto the wrong model. surface_model_id = cfg.bedrock_model_id if is_pr else cfg.issue_model_id author = _nested_get(item, "user", "login", default="") @@ -1132,9 +1126,8 @@ def _clamp(value, lo, hi): # Permissive: tolerates markdown bold and missing-colon variants like -# `**STATUS:** CONFIRMED`, `STATUS:CONFIRMED`, `STATUS: **CONFIRMED**`. The -# strict line-anchored form silently dropped findings whenever prompt -# guidance drifted to bolded markers. +# `**STATUS:** CONFIRMED`, `STATUS:CONFIRMED`, `STATUS: **CONFIRMED**`, so a +# bolded marker in the model output doesn't silently drop the finding. _STATUS_CONFIRMED_RE = re.compile( r"STATUS\s*:?\s*\**\s*CONFIRMED", re.IGNORECASE, @@ -1746,21 +1739,14 @@ def _build_clean_response(gh, head_sha, inline_comments, bot_name="shadow"): def _scrub_ci_summary(text): - """Neutralize HTML-comment delimiters AND sanitizer injection markers in - third-party check-run names (e.g., GitHub Actions matrix builds emit - `Build (system: x86_64)` — the `system:` substring would trip the - post-time `sanitize()` call in `act()` and null the entire clean - response to ESCALATE without a marker). For every injection-marker - substring present, inserts a zero-width-joiner between the first and - second character so the rendered text reads identically but the - substring no longer matches sanitize()'s lowercased contains-check. - Handles every marker shape — including spaceful ones like "ignore - previous instructions" that have no `:` or `<` to entitize.""" + """Neutralize comment delimiters + injection markers in third-party check-run + names before they reach act()'s sanitize() — e.g. a matrix build named + `Build (system: x86_64)` would otherwise trip the `system:` marker and null + the clean response to ESCALATE. Uses a zero-width joiner (invisible, but + breaks the substring match) rather than deletion so the name still reads + right; re-checks after each sub to catch repeats.""" text = text.replace("", "-->") from .sanitizer import _INJECTION_MARKERS - # Zero-width joiner (U+200D) is invisible in rendered text but breaks - # substring matches. Re-checking `marker in lower` after each substitution - # ensures repeat occurrences within the same string all get scrubbed. zwj = "‍" lower = text.lower() for marker in _INJECTION_MARKERS: @@ -1769,9 +1755,6 @@ def _scrub_ci_summary(text): pattern = re.compile(re.escape(marker), re.IGNORECASE) def _break(m): s = m.group(0) - # Insert zwj between char 0 and char 1; preserves case+spacing, - # invisible to readers, but the lowercased substring no longer - # equals the marker so sanitize() lets it through. return s[0] + zwj + s[1:] text = pattern.sub(_break, text) lower = text.lower() @@ -1880,9 +1863,8 @@ def _compute_cost_usd(metrics): # Permissive: tolerates markdown bold and `:` instead of `|` as the -# id/verdict separator (e.g. `**VERDICT:** C1 | UPHELD`, -# `VERDICT: C1: UPHELD`). The strict anchored form silently produced None -# whenever the model formatted verdicts in markdown. +# id/verdict separator (e.g. `**VERDICT:** C1 | UPHELD`, `VERDICT: C1: UPHELD`), +# so a markdown-formatted verdict isn't silently parsed as None. _VERDICT_RE = re.compile( r"VERDICT\s*:\s*\**\s*\S+\s*[\|:]\s*\**\s*(UPHELD|OVERTURNED)\b", re.IGNORECASE | re.MULTILINE, @@ -2024,19 +2006,11 @@ def _build_provenance(cfg): def _build_provenance_from_env(): - """Best-effort provenance for SKIP/ESCALATE paths that don't have a Config - in scope. Mirrors Config's env>default resolution and DEFAULTS: investigator - /critic default to Opus, but reporter AND issue default to Haiku (Config's - _DEFAULT_REPORTER_MODEL), NOT to BEDROCK_MODEL_ID — else an adopter who sets - only BEDROCK_MODEL_ID=Opus would see the reporter/issue misattributed to Opus - in early-exit artifacts while runtime actually ran Haiku. (yaml-only model - overrides are not visible here — this path has no Config; those artifacts are - the cheap SKIP/ESCALATE ones, and the full path uses _build_provenance(cfg).)""" - # Mirror Config's defaults from the single source of truth so a future - # default-model bump can't drift this early-exit path out of sync. Use the - # `(getenv or "").strip() or default` idiom on every line so an empty or - # whitespace env value falls back to the default, matching Config.env_or - # (a bare `getenv(name, default)` would stamp "" when the var is set-empty). + """Provenance for SKIP/ESCALATE paths with no Config in scope. Reporter AND + issue must default to Haiku (_DEFAULT_REPORTER_MODEL), NOT BEDROCK_MODEL_ID, + or setting only BEDROCK_MODEL_ID=Opus misattributes them to Opus while + runtime ran Haiku. Mirrors Config's defaults from the same constants so a + default bump can't drift this path.""" investigator = (os.getenv("BEDROCK_MODEL_ID") or "").strip() or _DEFAULT_MODEL reporter = (os.getenv("BEDROCK_REPORTER_MODEL_ID") or "").strip() or _DEFAULT_REPORTER_MODEL return { diff --git a/src/scripts/shadow/prompts.py b/src/scripts/shadow/prompts.py index 6b29eca..d55d877 100644 --- a/src/scripts/shadow/prompts.py +++ b/src/scripts/shadow/prompts.py @@ -198,16 +198,10 @@ def prompt_version(template): # Every prompt the bot loads at runtime, captured by # compute_prompt_provenance() and validated by doctor. Tuples of # (stage, env_var, filename, sm_fallback_to_disk). Order is the published -# artifact ordering; do not reorder casually. Each sm_fallback flag MUST match -# the corresponding getter, or provenance mislabels a fell-back prompt's source. -# -# All four surfaces are first-class here, not just PR review: the 3 issue/ -# followup prompts drive customer-visible output (issue classification, -# citation-backed answers, follow-up replies) exactly as the 5 PR prompts do, -# so they belong in the audit rollup and the doctor's loadability check too. -# The issue/followup core prompts are fail-closed (sm_fallback=False) to match -# their getters below — a missing/misconfigured secret must surface as "" (and -# ESCALATE), never silently run the bundled default on a customer-visible reply. +# artifact ordering; do not reorder. Each sm_fallback flag MUST match the +# corresponding getter, or provenance mislabels a fell-back prompt's source. +# The issue/followup core prompts are fail-closed (sm_fallback=False): a +# missing secret must surface as "" (→ESCALATE), not silently run the default. _ACTIVE_PROMPTS = ( ("investigator", "PR_INVESTIGATOR_PROMPT", "pr-investigator.txt", False), ("critic", "PR_CRITIC_PROMPT", "pr-critic.txt", False), diff --git a/src/scripts/shadow/sanitizer.py b/src/scripts/shadow/sanitizer.py index 54adb00..017d63a 100644 --- a/src/scripts/shadow/sanitizer.py +++ b/src/scripts/shadow/sanitizer.py @@ -20,13 +20,9 @@ (re.compile(r"-----BEGIN [A-Z ]*PRIVATE KEY-----"), "private_key_header"), (re.compile(r"AccountKey\s*=\s*[A-Za-z0-9+/=]{40,}"), "azure_storage_key"), (re.compile(r"_authToken\s*=\s*\S+"), "npm_authtoken"), - # URL with embedded credentials: scheme://[user]:pw@host (any scheme). - # Covers postgres://, mysql://, mongodb://, redis://, amqp://, ftp://, - # https?://, etc. Database URLs frequently carry creds in the userinfo - # segment and previously slipped past the https?:// floor. The user - # component is optional (`*`) so `redis://:password@host` — Redis's - # standard "no-username" shape — is still recognised. The leading - # `scheme://` anchor prevents false positives on prose like + # URL with embedded credentials: scheme://[user]:pw@host (any scheme, to + # catch db URLs like postgres://). User is optional so `redis://:pw@host` + # matches; the `scheme://` anchor avoids false positives on prose like # `meeting at 12:30@home`. (re.compile(r"[a-zA-Z][a-zA-Z0-9+.\-]*://[^\s/:]*:[^\s/@]+@"), "url_with_credentials"), # JWT (header.payload.signature). Real tokens have base64url segments diff --git a/tests/contract/test_cfn_template.py b/tests/contract/test_cfn_template.py index f58709d..449f107 100644 --- a/tests/contract/test_cfn_template.py +++ b/tests/contract/test_cfn_template.py @@ -41,10 +41,12 @@ def test_template_top_level_shape(doc): assert doc["AWSTemplateFormatVersion"] == "2010-09-09" assert "Description" in doc assert set(doc["Resources"].keys()) == { - "GitHubOidcProvider", "ShadowBotRole", "ShadowMonthlyBudget", + "GitHubOidcProvider", "ShadowBotRole", "ShadowMetricsRole", + "ShadowMonthlyBudget", "ShadowGuardrail", "ShadowGuardrailVersion", "ShadowAlarmTopic", "ShadowEscalationSpikeAlarm", - "ShadowInvocationSpikeAlarm", + "ShadowInvocationSpikeAlarm", "ShadowApprovalSpikeAlarm", + "ShadowHighRiskApprovalAlarm", } @@ -56,14 +58,15 @@ def test_required_parameters_present(doc): "ExistingOidcProviderArn", "MonthlyBudgetLimit", "BudgetEmailAddress", "EscalationSpikeThreshold", "InvocationSpikeThreshold", + "ApprovalSpikeThreshold", "HighRiskApprovalThreshold", "ProvisionGuardrail"} assert set(doc["Parameters"].keys()) == expected def test_required_outputs_present(doc): # README points adopters at these output names. - expected = {"ShadowRoleArn", "OidcProviderArn", "TrustPolicyScope", - "BedrockRegion", "NextSteps", + expected = {"ShadowRoleArn", "MetricsRoleArn", "OidcProviderArn", + "TrustPolicyScope", "BedrockRegion", "NextSteps", "GuardrailId", "GuardrailVersion", "AlarmTopicArn"} assert set(doc["Outputs"].keys()) == expected @@ -135,6 +138,50 @@ def test_monthly_budget_parameter_present(doc): assert params["BudgetEmailAddress"]["Default"] == "" +def test_metrics_role_is_putmetricdata_only(doc): + """The emit job's role must be least-privilege: only cloudwatch:PutMetricData + scoped to namespace Shadow, and NOTHING else (no Bedrock/Secrets/S3). This is + a distinct trust surface from shadow-review.yml, so a leak here would hand the + approval workflow the full engine role.""" + role = doc["Resources"]["ShadowMetricsRole"]["Properties"] + policies = role["Policies"] + assert len(policies) == 1, "metrics role must carry exactly one policy" + statements = policies[0]["PolicyDocument"]["Statement"] + assert len(statements) == 1 + stmt = statements[0] + assert stmt["Effect"] == "Allow" + assert stmt["Action"] == "cloudwatch:PutMetricData", \ + "metrics role must grant ONLY PutMetricData" + assert stmt["Condition"]["StringEquals"]["cloudwatch:namespace"] == "Shadow" + # No engine permission anywhere in THIS role's policies. + blob = str(policies) + for forbidden in ("bedrock:", "secretsmanager:", "s3:"): + assert forbidden not in blob, f"metrics role leaked {forbidden}" + # Trust must pin to auto-approve.yml, not shadow-review.yml. + cond = role["AssumeRolePolicyDocument"]["Statement"][0]["Condition"]["StringLike"] + jwr = cond["token.actions.githubusercontent.com:job_workflow_ref"] + assert "auto-approve.yml" in jwr and "shadow-review.yml" not in jwr + + +def test_approval_alarm_threshold_defaults(doc): + """HighRiskApprovalThreshold=0 is security-load-bearing: with + GreaterThanThreshold it pages on the first high-risk auto-approval. Pin the + defaults so a bump can't silently disable that.""" + params = doc["Parameters"] + assert params["ApprovalSpikeThreshold"]["Type"] == "Number" + assert params["ApprovalSpikeThreshold"]["Default"] == 5 + assert params["ApprovalSpikeThreshold"]["MinValue"] == 1 + assert params["HighRiskApprovalThreshold"]["Type"] == "Number" + assert params["HighRiskApprovalThreshold"]["Default"] == 0 + assert params["HighRiskApprovalThreshold"]["MinValue"] == 0 + # Default 0 only means "page on the first" under GreaterThanThreshold; pin + # the operator + notBreaching or the semantics silently change. + for name in ("ShadowApprovalSpikeAlarm", "ShadowHighRiskApprovalAlarm"): + props = doc["Resources"][name]["Properties"] + assert props["ComparisonOperator"] == "GreaterThanThreshold" + assert props["TreatMissingData"] == "notBreaching" + + def test_monthly_budget_resource_present_when_set(doc): """The Budget resource must filter on Bedrock spend only — a missing CostFilter would alert on the entire account's spend, masking what @@ -168,20 +215,29 @@ def test_behavioral_alarms_aggregate_the_emitted_metrics(doc): dimensions, and CloudWatch rejects SEARCH in an alarm. Metric names are cross-checked against cloudwatch.py so an emitter rename can't leave the alarm targeting a metric that's never published.""" - emitter_src = ( - Path(__file__).resolve().parents[2] - / "src/scripts/shadow/cloudwatch.py" - ).read_text() + root = Path(__file__).resolve().parents[2] + emitter_src = (root / "src/scripts/shadow/cloudwatch.py").read_text() + # ApprovalGranted / HighRiskApproval are published by auto-approve.yml, not + # the engine, so cross-check those two against the workflow. + workflow_src = (root / "examples/auto-approve.yml").read_text() alarms = { - "ShadowEscalationSpikeAlarm": ("Escalations", "EscalationSpikeThreshold"), - "ShadowInvocationSpikeAlarm": ("Invocations", "InvocationSpikeThreshold"), + "ShadowEscalationSpikeAlarm": ("Escalations", "EscalationSpikeThreshold", emitter_src, "engine"), + "ShadowInvocationSpikeAlarm": ("Invocations", "InvocationSpikeThreshold", emitter_src, "engine"), + "ShadowApprovalSpikeAlarm": ("ApprovalGranted", "ApprovalSpikeThreshold", workflow_src, "workflow"), + "ShadowHighRiskApprovalAlarm": ("HighRiskApproval", "HighRiskApprovalThreshold", workflow_src, "workflow"), } - for name, (metric, threshold_param) in alarms.items(): - # The metric the alarm targets must actually be emitted by the engine. - # Match _datum() tolerating whitespace/quote style - # so a benign reformat of cloudwatch.py doesn't false-fail this. - assert re.search(rf'_datum\(\s*[\'"]{metric}[\'"]', emitter_src), \ - f"{name} targets {metric}, which cloudwatch.py does not emit" + for name, (metric, threshold_param, src, kind) in alarms.items(): + # The metric the alarm targets must actually be published by its emitter. + if kind == "engine": + # Match _datum(), tolerating whitespace/quotes. + assert re.search(rf'_datum\(\s*[\'"]{metric}[\'"]', src), \ + f"{name} targets {metric}, which cloudwatch.py does not emit" + else: + # Workflow publishes via `MetricName=,` in an aws CLI call. + # Trailing comma anchors the name so a superstring rename (e.g. + # ApprovalGrantedV2) can't satisfy an alarm still on the old name. + assert f"MetricName={metric}," in src, \ + f"{name} targets {metric}, which auto-approve.yml does not emit" alarm = doc["Resources"][name] assert alarm["Type"] == "AWS::CloudWatch::Alarm" assert alarm["Condition"] == "CreateAlarms"