NMS-20180: Fix various menu-related issues after NMS-20174 Review - #8792
Open
synqotik wants to merge 3 commits into
Open
NMS-20180: Fix various menu-related issues after NMS-20174 Review#8792synqotik wants to merge 3 commits into
synqotik wants to merge 3 commits into
Conversation
…s/** The unpacked dist-menu artifact also ships an index.html (the Vite build input) at /ui-components/, which nothing links to and which should not be anonymously reachable; only assets/** needs the ROLE_ANONYMOUS rule from NMS-20174. Document the deliberate login-page preload in bootstrap.jsp, and pin the NMS-20174 access/quiet fixes with WebappIT smoke tests: anonymous asset access with correct content types, index.html requiring authentication, and login.jsp never executing the menu bundle.
… locale Under a Turkish default locale, locale-sensitive toLowerCase() turns "I" into a dotless ı, so a saved "/API" request would slip past the "/api" prefix check added in NMS-20174. Use Locale.ROOT and cover it with a test that runs the matcher under tr-TR.
The "does not cancel navigation for regular items" test only checked that performLogout was not called, so an unconditional preventDefault() (breaking navigation for every self-service item) would still pass. Assert event.defaultPrevented is false so the test covers what its name claims.
synqotik
marked this pull request as ready for review
August 13, 2026 19:56
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.
Fix various issues found in PR review for NMS-20174 (PR #8758) ("Fix Menubar/SideMenu issues in Firefox and Safari on JSP pages").
There were 9 different items. 5 are fixed here. 4 have been deferred as they represent more work or tradeoffs.
Fixed Items
Fix 1 — narrowed anonymous rule:
applicationContext-spring-security.xmlnow grants/ui-components/assets/**instead of/ui-components/**, with a comment explaining why (due to the strayindex.htmlin thedist-menuartifact, NMS-20180)./ui-components/index.htmlnow falls through to the/** → ROLE_USERcatch-all, so it 302s to login like any other page.Fix 3 — comment only: extended the
bootstrap.jsphead comment to mark the login-page stylesheet + modulepreload as deliberate cache-warming, cross-referencing the anonymous rule it depends on.Fix 4 — test gap: added
expect(event.defaultPrevented).toBe(false)to the "does not cancel navigation for regular items" test.FIx 7 —
WebappITsmoke tests (three new tests): anonymous GET of the menu JS/CSS returns 200 with the right content types;ui-components/index.htmlrequires auth (302, pinning fix 1's narrowing); andlogin.jspcontains no<script>tag for the menu bundle — with a positive control asserting the preload link is present so the negative assertion stays meaningful.Fix 9 — locale-safe URL matching (
LoginModuleUtils.java:163):toLowerCase()→toLowerCase(Locale.ROOT), with a comment explaining the Turkishdotless-ıproblem and added tests.Deferred Items
These four items were identified while reviewing the NMS-20174 fixes (PR #8758) but deliberately not fixed in NMS-20180. I will open separate tickets to follow up.
1. Vue menu disappeared from nine popup JSPs (documentation / QA spot-check)
What happened. NMS-20174 fix #2 made
bootstrap.jsphonor both quietmechanisms (the
param.quietinclude parameter and theBootstrap.flags("quiet")request attribute). The PR description only mentions
login.jsp, but nine other JSPsalso set the quiet flag and therefore silently lost the Vue menu with that change:
admin/discovery/add-er.jspadmin/discovery/add-ex-url.jspadmin/discovery/add-ir.jspadmin/discovery/add-specific.jspadmin/discovery/add-url.jspadmin/userGroupView/users/newPassword.jspgraph/forecast.jspgraph/nrtg.jspincludes/legendInfo-box.jspWhy it is probably fine. Every one of these is opened via
window.openas apopup/utility window. Losing the menu there is almost certainly the intended
rendering — and the pre-fix behavior (menu app executing in a popup that had asked
for quiet mode) is a plausible source of the original "menus overlapping content"
symptom.
2. Cache-buster stats the menu bundle files on every legacy page render
What happens today.
includes/bootstrap.jspcomputes the?v=cache-buster forthe menu bundle by calling
application.getRealPath(...)+File.lastModified()for both
index.jsandindex.csson every JSP page render (two filesystemstats per request). The
load-assets.jspprecedent it mirrors reads a cachedAssetLocator.lastModified()instead.Suggested fix. Memoize the computed version in a
ServletContextattribute,keyed on (or invalidated by) the files' mtimes, or re-checked on a short TTL.
Tradeoffs / why it was deferred.
There is no observed performance problem.
workflow (
build-and-copy.sh, ui/CLAUDE.md instructions): newly copied bundleswould keep serving the stale
?v=until a webapp restart. A correct fix needsmtime-keyed or TTL-based invalidation — more code for marginal benefit.
deleted anyway. Fixing this separately is only worthwhile if content-hashing is
rejected or slips.
Action: will consider folding it into the content-hashing work if/when we proceed with that./
3. Content-hashed filenames for the menu bundle (replace
?v=<mtime>)What happens today. The menu bundle is built with fixed filenames
(
ui/vite.config.menu.ts→assets/index.js,assets/index.css) becausebootstrap.jsphardcodes the URLs. Cache correctness relies on a?v=<deployed file mtime>query parameter appended bybootstrap.jsp(added in NMS-20174 fix #6).Why
?v=is not durable.Suggested fix.
Leaving out details, but adding a content hash to all filenames.
Costs / risks.
re-verification
or unreadable manifest must fail loudly with a sane fallback, or the menu breaks.
directory (already true for the main SPA — ui/CLAUDE.md documents the occasional
rm assets/*.*), and the manifest must be copied along with the assets.filenames.
Action: Will open a separate ticket to do the full content-hash + manifest change, targeted at the next
major release (Horizon 37).
4. Root fix for REST/API requests polluting the Spring Security request cache
Omitting details, but this is a larger effort as part of fixing
OpenNMSAuthSuccessHandlerandHttpSessionRequestCacheto not saved assets in the cache, and to more properly handle redirects.Action: Will open a separate ticket to do a comprehensive fix in Horizon 37.
External References