[GitHub] [wicket] salcho edited a comment on pull request #439: Wicket 6786: Add Fetch Metadata support

[GitBox <gi...@apache.org>]

salcho edited a comment on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-667014819


   Hi @svenmeier! 
   
   @eozmen410 has pointed out something that I missed before that brings us back to the reason why this is implemented the way it is and why we rejected the alternative you propose in our design:
   
   - The legacy check relies on the Referer and Origin headers to determine whether a request is cross-origin. These headers are not reliable since they are not always present and so the legacy check produces false positives, that is: it will, for instance, reject same-site requests that are legitimate because they might not carry Referer/Origin! Whenever we can determine the origin of a cross-origin request, this check works fine.
   
   - Fetch Metadata solves this problem by explicitly marking the request as same/cross-origin reliably, even in the absence of Origin/Referer, which makes the new check false postive-free, hence improving the previous check. That is, in scenarios where the legacy check would have rejected a legitimate request (because there was no Origin header), the new check can now approve the request because sec-fetch-site indicates same-origin.
   
   Because of this, running both checks serially would produce contradictions. The new check would approve and the legacy would reject. That is: whenever the legacy check makes a decision correctly, both legacy and new will agree on the final decision, but in cases where the legacy check produces a false positive, both legacy and new will contradict each other!
   
   In the model you're proposing, if at least one check rejects then the request is fully rejected and so we would be back in the world of false positives that we're trying to get out of.
   
   In summary: while having multiple Resource Isolation Policies is a good idea for the future (for implementing specific policies for iframes, for instance), this PR should be seen as improving **only one** of those policies and not as adding an additional one. 
   
   I hope that makes sense, but we would be happy to jump on a call or chat if you'd like to :)
   
   What do you think?
   
   (tagging @papegaaij for visibility and correctness, since he's very familiar with the legacy check)


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org