Skip to content

bugfix: log a warning instead on throwing an exception for invalid entityIDs#2449

Merged
monkeyiq merged 1 commit intosimplesamlphp-2.4from
bugfix/log-on-invalid-entity
May 28, 2025
Merged

bugfix: log a warning instead on throwing an exception for invalid entityIDs#2449
monkeyiq merged 1 commit intosimplesamlphp-2.4from
bugfix/log-on-invalid-entity

Conversation

@tvdijen
Copy link
Copy Markdown
Member

@tvdijen tvdijen commented May 28, 2025 •
edited
Loading

Closes #2448

While throwing an exception is the right thing to do, it may also be a little harsh.
Definitely not something we should do in a minor release.

@tvdijen tvdijen requested review from monkeyiq and thijskh May 28, 2025 08:08
monkeyiq
monkeyiq approved these changes May 28, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@monkeyiq monkeyiq left a comment

Choose a reason for hiding this comment

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

The $entityId is coming from the site config/metadata so if it is failing the tightened validURI logging a warning seems like a reasonable thing to do. The user might want to change this (if they can) but we should allow updates to the latest SSP in preference to forcing a perfect config.

@monkeyiq monkeyiq merged commit 5ae55b4 into simplesamlphp-2.4 May 28, 2025
55 of 56 checks passed
monkeyiq pushed a commit that referenced this pull request May 28, 2025
@tvdijen @monkeyiq
bugfix: log a warning instead on throwing an exception for invalid en…
9a77a7b 
…tityIDs (#2449)
@monkeyiq
Copy link
Copy Markdown
Contributor

monkeyiq commented May 28, 2025

I have cherry picked this to master as well. If we are loading these from a site's config then it is probably best to warn rather than refuse for these. If the SP metadata is coming from external sites which use poor URNs for entityId then we either log and allow or we force the sysadmin to script an update to change things and maybe try to change the entityids mid flight.

At any rate, I think working here is best for now and we can remove that try/catch if we want to deny such configs in the future.

@tvdijen tvdijen deleted the bugfix/log-on-invalid-entity branch May 29, 2025 09:11
tvdijen added a commit that referenced this pull request May 29, 2025
@tvdijen
bugfix: log a warning instead on throwing an exception for invalid en…
0f023ea 
…tityIDs (#2449)
tvdijen added a commit that referenced this pull request May 29, 2025
@tvdijen
bugfix: log a warning instead on throwing an exception for invalid en…
f3f5ea3 
…tityIDs (#2449)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@monkeyiq monkeyiq monkeyiq approved these changes

@thijskh thijskh Awaiting requested review from thijskh

Assignees

No one assigned

Labels

bug regression

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tvdijen @monkeyiq