Skip to content

fix: fail closed when permissions settings file cannot be read or parsed (fixes #4282) - #4321

Closed
TechWizard9999 wants to merge 12 commits into
rtk-ai:developfrom
TechWizard9999:fix/4282-permissions-fail-closed
Closed

TechWizard9999 wants to merge 12 commits into
rtk-ai:developfrom
TechWizard9999:fix/4282-permissions-fail-closed

Conversation

@TechWizard9999

Copy link
Copy Markdown
Contributor

Summary

Fixes #4282: When a settings.json file exists but cannot be read (permission denied) or parsed (JSON syntax error), its deny/ask/allow rules are silently dropped. Commands that should be denied instead receive Default verdict (exit 3).

Changes

  1. src/hooks/permissions.rs:

    • Added ConfigError variant to PermissionVerdict
    • load_permission_rules() now returns a config_error flag when a settings file exists but cannot be read/parsed
    • Changed error messages from warning to error level for visibility
    • Only skips missing files, not unreadable/unparseable ones
  2. src/hooks/decision.rs:

    • ConfigError verdict now maps to HookDecision::Deny (fail closed) in decide_with_params
  3. src/hooks/rewrite_cmd.rs:

    • Added ConfigError => 2 to expected_exit_code test helper
    • Added test for ConfigError exit code mapping

Behavior Change

  • Before: Unreadable/unparseable settings file -> rules silently dropped -> Default verdict (exit 3)
  • After: Unreadable/unparseable settings file -> ConfigError verdict -> Deny (exit 2, fail closed)

This ensures that a broken security configuration cannot silently degrade to permissive behavior. For distinguishing hosts (Claude, Cursor), the command is now properly denied. For non-distinguishing delegates, the behavior is unchanged (exit 2 treated as passthrough).

…s empty

Only insert the newline separator between body and hint when the filtered
body is non-empty. Fixes rtk-ai#4159.
- Add ConfigError verdict for unreadable/unparseable settings files
- load_permission_rules now returns config_error flag when file exists but cannot be read/parsed
- ConfigError verdict maps to Deny (fail closed) in decide_with_params
- Error messages use 'error' level (not 'warning') so they're visible even when hooks redirect stderr
- Fixes rtk-ai#4282
- Fix load_rules_for callers to handle 4-tuple return
- Add ConfigError case to expected_exit_code in rewrite_cmd.rs
- Add test for ConfigError exit code mapping
@TechWizard9999

Copy link
Copy Markdown
Contributor Author

@KuSh — ready for review. All CI checks pass (fmt, clippy, tests on ubuntu/mac/windows, security scans). This fixes #4282 by making unreadable/unparseable settings.json files fail closed (ConfigError → Deny) instead of silently dropping deny rules.

@rtk-wshm-sync-bot

Copy link
Copy Markdown

wshm · Automated triage by AI

📊 Automated PR Analysis

🐛 Type bug-fix
🟡 Risk medium

Summary

Fixes an issue where a Claude settings.json file that exists but is unreadable or contains invalid JSON caused its permission rules to be silently dropped, resulting in a permissive Default verdict instead of enforcing the intended deny rules. The PR introduces a ConfigError verdict that maps to Deny (fail closed) whenever a settings file exists but cannot be read or parsed, and includes an unrelated small fix/tests in runner.rs's emit_guarded for hint formatting when filtered output is empty.

Review Checklist

  • Tests present
  • Breaking change
  • Docs updated

Linked issues: #4282


Analyzed automatically by wshm · This is an automated analysis, not a human review.

@TechWizard9999

Copy link
Copy Markdown
Contributor Author

Closing as duplicate of #4287 (maintainer endorsed). #4287 implements fail-closed-to-Ask with clean CI and is the endorsed path forward. The hook-warning-surfacing part of #4282 remains open for a follow-up after #4287 merges.

@KuSh

KuSh commented Sep 28, 2026

Copy link
Copy Markdown
Collaborator

Thanks for keeping the backlog clean!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Permission rules: an unreadable or unparseable settings.json drops its deny rules instead of failing closed

2 participants