mlamouri@: avayvod@ told me you might know about this.

Sorry, I won't have time to look into this (and I'm not really working on Chromium anymore).

Maybe you could simply disable HSTS for the tests?

> Maybe you could simply disable HSTS for the tests?

Preloaded HSTS is not bypassable (by design). I don't want to introduce a hack. :-(

The most I remember is that for https the request monitor wasn't get called so the test expectation wasn't met. If there's another way to verify the rewrite of the YouTube URL from Flash to HTML5 in a browser test or to fix the request monitor, that would solve the problem too.

Worst case (if time's pressing and a better solution is not found quickly enough) you could always temporarily disable the tests and figure out how to fix them later with the media team (e.g. when Mounir is back).

> Worst case (if time's pressing and a better solution is not found quickly enough) you could always temporarily disable the tests and figure out how to fix them later with the media team (e.g. when Mounir is back).

I have no idea if the fix will be easy, so my plan will be to disable the test. :-/

mlamouri@chromium.org: Feel free to triage by assigning to someone else.

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

commit f6cd945bf26e1073ff478c3ba2ed59332faf912b
Author: Lucas Garron <lgarron@chromium.org>
Date: Fri Oct 13 21:52:36 2017

Disable ChromeContentRendererClientBrowserTest.RewriteYouTubeFlashEmbedObject test.

crrev.com/c/679834 will preload HSTS for youtube.com, which is incompatible with
the way ChromeContentRendererClientBrowserTest.RewriteYouTubeFlashEmbedObject is
currently written. No one with enough time or knowledge to rewrite this test
at the moment, so we're disabling it for now.

Bug:  771338 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: Iebedcb157079c2412c8539ad700a3fb4758149f9
Reviewed-on: https://chromium-review.googlesource.com/706665
Commit-Queue: Lucas Garron <lgarron@chromium.org>
Reviewed-by: Anton Vayvod <avayvod@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508830}
[modify] https://crrev.com/f6cd945bf26e1073ff478c3ba2ed59332faf912b/chrome/renderer/chrome_content_renderer_client_browsertest.cc

lgarron@, can you add a command line or something that I could then use in the test? I'm not sure there will be any way to get an integration test to work with HTTPS otherwise.

I don't think a command-line flag would be a good idea.

I would try the following:
- Swap out the TransportSecurityState object in the browser/request context with a mock that returns nothing as preloaded. (Or, better yet, allows overriding results for specific domains.)
- Implement code in TransportSecurityState to return the given domain as not preloaded, and only compile that code under the UNIT_TEST build flag.

Looks like ChromeContentRendererClientBrowserTest.RewriteYouTubeFlashEmbed and ChromeContentRendererClientBrowserTest.RewriteYouTubeFlashEmbedObject are both affected (earlier test runs only singled out the latter for me).

Okay, here are the options, scoped out a bit more:

# Modify SSLContextHelper.

1. Introduce SSLContextHelper::SetTransportSecurityStateForTesting:

    void SSLContextHelper::SetTransportSecurityStateForTesting(
            std::unique_ptr<net::TransportSecurityState*> transport_security_state) {
      transport_security_state_.reset(transport_security_state);
    }

2. Create a subclass of TransportSecurityState in chrome_content_renderer_client_browsertest.cc that does all the same stuff except returns false from GetStaticDomainState() when the host is youtube.com

# Modify TransportSecurityState (general)

1. Change SetTransportSecurityStateSourceForTesting() [1] to NET_EXPORT.

2. Export a bare net::TransportSecurityStateSource value that tests outside //net can use, e.g. the kHSTSSource value in net/http/transport_security_state_static_unittest_default.h

# Modify TransportSecurityState (simple)

1. Add a *new* static function in `transport_security_state.h`:

    void SSLContextHelper::UseEmptyTransportSecurityStateForTesting()

########

In all cases, we may need to require the caller to reset the transport security state to its original value when the current test is done, or do so automatically by using a scoped object (to tell when the test gets torn down). We should also surround the exposed test functions with `#ifdef UNIT_TEST` / `#endif  // UNIT_TEST`

cbentzel@, sleevi@: Do you have a preference among the ideas above? If not, do you have a good alternative?

[1] https://cs.chromium.org/chromium/src/net/http/transport_security_state.h?l=37&rcl=8cfdfd2eba5c6e3c8f456bd95362e4d30e111c7e

I like simple solutions so I would be happy with the third one :)

I'm not particularly fond of either; I think both solutions are probably brittle, and don't represent realistic tests that align with our users or the actual system under test.

So I'd like to try to figure out how we fix the request monitor, in line with comment #4. I'm not sure I understand why comment #10 suggests there's no way possible.

Note: EmbeddedTestServer supports TYPE_HTTPS, so rewriting the embeds from HTTP->HTTPS (both in the test embeds and in the listener) seems like a relatively simple solution?

rsleevi@, I tried this the other day but the embedded test server runs locally and trying to pretend its youtube.com leads to some security problems. I stopped there because I assumed this was WAI, wasn't it?

