-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
🐛 fix(github): make inbound/outbound assignment sync case-insensitive #101984
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -139,7 +139,7 @@ def sync_group_assignee_inbound_by_external_actor( | |
|
|
||
| external_actors = ExternalActor.objects.filter( | ||
| provider=EXTERNAL_PROVIDERS_REVERSE[ExternalProviderEnum(integration.provider)].value, | ||
| external_name=external_user_name, | ||
| external_name__iexact=external_user_name, | ||
| integration_id=integration.id, | ||
| user_id__isnull=False, | ||
| ).values_list("user_id", flat=True) | ||
|
Comment on lines
140
to
145
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. High severity vulnerability may affect your project—review required: ℹ️ Why this mattersAffected 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:
💬 Ignore this findingTo ignore this, reply with:
You can view more details on this finding in the Semgrep AppSec Platform here. |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -345,6 +345,41 @@ def test_assignment_with_external_actor( | |
| assert updated_assignee.email == "[email protected]" | ||
| mock_record_event.assert_called_with(EventLifecycleOutcome.SUCCESS, None, False, None) | ||
|
|
||
| @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") | ||
| def test_assignment_with_external_actor_case_insensitive( | ||
| self, | ||
| mock_record_event: mock.MagicMock, | ||
| ) -> None: | ||
| """Test assigning a group to a user via external actor.""" | ||
| assert self.group.get_assignee() is None | ||
|
|
||
| external_issue = self.create_integration_external_issue( | ||
| group=self.group, | ||
| key="JIRA-123", | ||
| integration=self.example_integration, | ||
| ) | ||
|
|
||
| # Create external user mapping | ||
| self.create_external_user( | ||
| user=self.test_user, | ||
| external_name="@JohnDoe", | ||
| provider=ExternalProviders.GITHUB.value, | ||
| integration=self.example_integration, | ||
| ) | ||
|
|
||
| sync_group_assignee_inbound_by_external_actor( | ||
| integration=self.example_integration, | ||
| external_user_name="@johndoe", | ||
| external_issue_key=external_issue.key, | ||
| assign=True, | ||
| ) | ||
|
|
||
| updated_assignee = self.group.get_assignee() | ||
| assert updated_assignee is not None | ||
| assert updated_assignee.id == self.test_user.id | ||
| assert updated_assignee.email == "[email protected]" | ||
| mock_record_event.assert_called_with(EventLifecycleOutcome.SUCCESS, None, False, None) | ||
|
|
||
| @mock.patch("sentry.integrations.utils.sync.where_should_sync") | ||
| @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") | ||
| def test_assign_with_multiple_groups( | ||
|
|
||
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.
https://docs.djangoproject.com/en/5.2/ref/models/querysets/#std-fieldlookup-iexact
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.
we also don't have any logic that prevents repeat external_names i believe, something we should enforce.