fix: suppress URL launch on drag-select and add scheme confirmation#814
Open
marshallhumble wants to merge 1 commit into
Open
fix: suppress URL launch on drag-select and add scheme confirmation#814marshallhumble wants to merge 1 commit into
marshallhumble wants to merge 1 commit into
Conversation
Store press_point in Dragging::Buffer; suppress on_open_hyperlink if cursor moved more than 4px between press and release. Fixes pop-os#516. Add launch_url_scheme_is_safe() with RFC 3986 scheme validation and an allowlist. Unusual schemes (ssh://, git://, file:, etc.) show a confirmation dialog with the full URI. Refs pop-os#463.
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
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.
Store press_point in Dragging::Buffer; suppress on_open_hyperlink if cursor moved more than 4px between press and release. Fixes #516.
Add launch_url_scheme_is_safe() with RFC 3986 scheme validation and an allowlist. Unusual schemes (ssh://, git://, file:, etc.) show a confirmation dialog with the full URI.
Since I am an Application Security Engineer I wanted to do a security review on the terminal I noticed that there was the possibility for untrusted input to be executed from URI in the term. I looked at issues to see if someone already noticed it and saw #516. This fixes that and adds the security fix.
Summary of changes:
Detail of Changes:
ButtonReleased consumed state.dragging with .take() before checking whether to fire on_open_hyperlink, discarding the information that a drag was in progress. So a ctrl+drag+release over a URL was indistinguishable from a ctrl+click.
The fix stores the raw pixel press position in Dragging::Buffer as press_point: Point and compares it against the release position. If the cursor moved more than LINK_CLICK_DRAG_THRESHOLD (4px) on either axis, the release is treated as a drag and the hyperlink handler is suppressed.
Message::LaunchUrl passed URIs directly to open::that_detached with no scheme validation. The URL regex matches ssh://, git://, and file: in addition to http(s). Any process writing to the PTY can paint one of these into the buffer. ssh:// URIs can trigger ProxyCommand or LocalCommand from ~/.ssh/config; file: can target a .desktop file. Combined with the drag-select bug above, this could fire without deliberate user intent.
The new launch_url_scheme_is_safe helper validates scheme syntax per RFC 3986 and checks against an allowlist. Safe schemes open immediately. Anything else stores the URL in pending_launch_url and the dialog() method renders a COSMIC confirmation dialog showing the full URI, with Open and Cancel actions. Escape also clears a pending URL, consistent with other dialogs in the app.