I think it is safe.

When URLLoadOptionPauseOnResponseStarted is set, SignedExchangeLoader::OnHTTPExchangeFound() doesn't pass the data to the URLLoaderClient.
https://chromium.googlesource.com/chromium/src/+/cb0dee4/content/browser/web_package/signed_exchange_loader.cc#361

SignedExchangeLoader waits for SignedExchangeLoader::ProceedWithResponse() to be called.
And then pass the data to the URLLoaderClient.
https://chromium.googlesource.com/chromium/src/+/cb0dee4/content/browser/web_package/signed_exchange_loader.cc#263

I am not convinced ;-)

What you enforce here is not related to what I explained. This is different.

How ProceedWithResponse() is supposed to work:

1) The MojoAsyncResourceHandler receives the headers.
   It stops the load. If had continued, then the InterceptResourceHandler may steal the response and do a Download.
2) The NavigationRequest runs its NavigationThrottle::WillProcessResponse. It can allow the download or not.
3) A) To proceed, it calls url_loader::ProceedWithResponse,
      The MojoAsyncResourceHandler resume the load. The response is intercepted, a download is triggered.
   B) To not proceed, it destroys the NavigationUrlLoader.

With the SignedExchangeLoader, ProceedWithResponse is called immediately, before the NavigationThrottle.

What we need to make a bug reproducer is:
1) Not to enable the network service.
2) A navigation that turns into a download.
3) A NavigationThrottle blocking the navigation.
4) Something to have this class called. WebPackage ??? (help)

ProceedWithResponse() is not needed with the NetworkService, the download is intercepted after the NavigationRequest, not before.

Without the network service:
============================

InterceptResourceHandler  -> MojoAsyncResourceHandler -> ... -> NavigationRequest -> Blink.
       |
       ⋅---------------------------------------------------------------------------> Download Manager.


With the network service:
============================

mojo::UrlLoader -> ... -> NavigationRequest -> Blink.
                                       |
                                       ⋅-----> download manager.

Does that convinces you?

I understand that NavigationThrottle's WillProcessResponse() checks could be skipped.
But I don't know how dangerous it is to skip the checks for downloads.
Do you think this is a security issue?


The SignedExchangeLoader is created only for the Signed Exchange responses.
https://chromium.googlesource.com/chromium/src/+/723b1ee/content/browser/web_package/signed_exchange_request_handler.cc#82

SignedExchangeLoader must call ProceedWithResponse().
This class need to read the response body to parse the signed exchange header, and generate a redirect.
https://chromium.googlesource.com/chromium/src/+/723b1ee/content/browser/web_package/signed_exchange_loader.cc#326
void SignedExchangeLoader::OnHTTPExchangeFound(...) {
  [...]
  forwarding_client_->OnReceiveRedirect(
      CreateRedirectInfo(request_url, outer_request_url_),
      std::move(outer_response_timing_info_)->CreateRedirectResponseHead());
}
During the process, NavigationHandleImpl doesn't recieve the response.
So NavigationRequest::OnWillProcessResponseChecksComplete() which calls ProceedWithResponse() is not executed.


In normal cases, MimeSniffingResourceHandler::MaybeStartInterception() doesn't start a Download for Signed Exchange responses because ShouldHandleAsSignedHTTPExchange() reutrns true.
But if the HTTP response header contains an attachment Content-Disposition header (ex: "content-disposition: attachment; filename=test.sxg"), MustDownload() returns true, and |intercepting_handler_| (InterceptingResourceHandler) steals the response and do a Download.
https://chromium.googlesource.com/chromium/src/+/723b1ee/content/browser/loader/mime_sniffing_resource_handler.cc#437
bool MimeSniffingResourceHandler::MaybeStartInterception() {
  if (!CanBeIntercepted())
    return true;
  [...]
  bool must_download = MustDownload();
  if (!must_download) {
    if (blink::IsSupportedMimeType(mime_type))
      return true;
    if (signed_exchange_utils::ShouldHandleAsSignedHTTPExchange(
            request()->url(), response_->head)) {
      return true;
    }
    [...]
  }
  // This request is a download.
  [...]
  std::unique_ptr<ResourceHandler> handler(
      host_->CreateResourceHandlerForDownload(...));
  intercepting_handler_->UseNewHandler(std::move(handler), std::string());
  return true;
}

https://chromium.googlesource.com/chromium/src/+/723b1ee/content/browser/loader/download_utils_impl.cc#17
bool MustDownload(const GURL& url,
                  const net::HttpResponseHeaders* headers,
                  const std::string& mime_type) {
  if (headers) {
    std::string disposition;
    if (headers->GetNormalizedHeader("content-disposition", &disposition) &&
        !disposition.empty() &&
        net::HttpContentDisposition(disposition, std::string())
            .is_attachment()) {
      return true;
    }
    [...]
  }

  return false;
}

In this case, the download is started without NavigationThrottle's WillProcessResponse() checks, because SignedExchangeLoader calls ProceedWithResponse().

It might be better to make CanBeIntercepted() return false when ShouldHandleAsSignedHTTPExchange() is true, not to trigger the download.

Ah, MimeSniffingResourceHandler::MaybeStartInterception() calls MimeSniffingResourceHandler::MustDownload(), not download_utils::MustDownload().
https://chromium.googlesource.com/chromium/src/+/723b1ee/content/browser/loader/mime_sniffing_resource_handler.cc#563

Sorry for the confusion.

> I understand that NavigationThrottle's WillProcessResponse() checks could be skipped.
> But I don't know how dangerous it is to skip the checks for downloads.
> Do you think this is a security issue?
I don't know. It looks like we can bypass at least some NavigationThrottle and some checks in NavigationRequest.

Here is a list of NavigationThrottle using WillProcessResponse():
* MetricsNavigationThrottle
* PDFIFrameNavigationThrottle
* PreviewsLitePageNavigationThrottle
* SSLErrorNavigationThrottle
* NewTabPageNavigationThrottle
* InterceptNavigationThrottle
* ActivationStateComputingNavigationThrottle
* SubframeNavigationFilteringThrottle
* SubresourceFilterSafeBrowsingActivationThrottle
* TargetHandler::Throttle
* AncestorThrottle
* BlockedSchemeNavigationThrottle
* MixedContentNavigationThrottle
* OriginPolicyThrottle

Most looks okay, maybe it can cause issues with {SSLError, MixedContent, Intercept}NavigationThrottle. I didn't check.

There are a few checks in NavigationRequest::OnResponseStarted() that might be bypassed as well.

> It might be better to make CanBeIntercepted() return false when ShouldHandleAsSignedHTTPExchange() is true, not to trigger the download.
If it didn't introduce regression, it looks good to me. Is it possible to have a download AND a signed exchange?

Downloading a signed exchange looks useful especially when we will support loading downloaded signed exchanges.

I will create a CL to support downloading a signed exchange.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/154dafcf806ed40df4496b2385cb6486c5b173a5

commit 154dafcf806ed40df4496b2385cb6486c5b173a5
Author: Tsuyoshi Horo <horo@chromium.org>
Date: Tue Nov 06 09:32:28 2018

Don't process signed exchange response with attachment Content-Disposition header

According to RFC 6266, when the disposition type matches "attachment" the
browser should prompt the user to save the response locally.

But when NetworkService is enabled, Chrome doesn't save the signed exchange, but
shows the internal content of the signed exchange even if "attachment" type
Content-Disposition header is set.

When NetworkService is disabled, Chrome downloads the signed exchange as a file
but the loading animation doesn't stop.
This is because:
  - In MimeSniffingResourceHandler::MaybeStartInterception(),
    InterceptingResourceHandler steals the response.
  - So NavigationURLLoaderImpl::URLLoaderRequestController::OnComplete() will
    not be called forever after SignedExchangeRequestHandler intercepts the
    response.

To fix this problem, this CL adds download_utils::MustDownload() check in
ShouldHandleAsSignedHTTPExchange().
So SignedExchangeRequestHandler::MaybeCreateLoaderForResponse() will not
intercept the response when "attachment" type Content-Disposition header is set.

Bug:  896659 
Change-Id: Ic1d4756f5823383d55144b0320a06851ca8ecc3e
Reviewed-on: https://chromium-review.googlesource.com/c/1293065
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605639}
[modify] https://crrev.com/154dafcf806ed40df4496b2385cb6486c5b173a5/content/browser/web_package/signed_exchange_request_handler_browsertest.cc
[modify] https://crrev.com/154dafcf806ed40df4496b2385cb6486c5b173a5/content/browser/web_package/signed_exchange_utils.cc
[modify] https://crrev.com/154dafcf806ed40df4496b2385cb6486c5b173a5/content/test/data/sxg/generate-test-sxgs.sh
[add] https://crrev.com/154dafcf806ed40df4496b2385cb6486c5b173a5/content/test/data/sxg/test.example.org_test_download.sxg
[add] https://crrev.com/154dafcf806ed40df4496b2385cb6486c5b173a5/content/test/data/sxg/test.example.org_test_download.sxg.mock-http-headers