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.

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

Jump to bottom

Add unique error messages for different "Invalid Signature" cases. #146

Closed
sebastien-mignot opened this issue Mar 3, 2016 · 4 comments
Closed

Add unique error messages for different "Invalid Signature" cases. #146

sebastien-mignot opened this issue Mar 3, 2016 · 4 comments
Labels
pr-welcome

Comments

@sebastien-mignot
Copy link

Hi,

Would it be possible to improve the "Invalid signature" errors ? In real life, it seems we have this error in many case, and it's a pain to debug. More so with the fact that it's often the end user that see this error, and not me (since i use passport-saml in my code, but it's the end user that does the request when he connects to my app through saml).

One who be really easy to change, it's the one when there is multiple assertions.
And for the others, it would be nice to be just a bit more precise, so we know in which usecase we are (this should be quick). And also to display e.g. "signature 'aPoeK...' not compatible with certificate 'vVm8...'", so user can verify that he didn't sign with the wrong certif or whatever (but this would be longer to do).
@gfriis-civitas
Copy link

gfriis-civitas commented May 16, 2016 •
edited
Loading

Agreed. This has made SAML integrations incredibly painful for me.

Is this error intentionally opaque? I understand that you do not want to reveal such low level information to end users, but application developers should be masking all errors with a generic error page.

I'd make a PR for it myself, but it doesn't look like any PRs have been merged since December of last year...

@markstos
Copy link
Contributor

markstos commented Aug 3, 2016 •
edited
Loading

saml.js contains 6 unique cases which all result in the same Invalid Signature diagnostic message.

Would it simply work to extend the strings to be more unique: Invalid Signature: More than one assertion? Just spell out some more detail in each case. Unless there's a reason that it /needs/ to have the same diagnostic in every case, this seems like the simplest solution.

@markstos markstos changed the title better error message Add unique error messages for different "Invalid Signature" cases. Oct 9, 2017
@gfriis-civitas
Copy link

Thanks for fixing this!

@sebastien-mignot
Copy link
Author

@markstos Thank's a lot, this is useful !

cjbarth added a commit to cjbarth/passport-saml that referenced this issue Sep 10, 2018
@cjbarth
Merge commit '966b97a54e7a4b25316716c20e1f524b64cf8a22' into CB/query…
bcb3447 
…-string-signing

* commit '966b97a54e7a4b25316716c20e1f524b64cf8a22':
  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
cjbarth added a commit to cjbarth/passport-saml that referenced this issue Sep 10, 2018
@cjbarth
Merge commit '966b97a54e7a4b25316716c20e1f524b64cf8a22' into session-…
c8da26e 
…url-params

* commit '966b97a54e7a4b25316716c20e1f524b64cf8a22':
  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
cjbarth added a commit to cjbarth/passport-saml that referenced this issue Sep 10, 2018
@cjbarth
Merge branch 'master' into session-url-params-on-head
c917ab6 
* 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
@cjbarth
Merge branch 'master' into session-url-params
735509f 
* 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
@cjbarth
Merge commit '966b97a54e7a4b25316716c20e1f524b64cf8a22' into multiple…
14de3d9 
…-authn-context

* commit '966b97a54e7a4b25316716c20e1f524b64cf8a22':
  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
cjbarth added a commit to cjbarth/passport-saml that referenced this issue Sep 12, 2018
@cjbarth
Merge commit '966b97a54e7a4b25316716c20e1f524b64cf8a22' into entry-po…
25c9186 
…int-check

* commit '966b97a54e7a4b25316716c20e1f524b64cf8a22':
  internal: provide unique failure messages for invalid signatures. Fixes node-saml#146
@snyk-bot snyk-bot mentioned this issue Dec 2, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
pr-welcome
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@markstos @sebastien-mignot @gfriis-civitas