FedCM spec: IdP should validate nonce to prevent CSRF?

[obfuscoder]

In the current draft of the FedCM spec (https://fedidcg.github.io/FedCM/), section 3.5. Identity assertion endpoint contains the following:

NOTE: An [IDP](https://fedidcg.github.io/FedCM/#idp) should validate the nonce, if present, to prevent CSRF-style attacks.

There is some confusion amongst implementers, what should be validated here?

The nonce is optional and it is the RP's responsibility to define unique nonce values when initiating the FedCM call and compare the returned nonce with the provided one.

[cbiesinger]

@bvandersloot-mozilla

[samuelgoto]

I'm thinking that this should be done as part of #536 in userland (e.g. an OAuth Profile layer), rather than in browserland in the FedCM spec per se.

@timcappalli, wdyt?

[achimschloss]

Agreed, if nonce was not already a top level parameter and removing it would be a breaking change it should be userland via https://github.com/fedidcg/FedCM/issues/556 - I guess it would be fair to just remove the note

[aaronpk]

the comment in the sample code in the spec says this:

// Assume there is a method returning a random number. Store the value in a variable which can
// later be used to check against the value in the token returned.

This assumes the client understands how to parse the token returned, which I thought was not an assumption the FedCM spec wanted to make. I would recommend removing the nonce parameter entirely in favor of moving all of that kind of thing into a different layer as described in #536.

If there is anything FedCM needs to do at the FedCM API layer to mitigate this kind of injection/replay attacks, it should provide the other half of the validation story as well.

[bc-pi]

@aaronpk's perspective on layering #582 (comment) makes a lot of sense.

[samuelgoto]

I would recommend removing the nonce parameter entirely in favor of moving all of that kind of thing into a different layer as described in #536.

Yeah, nonce I think would be an easy (and, I think, only?) target once we introduce w3c-fedid/custom-requests#2.

[cbiesinger]

In principle I agree with moving nonce to params. However, we have shipped it since the initial version of FedCM and so I am not sure we can remove it now for backwards compatibility reasons.

[obfuscoder]

As changing API may be a bit tricky and needs more thinking through, could you at least remove the NOTE I mentioned as @achimschloss suggested?

[samuelgoto]

As changing API may be a bit tricky and needs more thinking through, could you at least remove the NOTE I mentioned as @achimschloss suggested?

Yeah, that sounds reasonable. Care to send a PR?

We'll get to this at some point, but if you could put together a PR, you may beat us to it.

[npm1]

Should we close this issue now?

[samuelgoto]

Should we close this issue now?

Yeah, I think we can close this issue now that this PR has been merged:

#583

I would recommend removing the nonce parameter entirely in favor of moving all of that kind of thing into a different layer as described in #536.

The only suggestion left here unaddressed is this, and I kicked off a separate issue to track it independently of this one:

https://github.com/fedidcg/FedCM/issues/616