Skip to content

fix(rewrite): never rewrite sudo-prefixed commands - #3569

Closed
guyoron1 wants to merge 1 commit into
rtk-ai:developfrom
guyoron1:fix/sudo-prefix-no-rewrite
Closed

guyoron1 wants to merge 1 commit into
rtk-ai:developfrom
guyoron1:fix/sudo-prefix-no-rewrite

Conversation

@guyoron1

@guyoron1 guyoron1 commented Aug 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • The rewrite re-attached a leading sudo in front of the rewritten command (sudo iptables -L → sudo rtk iptables -L), inserting rtk into the elevation the user granted to exactly one binary. An elevated rtk then writes the SQLite history db and tee files, parses config and fires the telemetry ping as root (leaving root-owned files in user-owned data directories), and resolves the target binary itself instead of letting sudo's secure_path do it.
  • rewrite_segment_inner now returns None when the stripped env prefix contains a sudo token, so sudo-prefixed commands pass through unrewritten — the same mechanism already used for RTK_DISABLED=. Plain env / VAR=val prefixes still rewrite as before, and the suppression also covers compositions like sudo noglob git status and env FOO=1 sudo ….
  • Chose the pass-through option over rewriting to rtk sudo <tool> (no such passthrough exists today) — sudo-prefixed commands are rare and typically short-output, so the token cost of not filtering them is negligible next to running rtk's side effects as root.

Test plan

  • cargo fmt --all --check
  • cargo clippy --all-targets (no warnings)
  • cargo test — 2661 passed, 8 ignored
  • Updated/added unit tests: sudo docker ps → no rewrite, env FOO=1 sudo iptables -L → no rewrite, sudo noglob git status → no rewrite, FOO=1 noglob git status still rewrites.
  • Verified with the built binary: rtk rewrite "sudo docker ps" exits 1 (no rewrite); rtk rewrite "docker ps" and rtk rewrite "FOO=1 git status" still rewrite (exit 3).

Fixes #3412

'sudo <tool>' was rewritten to 'sudo rtk <tool>', inserting rtk into the
elevation: its SQLite history writes, tee files, config parsing and
telemetry ping all ran as root, and the target binary was resolved by rtk
inside the elevated process instead of by sudo's secure_path (rtk-ai#3412).

Commands with a sudo prefix are now passed through unrewritten. Plain
env-assignment and 'env' prefixes still rewrite as before.
@KuSh

KuSh commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator

Thank you for this, and sorry it took a while to get a decision. Your analysis of the problem is correct and well argued — the comment you wrote (sudo <tool> elevates exactly one binary; sudo rtk <tool> would run rtk's SQLite/tee/telemetry side effects as root and resolve the target outside secure_path) is precisely the reasoning we settled on, and it's now the rationale carried in the code. The env FOO=1 sudo iptables -L and sudo noglob git status cases you identified are the sharpest ones in this area, and they were genuinely not covered before you raised them.

I'm closing this in favour of #2810, which reaches the same passthrough behaviour but at a different point in the pipeline — and that difference matters for what comes next.

Why the location matters. This PR guards inside rewrite_segment_inner, after the env prefix has already been peeled, and leaves sudo\s+ in the ENV_PREFIX regex (registry.rs:61-69). #2810 instead removes sudo\s+ from ENV_PREFIX itself.

Behaviourally the two are equivalent today. They diverge on #146 (rtk sudo), which is the direction this is headed: rather than passing sudo through forever, rtk sudo docker ps will filter the inner command while keeping rtk itself outside the elevation — the child rtk runs unprivileged and hands the wrapper down as an internal transport, so none of the side effects you listed ever execute as root.

That design needs a ^sudo(\s|$) rule in rules.rs. Rules are matched after ENV_PREFIX strips the prefix, so as long as sudo\s+ stays in that regex, the leading sudo is gone before any rule is consulted and a ^sudo rule can never fire — it would be dead code. Removing it from ENV_PREFIX is the prerequisite; a guard placed downstream is not. The same applies to classify_command, which needs to see sudo as a leading word for rtk discover to categorise it.

Nothing you raised is being dropped. All four of your motivating cases — FOO=1 sudo …, env FOO=1 sudo …, bare sudo, and sudoedit … — pass through correctly under #2810 (I verified each against a build of that branch), but they were pinned by nothing there either. I've asked for exactly those assertions to be added to #2810's test_rewrite_sudo_passthrough before it merges, so the coverage your PR contributed survives the swap. sudo noglob git status is covered too, in its own test.

Two follow-ups also came out of this area, both filed: #3756 and #3757.

Closing as superseded rather than rejected — the fix is landing, just one layer up. Thanks again for the report and the diagnosis.

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

Labels

duplicate This issue or pull request already exists

Projects

None yet

2 participants