Skip to content

fix: enforce review check tool policy - #11128

Merged
DOsinga merged 1 commit into
mainfrom
jbg/security-review-tool-allowlist
Aug 11, 2026
Merged

fix: enforce review check tool policy#11128
DOsinga merged 1 commit into
mainfrom
jbg/security-review-tool-allowlist

Conversation

@jbg

@jbg jbg commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • reject goose review --no-orchestrate when any selected check declares a per-check tools policy
  • treat both nonempty and explicitly empty tool lists as policies that the legacy path cannot enforce
  • direct users to the default tool-free orchestrator and document the restriction

Security impact

The legacy single-prompt review path advertised per-check tool allowlists but delegated checks through Summon with the parent session's full developer toolset in Auto mode. Repository-controlled check content or reviewed diffs could therefore obtain capabilities outside the declared check policy. This change fails closed before constructing the full-tool session while preserving dry-run and the safe orchestrated paths.

Closes project-loupe/audit-goose#742 after post-merge verification.

Verification

  • cargo fmt --all -- --check
  • cargo test -p goose-cli commands::review::handler::tests
  • cargo build -p goose-cli
  • cargo clippy -p goose-cli --all-targets -- -D warnings
  • git diff --check

Coverage verifies unrestricted checks remain allowed and both nonempty and explicit-empty tool policies are rejected with safe rerun guidance.

This finding was discovered by Project Loupe.

@github-actions

Copy link
Copy Markdown
Contributor

Documentation preview deployed: https://pr-11128.goose-pr-previews-poc.pages.dev

@DOsinga DOsinga left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fail-closed handling looks good. The legacy path now rejects selected checks with any explicit tool policy, including an empty list, while preserving unrestricted and dry-run behavior. The tests cover the important cases, the CLI and guide document the restriction, and all applicable checks pass. Approved.

@DOsinga
DOsinga added this pull request to the merge queue Aug 11, 2026
Merged via the queue into main with commit 38debd7 Aug 11, 2026
26 checks passed
@DOsinga
DOsinga deleted the jbg/security-review-tool-allowlist branch August 11, 2026 13:38
michaelneale added a commit that referenced this pull request Aug 11, 2026
* origin/main:
  chore(release): bump version to 1.46.0 (minor) (#10920)
  fix: secure Copilot API endpoint transport (#11129)
  fix(providers): map kimi_code provider name and pass correct provider to create_request (#11130)
  fix(cli): honor provider overrides on session resume (#10810)
  fix: enforce review check tool policy (#11128)
  chore(deps-dev): bump vite from 7.3.1 to 8.2.1 in /ui (#10977)
  chore(deps): bump jsonschema from 0.46.10 to 0.49.4 (#10857)
  chore(deps): bump sigstore-verify from 0.10.0 to 0.11.0 (#10856)
  chore(deps): bump lopdf from 0.42.0 to 0.44.0 (#10854)
  chore(deps): bump ctor from 0.2.9 to 0.6.3 (#10852)
  chore(deps-dev): bump @types/node from 20.19.37 to 26.1.2 in /ui (#10979)
  chore(deps-dev): bump @electron/fuses from 1.8.0 to 2.1.3 in /ui (#10978)
  chore(deps-dev): bump electron from 41.10.3 to 43.3.0 in /ui (#10974)
  fix(permissions): match extension owners exactly (#10455)
  fix: normalize critical command patterns (#10989)
  fix: keep LiteLLM default local (#10996)
  fix: encode GCP Vertex URL path segments (#10998)
  fix: make Open Plugins installs transactional (#10999)

# Conflicts:
#	Cargo.lock
#	ui/desktop/package.json
#	ui/pnpm-lock.yaml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants