feat: add read-only validation for MongoDB aggregation pipelines and corresponding tests#1819
Merged
Conversation
…corresponding tests
📝 WalkthroughWalkthroughThis PR implements two security hardening features: (1) MongoDB aggregation pipeline validation that rejects write and server-side JavaScript stages in AI-driven queries, and (2) removal of sensitive OTP secret key from user info API responses with endpoint authentication enforcement. ChangesMongoDB Aggregation Pipeline Security Validation
User Information Exposure Prevention
Possibly Related PRs
Poem
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 introduces an AST-level validator to ensure MongoDB aggregation pipelines used by the AI tooling are read-only, and adds unit tests around the new validator. It also includes SaaS-related changes (new route and removing otpSecretKey from returned user info), which expands the PR scope beyond the title.
Changes:
- Add
isReadOnlyMongoAggregationPipelineto reject write stages and server-side JS operators at any nesting depth. - Enforce the new validator in
executeAggregationPipelinewithin the AI V7 use case. - Add AVA tests for the read-only pipeline validator (plus unrelated SaaS route/RO updates).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/ai-core/tools/query-validators.ts | Adds AST-walk read-only aggregation pipeline validator. |
| backend/src/entities/ai/use-cases/request-info-from-table-with-ai-v7.use.case.ts | Applies new validator before executing aggregation pipelines. |
| backend/test/ava-tests/non-saas-tests/non-saas-mongo-pipeline-readonly-validator.test.ts | Adds unit tests for the new read-only validator behavior. |
| backend/src/microservices/saas-microservice/data-structures/found-user-info.ro.ts | Excludes otpSecretKey from SaaS user info RO type. |
| backend/src/microservices/saas-microservice/utils/build-found-user-info-ro.ts | Removes otpSecretKey when building SaaS user info RO objects. |
| backend/src/microservices/saas-microservice/saas.module.ts | Adds SaaS route to middleware-protected routes list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+68
to
+76
| export function isReadOnlyMongoAggregationPipeline(pipeline: string): boolean { | ||
| let parsedPipeline: unknown; | ||
| try { | ||
| parsedPipeline = JSON.parse(pipeline); | ||
| } catch { | ||
| return false; | ||
| } | ||
| return !pipelineContainsForbiddenOperator(parsedPipeline); | ||
| } |
Comment on lines
+50
to
+52
| test('returns false (fail-closed) for an unparseable pipeline', (t) => { | ||
| t.false(isReadOnlyMongoAggregationPipeline('not valid json {')); | ||
| }); |
Comment on lines
122
to
126
| .forRoutes( | ||
| { path: 'saas/company/registered', method: RequestMethod.POST }, | ||
| { path: 'saas/user/:userId', method: RequestMethod.GET }, | ||
| { path: 'saas/users/email/:userEmail', method: RequestMethod.GET }, | ||
| { path: 'saas/user/register', method: RequestMethod.POST }, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/test/ava-tests/non-saas-tests/non-saas-mongo-pipeline-readonly-validator.test.ts (1)
50-52: 💤 Low valueConsider adding edge case tests for empty and non-array inputs.
The current coverage is solid for security scenarios. For completeness, you could optionally add tests for:
- Empty pipeline
[](should pass - valid read-only)- Non-array JSON like
{}(should pass per implementation, though not a valid MongoDB pipeline format)🧪 Optional additional tests
+test('allows an empty pipeline', (t) => { + t.true(isReadOnlyMongoAggregationPipeline('[]')); +}); + +test('allows non-array JSON (not a valid pipeline but parseable)', (t) => { + t.true(isReadOnlyMongoAggregationPipeline('{}')); +});🤖 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-mongo-pipeline-readonly-validator.test.ts` around lines 50 - 52, Add two edge-case tests for isReadOnlyMongoAggregationPipeline: one asserting that an empty pipeline string '[]' returns true (valid read-only) and another asserting that a non-array JSON string like '{}' returns true per current implementation; locate the tests near the existing non-saas-mongo-pipeline-readonly-validator.test cases and add assertions similar to the current test that call isReadOnlyMongoAggregationPipeline with those inputs and verify the expected boolean results.
🤖 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/saas-microservice/utils/build-found-user-info-ro.ts`:
- Around line 4-12: The mappers buildFoundUserInfoRO and
buildFoundUserInfoWithoutCompanyRO currently return "all except secrets" which
is fragile; change each to construct the return object from an explicit
allowlist of fields (or replace both with a single shared mapper) by explicitly
picking and assigning only the permitted properties from UserEntity into the
FoundUserInfoRO and FoundUserInfoWithoutCompanyRO shapes (e.g., id, email, name,
roles, createdAt, etc.), ensuring the object literal matches the declared return
types and omits any sensitive fields like password, otpSecretKey, company where
appropriate so new UserEntity fields won't leak by default.
---
Nitpick comments:
In
`@backend/test/ava-tests/non-saas-tests/non-saas-mongo-pipeline-readonly-validator.test.ts`:
- Around line 50-52: Add two edge-case tests for
isReadOnlyMongoAggregationPipeline: one asserting that an empty pipeline string
'[]' returns true (valid read-only) and another asserting that a non-array JSON
string like '{}' returns true per current implementation; locate the tests near
the existing non-saas-mongo-pipeline-readonly-validator.test cases and add
assertions similar to the current test that call
isReadOnlyMongoAggregationPipeline with those inputs and verify the expected
boolean results.
🪄 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: ca45e04e-8d7e-4b9a-b7f9-982e2b725623
📒 Files selected for processing (6)
backend/src/ai-core/tools/query-validators.tsbackend/src/entities/ai/use-cases/request-info-from-table-with-ai-v7.use.case.tsbackend/src/microservices/saas-microservice/data-structures/found-user-info.ro.tsbackend/src/microservices/saas-microservice/saas.module.tsbackend/src/microservices/saas-microservice/utils/build-found-user-info-ro.tsbackend/test/ava-tests/non-saas-tests/non-saas-mongo-pipeline-readonly-validator.test.ts
Comment on lines
4
to
12
| export function buildFoundUserInfoRO(user: UserEntity): FoundUserInfoRO { | ||
| const { password: _password, ...userInfo } = user; | ||
| const { password: _password, otpSecretKey: _otpSecretKey, ...userInfo } = user; | ||
| return userInfo; | ||
| } | ||
|
|
||
| export function buildFoundUserInfoWithoutCompanyRO(user: UserEntity): FoundUserInfoWithoutCompanyRO { | ||
| const { password: _password, company: _company, ...userInfo } = user; | ||
| const { password: _password, company: _company, otpSecretKey: _otpSecretKey, ...userInfo } = user; | ||
| return userInfo; | ||
| } |
There was a problem hiding this comment.
Switch these user mappers to an allowlist DTO.
Lines 5 and 10 still return “everything except known secrets”. That means any future sensitive UserEntity field will leak by default until every redaction site is updated, and the declared return types will not reliably catch that drift. Build the response from an explicit allowlist (or a single shared mapper that does) so this layer fails closed instead of open.
🤖 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/microservices/saas-microservice/utils/build-found-user-info-ro.ts`
around lines 4 - 12, The mappers buildFoundUserInfoRO and
buildFoundUserInfoWithoutCompanyRO currently return "all except secrets" which
is fragile; change each to construct the return object from an explicit
allowlist of fields (or replace both with a single shared mapper) by explicitly
picking and assigning only the permitted properties from UserEntity into the
FoundUserInfoRO and FoundUserInfoWithoutCompanyRO shapes (e.g., id, email, name,
roles, createdAt, etc.), ensuring the object literal matches the declared return
types and omits any sensitive fields like password, otpSecretKey, company where
appropriate so new UserEntity fields won't leak by default.
This was referenced Jun 3, 2026
Merged
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
New Features
Security
Tests