From 699d102e4a34b3f8cce8c8bcde1cf484f83935eb Mon Sep 17 00:00:00 2001 From: Jonathan Kilzi Date: Mon, 22 Jun 2026 22:41:30 +0300 Subject: [PATCH 1/6] fix: restore OpenShift OAuth session logout on user sign-out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OpenShiftAuthHandler.Logout was returning ("", nil) since PR #393 (EDM-2612), which cleared only the FlightCtl session cookie but left the OpenShift OAuth browser session active. Because the login page auto-redirects when a single non-K8s provider is configured, users were silently re-authenticated immediately after clicking Logout. The original discovery-based approach ({apiServerURL}/.well-known/ oauth-authorization-server) was problematic when apiServerURL pointed to the K8s API server, producing https://api.cluster:6443/logout as the logout target — an invalid endpoint. Fix: derive the logout URL directly from authURL (the OAuth authorization endpoint). authURL always holds the OAuth server host regardless of how apiServerURL is configured, so taking scheme://host and appending /logout gives the correct https://oauth-openshift.apps.{cluster}/logout without any HTTP round-trip. Fixes https://github.com/flightctl/flightctl-ui/issues/708 Assisted-by: Cursor Signed-off-by: Jonathan Kilzi --- proxy/auth/helpers_test.go | 20 ++++++++++ proxy/auth/openshift.go | 23 ++++++++++-- proxy/auth/openshift_test.go | 73 ++++++++++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 3 deletions(-) create mode 100644 proxy/auth/helpers_test.go create mode 100644 proxy/auth/openshift_test.go diff --git a/proxy/auth/helpers_test.go b/proxy/auth/helpers_test.go new file mode 100644 index 000000000..76e4a0c22 --- /dev/null +++ b/proxy/auth/helpers_test.go @@ -0,0 +1,20 @@ +package auth + +import ( + "fmt" + "net/http" + "net/url" +) + +// redirectBaseMatchesRequest is a test helper that checks whether url u's origin +// matches the effective request origin. Returns nil when they match, error otherwise. +func redirectBaseMatchesRequest(r *http.Request, u *url.URL) error { + ok, err := isSameSchemeAndHost(u.String(), r) + if err != nil { + return err + } + if !ok { + return fmt.Errorf("origin mismatch: redirect base %q does not match request origin", u) + } + return nil +} diff --git a/proxy/auth/openshift.go b/proxy/auth/openshift.go index 07ccee50a..b501a28bc 100644 --- a/proxy/auth/openshift.go +++ b/proxy/auth/openshift.go @@ -123,9 +123,26 @@ func (o *OpenShiftAuthHandler) openshiftClientForRedirect(redirectURI string) (* return client, nil } -func (o *OpenShiftAuthHandler) Logout(token string, _ string) (string, error) { - // The cookie will be cleared by the proxy - return "", nil +func (o *OpenShiftAuthHandler) Logout(_ string, _ string) (string, error) { + // Derive the OpenShift OAuth server logout URL directly from the authorization + // URL (scheme + host only, path replaced with /logout). This terminates the + // user's OpenShift OAuth browser session so the login page's auto-redirect + // cannot silently re-authenticate them. + // + // Using authURL avoids a discovery HTTP round-trip and is robust: authURL + // always points to the OAuth server regardless of whether apiServerURL is + // the K8s API server or the OAuth server. The prior approach that called + // {apiServerURL}/.well-known/oauth-authorization-server was removed in + // EDM-2612 because it produced a K8s API server URL as the logout target + // (e.g. https://api.cluster:6443/logout) which is not a valid endpoint. + if o.authURL == "" { + return "", nil + } + parsed, err := url.Parse(o.authURL) + if err != nil || parsed.Host == "" { + return "", nil + } + return parsed.Scheme + "://" + parsed.Host + "/logout", nil } func (o *OpenShiftAuthHandler) GetLoginRedirectURL(state string, codeChallenge string, redirectURI string) (string, error) { diff --git a/proxy/auth/openshift_test.go b/proxy/auth/openshift_test.go new file mode 100644 index 000000000..56280ca23 --- /dev/null +++ b/proxy/auth/openshift_test.go @@ -0,0 +1,73 @@ +package auth + +import ( + "testing" +) + +func TestOpenShiftLogout_ReturnsLogoutURLDerivedFromAuthURL(t *testing.T) { + t.Parallel() + + handler := &OpenShiftAuthHandler{ + authURL: "https://oauth-openshift.apps.cluster.example.com/oauth/authorize", + } + + logoutURL, err := handler.Logout("test-token", "https://ui.example.com") + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + + expected := "https://oauth-openshift.apps.cluster.example.com/logout" + if logoutURL != expected { + t.Fatalf("expected %q, got %q", expected, logoutURL) + } +} + +func TestOpenShiftLogout_StripsExistingPathAndQuery(t *testing.T) { + t.Parallel() + + handler := &OpenShiftAuthHandler{ + authURL: "https://oauth-openshift.apps.cluster.example.com/oauth/authorize?response_type=code", + } + + logoutURL, err := handler.Logout("test-token", "") + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + + expected := "https://oauth-openshift.apps.cluster.example.com/logout" + if logoutURL != expected { + t.Fatalf("expected %q, got %q", expected, logoutURL) + } +} + +func TestOpenShiftLogout_FallsBackGracefullyWhenAuthURLIsEmpty(t *testing.T) { + t.Parallel() + + handler := &OpenShiftAuthHandler{ + authURL: "", + } + + logoutURL, err := handler.Logout("test-token", "https://ui.example.com") + if err != nil { + t.Fatalf("expected no error on empty authURL, got: %v", err) + } + if logoutURL != "" { + t.Fatalf("expected empty logout URL when authURL is empty, got: %q", logoutURL) + } +} + +func TestOpenShiftLogout_FallsBackGracefullyWhenAuthURLIsInvalid(t *testing.T) { + t.Parallel() + + handler := &OpenShiftAuthHandler{ + authURL: "://not-a-valid-url", + } + + logoutURL, err := handler.Logout("test-token", "https://ui.example.com") + if err != nil { + t.Fatalf("expected no error on invalid authURL, got: %v", err) + } + if logoutURL != "" { + t.Fatalf("expected empty logout URL for invalid authURL, got: %q", logoutURL) + } +} From 9cdadf4ea424d078bb9e61b986c29e6451aa00e4 Mon Sep 17 00:00:00 2001 From: Jonathan Kilzi Date: Tue, 23 Jun 2026 16:28:35 +0300 Subject: [PATCH 2/6] fix: address code review on PR #709 - Add parsed.Scheme guard to prevent malformed logout URL for scheme-less inputs like "//host/path" (url.Parse returns Host but empty Scheme) - Use url.URL struct to build logout URL instead of string concatenation - Add *testing.T + t.Helper() to redirectBaseMatchesRequest; log only scheme://host in error message to avoid leaking query parameters - Consolidate four separate Logout test functions into a single table-driven TestOpenShiftLogout per project conventions - Add scheme-less auth URL regression test case to TestOpenShiftLogout - Add doc comments to openshiftClientForRedirect, Logout, and GetLoginRedirectURL to improve docstring coverage - Add comment explaining intentional url.Parse error swallow fallback Assisted-by: Cursor Signed-off-by: Jonathan Kilzi Co-authored-by: Cursor --- proxy/auth/helpers_test.go | 6 +- proxy/auth/openshift.go | 35 +++++++----- proxy/auth/openshift_test.go | 108 +++++++++++++++-------------------- proxy/auth/redirect_test.go | 6 +- 4 files changed, 74 insertions(+), 81 deletions(-) diff --git a/proxy/auth/helpers_test.go b/proxy/auth/helpers_test.go index 76e4a0c22..cd02bc4df 100644 --- a/proxy/auth/helpers_test.go +++ b/proxy/auth/helpers_test.go @@ -4,17 +4,19 @@ import ( "fmt" "net/http" "net/url" + "testing" ) // redirectBaseMatchesRequest is a test helper that checks whether url u's origin // matches the effective request origin. Returns nil when they match, error otherwise. -func redirectBaseMatchesRequest(r *http.Request, u *url.URL) error { +func redirectBaseMatchesRequest(t *testing.T, r *http.Request, u *url.URL) error { + t.Helper() ok, err := isSameSchemeAndHost(u.String(), r) if err != nil { return err } if !ok { - return fmt.Errorf("origin mismatch: redirect base %q does not match request origin", u) + return fmt.Errorf("origin mismatch: redirect base %s://%s does not match request origin", u.Scheme, u.Host) } return nil } diff --git a/proxy/auth/openshift.go b/proxy/auth/openshift.go index b501a28bc..ab1d988d1 100644 --- a/proxy/auth/openshift.go +++ b/proxy/auth/openshift.go @@ -47,6 +47,9 @@ func getOpenShiftAuthHandlerFromSpec(provider *v1beta1.AuthProvider, openshiftSp } apiServerURL = parsedURL.String() } else { + // url.Parse should not fail on a URL already validated by the API + // server; fall back to the raw string to preserve backward + // compatibility rather than failing provider initialization. apiServerURL = authURL } } else { @@ -101,6 +104,8 @@ func getOpenShiftAuthHandlerFromSpec(provider *v1beta1.AuthProvider, openshiftSp return handler, nil } +// openshiftClientForRedirect creates an osincli OAuth2 client configured for +// the given redirect URI using the handler's stored credentials and TLS config. func (o *OpenShiftAuthHandler) openshiftClientForRedirect(redirectURI string) (*osincli.Client, error) { oauthClientConfig := &osincli.ClientConfig{ ClientId: o.clientId, @@ -123,28 +128,32 @@ func (o *OpenShiftAuthHandler) openshiftClientForRedirect(redirectURI string) (* return client, nil } +// Logout derives the OpenShift OAuth server logout URL from the authorization +// URL (scheme + host only, path replaced with /logout). This terminates the +// user's OpenShift OAuth browser session so the login page's auto-redirect +// cannot silently re-authenticate them. +// +// Using authURL avoids a discovery HTTP round-trip and is robust: authURL +// always points to the OAuth server regardless of whether apiServerURL is +// the K8s API server or the OAuth server. The prior approach that called +// {apiServerURL}/.well-known/oauth-authorization-server was removed in +// EDM-2612 because it produced a K8s API server URL as the logout target +// (e.g. https://api.cluster:6443/logout) which is not a valid endpoint. +// +// Returns ("", nil) when authURL is empty, unparseable, or missing scheme/host. func (o *OpenShiftAuthHandler) Logout(_ string, _ string) (string, error) { - // Derive the OpenShift OAuth server logout URL directly from the authorization - // URL (scheme + host only, path replaced with /logout). This terminates the - // user's OpenShift OAuth browser session so the login page's auto-redirect - // cannot silently re-authenticate them. - // - // Using authURL avoids a discovery HTTP round-trip and is robust: authURL - // always points to the OAuth server regardless of whether apiServerURL is - // the K8s API server or the OAuth server. The prior approach that called - // {apiServerURL}/.well-known/oauth-authorization-server was removed in - // EDM-2612 because it produced a K8s API server URL as the logout target - // (e.g. https://api.cluster:6443/logout) which is not a valid endpoint. if o.authURL == "" { return "", nil } parsed, err := url.Parse(o.authURL) - if err != nil || parsed.Host == "" { + if err != nil || parsed.Scheme == "" || parsed.Host == "" { return "", nil } - return parsed.Scheme + "://" + parsed.Host + "/logout", nil + return (&url.URL{Scheme: parsed.Scheme, Host: parsed.Host, Path: "/logout"}).String(), nil } +// GetLoginRedirectURL returns the OAuth2 authorization URL the browser should +// be redirected to in order to initiate the OpenShift login flow. func (o *OpenShiftAuthHandler) GetLoginRedirectURL(state string, codeChallenge string, redirectURI string) (string, error) { client, err := o.openshiftClientForRedirect(redirectURI) if err != nil { diff --git a/proxy/auth/openshift_test.go b/proxy/auth/openshift_test.go index 56280ca23..e06add00f 100644 --- a/proxy/auth/openshift_test.go +++ b/proxy/auth/openshift_test.go @@ -4,70 +4,52 @@ import ( "testing" ) -func TestOpenShiftLogout_ReturnsLogoutURLDerivedFromAuthURL(t *testing.T) { +func TestOpenShiftLogout(t *testing.T) { t.Parallel() - handler := &OpenShiftAuthHandler{ - authURL: "https://oauth-openshift.apps.cluster.example.com/oauth/authorize", - } - - logoutURL, err := handler.Logout("test-token", "https://ui.example.com") - if err != nil { - t.Fatalf("expected no error, got: %v", err) - } - - expected := "https://oauth-openshift.apps.cluster.example.com/logout" - if logoutURL != expected { - t.Fatalf("expected %q, got %q", expected, logoutURL) - } -} - -func TestOpenShiftLogout_StripsExistingPathAndQuery(t *testing.T) { - t.Parallel() - - handler := &OpenShiftAuthHandler{ - authURL: "https://oauth-openshift.apps.cluster.example.com/oauth/authorize?response_type=code", - } - - logoutURL, err := handler.Logout("test-token", "") - if err != nil { - t.Fatalf("expected no error, got: %v", err) - } - - expected := "https://oauth-openshift.apps.cluster.example.com/logout" - if logoutURL != expected { - t.Fatalf("expected %q, got %q", expected, logoutURL) - } -} - -func TestOpenShiftLogout_FallsBackGracefullyWhenAuthURLIsEmpty(t *testing.T) { - t.Parallel() - - handler := &OpenShiftAuthHandler{ - authURL: "", - } - - logoutURL, err := handler.Logout("test-token", "https://ui.example.com") - if err != nil { - t.Fatalf("expected no error on empty authURL, got: %v", err) - } - if logoutURL != "" { - t.Fatalf("expected empty logout URL when authURL is empty, got: %q", logoutURL) - } -} - -func TestOpenShiftLogout_FallsBackGracefullyWhenAuthURLIsInvalid(t *testing.T) { - t.Parallel() - - handler := &OpenShiftAuthHandler{ - authURL: "://not-a-valid-url", - } - - logoutURL, err := handler.Logout("test-token", "https://ui.example.com") - if err != nil { - t.Fatalf("expected no error on invalid authURL, got: %v", err) - } - if logoutURL != "" { - t.Fatalf("expected empty logout URL for invalid authURL, got: %q", logoutURL) + tests := []struct { + name string + authURL string + wantURL string + }{ + { + name: "derives logout URL from auth URL", + authURL: "https://oauth-openshift.apps.cluster.example.com/oauth/authorize", + wantURL: "https://oauth-openshift.apps.cluster.example.com/logout", + }, + { + name: "strips path and query", + authURL: "https://oauth-openshift.apps.cluster.example.com/oauth/authorize?response_type=code", + wantURL: "https://oauth-openshift.apps.cluster.example.com/logout", + }, + { + name: "empty auth URL falls back", + authURL: "", + wantURL: "", + }, + { + name: "invalid auth URL falls back", + authURL: "://not-a-valid-url", + wantURL: "", + }, + { + name: "scheme-less auth URL falls back", + authURL: "//oauth-openshift.apps.cluster.example.com/oauth/authorize", + wantURL: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + handler := &OpenShiftAuthHandler{authURL: tt.authURL} + got, err := handler.Logout("test-token", "https://ui.example.com") + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + if got != tt.wantURL { + t.Fatalf("expected %q, got %q", tt.wantURL, got) + } + }) } } diff --git a/proxy/auth/redirect_test.go b/proxy/auth/redirect_test.go index 5a556da44..a31d2cb74 100644 --- a/proxy/auth/redirect_test.go +++ b/proxy/auth/redirect_test.go @@ -15,7 +15,7 @@ func TestRedirectBaseMatchesRequest_NormalizesDefaultHTTPSPort(t *testing.T) { r.Host = "ui.example.com:443" u := &url.URL{Scheme: "https", Host: "ui.example.com"} - if err := redirectBaseMatchesRequest(r, u); err != nil { + if err := redirectBaseMatchesRequest(t, r, u); err != nil { t.Fatalf("expected origins to match after normalization, got error: %v", err) } } @@ -27,7 +27,7 @@ func TestRedirectBaseMatchesRequest_NormalizesDefaultHTTPPort(t *testing.T) { r.Host = "ui.example.com:80" u := &url.URL{Scheme: "http", Host: "ui.example.com"} - if err := redirectBaseMatchesRequest(r, u); err != nil { + if err := redirectBaseMatchesRequest(t, r, u); err != nil { t.Fatalf("expected origins to match after normalization, got error: %v", err) } } @@ -40,7 +40,7 @@ func TestRedirectBaseMatchesRequest_RejectsNonDefaultPortMismatch(t *testing.T) r.Host = "ui.example.com:8443" u := &url.URL{Scheme: "https", Host: "ui.example.com"} - if err := redirectBaseMatchesRequest(r, u); err == nil { + if err := redirectBaseMatchesRequest(t, r, u); err == nil { t.Fatal("expected non-default port mismatch to be rejected") } } From 6257b75c2a9366f2b4ec1f95b86567581cae281d Mon Sep 17 00:00:00 2001 From: Jonathan Kilzi Date: Mon, 29 Jun 2026 14:35:25 +0300 Subject: [PATCH 3/6] fix: revoke OpenShift token server-side and prevent re-login loop on logout Replace the browser redirect to `{oauth-server}/logout` (which returned HTTP 405 for unauthenticated GET requests) with a server-side token revocation call: DELETE /apis/oauth.openshift.io/v1/oauthaccesstokens/{name} The token name is derived from the raw access token via SHA-256 + base64url encoding for `sha256~`-prefixed tokens; legacy tokens are used as-is (the value is the resource name). The DELETE is authenticated with the user's own Bearer token, so no extra service-account permission is needed. The proxy always returns an empty redirect URL, leaving the frontend to navigate to `/login`. The frontend is also updated to prevent the silent re-login loop that occurred when `LoginPage` detected a single non-K8s provider and immediately re-initiated the OAuth flow (even though the FlightCtl session had just been cleared): - `logout()` in `apiCalls.ts` sets a `sessionStorage` flag (`flightctl.justLoggedOut`) before navigating to `/login`. - `LoginPage` reads and clears the flag; when set, it skips the single-provider auto-redirect and shows the provider selector instead, so the user must click explicitly to log back in. Unit tests for `openShiftTokenName` and the new `Logout` behaviour are added in `openshift_test.go`; `helpers_test.go` gains a `TestMain` that initialises the logger so tests can exercise the warning paths. Closes #708 Signed-off-by: Jonathan Kilzi Co-authored-by: Cursor --- .../src/app/components/Login/LoginPage.tsx | 12 +- apps/standalone/src/app/utils/apiCalls.ts | 5 +- package-lock.json | 138 ----------------- proxy/auth/helpers_test.go | 8 + proxy/auth/openshift.go | 68 ++++++--- proxy/auth/openshift_test.go | 143 +++++++++++++++--- 6 files changed, 193 insertions(+), 181 deletions(-) diff --git a/apps/standalone/src/app/components/Login/LoginPage.tsx b/apps/standalone/src/app/components/Login/LoginPage.tsx index 58c611fca..fb3eacfc8 100644 --- a/apps/standalone/src/app/components/Login/LoginPage.tsx +++ b/apps/standalone/src/app/components/Login/LoginPage.tsx @@ -9,7 +9,7 @@ import { useFetch } from '@flightctl/ui-components/src/hooks/useFetch'; import { useTranslation } from '@flightctl/ui-components/src/hooks/useTranslation'; import { getProviderDisplayName } from '@flightctl/ui-components/src/utils/authProvider'; -import { apiProxy } from '../../utils/apiCalls'; +import { apiProxy, JUST_LOGGED_OUT_KEY } from '../../utils/apiCalls'; import LoginPageLayout from './LoginPageLayout'; const redirectToProviderLogin = async (provider: AuthProvider) => { @@ -38,6 +38,7 @@ const LoginPage = () => { const [userSelectedProvider, setUserSelectedProvider] = React.useState(null); const [defaultProviderName, setDefaultProviderName] = React.useState(''); const [isRedirecting, setIsRedirecting] = React.useState(false); + const [suppressAutoSelect, setSuppressAutoSelect] = React.useState(false); const handleProviderSelect = async (provider: AuthProvider) => { // Prevent multiple clicks while redirect is in progress @@ -83,7 +84,12 @@ const LoginPage = () => { if (providers.length > 0) { setProviders(providers); setDefaultProviderName(config.defaultProvider || ''); - if (providers.length === 1 && providers[0].spec.providerType !== ProviderType.K8s) { + const justLoggedOut = sessionStorage.getItem(JUST_LOGGED_OUT_KEY) === 'true'; + if (justLoggedOut) { + sessionStorage.removeItem(JUST_LOGGED_OUT_KEY); + setSuppressAutoSelect(true); + } + if (!justLoggedOut && providers.length === 1 && providers[0].spec.providerType !== ProviderType.K8s) { setIsRedirecting(true); try { await redirectToProviderLogin(providers[0]); @@ -125,7 +131,7 @@ const LoginPage = () => { ); } - const selectedProvider = userSelectedProvider || (providers.length === 1 ? providers[0] : null); + const selectedProvider = userSelectedProvider || (suppressAutoSelect ? null : providers.length === 1 ? providers[0] : null); if (selectedProvider?.spec.providerType === ProviderType.K8s) { return ( { return { api: apiName, url: `${apiProxy}/flightctl/api/v1/${path}` }; }; +export const JUST_LOGGED_OUT_KEY = 'flightctl.justLoggedOut'; + export const logout = async () => { const redirectBase = encodeURIComponent(window.location.origin); const response = await fetch(`${apiProxy}/logout?redirect_base=${redirectBase}`, { @@ -74,7 +76,8 @@ export const logout = async () => { if (url) { window.location.href = url; } else { - window.location.reload(); + sessionStorage.setItem(JUST_LOGGED_OUT_KEY, 'true'); + window.location.href = '/login'; } }; diff --git a/package-lock.json b/package-lock.json index 8b04e13a8..f01d15dff 100644 --- a/package-lock.json +++ b/package-lock.json @@ -248,24 +248,6 @@ "js-yaml": "bin/js-yaml.js" } }, - "libs/ansible": { - "name": "@flightctl/ansible", - "version": "0.0.0", - "extraneous": true, - "license": "MIT", - "dependencies": { - "react-router-dom": "^6.22.0" - } - }, - "libs/cypress": { - "name": "@flightctl/ui-tests-cypress", - "version": "0.0.0", - "extraneous": true, - "license": "MIT", - "devDependencies": { - "cypress": "^14.0.0" - } - }, "libs/i18n": { "name": "@flightctl/i18n", "version": "0.0.0", @@ -4139,101 +4121,6 @@ "dev": true, "license": "MIT" }, - "node_modules/@swc/core": { - "version": "1.3.104", - "resolved": "https://registry.npmjs.org/@swc/core/-/core-1.3.104.tgz", - "integrity": "sha512-9LWH/qzR/Pmyco+XwPiPfz59T1sryI7o5dmqb593MfCkaX5Fzl9KhwQTI47i21/bXYuCdfa9ySZuVkzXMirYxA==", - "dev": true, - "hasInstallScript": true, - "license": "Apache-2.0", - "optional": true, - "peer": true, - "dependencies": { - "@swc/counter": "^0.1.1", - "@swc/types": "^0.1.5" - }, - "engines": { - "node": ">=10" - }, - "funding": { - "type": "opencollective", - "url": "https://opencollective.com/swc" - }, - "optionalDependencies": { - "@swc/core-darwin-arm64": "1.3.104", - "@swc/core-darwin-x64": "1.3.104", - "@swc/core-linux-arm-gnueabihf": "1.3.104", - "@swc/core-linux-arm64-gnu": "1.3.104", - "@swc/core-linux-arm64-musl": "1.3.104", - "@swc/core-linux-x64-gnu": "1.3.104", - "@swc/core-linux-x64-musl": "1.3.104", - "@swc/core-win32-arm64-msvc": "1.3.104", - "@swc/core-win32-ia32-msvc": "1.3.104", - "@swc/core-win32-x64-msvc": "1.3.104" - }, - "peerDependencies": { - "@swc/helpers": "^0.5.0" - }, - "peerDependenciesMeta": { - "@swc/helpers": { - "optional": true - } - } - }, - "node_modules/@swc/core-linux-x64-gnu": { - "version": "1.3.104", - "resolved": "https://registry.npmjs.org/@swc/core-linux-x64-gnu/-/core-linux-x64-gnu-1.3.104.tgz", - "integrity": "sha512-MfX/wiRdTjE5uXHTDnaX69xI4UBfxIhcxbVlMj//N+7AX/G2pl2UFityfVMU2HpM12BRckrCxVI8F/Zy3DZkYQ==", - "cpu": [ - "x64" - ], - "dev": true, - "license": "Apache-2.0 AND MIT", - "optional": true, - "os": [ - "linux" - ], - "peer": true, - "engines": { - "node": ">=10" - } - }, - "node_modules/@swc/core-linux-x64-musl": { - "version": "1.3.104", - "resolved": "https://registry.npmjs.org/@swc/core-linux-x64-musl/-/core-linux-x64-musl-1.3.104.tgz", - "integrity": "sha512-5yeILaxA31gGEmquErO8yxlq1xu0XVt+fz5mbbKXKZMRRILxYxNzAGb5mzV41r0oHz6Vhv4AXX/WMCmeWl+HkQ==", - "cpu": [ - "x64" - ], - "dev": true, - "license": "Apache-2.0 AND MIT", - "optional": true, - "os": [ - "linux" - ], - "peer": true, - "engines": { - "node": ">=10" - } - }, - "node_modules/@swc/counter": { - "version": "0.1.2", - "resolved": "https://registry.npmjs.org/@swc/counter/-/counter-0.1.2.tgz", - "integrity": "sha512-9F4ys4C74eSTEUNndnER3VJ15oru2NumfQxS8geE+f3eB5xvfxpWyqE5XlVnxb/R14uoXi6SLbBwwiDSkv+XEw==", - "dev": true, - "license": "Apache-2.0", - "optional": true, - "peer": true - }, - "node_modules/@swc/types": { - "version": "0.1.5", - "resolved": "https://registry.npmjs.org/@swc/types/-/types-0.1.5.tgz", - "integrity": "sha512-myfUej5naTBWnqOCc/MdVOLVjXUXtIA+NpDrDBKJtLLg2shUjBu3cZmB/85RyitKc55+lUUyl7oRfLOvkr2hsw==", - "dev": true, - "license": "Apache-2.0", - "optional": true, - "peer": true - }, "node_modules/@tsconfig/node10": { "version": "1.0.9", "resolved": "https://registry.npmjs.org/@tsconfig/node10/-/node10-1.0.9.tgz", @@ -8887,31 +8774,6 @@ "node": ">= 0.8" } }, - "node_modules/encoding": { - "version": "0.1.13", - "resolved": "https://registry.npmjs.org/encoding/-/encoding-0.1.13.tgz", - "integrity": "sha512-ETBauow1T35Y/WZMkio9jiM0Z5xjHHmJ4XmjZOq1l/dXz3lr2sRn87nJy20RupqSh1F2m3HHPSp8ShIPQJrJ3A==", - "license": "MIT", - "optional": true, - "peer": true, - "dependencies": { - "iconv-lite": "^0.6.2" - } - }, - "node_modules/encoding/node_modules/iconv-lite": { - "version": "0.6.3", - "resolved": "https://registry.npmjs.org/iconv-lite/-/iconv-lite-0.6.3.tgz", - "integrity": "sha512-4fCk79wshMdzMp2rH06qWrJE4iolqLhCUH+OiuIgU++RB0+94NlDL81atO7GX55uUKueo0txHNtvEyI6D7WdMw==", - "license": "MIT", - "optional": true, - "peer": true, - "dependencies": { - "safer-buffer": ">= 2.1.2 < 3.0.0" - }, - "engines": { - "node": ">=0.10.0" - } - }, "node_modules/end-of-stream": { "version": "1.4.4", "resolved": "https://registry.npmjs.org/end-of-stream/-/end-of-stream-1.4.4.tgz", diff --git a/proxy/auth/helpers_test.go b/proxy/auth/helpers_test.go index cd02bc4df..3ee6c6b64 100644 --- a/proxy/auth/helpers_test.go +++ b/proxy/auth/helpers_test.go @@ -4,9 +4,17 @@ import ( "fmt" "net/http" "net/url" + "os" "testing" + + "github.com/flightctl/flightctl-ui/log" ) +func TestMain(m *testing.M) { + log.InitLogs() + os.Exit(m.Run()) +} + // redirectBaseMatchesRequest is a test helper that checks whether url u's origin // matches the effective request origin. Returns nil when they match, error otherwise. func redirectBaseMatchesRequest(t *testing.T, r *http.Request, u *url.URL) error { diff --git a/proxy/auth/openshift.go b/proxy/auth/openshift.go index ab1d988d1..884006ca7 100644 --- a/proxy/auth/openshift.go +++ b/proxy/auth/openshift.go @@ -1,13 +1,16 @@ package auth import ( + "crypto/sha256" "crypto/tls" + b64 "encoding/base64" "fmt" "net/http" "net/url" "strings" "github.com/flightctl/flightctl-ui/bridge" + "github.com/flightctl/flightctl-ui/log" "github.com/flightctl/flightctl/api/v1beta1" "github.com/openshift/osincli" ) @@ -128,28 +131,59 @@ func (o *OpenShiftAuthHandler) openshiftClientForRedirect(redirectURI string) (* return client, nil } -// Logout derives the OpenShift OAuth server logout URL from the authorization -// URL (scheme + host only, path replaced with /logout). This terminates the -// user's OpenShift OAuth browser session so the login page's auto-redirect -// cannot silently re-authenticate them. -// -// Using authURL avoids a discovery HTTP round-trip and is robust: authURL -// always points to the OAuth server regardless of whether apiServerURL is -// the K8s API server or the OAuth server. The prior approach that called -// {apiServerURL}/.well-known/oauth-authorization-server was removed in -// EDM-2612 because it produced a K8s API server URL as the logout target -// (e.g. https://api.cluster:6443/logout) which is not a valid endpoint. +// openShiftTokenName derives the OAuthAccessToken Kubernetes resource name from +// an access token value. Tokens prefixed with "sha256~" are named by hashing +// the full token string with SHA-256 and base64url-encoding the result (no +// padding). Legacy tokens (no prefix) are used as-is — the token value IS the +// resource name in that case. +func openShiftTokenName(token string) string { + if strings.HasPrefix(token, "sha256~") { + hash := sha256.Sum256([]byte(token)) + return "sha256~" + b64.RawURLEncoding.EncodeToString(hash[:]) + } + return token +} + +// Logout revokes the OpenShift OAuth access token server-side by issuing a +// DELETE to /apis/oauth.openshift.io/v1/oauthaccesstokens/{name}, authenticated +// with the token itself as the Bearer credential. Revocation invalidates the +// OpenShift session without requiring a browser redirect, avoiding the 405 that +// the OAuth server's /logout endpoint returns for unauthenticated GET requests. // -// Returns ("", nil) when authURL is empty, unparseable, or missing scheme/host. -func (o *OpenShiftAuthHandler) Logout(_ string, _ string) (string, error) { - if o.authURL == "" { +// Always returns ("", nil) — the caller falls back to a local page reload which +// lands the user on the login page with no valid session. +func (o *OpenShiftAuthHandler) Logout(token string, _ string) (string, error) { + if token == "" || o.apiServerURL == "" { + return "", nil + } + + tokenName := openShiftTokenName(token) + revokeURL := fmt.Sprintf("%s/apis/oauth.openshift.io/v1/oauthaccesstokens/%s", + strings.TrimSuffix(o.apiServerURL, "/"), tokenName) + + client := &http.Client{ + Transport: &http.Transport{TLSClientConfig: o.tlsConfig}, + } + + req, err := http.NewRequest(http.MethodDelete, revokeURL, nil) + if err != nil { + log.GetLogger().WithError(err).Warn("Failed to build OpenShift token revocation request") return "", nil } - parsed, err := url.Parse(o.authURL) - if err != nil || parsed.Scheme == "" || parsed.Host == "" { + req.Header.Set("Authorization", "Bearer "+token) + + resp, err := client.Do(req) + if err != nil { + log.GetLogger().WithError(err).Warn("Failed to revoke OpenShift OAuth token") return "", nil } - return (&url.URL{Scheme: parsed.Scheme, Host: parsed.Host, Path: "/logout"}).String(), nil + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusNoContent { + log.GetLogger().Warnf("OpenShift token revocation returned unexpected status %d", resp.StatusCode) + } + + return "", nil } // GetLoginRedirectURL returns the OAuth2 authorization URL the browser should diff --git a/proxy/auth/openshift_test.go b/proxy/auth/openshift_test.go index e06add00f..1987b98da 100644 --- a/proxy/auth/openshift_test.go +++ b/proxy/auth/openshift_test.go @@ -1,54 +1,153 @@ package auth import ( + "crypto/sha256" + b64 "encoding/base64" + "net/http" + "net/http/httptest" "testing" ) +// TestOpenShiftTokenName verifies the OAuthAccessToken Kubernetes resource name +// derivation: sha256~-prefixed tokens are hashed; legacy tokens are used as-is. +func TestOpenShiftTokenName(t *testing.T) { + t.Parallel() + + sha256Hash := func(s string) string { + hash := sha256.Sum256([]byte(s)) + return "sha256~" + b64.RawURLEncoding.EncodeToString(hash[:]) + } + + tests := []struct { + name string + token string + wantName string + }{ + { + name: "sha256~ prefix token is hashed", + token: "sha256~testtoken", + wantName: sha256Hash("sha256~testtoken"), + }, + { + name: "legacy token used as-is", + token: "opaque-legacy-token", + wantName: "opaque-legacy-token", + }, + { + name: "empty token unchanged", + token: "", + wantName: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := openShiftTokenName(tt.token) + if got != tt.wantName { + t.Fatalf("expected %q, got %q", tt.wantName, got) + } + }) + } +} + +// TestOpenShiftLogout verifies that Logout revokes the token server-side via the +// Kubernetes OAuthAccessToken API and always returns ("", nil) — never a redirect URL. func TestOpenShiftLogout(t *testing.T) { t.Parallel() + sha256Hash := func(s string) string { + hash := sha256.Sum256([]byte(s)) + return "sha256~" + b64.RawURLEncoding.EncodeToString(hash[:]) + } + tests := []struct { - name string - authURL string - wantURL string + name string + token string + apiServerURL string + serverStatus int + wantDeletePath string // empty means no HTTP call expected + wantAuthHeader string }{ { - name: "derives logout URL from auth URL", - authURL: "https://oauth-openshift.apps.cluster.example.com/oauth/authorize", - wantURL: "https://oauth-openshift.apps.cluster.example.com/logout", + name: "revokes sha256~ token via Kubernetes API", + token: "sha256~testtoken", + serverStatus: http.StatusOK, + wantDeletePath: "/apis/oauth.openshift.io/v1/oauthaccesstokens/" + sha256Hash("sha256~testtoken"), + wantAuthHeader: "Bearer sha256~testtoken", }, { - name: "strips path and query", - authURL: "https://oauth-openshift.apps.cluster.example.com/oauth/authorize?response_type=code", - wantURL: "https://oauth-openshift.apps.cluster.example.com/logout", + name: "revokes legacy token via Kubernetes API", + token: "legacy-opaque-token", + serverStatus: http.StatusOK, + wantDeletePath: "/apis/oauth.openshift.io/v1/oauthaccesstokens/legacy-opaque-token", + wantAuthHeader: "Bearer legacy-opaque-token", }, { - name: "empty auth URL falls back", - authURL: "", - wantURL: "", + name: "tolerates server error and still returns empty URL", + token: "sha256~sometoken", + serverStatus: http.StatusInternalServerError, + wantDeletePath: "/apis/oauth.openshift.io/v1/oauthaccesstokens/" + sha256Hash("sha256~sometoken"), + wantAuthHeader: "Bearer sha256~sometoken", }, { - name: "invalid auth URL falls back", - authURL: "://not-a-valid-url", - wantURL: "", + name: "empty token skips revocation", + token: "", + serverStatus: http.StatusOK, }, { - name: "scheme-less auth URL falls back", - authURL: "//oauth-openshift.apps.cluster.example.com/oauth/authorize", - wantURL: "", + name: "empty apiServerURL skips revocation", + token: "sha256~testtoken", + apiServerURL: "", // override: do not use server + serverStatus: http.StatusOK, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - handler := &OpenShiftAuthHandler{authURL: tt.authURL} - got, err := handler.Logout("test-token", "https://ui.example.com") + + var gotMethod, gotPath, gotAuth string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotMethod = r.Method + gotPath = r.URL.Path + gotAuth = r.Header.Get("Authorization") + w.WriteHeader(tt.serverStatus) + })) + defer server.Close() + + // Determine which apiServerURL to use + apiServerURL := server.URL + if tt.name == "empty apiServerURL skips revocation" { + apiServerURL = "" + } + + handler := &OpenShiftAuthHandler{apiServerURL: apiServerURL} + gotURL, err := handler.Logout(tt.token, "https://ui.example.com") + if err != nil { t.Fatalf("expected no error, got: %v", err) } - if got != tt.wantURL { - t.Fatalf("expected %q, got %q", tt.wantURL, got) + if gotURL != "" { + t.Fatalf("expected empty redirect URL, got %q", gotURL) + } + + if tt.wantDeletePath == "" { + // No HTTP call should have been made + if gotMethod != "" { + t.Fatalf("expected no HTTP call, but got %s %s", gotMethod, gotPath) + } + return + } + + if gotMethod != http.MethodDelete { + t.Fatalf("expected DELETE request, got %q", gotMethod) + } + if gotPath != tt.wantDeletePath { + t.Fatalf("expected path %q, got %q", tt.wantDeletePath, gotPath) + } + if gotAuth != tt.wantAuthHeader { + t.Fatalf("expected Authorization %q, got %q", tt.wantAuthHeader, gotAuth) } }) } From 3088aaf543fc9231644958f02873ac92fbdc7201 Mon Sep 17 00:00:00 2001 From: Jonathan Kilzi Date: Mon, 29 Jun 2026 14:47:08 +0300 Subject: [PATCH 4/6] fix: sort JUST_LOGGED_OUT_KEY import alphabetically ESLint sort-imports rule requires named imports within a declaration to be sorted alphabetically. JUST_LOGGED_OUT_KEY (uppercase) sorts before apiProxy. Signed-off-by: Jonathan Kilzi Co-authored-by: Cursor --- apps/standalone/src/app/components/Login/LoginPage.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/standalone/src/app/components/Login/LoginPage.tsx b/apps/standalone/src/app/components/Login/LoginPage.tsx index fb3eacfc8..7ca1a95fa 100644 --- a/apps/standalone/src/app/components/Login/LoginPage.tsx +++ b/apps/standalone/src/app/components/Login/LoginPage.tsx @@ -9,7 +9,7 @@ import { useFetch } from '@flightctl/ui-components/src/hooks/useFetch'; import { useTranslation } from '@flightctl/ui-components/src/hooks/useTranslation'; import { getProviderDisplayName } from '@flightctl/ui-components/src/utils/authProvider'; -import { apiProxy, JUST_LOGGED_OUT_KEY } from '../../utils/apiCalls'; +import { JUST_LOGGED_OUT_KEY, apiProxy } from '../../utils/apiCalls'; import LoginPageLayout from './LoginPageLayout'; const redirectToProviderLogin = async (provider: AuthProvider) => { From a2ab7af42d5066476cec383ccfbf660774d94d2c Mon Sep 17 00:00:00 2001 From: Jonathan Kilzi Date: Mon, 29 Jun 2026 14:54:00 +0300 Subject: [PATCH 5/6] fix: apply Prettier formatting to LoginPage.tsx Line was over the print-width limit; Prettier wraps the ternary expression. Signed-off-by: Jonathan Kilzi Co-authored-by: Cursor --- apps/standalone/src/app/components/Login/LoginPage.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/standalone/src/app/components/Login/LoginPage.tsx b/apps/standalone/src/app/components/Login/LoginPage.tsx index 7ca1a95fa..f93ce2352 100644 --- a/apps/standalone/src/app/components/Login/LoginPage.tsx +++ b/apps/standalone/src/app/components/Login/LoginPage.tsx @@ -131,7 +131,8 @@ const LoginPage = () => { ); } - const selectedProvider = userSelectedProvider || (suppressAutoSelect ? null : providers.length === 1 ? providers[0] : null); + const selectedProvider = + userSelectedProvider || (suppressAutoSelect ? null : providers.length === 1 ? providers[0] : null); if (selectedProvider?.spec.providerType === ProviderType.K8s) { return ( Date: Mon, 29 Jun 2026 15:02:27 +0300 Subject: [PATCH 6/6] fix: normalize apiServerURL and add timeout to token revocation Two issues found by CodeRabbit on the Logout() implementation: 1. URL normalization: o.apiServerURL can carry a path, query string, or fragment when derived from AuthorizationUrl (e.g. .../oauth/authorize?x=y). Appending /apis/oauth.openshift.io/... to such a value would corrupt the DELETE path. Introduce openShiftAPIServerBase() to extract only scheme+host before constructing the revocation URL. 2. Missing timeout: the revocation http.Client had no Timeout, so a stalled API server would block the proxy handler indefinitely. Set a 10s timeout so logout degrades fast rather than hanging. Tests added for openShiftAPIServerBase() and for the end-to-end behaviour when apiServerURL contains path/query components. Signed-off-by: Jonathan Kilzi Co-authored-by: Cursor --- proxy/auth/openshift.go | 18 ++++++++-- proxy/auth/openshift_test.go | 66 +++++++++++++++++++++++++++++++++++- 2 files changed, 81 insertions(+), 3 deletions(-) diff --git a/proxy/auth/openshift.go b/proxy/auth/openshift.go index 884006ca7..195315952 100644 --- a/proxy/auth/openshift.go +++ b/proxy/auth/openshift.go @@ -8,6 +8,7 @@ import ( "net/http" "net/url" "strings" + "time" "github.com/flightctl/flightctl-ui/bridge" "github.com/flightctl/flightctl-ui/log" @@ -144,6 +145,18 @@ func openShiftTokenName(token string) string { return token } +// openShiftAPIServerBase normalises apiServerURL to a plain scheme+host (no +// path, query, or fragment) so the revocation URL is always well-formed even +// when apiServerURL was derived from an AuthorizationUrl that carries extra +// components. +func openShiftAPIServerBase(rawURL string) string { + parsed, err := url.Parse(rawURL) + if err != nil || parsed.Host == "" { + return strings.TrimSuffix(rawURL, "/") + } + return parsed.Scheme + "://" + parsed.Host +} + // Logout revokes the OpenShift OAuth access token server-side by issuing a // DELETE to /apis/oauth.openshift.io/v1/oauthaccesstokens/{name}, authenticated // with the token itself as the Bearer credential. Revocation invalidates the @@ -158,11 +171,12 @@ func (o *OpenShiftAuthHandler) Logout(token string, _ string) (string, error) { } tokenName := openShiftTokenName(token) - revokeURL := fmt.Sprintf("%s/apis/oauth.openshift.io/v1/oauthaccesstokens/%s", - strings.TrimSuffix(o.apiServerURL, "/"), tokenName) + base := openShiftAPIServerBase(o.apiServerURL) + revokeURL := fmt.Sprintf("%s/apis/oauth.openshift.io/v1/oauthaccesstokens/%s", base, tokenName) client := &http.Client{ Transport: &http.Transport{TLSClientConfig: o.tlsConfig}, + Timeout: 10 * time.Second, } req, err := http.NewRequest(http.MethodDelete, revokeURL, nil) diff --git a/proxy/auth/openshift_test.go b/proxy/auth/openshift_test.go index 1987b98da..de1b79840 100644 --- a/proxy/auth/openshift_test.go +++ b/proxy/auth/openshift_test.go @@ -51,6 +51,59 @@ func TestOpenShiftTokenName(t *testing.T) { } } +// TestOpenShiftAPIServerBase verifies that openShiftAPIServerBase normalises +// the base URL by stripping any path, query, and fragment components. +func TestOpenShiftAPIServerBase(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + rawURL string + wantURL string + }{ + { + name: "plain base URL is unchanged", + rawURL: "https://api.cluster.example.com", + wantURL: "https://api.cluster.example.com", + }, + { + name: "trailing slash is removed", + rawURL: "https://api.cluster.example.com/", + wantURL: "https://api.cluster.example.com", + }, + { + name: "path is stripped", + rawURL: "https://api.cluster.example.com/oauth/authorize", + wantURL: "https://api.cluster.example.com", + }, + { + name: "query string is stripped", + rawURL: "https://api.cluster.example.com?foo=bar", + wantURL: "https://api.cluster.example.com", + }, + { + name: "fragment is stripped", + rawURL: "https://api.cluster.example.com#frag", + wantURL: "https://api.cluster.example.com", + }, + { + name: "path and query are both stripped", + rawURL: "https://oauth.example.com/oauth/authorize?response_type=code", + wantURL: "https://oauth.example.com", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := openShiftAPIServerBase(tt.rawURL) + if got != tt.wantURL { + t.Fatalf("expected %q, got %q", tt.wantURL, got) + } + }) + } +} + // TestOpenShiftLogout verifies that Logout revokes the token server-side via the // Kubernetes OAuthAccessToken API and always returns ("", nil) — never a redirect URL. func TestOpenShiftLogout(t *testing.T) { @@ -101,6 +154,14 @@ func TestOpenShiftLogout(t *testing.T) { apiServerURL: "", // override: do not use server serverStatus: http.StatusOK, }, + { + name: "apiServerURL with path and query is normalised", + token: "legacy-token", + serverStatus: http.StatusOK, + wantDeletePath: "/apis/oauth.openshift.io/v1/oauthaccesstokens/legacy-token", + wantAuthHeader: "Bearer legacy-token", + // apiServerURL is set by the test loop to server.URL + "/oauth/authorize?foo=bar" + }, } for _, tt := range tests { @@ -118,8 +179,11 @@ func TestOpenShiftLogout(t *testing.T) { // Determine which apiServerURL to use apiServerURL := server.URL - if tt.name == "empty apiServerURL skips revocation" { + switch tt.name { + case "empty apiServerURL skips revocation": apiServerURL = "" + case "apiServerURL with path and query is normalised": + apiServerURL = server.URL + "/oauth/authorize?foo=bar" } handler := &OpenShiftAuthHandler{apiServerURL: apiServerURL}