Skip to content

Commit 8f63d3f

Browse files
committed
fix: allow only columns of sibling polymorphic resources in filters on a polymorphic target
1 parent 1e88677 commit 8f63d3f

2 files changed

Lines changed: 155 additions & 15 deletions

File tree

‎adminforth/dataConnectors/baseConnector.ts‎

Lines changed: 42 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,40 @@ type AdminForthFilterNormalizationResult = {
2525
normalizedFilters?: AdminForthFilterInput;
2626
};
2727

28+
function polymorphicFilterableFields(resource: AdminForthResource): Set<string> | null {
29+
const resources = global.adminforth?.config?.resources;
30+
if (!resources) {
31+
return null;
32+
}
33+
const siblingResourceIds = new Set<string>();
34+
let isTarget = false;
35+
for (const res of resources) {
36+
for (const col of res.dataSourceColumns) {
37+
const polymorphicResources = col.foreignResource?.polymorphicResources;
38+
if (polymorphicResources?.some((pr) => pr.resourceId === resource.resourceId)) {
39+
isTarget = true;
40+
for (const pr of polymorphicResources) {
41+
if (pr.resourceId) {
42+
siblingResourceIds.add(pr.resourceId);
43+
}
44+
}
45+
}
46+
}
47+
}
48+
if (!isTarget) {
49+
return null;
50+
}
51+
const fields = new Set<string>();
52+
for (const res of resources) {
53+
if (res.resourceId === resource.resourceId || siblingResourceIds.has(res.resourceId)) {
54+
for (const col of res.dataSourceColumns) {
55+
fields.add(col.name);
56+
}
57+
}
58+
}
59+
return fields;
60+
}
61+
2862
async function publishShowPageUpdate(resource: AdminForthResource, recordId: string, updates: Record<string, any>) {
2963
await global.adminforth.websocket.publish(`/showPage/${resource.resourceId}/${String(recordId)}`,
3064
{
@@ -271,23 +305,16 @@ export default class AdminForthBaseConnector implements IAdminForthDataSourceCon
271305
}
272306

273307
const similar = suggestIfTypo(resource.dataSourceColumns.map((col) => col.name), normalizedFilter.field);
274-
275-
let isPolymorphicTarget = false;
276-
if (global.adminforth?.config?.resources) {
277-
isPolymorphicTarget = global.adminforth.config.resources.some(res =>
278-
res.dataSourceColumns.some(col =>
279-
col.foreignResource?.polymorphicResources?.some(pr =>
280-
pr.resourceId === resource.resourceId
281-
)
282-
)
283-
);
284-
}
285-
if (isPolymorphicTarget) {
286-
afLogger.trace(`⚠️ Field '${normalizedFilter.field}' not found in polymorphic target resource '${resource.resourceId}', allowing query to proceed.`);
308+
309+
const polymorphicFields = polymorphicFilterableFields(resource);
310+
const rightFieldAllowed = !comparingWithRightField
311+
|| resource.dataSourceColumns.some((col) => col.name == normalizedFilter.rightField)
312+
|| !!polymorphicFields?.has(normalizedFilter.rightField as string);
313+
if (polymorphicFields?.has(normalizedFilter.field) && rightFieldAllowed) {
314+
afLogger.trace(`Field '${normalizedFilter.field}' not in resource '${resource.resourceId}' but valid on a polymorphic sibling, allowing query to proceed.`);
287315
return { ok: true, error: '', normalizedFilters: normalizedFilter };
288-
} else {
289-
throw new Error(`Field '${normalizedFilter.field}' not found in resource '${resource.resourceId}'. ${similar ? `Did you mean '${similar}'?` : ''}`);
290316
}
317+
throw new Error(`Field '${normalizedFilter.field}' not found in resource '${resource.resourceId}'. ${similar ? `Did you mean '${similar}'?` : ''}`);
291318
}
292319
// value normalization
293320
if (comparingWithRightField) {
Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
import { mkdtempSync, rmSync } from 'node:fs';
2+
import { createRequire } from 'node:module';
3+
import os from 'node:os';
4+
import path from 'node:path';
5+
import { fileURLToPath, pathToFileURL } from 'node:url';
6+
7+
const currentDir = path.dirname(fileURLToPath(import.meta.url));
8+
const applicationDir = path.resolve(currentDir, '../application');
9+
10+
// resolves to the BUILT package (adminforth/dist) the test application runs on;
11+
// rebuild it (`npx tsc` in adminforth/) after editing adminforth sources or the assertions below test stale code
12+
const require = createRequire(import.meta.url);
13+
// jest's resolver does not know the `node:sqlite` builtin, so it is required directly
14+
const { DatabaseSync } = require('node:sqlite');
15+
const adminforthEntry = require.resolve('adminforth', { paths: [applicationDir] });
16+
const { default: AdminForth } = await import(pathToFileURL(adminforthEntry).href);
17+
18+
const fixtureDir = mkdtempSync(path.join(os.tmpdir(), 'af-filter-injection-'));
19+
const dbPath = path.join(fixtureDir, 'db.sqlite');
20+
const db = new DatabaseSync(dbPath);
21+
db.exec(`
22+
CREATE TABLE users (id INTEGER PRIMARY KEY, email TEXT NOT NULL);
23+
INSERT INTO users VALUES (1, 'a@example.test');
24+
INSERT INTO users VALUES (2, 'b@example.test');
25+
CREATE TABLE posts (id INTEGER PRIMARY KEY, title TEXT NOT NULL);
26+
INSERT INTO posts VALUES (1, 'hello');
27+
-- comments.record_id is a polymorphic foreign key, so it makes both users and posts
28+
-- polymorphic targets (the same shape the audit-log plugin produces for every resource)
29+
CREATE TABLE comments (id INTEGER PRIMARY KEY, record_type TEXT NOT NULL, record_id TEXT NOT NULL);
30+
`);
31+
db.close();
32+
33+
process.env.ADMINFORTH_SECRET ??= 'x'.repeat(64);
34+
35+
delete (globalThis as any).adminforth;
36+
const app = new AdminForth({
37+
baseUrl: '',
38+
auth: {
39+
usersResourceId: 'users',
40+
usernameField: 'email',
41+
passwordHashField: 'email',
42+
},
43+
dataSources: [{ id: 'sqlite', url: `sqlite://${dbPath}` }],
44+
resources: [
45+
{
46+
resourceId: 'users',
47+
table: 'users',
48+
dataSource: 'sqlite',
49+
columns: [
50+
{ name: 'id', primaryKey: true },
51+
{ name: 'email' },
52+
],
53+
},
54+
{
55+
resourceId: 'posts',
56+
table: 'posts',
57+
dataSource: 'sqlite',
58+
columns: [
59+
{ name: 'id', primaryKey: true },
60+
{ name: 'title' },
61+
],
62+
},
63+
{
64+
resourceId: 'comments',
65+
table: 'comments',
66+
dataSource: 'sqlite',
67+
columns: [
68+
{ name: 'id', primaryKey: true },
69+
{ name: 'record_type', showIn: { create: false, edit: false } },
70+
{
71+
name: 'record_id',
72+
foreignResource: {
73+
polymorphicOn: 'record_type',
74+
polymorphicResources: [
75+
{ resourceId: 'users', whenValue: 'users' },
76+
{ resourceId: 'posts', whenValue: 'posts' },
77+
],
78+
},
79+
},
80+
],
81+
},
82+
],
83+
menu: [{ label: 'Users', resourceId: 'users' }],
84+
});
85+
await app.discoverDatabases();
86+
87+
afterAll(() => {
88+
rmSync(fixtureDir, { recursive: true, force: true });
89+
});
90+
91+
// users is a polymorphic target (via comments.record_id), so it goes through the relaxed field check
92+
const usersResource = () => app.config.resources.find((r: any) => r.resourceId === 'users');
93+
const connector = () => (app as any).connectors.sqlite;
94+
95+
describe('filter injection hardening', () => {
96+
describe('field names are not passed through unchecked on a polymorphic target', () => {
97+
it.each([
98+
['a $-prefixed operator name', '$where'],
99+
['a $-prefixed comment', '$comment'],
100+
['a SQL identifier break', 'email" = "email'],
101+
['a truly unknown field', 'definitely_not_a_column'],
102+
])('rejects %s', (_, field) => {
103+
expect(() => connector().validateAndNormalizeFilters({ field, operator: 'eq', value: 'x' }, usersResource())).toThrow();
104+
});
105+
106+
it('still allows a column that exists on a sibling polymorphic target', () => {
107+
// `title` belongs to posts, not users, but both are targets of comments.record_id,
108+
// so filtering users by it must stay allowed (the reason the relaxed check exists)
109+
const result = connector().validateAndNormalizeFilters({ field: 'title', operator: 'eq', value: 'x' }, usersResource());
110+
expect(result.ok).toBe(true);
111+
});
112+
});
113+
});

0 commit comments

Comments
 (0)