Fix negative indexing in recentProjects and recentMilestones#3478
Fix negative indexing in recentProjects and recentMilestones#3478arkid15r merged 12 commits intoOWASP:mainfrom
Conversation
Summary by CodeRabbit
WalkthroughThis PR introduces a centralized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Can we extract the repetitive code into something reusable?
|
@ahmedxgouda could you verify the fix? |
ahmedxgouda
left a comment
ahmedxgouda
left a comment
There was a problem hiding this comment.
Good work. All things work. Just asking to use the new function in all queries and nodes' fields that use limit (e.g. recentIssues).
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/mentorship/api/internal/queries/mentorship.py (1)
102-130:⚠️ Potential issue | 🟠 MajorGuard negative/invalid
offsetto fully prevent negative slicing.
offsetis user-controlled; a negative value will raise Django'sValueError: Negative indexing is not supportedduring slicing, which Django explicitly disallows because QuerySets translate to SQL LIMIT/OFFSET and cannot support negative indices. Validate/clampoffsetthe same way aslimitto handle invalid input gracefully instead of crashing.✅ Suggested fix
if (normalized_limit := normalize_limit(limit, MAX_LIMIT)) is None: return [] + try: + offset = int(offset) + except (TypeError, ValueError): + return [] + if offset < 0: + return [] + try: module = Module.objects.only("id").get(key=module_key, program__key=program_key) @@ - issues = issues_qs[offset : offset + normalized_limit] + issues = issues_qs[offset : offset + normalized_limit]
🧹 Nitpick comments (1)
backend/apps/mentorship/api/internal/nodes/module.py (1)
85-95: Theoffsetparameter is not validated.While the
limitis correctly normalized, a negativeoffsetvalue could still cause issues with the slice[offset : offset + normalized_limit]. Consider validatingoffsetsimilarly (e.g.,max(0, offset)).♻️ Suggested fix
def issues( self, limit: int = 20, offset: int = 0, label: str | None = None ) -> list[IssueNode]: """Return paginated issues linked to this module, optionally filtered by label.""" if (normalized_limit := normalize_limit(limit, MAX_LIMIT)) is None: return [] + offset = max(0, offset) + queryset = self.issues.select_related("repository", "author").prefetch_related( "assignees", "labels" )
|
@cubic-dev-ai review this pr |
@arkid15r I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3478 +/- ##
==========================================
- Coverage 87.67% 87.50% -0.17%
==========================================
Files 462 462
Lines 14309 14363 +54
Branches 1910 1926 +16
==========================================
+ Hits 12545 12569 +24
- Misses 1346 1368 +22
- Partials 418 426 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
3 participants
Proposed change
This PR fixes an issue where passing a negative value to the limit parameter could crash the recentProjects -> repositories -> recentMilestones GraphQL query.
The affected resolvers now check and sanitize the limit value before using it to slice Django querysets. If the value is negative or invalid, an empty list is returned instead of causing an error.
This ensures the API no longer crashes when fuzzing or unexpected inputs are used.
Resolves #3414
Checklist
make check-testlocally: all warnings addressed, tests passed