From 4bc29cf94e2e635a2b1e3640e20c0baa33e44f19 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 19 Aug 2026 02:31:17 +0200 Subject: [PATCH 1/7] fix(server/static): serve a directory's own index instead of the SPA shell resolveStaticFilePath treated any path that stats as a directory the same as a missing file and fell through to the SPA fallback, so /docs/ returned the dashboard rather than dist/docs/index.html. The "API Docs" header link was broken on both the HTTP and the Lambda transport. Resolve /index.html first, re-running symlinkSafeContainedIn on the candidate: appending a constant filename keeps the path lexically inside the static dir, but os.Stat follows symlinks, so without the check /docs/ would serve a file that the direct /docs/index.html request rejects. docs.html now references its stylesheet absolutely so the page styles correctly whether it is reached as /docs or /docs/. Pins the wider contract while here: a table test asserts every route the SPA defines (tabs, sub-tabs, legacy redirects, the non-tab landing paths) still falls back to index.html on both transports, so a future root-level mux registration cannot silently shadow one. Refs #1775 --- frontend/src/docs.html | 3 +- internal/server/static.go | 53 +++++++++++--- internal/server/static_test.go | 122 +++++++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+), 11 deletions(-) diff --git a/frontend/src/docs.html b/frontend/src/docs.html index b9411770b..3620ef01e 100644 --- a/frontend/src/docs.html +++ b/frontend/src/docs.html @@ -5,7 +5,8 @@ CUDly API Documentation - + +
diff --git a/internal/server/static.go b/internal/server/static.go index cc0c8f1d2..fafb186c4 100644 --- a/internal/server/static.go +++ b/internal/server/static.go @@ -79,6 +79,37 @@ func symlinkSafeContainedIn(absDir, absFile string) bool { return true } +// directoryIndex resolves dirPath/index.html when the requested path is a +// directory that ships its own index. /docs/ must serve dist/docs/index.html, +// not the SPA shell: without this a directory stat falls straight through to +// the SPA fallback and the "API Docs" link renders the dashboard again. +// +// The candidate re-runs symlinkSafeContainedIn. Appending a constant filename +// keeps the path lexically inside absDir, but os.Stat follows symlinks, so a +// symlinked index.html could otherwise serve a file the direct request path +// (/docs/index.html) rejects. +func directoryIndex(absDir, dirPath, cleanPath string) (indexPath, indexClean string, ok bool) { + candidate := filepath.Join(dirPath, "index.html") + absCandidate, err := filepath.Abs(candidate) + if err != nil || !symlinkSafeContainedIn(absDir, absCandidate) { + return "", "", false + } + info, err := os.Stat(candidate) + if err != nil || info.IsDir() { + return "", "", false + } + return candidate, path.Join(cleanPath, "index.html"), true +} + +// spaIndex returns the SPA shell that client-side routes fall back to. +func spaIndex(dir string) (filePath, cleanPath string, ok bool) { + filePath = filepath.Join(dir, "index.html") + if _, err := os.Stat(filePath); err != nil { + return "", "", false + } + return filePath, "/index.html", true +} + // resolveStaticFilePath validates the URL path against directory traversal and // resolves the actual file path. Falls back to index.html for extensionless // paths (SPA routing). Returns the file path, the clean path used for content @@ -103,20 +134,22 @@ func resolveStaticFilePath(dir, urlPath string) (filePath, cleanPath string, ok return "", "", false } - info, err := os.Stat(filePath) //nolint:gosec // G703: error from Close handled in defer - if err != nil || info.IsDir() { - if path.Ext(cleanPath) != "" { - return "", "", false + if info, statErr := os.Stat(filePath); statErr == nil { + if !info.IsDir() { + return filePath, cleanPath, true } - // SPA fallback - filePath = filepath.Join(dir, "index.html") - cleanPath = "/index.html" - if _, err := os.Stat(filePath); err != nil { //nolint:gosec // G703: error from Close handled in defer - return "", "", false + if idxPath, idxClean, isDirIndex := directoryIndex(absDir, filePath, cleanPath); isDirIndex { + return idxPath, idxClean, true } } - return filePath, cleanPath, true + // Nothing servable at that path. An extension means the caller asked for a + // concrete asset, so a miss is a genuine 404; an extensionless path is a + // client-side route and gets the SPA shell. + if path.Ext(cleanPath) != "" { + return "", "", false + } + return spaIndex(dir) } // cacheControlForExt returns the Cache-Control header value for a file extension. diff --git a/internal/server/static_test.go b/internal/server/static_test.go index fa2199383..2de3bd1ef 100644 --- a/internal/server/static_test.go +++ b/internal/server/static_test.go @@ -334,3 +334,125 @@ func TestSpaFileServer_404WhenIndexMissing(t *testing.T) { testutil.AssertEqual(t, http.StatusNotFound, w.Code) } + +// ----- SPA catch-all: every frontend route (issue #1775) ----- + +// appRoutes lists every path the SPA can be deep-linked to, taken from the +// tab table and the legacy-redirect table in frontend/src/navigation.ts. A new +// nav entry (or a new root-level mux registration that shadows one) must not +// silently start serving something other than the SPA shell. +var appRoutes = []string{ + "/", + "/home", + "/opportunities", + "/plans", + "/purchases", + "/inventory", + "/inventory/active-commitments", + "/inventory/coverage", + "/inventory/ri-exchange", + "/admin", + "/admin/general", + "/admin/purchasing", + "/admin/accounts", + "/admin/users", + // Legacy paths kept alive by LEGACY_PATH_REDIRECTS for old bookmarks. + "/dashboard", + "/recommendations", + "/history", + "/settings", + "/ri-exchange", + // Non-tab SPA landing paths. + "/reset-password", + "/archera-insurance", + "/purchases/approve/abc-123", + "/purchases/cancel/abc-123", +} + +func TestSPAFallbackServesIndexForEveryAppRoute(t *testing.T) { + dir := makeStaticDir(t, map[string]string{ + "index.html": "spa", + "docs/index.html": "docs", + }) + + for _, route := range appRoutes { + t.Run(route, func(t *testing.T) { + if !isStaticPath(route) { + t.Fatalf("route %s is not classified as a static path; the API router would swallow it", route) + } + + content, _, _, found := serveStaticForLambda(dir, route) + testutil.AssertEqual(t, true, found) + testutil.AssertEqual(t, "spa", string(content)) + + handler := spaFileServer(dir) + req := httptest.NewRequestWithContext(context.Background(), http.MethodGet, route, nil) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + testutil.AssertEqual(t, http.StatusOK, w.Code) + testutil.AssertEqual(t, "spa", w.Body.String()) + }) + } +} + +// The API docs page is a real directory in the build output. Before #1775 a +// directory stat fell straight through to the SPA fallback, so the "API Docs" +// header link (href="/docs/") rendered the dashboard instead. +func TestResolveStaticFilePath_DirectoryIndex(t *testing.T) { + dir := makeStaticDir(t, map[string]string{ + "index.html": "spa", + "docs/index.html": "docs", + }) + + for _, route := range []string{"/docs", "/docs/"} { + t.Run(route, func(t *testing.T) { + content, _, _, found := serveStaticForLambda(dir, route) + testutil.AssertEqual(t, true, found) + testutil.AssertEqual(t, "docs", string(content)) + }) + } +} + +// A directory without its own index.html still falls back to the SPA shell, so +// asset directories (dist/js, dist/css) do not 404 into a broken page. +func TestResolveStaticFilePath_DirectoryWithoutIndexFallsBackToSPA(t *testing.T) { + dir := makeStaticDir(t, map[string]string{ + "index.html": "spa", + "js/app.js": "var x=1;", + }) + + content, _, _, found := serveStaticForLambda(dir, "/js") + testutil.AssertEqual(t, true, found) + testutil.AssertEqual(t, "spa", string(content)) +} + +// A directory index reached via /docs/ must not serve a file the direct +// request path (/docs/index.html) would reject. os.Stat follows symlinks, so +// the containment check has to run on the candidate, not just on the directory. +func TestResolveStaticFilePath_DirectoryIndexRejectsSymlinkEscape(t *testing.T) { + parent := t.TempDir() + staticDir := filepath.Join(parent, "static", "docs") + if err := os.MkdirAll(staticDir, 0o755); err != nil { + t.Fatalf("mkdir docs: %v", err) + } + root := filepath.Join(parent, "static") + if err := os.WriteFile(filepath.Join(root, "index.html"), []byte("spa"), 0o644); err != nil { + t.Fatalf("write index.html: %v", err) + } + outside := filepath.Join(parent, "secret.html") + if err := os.WriteFile(outside, []byte("should-not-serve"), 0o644); err != nil { + t.Fatalf("write secret.html: %v", err) + } + if err := os.Symlink(outside, filepath.Join(staticDir, "index.html")); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + // Both spellings must agree, and neither may serve the out-of-tree file. + direct, _, _, directFound := serveStaticForLambda(root, "/docs/index.html") + testutil.AssertEqual(t, false, directFound) + testutil.AssertEqual(t, "", string(direct)) + + viaDir, _, _, viaDirFound := serveStaticForLambda(root, "/docs/") + testutil.AssertEqual(t, true, viaDirFound) + testutil.AssertEqual(t, "spa", string(viaDir)) +} From 293648b54b515c63f53055395fcab92538da8602 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 19 Aug 2026 02:35:19 +0200 Subject: [PATCH 2/7] fix(frontend): resolve every route to its own page on direct load Three route-resolution defects, all of which render the Home dashboard in place of the page the user asked for. `key in record` consults the prototype chain, so a first path segment naming an Object.prototype member passed the membership test against the tab tables. /constructor resolved as a known tab, TABS['constructor'] came back as the Object constructor, and pushing that value threw DataCloneError out of init() before switchTab ran. The app was left on its unrouted markup: Home panel, Home nav highlight, the generic document title, and no event listeners bound. Use an own-property test at all five lookup sites. init() rebuilt the canonical URL inline with an Admin-only special case, so a deep-linked /inventory/ was truncated to /inventory before switchTab read the segment back out of the URL, landing the user on the default sub-section. canonicalTabPath now owns that URL for both the initial replaceState and switchTab's pushState, so the two cannot drift. viewPlanHistory and the "Exchange settings" jump still passed the pre-#340 tab names 'history' and 'settings', which are no longer known tabs and fell back to Home. The Plans page's "View history" button showed the Home dashboard; the Exchange settings button showed it under an Admin title and an /admin/purchasing URL. Routing viewPlanHistory to the Purchases tab needs skipDefaultLoad so the tab's own unscoped 7-day fetch cannot land after the plan-scoped one and overwrite it. The permission gate still applies, and the dual-control config now refreshes on both paths: the approval queue must never render as though four-eyes were off, which would offer a creator an Approve button on their own pending purchase. Closes #1775 --- frontend/src/__tests__/app.test.ts | 22 +++- frontend/src/__tests__/history.test.ts | 24 +++- frontend/src/__tests__/navigation.test.ts | 133 +++++++++++++++++++++- frontend/src/__tests__/riexchange.test.ts | 7 +- frontend/src/app.ts | 10 +- frontend/src/history.ts | 48 +++++--- frontend/src/navigation.ts | 63 ++++++---- frontend/src/riexchange.ts | 4 +- 8 files changed, 263 insertions(+), 48 deletions(-) diff --git a/frontend/src/__tests__/app.test.ts b/frontend/src/__tests__/app.test.ts index d7b9bca71..7a59b7719 100644 --- a/frontend/src/__tests__/app.test.ts +++ b/frontend/src/__tests__/app.test.ts @@ -38,7 +38,10 @@ jest.mock('../navigation', () => ({ applyTabFromPath: jest.fn().mockReturnValue('dashboard'), initRouter: jest.fn(), switchSettingsSubTab: jest.fn(), - getSettingsSubTabFromPath: jest.fn().mockReturnValue('general'), + // Deliberately not '/' + tab: init() must write whatever canonicalTabPath + // returns into the initial replaceState, because that is what preserves a + // deep-linked sub-tab segment for switchTab to read back. + canonicalTabPath: jest.fn((tab: string) => `/${tab}/sub-segment`), })); jest.mock('../recommendations', () => ({ @@ -110,6 +113,23 @@ describe('App Module', () => { expect(auth.updateUserUI).toHaveBeenCalled(); }); + test('seeds the URL from canonicalTabPath before routing', async () => { + (api.isAuthenticated as jest.Mock).mockReturnValue(true); + (api.getCurrentUser as jest.Mock).mockResolvedValue({ id: 'user-1', email: 'test@example.com' }); + (navigation.applyTabFromPath as jest.Mock).mockReturnValue('inventory'); + const replaceState = jest.spyOn(window.history, 'replaceState').mockImplementation(() => {}); + + await init(); + + expect(navigation.canonicalTabPath).toHaveBeenCalledWith('inventory'); + expect(replaceState).toHaveBeenCalledWith( + { tab: 'inventory', id: 0 }, + '', + expect.stringContaining('/inventory/sub-segment'), + ); + replaceState.mockRestore(); + }); + test('shows login modal on 401 error', async () => { (api.isAuthenticated as jest.Mock).mockReturnValue(true); (api.getCurrentUser as jest.Mock).mockRejectedValue({ status: 401 }); diff --git a/frontend/src/__tests__/history.test.ts b/frontend/src/__tests__/history.test.ts index 7e0830f81..ce78dce14 100644 --- a/frontend/src/__tests__/history.test.ts +++ b/frontend/src/__tests__/history.test.ts @@ -13,6 +13,13 @@ jest.mock('../navigation', () => ({ switchTab: jest.fn() })); +// Default: the session may view purchases. viewPlanHistory consults this +// before rendering into the Purchases tab, because switchTab renders a +// no-access placeholder there for sessions without view:purchases. +jest.mock('../permissions', () => ({ + canAccess: jest.fn().mockReturnValue(true), +})); + jest.mock('../utils', () => ({ // Mirrors the real formatCurrency behaviour: null/undefined/NaN -> '--', numbers -> '$' formatCurrency: jest.fn((val) => (val === null || val === undefined || isNaN(val)) ? '--' : `$${val}`), @@ -57,6 +64,7 @@ jest.mock('../state', () => ({ import * as api from '../api'; import { switchTab } from '../navigation'; +import { canAccess } from '../permissions'; describe('History Module', () => { beforeEach(() => { @@ -122,7 +130,10 @@ describe('History Module', () => { }); describe('viewPlanHistory', () => { - test('switches to history tab', async () => { + // Issue #1775: 'history' was the pre-#340 tab name, so this silently fell + // back to Home and the Plans page's "View history" button rendered the + // Home dashboard. + test('switches to the Purchases tab without its default load', async () => { (api.getHistory as jest.Mock).mockResolvedValue({ summary: {}, purchases: [] @@ -130,7 +141,16 @@ describe('History Module', () => { await viewPlanHistory('plan-123'); - expect(switchTab).toHaveBeenCalledWith('history'); + expect(switchTab).toHaveBeenCalledWith('purchases', { skipDefaultLoad: true }); + }); + + test('does not fetch when the session cannot view purchases', async () => { + (canAccess as jest.Mock).mockReturnValueOnce(false); + + await viewPlanHistory('plan-123'); + + expect(switchTab).toHaveBeenCalledWith('purchases', { skipDefaultLoad: true }); + expect(api.getHistory).not.toHaveBeenCalled(); }); test('calls getHistory with planId filter', async () => { diff --git a/frontend/src/__tests__/navigation.test.ts b/frontend/src/__tests__/navigation.test.ts index afa3f58da..1ab02d934 100644 --- a/frontend/src/__tests__/navigation.test.ts +++ b/frontend/src/__tests__/navigation.test.ts @@ -1,7 +1,7 @@ /** * Navigation module tests */ -import { switchTab, switchSettingsSubTab, switchInventorySubTab, getSettingsSubTabFromPath, getInventorySubTabFromPath } from '../navigation'; +import { switchTab, switchSettingsSubTab, switchInventorySubTab, getSettingsSubTabFromPath, getInventorySubTabFromPath, applyTabFromPath, canonicalTabPath } from '../navigation'; // Mock the dependent modules jest.mock('../dashboard', () => ({ @@ -61,6 +61,7 @@ import { loadGlobalSettings } from '../settings'; import { loadAutomationSettings } from '../riexchange'; import { canAccess } from '../permissions'; import { loadInventory } from '../inventory'; +import { isAdmin } from '../auth'; describe('Navigation Module', () => { beforeEach(() => { @@ -94,8 +95,13 @@ describe('Navigation Module', () => { `; - // Clear all mocks + // Clear all mocks. clearAllMocks() only drops recorded calls, not + // implementations, so restore the permissive defaults explicitly -- + // otherwise a test that flips isAdmin/canAccess to false leaks that into + // every test declared after it. jest.clearAllMocks(); + (isAdmin as jest.Mock).mockReturnValue(true); + (canAccess as jest.Mock).mockReturnValue(true); }); describe('switchTab', () => { @@ -437,6 +443,129 @@ describe('Navigation Module', () => { }); }); + // Issue #1775: resolving the URL on a direct load / refresh. Placed BEFORE + // the *FromPath describes for the same reason as switchInventorySubTab -- + // those replace window.location with a plain object. + describe('applyTabFromPath', () => { + test.each([ + ['/home', 'home'], + ['/opportunities', 'opportunities'], + ['/plans', 'plans'], + ['/purchases', 'purchases'], + ['/inventory', 'inventory'], + ['/admin', 'admin'], + ['/', 'home'], + ['/plans/', 'plans'], + ['/PLANS', 'plans'], + ['/plans/extra/segments', 'plans'], + ['/not-a-route', 'home'], + ])('%s resolves to the %s tab', (path, expected) => { + window.history.replaceState(null, '', path); + expect(applyTabFromPath()).toBe(expected); + }); + + test.each([ + ['/dashboard', 'home'], + ['/recommendations', 'opportunities'], + ['/history', 'purchases'], + ['/settings', 'admin'], + ['/ri-exchange', 'inventory'], + ])('legacy %s redirects to the %s tab and rewrites the URL', (path, expected) => { + window.history.replaceState(null, '', path); + expect(applyTabFromPath()).toBe(expected); + expect(window.location.pathname).toBe('/' + expected); + }); + + // A path whose first segment names an Object.prototype member used to pass + // the `segment in TABS` membership test, so switchTab then read an + // undefined TabMeta (or the Object constructor) and threw out of init(), + // stranding the app on its unrouted default markup. + test.each(['/constructor', '/toString', '/valueOf', '/hasOwnProperty', '/__proto__'])( + '%s is not a known route and falls back to home', + (path) => { + window.history.replaceState(null, '', path); + expect(applyTabFromPath()).toBe('home'); + }, + ); + + test('an Object.prototype sub-tab name is not a known admin sub-tab', () => { + window.history.replaceState(null, '', '/admin/constructor'); + expect(getSettingsSubTabFromPath()).toBe('general'); + switchSettingsSubTab(getSettingsSubTabFromPath(), { push: false }); + expect(document.title).toBe('CUDly — Admin · General'); + }); + }); + + // canonicalTabPath is what app.ts writes into the initial replaceState, so + // it must keep the sub-tab segment switchTab reads back out of the URL. + describe('canonicalTabPath', () => { + test.each(['home', 'opportunities', 'plans', 'purchases'])( + '%s has no sub-tab segment', + (tab) => { + window.history.replaceState(null, '', '/' + tab); + expect(canonicalTabPath(tab)).toBe('/' + tab); + }, + ); + + test.each(['active-commitments', 'coverage', 'ri-exchange'])( + 'a deep-linked /inventory/%s keeps its sub-tab segment', + (sub) => { + window.history.replaceState(null, '', '/inventory/' + sub); + expect(canonicalTabPath('inventory')).toBe('/inventory/' + sub); + }, + ); + + test('a bare /inventory canonicalises to the default sub-tab', () => { + window.history.replaceState(null, '', '/inventory'); + expect(canonicalTabPath('inventory')).toBe('/inventory/active-commitments'); + }); + + test.each(['general', 'purchasing', 'accounts', 'users'])( + 'a deep-linked /admin/%s keeps its sub-tab segment', + (sub) => { + window.history.replaceState(null, '', '/admin/' + sub); + // currentSettingsSubTab is module state; drive it through the real + // entry point so the assertion reflects an actual navigation. + switchSettingsSubTab(sub, { push: false }); + expect(canonicalTabPath('admin')).toBe('/admin/' + sub); + }, + ); + + // app.ts calls this at init, before anything has set currentSettingsSubTab, + // so the URL is the only source for the segment. A fresh module instance is + // the only way to observe that state from inside this file. + test('falls back to the URL when no admin sub-tab has been visited yet', () => { + window.history.replaceState(null, '', '/admin/users'); + jest.isolateModules(() => { + // eslint-disable-next-line @typescript-eslint/no-require-imports + const nav = require('../navigation') as typeof import('../navigation'); + expect(nav.canonicalTabPath('admin')).toBe('/admin/users'); + }); + }); + }); + + describe('switchTab skipDefaultLoad', () => { + test('purchases: reveals the tab without firing its default loads', () => { + switchTab('purchases', { skipDefaultLoad: true, push: false }); + expect(document.getElementById('purchases-tab')?.classList.contains('active')).toBe(true); + expect(initHistoryDateRange).not.toHaveBeenCalled(); + expect(loadHistory).not.toHaveBeenCalled(); + }); + + test('purchases: the view:purchases gate still applies', () => { + (canAccess as jest.Mock).mockReturnValue(false); + switchTab('purchases', { skipDefaultLoad: true, push: false }); + expect(document.getElementById('purchases-tab')?.textContent).toContain('do not have access'); + expect(loadHistory).not.toHaveBeenCalled(); + }); + + test('admin: reveals the tab without loading the default sub-tab', () => { + switchTab('admin', { skipDefaultLoad: true, push: false }); + expect(document.getElementById('admin-tab')?.classList.contains('active')).toBe(true); + expect(loadGlobalSettings).not.toHaveBeenCalled(); + }); + }); + describe('getSettingsSubTabFromPath', () => { // Canonical /admin/* paths (issue #340 IA rename) test('returns general for root admin path', () => { diff --git a/frontend/src/__tests__/riexchange.test.ts b/frontend/src/__tests__/riexchange.test.ts index a63a3cffc..461c3309e 100644 --- a/frontend/src/__tests__/riexchange.test.ts +++ b/frontend/src/__tests__/riexchange.test.ts @@ -673,11 +673,14 @@ describe('⚙︎ Exchange settings deep-link', () => { jest.resetAllMocks(); }); - it('switches to Settings → Purchasing when clicked', () => { + // Issue #1775: 'settings' was the pre-#340 tab name, so switchTab fell back + // to Home while switchSettingsSubTab still set the Admin title and pushed + // /admin/purchasing -- URL and title said Admin over the Home dashboard. + it('switches to Admin → Purchasing when clicked', () => { setupRIExchangeHandlers(); const btn = document.getElementById('ri-exchange-settings-btn')!; btn.click(); - expect(navigation.switchTab).toHaveBeenCalledWith('settings'); + expect(navigation.switchTab).toHaveBeenCalledWith('admin', { push: false, skipDefaultLoad: true }); expect(navigation.switchSettingsSubTab).toHaveBeenCalledWith('purchasing'); }); }); diff --git a/frontend/src/app.ts b/frontend/src/app.ts index 27b449543..bbeb3f183 100644 --- a/frontend/src/app.ts +++ b/frontend/src/app.ts @@ -7,7 +7,7 @@ import * as state from './state'; import { showLoginModal, showAdminSetupModal, showResetPasswordModal, updateUserUI } from './auth'; import { loadDashboard, setupDashboardHandlers } from './dashboard'; import { setupRecommendationsHandlers, getPurchaseModalRecommendations, clearPurchaseModalRecommendations, getFanOutBuckets, clearFanOutBuckets, getExecuteMode, clearExecuteMode, type FanOutBucket } from './recommendations'; -import { switchTab, applyTabFromPath, initRouter, switchSettingsSubTab, getSettingsSubTabFromPath } from './navigation'; +import { switchTab, applyTabFromPath, initRouter, switchSettingsSubTab, canonicalTabPath } from './navigation'; import { savePlan, setupPlanHandlers, closePlanModal, openNewPlanModal, closePurchaseModal } from './plans'; import { saveGlobalSettings, setupSettingsHandlers, resetSettings } from './settings'; import { setupUserHandlers } from './users'; @@ -91,14 +91,12 @@ export async function init(): Promise { // tab routing still runs underneath so the app is fully functional. handleArcheraDeeplink(); const target = applyTabFromPath(); - let url = '/' + target; - if (target === 'admin') { - url = '/admin/' + getSettingsSubTabFromPath(); - } + // switchTab below re-reads the sub-tab from the URL, so this rewrite must + // keep the segment of a deep-linked /inventory/ or /admin/. window.history.replaceState( { tab: target, id: 0 }, '', - url + window.location.search + window.location.hash, + canonicalTabPath(target) + window.location.search + window.location.hash, ); switchTab(target, { push: false }); setupEventListeners(); diff --git a/frontend/src/history.ts b/frontend/src/history.ts index 2615de798..4e6b14382 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -42,11 +42,28 @@ type StatusFilter = 'all' | 'pending' | 'completed' | 'failed' | 'expired' | 'ca let lastPurchases: HistoryPurchase[] = []; let activeStatusFilter: StatusFilter = 'all'; -// _fourEyesMode mirrors GlobalConfig.require_different_approver (issue #1005), -// refreshed on every loadHistory() call. Gates the inline Approve button so a -// creator can't approve their own pending purchase when dual-control is on. +// _fourEyesMode mirrors GlobalConfig.require_different_approver (issue #1005). +// Gates the inline Approve button so a creator can't approve their own pending +// purchase when dual-control is on. let _fourEyesMode = false; +/** + * Refresh _fourEyesMode and the dual-control banner from GlobalConfig. + * + * Every path that renders the approval queue must call this first, or the + * queue renders as though dual control were off and offers the creator an + * Approve button the backend will reject. A failed fetch keeps the previous + * value rather than throwing, so a config blip cannot block the render. + */ +async function refreshFourEyesMode(): Promise { + const cfgResponse = await api.getConfig().catch(() => null); + if (cfgResponse?.global) { + _fourEyesMode = cfgResponse.global.require_different_approver === true; + } + const banner = document.getElementById('four-eyes-banner'); + if (banner) banner.classList.toggle('hidden', !_fourEyesMode); +} + function normalizeStatus(p: HistoryPurchase): string { // Absent status → legacy DB row → counts as completed for filtering. return p.status || 'completed'; @@ -223,10 +240,19 @@ export function initHistoryDateRange(): void { * would be misleading. */ export async function viewPlanHistory(planId: string): Promise { - switchTab('history'); + // skipDefaultLoad: the tab's own unscoped 7-day fetch would land after the + // plan-scoped one below and overwrite it, and its date-range seeding is + // exactly what the doc comment above says not to do here. + switchTab('purchases', { skipDefaultLoad: true }); + // switchTab renders a no-access placeholder for sessions without + // view:purchases; don't overwrite it with the plan's purchases. + if (!canAccess('view', 'purchases')) return; try { - const data = await api.getHistory({ planId }) as unknown as HistoryResponse; + const [data] = await Promise.all([ + api.getHistory({ planId }) as unknown as Promise, + refreshFourEyesMode(), + ]); renderHistorySummary(data.summary ?? null); const purchases = data.purchases || []; renderApprovalQueue(purchases); @@ -312,18 +338,10 @@ export async function loadHistory(): Promise { provider, account_ids: accountIDs }; - const [data, cfgResponse] = await Promise.all([ + const [data] = await Promise.all([ api.getHistory(filters) as unknown as Promise, - // 4-eyes mode (issue #1005) lives on GlobalConfig; a failed fetch must - // not block the history render, so this leg fails closed to "no config" - // rather than throwing, and _fourEyesMode falls back to its last value. - api.getConfig().catch(() => null), + refreshFourEyesMode(), ]); - if (cfgResponse?.global) { - _fourEyesMode = cfgResponse.global.require_different_approver === true; - } - const banner = document.getElementById('four-eyes-banner'); - if (banner) banner.classList.toggle('hidden', !_fourEyesMode); renderHistorySummary(data.summary ?? null); const purchases = data.purchases || []; renderApprovalQueue(purchases); diff --git a/frontend/src/navigation.ts b/frontend/src/navigation.ts index af668c21b..014f11e2c 100644 --- a/frontend/src/navigation.ts +++ b/frontend/src/navigation.ts @@ -69,6 +69,39 @@ let historyId = 0; interface SwitchTabOptions { push?: boolean; skipDirtyGuard?: boolean; + /** + * Skip the tab's own default data load, for callers that switch to a tab + * only to render their own scoped view into it. The permission gate still + * applies. + */ + skipDefaultLoad?: boolean; +} + +/** + * Own-property membership test for the route tables above. `key in record` + * also matches Object.prototype members, so /constructor resolved as a known + * tab and switchTab then read a TabMeta that isn't one. + */ +function isKnownKey(record: Record, key: string): boolean { + return Object.prototype.hasOwnProperty.call(record, key); +} + +/** + * The canonical URL path for a tab. Inventory and Admin carry a sub-tab + * segment; every other tab is just /. + * + * Shared by switchTab's pushState and app.ts's initial replaceState: switchTab + * reads the sub-tab back out of the URL, so a caller that writes the URL first + * must not drop the segment. + */ +export function canonicalTabPath(tabName: string): string { + if (tabName === 'admin') { + return '/admin/' + (currentSettingsSubTab ?? getSettingsSubTabFromPath()); + } + if (tabName === 'inventory') { + return '/inventory/' + getInventorySubTabFromPath(); + } + return '/' + tabName; } /** @@ -91,7 +124,7 @@ function renderNoAccess(tabId: string): void { * Switch between tabs */ export function switchTab(tabName: string, opts: SwitchTabOptions = {}): void { - if (!(tabName in TABS)) tabName = 'home'; + if (!isKnownKey(TABS, tabName)) tabName = 'home'; const isSelfSwitch = tabName === currentTab; @@ -129,8 +162,11 @@ export function switchTab(tabName: string, opts: SwitchTabOptions = {}): void { renderNoAccess(`${tabName}-tab`); break; } - initHistoryDateRange(); + // Savings history is tab-scoped, never plan-scoped, so it loads even + // for a caller that brings its own purchase list. void loadSavingsHistory(); + if (opts.skipDefaultLoad) break; + initHistoryDateRange(); // Auto-load history so the Approval queue card and the Purchase // History table populate on first visit, without requiring the // user to click "Load History" just to see pending approvals. @@ -139,6 +175,7 @@ export function switchTab(tabName: string, opts: SwitchTabOptions = {}): void { void loadHistory(); break; case 'admin': + if (opts.skipDefaultLoad) break; switchSettingsSubTab(getSettingsSubTabFromPath(), { push: false }); break; case 'inventory': @@ -159,22 +196,10 @@ export function switchTab(tabName: string, opts: SwitchTabOptions = {}): void { if (opts.push !== false) { historyId += 1; - let url: string; - if (tabName === 'admin') { - url = '/admin/' + (currentSettingsSubTab ?? 'general'); - } else if (tabName === 'inventory') { - // Inventory carries a sub-tab segment in the URL (QA A.4), mirroring - // Admin. loadInventory() above already applied the sub-section from - // the path (or the default); reflect that same segment here so the - // canonical URL is /inventory/, never a bare /inventory. - url = '/inventory/' + getInventorySubTabFromPath(); - } else { - url = '/' + tabName; - } window.history.pushState( { tab: tabName, id: historyId }, '', - url + window.location.search + window.location.hash, + canonicalTabPath(tabName) + window.location.search + window.location.hash, ); } } @@ -189,7 +214,7 @@ export function getSettingsSubTabFromPath(): string { .replace(/\/+$/, '') .split('/'); const sub = (segments[1] ?? '').toLowerCase(); - return sub in SETTINGS_SUBTABS ? sub : 'general'; + return isKnownKey(SETTINGS_SUBTABS, sub) ? sub : 'general'; } /** @@ -242,7 +267,7 @@ export function getInventorySubTabFromPath(): string { * Manages section visibility, load lifecycle, and sub-tab URL history. */ export function switchSettingsSubTab(subTab: string, opts: SwitchTabOptions = {}): void { - if (!(subTab in SETTINGS_SUBTABS)) subTab = 'general'; + if (!isKnownKey(SETTINGS_SUBTABS, subTab)) subTab = 'general'; if ((subTab === 'accounts' || subTab === 'users') && !isAdmin()) { subTab = 'general'; @@ -311,12 +336,12 @@ export function applyTabFromPath(): string { .split('/')[0] ?.toLowerCase() ?? ''; if (segment === '') return 'home'; - if (segment in LEGACY_PATH_REDIRECTS) { + if (isKnownKey(LEGACY_PATH_REDIRECTS, segment)) { const canonical = LEGACY_PATH_REDIRECTS[segment]!; window.history.replaceState(null, '', '/' + canonical + window.location.search + window.location.hash); return canonical; } - return segment in TABS ? segment : 'home'; + return isKnownKey(TABS, segment) ? segment : 'home'; } /** diff --git a/frontend/src/riexchange.ts b/frontend/src/riexchange.ts index 677117b03..74eb8a0d6 100644 --- a/frontend/src/riexchange.ts +++ b/frontend/src/riexchange.ts @@ -194,7 +194,9 @@ export function setupRIExchangeHandlers(): void { const settingsBtn = document.getElementById('ri-exchange-settings-btn'); if (settingsBtn) { settingsBtn.addEventListener('click', () => { - switchTab('settings'); + // switchSettingsSubTab owns the sub-tab DOM, title and history push, so + // switchTab only has to reveal the Admin panel. + switchTab('admin', { push: false, skipDefaultLoad: true }); switchSettingsSubTab('purchasing'); const target = document.getElementById('ri-exchange-automation-settings'); if (target) { From 481644a54d3335c0ddc3dc1f30efd32739d39358 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 19 Aug 2026 02:36:20 +0200 Subject: [PATCH 3/7] test(frontend/e2e): direct-load and refresh coverage for every route The reported failure is only observable on the initial-load path: a client-side nav to the same route works, so any test that clicks the nav item stays green while the bug is live. Each case points the browser straight at the URL and then reloads it, asserting what the user sees: the highlighted nav item, the visible panel, the document title, and the canonical URL, plus no uncaught page error. Covers every tab, both sub-tab families, the legacy redirects, an unknown path, and the Object.prototype segments. Nine of the 24 cases fail against the pre-fix bundle. Refs #1775 --- frontend/tests-e2e/deeplink.spec.ts | 118 ++++++++++++++++++++++++++++ 1 file changed, 118 insertions(+) create mode 100644 frontend/tests-e2e/deeplink.spec.ts diff --git a/frontend/tests-e2e/deeplink.spec.ts b/frontend/tests-e2e/deeplink.spec.ts new file mode 100644 index 000000000..09a201b8a --- /dev/null +++ b/frontend/tests-e2e/deeplink.spec.ts @@ -0,0 +1,118 @@ +/** + * Deep-link / refresh smoke for every route the SPA defines (issue #1775). + * + * The reported failure ("loading /plans directly renders the Home dashboard") + * is only observable on the *initial load* path: a client-side nav to the same + * route works, so any test that clicks the nav item stays green while the bug + * is live. Each case therefore navigates the browser straight at the URL, and + * then reloads it, asserting on what the user actually sees: which nav item is + * highlighted, which panel is on screen, the tab title, and the canonical URL. + * + * `npx serve -s dist` (playwright.config.ts webServer) mirrors the SPA + * fallback the Go static handler performs, so a failure here is a client + * routing failure. The server half of the same contract is pinned by + * TestSPAFallbackServesIndexForEveryAppRoute in internal/server/static_test.go. + */ + +import { test, expect, type Page } from '@playwright/test'; +import { mockApi, seedAuth } from './fixtures/recs'; + +interface RouteCase { + /** URL the browser is pointed at. */ + url: string; + /** data-tab of the nav item that must end up highlighted. */ + tab: string; + /** document.title after the route resolves. */ + title: string; + /** location.pathname after the app canonicalises the URL. */ + canonical: string; +} + +const ROUTES: RouteCase[] = [ + { url: '/', tab: 'home', title: 'CUDly — Home', canonical: '/home' }, + { url: '/home', tab: 'home', title: 'CUDly — Home', canonical: '/home' }, + { url: '/opportunities', tab: 'opportunities', title: 'CUDly — Opportunities', canonical: '/opportunities' }, + { url: '/plans', tab: 'plans', title: 'CUDly — Plans', canonical: '/plans' }, + { url: '/purchases', tab: 'purchases', title: 'CUDly — Purchases', canonical: '/purchases' }, + + // Inventory and Admin carry a sub-tab segment. The initial replaceState must + // preserve it: it is the input switchTab reads to pick the sub-section. + { url: '/inventory', tab: 'inventory', title: 'CUDly — Inventory & Coverage', canonical: '/inventory/active-commitments' }, + { url: '/inventory/active-commitments', tab: 'inventory', title: 'CUDly — Inventory & Coverage', canonical: '/inventory/active-commitments' }, + { url: '/inventory/coverage', tab: 'inventory', title: 'CUDly — Inventory & Coverage', canonical: '/inventory/coverage' }, + { url: '/inventory/ri-exchange', tab: 'inventory', title: 'CUDly — Inventory & Coverage', canonical: '/inventory/ri-exchange' }, + { url: '/admin', tab: 'admin', title: 'CUDly — Admin · General', canonical: '/admin/general' }, + { url: '/admin/general', tab: 'admin', title: 'CUDly — Admin · General', canonical: '/admin/general' }, + { url: '/admin/purchasing', tab: 'admin', title: 'CUDly — Admin · Purchasing policies', canonical: '/admin/purchasing' }, + { url: '/admin/accounts', tab: 'admin', title: 'CUDly — Admin · Accounts & onboarding', canonical: '/admin/accounts' }, + { url: '/admin/users', tab: 'admin', title: 'CUDly — Admin · Users, roles & API keys', canonical: '/admin/users' }, + + // Pre-#340 bookmarks, kept alive by LEGACY_PATH_REDIRECTS. + { url: '/dashboard', tab: 'home', title: 'CUDly — Home', canonical: '/home' }, + { url: '/recommendations', tab: 'opportunities', title: 'CUDly — Opportunities', canonical: '/opportunities' }, + { url: '/history', tab: 'purchases', title: 'CUDly — Purchases', canonical: '/purchases' }, + { url: '/settings', tab: 'admin', title: 'CUDly — Admin · General', canonical: '/admin/general' }, + { url: '/ri-exchange', tab: 'inventory', title: 'CUDly — Inventory & Coverage', canonical: '/inventory/active-commitments' }, + + // Unknown paths land on Home rather than an unrouted shell. + { url: '/not-a-route', tab: 'home', title: 'CUDly — Home', canonical: '/home' }, + + // A first segment naming an Object.prototype member used to pass the + // `segment in TABS` membership test; pushing the resulting non-tab value + // threw DataCloneError out of init(), leaving the app on its unrouted + // markup (Home panel, generic title, no event listeners bound). + { url: '/constructor', tab: 'home', title: 'CUDly — Home', canonical: '/home' }, + { url: '/__proto__', tab: 'home', title: 'CUDly — Home', canonical: '/home' }, + { url: '/admin/constructor', tab: 'admin', title: 'CUDly — Admin · General', canonical: '/admin/general' }, +]; + +/** What the user can see once routing has settled. */ +async function observe(page: Page) { + return page.evaluate(() => ({ + title: document.title, + path: window.location.pathname, + tab: document.querySelector('.tab-btn.active')?.getAttribute('data-tab') ?? null, + panel: document.querySelector('.tab-content.active')?.id ?? null, + })); +} + +for (const route of ROUTES) { + test(`${route.url} renders the ${route.tab} page on direct load and on refresh`, async ({ page }) => { + // Uncaught exceptions only. The stub API fixture 404s endpoints this spec + // does not care about, and those are reported as console errors by design. + const pageErrors: string[] = []; + page.on('pageerror', (err) => pageErrors.push(String(err))); + + await seedAuth(page); + await mockApi(page); + + await page.goto(route.url); + await expect(page.locator(`.tab-content.active#${route.tab}-tab`)).toBeAttached(); + + const onLoad = await observe(page); + expect(onLoad).toEqual({ + title: route.title, + path: route.canonical, + tab: route.tab, + panel: `${route.tab}-tab`, + }); + + // Refresh from the canonical URL the app just wrote: the issue reports the + // failure on refresh as well as on the first load, and the canonical URL is + // what a user bookmarks or shares. + await page.reload(); + await expect(page.locator(`.tab-content.active#${route.tab}-tab`)).toBeAttached(); + expect(await observe(page)).toEqual(onLoad); + + expect(pageErrors).toEqual([]); + }); +} + +test('a deep-linked Inventory sub-tab opens that sub-section, not the default', async ({ page }) => { + await seedAuth(page); + await mockApi(page); + + await page.goto('/inventory/coverage'); + await expect(page.locator('#inventory-tab .sub-tab-btn.active')).toHaveText(/coverage/i); + expect(new URL(page.url()).pathname).toBe('/inventory/coverage'); +}); From 0edb9b9d9a6d007ba8f640136269cc4aba0a3687 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 19 Aug 2026 03:53:51 +0200 Subject: [PATCH 4/7] fix(server/static): restore the gosec G703 suppressions with accurate justifications The three os.Stat calls in this file are flagged by gosec's taint analysis as G703 path traversal. The suppressions were dropped during review on the claim that G703 was not the applicable rule; it is, and CI's golangci-lint v2.10.1 fails on all three. A newer local golangci (2.11.4) did not report them, so the removal passed local verification. Restore them in the form that was already proven on main, but with justifications that name a guard which actually exists rather than the copied-forward "error from Close handled in defer" text, which described nothing on these lines: - resolveStaticFilePath: filePath passed filepath.Abs and symlinkSafeContainedIn immediately above - directoryIndex: the candidate passed its own symlinkSafeContainedIn - spaIndex: the operator-set STATIC_DIR plus a constant filename, with no request-derived segment Verified against v2.10.1 specifically, matching CI, rather than whatever version was on PATH. --- internal/server/static.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/server/static.go b/internal/server/static.go index fafb186c4..8cb7156f4 100644 --- a/internal/server/static.go +++ b/internal/server/static.go @@ -94,7 +94,7 @@ func directoryIndex(absDir, dirPath, cleanPath string) (indexPath, indexClean st if err != nil || !symlinkSafeContainedIn(absDir, absCandidate) { return "", "", false } - info, err := os.Stat(candidate) + info, err := os.Stat(candidate) //nolint:gosec // G703: candidate is absCandidate, checked by the symlinkSafeContainedIn call directly above if err != nil || info.IsDir() { return "", "", false } @@ -104,7 +104,7 @@ func directoryIndex(absDir, dirPath, cleanPath string) (indexPath, indexClean st // spaIndex returns the SPA shell that client-side routes fall back to. func spaIndex(dir string) (filePath, cleanPath string, ok bool) { filePath = filepath.Join(dir, "index.html") - if _, err := os.Stat(filePath); err != nil { + if _, err := os.Stat(filePath); err != nil { //nolint:gosec // G703: dir is the operator-set STATIC_DIR and the filename is a constant; no request-derived segment reaches this path return "", "", false } return filePath, "/index.html", true @@ -134,7 +134,7 @@ func resolveStaticFilePath(dir, urlPath string) (filePath, cleanPath string, ok return "", "", false } - if info, statErr := os.Stat(filePath); statErr == nil { + if info, statErr := os.Stat(filePath); statErr == nil { //nolint:gosec // G703: filePath passed filepath.Abs + symlinkSafeContainedIn above, which rejects any path resolving outside dir if !info.IsDir() { return filePath, cleanPath, true } From 0588eeb7792bc5cdb8cc2f5f5a51fb9098580b31 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 19 Aug 2026 04:23:07 +0200 Subject: [PATCH 5/7] fix: close the SPA-shell symlink gap and fail closed on unreadable dual-control config Two findings from review. spaIndex served the SPA shell without the containment check its sibling directoryIndex runs. A direct /index.html request is validated by resolveStaticFilePath before it gets there, so a symlinked shell pointing out of the static dir was refused at /index.html and served at every client-side route. Same asymmetry, opposite spelling. The regression test asserts both spellings reach the same verdict. refreshFourEyesMode left _fourEyesMode at its previous value when the config fetch failed, which on a first load means false. That renders the approval queue as though dual control were off and offers the creator an Approve action the backend rejects. An unreadable config now fails closed: the UI can be more restrictive than the backend, never less. A config that reads successfully without the flag is a known off, not an outage, and is still treated as off; both directions are covered by tests. --- .../src/__tests__/four-eyes-approval.test.ts | 35 +++++++++++++++++++ frontend/src/history.ts | 15 +++++--- internal/server/static.go | 15 ++++++-- internal/server/static_test.go | 32 +++++++++++++++++ 4 files changed, 89 insertions(+), 8 deletions(-) diff --git a/frontend/src/__tests__/four-eyes-approval.test.ts b/frontend/src/__tests__/four-eyes-approval.test.ts index efb105f8b..4788fdfd0 100644 --- a/frontend/src/__tests__/four-eyes-approval.test.ts +++ b/frontend/src/__tests__/four-eyes-approval.test.ts @@ -197,6 +197,41 @@ describe('4-eyes approval mode (issue #1005)', () => { expect(document.getElementById('four-eyes-banner')?.classList.contains('hidden')).toBe(true); }); + // An unreadable config must fail closed. Failing open would offer the + // creator an Approve action the backend rejects, which is a UI claim the + // system will not honour. + test('dual control is ON when the config fetch fails', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(REG_USER); + (api.getConfig as jest.Mock).mockRejectedValue(new Error('config unavailable')); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ purchase_id: 'exec-own', created_by_user_id: REG_USER.id })], + }); + + await loadHistory(); + + const list = document.getElementById('history-list')!; + expect(list.querySelectorAll('.history-approve-btn')).toHaveLength(0); + expect(document.getElementById('four-eyes-banner')?.classList.contains('hidden')).toBe(false); + }); + + // A config that reads successfully but omits the flag is a known "off", + // not an outage, so it must not be forced closed. + test('dual control is OFF when the config reads successfully without the flag', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(REG_USER); + (api.getConfig as jest.Mock).mockResolvedValue({ global: {} }); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ purchase_id: 'exec-own', created_by_user_id: REG_USER.id })], + }); + + await loadHistory(); + + const list = document.getElementById('history-list')!; + expect(list.querySelectorAll('.history-approve-btn')).toHaveLength(1); + expect(document.getElementById('four-eyes-banner')?.classList.contains('hidden')).toBe(true); + }); + test('badge shown when button hidden by mode (admin approve-any self-approval)', async () => { // Admin holds approve-any, which would normally show Approve on every // pending row regardless of creator. Four-eyes still blocks self-approval diff --git a/frontend/src/history.ts b/frontend/src/history.ts index 4e6b14382..9f42257c5 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -52,14 +52,19 @@ let _fourEyesMode = false; * * Every path that renders the approval queue must call this first, or the * queue renders as though dual control were off and offers the creator an - * Approve button the backend will reject. A failed fetch keeps the previous - * value rather than throwing, so a config blip cannot block the render. + * Approve button the backend will reject. + * + * An unreadable config fails closed, to dual-control ON. The fetch never + * throws, so a config blip cannot block the render; it can only make the UI + * more restrictive than the backend, never less. The cost is a temporarily + * hidden Approve button during an outage, which self-corrects on the next + * successful load. */ async function refreshFourEyesMode(): Promise { const cfgResponse = await api.getConfig().catch(() => null); - if (cfgResponse?.global) { - _fourEyesMode = cfgResponse.global.require_different_approver === true; - } + _fourEyesMode = cfgResponse?.global + ? cfgResponse.global.require_different_approver === true + : true; const banner = document.getElementById('four-eyes-banner'); if (banner) banner.classList.toggle('hidden', !_fourEyesMode); } diff --git a/internal/server/static.go b/internal/server/static.go index 8cb7156f4..dbfa42bd5 100644 --- a/internal/server/static.go +++ b/internal/server/static.go @@ -102,9 +102,18 @@ func directoryIndex(absDir, dirPath, cleanPath string) (indexPath, indexClean st } // spaIndex returns the SPA shell that client-side routes fall back to. -func spaIndex(dir string) (filePath, cleanPath string, ok bool) { +// +// Runs the same containment check as directoryIndex. A direct /index.html +// request is validated by resolveStaticFilePath before it gets here, so +// without this a symlinked shell pointing out of dir would be refused at +// /index.html and served at every client-side route. +func spaIndex(absDir, dir string) (filePath, cleanPath string, ok bool) { filePath = filepath.Join(dir, "index.html") - if _, err := os.Stat(filePath); err != nil { //nolint:gosec // G703: dir is the operator-set STATIC_DIR and the filename is a constant; no request-derived segment reaches this path + absFile, err := filepath.Abs(filePath) + if err != nil || !symlinkSafeContainedIn(absDir, absFile) { + return "", "", false + } + if _, err := os.Stat(filePath); err != nil { //nolint:gosec // G703: filePath is dir plus a constant filename, checked by the symlinkSafeContainedIn call directly above return "", "", false } return filePath, "/index.html", true @@ -149,7 +158,7 @@ func resolveStaticFilePath(dir, urlPath string) (filePath, cleanPath string, ok if path.Ext(cleanPath) != "" { return "", "", false } - return spaIndex(dir) + return spaIndex(absDir, dir) } // cacheControlForExt returns the Cache-Control header value for a file extension. diff --git a/internal/server/static_test.go b/internal/server/static_test.go index 2de3bd1ef..016dbdec1 100644 --- a/internal/server/static_test.go +++ b/internal/server/static_test.go @@ -456,3 +456,35 @@ func TestResolveStaticFilePath_DirectoryIndexRejectsSymlinkEscape(t *testing.T) testutil.AssertEqual(t, true, viaDirFound) testutil.AssertEqual(t, "spa", string(viaDir)) } + +// The SPA shell is served for every client-side route, so it needs the same +// containment check as the direct request path. Without it a symlinked +// index.html pointing out of the static dir is refused at /index.html and +// served at /plans, which is the same asymmetry directoryIndex avoids. +func TestSPAFallbackRejectsSymlinkedShell(t *testing.T) { + parent := t.TempDir() + root := filepath.Join(parent, "static") + if err := os.MkdirAll(root, 0o755); err != nil { + t.Fatalf("mkdir static: %v", err) + } + outside := filepath.Join(parent, "secret.html") + if err := os.WriteFile(outside, []byte("should-not-serve"), 0o644); err != nil { + t.Fatalf("write secret.html: %v", err) + } + if err := os.Symlink(outside, filepath.Join(root, "index.html")); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + // Direct spelling: already rejected before this change. + _, _, _, directFound := serveStaticForLambda(root, "/index.html") + testutil.AssertEqual(t, false, directFound) + + // Client-side route: must reach the same verdict, not serve the target. + for _, route := range []string{"/plans", "/inventory/coverage", "/"} { + t.Run(route, func(t *testing.T) { + content, _, _, found := serveStaticForLambda(root, route) + testutil.AssertEqual(t, false, found) + testutil.AssertEqual(t, "", string(content)) + }) + } +} From de9bde85e5caff36c8356489efcd33eb9cbeedaf Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 19 Aug 2026 04:49:15 +0200 Subject: [PATCH 6/7] test(server/static): prove path traversal is unreachable, and name the real guard The gosec G703 findings were suppressed on an argument. This replaces the argument with evidence: adversarial cases across all three entry points (resolveStaticFilePath, the HTTP handler, and the Lambda transport), covering "..", encoded and double-encoded traversal, backslash separators, absolute paths, NUL bytes, and symlinks escaping the root. They assert on the resolved path with symlinks followed, and on the served bytes, not on status codes: a handler that returns the wrong file with 200 passes a status-only assertion. A mutation matrix over the guards, running the whole package rather than a name-filtered subset, establishes which one carries the weight: symlinkSafeContainedIn, any one of its 3 sites -> tests fail path.Clean alone -> tests still pass path.Clean + containment -> escapes, serves out of root So symlinkSafeContainedIn is load-bearing at every site, and path.Clean is defense in depth for the lexical cases only. It cannot resolve a symlink, which is the escape the containment check exists for. The suppression comments now say that rather than crediting the wrong guard. The symlink cases live in the shared hostile fixture rather than only in separately-named tests, so one `-run Hostile` exercises both classes. Keeping them apart is what let an earlier name-filtered mutation run skip every case that touches symlinkSafeContainedIn and wrongly conclude it was redundant. Also converts the three suppressions from //nolint:gosec to the #nosec form already used in this file, verified against golangci-lint v2.10.1. --- internal/server/static.go | 18 +++- internal/server/static_test.go | 154 +++++++++++++++++++++++++++++++++ 2 files changed, 169 insertions(+), 3 deletions(-) diff --git a/internal/server/static.go b/internal/server/static.go index dbfa42bd5..80cebbabb 100644 --- a/internal/server/static.go +++ b/internal/server/static.go @@ -94,7 +94,10 @@ func directoryIndex(absDir, dirPath, cleanPath string) (indexPath, indexClean st if err != nil || !symlinkSafeContainedIn(absDir, absCandidate) { return "", "", false } - info, err := os.Stat(candidate) //nolint:gosec // G703: candidate is absCandidate, checked by the symlinkSafeContainedIn call directly above + // #nosec G703 -- candidate is dirPath (already contained) plus a constant + // filename, then re-checked by the symlinkSafeContainedIn call above, which + // catches a symlinked index.html whose target sits outside dir. + info, err := os.Stat(candidate) if err != nil || info.IsDir() { return "", "", false } @@ -113,7 +116,11 @@ func spaIndex(absDir, dir string) (filePath, cleanPath string, ok bool) { if err != nil || !symlinkSafeContainedIn(absDir, absFile) { return "", "", false } - if _, err := os.Stat(filePath); err != nil { //nolint:gosec // G703: filePath is dir plus a constant filename, checked by the symlinkSafeContainedIn call directly above + // #nosec G703 -- dir plus a constant filename, re-checked by the + // symlinkSafeContainedIn call above. See TestSPAFallbackRejectsSymlinkedShell: + // without that check a symlinked shell is refused at /index.html and served + // at every extensionless route. + if _, err := os.Stat(filePath); err != nil { return "", "", false } return filePath, "/index.html", true @@ -143,7 +150,12 @@ func resolveStaticFilePath(dir, urlPath string) (filePath, cleanPath string, ok return "", "", false } - if info, statErr := os.Stat(filePath); statErr == nil { //nolint:gosec // G703: filePath passed filepath.Abs + symlinkSafeContainedIn above, which rejects any path resolving outside dir + // #nosec G703 -- guarded by filepath.Abs + symlinkSafeContainedIn above. + // That check is the load-bearing one and removing it alone fails the + // Hostile suite and both symlink tests, because a symlink is the escape + // path.Clean cannot see. path.Clean is defense in depth for the lexical + // cases only: removing it alone fails nothing, removing both escapes dir. + if info, statErr := os.Stat(filePath); statErr == nil { if !info.IsDir() { return filePath, cleanPath, true } diff --git a/internal/server/static_test.go b/internal/server/static_test.go index 016dbdec1..558b51be7 100644 --- a/internal/server/static_test.go +++ b/internal/server/static_test.go @@ -6,6 +6,8 @@ import ( "net/http/httptest" "os" "path/filepath" + "strconv" + "strings" "testing" "github.com/LeanerCloud/CUDly/internal/testutil" @@ -488,3 +490,155 @@ func TestSPAFallbackRejectsSymlinkedShell(t *testing.T) { }) } } + +// hostileRoot builds a static root with a sibling tree outside it holding a +// file that must never be served, and returns (root, outsideFile). +func hostileRoot(t *testing.T) (string, string) { + t.Helper() + parent := t.TempDir() + root := filepath.Join(parent, "static") + if err := os.MkdirAll(filepath.Join(root, "docs"), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + for name, body := range map[string]string{ + "index.html": "spa", + "docs/index.html": "docs", + } { + if err := os.WriteFile(filepath.Join(root, name), []byte(body), 0o644); err != nil { + t.Fatalf("write %s: %v", name, err) + } + } + outside := filepath.Join(parent, "secret.txt") + if err := os.WriteFile(outside, []byte("TOP-SECRET"), 0o644); err != nil { + t.Fatalf("write secret: %v", err) + } + // Symlinks that escape the root, inside the same fixture as the lexical + // cases. Keeping them here means one `-run Hostile` covers both classes; + // when they lived only in separately-named tests, a name-filtered + // mutation run silently skipped the only cases that exercise + // symlinkSafeContainedIn, which is the check path.Clean cannot stand in + // for. + if err := os.Symlink(outside, filepath.Join(root, "escape.txt")); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + if err := os.Symlink(outside, filepath.Join(root, "docs", "leak.html")); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + return root, outside +} + +// Asserting on the resolved path, not on a status code: a handler that serves +// the wrong file with 200 passes a status-only assertion. The invariant is that +// whatever resolveStaticFilePath hands back is inside the served root, with +// symlinks resolved, or it hands back nothing. +func TestResolveStaticFilePath_HostilePathsNeverEscapeRoot(t *testing.T) { + root, outside := hostileRoot(t) + realRoot, err := filepath.EvalSymlinks(root) + if err != nil { + t.Fatalf("evalsymlinks root: %v", err) + } + + hostile := []string{ + "/../secret.txt", + "/../../secret.txt", + "/..", + "/../", + "/docs/../../secret.txt", + "/./../secret.txt", + "/....//secret.txt", + "/..%2fsecret.txt", // encoded, as the Lambda RawPath transport delivers it + "/%2e%2e%2fsecret.txt", // fully encoded + "/%252e%252e%252fsecret.txt", // double encoded + "/..\\secret.txt", // backslash separator + "/" + outside, // absolute path pasted into the URL + outside, // absolute path, no leading join + "//secret.txt", + "/docs/./../../secret.txt", + "/\x00/secret.txt", // NUL byte + "/plans\x00.html", + "/escape.txt", // symlink in the root pointing outside it + "/docs/leak.html", // symlink one level down + } + + for _, p := range hostile { + t.Run(strconv.Quote(p), func(t *testing.T) { + filePath, _, ok := resolveStaticFilePath(root, p) + if !ok { + return // refused outright, which is a valid answer + } + abs, absErr := filepath.Abs(filePath) + if absErr != nil { + t.Fatalf("abs(%q): %v", filePath, absErr) + } + // Resolve symlinks before comparing: a lexically-contained path + // whose target is outside is exactly the bypass being tested for. + if resolved, symErr := filepath.EvalSymlinks(abs); symErr == nil { + abs = resolved + } + if !isPathContainedIn(abs, realRoot) { + t.Fatalf("path %q escaped the root: resolved to %q, outside %q", p, abs, realRoot) + } + // Belt and braces: never the secret, whatever the path. + if data, readErr := os.ReadFile(abs); readErr == nil && strings.Contains(string(data), "TOP-SECRET") { + t.Fatalf("path %q served the out-of-root secret from %q", p, abs) + } + }) + } +} + +// Same matrix through the real HTTP handler, so Go's own URL decoding is in the +// loop rather than assumed. Asserts on the served bytes, not the status. +func TestSpaFileServer_HostilePathsNeverServeOutOfRoot(t *testing.T) { + root, _ := hostileRoot(t) + handler := spaFileServer(root) + + for _, target := range []string{ + "/../secret.txt", + "/../../secret.txt", + "/docs/../../secret.txt", + "/..%2fsecret.txt", + "/%2e%2e%2fsecret.txt", + "/%252e%252e%252fsecret.txt", + "/..\\secret.txt", + "//secret.txt", + "/....//secret.txt", + "/escape.txt", + "/docs/leak.html", + } { + t.Run(target, func(t *testing.T) { + req := httptest.NewRequestWithContext(context.Background(), http.MethodGet, target, nil) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + if strings.Contains(w.Body.String(), "TOP-SECRET") { + t.Fatalf("target %q served out-of-root content (status %d)", target, w.Code) + } + }) + } +} + +// The Lambda transport passes RawPath, which is not decoded the way +// http.Request.URL.Path is, so the two transports must be checked separately. +func TestServeStaticForLambda_HostilePathsNeverServeOutOfRoot(t *testing.T) { + root, outside := hostileRoot(t) + + for _, p := range []string{ + "/../secret.txt", + "/../../secret.txt", + "/docs/../../secret.txt", + "/..%2fsecret.txt", + "/%2e%2e%2fsecret.txt", + "/%252e%252e%252fsecret.txt", + "/..\\secret.txt", + "//secret.txt", + "/escape.txt", + "/docs/leak.html", + outside, + } { + t.Run(strconv.Quote(p), func(t *testing.T) { + content, _, _, _ := serveStaticForLambda(root, p) + if strings.Contains(string(content), "TOP-SECRET") { + t.Fatalf("path %q served out-of-root content", p) + } + }) + } +} From ea21439fcdf3db6d519f3f53af055a79fff2ed55 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 19 Aug 2026 05:36:35 +0200 Subject: [PATCH 7/7] test(server/static): keep the lexical hostile cases running without symlink support hostileRoot called t.Skipf when os.Symlink failed, which skipped the whole test: traversal, encoded and double-encoded paths, absolute paths and NUL bytes all vanished along with the symlink cases, and go test reported SKIP. On a platform without symlink support the entire traversal defense would have gone unverified while the suite still looked green. The lexical fixture is now built unconditionally and the symlink cases are additive, returned by symlinkCases() so callers append instead of branching. Simulating a symlink failure now runs 35 lexical subtests and skips nothing, against 0 subtests and 3 skips before. Same shape as the -run filter that hid these cases from the earlier mutation run: a check whose failure is indistinguishable from success. Reported by CodeRabbit on de9bde85e. --- internal/server/static_test.go | 73 +++++++++++++++++++++------------- 1 file changed, 45 insertions(+), 28 deletions(-) diff --git a/internal/server/static_test.go b/internal/server/static_test.go index 558b51be7..ed19f0e84 100644 --- a/internal/server/static_test.go +++ b/internal/server/static_test.go @@ -492,8 +492,14 @@ func TestSPAFallbackRejectsSymlinkedShell(t *testing.T) { } // hostileRoot builds a static root with a sibling tree outside it holding a -// file that must never be served, and returns (root, outsideFile). -func hostileRoot(t *testing.T) (string, string) { +// file that must never be served. Returns (root, outsideFile, symlinked), where +// symlinked reports whether the symlink cases could be created. +// +// The lexical fixture is built unconditionally and the symlink cases are +// additive, so a platform without symlink support loses the symlink cases only. +// t.Skip here would take the traversal, encoded-path, absolute-path and NUL +// cases with it and report the whole suite as skipped, which reads as a pass. +func hostileRoot(t *testing.T) (string, string, bool) { t.Helper() parent := t.TempDir() root := filepath.Join(parent, "static") @@ -512,19 +518,32 @@ func hostileRoot(t *testing.T) (string, string) { if err := os.WriteFile(outside, []byte("TOP-SECRET"), 0o644); err != nil { t.Fatalf("write secret: %v", err) } - // Symlinks that escape the root, inside the same fixture as the lexical - // cases. Keeping them here means one `-run Hostile` covers both classes; - // when they lived only in separately-named tests, a name-filtered - // mutation run silently skipped the only cases that exercise - // symlinkSafeContainedIn, which is the check path.Clean cannot stand in - // for. - if err := os.Symlink(outside, filepath.Join(root, "escape.txt")); err != nil { - t.Skipf("symlinks unavailable: %v", err) + // Symlinks that escape the root, in the same fixture as the lexical cases. + // Keeping them together means one `-run Hostile` covers both classes; when + // they lived only in separately-named tests, a name-filtered mutation run + // silently skipped the only cases exercising symlinkSafeContainedIn, which + // is the check path.Clean cannot stand in for. + symlinked := true + for _, link := range []string{ + filepath.Join(root, "escape.txt"), + filepath.Join(root, "docs", "leak.html"), + } { + if err := os.Symlink(outside, link); err != nil { + t.Logf("symlinks unavailable, symlink cases skipped (lexical cases still run): %v", err) + symlinked = false + break + } } - if err := os.Symlink(outside, filepath.Join(root, "docs", "leak.html")); err != nil { - t.Skipf("symlinks unavailable: %v", err) + return root, outside, symlinked +} + +// symlinkCases returns the escaping-symlink paths when the fixture could create +// them, and nothing otherwise, so callers append rather than branch. +func symlinkCases(symlinked bool) []string { + if !symlinked { + return nil } - return root, outside + return []string{"/escape.txt", "/docs/leak.html"} } // Asserting on the resolved path, not on a status code: a handler that serves @@ -532,13 +551,13 @@ func hostileRoot(t *testing.T) (string, string) { // whatever resolveStaticFilePath hands back is inside the served root, with // symlinks resolved, or it hands back nothing. func TestResolveStaticFilePath_HostilePathsNeverEscapeRoot(t *testing.T) { - root, outside := hostileRoot(t) + root, outside, symlinked := hostileRoot(t) realRoot, err := filepath.EvalSymlinks(root) if err != nil { t.Fatalf("evalsymlinks root: %v", err) } - hostile := []string{ + hostile := append([]string{ "/../secret.txt", "/../../secret.txt", "/..", @@ -556,9 +575,7 @@ func TestResolveStaticFilePath_HostilePathsNeverEscapeRoot(t *testing.T) { "/docs/./../../secret.txt", "/\x00/secret.txt", // NUL byte "/plans\x00.html", - "/escape.txt", // symlink in the root pointing outside it - "/docs/leak.html", // symlink one level down - } + }, symlinkCases(symlinked)...) for _, p := range hostile { t.Run(strconv.Quote(p), func(t *testing.T) { @@ -589,10 +606,10 @@ func TestResolveStaticFilePath_HostilePathsNeverEscapeRoot(t *testing.T) { // Same matrix through the real HTTP handler, so Go's own URL decoding is in the // loop rather than assumed. Asserts on the served bytes, not the status. func TestSpaFileServer_HostilePathsNeverServeOutOfRoot(t *testing.T) { - root, _ := hostileRoot(t) + root, _, symlinked := hostileRoot(t) handler := spaFileServer(root) - for _, target := range []string{ + targets := append([]string{ "/../secret.txt", "/../../secret.txt", "/docs/../../secret.txt", @@ -602,9 +619,9 @@ func TestSpaFileServer_HostilePathsNeverServeOutOfRoot(t *testing.T) { "/..\\secret.txt", "//secret.txt", "/....//secret.txt", - "/escape.txt", - "/docs/leak.html", - } { + }, symlinkCases(symlinked)...) + + for _, target := range targets { t.Run(target, func(t *testing.T) { req := httptest.NewRequestWithContext(context.Background(), http.MethodGet, target, nil) w := httptest.NewRecorder() @@ -619,9 +636,9 @@ func TestSpaFileServer_HostilePathsNeverServeOutOfRoot(t *testing.T) { // The Lambda transport passes RawPath, which is not decoded the way // http.Request.URL.Path is, so the two transports must be checked separately. func TestServeStaticForLambda_HostilePathsNeverServeOutOfRoot(t *testing.T) { - root, outside := hostileRoot(t) + root, outside, symlinked := hostileRoot(t) - for _, p := range []string{ + paths := append([]string{ "/../secret.txt", "/../../secret.txt", "/docs/../../secret.txt", @@ -630,10 +647,10 @@ func TestServeStaticForLambda_HostilePathsNeverServeOutOfRoot(t *testing.T) { "/%252e%252e%252fsecret.txt", "/..\\secret.txt", "//secret.txt", - "/escape.txt", - "/docs/leak.html", outside, - } { + }, symlinkCases(symlinked)...) + + for _, p := range paths { t.Run(strconv.Quote(p), func(t *testing.T) { content, _, _, _ := serveStaticForLambda(root, p) if strings.Contains(string(content), "TOP-SECRET") {