1256248 - Check channel to allow newtab testing without content-signatures

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Franziskus Kiefer [:franziskus]]

Bug 1249642 introduces remote channels for newtab. This bug introduces a check to allow testing environments to disable content-signatures on remote newtab.

Comment 1

[Franziskus Kiefer [:franziskus]]

Attachment: cs_honour_channel_mode.patch

checking browser.newtabpage.remote.mode pref to disable content signature checks.

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8730122 [details] [diff] [review]
cs_honour_channel_mode.patch

Review of attachment 8730122 [details] [diff] [review]:
-----------------------------------------------------------------

One question: how are we testing that content signature checking works?  As test and test2 are for mochitest, how do we do the checks then?

My r+ depends on answering that questin.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +89,5 @@
>  #define TELEMETRY_ENABLED        "toolkit.telemetry.enabled"
>  #define ALLOW_EXPERIMENTS        "network.allow-experiments"
>  #define SAFE_HINT_HEADER_VALUE   "safeHint.enabled"
>  #define SECURITY_PREFIX          "security."
> +#define NEWTAB_CHANNEL           "browser.newtabpage.remote.mode"

this definitely can't have a name "NEWTAB_CHANNEL"

NEW_TAB_REMOTE_MODE maybe?

@@ +1643,5 @@
> +    // remote content-signature testing option
> +    if (PREF_CHANGED(NEWTAB_CHANNEL)) {
> +        nsAutoCString channel;
> +        prefs->GetCharPref(NEWTAB_CHANNEL, getter_Copies(channel));
> +        if (channel.EqualsASCII("test") ||

EqualsLiteral please

@@ +1645,5 @@
> +        nsAutoCString channel;
> +        prefs->GetCharPref(NEWTAB_CHANNEL, getter_Copies(channel));
> +        if (channel.EqualsASCII("test") ||
> +            channel.EqualsASCII("test2") ||
> +            channel.EqualsASCII("dev")) {

I really like the names.. 

[sarcasm]
    |
    |

::: netwerk/protocol/http/nsHttpHandler.h
@@ +336,5 @@
>      SpdyInformation *SpdyInfo() { return &mSpdyInfo; }
>      bool IsH2MandatorySuiteEnabled() { return mH2MandatorySuiteEnabled; }
>  
> +    // returns true if content-signature test pref is set
> +    bool NewTabCSDisabled() { return mNewTabCSDisabled; }

please don't use cryptic abbreviations.  what "CS" stands for?  hard to figure out when this is used in the code

Comment 3

[Franziskus Kiefer [:franziskus]]

Thanks for your quick review. I'll fix the things you mentioned tomorrow.

(In reply to Honza Bambas (:mayhemer) from comment #2)
> Comment on attachment 8730122 [details] [diff] [review]
> cs_honour_channel_mode.patch
> 
> Review of attachment 8730122 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One question: how are we testing that content signature checking works?  As
> test and test2 are for mochitest, how do we do the checks then?
> 
> My r+ depends on answering that questin.

The tests for content-signatures predate those channel mode definitions and thus forced remote newtab and thus content-signature verification in a different way. We'll update the tests later. Another channel mode gets added for that (probably test3...), which enforces content-signatures. But I wanted to get this one landed to avoid breaking other tests that won't work if content-signatures are always enforced.

Comment 4

[Franziskus Kiefer [:franziskus]]

Attachment: cs_honour_channel_mode.patch

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6e8d3b42619

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f6e8d3b42619