Cache API should block 206 responses

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: bkelly, Assigned: tt)

Tracking

(Blocks 3 bugs)

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 affected, firefox53 fixed)

Details

(Whiteboard: btpp-fixlater)

Attachments

(2 attachments, 4 obsolete attachments)

One part of fixing range response handling is preventing 206 responses from being stored in Cache API:

  https://github.com/slightlyoff/ServiceWorker/issues/703#issuecomment-175273497

There are still some questions about all of this, but blocking those now seems safe and reasonable.
Whiteboard: btpp-fixlater
(Assignee)

Comment 2

2 years ago
I would like to take this bug.
Assignee: nobody → ttung
Status: NEW → ASSIGNED
(Assignee)

Comment 3

2 years ago
Hi Ben,

I implement this patch for blocking 206 response in add(), addAll() and put(). Could you help me to review this patch? Thanks!
Attachment #8819192 - Flags: review?(bkelly)
(Assignee)

Comment 4

2 years ago
Patch for WPT test. Could you help me to review this patch as well, Ben? Thanks!
Attachment #8819193 - Flags: review?(bkelly)
Comment on attachment 8819192 [details] [diff] [review]
Part 1: Cache API should block 206 response.

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

r=me with comments addressed.

::: dom/cache/Cache.cpp
@@ +182,5 @@
> +      if (response->Status() == 206) {
> +        nsAutoString status;
> +        status.AppendInt(response->Status());
> +        ErrorResult rv;
> +        rv.ThrowTypeError<MSG_CACHE_INVALID_RESPONSE_STATUS>(status);

Can you just use MSG_CACHE_ADD_FAILED_RESPONSE instead?  It pretty much says the correct thing and also includes the request URL which is helpful for context.

@@ +418,5 @@
>  
> +  if (NS_WARN_IF(aResponse.Status() == 206)) {
> +    nsAutoString status;
> +    status.AppendInt(aResponse.Status());
> +    aRv.ThrowTypeError<MSG_CACHE_INVALID_RESPONSE_STATUS>(status);

Can you make a helper method like IsValidPutRequestMethod() that performs this check and throws the TypeError if necessary?  We could call it IsValidPutResponseStatus().

https://dxr.mozilla.org/mozilla-central/source/dom/cache/Cache.cpp#58
Attachment #8819192 - Flags: review?(bkelly) → review+
Comment on attachment 8819193 [details] [diff] [review]
Part 2: Add WPT tests for blocking 206 response.

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

r=me with comments addressed.

::: testing/web-platform/tests/service-workers/cache-storage/script-tests/cache-add.js
@@ +82,5 @@
>    }, 'Cache.add with request with null body (not consumed)');
>  
>  cache_test(function(cache) {
>      return assert_promise_rejects(
> +      cache.add('../resources/fetch-status.php?status=206'),

This seems wrong.  We don't use .php files.  We should have a .py instead:

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/cache-storage/resources/fetch-status.py

So please change these to fetch-status.py?status=206.
Attachment #8819193 - Flags: review?(bkelly) → review+
(Assignee)

Comment 8

2 years ago
(In reply to Ben Kelly [:bkelly] from comment #6)
Thanks for the reviews!

> @@ +418,5 @@
> >  
> > +  if (NS_WARN_IF(aResponse.Status() == 206)) {
> > +    nsAutoString status;
> > +    status.AppendInt(aResponse.Status());
> > +    aRv.ThrowTypeError<MSG_CACHE_INVALID_RESPONSE_STATUS>(status);
> 
> Can you make a helper method like IsValidPutRequestMethod() that performs
> this check and throws the TypeError if necessary?  We could call it
> IsValidPutResponseStatus().
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/cache/Cache.cpp#58

Sure, but is it only called by Cache::Put()? I used to think about it, but the helper function will be only used in Cache::Put(). Thus, I decided to remove it in previous patch(Part 1, Version 0). 

Could you help me to review this patch again? Thanks! I'm not sure my patch address your comment.
Attachment #8819192 - Attachment is obsolete: true
Attachment #8820112 - Flags: review?(bkelly)
(Assignee)

Comment 9

2 years ago
Address the comment!

(In reply to Ben Kelly [:bkelly] from comment #7)
> So please change these to fetch-status.py?status=206.

Thanks for pointing out using wrong file. I didn't notice that...

I also correct the other test which calls .php as well.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=031a4c9b63dc909e8bae3a8894db0d48ac56b047
Attachment #8819193 - Attachment is obsolete: true
Comment on attachment 8820112 [details] [diff] [review]
Part 1: Cache API should block 206 response. r=bkelly.

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

::: dom/cache/Cache.cpp
@@ +177,5 @@
>        // Do not allow the convenience methods .add()/.addAll() to store failed
> +      // or invalid responses.  A consequence of this is that these methods
> +      // cannot be used to store opaque or opaqueredirect responses since they
> +      // always expose a 0 status value.
> +      if (!response->Ok() || response->Status() == 206) {

Hmm, I'd prefer to keep all the status checking code in that IsValidPutResponseStatus() check.

Can you maybe change it to be:
  enum class PutStatusPolicy {
    Default,
    RequireOK
  };

  bool
  IsValidPut ResponseStatus(Response* aResponse, PutStatusPolicy aPolicy,
                            ErrorResult& aRv)
  {
    // Check !Ok() if aPolicy == PutStatusPolicy::RequireOK
  }

Then you can just replace this whole block with:

  if (!IsValidPutResponseStatus(response, PutStatusPolicy::RequireOK, rv)
Attachment #8820112 - Flags: review?(bkelly)
(Assignee)

Comment 11

2 years ago
(In reply to Ben Kelly [:bkelly] from comment #10)
Thanks for feedback! I address the comment. 

Could you help me to review it? Thanks!
Attachment #8820112 - Attachment is obsolete: true
Attachment #8820529 - Flags: review?(bkelly)
Comment on attachment 8820529 [details] [diff] [review]
Part 1: Cache API should block 206 response.

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

Thanks!  Just a couple nits.  r=me

::: dom/cache/Cache.cpp
@@ +187,5 @@
> +      // or invalid responses.  A consequence of this is that these methods
> +      // cannot be used to store opaque or opaqueredirect responses since they
> +      // always expose a 0 status value.
> +      ErrorResult errorResult;
> +      if (NS_WARN_IF(!IsValidPutResponseStatus(*response,

Please remove this NS_WARN_IF().  Failed network requests are pretty common and we have the console message.

@@ +422,5 @@
>    if (NS_WARN_IF(!IsValidPutRequestMethod(aRequest, aRv))) {
>      return nullptr;
>    }
>  
> +  if (NS_WARN_IF(!IsValidPutResponseStatus(aResponse, PutStatusPolicy::Default,

Again, please remove the NS_WARN_IF().  We have the console message for this.
Attachment #8820529 - Flags: review?(bkelly) → review+
(Assignee)

Comment 14

2 years ago
(In reply to Ben Kelly [:bkelly] from comment #13)

Thanks for the review! Addressed the comment!
Attachment #8820529 - Attachment is obsolete: true
(Assignee)

Comment 15

2 years ago
Comment on attachment 8820113 [details] [diff] [review]
[Final] Part 2: Add WPT tests for blocking 206 response. r=bkelly.

Update the description to mark as "[Final]" since it has already gotten r+.
Attachment #8820113 - Attachment description: Part 2: Add WPT tests for blocking 206 response. r=bkelly. → [Final] Part 2: Add WPT tests for blocking 206 response. r=bkelly.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 16

2 years ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84fc5342b9b7
Part 1: Cache API should block 206 response. r=bkelly.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c48b87e3ee4
Part 2: Add WPT tests for blocking 206 response. r=bkelly.
Keywords: checkin-needed

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/84fc5342b9b7
https://hg.mozilla.org/mozilla-central/rev/9c48b87e3ee4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Reporter)

Updated

11 months ago
Blocks: 1465074
You need to log in before you can comment on or make changes to this bug.