Skip to content

Commit dbb5405

Browse files
igalklebanovigalklebanov
authored
feat: further harden JSON path .key(...) and .at(...) against SQL injections and exfiltrations. (#1804)
1 parent 73192e4 commit dbb5405

7 files changed

Lines changed: 381 additions & 177 deletions

File tree

‎src/dialect/mysql/mysql-query-compiler.ts‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
import type { CreateIndexNode } from '../../operation-node/create-index-node.js'
22
import { DefaultQueryCompiler } from '../../query-compiler/default-query-compiler.js'
33

4-
const LITERAL_ESCAPE_REGEX = /\\|'/g
4+
const LITERAL_ESCAPE_REGEX = /[\\']/g
55
const ID_WRAP_REGEX = /`/g
6+
const JSON_PATH_MEMBER_ESCAPE_REGEX = /[\\'"]/g
67

78
export class MysqlQueryCompiler extends DefaultQueryCompiler {
89
protected override getCurrentParameterPlaceholder(): string {
@@ -50,6 +51,17 @@ export class MysqlQueryCompiler extends DefaultQueryCompiler {
5051
)
5152
}
5253

54+
/**
55+
* Member values appear inside `"..."` in the JSON path, which itself sits
56+
* inside a SQL string literal. They must therefore be escaped twice — once
57+
* for the JSON path grammar, then again for MySQL's string literal parser.
58+
*/
59+
protected override sanitizeJSONPathMemberValue(value: string): string {
60+
return value.replace(JSON_PATH_MEMBER_ESCAPE_REGEX, (char) =>
61+
char === '\\' ? '\\\\\\\\' : char === "'" ? "''" : '\\\\"',
62+
)
63+
}
64+
5365
protected override visitCreateIndex(node: CreateIndexNode): void {
5466
this.append('create ')
5567

‎src/dialect/sqlite/sqlite-query-compiler.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import type { OrActionNode } from '../../operation-node/or-action-node.js'
33
import { DefaultQueryCompiler } from '../../query-compiler/default-query-compiler.js'
44

55
const ID_WRAP_REGEX = /"/g
6+
const JSON_PATH_MEMBER_ESCAPE_REGEX = /[\\'"]/g
67

78
export class SqliteQueryCompiler extends DefaultQueryCompiler {
89
protected override visitOrAction(node: OrActionNode): void {
@@ -38,6 +39,12 @@ export class SqliteQueryCompiler extends DefaultQueryCompiler {
3839
return identifier.replace(ID_WRAP_REGEX, '""')
3940
}
4041

42+
protected override sanitizeJSONPathMemberValue(value: string): string {
43+
return value.replace(JSON_PATH_MEMBER_ESCAPE_REGEX, (char) =>
44+
char === '\\' ? '\\\\' : char === "'" ? "''" : '\\"',
45+
)
46+
}
47+
4148
protected override visitDefaultInsertValue(_: DefaultInsertValueNode): void {
4249
// sqlite doesn't support the `default` keyword in inserts.
4350
this.append('null')

‎src/query-builder/json-path-builder.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ import { isOperationNodeSource } from '../operation-node/operation-node-source.j
1616
import type { OperationNode } from '../operation-node/operation-node.js'
1717
import { ValueNode } from '../operation-node/value-node.js'
1818

19+
const HASH_NEGATIVE_INDEX_REGEX = /^#-\d+$/
20+
1921
export class JSONPathBuilder<S, O = S> {
2022
readonly #node: JSONReferenceNode | JSONPathNode
2123

@@ -96,6 +98,16 @@ export class JSONPathBuilder<S, O = S> {
9698
>(
9799
index: `${I}` extends `${any}.${any}` | `#--${any}` ? never : I,
98100
): TraversedJSONPathBuilder<S, O2> {
101+
if (
102+
(typeof index !== 'number' && typeof index !== 'string') ||
103+
(typeof index === 'number' && !Number.isInteger(index)) ||
104+
(typeof index === 'string' &&
105+
index !== 'last' &&
106+
!HASH_NEGATIVE_INDEX_REGEX.test(index))
107+
) {
108+
throw new Error(`Unexpected index value in .at(...): ${index}`)
109+
}
110+
99111
return this.#createBuilderWithPathLeg('ArrayLocation', index)
100112
}
101113

‎src/query-compiler/default-query-compiler.ts‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,7 @@ import type { QueryId } from '../util/query-id.js'
119119
import type { RenameConstraintNode } from '../operation-node/rename-constraint-node.js'
120120

121121
const LIT_WRAP_REGEX = /'/g
122+
const JSON_PATH_MEMBER_WRAP_REGEX = /['"]/g
122123

123124
export class DefaultQueryCompiler
124125
extends OperationNodeVisitor
@@ -1625,16 +1626,16 @@ export class DefaultQueryCompiler
16251626
protected override visitJSONPathLeg(node: JSONPathLegNode): void {
16261627
const isArrayLocation = node.type === 'ArrayLocation'
16271628

1628-
this.append(isArrayLocation ? '[' : '.')
1629-
1630-
this.append(
1631-
typeof node.value === 'string'
1632-
? this.sanitizeStringLiteral(node.value)
1633-
: String(node.value),
1634-
)
1629+
const value = String(node.value)
16351630

16361631
if (isArrayLocation) {
1632+
this.append('[')
1633+
this.append(this.sanitizeStringLiteral(value))
16371634
this.append(']')
1635+
} else {
1636+
this.append('."')
1637+
this.append(this.sanitizeJSONPathMemberValue(value))
1638+
this.append('"')
16381639
}
16391640
}
16401641

@@ -1822,6 +1823,12 @@ export class DefaultQueryCompiler
18221823
return value.replace(LIT_WRAP_REGEX, "''")
18231824
}
18241825

1826+
protected sanitizeJSONPathMemberValue(value: string): string {
1827+
return value.replace(JSON_PATH_MEMBER_WRAP_REGEX, (char) =>
1828+
char === "'" ? "''" : '\\"',
1829+
)
1830+
}
1831+
18251832
protected addParameter(parameter: unknown): void {
18261833
this.#parameters.push(parameter)
18271834
}

‎test/node/src/json-traversal.test.ts‎

Lines changed: 20 additions & 150 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,18 @@
11
import {
2-
ColumnDefinitionBuilder,
3-
JSONColumnType,
4-
ParseJSONResultsPlugin,
5-
SqlBool,
6-
sql,
7-
} from '../../../dist/cjs/index.js'
8-
import {
9-
BuiltInDialect,
102
DIALECTS,
3+
type JSONTestContext,
114
NOT_SUPPORTED,
12-
clearDatabase,
13-
destroyTest,
5+
clearJSONDatabase,
6+
destroyJSONTest,
147
expect,
15-
initTest,
16-
insertDefaultDataSet,
8+
initJSONTest,
9+
insertDefaultJSONDataSet,
1710
testSql,
1811
} from './test-setup.js'
1912

20-
type TestContext = Awaited<ReturnType<typeof initJSONTest>>
21-
2213
for (const dialect of DIALECTS.filter((dialect) => dialect !== 'mssql')) {
2314
describe(`${dialect}: json traversal`, () => {
24-
let ctx: TestContext
15+
let ctx: JSONTestContext
2516

2617
before(async function () {
2718
ctx = await initJSONTest(this, dialect)
@@ -54,12 +45,12 @@ for (const dialect of DIALECTS.filter((dialect) => dialect !== 'mssql')) {
5445
postgres: NOT_SUPPORTED,
5546
mysql: {
5647
parameters: [],
57-
sql: "select `website`->'$.url' as `website_url` from `person_metadata`",
48+
sql: 'select `website`->\'$."url"\' as `website_url` from `person_metadata`',
5849
},
5950
mssql: NOT_SUPPORTED,
6051
sqlite: {
6152
parameters: [],
62-
sql: `select "website"->>'$.url' as "website_url" from "person_metadata"`,
53+
sql: `select "website"->>'$."url"' as "website_url" from "person_metadata"`,
6354
},
6455
})
6556

@@ -116,12 +107,12 @@ for (const dialect of DIALECTS.filter((dialect) => dialect !== 'mssql')) {
116107
postgres: NOT_SUPPORTED,
117108
mysql: {
118109
parameters: [],
119-
sql: "select `profile`->'$.auth.roles' as `roles` from `person_metadata`",
110+
sql: 'select `profile`->\'$."auth"."roles"\' as `roles` from `person_metadata`',
120111
},
121112
mssql: NOT_SUPPORTED,
122113
sqlite: {
123114
parameters: [],
124-
sql: `select "profile"->>'$.auth.roles' as "roles" from "person_metadata"`,
115+
sql: `select "profile"->>'$."auth"."roles"' as "roles" from "person_metadata"`,
125116
},
126117
})
127118

@@ -145,12 +136,12 @@ for (const dialect of DIALECTS.filter((dialect) => dialect !== 'mssql')) {
145136
postgres: NOT_SUPPORTED,
146137
mysql: {
147138
parameters: [],
148-
sql: "select `profile`->'$.tags[0]' as `main_tag` from `person_metadata`",
139+
sql: 'select `profile`->\'$."tags"[0]\' as `main_tag` from `person_metadata`',
149140
},
150141
mssql: NOT_SUPPORTED,
151142
sqlite: {
152143
parameters: [],
153-
sql: `select "profile"->>'$.tags[0]' as "main_tag" from "person_metadata"`,
144+
sql: `select "profile"->>'$."tags"[0]' as "main_tag" from "person_metadata"`,
154145
},
155146
})
156147

@@ -178,12 +169,12 @@ for (const dialect of DIALECTS.filter((dialect) => dialect !== 'mssql')) {
178169
postgres: NOT_SUPPORTED,
179170
mysql: {
180171
parameters: [],
181-
sql: "select `experience`->'$[0].establishment' as `establishment` from `person_metadata`",
172+
sql: 'select `experience`->\'$[0]."establishment"\' as `establishment` from `person_metadata`',
182173
},
183174
mssql: NOT_SUPPORTED,
184175
sqlite: {
185176
parameters: [],
186-
sql: `select "experience"->>'$[0].establishment' as "establishment" from "person_metadata"`,
177+
sql: `select "experience"->>'$[0]."establishment"' as "establishment" from "person_metadata"`,
187178
},
188179
})
189180

@@ -331,12 +322,12 @@ for (const dialect of DIALECTS.filter((dialect) => dialect !== 'mssql')) {
331322
postgres: NOT_SUPPORTED,
332323
mysql: {
333324
parameters: [12],
334-
sql: "select * from `person_metadata` where `profile`->'$.auth.login_count' = ?",
325+
sql: `select * from \`person_metadata\` where \`profile\`->'$."auth"."login_count"' = ?`,
335326
},
336327
mssql: NOT_SUPPORTED,
337328
sqlite: {
338329
parameters: [12],
339-
sql: `select * from "person_metadata" where "profile"->>'$.auth.login_count' = ?`,
330+
sql: `select * from "person_metadata" where "profile"->>'$."auth"."login_count"' = ?`,
340331
},
341332
})
342333

@@ -360,12 +351,12 @@ for (const dialect of DIALECTS.filter((dialect) => dialect !== 'mssql')) {
360351
postgres: NOT_SUPPORTED,
361352
mysql: {
362353
parameters: [],
363-
sql: "select * from `person_metadata` order by `profile`->'$.auth.login_count' desc",
354+
sql: 'select * from `person_metadata` order by `profile`->\'$."auth"."login_count"\' desc',
364355
},
365356
mssql: NOT_SUPPORTED,
366357
sqlite: {
367358
parameters: [],
368-
sql: `select * from "person_metadata" order by "profile"->>'$.auth.login_count' desc`,
359+
sql: `select * from "person_metadata" order by "profile"->>'$."auth"."login_count"' desc`,
369360
},
370361
})
371362