I tried to rewrite these tests using a URLRequestInterceptor last week. With this interceptor it is possible to verify that the URL is rewritten but it is not possible to verify the mime type because the request headers aren't available when the interceptor is called (as far as I understand it :-)) so you only cover half the original testcase.

Also tried to use the HTTPS version of the EmbeddedTestServer but the requests still fail because they are sent to youtube.com for which the test server doesn't have a valid certificate. The youtube.com request are triggered by the embed (through ExecuteScript()). Maybe a MockCertVerifier that excepts the localhost certificate for youtube.com could be the best solution here?

Yes, the 'normal' way in which we would test this:

- Start embedded server listening on HTTPS
- Set a MockHostResolver to resolve your 'target' site to your embedded server
- Set a MockCertVerifier to accept the embedded server's cert as valid for your 'target' site
- Profit

See //chrome/browser/ssl_browser_tests (which, broadly speaking, use the CertVerifierBrowserTest)

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

commit bf632cf20272905d4c1b387065fa4a1b93a96699
Author: Lucas Garron <lgarron@chromium.org>
Date: Wed Oct 18 10:44:35 2017

Disable ChromeContentRendererClientBrowserTest.RewriteYouTubeFlashEmbed.

This is the sister test to ChromeContentRendererClientBrowserTest.RewriteYouTubeFlashEmbedObject. Both of them are incompatible with preloading HSTS for youtube.com as written.

Bug:  771338 
Change-Id: I6d3357cbbd3ae978721e0e7c7cafd9410c274db9
Reviewed-on: https://chromium-review.googlesource.com/722448
Reviewed-by: Lucas Garron <lgarron@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Lucas Garron <lgarron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509734}
[modify] https://crrev.com/bf632cf20272905d4c1b387065fa4a1b93a96699/chrome/renderer/chrome_content_renderer_client_browsertest.cc

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

commit 2f1deeb999de52ae84f1681c5e62a912984ffe46
Author: Lucas Garron <lgarron@chromium.org>
Date: Thu Oct 19 23:34:43 2017

Disable ChromeContentRendererClientBrowserTest.RewriteYouTubeFlashEmbedObject test.

crrev.com/c/679834 will preload HSTS for youtube.com, which is incompatible with
the way ChromeContentRendererClientBrowserTest.RewriteYouTubeFlashEmbedObject is
currently written. No one with enough time or knowledge to rewrite this test
at the moment, so we're disabling it for now.

TBR=lgarron@chromium.org

(cherry picked from commit f6cd945bf26e1073ff478c3ba2ed59332faf912b)

Bug:  771338 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: Iebedcb157079c2412c8539ad700a3fb4758149f9
Reviewed-on: https://chromium-review.googlesource.com/706665
Commit-Queue: Lucas Garron <lgarron@chromium.org>
Reviewed-by: Anton Vayvod <avayvod@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#508830}
Reviewed-on: https://chromium-review.googlesource.com/729535
Reviewed-by: Lucas Garron <lgarron@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#92}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/2f1deeb999de52ae84f1681c5e62a912984ffe46/chrome/renderer/chrome_content_renderer_client_browsertest.cc

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

commit 376bcff59b5f754fff3a08d67b10950959453b4a
Author: Lucas Garron <lgarron@chromium.org>
Date: Thu Oct 19 23:59:11 2017

Disable ChromeContentRendererClientBrowserTest.RewriteYouTubeFlashEmbed.

This is the sister test to ChromeContentRendererClientBrowserTest.RewriteYouTubeFlashEmbedObject. Both of them are incompatible with preloading HSTS for youtube.com as written.

TBR=lgarron@chromium.org

(cherry picked from commit bf632cf20272905d4c1b387065fa4a1b93a96699)

Bug:  771338 
Change-Id: I6d3357cbbd3ae978721e0e7c7cafd9410c274db9
Reviewed-on: https://chromium-review.googlesource.com/722448
Reviewed-by: Lucas Garron <lgarron@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Lucas Garron <lgarron@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#509734}
Reviewed-on: https://chromium-review.googlesource.com/729715
Cr-Commit-Position: refs/branch-heads/3239@{#93}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/376bcff59b5f754fff3a08d67b10950959453b4a/chrome/renderer/chrome_content_renderer_client_browsertest.cc

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

commit 0cbb54fd8f26e8ae07331debaa41861e47033e48
Author: Mounir Lamouri <mlamouri@chromium.org>
Date: Mon Oct 23 15:15:33 2017

Migrate YouTube Flash rewrite to HTTPS.

This is re-enabling tests that have been disabled when youtube.com
was added to the HSTS list.

Bug:  771338 
Change-Id: Ib8ccdcb3bf92d8863624b95b64539bb2815312e5
Reviewed-on: https://chromium-review.googlesource.com/725428
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510796}
[modify] https://crrev.com/0cbb54fd8f26e8ae07331debaa41861e47033e48/chrome/renderer/DEPS
[modify] https://crrev.com/0cbb54fd8f26e8ae07331debaa41861e47033e48/chrome/renderer/chrome_content_renderer_client_browsertest.cc