Defend against undefined NotOnOrAfter

[joshgummersall]

The docs explain that NotOnOrAfter and NotBefore will only be validated if they exist in the SAML response. I found that this produces a runtime exception that NotOnOrAfter is undefined, thrown here. This was the simplest fix I could find that does not affect any existing tests. Thanks for your time & a great library!

[cjbarth]

We also need a test to make sure that this doesn't break again.

[cjbarth]

Correction to this logic should be in this.calcMaxAgeAssertionTime(), not here. We should be able to handle bad data coming in and still return a proper value per the existing logic.

[joshgummersall]

@cjbarth, checkTimestampsValidityError properly handles maxTimeLimitMs that is undefined, would you say it's acceptable to return undefined from checkTimestampsValidityError?

[cjbarth]

@joshgummersall, thanks for your PR. Please note the comments and we'll do our best to help you get this code landed.

[codecov]

Codecov Report

Merging #289 (c20db86) into master (9657c66) will decrease coverage by 0.13%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #289      +/-   ##
==========================================
- Coverage   81.49%   81.37%   -0.13%     
==========================================
  Files          11       11              
  Lines         816      816              
  Branches      252      253       +1     
==========================================
- Hits          665      664       -1     
  Misses         63       63              
- Partials       88       89       +1     

Impacted Files	Coverage Δ
src/saml.ts	78.06% <0.00%> (-0.19%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[srd90]

For the record PR's description's this link is now referring to master branch content and thus shall start to refer to incorrect line when file is updated:

…undefined, thrown here. This…

Here is link which point to the version that was at the master branch at the time this PR was submitted.

---

Based on content of this PR it seems that auth response that triggered this issue had element SubjectConfirmationData which did not contain NotOnOrAfter attribute (i.e. Response/Assertion/Subject/SubjectConfirmation/SubjectConfirmationData did not have that attribute)??

If thats the case then IMHO that is against SAML spec. See https://docs.oasis-open.org/security/saml/v2.0/saml-profiles-2.0-os.pdf and chapter:

• 4 SSO Profiles of SAML
• 4.1 Web Browser SSO Profile
• 4.1.4 Use of Authentication Request Protocol
• 4.1.4.2 <Response> Usage

i.e. IMHO lines 554 - 560 of that document describes requirements for SubjectConfirmationData element at the aforementioned context (NotOnOrAfter must be present).

fwiw, Same lines (and following chapter 4.1.4.3) state that Recipient must also be present and it must be validated. passport-saml / node-saml does not do that at the moment (related issue: node-saml/passport-saml#509 ).

[joshgummersall]

@srd90 yeah, my reading of the spec you shared is the same. It seems that NotBefore is optional, but NotOnOrAfter is mandatory. I'm working with an internal IDP that I think I can submit a fix for instead or trying to land this. Thanks for sharing that resource!

[joshgummersall]

All squared away, thanks again for the pointers!