Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verify that fetch API `force-cache` and `only-if-cached` respect no-s… #10094

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@wanderview
Copy link
Contributor

wanderview commented Mar 19, 2018

…tore/no-cache headers.

@wpt-pr-bot wpt-pr-bot added the fetch label Mar 19, 2018

@wpt-pr-bot wpt-pr-bot requested review from annevk, jdm, mnot, youennf and yutakahirano Mar 19, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 19, 2018

Build PASSED

Started: 2018-03-19 14:40:40
Finished: 2018-03-19 14:54:25

View more information about this build on:

state: "stale",
cache_control: "no-store",
pragma: "no-cache",
request_cache: ["default", "force-cache"],

This comment has been minimized.

Copy link
@youennf

youennf Mar 22, 2018

Contributor

The case stale/no-store/default is probably already covered in fetch/api/request/request-cache-default.html.
We probably can remove it.

This comment has been minimized.

Copy link
@wanderview

wanderview Mar 22, 2018

Author Contributor

The purpose of this added test case is that force-cache does not override the browsers behavior of suppressing no-store responses that may be hanging around in the browser. For example, the browser may still have this data resident to support devtools viewsource, etc. Setting force-cache should not allow content to observe these no-store responses.

This comment has been minimized.

Copy link
@youennf

youennf Mar 23, 2018

Contributor

Sure, instead of request_cache: ["default", "force-cache"], request_cache: ["force-cache"] should be sufficient.

This comment has been minimized.

Copy link
@wanderview

wanderview Mar 27, 2018

Author Contributor

Don't I need the "default" there so that the http cache has a chance to see the no-store response? Or does the test infrastructure prime cache in some other way? Thanks.

state: "stale",
cache_control: "no-store",
pragma: "no-cache",
request_cache: ["default", "only-if-cached"],

This comment has been minimized.

Copy link
@youennf

youennf Mar 22, 2018

Contributor

Ditto here

@@ -75,7 +79,7 @@ function make_url(uuid, id, value, content, info) {
"&content=" + content +
"&" + id + "=" + value +
"&expires=" + dates[info.state] +
vary + cache_control + ignore_request_headers;
vary + cache_control + pragma + ignore_request_headers;

This comment has been minimized.

Copy link
@youennf

youennf Mar 22, 2018

Contributor

It is not clear to me what you are trying to do here with Pragma.
cache.py is grabbing the Pragma request header and storing it in its state so that it can later be validated by the JS test. Adding it in the URL should not change anything.
We are adding Cache-Control in the URL so that the response will contain Cache-Control header value.

This comment has been minimized.

Copy link
@wanderview

wanderview Mar 22, 2018

Author Contributor

It was my understanding that to get a response to be consistently ignored by browser caches you needed to set both of these headers:

cache-control: no-store
pragma: no-cache

This diff hunk was about getting the pragma header set on the response toward this end.

This comment has been minimized.

Copy link
@youennf

youennf Mar 23, 2018

Contributor

I see. In that case, cache.py would need to be updated to add Pragma as a response header baed on the request URL parameters, like already done for Expires for instance.

This comment has been minimized.

Copy link
@wanderview

wanderview Mar 23, 2018

Author Contributor

Oh, I see. I saw that pragma was being read from the request in cache.py and I guess I thought it was reflected back.

Since the test is adequate to cover the situation I'm trying to test without the pragma stuff actually working, then I guess I'll just remove it. Looking closer, it seems its fine for a server to simply return cache-control: no-store in the response header.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.