Skip to content

Commit 74cb4d4

Browse files
H2ShamiH2Shami
committed
fix ddl security issue for property delete
1 parent 6763e6f commit 74cb4d4

File tree

6 files changed

+244
-140
lines changed

6 files changed

+244
-140
lines changed

‎bifrost/lib/clients/jawnTypes/public.ts‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2805,11 +2805,6 @@ Json: JsonObject;
28052805
error: null;
28062806
};
28072807
"Result_Property-Array.string_": components["schemas"]["ResultSuccess_Property-Array_"] | components["schemas"]["ResultError_string_"];
2808-
"ResultSuccess_unknown-Array_": {
2809-
data: unknown[];
2810-
/** @enum {number|null} */
2811-
error: null;
2812-
};
28132808
"ResultSuccess__value-string--cost-number_-Array_": {
28142809
data: {
28152810
/** Format: double */
@@ -6715,7 +6710,13 @@ export interface operations {
67156710
/** @description Ok */
67166711
200: {
67176712
content: {
6718-
"application/json": components["schemas"]["Result_Property-Array.string_"];
6713+
"application/json": components["schemas"]["ResultError_string_"] | components["schemas"]["ResultSuccess_Property-Array_"] | {
6714+
error: string;
6715+
data: unknown;
6716+
} | {
6717+
error: unknown;
6718+
data: components["schemas"]["Property"][];
6719+
};
67196720
};
67206721
};
67216722
};
@@ -6732,7 +6733,10 @@ export interface operations {
67326733
/** @description Ok */
67336734
200: {
67346735
content: {
6735-
"application/json": components["schemas"]["ResultError_string_"] | components["schemas"]["ResultSuccess_unknown-Array_"] | components["schemas"]["ResultSuccess_string_"] | {
6736+
"application/json": {
6737+
error: string;
6738+
data: unknown;
6739+
} | {
67366740
error: unknown;
67376741
data: {
67386742
ok: boolean;
@@ -6764,7 +6768,10 @@ export interface operations {
67646768
/** @description Ok */
67656769
200: {
67666770
content: {
6767-
"application/json": components["schemas"]["ResultError_string_"] | components["schemas"]["ResultSuccess_unknown-Array_"] | components["schemas"]["ResultSuccess_string_"] | {
6771+
"application/json": {
6772+
error: string;
6773+
data: unknown;
6774+
} | {
67686775
error: unknown;
67696776
data: {
67706777
ok: boolean;

‎docs/swagger.json‎

Lines changed: 59 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -7931,27 +7931,6 @@
79317931
}
79327932
]
79337933
},
7934-
"ResultSuccess_unknown-Array_": {
7935-
"properties": {
7936-
"data": {
7937-
"items": {},
7938-
"type": "array"
7939-
},
7940-
"error": {
7941-
"type": "number",
7942-
"enum": [
7943-
null
7944-
],
7945-
"nullable": true
7946-
}
7947-
},
7948-
"required": [
7949-
"data",
7950-
"error"
7951-
],
7952-
"type": "object",
7953-
"additionalProperties": false
7954-
},
79557934
"ResultSuccess__value-string--cost-number_-Array_": {
79567935
"properties": {
79577936
"data": {
@@ -19265,7 +19244,43 @@
1926519244
"content": {
1926619245
"application/json": {
1926719246
"schema": {
19268-
"$ref": "#/components/schemas/Result_Property-Array.string_"
19247+
"anyOf": [
19248+
{
19249+
"$ref": "#/components/schemas/ResultError_string_"
19250+
},
19251+
{
19252+
"$ref": "#/components/schemas/ResultSuccess_Property-Array_"
19253+
},
19254+
{
19255+
"properties": {
19256+
"error": {
19257+
"type": "string"
19258+
},
19259+
"data": {}
19260+
},
19261+
"required": [
19262+
"error",
19263+
"data"
19264+
],
19265+
"type": "object"
19266+
},
19267+
{
19268+
"properties": {
19269+
"error": {},
19270+
"data": {
19271+
"items": {
19272+
"$ref": "#/components/schemas/Property"
19273+
},
19274+
"type": "array"
19275+
}
19276+
},
19277+
"required": [
19278+
"error",
19279+
"data"
19280+
],
19281+
"type": "object"
19282+
}
19283+
]
1926919284
}
1927019285
}
1927119286
}
@@ -19304,13 +19319,17 @@
1930419319
"schema": {
1930519320
"anyOf": [
1930619321
{
19307-
"$ref": "#/components/schemas/ResultError_string_"
19308-
},
19309-
{
19310-
"$ref": "#/components/schemas/ResultSuccess_unknown-Array_"
19311-
},
19312-
{
19313-
"$ref": "#/components/schemas/ResultSuccess_string_"
19322+
"properties": {
19323+
"error": {
19324+
"type": "string"
19325+
},
19326+
"data": {}
19327+
},
19328+
"required": [
19329+
"error",
19330+
"data"
19331+
],
19332+
"type": "object"
1931419333
},
1931519334
{
1931619335
"properties": {
@@ -19405,13 +19424,17 @@
1940519424
"schema": {
1940619425
"anyOf": [
1940719426
{
19408-
"$ref": "#/components/schemas/ResultError_string_"
19409-
},
19410-
{
19411-
"$ref": "#/components/schemas/ResultSuccess_unknown-Array_"
19412-
},
19413-
{
19414-
"$ref": "#/components/schemas/ResultSuccess_string_"
19427+
"properties": {
19428+
"error": {
19429+
"type": "string"
19430+
},
19431+
"data": {}
19432+
},
19433+
"required": [
19434+
"error",
19435+
"data"
19436+
],
19437+
"type": "object"
1941519438
},
1941619439
{
1941719440
"properties": {

‎valhalla/jawn/src/controllers/public/propertyController.ts‎

Lines changed: 96 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,7 @@ import {
88
Security,
99
Tags,
1010
} from "tsoa";
11-
import { dbQueryClickhouse } from "../../lib/shared/db/dbExecute";
12-
import { clickhouseDb } from "../../lib/db/ClickhouseWrapper";
11+
import { dbExecute, dbQueryClickhouse } from "../../lib/shared/db/dbExecute";
1312
import {
1413
buildFilterWithAuthClickHouse,
1514
buildFilterWithAuthClickHouseOrganizationProperties,
@@ -48,24 +47,53 @@ export class PropertyController extends Controller {
4847
filter: {},
4948
});
5049

50+
// Fetch all org properties from ClickHouse, then filter using Postgres hidden set
5151
const query = `
5252
SELECT DISTINCT organization_properties.property_key AS property
5353
FROM organization_properties
54-
LEFT JOIN default.hidden_property_keys AS hp
55-
ON hp.organization_id = organization_properties.organization_id
56-
AND hp.key = organization_properties.property_key
5754
WHERE (
5855
${builtFilter.filter}
59-
AND (hp.is_hidden = 0 OR hp.is_hidden IS NULL)
6056
)
6157
`;
6258

63-
const properties = await dbQueryClickhouse<Property>(
59+
const propertiesRes = await dbQueryClickhouse<Property>(
6460
query,
6561
builtFilter.argsAcc,
6662
);
63+
if (propertiesRes.error) {
64+
return propertiesRes;
65+
}
66+
67+
// Ensure backing table exists and get hidden keys from Postgres
68+
await dbExecute(
69+
`
70+
CREATE TABLE IF NOT EXISTS helicone_hidden_property_keys (
71+
organization_id UUID NOT NULL,
72+
key TEXT NOT NULL,
73+
is_hidden BOOLEAN NOT NULL DEFAULT TRUE,
74+
updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW(),
75+
PRIMARY KEY (organization_id, key)
76+
)
77+
`,
78+
[],
79+
);
6780

68-
return properties;
81+
const hiddenRes = await dbExecute<{ property: string }>(
82+
`SELECT key as property
83+
FROM helicone_hidden_property_keys
84+
WHERE organization_id = $1 AND is_hidden = TRUE`,
85+
[request.authParams.organizationId],
86+
);
87+
if (hiddenRes.error) {
88+
return { data: null, error: hiddenRes.error };
89+
}
90+
91+
const hiddenSet = new Set((hiddenRes.data ?? []).map((r) => r.property));
92+
const filtered = (propertiesRes.data ?? []).filter(
93+
(p) => !hiddenSet.has(p.property),
94+
);
95+
96+
return { data: filtered, error: null };
6997
}
7098

7199
@Post("hide")
@@ -81,22 +109,29 @@ export class PropertyController extends Controller {
81109
throw new Error("Property key is required");
82110
}
83111

84-
// Ensure any existing entry is removed, then insert hidden flag
85-
const deleteQuery = `
86-
ALTER TABLE default.hidden_property_keys
87-
DELETE WHERE organization_id = {val_0: UUID} AND key = {val_1: String}
88-
`;
89-
const delRes = await dbQueryClickhouse(deleteQuery, [orgId, key]);
90-
if (delRes.error) {
91-
return delRes;
92-
}
112+
// Use Postgres upsert to track hidden state (avoids ClickHouse DDL/DML)
113+
await dbExecute(
114+
`
115+
CREATE TABLE IF NOT EXISTS helicone_hidden_property_keys (
116+
organization_id UUID NOT NULL,
117+
key TEXT NOT NULL,
118+
is_hidden BOOLEAN NOT NULL DEFAULT TRUE,
119+
updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW(),
120+
PRIMARY KEY (organization_id, key)
121+
)
122+
`,
123+
[],
124+
);
93125

94-
const insRes = await clickhouseDb.dbInsertClickhouse(
95-
"hidden_property_keys",
96-
[{ organization_id: orgId, key, is_hidden: 1 }],
126+
const upsert = await dbExecute(
127+
`INSERT INTO helicone_hidden_property_keys (organization_id, key, is_hidden)
128+
VALUES ($1, $2, TRUE)
129+
ON CONFLICT (organization_id, key)
130+
DO UPDATE SET is_hidden = EXCLUDED.is_hidden, updated_at = NOW()`,
131+
[orgId, key],
97132
);
98-
if (insRes.error) {
99-
return insRes;
133+
if (upsert.error) {
134+
return { data: null, error: upsert.error };
100135
}
101136

102137
return { data: { ok: true }, error: null };
@@ -108,15 +143,26 @@ export class PropertyController extends Controller {
108143
) {
109144
const orgId = request.authParams.organizationId;
110145

111-
const query = `
112-
SELECT key AS property
113-
FROM default.hidden_property_keys
114-
WHERE organization_id = {val_0: UUID}
115-
AND is_hidden = 1
116-
ORDER BY key
117-
`;
146+
await dbExecute(
147+
`
148+
CREATE TABLE IF NOT EXISTS helicone_hidden_property_keys (
149+
organization_id UUID NOT NULL,
150+
key TEXT NOT NULL,
151+
is_hidden BOOLEAN NOT NULL DEFAULT TRUE,
152+
updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW(),
153+
PRIMARY KEY (organization_id, key)
154+
)
155+
`,
156+
[],
157+
);
118158

119-
return dbQueryClickhouse<Property>(query, [orgId]);
159+
return dbExecute<Property>(
160+
`SELECT key AS property
161+
FROM helicone_hidden_property_keys
162+
WHERE organization_id = $1 AND is_hidden = TRUE
163+
ORDER BY key`,
164+
[orgId],
165+
);
120166
}
121167

122168
@Post("restore")
@@ -132,21 +178,28 @@ export class PropertyController extends Controller {
132178
throw new Error("Property key is required");
133179
}
134180

135-
const deleteQuery = `
136-
ALTER TABLE default.hidden_property_keys
137-
DELETE WHERE organization_id = {val_0: UUID} AND key = {val_1: String}
138-
`;
139-
const delRes = await dbQueryClickhouse(deleteQuery, [orgId, key]);
140-
if (delRes.error) {
141-
return delRes;
142-
}
181+
await dbExecute(
182+
`
183+
CREATE TABLE IF NOT EXISTS helicone_hidden_property_keys (
184+
organization_id UUID NOT NULL,
185+
key TEXT NOT NULL,
186+
is_hidden BOOLEAN NOT NULL DEFAULT TRUE,
187+
updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW(),
188+
PRIMARY KEY (organization_id, key)
189+
)
190+
`,
191+
[],
192+
);
143193

144-
const insRes = await clickhouseDb.dbInsertClickhouse(
145-
"hidden_property_keys",
146-
[{ organization_id: orgId, key, is_hidden: 0 }],
194+
const upsert = await dbExecute(
195+
`INSERT INTO helicone_hidden_property_keys (organization_id, key, is_hidden)
196+
VALUES ($1, $2, FALSE)
197+
ON CONFLICT (organization_id, key)
198+
DO UPDATE SET is_hidden = EXCLUDED.is_hidden, updated_at = NOW()`,
199+
[orgId, key],
147200
);
148-
if (insRes.error) {
149-
return insRes;
201+
if (upsert.error) {
202+
return { data: null, error: upsert.error };
150203
}
151204

152205
return { data: { ok: true }, error: null };

‎valhalla/jawn/src/tsoa-build/public/routes.ts‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2528,15 +2528,6 @@ const models: TsoaRoute.Models = {
25282528
"type": {"dataType":"union","subSchemas":[{"ref":"ResultSuccess_Property-Array_"},{"ref":"ResultError_string_"}],"validators":{}},
25292529
},
25302530
// WARNING: This file was auto-generated with tsoa. Please do not modify it. Re-run tsoa to re-generate this file: https://github.com/lukeautry/tsoa
2531-
"ResultSuccess_unknown-Array_": {
2532-
"dataType": "refObject",
2533-
"properties": {
2534-
"data": {"dataType":"array","array":{"dataType":"any"},"required":true},
2535-
"error": {"dataType":"enum","enums":[null],"required":true},
2536-
},
2537-
"additionalProperties": false,
2538-
},
2539-
// WARNING: This file was auto-generated with tsoa. Please do not modify it. Re-run tsoa to re-generate this file: https://github.com/lukeautry/tsoa
25402531
"ResultSuccess__value-string--cost-number_-Array_": {
25412532
"dataType": "refObject",
25422533
"properties": {

0 commit comments

Comments
 (0)