Conversation
github-actions
bot
added
backend
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| public static void checkAndStoreRedirectUriInSession(HttpSession session, String redirectUri) { | ||
| checkAndStoreRedirectUriInSession(session, redirectUri, new String[0]); | ||
| } |
There was a problem hiding this comment.
⚠️ Security: No-args overload of checkAndStoreRedirectUriInSession skips validation
The 2-argument overload checkAndStoreRedirectUriInSession(session, redirectUri) at line 456 delegates to the varargs version with an empty array. Because the varargs version guards validation behind trustedBaseUrls.length > 0 (line 466-467), any caller using the 2-arg overload stores the redirect URI in the session without any open-redirect validation. This defeats the purpose of the new RedirectUriValidator. If this overload is kept for backward compatibility, it should either always validate or be removed/deprecated so callers are forced to supply trusted URLs.
Suggested fix:
Either remove the 2-arg overload entirely, or make the varargs version reject the URI when no trusted URLs are provided:
if (trustedBaseUrls == null || trustedBaseUrls.length == 0
|| !RedirectUriValidator.isSafe(redirectUri, trustedBaseUrls)) {
LOG.warn("[OIDC] Rejecting redirect URI — no allowlist or not in allowlist");
throw new TechnicalException("Redirect URI is not in the allowlist");
}
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
❌ UI Checkstyle Failed❌ ESLint + Prettier + Organise Imports (src)One or more source files have linting or formatting issues. Affected files
Fix locally (fast — only checks files changed in this branch): make ui-checkstyle-changed |
There was a problem hiding this comment.
Pull request overview
This PR aims to harden SSO/SAML flows (open-redirect/XSS protections, stricter SAML defaults) and includes additional, seemingly unrelated ingestion integration tests.
Changes:
- UI: Remove
dangerouslySetInnerHTMLhighlighting in advanced search dropdown labels; build mention suggestion DOM nodes safely in FeedEditor. - Service: Introduce
RedirectUriValidatorand apply redirect allowlist checks in SAML and OIDC login/callback flows; store SAML AuthnRequest ID and validate it on callback. - Spec/config: Flip SAML security defaults (
strictMode,wantMessagesSigned,wantAssertionsSigned) totrue; update example YAMLs accordingly. - Ingestion: Add Fivetran mock-server based integration tests.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| openmetadata-ui/src/main/resources/ui/src/utils/AdvancedSearchUtils.tsx | Replaces HTML-string highlight with React nodes (removes dangerouslySetInnerHTML). |
| openmetadata-ui/src/main/resources/ui/src/utils/AdvancedSearchUtils.test.tsx | Updates tests for React-node highlighting and adds XSS regression coverage. |
| openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/FeedEditor/FeedEditor.tsx | Avoids string-built HTML for mention items by using DOM APIs and textContent. |
| openmetadata-spec/src/main/resources/json/schema/security/client/samlSSOClientConfig.json | Makes SAML security defaults stricter (potentially breaking). |
| openmetadata-service/src/main/java/org/openmetadata/service/security/saml/SamlLoginServlet.java | Adds redirect allowlist validation and stores SAML request ID in session. |
| openmetadata-service/src/main/java/org/openmetadata/service/security/auth/SamlAuthServletHandler.java | Adds redirect allowlist validation + request ID validation on callback. |
| openmetadata-service/src/main/java/org/openmetadata/service/security/RedirectUriValidator.java | New allowlist validator for post-auth redirect URIs. |
| openmetadata-service/src/main/java/org/openmetadata/service/security/AuthenticationCodeFlowHandler.java | Adds allowlist enforcement for OIDC redirect URIs and reduces sensitive logging. |
| openmetadata-service/src/main/java/org/openmetadata/service/security/jwt/JWTTokenGenerator.java | Sets token type based on user.isBot instead of always treating as BOT. |
| openmetadata-service/src/main/java/org/openmetadata/service/security/DefaultAuthorizer.java | Ensures impersonation authorization is checked for admin/admin-or-bot endpoints too. |
| openmetadata-service/src/test/java/org/openmetadata/service/security/auth/SamlAuthServletHandlerTest.java | Updates SAML handler tests for new redirect validation/session usage. |
| openmetadata-service/src/test/java/org/openmetadata/service/security/AuthenticationCodeFlowHandlerTest.java | Adjusts expected redirect URI in a test. |
| conf/openmetadata.yaml | Updates sample SAML security defaults to true. |
| docker/development/distributed-test/local/server1.yaml | Updates sample SAML security defaults to true. |
| docker/development/distributed-test/local/server2.yaml | Updates sample SAML security defaults to true. |
| docker/development/distributed-test/local/server3.yaml | Updates sample SAML security defaults to true. |
| ingestion/tests/integration/fivetran/conftest.py | Adds mock HTTP server + fixtures for Fivetran integration tests. |
| ingestion/tests/integration/fivetran/test_fivetran_client.py | Adds Fivetran client integration tests (pagination, HVA scenario, etc.). |
| ingestion/tests/integration/fivetran/init.py | Marks the new integration test directory as a package. |
| """ | ||
| Fivetran integration tests using a mock HTTP server | ||
| """ | ||
| import pytest | ||
|
|
||
| from metadata.generated.schema.entity.services.connections.pipeline.fivetranConnection import ( | ||
| FivetranConnection, | ||
| ) | ||
| from metadata.ingestion.source.pipeline.fivetran.client import FivetranClient |
There was a problem hiding this comment.
This file introduces new Fivetran ingestion integration tests, which appears unrelated to the PR’s stated focus (“Improvements to SSO”). Please split these changes into a separate PR (or update the PR title/description to reflect the additional scope) to keep review, release notes, and rollback surface area manageable.
| String trustedCallback = SamlSettingsHolder.getInstance().getRelayState(); | ||
| String baseRequestUrl = buildBaseRequestUrl(request); | ||
| if (RedirectUriValidator.isSafe(redirectUri, trustedCallback, baseRequestUrl)) { | ||
| session.setAttribute(SESSION_REDIRECT_URI, redirectUri); |
There was a problem hiding this comment.
SamlSettingsHolder.getInstance() returns a new holder instance each call, and relayState is only set inside initDefaultSettings() on a different instance. As a result getInstance().getRelayState() will always be null here, so the allowlist check effectively falls back to buildBaseRequestUrl(request) only (and may allow unintended redirects). Use the configured SAML callback/relayState from the active auth config (or make relayState static/singleton) instead of reading it from a fresh instance.
| if (callbackUrl != null) { | ||
| req.getSession(true).setAttribute(SESSION_REDIRECT_URI, callbackUrl); | ||
| String trustedCallback = SamlSettingsHolder.getInstance().getRelayState(); | ||
| if (RedirectUriValidator.isSafe(callbackUrl, trustedCallback, buildBaseRequestUrl(req))) { | ||
| session.setAttribute(SESSION_REDIRECT_URI, callbackUrl); |
There was a problem hiding this comment.
Using buildBaseRequestUrl(req) (derived from the incoming request host/scheme) as an allowlisted base URL is unsafe: the Host header can be attacker-controlled, which can make an external callbackUrl appear “same-origin” and bypass the redirect allowlist. This should validate against a configured/public base URL (e.g., serverUrl / configured SAML callback origin), not a value constructed from the request.
| String trustedCallback = SamlSettingsHolder.getInstance().getRelayState(); | ||
| if (RedirectUriValidator.isSafe(callbackUrl, trustedCallback, buildBaseRequestUrl(req))) { |
There was a problem hiding this comment.
SamlSettingsHolder.getInstance().getRelayState() will always be null because getInstance() constructs a new holder and relayState is only set during initDefaultSettings() on another instance. This means the allowlist is not actually anchored to the configured relayState/callback. Please fetch relayState from the configured SAML settings (or refactor SamlSettingsHolder to retain relayState) before relying on it for redirect validation.
| String scheme = uri.getScheme(); | ||
| String host = uri.getHost(); | ||
| if (scheme == null | ||
| || host == null | ||
| || !ALLOWED_SCHEMES.contains(scheme.toLowerCase(Locale.ROOT))) { | ||
| LOG.warn("Rejecting redirect URI with unsupported scheme or missing host"); | ||
| return false; | ||
| } | ||
| for (String base : allowedBaseUrls) { | ||
| if (CommonUtil.nullOrEmpty(base)) { | ||
| continue; | ||
| } | ||
| try { | ||
| URI baseUri = new URI(base); | ||
| if (baseUri.getHost() == null) { | ||
| continue; | ||
| } | ||
| if (scheme.equalsIgnoreCase(baseUri.getScheme()) | ||
| && host.equalsIgnoreCase(baseUri.getHost()) | ||
| && uri.getPort() == baseUri.getPort()) { | ||
| return true; |
There was a problem hiding this comment.
Port matching uses uri.getPort() == baseUri.getPort(), which treats an omitted port as -1. This will incorrectly reject equivalent URLs like https://host/path (port -1) vs https://host:443/path (port 443), and similarly for http/80. Normalize -1 to the scheme’s default port (or compare effective ports) before deciding allowlist membership.
| public static boolean isSafe(String candidate, String... allowedBaseUrls) { | ||
| if (CommonUtil.nullOrEmpty(candidate)) { | ||
| return false; | ||
| } | ||
| if (candidate.startsWith("/") && !candidate.startsWith("//")) { | ||
| return true; | ||
| } | ||
| URI uri; | ||
| try { | ||
| uri = new URI(candidate); | ||
| } catch (URISyntaxException e) { | ||
| LOG.warn("Rejecting malformed redirect URI"); | ||
| return false; | ||
| } | ||
| String scheme = uri.getScheme(); | ||
| String host = uri.getHost(); | ||
| if (scheme == null | ||
| || host == null | ||
| || !ALLOWED_SCHEMES.contains(scheme.toLowerCase(Locale.ROOT))) { | ||
| LOG.warn("Rejecting redirect URI with unsupported scheme or missing host"); | ||
| return false; | ||
| } | ||
| for (String base : allowedBaseUrls) { | ||
| if (CommonUtil.nullOrEmpty(base)) { | ||
| continue; | ||
| } | ||
| try { | ||
| URI baseUri = new URI(base); | ||
| if (baseUri.getHost() == null) { | ||
| continue; | ||
| } | ||
| if (scheme.equalsIgnoreCase(baseUri.getScheme()) | ||
| && host.equalsIgnoreCase(baseUri.getHost()) | ||
| && uri.getPort() == baseUri.getPort()) { | ||
| return true; | ||
| } | ||
| } catch (URISyntaxException ignored) { | ||
| // Try next base URL. | ||
| } | ||
| } | ||
| LOG.warn("Rejecting redirect URI (host not in allowlist)"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
RedirectUriValidator is a new security-critical utility but there are no unit tests covering key cases (e.g., absolute URL allowlisted vs rejected, default-port normalization, malformed URIs, scheme-relative // URLs). Adding focused unit tests would prevent regressions and ensure the open-redirect protections behave as intended.
| @@ -107,12 +107,12 @@ | |||
| "wantMessagesSigned": { | |||
| "description": "SP requires the messages received to be signed.", | |||
| "type": "boolean", | |||
| "default": false | |||
| "default": true | |||
| }, | |||
| "wantAssertionsSigned": { | |||
| "description": "SP requires the assertions received to be signed.", | |||
| "type": "boolean", | |||
| "default": false | |||
| "default": true | |||
There was a problem hiding this comment.
Changing the default values of strictMode, wantMessagesSigned, and wantAssertionsSigned from false to true is a behavior change that can break existing SAML setups that relied on the old defaults. This should be called out explicitly in the PR description (and potentially treated as a backward-incompatible change / documented in release notes), otherwise operators may be surprised by failed SSO after upgrade.
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
Jest test CoverageUI tests summary
|
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
|
|
🔴 Playwright Results — 1 failure(s), 29 flaky✅ 3651 passed · ❌ 1 failed · 🟡 29 flaky · ⏭️ 89 skipped
Genuine Failures (failed on all attempts)❌
|
Describe your changes:
Fixes
I worked on ... because ...
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>