diff --git a/app/components/MainComponent.vue b/app/components/MainComponent.vue index 2f88c1f3..9e35b491 100644 --- a/app/components/MainComponent.vue +++ b/app/components/MainComponent.vue @@ -174,6 +174,8 @@ export default defineNuxtComponent({ const config = useRuntimeConfig(); this.isLoading = true; + // Clear previous API errors when making a new request + this.apiError = undefined; try { const options = Options.fromRoute(this.route, this.dateRange.since, this.dateRange.until); @@ -216,7 +218,7 @@ export default defineNuxtComponent({ this.apiError = `404 Not Found - is the ${this.config?.public?.scope || ''} org:"${this.config?.public?.githubOrg || ''}" ent:"${this.config?.public?.githubEnt || ''}" team:"${this.config?.public?.githubTeam}" correct? ${error.message}`; break; case 422: - this.apiError = `422 Unprocessable Entity - Is the Copilot Metrics API enabled for the Org/Ent? ${error.message}`; + this.apiError = `422 Unprocessable Entity - Is the Copilot Metrics API enabled for the Org/Ent? When changing filters, try adjusting the "from" date. ${error.message}`; break; case 500: this.apiError = `500 Internal Server Error - most likely a bug in the app. Error: ${error.message}`; diff --git a/server/api/teams.ts b/server/api/teams.ts index f4843ce4..31591c43 100644 --- a/server/api/teams.ts +++ b/server/api/teams.ts @@ -1,12 +1,6 @@ import { Options, type Scope } from '@/model/Options' import type { H3Event, EventHandlerRequest } from 'h3' -const cache = new Map() - -interface CacheData { - data: Team[] - valid_until: number -} interface Team { name: string; slug: string; description: string } interface GitHubTeam { name: string; slug: string; description?: string } @@ -72,16 +66,6 @@ export async function getTeams(event: H3Event): Promise Math.floor(Date.now() / 1000)) { - logger.info(`Returning cached data for ${event.path}`) - return cachedData.data - } - cache.delete(event.path) - } - if (!event.context.headers.has('Authorization')) { logger.error('No Authentication provided') throw new TeamsError('No Authentication provided', 401) @@ -114,7 +98,5 @@ export async function getTeams(event: H3Event): Promise(); @@ -24,6 +25,50 @@ class MetricsError extends Error { } } +/** + * Builds a cache key for metrics data that is bound to the caller's Authorization header (hashed) + path + query. + * Exported for unit testing. + */ +type QueryParamValue = string | string[] | undefined; +type QueryParams = Record; + +export function buildMetricsCacheKey(path: string, query: QueryParams, authHeader: string): string { + // Split existing query params from provided path (if any) + const [rawPath, existingQueryString] = path.split('?'); + const merged = new Map(); + + // Add existing params first + if (existingQueryString) { + const existingParams = new URLSearchParams(existingQueryString); + existingParams.forEach((value, key) => { + merged.set(key, value); + }); + } + + // Merge in provided query object (overrides existing) + Object.entries(query).forEach(([k, v]) => { + if (v === undefined || v === null) return; + if (Array.isArray(v)) { + if (v.length === 0) return; + merged.set(k, v.join(',')); + } else if (v !== '') { + merged.set(k, v); + } + }); + + // Build stable, sorted query string + const sortedKeys = Array.from(merged.keys()).sort(); + const finalParams = new URLSearchParams(); + sortedKeys.forEach(k => { + const val = merged.get(k); + if (val !== undefined) finalParams.set(k, val); + }); + const finalQueryString = finalParams.toString(); + + const authFingerprint = createHash('sha256').update(authHeader).digest('hex').slice(0, 16); // short fingerprint + return `${authFingerprint}:${rawPath}${finalQueryString ? `?${finalQueryString}` : ''}`; +} + export async function getMetricsData(event: H3Event): Promise { const logger = console; const query = getQuery(event); @@ -52,26 +97,33 @@ export async function getMetricsData(event: H3Event): Promi return usageData; } - if (cache.has(event.path)) { - const cachedData = cache.get(event.path); - if (cachedData && cachedData.valid_until > Date.now() / 1000) { - logger.info(`Returning cached data for ${event.path}`); + // Authorization must be validated BEFORE any cache lookup to prevent leakage of cached data + const authHeader = event.context.headers.get('Authorization'); + if (!authHeader) { + logger.error('No Authentication provided'); + throw new MetricsError('No Authentication provided', 401); + } + + // Build auth-bound cache key + const path = event.path || '/api/metrics'; // fallback path (should always exist in practice) + const cacheKey = buildMetricsCacheKey(path, query as QueryParams, authHeader); + + // Attempt cache lookup with auth fingerprint validation + const cachedData = cache.get(cacheKey); + if (cachedData) { + if (cachedData.valid_until > Date.now() / 1000) { + logger.info(`Returning cached data for ${cacheKey}`); return cachedData.data; } else { - logger.info(`Cached data for ${event.path} is expired, fetching new data`); - cache.delete(event.path); + logger.info(`Cached data for ${cacheKey} is expired or fingerprint mismatch, fetching new data`); + cache.delete(cacheKey); } } - if (!event.context.headers.has('Authorization')) { - logger.error('No Authentication provided'); - throw new MetricsError('No Authentication provided', 401); - } - logger.info(`Fetching metrics data from ${apiUrl}`); try { - const response = await $fetch(apiUrl, { + const response = await $fetch(apiUrl, { headers: event.context.headers }) as unknown[]; @@ -81,10 +133,12 @@ export async function getMetricsData(event: H3Event): Promi const filteredUsageData = filterHolidaysFromMetrics(usageData, options.excludeHolidays || false, options.locale); // metrics is the old API format const validUntil = Math.floor(Date.now() / 1000) + 5 * 60; // Cache for 5 minutes - cache.set(event.path, { data: filteredUsageData, valid_until: validUntil }); + cache.set(cacheKey, { data: filteredUsageData, valid_until: validUntil }); return filteredUsageData; } catch (error: unknown) { logger.error('Error fetching metrics data:', error); + // Clear any cached data for this request to prevent stale data on retry + cache.delete(cacheKey); const errorMessage = error instanceof Error ? error.message : String(error); const statusCode = (error && typeof error === 'object' && 'statusCode' in error) ? (error as { statusCode: number }).statusCode @@ -125,17 +179,12 @@ function updateMockDataDates(originalData: CopilotMetrics[], since?: string, unt } // Update dates in the dataset, copying existing entries when needed - const result = dateRange.map((date, index) => { - // Use existing data entries, cycling through them + const result: CopilotMetrics[] = dateRange.map((date, index) => { const dataIndex = index % originalData.length; - const entry = { ...originalData[dataIndex] }; - - // Update the date - entry.date = date.toISOString().split('T')[0]; - - return entry; + const src = originalData[dataIndex]; + const newDate = date.toISOString().split('T')[0]; + return { ...src, date: newDate }; }); - return result; } diff --git a/tests/metrics-auth-cache.nuxt.spec.ts b/tests/metrics-auth-cache.nuxt.spec.ts new file mode 100644 index 00000000..d8048338 --- /dev/null +++ b/tests/metrics-auth-cache.nuxt.spec.ts @@ -0,0 +1,63 @@ +// @vitest-environment nuxt +import { describe, it, expect } from 'vitest' +import { buildMetricsCacheKey } from '../shared/utils/metrics-util' + +/** + * These tests validate that the new cache key generation logic binds cache entries + * to the caller's Authorization header fingerprint and that different auth headers + * produce distinct keys even when path + query are identical. + */ + +describe('Metrics Auth-Bound Cache Key', () => { + const path = '/api/metrics' + const query = { since: '2024-01-01', until: '2024-01-31', scope: 'organization', githubOrg: 'test-org' } + + it('produces different keys for different auth headers', () => { + const keyA1 = buildMetricsCacheKey(path, query, 'token userA-1') + const keyA2 = buildMetricsCacheKey(path, query, 'token userA-2') + const keyB = buildMetricsCacheKey(path, query, 'token userB-1') + + expect(keyA1).not.toBe(keyA2) + expect(keyA1.split(':')[0]).not.toBe(keyA2.split(':')[0]) + expect(keyA1.split(':')[0]).not.toBe(keyB.split(':')[0]) + }) + + it('is stable for same auth header + query', () => { + const key1 = buildMetricsCacheKey(path, query, 'token stable-user') + const key2 = buildMetricsCacheKey(path, query, 'token stable-user') + expect(key1).toBe(key2) + }) + + it('filters out undefined/empty query params', () => { + const key = buildMetricsCacheKey(path, { since: '2024-01-01', empty: '', undef: undefined }, 'token x') + expect(key).toContain('since=2024-01-01') + expect(key).not.toContain('empty=') + expect(key).not.toContain('undef=') + }) + + it('joins array query params deterministically', () => { + const key = buildMetricsCacheKey(path, { since: ['2024-01-01'], tag: ['a','b'] }, 'token y') + expect(key).toContain('since=2024-01-01') + expect(key).toMatch(/tag=a%2Cb|tag=b%2Ca/) // order preserved from input array join + }) + + it('merges existing path query params with provided query object without duplication', () => { + const pathWithQuery = '/api/metrics?since=2024-01-01&scope=organization' + const key = buildMetricsCacheKey(pathWithQuery, { githubOrg: 'test-org', since: '2024-01-01' }, 'token z') + // should not duplicate since param and should include githubOrg + const pieces = key.split(':') + expect(pieces.length).toBeGreaterThan(1) + const qs = pieces.slice(1).join(':').split('?')[1] + expect(qs?.match(/since=/g)?.length).toBe(1) + expect(qs).toContain('githubOrg=test-org') + }) + + it('sorts final query params for stable ordering', () => { + const key1 = buildMetricsCacheKey('/api/metrics?b=2&a=1', { c: '3' }, 'token sort') + const key2 = buildMetricsCacheKey('/api/metrics?a=1&b=2', { c: '3' }, 'token sort') + expect(key1).toBe(key2) + const qs = key1.split('?')[1] + expect(qs).toBeDefined() + expect(qs!.startsWith('a=1&b=2&c=3')).toBe(true) + }) +}) diff --git a/tests/metrics-cache.nuxt.spec.ts b/tests/metrics-cache.nuxt.spec.ts new file mode 100644 index 00000000..cec7ddd8 --- /dev/null +++ b/tests/metrics-cache.nuxt.spec.ts @@ -0,0 +1,67 @@ +// @vitest-environment nuxt +import { describe, it, expect, beforeEach } from 'vitest' + +describe('Metrics Cache Key Generation', () => { + it('should create unique cache keys for different query parameters', () => { + // Test the cache key logic that was implemented + const createCacheKey = (path: string, query: Record) => { + const queryString = new URLSearchParams(query).toString() + return queryString ? `${path}?${queryString}` : path + } + + const path = '/api/metrics' + + // Different date ranges should create different cache keys + const query1 = { since: '2024-01-01', until: '2024-01-31', scope: 'organization', githubOrg: 'test-org' } + const query2 = { since: '2024-02-01', until: '2024-02-28', scope: 'organization', githubOrg: 'test-org' } + const query3 = { since: '2024-01-01', until: '2024-01-31', scope: 'organization', githubOrg: 'test-org' } + + const key1 = createCacheKey(path, query1) + const key2 = createCacheKey(path, query2) + const key3 = createCacheKey(path, query3) + + // Different date ranges should have different keys + expect(key1).not.toBe(key2) + + // Same parameters should have same key + expect(key1).toBe(key3) + + // Keys should include query parameters + expect(key1).toContain('since=2024-01-01') + expect(key1).toContain('until=2024-01-31') + expect(key2).toContain('since=2024-02-01') + expect(key2).toContain('until=2024-02-28') + }) + + it('should handle empty query parameters', () => { + const createCacheKey = (path: string, query: Record) => { + const queryString = new URLSearchParams(query).toString() + return queryString ? `${path}?${queryString}` : path + } + + const path = '/api/metrics' + const emptyQuery = {} + + const key = createCacheKey(path, emptyQuery) + expect(key).toBe(path) + }) + + it('should handle undefined query values', () => { + const createCacheKey = (path: string, query: Record) => { + // Filter out undefined values before creating query string + const filteredQuery = Object.fromEntries( + Object.entries(query).filter(([_, value]) => value !== undefined) + ) + const queryString = new URLSearchParams(filteredQuery).toString() + return queryString ? `${path}?${queryString}` : path + } + + const path = '/api/metrics' + const queryWithUndefined = { since: '2024-01-01', until: undefined, scope: 'organization' } + + const key = createCacheKey(path, queryWithUndefined) + expect(key).toContain('since=2024-01-01') + expect(key).toContain('scope=organization') + expect(key).not.toContain('until=') + }) +}) \ No newline at end of file diff --git a/tests/test.setup.ts b/tests/test.setup.ts new file mode 100644 index 00000000..e4cacd61 --- /dev/null +++ b/tests/test.setup.ts @@ -0,0 +1,38 @@ +import * as urlLib from 'url'; + +// Global test setup +// Force mock mode so server-side handlers use local mock data instead of hitting GitHub APIs. +process.env.NUXT_PUBLIC_IS_DATA_MOCKED = 'true'; + +function isGitHub(url: string): boolean { + const host = urlLib.parse(url).host; + return host === 'api.github.com' || host === 'api.github.com:443'; +} + +// Block accidental real GitHub API calls if mock mode logic fails. +const originalFetch: typeof globalThis.fetch | undefined = globalThis.fetch; +if (originalFetch) { + globalThis.fetch = (async (...args: Parameters): Promise => { + const url = String(args[0]); + if (isGitHub(url)) { + throw new Error(`Blocked external GitHub API call during tests: ${url}`); + } + return originalFetch(...args); + }) as typeof fetch; +} + +// Stub $fetch (ofetch) similarly if present at runtime. +if (typeof globalThis.$fetch === 'function') { + const original$fetch = globalThis.$fetch; + const wrapped: typeof globalThis.$fetch = ((url: unknown, opts: unknown) => { + const str = String(url); + if (isGitHub(str)) { + return Promise.reject(new Error(`Blocked external GitHub API call during tests via $fetch: ${str}`)); + } + return original$fetch(url as never, opts as never); + }) as typeof globalThis.$fetch; + // Preserve special properties if present + (wrapped as unknown as { raw?: unknown }).raw = (original$fetch as unknown as { raw?: () => unknown }).raw?.bind(original$fetch); + (wrapped as unknown as { create?: unknown }).create = (original$fetch as unknown as { create?: () => unknown }).create?.bind(original$fetch); + globalThis.$fetch = wrapped; +} diff --git a/vitest.config.ts b/vitest.config.ts index 07498aad..e4b0088c 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -5,6 +5,7 @@ export default defineVitestConfig({ test: { exclude: ['**/node_modules/**', '**/e2e-tests/**'], environment: 'nuxt', - globals: true // Use describe, test/expect, etc. without importing + globals: true, // Use describe, test/expect, etc. without importing + setupFiles: ['./tests/test.setup.ts'] } })