Repository navigation
Link handling: external image fetch follows redirects (even https to http) and the opener gets the raw string - #233
Merged
Merged
Conversation
The system browser got the raw href of a chat link, not the parsed one, and the external image fetch followed redirects blindly, even from https to http. Add one validation (http, https and ftp, no whitespace or control characters, no embedded credentials, at most 2,048 bytes) whose normalised URL is what the opener gets, with a refused link landing on the viewer's existing failed screen, and a redirect policy for the image fetch: at most five hops, only http and https, no credentials, no https to http downgrade, and no hop from a public host to a local one. Fixes #207.
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.
Closes #207.
Two gaps in link handling:
open_in_browservalidated the link withUrl::parsebut passed the originalhreftoxdg-open,openorrundll32. A newmedia::validated_url(href)does the validation once and returns the parsed, normalised URL (lowercase scheme and host, punycode for IDN, default port dropped,\as/), and that string is what the opener gets, from every call site (chat links, the media viewer button, the credits links, the release page,OpenLink). It refuses whitespace or control characters anywhere, more than 2,048 bytes, a parse failure, any scheme other thanhttp,httpsandftp(what the oldis_openableallowed) and embedded user names or passwords. A refused chat click opens the media viewer in its existing "failed" state with the reason; a refusal at the opener is only logged, without the link.fetch_publicnow installs a custom redirect policy (media::redirect_decision): at most five hops, onlyhttpandhttps(neverftp), no credentials in the next URL, nohttpstohttpdowngrade even on the same host, and a public host may not redirect to a local one (localhost,*.localhost, or an IP literal that is loopback, private, link-local, CGNAT, unspecified or broadcast, IPv4-mapped IPv6 included; a link that is already local may stay local). The code had no private-host restriction before, so this one applies to redirects only and doesn't resolve DNS: a public name pointing at a private address isn't caught. Errors from the fetch never include the URL.\@,%20, IDN), the refusals (whitespace, controls, userinfo,javascript:,file:,data:,mailto:, custom schemes, garbage, the 2,048 boundary),ftpaccepted andftp://user:pw@hostrefused, every redirect decision (including127.1, decimal IPs, mapped IPv6 and CGNAT edges) and mock-server tests for a relative redirect, a loop and a redirect toftp. No new dependency (reqwest::Urlis theurlcrate), no new string. README and the feature matrix describe the rules.Error::source()(if it wraps it differently the text would read "error following redirect" and that test would fail);without_urland thePolicy::customsignatures also follow my reading of 0.13.5. Thehttps-to-httpand public-to-local refusals are covered by the pure tests only, since the mock server is plainhttpon loopback. On the viewer's failed screen for a refused link the "Open in browser" button does nothing, because there is no URL to open.Branch:
enhancement-issue-207