Skip to content
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

Merged
merged 5 commits into from
Aug 26, 2022
Merged

Conversation

npm1
Copy link
Collaborator

@npm1 npm1 commented Aug 25, 2022

Fixes #325


Preview | Diff

@npm1 npm1 requested a review from samuelgoto August 25, 2022 21:21
Copy link
Collaborator

@samuelgoto samuelgoto left a 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

spec/index.bs Outdated Show resolved Hide resolved
@@ -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
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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]>
@npm1
Copy link
Collaborator Author

npm1 commented Aug 26, 2022

ptal

@samuelgoto samuelgoto merged commit 3f86d58 into w3c-fedid:main Aug 26, 2022
github-actions bot added a commit that referenced this pull request Aug 26, 2022
SHA: 3f86d58
Reason: push, by @samuelgoto

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@npm1 npm1 deleted the referrerCheck branch August 26, 2022 19:04
Copy link
Contributor

@kdenhartog kdenhartog left a 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
Copy link
Contributor

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.

@npm1 npm1 mentioned this pull request Aug 26, 2022
cbiesinger pushed a commit to cbiesinger/WebID that referenced this pull request Oct 7, 2022
* Add mandatory IDP referrer check

* Update spec/index.bs

Co-authored-by: sam goto <[email protected]>

* style

Co-authored-by: sam goto <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IdP has to check the referrer of RP not to give IdToken to unexpected RPs
3 participants