fix(integrations): Return 400 instead of 500 on partial code mapping PUT#120602
fix(integrations): Return 400 instead of 500 on partial code mapping PUT#120602scttcper wants to merge 4 commits into
Conversation
The config lookup only filtered on org integration ids, and the new project came straight out of `request.data` in `convert_args`. Two problems fell out of that. A PUT without `projectId` never set the `new_project` kwarg, so the handler blew up on a missing positional arg and returned a 500 instead of a validation error. And `get_project` was called with whatever string was in the payload, so a non-numeric `projectId` hit the db as garbage. Now the lookup filters on `organization_id` directly, and the new project is resolved from `serializer.validated_data` after validation runs. The serializer already marks `projectId` required and camelizes its error keys, so the missing-field case returns the normal 400 field errors with no special casing in the handler. Also catches ValueError on the config lookup since the url pattern for `config_id` is `[^/]+`, not digits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
the
Scope: Backend
Unrelated to the 500 fix, and `organization_integration_id__in` already scopes the lookup to the current org since those ids come from the org itself. Keeping the pr focused. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ional Restores the original shape. `new_project` needs to exist so we access check the project a mapping is being moved to, otherwise you could move one into a project you can't see. The only thing wrong with it was that `put()` took it as required while `convert_args` set it conditionally. Now it defaults to None and the access check only includes it when the body actually asks to move the mapping. The project lookup moved into the existing try so a non-numeric `projectId` 404s like a foreign or unknown one already did, instead of blowing up on int(). Keeps every other status code the same as before, only the three 500s turn into 4xx. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 28bcc5b. Configure here.
| projects = [config.project_repository.project] | ||
| if new_project is not None: | ||
| projects.append(new_project) | ||
| if not request.access.has_projects_access(projects): |
There was a problem hiding this comment.
Snake_case bypasses project access check
High Severity
Making new_project optional means a PUT can skip the destination access check when the body uses snake_case project_id instead of projectId. convert_args only reads projectId, so new_project stays unset and has_projects_access only covers the current project, while RepositoryProjectPathConfigSerializer still accepts project_id and can move the mapping. Previously this path 500'd on the required new_project argument, so the bypass was unreachable.
Reviewed by Cursor Bugbot for commit 28bcc5b. Configure here.
… PUT Bugbot caught a real hole in the previous commit. Making `new_project` optional meant a body using `project_id` instead of `projectId` skipped the destination access check entirely, since `convert_args` looked for the literal camelCase key while the serializer normalizes both spellings and happily moves the mapping. A member could move a code mapping onto a project they can't see. It was a 500 before, so it wasn't reachable. Normalizes the body with the same helpers the serializer uses instead of matching one spelling, so `projectId`, `project_id` and `ProjectId` all resolve to the same lookup and get access checked. Test fails with 200 on the old code and 403 on this one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Good catch on the snake_case bypass, confirmed it was real. A member with access to the mapping's current project could PUT Fixed in a63d3e3 by normalizing the body with the same |
2 participants
A PUT that left out
projectId, e.g.{"sourceRoot": ""}, returned a 500.put()tooknew_projectas a required arg butconvert_argsonly set it when the body had aprojectId, so the handler was called without it.new_projectis now optional and only included in the access check when the body actually asks to move the mapping. Missing required fields come back as a normal 400, and a garbageprojectIdorconfig_id404s instead of blowing up onint(). Every other status code is unchanged.fixes SENTRY-5ENF