Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling of invalid and empty filter queries #22048

Merged
merged 12 commits into from
May 7, 2024
5 changes: 5 additions & 0 deletions .changeset/three-dolls-ring.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---

Check warning on line 1 in .changeset/three-dolls-ring.md

View workflow job for this annotation

GitHub Actions / Lint

File ignored by default.
'@directus/api': patch
---

Fixed handling of invalid and empty filter queries
22 changes: 16 additions & 6 deletions api/src/utils/apply-query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,11 @@ export function applyFilter(

const { relation, relationType } = getRelationInfo(relations, collection, pathRoot);

const { operator: filterOperator, value: filterValue } = getOperation(key, value);
const operation = getOperation(key, value);

if (!operation) continue;

const { operator: filterOperator, value: filterValue } = operation;

if (
filterPath.length > 1 ||
Expand Down Expand Up @@ -912,25 +916,31 @@ export function applyAggregate(

function getFilterPath(key: string, value: Record<string, any>) {
const path = [key];
const childKey = Object.keys(value)[0]!;
const childKey = Object.keys(value)[0];

if (typeof childKey === 'string' && childKey.startsWith('_') === true && !['_none', '_some'].includes(childKey)) {
return path;
}

if (isPlainObject(value)) {
if (childKey && isPlainObject(value)) {
path.push(...getFilterPath(childKey, Object.values(value)[0]));
}
paescuj marked this conversation as resolved.
Show resolved Hide resolved

return path;
}

function getOperation(key: string, value: Record<string, any>): { operator: string; value: any } {
function getOperation(key: string, value: Record<string, any>): { operator: string; value: any } | null {
if (key.startsWith('_') && !['_and', '_or', '_none', '_some'].includes(key)) {
return { operator: key as string, value };
return { operator: key, value };
} else if (isPlainObject(value) === false) {
return { operator: '_eq', value };
}

return getOperation(Object.keys(value)[0]!, Object.values(value)[0]);
const childKey = Object.keys(value)[0];

if (childKey) {
return getOperation(childKey, Object.values(value)[0]);
}

return null;
}
26 changes: 24 additions & 2 deletions api/src/utils/sanitize-query.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { useEnv } from '@directus/env';
import { parseFilter, parseJSON } from '@directus/utils';
import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest';
import { sanitizeQuery } from './sanitize-query.js';

Expand All @@ -7,10 +8,11 @@ import { sanitizeQuery } from './sanitize-query.js';
vi.mock('@directus/env', () => ({ useEnv: vi.fn().mockReturnValue({}) }));

vi.mock('@directus/utils', async () => {
const actual = (await vi.importActual('@directus/utils')) as any;
const actual = await vi.importActual<typeof import('@directus/utils')>('@directus/utils');

return {
...actual,
parseJSON: vi.fn().mockImplementation(actual.parseJSON),
parseFilter: vi.fn().mockImplementation((value) => value),
};
});
Expand Down Expand Up @@ -200,21 +202,41 @@ describe('sort', () => {
});

describe('filter', () => {
test('should accept valid value', () => {
test('should accept valid filter', () => {
const filter = { field_a: { _eq: 'test' } };

const sanitizedQuery = sanitizeQuery({ filter });

expect(sanitizedQuery.filter).toEqual({ field_a: { _eq: 'test' } });
});

test('should throw error on invalid filter', () => {
const filter = { field_a: null };

vi.mocked(parseFilter).mockImplementationOnce(() => {
throw new Error();
});

expect(() => sanitizeQuery({ filter })).toThrowError('Invalid query. Invalid filter object.');
});

test('should parse as json when it is a string', () => {
const filter = '{ "field_a": { "_eq": "test" } }';

const sanitizedQuery = sanitizeQuery({ filter });

expect(sanitizedQuery.filter).toEqual({ field_a: { _eq: 'test' } });
});

test('should throw error on invalid json', () => {
const filter = '{ "field_a": }';

vi.mocked(parseJSON).mockImplementationOnce(() => {
throw new Error();
});

expect(() => sanitizeQuery({ filter })).toThrowError('Invalid query. Invalid JSON for filter object.');
});
});

describe('offset', () => {
Expand Down
19 changes: 11 additions & 8 deletions api/src/utils/sanitize-query.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { useEnv } from '@directus/env';
import type { Accountability, Aggregate, Filter, Query } from '@directus/types';
import { InvalidQueryError } from '@directus/errors';
import type { Accountability, Aggregate, Query } from '@directus/types';
import { parseFilter, parseJSON } from '@directus/utils';
import { flatten, get, isPlainObject, merge, set } from 'lodash-es';
import { useLogger } from '../logger.js';
Expand Down Expand Up @@ -137,19 +138,21 @@ function sanitizeAggregate(rawAggregate: any): Aggregate {
}

function sanitizeFilter(rawFilter: any, accountability: Accountability | null) {
const logger = useLogger();

let filters: Filter | null = rawFilter;
let filters = rawFilter;

if (typeof rawFilter === 'string') {
if (typeof filters === 'string') {
try {
filters = parseJSON(rawFilter);
filters = parseJSON(filters);
} catch {
logger.warn('Invalid value passed for filter query parameter.');
throw new InvalidQueryError({ reason: 'Invalid JSON for filter object' });
}
}

return parseFilter(filters, accountability);
try {
return parseFilter(filters, accountability);
} catch {
throw new InvalidQueryError({ reason: 'Invalid filter object' });
}
}

function sanitizeLimit(rawLimit: any) {
Expand Down
6 changes: 2 additions & 4 deletions api/src/utils/validate-query.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { useEnv } from '@directus/env';
import { InvalidQueryError } from '@directus/errors';
import type { Query } from '@directus/types';
import type { Filter, Query } from '@directus/types';
import Joi from 'joi';
import { isPlainObject, uniq } from 'lodash-es';
import { stringify } from 'wellknown';
Expand Down Expand Up @@ -52,9 +52,7 @@ export function validateQuery(query: Query): Query {
return query;
}

function validateFilter(filter: Query['filter']) {
if (!filter) throw new InvalidQueryError({ reason: 'Invalid filter object' });

function validateFilter(filter: Filter) {
for (const [key, nested] of Object.entries(filter)) {
if (key === '_and' || key === '_or') {
nested.forEach(validateFilter);
Expand Down