-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(aci milestone 3): fake IDs in serializer if new models don't have old model equivalents #101974
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
base: master
Are you sure you want to change the base?
Conversation
| except DataConditionAlertRuleTrigger.DoesNotExist: | ||
| # this data condition does not have an analog in the old system, | ||
| # but we need to return *something* | ||
| return detector_trigger.id + OFFSET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Data Retrieval Error Handling
The change from DataCondition.objects.filter() to DataCondition.objects.get() on line 54 can cause unhandled DoesNotExist or MultipleObjectsReturned exceptions. If the get() call fails, these exceptions will propagate, preventing the method from returning a fallback ID and causing serialization to fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mm I guess this could happen if you link a workflow to a detector where the workflow has a warning action but the detector does not 🤔 I'll add a fallback case. EDIT: this seems like its own (larger) concern. Will address in a separate PR 👍
| ) | ||
| from sentry.workflow_engine.models.data_condition import Condition | ||
|
|
||
| OFFSET = 10**9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| alert_rule_id=alert_rule_id | ||
| ) | ||
| except AlertRuleDetector.DoesNotExist: | ||
| detector_id = int(alert_rule_id) - OFFSET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This operation is a bijection so fetching the detector ID works out nicely :)
| alert_rule_id = AlertRuleDetector.objects.values_list( | ||
| "alert_rule_id", flat=True | ||
| ).get(detector=obj) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High severity vulnerability may affect your project—review required:
Line 335 lists a dependency (django) with a known High severity vulnerability.
ℹ️ Why this matters
Affected versions of Django are vulnerable to Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection'). SQL injection in Django's ORM column aliases: when using QuerySet.annotate(), QuerySet.alias(), QuerySet.aggregate(), or QuerySet.extra() with dictionary expansion (**kwargs), the dictionary keys are used unescaped as SQL column aliases. On MySQL and MariaDB backends, an attacker who can influence those keys (for example, by passing a crafted dict of annotations) can inject arbitrary SQL into the generated query.
To resolve this comment:
Check if you are using Django with MySQL or MariaDB.
- If you're affected, upgrade this dependency to at least version 5.2.7 at uv.lock.
- If you're not affected, comment
/fp we don't use this [condition]
💬 Ignore this finding
To ignore this, reply with:
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
You can view more details on this finding in the Semgrep AppSec Platform here.
| alert_rule_trigger_id = DataConditionAlertRuleTrigger.objects.values_list( | ||
| "alert_rule_trigger_id", flat=True | ||
| ).get(data_condition=detector_trigger) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High severity vulnerability may affect your project—review required:
Line 59 lists a dependency (django) with a known High severity vulnerability.
ℹ️ Why this matters
Affected versions of Django are vulnerable to Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection'). SQL injection in Django's ORM column aliases: when using QuerySet.annotate(), QuerySet.alias(), QuerySet.aggregate(), or QuerySet.extra() with dictionary expansion (**kwargs), the dictionary keys are used unescaped as SQL column aliases. On MySQL and MariaDB backends, an attacker who can influence those keys (for example, by passing a crafted dict of annotations) can inject arbitrary SQL into the generated query.
To resolve this comment:
Check if you are using Django with MySQL or MariaDB.
- If you're affected, upgrade this dependency to at least version 5.2.7 at uv.lock.
- If you're not affected, comment
/fp we don't use this [condition]
💬 Ignore this finding
To ignore this, reply with:
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
You can view more details on this finding in the Semgrep AppSec Platform here.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #101974 +/- ##
========================================
Coverage 80.97% 80.97%
========================================
Files 8729 8730 +1
Lines 388412 388441 +29
Branches 24628 24628
========================================
+ Hits 314502 314527 +25
- Misses 73550 73554 +4
Partials 360 360 |
Actions use new -> old model serializers even if the workflow engine objects were single written, and thus we were failing to serialize and failing to send actions if the lookup table objects do not exist.
To fix this, if we cannot find a lookup table equivalent for a workflow engine object, set a fake ID for it equal to (workflow engine object ID + ONE BILLION). This is safe because the serialized legacy IDs are not used for action firing, with the exception of charts.
The charts will not show incident demarcation lines until we switch to using the open period serializer instead of the incidents serializer; however, because the actions currently fail to send at all, I feel that this is acceptable until the chart change lands.