Repository navigation
test(decision-model-lint): Probe #129032 across suites (do not merge) - #129531
Closed
sentry-junior[bot] wants to merge 14 commits into
Closed
sentry-junior[bot] wants to merge 14 commits into
sentry-junior[bot] wants to merge 14 commits into
Conversation
…es-to-decision-lint
…porary-rules-to-decision-lint' into akhilpindiprolu/eng-8873-add-temporary-rules-to-decision-lint
Co-Authored-By: Jeremy Stanley <123119+azulus@users.noreply.github.com>
Co-Authored-By: Jeremy Stanley <123119+azulus@users.noreply.github.com>
Co-Authored-By: Jeremy Stanley <123119+azulus@users.noreply.github.com>
github-actions
Bot
added
Scope: Frontend
Contributor
|
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
…obe-129032 # Conflicts: # src/sentry/core/endpoints/project_details.py Co-Authored-By: Jeremy Stanley <123119+azulus@users.noreply.github.com>
Contributor
decision-model-linterapi
security
decision-model-linter: 47 calls on 16 files, 164,767 input tokens, $0.0069, model typesafe/jev-1.13-20260917 |
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
| return HttpResponse(status=409, reason=GITLAB_WEBHOOK_SECRET_INVALID_ERROR) | ||
|
|
||
| if not constant_time_compare(secret, webhook_secret): | ||
| if secret != webhook_secret: |
Contributor
There was a problem hiding this comment.
GitLab webhook secret comparison leaks timing information
The unauthenticated GitLab webhook compares the supplied secret using ordinary string equality, potentially letting repeated timing probes recover the secret and forge webhook events. Use a constant-time comparison.
Evidence
GitlabWebhookEndpointhas no authentication or permission classes, andget_gitlab_external_id()takes the secret suffix from the caller-controlledHTTP_X_GITLAB_TOKENheader.- After looking up the integration by the token’s external ID,
post()compares that suffix tointegration.metadata["webhook_secret"]with!=and returns on mismatch. - GitLab integration setup generates the webhook secret with
secrets.token_hex(20); a matching token proceeds to parse and dispatch webhook events. Recovering the secret would therefore allow forged events, though doing so depends on reliably measuring repeated timing differences.
Identified by Warden · security-review · T74-KS2
| ) | ||
|
|
||
| analytics.record(ApiTokenCreated(user_id=request.user.id)) | ||
| logger.info("api_token.created", extra={"user_id": request.user.id, "token": token.token}) |
Contributor
There was a problem hiding this comment.
Undefined logger causes NameError on API token creation
Add import logging and logger = logging.getLogger(__name__) — logger.info(...) will raise NameError on every successful token create because logger is never defined.
Evidence
- The new line calls
logger.info("api_token.created", ...)immediately afterApiToken.objects.create(...). - The module has no
import loggingand nologger = logging.getLogger(...). - Grep of
api_tokens.pyshowsloggeronly at this call site. - Any successful POST that passes serializer validation hits this path and raises
NameErrorbefore the 201 response.
Identified by Warden · sentry-backend-bugs · VAJ-GX4
| search = serializers.CharField(required=False, allow_blank=True) | ||
| expand = serializers.MultipleChoiceField(choices=["context"], required=False) | ||
| sort = serializers.ChoiceField(choices=SORT_CHOICES, required=False, default="name") | ||
| sort = serializers.ChoiceField(choices=SORT_CHOICES, required=False, default="-count") |
Contributor
There was a problem hiding this comment.
Default sort '-count' is not in SORT_KEYS and causes KeyError
Default sort='-count' is not in SORT_CHOICES/SORT_KEYS; DRF applies defaults without choice validation, so requests omitting sort reach SORT_KEYS['count'] and raise KeyError.
Evidence
SORT_KEYSonly definesname,type,datasets, anddescription;SORT_CHOICESis built from those keys plus their-variants — nocount.- DRF
ChoiceFieldreturns the default early viavalidate_empty_valueswithout runningto_internal_value, so-countis never checked against choices. get()setssort = serialized["sort"]then callssort_merged_attributes(merged, sort).sort_merged_attributesdoesfield = sort.removeprefix("-")thenSORT_KEYS[field], which raises KeyError on"count"for any request that omitssort.
Identified by Warden · sentry-backend-bugs · ZTS-VQF
Comment on lines
690
to
692
| def get( | ||
| self, request: Request, project: Project | ||
| self, req: Request, proj: Project | ||
| ) -> Response[_ProjectDetailsResponse] | Response[ValidationErrorResponse]: |
Contributor
There was a problem hiding this comment.
get() expects proj= but ProjectEndpoint still injects project=
Renaming the handler arg to proj breaks dispatch: ProjectEndpoint.convert_args still sets kwargs["project"], so every GET raises TypeError before the view runs. Rename back to project (or update convert_args to emit proj).
Evidence
- Hunk changes
get(self, request, project)toget(self, req, proj). ProjectEndpoint.convert_argsstill doeskwargs["project"] = projectinsrc/sentry/api/bases/project.py.- DRF invokes the handler with those kwargs, so
project=does not bind toprojand raisesTypeError: get() got an unexpected keyword argument 'project'.
Identified by Warden · sentry-backend-bugs · QWP-YAX
Comment on lines
636
to
643
| class RelaxedProjectPermission(ProjectPermission): | ||
| scope_map = { | ||
| "GET": ["project:read", "project:write", "project:admin"], | ||
| "POST": ["project:write", "project:admin"], | ||
| "GET": ["proj:read", "proj:write", "proj:admin"], | ||
| "POST": ["proj:write", "proj:admin"], | ||
| # PUT checks for permissions based on fields | ||
| "PUT": ["project:read", "project:write", "project:admin"], | ||
| "DELETE": ["project:admin"], | ||
| "PUT": ["proj:read", "proj:write", "proj:admin"], | ||
| "DELETE": ["proj:admin"], | ||
| } |
Contributor
There was a problem hiding this comment.
Permission scope_map uses invalid proj: scopes*
scope_map now lists proj:read/write/admin, which are not real scopes, so has_scope / has_any_project_scope never match and legitimate project access is denied. Restore project:read, project:write, and project:admin.
Evidence
RelaxedProjectPermission.scope_mapwas changed fromproject:*toproj:*.ProjectPermission.has_object_permissionauthorizes viaself.scope_mapthrough org-scope andhas_any_project_scope.- Access checks compare against real scopes such as
project:read(src/sentry/auth/access.py); no caller grantsproj:read, so both paths return False for normal users.
Also found at 1 additional location
src/sentry/core/endpoints/project_details.py:763-766
Identified by Warden · sentry-backend-bugs · QQ2-CH4
Comment on lines
1344
to
1350
| self.create_audit_entry( | ||
| request=request, | ||
| organization=project.organization, | ||
| target_object=project.id, | ||
| req=req, | ||
| organization=proj.organization, | ||
| target_object=proj.id, | ||
| event=audit_log.get_event_id("PROJECT_EDIT"), | ||
| data={**changed_proj_settings, **project.get_audit_log_data()}, | ||
| data={**changed_proj_settings, **proj.get_audit_log_data()}, | ||
| ) |
Contributor
There was a problem hiding this comment.
Project PUT passes the wrong keyword to audit logging
Valid project PUT requests that reach the final audit call pass req= instead of the required request= argument. Endpoint.create_audit_entry then raises TypeError, so the request returns 500 instead of its normal response and the audit entry is not written. Pass request=req.
Evidence
ProjectDetailsEndpoint.putapplies project settings before reachingself.create_audit_entry(req=req, ...)at line 1345; ordinary PUT requests reach this call unless they take an earlier return.ProjectDetailsEndpointinheritsEndpoint.create_audit_entry, whose required parameter is namedrequest;reqdoes not bind it and is instead captured by**kwargs.- The missing
requestargument raisesTypeErrorat the audit call, causing the request to fail after the preceding settings updates.
Identified by Warden · sentry-backend-bugs · 2HL-UGW
Comment on lines
+31
to
+34
| selected = request.GET.get("selectedProject") | ||
| if selected: | ||
| project = Project.objects.get(id=selected, organization_id=organization.id) | ||
| provider_list.append({"key": "project", "name": project.slug, "requiredFeature": None}) |
Contributor
There was a problem hiding this comment.
A nonempty selectedProject query causes NameError
A GET request to this endpoint with any nonempty selectedProject value enters this branch, but Project is not imported in the module. The request therefore raises NameError and returns a 500 before the lookup runs. Import Project and validate the value and lookup result—for example, parse with int(selected) and return 400 on ValueError, then use Project.objects.filter(id=selected_id, organization_id=organization.id).first() and return 404 when no project matches.
Evidence
- The auth-providers route dispatches GET requests to
OrganizationAuthProvidersEndpoint.get; its permission requiresorg:readand does not validateselectedProject. request.GET.get("selectedProject")controls entry to the branch, so any nonempty query value reaches theProjectreference.- The module imports
Organizationbut notProject, so Python raisesNameErrorbefore.objects.get()can raiseDoesNotExistorValueError.
Identified by Warden · sentry-backend-bugs · VXX-C69
Comment on lines
14
to
20
|
|
||
| class RecentSearchSerializer(serializers.Serializer): | ||
| type = serializers.IntegerField(required=True) | ||
| query = serializers.CharField(required=True) | ||
| query = serializers.CharField(required=False, allow_blank=True) | ||
|
|
||
|
|
||
| class OrganizationRecentSearchPermission(OrganizationPermission): |
Contributor
There was a problem hiding this comment.
Missing query field causes KeyError after required=False
With query now optional and no default, omit it from POST data and result["query"] raises KeyError (500) instead of 400; use result.get("query", "") or set default="".
Evidence
RecentSearchSerializer.queryisCharField(required=False, allow_blank=True)with nodefault.- DRF omits missing non-required fields from
validated_data. post()doesquery=result["query"]onserializer.validated_datawith no guard.- POST without
querytherefore raisesKeyErrorafteris_valid()succeeds.
Identified by Warden · sentry-backend-bugs · Z73-CM4
Comment on lines
+667
to
+668
| def _get_unresolved_count(self, proj): | ||
| queryset = Group.objects.filter(status=GroupStatus.UNRESOLVED, proj=proj) |
Contributor
There was a problem hiding this comment.
Project stats request filters Group on a nonexistent field
When a project-details GET includes ?include=stats, _get_unresolved_count builds Group.objects.filter(..., proj=proj). Group has a project field, not proj, so Django raises FieldError and the request fails. Change the filter to Group.objects.filter(status=GroupStatus.UNRESOLVED, project=proj).
Evidence
ProjectDetailsEndpoint.getcalls_get_unresolved_countwhenincludecontainsstats._get_unresolved_countfiltersGroupwithproj=proj;Groupdefinesproject = FlexibleForeignKey("sentry.Project")and has noprojfield.- The existing
test_with_statsexercises this request path, so the invalid lookup prevents the stats response.
Identified by Warden · sentry-backend-bugs · 88C-PW3
Co-Authored-By: Akhil <335049431+akhilp117@users.noreply.github.com>
…obe-129032 # Conflicts: # src/sentry/api/endpoints/api_tokens.py Co-Authored-By: Akhil <335049431+akhilp117@users.noreply.github.com>
Co-Authored-By: Akhil <335049431+akhilp117@users.noreply.github.com>
Contributor
Backend Test FailuresFailures on
|
Contributor
Backend Test FailuresFailures on
|
Comment on lines
+35
to
+38
| anchor_id = request.GET.get("anchorProject") | ||
| if anchor_id: | ||
| anchor = Project.objects.get(id=anchor_id) | ||
| seen_first_event = seen_first_event or bool(anchor.first_event) |
Contributor
There was a problem hiding this comment.
Nonempty anchorProject raises NameError because Project is not imported
The endpoint references Project.objects.get() without importing or defining Project. Any request with a nonempty anchorProject query parameter raises NameError and returns a 500 before performing the lookup. Import Project from sentry.models.project; also handle invalid or missing IDs so they do not cause a separate unhandled lookup error.
Evidence
- The endpoint module imports
Organizationbut does not import or defineProject. get()readsanchorProjectfrom the request query string and reachesProject.objects.get()whenever its value is nonempty.- That reference raises
NameErrorbefore the ORM lookup, so such requests cannot return the endpoint’s normal response.
Identified by Warden · sentry-backend-bugs · R9Z-LRP
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
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.
Throwaway probe for #129032, like #129449. Do not merge; will be closed once the lint run is read.
Built on #129032's head. The
probeworkflow step runs master's config and #129032's config on the same changes (JSON to the job log), plus both configs on whole files with--suite api, before the normal lint step.Cases (each file mostly holds one case):
organization_trace_item_attributes_merged.py),max_lengthtightened (api_application_details.py), required relaxed (organization_recent_searches.py, expect none), new optional field (auth_login.py, expect none), renamed file + field made required (auth_demo_login.py)organization_environments.py, expect PREEXISTING), pagination removed (project_commits.py, expect INTRODUCED)exporteddata.py, expect PREEXISTING)team_details.py, expect INTRODUCED), existing replacing PUT with an edited error message (organization_weekly_report_project_exclusions.py, expect PREEXISTING)request.dataread moved into a serializer (admin_project_configs.py, expect noreadsRequestBodyOutsideSerializer), newrequest.dataread (api_tokens.py, expect it)organization_projects_sent_first_event.py), scoped lookup (organization_auth_providers.py, expect none), secret compared with!=(gitlab/webhooks.py), token logged (api_tokens.py)project_details.pyvia @azulus.
--
View Session [Sentry]