feat: implement public access permissions for unauthenticated users#1844
Merged
Conversation
- Added `public_cedar_policy` field to the ConnectionEntity to store public access policies. - Introduced `getReadableColumnsForPublic` method in CedarPermissionsService to evaluate public access permissions. - Created `generatePublicCedarPolicy` function to generate policies for public access based on specified tables and columns. - Developed DTOs for public permissions configuration, including `SetPublicPermissionsDto` and `PublicPermissionsResponseDto`. - Updated guards to handle public access requests, ensuring only allowed actions (table:query and column:read) are permitted for unauthenticated users. - Implemented tests to validate public access functionality, including permission setting and retrieval. - Added migration to introduce `public_cedar_policy` column in the database.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds unauthenticated public read access to table CRUD endpoints. A new ChangesPublic Access for Table CRUD Endpoints
Sequence DiagramssequenceDiagram
participant Client
participant PublicOrAuthMiddleware
participant TableGuard
participant CedarAuthorizationService
participant PureGetRowsFromTableUseCase
participant CedarPermissionsService
participant ConnectionRepository
Client->>PublicOrAuthMiddleware: GET /table/crud/rows/:connectionId (no credentials)
PublicOrAuthMiddleware->>Client: req.decoded = {} (pass through)
Client->>TableGuard: canActivate (cognitoUserName absent)
TableGuard->>CedarAuthorizationService: isPublicAccessEnabled(connectionId)
CedarAuthorizationService->>ConnectionRepository: getConnectionPublicCedarPolicy(connectionId)
ConnectionRepository-->>CedarAuthorizationService: policyText
CedarAuthorizationService-->>TableGuard: enabled: true
TableGuard->>CedarAuthorizationService: validate({PUBLIC_USER_ID, TableQuery, publicAccess:true})
CedarAuthorizationService-->>TableGuard: allowed: true
TableGuard-->>Client: canActivate = true
Client->>PureGetRowsFromTableUseCase: execute(userId=undefined, connectionId, ...)
PureGetRowsFromTableUseCase->>CedarPermissionsService: getReadableColumnsForPublic(connectionId, tableName, allColumns)
CedarPermissionsService-->>PureGetRowsFromTableUseCase: Set<string> readableColumns
PureGetRowsFromTableUseCase->>PureGetRowsFromTableUseCase: filterRowsByReadableColumns(rows, readableColumns)
PureGetRowsFromTableUseCase-->>Client: filtered rows
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Implements configurable public (unauthenticated) read access to connection tables/columns by storing a generated Cedar policy on the connection and teaching the CRUD read path to authenticate optionally and enforce column-level visibility.
Changes:
- Add
public_cedar_policystorage onConnectionEntity+ migration, repository accessors, and CRUD endpoints to GET/PUT public permissions. - Allow unauthenticated requests onto pure-CRUD routes via
PublicOrAuthMiddleware, with guards evaluating the connection’s public policy fortable:query. - Enforce public column-level visibility by filtering returned row data using
CedarPermissionsService.getReadableColumnsForPublic, with added AVA tests.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/test/ava-tests/non-saas-tests/non-saas-table-pure-crud-operations-e2e.test.ts | Adds E2E coverage for public querying, column stripping, and write refusal. |
| backend/test/ava-tests/non-saas-tests/non-saas-cedar-policy-generator.test.ts | Adds unit tests for generated public Cedar policy output. |
| backend/src/migrations/1781536092947-AddPublicCedarPolicyToConnection.ts | Adds DB column public_cedar_policy on connection. |
| backend/src/guards/table-edit.guard.ts | Updates guard to account for unauthenticated requests (public write denial path). |
| backend/src/guards/table-delete.guard.ts | Updates guard to account for unauthenticated requests (public write denial path). |
| backend/src/guards/table-add.guard.ts | Updates guard to account for unauthenticated requests (public write denial path). |
| backend/src/guards/query-table.guard.ts | Allows unauthenticated table querying when connection public policy permits it. |
| backend/src/exceptions/text/messages.ts | Adds messages for public-policy validation errors. |
| backend/src/entities/table/utils/validate-connection.util.ts | Allows agent-email lookup to tolerate missing userId for public requests. |
| backend/src/entities/table/table-pure-crud-operations/use-cases/pure-read-row-from-table.use.case.ts | Filters single-row output to readable columns (public vs authenticated). |
| backend/src/entities/table/table-pure-crud-operations/use-cases/pure-get-rows-from-table.use.case.ts | Filters multi-row output to readable columns (public vs authenticated). |
| backend/src/entities/table/table-pure-crud-operations/table-pure-crud-operations.module.ts | Applies PublicOrAuthMiddleware to pure CRUD routes. |
| backend/src/entities/table/table-pure-crud-operations/table-pure-crud-operations.controller.ts | Uses @OptionalUserId() for read routes to support public access. |
| backend/src/entities/table/table-pure-crud-operations/application/data-structures/pure-read-row.ds.ts | Makes userId optional for public reads. |
| backend/src/entities/table/table-pure-crud-operations/application/data-structures/pure-get-rows.ds.ts | Makes userId optional for public reads. |
| backend/src/entities/connection/repository/custom-connection-repository-extension.ts | Adds get/update methods for public_cedar_policy. |
| backend/src/entities/connection/repository/connection.repository.interface.ts | Extends interface with public-policy accessors. |
| backend/src/entities/connection/connection.entity.ts | Adds public_cedar_policy column mapping. |
| backend/src/entities/cedar-authorization/dto/public-permissions.dto.ts | Introduces DTOs for configuring/returning public permissions. |
| backend/src/entities/cedar-authorization/cedar-policy-generator.ts | Adds generatePublicCedarPolicy() for public access policies. |
| backend/src/entities/cedar-authorization/cedar-permissions.service.ts | Adds getReadableColumnsForPublic() + public policy loading/caching. |
| backend/src/entities/cedar-authorization/cedar-authorization.service.ts | Adds public-policy validation, storage, and public authorization evaluation. |
| backend/src/entities/cedar-authorization/cedar-authorization.module.ts | Registers new public-permissions routes under auth middleware. |
| backend/src/entities/cedar-authorization/cedar-authorization.controller.ts | Adds GET/PUT endpoints for public permissions. |
| backend/src/entities/cedar-authorization/cedar-action-map.ts | Adds PUBLIC_USER_ID and publicAccess flag to validation request. |
| backend/src/decorators/optional-user-id.decorator.ts | Adds decorator to allow missing user id for public routes. |
| backend/src/authorization/public-or-auth.middleware.ts | Adds middleware that authenticates if present, otherwise allows public-through. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+14
to
+15
| if (!table.tableName) continue; | ||
| const tableRef = `RocketAdmin::Table::"${connectionId}/${table.tableName}"`; |
| const readableColumns = table.readableColumns; | ||
| if (readableColumns && readableColumns.length > 0) { | ||
| for (const columnName of readableColumns) { | ||
| const columnRef = `RocketAdmin::Column::"${connectionId}/${table.tableName}/${columnName}"`; |
Comment on lines
+30
to
+51
| // Public requests can never write: refuse outright when unauthenticated. The public | ||
| // policy may only grant QueryTable + ColumnRead, so TableAdd is never permitted. | ||
| if (!cognitoUserName) { | ||
| const publicEnabled = await this.cedarAuthService.isPublicAccessEnabled(connectionId); | ||
| if (!publicEnabled) { | ||
| reject(new ForbiddenException(Messages.DONT_HAVE_PERMISSIONS)); | ||
| return; | ||
| } | ||
| const allowedPublic = await this.cedarAuthService.validate({ | ||
| userId: PUBLIC_USER_ID, | ||
| action: CedarAction.TableAdd, | ||
| connectionId, | ||
| tableName, | ||
| publicAccess: true, | ||
| }); | ||
| if (allowedPublic) { | ||
| resolve(true); | ||
| return; | ||
| } | ||
| reject(new ForbiddenException(Messages.DONT_HAVE_PERMISSIONS)); | ||
| return; | ||
| } |
Comment on lines
+30
to
+51
| // Public requests can never write: refuse outright when unauthenticated. The public | ||
| // policy may only grant QueryTable + ColumnRead, so TableEdit is never permitted. | ||
| if (!cognitoUserName) { | ||
| const publicEnabled = await this.cedarAuthService.isPublicAccessEnabled(connectionId); | ||
| if (!publicEnabled) { | ||
| reject(new ForbiddenException(Messages.DONT_HAVE_PERMISSIONS)); | ||
| return; | ||
| } | ||
| const allowedPublic = await this.cedarAuthService.validate({ | ||
| userId: PUBLIC_USER_ID, | ||
| action: CedarAction.TableEdit, | ||
| connectionId, | ||
| tableName, | ||
| publicAccess: true, | ||
| }); | ||
| if (allowedPublic) { | ||
| resolve(true); | ||
| return; | ||
| } | ||
| reject(new ForbiddenException(Messages.DONT_HAVE_PERMISSIONS)); | ||
| return; | ||
| } |
Comment on lines
+30
to
+51
| // Public requests can never write: refuse outright when unauthenticated. The public | ||
| // policy may only grant QueryTable + ColumnRead, so TableDelete is never permitted. | ||
| if (!cognitoUserName) { | ||
| const publicEnabled = await this.cedarAuthService.isPublicAccessEnabled(connectionId); | ||
| if (!publicEnabled) { | ||
| reject(new ForbiddenException(Messages.DONT_HAVE_PERMISSIONS)); | ||
| return; | ||
| } | ||
| const allowedPublic = await this.cedarAuthService.validate({ | ||
| userId: PUBLIC_USER_ID, | ||
| action: CedarAction.TableDelete, | ||
| connectionId, | ||
| tableName, | ||
| publicAccess: true, | ||
| }); | ||
| if (allowedPublic) { | ||
| resolve(true); | ||
| return; | ||
| } | ||
| reject(new ForbiddenException(Messages.DONT_HAVE_PERMISSIONS)); | ||
| return; | ||
| } |
| * request continues. Downstream guards then decide whether the connection's public policy grants | ||
| * access. An invalid/expired credential still fails fast. | ||
| * | ||
| * This is applied only to read-capable pure CRUD routes; write routes keep AuthWithApiMiddleware. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
backend/src/entities/cedar-authorization/cedar-permissions.service.ts (1)
626-644: 💤 Low valueDuplicate
loadPublicPolicylogic across services.Both
CedarPermissionsService.loadPublicPolicyandCedarAuthorizationService.loadPublicPolicycontain identical caching and repository lookup logic. Consider extracting this to a shared utility or having one service delegate to the other to reduce duplication and maintenance burden.🤖 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/cedar-authorization/cedar-permissions.service.ts` around lines 626 - 644, The loadPublicPolicy method with its caching and repository lookup logic is duplicated between CedarPermissionsService and CedarAuthorizationService. Extract this logic into a shared utility method or service that both can delegate to. The method should encapsulate the Cacher.getCedarPolicyCache check, the getConnectionPublicCedarPolicy repository call, and the Cacher.setCedarPolicyCache call, along with the trim and length validation logic. This eliminates duplication and ensures consistent behavior across both services.backend/src/guards/table-add.guard.ts (1)
29-51: 💤 Low valueRedundant public-access validation in write guards. All three write guards (
TableAddGuard,TableDeleteGuard,TableEditGuard) perform async calls to check public access and validate against Cedar policy for actions that public access can never grant. SincevalidatePubliconly handlesTableQuery/TableReadandColumnRead, these validations always returnfalse. The pattern is defense-in-depth but adds latency for unauthenticated write attempts.
backend/src/guards/table-add.guard.ts#L29-L51: Simplify to immediately reject when!cognitoUserNameinstead of callingisPublicAccessEnabledandvalidate.backend/src/guards/table-delete.guard.ts#L29-L51: Same simplification - immediately reject unauthenticated requests.backend/src/guards/table-edit.guard.ts#L29-L51: Same simplification - immediately reject unauthenticated requests.🤖 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/guards/table-add.guard.ts` around lines 29 - 51, The write guards are performing redundant async validation checks for public access on write operations that public access can never permit, adding unnecessary latency. In backend/src/guards/table-add.guard.ts (lines 29-51), remove the async calls to isPublicAccessEnabled and cedarAuthService.validate when !cognitoUserName exists, and instead immediately reject with ForbiddenException since TableAdd is never permitted for public access. Apply the same simplification to backend/src/guards/table-delete.guard.ts (lines 29-51) for TableDelete action and backend/src/guards/table-edit.guard.ts (lines 29-51) for TableEdit action - in all three files, when an unauthenticated request is detected, reject it immediately without attempting any Cedar policy validation for those write operations.backend/src/entities/cedar-authorization/dto/public-permissions.dto.ts (1)
24-25: ⚡ Quick winReplace string concatenation with a single template literal.
Use one template literal for
descriptioninstead of+concatenation to match project style.Suggested refactor
`@ApiProperty`({ - description: - 'Tables exposed to unauthenticated (public) users. Each entry grants QueryTable and ColumnRead. ' + - 'Pass an empty array to disable public access for the connection.', + description: `Tables exposed to unauthenticated (public) users. Each entry grants QueryTable and ColumnRead. Pass an empty array to disable public access for the connection.`, type: [PublicTablePermissionDto], })As per coding guidelines, “Use template literals instead of string concatenation.”
🤖 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/cedar-authorization/dto/public-permissions.dto.ts` around lines 24 - 25, The description property in the public-permissions.dto.ts file uses string concatenation with the + operator across multiple lines. Replace this concatenation with a single template literal (using backticks) to combine the two string parts into one continuous string, which aligns with the project's coding guidelines that prefer template literals over concatenation.Source: Coding guidelines
backend/src/entities/cedar-authorization/cedar-policy-generator.ts (1)
10-10: ⚡ Quick winUse an arrow function export for guideline compliance.
Switching this declaration to
export const ... = (...) => ...aligns with the repository TS/JS style rule.Suggested refactor
-export function generatePublicCedarPolicy(connectionId: string, tables: Array<IPublicTablePermission>): string { +export const generatePublicCedarPolicy = ( + connectionId: string, + tables: Array<IPublicTablePermission>, +): string => { ... -} +};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/src/entities/cedar-authorization/cedar-policy-generator.ts` at line 10, The function generatePublicCedarPolicy is declared using the traditional function declaration syntax with export function, but the repository style guidelines prefer arrow function syntax. Convert this to use export const generatePublicCedarPolicy = (connectionId: string, tables: Array<IPublicTablePermission>): string => { ... } to align with the coding standards.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/authorization/public-or-auth.middleware.ts`:
- Around line 34-35: The docblock for the PublicOrAuthMiddleware (around line
34) incorrectly states that write routes keep AuthWithApiMiddleware, but this
middleware is actually applied to write routes as well. Update the comment to
accurately reflect the actual scope of where this middleware is applied -
clarify that it is used on both read and write routes in the pure CRUD
operations, not just read-capable routes, to prevent misleading future
maintainers.
- Around line 59-65: In the catch block of the public-or-auth.middleware.ts
file, add explicit handling for JWT verification errors before the final
InternalServerErrorException throw. After checking for HttpException and
UnauthorizedException, add a condition to detect JsonWebTokenError,
TokenExpiredError, and NotBeforeError instances and throw UnauthorizedException
instead of InternalServerErrorException. This ensures that invalid or expired
JWT tokens return a 401 Unauthorized response rather than a 500 Internal Server
Error.
In `@backend/src/entities/cedar-authorization/cedar-authorization.service.ts`:
- Around line 423-435: The validatePublicPolicyActions method's regex pattern
only extracts actions from the `action == RocketAdmin::Action::"..."` syntax but
fails to capture actions from the `action in [...]` syntax, allowing
unauthorized actions to bypass validation. Update the regex extraction logic to
match both patterns: modify the existing pattern or add an additional regex to
also capture actions from the `action in [...]` format where multiple actions
can be listed. Extract all matched actions from both syntaxes and add them to
the actions array so that the subsequent for loop validation against the allowed
Set covers all action specifications in the policy text.
In `@backend/src/entities/cedar-authorization/cedar-policy-generator.ts`:
- Around line 15-31: The table.tableName and columnName values are being
directly interpolated into Cedar policy resource identifiers without escaping
special characters. Create or use an escaping function to properly escape these
identifier parts before they are interpolated into the Cedar policy strings.
Apply this escaping to all instances where table.tableName is used in the
tableRef variable construction and where columnName is used in the columnRef
variable construction to prevent quotes or backslashes in these values from
breaking the policy syntax.
In
`@backend/src/entities/connection/repository/custom-connection-repository-extension.ts`:
- Around line 211-217: The updateConnectionPublicCedarPolicy method does not
verify whether the update query actually affected any rows, allowing it to
silently succeed even when the connectionId does not exist. Capture the result
returned by the .execute() call on the query builder, check the affected rows
count (accessed via the result object's affected property), and throw an
appropriate error if no rows were affected, indicating that the connection was
not found.
---
Nitpick comments:
In `@backend/src/entities/cedar-authorization/cedar-permissions.service.ts`:
- Around line 626-644: The loadPublicPolicy method with its caching and
repository lookup logic is duplicated between CedarPermissionsService and
CedarAuthorizationService. Extract this logic into a shared utility method or
service that both can delegate to. The method should encapsulate the
Cacher.getCedarPolicyCache check, the getConnectionPublicCedarPolicy repository
call, and the Cacher.setCedarPolicyCache call, along with the trim and length
validation logic. This eliminates duplication and ensures consistent behavior
across both services.
In `@backend/src/entities/cedar-authorization/cedar-policy-generator.ts`:
- Line 10: The function generatePublicCedarPolicy is declared using the
traditional function declaration syntax with export function, but the repository
style guidelines prefer arrow function syntax. Convert this to use export const
generatePublicCedarPolicy = (connectionId: string, tables:
Array<IPublicTablePermission>): string => { ... } to align with the coding
standards.
In `@backend/src/entities/cedar-authorization/dto/public-permissions.dto.ts`:
- Around line 24-25: The description property in the public-permissions.dto.ts
file uses string concatenation with the + operator across multiple lines.
Replace this concatenation with a single template literal (using backticks) to
combine the two string parts into one continuous string, which aligns with the
project's coding guidelines that prefer template literals over concatenation.
In `@backend/src/guards/table-add.guard.ts`:
- Around line 29-51: The write guards are performing redundant async validation
checks for public access on write operations that public access can never
permit, adding unnecessary latency. In backend/src/guards/table-add.guard.ts
(lines 29-51), remove the async calls to isPublicAccessEnabled and
cedarAuthService.validate when !cognitoUserName exists, and instead immediately
reject with ForbiddenException since TableAdd is never permitted for public
access. Apply the same simplification to
backend/src/guards/table-delete.guard.ts (lines 29-51) for TableDelete action
and backend/src/guards/table-edit.guard.ts (lines 29-51) for TableEdit action -
in all three files, when an unauthenticated request is detected, reject it
immediately without attempting any Cedar policy validation for those write
operations.
🪄 Autofix (Beta)
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
Run ID: f6adc6c9-5dd3-402f-ac24-0f3694b8e29d
📒 Files selected for processing (27)
backend/src/authorization/public-or-auth.middleware.tsbackend/src/decorators/optional-user-id.decorator.tsbackend/src/entities/cedar-authorization/cedar-action-map.tsbackend/src/entities/cedar-authorization/cedar-authorization.controller.tsbackend/src/entities/cedar-authorization/cedar-authorization.module.tsbackend/src/entities/cedar-authorization/cedar-authorization.service.tsbackend/src/entities/cedar-authorization/cedar-permissions.service.tsbackend/src/entities/cedar-authorization/cedar-policy-generator.tsbackend/src/entities/cedar-authorization/dto/public-permissions.dto.tsbackend/src/entities/connection/connection.entity.tsbackend/src/entities/connection/repository/connection.repository.interface.tsbackend/src/entities/connection/repository/custom-connection-repository-extension.tsbackend/src/entities/table/table-pure-crud-operations/application/data-structures/pure-get-rows.ds.tsbackend/src/entities/table/table-pure-crud-operations/application/data-structures/pure-read-row.ds.tsbackend/src/entities/table/table-pure-crud-operations/table-pure-crud-operations.controller.tsbackend/src/entities/table/table-pure-crud-operations/table-pure-crud-operations.module.tsbackend/src/entities/table/table-pure-crud-operations/use-cases/pure-get-rows-from-table.use.case.tsbackend/src/entities/table/table-pure-crud-operations/use-cases/pure-read-row-from-table.use.case.tsbackend/src/entities/table/utils/validate-connection.util.tsbackend/src/exceptions/text/messages.tsbackend/src/guards/query-table.guard.tsbackend/src/guards/table-add.guard.tsbackend/src/guards/table-delete.guard.tsbackend/src/guards/table-edit.guard.tsbackend/src/migrations/1781536092947-AddPublicCedarPolicyToConnection.tsbackend/test/ava-tests/non-saas-tests/non-saas-cedar-policy-generator.test.tsbackend/test/ava-tests/non-saas-tests/non-saas-table-pure-crud-operations-e2e.test.ts
Comment on lines
+34
to
+35
| * This is applied only to read-capable pure CRUD routes; write routes keep AuthWithApiMiddleware. | ||
| */ |
There was a problem hiding this comment.
Correct the middleware scope docblock.
Line 34 says write routes keep AuthWithApiMiddleware, but this middleware is wired to write routes too (backend/src/entities/table/table-pure-crud-operations/table-pure-crud-operations.module.ts, Lines 71-85). Please align the comment to avoid misleading maintenance guidance.
🤖 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/authorization/public-or-auth.middleware.ts` around lines 34 - 35,
The docblock for the PublicOrAuthMiddleware (around line 34) incorrectly states
that write routes keep AuthWithApiMiddleware, but this middleware is actually
applied to write routes as well. Update the comment to accurately reflect the
actual scope of where this middleware is applied - clarify that it is used on
both read and write routes in the pure CRUD operations, not just read-capable
routes, to prevent misleading future maintainers.
Comment on lines
+59
to
+65
| } catch (error) { | ||
| Sentry.captureException(error); | ||
| if (error instanceof HttpException || error instanceof UnauthorizedException) { | ||
| throw error; | ||
| } | ||
| throw new InternalServerErrorException(Messages.AUTHORIZATION_REJECTED); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "public-or-auth.middleware.ts" | head -5Repository: rocket-admin/rocketadmin
Length of output: 125
🏁 Script executed:
cat -n backend/src/authorization/public-or-auth.middleware.tsRepository: rocket-admin/rocketadmin
Length of output: 5413
🏁 Script executed:
# Check jsonwebtoken package version and available error types
cat package.json | grep -A 5 '"jsonwebtoken"'Repository: rocket-admin/rocketadmin
Length of output: 50
🏁 Script executed:
# Search for jwt imports and usage in the middleware file
rg "import.*jwt|jsonwebtoken" backend/src/authorization/public-or-auth.middleware.ts -A 2 -B 2Repository: rocket-admin/rocketadmin
Length of output: 444
🏁 Script executed:
# Get lines around the catch block to see the actual code
sed -n '50,70p' backend/src/authorization/public-or-auth.middleware.ts | cat -nRepository: rocket-admin/rocketadmin
Length of output: 847
🏁 Script executed:
# Check if TwoFaRequiredException extends HttpException
rg "class TwoFaRequiredException" -A 5Repository: rocket-admin/rocketadmin
Length of output: 703
🏁 Script executed:
# Check the module wiring to see which routes use PublicOrAuthMiddleware
find . -type f -name "table-pure-crud-operations.module.ts" -exec cat -n {} \;Repository: rocket-admin/rocketadmin
Length of output: 4347
🏁 Script executed:
# Check BaseRocketAdminException to see if it extends HttpException
rg "class BaseRocketAdminException" -A 10Repository: rocket-admin/rocketadmin
Length of output: 1306
🏁 Script executed:
# Check how jwt errors are typically imported and used in Node/jsonwebtoken
web_search
jsonwebtoken error types JsonWebTokenError TokenExpiredError NotBeforeError importRepository: rocket-admin/rocketadmin
Length of output: 170
🌐 Web query:
jsonwebtoken error types JsonWebTokenError TokenExpiredError NotBeforeError
💡 Result:
The jsonwebtoken library uses three primary error classes to handle specific failures during the token verification process [1][2]. These errors are returned as the first argument in an asynchronous callback or thrown synchronously when no callback is provided [2][3]. TokenExpiredError This error is thrown when the token's exp (expiration) claim indicates that the token is no longer valid [1][2]. - Name: TokenExpiredError [1][2] - Message: 'jwt expired' [3] - Additional Property: expiredAt (the date when the token expired) [3] NotBeforeError This error is thrown when the current time is before the time specified in the nbf (not before) claim [1][3]. - Name: NotBeforeError [1][2] - Message: 'jwt not active' [1][3] - Additional Property: date (the date before which the token was not active) [1][3] JsonWebTokenError This is a general error class for various issues encountered while parsing or validating a token [1][2]. Common messages include [1][3]: - 'invalid token': The header or payload could not be parsed. - 'jwt malformed': The token does not consist of three components delimited by a dot (.). - 'jwt signature is required' - 'invalid signature' - 'jwt audience invalid', 'jwt issuer invalid', 'jwt id invalid', 'jwt subject invalid': Thrown when claims do not match the options provided to verify [1][3]. Note: When working with untrusted input, always use jwt.verify rather than jwt.decode, as the latter does not perform signature verification [4]. Additionally, jsonwebtoken depends on Node.js-specific modules (like crypto) and may not be compatible with edge runtimes or environments lacking these APIs [5].
Citations:
- 1: https://www.npmjs.com/package/jsonwebtoken
- 2: https://github.com/auth0/node-jsonwebtoken/
- 3: https://github.com/auth0/node-jsonwebtoken/blob/bc28861f/README.md
- 4: https://dev.to/chandan_karn_fb750e731394/3-broken-auth-patterns-cursor-keeps-writing-into-your-api-119o
- 5: https://dev.to/jtorchia/nextjs-16-middleware-authorization-patterns-that-scale-and-the-ones-that-cause-race-conditions-4pfk
🏁 Script executed:
# Check how jwt.verify errors are caught in actual code or if there are other tests/examples
rg "jwt.verify" -B 5 -A 10 | head -50Repository: rocket-admin/rocketadmin
Length of output: 3787
Return 401 for invalid/expired JWTs instead of 500.
At line 64, non-HttpException errors are caught and converted to InternalServerErrorException. When jwt.verify() fails (invalid/expired token), it throws JsonWebTokenError, TokenExpiredError, or NotBeforeError—none of which are HttpException instances. These errors fall through to the 500 response instead of returning 401. Add explicit handling for JWT errors to throw UnauthorizedException:
Suggested fix
} catch (error) {
Sentry.captureException(error);
if (error instanceof HttpException || error instanceof UnauthorizedException) {
throw error;
}
+ if (
+ error instanceof jwt.JsonWebTokenError ||
+ error instanceof jwt.TokenExpiredError ||
+ error instanceof jwt.NotBeforeError
+ ) {
+ throw new UnauthorizedException('JWT verification failed');
+ }
throw new InternalServerErrorException(Messages.AUTHORIZATION_REJECTED);
}🤖 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/authorization/public-or-auth.middleware.ts` around lines 59 - 65,
In the catch block of the public-or-auth.middleware.ts file, add explicit
handling for JWT verification errors before the final
InternalServerErrorException throw. After checking for HttpException and
UnauthorizedException, add a condition to detect JsonWebTokenError,
TokenExpiredError, and NotBeforeError instances and throw UnauthorizedException
instead of InternalServerErrorException. This ensures that invalid or expired
JWT tokens return a 401 Unauthorized response rather than a 500 Internal Server
Error.
Comment on lines
+211
to
+217
| async updateConnectionPublicCedarPolicy(connectionId: string, publicCedarPolicy: string | null): Promise<void> { | ||
| await this.createQueryBuilder() | ||
| .update(ConnectionEntity) | ||
| .set({ public_cedar_policy: publicCedarPolicy }) | ||
| .where('id = :connectionId', { connectionId }) | ||
| .execute(); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the actual implementation of updateConnectionPublicCedarPolicy
cat -n backend/src/entities/connection/repository/custom-connection-repository-extension.ts | sed -n '200,225p'Repository: rocket-admin/rocketadmin
Length of output: 1325
🏁 Script executed:
# Check the complete file structure to understand the context
wc -l backend/src/entities/connection/repository/custom-connection-repository-extension.tsRepository: rocket-admin/rocketadmin
Length of output: 157
🏁 Script executed:
# Search for usages of updateConnectionPublicCedarPolicy
rg "updateConnectionPublicCedarPolicy" --type ts --type tsx -A 3 -B 1Repository: rocket-admin/rocketadmin
Length of output: 96
🏁 Script executed:
# Check how cedar-authorization.service.ts uses this method
cat -n backend/src/entities/cedar-authorization/cedar-authorization.service.ts | grep -A 10 -B 2 "updateConnectionPublicCedarPolicy"Repository: rocket-admin/rocketadmin
Length of output: 892
🏁 Script executed:
# Look for similar update methods in the repository to see the pattern used elsewhere
ast-grep --pattern $'async $_($$$) {
const result = await this.createQueryBuilder()
.update($_)
$$$
.execute();
if ($_) {
$$$
}
}' | head -50Repository: rocket-admin/rocketadmin
Length of output: 50
🏁 Script executed:
# Search for other .execute() calls in repository files to see how they handle results
rg "\.execute\(\)" backend/src/entities --type ts -B 3 -A 3 | head -100Repository: rocket-admin/rocketadmin
Length of output: 10691
Check affected rows and throw error when connection is not found.
The update query at line 216 ignores the result, so a nonexistent connectionId silently fails without error while callers receive success. Line 406 in cedar-authorization.service.ts calls this method and immediately returns success without knowing if the update actually succeeded, creating a data integrity risk.
Suggested fix
async updateConnectionPublicCedarPolicy(connectionId: string, publicCedarPolicy: string | null): Promise<void> {
- await this.createQueryBuilder()
+ const result = await this.createQueryBuilder()
.update(ConnectionEntity)
.set({ public_cedar_policy: publicCedarPolicy })
.where('id = :connectionId', { connectionId })
.execute();
+ if (!result.affected) {
+ throw new Error('Connection not found');
+ }
},📝 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
| async updateConnectionPublicCedarPolicy(connectionId: string, publicCedarPolicy: string | null): Promise<void> { | |
| await this.createQueryBuilder() | |
| .update(ConnectionEntity) | |
| .set({ public_cedar_policy: publicCedarPolicy }) | |
| .where('id = :connectionId', { connectionId }) | |
| .execute(); | |
| }, | |
| async updateConnectionPublicCedarPolicy(connectionId: string, publicCedarPolicy: string | null): Promise<void> { | |
| const result = await this.createQueryBuilder() | |
| .update(ConnectionEntity) | |
| .set({ public_cedar_policy: publicCedarPolicy }) | |
| .where('id = :connectionId', { connectionId }) | |
| .execute(); | |
| if (!result.affected) { | |
| throw new Error('Connection not found'); | |
| } | |
| }, |
🤖 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/connection/repository/custom-connection-repository-extension.ts`
around lines 211 - 217, The updateConnectionPublicCedarPolicy method does not
verify whether the update query actually affected any rows, allowing it to
silently succeed even when the connectionId does not exist. Capture the result
returned by the .execute() call on the query builder, check the affected rows
count (accessed via the result object's affected property), and throw an
appropriate error if no rows were affected, indicating that the connection was
not found.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/test/ava-tests/non-saas-tests/non-saas-table-pure-crud-operations-e2e.test.ts (1)
672-675: ⚡ Quick winStrengthen the invalid-JWT invariant to assert an auth failure class, not just “not 200”.
At Line 674,
t.not(res.status, 200)can pass on unrelated server errors (e.g., 500), which weakens this security test. Assert an explicit auth-denial status (typically 401/403) so the test catches real regressions instead of incidental failures.Suggested change
- // A malformed credential must be rejected, never silently downgraded to public access. - // (The existing middleware surfaces this as a 4xx/5xx; the invariant is "not 200, no data leaked".) - t.not(res.status, 200); + // A malformed credential must be rejected, never silently downgraded to public access. + // Assert an explicit auth failure to avoid masking unrelated 5xx failures. + t.true([401, 403].includes(res.status)); t.is(Object.hasOwn(JSON.parse(res.text), 'rows'), false);🤖 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-table-pure-crud-operations-e2e.test.ts` around lines 672 - 675, The current assertion `t.not(res.status, 200)` in the test is too weak because it allows any non-200 status, including unrelated server errors like 500, which defeats the purpose of testing that a malformed JWT is properly rejected. Replace this assertion with an explicit check for auth-denial status codes (401 or 403) to ensure the test specifically validates that the endpoint returns an authentication failure rather than just any error. This ensures the test catches real regressions in JWT validation behavior instead of passing on incidental server failures.
🤖 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.
Nitpick comments:
In
`@backend/test/ava-tests/non-saas-tests/non-saas-table-pure-crud-operations-e2e.test.ts`:
- Around line 672-675: The current assertion `t.not(res.status, 200)` in the
test is too weak because it allows any non-200 status, including unrelated
server errors like 500, which defeats the purpose of testing that a malformed
JWT is properly rejected. Replace this assertion with an explicit check for
auth-denial status codes (401 or 403) to ensure the test specifically validates
that the endpoint returns an authentication failure rather than just any error.
This ensures the test catches real regressions in JWT validation behavior
instead of passing on incidental server failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ecfcba5-5258-498e-a831-4c7c227cfacd
📒 Files selected for processing (1)
backend/test/ava-tests/non-saas-tests/non-saas-table-pure-crud-operations-e2e.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/src/entities/cedar-authorization/cedar-policy-generator.ts (1)
10-21: ⚡ Quick winUse arrow helpers to match the TypeScript style rule.
These new local helpers are function declarations; converting them to
constarrow functions keeps the changed code aligned with the repository rule.As per coding guidelines, "Prefer arrow functions over function declarations."
Proposed refactor
-function escapeCedarString(value: string): string { +const escapeCedarString = (value: string): string => { return value .replace(/\\/g, '\\\\') .replace(/"/g, '\\"') .replace(/\n/g, '\\n') .replace(/\r/g, '\\r') .replace(/\t/g, '\\t'); -} +}; -function cedarEntityRef(entityType: string, id: string): string { +const cedarEntityRef = (entityType: string, id: string): string => { return `${entityType}::"${escapeCedarString(id)}"`; -} +};🤖 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/cedar-authorization/cedar-policy-generator.ts` around lines 10 - 21, The functions escapeCedarString and cedarEntityRef are declared using function declarations instead of arrow functions, which violates the TypeScript style rule that prefers arrow functions. Convert both function declarations to const arrow function syntax, maintaining their type signatures and implementation logic. Replace the function declaration syntax with const variable assignments using arrow function syntax for both escapeCedarString and cedarEntityRef.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/test/ava-tests/non-saas-tests/non-saas-cedar-policy-generator.test.ts`:
- Around line 378-394: The two test cases for generatePublicCedarPolicy claim to
test backslash escaping but the malicious inputs contain only literal `\n`
characters, not actual backslashes. Additionally, using `split('\n\n').length`
to verify no policy injection is insufficient since it can miss injected
`permit(` statements that aren't separated by blank lines. Add actual backslash
characters to both malicious inputs (in the table name test and the column name
test) and replace the blank-line-based detection with a more robust check that
directly searches for unauthorized `permit(` patterns in the result, regardless
of spacing, to properly validate that policy injection is prevented.
---
Nitpick comments:
In `@backend/src/entities/cedar-authorization/cedar-policy-generator.ts`:
- Around line 10-21: The functions escapeCedarString and cedarEntityRef are
declared using function declarations instead of arrow functions, which violates
the TypeScript style rule that prefers arrow functions. Convert both function
declarations to const arrow function syntax, maintaining their type signatures
and implementation logic. Replace the function declaration syntax with const
variable assignments using arrow function syntax for both escapeCedarString and
cedarEntityRef.
🪄 Autofix (Beta)
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
Run ID: 28e72fcb-3153-4953-92f9-5707e9f83ab4
📒 Files selected for processing (2)
backend/src/entities/cedar-authorization/cedar-policy-generator.tsbackend/test/ava-tests/non-saas-tests/non-saas-cedar-policy-generator.test.ts
Comment on lines
+378
to
+394
| test('generatePublicCedarPolicy escapes quotes/backslashes/newlines in table names (no policy injection)', (t) => { | ||
| const malicious = 'evil") ;\n\npermit(principal, action, resource);\n\n//'; | ||
| const result = generatePublicCedarPolicy(connectionId, [{ tableName: malicious }]); | ||
| t.is(result.split('\n\n').length, 2); | ||
| t.true(result.includes('\\"')); | ||
| t.true(result.includes('\\n')); | ||
| const actions = [...result.matchAll(/action\s*==\s*RocketAdmin::Action::"([^"]+)"/g)].map((m) => m[1]); | ||
| t.deepEqual(new Set(actions), new Set(['table:query', 'column:read'])); | ||
| }); | ||
|
|
||
| test('generatePublicCedarPolicy escapes quotes/backslashes/newlines in column names (no policy injection)', (t) => { | ||
| const maliciousColumn = 'c") ;\npermit(principal, action, resource); //'; | ||
| const result = generatePublicCedarPolicy(connectionId, [{ tableName: 'users', readableColumns: [maliciousColumn] }]); | ||
| t.is(result.split('\n\n').length, 2); | ||
| t.true(result.includes('\\"')); | ||
| const actions = [...result.matchAll(/action\s*==\s*RocketAdmin::Action::"([^"]+)"/g)].map((m) => m[1]); | ||
| t.deepEqual(new Set(actions), new Set(['table:query', 'column:read'])); |
There was a problem hiding this comment.
Strengthen the escaping tests for backslashes and injected permits.
These cases say they cover backslashes, but the malicious inputs do not contain one. Also, split('\n\n') can miss an injected permit( that is not separated by a blank line, especially in the column-name case.
Proposed test hardening
test('generatePublicCedarPolicy escapes quotes/backslashes/newlines in table names (no policy injection)', (t) => {
- const malicious = 'evil") ;\n\npermit(principal, action, resource);\n\n//';
+ const malicious = 'evil\\table") ;\n\npermit(principal, action, resource);\n\n//';
const result = generatePublicCedarPolicy(connectionId, [{ tableName: malicious }]);
- t.is(result.split('\n\n').length, 2);
+ t.is((result.match(/permit\(/g) ?? []).length, 2);
t.true(result.includes('\\"'));
+ t.true(result.includes('evil\\\\table'));
t.true(result.includes('\\n'));
const actions = [...result.matchAll(/action\s*==\s*RocketAdmin::Action::"([^"]+)"/g)].map((m) => m[1]);
t.deepEqual(new Set(actions), new Set(['table:query', 'column:read']));
});
test('generatePublicCedarPolicy escapes quotes/backslashes/newlines in column names (no policy injection)', (t) => {
- const maliciousColumn = 'c") ;\npermit(principal, action, resource); //';
+ const maliciousColumn = 'c\\name") ;\npermit(principal, action, resource); //';
const result = generatePublicCedarPolicy(connectionId, [{ tableName: 'users', readableColumns: [maliciousColumn] }]);
- t.is(result.split('\n\n').length, 2);
+ t.is((result.match(/permit\(/g) ?? []).length, 2);
t.true(result.includes('\\"'));
+ t.true(result.includes('c\\\\name'));
+ t.true(result.includes('\\n'));
const actions = [...result.matchAll(/action\s*==\s*RocketAdmin::Action::"([^"]+)"/g)].map((m) => m[1]);
t.deepEqual(new Set(actions), new Set(['table:query', 'column:read']));
});🤖 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-policy-generator.test.ts`
around lines 378 - 394, The two test cases for generatePublicCedarPolicy claim
to test backslash escaping but the malicious inputs contain only literal `\n`
characters, not actual backslashes. Additionally, using `split('\n\n').length`
to verify no policy injection is insufficient since it can miss injected
`permit(` statements that aren't separated by blank lines. Add actual backslash
characters to both malicious inputs (in the table name test and the column name
test) and replace the blank-line-based detection with a more robust check that
directly searches for unauthorized `permit(` patterns in the result, regardless
of spacing, to properly validate that policy injection is prevented.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
public_cedar_policyfield to the ConnectionEntity to store public access policies.getReadableColumnsForPublicmethod in CedarPermissionsService to evaluate public access permissions.generatePublicCedarPolicyfunction to generate policies for public access based on specified tables and columns.SetPublicPermissionsDtoandPublicPermissionsResponseDto.public_cedar_policycolumn in the database.Summary by CodeRabbit
New Features
GET/PUT /connection/public-permissions/:connectionIdto view and set public permissions (persisted server-side).Tests