Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,14 @@ export class ExportCSVFromTableUseCase
if (isHexString(searchingFieldValue)) {
searchingFieldValue = hexToBinary(searchingFieldValue) as any;
// Readable columns only — a binary search must not reach a withheld column either.
tableSettings.search_fields = queryableStructure
// This must land on the settings object the DAO actually receives; assigning it to
// `tableSettings` (which was already consumed above) had no effect on the query.
Comment on lines 111 to +115
const binarySearchFields = queryableStructure
.filter((field) => isBinary(field.data_type))
.map((field) => field.column_name);
if (binarySearchFields.length > 0) {
builtDAOsTableSettings.search_fields = binarySearchFields;
}
}

const rowsStream = await dao.getTableRowsStream(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ import {
filterReferencedTablesByPermission,
} from '../utils/process-referenced-tables.util.js';
import { removePasswordsFromRowsUtil } from '../utils/remove-password-from-row.util.js';
import {
assertSomeColumnReadable,
restrictTableSettingsToReadableColumns,
} from '../utils/restrict-query-to-readable-columns.util.js';
import { getUserEmailForAgent, validateConnection } from '../utils/validate-connection.util.js';
import { IGetRowByPrimaryKey } from './table-use-cases.interface.js';

Expand Down Expand Up @@ -133,13 +137,31 @@ export class GetRowByPrimaryKeyUseCase
),
);
}
// Column-level read permission (the ColumnRead half of table:read), resolved BEFORE the query
// so a withheld column is never selected — and so a caller who may read no column at all gets
// a 403 instead of a row-existence answer (plan 13 P0-3; same rule as
// `pure-read-row-from-table.use.case.ts`).
const allColumnNames = tableStructure.map((column) => column.column_name);
const readableColumns = await this.cedarPermissions.getReadableColumns(
userId,
connectionId,
tableName,
allColumnNames,
);
assertSomeColumnReadable(readableColumns);

