Skip to content

Commit

Permalink
Disable redirects to non-HTTP URLs in release notes webview
Browse files Browse the repository at this point in the history
  • Loading branch information
kornelski committed Jan 29, 2016
1 parent 4022b01 commit 70f6929
Showing 1 changed file with 10 additions and 1 deletion.
11 changes: 10 additions & 1 deletion Sparkle/SUUpdateAlert.m
Original file line number Diff line number Diff line change
Expand Up @@ -236,8 +236,17 @@ - (void)webView:(WebView *)sender didFinishLoadForFrame:frame

- (void)webView:(WebView *)__unused sender decidePolicyForNavigationAction:(NSDictionary *)__unused actionInformation request:(NSURLRequest *)request frame:(WebFrame *)__unused frame decisionListener:(id<WebPolicyDecisionListener>)listener
{
NSURL *requestURL = request.URL;
NSString *scheme = requestURL.scheme;
BOOL whitelistedSafe = [@"http" isEqualToString:scheme] || [@"https" isEqualToString:scheme] || [@"about:blank" isEqualToString:requestURL.absoluteString];

// Do not allow redirects to dangerous protocols such as file://
if (!whitelistedSafe) {
[listener ignore];
return;
}

if (self.webViewFinishedLoading) {
NSURL *requestURL = request.URL;
if (requestURL) {
[[NSWorkspace sharedWorkspace] openURL:requestURL];
}
Expand Down

5 comments on commit 70f6929

@chris0e3
Copy link

Choose a reason for hiding this comment

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

So this breaks any mailto:, feed: or data: links on the page. Is that good?

@kornelski
Copy link
Member Author

Choose a reason for hiding this comment

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

We can't pass data: to sharedWorkspace. I don't think it'd work, but if it worked, it'd be unsafe, since data: URLs can contain executables.

We should whitelist mailto:.

I'm not sure about feed:. Is it still supported? Can we trust it to be safe?

@chris0e3
Copy link

Choose a reason for hiding this comment

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

Perhaps data: should still call [listener use]. (See https://en.wikipedia.org/wiki/Data_URI_scheme).
The RSS new reader Vienna uses feed:.

What’s dangerous other than file:?

@jakepetroules
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also perhaps be configurable. Some people might want the original behavior if their appcast is delivered over SSL.

Don't forget custom protocols which may be of use as well.

@kornelski
Copy link
Member Author

Choose a reason for hiding this comment

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

What’s dangerous other than file:?

I assume everything, until proven innocent. Even browsers ask before opening unknown schemes, and — as the bug has taught us — openURL is more powerful and trusted than opening the same URL in the browser.

Please sign in to comment.