fix: panic on corrupted permission.yaml instead of silently allowing all (#7432)#7458
Conversation
…all tools When permission.yaml contains malformed YAML, the parser error was silently swallowed via unwrap_or_else(|_| HashMap::new()), replacing all permission entries (including never_allow) with an empty map. This is a fail-open security bug: denied tools become allowed with zero user notification. Now the code logs a tracing::error with the parse error details and panics, refusing to start with corrupted security configuration. This is consistent with the existing panic on read failure and is the safe default for security-critical config. Fixes #7432
|
hey @marlonbarreto-git is this the kind of thing that would be better - not sure if panic, but will at least stop it. coudl also revert to most strict but then that could be annoying too if it wasn't that before? reset things? offer to help> ideas? |
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical security vulnerability where corrupted permission.yaml files were silently ignored, causing all permission restrictions (including never_allow entries) to be wiped and replaced with an empty map. This created a fail-open scenario where previously blocked tools became allowed without user notification.
Changes:
- Replace silent error handling with explicit panic when
permission.yamlcannot be parsed - Add error logging before panic to provide diagnostic information
- Add test coverage to verify panic behavior on corrupted YAML
| use std::fs; | ||
| use std::path::{Path, PathBuf}; | ||
| use std::sync::{Arc, LazyLock, RwLock}; | ||
| use tracing; |
There was a problem hiding this comment.
Import the tracing macro directly instead of the module. Change use tracing; to use tracing::error; to match the idiomatic Rust pattern used throughout the codebase. This allows using error!() instead of tracing::error!().
| use tracing; | |
| use tracing::error; |
|
Hey @michaelneale — thanks for picking this up! The fix is a clear improvement over the silent reset. Some thoughts on the approach: Panicking is the right call here — this is a security-relevant config file. A corrupted The existing code already uses One small thought: if you wanted to be slightly more graceful in the future, the alternative would be to fall back to a deny-all default ( The test looks clean — |
|
@marlonbarreto-git yeah I think the silent failure of it muddling along could be frustrating, so better to blow up like this. I don't know how a file like that gets corrupted but not great! |
* main: (46 commits) chore(deps): bump minimatch from 10.1.1 to 10.2.3 in /evals/open-model-gym/suite (#7498) chore(deps): bump swiper from 11.2.10 to 12.1.2 in /documentation (#7368) Better network failure error & antrhopic retry (#7595) feat: make the text bar persistent and add a queue for messages (#7560) fix: outdated clippy command in goosehints (#7590) chore(deps): bump hono from 4.11.7 to 4.12.1 in /evals/open-model-gym/mcp-harness (#7417) chore(deps-dev): bump ajv from 6.12.6 to 6.14.0 in /ui/desktop (#7437) chore(deps): bump ajv from 8.17.1 to 8.18.0 in /evals/open-model-gym/mcp-harness (#7491) chore(deps): bump hono from 4.12.0 to 4.12.2 in /ui/desktop (#7515) chore(deps-dev): bump rollup from 4.57.1 to 4.59.0 in /ui/desktop (#7522) chore(deps): bump minimatch in /ui/desktop (#7572) fix: validate configure probe for streaming providers (#7564) Dockerfile: add missing build/runtime dependencies (#7546) fix(claude-code): Permission routing for smart-approve (#7501) Add base_path field to custom provider config (#7558) fix(cli): avoid debug logging by default in CLI (#7569) fix: panic on corrupted permission.yaml instead of silently allowing all (#7432) (#7458) fix(openai): handle null reasoning effort in Responses API (#7469) Allow GOOSE_NODE_DIR override in batch file (#7422) feat: add analyze platform extension with tree-sitter AST parsing (#7542) ...
Tentative fix for #7432.
When permission.yaml is corrupted/malformed, the YAML parse error was silently swallowed and an empty HashMap returned — wiping all never_allow entries. Denied tools became silently re-allowed.
Fix: panic with a clear error message pointing to the corrupted file. Consistent with existing pattern (file read already uses .expect()). Includes test.