From 114906149533cf2cd44b37019678c2a1fee5842f Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Fri, 17 Jul 2026 14:34:20 +0200 Subject: [PATCH 1/2] ZCU-PUB/test: reload redirect must be absolute and must not fire during SSR Replicates the redirect loop after a Shibboleth login: the relative 'reload/' URL is resolved against the current URL (e.g. /bitstreams//download), producing growing /bitstreams//reload/reload/... URLs, because during SSR the redirect repeats on every request (the dsRedirectUrl cookie cannot be cleared server-side - ServerCookieService.remove is a no-op). Also stops static-page.component.spec from mutating the shared environment object (it replaced environment.ui, breaking later specs reading environment.ui.nameSpace). Co-Authored-By: Claude Fable 5 --- src/app/core/auth/auth.effects.spec.ts | 49 +++++++++++++++++++ src/app/core/auth/auth.service.spec.ts | 30 ++++++++---- src/app/core/locale/locale.service.spec.ts | 38 +++++++++++++- .../server-hard-redirect.service.spec.ts | 33 ++++++++++++- .../static-page/static-page.component.spec.ts | 4 +- 5 files changed, 141 insertions(+), 13 deletions(-) diff --git a/src/app/core/auth/auth.effects.spec.ts b/src/app/core/auth/auth.effects.spec.ts index 2e6ba917aae..ea827682d1f 100644 --- a/src/app/core/auth/auth.effects.spec.ts +++ b/src/app/core/auth/auth.effects.spec.ts @@ -1,5 +1,7 @@ import { fakeAsync, TestBed, tick } from '@angular/core/testing'; +import { NgZone } from '@angular/core'; +import { Actions } from '@ngrx/effects'; import { provideMockActions } from '@ngrx/effects/testing'; import { Store, StoreModule } from '@ngrx/store'; import { MockStore, provideMockStore } from '@ngrx/store/testing'; @@ -17,6 +19,7 @@ import { CheckAuthenticationTokenCookieAction, LogOutErrorAction, LogOutSuccessAction, + RedirectAfterLoginSuccessAction, RefreshTokenErrorAction, RefreshTokenSuccessAction, RetrieveAuthenticatedEpersonAction, @@ -176,6 +179,52 @@ describe('AuthEffects', () => { done(); }); + describe('when a redirect url is set', () => { + const redirectUrl = '/bitstreams/eb0ca4e9-3e00-4c1f-8b19-a2c9f4f01234/download'; + + beforeEach(() => { + spyOn((authEffects as any).authService, 'storeToken'); + authServiceStub.setRedirectUrl(redirectUrl); + actions = hot('--a-', { + a: { + type: AuthActionTypes.AUTHENTICATED_SUCCESS, payload: { + authenticated: true, + authToken: token, + userHref: EPersonMock._links.self.href + } + } + }); + }); + + afterEach(() => { + authServiceStub.setRedirectUrl(undefined); + }); + + it('should return a REDIRECT_AFTER_LOGIN_SUCCESS action in the browser', () => { + const expected = cold('--b-', { b: new RedirectAfterLoginSuccessAction(redirectUrl) }); + + expect(authEffects.authenticatedSuccess$).toBeObservable(expected); + }); + + it('should return a RETRIEVE_AUTHENTICATED_EPERSON action during server-side rendering', () => { + // The server cannot clear the redirect cookie (ServerCookieService.set/remove are no-ops), + // so a hard redirect during SSR would repeat on every request and create a redirect loop. + // The redirect must be left to the browser. + const serverAuthEffects: AuthEffects = new (AuthEffects as any)( + TestBed.inject(Actions), + TestBed.inject(NgZone), + authorizationService, + authServiceStub as any, + TestBed.inject(Store) as any, + 'server' + ); + + const expected = cold('--b-', { b: new RetrieveAuthenticatedEpersonAction(EPersonMock._links.self.href) }); + + expect(serverAuthEffects.authenticatedSuccess$).toBeObservable(expected); + }); + }); + }); describe('checkToken$', () => { diff --git a/src/app/core/auth/auth.service.spec.ts b/src/app/core/auth/auth.service.spec.ts index b38d17aecdb..c6bddf4fe8e 100644 --- a/src/app/core/auth/auth.service.spec.ts +++ b/src/app/core/auth/auth.service.spec.ts @@ -34,6 +34,7 @@ import { NotificationsServiceStub } from '../../shared/testing/notifications-ser import { SetUserAsIdleAction, UnsetUserAsIdleAction } from './auth.actions'; import { SpecialGroupDataMock, SpecialGroupDataMock$ } from '../../shared/testing/special-group.mock'; import { cold } from 'jasmine-marbles'; +import { environment } from '../../../environments/environment'; describe('AuthService test', () => { @@ -103,7 +104,7 @@ describe('AuthService test', () => { linkService = { resolveLinks: {} }; - hardRedirectService = jasmine.createSpyObj('hardRedirectService', ['redirect']); + hardRedirectService = jasmine.createSpyObj('hardRedirectService', ['redirect', 'getCurrentRoute']); spyOn(linkService, 'resolveLinks').and.returnValue({ authenticated: true, eperson: observableOf({ payload: {} }) }); } @@ -374,28 +375,39 @@ describe('AuthService test', () => { expect(storage.remove).toHaveBeenCalled(); }); - it('should redirect to reload with redirect url', () => { + // The reload URL must be absolute (nameSpace-aware): a relative 'reload/...' URL is resolved + // against the current URL - e.g. against /bitstreams//download after an external + // (Shibboleth) login - producing invalid nested URLs like /bitstreams//reload/reload/... + const reloadPrefix = environment.ui.nameSpace.replace(/\/$/, '') + '/reload/'; + + it('should redirect to the absolute reload URL with redirect url', () => { authService.navigateToRedirectUrl('/collection/123'); // Reload with redirect URL set to /collection/123 - expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('reload/[0-9]*\\?redirect=' + encodeURIComponent('/collection/123')))); + expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('^' + reloadPrefix + '[0-9]+\\?redirect=' + encodeURIComponent('/collection/123') + '$'))); }); - it('should redirect to reload with /home', () => { + it('should redirect to the absolute reload URL with /home', () => { authService.navigateToRedirectUrl('/home'); // Reload with redirect URL set to /home - expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('reload/[0-9]*\\?redirect=' + encodeURIComponent('/home')))); + expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('^' + reloadPrefix + '[0-9]+\\?redirect=' + encodeURIComponent('/home') + '$'))); }); - it('should redirect to regular reload and not to /login', () => { + it('should redirect to the absolute reload URL and not to /login', () => { authService.navigateToRedirectUrl('/login'); // Reload without a redirect URL - expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('reload/[0-9]*(?!\\?)$'))); + expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('^' + reloadPrefix + '[0-9]+$'))); }); - it('should redirect to regular reload when no redirect url is found', () => { + it('should redirect to the absolute reload URL when no redirect url is found', () => { authService.navigateToRedirectUrl(undefined); // Reload without a redirect URL - expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('reload/[0-9]*(?!\\?)$'))); + expect(hardRedirectService.redirect).toHaveBeenCalledWith(jasmine.stringMatching(new RegExp('^' + reloadPrefix + '[0-9]+$'))); + }); + + it('should not redirect again when the current route is already the reload page', () => { + hardRedirectService.getCurrentRoute.and.returnValue(reloadPrefix + '123456789?redirect=' + encodeURIComponent('/home')); + authService.navigateToRedirectUrl('/collection/123'); + expect(hardRedirectService.redirect).not.toHaveBeenCalled(); }); describe('impersonate', () => { diff --git a/src/app/core/locale/locale.service.spec.ts b/src/app/core/locale/locale.service.spec.ts index 39356fdf970..5d08868a81d 100644 --- a/src/app/core/locale/locale.service.spec.ts +++ b/src/app/core/locale/locale.service.spec.ts @@ -1,6 +1,9 @@ -import { TestBed, waitForAsync } from '@angular/core/testing'; +import { fakeAsync, TestBed, tick, waitForAsync } from '@angular/core/testing'; import { TranslateLoader, TranslateModule, TranslateService } from '@ngx-translate/core'; +import { of } from 'rxjs'; + +import { environment } from '../../../environments/environment'; import { CookieService } from '../services/cookie.service'; import { CookieServiceMock } from '../../shared/mocks/cookie.service.mock'; @@ -147,4 +150,37 @@ describe('LocaleService test suite', () => { expect(service.getLanguageCodeList).toHaveBeenCalled(); }); }); + + describe('refreshAfterChangeLanguage', () => { + let originalUi; + let originalGetCurrentUrl; + + beforeEach(() => { + // Pin the tested nameSpace value: environment is a shared mutable object and some specs + // replace environment.ui entirely, so this test must not rely on the suite order. + originalUi = (environment as any).ui; + (environment as any).ui = Object.assign({}, originalUi, { nameSpace: '/angular-dspace' }); + originalGetCurrentUrl = (routeService as any).getCurrentUrl; + }); + + afterEach(() => { + (environment as any).ui = originalUi; + // routeServiceStub is a shared module-level singleton - restore it + (routeService as any).getCurrentUrl = originalGetCurrentUrl; + }); + + it('should hard redirect to the absolute (nameSpace-aware) reload URL', fakeAsync(() => { + // A relative 'reload/...' URL would be resolved against the current URL + // (e.g. /items/) and produce an invalid nested URL like /items//reload/... + const currentUrl = '/items/1234'; + const fakeLocation = { href: '' }; + serviceAsAny._window = { nativeWindow: { location: fakeLocation } }; + (routeService as any).getCurrentUrl = jasmine.createSpy('getCurrentUrl').and.returnValue(of(currentUrl)); + + service.refreshAfterChangeLanguage(); + tick(); + + expect(fakeLocation.href).toMatch(new RegExp('^/angular-dspace/reload/[0-9]+\\?redirect=' + encodeURIComponent(currentUrl) + '$')); + })); + }); }); diff --git a/src/app/core/services/server-hard-redirect.service.spec.ts b/src/app/core/services/server-hard-redirect.service.spec.ts index 6bd58289211..9aa35a888cb 100644 --- a/src/app/core/services/server-hard-redirect.service.spec.ts +++ b/src/app/core/services/server-hard-redirect.service.spec.ts @@ -23,7 +23,7 @@ describe('ServerHardRedirectService', () => { }); describe('when performing a default redirect', () => { - const redirect = 'test redirect'; + const redirect = '/test/redirect'; beforeEach(() => { service.redirect(redirect); @@ -36,7 +36,7 @@ describe('ServerHardRedirectService', () => { }); describe('when performing a 301 redirect', () => { - const redirect = 'test 301 redirect'; + const redirect = '/test/301/redirect'; const redirectStatusCode = 301; beforeEach(() => { @@ -49,6 +49,35 @@ describe('ServerHardRedirectService', () => { }); }); + describe('when performing a redirect to a relative url', () => { + // A relative URL in the Location header is resolved by the browser against the request URL, + // e.g. 'reload/123' requested at /bitstreams//download resolves + // to /bitstreams//reload/123 - such a redirect must never be emitted + const redirect = 'reload/123456789'; + + beforeEach(() => { + service.redirect(redirect); + }); + + it('should redirect to the URL prefixed with a slash', () => { + expect(mockResponse.redirect).toHaveBeenCalledWith(302, '/' + redirect); + expect(mockResponse.end).toHaveBeenCalled(); + }); + }); + + describe('when performing a redirect to an external url', () => { + const redirect = 'https://external-host.com/path'; + + beforeEach(() => { + service.redirect(redirect); + }); + + it('should redirect to the unchanged external URL', () => { + expect(mockResponse.redirect).toHaveBeenCalledWith(302, redirect); + expect(mockResponse.end).toHaveBeenCalled(); + }); + }); + describe('when requesting the current route', () => { beforeEach(() => { diff --git a/src/app/static-page/static-page.component.spec.ts b/src/app/static-page/static-page.component.spec.ts index 97df3c3d420..153675f146c 100644 --- a/src/app/static-page/static-page.component.spec.ts +++ b/src/app/static-page/static-page.component.spec.ts @@ -27,7 +27,9 @@ describe('StaticPageComponent', () => { getCurrentLanguageCode: jasmine.createSpy('getCurrentLanguageCode'), }); - appConfig = Object.assign(environment, { + // Do not mutate the shared `environment` object - replacing `environment.ui` would + // break any later spec that reads e.g. environment.ui.nameSpace + appConfig = Object.assign({}, environment, { ui: { namespace: 'testNamespace' } From 3333bd4fc72532afd713ed2bf743fc6825593b97 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Fri, 17 Jul 2026 14:34:20 +0200 Subject: [PATCH 2/2] ZCU-PUB/fix: absolute reload URL and no post-login hard redirect during SSR Fixes the redirect loop after a Shibboleth login: - navigateToRedirectUrl() and refreshAfterChangeLanguage() build an absolute, ui.nameSpace-aware /reload/ URL. A relative 'reload/' URL in the Location header is resolved against the request URL (e.g. /bitstreams//download after an external login), producing nested /bitstreams//reload/... URLs which never match the top-level 'reload/:rnd' route. navigateToRedirectUrl() also skips the redirect when the current path already is the reload page. - authenticatedSuccess$ dispatches RedirectAfterLoginSuccess only in the browser. The server cannot clear the dsRedirectUrl cookie (ServerCookieService.set/remove are no-ops), so a hard redirect during SSR repeats on every request - one 'reload/' segment per 302 hop - until the browser aborts at its redirect limit. - ServerHardRedirectService.redirect() never emits a relative Location header. Co-Authored-By: Claude Fable 5 --- src/app/core/auth/auth.effects.ts | 12 +++++++++--- src/app/core/auth/auth.service.ts | 14 +++++++++++++- src/app/core/locale/locale.service.ts | 9 +++++++-- .../core/services/server-hard-redirect.service.ts | 7 +++++++ 4 files changed, 36 insertions(+), 6 deletions(-) diff --git a/src/app/core/auth/auth.effects.ts b/src/app/core/auth/auth.effects.ts index 281355b769e..fe6aa3cc143 100644 --- a/src/app/core/auth/auth.effects.ts +++ b/src/app/core/auth/auth.effects.ts @@ -1,4 +1,5 @@ -import { Injectable, NgZone } from '@angular/core'; +import { Inject, Injectable, NgZone, PLATFORM_ID } from '@angular/core'; +import { isPlatformBrowser } from '@angular/common'; import { asyncScheduler, @@ -100,7 +101,11 @@ export class AuthEffects { map((redirectUrl: string) => [action, redirectUrl]) )), map(([action, redirectUrl]: [AuthenticatedSuccessAction, string]) => { - if (hasValue(redirectUrl)) { + // Perform the redirect only in the browser: the server cannot clear the redirect cookie + // (ServerCookieService.set/remove are no-ops), so a hard redirect during SSR would repeat + // on every request and create a redirect loop. + // The browser re-runs this effect after hydration and performs the redirect itself. + if (hasValue(redirectUrl) && isPlatformBrowser(this.platformId)) { return new RedirectAfterLoginSuccessAction(redirectUrl); } else { return new RetrieveAuthenticatedEpersonAction(action.payload.userHref); @@ -287,6 +292,7 @@ export class AuthEffects { private zone: NgZone, private authorizationsService: AuthorizationDataService, private authService: AuthService, - private store: Store) { + private store: Store, + @Inject(PLATFORM_ID) private platformId: any) { } } diff --git a/src/app/core/auth/auth.service.ts b/src/app/core/auth/auth.service.ts index 5efcf7ef7ae..919e00d4927 100644 --- a/src/app/core/auth/auth.service.ts +++ b/src/app/core/auth/auth.service.ts @@ -487,9 +487,21 @@ export class AuthService { * @param redirectUrl */ public navigateToRedirectUrl(redirectUrl: string) { + // Don't do redirect if the current page already is the reload page, + // otherwise the reload could be repeated indefinitely + // (only the path is checked - a query string could legitimately contain '/reload/') + const currentRoute = this.hardRedirectService.getCurrentRoute(); + if (hasValue(currentRoute) && currentRoute.split('?')[0].includes('/reload/')) { + return; + } // Don't do redirect if already on reload url if (!hasValue(redirectUrl) || !redirectUrl.includes('reload/')) { - let url = `reload/${new Date().getTime()}`; + // The reload URL must be absolute (nameSpace-aware). A relative 'reload/...' URL is resolved + // against the current URL - e.g. against /bitstreams//download after an external + // (Shibboleth) login - and produces invalid nested URLs like /bitstreams//reload/... + // which never match the 'reload/:rnd' route. + const nameSpace = (environment.ui.nameSpace || '').replace(/\/$/, ''); + let url = `${nameSpace}/reload/${new Date().getTime()}`; if (isNotEmpty(redirectUrl) && !redirectUrl.startsWith(LOGIN_ROUTE)) { url += `?redirect=${encodeURIComponent(redirectUrl)}`; } diff --git a/src/app/core/locale/locale.service.ts b/src/app/core/locale/locale.service.ts index cd705c2814d..cb26903260b 100644 --- a/src/app/core/locale/locale.service.ts +++ b/src/app/core/locale/locale.service.ts @@ -191,8 +191,13 @@ export class LocaleService { public refreshAfterChangeLanguage() { this.routeService.getCurrentUrl().pipe(take(1)).subscribe((currentURL) => { // Hard redirect to the reload page with a unique number behind it - // so that all state is definitely lost - this._window.nativeWindow.location.href = `reload/${new Date().getTime()}?redirect=` + encodeURIComponent(currentURL); + // so that all state is definitely lost. + // The reload URL must be absolute (nameSpace-aware). A relative 'reload/...' URL is resolved + // against the current URL and produces invalid nested URLs like /items//reload/... + // which never match the 'reload/:rnd' route. + const nameSpace = (environment.ui.nameSpace || '').replace(/\/$/, ''); + this._window.nativeWindow.location.href = + `${nameSpace}/reload/${new Date().getTime()}?redirect=` + encodeURIComponent(currentURL); }); } diff --git a/src/app/core/services/server-hard-redirect.service.ts b/src/app/core/services/server-hard-redirect.service.ts index d71318d7b8e..8981f9698ac 100644 --- a/src/app/core/services/server-hard-redirect.service.ts +++ b/src/app/core/services/server-hard-redirect.service.ts @@ -26,6 +26,13 @@ export class ServerHardRedirectService extends HardRedirectService { */ redirect(url: string, statusCode?: number) { + // A relative URL in the Location header is resolved by the browser against the request URL + // (e.g. 'reload/123' requested at /bitstreams//download resolves to + // /bitstreams//reload/123) - make sure only absolute URLs are emitted + if (!url.startsWith('/') && !/^https?:\/\//i.test(url)) { + url = '/' + url; + } + if (url === this.req.url) { return; }