Skip to content

Different approach on fixing issue of pull request #69 #74

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Different approach on fixing issue of pull request #69 #74

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions lib/saml2.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,17 @@ add_namespaces_to_child_assertions = (xml_string) ->
parse_authn_response = (saml_response, sp_private_keys, idp_certificates, allow_unencrypted, ignore_signature, require_session_index, cb) ->
user = {}

# strip of possible enveloping xml tags:
saml_response = saml_response.getElementsByTagNameNS(XMLNS.SAMLP, 'Response')[0] or saml_response

# check if we have a special case where the complete response is signed (an envelopped signature):
if !ignore_signature
for cert in idp_certificates or []
signed_data_from_complete_saml_response = check_saml_signature(saml_response.toString(), cert)
if signed_data_from_complete_saml_response.length is 1 and signed_data_from_complete_saml_response[0] == saml_response.toString()
Copy link
Contributor

Choose a reason for hiding this comment

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

When there are multiple Signature nodes in the response, this throws

TypeError: Cannot read property 'length' of null
    at parse_authn_response (/Users/mdolan/Repos/saml-test/node_modules/saml2-js/lib-js/saml2.js:603:50)

(because check_saml_signature returns null https://github.com/Clever/saml2/blob/master/lib/saml2.coffee#L178)

Maybe just need this to be signed_data_from_complete_saml_response?.length is 1?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link

Choose a reason for hiding this comment

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

Also, is there a reason for the string comparison? There are slight differences in ordering of the fields for me

# parse reponse without checking for signatures anymore:
return parse_authn_response(saml_response, sp_private_keys, idp_certificates, allow_unencrypted, true, require_session_index, cb)

async.waterfall [
(cb_wf) ->
decrypt_assertion saml_response, sp_private_keys, (err, result) ->
Expand Down