Check channel to allow newtab testing without content-signatures

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: franziskus, Assigned: franziskus)

Tracking

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Bug 1249642 introduces remote channels for newtab. This bug introduces a check to allow testing environments to disable content-signatures on remote newtab.
Posted patch cs_honour_channel_mode.patch (obsolete) — Splinter Review
checking browser.newtabpage.remote.mode pref to disable content signature checks.
Attachment #8730122 - Flags: review?(honzab.moz)
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
Attachment #8730122 - Flags: review?(honzab.moz) → review+
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.
Flags: needinfo?(honzab.moz)

Comment 6

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f6e8d3b42619
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Flags: needinfo?(honzab.moz)
You need to log in before you can comment on or make changes to this bug.