@@ -397,12 +388,12 @@ for (const dialect of DIALECTS.filter((dialect) => dialect !== 'mssql')) {
397388
postgres: NOT_SUPPORTED,
398389
mysql: {
399390
parameters: ['Papa Johns', 911],
400-
sql: "update `person_metadata` set `experience` = json_set(`experience`, '$[last].establishment', ?) where `person_id` = ?",
391+
sql: 'update `person_metadata` set `experience` = json_set(`experience`, \'$[last]."establishment"\', ?) where `person_id` = ?',
401392
},
402393
mssql: NOT_SUPPORTED,
403394
sqlite: {
404395
parameters: ['Papa Johns', 911],
405-
sql: `update "person_metadata" set "experience" = json_set("experience", '$[#-1].establishment', ?) where "person_id" = ?`,
396+
sql: `update "person_metadata" set "experience" = json_set("experience", '$[#-1]."establishment"', ?) where "person_id" = ?`,
406397
},
407398
})
408399

@@ -722,124 +713,3 @@ for (const dialect of DIALECTS.filter((dialect) => dialect !== 'mssql')) {
722713
}
723714
})
724715
}
725-
726-
async function initJSONTest<D extends BuiltInDialect>(
727-
ctx: Mocha.Context,
728-
dialect: D,
729-
) {
730-
const testContext = await initTest(ctx, dialect)
731-
732-
let db = testContext.db.withTables<{
733-
person_metadata: {
734-
person_id: number
735-
website: JSONColumnType<{ url: string }>
736-
nicknames: JSONColumnType<string[]>
737-
profile: JSONColumnType<{
738-
auth: {
739-
roles: string[]
740-
last_login?: { device: string }
741-
is_verified: SqlBool
742-
login_count: number
743-
}
744-
avatar: string | null
745-
tags: string[]
746-
}>
747-
experience: JSONColumnType<
748-
{
749-
establishment: string
750-
}[]
751-
>
752-
schedule: JSONColumnType<{ name: string; time: string }[][][]>
753-
}
754-
}>()
755-
756-
if (dialect === 'sqlite') {
757-
db = db.withPlugin(new ParseJSONResultsPlugin())
758-
}
759-
760-
const jsonColumnDataType = resolveJSONColumnDataType(dialect)
761-
const notNull = (cb: ColumnDefinitionBuilder) => cb.notNull()
762-
763-
await db.schema
764-
.createTable('person_metadata')
765-
.addColumn('person_id', 'integer', (cb) =>
766-
cb.primaryKey().references('person.id'),
767-
)
768-
.addColumn('website', jsonColumnDataType, notNull)
769-
.addColumn('nicknames', jsonColumnDataType, notNull)
770-
.addColumn('profile', jsonColumnDataType, notNull)
771-
.addColumn('experience', jsonColumnDataType, notNull)
772-
.addColumn('schedule', jsonColumnDataType, notNull)
773-
.execute()
774-
775-
return { ...testContext, db }
776-
}
777-
778-
function resolveJSONColumnDataType(dialect: BuiltInDialect) {
779-
switch (dialect) {
780-
case 'postgres':
781-
return 'jsonb'
782-
case 'mysql':
783-
return 'json'
784-
case 'mssql':
785-
return sql`nvarchar(max)`
786-
case 'sqlite':
787-
return 'text'
788-
}
789-
}
790-
791-
async function insertDefaultJSONDataSet(ctx: TestContext) {
792-
await insertDefaultDataSet(ctx as any)
793-
794-
const people = await ctx.db
795-
.selectFrom('person')
796-
.select(['id', 'first_name', 'last_name'])
797-
.execute()
798-
799-
await ctx.db
800-
.insertInto('person_metadata')
801-
.values(
802-
people
803-
.filter((person) => person.first_name && person.last_name)
804-
.map((person, index) => ({
805-
person_id: person.id,
806-
website: JSON.stringify({
807-
url: `https://www.${person.first_name!.toLowerCase()}${person.last_name!.toLowerCase()}.com`,
808-
}),
809-
nicknames: JSON.stringify([
810-
`${person.first_name![0]}.${person.last_name![0]}.`,
811-
`${person.first_name} the Great`,
812-
`${person.last_name} the Magnificent`,
813-
]),
814-
profile: JSON.stringify({
815-
tags: ['awesome'],
816-
auth: {
817-
roles: ['contributor', 'moderator'],
818-
last_login: {
819-
device: 'android',
820-
},
821-
login_count: 12 + index,
822-
is_verified: true,
823-
},
824-
avatar: null,
825-
}),
826-
experience: JSON.stringify([
827-
{
828-
establishment: 'The University of Life',
829-
},
830-
]),
831-
schedule: JSON.stringify([[[{ name: 'Gym', time: '12:15' }]]]),
832-
})),
833-
)
834-
.execute()
835-
}
836-
837-
async function clearJSONDatabase(ctx: TestContext) {
838-
await ctx.db.deleteFrom('person_metadata').execute()
839-
await clearDatabase(ctx as any)
840-
}
841-
842-
async function destroyJSONTest(ctx: TestContext) {
843-
await ctx.db.schema.dropTable('person_metadata').execute()
844-
await destroyTest(ctx as any)
845-
}

0 commit comments

Comments
 (0)