diff --git a/changelog/unreleased/bugfix-oidc-logout-query-param-replication b/changelog/unreleased/bugfix-oidc-logout-query-param-replication new file mode 100644 index 00000000000..f0b2c0634eb --- /dev/null +++ b/changelog/unreleased/bugfix-oidc-logout-query-param-replication @@ -0,0 +1,6 @@ +Bugfix: Broken re-login after logout + +After a user logged out, it was no longer possible to login without reloading the ocis root domain, +this has now been fixed and only allowed query-params are taken into account. + +https://github.com/owncloud/web/pull/8694 diff --git a/packages/web-runtime/src/router/helpers.ts b/packages/web-runtime/src/router/helpers.ts index a594dc5d0dd..e0a4706a78b 100644 --- a/packages/web-runtime/src/router/helpers.ts +++ b/packages/web-runtime/src/router/helpers.ts @@ -1,5 +1,6 @@ import { base, router } from './index' import { RouteLocation, RouteParams, Router, RouteRecordNormalized } from 'vue-router' +import pick from 'lodash-es/pick' import { AuthContext, authContextValues, @@ -10,7 +11,18 @@ import { export const buildUrl = (pathname) => { const isHistoryMode = !!base const baseUrl = new URL(window.location.href.split('#')[0]) - baseUrl.search = window.location.search + + // searchParams defines a set of search parameters which should be part of new url. + // The resulting querystring only contains the properties listed here. + const searchParams = new URLSearchParams( + pick(Object.fromEntries(new URLSearchParams(window.location.search).entries()), [ + // needed for private links + 'details' + ]) + ).toString() + + baseUrl.search = searchParams + if (isHistoryMode) { // in history mode we can't determine the base path, it must be provided by the document baseUrl.pathname = new URL(base.href).pathname diff --git a/packages/web-runtime/src/services/auth/authService.ts b/packages/web-runtime/src/services/auth/authService.ts index a46e66e9f54..9ea6f808fec 100644 --- a/packages/web-runtime/src/services/auth/authService.ts +++ b/packages/web-runtime/src/services/auth/authService.ts @@ -8,6 +8,7 @@ import { extractPublicLinkToken, isPublicLinkContext, isUserContext } from '../. import { unref } from 'vue' import { Ability } from 'web-pkg/src/utils' import { Language } from 'vue3-gettext' +import pick from 'lodash-es/pick' export class AuthService { private clientService: ClientService @@ -164,20 +165,25 @@ export class AuthService { * Sign in callback gets called from the IDP after initial login. */ public async signInCallback() { - // craft a url that the parser in oidc-client-ts can handle… this is required for oauth2 logins - const url = - '/?' + - new URLSearchParams( - unref(this.router.currentRoute).query as Record - ).toString() + const currentQuery = unref(this.router.currentRoute).query + // craft an url that the parser in oidc-client-ts can handle… this is required for oauth2 logins + const url = '/?' + new URLSearchParams(currentQuery as Record).toString() try { await this.userManager.signinRedirectCallback(url) const redirectUrl = this.userManager.getAndClearPostLoginRedirectUrl() + + // transportQuery defines a set of query parameters which should be part of router route replace. + // The resulting object only contains the properties listed here. + const transportQuery = pick(currentQuery, [ + // needed for private links + 'details' + ]) + return this.router.replace({ path: redirectUrl, - query: unref(this.router.currentRoute).query + query: transportQuery }) } catch (e) { console.warn('error during authentication:', e) diff --git a/packages/web-runtime/tests/unit/router/index.spec.ts b/packages/web-runtime/tests/unit/router/index.spec.ts index c8e5a6de665..0478851b4a2 100644 --- a/packages/web-runtime/tests/unit/router/index.spec.ts +++ b/packages/web-runtime/tests/unit/router/index.spec.ts @@ -15,6 +15,9 @@ describe('buildUrl', () => { ${'https://localhost:9200/files/list/all'} | ${'/foo'} | ${'/login/foo'} | ${'https://localhost:9200/foo/login/foo'} ${'https://localhost:9200/files/list/all'} | ${'/'} | ${'/bar.html'} | ${'https://localhost:9200/bar.html'} ${'https://localhost:9200/files/list/all'} | ${'/foo'} | ${'/bar.html'} | ${'https://localhost:9200/foo/bar.html'} + ${'https://localhost:9200/files/list/all?details=91953740'} | ${'/foo'} | ${'/bar.html'} | ${'https://localhost:9200/foo/bar.html?details=91953740'} + ${'https://localhost:9200/files/list/all?details=91953740&unknown=87987987'} | ${'/foo'} | ${'/bar.html'} | ${'https://localhost:9200/foo/bar.html?details=91953740'} + ${'https://localhost:9200/files/list/all?unknown=87987987'} | ${'/foo'} | ${'/bar.html'} | ${'https://localhost:9200/foo/bar.html'} `('$path -> $expected', async ({ location, base, path, expected }) => { delete window.location window.location = new URL(location) as any diff --git a/packages/web-runtime/tests/unit/services/auth/authService.spec.ts b/packages/web-runtime/tests/unit/services/auth/authService.spec.ts new file mode 100644 index 00000000000..cb1b603dc84 --- /dev/null +++ b/packages/web-runtime/tests/unit/services/auth/authService.spec.ts @@ -0,0 +1,27 @@ +import { AuthService } from 'web-runtime/src/services/auth/authService' + +describe('AuthService', () => { + describe('signInCallback', () => { + it.each([ + [{ details: 'details' }, { details: 'details' }], + [{ details: 'details', unknown: 'unknown' }, { details: 'details' }], + [{ unknown: 'unknown' }, {}] + ])('forwards only whitelisted query parameters: %s = %s', async (query, expected: any) => { + const authService = new AuthService() + const replace = jest.fn() + + jest.replaceProperty(authService as any, 'userManager', { + signinRedirectCallback: jest.fn(), + getAndClearPostLoginRedirectUrl: jest.fn() + }) + + jest.replaceProperty(authService as any, 'router', { + currentRoute: { query }, + replace + }) + await authService.signInCallback() + + expect(replace.mock.calls[0][0].query).toEqual(expected) + }) + }) +})