Skip to content
This repository has been archived by the owner on Jul 24, 2023. It is now read-only.

Issue #125 - Addressing missing server url in check_signature method #128

Closed
wants to merge 1 commit into from

Conversation

rbebersole
Copy link

The verify_discovery_results method call was moved after the check_signature method call to address a security vulnerability (issue #121). Apparently, the check_signature method relied on the endpoint instance variable being defined by the verify_discovery_results method. As a result, the check_signature method always fails because the server_url is nil.

This fix initializes the endpoint variable within the check_signature method and populates it with the server url passed by the OPENID2 client.

Note: I followed the link for instructions on contributing, but I could not find any instructions on the target site. So I used what the other contributors did as a guide.

@rbebersole
Copy link
Author

I believe the error in the failed check is related to the installing rubinius-3 and not from the coding change.

Copy link
Contributor

@utkarsh2102 utkarsh2102 left a comment

Choose a reason for hiding this comment

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

This looks good, thanks for the PR!
@tobiashm, could you take a final look and merge and release?

Comment on lines +211 to +212
# This fix corrects issue #125 - Unable to complete OpenID login
# with ruby-openid 2.9.0/2.9.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we can ✂️ this out from here?

Copy link
Author

Choose a reason for hiding this comment

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

That is fine to remove those two comment lines during the merge.

@timcappalli
Copy link
Member

This repo is being archived. Closing PR.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants