Skip to content

add verification to NotOnOrAfter date#9

Merged
guyneedhamcanva merged 3 commits intomasterfrom
guyn-saml-not-after-check
Mar 16, 2026
Merged

add verification to NotOnOrAfter date#9
guyneedhamcanva merged 3 commits intomasterfrom
guyn-saml-not-after-check

Conversation

@guyneedhamcanva
Copy link

Allows us to unskip this test: https://github.com/kiln/flourish/blob/169bd480245342ffaf609401470e21708d6c98ab/server/test/app/user/saml.ts#L203

Adds an assertion that the Subject NotOnOrAfter cannot be too old.

@jontyt jontyt self-requested a review January 28, 2026 08:14
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

@guyneedhamcanva guyneedhamcanva force-pushed the guyn-saml-not-after-check branch from d835a92 to aead004 Compare March 16, 2026 10:09
@guyneedhamcanva guyneedhamcanva changed the base branch from guyn-update-fork-from-upstream to master March 16, 2026 10:10
In @xmldom/xmldom 0.8.x, getAttribute returns "" for non-existent
attributes, not null. The CoffeeScript existential check only guards
against null/undefined, so an empty string passed through causing
Date.parse("") to return NaN and falsely reject valid SAML responses
where NotOnOrAfter is optional per SAML spec.

Use hasAttribute to properly check if the attribute exists before
getting its value.

Added tests for:
- Missing NotOnOrAfter attribute (should be accepted)
- Empty NotOnOrAfter attribute (should be rejected)
- Malformed NotOnOrAfter attribute (should be rejected)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@guyneedhamcanva guyneedhamcanva merged commit caf425e into master Mar 16, 2026
2 checks passed
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.

2 participants