-
-
Notifications
You must be signed in to change notification settings - Fork 554
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
fix: SteamOpenId does not validate identity url #807
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #807 +/- ##
==========================================
+ Coverage 77.80% 77.84% +0.03%
==========================================
Files 330 330
Lines 10097 10105 +8
Branches 1195 1198 +3
==========================================
+ Hits 7856 7866 +10
+ Misses 2087 2086 -1
+ Partials 154 153 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Anyone tried my suggestion in #538 (comment) instead? This patch IMHO doesn't address #538 at all. |
The Steam openId login can still be stateless. Also I don't see how adding stores in #538 (comment) address the problem, since it doesn't validate the op_endpoint/claim_id from the response still. Could you explain more about your fix? |
If I understood #538 correctly, the identify URL can be spoofed by third-party server. Restricting it to spoofing valid URL doesn't IMHO address the issue. What we really need is validating that the assertion is coming from the correct server, and keeping state should ensure that. (I might be wrong, my OpenID knowledge is very limited) |
Merged as this does not harm, but I'm still not convinced this really addresses the original issue. |
Proposed changes
The implementation of SteamOpenId does not validate identity URL and could be impersonated by attackers.
Types of changes
Please check the type of change your PR introduces:
Checklist
Put an
x
in the boxes that apply. You can also fill these out after creatingthe PR. If you're unsure about any of them, don't hesitate to ask. We're here to
help! This is simply a reminder of what we are going to look for before merging
your code.
Other information
Any other information that is important to this PR such as screenshots of how
the component looks before and after the change.