Skip to content

Fix: address code review findings across core, tui, and tab scripts#1296

Open
Abs313a wants to merge 3 commits into
ChrisTitusTech:mainfrom
Abs313a:code-review
Open

Fix: address code review findings across core, tui, and tab scripts#1296
Abs313a wants to merge 3 commits into
ChrisTitusTech:mainfrom
Abs313a:code-review

Conversation

@Abs313a

@Abs313a Abs313a commented May 26, 2026

Copy link
Copy Markdown
Contributor

Type of Change

  • Bug fix
  • Security patch

Description

This PR bundles independent fixes found during code review. Each change addresses a specific, reproducible bug. Grouped by category:

Security

  1. core/tabs/utils/ssh.sh
    The "Add a new system" flow appended StrictHostKeyChecking no and UserKnownHostsFile=/dev/null to ~/.ssh/config for every host, silently disabling MITM protection. Both lines removed; SSH now uses default secure-by-default behavior.

  2. core/tabs/common-script.sh:checkEscalationTool
    When running as root, ESCALATION_TOOL was set to eval, so every "$ESCALATION_TOOL" cmd "$arg" invocation across all 170 tab scripts re-tokenized and re-expanded its arguments. Paths containing spaces split into multiple args; shell metacharacters were re-interpreted, opening an injection vector. Replaced with env, which runs the command in the current environment without re-parsing.

Logic

  1. core/src/inner.rs:Entry::is_supported
    The SystemDataType::FileExists precondition arm ignored the matches flag, so [[preconditions]] matches = false, data = "file_exists", values = … behaved identically to matches = true. Three sibling arms (Environment, ContainingFile, CommandExists) all compare == *matches; this arm now does the same.

  2. core/tabs/common-script.sh:checkSuperUser
    Privileged-group detection used unanchored grep, so groups like wheelchair, rooted, or sudoers were accepted as wheel/root/sudo. Switched to whole-word match against id -nG via POSIX case. Additionally, the final guard grep -q "${SUGROUP}" ran with SUGROUP unset when no super-user group was found. Because grep -q "" matches every line, unprivileged users silently passed. Added explicit [ -z "$SUGROUP" ] fail-fast.

  3. core/tabs/common-service-script.sh:startAndEnableService
    The sv (runit) branch only called enableService and never startService, contradicting the function name. The systemd branch (enable --now) and openrc branch (explicit startService) both start the service; sv now matches.

Stability - eliminating TUI panics

  1. tui/src/floating_text.rs:from_command
    .unwrap() on an Option that was None for Command::None and for LocalFile read errors crashed the TUI when previewing an unreadable file. Now renders a fallback string.

  2. tui/src/running_command.rs:kill_child
    Unconditional .take().unwrap() panicked on double-call (rapid Ctrl-C while the child was being killed but had not yet exited), and recv().unwrap() panicked when the spawn thread dropped its sender after a spawn_command failure. Both failure modes are now no-ops.

  3. tui/src/running_command.rs:screen
    pty_master.resize(...).unwrap() ran every frame and panicked mid-draw on a closed pty (child exited) or zero-dimension target (extreme terminal resize). Failures now ignored; the screen still renders from the existing buffer.

  4. tui/src/confirmation.rs:scroll_down
    self.names.len() - 1 underflowed usize and panicked if a ConfirmPrompt was ever constructed with an empty list. Replaced with saturating_sub(1).

  5. core/src/inner.rs:get_shebang
    reader.lines().next().unwrap().unwrap() panicked at startup on any script whose only bytes were #! (no newline) or whose shebang line contained invalid UTF-8, aborting the entire TUI before any UI was drawn. Now falls back to the default executable.

Hardening - installer scripts

  1. core/tabs/common-script.sh (AUR yay-bin install path)
    cd /opt && git clone … failed when /opt/yay-bin already existed; chown -R "$USER":"$USER" failed when $USER was unset (cron, su -c); relative cd yay-bin was fragile. Pre-clean /opt/yay-bin, fall back to id -un for the install user, use absolute cd /opt/yay-bin.

  2. core/tabs/applications-setup/communication-apps/discord-setup.sh
    Under #!/bin/sh -e, an apt-get install ./discord.deb failure aborted the script before the cleanup rm, leaving the deb in CWD. Added trap 'rm -f discord.deb' EXIT. Also switched curl -Lo to curl -fLo so HTTP errors fail loudly instead of writing an HTML error body as the .deb.

  3. core/tabs/applications-setup/linutil-installer.sh:install_extra
    curl 'https://raw.githubusercontent.com/…' | tee /usr/share/… lacked -f, so a 404/5xx from GitHub would write the HTML error body into /usr/share/man/man1/linutil.1 and /usr/share/applications/linutil.desktop as root. Switched both fetches to curl -fsSL.

Verification

All checks pass on the modified tree:

  • cargo check --workspace --all-targets
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo fmt --check
  • shellcheck on all modified shell scripts

Notable behavior change

common-script.sh:checkEscalationTool now sets ESCALATION_TOOL="env" instead of "eval" when running as root. For all in-repo callers this is a drop-in replacement; the difference is that env passes args through untouched while eval re-parsed them as shell input (the injection vector this fixes). Callers that intentionally relied on eval's re-parsing, for example passing "apt-get install -y firefox" as a single quoted string, will need to switch to already-tokenized args.

@Abs313a Abs313a requested a review from ChrisTitusTech as a code owner May 26, 2026 11:34
apt-get|nala)
curl -Lo discord.deb "https://discord.com/api/download?platform=linux&format=deb"
# Ensure the downloaded .deb is removed even on failure under `set -e`.
trap 'rm -f discord.deb' EXIT

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is weird... Why?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChrisTitusTech This was meant to clean up discord.deb if curl or the package install fails under set -e; otherwise the later rm is skipped.
That said, I agree it is more machinery than this script needs and trap - EXIT can wipe a previous EXIT trap if one is ever added. I can simplify it to match the surrounding install scripts..

# Conflicts:
#	core/tabs/common-script.sh
@github-actions github-actions Bot added bug rust Pull requests that update rust code script labels Jun 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug rust Pull requests that update rust code script

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants