Skip to content

Commit 9960f6e

Browse files
liberuumliberuum
authored
Merge pull request #39 from makerdao-ses/fix/sql-injection-hardening
Fix SQL injection and security vulnerabilities
2 parents ad92cfb + 2efbbef commit 9960f6e

8 files changed

Lines changed: 99 additions & 35 deletions

File tree

‎src/index.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { ApolloServerPluginLandingPageLocalDefault } from '@apollo/server/plugin
55

66
import express from "express";
77
import http from "http";
8+
import crypto from "crypto";
89
import cors from "cors";
910

1011
import compression from "compression";
@@ -52,7 +53,14 @@ function buildExpressApp() {
5253
});
5354
});
5455
app.get('/update-cache', async (_req, res) => {
55-
if (_req.headers['refresh-cache'] !== process.env.REFRESH_CACHE_SECRET) {
56+
const headerVal = _req.headers['refresh-cache'];
57+
const secretVal = process.env.REFRESH_CACHE_SECRET;
58+
if (
59+
typeof headerVal !== 'string' ||
60+
!secretVal ||
61+
Buffer.byteLength(headerVal) !== Buffer.byteLength(secretVal) ||
62+
!crypto.timingSafeEqual(Buffer.from(headerVal), Buffer.from(secretVal))
63+
) {
5664
return res.status(401).json({
5765
status: 'error',
5866
message: 'Unauthorized',

‎src/modules/Auth/db.ts‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,11 @@ export class AuthModel {
2929
paramName: string,
3030
paramValue: string | number | boolean,
3131
): Promise<User[]> {
32-
return this.knex("User").where(`${paramName}`, paramValue);
32+
const allowedColumns = ['id', 'username', 'password', 'active'];
33+
if (!allowedColumns.includes(paramName)) {
34+
throw new Error(`Invalid column name: ${paramName}`);
35+
}
36+
return this.knex("User").where(paramName, paramValue);
3337
}
3438

3539
async getResourceId(userId: number) {
@@ -257,8 +261,12 @@ export class AuthModel {
257261
})
258262
.orderBy("User.id", "asc");
259263
if (paramName !== undefined && paramValue !== undefined) {
264+
const allowedColumns = ['id', 'username', 'active'];
265+
if (!allowedColumns.includes(paramName)) {
266+
throw new Error(`Invalid column name: ${paramName}`);
267+
}
260268
return await baseQuery.where(
261-
paramName === "id" ? "User.id" : `${paramName}`,
269+
paramName === "id" ? "User.id" : paramName,
262270
paramValue,
263271
);
264272
}

‎src/modules/BudgetStatement/ResolverCache.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ export class ResolverCache {
9797
.insert({
9898
hash,
9999
expiry: this._knex.raw(
100-
"CURRENT_TIMESTAMP + interval '" + expirationInMinutes + " minutes'",
100+
"CURRENT_TIMESTAMP + make_interval(mins => ?)", [expirationInMinutes],
101101
),
102102
data: JSON.stringify(outputGroups),
103103
})

‎src/modules/BudgetStatement/db.ts‎

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -230,17 +230,18 @@ export class BudgetStatementModel {
230230

231231
// use other join query if lastModified filter is set
232232
if (filter?.filter?.sortByLastModified) {
233+
const sortDir = filter.filter.sortByLastModified.toUpperCase() === 'DESC' ? 'DESC' : 'ASC';
233234
const subquery = this.knex
234235
.select(this.knex.raw("DISTINCT ON ((params->>'budgetStatementId')::integer) params->>'budgetStatementId' as id, created_at"))
235236
.from("ChangeTrackingEvents")
236237
.whereRaw("params->>'budgetStatementId' IS NOT NULL")
237-
.orderByRaw(`(params->>'budgetStatementId')::integer, created_at ${filter?.filter?.sortByLastModified.toUpperCase()}`);
238+
.orderByRaw(`(params->>'budgetStatementId')::integer, created_at ${sortDir}`);
238239

239240
query = this.knex
240241
.select("BudgetStatement.*", "cte.created_at as last_modified")
241242
.from("BudgetStatement")
242243
.leftJoin(subquery.as('cte'), 'BudgetStatement.id', this.knex.raw('cte.id::integer'))
243-
.orderByRaw(`last_modified ${filter?.filter?.sortByLastModified} NULLS LAST, month DESC`);
244+
.orderByRaw(`last_modified ${sortDir} NULLS LAST, month DESC`);
244245
}
245246

246247
if (filter?.limit !== undefined && filter?.offset !== undefined) {
@@ -386,6 +387,13 @@ export class BudgetStatementModel {
386387
.select("*")
387388
.from("BudgetStatementLineItem")
388389
.orderBy("month", "desc");
390+
const allowedLineItemColumns = ['id', 'budgetStatementWalletId', 'month', 'position', 'group', 'budgetCategory', 'forecast', 'actual', 'comments', 'canonicalBudgetCategory', 'headcountExpense', 'budgetCap', 'payment'];
391+
if (paramName !== undefined && !allowedLineItemColumns.includes(paramName)) {
392+
throw new Error(`Invalid column name: ${paramName}`);
393+
}
394+
if (secondParamName !== undefined && !allowedLineItemColumns.includes(secondParamName)) {
395+
throw new Error(`Invalid column name: ${secondParamName}`);
396+
}
389397
if (offset != undefined && limit != undefined) {
390398
return baseQuery.limit(limit).offset(offset);
391399
} else if (
@@ -394,16 +402,16 @@ export class BudgetStatementModel {
394402
secondParamName === undefined &&
395403
secondParamValue === undefined
396404
) {
397-
return baseQuery.where(`${paramName}`, paramValue);
405+
return baseQuery.where(paramName, paramValue);
398406
} else if (
399407
paramName !== undefined &&
400408
paramValue !== undefined &&
401409
secondParamName !== undefined &&
402410
secondParamValue !== undefined
403411
) {
404412
return baseQuery
405-
.where(`${paramName}`, paramValue)
406-
.andWhere(`${secondParamName}`, secondParamValue);
413+
.where(paramName, paramValue)
414+
.andWhere(secondParamName, secondParamValue);
407415
} else {
408416
return baseQuery;
409417
}
@@ -474,6 +482,13 @@ export class BudgetStatementModel {
474482
secondParamName?: string | undefined,
475483
secondParamValue?: string | number | boolean | undefined,
476484
): Promise<BudgetStatementComment[]> {
485+
const allowedCommentColumns = ['id', 'budgetStatementId', 'authorId', 'timestamp', 'comment', 'status'];
486+
if (paramName !== undefined && !allowedCommentColumns.includes(paramName)) {
487+
throw new Error(`Invalid column name: ${paramName}`);
488+
}
489+
if (secondParamName !== undefined && !allowedCommentColumns.includes(secondParamName)) {
490+
throw new Error(`Invalid column name: ${secondParamName}`);
491+
}
477492
const baseQuery = this.knex
478493
.select("*")
479494
.from("BudgetStatementComment")
@@ -484,16 +499,16 @@ export class BudgetStatementModel {
484499
secondParamName === undefined &&
485500
secondParamValue === undefined
486501
) {
487-
return baseQuery.where(`${paramName}`, paramValue);
502+
return baseQuery.where(paramName, paramValue);
488503
} else if (
489504
paramName !== undefined &&
490505
paramValue !== undefined &&
491506
secondParamName !== undefined &&
492507
secondParamValue !== undefined
493508
) {
494509
return baseQuery
495-
.where(`${paramName}`, paramValue)
496-
.andWhere(`${secondParamName}`, secondParamValue);
510+
.where(paramName, paramValue)
511+
.andWhere(secondParamName, secondParamValue);
497512
} else {
498513
return baseQuery;
499514
}

‎src/modules/BudgetStatement/schema/budgetStatement.ts‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -756,7 +756,7 @@ export const resolvers = {
756756
input[0].ownerId,
757757
);
758758
}
759-
if (parseInt(allowed[0].count) > 0 || input[0].ownerId === null) {
759+
if (parseInt(allowed[0].count) > 0) {
760760
if (input.length < 1) {
761761
throw new Error("No input data");
762762
}
@@ -803,15 +803,17 @@ export const resolvers = {
803803
throw new Error("Account disabled. Reach admin for more info.");
804804
}
805805
const cuIdFromInput = input.pop();
806-
let allowed = { count: 0 };
807-
if (cuIdFromInput.ownerType !== "Delegates") {
806+
let allowed: any = { count: 0 };
807+
if (cuIdFromInput.ownerType === "Delegates") {
808+
[allowed] = await dataSources.db.Auth.canUpdate(userObj.id, "Delegates", null);
809+
} else {
808810
[allowed] = await dataSources.db.Auth.canUpdateCoreUnit(
809811
userObj.id,
810812
cuIdFromInput.ownerType,
811813
cuIdFromInput.cuId,
812814
);
813815
}
814-
if (allowed.count > 0 || cuIdFromInput.ownerType === "Delegates") {
816+
if (parseInt(allowed.count) > 0) {
815817
//Tacking Change
816818
let CU;
817819
if (cuIdFromInput.ownerType === "Delegates") {
@@ -961,15 +963,17 @@ export const resolvers = {
961963
throw new Error("Account disabled. Reach admin for more info.");
962964
}
963965
const cuIdFromInput = input.pop();
964-
let allowed = { count: 0 };
965-
if (cuIdFromInput.ownerType !== "Delegates") {
966+
let allowed: any = { count: 0 };
967+
if (cuIdFromInput.ownerType === "Delegates") {
968+
[allowed] = await dataSources.db.Auth.canUpdate(userObj.id, "Delegates", null);
969+
} else {
966970
[allowed] = await dataSources.db.Auth.canUpdateCoreUnit(
967971
userObj.id,
968972
cuIdFromInput.ownerType,
969973
cuIdFromInput.cuId,
970974
);
971975
}
972-
if (allowed.count > 0 || cuIdFromInput.ownerType === "Delegates") {
976+
if (parseInt(allowed.count) > 0) {
973977
//Tacking Change
974978
let CU;
975979
if (cuIdFromInput.ownerType === "Delegates") {

‎src/modules/BudgetStatement/schema/utils.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -128,8 +128,8 @@ export const resolveBudgetPath = async (path: string, knex: Knex) => {
128128
// Escape single quotes in path
129129
// include / at the end to ensure we are matching the full path
130130
path = path.endsWith('/') ? path : `${path}/`;
131-
const escapedPath = path.replace(/'/g, "''");
132-
query = query.where('path', 'like', `${escapedPath}%`);
131+
const escapedPath = path.replace(/[%_\\]/g, '\\$&');
132+
query = query.whereRaw(`"path" LIKE ? ESCAPE '\\'`, [`${escapedPath}%`]);
133133

134134
const result = await query;
135135
return result.map((item: any) => item.budgetStatementId);

‎src/modules/ChangeTracking/db.ts‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -122,10 +122,16 @@ export class ChangeTrackingModel {
122122
}
123123

124124
getBsEvents(bsId: string) {
125+
let parsedBsId: any;
126+
try {
127+
parsedBsId = JSON.parse(bsId);
128+
} catch {
129+
return this.knex.select("*").from("ChangeTrackingEvents").whereRaw("1 = 0");
130+
}
125131
return this.knex
126132
.select("*")
127133
.from("ChangeTrackingEvents")
128-
.whereRaw("params->>? = ?", ["budgetStatementId", JSON.parse(bsId)])
134+
.whereRaw("params->>? = ?", ["budgetStatementId", parsedBsId])
129135
.orderBy("ChangeTrackingEvents.id", "desc");
130136
}
131137

@@ -333,6 +339,13 @@ export class ChangeTrackingModel {
333339
secondParamName: string | undefined,
334340
secondParamValue: string | undefined,
335341
) {
342+
const allowedColumns = ['id', 'userId', 'collection', 'data', 'lastVisit'];
343+
if (paramName !== undefined && !allowedColumns.includes(paramName)) {
344+
throw new Error(`Invalid column name: ${paramName}`);
345+
}
346+
if (secondParamName !== undefined && !allowedColumns.includes(secondParamName)) {
347+
throw new Error(`Invalid column name: ${secondParamName}`);
348+
}
336349
if (
337350
paramName === undefined &&
338351
paramValue === undefined &&
@@ -342,13 +355,13 @@ export class ChangeTrackingModel {
342355
return this.knex("UserActivity").orderBy("id", "desc").limit(1);
343356
} else if (secondParamName == undefined && secondParamValue == undefined) {
344357
return this.knex("UserActivity")
345-
.where(`${paramName}`, paramValue)
358+
.where(paramName!, paramValue)
346359
.orderBy("id", "desc")
347360
.limit(1);
348361
} else {
349362
return this.knex("UserActivity")
350-
.where(`${paramName}`, paramValue)
351-
.andWhere(`${secondParamName}`, secondParamValue)
363+
.where(paramName!, paramValue)
364+
.andWhere(secondParamName!, secondParamValue)
352365
.orderBy("id", "desc")
353366
.limit(1);
354367
}

‎src/modules/Roadmap/db.ts‎

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -94,13 +94,20 @@ export class RoadmapModel {
9494
this.coreUnitModel = coreUnitModel;
9595
}
9696

97+
private validateColumn(paramName: string, allowedColumns: string[]): void {
98+
if (!allowedColumns.includes(paramName)) {
99+
throw new Error(`Invalid column name: ${paramName}`);
100+
}
101+
}
102+
97103
async getRoadmaps(
98104
paramName?: string | undefined,
99105
paramValue?: string | number | boolean | undefined,
100106
): Promise<Roadmap[]> {
101107
const baseQuery = this.knex.select("*").from("Roadmap").orderBy("id");
102108
if (paramName !== undefined && paramValue !== undefined) {
103-
return baseQuery.where(`${paramName}`, paramValue);
109+
this.validateColumn(paramName, ['id', 'ownerCuId', 'roadmapCode', 'roadmapName', 'comments', 'roadmapStatus', 'strategicInitiative', 'roadmapSummary']);
110+
return baseQuery.where(paramName, paramValue);
104111
} else {
105112
return baseQuery;
106113
}
@@ -115,7 +122,8 @@ export class RoadmapModel {
115122
.from("RoadmapStakeholder")
116123
.orderBy("id");
117124
if (paramName !== undefined && paramValue !== undefined) {
118-
return baseQuery.where(`${paramName}`, paramValue);
125+
this.validateColumn(paramName, ['id', 'stakeholderId', 'roadmapId', 'stakeholderRoleId']);
126+
return baseQuery.where(paramName, paramValue);
119127
} else {
120128
return baseQuery;
121129
}
@@ -130,7 +138,8 @@ export class RoadmapModel {
130138
.from("StakeholderRole")
131139
.orderBy("id");
132140
if (paramName !== undefined && paramValue !== undefined) {
133-
return baseQuery.where(`${paramName}`, paramValue);
141+
this.validateColumn(paramName, ['id', 'stakeholderRoleName']);
142+
return baseQuery.where(paramName, paramValue);
134143
} else {
135144
return baseQuery;
136145
}
@@ -142,7 +151,8 @@ export class RoadmapModel {
142151
): Promise<Stakeholder[]> {
143152
const baseQuery = this.knex.select("*").from("Stakeholder").orderBy("id");
144153
if (paramName !== undefined && paramValue !== undefined) {
145-
return baseQuery.where(`${paramName}`, paramValue);
154+
this.validateColumn(paramName, ['id', 'name', 'stakeholderContributorId', 'stakeholderCuCode']);
155+
return baseQuery.where(paramName, paramValue);
146156
} else {
147157
return baseQuery;
148158
}
@@ -154,7 +164,8 @@ export class RoadmapModel {
154164
): Promise<RoadmapOutput[]> {
155165
const baseQuery = this.knex.select("*").from("RoadmapOutput").orderBy("id");
156166
if (paramName !== undefined && paramValue !== undefined) {
157-
return baseQuery.where(`${paramName}`, paramValue);
167+
this.validateColumn(paramName, ['id', 'outputId', 'roadmapId', 'outputTypeId']);
168+
return baseQuery.where(paramName, paramValue);
158169
} else {
159170
return baseQuery;
160171
}
@@ -166,7 +177,8 @@ export class RoadmapModel {
166177
): Promise<Output[]> {
167178
const baseQuery = this.knex.select("*").from("Output").orderBy("id");
168179
if (paramName !== undefined && paramValue !== undefined) {
169-
return baseQuery.where(`${paramName}`, paramValue);
180+
this.validateColumn(paramName, ['id', 'name', 'outputUrl', 'outputDate']);
181+
return baseQuery.where(paramName, paramValue);
170182
} else {
171183
return baseQuery;
172184
}
@@ -178,7 +190,8 @@ export class RoadmapModel {
178190
): Promise<OutputType[]> {
179191
const baseQuery = this.knex.select("*").from("OutputType").orderBy("id");
180192
if (paramName !== undefined && paramValue !== undefined) {
181-
return baseQuery.where(`${paramName}`, paramValue);
193+
this.validateColumn(paramName, ['id', 'outputType']);
194+
return baseQuery.where(paramName, paramValue);
182195
} else {
183196
return baseQuery;
184197
}
@@ -190,7 +203,8 @@ export class RoadmapModel {
190203
): Promise<Milestone[]> {
191204
const baseQuery = this.knex.select("*").from("Milestone").orderBy("id");
192205
if (paramName !== undefined && paramValue !== undefined) {
193-
return baseQuery.where(`${paramName}`, paramValue);
206+
this.validateColumn(paramName, ['id', 'roadmapId', 'taskId']);
207+
return baseQuery.where(paramName, paramValue);
194208
} else {
195209
return baseQuery;
196210
}
@@ -202,7 +216,8 @@ export class RoadmapModel {
202216
): Promise<Task[]> {
203217
const baseQuery = this.knex.select("*").from("Task").orderBy("id");
204218
if (paramName !== undefined && paramValue !== undefined) {
205-
return baseQuery.where(`${paramName}`, paramValue);
219+
this.validateColumn(paramName, ['id', 'parentId', 'taskName', 'taskStatus', 'ownerStakeholderId', 'startDate', 'target', 'completedPercentage', 'confidenceLevel', 'comments']);
220+
return baseQuery.where(paramName, paramValue);
206221
} else {
207222
return baseQuery;
208223
}
@@ -214,7 +229,8 @@ export class RoadmapModel {
214229
): Promise<Review[]> {
215230
const baseQuery = this.knex.select("*").from("Review").orderBy("id");
216231
if (paramName !== undefined && paramValue !== undefined) {
217-
return baseQuery.where(`${paramName}`, paramValue);
232+
this.validateColumn(paramName, ['id', 'taskId', 'reviewDate', 'reviewOutcome']);
233+
return baseQuery.where(paramName, paramValue);
218234
} else {
219235
return baseQuery;
220236
}

0 commit comments

Comments
 (0)