fix reproduced audit bugs#467
Open
BANANASJIM wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThis PR replaces child-process config validation with in-process validation in CLI edit/init flows, adds mapping/device config validation tightening (target resolution, macro steps, layer triggers, init-report bounds, safe mapping names), extends immutable uninstall drop-in cleanup, preserves timer-staged tap bits across mapper layer changes, and fixes touchpad emit state commit ordering. ChangesCLI Config Commands
Config Validation
Install Uninstall Cleanup
Mapper and Touchpad State
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant EditCLI as edit.zig run()
participant Editor
participant Validator as validate_mod
User->>EditCLI: padctl config edit <name>
EditCLI->>Editor: spawn editor process
Editor-->>EditCLI: process term status
EditCLI->>EditCLI: editorSucceeded(term)
alt editor exited 0
EditCLI->>Validator: validateEditedFile(path)
Validator-->>EditCLI: errors[] or empty
alt no errors
EditCLI-->>User: "Validation: OK"
else errors present
EditCLI-->>User: print file/message pairs
EditCLI-->>User: return error.ValidationFailed
end
else editor failed
EditCLI-->>User: return error.EditorFailed
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoHarden config validation and preserve staged input state across cleanup/retries
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
Code Review by Qodo
1. Config edit path traversal
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
24
to
25
| return std.fmt.allocPrint(allocator, "{s}/{s}", .{ dirs[0], filename }); | ||
| } |
There was a problem hiding this comment.
1. Config edit path traversal 🐞 Bug ⛨ Security
padctl config edit constructs the target path by concatenating the user-supplied name into <mapping_dir>/<name>.toml without enforcing the new safe-name rules, so inputs like "../escape" can resolve outside the mappings directory. This can lead to editing/validating arbitrary files (especially impactful when run with elevated privileges).
Agent Prompt
### Issue description
`src/cli/config/edit.zig` accepts a user-supplied mapping “name” and builds a filesystem path via string concatenation. Because the name is not validated as a safe mapping stem, values containing path separators or traversal segments (e.g. `../...`) can escape the mappings directory.
### Issue Context
The repo now has a shared safe-name policy (`mapping_discovery.isSafeMappingName`) used to prevent traversal and unsafe identifiers. `config edit` should follow the same policy.
### Fix Focus Areas
- src/cli/config/edit.zig[12-25]
- src/cli/config/edit.zig[52-55]
- src/config/mapping_discovery.zig[12-20]
### Suggested fix
- Treat the CLI argument as a *stem*, not a path.
- If the user passes a value ending in `.toml`, strip the suffix to get the stem.
- Reject names that are not `mapping_discovery.isSafeMappingName(stem)` (this also rejects `/`, `..`, etc.).
- Rebuild `filename` as `{stem}.toml` after validation.
- (Optional) If you intentionally want to support editing an arbitrary path, add an explicit flag (e.g. `--path`) and keep the default safe behavior for stems.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
79
to
91
| /// Find a mapping profile by name. Returns the full path or null. Caller frees. | ||
| pub fn findMapping(allocator: std.mem.Allocator, name: []const u8) !?[]const u8 { | ||
| if (!isSafeMappingName(name)) return null; | ||
| const dirs = try paths.resolveMappingConfigDirs(allocator); | ||
| defer paths.freeConfigDirs(allocator, dirs); | ||
| return findMappingFromDirs(allocator, name, dirs); | ||
| } | ||
|
|
||
| pub fn findMappingFromDirs(allocator: std.mem.Allocator, name: []const u8, dirs: []const []const u8) !?[]const u8 { | ||
| if (!isSafeMappingName(name)) return null; | ||
| const filename = try std.fmt.allocPrint(allocator, "{s}.toml", .{name}); | ||
| defer allocator.free(filename); | ||
|
|
There was a problem hiding this comment.
2. Discovery/lookup name mismatch 🐞 Bug ≡ Correctness
discoverMappingsFromDirs returns mapping profile names derived from filenames without applying isSafeMappingName, but findMapping now refuses unsafe names; code paths that discover a name then later resolve via findMapping can fail unexpectedly. In particular, supervisor chord-switch returns p.name from discovery and then fails lookup when that name is deemed unsafe.
Agent Prompt
### Issue description
`mapping_discovery.discoverMappingsFromDirs()` can return profile names that `mapping_discovery.findMapping()` will later reject (because `findMapping()` now enforces `isSafeMappingName`). This creates an internal inconsistency where discovery surfaces a mapping but later lookups (and flows built on them) fail.
### Issue Context
Supervisor chord-switch uses discovery to pick a mapping by `chord_index`, returning `p.name`, and then later resolves that name via `findMapping()`. If a filename stem contains characters outside the safe-name vocabulary (e.g. `.`), chord-switch can select it but then fail to resolve it.
### Fix Focus Areas
- src/config/mapping_discovery.zig[14-20]
- src/config/mapping_discovery.zig[47-73]
- src/config/mapping_discovery.zig[79-93]
- src/supervisor.zig[1925-1951]
- src/supervisor.zig[1070-1077]
### Suggested fix (preferred)
- Apply `isSafeMappingName()` during discovery:
- In `discoverMappingsFromDirs`, after computing `name = stem`, `continue` unless `isSafeMappingName(name)`.
- This guarantees that any discovered `.name` is resolvable via `findMapping`.
### Alternative fix
- Keep discovery as-is, but change chord-switch to return `p.path` (absolute path) instead of `p.name`, and pass that into `handleSwitch`; this avoids re-resolving by name.
### Notes
If you intentionally want to support dotted stems (e.g. `foo.v1.toml`), then you should instead relax `isSafeMappingName` to match the supported filename grammar, and ensure install-time identifier rules are updated consistently.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cli/install/phase.zig (1)
575-596: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInconsistent drop-in cleanup: system-scope still wipes custom drop-ins.
The new user-scope cleanup (Lines 593-595) correctly uses
deleteDirAbsolute, which only succeeds when the directory is empty — preserving any custom*.conffiles a user added alongsideimmutable.conf. However, the system-scope cleanup a few lines above (Lines 590-592, unchanged) still usesstd.fs.deleteTreeAbsoluteon/etc/systemd/system/padctl.service.d, which unconditionally deletes the entire directory tree including any custom drop-ins.Since this cohort's explicit goal is to preserve custom drop-ins on immutable uninstall, a system-wide immutable install (
sudo padctl install --immutable, no--user-service) will still lose custom drop-in files that the user-scope path now protects. Consider aligning the system-scope cleanup to also only remove the directory if it's empty (or only whenimmutable.confwas the sole surviving file), matching the new user-scope behavior.🛠️ Suggested fix to align system-scope behavior
const dropin_dir = try std.fmt.allocPrint(allocator, "{s}/etc/systemd/system/padctl.service.d", .{destdir}); defer allocator.free(dropin_dir); - std.fs.deleteTreeAbsolute(dropin_dir) catch {}; + std.fs.deleteDirAbsolute(dropin_dir) catch {};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/install/phase.zig` around lines 575 - 596, The immutable uninstall cleanup in `phase.zig` still wipes custom system-scoped drop-ins because the `deleteTreeAbsolute` call on `padctl.service.d` removes the entire directory tree. Update the system-scope path in the same cleanup block that handles `effective_immutable` so it matches the `user_dropin_dir` behavior: use `std.fs.deleteDirAbsolute` (or equivalent empty-directory-only removal) instead of recursively deleting the directory. Keep the existing file deletions for `immutable.conf`, and ensure the cleanup only removes the drop-in directory when it is empty so custom `*.conf` files are preserved.src/cli/config/edit.zig (1)
12-25: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winsrc/cli/config/edit.zig:12-25 — Reject raw
nameinputs here.resolveMappingPath()concatenates the caller-supplied value intodirs[0]/{filename}, andpaths.findConfig()only joins paths. A../../...name can escape the mapping dirs and makeconfig editopen/validate an arbitrary file. ReuseisSafeMappingName()here or reject path separators/...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/config/edit.zig` around lines 12 - 25, resolveMappingPath() is currently accepting raw caller-supplied names and building a path with dirs[0] and filename, which can let config edit escape the mapping directories. Add validation in resolveMappingPath() (or call isSafeMappingName() before any path resolution) to reject path separators, .. segments, and other unsafe names before using paths.findConfig() or the fallback allocPrint join. Keep the fix localized to resolveMappingPath() and the existing mapping-name validation helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/test/bugfix_regression_test.zig`:
- Around line 219-256: The regression test in the gesture/macro cleanup scenario
is not hitting the cleanup path at the promotion frame, so it may miss the
stale-bit bug. Update the test around the existing helper setup in
bugfix_regression_test.zig so the layer activation change and staged tap
promotion happen in the same apply() frame that exercises active_changed
cleanup, using the existing helpers.makeMapper, onMacroTimerExpired, and apply
flow to trigger the bug before the fix and verify the tap bit is cleared
correctly afterward.
---
Outside diff comments:
In `@src/cli/config/edit.zig`:
- Around line 12-25: resolveMappingPath() is currently accepting raw
caller-supplied names and building a path with dirs[0] and filename, which can
let config edit escape the mapping directories. Add validation in
resolveMappingPath() (or call isSafeMappingName() before any path resolution) to
reject path separators, .. segments, and other unsafe names before using
paths.findConfig() or the fallback allocPrint join. Keep the fix localized to
resolveMappingPath() and the existing mapping-name validation helper.
In `@src/cli/install/phase.zig`:
- Around line 575-596: The immutable uninstall cleanup in `phase.zig` still
wipes custom system-scoped drop-ins because the `deleteTreeAbsolute` call on
`padctl.service.d` removes the entire directory tree. Update the system-scope
path in the same cleanup block that handles `effective_immutable` so it matches
the `user_dropin_dir` behavior: use `std.fs.deleteDirAbsolute` (or equivalent
empty-directory-only removal) instead of recursively deleting the directory.
Keep the existing file deletions for `immutable.conf`, and ensure the cleanup
only removes the drop-in directory when it is empty so custom `*.conf` files are
preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bcc2a2c0-f191-474b-9981-5d4f42de2f05
📒 Files selected for processing (11)
src/cli/config/edit.zigsrc/cli/config/init.zigsrc/cli/install/phase.zigsrc/cli/install/tests.zigsrc/config/device.zigsrc/config/mapping.zigsrc/config/mapping_discovery.zigsrc/core/mapper.zigsrc/io/uinput.zigsrc/test/bugfix_regression_test.zigsrc/tools/validate.zig
💤 Files with no reviewable changes (1)
- src/core/mapper.zig
Comment on lines
+219
to
+256
| test "macro timer-staged gamepad tap survives layer active change cleanup" { | ||
| const allocator = testing.allocator; | ||
|
|
||
| var ctx = try helpers.makeMapper( | ||
| \\[[macro]] | ||
| \\name = "delayed_a" | ||
| \\steps = [ | ||
| \\ { delay = 10 }, | ||
| \\ { tap = "A" }, | ||
| \\] | ||
| \\ | ||
| \\[[layer]] | ||
| \\name = "menu" | ||
| \\trigger = "Select" | ||
| \\activation = "toggle" | ||
| \\ | ||
| \\[remap] | ||
| \\M1 = "macro:delayed_a" | ||
| , allocator); | ||
| defer ctx.deinit(); | ||
| var m = &ctx.mapper; | ||
|
|
||
| const m1_mask = helpers.btnMask(.M1); | ||
| const select_mask = helpers.btnMask(.Select); | ||
| const a_mask = helpers.btnMask(.A); | ||
|
|
||
| _ = try m.apply(.{ .buttons = m1_mask | select_mask }, 16, 0); | ||
| try testing.expectEqual(@as(usize, 1), m.active_macros.items.len); | ||
|
|
||
| _ = m.onMacroTimerExpired(11 * std.time.ns_per_ms); | ||
| try testing.expect((m.macro_timer_tap_pending & a_mask) != 0); | ||
|
|
||
| const ev = try m.apply(.{ .buttons = m1_mask }, 16, 12 * std.time.ns_per_ms); | ||
| try testing.expect((ev.gamepad.buttons & a_mask) != 0); | ||
|
|
||
| const release = try m.apply(.{ .buttons = m1_mask }, 16, 13 * std.time.ns_per_ms); | ||
| try testing.expectEqual(@as(u64, 0), release.gamepad.buttons & a_mask); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect where staged tap bits are cleared and how toggle layers report active_changed.
rg -nP 'macro_timer_tap_pending|gesture_timer_tap_pending' src/core/mapper.zig
echo '--- handleLayerActiveChanged ---'
ast-grep outline src/core/mapper.zig --match handleLayerActiveChanged --view expanded
echo '--- toggle activation / active_changed on release ---'
rg -nP 'active_changed|toggle' src/core/layer.zig -C2Repository: BANANASJIM/padctl
Length of output: 15400
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- mapper apply / staged tap promotion ---'
sed -n '360,430p' src/core/mapper.zig
echo '--- layer trigger processing around toggle semantics ---'
sed -n '80,150p' src/core/layer.zig
echo '--- regression tests around the macro/gesture cases ---'
sed -n '219,288p' src/test/bugfix_regression_test.zig
echo '--- any mutation/audit tests for staged tap clearing ---'
rg -n 'macro_timer_tap_pending|gesture_timer_tap_pending|active_changed|tap_pending' src/test src/core -C2Repository: BANANASJIM/padctl
Length of output: 29762
Rework the gesture regression to hit cleanup on the promotion frame
src/test/bugfix_regression_test.zig:258-288 The macro case already exercises the release-edge toggle and should fail pre-fix; the gesture case still toggles the layer at t=10, so the staged tap is promoted later with no concurrent active_changed and won’t catch the stale-bit clearing bug.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/bugfix_regression_test.zig` around lines 219 - 256, The regression
test in the gesture/macro cleanup scenario is not hitting the cleanup path at
the promotion frame, so it may miss the stale-bit bug. Update the test around
the existing helper setup in bugfix_regression_test.zig so the layer activation
change and staged tap promotion happen in the same apply() frame that exercises
active_changed cleanup, using the existing helpers.makeMapper,
onMacroTimerExpired, and apply flow to trigger the bug before the fix and verify
the tap bit is cleared correctly afterward.
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
1 participant
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
Verification
zig fmt --check src/ tools/git diff --check1864/1875 tests passed; 11 skippedzig-out/bin/padctltest-tsan:1874/1885 tests passed; 11 skippedSummary by CodeRabbit
New Features
Bug Fixes