a26b507361
Row-level permission predicates were only ever applied to a query's main
alias, so any SQL join leaked rows the caller is not allowed to see. The
visible symptom: dashboard charts grouped by a relation field (e.g.
Opportunities by Company) read the group dimension off an unfiltered
joined table, surfacing hidden companies as chart labels.
`WorkspaceSelectQueryBuilder` now applies the joined object's predicate
to every relation join's `ON` condition. Using `ON` rather than `WHERE`
keeps left-join semantics correct: a visible record linked to a hidden
related row is still counted, it just falls into the null group instead
of being attributed to the hidden row.
This closes the same class of leak in relation filters and
order-by-on-relation, plus the three paths that serialize a builder via
`.getQuery()` and never reach the execution overrides (group-by with
records, per-parent relation limiting, mutation id subqueries). The
per-parent fix also stops hidden rows from consuming `LIMIT` slots
before being filtered out.
```mermaid
flowchart TD
A["WorkspaceSelectQueryBuilder<br/>SELECT FROM person LEFT JOIN company"]
A --> B["getMany / getOne / getCount / execute<br/>(execution overrides)"]
A --> C["getQuery() serialization:<br/>group-by with records,<br/>per-parent relation limit,<br/>mutation id subquery"]
B --> D["validatePermissions()"]
D --> E["applyRowLevelPermissionPredicates<br/>ToMainAliasAndJoinedRelations()"]
C --> E
E --> F["main alias:<br/>WHERE person predicate"]
E --> G["every relation join:<br/>ON person.companyId = company.id<br/>AND company predicate"]
F --> H["hidden companies never surface as group dimensions,<br/>relation-filter matches or sort keys;<br/>a hidden link sorts as NULL and the row is still counted"]
G --> H
```
The last two commits remove the duplication this fix would otherwise
have introduced: one shared `and`/`or`/`not` filter walker (the GraphQL
filter parser and the RLS util were verbatim forks), one RLS
record-filter resolver used by all three call sites, and one shared set
of RLS integration-test fixtures. Behaviour-preserving, with new
characterization tests pinning the emitted condition tree.
Reviewer notes:
- Results change where a join is involved: relation filters no longer
match hidden related records, and order-by-on-relation sorts
hidden-linked rows as null, which can shift pagination.
- Joins on subqueries/custom tables are skipped, and objects with no
predicates for the role are a no-op, so admins and system contexts are
unaffected.
- Timeline messaging inner joins are filtered too, so thread counts can
change for restricted roles.
- Predicates that need the current workspace member (Me) are still
skipped for API key and application contexts, on joins as on the main
alias.
- The join renderer skips the field-level read-permission check the
main-alias parser performs: predicates on read-restricted fields still
filter joins, and the field values are never selected.
- One user-facing change beyond the leak fix: the empty-array filter
error no longer echoes the submitted value back (`Invalid filter value:
"<value>"` -> `Invalid filter value`), on every filter path rather than
just RLS. Catalogs are not regenerated here, so it falls back to English
until the next i18n sync.
<!-- This is an auto-generated description by cubic. -->
<a
href="https://cubic.dev/pr/twentyhq/twenty/pull/23369?utm_source=github"
target="_blank" rel="noopener noreferrer"
data-no-image-dialog="true"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img
alt="Review in cubic"
src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a>
<!-- End of auto-generated description by cubic. -->
65 lines
1.8 KiB
TypeScript
65 lines
1.8 KiB
TypeScript
import {
|
|
Brackets,
|
|
NotBrackets,
|
|
type ObjectLiteral,
|
|
type WhereExpressionBuilder,
|
|
} from 'typeorm';
|
|
|
|
export type RecordedWhereNode =
|
|
| { kind: 'sql'; sql: string; parameters: ObjectLiteral | undefined }
|
|
| { kind: 'brackets'; children: RecordedWhereCall[] }
|
|
| { kind: 'notBrackets'; children: RecordedWhereCall[] };
|
|
|
|
export type RecordedWhereCall = {
|
|
method: 'where' | 'andWhere' | 'orWhere';
|
|
node: RecordedWhereNode;
|
|
};
|
|
|
|
export type WhereExpressionRecorder = {
|
|
whereExpression: WhereExpressionBuilder;
|
|
calls: RecordedWhereCall[];
|
|
};
|
|
|
|
export const createWhereExpressionRecorder = (): WhereExpressionRecorder => {
|
|
const calls: RecordedWhereCall[] = [];
|
|
|
|
const recordNode = (
|
|
condition: unknown,
|
|
parameters: ObjectLiteral | undefined,
|
|
): RecordedWhereNode => {
|
|
if (condition instanceof NotBrackets) {
|
|
const childRecorder = createWhereExpressionRecorder();
|
|
|
|
condition.whereFactory(childRecorder.whereExpression);
|
|
|
|
return { kind: 'notBrackets', children: childRecorder.calls };
|
|
}
|
|
|
|
if (condition instanceof Brackets) {
|
|
const childRecorder = createWhereExpressionRecorder();
|
|
|
|
condition.whereFactory(childRecorder.whereExpression);
|
|
|
|
return { kind: 'brackets', children: childRecorder.calls };
|
|
}
|
|
|
|
return { kind: 'sql', sql: String(condition), parameters };
|
|
};
|
|
|
|
const recordCall =
|
|
(method: RecordedWhereCall['method']) =>
|
|
(condition: unknown, parameters?: ObjectLiteral) => {
|
|
calls.push({ method, node: recordNode(condition, parameters) });
|
|
|
|
return whereExpression;
|
|
};
|
|
|
|
const whereExpression = {
|
|
where: recordCall('where'),
|
|
andWhere: recordCall('andWhere'),
|
|
orWhere: recordCall('orWhere'),
|
|
} as unknown as WhereExpressionBuilder;
|
|
|
|
return { whereExpression, calls };
|
|
};
|