Display SP service name on the authentication page - #273
Open
kayjoosten wants to merge 10 commits into
Open
kayjoosten wants to merge 10 commits into
kayjoosten wants to merge 10 commits into
Conversation
Gateway forwards the SP's display name in an mdui:UIInfo SAML extension on the AuthnRequest, but Webauthn's authentication page never surfaced it. Users had no way to confirm which service they were stepping up for. Bump stepup-gssp-bundle and stepup-saml-bundle to the feature branches that preserve and expose the mdui:UIInfo chunk (AuthenticationService::getMdui()). Add ServiceNameResolver to pick the best-matching DisplayName for the current locale (exact match, then en, then first available) and ServiceNameFormatter to sanitize it (strip control chars, collapse whitespace, truncate at 40 chars with an ellipsis), mirroring the logic already shipped in Stepup-gssp-example. Wire the resolved name into AuthenticationController and render it conditionally in authentication.html.twig, with translations for en/nl. When no mdui extension is present the page renders exactly as before, so this is a purely additive, backwards-compatible change. Covered by unit tests for the resolver/formatter and an integration test rendering the real Twig template with and without a service name. Manually verified end-to-end against devconf with a fake USB CDP virtual authenticator (Playwright), screenshots taken of both states.
PKSA-rxdv-j1j4-96fj and PKSA-1983-c8jn-trgm targeted simplesamlphp/saml2 versions below 4.16.16 and 4.6.14 respectively. The resolved version here is 4.19.x, well past both fixes, so composer audit no longer flags them and the ignore entries were dead weight - noise that wrongly implies the vulnerabilities are still present and mitigated by us. Kept the two entries that still apply (PKSA-1fc7, PKSA-yk3g), which have no upstream fix and are documented as mitigated/unreachable.
Both remaining ignored advisories (PKSA-1fc7 XPath DoS, PKSA-yk3g HTTP-Artifact TLS confusion) claimed no patched simplesamlphp/saml2 release existed. That's no longer true: comparing the v4.19.2..v4.19.3 tags shows both fixes were backported and released, and our resolved version is 4.19.3. The audit.ignore config key only affects a dev manually running composer audit locally - CI's daily-security-check.yml uses symfonycorp/security-checker-action, a separate tool that doesn't read this config at all, so removing it doesn't change CI behaviour. Composer's own advisory feed still reports a coarse affected-version range that hasn't caught up with the precise per-branch patch, so `composer audit` may show these two as findings again until that feed updates - that's noise in the advisory database, not a real gap.
…tched" This reverts commit c48853c.
Moved into surfnet/stepup-gssp-bundle to avoid duplicating the same locale-matching/sanitization logic across every GSSP app (Tiqr, Webauthn, gssp-example).
johanib
reviewed
Jul 22, 2026
| }, | ||
| "audit": { | ||
| "ignore": { | ||
| "PKSA-1fc7-xrz7-vw78": "GHSA-5cjr-mxj5-wmrx: DoS via XPath Transform in ds:Signature processing (transitive via surfnet/stepup-saml-bundle -> simplesamlphp/saml2). No patched simplesamlphp/saml2 4.x release exists or is planned. Mitigated in surfnet/stepup-saml-bundle by Surfnet\\SamlBundle\\Signing\\SignatureTransformGuard, called from PostBinding::processResponse() before every signature verification. See Stepup-saml-bundle PR #137.", |
Contributor
There was a problem hiding this comment.
Update minimal versions so these ignores are no longer needed? (if possible)
| "surfnet/stepup-gssp-bundle": "^6.0", | ||
| "surfnet/stepup-saml-bundle": "^7.0", | ||
| "surfnet/stepup-gssp-bundle": "dev-feature/issue-48-preserve-mdui-uiinfo as 6.0.99", | ||
| "surfnet/stepup-saml-bundle": "dev-feature/issue-48-mdui-chunk as 7.0.99", |
Contributor
There was a problem hiding this comment.
Need to adjust before merging.
composer.lock still resolved surfnet/stepup-gssp-bundle to a commit before its truncation fix (40 chars total, not 41), since that dependency's branch hadn't been pushed yet. Re-lock now that it has. Also restore the "Adding a language" doc section describing how ServiceNameResolver's locale matching works, updated to point at surfnet/stepup-gssp-bundle (the resolver moved there, this repo no longer has its own copy).
… betas surfnet/stepup-gssp-bundle 6.1.0-beta1 (PR #49) and surfnet/stepup-saml-bundle 7.1.0-beta1 (PR #137) are now tagged, so this no longer needs to float on dev-branch requirements (or the "as X.Y.99" version alias that pin required). Verified: phpstan clean, unit-tests show the same 9 pre-existing DB-connection errors as before this change (integration tests requiring the docker mariadb host, unrelated to this bump; confirmed identical on unmodified composer.lock too).
Registration silently hung in Firefox after touching the key, with no client- or server-side error. The options sent to the browser carried an empty user.displayName: the client never actually sends that field, so the server fell back to "". Firefox rejects an empty displayName; Chrome doesn't enforce it. This app only has one static configured display name anyway, so use that as the fallback instead of an empty string.
Both PKSA-1fc7-xrz7-vw78 and PKSA-yk3g-3g3t-ts6q were ignored assuming no patched simplesamlphp/saml2 4.x release existed. simplesamlphp/saml2 is already locked at 4.19.3, which is the version that fixed GHSA-5cjr-mxj5-wmrx, and composer audit reports no advisories at all for the current lock file. Remove the now-dead ignore entries.
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.
Summary
Test plan
composer checkpasses (lint, phpcs, phpstan, phpmd, unit tests, doctrine schema)