diff --git a/AGENTS.md b/AGENTS.md index 94af240b..64d1ee01 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -112,3 +112,9 @@ Custom error classes in `src/errors/`: APIError, NotFoundError, UnauthorizedErro ## PR Requirements PRs require: changes description, testing criteria with Loom video, and impact analysis (see `.github/PULL_REQUEST_TEMPLATE.md`). + +## Engineering note: +- After a successful implementation, the changes will be reviewed by the team lead and greptileAI in github. +- Do not use let unless absolutely necessary. Use const instead. +- Always keep the comments short, on point and easy to understand with easy wordings. This is must. +- Follow DRY, KISS, SOLID, YAGNI principles. \ No newline at end of file diff --git a/src/app/global-error.tsx b/src/app/global-error.tsx index 71ed7809..1c7aedba 100644 --- a/src/app/global-error.tsx +++ b/src/app/global-error.tsx @@ -6,6 +6,7 @@ import { useEffect } from 'react' export default function GlobalError({ error }: { error: Error & { digest?: string } }) { useEffect(() => { + // Server errors are redacted here in prod; auth errors are filtered in onRequestError. Sentry.captureException(error) }, [error]) diff --git a/src/features/auth/lib/authenticate.ts b/src/features/auth/lib/authenticate.ts index 319440eb..f5bd125d 100644 --- a/src/features/auth/lib/authenticate.ts +++ b/src/features/auth/lib/authenticate.ts @@ -1,5 +1,5 @@ import AssemblyClient from '@assembly/assembly-client' -import { AssemblyInvalidTokenError, AssemblyTokenParseError } from '@assembly/errors' +import { AssemblyInvalidTokenError, AssemblyMissingHeadersError, AssemblyTokenParseError } from '@assembly/errors' import type { User } from '@auth/lib/user.entity' import { getSanitizedHeaders, isAuthorized } from '@auth/lib/utils' import { HttpStatusCode } from 'axios' @@ -96,18 +96,28 @@ export const authenticateProxy = async (req: NextRequest): Promise * Uses: AuthenticatedAPIHeaders * @param headers containing required token payload header * @returns {User} instance modeled from the token payload headers + * @throws AssemblyMissingHeadersError when the proxy did not inject the token or workspaceId header * @throws AssemblyInvalidTokenError when the token payload headers are invalid */ export const authenticateHeaders = (headers: Headers): User => { const get = (headerName: string) => headers.get(headerName) || undefined - const token = z.string().parse(get(AuthenticatedAPIHeaders.CUSTOM_APP_TOKEN)) + const token = get(AuthenticatedAPIHeaders.CUSTOM_APP_TOKEN) + const workspaceId = get(AuthenticatedAPIHeaders.WORKSPACE_ID) const internalUserId = get(AuthenticatedAPIHeaders.INTERNAL_USER_ID) const clientId = get(AuthenticatedAPIHeaders.CLIENT_ID) const companyId = get(AuthenticatedAPIHeaders.COMPANY_ID) - const workspaceId = z.string().parse(get(AuthenticatedAPIHeaders.WORKSPACE_ID)) + + if (!token || !workspaceId) { + // Log the detail; throw generic so withErrorHandler can't leak it to clients. + const missing = [!token && 'token', !workspaceId && 'workspaceId'].filter(Boolean).join(', ') + console.warn(`AssemblyMissingHeadersError :: missing auth header(s): ${missing}`) + throw new AssemblyMissingHeadersError() + } if (!internalUserId && !clientId) { + // Log it: onRequestError drops this from Sentry. + console.warn('AssemblyInvalidTokenError :: headers lack both internalUserId and clientId') throw new AssemblyInvalidTokenError() } diff --git a/src/instrumentation.ts b/src/instrumentation.ts index af853730..ae612645 100644 --- a/src/instrumentation.ts +++ b/src/instrumentation.ts @@ -1,3 +1,4 @@ +import { AssemblyMissingHeadersError } from '@assembly/errors' import * as Sentry from '@sentry/nextjs' export async function register() { @@ -10,4 +11,8 @@ export async function register() { } } -export const onRequestError = Sentry.captureRequestError +export const onRequestError: typeof Sentry.captureRequestError = (err, request, context) => { + // Only suppress expected 401 noise (OUT-4013); other auth errors may mask real SDK failures. + if (err instanceof AssemblyMissingHeadersError) return + return Sentry.captureRequestError(err, request, context) +} diff --git a/tests/unit/authenticate-headers.test.ts b/tests/unit/authenticate-headers.test.ts new file mode 100644 index 00000000..858867a8 --- /dev/null +++ b/tests/unit/authenticate-headers.test.ts @@ -0,0 +1,109 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +// authenticate.ts imports @assembly/assembly-client which has heavy SDK dependencies +// that don't resolve in vitest's ESM environment. Mock it since authenticateHeaders +// doesn't use AssemblyClient at all. +vi.mock('@assembly/assembly-client', () => ({ + default: class MockAssemblyClient {}, +})) + +// Silence the missing-header diagnostic and let us assert on it. +const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => undefined) + +beforeEach(() => { + warnSpy.mockClear() +}) + +// Use dynamic import for modules that have vi.mock dependencies so vitest +// hoists the mock before resolution. +const { AssemblyInvalidTokenError, AssemblyMissingHeadersError } = await import('@assembly/errors') +const { AuthenticatedAPIHeaders } = await import('@/app/types') +const { authenticateHeaders } = await import('@auth/lib/authenticate') + +const validInternalHeaders = { + [AuthenticatedAPIHeaders.CUSTOM_APP_TOKEN]: 'valid-token', + [AuthenticatedAPIHeaders.INTERNAL_USER_ID]: 'user_123', + [AuthenticatedAPIHeaders.WORKSPACE_ID]: 'ws_456', +} + +const validClientHeaders = { + [AuthenticatedAPIHeaders.CUSTOM_APP_TOKEN]: 'valid-token', + [AuthenticatedAPIHeaders.CLIENT_ID]: 'client_789', + [AuthenticatedAPIHeaders.COMPANY_ID]: 'company_012', + [AuthenticatedAPIHeaders.WORKSPACE_ID]: 'ws_456', +} + +const buildHeaders = (init?: Record): Headers => new Headers(init) + +describe('authenticateHeaders', () => { + it('throws AssemblyMissingHeadersError when CUSTOM_APP_TOKEN is missing', () => { + const headers = buildHeaders({ + [AuthenticatedAPIHeaders.WORKSPACE_ID]: 'ws_456', + [AuthenticatedAPIHeaders.INTERNAL_USER_ID]: 'user_123', + }) + + expect(() => authenticateHeaders(headers)).toThrow(AssemblyMissingHeadersError) + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('token')) + }) + + it('throws AssemblyMissingHeadersError when WORKSPACE_ID is missing', () => { + const headers = buildHeaders({ + [AuthenticatedAPIHeaders.CUSTOM_APP_TOKEN]: 'valid-token', + [AuthenticatedAPIHeaders.INTERNAL_USER_ID]: 'user_123', + }) + + expect(() => authenticateHeaders(headers)).toThrow(AssemblyMissingHeadersError) + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('workspaceId')) + }) + + it('throws AssemblyMissingHeadersError when both CUSTOM_APP_TOKEN and WORKSPACE_ID are missing', () => { + const headers = buildHeaders({ + [AuthenticatedAPIHeaders.INTERNAL_USER_ID]: 'user_123', + }) + + expect(() => authenticateHeaders(headers)).toThrow(AssemblyMissingHeadersError) + }) + + it('throws AssemblyMissingHeadersError with an empty headers object', () => { + const headers = buildHeaders() + + expect(() => authenticateHeaders(headers)).toThrow(AssemblyMissingHeadersError) + }) + + it('throws AssemblyInvalidTokenError when both internalUserId and clientId are missing', () => { + const headers = buildHeaders({ + [AuthenticatedAPIHeaders.CUSTOM_APP_TOKEN]: 'valid-token', + [AuthenticatedAPIHeaders.WORKSPACE_ID]: 'ws_456', + }) + + expect(() => authenticateHeaders(headers)).toThrow(AssemblyInvalidTokenError) + }) + + it('returns a User for valid internal-user headers', () => { + const headers = buildHeaders(validInternalHeaders) + + const user = authenticateHeaders(headers) + + expect(user).toEqual({ + token: 'valid-token', + internalUserId: 'user_123', + clientId: undefined, + companyId: undefined, + workspaceId: 'ws_456', + }) + }) + + it('returns a User for valid client headers', () => { + const headers = buildHeaders(validClientHeaders) + + const user = authenticateHeaders(headers) + + expect(user).toEqual({ + token: 'valid-token', + internalUserId: undefined, + clientId: 'client_789', + companyId: 'company_012', + workspaceId: 'ws_456', + }) + }) +}) diff --git a/tests/unit/on-request-error.test.ts b/tests/unit/on-request-error.test.ts new file mode 100644 index 00000000..37cc8778 --- /dev/null +++ b/tests/unit/on-request-error.test.ts @@ -0,0 +1,83 @@ +import { AssemblyInvalidTokenError, AssemblyMissingHeadersError, AssemblyTokenParseError } from '@assembly/errors' +import type { captureRequestError } from '@sentry/nextjs' +import { afterEach, beforeEach, expect, it, vi } from 'vitest' + +// Mock @sentry/nextjs before importing instrumentation +const mockCaptureRequestError = vi.fn() +vi.mock('@sentry/nextjs', () => ({ + captureRequestError: mockCaptureRequestError, +})) + +const { onRequestError } = await import('@/instrumentation') + +type SentryRequestInfo = Parameters[1] +type SentryErrorContext = Parameters[2] + +const fakeRequest: SentryRequestInfo = { + path: '/', + method: 'GET', + headers: {}, +} + +const fakeContext: SentryErrorContext = { + routerKind: 'App Router', + routePath: '/', + routeType: 'render', +} + +beforeEach(() => { + mockCaptureRequestError.mockReset() +}) + +afterEach(() => { + vi.restoreAllMocks() +}) + +it('does not call Sentry for AssemblyMissingHeadersError', () => { + const error = new AssemblyMissingHeadersError() + + onRequestError(error, fakeRequest, fakeContext) + + expect(mockCaptureRequestError).not.toHaveBeenCalled() +}) + +// Must reach Sentry: AssemblyClient's catch-all can mask real SDK failures as this error. +it('calls Sentry for AssemblyInvalidTokenError', () => { + const error = new AssemblyInvalidTokenError() + + onRequestError(error, fakeRequest, fakeContext) + + expect(mockCaptureRequestError).toHaveBeenCalledWith(error, fakeRequest, fakeContext) +}) + +it('calls Sentry for AssemblyTokenParseError', () => { + const error = new AssemblyTokenParseError() + + onRequestError(error, fakeRequest, fakeContext) + + expect(mockCaptureRequestError).toHaveBeenCalledWith(error, fakeRequest, fakeContext) +}) + +it('calls Sentry for a generic Error', () => { + const error = new Error('something unexpected') + + onRequestError(error, fakeRequest, fakeContext) + + expect(mockCaptureRequestError).toHaveBeenCalledWith(error, fakeRequest, fakeContext) +}) + +it('calls Sentry for a generic Error only once', () => { + const error = new Error('something unexpected') + + onRequestError(error, fakeRequest, fakeContext) + + expect(mockCaptureRequestError).toHaveBeenCalledTimes(1) +}) + +it('calls Sentry for a non-Error thrown value', () => { + const error = 'a thrown string' + + onRequestError(error, fakeRequest, fakeContext) + + expect(mockCaptureRequestError).toHaveBeenCalledWith(error, fakeRequest, fakeContext) +})