TUL/Browse links: carry only the parameters a browse page uses - #1453
Open
milanmajchrak wants to merge 3 commits into
Open
TUL/Browse links: carry only the parameters a browse page uses#1453milanmajchrak wants to merge 3 commits into
milanmajchrak wants to merge 3 commits into
Conversation
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 DSpace#2735 but without carrying scope, which broke scoped browse (DSpace#5209, still open). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
milanmajchrak
force-pushed
the
fix/tul-ssr-exclude-browse
branch
from
August 17, 2026 07:21
fc1e089 to
5ea7bb6
Compare
milanmajchrak
changed the title
There was a problem hiding this comment.
Pull request overview
Restricts browse-entry links to supported browse parameters, preventing unbounded reflection of unknown query parameters.
Changes:
- Explicitly preserves scope and pagination/sort parameters.
- Removes query-parameter merging from browse-entry links.
- Adds query-parameter unit tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
browse-entry-list-element.component.ts |
Builds an explicit query-parameter allowlist. |
browse-entry-list-element.component.html |
Disables query-parameter merging. |
browse-entry-list-element.component.spec.ts |
Tests parameter projection behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
References
Description
pr-1453-explained.html
Browse value links copied the whole current query string into themselves. They now carry only the parameters a browse page actually uses.
Instructions for Reviewers
A parameter nobody asked for used to come back in all 21 links on a browse page. A crawler that does not decode HTML entities then reads the escaped
&as part of the next parameter name:?zzz=1comes back as&zzz=1, gets requested asamp;zzz=1, comes back asamp;amp;zzz=1, and so on without end, because every one of those is a valid 200 page with 21 fresh links. On TUL this reached 11.7M such requests against 7.7M real ones, chains 35 levels deep, and three days of backend at 130 % CPU.List of changes in this PR:
browse-entry-list-element.component.html: dropped[queryParamsHandling]="'merge'". That was the only route by which an unknown parameter entered the link.browse-entry-list-element.component.ts:getQueryParams()now also readsscope,bbm.rpp,bbm.sfandbbm.sd, the parametersmergeused to bring along. Anything else in the URL is dropped.RouteServicemock answers per parameter name, and one test asserts on the renderedhrefwith a realRouter, since the reflection happened insideRouterLinkrather than in the params object.The list comes from the source, not from logs. Browse-by components read
scope,startsWith,valueandauthority;PaginationServicereadsbbm.page,bbm.rpp,bbm.sfandbbm.sd, andbbmis the only pagination id underbrowse-by/.browseDefinitionarrives throughroute.dataviaBrowseByGuard, not the query string. Every other query parameter in the app belongs to search, the comcol create form, submission import or access control, none of which sits on a browse route.Upstream removed the same merge in DSpace#2735 but did not carry
scopewith it, which broke browsing inside a community or collection (DSpace#5209, open, present since 7.6.6). Keepingscopeavoids repeating that.How to test:
curl -s 'https://<host>/browse/author?zzz=1' | grep -c 'zzz=1'prints 0 after the deploy. On an affected instance it is non-zero.?scope=.Only the browse value link changed. Pagination and starts-with links keep their own merge, so anything legitimately in the URL still survives a click through those. Steps 2 and 3 have not been run in a browser yet, they need a deploy; everything else is covered by CI and the specs.
Considered and dropped: porting the SSR exclusion of
/browsefrom 7.6.5 (DSpace#4332). It serves the page as a CSR shell, so the reflection stays and browse pages leave the index for crawlers that do not run JavaScript. Reasonable as a separate performance change, not as the fix for this.Checklist
mainbranch of code (againstcustomer/TUL, which runs 7.5.0)npm run lintnpm run check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation. (none added)