Skip to content

Commit 3351391

Browse files
security: fix remaining HIGH findings (H2, H8, H10) (#5600)
H2 (remaining): Validate/clamp LIMIT and OFFSET as safe integers in adminController, MetricsManager, and HeliconeDatasetManager. H8: Remove query parameters from error log statements in dbExecute, ClickhouseWrapper (jawn + worker), and valhalla. Prevents PII/secrets from leaking into logs. H10: Replace blanket /v1/public/* auth bypass with explicit whitelist of known public route prefixes. Prevents accidental exposure of new endpoints added under /v1/public/.
1 parent 8b2c14c commit 3351391

File tree

8 files changed

+27
-10
lines changed

8 files changed

+27
-10
lines changed

valhalla/jawn/src/controllers/private/adminController.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -208,8 +208,8 @@ export class AdminController extends Controller {
208208
}> {
209209
await authCheckThrow(request.authParams.userId);
210210

211-
const limit = body.limit ?? 10;
212-
const minRequests = body.minRequests ?? 1_000_000;
211+
const limit = Math.max(1, Math.min(Math.floor(Number(body.limit) || 10), 1000));
212+
const minRequests = Math.max(0, Math.floor(Number(body.minRequests) || 1_000_000));
213213

214214
// Fetch top organizations by usage in the past month
215215
const topOrgsQuery = `

valhalla/jawn/src/lib/db/ClickhouseWrapper.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ export class ClickhouseClientWrapper {
101101
}
102102
return { data: validated.data, error: null };
103103
} catch (err) {
104-
console.error("Error executing Clickhouse query: ", query, parameters);
104+
console.error("Error executing Clickhouse query: ", query);
105105
console.error(err);
106106
// Extract error message properly - Error objects don't stringify well
107107
const errorMessage = err instanceof Error ? err.message : String(err);

valhalla/jawn/src/lib/db/valhalla.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ class ValhallaDB implements IValhallaDB {
109109
try {
110110
client = await this.pool.connect();
111111
} catch (thrownErr) {
112-
console.error("Error in query", query, thrownErr);
112+
console.error("Error in query", query);
113113
return err(JSON.stringify(thrownErr));
114114
}
115115

@@ -118,7 +118,7 @@ class ValhallaDB implements IValhallaDB {
118118
client.release();
119119
return ok(result);
120120
} catch (thrownErr) {
121-
console.error("Error in query", query, thrownErr);
121+
console.error("Error in query", query);
122122
client.release();
123123
return err(JSON.stringify(thrownErr));
124124
}

valhalla/jawn/src/lib/shared/db/dbExecute.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ export async function dbExecute<T>(
8585

8686
return { data: result, error: null };
8787
} catch (err) {
88-
console.error("Error executing query: ", query, parameters);
88+
console.error("Error executing query: ", query);
8989
console.error(err);
9090
return { data: null, error: JSON.stringify(err) };
9191
}

valhalla/jawn/src/managers/MetricsManager.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -723,6 +723,8 @@ export class MetricsManager extends BaseManager {
723723
if (isNaN(offset) || isNaN(limit)) {
724724
return { data: null, error: "Invalid offset or limit" };
725725
}
726+
offset = Math.max(0, Math.floor(offset));
727+
limit = Math.max(1, Math.min(Math.floor(limit), 10000));
726728

727729
const builtFilter = await buildFilterWithAuthClickHouse({
728730
org_id: this.authParams.organizationId,
@@ -790,6 +792,8 @@ export class MetricsManager extends BaseManager {
790792
if (isNaN(offset) || isNaN(limit)) {
791793
return { data: null, error: "Invalid offset or limit" };
792794
}
795+
offset = Math.max(0, Math.floor(offset));
796+
limit = Math.max(1, Math.min(Math.floor(limit), 10000));
793797

794798
const builtFilter = await buildFilterWithAuthClickHouse({
795799
org_id: this.authParams.organizationId,

valhalla/jawn/src/managers/dataset/HeliconeDatasetManager.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,8 @@ export class HeliconeDatasetManager extends BaseManager {
108108
}
109109

110110
async query(datasetId: string, params: { offset: number; limit: number }) {
111+
const limit = Math.max(1, Math.min(Math.floor(Number(params.limit) || 50), 10000));
112+
const offset = Math.max(0, Math.floor(Number(params.offset) || 0));
111113
const query = `
112114
SELECT
113115
hdr.id,
@@ -120,8 +122,8 @@ export class HeliconeDatasetManager extends BaseManager {
120122
AND hdr.organization_id = $2
121123
AND hd.deleted_at IS NULL
122124
ORDER BY hdr.created_at DESC
123-
LIMIT ${params.limit}
124-
OFFSET ${params.offset}
125+
LIMIT ${limit}
126+
OFFSET ${offset}
125127
`;
126128

127129
const result = await dbExecute<HeliconeDatasetRow>(query, [

valhalla/jawn/src/middleware/auth.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,18 @@ export const authMiddleware = async (
6868
res: Response,
6969
next: NextFunction
7070
) => {
71-
if (req.path.startsWith("/v1/public")) {
71+
// Public routes — explicitly whitelisted to prevent accidental exposure
72+
const PUBLIC_ROUTE_PREFIXES = [
73+
"/v1/public/model-registry",
74+
"/v1/public/stats",
75+
"/v1/public/security",
76+
"/v1/public/alert-banner",
77+
"/v1/public/status/provider",
78+
"/v1/public/pi",
79+
"/v1/public/compare",
80+
"/v1/public/waitlist",
81+
];
82+
if (PUBLIC_ROUTE_PREFIXES.some((prefix) => req.path.startsWith(prefix))) {
7283
next();
7384
return;
7485
}

worker/src/lib/db/ClickhouseWrapper.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ export class ClickhouseClientWrapper {
9494
});
9595
return { data: await queryResult.json<T[]>(), error: null };
9696
} catch (err) {
97-
console.error("Error executing query: ", query, parameters);
97+
console.error("Error executing query: ", query);
9898
console.error(err);
9999
return {
100100
data: null,

0 commit comments

Comments
 (0)