Skip to content

Commit 4c20bb7

Browse files
security: fix HIGH vulnerabilities (H1, H2, H3, H6) (#5599)
H1: AlertStore SQL injection — sanitize grouping property names (escape quotes), validate grouping column names against whitelist, and validate threshold/minimum_request_count as numbers. H2: SessionManager SQL injection — validate and clamp limit/offset as integers before interpolation into ClickHouse queries. H3: Feedback API SQL injection — convert org_id from string interpolation to parameterized query ($1). H6: HqlStore path traversal — validate fileName with path.basename() check and reject paths containing '..'.
1 parent 2fde522 commit 4c20bb7

File tree

4 files changed

+42
-11
lines changed

4 files changed

+42
-11
lines changed

valhalla/jawn/src/lib/stores/HqlStore.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { createArrayCsvWriter } from "csv-writer";
22
import { err, ok, Result } from "../../packages/common/result";
33
import { S3Client } from "../shared/db/s3Client";
44
import fs from "fs";
5+
import path from "path";
56

67
const HQL_STORE_BUCKET = process.env.HQL_STORE_BUCKET || "hql-store";
78

@@ -24,7 +25,12 @@ export class HqlStore {
2425
rows: Record<string, any>[]
2526
): Promise<Result<string, string>> {
2627
// if result is ok, if over 100k store in s3 and return url
27-
const key = `${organizationId}/${fileName}`;
28+
// Validate fileName to prevent path traversal
29+
const baseName = path.basename(fileName);
30+
if (baseName !== fileName || fileName.includes("..")) {
31+
return err("Invalid file name");
32+
}
33+
const key = `${organizationId}/${baseName}`;
2834
// Determine column order from keys of first row
2935
if (rows.length === 0) {
3036
return err("No data to export");

valhalla/jawn/src/managers/SessionManager.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -344,14 +344,18 @@ export class SessionManager {
344344
): Promise<Result<SessionResult[], string>> {
345345
const {
346346
timezoneDifference,
347-
offset = 0,
348-
limit = 50,
347+
offset: rawOffset = 0,
348+
limit: rawLimit = 50,
349349
} = requestBody;
350350

351351
if (!isValidTimeZoneDifference(timezoneDifference)) {
352352
return err("Invalid timezone difference");
353353
}
354354

355+
// Validate and clamp numeric values to prevent SQL injection
356+
const limit = Math.max(0, Math.min(Math.floor(Number(rawLimit) || 50), 1000));
357+
const offset = Math.max(0, Math.floor(Number(rawOffset) || 0));
358+
355359
const { builtFilter, havingFilter } = await this.buildSessionFilters(requestBody);
356360

357361
// Step 1 get all the properties given this filter

web/pages/api/feedback/index.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,11 @@ async function getFeedbackMetrics(
1717
SELECT f.name, f.data_type
1818
FROM feedback_metrics f
1919
JOIN helicone_api_keys h ON f.helicone_api_key_id = h.id
20-
WHERE h.organization_id = '${org_id}'
20+
WHERE h.organization_id = $1
2121
LIMIT 1000;
2222
`;
2323

24-
const { data, error } = await dbExecute<FeedbackMetric>(query, []);
24+
const { data, error } = await dbExecute<FeedbackMetric>(query, [org_id]);
2525
if (error !== null) {
2626
return { data: null, error: error };
2727
}

worker/src/lib/db/AlertStore.ts

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -53,16 +53,25 @@ export class AlertStore {
5353
private clickhouseClient: ClickhouseClientWrapper
5454
) {}
5555

56+
private static readonly SAFE_IDENTIFIER = /^[a-zA-Z0-9_]+$/;
57+
5658
private buildGroupByColumn(alert: Alert): string {
5759
if (!alert.grouping) return "";
5860

5961
if (alert.grouping_is_property) {
60-
return `properties['${alert.grouping}']`;
62+
// Sanitize property name: escape single quotes to prevent injection
63+
const sanitized = alert.grouping.replace(/'/g, "\\'");
64+
return `properties['${sanitized}']`;
6165
}
6266

63-
return (
64-
AlertStore.STANDARD_GROUPING_MAP[alert.grouping] || alert.grouping
65-
);
67+
const mapped = AlertStore.STANDARD_GROUPING_MAP[alert.grouping];
68+
if (!mapped) {
69+
// Reject unknown grouping values that aren't in the whitelist
70+
if (!AlertStore.SAFE_IDENTIFIER.test(alert.grouping)) {
71+
throw new Error(`Invalid grouping column: ${alert.grouping}`);
72+
}
73+
}
74+
return mapped || alert.grouping;
6675
}
6776

6877
private async applyAlertFilter(
@@ -114,11 +123,23 @@ export class AlertStore {
114123
): string {
115124
if (!alert.grouping) return query;
116125

126+
// Validate numeric values to prevent injection
127+
const threshold = Number(alert.threshold);
128+
if (isNaN(threshold)) {
129+
throw new Error(`Invalid threshold value: ${alert.threshold}`);
130+
}
131+
117132
query += ` GROUP BY groupValue`;
118-
query += ` HAVING ${thresholdColumn} >= ${alert.threshold}`;
133+
query += ` HAVING ${thresholdColumn} >= ${threshold}`;
119134

120135
if (alert.minimum_request_count) {
121-
query += ` AND requestCount >= ${alert.minimum_request_count}`;
136+
const minCount = Number(alert.minimum_request_count);
137+
if (isNaN(minCount)) {
138+
throw new Error(
139+
`Invalid minimum_request_count: ${alert.minimum_request_count}`
140+
);
141+
}
142+
query += ` AND requestCount >= ${minCount}`;
122143
}
123144

124145
return query;

0 commit comments

Comments
 (0)