Skip to content

Commit 7c47366

Browse files
waleedlatif1waleedlatif1
committed
fix(knowledge): apply knowledge-base access checks consistently across auth types
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.
1 parent 4f5ad20 commit 7c47366

6 files changed

Lines changed: 226 additions & 34 deletions

File tree

‎apps/sim/app/api/knowledge/[id]/tag-definitions/route.test.ts‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
/**
2+
* Tests for knowledge base tag definitions API route
3+
*
4+
* @vitest-environment node
5+
*/
6+
import {
7+
createMockRequest,
8+
hybridAuthMockFns,
9+
knowledgeApiUtilsMock,
10+
knowledgeApiUtilsMockFns,
11+
} from '@sim/testing'
12+
import { beforeEach, describe, expect, it, vi } from 'vitest'
13+
14+
const { mockGetTagDefinitions, mockCreateTagDefinition } = vi.hoisted(() => ({
15+
mockGetTagDefinitions: vi.fn(),
16+
mockCreateTagDefinition: vi.fn(),
17+
}))
18+
19+
vi.mock('@/lib/knowledge/tags/service', () => ({
20+
getTagDefinitions: mockGetTagDefinitions,
21+
createTagDefinition: mockCreateTagDefinition,
22+
}))
23+
24+
vi.mock('@/app/api/knowledge/utils', () => knowledgeApiUtilsMock)
25+
26+
import { GET, POST } from '@/app/api/knowledge/[id]/tag-definitions/route'
27+
28+
const KB_ID = 'kb-victim'
29+
const TAG_DEFINITIONS = [
30+
{ id: 'tag-def-1', tagSlot: 'tag1', displayName: 'Client Name', fieldType: 'text' },
31+
]
32+
const CREATE_BODY = { tagSlot: 'tag1', displayName: 'Injected', fieldType: 'text' }
33+
34+
const params = () => ({ params: Promise.resolve({ id: KB_ID }) })
35+
36+
const { mockCheckKnowledgeBaseAccess, mockCheckKnowledgeBaseWriteAccess } = knowledgeApiUtilsMockFns
37+
38+
/** Stubs the auth result the route sees. Omit `userId` for a JWT with no acting user. */
39+
function authenticateAs(userId?: string, authType = 'internal_jwt') {
40+
hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({
41+
success: true,
42+
authType,
43+
...(userId ? { userId } : {}),
44+
})
45+
}
46+
47+
const granted = { hasAccess: true, knowledgeBase: { id: KB_ID, userId: 'user-1' } }
48+
49+
describe('Knowledge Base Tag Definitions API Route', () => {
50+
beforeEach(() => {
51+
vi.clearAllMocks()
52+
mockGetTagDefinitions.mockResolvedValue(TAG_DEFINITIONS)
53+
mockCreateTagDefinition.mockResolvedValue({ id: 'tag-def-new' })
54+
})
55+
56+
describe('GET /api/knowledge/[id]/tag-definitions', () => {
57+
it('returns tag definitions to a caller with read access', async () => {
58+
authenticateAs('user-1', 'session')
59+
mockCheckKnowledgeBaseAccess.mockResolvedValue(granted)
60+
61+
const response = await GET(createMockRequest('GET'), params())
62+
63+
expect(response.status).toBe(200)
64+
await expect(response.json()).resolves.toEqual({ success: true, data: TAG_DEFINITIONS })
65+
})
66+
67+
it('gates reads on read access, not write access', async () => {
68+
authenticateAs('user-1', 'session')
69+
mockCheckKnowledgeBaseAccess.mockResolvedValue(granted)
70+
71+
await GET(createMockRequest('GET'), params())
72+
73+
expect(mockCheckKnowledgeBaseAccess).toHaveBeenCalledWith(KB_ID, 'user-1')
74+
expect(mockCheckKnowledgeBaseWriteAccess).not.toHaveBeenCalled()
75+
})
76+
77+
it('authorizes internal JWT callers instead of trusting them', async () => {
78+
authenticateAs('attacker-1')
79+
mockCheckKnowledgeBaseAccess.mockResolvedValue({ hasAccess: false })
80+
81+
const response = await GET(createMockRequest('GET'), params())
82+
83+
expect(response.status).toBe(403)
84+
expect(mockCheckKnowledgeBaseAccess).toHaveBeenCalledWith(KB_ID, 'attacker-1')
85+
expect(mockGetTagDefinitions).not.toHaveBeenCalled()
86+
})
87+
88+
it('returns 404 for an unknown knowledge base', async () => {
89+
authenticateAs('attacker-1')
90+
mockCheckKnowledgeBaseAccess.mockResolvedValue({ hasAccess: false, notFound: true })
91+
92+
const response = await GET(createMockRequest('GET'), params())
93+
94+
expect(response.status).toBe(404)
95+
expect(mockGetTagDefinitions).not.toHaveBeenCalled()
96+
})
97+
98+
it('rejects a JWT that carries no acting user', async () => {
99+
authenticateAs()
100+
101+
const response = await GET(createMockRequest('GET'), params())
102+
103+
expect(response.status).toBe(401)
104+
expect(mockCheckKnowledgeBaseAccess).not.toHaveBeenCalled()
105+
expect(mockGetTagDefinitions).not.toHaveBeenCalled()
106+
})
107+
})
108+
109+
describe('POST /api/knowledge/[id]/tag-definitions', () => {
110+
it('creates a tag definition for a caller with write access', async () => {
111+
authenticateAs('user-1', 'session')
112+
mockCheckKnowledgeBaseWriteAccess.mockResolvedValue(granted)
113+
114+
const response = await POST(createMockRequest('POST', CREATE_BODY), params())
115+
116+
expect(response.status).toBe(200)
117+
expect(mockCreateTagDefinition).toHaveBeenCalledWith(
118+
expect.objectContaining({ knowledgeBaseId: KB_ID, tagSlot: 'tag1' }),
119+
expect.any(String)
120+
)
121+
})
122+
123+
it('authorizes internal JWT callers instead of trusting them', async () => {
124+
authenticateAs('attacker-1')
125+
mockCheckKnowledgeBaseWriteAccess.mockResolvedValue({ hasAccess: false })
126+
127+
const response = await POST(createMockRequest('POST', CREATE_BODY), params())
128+
129+
expect(response.status).toBe(403)
130+
expect(mockCheckKnowledgeBaseWriteAccess).toHaveBeenCalledWith(KB_ID, 'attacker-1')
131+
expect(mockCreateTagDefinition).not.toHaveBeenCalled()
132+
})
133+
134+
it('rejects a JWT that carries no acting user', async () => {
135+
authenticateAs()
136+
137+
const response = await POST(createMockRequest('POST', CREATE_BODY), params())
138+
139+
expect(response.status).toBe(401)
140+
expect(mockCheckKnowledgeBaseWriteAccess).not.toHaveBeenCalled()
141+
expect(mockCreateTagDefinition).not.toHaveBeenCalled()
142+
})
143+
})
144+
})

‎apps/sim/app/api/knowledge/[id]/tag-definitions/route.ts‎

Lines changed: 16 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -3,17 +3,16 @@ import { generateId } from '@sim/utils/id'
33
import { type NextRequest, NextResponse } from 'next/server'
44
import { createTagDefinitionContract } from '@/lib/api/contracts/knowledge'
55
import { parseRequest } from '@/lib/api/server'
6-
import { AuthType, checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
6+
import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
77
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
88
import { SUPPORTED_FIELD_TYPES } from '@/lib/knowledge/constants'
99
import { createTagDefinition, getTagDefinitions } from '@/lib/knowledge/tags/service'
10-
import { checkKnowledgeBaseWriteAccess } from '@/app/api/knowledge/utils'
10+
import { checkKnowledgeBaseAccess, checkKnowledgeBaseWriteAccess } from '@/app/api/knowledge/utils'
1111

1212
export const dynamic = 'force-dynamic'
1313

1414
const logger = createLogger('KnowledgeBaseTagDefinitionsAPI')
1515

16-
// GET /api/knowledge/[id]/tag-definitions - Get all tag definitions for a knowledge base
1716
export const GET = withRouteHandler(
1817
async (req: NextRequest, { params }: { params: Promise<{ id: string }> }) => {
1918
const requestId = generateId().slice(0, 8)
@@ -23,19 +22,16 @@ export const GET = withRouteHandler(
2322
logger.info(`[${requestId}] Getting tag definitions for knowledge base ${knowledgeBaseId}`)
2423

2524
const auth = await checkSessionOrInternalAuth(req, { requireWorkflowId: false })
26-
if (!auth.success) {
25+
if (!auth.success || !auth.userId) {
2726
return NextResponse.json({ error: auth.error || 'Unauthorized' }, { status: 401 })
2827
}
2928

30-
// For session auth, verify KB access. Internal JWT is trusted.
31-
if (auth.authType === AuthType.SESSION && auth.userId) {
32-
const accessCheck = await checkKnowledgeBaseWriteAccess(knowledgeBaseId, auth.userId)
33-
if (!accessCheck.hasAccess) {
34-
return NextResponse.json(
35-
{ error: accessCheck.notFound ? 'Not found' : 'Forbidden' },
36-
{ status: accessCheck.notFound ? 404 : 403 }
37-
)
38-
}
29+
const accessCheck = await checkKnowledgeBaseAccess(knowledgeBaseId, auth.userId)
30+
if (!accessCheck.hasAccess) {
31+
return NextResponse.json(
32+
{ error: accessCheck.notFound ? 'Not found' : 'Forbidden' },
33+
{ status: accessCheck.notFound ? 404 : 403 }
34+
)
3935
}
4036

4137
const tagDefinitions = await getTagDefinitions(knowledgeBaseId)
@@ -55,7 +51,6 @@ export const GET = withRouteHandler(
5551
}
5652
)
5753

58-
// POST /api/knowledge/[id]/tag-definitions - Create a new tag definition
5954
export const POST = withRouteHandler(
6055
async (req: NextRequest, context: { params: Promise<{ id: string }> }) => {
6156
const requestId = generateId().slice(0, 8)
@@ -65,19 +60,16 @@ export const POST = withRouteHandler(
6560
logger.info(`[${requestId}] Creating tag definition for knowledge base ${knowledgeBaseId}`)
6661

6762
const auth = await checkSessionOrInternalAuth(req, { requireWorkflowId: false })
68-
if (!auth.success) {
63+
if (!auth.success || !auth.userId) {
6964
return NextResponse.json({ error: auth.error || 'Unauthorized' }, { status: 401 })
7065
}
7166

72-
// For session auth, verify KB access. Internal JWT is trusted.
73-
if (auth.authType === AuthType.SESSION && auth.userId) {
74-
const accessCheck = await checkKnowledgeBaseWriteAccess(knowledgeBaseId, auth.userId)
75-
if (!accessCheck.hasAccess) {
76-
return NextResponse.json(
77-
{ error: accessCheck.notFound ? 'Not found' : 'Forbidden' },
78-
{ status: accessCheck.notFound ? 404 : 403 }
79-
)
80-
}
67+
const accessCheck = await checkKnowledgeBaseWriteAccess(knowledgeBaseId, auth.userId)
68+
if (!accessCheck.hasAccess) {
69+
return NextResponse.json(
70+
{ error: accessCheck.notFound ? 'Not found' : 'Forbidden' },
71+
{ status: accessCheck.notFound ? 404 : 403 }
72+
)
8173
}
8274

8375
const parsed = await parseRequest(createTagDefinitionContract, req, context)

‎apps/sim/tools/params.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -667,7 +667,7 @@ export async function createLLMToolSchema(
667667
}
668668

669669
const propertySchema = buildParameterSchema(toolConfig.id, paramId, param)
670-
const enrichedSchema = await enrichmentConfig.enrichSchema(dependencyValue)
670+
const enrichedSchema = await enrichmentConfig.enrichSchema(dependencyValue, enrichmentContext)
671671

672672
if (enrichedSchema) {
673673
safeAssign(propertySchema, enrichedSchema as Record<string, unknown>)

‎apps/sim/tools/schema-enrichers.test.ts‎

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ vi.mock('@/executor/utils/http', () => ({
2121
extractAPIErrorMessage: mockExtractAPIErrorMessage,
2222
}))
2323

24-
import { enrichTableToolSchema } from '@/tools/schema-enrichers'
24+
import { enrichKBTagsSchema, enrichTableToolSchema } from '@/tools/schema-enrichers'
2525

2626
const ORIGINAL_SCHEMA = {
2727
type: 'object' as const,
@@ -102,3 +102,41 @@ describe('enrichTableToolSchema', () => {
102102
).rejects.toThrow('Workspace ID is required to enrich table tool schema for table-1')
103103
})
104104
})
105+
106+
describe('enrichKBTagsSchema', () => {
107+
beforeEach(() => {
108+
vi.clearAllMocks()
109+
mockBuildAuthHeaders.mockResolvedValue({ Authorization: 'Bearer internal-token' })
110+
})
111+
112+
afterEach(() => {
113+
vi.unstubAllGlobals()
114+
})
115+
116+
it('fetches tag definitions as the acting user so the route can authorize them', async () => {
117+
const mockFetch = vi.fn().mockResolvedValue(
118+
new Response(
119+
JSON.stringify({
120+
success: true,
121+
data: [{ id: 'td-1', tagSlot: 'tag1', displayName: 'Client', fieldType: 'text' }],
122+
}),
123+
{ status: 200, headers: { 'Content-Type': 'application/json' } }
124+
)
125+
)
126+
vi.stubGlobal('fetch', mockFetch)
127+
128+
const result = await enrichKBTagsSchema('kb-1', { userId: 'user-1' })
129+
130+
expect(mockBuildAuthHeaders).toHaveBeenCalledWith('user-1')
131+
expect(result?.properties).toEqual({ Client: { type: 'string', description: 'text tag' } })
132+
})
133+
134+
it('skips enrichment without an acting user rather than issuing an unauthorized request', async () => {
135+
const mockFetch = vi.fn()
136+
vi.stubGlobal('fetch', mockFetch)
137+
138+
await expect(enrichKBTagsSchema('kb-1', {})).resolves.toBeNull()
139+
expect(mockFetch).not.toHaveBeenCalled()
140+
expect(mockBuildAuthHeaders).not.toHaveBeenCalled()
141+
})
142+
})

‎apps/sim/tools/schema-enrichers.ts‎

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -114,13 +114,22 @@ function mapFieldTypeToSchemaType(fieldType: string): string {
114114
}
115115

116116
/**
117-
* Fetches tag definitions from knowledge base
117+
* Fetches tag definitions from a knowledge base as the acting user, whose id the
118+
* route requires to authorize the read.
118119
*/
119-
async function fetchTagDefinitions(knowledgeBaseId: string): Promise<TagDefinition[]> {
120+
async function fetchTagDefinitions(
121+
knowledgeBaseId: string,
122+
context: WorkflowToolExecutionContext
123+
): Promise<TagDefinition[]> {
124+
if (!context.userId) {
125+
logger.warn(`Skipping tag definition enrichment for KB ${knowledgeBaseId}: no acting user`)
126+
return []
127+
}
128+
120129
try {
121130
const { buildAuthHeaders, buildAPIUrl } = await import('@/executor/utils/http')
122131

123-
const headers = await buildAuthHeaders()
132+
const headers = await buildAuthHeaders(context.userId)
124133
const url = buildAPIUrl(`/api/knowledge/${knowledgeBaseId}/tag-definitions`)
125134

126135
logger.info(`Fetching tag definitions for KB ${knowledgeBaseId} from ${url.toString()}`)
@@ -145,13 +154,16 @@ async function fetchTagDefinitions(knowledgeBaseId: string): Promise<TagDefiniti
145154
* Fetches KB tag definitions and builds a schema for LLM consumption.
146155
* Returns an object schema where each property is a tag the LLM can set.
147156
*/
148-
export async function enrichKBTagsSchema(knowledgeBaseId: string): Promise<{
157+
export async function enrichKBTagsSchema(
158+
knowledgeBaseId: string,
159+
context: WorkflowToolExecutionContext
160+
): Promise<{
149161
type: string
150162
properties?: Record<string, { type: string; description?: string }>
151163
description?: string
152164
required?: string[]
153165
} | null> {
154-
const tagDefinitions = await fetchTagDefinitions(knowledgeBaseId)
166+
const tagDefinitions = await fetchTagDefinitions(knowledgeBaseId, context)
155167

156168
if (tagDefinitions.length === 0) {
157169
return null
@@ -181,12 +193,15 @@ export async function enrichKBTagsSchema(knowledgeBaseId: string): Promise<{
181193
* Fetches KB tag definitions and builds a schema for tag filters.
182194
* Returns an array schema where each item is a filter with tagName and tagValue.
183195
*/
184-
export async function enrichKBTagFiltersSchema(knowledgeBaseId: string): Promise<{
196+
export async function enrichKBTagFiltersSchema(
197+
knowledgeBaseId: string,
198+
context: WorkflowToolExecutionContext
199+
): Promise<{
185200
type: string
186201
items?: Record<string, unknown>
187202
description?: string
188203
} | null> {
189-
const tagDefinitions = await fetchTagDefinitions(knowledgeBaseId)
204+
const tagDefinitions = await fetchTagDefinitions(knowledgeBaseId, context)
190205

191206
if (tagDefinitions.length === 0) {
192207
return null

‎apps/sim/tools/types.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,10 @@ interface SchemaEnrichmentConfig {
309309
/** The param ID that this enrichment depends on (e.g., 'knowledgeBaseId', 'workflowId') */
310310
dependsOn: string
311311
/** Function to fetch and build dynamic schema based on the dependency value */
312-
enrichSchema: (dependencyValue: string) => Promise<{
312+
enrichSchema: (
313+
dependencyValue: string,
314+
context: WorkflowToolExecutionContext
315+
) => Promise<{
313316
type: string
314317
properties?: Record<string, { type: string; description?: string }>
315318
description?: string

0 commit comments

Comments
 (0)