let rowData: Record<string, unknown>;
const builtDAOsTableSettings = buildDAOsTableSettingsDs(
buildCommonTableSettingsInput(tableSettings),
personalTableSettings,
);
// The DAO's copy carries the withheld columns in `excluded_fields`, which bounds its
// `select()` list. The response keeps the unrestricted copy so the withheld column NAMES are
// not disclosed through `table_settings`.
const daoTableSettings = { ...builtDAOsTableSettings };
restrictTableSettingsToReadableColumns(daoTableSettings, readableColumns, allColumnNames);
try {
rowData = await dao.getRowByPrimaryKey(tableName, primaryKey, builtDAOsTableSettings, userEmail);
rowData = await dao.getRowByPrimaryKey(tableName, primaryKey, daoTableSettings, userEmail);
Comment on lines +140 to +164

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Authorize the primary-key predicate before the row query.

assertSomeColumnReadable() allows this query when any column is readable. It does not require the submitted primary-key columns to be readable. A caller can submit a withheld primary key and distinguish an existing row from a missing row.

Reject the request with 403 when any received primary-key column is absent from readableColumns. Add a test where id is withheld but another column is readable.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/src/entities/table/use-cases/get-row-by-primary-key.use.case.ts`
around lines 140 - 164, Validate the submitted primary-key columns against
readableColumns before invoking dao.getRowByPrimaryKey; reject with 403 when any
primary-key column is withheld, while preserving the existing
assertSomeColumnReadable behavior. Add coverage for a request where id is
unreadable but another column remains readable, ensuring the query is denied.

} catch (e) {
throw new UnknownSQLException(getErrorMessage(e), ExceptionOperations.FAILED_TO_GET_ROW_BY_PRIMARY_KEY);
}
Expand All @@ -155,16 +177,9 @@ export class GetRowByPrimaryKeyUseCase
rowData = removePasswordsFromRowsUtil(rowData, tableWidgets);
let formedTableStructure = formFullTableStructure(tableStructure, tableSettings);

// Column-level read permission (the ColumnRead half of table:read): strip columns the
// user may not read from the row and metadata.
const allColumnNames = tableStructure.map((column) => column.column_name);
const readableColumns = await this.cedarPermissions.getReadableColumns(
userId,
connectionId,
tableName,
allColumnNames,
);
let listFields = findAvailableFields(builtDAOsTableSettings, tableStructure);
// Response-side projection, on top of the query-level restriction above (defense in depth: a
// widget or a DAO that ignores `excluded_fields` must not put a withheld column in the row).
let listFields = findAvailableFields(daoTableSettings, tableStructure);
if (!isAllColumnsReadable(readableColumns, allColumnNames)) {
Comment on lines +180 to 183
rowData = filterRowByReadableColumns(rowData, readableColumns);
formedTableStructure = filterStructureByReadableColumns(formedTableStructure, readableColumns);
Expand Down
57 changes: 37 additions & 20 deletions backend/src/entities/table/use-cases/get-table-rows.use.case.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ import { findOrderingFieldUtil } from '../utils/find-ordering-field.util.js';
import { formFullTableStructure } from '../utils/form-full-table-structure.js';
import { isHexString } from '../utils/is-hex-string.js';
import { processRowsUtil } from '../utils/process-found-rows-util.js';
import {
assertSomeColumnReadable,
readableTableStructure,
restrictTableSettingsToReadableColumns,
} from '../utils/restrict-query-to-readable-columns.util.js';
import { getUserEmailForAgent, validateConnection } from '../utils/validate-connection.util.js';
import { IGetTableRows } from './table-use-cases.interface.js';

Expand Down Expand Up @@ -113,11 +118,28 @@ export class GetTableRowsUseCase extends AbstractUseCase<GetTableRowsDs, FoundTa
this._dbContext.tableFiltersRepository.findTableFiltersForTableInConnection(tableName, connectionId),
this._dbContext.personalTableSettingsRepository.findUserTableSettings(userId, connectionId, tableName),
]);
// Column-level read permission (the ColumnRead half of table:read) must bound the QUERY,
// not only the response (plan 13 P0-3): resolved here, BEFORE filters, search, ordering
// and autocomplete are parsed, so a withheld column can be neither filtered, searched,
// ordered by nor selected. Stripping it from the rows afterwards still let
// `pagination.total` answer "does this column start with X?" one character at a time.
// Keep in step with the pure-CRUD twin (`pure-get-rows-from-table.use.case.ts`).
const allColumnNames = tableStructure.map((column) => column.column_name);
const readableColumns = await this.cedarPermissions.getReadableColumns(
userId,
connectionId,
tableName,
allColumnNames,
);
assertSomeColumnReadable(readableColumns);
const restrictColumns = !isAllColumnsReadable(readableColumns, allColumnNames);
const queryableStructure = readableTableStructure(tableStructure, readableColumns);

const filteringFields: Array<FilteringFieldsDs> = isObjectEmpty(filters)
? findFilteringFieldsUtil(query, tableStructure)
: parseFilteringFieldsFromBodyData(filters ?? {}, tableStructure);
? findFilteringFieldsUtil(query, queryableStructure)
: parseFilteringFieldsFromBodyData(filters ?? {}, queryableStructure);

const orderingField = findOrderingFieldUtil(query, tableStructure, tableSettings);
const orderingField = findOrderingFieldUtil(query, queryableStructure, tableSettings);

const configured = !!tableSettings;

Expand All @@ -135,7 +157,7 @@ export class GetTableRowsUseCase extends AbstractUseCase<GetTableRowsDs, FoundTa

const autocompleteFields: AutocompleteFieldsDs =
autocomplete && referencedColumn
? findAutocompleteFieldsUtil(query, tableStructure, tableSettings, referencedColumn)
? findAutocompleteFieldsUtil(query, queryableStructure, tableSettings, referencedColumn)
Comment on lines 158 to +160

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Keep the identity column inside the readable-column boundary.

findAutocompleteFieldsUtil() receives queryableStructure, but it adds tableSettings.identity_column without checking that the identity column is in that structure. If the identity column is withheld, autocomplete still searches it.

Only add identity_column when it is in the readable structure. Add an end-to-end test with a readable referenced column and a withheld identity column.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/src/entities/table/use-cases/get-table-rows.use.case.ts` around lines
158 - 160, Update the autocomplete setup in the get-table-rows use case and
findAutocompleteFieldsUtil flow so tableSettings.identity_column is added only
when it exists in queryableStructure, preventing withheld identity columns from
being searched. Add an end-to-end test covering a readable referenced column
with the identity column withheld.

: { fields: [], value: '' };

const builtDAOsTableSettings = buildDAOsTableSettingsDs(
Expand All @@ -148,24 +170,31 @@ export class GetTableRowsUseCase extends AbstractUseCase<GetTableRowsDs, FoundTa
}
if (
isHexString(searchingFieldValue) &&
(tableStructure.some((field) => isBinary(field.data_type)) ||
(queryableStructure.some((field) => isBinary(field.data_type)) ||
connection.type === ConnectionTypesEnum.mongodb ||
connection.type === ConnectionTypesEnum.agent_mongodb)
) {
searchingFieldValue = hexToBinary(searchingFieldValue) as any;
builtDAOsTableSettings.search_fields = tableStructure
// Readable columns only — a binary search must not reach a withheld column either.
builtDAOsTableSettings.search_fields = queryableStructure
.filter((field) => isBinary(field.data_type))
.map((field) => field.column_name);
if (connection.type === 'mongodb' || connection.type === 'agent_mongodb') {
builtDAOsTableSettings.search_fields.push('_id');
}
Comment on lines 182 to 184

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Do not add a withheld MongoDB _id field to the search.

This branch adds _id even when readableColumns does not contain _id. A hexadecimal search can then use a withheld field as a predicate and disclose row existence or permitted values from the matched row.

Add _id only when it is readable. Add a MongoDB permission test where _id is withheld.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/src/entities/table/use-cases/get-table-rows.use.case.ts` around lines
182 - 184, Update the MongoDB branch in the table-row use case to append `_id`
to `builtDAOsTableSettings.search_fields` only when `readableColumns` includes
`_id`, preventing searches from using withheld fields. Add a permission test
covering MongoDB behavior when `_id` is not readable.

}

// The settings the DAO gets carry the withheld columns in `excluded_fields`, which bounds
// its `select()` list and its default search fields. The response keeps the unrestricted
// copy, so the withheld column NAMES are not disclosed through `table_settings`.
Comment on lines +187 to +189
const daoTableSettings = { ...builtDAOsTableSettings };
restrictTableSettingsToReadableColumns(daoTableSettings, readableColumns, allColumnNames);

Comment on lines +190 to +192
let rows: FoundRowsDS;
try {
rows = await dao.getRowsFromTable(
tableName,
builtDAOsTableSettings,
daoTableSettings,
page,
perPage,
searchingFieldValue,
Expand Down Expand Up @@ -212,22 +241,10 @@ export class GetTableRowsUseCase extends AbstractUseCase<GetTableRowsDs, FoundTa

const largeDataset = rows.large_dataset || rows.pagination.total > Constants.LARGE_DATASET_ROW_LIMIT;

const listFields = findAvailableFields(builtDAOsTableSettings, tableStructure);
const listFields = findAvailableFields(daoTableSettings, tableStructure);
const actionEventsDtos = customActionEvents.map((el) => buildActionEventDto(el));
const savedFiltersRO = savedTableFilters.map((el) => buildCreatedTableFilterRO(el));

// Column-level read permission (the ColumnRead half of table:read). Computed once;
// when the user lacks read access to some columns we strip them from the rows and
// metadata below, after foreign-key identity enrichment has run.
const allColumnNames = tableStructure.map((column) => column.column_name);
const readableColumns = await this.cedarPermissions.getReadableColumns(
userId,
connectionId,
tableName,
allColumnNames,
);
const restrictColumns = !isAllColumnsReadable(readableColumns, allColumnNames);

const rowsRO = {
rows: rows.data,
primaryColumns: tablePrimaryColumns,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,176 @@ test.serial(
},
);

// Plan 13 P0-3 on the AUTHENTICATED read path: stripping a withheld column from the response still
// let a filter/search on it run in SQL, so `pagination.total` answered "does this column start with
// X?" one character at a time. The readable set now bounds the query itself, so a filter on a
// withheld column is ignored exactly like one naming a column that does not exist.
// The seeded table has 42 rows, 3 of them carrying 'Vasia' in `testTableColumnName`; the counts below
// are the oracle (42 = the filter was dropped, 3 = it ran).
function readOnlyCedarPolicy(connectionId: string, tableName: string, readableColumns: Array<string>): string {
return [
`permit(\n principal,\n action == RocketAdmin::Action::"connection:read",\n resource == RocketAdmin::Connection::"${connectionId}"\n);`,
`permit(\n principal,\n action == RocketAdmin::Action::"table:query",\n resource == RocketAdmin::Table::"${connectionId}/${tableName}"\n);`,
...readableColumns.map(
(columnName) =>
`permit(\n principal,\n action == RocketAdmin::Action::"column:read",\n resource == RocketAdmin::Column::"${connectionId}/${tableName}/${columnName}"\n);`,
),
].join('\n\n');
}

test.serial(
`${currentTest} authenticated rows: a filter on a withheld column is ignored in both filter forms`,
async (t) => {
try {
const testData = await createConnectionsAndInviteNewUserInNewGroupWithGroupPermissions(app);
const connectionId = testData.connections.firstId;
const groupId = testData.groups.createdGroupId;
const tableName = testData.firstTableInfo.testTableName;
// The 'Vasia' column is WITHHELD; only id + the email column are readable.
const hiddenColumn = testData.firstTableInfo.testTableColumnName;
const allowedColumn = testData.firstTableInfo.testTableSecondColumnName;

const savePolicyResponse = await request(app.getHttpServer())
.post(`/connection/cedar-policy/${connectionId}`)
.send({ cedarPolicy: readOnlyCedarPolicy(connectionId, tableName, ['id', allowedColumn]), groupId })
.set('Cookie', testData.users.adminUserToken)
.set('Content-Type', 'application/json')
.set('Accept', 'application/json');
t.is(savePolicyResponse.status, 201);

// Query-string filter form (`f_<col>__eq`), used by GET /table/rows.
const queryFiltered = await request(app.getHttpServer())
.get(`/table/rows/${connectionId}?tableName=${tableName}&page=1&perPage=10&f_${hiddenColumn}__eq=Vasia`)
.set('Cookie', testData.users.simpleUserToken)
.set('Content-Type', 'application/json')
.set('Accept', 'application/json');
t.is(queryFiltered.status, 200);
t.is(queryFiltered.body.pagination.total, 42);
t.false(Object.keys(queryFiltered.body.rows[0]).includes(hiddenColumn));

// Body filter form, used by POST /table/rows/find.
const bodyFiltered = await request(app.getHttpServer())
.post(`/table/rows/find/${connectionId}?tableName=${tableName}&page=1&perPage=10`)
.send({ filters: { [hiddenColumn]: { eq: 'Vasia' } } })
.set('Cookie', testData.users.simpleUserToken)
.set('Content-Type', 'application/json')
.set('Accept', 'application/json');
t.is(bodyFiltered.status, 200);
t.is(bodyFiltered.body.pagination.total, 42);
} catch (error) {
console.error(error);
throw error;
}
},
);

test.serial(`${currentTest} authenticated rows: search never reaches a withheld column`, async (t) => {
try {
const testData = await createConnectionsAndInviteNewUserInNewGroupWithGroupPermissions(app);
const connectionId = testData.connections.firstId;
const groupId = testData.groups.createdGroupId;
const tableName = testData.firstTableInfo.testTableName;
const hiddenColumn = testData.firstTableInfo.testTableColumnName;
const allowedColumn = testData.firstTableInfo.testTableSecondColumnName;

const savePolicyResponse = await request(app.getHttpServer())
.post(`/connection/cedar-policy/${connectionId}`)
.send({ cedarPolicy: readOnlyCedarPolicy(connectionId, tableName, ['id', allowedColumn]), groupId })
.set('Cookie', testData.users.adminUserToken)
.set('Content-Type', 'application/json')
.set('Accept', 'application/json');
t.is(savePolicyResponse.status, 201);

// 'Vasia' exists only in the withheld column: the search must match nothing rather than
// returning those 3 rows (before the fix, search ILIKEd every text column of the table).
const searched = await request(app.getHttpServer())
.get(`/table/rows/${connectionId}?tableName=${tableName}&page=1&perPage=10&search=Vasia`)
.set('Cookie', testData.users.simpleUserToken)
.set('Content-Type', 'application/json')
.set('Accept', 'application/json');
t.is(searched.status, 200);
t.is(searched.body.rows.length, 0);
t.not(searched.body.pagination.total, 3);
Comment on lines +342 to +344

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert that the search total is zero.

t.not(searched.body.pagination.total, 3) passes for any non-three total. The test states that the search must match nothing, so assert pagination.total === 0.

Proposed test correction
-		t.not(searched.body.pagination.total, 3);
+		t.is(searched.body.pagination.total, 0);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
t.is(searched.status, 200);
t.is(searched.body.rows.length, 0);
t.not(searched.body.pagination.total, 3);
t.is(searched.status, 200);
t.is(searched.body.rows.length, 0);
t.is(searched.body.pagination.total, 0);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/test/ava-tests/non-saas-tests/non-saas-cedar-save-policy-e2e.test.ts`
around lines 342 - 344, Update the assertions in the search test to require
searched.body.pagination.total to equal zero, replacing the non-specific
inequality check while preserving the existing status and rows assertions.

} catch (error) {
console.error(error);
throw error;
}
});

