Bug 64580

Created attachment 100911 [details] first cut for review

Attachment 100911 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/html/HTMLAnchorElement.cpp'..." exit_code: 1 Source/WebCore/html/HTMLAnchorElement.h:118: The parameter name "event" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/loader/FrameLoader.h:111: Use 0 instead of NULL. [readability/null] [5] Source/WebCore/loader/FrameLoader.h:348: The parameter name "sourceRequest" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/loader/FrameLoader.h:348: Use 0 instead of NULL. [readability/null] [5] Source/WebCore/loader/FrameLoader.cpp:268: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 5 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 100911 [details] first cut for review View in context: https://bugs.webkit.org/attachment.cgi?id=100911&action=review Some notes below. You'll also need to add this to HTMLAnchorElement.idl so that it can be feature-detected. > Source/WebCore/html/HTMLAnchorElement.cpp:500 > + String url = stripLeadingAndTrailingHTMLSpaces(getAttribute(hrefAttr)); getAttribute -> fastGetAttibute, I believe. > Source/WebCore/html/HTMLAnchorElement.cpp:503 > + bool download = hasAttribute("download"); "download" should be added to the appropriate HTMLNames.in file, alongside the other attributes. > Source/WebCore/html/HTMLAnchorElement.cpp:504 > + String name = download ? getAttribute("download") : ""; We're using "" as distinct from String() here? Also probably should be fastGetAttribute. > Source/WebCore/loader/FrameLoader.cpp:264 > +void FrameLoader::urlSelected(const KURL& url, const String& passedTarget, PassRefPtr<Event> triggeringEvent, bool lockHistory, bool lockBackForwardList, ReferrerPolicy referrerPolicy, bool shouldDownload, const String* downloadName) const String& rather than const String* > Source/WebCore/platform/network/ResourceRequestBase.h:159 > + bool shouldDownload() const { return m_shouldDownload; } > + void setShouldDownload(bool should) { m_shouldDownload = should; } > + > + String downloadName() const { return m_downloadName; } > + void setDownloadName(String name) { m_downloadName = name; } You can't add things to ResourceRequestBase that don't round-trip through NSURLRequest (or whatever that type is called).

Created attachment 100918 [details] updated Addressed the comments. Regarding ResourceRequestBase: are there additional places I need to change to make this acceptable, or is this not the right place to make the change at all? In the latter case, perhaps this could go in FrameLoadRequest instead?

Attachment 100918 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/html/HTMLAnchorElement.cpp'..." exit_code: 1 Source/WebCore/html/HTMLAnchorElement.h:118: The parameter name "event" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/loader/FrameLoader.h:348: The parameter name "sourceRequest" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/loader/FrameLoader.h:348: Use 0 instead of NULL. [readability/null] [5] Source/WebCore/loader/FrameLoader.cpp:268: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 4 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #4) > is this not the right place to make the change at all? I think it's not. > In the latter case, perhaps this could go in FrameLoadRequest instead? I think that would be better. Not sure.

