Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[backend] validate elUpdateElement input value against schema before indexing (#5696) #6046

Merged
merged 22 commits into from Mar 11, 2024

Conversation

labo-flg
Copy link
Member

@labo-flg labo-flg commented Feb 20, 2024

Proposed changes

  • insert a validation step at the start of engine elUpdateElement function
  • validate the payload before indexing, against the schema

Related issues

@labo-flg labo-flg self-assigned this Feb 20, 2024
@labo-flg labo-flg added the filigran team use to identify PR from the Filigran team label Feb 20, 2024
@labo-flg labo-flg force-pushed the issue/5696-validate-fieldpatch-schema branch 6 times, most recently from 5f46825 to b19fd4a Compare February 23, 2024 09:01
Copy link

codecov bot commented Feb 23, 2024

Codecov Report

Attention: Patch coverage is 91.91919% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 66.71%. Comparing base (d793a45) to head (ba1f957).
Report is 1 commits behind head on master.

Files Patch % Lines
...orm/opencti-graphql/src/schema/schema-validator.ts 40.00% 3 Missing ⚠️
...rm/opencti-graphql/src/schema/schema-attributes.ts 97.56% 2 Missing ⚠️
...ncti-platform/opencti-graphql/src/types/store.d.ts 0.00% 2 Missing ⚠️
...orm/opencti-graphql/src/domain/stixDomainObject.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6046      +/-   ##
==========================================
+ Coverage   66.67%   66.71%   +0.04%     
==========================================
  Files         541      541              
  Lines       64420    64501      +81     
  Branches     5256     5288      +32     
==========================================
+ Hits        42950    43033      +83     
+ Misses      21470    21468       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@labo-flg labo-flg force-pushed the issue/5696-validate-fieldpatch-schema branch from f13a150 to 93496ec Compare February 23, 2024 14:39
@labo-flg labo-flg marked this pull request as ready for review February 23, 2024 16:24
@labo-flg labo-flg changed the title [backend] validate fieldPatch input value against schema before indexing (#5696) [backend] validate elUpdateElement input value against schema before indexing (#5696) Feb 23, 2024
@lndrtrbn lndrtrbn self-requested a review February 26, 2024 10:36
Copy link
Member

@lndrtrbn lndrtrbn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this (imo valid) input:

{
  "id": "38fe587f-a73b-46ad-856f-cc2710246d42",
  "input": {
    "key": "user_confidence_level",
    "object_path": "/user_confidence_level/overrides",
    "value": [
      {
        "entity_type": "Administrative-Area",
        "max_confidence": 60
      },
      {
        "entity_type": "Country",
        "max_confidence": 40
      }
    ]
  }
}

I have the following error:

{
      "message": "Validation error",
      "name": "VALIDATION_ERROR",
      "time_thrown": "2024-02-26T13:56:35.665Z",
      "data": {
        "http_status": 500,
        "genre": "BUSINESS",
        "field": "user_confidence_level",
        "message": "Attribute user_confidence_level cannot be multiple",
        "data": {
          "key": "user_confidence_level",
          "object_path": "/user_confidence_level/overrides",
          "value": [
            {
              "entity_type": "Administrative-Area",
              "max_confidence": 60
            },
            {
              "entity_type": "Country",
              "max_confidence": 40
            }
          ]
        }
      }
    }

Like if the object_path was not taking in account

@labo-flg
Copy link
Member Author

This seems like a valid input yes.
I'll investigate to see how it comes this payload is produced. If it's a valid payload, it's a bug on the validation process.
Otherwise... Maybe we do not handle objectpath on multiple like that (only on indexed path).

@labo-flg labo-flg force-pushed the issue/5696-validate-fieldpatch-schema branch from ea5aee0 to 15fcc65 Compare February 26, 2024 16:53
@labo-flg
Copy link
Member Author

I've pushed a commit to allow such case @lndrtrbn, and added some integration tests.

@lndrtrbn lndrtrbn force-pushed the issue/5696-validate-fieldpatch-schema branch from 2243e49 to 9384a8c Compare February 27, 2024 14:54
@Kedae Kedae force-pushed the issue/5696-validate-fieldpatch-schema branch from 9384a8c to 9513d34 Compare February 27, 2024 19:15
@labo-flg labo-flg force-pushed the issue/5696-validate-fieldpatch-schema branch from 4a958a2 to 7f3c259 Compare March 11, 2024 08:23
@labo-flg labo-flg merged commit 7f524f6 into master Mar 11, 2024
8 checks passed
@labo-flg labo-flg deleted the issue/5696-validate-fieldpatch-schema branch March 11, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filigran team use to identify PR from the Filigran team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants