Skip to content

Commit 6c90338

Browse files
fixes
1 parent 5e5c134 commit 6c90338

9 files changed

Lines changed: 183 additions & 216 deletions

File tree

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
import { describe, expect, it } from 'vitest';
2+
import { enforceQuerySecurity } from '../../utils/signozHelpers';
3+
4+
describe('enforceQuerySecurity', () => {
5+
const validQuery = "SELECT * FROM t WHERE attributes_string['tenant.id'] = {{.tenant_id}}";
6+
7+
it('injects tenant_id variable', () => {
8+
const payload = {
9+
compositeQuery: { chQueries: { A: { query: validQuery } } },
10+
};
11+
const result = enforceQuerySecurity(payload, 'tenant-1');
12+
expect(result.error).toBeUndefined();
13+
expect(result.payload.variables.tenant_id).toBe('tenant-1');
14+
});
15+
16+
it('injects project_id variable when provided', () => {
17+
const payload = {
18+
compositeQuery: { chQueries: { A: { query: validQuery } } },
19+
};
20+
const result = enforceQuerySecurity(payload, 'tenant-1', 'project-1');
21+
expect(result.payload.variables).toEqual({
22+
tenant_id: 'tenant-1',
23+
project_id: 'project-1',
24+
});
25+
});
26+
27+
it('does not inject project_id when not provided', () => {
28+
const payload = {
29+
compositeQuery: { chQueries: { A: { query: validQuery } } },
30+
};
31+
const result = enforceQuerySecurity(payload, 'tenant-1');
32+
expect(result.payload.variables.project_id).toBeUndefined();
33+
});
34+
35+
it('overwrites client-provided tenant_id (anti-spoofing)', () => {
36+
const payload = {
37+
variables: { tenant_id: 'attacker-tenant' },
38+
compositeQuery: { chQueries: { A: { query: validQuery } } },
39+
};
40+
const result = enforceQuerySecurity(payload, 'legitimate-tenant');
41+
expect(result.payload.variables.tenant_id).toBe('legitimate-tenant');
42+
});
43+
44+
it('overwrites client-provided project_id (anti-spoofing)', () => {
45+
const payload = {
46+
variables: { project_id: 'attacker-project' },
47+
compositeQuery: { chQueries: { A: { query: validQuery } } },
48+
};
49+
const result = enforceQuerySecurity(payload, 'tenant-1', 'real-project');
50+
expect(result.payload.variables.project_id).toBe('real-project');
51+
});
52+
53+
it('initializes variables object when missing', () => {
54+
const payload = {
55+
compositeQuery: { chQueries: { A: { query: validQuery } } },
56+
};
57+
const result = enforceQuerySecurity(payload, 'tenant-1', 'project-1');
58+
expect(result.payload.variables).toEqual({
59+
tenant_id: 'tenant-1',
60+
project_id: 'project-1',
61+
});
62+
});
63+
64+
it('rejects query missing {{.tenant_id}} reference', () => {
65+
const payload = {
66+
compositeQuery: {
67+
chQueries: {
68+
malicious: {
69+
query: 'SELECT * FROM signoz_traces.distributed_signoz_index_v3 LIMIT 1000',
70+
},
71+
},
72+
},
73+
};
74+
const result = enforceQuerySecurity(payload, 'tenant-1');
75+
expect(result.error).toBe('Query "malicious" is missing required {{.tenant_id}} tenant filter');
76+
});
77+
78+
it('rejects when any one of multiple queries is missing tenant filter', () => {
79+
const payload = {
80+
compositeQuery: {
81+
chQueries: {
82+
good: { query: validQuery },
83+
bad: { query: 'SELECT count() FROM t' },
84+
},
85+
},
86+
};
87+
const result = enforceQuerySecurity(payload, 'tenant-1');
88+
expect(result.error).toContain('bad');
89+
});
90+
91+
it('passes when all queries reference {{.tenant_id}}', () => {
92+
const payload = {
93+
compositeQuery: {
94+
chQueries: {
95+
q1: { query: validQuery },
96+
q2: {
97+
query: `SELECT count() FROM t WHERE attributes_string['tenant.id'] = {{.tenant_id}}`,
98+
},
99+
},
100+
},
101+
};
102+
const result = enforceQuerySecurity(payload, 'tenant-1');
103+
expect(result.error).toBeUndefined();
104+
});
105+
106+
it('passes when no chQueries present (non-clickhouse payload)', () => {
107+
const payload = { compositeQuery: {} };
108+
const result = enforceQuerySecurity(payload, 'tenant-1');
109+
expect(result.error).toBeUndefined();
110+
expect(result.payload.variables.tenant_id).toBe('tenant-1');
111+
});
112+
113+
it('does not mutate the original payload', () => {
114+
const original = {
115+
variables: { tenant_id: 'original' },
116+
compositeQuery: { chQueries: { A: { query: validQuery } } },
117+
};
118+
enforceQuerySecurity(original, 'new-tenant');
119+
expect(original.variables.tenant_id).toBe('original');
120+
});
121+
});

agents-api/src/domains/manage/routes/signoz.ts

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,12 @@ app.post('/query', async (c) => {
6161
}
6262
}
6363

64-
// Always enforce server-side tenant filter, and project filter if provided
65-
// Automatically detects builder vs clickhouse_sql queries
66-
payload = enforceQuerySecurity(payload, tenantId, requestedProjectId);
64+
const secured = enforceQuerySecurity(payload, tenantId, requestedProjectId);
65+
if (secured.error) {
66+
logger.warn({ tenantId, error: secured.error }, 'Query rejected: missing tenant filter');
67+
return c.json({ error: 'Bad Request', message: secured.error }, 400);
68+
}
69+
payload = secured.payload;
6770
logger.debug({ tenantId, projectId: requestedProjectId }, 'Security filters enforced');
6871

6972
const signozUrl = env.SIGNOZ_URL || env.PUBLIC_SIGNOZ_URL;
@@ -202,9 +205,17 @@ app.post('/query-batch', async (c) => {
202205
'SIGNOZ-API-KEY': signozApiKey,
203206
};
204207

208+
const securedPagination = enforceQuerySecurity(paginationPayload, tenantId, requestedProjectId);
209+
if (securedPagination.error) {
210+
logger.warn(
211+
{ tenantId, error: securedPagination.error },
212+
'Pagination query rejected: missing tenant filter'
213+
);
214+
return c.json({ error: 'Bad Request', message: securedPagination.error }, 400);
215+
}
216+
205217
try {
206-
const securedPagination = enforceQuerySecurity(paginationPayload, tenantId, requestedProjectId);
207-
const step1 = await axios.post(signozEndpoint, securedPagination, {
218+
const step1 = await axios.post(signozEndpoint, securedPagination.payload, {
208219
headers: signozHeaders,
209220
timeout: 30000,
210221
});
@@ -222,7 +233,14 @@ app.post('/query-batch', async (c) => {
222233

223234
const detailWithIds = injectConversationIdFilter(detailPayloadTemplate, conversationIds);
224235
const securedDetail = enforceQuerySecurity(detailWithIds, tenantId, requestedProjectId);
225-
const step2 = await axios.post(signozEndpoint, securedDetail, {
236+
if (securedDetail.error) {
237+
logger.warn(
238+
{ tenantId, error: securedDetail.error },
239+
'Detail query rejected: missing tenant filter'
240+
);
241+
return c.json({ error: 'Bad Request', message: securedDetail.error }, 400);
242+
}
243+
const step2 = await axios.post(signozEndpoint, securedDetail.payload, {
226244
headers: signozHeaders,
227245
timeout: 30000,
228246
});
@@ -267,7 +285,7 @@ function injectConversationIdFilter(payload: any, conversationIds: string[]): an
267285

268286
for (const key of Object.keys(chQueries)) {
269287
if (chQueries[key]?.query) {
270-
chQueries[key].query = chQueries[key].query.replace('__CONVERSATION_IDS__', inClause);
288+
chQueries[key].query = chQueries[key].query.replaceAll('__CONVERSATION_IDS__', inClause);
271289
}
272290
}
273291
return modified;
Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,15 @@
11
/**
2-
* Enforce tenant/project security on ClickHouse SQL queries by injecting
3-
* server-side variables. SQL queries use {{.tenant_id}} / {{.project_id}}
4-
* via SigNoz variable substitution, preventing SQL injection.
2+
* Enforce tenant/project security on ClickHouse SQL queries by:
3+
* 1. Injecting server-side variables (tenant_id, project_id) — always overrides client values
4+
* 2. Validating every chQuery references {{.tenant_id}} to prevent tenant isolation bypass
55
*
6-
* Always overrides client-provided values to prevent spoofing.
6+
* Returns null if valid, or an error message string if a query is missing the tenant filter.
77
*/
8-
export function enforceQuerySecurity(payload: any, tenantId: string, projectId?: string): any {
8+
export function enforceQuerySecurity(
9+
payload: any,
10+
tenantId: string,
11+
projectId?: string
12+
): { payload: any; error?: string } {
913
const modifiedPayload = JSON.parse(JSON.stringify(payload));
1014
if (!modifiedPayload.variables) {
1115
modifiedPayload.variables = {};
@@ -14,5 +18,19 @@ export function enforceQuerySecurity(payload: any, tenantId: string, projectId?:
1418
if (projectId) {
1519
modifiedPayload.variables.project_id = projectId;
1620
}
17-
return modifiedPayload;
21+
22+
const chQueries = modifiedPayload.compositeQuery?.chQueries;
23+
if (chQueries) {
24+
for (const [name, entry] of Object.entries(chQueries)) {
25+
const query = (entry as any)?.query;
26+
if (typeof query === 'string' && !query.includes('{{.tenant_id}}')) {
27+
return {
28+
payload: modifiedPayload,
29+
error: `Query "${name}" is missing required {{.tenant_id}} tenant filter`,
30+
};
31+
}
32+
}
33+
}
34+
35+
return { payload: modifiedPayload };
1836
}

agents-manage-ui/src/app/api/signoz/conversations/[conversationId]/route.ts

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,6 @@ async function signozQuery(
139139
};
140140
});
141141
} catch (e) {
142-
const logger = getLogger('signoz-query');
143142
logger.error({ error: e }, 'SigNoz query error');
144143

145144
if (axios.isAxiosError(e)) {
@@ -177,6 +176,7 @@ function safeJsonParse(str: unknown): Record<string, any> {
177176

178177
function buildConversationPayload(
179178
conversationId: string,
179+
tenantId: string,
180180
start = Date.now() - DEFAULT_LOOKBACK_MS,
181181
end = Date.now()
182182
) {
@@ -186,6 +186,7 @@ function buildConversationPayload(
186186
step: 60,
187187
variables: {
188188
conversation_id: conversationId,
189+
tenant_id: tenantId,
189190
},
190191
compositeQuery: {
191192
queryType: 'clickhouse_sql',
@@ -207,7 +208,8 @@ function buildConversationPayload(
207208
toJSONString(attributes_number) AS attrs_num,
208209
toJSONString(attributes_bool) AS attrs_bool
209210
FROM signoz_traces.distributed_signoz_index_v3
210-
WHERE attributes_string['conversation.id'] = {{.conversation_id}}
211+
WHERE attributes_string['tenant.id'] = {{.tenant_id}}
212+
AND attributes_string['conversation.id'] = {{.conversation_id}}
211213
AND timestamp BETWEEN {{.start_datetime}} AND {{.end_datetime}}
212214
AND ts_bucket_start BETWEEN {{.start_timestamp}} - 1800 AND {{.end_timestamp}}
213215
ORDER BY timestamp ASC
@@ -250,7 +252,7 @@ export async function GET(
250252
const start = startParam ? Number(startParam) : now - DEFAULT_LOOKBACK_MS;
251253
const end = endParam ? Number(endParam) : now;
252254

253-
const payload = buildConversationPayload(conversationId, start, end);
255+
const payload = buildConversationPayload(conversationId, tenantId, start, end);
254256
const allSpans = await signozQuery(payload, tenantId, cookieHeader);
255257

256258
const toolCallSpans = allSpans.filter(
@@ -277,7 +279,7 @@ export async function GET(
277279
const contextFetcherSpans = allSpans.filter(
278280
(s) => getString(s, SPAN_KEYS.NAME) === SPAN_NAMES.CONTEXT_FETCHER
279281
);
280-
const durationSpans = allSpans;
282+
281283
const artifactProcessingSpans = allSpans.filter(
282284
(s) => getString(s, SPAN_KEYS.NAME) === SPAN_NAMES.ARTIFACT_PROCESSING
283285
);
@@ -319,9 +321,9 @@ export async function GET(
319321
if (agentId || agentName) break;
320322
}
321323

322-
// Build parent-span map from durationSpans (already fetched in builder query)
324+
// Build parent-span map from allSpans (already fetched in builder query)
323325
const spanIdToParentSpanId = new Map<string, string | null>();
324-
for (const span of durationSpans) {
326+
for (const span of allSpans) {
325327
const spanId = getString(span, SPAN_KEYS.SPAN_ID, '');
326328
const parentSpanId = getString(span, SPAN_KEYS.PARENT_SPAN_ID, '') || null;
327329
if (spanId) {
@@ -990,7 +992,7 @@ export async function GET(
990992
}
991993

992994
// Pre-parse all timestamps once for better performance
993-
const allSpanTimes = durationSpans.map((s) => new Date(s.timestamp).getTime());
995+
const allSpanTimes = allSpans.map((s) => new Date(s.timestamp).getTime());
994996
const operationStartTime = allSpanTimes.length > 0 ? Math.min(...allSpanTimes) : null;
995997
const operationEndTime = allSpanTimes.length > 0 ? Math.max(...allSpanTimes) : null;
996998

agents-manage-ui/src/app/api/signoz/spans/[spanId]/route.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ export async function GET(req: NextRequest, context: RouteContext<'/api/signoz/s
5252
step: 60,
5353
variables: {
5454
conversation_id: conversationId,
55+
tenant_id: tenantId,
5556
span_id: spanId,
5657
},
5758
compositeQuery: {
@@ -69,7 +70,8 @@ export async function GET(req: NextRequest, context: RouteContext<'/api/signoz/s
6970
toJSONString(attributes_bool) AS attributes_bool_json,
7071
toJSONString(resources_string) AS resources_string_json
7172
FROM signoz_traces.${tableName}
72-
WHERE attributes_string['conversation.id'] = {{.conversation_id}}
73+
WHERE attributes_string['tenant.id'] = {{.tenant_id}}
74+
AND attributes_string['conversation.id'] = {{.conversation_id}}
7375
AND span_id = {{.span_id}}
7476
AND timestamp BETWEEN {{.start_datetime}} AND {{.end_datetime}}
7577
AND ts_bucket_start BETWEEN {{.start_timestamp}} - 1800 AND {{.end_timestamp}}

agents-manage-ui/src/components/traces/timeline/timeline-item.tsx

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -225,8 +225,7 @@ export function TimelineItem({
225225

226226
const { Icon, className, style: iconStyle } = statusIcon(typeForIcon as any, activity.status);
227227
const formattedDateTime = formatDateTime(activity.timestamp, { local: true });
228-
const isoDateTime = new Date(activity.timestamp);
229-
228+
const isoDateTime = new Date(activity.timestamp).toISOString();
230229
// Determine text color based on status
231230
const textColorClass =
232231
activity.status === ACTIVITY_STATUS.ERROR

packages/agents-core/src/client-exports.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,6 @@ export function generateIdFromName(name: string): string {
147147
export { type OrgRole, OrgRoles, type ProjectRole, ProjectRoles } from './auth/authz/types';
148148
export * from './constants/context-breakdown';
149149
export * from './constants/otel-attributes';
150-
export * from './constants/signoz-queries';
151150
export { CredentialStoreType, MCPTransportType } from './types';
152151
export { detectAuthenticationRequired } from './utils/auth-detection';
153152
export { transformToJson } from './validation/extend-schemas';

0 commit comments

Comments
 (0)