-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Description
View implementation branch
approval module code
Contact Page
Proposal: Adopt 'Approval' Module for Command Safety Assessment
1. Summary
This document proposes the formal adoption of the new approval module as the standard for all command safety and security assessments within the codex-rs core. This module is designed to be a robust, flexible, and maintainable solution that learns from our two previous approaches (execpolicy and the safety module). It provides a clean, data-driven, and extensible framework for classifying command intent, marking a significant evolution in our security architecture.
2. Motivation and Vision
The design decision to sandbox the .git folder raised concerns about robust command evaluation while meeting functional expectations, as indicated in openai/codex#5034: Unable to use git even with workspace-write.
A robust security model provides granular policies for operations, but the existing framework for command safety assessment lacks the necessary flexibility. Further developing this safety module is too complex and brittle, so a new approach is needed.
The approval module introduces that flexibility by classifying various operations based on data-driven rules. These rules currently map to legacy policies defined in SafetyCheck, but in the future, the application might support more granular policies such that VCS operations can run in their own sandboxes. This is, of course, conditional on robust classification rules. The rules implemented in the approval module are intended to reflect the policies of the existing safety module for now, while permitting policy extensions for narrowly scoped rules in the future.
3. Background
To date, two distinct approaches to command safety appear in the VCS history: execpolicy and the safety module.
The execpolicy Approach
execpolicy represents a formal and rigorous security model that treats security policy as data, parsed from a Starlark file.
- Positives: It enforces a strict, schema-driven validation for commands and their arguments. Its separation of policy (Starlark) from mechanism (Rust) is architecturally clean.
- Limitations: Its design focuses heavily on client-side security, attempting to validate file system paths against whitelists. This is largely outside the scope of concern for our application, which should focus on ensuring that agent-initiated actions are safe, rather than enforcing user-side filesystem permissions. Its rigid schema-based design also does not naturally fit the dynamic nature of shell commands.
The safety Module Approach
The safety module, whose commit history indicates it was chosen for its simplicity, represents a more pragmatic approach to security assessment.
- Positives: It introduced the clean
SafetyCheckenum (AutoApprove,AskUser,Reject) and provided a straightforward way to assess commands using internal Rust logic. This allowed for rapid development and iteration. - Limitations: This simplicity presents several challenges: The logic for classifying a command is tightly coupled with the logic for evaluating user policies and platform capabilities, making it difficult to maintain and extend.
The lessons learned from these two distinct approaches (the overly formal, client-focused nature of execpolicy and the maintainability challenges of the simpler safety module) have directly informed the design of a third, more balanced solution.
4. A Comparative Analysis
| Feature | safety Module |
execpolicy Module |
approval Module (Proposed) |
|---|---|---|---|
| Primary Focus | Simple, direct command validation. | Client-side security via path validation. | Application-level safety via command intent classification. |
| Architecture | Monolithic function with tangled logic. | Starlark policy engine; schema-based. | Data-driven Rust rules engine with a clean Separation of Concerns (SoC). |
| Performance | Simple lookups. | Policy parsing overhead. | Efficient O(1) primary command lookup. |
| Scope | Single commands. | Single commands; design doesn't natively handle shell syntax. | Understands single commands plus word-only pipelines (no redirection or subshells)/short-circuit chains; rejects complex shell features (redirection, subshells, expansions). |
| Flexibility | Brittle; simple allow/deny lists. | Rigid; fails on unknown commands. | Flexible; classifies knowns, with safe fallback (AskUser) for unknowns. |
| Maintainability | Low; interwoven logic is hard to test. | Medium; requires maintaining separate .policy files. |
High; rules are expressive, testable Rust code. |
5. The 'Approval' Module: A Turnkey Middle Ground
The approval module was designed from the ground up to be a pragmatic, powerful, and turnkey solution that finds the middle ground between the previous two approaches.
- Correct Focus: It concentrates on application-level safety by classifying a command's intent (e.g.,
DeletesData,ReadsVcs), which is the correct scope of concern for our tool. - High Performance and Structure: It replaces simple list matching with a robust, AST-based analysis backed by a lazily-built rule index that keeps primary command lookups at O(1) and exposes subcommand knowledge to the parser.
- Git-Aware Semantics:
gitinvocations are parsed into a dedicated semantic model, allowing rule authors to reason about rich operations (e.g.,reset --hard) rather than brittle token slices. - Consistent argv Normalization: The parser strips repeated
sudo, resolves basename tools, isolates flag clusters, and promotes the first non-flag token to a subcommand when appropriate. This ensures that every rule operates on a predictableSimpleAst. - Risk Aggregation for Pipelines: When multiple simple commands are joined, the classifier aggregates their categories and escalates decisions based on the highest-risk member, preventing lower-risk steps from masking destructive ones.
- Familiar Shell Unwrapping, Broader Coverage: When a command matches the
<shell> -c|-lc "<script>"pattern, the new parser mirrors the legacy unwrapping behavior but adds a heuristic that accepts any*shbinary instead of onlybash -lc, while still rejecting scripts that use disallowed constructs. - A Drop-in Decision Engine: The module keeps the pure
SafetyCheckdecision API that callers already rely on, while layering clearerCommandDecision::permit/require_approval/denyhelpers and the AST-based classifier to make integration and future rule additions safer and more maintainable. - Extensible without Bloat: New command rules can be added directly, without extending the limited Domain-Specific Language of Starlark. It can match based on simple rules and accepts function pointers in order to allow parsing of complex predicates. It leverages the existing
tree-sitterdependency for shell parsing, adding no new external dependencies. - Patch Assessment Still Pending: The groundwork for patch safety lives alongside the command pipeline, but the actual rule engine (
assess_patch_safety) currently returns a placeholder decision and will be tackled in follow-on work.
6. Potential concerns
-
Transitioning from the legacy filter:
- The existing implementation grants unconditional trust to a hard-coded allowlist that leads to gaps. The
approvalmodule inherits the existing regression tests and extends them with richer parser and predicate suites to verify correctness.
- The existing implementation grants unconditional trust to a hard-coded allowlist that leads to gaps. The
-
"The
approvalmodule's shell parser adds performance overhead."- The overhead of parsing is a direct and necessary cost for the critical feature of understanding shell commands. While other systems could theoretically be extended to do this, the
approvalmodule is designed around this capability from the ground up. Profiling reveals decisions are made in microseconds, and because the primary command classification is a highly efficient O(1) lookup, it can be extended indefinitely without affecting performance.
- The overhead of parsing is a direct and necessary cost for the critical feature of understanding shell commands. While other systems could theoretically be extended to do this, the
-
"Isn't
execpolicy's strict file-path validation more secure?"- Execpolicy is very good at what it does, but its expectations about a user's environment are very specific and brittle. Variations in filesystems and distros may use different canonical paths, which would need to be whitelisted or modified to suit various use cases. it is also unaware of the modules within core, so it would require a substantial redesign to integrate with existing code. Again, the
approvalmodule seeks to strike a balance between the haphazard implementation that exists now and the need for flexibility in the future.
- Execpolicy is very good at what it does, but its expectations about a user's environment are very specific and brittle. Variations in filesystems and distros may use different canonical paths, which would need to be whitelisted or modified to suit various use cases. it is also unaware of the modules within core, so it would require a substantial redesign to integrate with existing code. Again, the
7. Future Opportunities
The current implementation conservatively denies an entire joined command sequence when any member of that sequence is deemed unsafe. A follow-on refinement could allow sequential execution incrementally, stopping at the first command that requires user input or is rejected. Streaming approvals would cut down on agent round-trips, reduce redundant reasoning, and lower the interactive workload while preserving safety guarantees.
8. Conclusion
The approval module represents a significant step forward in the maturity and robustness of the runtime security model. It builds on prior approaches, combining the rigor of a rules-based system with the flexibility required for a dynamic, AI-driven environment. It is more secure, more maintainable, and better aligned with the project's long-term needs.