-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add mandatory IDP referrer check #338
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Will merge when comments are addressed
@@ -855,6 +855,10 @@ account_id=123&client_id=client1234&nonce=Ct60bD&disclosure_text_shown=true | |||
``` | |||
</div> | |||
|
|||
An [=IDP=] needs to check the referrer to ensure that a malicious [=RP=] does not receive an ID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a normative conformace requirement to IDPs? If so, is there something like NOTE: that we could use to collect those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything within a note is non-normative text. So I chose to not put it inside a note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added style per offline discussion.
Co-authored-by: sam goto <[email protected]>
ptal |
SHA: 3f86d58 Reason: push, by @samuelgoto Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editorial nit added
@@ -855,6 +861,13 @@ account_id=123&client_id=client1234&nonce=Ct60bD&disclosure_text_shown=true | |||
``` | |||
</div> | |||
|
|||
<div class=idp-normative-text> | |||
An [=IDP=] needs to check the referrer to ensure that a malicious [=RP=] does not receive an ID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: using RFC 2119 language here to identify this more clearly as a normative statement would be helpful.
E.g. An IDP MUST check the referrer header matches the expected client id for the RP. If the referrer does not match the client id the IDP MUST reject the request.
* Add mandatory IDP referrer check * Update spec/index.bs Co-authored-by: sam goto <[email protected]> * style Co-authored-by: sam goto <[email protected]>
Fixes #325
Preview | Diff