camillelamy / explainers

11 stars 5 forks source link

Special case redirects less #12

Open annevk opened 4 years ago

annevk commented 4 years ago

If you navigate from A to B which redirects to C, why would we compare A, B, and C? Why not first compare A and B and report, and then compare B and C and report? That's also how the specification handles the switching and redirects could perfectly carry the reporting headers.

That would reduce the conceptual complexity a bit I think.

camillelamy commented 4 years ago

As currently written in the explainer and spec PR, we send a report if the redirect carries reporting headers. We also check at the end if the final page ended up in another BCG or not, and send reports accordingly. We can remove that final check though I am worried that we will miss potentially useful reports.

For example, if we do A1->A2->B->A2->A1 and only A1 has reporting, we might miss the BCG switch.

Also, one note, when we do A->B->C, the COOP spec as it is written does not compare B with C, but first A with B, and then A with C. So sending a report based on the comparison of B to C does not match what we currently have in the spec.

annevk commented 4 years ago

That's interesting and seems a little weird, though I also cannot construct an example where the difference matters. But since it does for reporting I wonder if we should adjust that. What do you think?

(I think if you want useful reporting you should probably deploy it for your entire origin, especially if you have redirect setups like that.)

camillelamy commented 4 years ago

Thinking some more about it, comparing each redirect with each other or with the current page will yield different results in the same-origin-allow-popups case.

For example: 1) Page A with COOP same-origin-allow-popups opens a popup to B. 2) The response to B (COOP unsafe-none) is a redirect. 3) We end up on A (same-origin-allow-popups).

The way the spec currently works, in step 2 we compare B (unsafe-none) with A (same-origin-allow-popups) and decide we don't switch (initial navigation). Then in step 3, we compare A (same-origin-allow-popups) with A (same-origin-allow-popups) and decide we don't switch.

If we were to compare redirects to each other, the check in step 2 stays the same. However, in step 3 we compare A (same-origin-allow-popups) with B (unsafe-none) and this will return that we should switch BCG, because in this case the current COOP is unsafe-none while the navigation COOP is same-origin-allow-popups (and not the reverse).

So by comparing each redirect we do end up with a BCG swap of the popup. I can see arguments for having one behavior or another, so we should agree on which one is the correct one and have the spec align on this.

annevk commented 4 years ago

Thank you! I think due to confused deputy we should swap in that case.

cc @arturjanc

camillelamy commented 4 years ago

Note that it would also happen on a redirect to a same origin website. For example: A(same-origin-allow-popups) -> A(unsafe-none) -> A(same-origin-allow-popups) would also trigger a BCG switch if we compare redirects to each other.

camillelamy commented 4 years ago

I have created https://github.com/whatwg/html/pull/5739 to use the previous redirect rather than the current page when checking COOP (if we go in this direction),

arturjanc commented 4 years ago

I thought about this a bit and I'm leaning towards being more worried about compatibility than about security here, so I think the current behavior (compare A with B, then compare A with C) is marginally better.

Basically, from a security point of view, I can't think of a case where an attacker could redirect a document A through a same-origin redirector (with a COOP of 'unsafe-none') to land on a same-origin document C matching the COOP of A (and so retaining the BCG), where the attacker couldn't just directly navigate A to C.

But when it comes to compatibility, I'd be worried that it's not always trivial to set COOP headers on redirect responses because sometimes they're generated by a different layer of the application stack or in middleware that writes out a redirect response before other request processors have a chance to run. This means that developers may run into cases where they unexpectedly lose the BCG even on benign same-origin navigations involving redirects.

I don't feel very strongly about this though, especially if it would be simpler spec-wise, as Anne suggests.

camillelamy commented 4 years ago

The only change is the one outlined before, about same-origin-allow-popups. In other cases, a badly configured redirect header will result in a bcg switch in both cases.

It does make things simpler spec wise, since we don't need to keep track of the whole redirect chain and instead only have to reason about two URLs. Also, I did just modify Chrome's implementation to actually do that.

arturjanc commented 4 years ago

Alright, let's go with this model; we can keep an eye out for any unforeseen consequences at the application level, and revisit this if there's a good reason to do so.