fix: stop dropping cache:false on /api/security/** - #2144
Merged
Conversation
/api/security/** was declared in both top-level routeRules and nitro.routeRules; Nitro doesn't merge duplicate keys, so the nitro entry silently replaced cache:false, letting the route fall through to the redis catch-all. Nitro's cachedEventHandler then stripped the POST body before update.post.ts ever read it, causing a 500 on every "Update results" click. Collapse the duplicate declarations into one, remove dead *.post/.put/.delete/.patch rules that never matched, guard against undefined body in the handler, and stop masking 404/400 errors as 500 in the catch block. Signed-off-by: Efren Lim <elim@linuxfoundation.org>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes production failures when triggering security updates by preserving no-cache rules and improving API error handling.
Changes:
- Consolidates duplicate caching rules.
- Adds route-rule regression tests.
- Improves request-body and typed-error handling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
frontend/setup/caching.ts |
Consolidates API caching configuration. |
frontend/setup/caching.test.ts |
Tests cache rules and duplicate patterns. |
frontend/server/api/security/update.post.ts |
Guards missing bodies and preserves typed errors. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const { slug } = body; | ||
| // Sanitize repoUrl for use in workflowId (replace non-alphanumeric chars with dashes). | ||
| // Computed before the try so it's available to the catch block's error log. | ||
| const sanitizedRepo = body.repoUrl.replace(/[^a-zA-Z0-9]/g, '-').replace(/-+/g, '-'); |
Comment on lines
+128
to
+131
| // Re-throw errors already thrown above (404 project not found, 400 repo mismatch) instead | ||
| // of masking them as a generic 500 below. | ||
| if (err && typeof err === 'object' && 'statusCode' in err) { | ||
| throw err; |
Comment on lines
+11
to
13
| afterEach(() => { | ||
| process.env.NUXT_APP_ENV = originalAppEnv; | ||
| }); |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
frontend/setup/caching.test.ts:12
- When
NUXT_APP_ENVwas initially unset, assigningundefinedtoprocess.envstores the literal string"undefined"instead of removing the variable. This leaves the worker with a mutated environment after the suite and can affect later tests; delete the key when there was no original value.
process.env.NUXT_APP_ENV = originalAppEnv;
gaspergrom
approved these changes
Sep 1, 2026
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.
Summary
Fixes DE-1044 — clicking Update results on the Security & Best Practices page returns a 500 for every project.
/api/security/**(and/api/**) was declared in both top-levelrouteRulesandnitro.routeRulesinfrontend/setup/caching.ts. Nitro doesn't merge two rules for the same exact pattern — the later one replaces the earlier wholesale — socache: falsewas silently dropped and the route fell through to the/**catch-all's redis cache.cachedEventHandlerwrapper then clones the incoming POST request without preservingcontent-length/content-type, soreadBody()inupdate.post.tsreturnedundefined, andbody.slugthrew an unhandledTypeError→ 500.cachedEventHandler), not guessed.Changes
frontend/setup/caching.ts: collapsed the duplicated route-rule keys into single declarations; deleted dead/api/**/*.post|.put|.delete|.patchrules that never matched (route rules match paths, not handler filenames).frontend/server/api/security/update.post.ts: guard against an undefined body (body?.slug), computeworkflowIdbefore thetryso it's available for error logging, and re-throw already-typed errors (404 project not found, 400 repo mismatch) instead of masking them as a generic 500.frontend/setup/caching.test.ts: added a direct regression test asserting/api/security/**keepscache: falsein production, plus a static-analysis test guarding against any route pattern being declared twice.Test plan
pnpm test setup/caching.test.ts— 6/6 passing, including the new DE-1044 regression testpnpm tsc-check— cleanpnpm lint— 0 errorsNUXT_APP_ENV=production pnpm build— confirmed compilednitro.mjsnow has"cache": falsealongside headers for/api/security/**/project/electron-fdc3/repository/finos_fdc3-sail/security— expect success toast and a Temporal workflow started; a second immediate click should return a 429 "already in progress" toast, not a 500🤖 Generated with Claude Code