test.serial(`${currentTest} authenticated rows: filter and search on a READABLE column still work`, async (t) => {
try {
const testData = await createConnectionsAndInviteNewUserInNewGroupWithGroupPermissions(app);
const connectionId = testData.connections.firstId;
const groupId = testData.groups.createdGroupId;
const tableName = testData.firstTableInfo.testTableName;
const allowedColumn = testData.firstTableInfo.testTableColumnName;

const savePolicyResponse = await request(app.getHttpServer())
.post(`/connection/cedar-policy/${connectionId}`)
.send({ cedarPolicy: readOnlyCedarPolicy(connectionId, tableName, ['id', allowedColumn]), groupId })
.set('Cookie', testData.users.adminUserToken)
.set('Content-Type', 'application/json')
.set('Accept', 'application/json');
t.is(savePolicyResponse.status, 201);

const filtered = await request(app.getHttpServer())
.get(`/table/rows/${connectionId}?tableName=${tableName}&page=1&perPage=10&f_${allowedColumn}__eq=Vasia`)
.set('Cookie', testData.users.simpleUserToken)
.set('Content-Type', 'application/json')
.set('Accept', 'application/json');
t.is(filtered.status, 200);
t.is(filtered.body.pagination.total, 3);
t.is(filtered.body.rows.length, 3);

const searched = await request(app.getHttpServer())
.get(`/table/rows/${connectionId}?tableName=${tableName}&page=1&perPage=10&search=Vasia`)
.set('Cookie', testData.users.simpleUserToken)
.set('Content-Type', 'application/json')
.set('Accept', 'application/json');
t.is(searched.status, 200);
t.is(searched.body.rows.length, 3);
} catch (error) {
console.error(error);
throw error;
}
});

