Skip to content

[codex] OM-SEC-17: Allowlist update restart markers and commands - #9472

Open
AFOliveira wants to merge 37 commits into
omacom:quattrofrom
AFOliveira:codex/om-sec-17-restart-marker-allowlist
Open

[codex] OM-SEC-17: Allowlist update restart markers and commands#9472
AFOliveira wants to merge 37 commits into
omacom:quattrofrom
AFOliveira:codex/om-sec-17-restart-marker-allowlist

Conversation

@AFOliveira

@AFOliveira AFOliveira commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Writable restart markers now select only supported, zero-argument Omarchy component commands through an explicit allowlist. Commands resolve inside the validated source tree, including the matching packaged /usr/share/omarchy/bin links to /usr/bin. Unknown names, generic dispatchers and non-regular markers cannot select a command. Failed restarts keep their marker for retry; a shell marker triggers only the single normal shell refresh.

Finding: OM-SEC-17. Reported and remediated by Afonso Oliveira. The #9469 prerequisite retains Roger Piñol's and Sean Huber's credits for their update/Pacman and Stay Awake reports.

Order: #9457#9469#9467 → this PR. This branch includes those actual prerequisites through ordinary merges. It preserves their canonical startup, source-root validation, command-scoped authorization, cancellation cleanup, and corrected inhibitor identity. #9474 is a separate downstream migration change. #9473 remains closed and is not a prerequisite.

At d84c05f1e2a3746ff1bbec5ea87546a38f718313, an independent Daybreak review found no remaining blocker in this PR's four-file delta. Focused restart-security and update-phase tests, 112 CLI checks, Bash syntax and diff checks pass. Coverage includes every allowlisted target, fixed packaged/development resolution, non-regular markers, failed-marker retry, signals, startup and PATH handling, and service/reboot ordering.

Ready for review on the corrected stack. This does not establish acceptable prompt counts for the entire historical update/migration queue or validate every actual component restart on installed hardware. The previously measured 163-prompt legacy upgrade is an unresolved usability failure, not release-readiness evidence.

@AFOliveira
AFOliveira marked this pull request as draft September 1, 2026 09:49
@ErikMelton

Copy link
Copy Markdown
Member

2 Changes:

  1. The new sudo wrapper breaks option-bearing calls. default/omarchy/sudo-no-update/sudo executes:

exec /usr/bin/sudo -N -- "$@"

The -- makes subsequent arguments command arguments rather than sudo options. For example, omarchy-update-stay-awake calls sudo -v, which becomes:

/usr/bin/sudo -N -- -v

This attempts to execute a command named -v and aborts a normal terminal update under set -e. The wrapper needs to preserve supported sudo options, with regression coverage for sudo -v.

  1. The original user PATH is lost before hooks and mise run. omarchy-update saves user_path, replaces and exports PATH, and then re-executes itself through script. Because user_path is not preserved across that re-exec, the new process captures the already-sanitized path. The later PATH="$user_path" calls therefore do not restore paths such as ~/.local/bin. Post-update hooks can silently lose commands, and a user-installed mise may be skipped.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The sudo validation flow can break sleep inhibition, existing update tests bypass their stubs, and the claimed marker security regression test is absent.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Hardens update restart handling against attacker-controlled markers and reusable sudo authorization.

Changes:

  • Allowlists restart markers and resolves fixed command paths.
  • Adds non-reusable sudo and trusted-source boundaries.
  • Reorders privileged and user-controlled update phases.
File summaries
File Description
AGENTS.md Documents secure bash -p exceptions.
bin/omarchy-update Adds trust validation, sudo isolation, and sequencing.
bin/omarchy-update-restart Validates markers and separates restart phases.
default/omarchy/sudo-no-update/sudo Wraps sudo with --no-update.
test/shell.d/update-sequence-test.sh Extends update-order and boundary testing.
Review details
  • Files reviewed: 3/5 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread default/omarchy/sudo-no-update/sudo Outdated
@AFOliveira
AFOliveira marked this pull request as ready for review September 7, 2026 13:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The refresh hook can precede later cache-populating sudo calls in omarchy-reinstall-pkgs, reopening the authorization-reuse window.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 26/37 changed files
  • Comments generated: 1
  • Review effort level: Balanced

├── post-update.d/ # During `omarchy update`, after system packages and migrations
├── pre-refresh-pacman.d/ # Before `omarchy refresh pacman` re-syncs packages
├── post-update.d/ # At the end of `omarchy update`, after privileged work
├── pre-refresh-pacman.d/ # After `omarchy refresh pacman` finishes (legacy name)
@ErikMelton

Copy link
Copy Markdown
Member

LGTM, pending hook doc fix.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants