Make Issuer Required in the Types Too (like it is at runtime)#90
Merged
cjbarth merged 1 commit intonode-saml:masterfrom Jun 16, 2022
Conversation
cjbarth
approved these changes
Jun 16, 2022
Collaborator
cjbarth
left a comment
There was a problem hiding this comment.
Thanks for catching this.
Collaborator
|
@sadikinfosec , could you please have a look at #246 and comment on how your fix appears to be breaking his case? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Resolves #89
We had an application using the 4.0.0-beta.2 version that we needed to update to use the latest master commit instead to pick up the update to the xml-encryption dependency that patched a security vulnerability.
That app was not previously passing a value for "issuer" to the SAML constructor, but was relying on the library to provide a default.
After updating, this caused problems similar to the ones mentioned in the Github issue, and after rolling back and trying again, we patched this by providing a value for "issuer" to fit the new requirement.
We noted this could've been caught earlier had TS warned us about the new requirement, and so wanted to try and update the library to that effect for anyone else who might come across this change in the future.
Checklist: