fix(builtins): surface readonly errors from unset/declare/export#1553
Merged
fix(builtins): surface readonly errors from unset/declare/export#1553
Conversation
Closes the integrity-bypass gap in TM-INJ-019/020/021: all three builtins were silently skipping the operation when the target was readonly, so a caller could not distinguish a refused write from a successful one. Real bash prints "bash: <op>: <name>: readonly variable" on stderr and exits 1. Mitigates: - TM-INJ-019: unset on readonly variable - TM-INJ-020: declare X=new over readonly X=old - TM-INJ-021: export X=new over readonly X=old The interpreter's execute_unset_builtin path was also silently skipping (separate from builtins/vars.rs::Unset which already reported correctly); both paths now match. Remaining args after a refused write keep being processed, matching bash.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
bashkit | 4ae00f6 | Commit Preview URL Branch Preview URL |
May 06 2026, 04:52 AM |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the integrity-bypass gap in three threats from
specs/threat-model.md§4. All three builtins were silently skipping the operation when the target was a readonly variable, leaving the caller unable to distinguish a refused write from a success. Real bash printsbash: <op>: <name>: readonly variableon stderr and exits 1; we now match.Mitigates:
unseton readonly variabledeclare X=newoverreadonly X=oldexport X=newoverreadonly X=oldWhy
The threat model already documented the gap as
OPENwith the recommendation "Check readonly attribute in ". The check was actually present (the value never got overwritten), but the result was a silent skip —continuewithout stderr or exit-code change. A script that doesexport TOKEN=new || die "couldn't rotate"would happily proceed with the old value and never see the failure. Fixing this also matches Bash semantics, so spec-compatibility benefits.How
crates/bashkit/src/builtins/export.rs: accumulatestderrandexit_code = 1when refusing an overwrite; return them on theExecResult.crates/bashkit/src/interpreter/mod.rs:declareassignment loop: same accumulator pattern; the finalExecResultcarries the error state.execute_unset_builtin: was a separate code path frombuiltins/vars.rs::Unsetand was silently skipping both readonly variables and internal markers; now both paths report the standard message and exit 1. (builtins/vars.rs::Unsetwas already correct.)declare_continues_after_readonly_errortest guards this.specs/threat-model.md: §4 status flipped to MITIGATED with the actual mitigation text; "Open (Blackbox)" table strikes through the three rows.Tests
New tests in
crates/bashkit/tests/blackbox_security_tests.rs::finding_readonly_bypass:unset_readonly_reports_error_and_exit_1declare_readonly_reports_error_and_exit_1export_readonly_reports_error_and_exit_1declare_continues_after_readonly_error— covers the multi-arg caseThe existing tests in the same module (which already verified value preservation) continue to pass unchanged.
Verified locally:
cargo test -p bashkit --lib: 2207 pass.cargo test -p bashkit --test blackbox_security_tests: 82 pass (10 in the readonly module).cargo test -p bashkit --test threat_model_tests: 170 pass (1 pre-existing unrelated failure onmain:builtin_parser_depth::threat_jq_moderate_nesting_works).cargo test -p bashkit --test spec_tests: all green.cargo fmt --check,cargo clippy -p bashkit --all-targets -- -D warnings: clean.Test plan
unset,declare,exportdeclarestill processes good args after a readonly hitGenerated by Claude Code