Fix codebase review findings: installer traversal, IPC robustness, stale view paints, plugin home dirs#53
Open
Mechazawa wants to merge 6 commits into
Open
Fix codebase review findings: installer traversal, IPC robustness, stale view paints, plugin home dirs#53Mechazawa wants to merge 6 commits into
Mechazawa wants to merge 6 commits into
Conversation
A remotely fetched manifest with a name like "../../x" reached plugins_dir.join(name) unchecked, and move_into_plugins_dir remove_dir_all's the destination before the rename — so a malicious manifest could delete and write directories outside the plugins dir. The entry field had the same problem via the staging-dir write and the loader's entry_path join. Validate both in require_installer_fields: name must be a single normal path component, entry a relative path of normal components. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The accept loop read each connection inline with no timeout and propagated per-connection errors: a client that connected without sending wedged every later --open behind it, and a single ECONNRESET killed the IPC thread for the daemon's remaining lifetime. Read each connection on its own short-lived thread and log-and-drop per-connection failures instead of returning. A read timeout on the accept thread would have been simpler, but SO_RCVTIMEO hangs under load on macOS (reproducible via the full test suite). Reads are capped at 4 KiB — commands are a few bytes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
paint_view_tree ran unconditionally off the invoke_from_event_loop queue, so a render that raced an Esc landed after the frame was popped and re-showed the dead view — with the stack empty, the next Esc was a no-op and the user was stuck on stale content. A buried frame's re-render could likewise overwrite the visible frame. Gate the paint on (plugin, handle) matching the top of the view stack, mirroring the guard handle_view_close_request already does. paint_error stays unguarded: the crash banner intentionally paints after its frame already closed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
readDir and system.exec never see a shell, so "~/Applications", "~/.local/share/applications", "~/Library/PreferencePanes" and mdfind's -onlyin ~ were literal paths: user-level apps, panes and the whole macOS file search silently returned nothing. Use os.homedir(), which the runtime has exposed since the llrt migration. Also drop app-launcher's unused system.exec capability — it only uses the exec action descriptor from highbeam:actions, which needs no capability grant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Hoist the top-frame predicate into ViewStack::is_top, route IPC accept/spawn failures through LogErr::log_warn, express the name check through is_safe_relative_path, guard move_into_plugins_dir itself against traversal (it remove_dir_all's a joined path and is pub), and hoist os.homedir() in file-search to match its sibling plugins. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The earlier commit blamed a hang in SO_RCVTIMEO; the real mechanism is narrower. On macOS, setsockopt fails with EINVAL once the unix socket's peer has fully disconnected, even with data still readable in the buffer (verified standalone; Linux accepts the call). send() is write-then-close, so accept loses that race whenever the machine is busy — which is why only the loaded parallel test suite tripped it. The original error path dropped the connection on EINVAL, losing a perfectly readable command; it only looked like a hang because the diagnostic went to an unsubscribed tracing channel. Thread-per-connection stays: there is no safe place to set a read timeout under this client pattern. Doc comment now records the actual quirk. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Fixes from a full-codebase review.
name/entryreachedplugins_dir.join()unvalidated, and the destination isremove_dir_all'd before the move, so a malicious manifest could delete and write outside the plugins dir. Both fields are now validated at the installer chokepoint, with a backstop insidemove_into_plugins_diritself.--openbehind it, and a single per-connection error killed the IPC thread for the daemon's remaining lifetime. Connections now read on short-lived threads with a 4 KiB cap; failures are logged and dropped. A read timeout was the obvious alternative, but on macOSsetsockoptfails with EINVAL once the peer has disconnected (andsend()is write-then-close), verified with a standalone repro. The quirk is recorded in therun()doc comment.ViewStack::is_top).~/Applications,~/.local/share/applications,~/Library/PreferencePanesandmdfind -onlyin ~were all literal paths (nothing expands~without a shell), so user-level apps, prefpanes and macOS file search silently returned nothing. Switched toos.homedir().system.execcapability; the plugin only uses the capability-freeexecaction descriptor fromhighbeam:actions.🤖 Generated with Claude Code