test.serial(`${currentTest} authenticated reads fail CLOSED when no column is readable at all`, async (t) => {
try {
const testData = await createConnectionsAndInviteNewUserInNewGroupWithGroupPermissions(app);
const connectionId = testData.connections.firstId;
const groupId = testData.groups.createdGroupId;
const tableName = testData.firstTableInfo.testTableName;

// table:query but NOT a single column:read. Answering "rows with every column stripped"
// would still hand back a real pagination.total to mine, so this is a 403.
const savePolicyResponse = await request(app.getHttpServer())
.post(`/connection/cedar-policy/${connectionId}`)
.send({ cedarPolicy: readOnlyCedarPolicy(connectionId, tableName, []), groupId })
.set('Cookie', testData.users.adminUserToken)
.set('Content-Type', 'application/json')
.set('Accept', 'application/json');
t.is(savePolicyResponse.status, 201);

const getRows = await request(app.getHttpServer())
.get(`/table/rows/${connectionId}?tableName=${tableName}&page=1&perPage=10`)
.set('Cookie', testData.users.simpleUserToken)
.set('Content-Type', 'application/json')
.set('Accept', 'application/json');
t.is(getRows.status, 403);

const getRow = await request(app.getHttpServer())
.get(`/table/row/${connectionId}?tableName=${tableName}&id=1`)
.set('Cookie', testData.users.simpleUserToken)
.set('Content-Type', 'application/json')
.set('Accept', 'application/json');
t.is(getRow.status, 403);
} catch (error) {
console.error(error);
throw error;
}
});

test.serial(
`${currentTest} should enforce QueryTable - user without table:query is denied before the query`,
async (t) => {
Expand Down
Loading