feat(sso): check assertion type

[enmande]

🎟️ Tracking

PM-42627

📔 Objective

PM-42627

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Re-reviewed after the five commits since the last round (ff3f403…496bca4). The Saml2EncryptedAssertionInspector still runs on unauthenticated ACS input and still handles that correctly: ClassifyAlgorithm collapses every value outside the known set to the constant "unrecognized", so an attacker-controlled Algorithm attribute cannot reach the metric tag and cardinality stays bounded to three values regardless of how many xenc:EncryptedKey elements arrive. The earlier WantAssertionsSigned change was reverted per review discussion and replaced with a PM-42982 TODO, so the plaintext-assertion signature check is back to its original, unweakened form. Both findings from the previous round are addressed, and the SamlEncryptionAlgorithms → Saml2KeyTransportEncryptionAlgorithms rename has no remaining references anywhere in the repository.

Code Review Details

No new findings.

Previous round's findings, verified resolved at head:

• ♻️ Metric unit and description now read {requests} and "SAML ACS requests carrying at least one assertion key…", matching the deduped per-request-per-algorithm value the counter records (Saml2AssertionMetrics.cs:18-19, commit 2b615238c).
• ♻️ WithFakeLogging(), the _enableFakeLogging field, and the BuildAsync fake-logging block are gone; no references remain under bitwarden_license/ (commit 3d7c56f11).
• The three MetricCollector disposal warnings from the code-quality bot are handled by the MetricTestContext / Arrangement disposable records with using var at each call site (commit 496bca45c).

Notes, not findings:

• The generic catch in TryRecordUnsupportedKeyTransportAlgorithms is documented as deliberate fail-open on the SSO login path and is covered by CouldHandleAsync_AlgorithmInspectionThrows_DoesNotPropagate.
• SsoAuthenticationMiddleware iterates every request-handler scheme, but the ModulePath and Issuer/entity-ID guards in CouldHandleAsync run before the inspector, so a single ACS request cannot double-count across organizations.
• The hold label is applied and Enforce Label is failing, so merge is blocked on process rather than on code.

Dependency Changes

Package	Change	Ecosystem
Microsoft.Extensions.Diagnostics.Testing	New (10.8.0)	NuGet

Test-scope only, and not net-new to the repository — the same [10.8.0] pin already exists in Core.Test, Identity.Test, SharedWeb.Test, and Infrastructure.IntegrationTest. No AppSec approval finding. Both packages.lock.json files were regenerated alongside the manifest changes.

PR Metadata Assessment

• ❓ QUESTION: The Objective section still repeats the Jira link with no prose. A sentence on what the counter measures and what decision it informs would help reviewers and future readers.

[codecov]

Codecov Report

❌ Patch coverage is 98.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 64.03%. Comparing base (71f085f) to head (496bca4).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
.../Sso/Utilities/Saml2EncryptedAssertionInspector.cs	97.72%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8301      +/-   ##
==========================================
+ Coverage   63.88%   64.03%   +0.14%     
==========================================
  Files        2471     2473       +2     
  Lines      105923   106017      +94     
  Branches     9598     9613      +15     
==========================================
+ Hits        67673    67885     +212     
+ Misses      35909    35772     -137     
- Partials     2341     2360      +19     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[JaredSnider-Bitwarden]

Excellent work on the iteration! Thank you very much!