search

GSA / notifications-admin

The UI of Notify.gov
https://notify.gov
Other
11 stars 2 forks source link

Verify state for login.gov #2075

Closed xlorepdarkhelm closed 2 weeks ago

xlorepdarkhelm commented 3 weeks ago

A note to PR reviewers: it may be helpful to review our code review documentation to know what to keep in mind while reviewing pull requests.

Description

the state was largely unused by our system, and was seen as a necessity for working with login.gov. Now we are checking that the state is correct on return from login.gov. It is sent as a query parameter back from login.gov, and we are now simply checking that it matches a value we have stored in redis.

This also is now generated from the admin, and then passed into the api endpoints for both creating a new invitation, and resending an expired invitation. As such, everything from login.gov is now checked for both state and nonce (the previous work I had done). This will help eliminate possible ingresses from bad actors.

https://github.com/GSA/us-notify-compliance/issues/51

This must only be merged when both this ticket and the one linked here is finished: https://github.com/GSA/notifications-api/pull/1386

Security Considerations

Checking state, like nonce, is important for the security of the login process.

xlorepdarkhelm commented 2 weeks ago

FYI - I am testing that this all works as intended, I believe it will, and will update when I have it confirmed. Please do not merge until I say it all 100% works.

xlorepdarkhelm commented 2 weeks ago

FYI - I am testing that this all works as intended, I believe it will, and will update when I have it confirmed. Please do not merge until I say it all 100% works.

This all works now.