Closed Bug 1681179 Opened 3 years ago Closed 7 months ago

Interaction of ServiceWorker automatic internal redirect to URL of provided response can interact with Firefox's "view image" feature in unexpected ways as "view image" will use the final URL from the response

Categories

(Core :: DOM: Service Workers, defect, P3)

Firefox 83
defect

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: woods, Unassigned)

Details

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:84.0) Gecko/20100101 Firefox/84.0

Steps to reproduce:

At Fastmail, we proxy images for user security and privacy. The proxied URL includes a query parameter which we remove in our service worker implementation; we retrieve and put images into the cache with the query parameter removed from the request URL.

When rendering an image from the service worker, the context menu references the original image URL from the time at which the requested image was inserted into the cache.

You can see a reduced example of this issue at this location: https://woods.fastmailteam.com/worker/

Load the page, observe the other_param=0 query parameter. Click the "Update query param" button, right click the image, and click "Inspect Element" — note that the image has been updated so that the query string now has other_param=1. Right click the image and click "View Image" or "Copy Image Location" — the context menu incorrectly uses the original value.

Actual results:

* The user requests example.com/example.png?some_param=1&other_param=0
* The service worker searches the cache for example.com/example.png?some_param=1 and does not find an entry
* The image is successfully fetched from example.com/example.png?some_param=1&other_param=0 and placed into the cache with the key example.com/example.png?some_param=1
* the user returns some time later, and example.com/example.png?some_param=1&other_param=0 is no longer a valid request; instead, the application requests example.com/example.png?some_param=1&other_param=1

  • the service worker searches the cache for example.com/example.png?some_param=1 and returns the value it has found
    * the user right clicks the image and clicks "View Image" only to find that example.com/example.png?some_param=1&other_param=0 has been opened!

Expected results:

"View Image" and "Copy Image Location" should use the correct URL. Chrome and Safari behave as expected, using the correct URL in the context menu.

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Component: Untriaged → DOM: Service Workers
Product: Firefox → Core
Severity: -- → S4
Priority: -- → P3

So, I think the ServiceWorker is doing the right thing here and we end up in the land of UX decisions. In your reduced example (Thank You!), there's an entry with a Request with a URL of "https://woods.fastmailteam.com/worker/fastmail.png?someparam=1&" and its corresponding Response URL is "https://woods.fastmailteam.com/worker/fastmail.png?someparam=1&other_param=0". The Response URL is what is perceived by the fetch algorithm used for image fetching, and that appears to be what the view image command is using. (Specifically, using some about:serviceworkers logging, I see that the navigation to the image triggered by view image is explicitly using the URL "https://woods.fastmailteam.com/worker/fastmail.png?someparam=1&other_param=0".)

:mconley, do you have thoughts on the operation of view image and any intent behind it using the final channel URL versus what's specified in the source tag given that it sounds like we differ from other browsers here?

Note that it's of course possible to resynthesize a response that has the actual URL you want to present to content.

Flags: needinfo?(mconley)

Note that it's of course possible to resynthesize a response that has the actual URL you want to present to content.

Can you please give us a pointer on how we might do this? The Response constructor does not seem to give you a way to initialise the url, which is a read-only field. Am I missing something here?

Apologies, was in a rush to get to dinner and I shouldn't have omitted the non-obvious part and using implementation terminology that's not in the specs.

Indeed, if you create a new Response, you will be unable to specify the URL and so it will be null. This null URL will result in the Response received by the page falling back to using the request's URL. So you can use blob() to get the data from the Response as a Blob, then pass that Blob directly to respondWith where it will be automatically passed to the Response constructor or you can have already created the Response yourself, etc. (In the Gecko implementation we refer to body streams provided (in)directly to respondWith as synthesized responses, which I thought was used more widely; maybe we use it in spec discussions, but seeing that it's not in the spec I'll try and avoid using that term in the future.)

Implementation-wise, this may end up being somewhat suboptimal as Firefox currently implements things (but we're improving that), so it does seem like maybe view image's behavior should be getting changed here if we're deviating from other browsers and there isn't a particular user agency reason to diverge. In particular, I wonder if view image might normally do the right thing via the image cache but the involvement of the ServiceWorker results in a navigation being necessary.

Got it, thanks. I created a version of our test case where the service worker now does response = new Response(response.body, response) when it gets the response from the cache, and that does indeed seem to fix the issue.

Out of interest, is this any more or less efficient than using the blob() method and passing the blob as the response directly? We probably wouldn't do this, because (in general) we want to preserve CSP headers and the like, but it would be good to know! And is all of this less efficient than just responding with the Response object directly from the cache? I thought this wouldn't be too bad as the design of the Response object is such that this would not require copying the body as it's consumed by the new Response constructor so the memory can be transferred directly, but I don't know the Gecko internals so I'm sure there's more going on here.

I think what you've decided on (response = new Response(response.body, response)) is probably the best choice as something that's sane now and can be most optimized in the future by browsers if it's not already.

It turns out my concerns about the efficiency of this in Firefox were unfounded. I looked into the aspects of the implementation I was less actively familiar with and the underlying stream abstraction will be efficiently passed through in this case. Specifically we'll extract and put the Response's underlying stream directly in the readable stream and then extract it back out when creating the new Response.

It's also more efficient than using a Blob because Blobs inherently can be read a potentially infinite number of times, which can lead to the need to buffer their contents which can be inefficient when the backing stream isn't just a file on the disk.

Thanks, that's good to know; we really appreciate you checking for us. We're happy we have a good solution to this then from our end, and I agree this is arguably a UX decision rather than defect, although as the odd one out here, Firefox may still want to consider changing behaviour.

On a mild tangent, it's interesting that with a service worker you could disable the "View image" functionality by putting fake URLs for the image sources and having the service worker translate it for the actual fetch (and then erasing the url from the response to handle current Firefox behaviour). If the images are on a separate domain, going directly to the URL won't invoke the service worker and so the fake URL won't work if loaded directly. I hope that doesn't catch on; I can see that being quite annoying!

Closing because I don't think there's any further action required here, but re-titling to make it easier to rediscover this bug as it would not be surprising for us to need to revisit this in the future on a spec or Firefox front-end basis.

Status: UNCONFIRMED → RESOLVED
Closed: 7 months ago
Flags: needinfo?(mconley)
Resolution: --- → WORKSFORME
Summary: Context menu for images uses incorrect values when service worker caches and acts upon transformed URLs → Interaction of ServiceWorker automatic internal redirect to URL of provided response can interact with Firefox's "view image" feature in unexpected ways as "view image" will use the final URL from the response
You need to log in before you can comment on or make changes to this bug.