-
-
Notifications
You must be signed in to change notification settings - Fork 566
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 DigestMethod lookup bug. #144
Fix DigestMethod lookup bug. #144
Conversation
should "correctly obtain the digest method with alternate namespace declaration" do | ||
document = XMLSecurity::SignedDocument.new(fixture(:adfs_response_xmlns, false)) | ||
base64cert = document.elements["//X509Certificate"].text | ||
document.validate_signature(base64cert, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test should have an assertion and not just rely on an exception not being raised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please improve this test, @curious-attempt-bunny
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing. What about it do you want to see changed?
Am Sep 9, 2014 um 11:44 AM schrieb Ben notifications@github.com:
In test/xml_security_test.rb:
@@ -52,6 +52,12 @@ class XmlSecurityTest < Test::Unit::TestCase
assert_equal("Key validation error", exception.message)
end
- should "correctly obtain the digest method with alternate namespace declaration" do
document = XMLSecurity::SignedDocument.new(fixture(:adfs_response_xmlns, false))
base64cert = document.elements["//X509Certificate"].text
can you please improve this test, @curious-attempt-bunnydocument.validate_signature(base64cert, false)
—
Reply to this email directly or view it on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add an actual assertion? like http://apidock.com/ruby/Test/Unit/Assertions/assert_raise
+1 with a minor suggestion for the test. |
See also #129 |
@luisvm @inakidelamadrid can you guys review? i give 👍 if the test can be improved |
I am currently running into this issue and would really like this bug fix merged. |
@phene Done. |
@pitbulk @luisvm @inakidelamadrid @pwnetrationguru guys, can you please review this? it looks 👍 to me |
👍 and 💥 |
👍 |
👍 looking good |
shipping it |
Fix DigestMethod lookup bug.
thanks for your patience @curious-attempt-bunny @cbshakumar |
Thanks for maintaining ruby-saml!
|
welcome @curious-attempt-bunny ! |
Thanks!
Thanks for maintaining the ruby-saml library. This is a great service you're providing. It's been a while since I found a bug in ruby-saml. Here's a fix for this one. I hope you find the tests satisfactory.
Passes validation
<Signature xmlns:ds='http://www.w3.org/2000/09/xmldsig#'>
...
<ds:DigestMethod Algorithm='http://www.w3.org/2001/04/xmlenc#sha256'/>
Fails validation with a Digest Mismatch error
<Signature xmlns='http://www.w3.org/2000/09/xmldsig#'>
...
<DigestMethod Algorithm='http://www.w3.org/2001/04/xmlenc#sha256'/>
Details
This bug is masked by sha1 being the default
digest_algorithm
when the lookup of theDigestMethod
fails.