Open Bug 1222541 Opened 4 years ago Updated 4 months ago

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

Categories

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

defect

Tracking

()

People

(Reporter: callahad, Assigned: quasicomputational)

References

Details

(Keywords: dev-doc-needed, DevAdvocacy, Whiteboard: [necko-backlog][DevRel:P3])

Attachments

(2 files, 5 obsolete files)

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
That code is moving to a helper in bug 1221365 which could probably be used here.
Depends on: 1221365
No longer depends on: 1162772
when the helper is ready I'm fine with doing this. thanks
Whiteboard: [necko-backlog]
Whiteboard: [necko-backlog] → [necko-backlog][DevRel:P3]
Flags: platform-rel?
platform-rel: --- → ?
platform-rel: ? → ---
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: P1 → P3
Attachment #9033700 - Flags: review?(dd.mozilla)
Attachment #9033701 - Flags: review?(dd.mozilla)
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 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
Attachment #9033701 - Flags: review?(dd.mozilla)
Attachment #9033700 - Flags: review?(dd.mozilla) → review+

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

Attachment #9033700 - Attachment is obsolete: true
Attachment #9038337 - Flags: review?(dd.mozilla)

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

Attachment #9033701 - Attachment is obsolete: true
Attachment #9038338 - Flags: review?(dd.mozilla)

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

Assignee: nobody → quasicomputational
Attachment #9038338 - Attachment is obsolete: true
Attachment #9038338 - Flags: review?(dd.mozilla)
Attachment #9038381 - Flags: review?(dd.mozilla)
Keywords: dev-doc-needed

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 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.
Attachment #9038337 - Flags: review?(dd.mozilla) → review+
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
Attachment #9038381 - Flags: review?(dd.mozilla)

(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.

  1. 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.

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

Attachment #9038337 - Attachment is obsolete: true
Attachment #9040955 - Flags: review?(dd.mozilla)

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

Attachment #9038381 - Attachment is obsolete: true
Attachment #9040956 - Flags: review?(dd.mozilla)
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." ?
Attachment #9040956 - Flags: review?(dd.mozilla) → review+
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.
Attachment #9040955 - Flags: review?(dd.mozilla) → review+

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

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.

You need to log in before you can comment on or make changes to this bug.