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__/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/__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/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/frontend/src/history.ts b/frontend/src/history.ts index 2615de798..9f42257c5 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -42,11 +42,33 @@ 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. + * + * 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); + _fourEyesMode = cfgResponse?.global + ? cfgResponse.global.require_different_approver === true + : 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 +245,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 +343,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) { 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'); +}); diff --git a/internal/server/static.go b/internal/server/static.go index cc0c8f1d2..80cebbabb 100644 --- a/internal/server/static.go +++ b/internal/server/static.go @@ -79,6 +79,53 @@ 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 + } + // #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 + } + return candidate, path.Join(cleanPath, "index.html"), true +} + +// spaIndex returns the SPA shell that client-side routes fall back to. +// +// 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") + absFile, err := filepath.Abs(filePath) + if err != nil || !symlinkSafeContainedIn(absDir, absFile) { + return "", "", false + } + // #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 +} + // 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 +150,27 @@ 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 + // #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 } - // 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(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 fa2199383..ed19f0e84 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" @@ -334,3 +336,326 @@ 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)) +} + +// 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)) + }) + } +} + +// hostileRoot builds a static root with a sibling tree outside it holding a +// 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") + 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, 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 + } + } + 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 []string{"/escape.txt", "/docs/leak.html"} +} + +// 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, symlinked := hostileRoot(t) + realRoot, err := filepath.EvalSymlinks(root) + if err != nil { + t.Fatalf("evalsymlinks root: %v", err) + } + + hostile := append([]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", + }, symlinkCases(symlinked)...) + + 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, _, symlinked := hostileRoot(t) + handler := spaFileServer(root) + + targets := append([]string{ + "/../secret.txt", + "/../../secret.txt", + "/docs/../../secret.txt", + "/..%2fsecret.txt", + "/%2e%2e%2fsecret.txt", + "/%252e%252e%252fsecret.txt", + "/..\\secret.txt", + "//secret.txt", + "/....//secret.txt", + }, 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() + 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, symlinked := hostileRoot(t) + + paths := append([]string{ + "/../secret.txt", + "/../../secret.txt", + "/docs/../../secret.txt", + "/..%2fsecret.txt", + "/%2e%2e%2fsecret.txt", + "/%252e%252e%252fsecret.txt", + "/..\\secret.txt", + "//secret.txt", + 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") { + t.Fatalf("path %q served out-of-root content", p) + } + }) + } +}