Skip to content

Commit

Permalink
Silence warnings using accept_potentially_dangerous_queries: true
Browse files Browse the repository at this point in the history
  • Loading branch information
rkistner committed Jul 8, 2024
1 parent 8820b34 commit 777a035
Show file tree
Hide file tree
Showing 9 changed files with 98 additions and 19 deletions.
5 changes: 3 additions & 2 deletions packages/sync-rules/src/SqlBucketDescriptor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
EvaluatedParametersResult,
EvaluationResult,
QueryBucketIdOptions,
QueryParseOptions,
RequestParameters,
SourceSchema,
SqliteRow
Expand Down Expand Up @@ -57,8 +58,8 @@ export class SqlBucketDescriptor {
};
}

addParameterQuery(sql: string, schema: SourceSchema | undefined): QueryParseResult {
const parameterQuery = SqlParameterQuery.fromSql(this.name, sql, schema);
addParameterQuery(sql: string, schema: SourceSchema | undefined, options: QueryParseOptions): QueryParseResult {
const parameterQuery = SqlParameterQuery.fromSql(this.name, sql, schema, options);
if (this.bucket_parameters == null) {
this.bucket_parameters = parameterQuery.bucket_parameters;
} else {
Expand Down
12 changes: 8 additions & 4 deletions packages/sync-rules/src/SqlParameterQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
ParameterMatchClause,
ParameterValueClause,
QueryBucketIdOptions,
QueryParseOptions,
QuerySchema,
RequestParameters,
RowValueClause,
Expand All @@ -33,7 +34,8 @@ export class SqlParameterQuery {
static fromSql(
descriptor_name: string,
sql: string,
schema?: SourceSchema
schema?: SourceSchema,
options?: QueryParseOptions
): SqlParameterQuery | StaticSqlParameterQuery {
const parsed = parse(sql, { locationTracking: true });
const rows = new SqlParameterQuery();
Expand All @@ -49,7 +51,7 @@ export class SqlParameterQuery {

if (q.from == null) {
// E.g. SELECT token_parameters.user_id as user_id WHERE token_parameters.is_admin
return StaticSqlParameterQuery.fromSql(descriptor_name, sql, q);
return StaticSqlParameterQuery.fromSql(descriptor_name, sql, q, options);
}

rows.errors.push(...checkUnsupportedFeatures(sql, q));
Expand Down Expand Up @@ -137,8 +139,10 @@ export class SqlParameterQuery {
rows.tools = tools;
rows.errors.push(...tools.errors);

if (rows.usesDangerousRequestParameters) {
rows.errors.push(new SqlRuleError('Pontially dangerous query based on unauthenticated client parameters', sql));
if (rows.usesDangerousRequestParameters && !options?.accept_potentially_dangerous_queries) {
let err = new SqlRuleError('Pontially dangerous query based on unauthenticated client parameters', sql);
err.type = 'warning';
rows.errors.push(err);
}
return rows;
}
Expand Down
28 changes: 24 additions & 4 deletions packages/sync-rules/src/SqlSyncRules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,15 @@ import {
isEvaluatedRow,
isEvaluationError,
QueryBucketIdOptions,
QueryParseOptions,
RequestParameters,
SourceSchema,
SqliteRow,
SyncRules
} from './types.js';

const ACCEPT_POTENTIALLY_DANGEROUS_QUERIES = Symbol('ACCEPT_POTENTIALLY_DANGEROUS_QUERIES');

export class SqlSyncRules implements SyncRules {
bucket_descriptors: SqlBucketDescriptor[] = [];
idSequence = new IdSequence();
Expand Down Expand Up @@ -50,7 +53,19 @@ export class SqlSyncRules implements SyncRules {
const schema = options?.schema;

const lineCounter = new LineCounter();
const parsed = parseDocument(yaml, { schema: 'core', keepSourceTokens: true, lineCounter });
const parsed = parseDocument(yaml, {
schema: 'core',
keepSourceTokens: true,
lineCounter,
customTags: [
{
tag: '!accept_potentially_dangerous_queries',
resolve(_text: string, _onError: (error: string) => void) {
return ACCEPT_POTENTIALLY_DANGEROUS_QUERIES;
}
}
]
});

const rules = new SqlSyncRules(yaml);

Expand Down Expand Up @@ -81,23 +96,28 @@ export class SqlSyncRules implements SyncRules {
const { key: keyScalar, value } = entry as { key: Scalar; value: YAMLMap };
const key = keyScalar.toString();

const accept_potentially_dangerous_queries =
value.get('accept_potentially_dangerous_queries', true)?.value == true;
const options: QueryParseOptions = {
accept_potentially_dangerous_queries
};
const parameters = value.get('parameters', true) as unknown;
const dataQueries = value.get('data', true) as unknown;

const descriptor = new SqlBucketDescriptor(key, rules.idSequence);

if (parameters instanceof Scalar) {
rules.withScalar(parameters, (q) => {
return descriptor.addParameterQuery(q, schema);
return descriptor.addParameterQuery(q, schema, options);
});
} else if (parameters instanceof YAMLSeq) {
for (let item of parameters.items) {
rules.withScalar(item, (q) => {
return descriptor.addParameterQuery(q, schema);
return descriptor.addParameterQuery(q, schema, options);
});
}
} else {
descriptor.addParameterQuery('SELECT', schema);
descriptor.addParameterQuery('SELECT', schema, options);
}

if (!(dataQueries instanceof YAMLSeq)) {
Expand Down
10 changes: 6 additions & 4 deletions packages/sync-rules/src/StaticSqlParameterQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { SelectedColumn, SelectFromStatement } from 'pgsql-ast-parser';
import { SqlRuleError } from './errors.js';
import { SqlTools } from './sql_filters.js';
import { checkUnsupportedFeatures, isClauseError, isParameterValueClause, sqliteBool } from './sql_support.js';
import { ParameterValueClause, RequestParameters, SqliteJsonValue } from './types.js';
import { ParameterValueClause, QueryParseOptions, RequestParameters, SqliteJsonValue } from './types.js';
import { getBucketId, isJsonValue } from './utils.js';

/**
Expand All @@ -12,7 +12,7 @@ import { getBucketId, isJsonValue } from './utils.js';
* SELECT token_parameters.user_id as user_id WHERE token_parameters.is_admin
*/
export class StaticSqlParameterQuery {
static fromSql(descriptor_name: string, sql: string, q: SelectFromStatement) {
static fromSql(descriptor_name: string, sql: string, q: SelectFromStatement, options?: QueryParseOptions) {
const query = new StaticSqlParameterQuery();

query.errors.push(...checkUnsupportedFeatures(sql, q));
Expand Down Expand Up @@ -50,8 +50,10 @@ export class StaticSqlParameterQuery {

query.errors.push(...tools.errors);

if (query.usesDangerousRequestParameters) {
query.errors.push(new SqlRuleError('Pontially dangerous query based on unauthenticated client parameters', sql));
if (query.usesDangerousRequestParameters && !options?.accept_potentially_dangerous_queries) {
let err = new SqlRuleError('Pontially dangerous query based on unauthenticated client parameters', sql);
err.type = 'warning';
query.errors.push(err);
}
return query;
}
Expand Down
4 changes: 4 additions & 0 deletions packages/sync-rules/src/json_schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ export const syncRulesSchema: ajvModule.Schema = {
required: ['data'],
examples: [{ data: ['select * from mytable'] }],
properties: {
accept_potentially_dangerous_queries: {
description: 'If true, disables warnings on potentially dangerous queries',
type: 'boolean'
},
parameters: {
description: 'Parameter query(ies)',
anyOf: [
Expand Down
4 changes: 4 additions & 0 deletions packages/sync-rules/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ export interface SyncRules {
evaluateParameterRow(table: SourceTableInterface, row: SqliteRow): EvaluatedParametersResult[];
}

export interface QueryParseOptions {
accept_potentially_dangerous_queries?: boolean;
}

export interface EvaluatedParameters {
lookup: SqliteJsonValue[];

Expand Down
16 changes: 12 additions & 4 deletions packages/sync-rules/test/src/parameter_queries.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,9 @@ describe('parameter queries', () => {

test('request.parameters()', function () {
const sql = "SELECT FROM posts WHERE category = request.parameters() ->> 'category_id'";
const query = SqlParameterQuery.fromSql('mybucket', sql) as SqlParameterQuery;
const query = SqlParameterQuery.fromSql('mybucket', sql, undefined, {
accept_potentially_dangerous_queries: true
}) as SqlParameterQuery;
expect(query.errors).toEqual([]);
query.id = '1';
expect(query.evaluateParameterRow({ id: 'group1', category: 'red' })).toEqual([
Expand All @@ -463,7 +465,9 @@ describe('parameter queries', () => {

test('nested request.parameters() (1)', function () {
const sql = "SELECT FROM posts WHERE category = request.parameters() -> 'details' ->> 'category'";
const query = SqlParameterQuery.fromSql('mybucket', sql) as SqlParameterQuery;
const query = SqlParameterQuery.fromSql('mybucket', sql, undefined, {
accept_potentially_dangerous_queries: true
}) as SqlParameterQuery;
expect(query.errors).toEqual([]);
query.id = '1';
expect(query.getLookups(normalizeTokenParameters({}, { details: { category: 'red' } }))).toEqual([
Expand All @@ -473,7 +477,9 @@ describe('parameter queries', () => {

test('nested request.parameters() (2)', function () {
const sql = "SELECT FROM posts WHERE category = request.parameters() ->> 'details.category'";
const query = SqlParameterQuery.fromSql('mybucket', sql) as SqlParameterQuery;
const query = SqlParameterQuery.fromSql('mybucket', sql, undefined, {
accept_potentially_dangerous_queries: true
}) as SqlParameterQuery;
expect(query.errors).toEqual([]);
query.id = '1';
expect(query.getLookups(normalizeTokenParameters({}, { details: { category: 'red' } }))).toEqual([
Expand All @@ -484,7 +490,9 @@ describe('parameter queries', () => {
test('IN request.parameters()', function () {
// Can use -> or ->> here
const sql = "SELECT id as region_id FROM regions WHERE name IN request.parameters() -> 'region_names'";
const query = SqlParameterQuery.fromSql('mybucket', sql) as SqlParameterQuery;
const query = SqlParameterQuery.fromSql('mybucket', sql, undefined, {
accept_potentially_dangerous_queries: true
}) as SqlParameterQuery;
expect(query.errors).toEqual([]);
query.id = '1';
expect(query.evaluateParameterRow({ id: 'region1', name: 'colorado' })).toEqual([
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@ describe('static parameter queries', () => {

test('request.parameters()', function () {
const sql = "SELECT request.parameters() ->> 'org_id' as org_id";
const query = SqlParameterQuery.fromSql('mybucket', sql) as StaticSqlParameterQuery;
const query = SqlParameterQuery.fromSql('mybucket', sql, undefined, {
accept_potentially_dangerous_queries: true
}) as StaticSqlParameterQuery;
expect(query.errors).toEqual([]);

expect(query.getStaticBucketIds(normalizeTokenParameters({}, { org_id: 'test' }))).toEqual(['mybucket["test"]']);
Expand Down
34 changes: 34 additions & 0 deletions packages/sync-rules/test/src/sync_rules.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -679,6 +679,40 @@ bucket_definitions:
]);
});

test('dangerous query errors', () => {
const rules = SqlSyncRules.fromYaml(
`
bucket_definitions:
mybucket:
parameters: SELECT request.parameters() ->> 'project_id' as project_id
data: []
`,
{ schema: BASIC_SCHEMA }
);

expect(rules.errors).toMatchObject([
{
message: 'Pontially dangerous query based on unauthenticated client parameters',
type: 'warning'
}
]);
});

test('dangerous query errors - ignored', () => {
const rules = SqlSyncRules.fromYaml(
`
bucket_definitions:
mybucket:
accept_potentially_dangerous_queries: true
parameters: SELECT request.parameters() ->> 'project_id' as project_id
data: []
`,
{ schema: BASIC_SCHEMA }
);

expect(rules.errors).toEqual([]);
});

test('schema generation', () => {
const schema = new StaticSchema([
{
Expand Down

0 comments on commit 777a035

Please sign in to comment.