TUL/fix(ssr): don't cache non-2XX responses (soft-404 on unknown pages) - #1463
Closed
Kasinhou wants to merge 1 commit into
Closed
TUL/fix(ssr): don't cache non-2XX responses (soft-404 on unknown pages)#1463Kasinhou wants to merge 1 commit into
Kasinhou wants to merge 1 commit into
Conversation
An unknown route or `/static/<missing>` page renders the 404 page correctly, but `saveToCache()` stored that rendered page in the bot/anonymous SSR cache without checking the status code. On the next request the cached copy was replayed via `res.send(cachedCopy)` (which does not restore the status), so a correct 404 turned into HTTP 200 - a soft-404. UNIVERSAL-016 (dspace-ui-tests notFoundPage.spec.ts, both "non-existent route" and "non-existent static page") therefore failed on tul with 200 for the well-known test URIs, while a fresh/cache-busted path still returned 404. Skip caching when the response status is not 2XX (add the `hasNotSucceeded` guard already used on dtq-dev/jcu). The 404 page is then never cached and can no longer be served as 200. Note: a redeploy/restart is still needed once to clear the stale 200 entries already sitting in the in-memory cache; this guard prevents them recurring. Refs dataquest-dev/dspace-customers#566 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR prevents SSR from caching rendered error pages (notably 404s) so that cached content can’t later be replayed with an incorrect HTTP 200 status (“soft-404”) in the TUL deployment.
Changes:
- Add a guard in
saveToCache()to skip caching when the current response status is not successful (non-2xx). - Introduce a
hasNotSucceeded()helper intended to detect non-2xx status codes.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+470
to
+473
| function hasNotSucceeded(statusCode) { | ||
| const rgx = new RegExp(/^20+/); | ||
| return !rgx.test(statusCode); | ||
| } |
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.
Fixes soft-404 on tul: an unknown route /
/static/<missing>renders the 404 page but the SSR cache replayed it as HTTP 200.saveToCache()stored the rendered page without checking status; serving a cached copy (res.send(cachedCopy)) does not restore the status. Add thehasNotSucceededguard (as on dtq-dev/jcu) so non-2XX responses are never cached. tul has no StaticPageComponent, so both notFoundPage tests (route + static) share this 404 route and this one fix covers both. Note: a one-time redeploy clears the stale 200 cache entries; the guard prevents recurrence. Refs dataquest-dev/dspace-customers#566