1222541 - Allow Accept-Encoding: Brotli on local connections (localhost/127.0.0.1/::1)

Status: ASSIGNED

(Core :: Networking: HTTP, defect, P3)

Description

[Dan Callahan [:callahad]]

Sending Accept-Encoding with br is currently restricted to secure origins in netwerk/protocol/http/nsHttpHandler.cpp.

It would be fantastic if, for testing and development, localhost was exempted from this requirement, in the same way it is for ServiceWorkers in https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#1415-1419

Comment 1

[Matthew N. [:MattN]]

That code is moving to a helper in bug 1221365 which could probably be used here.

Comment 2

[Patrick McManus [:mcmanus]]

when the helper is ready I'm fine with doing this. thanks

Comment 3

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 4

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 5

[quasicomputational]

Attachment: 1/2 - test accept-encoding behaviour on secure origins

Comment 6

[quasicomputational]

Attachment: 2/2 - enable Brotli on localhost.

Comment 7

[quasicomputational]

Hmm, I don't seem to be able to add dev-doc-needed. Could someone do that for me, please? I think this change, if landed, ought definitely to go in the release notes.

Comment 8

[Dragana Damjanovic [:dragana]]

Comment on attachment 9033701 [details] [diff] [review]
2/2 - enable Brotli on localhost.

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

can you also add a comment in modules/libpref/init/all.js next to "network.http.accept-encoding.secure" that this pref is used for http2 and potentially trustworthy url as per   nsIContentSecurityManager::isOriginPotentiallyTrustworthy

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +319,4 @@
>    }
>  }
>  
> +nsresult HttpBaseChannel::IsSecureOrigin(bool *isSecure) {

can you change the name of this function to 'IsOriginPotentiallyTrustworthy'

and instead of 'isSecure' 'isTrustworthy'

I do not want to call it secure, because people may misinterpreted it as http2.

@@ +353,4 @@
>    // Construct connection info object
>    nsAutoCString host;
>    int32_t port = -1;
> +  bool isSecure = false;

can you call this 'isTrustworthy', please.

@@ +385,4 @@
>    rv = mRequestHead.SetHeader(nsHttp::Host, hostLine);
>    if (NS_FAILED(rv)) return rv;
>  
> +  rv = gHttpHandler->AddStandardRequestHeaders(&mRequestHead, isSecure);

in function AddStandardRequestHeaders can you change variable 'isSecure' to be 'isTrustworthy' as well?

@@ +1235,4 @@
>        break;
>      }
>  
> +    bool isSecure = false;

here as well

@@ +1239,5 @@
> +
> +    rv = IsSecureOrigin(&isSecure);
> +    if (NS_FAILED(rv)) return rv;
> +
> +    if (gHttpHandler->IsAcceptableEncoding(val, isSecure)) {

also in function IsAcceptableEncoding

Comment 9

[quasicomputational]

Attachment: 1/2 - test accept-encoding behaviour on secure origins

Rebased. (And also fixed getting the bug number wrong in the commit message, embarrassingly.)

Comment 10

[quasicomputational]

Attachment: 2/2 - enable Brotli on localhost.

As suggested, names have been changed to avoid implying security when it's only potential trustworthiness.

Comment 11

[quasicomputational]

Attachment: 2/2 - enable Brotli on localhost.

It helps if I actually commit the changes before generating a patch, doesn't it?

Comment 12

[quasicomputational]

Two questions that've occurred to me since yesterday:

1. If IsOriginPotentiallyTrustworthy can't get the principal or the security service, it doesn't set isPotentiallyTrustworthy at all. Is that the desired behaviour, effectively leaving it up to the caller and expecting them to initialise the variable beforehand? I think I should document that, if so. Alternatively, should it prefer to fail closed? I have no idea under what circumstances those calls can fail.

2. This is going to need an intent-to-implement/ship on m.d.platform, right? Would it be appropriate for me to send that, or should that be a Mozilla employee?

Comment 13

[Dragana Damjanovic [:dragana]]

Comment on attachment 9038337 [details] [diff] [review]
1/2 - test accept-encoding behaviour on secure origins

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

::: netwerk/test/mochitests/test_secure_content_encoding.html
@@ +14,5 @@
> +
> +    let response;
> +    let body;
> +
> +    const SECURE = "Secure headers sent, Brotli encoding used";

can you rename this and also change the comment. there are not secure headers, but rather headers for trustworthy origins.

Comment 14

[Dragana Damjanovic [:dragana]]

Comment on attachment 9038381 [details] [diff] [review]
2/2 - enable Brotli on localhost.

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

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +326,5 @@
>    mFlashPluginState = aState;
>  }
>  
> +nsresult HttpBaseChannel::IsOriginPotentiallyTrustworthy(
> +    bool *isPotentiallyTrustworthy) {

please set isPotentiallyTrustworthy to false at the beginning og the function.

@@ +395,4 @@
>    rv = mRequestHead.SetHeader(nsHttp::Host, hostLine);
>    if (NS_FAILED(rv)) return rv;
>  
> +  rv = gHttpHandler->AddStandardRequestHeaders(&mRequestHead,

can you also change 'isSecure' to 'isTrustworthy' in the  nsHttpHandler::AddStandardRequestHeader function

Comment 15

[Dragana Damjanovic [:dragana]]

(In reply to quasicomputational from comment #12)

Two questions that've occurred to me since yesterday:

1. If IsOriginPotentiallyTrustworthy can't get the principal or the security service, it doesn't set isPotentiallyTrustworthy at all. Is that the desired behaviour, effectively leaving it up to the caller and expecting them to initialise the variable beforehand? I think I should document that, if so. Alternatively, should it prefer to fail closed? I have no idea under what circumstances those calls can fail.

we should set it to false at the beginning of IsOriginPotentiallyTrustworthy.

2. This is going to need an intent-to-implement/ship on m.d.platform, right? Would it be appropriate for me to send that, or should that be a Mozilla employee?

You can send this as well it does not need to be a Mozilla employee.

Comment 16

[quasicomputational]

Attachment: 1/2 - test accept-encoding behaviour on secure origins

Everything's 'trustworthy' now instead of 'secure', except for the pref name.

Comment 17

[quasicomputational]

Attachment: 2/2 - enable Brotli on localhost.

More trustworthification, as well as ensuring the trustworthiness check fails closed instead of fails undefined.

Comment 18

[Dragana Damjanovic [:dragana]]

Comment on attachment 9040956 [details] [diff] [review]
2/2 - enable Brotli on localhost.

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

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +329,5 @@
>  }
>  
> +nsresult HttpBaseChannel::IsOriginPotentiallyTrustworthy(
> +    bool *isPotentiallyTrustworthy) {
> +  // Fail closed.

What about something like "If cannot get a URIPrincipal we will treated as a not trustworthy origin." ?

Comment 19

[Dragana Damjanovic [:dragana]]

Comment on attachment 9040955 [details] [diff] [review]
1/2 - test accept-encoding behaviour on secure origins

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

I have already gave you r+. So if you update a patch that has a r+ you can set r+ by yourself and you do not need to ask for a review again. You should ask for another review when you make significant changes to the patch.

Comment 20

[Tessa]

just got bit by this, is there any update on when this patch to fix brotli support will be merged?

Comment 21

[quasicomputational]

No, sorry - I don't have the time/patience presently to shepherd it through the process and I don't expect to soon.

I think the patches are in a good state, though! I don't know if they need rebasing now, but if anyone wants to take them up and get them merged, please feel free.