From 51826e4775a297e5ff4a0471b87245ee4e3c4269 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 8 Aug 2026 12:06:01 -0700 Subject: [PATCH 1/2] fix(knowledge): apply knowledge-base access checks consistently across auth types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tag-definitions route only ran its knowledge-base access check for browser sessions, skipping it for internal JWT callers. Authorize on the acting user for every auth type instead — read access for GET, write access for POST — and require an acting user to be present, matching the sibling knowledge routes. Thread the acting user through the KB tag schema enrichers so their request carries the identity the route now authorizes. --- .../[id]/tag-definitions/route.test.ts | 144 ++++++++++++++++++ .../knowledge/[id]/tag-definitions/route.ts | 40 ++--- apps/sim/tools/params.ts | 2 +- apps/sim/tools/schema-enrichers.test.ts | 40 ++++- apps/sim/tools/schema-enrichers.ts | 29 +++- apps/sim/tools/types.ts | 5 +- 6 files changed, 226 insertions(+), 34 deletions(-) create mode 100644 apps/sim/app/api/knowledge/[id]/tag-definitions/route.test.ts diff --git a/apps/sim/app/api/knowledge/[id]/tag-definitions/route.test.ts b/apps/sim/app/api/knowledge/[id]/tag-definitions/route.test.ts new file mode 100644 index 00000000000..09d8dc2e2d6 --- /dev/null +++ b/apps/sim/app/api/knowledge/[id]/tag-definitions/route.test.ts @@ -0,0 +1,144 @@ +/** + * Tests for knowledge base tag definitions API route + * + * @vitest-environment node + */ +import { + createMockRequest, + hybridAuthMockFns, + knowledgeApiUtilsMock, + knowledgeApiUtilsMockFns, +} from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockGetTagDefinitions, mockCreateTagDefinition } = vi.hoisted(() => ({ + mockGetTagDefinitions: vi.fn(), + mockCreateTagDefinition: vi.fn(), +})) + +vi.mock('@/lib/knowledge/tags/service', () => ({ + getTagDefinitions: mockGetTagDefinitions, + createTagDefinition: mockCreateTagDefinition, +})) + +vi.mock('@/app/api/knowledge/utils', () => knowledgeApiUtilsMock) + +import { GET, POST } from '@/app/api/knowledge/[id]/tag-definitions/route' + +const KB_ID = 'kb-victim' +const TAG_DEFINITIONS = [ + { id: 'tag-def-1', tagSlot: 'tag1', displayName: 'Client Name', fieldType: 'text' }, +] +const CREATE_BODY = { tagSlot: 'tag1', displayName: 'Injected', fieldType: 'text' } + +const params = () => ({ params: Promise.resolve({ id: KB_ID }) }) + +const { mockCheckKnowledgeBaseAccess, mockCheckKnowledgeBaseWriteAccess } = knowledgeApiUtilsMockFns + +/** Stubs the auth result the route sees. Omit `userId` for a JWT with no acting user. */ +function authenticateAs(userId?: string, authType = 'internal_jwt') { + hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({ + success: true, + authType, + ...(userId ? { userId } : {}), + }) +} + +const granted = { hasAccess: true, knowledgeBase: { id: KB_ID, userId: 'user-1' } } + +describe('Knowledge Base Tag Definitions API Route', () => { + beforeEach(() => { + vi.clearAllMocks() + mockGetTagDefinitions.mockResolvedValue(TAG_DEFINITIONS) + mockCreateTagDefinition.mockResolvedValue({ id: 'tag-def-new' }) + }) + + describe('GET /api/knowledge/[id]/tag-definitions', () => { + it('returns tag definitions to a caller with read access', async () => { + authenticateAs('user-1', 'session') + mockCheckKnowledgeBaseAccess.mockResolvedValue(granted) + + const response = await GET(createMockRequest('GET'), params()) + + expect(response.status).toBe(200) + await expect(response.json()).resolves.toEqual({ success: true, data: TAG_DEFINITIONS }) + }) + + it('gates reads on read access, not write access', async () => { + authenticateAs('user-1', 'session') + mockCheckKnowledgeBaseAccess.mockResolvedValue(granted) + + await GET(createMockRequest('GET'), params()) + + expect(mockCheckKnowledgeBaseAccess).toHaveBeenCalledWith(KB_ID, 'user-1') + expect(mockCheckKnowledgeBaseWriteAccess).not.toHaveBeenCalled() + }) + + it('authorizes internal JWT callers instead of trusting them', async () => { + authenticateAs('attacker-1') + mockCheckKnowledgeBaseAccess.mockResolvedValue({ hasAccess: false }) + + const response = await GET(createMockRequest('GET'), params()) + + expect(response.status).toBe(403) + expect(mockCheckKnowledgeBaseAccess).toHaveBeenCalledWith(KB_ID, 'attacker-1') + expect(mockGetTagDefinitions).not.toHaveBeenCalled() + }) + + it('returns 404 for an unknown knowledge base', async () => { + authenticateAs('attacker-1') + mockCheckKnowledgeBaseAccess.mockResolvedValue({ hasAccess: false, notFound: true }) + + const response = await GET(createMockRequest('GET'), params()) + + expect(response.status).toBe(404) + expect(mockGetTagDefinitions).not.toHaveBeenCalled() + }) + + it('rejects a JWT that carries no acting user', async () => { + authenticateAs() + + const response = await GET(createMockRequest('GET'), params()) + + expect(response.status).toBe(401) + expect(mockCheckKnowledgeBaseAccess).not.toHaveBeenCalled() + expect(mockGetTagDefinitions).not.toHaveBeenCalled() + }) + }) + + describe('POST /api/knowledge/[id]/tag-definitions', () => { + it('creates a tag definition for a caller with write access', async () => { + authenticateAs('user-1', 'session') + mockCheckKnowledgeBaseWriteAccess.mockResolvedValue(granted) + + const response = await POST(createMockRequest('POST', CREATE_BODY), params()) + + expect(response.status).toBe(200) + expect(mockCreateTagDefinition).toHaveBeenCalledWith( + expect.objectContaining({ knowledgeBaseId: KB_ID, tagSlot: 'tag1' }), + expect.any(String) + ) + }) + + it('authorizes internal JWT callers instead of trusting them', async () => { + authenticateAs('attacker-1') + mockCheckKnowledgeBaseWriteAccess.mockResolvedValue({ hasAccess: false }) + + const response = await POST(createMockRequest('POST', CREATE_BODY), params()) + + expect(response.status).toBe(403) + expect(mockCheckKnowledgeBaseWriteAccess).toHaveBeenCalledWith(KB_ID, 'attacker-1') + expect(mockCreateTagDefinition).not.toHaveBeenCalled() + }) + + it('rejects a JWT that carries no acting user', async () => { + authenticateAs() + + const response = await POST(createMockRequest('POST', CREATE_BODY), params()) + + expect(response.status).toBe(401) + expect(mockCheckKnowledgeBaseWriteAccess).not.toHaveBeenCalled() + expect(mockCreateTagDefinition).not.toHaveBeenCalled() + }) + }) +}) diff --git a/apps/sim/app/api/knowledge/[id]/tag-definitions/route.ts b/apps/sim/app/api/knowledge/[id]/tag-definitions/route.ts index 8d8b1cc41be..811077245b3 100644 --- a/apps/sim/app/api/knowledge/[id]/tag-definitions/route.ts +++ b/apps/sim/app/api/knowledge/[id]/tag-definitions/route.ts @@ -3,17 +3,16 @@ import { generateId } from '@sim/utils/id' import { type NextRequest, NextResponse } from 'next/server' import { createTagDefinitionContract } from '@/lib/api/contracts/knowledge' import { parseRequest } from '@/lib/api/server' -import { AuthType, checkSessionOrInternalAuth } from '@/lib/auth/hybrid' +import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { SUPPORTED_FIELD_TYPES } from '@/lib/knowledge/constants' import { createTagDefinition, getTagDefinitions } from '@/lib/knowledge/tags/service' -import { checkKnowledgeBaseWriteAccess } from '@/app/api/knowledge/utils' +import { checkKnowledgeBaseAccess, checkKnowledgeBaseWriteAccess } from '@/app/api/knowledge/utils' export const dynamic = 'force-dynamic' const logger = createLogger('KnowledgeBaseTagDefinitionsAPI') -// GET /api/knowledge/[id]/tag-definitions - Get all tag definitions for a knowledge base export const GET = withRouteHandler( async (req: NextRequest, { params }: { params: Promise<{ id: string }> }) => { const requestId = generateId().slice(0, 8) @@ -23,19 +22,16 @@ export const GET = withRouteHandler( logger.info(`[${requestId}] Getting tag definitions for knowledge base ${knowledgeBaseId}`) const auth = await checkSessionOrInternalAuth(req, { requireWorkflowId: false }) - if (!auth.success) { + if (!auth.success || !auth.userId) { return NextResponse.json({ error: auth.error || 'Unauthorized' }, { status: 401 }) } - // For session auth, verify KB access. Internal JWT is trusted. - if (auth.authType === AuthType.SESSION && auth.userId) { - const accessCheck = await checkKnowledgeBaseWriteAccess(knowledgeBaseId, auth.userId) - if (!accessCheck.hasAccess) { - return NextResponse.json( - { error: accessCheck.notFound ? 'Not found' : 'Forbidden' }, - { status: accessCheck.notFound ? 404 : 403 } - ) - } + const accessCheck = await checkKnowledgeBaseAccess(knowledgeBaseId, auth.userId) + if (!accessCheck.hasAccess) { + return NextResponse.json( + { error: accessCheck.notFound ? 'Not found' : 'Forbidden' }, + { status: accessCheck.notFound ? 404 : 403 } + ) } const tagDefinitions = await getTagDefinitions(knowledgeBaseId) @@ -55,7 +51,6 @@ export const GET = withRouteHandler( } ) -// POST /api/knowledge/[id]/tag-definitions - Create a new tag definition export const POST = withRouteHandler( async (req: NextRequest, context: { params: Promise<{ id: string }> }) => { const requestId = generateId().slice(0, 8) @@ -65,19 +60,16 @@ export const POST = withRouteHandler( logger.info(`[${requestId}] Creating tag definition for knowledge base ${knowledgeBaseId}`) const auth = await checkSessionOrInternalAuth(req, { requireWorkflowId: false }) - if (!auth.success) { + if (!auth.success || !auth.userId) { return NextResponse.json({ error: auth.error || 'Unauthorized' }, { status: 401 }) } - // For session auth, verify KB access. Internal JWT is trusted. - if (auth.authType === AuthType.SESSION && auth.userId) { - const accessCheck = await checkKnowledgeBaseWriteAccess(knowledgeBaseId, auth.userId) - if (!accessCheck.hasAccess) { - return NextResponse.json( - { error: accessCheck.notFound ? 'Not found' : 'Forbidden' }, - { status: accessCheck.notFound ? 404 : 403 } - ) - } + const accessCheck = await checkKnowledgeBaseWriteAccess(knowledgeBaseId, auth.userId) + if (!accessCheck.hasAccess) { + return NextResponse.json( + { error: accessCheck.notFound ? 'Not found' : 'Forbidden' }, + { status: accessCheck.notFound ? 404 : 403 } + ) } const parsed = await parseRequest(createTagDefinitionContract, req, context) diff --git a/apps/sim/tools/params.ts b/apps/sim/tools/params.ts index 75dc85c2a5e..ebccb601991 100644 --- a/apps/sim/tools/params.ts +++ b/apps/sim/tools/params.ts @@ -667,7 +667,7 @@ export async function createLLMToolSchema( } const propertySchema = buildParameterSchema(toolConfig.id, paramId, param) - const enrichedSchema = await enrichmentConfig.enrichSchema(dependencyValue) + const enrichedSchema = await enrichmentConfig.enrichSchema(dependencyValue, enrichmentContext) if (enrichedSchema) { safeAssign(propertySchema, enrichedSchema as Record) diff --git a/apps/sim/tools/schema-enrichers.test.ts b/apps/sim/tools/schema-enrichers.test.ts index 655507ab514..f7542177272 100644 --- a/apps/sim/tools/schema-enrichers.test.ts +++ b/apps/sim/tools/schema-enrichers.test.ts @@ -21,7 +21,7 @@ vi.mock('@/executor/utils/http', () => ({ extractAPIErrorMessage: mockExtractAPIErrorMessage, })) -import { enrichTableToolSchema } from '@/tools/schema-enrichers' +import { enrichKBTagsSchema, enrichTableToolSchema } from '@/tools/schema-enrichers' const ORIGINAL_SCHEMA = { type: 'object' as const, @@ -102,3 +102,41 @@ describe('enrichTableToolSchema', () => { ).rejects.toThrow('Workspace ID is required to enrich table tool schema for table-1') }) }) + +describe('enrichKBTagsSchema', () => { + beforeEach(() => { + vi.clearAllMocks() + mockBuildAuthHeaders.mockResolvedValue({ Authorization: 'Bearer internal-token' }) + }) + + afterEach(() => { + vi.unstubAllGlobals() + }) + + it('fetches tag definitions as the acting user so the route can authorize them', async () => { + const mockFetch = vi.fn().mockResolvedValue( + new Response( + JSON.stringify({ + success: true, + data: [{ id: 'td-1', tagSlot: 'tag1', displayName: 'Client', fieldType: 'text' }], + }), + { status: 200, headers: { 'Content-Type': 'application/json' } } + ) + ) + vi.stubGlobal('fetch', mockFetch) + + const result = await enrichKBTagsSchema('kb-1', { userId: 'user-1' }) + + expect(mockBuildAuthHeaders).toHaveBeenCalledWith('user-1') + expect(result?.properties).toEqual({ Client: { type: 'string', description: 'text tag' } }) + }) + + it('skips enrichment without an acting user rather than issuing an unauthorized request', async () => { + const mockFetch = vi.fn() + vi.stubGlobal('fetch', mockFetch) + + await expect(enrichKBTagsSchema('kb-1', {})).resolves.toBeNull() + expect(mockFetch).not.toHaveBeenCalled() + expect(mockBuildAuthHeaders).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/tools/schema-enrichers.ts b/apps/sim/tools/schema-enrichers.ts index a0132aa6da7..27f91fd50f7 100644 --- a/apps/sim/tools/schema-enrichers.ts +++ b/apps/sim/tools/schema-enrichers.ts @@ -114,13 +114,22 @@ function mapFieldTypeToSchemaType(fieldType: string): string { } /** - * Fetches tag definitions from knowledge base + * Fetches tag definitions from a knowledge base as the acting user, whose id the + * route requires to authorize the read. */ -async function fetchTagDefinitions(knowledgeBaseId: string): Promise { +async function fetchTagDefinitions( + knowledgeBaseId: string, + context: WorkflowToolExecutionContext +): Promise { + if (!context.userId) { + logger.warn(`Skipping tag definition enrichment for KB ${knowledgeBaseId}: no acting user`) + return [] + } + try { const { buildAuthHeaders, buildAPIUrl } = await import('@/executor/utils/http') - const headers = await buildAuthHeaders() + const headers = await buildAuthHeaders(context.userId) const url = buildAPIUrl(`/api/knowledge/${knowledgeBaseId}/tag-definitions`) logger.info(`Fetching tag definitions for KB ${knowledgeBaseId} from ${url.toString()}`) @@ -145,13 +154,16 @@ async function fetchTagDefinitions(knowledgeBaseId: string): Promise description?: string required?: string[] } | null> { - const tagDefinitions = await fetchTagDefinitions(knowledgeBaseId) + const tagDefinitions = await fetchTagDefinitions(knowledgeBaseId, context) if (tagDefinitions.length === 0) { return null @@ -181,12 +193,15 @@ export async function enrichKBTagsSchema(knowledgeBaseId: string): Promise<{ * Fetches KB tag definitions and builds a schema for tag filters. * Returns an array schema where each item is a filter with tagName and tagValue. */ -export async function enrichKBTagFiltersSchema(knowledgeBaseId: string): Promise<{ +export async function enrichKBTagFiltersSchema( + knowledgeBaseId: string, + context: WorkflowToolExecutionContext +): Promise<{ type: string items?: Record description?: string } | null> { - const tagDefinitions = await fetchTagDefinitions(knowledgeBaseId) + const tagDefinitions = await fetchTagDefinitions(knowledgeBaseId, context) if (tagDefinitions.length === 0) { return null diff --git a/apps/sim/tools/types.ts b/apps/sim/tools/types.ts index 94016b758ef..c9b15ee356c 100644 --- a/apps/sim/tools/types.ts +++ b/apps/sim/tools/types.ts @@ -309,7 +309,10 @@ interface SchemaEnrichmentConfig { /** The param ID that this enrichment depends on (e.g., 'knowledgeBaseId', 'workflowId') */ dependsOn: string /** Function to fetch and build dynamic schema based on the dependency value */ - enrichSchema: (dependencyValue: string) => Promise<{ + enrichSchema: ( + dependencyValue: string, + context: WorkflowToolExecutionContext + ) => Promise<{ type: string properties?: Record description?: string From 3419e4f56a7b3e9d22e604486a7a284f5e6db224 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 8 Aug 2026 12:11:24 -0700 Subject: [PATCH 2/2] chore(tests): mark tag-definition test fixtures as const --- .../app/api/knowledge/[id]/tag-definitions/route.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/sim/app/api/knowledge/[id]/tag-definitions/route.test.ts b/apps/sim/app/api/knowledge/[id]/tag-definitions/route.test.ts index 09d8dc2e2d6..23231c66581 100644 --- a/apps/sim/app/api/knowledge/[id]/tag-definitions/route.test.ts +++ b/apps/sim/app/api/knowledge/[id]/tag-definitions/route.test.ts @@ -28,8 +28,8 @@ import { GET, POST } from '@/app/api/knowledge/[id]/tag-definitions/route' const KB_ID = 'kb-victim' const TAG_DEFINITIONS = [ { id: 'tag-def-1', tagSlot: 'tag1', displayName: 'Client Name', fieldType: 'text' }, -] -const CREATE_BODY = { tagSlot: 'tag1', displayName: 'Injected', fieldType: 'text' } +] as const +const CREATE_BODY = { tagSlot: 'tag1', displayName: 'Injected', fieldType: 'text' } as const const params = () => ({ params: Promise.resolve({ id: KB_ID }) }) @@ -44,7 +44,7 @@ function authenticateAs(userId?: string, authType = 'internal_jwt') { }) } -const granted = { hasAccess: true, knowledgeBase: { id: KB_ID, userId: 'user-1' } } +const granted = { hasAccess: true, knowledgeBase: { id: KB_ID, userId: 'user-1' } } as const describe('Knowledge Base Tag Definitions API Route', () => { beforeEach(() => {