Skip to content

Log warning for SAMLRequests with IssueInstant older than 24 hours#2061

Merged
kayjoosten merged 1 commit into
mainfrom
feature/issue-1972-check-old-issueinstant
Jul 21, 2026
Merged

Log warning for SAMLRequests with IssueInstant older than 24 hours#2061
kayjoosten merged 1 commit into
mainfrom
feature/issue-1972-check-old-issueinstant

Conversation

@kayjoosten

Copy link
Copy Markdown
Contributor

Summary

Test plan

  • New unit tests in BindingsTest.php: old-SP warning, recent-SP notice, old-IdP still generic notice, fresh-SP no log
  • Full eb4 PHPUnit suite (225/225)
  • phpcs-legacy, docheader clean

Closes #1972

@johanib johanib left a comment

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.

LGTM!

Nitpick:
The commit message contains implementation details.

How does it address the issue?                                        
This change adds a dedicated warning for severely stale AuthnRequests,
taking priority over the existing minor-drift notice so only the      
single most-applicable message fires per request. The check is scoped 
to AuthnRequests only, since Responses are handled differently and    
weren't part of the reported issue. This is logging only, no         
requests are blocked.                                                 

Why is this change needed?
Prior to this change, EngineBlock only logged a generic clock-skew
notice when an incoming message's IssueInstant differed from server
time by more than 30 seconds, in either direction, for both
AuthnRequests and Responses. There was no way to distinguish an SP
sending severely stale AuthnRequests (>24h old, possibly indicating
replay or a badly out-of-sync clock) from routine minor clock drift.

How does it address the issue?
This change adds a dedicated warning for severely stale AuthnRequests,
taking priority over the existing minor-drift notice so only the
single most-applicable message fires per request. The check is scoped
to AuthnRequests only, since Responses are handled differently and
weren't part of the reported issue. This is logging only, no
requests are blocked.

#1972
@kayjoosten
kayjoosten force-pushed the feature/issue-1972-check-old-issueinstant branch from 8b0a5f4 to 58c7561 Compare July 21, 2026 09:13
@kayjoosten

Copy link
Copy Markdown
Contributor Author

Only changed the commit message, doesnt need rereview

@kayjoosten
kayjoosten merged commit f034748 into main Jul 21, 2026
2 checks passed
@kayjoosten
kayjoosten deleted the feature/issue-1972-check-old-issueinstant branch July 21, 2026 09:24
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.

Check for and log old IssueInstants

2 participants