(In reply to comment #6) > (In reply to comment #4) > > is this not the right place to make the change at all? > > I think it's not. > > > In the latter case, perhaps this could go in FrameLoadRequest instead? > > I think that would be better. Not sure. Just to clarify: does the restriction on ResourceRequestBase apply to ResourceRequest too? :)

ResourceRequest is the class. ResourceRequestBase is just an implementation detail. I don’t think that policy about downloading belongs in ResourceRequest. That class is only about the specifications needed when loading a resource. FrameLoadRequest is probably an OK place for this since it’s where we handle frame targeting, another feature of the anchor element. Longer term it would be nice to go more directly to the downloading machinery and not treat this as a frame load at all. It’s not a frame load. For example, I don’t think HTMLAnchorElement should necessarily even call urlSelected in the download case.

Created attachment 100933 [details] directly call startDownload I have made the change to call startDownload early directly from HTMLAnchorElement instead of going through urlSelected. Some notes: (1) I have tried to make sure that the ResourceRequest is properly updated with the referrer, user-agent etc. by adding FrameLoader::prepareResourceRequest (2) Added an optional 'String& suggestedName' parameter to WebCore::FrameLoaderClient::startDownload. It's currently unused in all implementations, but if this looks reasonable, I will hook it up at least for chromium. I really appreciate the quick reviews. Thanks!

Attachment 100933 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/html/HTMLAnchorElement.cpp'..." exit_code: 1 Source/WebCore/html/HTMLAnchorElement.h:118: Extra space before ) [whitespace/parens] [2] Source/WebCore/html/HTMLAnchorElement.cpp:506: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebCore/html/HTMLAnchorElement.cpp:511: An else should appear on the same line as the preceding } [whitespace/newline] [4] Total errors found: 3 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 100933 [details] directly call startDownload Attachment 100933 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9097139

Where does this feature come from? I can't find it in HTML5 spec.

(In reply to comment #12) > Where does this feature come from? I can't find it in HTML5 spec. This is being discussed here: http://www.mail-archive.com/whatwg@lists.whatwg.org/msg27416.html

Comment on attachment 100933 [details] directly call startDownload Thank you! New features need to be discussed on webkit-dev mailing list prior to being added to WebKit: <http://www.webkit.org/coding/adding-features.html>. It may be good to wait until a discussion on WhatWG list settles down first. r- since this can't be landed unless there is a wide agreement that this is a good feature addition.

Comment on attachment 100933 [details] directly call startDownload View in context: https://bugs.webkit.org/attachment.cgi?id=100933&action=review > Source/WebCore/html/HTMLAnchorElement.idl:29 > + attribute [Reflect] DOMString download; ap: we are working on reaching consensus on how best to reflect this option. it is clearly something everyone wants, but we just have to find the right form for it to take. we may want to vendor prefix this as well just as was done for <input type=file webkitdirectory>: <a webkitdownload=filename>. of course, that depends on the outcome of the working group thread.

It looks like @download=filename now has the support of Mozilla: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-July/032490.html

Created attachment 101085 [details] Check origin before downloading As per the discussion, I have made the change to check the origin first before allowing download. With this change, blob URLs, filesystem URLs and URLs for the same origin can be downloaded. Otherwise, the 'download' attribute is silently ignored. Or would it be more desirable to not follow the link at all and show an error to the user instead?

Comment on attachment 101085 [details] Check origin before downloading View in context: https://bugs.webkit.org/attachment.cgi?id=101085&action=review > Source/WebCore/loader/FrameLoader.cpp:1237 > + String referrer = request.httpReferrer(); > + if (referrer.isEmpty()) > + referrer = m_outgoingReferrer; > + if (SecurityOrigin::shouldHideReferrer(request.url(), referrer) || referrerPolicy == NoReferrer) > + referrer = String(); Is this code duplicated from somewhere else in FrameLoader? It would be nice not to duplicate.

Obviously, this patch is missing tests.

(In reply to comment #18) > (From update of attachment 101085 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101085&action=review > > > Source/WebCore/loader/FrameLoader.cpp:1237 > > + String referrer = request.httpReferrer(); > > + if (referrer.isEmpty()) > > + referrer = m_outgoingReferrer; > > + if (SecurityOrigin::shouldHideReferrer(request.url(), referrer) || referrerPolicy == NoReferrer) > > + referrer = String(); > > Is this code duplicated from somewhere else in FrameLoader? It would be nice not to duplicate. It's a slightly modified piece of code from FrameLoader::loadURL and FrameLoader::urlSelected. But it doesn't look easily possible to refactor in a way that it can be used from loadURL/urlSelected too.

(In reply to comment #19) > Obviously, this patch is missing tests. Indeed. I wanted to make sure I have put the pieces in the right places before working on the tests and updating the ChangeLog files :-)

Created attachment 101090 [details] Always allow data URLs Made change to always allow data URLs.

Comment on attachment 101090 [details] Always allow data URLs View in context: https://bugs.webkit.org/attachment.cgi?id=101090&action=review > Source/WebCore/html/HTMLAnchorElement.cpp:506 > + if (download && (kurl.protocolIsData() || document()->securityOrigin()->canRequest(kurl))) { Please don't add special cases for data URLs. The data URL bug is part of a larger problem in WebKit.

Created attachment 101111 [details] Add test, remove special case for data URLs, ChangeLog I have removed the special casing for data URLs. I have also added a couple of tests.

Comment on attachment 101111 [details] Add test, remove special case for data URLs, ChangeLog View in context: https://bugs.webkit.org/attachment.cgi?id=101111&action=review R-, mostly for the Origin header / FrameLoader question. Please feel free to renominate if I'm not understanding things properly. Thanks for adding tests in this iteration! > Source/WebCore/html/HTMLAnchorElement.idl:29 > + attribute [Reflect] DOMString download; Doe this API need to be conditional on something? Do all the ports understand how to use the suggested file name? From reading the code, it seems like this feature might be half-implemented on most ports because it will trigger the download but not suggest the file name. Maybe I'm misunderstanding? > Source/WebCore/loader/FrameLoader.cpp:1243 > + if (!referrer.isEmpty()) { > + request.setHTTPReferrer(referrer); > + RefPtr<SecurityOrigin> referrerOrigin = SecurityOrigin::createFromString(referrer); > + addHTTPOriginIfNeeded(request, referrerOrigin->toString()); > + } else Why are we adding an Origin header to GET requests? Normally we don't do that unless we're using CORS. This request doesn't seem to be a CORS request, however, because it's not using the CORS functions for preparing the request (which do things like remove authentication information). > Source/WebCore/loader/FrameLoader.h:275 > + void prepareResourceRequest(ResourceRequest&, ReferrerPolicy); This function name is very confusing. Should we call this function all the time when sending a request, or only in some circumstances? With a name like "prepareResourceRequest" it sounds like we should call this all the time because we always want to prepare resource requests before issuing them. > Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:1329 > -void FrameLoaderClient::startDownload(const ResourceRequest& request) > +void FrameLoaderClient::startDownload(const ResourceRequest& request, const String& suggestedName) For example, on GTK, this looks like this function triggers the download but ignores the suggestedName.

I haven't been following the whatwg thread in much detail, but are we supposed to use CORS when issuing these requests? If so, instead of adding the Origin header manually, we can use these functions: http://trac.webkit.org/browser/trunk/Source/WebCore/loader/CrossOriginAccessControl.h Specifically, updateRequestForAccessControl understands how to do this work correctly.

Comment on attachment 101111 [details] Add test, remove special case for data URLs, ChangeLog View in context: https://bugs.webkit.org/attachment.cgi?id=101111&action=review >> Source/WebCore/loader/FrameLoader.cpp:1243 >> + if (!referrer.isEmpty()) { >> + request.setHTTPReferrer(referrer); >> + RefPtr<SecurityOrigin> referrerOrigin = SecurityOrigin::createFromString(referrer); >> + addHTTPOriginIfNeeded(request, referrerOrigin->toString()); >> + } else > > Why are we adding an Origin header to GET requests? Normally we don't do that unless we're using CORS. This request doesn't seem to be a CORS request, however, because it's not using the CORS functions for preparing the request (which do things like remove authentication information). The more I think about this code, the more I realize that it's not correct. For example, if the document is sandboxed (e.g., with the HTML5 sandbox attribute), this code will give use a non-"null" Origin header, which is wrong because the document is in a unique origin. Instead, assuming we want to add an Origin header, we should generate the header from the document's SecurityOrigin object instead of creating a fake one from the Referer. That will do the right thing in all cases, and is what the updateRequestForAccessControl function does.

See also this thread: <http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-July/027455.html>. The WhatWG discussion has not yet reached the point of reconciling these proposals. Honestly, I don't know if CORS should have anything to do with @download. Clearly, there are security implications, but CORS is not an answer to every security problem on the Web. There are two new capabilities given by the proposed attribute - force downloading a resource, and rename it in the process. Neither is what a server operator would think about when opening a resource for cross origin use.

Comment on attachment 101111 [details] Add test, remove special case for data URLs, ChangeLog View in context: https://bugs.webkit.org/attachment.cgi?id=101111&action=review >> Source/WebCore/html/HTMLAnchorElement.idl:29 >> + attribute [Reflect] DOMString download; > > Doe this API need to be conditional on something? Do all the ports understand how to use the suggested file name? From reading the code, it seems like this feature might be half-implemented on most ports because it will trigger the download but not suggest the file name. Maybe I'm misunderstanding? Nope, you are correct. The suggested filename is not used in any of the ports yet. I will be submitting another CL to use this name in chromium (once it is updated to handle the suggested name). Should this be conditional on PLATFORM(CHROMIUM) in that case? >>> Source/WebCore/loader/FrameLoader.cpp:1243 >>> + } else >> >> Why are we adding an Origin header to GET requests? Normally we don't do that unless we're using CORS. This request doesn't seem to be a CORS request, however, because it's not using the CORS functions for preparing the request (which do things like remove authentication information). > > The more I think about this code, the more I realize that it's not correct. For example, if the document is sandboxed (e.g., with the HTML5 sandbox attribute), this code will give use a non-"null" Origin header, which is wrong because the document is in a unique origin. Instead, assuming we want to add an Origin header, we should generate the header from the document's SecurityOrigin object instead of creating a fake one from the Referer. That will do the right thing in all cases, and is what the updateRequestForAccessControl function does. Ah, I see. I am going to remove the Origin header since this is, indeed, not a CORS request. (I wasn't aware of the implications you point out. Thanks) >> Source/WebCore/loader/FrameLoader.h:275 >> + void prepareResourceRequest(ResourceRequest&, ReferrerPolicy); > > This function name is very confusing. Should we call this function all the time when sending a request, or only in some circumstances? With a name like "prepareResourceRequest" it sounds like we should call this all the time because we always want to prepare resource requests before issuing them. I can rename it (e.g. prepareResourceRequestForAnchorDownload) to make it more obvious what it does, or get rid of the function altogether and update the ResourceRequest in HTMLAnchorElement::handleClick. WDYS?

> Should this be conditional on PLATFORM(CHROMIUM) in that case? Maybe add an ENABLE macro? It seems somewhat silly to have an entire ENABLE macro for such a trivial feature. There isn't any necessary connection between PLATFORM(CHROMIUM) and the presence of this API, so using that macro seems inappropriate. > I can rename it (e.g. prepareResourceRequestForAnchorDownload) to make it more obvious what it does, or get rid of the function altogether and update the ResourceRequest in HTMLAnchorElement::handleClick. WDYS? I'd rather the preparation of the resource request happened naturally in the course of FrameLoader handling the request. It's not entirely clear to me why a ResourceRequest for an anchor download is a special case for FrameLoader. How do the other folks who call startDownload handle this issue? These are all detail points, however. It's important that you also address Alexey's comments in Comment #28.

> > I can rename it (e.g. prepareResourceRequestForAnchorDownload) to make it more obvious what it does, or get rid of the function altogether and update the ResourceRequest in HTMLAnchorElement::handleClick. WDYS? > > I'd rather the preparation of the resource request happened naturally in the course of FrameLoader handling the request. It's not entirely clear to me why a ResourceRequest for an anchor download is a special case for FrameLoader. How do the other folks who call startDownload handle this issue? > Darin A suggested in https://bugs.webkit.org/show_bug.cgi?id=64580#c8 that perhaps HTMLAnchorElement could start the download directly without having to go through FrameLoader. I wanted to try that out and see if that is doable now cleanly. But perhaps a larger refactoring would be necessary for that, and for now it's best to go through FrameLoader?

(In reply to comment #28) > See also this thread: <http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-July/027455.html>. The WhatWG discussion has not yet reached the point of reconciling these proposals. > > Honestly, I don't know if CORS should have anything to do with @download. Clearly, there are security implications, but CORS is not an answer to every security problem on the Web. There are two new capabilities given by the proposed attribute - force downloading a resource, and rename it in the process. Neither is what a server operator would think about when opening a resource for cross origin use. Indeed, the discussion around cross-origin URLs/CORS continues, and doesn't seem to have converged yet. This CL currently ignores 'download' for cross-origin URLs entirely. Perhaps it should remain like this until there is an agreement on how to deal with them?

> Perhaps it should remain like this until there is an agreement on how to deal with them? Unfortunately, I don't think the security check you've added in this patch deals correctly with redirects. How does the download system know not to follow cross-origin redirects?

(In reply to comment #33) > > Perhaps it should remain like this until there is an agreement on how to deal with them? > > Unfortunately, I don't think the security check you've added in this patch deals correctly with redirects. How does the download system know not to follow cross-origin redirects? Ah, yes. You are correct. Cross-origin redirects will cause a problem. You had brought this up before, sorry I missed it.

Looks like this is specified now in HTML: http://www.whatwg.org/specs/web-apps/current-work/#attr-hyperlink-download

Created attachment 101736 [details] updated I have updated the patch to remove the check for SecurityOrigin. I have removed FrameLoader::prepareResourceForRequest, so now the Request is updated directly in HTMLAnchorElement before starting the download.

Comment on attachment 101736 [details] updated View in context: https://bugs.webkit.org/attachment.cgi?id=101736&action=review I like the new approach of not change ResourceRequestBase. Two comments below. > Source/WebCore/html/HTMLAnchorElement.cpp:512 > + if (!referrer.isEmpty()) > + request.setHTTPReferrer(referrer); Don't we need to call shouldHideReferrer? > Source/WebCore/html/HTMLAnchorElement.idl:29 > + attribute [Reflect] DOMString download; Looks like we still have the problem of this feature being half-implemented on non-Chromium ports. We either need to fully implementing it or have it be invisible. I'm not sure we can fully implement it without changing the embedders on those platforms, so we'll probably need to disable it.

Created attachment 101739 [details] shouldHideReferrer Indeed. Updated to use shouldHideReferrer.

Comment on attachment 101739 [details] shouldHideReferrer View in context: https://bugs.webkit.org/attachment.cgi?id=101739&action=review > Source/WebCore/html/HTMLAnchorElement.idl:29 > + attribute [Reflect] DOMString download; We still have the feature-detection / half-implementation issue.

(In reply to comment #39) > (From update of attachment 101739 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101739&action=review > > > Source/WebCore/html/HTMLAnchorElement.idl:29 > > + attribute [Reflect] DOMString download; > > We still have the feature-detection / half-implementation issue. Indeed. How do you suggest we go about this? As you mentioned earlier, using a PLATFORM check or an ENABLE flag both seem ... kind of unreasonable for something like this.

> Indeed. How do you suggest we go about this? As you mentioned earlier, using a PLATFORM check or an ENABLE flag both seem ... kind of unreasonable for something like this. I think the ENABLE macro is the lesser of two evils here. My thinking process is the following: 1) We can't implement the entire feature in WebKit, so we'll need to be able to turn it on for some platforms but not others. 2) JSC doesn't support EnableAtRuntime yet, so we need a compile-time option in the IDL. 3) This feature isn't specific to the Chromium platform. Chromium is just the first platform to implement the feature. That means PLATFORM(CHROMIUM) isn't right. That leaves an ENABLE macro. The only problem with using an ENABLE macro is that the feature is small, but that's not really a big problem.

ENABLE_DOWNLOAD_ATTRIBUTE

Created attachment 101754 [details] ENABLE Added ENABLE_DOWNLOAD_ATTRIBUTE check, and turned it on for chromium.

Comment on attachment 101754 [details] ENABLE View in context: https://bugs.webkit.org/attachment.cgi?id=101754&action=review > Source/WebCore/html/HTMLAnchorElement.cpp:505 > +#if defined(ENABLE_DOWNLOAD_ATTRIBUTE) #if ENABLE(DOWNLOAD_ATTRIBUTE) > Source/WebCore/html/HTMLAnchorElement.idl:59 > +#if defined(ENABLE_DOWNLOAD_ATTRIBUTE) #if ENABLE(DOWNLOAD_ATTRIBUTE)

Created attachment 101763 [details] proper use of ENABLE macro, and ChangeLog files Used the proper ENABLE macro, and updated the ChangeLog files accordingly.

Comment on attachment 101754 [details] ENABLE View in context: https://bugs.webkit.org/attachment.cgi?id=101754&action=review > Source/WebCore/html/HTMLAnchorElement.cpp:507 > + bool download = hasAttribute(downloadAttr); > + if (download) { Why not "if (hasAttribute(downloadAttr))"? > Source/WebCore/html/HTMLAnchorElement.cpp:515 > + if (!hasRel(RelationNoReferrer)) { > + String referrer = frame->loader()->outgoingReferrer(); > + if (!referrer.isEmpty() && !SecurityOrigin::shouldHideReferrer(kurl, referrer)) > + request.setHTTPReferrer(referrer); > + frame->loader()->addExtraFieldsToMainResourceRequest(request); > + } It's pretty horrible to duplicate this code. > Source/WebCore/html/HTMLAnchorElement.idl:60 > +#if defined(ENABLE_DOWNLOAD_ATTRIBUTE) > + attribute [Reflect] DOMString download; Don't we have Conditional for attributes now?

Comment on attachment 101763 [details] proper use of ENABLE macro, and ChangeLog files View in context: https://bugs.webkit.org/attachment.cgi?id=101763&action=review There are no tests for programmatic setting and unsetting of download here. > Source/WebCore/loader/FrameLoaderClient.h:182 > + virtual void startDownload(const ResourceRequest&, const String& = String()) = 0; This argument definitely needs a name.

Comment on attachment 101763 [details] proper use of ENABLE macro, and ChangeLog files View in context: https://bugs.webkit.org/attachment.cgi?id=101763&action=review > LayoutTests/fast/dom/HTMLAnchorElement/anchor-nodownload.html:24 > + var evt = document.createEvent("MouseEvent"); > + evt.initMouseEvent('click', true, true); > + link.dispatchEvent(evt); Ouch. I think that programmatically created mouse events should not be able to start download. > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:795 > +void WebFrameLoaderClient::startDownload(const ResourceRequest& request, const String& suggestedName) > { > m_frame->startDownload(request); > } How doesn't this unused argument no break the build? Ditto elsewhere. > Source/WebKit2/WebProcess/WebPage/WebFrame.h:73 > + void startDownload(const WebCore::ResourceRequest&, const String& = String()); I guess it's not a big deal to modify to WebFrame, but it doesn't seem necessary.

(In reply to comment #48) > (From update of attachment 101763 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101763&action=review > > > LayoutTests/fast/dom/HTMLAnchorElement/anchor-nodownload.html:24 > > + var evt = document.createEvent("MouseEvent"); > > + evt.initMouseEvent('click', true, true); > > + link.dispatchEvent(evt); > > Ouch. I think that programmatically created mouse events should not be able to start download. You can programmatically navigate to an URL that may be configured to return a Content-Disposition header. Why is this any different? What are you worried about?

I'm worried about Safari carpet bombing (e.g. <http://blogs.pcmag.com/securitywatch/2008/05/safari_carpet_bombing.php>). If my reading is correct, HTML5 says that synthetic events shouldn't work with links: ---------------- When a user agent is to run synthetic click activation steps on an element, the user agent must run pre-click activation steps on the element, then fire a click event at the element. The default action of this click event must be to run post-click activation steps on the element. If the event is canceled, the user agent must run canceled activation steps on the element instead. ----------------

(In reply to comment #50) > I'm worried about Safari carpet bombing (e.g. <http://blogs.pcmag.com/securitywatch/2008/05/safari_carpet_bombing.php>). I don't understand why this adds any kind of new "carpet bombing" vector. A web page can already trigger downloads automatically using a cooperative server. What am I missing? > If my reading is correct, HTML5 says that synthetic events shouldn't work with links: I think your reading of the spec is correct. I would actually quote the 'activation behavior' section of a elements: If the click event in question is not trusted (i.e. a click() method call was the reason for the event being dispatched), and either the a element has a download attribute or the element's target attribute is present and applying the rules for choosing a browsing context given a browsing context name, using the value of the target attribute as the browsing context name, would result in there not being a chosen browsing context, then raise an INVALID_ACCESS_ERR exception and abort these steps. ^^^ We can extract the following from the above text: If the click event in question is not trusted, and [...] the a element has a download attribute [...], then raise an INVALID_ACCESS_ERR exception and abort these steps. I really wonder why that was put in the spec. I don't see what problem that is solving that wouldn't already exist. Will we require there to be a user gesture active in order for someone to use the FileSaver API? If it is so important that there be a user gesture present, then what about click jacking attacks?

> I don't understand why this adds any kind of new "carpet bombing" vector. A web page can already trigger downloads automatically using a cooperative server. What am I missing? I think that your analysis is accurate. The difference is that this is a new feature, so it's super safe to prevent programmatic downloading here from the start, and look into changing regular link behavior as a more dangerous fix later. > I really wonder why that was put in the spec. I don't see what problem that > is solving that wouldn't already exist. Will we require there to be a user > gesture active in order for someone to use the FileSaver API? (1) I don't know the history of that, but I like that direction. (2) Yes. I guess so?.. > If it is so important that there be a user gesture present, then what about > click jacking attacks? Is that something that can easily be prevented from the start? Otherwise, that may be a problem to think about in the future as the HTML5 platform matures. As a possibly obvious comment, I'm not talking about a user gesture being present - if that were the requirement, then a page could click() any number of links when handling a click on text content, for example. It should be an actual difference between real and synthetic events.

(In reply to comment #52) > > I don't understand why this adds any kind of new "carpet bombing" vector. A web page can already trigger downloads automatically using a cooperative server. What am I missing? > > I think that your analysis is accurate. The difference is that this is a new feature, so it's super safe to prevent programmatic downloading here from the start, and look into changing regular link behavior as a more dangerous fix later. You agree with me that being conservative here has no technical merits, and yet you prefer to be conservative? I'm not sure which analysis you are agreeing with :-) > > If it is so important that there be a user gesture present, then what about > > click jacking attacks? > > Is that something that can easily be prevented from the start? Otherwise, that may be a problem to think about in the future as the HTML5 platform matures. I don't see how to avoid it. Clearly the page can be moving an anchor tag around, with the intention of tricking the user into clicking on the anchor tag accidentally. If we think we are protecting something by requiring a real user click on an anchor tag to authorize something, then we are mistaken. There are many things in HTML that were invented without considering click-jacking attacks. > As a possibly obvious comment, I'm not talking about a user gesture being present - if that were the requirement, then a page could click() any number of links when handling a click on text content, for example. It should be an actual difference between real and synthetic events. Yeah, sorry for introducing confusion there. There's not a big difference really. Though with click-jacking you can only get a single anchor tag to be clicked, as opposed to many, if the goal is to prevent @download from triggering a download without the user's intent, then the HTML spec doesn't achieve that goal.

I'm only talking about preventing carpet bombing, not about preventing downloading a single file. As you explain, the latter cannot be always achieved if a download starts automatically, as it does in Safari on Mac. Oh, and I'm also talking about HTML5 compliance now :)

The activation behaviour is irrelevant for synthetic click events, since the activation behaviour only runs for UA-created click events (either from real clicks, or from the click() method). I do not think that Web pages should be able to trigger downloads automatically using any kind of server, cooperative or otherwise. A navigation that triggers a fetch that is treated "as a download" (to use the new spec terminology) should IMHO not automatically result in a file being stored on the user's machine. (Those pages that use meta refreshes for downloads drive me crazy. Just let me download the file from the earlier page, don't show me a separate HTML page and trigger the download from there. Gah.) But I'm probably in the minority. If you are going to allow the click() method to trigger the download as well, then let me know and I'll make honouring it in that case optional instead of disallowed.

Comment on attachment 101763 [details] proper use of ENABLE macro, and ChangeLog files r- since there were several comments to address, and other reviewers had a chance to see this in queue already. I recommend doing what HTML5 says now, and not allowing synthetic clicks to start downloads, unless there is a big reason for Chrome to want otherwise.

(In reply to comment #56) > (From update of attachment 101763 [details]) > r- since there were several comments to address, and other reviewers had a chance to see this in queue already. > > I recommend doing what HTML5 says now, and not allowing synthetic clicks to start downloads, unless there is a big reason for Chrome to want otherwise. Should synthetic clicks on a link with 'download' be completely ignored, or should only the 'download' attribute be ignored, and the click should trigger a normal navigation?

Let's not changing any currently existing behaviors in this patch.

Created attachment 101825 [details] updated I believe I have addressed all the comments (e.g. use 'Conditional', add name for the new parameter to startDownload, comment out unused parameters etc.). I have also added two tests that sets/unsets the 'download' attribute.

> > Source/WebCore/html/HTMLAnchorElement.cpp:515 > > + if (!hasRel(RelationNoReferrer)) { > > + String referrer = frame->loader()->outgoingReferrer(); > > + if (!referrer.isEmpty() && !SecurityOrigin::shouldHideReferrer(kurl, referrer)) > > + request.setHTTPReferrer(referrer); > > + frame->loader()->addExtraFieldsToMainResourceRequest(request); > > + } > > It's pretty horrible to duplicate this code. I couldn't find a clean way of avoiding the duplication here. The code is reasonably small and simple that perhaps this duplication is tolerable? But I am all-ears for suggestions to make this better :-)

Comment on attachment 101763 [details] proper use of ENABLE macro, and ChangeLog files View in context: https://bugs.webkit.org/attachment.cgi?id=101763&action=review >> Source/WebCore/loader/FrameLoaderClient.h:182 >> + virtual void startDownload(const ResourceRequest&, const String& = String()) = 0; > > This argument definitely needs a name. Added name. >> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:795 >> } > > How doesn't this unused argument no break the build? Ditto elsewhere. I think -Wno-unused-parameter flag is used for compiling? I have put the variable name in a /* comment since that style seems to be used in a few places in WebKit to avoid the issue. >> Source/WebKit2/WebProcess/WebPage/WebFrame.h:73 >> + void startDownload(const WebCore::ResourceRequest&, const String& = String()); > > I guess it's not a big deal to modify to WebFrame, but it doesn't seem necessary. I have removed this from here.

Comment on attachment 101825 [details] updated View in context: https://bugs.webkit.org/attachment.cgi?id=101825&action=review But this still allows synthetic events to start download? I thought that you were going to not allow for that? > Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1076 > +void FrameLoaderClientImpl::startDownload(const ResourceRequest& request, const String& /* suggestedName */) Another option would be to just omit the name.

(In reply to comment #62) > (From update of attachment 101825 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101825&action=review > > But this still allows synthetic events to start download? I thought that you were going to not allow for that? Ah, I misunderstood the suggestion to not change the behaviour in this patch :) I will be making the change to not trigger a download for synthetic clicks; I suppose I should be using isSimulated() to check?

> I will be making the change to not trigger a download for synthetic clicks; I suppose I should be using isSimulated() to check? I'm not sure that's right: Document::createEvent calls MouseEvent::create(), which calls MouseEvent::MouseEvent(), which calls MouseRelatedEvent::MouseRelatedEvent(), which sets m_isSimulated(false). I'm not sure we distinguish between synthetic and non-synthetic events in the DOM. Another approach is to limit this action to occurring during a user gesture by checking ScriptController::isUserGesture. That's consistent with how we handle other spam-like issues, which this appears to be.

As discussed with Darin, checking for user gesture is insufficient, because a handler for a single unrelated gesture could download any number of files, which is exactly what need to be prevented. We make a distinction for keyboard events implicitly, by checking whether there is an underlying platform event. I guess we need to make isSimulated() actually work - per Adam's analysis, it seems just broken now.

> As discussed with Darin, checking for user gesture is insufficient, because a handler for a single unrelated gesture could download any number of files, which is exactly what need to be prevented. I'm not sure why we're putting these anti-spam measures in WebCore. It seems like they should be the responsibility of the embedder. For example, Chrome already has embedder-side anti-spam measures for downloads. Other embedders, in different scenarios, might well want to make other trade-offs w.r.t. download spam. Perhaps the best approach is what we do with pop-ups: provide the embedder with a choice about whether to allow the download. In this case, specifically, Chrome would choose to have WebCore always allow the download and have the higher level anti-spam measures for downloads kick in. That approach has a number of advantages. For example, if a page initiates a download via this API and via another API, the anti-spam algorithm sees both downloads and can take appropriate action. For example, the algorithm might permit one of the downloads, block the other, and display some sort of UI explaining what happened to the user. Given that this feature is implemented only on PLATFORM(CHROMIUM) in this patch, adding anti-spam now is not necessary and would add unused complexity. It seems appropriate, then, to land this patch as-is and revisit the anti-spam question when another port is interested in implementing this API.

> I'm not sure why we're putting these anti-spam measures in WebCore Just implementing what HTML5 says.

Comment on attachment 101825 [details] updated Marking r- to that effect.

> Just implementing what HTML5 says. Hixie says in Comment #55 that he'll update the spec to allow this behavior if that's what we choose to implement. It seems clear to me that we should land this patch as-is, at least as a first iteration. If we want to add anti-spam in WebCore later, we can certainly do that. IMHO, we shouldn't handle anti-spam in WebCore because that's better handled by the embedder, like we do for other spam, like poups and alerts.

Comment on attachment 101825 [details] updated Marking R+ because this patch seems correct and ap only marked it r- based on the current spec text, which the editor has said he'll update if we go this route in the implementation.

"The spec said so" is a pretty bad reason to implement something, especially when you're the first implementation, the spec is less than a week old, and it was written was someone with my track record at making mistakes. :-)

This has been discussed in several rounds already, with no reason given not to do what the spec says. This bugzilla flag resetting war is stupid - Adam, could you please refrain from it until this is settled?

Is there a reason for Google contributors to want making synthetic events work here?

(In reply to comment #71) > "The spec said so" is a pretty bad reason to implement something, especially when you're the first implementation, the spec is less than a week old, and it was written was someone with my track record at making mistakes. :-) I thought that was a nice short reason to give after a few rounds of discussion, which seem to have been ignored by this particular reviewer. Said reviewer has also missed a number of other issues with this patch previously. Forcing cq+ over a previous r- on Sunday hasn't gone unnoticed, too. Stay classy!

(In reply to comment #73) > Is there a reason for Google contributors to want making synthetic events work here? It is not about enabling a feature for Google. AFAIK, the customer use case would be solved with the restricted form of a@download. My objection is that restricting a@download adds complexity to the platform in a way that is inconsistent with other features of the platform. Script can already create as many downloads as it likes by: 1- Creating a Data URL, Blob URL or FileSystem URL with mime type application/octet-stream, and navigating an iframe to that Data URL. (The FileSystem case just requires that you use an unknown file extension.) 2- Arranging to have a server provide a C-D header, and navigating an iframe to that HTTP URL. This means that placing a restriction on a@download doesn't prevent someone who wishes to spam the browser's download system. The browser will need to have anti-spam measures to manage those vectors. Second, on the topic of user-gesture. If we implement FileSaver, then we will not have a click target. We will instead need to rely on user-gesture if we wanted to limit FileSavers use. If we are going to limit FileSaver based on user-gesture, then we might as well use a consistent policy for scripts click()ing an anchor tag, otherwise it is just inconsistent policy. As you can see, I'm arguing for consistency in the platform. I think this is a good goal. My preference from most to least preferred: 1) No web platform restrictions on a@download and FileSaver. This is consistent with navigating an iframe. 2) Restrict a@download and FileSaver based on whether or not there is a user gesture.

Here's a motivating use case for unrestricted a@download. A web mail program might wish to offer the user the option to "download all" attachments. That would require clicks on multiple anchor tags, but if that clicking cannot be automated by JS, then the web app has no way to provide the user with this feature. However, an online web mail program could offer this feature.

We had a long IRC discussion with Darin Fisher, and the above use case was the only one that needed default event handlers to work with simulated events. Seems that not letting a simulated event trigger a download is still the way to go for this bug.

> Seems that not letting a simulated event trigger a download is still the way to go for this bug. Maybe I'm missing something, but this restriction doesn't seem to actually prevent the web site from doing anything because there are lots of other avenues for spamming downloads. Is there some technical reason why this restriction buys us anything?

I strongly agree with ap that dispatchEvent()-generated events should not cause a link to do anything. But that's got nothing to do with the changes for download=""; the spec has long said that script-dispatched events do not have UA-provided default actions, since the event model is that the default action is, by definition, whatever the code that dispatched the event makes it. When script dispatches the event, there is no browser-provided default action since the browser didn't dispatch the event, and thus the link should do nothing. However, when the click() method is called, it _should_ do something, because the click() method both dispatches the event *and does the default action*, again by definition. Now in this case there are two things the spec currently says should not happen that would happen in the case of a real trusted click: popups shouldn't open, and nothing should happen if the download="" attribute is present. I also think we should change the spec to say that if you navigate to a URL that then triggers a download, the download shouldn't happen. Currently this isn't in the spec. Unfortunately this not being in the spec means that the restriction on download="" is pretty lame, since it means you can do the downloads with a bunch of iframes but not with a bunch of links. In the medium term I intend to make the spec consistent so that either no downloads are allowed without user gesture, or all downloads triggered from click() will work as if they had a user gesture. I do not intend to change the thing with synthetic events (those should not ever work, that's a long-standing bug in WebKit), and do not intend to change the popup blocking. HTH.

(The point being, you should definitely prevent synthetic events from ever having a default action, and for click(), you should really either add the restriction against downloads even in the navigation case, or remove it in the download="" case. Having it only for download="" makes no sense. I'll update the spec to match implementations when there are some.)

For some context, WebKit doesn't currently expose click() on HTMLAnchorElement at all. I think that we only support it on buttons.

Comment on attachment 101825 [details] updated Requesting r?/cq? since it sounds like there is agreement that synthetic clicks should be handled to not trigger default action in general, but doing that especially for only a@download in this bug is inconsistent.

Comment on attachment 101825 [details] updated If you want to add a check for synthetic events for HTMLAnchorElement in general, you are welcome to do this in a separate bug. But adding a new feature without the check is not the way to go - it's something that needs to be done, and now is the best time to do it.

(In reply to comment #79) Hixie, thank you for the detailed explanation! It was very educational. I was indeed conflating click() and synthetic events. I agree that a synthetic "click" event should not perform the default action. Here's what I think we should do: 1- Add support for a@download. 2- Disable performing the default action for synthetic events. 3- Add support for HTMLAnchorElement::click(). I think each of these steps should be separate bugs / patches. > But adding a new feature without the check is not the way to go - it's something > that needs to be done, and now is the best time to do it. I think you are arguing that if we do not add the check for synthetic events in this case that we might lead people to depend on being able to use synthetic click events to trigger a download. That might make it harder to implement #2. I'm assuming the code to check for a synthetic event is fairly trivial?

(In reply to comment #80) > (The point being, you should definitely prevent synthetic events from ever having a default action, and for click(), you should really either add the restriction against downloads even in the navigation case, or remove it in the download="" case. Having it only for download="" makes no sense. I'll update the spec to match implementations when there are some.) Agreed. I don't think we can get away with preventing downloads triggered via script navigating a window as that would break many web sites. We can't require a user gesture.

> I think you are arguing that if we do not add the check for synthetic events in this > case that we might lead people to depend on being able to use synthetic click events > to trigger a download. That might make it harder to implement #2. That's correct, I prefer this check to be implemented upfront for this reason. There is no good reason to land this patch with regression tests using dispatchEvent just to change them the next day, when dispatchEvent stops working. > I'm assuming the code to check for a synthetic event is fairly trivial? Yes, adding it in @download code path shouldn't be a significant slowdown to getting this feature implemented and enabled in Chrome. As Adam Barth mentioned in comment 64, there is some investigation to do, but it's unlikely to be very difficult.

> 2- Disable performing the default action for synthetic events. ^^^ It's unclear to me what sort of compatibility risk this change would incur. Whether we want to make that change globally throughout the project is a separate question from what we do with this patch. The correct way to implement (2) is to change when HTMLAnchorElement::defaultEventHandler is called, not do donk around with the "is synthetic" bool. Changing when HTMLAnchorElement::defaultEventHandler is called will change both the link following and the downloading behavior simultaneously, which means if we tried to implement that restriction this patch we'd either 1) face the compat risk of changing how dispatchEvent works for following hyperlinks, or 2) need to used a hacky implementation of the restriction other than changing when HTMLAnchorElement::defaultEventHandler is called. Neither of those alternatives is appealing. Therefore, we should proceed with this patch as-is and then decide whether to implement (2) globally. Once we make that decision, we can implement it correctly.

The fact that this patch contains a testing that uses dispatchEvent is irrelevant. If we globally change how dispatchEvent works in WebKit, there are going to be lots of tests that need to be updated. In any case, we're no longer talking about whether this patch is beneficial. We're now just talking about the order in which we wish to make future changes. IMHO, this patch has been delayed long enough by this non-issue.

This is a new feature. It is very relevant it is implemented correctly from the start. The correct behavior could have been implemented in a fraction of time spent debating whether to land a patch with incorrect behavior.

> The correct behavior could have been implemented in a fraction of time spent debating whether to land a patch with incorrect behavior. You haven't responded the content of Comment #87, which explains why this is not as simple that. We shouldn't gate this feature on incurring the global compat risk, nor should we implement the restriction only for this feature because that's not the correct way to implement the restriction. In any case, I'm tired of this discussion. It's not going anywhere. This is an incredibly minor feature. The fact that we need to have a massive bug thread about this issue tells me there is something wrong with how the project is functioning.

I don't know what to respond to comment 87. It contains a claim that a difficult and risky way to implement this is correct, but I don't know why it is better than the alternative.

What is the alternative?

To save a round-trip, if you're suggestion is to use isSynthetic, that's not a good design. We tried that approach with user gestures, and it was a mess because folks need to correct handle the bit in many places and the consequences are subtle. As an example, our implementation of isSynthetic is currently wrong in a couple places which I found by inspecting the code. The trail of tears from that sort of design is why I ripped out wasUserGesture. The model Hixie suggests in Comment #79 is a better approach for implementing that behavior (assuming we want to implement that behavior).

Looking into isSimulated in more detail, it's only used to adjust the location of the event based on zoom settings, which is why it's very robust. It's also only present on MouseRelatedEvent. I think the right way to resolve this issue is to change the test to use EventSender and then land this patch. We can take up the question of whether to globally change how we deal with default event handlers in another bug.

(In reply to comment #94) > I think the right way to resolve this issue is to change the test to use EventSender and then land this patch. We can take up the question of whether to globally change how we deal with default event handlers in another bug. ^^^ Agreed. I'm not against limiting what dispatchEvent can do, but I don't think we need to do that in this patch. I agree that it should be done globally in a clean, maintainable way. I also agree that EventSender is the right thing to use now, so that when we change the behavior of dispatchEvent, we don't have to rewrite these tests.

Created attachment 102046 [details] Patch with updated tests

> which is why it's very robust it's => it isn't

Is anyone willing to take responsibility for making sure that dispatchEvent() on <a download> doesn't trigger a download before any significant WebKit browser release?

Comment on attachment 102046 [details] Patch with updated tests LGTM

> Is anyone willing to take responsibility for making sure that dispatchEvent() on <a download> doesn't trigger a download before any significant WebKit browser release? Perhaps more appropriate would be a willingness to undertake the compat risk of changing dispatchEvent not trigger a download if/when we change WebKit's behavior globally.

No. If we can't implement this feature with standard compatible behavior because WebKit is "globally" not ready for that, then fixing synthetic events "globally" becomes a pre-requisite.

Comment on attachment 102046 [details] Patch with updated tests Attachment 102046 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9250472

> No. If we can't implement this feature with standard compatible behavior because WebKit is "globally" not ready for that, then fixing synthetic events "globally" becomes a pre-requisite. I don't believe it's reasonable to gate this feature on changing WebKit's default event handler behavior globally. Sorry.

(In reply to comment #101) > No. If we can't implement this feature with standard compatible behavior because WebKit is "globally" not ready for that, then fixing synthetic events "globally" becomes a pre-requisite. Alexey, I'm struggling to understand your argument. I think it makes sense to separate the a@download feature from the behavior of dispatchEvent. We can agree that changing the behavior of dispatchEvent is a larger challenge, and it should be done in a separate patch. I'm not concerned about the order of these patches. Here's why: No other browsers ship a@download yet. Firefox, which appears interested in this feature too, does not perform the default action when a synthetic event is dispatched on an anchor element. So, it is unlikely that apps would start depending on this behavior since it would not be universally supported. The HTML spec even disallows it. Frankly, I think you are making this into a much bigger deal than it needs to be. I think there is probably more risk involved with a one-off "isSynthetic" check than temporarily shipping code that permits a synethetic click to trigger a download. Put another way, I don't mind making a breaking change to Chrome later that removes the ability for a synthetic click to trigger a download.

Created attachment 102058 [details] Patch that builds on mac

This patch adds a feature that's not working as specified in a significant way, and there is no clear path proposed towards fixing that. That's not a separate concern in my opinion. Thank you for updating the tests. After all this discussion, I think that it's appropriate to also add a test verifying that initEvent/dispatchEvent don't trigger a download.

> I think there is probably more risk involved with a one-off "isSynthetic" check than temporarily shipping code that permits a synethetic click to trigger a download. I think that it's more about being a poor engineering practice than a direct risk. I'd grade the problems with this poor practice as very small. I appreciate your willingness to not get Chrome fixed on non-compliant behavior, but is there a need for it? It seems quite trivial to get it right from the start.

(In reply to comment #106) > This patch adds a feature that's not working as specified in a significant way, and there is no clear path proposed towards fixing that. That's not a separate concern in my opinion. Doesn't https://bugs.webkit.org/show_bug.cgi?id=64580#c84 provide a clear path toward fixing dispatchEvent to not perform the default action? At any rate, I filed a bug about suppressing the default action for synthetic events: https://bugs.webkit.org/show_bug.cgi?id=65215

Sorry, I thought that later comments in the bug meant that the proposal from comment 84 was no longer on the table. I don't see any possible practical problem with landing this patch if fixing bug 65215 can get a higher priority than "if/when". Having both an implementation of a@download and a fix for this long-standing bug would be great, and more than what I was asking for. Thank you!

(In reply to comment #109) > Sorry, I thought that later comments in the bug meant that the proposal from comment 84 was no longer on the table. > > I don't see any possible practical problem with landing this patch if fixing bug 65215 can get a higher priority than "if/when". Having both an implementation of a@download and a fix for this long-standing bug would be great, and more than what I was asking for. Thank you! Yes, I think that it is valuable to match the spec with regards to dispatchEvent and default actions. I also like implementing the .click() property.

Comment on attachment 102058 [details] Patch that builds on mac Clearing flags on attachment: 102058 Committed r91797: <http://trac.webkit.org/changeset/91797>

All reviewed patches have been landed. Closing bug.