Skip to content

Add mandatory IDP referrer check - #338

Merged
samuelgoto merged 5 commits into
w3c-fedid:mainfrom
npm1:referrerCheck
Aug 26, 2022
Merged

Add mandatory IDP referrer check#338
samuelgoto merged 5 commits into
w3c-fedid:mainfrom
npm1:referrerCheck

Conversation

@npm1

@npm1 npm1 commented Aug 25, 2022

Copy link
Copy Markdown
Collaborator

Fixes #325


Preview | Diff

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

@samuelgoto samuelgoto left a comment

Copy link
Copy Markdown
Collaborator

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

Comment thread spec/index.bs Outdated
Comment thread spec/index.bs
```
</div>

An [=IDP=] needs to check the referrer to ensure that a malicious [=RP=] does not receive an ID

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 <samuelgoto@gmail.com>
@npm1

npm1 commented Aug 26, 2022

Copy link
Copy Markdown
Collaborator Author

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

@kdenhartog kdenhartog left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Editorial nit added

Comment thread spec/index.bs
</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
Copy Markdown
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 <samuelgoto@gmail.com>

* style

Co-authored-by: sam goto <samuelgoto@gmail.com>
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