Backend agent table public permissions - #1867
Conversation
📝 WalkthroughWalkthroughThe table read use cases now resolve readable columns before querying. DAO settings, filters, ordering, autocomplete, binary searches, response fields, and CSV searches exclude unreadable columns. End-to-end tests cover readable, withheld, and fully denied access. ChangesReadable Column Enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GetTableRowsUseCase
participant CedarPermissions
participant DAO
Client->>GetTableRowsUseCase: request rows with filters or search
GetTableRowsUseCase->>CedarPermissions: resolve readable columns
CedarPermissions-->>GetTableRowsUseCase: readable column list
GetTableRowsUseCase->>DAO: query with restricted table settings
DAO-->>GetTableRowsUseCase: restricted rows and fields
GetTableRowsUseCase-->>Client: filtered response
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
backend/test/ava-tests/non-saas-tests/non-saas-cedar-save-policy-e2e.test.ts (1)
261-270: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse an arrow function for the policy helper.
Replace
function readOnlyCedarPolicy(...)with aconstarrow function.As per coding guidelines, “Prefer arrow functions over function declarations”.
🤖 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 261 - 270, Change readOnlyCedarPolicy from a function declaration to a const-bound arrow function, preserving its parameters, return type, and existing policy-generation behavior.Source: Coding guidelines
backend/src/entities/table/use-cases/get-table-rows.use.case.ts (1)
177-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
as anywith the explicit binary search value type.
hexToBinary()returnsBuffer, butsearchingFieldValueis typed asstringinGetTableRowsDsandgetRowsFromTable()signatures. Type the request input, use-case mutation, and DAO parameter asstring | Bufferinstead of bypassing the contract withas any.🤖 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` at line 177, Replace the as any cast in the get-table-rows flow with an explicit string | Buffer contract: update the request input, the searchingFieldValue mutation in the use case, and the getRowsFromTable DAO parameter so hexToBinary() can return Buffer without bypassing types.Source: Coding guidelines
🤖 Prompt for all review comments with 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.
Inline comments:
In `@backend/src/entities/table/use-cases/get-row-by-primary-key.use.case.ts`:
- Around line 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.
In `@backend/src/entities/table/use-cases/get-table-rows.use.case.ts`:
- Around line 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.
- Around line 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.
In
`@backend/test/ava-tests/non-saas-tests/non-saas-cedar-save-policy-e2e.test.ts`:
- Around line 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.
---
Nitpick comments:
In `@backend/src/entities/table/use-cases/get-table-rows.use.case.ts`:
- Line 177: Replace the as any cast in the get-table-rows flow with an explicit
string | Buffer contract: update the request input, the searchingFieldValue
mutation in the use case, and the getRowsFromTable DAO parameter so
hexToBinary() can return Buffer without bypassing types.
In
`@backend/test/ava-tests/non-saas-tests/non-saas-cedar-save-policy-e2e.test.ts`:
- Around line 261-270: Change readOnlyCedarPolicy from a function declaration to
a const-bound arrow function, preserving its parameters, return type, and
existing policy-generation behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a96d651c-7161-4078-bd6c-984ae2f01880
📒 Files selected for processing (4)
backend/src/entities/table/use-cases/export-csv-from-table.use.case.tsbackend/src/entities/table/use-cases/get-row-by-primary-key.use.case.tsbackend/src/entities/table/use-cases/get-table-rows.use.case.tsbackend/test/ava-tests/non-saas-tests/non-saas-cedar-save-policy-e2e.test.ts
| // 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); |
There was a problem hiding this comment.
🔒 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.
| const autocompleteFields: AutocompleteFieldsDs = | ||
| autocomplete && referencedColumn | ||
| ? findAutocompleteFieldsUtil(query, tableStructure, tableSettings, referencedColumn) | ||
| ? findAutocompleteFieldsUtil(query, queryableStructure, tableSettings, referencedColumn) |
There was a problem hiding this comment.
🔒 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.
| if (connection.type === 'mongodb' || connection.type === 'agent_mongodb') { | ||
| builtDAOsTableSettings.search_fields.push('_id'); | ||
| } |
There was a problem hiding this comment.
🔒 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.
| t.is(searched.status, 200); | ||
| t.is(searched.body.rows.length, 0); | ||
| t.not(searched.body.pagination.total, 3); |
There was a problem hiding this comment.
🎯 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.
| 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.
There was a problem hiding this comment.
Pull request overview
This PR tightens column-level read permissions on the authenticated “table read” paths so that permissions bound the query itself (filters/search/ordering/select list), not just the returned rows—closing a pagination.total-based inference/oracle leak.
Changes:
- Adds AVA e2e coverage ensuring filters/search on withheld columns are ignored and that “no readable columns” fails closed (403).
- Updates
GetTableRowsUseCaseto compute readable columns early and parse filters/ordering/autocomplete against a readable-only structure, while restricting DAO settings viaexcluded_fields. - Updates
GetRowByPrimaryKeyUseCasesimilarly and fixesExportCSVFromTableUseCasebinary-search settings to actually affect the DAO query.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| backend/test/ava-tests/non-saas-tests/non-saas-cedar-save-policy-e2e.test.ts | Adds regression tests for withheld-column filter/search behavior and fail-closed behavior when no columns are readable. |
| backend/src/entities/table/use-cases/get-table-rows.use.case.ts | Restricts table-rows query inputs/settings to readable columns to prevent inference via filtering/search/ordering. |
| backend/src/entities/table/use-cases/get-row-by-primary-key.use.case.ts | Applies query-level restriction to readable columns before fetching a single row, with defense-in-depth response projection. |
| backend/src/entities/table/use-cases/export-csv-from-table.use.case.ts | Ensures binary search fields are applied to the actual DAO settings and aligns query restriction with readable columns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const daoTableSettings = { ...builtDAOsTableSettings }; | ||
| restrictTableSettingsToReadableColumns(daoTableSettings, readableColumns, allColumnNames); | ||
|
|
| // 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`. |
| 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. |
| // 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)) { |
Summary by CodeRabbit