mirror of https://github.com/windmill-labs/windmill.git synced 2026-09-08 16:03:27 +00:00

Files

T

Ruben Fiszelandwindmill-internal-app[bot] 78cf6c7f81 fix(saml): preserve deep links from /a/[...path] across SAML round-trip (#9259 )

* [ee] fix(saml): preserve deep links from /a/[...path] across SAML round-trip

Fixes WIN-1962.

PR #9225 only covered users who pass through /user/login on their way to
the IdP — that's where `redirectSaml()` runs and where the deep link gets
stuffed into `RelayState`. The reported flow doesn't go through that
page: it hits `/a/[...path]` (the public-app custom-path route, outside
the `(logged)` layout) where `PublicApp.svelte` renders its own `<Login>`
and was passing `page.url.toString()` as `rd` — the full URL.

Three problems compounded:

1. `redirectSaml()` only set `RelayState` when `rd.startsWith('/')`,
   so a full URL silently fell through and the deep link was lost.
   The IdP echoed back the SP-library default (BASE_URL), which the
   ACS validator correctly rejected as a potential open-redirect.
2. `persistRd()` stored the full URL in `localStorage.rd`. On the
   fallback landing at `/user/login`, the post-login redirect saw
   an `http://...` value, hit the cross-origin branch, and bounced
   to `/` — which from a logged-in but workspace-less state shows
   the "Loading user…" modal forever (bug 2).
3. The EE `safe_relay_state_redirect` validator rejected any full
   URL, including same-origin ones, so even IdPs that prepend the
   origin or that pass a configured absolute deep link via
   IdP-initiated SSO got dropped on the floor.

The fix is a single concept applied at every layer: reduce a redirect
target to a safe same-origin relative path, or refuse it.

Frontend:
- `logoutRedirect.ts`: new `toSameOriginRelativePath(rd)` helper that
  accepts both `/foo` and `https://current-origin/foo`, with the same
  open-redirect guards as the backend (length cap, control chars, no
  protocol-relative or back-slash tricks). Returns `null` for
  cross-origin or malformed input.
- `PublicApp.svelte`: pass `pathname + search + hash` to `<Login>`
  instead of the full URL — this alone fixes the happy path.
- `Login.svelte`: `redirectSaml()`, `persistRd()`, and `redirectUser()`
  all route through the helper, so full URLs from `/a/[...path]` are
  reduced before being put in `RelayState`/`localStorage`/`goto()`.
- `/user/login/+page.svelte`: the same reduction is applied to the
  resolved `rd` so any stale full-URL value in `localStorage.rd` still
  navigates to the intended page instead of falling into the
  cross-origin branch.

Backend (EE companion: windmill-ee-private#TBD):
- `safe_relay_state_redirect` now reduces a `RelayState` whose origin
  matches `BASE_URL` to its path before applying the same-origin path
  safety rules. Bare BASE_URL with no path still falls back to
  `/user/login` (no useful deep link to honor).
- New `same_origin_relative_path` helper + expanded unit tests.

Test plan:
- [x] Frontend: `vitest run src/lib/logoutRedirect.test.ts` — 9 passed
- [x] Backend: `cargo test -p windmill-api ... saml_ee::tests` — 3 passed
  (`honors_same_origin_relative_path`, `reduces_same_origin_full_url_to_path`,
  `falls_back_on_open_redirect_attempts`)
- [ ] Manual e2e (needs configured SAML IdP — not on local CE):
  - Unauthenticated visit to `/a/<path>` → click SSO → SAML → land on
    `/a/<path>` (RelayState now carries the relative path).
  - IdP that echoes BASE_URL as default → ACS still falls back to
    `/user/login` (no useful path to honor), but the page no longer
    hangs: the stale full-URL `localStorage.rd` is reduced to its path
    and the post-login redirect navigates to it.
  - Tampered `RelayState` (`//evil.com`, `https://evil.com/x`) → ACS
    rejects, lands on `/user/login`.

* chore: update ee-repo-ref to 3489c243b0e5a8eb0dbc86e90917fbe72843573b

This commit updates the EE repository reference after PR #584 was merged in windmill-ee-private.

Previous ee-repo-ref: 635ff3eeb8e47bb84d5686942605f67f8f6224b4

New ee-repo-ref: 3489c243b0e5a8eb0dbc86e90917fbe72843573b

Automated by sync-ee-ref workflow.

---------

Co-authored-by: windmill-internal-app[bot] <windmill-internal-app[bot]@users.noreply.github.com>

2026-05-20 12:46:58 +00:00