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

qs.parse returns invalid key with brackets when using nested array query #1751

Merged
merged 13 commits into from
Nov 6, 2024
Merged
4 changes: 2 additions & 2 deletions packages/host/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
"@types/matrix-js-sdk": "^11.0.1",
"@types/ms": "^0.7.34",
"@types/pluralize": "^0.0.30",
"@types/qs": "^6.9.14",
"@types/qs": "^6.9.17",
"@types/qunit": "^2.11.3",
"@types/rsvp": "^4.0.9",
"@types/uuid": "^9.0.8",
Expand Down Expand Up @@ -159,7 +159,7 @@
"pluralize": "^8.0.0",
"prettier": "^3.0.3",
"prettier-plugin-ember-template-tag": "^1.1.0",
"qs": "^6.12.3",
"qs": "^6.13.0",
"qunit": "^2.20.0",
"qunit-dom": "^2.0.0",
"safe-stable-stringify": "^2.4.3",
Expand Down
54 changes: 54 additions & 0 deletions packages/host/tests/unit/qs-test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import qs from 'qs';
import { module, test } from 'qunit';

import { parseQuery, Query } from '@cardstack/runtime-common/query';

module('Unit | qs | parse', function () {
test('parseQuery errors out if the query is too deep', async function (assert) {
assert.throws(
() => parseQuery('a[b][c][d][e][f][g][h][i][j][k][l]=m'),
/RangeError: Input depth exceeded depth option of 10 and strictDepth is true/,
);
});
test('invertibility: applying stringify and parse on object will return the same object', async function (assert) {
let testRealmURL = 'https://example.com/';
let query: Query = {
filter: {
on: {
module: `${testRealmURL}book`,
name: 'Book',
},
every: [
{
eq: {
'author.firstName': 'Cardy',
},
},
{
any: [
{
eq: {
'author.lastName': 'Jones',
},
},
{
eq: {
'author.lastName': 'Stackington Jr. III',
},
},
],
},
],
},
sort: [
{
by: 'author.lastName',
on: { module: `${testRealmURL}book`, name: 'Book' },
},
],
};
let queryString = qs.stringify(query);
let parsedQuery: any = parseQuery(queryString);
assert.deepEqual(parsedQuery, query);
});
});
4 changes: 2 additions & 2 deletions packages/realm-server/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
"@types/mime-types": "^2.1.1",
"@types/node": "^18.18.5",
"@types/pg": "^8.11.5",
"@types/qs": "^6.9.14",
"@types/qs": "^6.9.17",
"@types/qunit": "^2.11.3",
"@types/sane": "^2.0.1",
"@types/supertest": "^2.0.12",
Expand Down Expand Up @@ -51,7 +51,7 @@
"pg": "^8.11.5",
"prettier": "^2.8.4",
"prettier-plugin-ember-template-tag": "^1.1.0",
"qs": "^6.12.3",
"qs": "^6.13.0",
"qunit": "^2.20.0",
"sane": "^5.0.1",
"sql-parser-cst": "^0.28.0",
Expand Down
4 changes: 2 additions & 2 deletions packages/runtime-common/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
"@types/jsonwebtoken": "^9.0.5",
"@types/lodash": "^4.14.182",
"@types/pluralize": "^0.0.30",
"@types/qs": "^6.9.14",
"@types/qs": "^6.9.17",
"@types/uuid": "^9.0.8",
"babel-import-util": "^1.2.2",
"babel-plugin-ember-template-compilation": "^2.2.1",
Expand All @@ -45,7 +45,7 @@
"loglevel": "^1.8.1",
"marked": "^12.0.1",
"pluralize": "^8.0.0",
"qs": "^6.12.3",
"qs": "^6.13.0",
"qunit": "^2.20.0",
"recast": "^0.23.4",
"safe-stable-stringify": "^2.4.3",
Expand Down
32 changes: 23 additions & 9 deletions packages/runtime-common/query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -307,9 +307,9 @@ function assertEveryFilter(
`${pointer.join('/') || '/'}: every must be an array of Filters`,
);
} else {
filter.every.every((value: any, index: number) =>
assertFilter(value, pointer.concat(`[${index}]`)),
);
Comment on lines -310 to -312
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.every is problematic because it expects to return a boolean. So, not all assertions are being run thru

filter.every.forEach((value: any, index: number) => {
assertFilter(value, pointer.concat(`[${index}]`));
});
}
}

Expand Down Expand Up @@ -348,9 +348,10 @@ function assertEqFilter(
if (typeof filter.eq !== 'object' || filter.eq == null) {
throw new Error(`${pointer.join('/') || '/'}: eq must be an object`);
}
Object.entries(filter.eq).every(([key, value]) =>
assertJSONValue(value, pointer.concat(key)),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pointer.concat doesn't even mutate the pointer. I would prefer to handle in another PR

);
Object.entries(filter.eq).forEach(([key, value]) => {
assertKey(key, pointer);
assertJSONValue(value, pointer.concat(key));
});
}

function assertContainsFilter(
Expand All @@ -371,9 +372,10 @@ function assertContainsFilter(
if (typeof filter.contains !== 'object' || filter.contains == null) {
throw new Error(`${pointer.join('/') || '/'}: contains must be an object`);
}
Object.entries(filter.contains).every(([key, value]) =>
assertJSONValue(value, pointer.concat(key)),
);
Object.entries(filter.contains).forEach(([key, value]) => {
assertKey(key, pointer);
assertJSONValue(value, pointer.concat(key));
});
}

function assertRangeFilter(
Expand Down Expand Up @@ -419,3 +421,15 @@ function assertRangeFilter(
});
});
}

export function assertKey(key: string, pointer: string[]) {
if (key.startsWith('[') && key.endsWith(']')) {
throw new Error(
`${pointer.join('/')}: field names cannot be wrapped in brackets: ${key}`,
);
}
}

export const parseQuery = (queryString: string) => {
return qs.parse(queryString, { depth: 10, strictDepth: true });
};
8 changes: 4 additions & 4 deletions packages/runtime-common/realm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ import {
SupportedMimeType,
lookupRouteTable,
} from './router';
import { assertQuery } from './query';
import { assertQuery, parseQuery } from './query';
import type { Readable } from 'stream';
import { type CardDef } from 'https://cardstack.com/base/card-api';
import type * as CardAPI from 'https://cardstack.com/base/card-api';
Expand All @@ -76,7 +76,6 @@ import { fetcher } from './fetcher';
import { RealmIndexQueryEngine } from './realm-index-query-engine';
import { RealmIndexUpdater } from './realm-index-updater';

import qs from 'qs';
import {
MatrixBackendAuthentication,
Utils,
Expand Down Expand Up @@ -1548,7 +1547,7 @@ export class Realm {
request.headers.get('X-Boxel-Building-Index'),
);

let cardsQuery = qs.parse(new URL(request.url).search.slice(1));
let cardsQuery = parseQuery(new URL(request.url).search.slice(1));
assertQuery(cardsQuery);

let doc = await this.#realmIndexQueryEngine.search(cardsQuery, {
Expand All @@ -1572,7 +1571,8 @@ export class Realm {
request.headers.get('X-Boxel-Building-Index'),
);

let parsedQueryString = qs.parse(new URL(request.url).search.slice(1));
let href = new URL(request.url).search.slice(1);
let parsedQueryString = parseQuery(href);
let htmlFormat = parsedQueryString.prerenderedHtmlFormat as string;
let cardUrls = parsedQueryString.cardUrls as string[];

Expand Down
Loading
Loading