Skip to content

fix: catch possible SAML response signing error#112

Merged
forrest-ua merged 6 commits intoauth0:masterfrom
forrest-ua:catch-sign-response-error
Jan 22, 2021
Merged

fix: catch possible SAML response signing error#112
forrest-ua merged 6 commits intoauth0:masterfrom
forrest-ua:catch-sign-response-error

Conversation

@forrest-ua
Copy link
Copy Markdown
Contributor

Description

This PR allows to catch an error that might happen due to invalid signing key (badly formatted) during SAML response signing.

Testing

  • This change adds test coverage for new/changed/fixed functionality

Copy link
Copy Markdown
Contributor

@luuuis luuuis left a comment

Choose a reason for hiding this comment

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

CI is failing; perhaps state is carrying across from one test to another.

Comment thread test/samlp.tests.js
});

describe('when invalid signing key is used', function () {
before(function () {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you need to undo this in after?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point! I've added after to clean up after the test execution, even though it was not a reason why tests were failing (see the comment below).

@forrest-ua
Copy link
Copy Markdown
Contributor Author

I found that the same error happens in master branch, after some investigation it turns out the issue is caused by the new release of cheerio-select package. node-samlp is using cheerio@~0.10.7 to extract data from html page in the tests, the problem is that cheerio@~0.10.7 was using cheerio-select@* as dependency. And that made npm to load latest available cheerio-select@1.1.0 (MAJOR update released just recently) instead of previously used cheerio-select@0.0.3

Screen Shot 2021-01-21 at 12 54 33

I've pinned previosly used cheerio-select@0.0.3, as it's a dev dependency, and not to introduce more changes to this patch change.

Comment thread lib/samlp.js
@forrest-ua forrest-ua merged commit 6cec7bd into auth0:master Jan 22, 2021
@forrest-ua forrest-ua deleted the catch-sign-response-error branch January 22, 2021 13:33
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

Successfully merging this pull request may close these issues.

3 participants