w3c / resource-timing

Resource Timing
https://w3c.github.io/resource-timing/
Other
122 stars 37 forks source link

transferSize might reveal HttpOnly cookies #238

Closed annevk closed 3 years ago

annevk commented 4 years ago

Which does not seem ideal. The web platform networking APIs purposefully filter out cookie headers everywhere. (Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1679737.)

rniwa commented 3 years ago

This is one of many reasons Safari / WebKit doesn't expose transferSize.

marcelduran commented 3 years ago

Hence TAO (timing-allow-origin) header, right? Or am I missing something?

On Sat, Nov 21, 2020, 6:11 PM Ryosuke Niwa notifications@github.com wrote:

This is one of many reasons Safari / WebKit doesn't expose transferSize.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/w3c/resource-timing/issues/238#issuecomment-731670235, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAPWVA3LRF3YYMGTVRXWVTSRBXN5ANCNFSM4T43YHIA .

rniwa commented 3 years ago

Hence TAO (timing-allow-origin) header, right? Or am I missing something?

TAO wouldn't help here. This isn't a cross origin issue.

marcelduran commented 3 years ago

I'm not sure I'm following. Why transferSize should also not be available from the same origin? Mind providing an example?

On Sat, Nov 21, 2020, 11:08 PM Ryosuke Niwa notifications@github.com wrote:

Hence TAO (timing-allow-origin) header, right? Or am I missing something?

TAO wouldn't help here. This isn't a cross origin issue.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/w3c/resource-timing/issues/238#issuecomment-731708353, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAPWVBRS66M5S3VSMJXGPDSRC2FHANCNFSM4T43YHIA .

yoavweiss commented 3 years ago

httpOnly cookies should not be exposed to web content, even for the same origin. transferSize can enable sites to estimate those cookie sizes, which can reveal variations in those cookies which should not be observable.

bdekoz commented 3 years ago

hey @rniwa can you expand a bit on what keeps Safari / WebKit from exposing transferSize or encoded transfer size? Is there a common x-kind of transfer size that is acceptable to all the vendors here?

nicjansma commented 3 years ago

@annevk Could you provide a brief summary of how you could use transferSize to detect HttpOnly cookies? When I first saw this I was thinking you could compare document.cookie.length to transferSize-encodedBodySize (to get "headers size"), but the later would also obviously include the other HTTP headers as well. I don't think the client would be able to know the exact size of the "other" headers unless they changed very little by time/useragent.

annevk commented 3 years ago

@nicjansma you could just do a fetch(), no?

rniwa commented 3 years ago

@nicjansma you could just do a fetch(), no?

Also, we don't want to make it a requirement for web servers to insert varying lengths of HTTP(S) headers to obscure the presence of HttpOnly cookies.

nicjansma commented 3 years ago

@annevk Ah yep, thanks, forgot XHR/fetch have access to all headers in some cases.

yoavweiss commented 3 years ago

This was discussed on the WG call the other week.

Seems like a good path forward would be to remove the header sizes from transferSize, and replace it with a fixed value (e.g. 300 bytes) that would enable the current cache state related use cases ("is the resource fetched from cache?" and "was the resource successfully revalidated?") to remain viable without breaking existing code that relies on current values.

Krinkle commented 3 years ago

I've analysed usage in Wikimedia codebases, and the libraries we distribute currently. Indeed the only use case currently found was to "guess" whether a resource was a local cache hit, 304 Not Modified response, or fresh download.

Follows this pattern generally:

// Resource Timing API
if (nt.transferSize === 0) {
    browserCache = 'local hit';
} else if (nt.transferSize > 0 && nt.encodedBodySize > 0 && nt.transferSize < nt.encodedBodySize ) {
    browserCache = 'local hit (after HTTP 304)';
} else {
    browserCache = 'miss (HTTP 200 OK)';
}

I support the mitigation reducing transferSize to just a fixed amount for headers plus encodedBodySize, if transferred.

Now that we've learned that we don't want to expose the actual transfer size, I wonder it would make sense to remove this in a future interation of the spec. From what I can tell, it seems likely safe for web compat for it to become undefined, as the original spec didn't have it (afaik), and Safari still doesn't implement it.

In that case, we may want to first decide how to provide the cache status in a more intiutive manner for future generations, since this definitely doesn't seem like a good API for it long-term. It would be confusing both because it doesn't provide what it appears to, and the thing it actually provides (cache status) is hard to learn/discover this way. It seems appealing to jump for a shiny new field like cacheStatus holding three possible values, but that might be rather inflexible over the long-term and presents the same problem we've since faced in other APIs when we try to capture complex information in a single string, which means we can't then add new values to it that are related without breaking existing code. In this case something more split up might make sense e.g. boolean wasBodyTransferred. Depending on whether we want to long-term expose http status codes and/or to differenteiate between 304 vs fully local hit, we might want to add another bool or integer field.

(Wording of "was" vs "did" still TBD in https://github.com/w3ctag/design-reviews/issues/547)