Backend frontend agent#1848
Merged
Merged
Conversation
…mpany subscription metadata retrieval
…ing connection table names
|
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)
📝 WalkthroughWalkthroughThe PR adds two agents microservice endpoints— ChangesAgents Microservice: New Use-Cases and Endpoints
Public CRUD CORS Middleware
Sequence Diagram(s)sequenceDiagram
participant Client
participant AgentsController
participant GetAiConnectionTablesUseCase
participant setupAiConnection
participant CedarPermissionsService
Client->>AgentsController: POST /ai/data/:connectionId/tables
AgentsController->>GetAiConnectionTablesUseCase: execute({ connectionId, userId, masterPassword })
GetAiConnectionTablesUseCase->>setupAiConnection: create/access connection
setupAiConnection-->>GetAiConnectionTablesUseCase: dataAccessObject, foundConnection
GetAiConnectionTablesUseCase->>setupAiConnection: getTablesFromDB()
setupAiConnection-->>GetAiConnectionTablesUseCase: raw table names
loop for each table name
GetAiConnectionTablesUseCase->>CedarPermissionsService: improvedCheckTableRead(userId, connectionId, tableName)
CedarPermissionsService-->>GetAiConnectionTablesUseCase: allowed: boolean
end
GetAiConnectionTablesUseCase-->>AgentsController: { tables: readableTableNames }
AgentsController-->>Client: AiConnectionTablesRO
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 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
This PR extends the backend to support (1) wildcard CORS handling for the public /table/crud endpoints and (2) new “agents microservice” endpoints/use-cases for AI table discovery and company subscription metadata, aligning the backend API with agent-driven workflows.
Changes:
- Added an Express middleware to reflect arbitrary origins for
/table/crudroutes and short-circuit OPTIONS preflights. - Expanded the agents microservice with new endpoints/use-cases to list readable tables for a connection and to fetch company subscription metadata.
- Added/updated AVA e2e coverage to validate the new CORS behavior on
/table/crudroutes.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/test/ava-tests/non-saas-tests/non-saas-table-pure-crud-operations-e2e.test.ts | Registers the new CORS middleware in test app setup and adds e2e assertions for preflight + credentialed requests. |
| backend/src/middlewares/public-crud-cors.middleware.ts | Implements wildcard/reflected-origin CORS handling specifically for /table/crud (including OPTIONS short-circuit). |
| backend/src/main.ts | Registers the new CORS middleware before global enableCors() so /table/crud can accept arbitrary origins. |
| backend/src/common/data-injection.tokens.ts | Adds new DI tokens for the agents microservice use-cases. |
| backend/src/microservices/agents-microservice/agents.module.ts | Wires new use-cases into the agents module providers. |
| backend/src/microservices/agents-microservice/agents.controller.ts | Adds endpoints for connection table listing and company subscription info. |
| backend/src/microservices/agents-microservice/use-cases/get-ai-connection-tables.use.case.ts | New use-case to list tables and filter them by Cedar read permission. |
| backend/src/microservices/agents-microservice/use-cases/get-company-subscription-info.use.case.ts | New use-case to resolve subscription metadata via the SaaS gateway. |
| backend/src/microservices/agents-microservice/use-cases/agents-use-cases.interface.ts | Adds interfaces for the new use-cases. |
| backend/src/microservices/agents-microservice/dto/agents-company.dtos.ts | Adds DTO for subscription info request. |
| backend/src/microservices/agents-microservice/data-structures/agents.ds.ts | Adds DS for subscription info input. |
| backend/src/microservices/agents-microservice/data-structures/agents-responses.ds.ts | Adds response models for AI table list and subscription info. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+27
to
+35
| const { foundConnection, dataAccessObject } = await setupAiConnection( | ||
| this._dbContext, | ||
| connectionId, | ||
| masterPassword, | ||
| userId, | ||
| ); | ||
|
|
||
| const tables = await dataAccessObject.getTablesFromDB(); | ||
| const tableNames = tables.map((table) => table.tableName?.trim()).filter((name): name is string => Boolean(name)); |
Comment on lines
+37
to
+41
| const readableFlags = await Promise.all( | ||
| tableNames.map((tableName) => | ||
| this.cedarPermissions.improvedCheckTableRead(userId, foundConnection.id, tableName), | ||
| ), | ||
| ); |
Comment on lines
+56
to
+57
| @ApiPropertyOptional({ nullable: true, description: 'FREE_PLAN | TEAM_PLAN | ENTERPRISE_PLAN | ANNUAL_* | null' }) | ||
| subscriptionLevel: string | null; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/src/middlewares/public-crud-cors.middleware.ts (1)
24-24: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueHandle potential string array from header value.
The type assertion
as stringdoesn't account for the fact thatreq.headers['access-control-request-headers']could bestring[]if the header is sent multiple times. While uncommon for this specific header, defensive handling would be more robust.♻️ Proposed refactor to handle array values
res.header( 'Access-Control-Allow-Headers', - (req.headers['access-control-request-headers'] as string) ?? + (Array.isArray(req.headers['access-control-request-headers']) + ? req.headers['access-control-request-headers'][0] + : req.headers['access-control-request-headers']) ?? 'Content-Type, Authorization, x-api-key, masterpwd', );🤖 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/middlewares/public-crud-cors.middleware.ts` at line 24, The type assertion `as string` on the access-control-request-headers value does not account for the possibility that req.headers values can be either string or string[]. Modify the handling to check whether the value is an array and handle both cases appropriately, either by joining array elements with a comma or by selecting the appropriate value before using it as a string. This defensive approach will prevent potential runtime issues when the header is sent multiple times.
🤖 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/microservices/agents-microservice/use-cases/get-company-subscription-info.use.case.ts`:
- Around line 40-43: In the get-company-subscription-info.use.case.ts file,
locate the null check for the company variable returned from
this._dbContext.companyInfoRepository.findCompanyInfoByUserId(userId). The
current implementation throws HttpStatus.FORBIDDEN when company is not found,
but this should be changed to HttpStatus.NOT_FOUND since the error message is
Messages.COMPANY_NOT_FOUND, which represents missing data rather than an
authorization issue. Replace HttpStatus.FORBIDDEN with HttpStatus.NOT_FOUND in
the throw statement within the if (!company) block.
In `@backend/src/middlewares/public-crud-cors.middleware.ts`:
- Around line 22-26: The Access-Control-Allow-Headers response header in the
public-crud-cors.middleware.ts file reflects the access-control-request-headers
value from the request without sanitization, creating an HTTP Response Splitting
vulnerability. Sanitize the reflected header value by removing CRLF characters
(\r and \n) before using it in the res.header call. Apply a sanitization
function or string replacement to strip these characters from the
req.headers['access-control-request-headers'] value to prevent header injection
attacks.
---
Nitpick comments:
In `@backend/src/middlewares/public-crud-cors.middleware.ts`:
- Line 24: The type assertion `as string` on the access-control-request-headers
value does not account for the possibility that req.headers values can be either
string or string[]. Modify the handling to check whether the value is an array
and handle both cases appropriately, either by joining array elements with a
comma or by selecting the appropriate value before using it as a string. This
defensive approach will prevent potential runtime issues when the header is sent
multiple times.
🪄 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: f6172f7a-8af9-4b9d-ac5a-404367beb3b8
📒 Files selected for processing (12)
backend/src/common/data-injection.tokens.tsbackend/src/main.tsbackend/src/microservices/agents-microservice/agents.controller.tsbackend/src/microservices/agents-microservice/agents.module.tsbackend/src/microservices/agents-microservice/data-structures/agents-responses.ds.tsbackend/src/microservices/agents-microservice/data-structures/agents.ds.tsbackend/src/microservices/agents-microservice/dto/agents-company.dtos.tsbackend/src/microservices/agents-microservice/use-cases/agents-use-cases.interface.tsbackend/src/microservices/agents-microservice/use-cases/get-ai-connection-tables.use.case.tsbackend/src/microservices/agents-microservice/use-cases/get-company-subscription-info.use.case.tsbackend/src/middlewares/public-crud-cors.middleware.tsbackend/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
🤖 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-table-pure-crud-operations-e2e.test.ts`:
- Around line 750-762: The test callback parameters in the test.serial function
for "a malformed Origin is not reflected back (header-injection guard)" and the
other test at line 766 are missing required TypeScript type annotations. Add the
type annotation `ExecutionContext` to the `t` parameter and add the return type
`: Promise<void>` to the async callback function signature in both test.serial
calls to comply with TypeScript coding guidelines.
🪄 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: 0c692645-1b89-40c2-9808-d07636a0d680
📒 Files selected for processing (3)
backend/src/microservices/agents-microservice/use-cases/get-company-subscription-info.use.case.tsbackend/src/middlewares/public-crud-cors.middleware.tsbackend/test/ava-tests/non-saas-tests/non-saas-table-pure-crud-operations-e2e.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/microservices/agents-microservice/use-cases/get-company-subscription-info.use.case.ts
- backend/src/middlewares/public-crud-cors.middleware.ts
Comment on lines
+750
to
+762
| test.serial(`${currentTest} a malformed Origin is not reflected back (header-injection guard)`, async (t) => { | ||
| const malformedOrigin = 'https://evil.example.com/path with spaces'; | ||
|
|
||
| const res = await request(app.getHttpServer()) | ||
| .options(`/table/crud/${faker.string.uuid()}?tableName=whatever`) | ||
| .set('Origin', malformedOrigin) | ||
| .set('Access-Control-Request-Method', 'POST'); | ||
|
|
||
| // Still answered as a preflight, but a value that does not match a valid origin is dropped, not echoed. | ||
| t.is(res.status, 204); | ||
| t.is(res.headers['access-control-allow-origin'], undefined); | ||
| t.is(res.headers['access-control-allow-credentials'], undefined); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify untyped AVA async callback params in TS tests
rg -nP --type=ts 'test\.serial\([^)]*,\s*async\s*\(\s*[a-zA-Z_]\w*\s*\)\s*=>'Repository: rocket-admin/rocketadmin
Length of output: 50381
Add type annotations to AVA test callback parameters.
The test callbacks at lines 750 and 766 lack required TypeScript type annotations. Add t: ExecutionContext and : Promise<void> to each callback signature per the TypeScript coding guidelines.
Suggested fix
-import test from 'ava';
+import test, { type ExecutionContext } from 'ava';
-test.serial(`${currentTest} a malformed Origin is not reflected back (header-injection guard)`, async (t) => {
+test.serial(
+ `${currentTest} a malformed Origin is not reflected back (header-injection guard)`,
+ async (t: ExecutionContext): Promise<void> => {Apply the same annotations to both test callbacks in the file.
🤖 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 750 - 762, The test callback parameters in the test.serial function
for "a malformed Origin is not reflected back (header-injection guard)" and the
other test at line 766 are missing required TypeScript type annotations. Add the
type annotation `ExecutionContext` to the `t` parameter and add the return type
`: Promise<void>` to the async callback function signature in both test.serial
calls to comply with TypeScript coding guidelines.
Source: Coding guidelines
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.
Summary by CodeRabbit
/table/crudroutes with safer origin reflection, credential support, and correctOPTIONSpreflight responses./table/crudCORS, including authenticated/unauthenticated behavior and edge cases for malformedOriginand request headers.