mirror of
https://github.com/andwati/zitadel.git
synced 2026-11-09 23:55:12 +00:00
fix(login): don't prepend base path to absolute IdP URL on login_hint redirect (#12610)
# Which Problems Are Solved When an OIDC authorize request carries a `login_hint` that resolves via domain discovery to an organization configured to auto-redirect to a single external IdP, Login V2's server-side flow initiation returns a malformed `Location` header: the login base path is concatenated with the absolute IdP authorize URL without a separator (e.g. `https://<instance>/ui/v2/loginhttps://login.microsoftonline.com/...`). The flow ends in `{"code":5,"message":"Not Found"}` and the IdP is never contacted. The same IdP works when the user submits the login name interactively, because the client-side handler distinguishes external URLs; only the server-side `login_hint` fast-path introduced in #12431 is affected. Additionally, the same fast-path only handled the `redirect` response shape from `sendLoginname`. When the discovered organization's IdP is SAML with POST binding, `sendLoginname` returns `samlData` (form fields instead of a URL), which was silently ignored — the user fell back to the prefilled `/loginname` screen instead of being signed in silently, unlike the client-side flow, which auto-submits the SAML AuthnRequest form. # How the Problems Are Solved `resolveLoginHint` passed every redirect returned by `sendLoginname` through `constructUrl`, which unconditionally prepends `NEXT_PUBLIC_BASE_PATH` and assumes a relative path. It now checks `isExternalUrl` first: absolute URLs are validated with `isSafeRedirectUri` (blocking `javascript:`/`data:`/etc., falling back to `/loginname` if unsafe) and redirected as-is, while relative paths keep being resolved against the base path — matching the handling already used in the idp-scope branch of `handleOIDCFlowInitiation` and in the client-side `handleServerActionResponse`. `resolveLoginHint` also handles the `samlData` response shape now: after validating the target URL with `isSafeRedirectUri`, it responds with the same auto-submit HTML form used by the existing SAML flows, so a `login_hint` resolving to a POST-binding SAML IdP completes silently as well. Since this form was previously duplicated inline three times in `flow-initiation.ts`, it is extracted into a shared `buildAutoSubmitFormResponse` helper used by all call sites. Regression tests cover the absolute-URL redirect (fails on the previous code with the exact malformed URL), the SAML POST auto-submit response, and the unsafe-scheme fallbacks for both shapes.
This commit is contained in:
@@ -533,6 +533,117 @@ describe("handleOIDCFlowInitiation — org-scoped session filtering", () => {
|
||||
expect(location).toContain("/password");
|
||||
expect(location).not.toContain("/loginname");
|
||||
});
|
||||
|
||||
test("should redirect to an absolute IdP URL as-is without prepending the base path (domain discovery auto-redirect)", async () => {
|
||||
mockGetAuthRequest.mockResolvedValue({
|
||||
authRequest: {
|
||||
id: "abc123",
|
||||
uiLocales: [],
|
||||
scope: [],
|
||||
prompt: [],
|
||||
loginHint: "[email protected]",
|
||||
},
|
||||
});
|
||||
|
||||
// Simulate a base path being configured, as in ZITADEL Cloud (/ui/v2/login).
|
||||
mockConstructUrl.mockImplementation((_req: any, path: string) => {
|
||||
return new URL(`https://example.com/ui/v2/login${path}`);
|
||||
});
|
||||
|
||||
// sendLoginname resolved the hint via domain discovery to an org with a
|
||||
// single external IdP and returns the absolute authorize URL of that IdP.
|
||||
const idpUrl = "https://login.microsoftonline.com/tenant-id/oauth2/v2.0/authorize?client_id=xyz&state=abc";
|
||||
mockSendLoginname.mockResolvedValue({ redirect: idpUrl });
|
||||
|
||||
const res = await handleOIDCFlowInitiation(makeBaseParams({ sessions: [] }));
|
||||
|
||||
const location = res.headers.get("location") ?? "";
|
||||
expect(location).toBe(idpUrl);
|
||||
// The base path must never be glued onto an absolute URL
|
||||
// (regression: https://<host>/ui/v2/loginhttps://login.microsoftonline.com/...).
|
||||
expect(location).not.toContain("/ui/v2/login");
|
||||
expect(mockConstructUrl).not.toHaveBeenCalledWith(expect.anything(), idpUrl);
|
||||
});
|
||||
|
||||
test("should render an auto-submit form when loginHint resolves to a SAML POST-binding IdP", async () => {
|
||||
mockGetAuthRequest.mockResolvedValue({
|
||||
authRequest: {
|
||||
id: "abc123",
|
||||
uiLocales: [],
|
||||
scope: [],
|
||||
prompt: [],
|
||||
loginHint: "[email protected]",
|
||||
},
|
||||
});
|
||||
|
||||
// sendLoginname resolved the hint via domain discovery to an org whose
|
||||
// single IdP is SAML with POST binding: the AuthnRequest is delivered as
|
||||
// form fields, not a redirect URL.
|
||||
mockSendLoginname.mockResolvedValue({
|
||||
samlData: {
|
||||
url: "https://adfs.example.com/adfs/ls",
|
||||
fields: { SAMLRequest: "PHNhbWxwOkF1dGhuUmVxdWVzdD4=", RelayState: "relay-123" },
|
||||
},
|
||||
});
|
||||
|
||||
const res = await handleOIDCFlowInitiation(makeBaseParams({ sessions: [] }));
|
||||
|
||||
// No redirect: the response is an HTML page auto-posting the form to the IdP.
|
||||
expect(res.headers.get("location")).toBeNull();
|
||||
expect(res.headers.get("content-type")).toContain("text/html");
|
||||
|
||||
const html = await res.text();
|
||||
expect(html).toContain('action="https://adfs.example.com/adfs/ls"');
|
||||
expect(html).toContain('name="SAMLRequest"');
|
||||
expect(html).toContain('value="PHNhbWxwOkF1dGhuUmVxdWVzdD4="');
|
||||
expect(html).toContain('name="RelayState"');
|
||||
expect(html).toContain("document.forms[0].submit()");
|
||||
});
|
||||
|
||||
test("should block unsafe SAML post URLs from loginHint resolution and fall back to /loginname", async () => {
|
||||
mockGetAuthRequest.mockResolvedValue({
|
||||
authRequest: {
|
||||
id: "abc123",
|
||||
uiLocales: [],
|
||||
scope: [],
|
||||
prompt: [],
|
||||
loginHint: "[email protected]",
|
||||
},
|
||||
});
|
||||
|
||||
mockSendLoginname.mockResolvedValue({
|
||||
samlData: {
|
||||
url: "javascript:alert(1)",
|
||||
fields: { SAMLRequest: "abc" },
|
||||
},
|
||||
});
|
||||
|
||||
const res = await handleOIDCFlowInitiation(makeBaseParams({ sessions: [] }));
|
||||
|
||||
const location = res.headers.get("location") ?? "";
|
||||
expect(location).toContain("/loginname");
|
||||
expect(location).not.toContain("javascript:");
|
||||
});
|
||||
|
||||
test("should block unsafe absolute redirect URLs from loginHint resolution and fall back to /loginname", async () => {
|
||||
mockGetAuthRequest.mockResolvedValue({
|
||||
authRequest: {
|
||||
id: "abc123",
|
||||
uiLocales: [],
|
||||
scope: [],
|
||||
prompt: [],
|
||||
loginHint: "[email protected]",
|
||||
},
|
||||
});
|
||||
|
||||
mockSendLoginname.mockResolvedValue({ redirect: "javascript:alert(1)" });
|
||||
|
||||
const res = await handleOIDCFlowInitiation(makeBaseParams({ sessions: [] }));
|
||||
|
||||
const location = res.headers.get("location") ?? "";
|
||||
expect(location).toContain("/loginname");
|
||||
expect(location).not.toContain("javascript:");
|
||||
});
|
||||
});
|
||||
|
||||
describe("handleOIDCFlowInitiation — Prompt.LOGIN + loginHint requestId prefix", () => {
|
||||
@@ -634,3 +745,130 @@ describe("handleOIDCFlowInitiation — Prompt.LOGIN + loginHint requestId prefix
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("handleOIDCFlowInitiation — idp scope (urn:zitadel:iam:org:idp:id)", () => {
|
||||
let mockGetAuthRequest: ReturnType<typeof vi.fn>;
|
||||
let mockConstructUrl: ReturnType<typeof vi.fn>;
|
||||
let mockGetActiveIdentityProviders: ReturnType<typeof vi.fn>;
|
||||
let mockStartIdentityProviderFlow: ReturnType<typeof vi.fn>;
|
||||
let mockIdpTypeToSlug: ReturnType<typeof vi.fn>;
|
||||
|
||||
beforeEach(async () => {
|
||||
vi.clearAllMocks();
|
||||
vi.unstubAllEnvs();
|
||||
|
||||
const zitadel = await import("@/lib/zitadel");
|
||||
const serviceUrl = await import("@/lib/service-url");
|
||||
const authUtils = await import("@/lib/auth-utils");
|
||||
const idpLib = await import("@/lib/idp");
|
||||
|
||||
mockGetAuthRequest = vi.mocked(zitadel.getAuthRequest);
|
||||
mockConstructUrl = vi.mocked(serviceUrl.constructUrl);
|
||||
mockGetActiveIdentityProviders = vi.mocked(zitadel.getActiveIdentityProviders);
|
||||
mockStartIdentityProviderFlow = vi.mocked(zitadel.startIdentityProviderFlow);
|
||||
mockIdpTypeToSlug = vi.mocked(idpLib.idpTypeToSlug);
|
||||
vi.mocked(authUtils.getValidLocaleFromUILocales).mockReturnValue(null);
|
||||
|
||||
mockConstructUrl.mockImplementation((_req: any, path: string) => {
|
||||
return new URL(`https://example.com${path}`);
|
||||
});
|
||||
mockIdpTypeToSlug.mockReturnValue("azure");
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.unstubAllEnvs();
|
||||
});
|
||||
|
||||
test("should use the type of the scoped IdP, not the first active IdP (multi-IdP org)", async () => {
|
||||
const { IdentityProviderType } = await import("@zitadel/proto/zitadel/settings/v2/login_settings_pb");
|
||||
|
||||
mockGetAuthRequest.mockResolvedValue({
|
||||
authRequest: {
|
||||
id: "abc123",
|
||||
uiLocales: [],
|
||||
scope: ["urn:zitadel:iam:org:idp:id:idp-2"],
|
||||
prompt: [],
|
||||
loginHint: undefined,
|
||||
},
|
||||
});
|
||||
|
||||
// Two active IdPs: the scope selects the SECOND one. Regression: the slug
|
||||
// was previously derived from identityProviders[0].type.
|
||||
mockGetActiveIdentityProviders.mockResolvedValue({
|
||||
identityProviders: [
|
||||
{ id: "idp-1", type: IdentityProviderType.GITHUB },
|
||||
{ id: "idp-2", type: IdentityProviderType.AZURE_AD },
|
||||
],
|
||||
});
|
||||
|
||||
mockStartIdentityProviderFlow.mockResolvedValue({
|
||||
url: "https://login.microsoftonline.com/tenant/oauth2/v2.0/authorize?client_id=xyz",
|
||||
});
|
||||
|
||||
const res = await handleOIDCFlowInitiation(makeBaseParams({ sessions: [] }));
|
||||
|
||||
expect(mockIdpTypeToSlug).toHaveBeenCalledWith(IdentityProviderType.AZURE_AD);
|
||||
expect(mockIdpTypeToSlug).not.toHaveBeenCalledWith(IdentityProviderType.GITHUB);
|
||||
expect(res.headers.get("location")).toBe("https://login.microsoftonline.com/tenant/oauth2/v2.0/authorize?client_id=xyz");
|
||||
});
|
||||
|
||||
test("should block unsafe IdP URLs with a 400 instead of redirecting or rendering a form", async () => {
|
||||
const { IdentityProviderType } = await import("@zitadel/proto/zitadel/settings/v2/login_settings_pb");
|
||||
|
||||
mockGetAuthRequest.mockResolvedValue({
|
||||
authRequest: {
|
||||
id: "abc123",
|
||||
uiLocales: [],
|
||||
scope: ["urn:zitadel:iam:org:idp:id:idp-1"],
|
||||
prompt: [],
|
||||
loginHint: undefined,
|
||||
},
|
||||
});
|
||||
|
||||
mockGetActiveIdentityProviders.mockResolvedValue({
|
||||
identityProviders: [{ id: "idp-1", type: IdentityProviderType.SAML }],
|
||||
});
|
||||
|
||||
mockStartIdentityProviderFlow.mockResolvedValue({
|
||||
url: "javascript:alert(1)",
|
||||
fields: { SAMLRequest: "abc" },
|
||||
});
|
||||
|
||||
const res = await handleOIDCFlowInitiation(makeBaseParams({ sessions: [] }));
|
||||
|
||||
expect(res.status).toBe(400);
|
||||
const body = await res.json();
|
||||
expect(body.error).toContain("Unsafe redirect URI");
|
||||
});
|
||||
|
||||
test("should render the auto-submit form for a scoped SAML POST-binding IdP", async () => {
|
||||
const { IdentityProviderType } = await import("@zitadel/proto/zitadel/settings/v2/login_settings_pb");
|
||||
|
||||
mockGetAuthRequest.mockResolvedValue({
|
||||
authRequest: {
|
||||
id: "abc123",
|
||||
uiLocales: [],
|
||||
scope: ["urn:zitadel:iam:org:idp:id:idp-1"],
|
||||
prompt: [],
|
||||
loginHint: undefined,
|
||||
},
|
||||
});
|
||||
|
||||
mockGetActiveIdentityProviders.mockResolvedValue({
|
||||
identityProviders: [{ id: "idp-1", type: IdentityProviderType.SAML }],
|
||||
});
|
||||
|
||||
mockStartIdentityProviderFlow.mockResolvedValue({
|
||||
url: "https://adfs.example.com/adfs/ls",
|
||||
fields: { SAMLRequest: "PHNhbWxwOkF1dGhuUmVxdWVzdD4=", RelayState: "relay-123" },
|
||||
});
|
||||
|
||||
const res = await handleOIDCFlowInitiation(makeBaseParams({ sessions: [] }));
|
||||
|
||||
expect(res.headers.get("location")).toBeNull();
|
||||
expect(res.headers.get("content-type")).toContain("text/html");
|
||||
const html = await res.text();
|
||||
expect(html).toContain('action="https://adfs.example.com/adfs/ls"');
|
||||
expect(html).toContain('name="SAMLRequest"');
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user