fix(api): holistic prototype pollution guard in base-orm repository#1955
Open
anis-marrouchi wants to merge 2 commits into
Open
fix(api): holistic prototype pollution guard in base-orm repository#1955anis-marrouchi wants to merge 2 commits into
anis-marrouchi wants to merge 2 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a recursive payload-safety validation to BaseOrmRepository that flattens and inspects property paths for forbidden segments ( ChangesPrototype-Pollution Guard
Sequence DiagramsequenceDiagram
participant Client
participant Repository as BaseOrmRepository
participant Validator as assertSafePayload
participant Persistence as DB/Storage
Client->>Repository: call create/update with payload
Repository->>Validator: assertSafePayload(payload)
Validator->>Validator: flatten payload keys
Validator->>Validator: check keys for forbidden segments
alt Forbidden segment found
Validator-->>Repository: throw BadRequestException
Repository-->>Client: respond with BadRequest
else All keys safe
Validator-->>Repository: approval
Repository->>Persistence: persist data
Persistence-->>Repository: success
Repository-->>Client: return result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Review rate limit: 6/8 reviews remaining, refill in 14 minutes and 56 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/api/src/utils/generics/base-orm.repository.ts (1)
144-154: Arrays containing forbidden keys are not validated.The
flatten()helper treats arrays as leaf values (perpackages/api/src/utils/helpers/flatten.ts), so payloads like{ items: [{ __proto__: {} }] }would pass validation. Additionally, if an array is passed directly,flatten()throws a rawErrorrather thanBadRequestException.Consider recursively inspecting array elements:
♻️ Proposed enhancement
private assertSafePayload(data: unknown): void { - if (!data || typeof data !== 'object') return; + if (!data || typeof data !== 'object') return; + if (Array.isArray(data)) { + data.forEach((item) => this.assertSafePayload(item)); + return; + } const flatKeys = Object.keys( flatten(data as Record<string, unknown>) as Record<string, unknown>, ); const forbidden = flatKeys.find((key) => hasForbiddenSegment(key)); if (forbidden) { throw new BadRequestException(`Forbidden property path: "${forbidden}"`); } + // Recursively check array values + for (const value of Object.values(data)) { + if (Array.isArray(value)) { + value.forEach((item) => this.assertSafePayload(item)); + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/utils/generics/base-orm.repository.ts` around lines 144 - 154, The assertSafePayload function currently relies on flatten(), which treats arrays as leaves and can also throw a raw Error for top-level arrays; update assertSafePayload to handle arrays explicitly by recursively iterating array elements and validating each element (if element is an object, obtain its flattened keys or recurse) and by catching any errors from flatten() and rethrowing them as BadRequestException; reference the existing symbols assertSafePayload, flatten, hasForbiddenSegment, and BadRequestException so the fix: detect Array.isArray(data) and either recurse into elements or wrap flatten() usage in try/catch to convert errors and then run hasForbiddenSegment against every key found in nested objects and arrays to ensure forbidden segments are rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/api/src/utils/generics/base-orm.repository.ts`:
- Around line 144-154: The assertSafePayload function currently relies on
flatten(), which treats arrays as leaves and can also throw a raw Error for
top-level arrays; update assertSafePayload to handle arrays explicitly by
recursively iterating array elements and validating each element (if element is
an object, obtain its flattened keys or recurse) and by catching any errors from
flatten() and rethrowing them as BadRequestException; reference the existing
symbols assertSafePayload, flatten, hasForbiddenSegment, and BadRequestException
so the fix: detect Array.isArray(data) and either recurse into elements or wrap
flatten() usage in try/catch to convert errors and then run hasForbiddenSegment
against every key found in nested objects and arrays to ensure forbidden
segments are rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46b862de-8885-427a-ae5e-c93549290bce
📒 Files selected for processing (2)
packages/api/src/utils/generics/base-orm.repository.spec.tspackages/api/src/utils/generics/base-orm.repository.ts
anis-marrouchi
force-pushed
the
5-holistic-prototype-pollution-guard
branch
from
April 29, 2026 11:25
1ff93e3 to
ede67aa
Compare
anis-marrouchi
added a commit
to anis-marrouchi/Hexabot
that referenced
this pull request
Apr 30, 2026
flatten() treats arrays as leaf values, so forbidden keys inside array
elements (e.g. { items: [{ __proto__: {} }] }) bypassed the guard.
Iterate flattened entries and recurse into any array values.
Addresses CodeRabbit review nitpick on PR hexabot-ai#1955.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/api/src/utils/generics/base-orm.repository.spec.ts (1)
276-361: ⚡ Quick winAssert the database stays unchanged when these guards fire.
These cases only verify that a
BadRequestExceptionis thrown. A partial-write bug increateManyorupdateManywould still pass, so add a quick post-rejection count/lookup assertion to prove validation happens before persistence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/utils/generics/base-orm.repository.spec.ts` around lines 276 - 361, The tests only assert BadRequestException but don't verify no partial writes occurred; after each expect(...rejects...) for methods dummyRepository.create, createMany, updateOne, updateMany add a short post-rejection assertion that the repository state is unchanged — e.g., for create/createMany assert count() equals baselineEntities.length (or that no new entity with the forbidden payload exists), and for updateOne/updateMany verify the target entity (baselineEntities[0]) fields remain unchanged (or that total count is unchanged) to prove validation runs before persistence; update the spec blocks referencing dummyRepository.create, createMany, updateOne, updateMany and baselineEntities accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/api/src/utils/generics/base-orm.repository.spec.ts`:
- Around line 276-361: The tests only assert BadRequestException but don't
verify no partial writes occurred; after each expect(...rejects...) for methods
dummyRepository.create, createMany, updateOne, updateMany add a short
post-rejection assertion that the repository state is unchanged — e.g., for
create/createMany assert count() equals baselineEntities.length (or that no new
entity with the forbidden payload exists), and for updateOne/updateMany verify
the target entity (baselineEntities[0]) fields remain unchanged (or that total
count is unchanged) to prove validation runs before persistence; update the spec
blocks referencing dummyRepository.create, createMany, updateOne, updateMany and
baselineEntities accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 880e844a-756c-4d3a-a5a5-42260029c7b1
📒 Files selected for processing (2)
packages/api/src/utils/generics/base-orm.repository.spec.tspackages/api/src/utils/generics/base-orm.repository.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/api/src/utils/generics/base-orm.repository.ts
Add assertSafePayload() check to create, createMany, updateOne, and updateMany methods. Uses the existing hasForbiddenSegment helper to reject __proto__, constructor, and prototype keys in flattened payloads. Closes #5 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
flatten() treats arrays as leaf values, so forbidden keys inside array
elements (e.g. { items: [{ __proto__: {} }] }) bypassed the guard.
Iterate flattened entries and recurse into any array values.
Addresses CodeRabbit review nitpick on PR hexabot-ai#1955.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
anis-marrouchi
force-pushed
the
5-holistic-prototype-pollution-guard
branch
from
May 2, 2026 09:43
6840963 to
ae115f9
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/api/src/utils/generics/base-orm.repository.spec.ts (1)
311-321: 💤 Low valueTest title slightly misrepresents what is being verified.
assertSafePayloadis invoked unconditionally before theshouldFlattenbranch inupdateOne(context snippet 4, line 350), so the guard fires the same way whether or notshouldFlatten: trueis passed. The test validates thatupdateOnerejects a nested forbidden key (specificallydynamicField.__proto__), which is a useful and distinct case from Test 4's root-level__proto__, but the "in flatten mode" framing implies the option affects when or how the guard runs.✏️ Suggested rename
- it('should reject updateOne with nested forbidden key in flatten mode', async () => { + it('should reject updateOne with nested forbidden key', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/utils/generics/base-orm.repository.spec.ts` around lines 311 - 321, The test title is misleading because assertSafePayload runs before the shouldFlatten branch in updateOne, so the failure is due to a nested forbidden key (dynamicField.__proto__) regardless of flattening; rename the spec description in the it(...) call (currently 'should reject updateOne with nested forbidden key in flatten mode') to something like 'should reject updateOne with nested forbidden key (dynamicField.__proto__)' to accurately describe the behavior tested and keep the test code (payload, dummyRepository.updateOne call and options) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/api/src/utils/generics/base-orm.repository.spec.ts`:
- Around line 311-321: The test title is misleading because assertSafePayload
runs before the shouldFlatten branch in updateOne, so the failure is due to a
nested forbidden key (dynamicField.__proto__) regardless of flattening; rename
the spec description in the it(...) call (currently 'should reject updateOne
with nested forbidden key in flatten mode') to something like 'should reject
updateOne with nested forbidden key (dynamicField.__proto__)' to accurately
describe the behavior tested and keep the test code (payload,
dummyRepository.updateOne call and options) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3a82d36-88fd-4635-b772-6c662c7b4b3f
📒 Files selected for processing (2)
packages/api/src/utils/generics/base-orm.repository.spec.tspackages/api/src/utils/generics/base-orm.repository.ts
✅ Files skipped from review due to trivial changes (1)
- packages/api/src/utils/generics/base-orm.repository.ts
Contributor
|
Thank you @anis-marrouchi If I'm not mistaken, prototype pollution could affect object fields not all the payload. Meaning that only TypeORM entity attributes that are decorated with @JsonColumn() are most likely to be a target of such a vulnerability. I wonder if we could rather update the actionDtoToEntity() method to look into the metadata of the attribute in order to assess that sanitization is needed or not. I wonder also how TypeORM handles JSON column data, it may be that TypeORM has that protection mechanism already. |
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
2 participants
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
assertSafePayload()validation tocreate,createMany,updateOne, andupdateManyinBaseOrmRepositoryhasForbiddenSegmenthelper to reject__proto__,constructor, andprototypekeys in flattened payloads before they reach the ORM layerContext
This is a follow-up to #1940 which addressed prototype pollution in search filters. While that PR hardened the query/filter path, the create and update paths in
BaseOrmRepositorywere still unguarded. This PR closes that gap.Closes #5
Test plan
__proto__,constructor,prototypein create/createMany/updateOne/updateMany🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests