Add SAML metadata XML file upload to auto-fill IdP configuration#26420
Add SAML metadata XML file upload to auto-fill IdP configuration#26420
Conversation
aji-aju
added
UI
|
@aji-aju the goal here is not parse the file in the UI, UI should just upload and backend needs to fill-in the details |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| ); | ||
| let certText: string | undefined; | ||
|
|
||
| for (let i = 0; i < keyDescriptors.length; i++) { |
There was a problem hiding this comment.
⚠️ Bug: KeyDescriptor fallback picks wrong cert when signing key exists
The comment at line 1190 says "Look for a KeyDescriptor with use='signing', then fall back to any KeyDescriptor", but the single-pass loop (lines 1197–1210) accepts the first match of either use="signing" or no use attribute. If an unattributed <KeyDescriptor> appears before one with use="signing" in the XML, the general-purpose key is selected and the loop breaks — the explicit signing key is never considered.
Per the SAML metadata spec, use="signing" should be preferred. A two-pass approach (or tracking a fallback separately) is needed.
Suggested fix:
let certText: string | undefined;
let fallbackCertText: string | undefined;
for (let i = 0; i < keyDescriptors.length; i++) {
const use = keyDescriptors[i].getAttribute('use');
const x509 = keyDescriptors[i].getElementsByTagNameNS(XMLDSIG_NS, 'X509Certificate')[0];
if (use === 'signing' && x509?.textContent) {
certText = x509.textContent.replace(/\s+/g, '');
break; // Preferred — stop immediately
}
if (!use && !fallbackCertText && x509?.textContent) {
fallbackCertText = x509.textContent.replace(/\s+/g, '');
}
}
certText = certText ?? fallbackCertText;
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
|
|
||
| const handleMetadataFileUpload = useCallback( | ||
| (event: React.ChangeEvent<HTMLInputElement>) => { | ||
| const file = event.target.files?.[0]; |
There was a problem hiding this comment.
💡 Edge Case: No file size guard on metadata XML upload
The handleMetadataFileUpload handler reads the entire file into memory via FileReader.readAsText() with no size check. While this is a client-side-only risk (no server upload), an accidentally selected large file could freeze or crash the browser tab. Adding a simple file.size guard (e.g., reject files > 1 MB) before reading would be a cheap safeguard.
Suggested fix:
const file = event.target.files?.[0];
if (!file) {
return;
}
const MAX_XML_SIZE = 1 * 1024 * 1024; // 1 MB
if (file.size > MAX_XML_SIZE) {
showErrorToast(t('message.file-size-exceeds-limit'));
return;
}
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
| } | ||
|
|
||
| const reader = new FileReader(); | ||
| reader.onload = (e) => { |
There was a problem hiding this comment.
💡 Edge Case: Missing FileReader.onerror handler
The FileReader has an onload callback but no onerror handler. If readAsText fails (e.g., file becomes unreadable after selection), the error is silently swallowed and the user gets no feedback.
Suggested fix:
reader.onerror = () => {
showErrorToast(t('message.metadata-xml-parse-failed'));
};
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
Jest test CoverageUI tests summary
|
|
🔍 CI failure analysis for 9e0a08f: Playwright test project "stateful" is missing from configuration causing test failures on shard 6; two GitHub action download failures due to 401 authentication errors are infrastructure issues unrelated to the PR.OverviewFour CI logs analyzed across 4 unique error templates. Findings include 1 PR-related test failure and 2 infrastructure/authentication failures that are unrelated to the PR changes. The PR adds SAML metadata XML file upload functionality with extensive locale file updates and new test coverage. FailuresPlaywright Project "stateful" Not Found (confidence: high)
GitHub Action Download: jlumbroso/free-disk-space (confidence: high)
GitHub Action Download: lewagon/wait-on-check-action (confidence: high)
Summary
Code Review
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
|
🟡 Playwright Results — all passed (7 flaky)✅ 1013 passed · ❌ 0 failed · 🟡 7 flaky · ⏭️ 53 skipped
🟡 7 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
2 participants
Describe your changes:
Fixes #26354
Screen.Recording.2026-03-12.at.9.48.37.AM.mov
I worked on ... because ...
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>