Skip to content
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

Invalid Base64 encoding results in "TypeError: Cannot read property 'documentElement' of null" #238

Closed
markstos opened this issue Oct 7, 2017 · 0 comments

Comments

@markstos
Copy link
Contributor

markstos commented Oct 7, 2017

If a response is received with invalid Base64 encoding, and the response is attempted to validated, instead of throwing a reasonable error, validatePostResponse will fail with this error:

TypeError: Cannot read property 'documentElement' of null

The issue could be resolved with better error checking. If this fails:

var doc = new xmldom.DOMParser().parseFromString(xml);

Then "doc" will be null. We can test for that and return error that the either the response was not Base64 encoded, or the XML was malformed. By testing xml on the line above:

var xml = new Buffer(container.SAMLResponse, 'base64').toString('utf8');  

We may be able to differiate between the case when the base64 encoding was broken and the XML was malformed.

This related to #118, which also suggested that this error could handled better.

markstos added a commit that referenced this issue Oct 9, 2017
Before, given junk input, validatePostResponse would fail with:

   TypeError: Cannot read property 'documentElement' of null

Now we'll fail with:

    SAMLResponse is not valid base64-encoded XML

To make this work, we primarily just needed to as a simple additional error check,
but to throw the error properly, we needed to move a bit of logic into
a nearby promise, but keeping some variable definitions in the outer
scope where they continue to be expected.
cjbarth added a commit to cjbarth/passport-saml that referenced this issue Sep 10, 2018
…-string-signing

* commit 'da829fc0216ed961ea7cb8a6234df65a60f51114':
  Use crypto.randomBytes for ID generation (node-saml#235)
  BugFix: Fail gracefully when SAML Response is invalid. Fixes node-saml#238
  docs: Improve docs for privateKey format. Ref node-saml#230
  Drop support for Node versions < 4.
  v0.20.2
cjbarth added a commit to cjbarth/passport-saml that referenced this issue Sep 10, 2018
…url-params

* commit 'da829fc0216ed961ea7cb8a6234df65a60f51114':
  Use crypto.randomBytes for ID generation (node-saml#235)
  BugFix: Fail gracefully when SAML Response is invalid. Fixes node-saml#238
  docs: Improve docs for privateKey format. Ref node-saml#230
  Drop support for Node versions < 4.
  v0.20.2
cjbarth added a commit to cjbarth/passport-saml that referenced this issue Sep 10, 2018
* master: (51 commits)
  start to use the debug module.
  v0.34.0: release
  internal: provide unique failure messages for invalid signatures. Fixes node-saml#146
  package.json: bump version to 0.33.0
  docs: mention that disableRequestAuthnContext helps with AD FS
  New Feature: allow customizing the name of the strategy.
  bump version to v0.32.1
  README: link to where our Changes are documented.
  Audience validation
  README: fix typo `s/ADSF/ADFS/`
  jshint: fix jshint violation.
  v0.31.0 release
  README: update link description for ADFS docs.
  Upd: Mention ADFS 2016 with NameIDFormatError. (node-saml#242)
  Support multiple and dynamic signing certificates (node-saml#218)
  v0.30.0
  Ignore .tern-port files
  Use crypto.randomBytes for ID generation (node-saml#235)
  BugFix: Fail gracefully when SAML Response is invalid. Fixes node-saml#238
  docs: Improve docs for privateKey format. Ref node-saml#230
  ...

# Conflicts:
#	README.md
#	test/samlTests.js
#	test/tests.js
cjbarth added a commit to cjbarth/passport-saml that referenced this issue Sep 10, 2018
* master: (51 commits)
  start to use the debug module.
  v0.34.0: release
  internal: provide unique failure messages for invalid signatures. Fixes node-saml#146
  package.json: bump version to 0.33.0
  docs: mention that disableRequestAuthnContext helps with AD FS
  New Feature: allow customizing the name of the strategy.
  bump version to v0.32.1
  README: link to where our Changes are documented.
  Audience validation
  README: fix typo `s/ADSF/ADFS/`
  jshint: fix jshint violation.
  v0.31.0 release
  README: update link description for ADFS docs.
  Upd: Mention ADFS 2016 with NameIDFormatError. (node-saml#242)
  Support multiple and dynamic signing certificates (node-saml#218)
  v0.30.0
  Ignore .tern-port files
  Use crypto.randomBytes for ID generation (node-saml#235)
  BugFix: Fail gracefully when SAML Response is invalid. Fixes node-saml#238
  docs: Improve docs for privateKey format. Ref node-saml#230
  ...

# Conflicts:
#	README.md
#	test/samlTests.js
cjbarth added a commit to cjbarth/passport-saml that referenced this issue Sep 11, 2018
…-authn-context

* commit 'da829fc0216ed961ea7cb8a6234df65a60f51114':
  Use crypto.randomBytes for ID generation (node-saml#235)
  BugFix: Fail gracefully when SAML Response is invalid. Fixes node-saml#238
  docs: Improve docs for privateKey format. Ref node-saml#230
  Drop support for Node versions < 4.
  v0.20.2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant