fix(install): tag the secondary installer's journalctl grants NOEXEC too - #491
Conversation
Review was right on all three counts, and the first is the one that matters: scripts/install/configure_web_sudo.sh writes the same three wildcard journalctl rules as first_time_install.sh and none of them carried NOEXEC. So this PR closed the pager escape on one installer path and left it open on the other, which is close to no fix at all -- a rig configured through that script still hands out a root shell via less's "!command". The test could not have caught it, for two independent reasons. INSTALLERS did not list the file. And even listed, _grant_lines() kept the raw source line: that installer echoes its rules, so each one ends in a quote rather than the wildcard, and the trailing-* check skipped every one of them. Either alone would have hidden it. Both fixed: the file is covered, and an echoed rule is unwrapped to the sudoers line it actually emits. The selector test now covers -t ledmatrix as well. It asserted only the two -u forms, so deleting the -t rule would have passed. Verified by removing NOEXEC again from the secondary installer: four of the six tests fail, where before the suite passed with the vulnerability present. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
Warning Review limit reached
Next review available in:23 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
Up to standards ✅🟢 Issues |
Uh oh!
There was an error while loading. Please reload this page.
Why this is a new PR
This change was reviewed and written for #472, but it never reached that PR's branch — I pushed it to a local branch that was never published, so #472 merged (
fe5a3aa9) carrying only thefirst_time_install.shhalf of the fix.scripts/install/configure_web_sudo.shonmaintoday still writes the untagged grants:So this re-lands it against
main. My mistake, not a review miss.The fix
configure_web_sudo.shwrites the same three wildcardjournalctlgrants that #472 tagged infirst_time_install.sh. Each rule ends in*, so the web user controls the tail of the command line, andjournalctlpages throughlessby default — andlesswill run a shell via!command.NOPASSWD:NOEXEC:stops the granted command from executing anything of its own.All three are now
NOPASSWD:NOEXEC:, matchingfirst_time_install.sh.Why the existing suite didn't catch it
test/test_sudoers_noexec_on_pagers.pyonly walkedfirst_time_install.shandconfigure_wifi_permissions.sh.configure_web_sudo.shwas simply not inINSTALLERS, so the suite was green while three untagged wildcard rules sat in it. It is now listed.The suite also only recognised rules written literally into a heredoc — this installer
echos them instead, and an echoed rule ends in a quote, so the trailing-*check skipped it and the rule was never examined at all. Echoed rules are now unwrapped before matching.Verification
Mutation-checked — untagging the three grants fails the suite:
And a renamed installer path fails rather than silently dropping out of the walk (
test_the_installers_are_presentalready covers this — I checked before adding a duplicate):Clean:
test_sudoers_noexec_on_pagers.py+test_sudo_allowlist_covers_calls.pyall pass.