fix: Improve elastic filters handling (#17508) - #17510
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #17510 +/- ##
=======================================
Coverage 34.09% 34.10%
=======================================
Files 3384 3384
Lines 138002 138012 +10
Branches 37353 37355 +2
=======================================
+ Hits 47048 47063 +15
+ Misses 90954 90949 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR hardens and refines OpenCTI’s ElasticSearch filtering pipeline in opencti-graphql, specifically by preventing unsafe script-based filter operators from being accepted through the generic filter grammar and by introducing a dedicated internal-only path for trusted Painless clauses.
Changes:
- Forbid
internal_scriptfrom passing filter format validation, and throw if it somehow reaches query building. - Introduce
internalScriptFiltersas a dedicated internal-only option and migrate report orphan detection to use it. - Improve ES query safety by parameterizing scripted field access and escaping wildcard query-string values.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| opencti-platform/opencti-graphql/src/utils/filtering/filtering-utils.ts | Strengthens filter validation and changes composed-key validation logic. |
| opencti-platform/opencti-graphql/src/domain/report.js | Migrates orphan-object deletion logic off internal_script operator into internalScriptFilters. |
| opencti-platform/opencti-graphql/src/database/middleware-loader.ts | Extends list/count argument typing to carry internal-only script filters. |
| opencti-platform/opencti-graphql/src/database/engine.ts | Enforces internal_script rejection, adds internalScriptFilters support, and improves ES query escaping/parameterization. |
| return ( | ||
| filter.key && isNotEmptyField(filter.key) | ||
| filter.key && isNotEmptyField(filter.key) && (filter.operator as string) !== 'internal_script' | ||
| ); |
fa20eff to
63aa41a
Compare
| // runtime check is needed: raw JSON-parsed filters bypass the enum typing entirely. | ||
| return ( | ||
| filter.key && isNotEmptyField(filter.key) | ||
| filter.key && isNotEmptyField(filter.key) && (filter.operator as string) !== 'internal_script' |
There was a problem hiding this comment.
throw a different and specific error if filter.operator = 'internal_script' rather than the generic one 'Incorrect filters format'
There was a problem hiding this comment.
Rather check the operator is valid (ie among an existing list)? We can't check all the not-valid operators like this one because one day the operator existed
| script: values[i].toString(), | ||
| }, | ||
| }); | ||
| } else if (operator === 'internal_script') { |
There was a problem hiding this comment.
remove this 'else if' which is not necessary here ? We don't check all the not-existing operators.
Suggestion: instead of the last 'else', we write 'else if(range_operators)'
And then : else { throw Error ('Not supported filter operator') }
|
Missing generic backend tests |
63aa41a to
993fc5e
Compare
Done Archidoit |
| }, | ||
| }); | ||
| } else { | ||
| throw UnsupportedError('Not supported filter operator'); |
There was a problem hiding this comment.
| throw UnsupportedError('Not supported filter operator'); | |
| throw UnsupportedError('Not supported filter operator', { filter, filterOperator ); |
Add the filter to have more content in the error thrown
| }); | ||
|
|
||
| it('should buildLocalMustFilter with internal_script should work', () => { | ||
| it('unknown keys must be rejected by buildLocalMustFilter', () => { |
There was a problem hiding this comment.
| it('unknown keys must be rejected by buildLocalMustFilter', () => { | |
| it('unknown filter operators must be rejected by buildLocalMustFilter', () => { |
|
|
||
| await expect( | ||
| countAllThings(testContext, ADMIN_USER, { filters: JSON.parse(filtersString) }), | ||
| ).rejects.toThrow(); |
There was a problem hiding this comment.
maybe add the error message expected
| filterGroups: [], | ||
| } as FilterGroup; | ||
|
|
||
| expect(() => checkFiltersValidity(filterGroup)).toThrowError(); |
There was a problem hiding this comment.
maybe check also the expected error message
| `; | ||
|
|
||
| const addMalware = async (name) => { | ||
| const result = await queryAsAdmin({ query: MALWARE_ADD_QUERY, variables: { input: { name } } }); |
There was a problem hiding this comment.
if you create new entities in test, you should update the counters to avoid failing tests in raw-test.js (testUpdatedCounter, testCreatedCounter, etc).
993fc5e to
1a58e87
Compare
1a58e87 to
715177c
Compare
Proposed changes
internal_scripttruly internalRelated issues
How to test this PR
Checklist
Further comments