Remove extra quotes around values passed for clear-site-data header

[ksmolder]

Implementation of the patch as suggested by @tfd0207 for issue #12568.
Removing the double quotes from each value set for Clear-Site-Data makes Firefox bahave as expected. Also validated in Chrome and Edge. Currently no access to Safari.

[rullzer]

This is not according to spec: https://www.w3.org/TR/clear-site-data/

[ghost]

It seems, as far as i read the specs, this IS according to the specs. Besides that the specs are still a draft and thus work in progress, the examples are misleading (at least to me).

As stated in the w3c draft in 3.1 https://www.w3.org/TR/clear-site-data/#header

Clear-Site-Data = 1#( quoted-string )
; #rule is defined in Section 7 of RFC 7230.

So according to this rule, a valid quoted string is "value1, value2, value3" and not "value1", "value2", "value3"

Maybe that's where the draft's examples are a bit misleading, because every value is quoted on its own and i think Example4, the killswitch is wrong.

The only thing to mention is that double-quotes have to be used and not as i did single-quotes. I can not verify it in other browsers, so someone else has to check that it works with double-quotes as it has to be conform the RFC.

Hope this helps.

[kesselb]

Here is the code chromium used to parse the clear-site-data header:
https://github.com/chromium/chromium/blob/489cf13fb7bb36075efff2edc2737557279f8d81/content/browser/browsing_data/clear_site_data_handler.cc#L220

Dont know c that well but from the code i would guess that "cookie", "storage" is the expected header for chromium.

This code is used by firefox to parse the header:
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/clearsitedata/ClearSiteData.cpp#275

Not sure but from the parse method i would expected that firefox requires "cookie", "storage" as well.

[ksmolder]

It seems you are right. The way the header value is currently populated in Nextcloud, seems to follow the standard. The Chromium and Firefox code confirm the correctness of the current implementation as well.
The fact Chromium/ Chrome display an Unrecognized type: "executionContexts" error as mentioned in #12568, is probably because the clear_site_data_handler.cc file does not mention the "executionContexts" value and hence Chromium/ Chrome does not (yet) support it.

So at this point I'm pretty convinced that this PR is bogus and should be refused/ closed. Agree?
Thanks for all the feedback!

[ghost]

I read the way Chromium and Firefox parse the values. Indeed, it is with double-quotes.

Rests the question, why it behaves like it does in Chrome/Chromium and Firefox.
Without the "executionContexts" value it works as expected. Even if it should be parsed according to the Firefox-code.

So problem is in the browsercode as i think and not in Nextcloud. Let keep it on the "work in progress" on that header. Maybe it will get changed a few times more.

I agree that this can be closed.

[ksmolder]

With respect to Nextcloud, what does the "executionContexts" exactly resets? As Chrome/ Chromium does not know how to handle this value and Firefox apparently does something fishy with it, we might just remove this value completely. But I do not know why it was added to the header in the first place, so we should get confirmation of someone with more background knowledge to confirm this would be an acceptable and valid fix.

[rullzer]

executionContexts forces a reload of all the execution context. Basically it issues a reload so we can be sure there is nothing left over.

[rullzer]

So yes since we seem to follow the specs. Lets close this.