Conversation
santiagorodriguez96
left a comment
santiagorodriguez96
left a comment
There was a problem hiding this comment.
Looking good overall! Just a couple of little things to discuss 🙂
| context "when top_origin is not set" do | ||
| let(:client_top_origin) { nil } | ||
|
|
||
| it_behaves_like "an invalid attestation response that raises", WebAuthn::TopOriginVerificationError | ||
| end |
There was a problem hiding this comment.
I'm not entirely sure about raising an error if allowed_top_origins is allow_all and topOrigin is not set. Why do you think we should do it that way?
There was a problem hiding this comment.
I understood that in L3 when crossOrigin is true there MUST be something set in topOrigin, but I'm not sure TBH
There was a problem hiding this comment.
I was thinking about a relying party with verify_cross_origin enabled and allowed_top_origins as :all more as a L2 relying party – as mentioned on this comment:
It is probably correct that an "L2" RP, expecting crossOrigin: true, topOrigin: undefined, should accept an L3 client data with "crossOrigin":true,"topOrigin":"", since "topOrigin" is an unknown attribute to that RP and so it falls under the "allow any trailing attributes" rule in the final step.
However, by doing this then it would be basically the same as ignoring the cross origin verification, with the difference that it will fail is crossOrigin is false but topOrigin is present, which is not really the behavior of a L2 RP 🤷
There was a problem hiding this comment.
I think we should consider if it's worth adding this?
The value that it provides is that a RP that sets :all as the allowed origin is that responses with crossOrigin being false and topOrigin being nil and with crossOrigin being false and topOrigin being something will both be considered invalid – but I'm not sure if users of the gem that wish to allow any top origin will care for those scenarios...
We could leave it as it's currently is and make users of the gem that wish to allow any top origin to simply ignore the cross origin verification in the configuration.
What do you think? Am I'm missing something?
There was a problem hiding this comment.
I was thinking of this feature as more of an L3 concern than an L2 one. When the time comes to fully align with the L3 specification, we could remove the verify_cross_origin option entirely. For projects that still need similar behavior, the :all option would cover that use case.
That said, we can revisit this once we’re closer to that transition. I’m okay with not adding it for now.
2 participants
Summary
Allow accepting any topOrigin value.
References