From 5ea7bb696ffb08dda8b158e85dcd2a84a295b195 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Mon, 17 Aug 2026 09:21:26 +0200 Subject: [PATCH 1/3] TUL/Browse links: carry only the parameters a browse page uses The browse entry link merged the whole current query string into every link it generated. An unknown parameter therefore came back in 21 links per page, and a crawler that does not decode HTML entities turned the escaped separator into a longer amp;value on each pass - an unbounded URL space that made up 52 % of all requests during the Aug 6-9 outage. Build the parameters explicitly instead. scope, page size and sort are carried over, everything else is dropped. Upstream removed the same merge in #2735 but without carrying scope, which broke scoped browse (DSpace/dspace-angular#5209, still open). Co-Authored-By: Claude Opus 5 --- .../browse-entry-list-element.component.html | 2 +- ...rowse-entry-list-element.component.spec.ts | 47 +++++++++++++++++-- .../browse-entry-list-element.component.ts | 19 ++++++-- 3 files changed, 61 insertions(+), 7 deletions(-) diff --git a/src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.html b/src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.html index dcbdd77bffa..b3341df3705 100644 --- a/src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.html +++ b/src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.html @@ -1,5 +1,5 @@
- + {{object.value}} diff --git a/src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.spec.ts b/src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.spec.ts index a4490bd9519..8156fa94c8a 100644 --- a/src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.spec.ts +++ b/src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.spec.ts @@ -7,6 +7,7 @@ import { BrowseEntry } from '../../../core/shared/browse-entry.model'; import { PaginationService } from '../../../core/pagination/pagination.service'; import { RouteService } from '../../../core/services/route.service'; import { of as observableOf } from 'rxjs'; +import { take } from 'rxjs/operators'; let browseEntryListElementComponent: BrowseEntryListElementComponent; let fixture: ComponentFixture; @@ -18,15 +19,18 @@ const mockValue: BrowseEntry = Object.assign(new BrowseEntry(), { let paginationService; let routeService; const pageParam = 'bbm.page'; +let queryParamsInUrl: { [name: string]: string }; function init() { paginationService = jasmine.createSpyObj('paginationService', { getPageParam: pageParam }); - routeService = jasmine.createSpyObj('routeService', { - getQueryParameterValue: observableOf('1') - }); + queryParamsInUrl = { [pageParam]: '1' }; + routeService = jasmine.createSpyObj('routeService', ['getQueryParameterValue']); + routeService.getQueryParameterValue.and.callFake( + (name: string) => observableOf(queryParamsInUrl[name]) + ); } describe('BrowseEntryListElementComponent', () => { beforeEach(waitForAsync(() => { @@ -61,4 +65,41 @@ describe('BrowseEntryListElementComponent', () => { expect(browseEntryLink.nativeElement.textContent.trim()).toBe(mockValue.value); }); }); + + describe('queryParams', () => { + let emitted; + + const buildQueryParams = () => { + browseEntryListElementComponent.object = mockValue; + fixture.detectChanges(); + browseEntryListElementComponent.queryParams$.pipe(take(1)).subscribe((p) => emitted = p); + }; + + it('should keep the scope of the community or collection being browsed', () => { + queryParamsInUrl.scope = '0eb1f4d0-fd7c-4c2c-b0d9-32ee18f5e1c1'; + buildQueryParams(); + + expect(emitted.scope).toBe('0eb1f4d0-fd7c-4c2c-b0d9-32ee18f5e1c1'); + }); + + it('should keep the page size and sort chosen by the user', () => { + queryParamsInUrl['bbm.rpp'] = '40'; + queryParamsInUrl['bbm.sf'] = 'title'; + queryParamsInUrl['bbm.sd'] = 'DESC'; + buildQueryParams(); + + expect(emitted['bbm.rpp']).toBe('40'); + expect(emitted['bbm.sf']).toBe('title'); + expect(emitted['bbm.sd']).toBe('DESC'); + }); + + it('should drop parameters it does not recognise', () => { + queryParamsInUrl['amp;value'] = 'Some Author'; + queryParamsInUrl.utm_source = 'newsletter'; + buildQueryParams(); + + expect(Object.keys(emitted)).not.toContain('amp;value'); + expect(Object.keys(emitted)).not.toContain('utm_source'); + }); + }); }); diff --git a/src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.ts b/src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.ts index 667da726ed8..984a116de0e 100644 --- a/src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.ts +++ b/src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.ts @@ -8,7 +8,7 @@ import { PaginationService } from '../../../core/pagination/pagination.service'; import { Params } from '@angular/router'; import { BBM_PAGINATION_ID } from '../../../browse-by/browse-by-metadata-page/browse-by-metadata-page.component'; import { RouteService } from 'src/app/core/services/route.service'; -import { Observable } from 'rxjs'; +import { combineLatest, Observable } from 'rxjs'; import { map } from 'rxjs/operators'; @Component({ @@ -37,16 +37,29 @@ export class BrowseEntryListElementComponent extends AbstractListableElementComp /** * Get the query params to access the item page of this browse entry. + * + * Carries over only the parameters a browse page actually uses. Anything else in the current URL + * is dropped, so a malformed parameter cannot be reflected back into the links we generate. */ private getQueryParams(): Observable { const pageParamName = this.paginationService.getPageParam(BBM_PAGINATION_ID); - return this.routeService.getQueryParameterValue(pageParamName).pipe( - map((currentPage) => { + return combineLatest([ + this.routeService.getQueryParameterValue(pageParamName), + this.routeService.getQueryParameterValue('scope'), + this.routeService.getQueryParameterValue(`${BBM_PAGINATION_ID}.rpp`), + this.routeService.getQueryParameterValue(`${BBM_PAGINATION_ID}.sf`), + this.routeService.getQueryParameterValue(`${BBM_PAGINATION_ID}.sd`), + ]).pipe( + map(([currentPage, scope, rpp, sortField, sortDirection]) => { return { value: this.object.value, authority: !!this.object.authority ? this.object.authority : undefined, + scope: scope || undefined, startsWith: undefined, [pageParamName]: null, + [`${BBM_PAGINATION_ID}.rpp`]: rpp || undefined, + [`${BBM_PAGINATION_ID}.sf`]: sortField || undefined, + [`${BBM_PAGINATION_ID}.sd`]: sortDirection || undefined, [BBM_PAGINATION_ID + '.return']: currentPage }; }) From b3547d9722efef8ccfa5576250dcef041ae561cb Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Mon, 17 Aug 2026 15:28:24 +0200 Subject: [PATCH 2/3] TUL/Test the generated href, not just the params object The previous test could not fail: the mock only answers for the names getQueryParams() asks about, so an unknown key never reached the result. The reflection happened in RouterLink, so the assertion has to be on the rendered href with a real Router in place. --- ...rowse-entry-list-element.component.spec.ts | 51 ++++++++++++++++--- 1 file changed, 43 insertions(+), 8 deletions(-) diff --git a/src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.spec.ts b/src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.spec.ts index 8156fa94c8a..7b8f712d53d 100644 --- a/src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.spec.ts +++ b/src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.spec.ts @@ -1,6 +1,8 @@ -import { ComponentFixture, TestBed, waitForAsync } from '@angular/core/testing'; +import { ComponentFixture, fakeAsync, TestBed, tick, waitForAsync } from '@angular/core/testing'; import { ChangeDetectionStrategy, NO_ERRORS_SCHEMA } from '@angular/core'; import { By } from '@angular/platform-browser'; +import { Router } from '@angular/router'; +import { RouterTestingModule } from '@angular/router/testing'; import { TruncatePipe } from '../../utils/truncate.pipe'; import { BrowseEntryListElementComponent } from './browse-entry-list-element.component'; import { BrowseEntry } from '../../../core/shared/browse-entry.model'; @@ -93,13 +95,46 @@ describe('BrowseEntryListElementComponent', () => { expect(emitted['bbm.sd']).toBe('DESC'); }); - it('should drop parameters it does not recognise', () => { - queryParamsInUrl['amp;value'] = 'Some Author'; - queryParamsInUrl.utm_source = 'newsletter'; - buildQueryParams(); + }); - expect(Object.keys(emitted)).not.toContain('amp;value'); - expect(Object.keys(emitted)).not.toContain('utm_source'); - }); + describe('the rendered link', () => { + const scopeUUID = 'a2f2d0a1-3f0e-4d3a-9c1b-5f7e8a9b0c1d'; + + beforeEach(waitForAsync(() => { + // the suite above already instantiated the TestBed, and this block needs a real Router in it + TestBed.resetTestingModule(); + init(); + queryParamsInUrl.scope = scopeUUID; + TestBed.configureTestingModule({ + imports: [ + RouterTestingModule.withRoutes([ + { path: 'browse/author', component: BrowseEntryListElementComponent } + ]) + ], + declarations: [BrowseEntryListElementComponent, TruncatePipe], + providers: [ + { provide: 'objectElementProvider', useValue: { mockValue } }, + {provide: PaginationService, useValue: paginationService}, + {provide: RouteService, useValue: routeService}, + ], + schemas: [NO_ERRORS_SCHEMA] + }).compileComponents(); + })); + + it('should keep the scope but not a parameter the page never asked for', fakeAsync(() => { + const router = TestBed.inject(Router); + router.navigate(['/browse/author'], { + queryParams: { scope: scopeUUID, 'amp;value': 'Some Author' } + }); + tick(); + + fixture = TestBed.createComponent(BrowseEntryListElementComponent); + fixture.componentInstance.object = mockValue; + fixture.detectChanges(); + + const href = fixture.debugElement.query(By.css('a.lead')).nativeElement.getAttribute('href'); + expect(href).toContain(`scope=${scopeUUID}`); + expect(href).not.toContain('amp'); + })); }); }); From af4690195cc52ee22c14676581386064b4690501 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Tue, 18 Aug 2026 13:20:02 +0200 Subject: [PATCH 3/3] TUL/Drive the link test from the URL, not from a stub The previous version mocked RouteService and then navigated the router, which made it look like the URL drove the assertions when it did not. This suite uses the real RouteService, so scope, pagination and the malformed parameter all reach the component the way they do in a browser. Restoring queryParamsHandling merge makes it fail. --- ...rowse-entry-list-element.component.spec.ts | 161 +++++++++--------- 1 file changed, 85 insertions(+), 76 deletions(-) diff --git a/src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.spec.ts b/src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.spec.ts index 7b8f712d53d..0dbff7ee46f 100644 --- a/src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.spec.ts +++ b/src/app/shared/object-list/browse-entry-list-element/browse-entry-list-element.component.spec.ts @@ -1,15 +1,16 @@ import { ComponentFixture, fakeAsync, TestBed, tick, waitForAsync } from '@angular/core/testing'; -import { ChangeDetectionStrategy, NO_ERRORS_SCHEMA } from '@angular/core'; +import { ChangeDetectionStrategy, Component, NO_ERRORS_SCHEMA } from '@angular/core'; import { By } from '@angular/platform-browser'; -import { Router } from '@angular/router'; +import { Params, Router } from '@angular/router'; import { RouterTestingModule } from '@angular/router/testing'; +import { Store } from '@ngrx/store'; import { TruncatePipe } from '../../utils/truncate.pipe'; import { BrowseEntryListElementComponent } from './browse-entry-list-element.component'; import { BrowseEntry } from '../../../core/shared/browse-entry.model'; import { PaginationService } from '../../../core/pagination/pagination.service'; import { RouteService } from '../../../core/services/route.service'; import { of as observableOf } from 'rxjs'; -import { take } from 'rxjs/operators'; + let browseEntryListElementComponent: BrowseEntryListElementComponent; let fixture: ComponentFixture; @@ -18,25 +19,21 @@ const mockValue: BrowseEntry = Object.assign(new BrowseEntry(), { value: 'De Langhe Kristof' }); -let paginationService; -let routeService; const pageParam = 'bbm.page'; -let queryParamsInUrl: { [name: string]: string }; - -function init() { - paginationService = jasmine.createSpyObj('paginationService', { - getPageParam: pageParam - }); - queryParamsInUrl = { [pageParam]: '1' }; - routeService = jasmine.createSpyObj('routeService', ['getQueryParameterValue']); - routeService.getQueryParameterValue.and.callFake( - (name: string) => observableOf(queryParamsInUrl[name]) - ); +@Component({ template: '' }) +class DummyComponent { } + describe('BrowseEntryListElementComponent', () => { beforeEach(waitForAsync(() => { - init(); + const paginationService = jasmine.createSpyObj('paginationService', { + getPageParam: pageParam + }); + const routeService = jasmine.createSpyObj('routeService', { + getQueryParameterValue: observableOf(undefined) + }); + TestBed.configureTestingModule({ declarations: [BrowseEntryListElementComponent, TruncatePipe], providers: [ @@ -67,74 +64,86 @@ describe('BrowseEntryListElementComponent', () => { expect(browseEntryLink.nativeElement.textContent.trim()).toBe(mockValue.value); }); }); +}); - describe('queryParams', () => { - let emitted; +describe('BrowseEntryListElementComponent link', () => { + // The real RouteService is used here on purpose: every parameter below reaches the component + // through the router, the way it does in the browser, so the assertions are on what the URL + // actually produces rather than on what a stub was told to answer. + const scopeUUID = 'a2f2d0a1-3f0e-4d3a-9c1b-5f7e8a9b0c1d'; + let router: Router; - const buildQueryParams = () => { - browseEntryListElementComponent.object = mockValue; - fixture.detectChanges(); - browseEntryListElementComponent.queryParams$.pipe(take(1)).subscribe((p) => emitted = p); - }; + const hrefFor = (queryParams: Params): string => { + void router.navigate(['/browse/author'], { queryParams }); + tick(); - it('should keep the scope of the community or collection being browsed', () => { - queryParamsInUrl.scope = '0eb1f4d0-fd7c-4c2c-b0d9-32ee18f5e1c1'; - buildQueryParams(); + fixture = TestBed.createComponent(BrowseEntryListElementComponent); + fixture.componentInstance.object = mockValue; + fixture.detectChanges(); + + return fixture.debugElement.query(By.css('a.lead')).nativeElement.getAttribute('href'); + }; - expect(emitted.scope).toBe('0eb1f4d0-fd7c-4c2c-b0d9-32ee18f5e1c1'); + beforeEach(waitForAsync(() => { + const paginationService = jasmine.createSpyObj('paginationService', { + getPageParam: pageParam }); - it('should keep the page size and sort chosen by the user', () => { - queryParamsInUrl['bbm.rpp'] = '40'; - queryParamsInUrl['bbm.sf'] = 'title'; - queryParamsInUrl['bbm.sd'] = 'DESC'; - buildQueryParams(); + TestBed.configureTestingModule({ + imports: [ + RouterTestingModule.withRoutes([ + { path: 'browse/author', component: DummyComponent } + ]) + ], + declarations: [BrowseEntryListElementComponent, DummyComponent, TruncatePipe], + providers: [ + { provide: 'objectElementProvider', useValue: { mockValue } }, + {provide: PaginationService, useValue: paginationService}, + {provide: Store, useValue: jasmine.createSpyObj('store', ['dispatch'])}, + ], + schemas: [NO_ERRORS_SCHEMA] + }).compileComponents(); + })); - expect(emitted['bbm.rpp']).toBe('40'); - expect(emitted['bbm.sf']).toBe('title'); - expect(emitted['bbm.sd']).toBe('DESC'); - }); + beforeEach(() => { + router = TestBed.inject(Router); + }); + it('should read its parameters through the real RouteService', () => { + expect(TestBed.inject(RouteService) instanceof RouteService).toBeTruthy(); }); - describe('the rendered link', () => { - const scopeUUID = 'a2f2d0a1-3f0e-4d3a-9c1b-5f7e8a9b0c1d'; - - beforeEach(waitForAsync(() => { - // the suite above already instantiated the TestBed, and this block needs a real Router in it - TestBed.resetTestingModule(); - init(); - queryParamsInUrl.scope = scopeUUID; - TestBed.configureTestingModule({ - imports: [ - RouterTestingModule.withRoutes([ - { path: 'browse/author', component: BrowseEntryListElementComponent } - ]) - ], - declarations: [BrowseEntryListElementComponent, TruncatePipe], - providers: [ - { provide: 'objectElementProvider', useValue: { mockValue } }, - {provide: PaginationService, useValue: paginationService}, - {provide: RouteService, useValue: routeService}, - ], - schemas: [NO_ERRORS_SCHEMA] - }).compileComponents(); - })); - - it('should keep the scope but not a parameter the page never asked for', fakeAsync(() => { - const router = TestBed.inject(Router); - router.navigate(['/browse/author'], { - queryParams: { scope: scopeUUID, 'amp;value': 'Some Author' } - }); - tick(); - - fixture = TestBed.createComponent(BrowseEntryListElementComponent); - fixture.componentInstance.object = mockValue; - fixture.detectChanges(); + it('should carry over the scope and the pagination settings', fakeAsync(() => { + const href = hrefFor({ + scope: scopeUUID, + 'bbm.rpp': '40', + 'bbm.sf': 'title', + 'bbm.sd': 'DESC' + }); - const href = fixture.debugElement.query(By.css('a.lead')).nativeElement.getAttribute('href'); - expect(href).toContain(`scope=${scopeUUID}`); - expect(href).not.toContain('amp'); - })); - }); + expect(href).toContain(`scope=${scopeUUID}`); + expect(href).toContain('bbm.rpp=40'); + expect(href).toContain('bbm.sf=title'); + expect(href).toContain('bbm.sd=DESC'); + })); + + it('should replace the current page with the page to return to', fakeAsync(() => { + const href = hrefFor({ 'bbm.page': '3' }); + + expect(href).toContain('bbm.return=3'); + expect(href).not.toContain('bbm.page='); + })); + + it('should drop a parameter the browse page never asked for', fakeAsync(() => { + const href = hrefFor({ scope: scopeUUID, 'amp;value': 'Some Author' }); + + expect(href).toContain(`scope=${scopeUUID}`); + expect(href).not.toContain('amp'); + })); + + it('should not pass on a scope that is present but empty', fakeAsync(() => { + const href = hrefFor({ scope: '' }); + + expect(href).not.toContain('scope='); + })); });