Skip to content

Conversation

scott-ray-wilson
Copy link
Contributor

Description 📣

This PRs corrects the argument order of connection and actor org Id on org connection permission checks

Type ✨

  • Bug fix
  • New feature
  • Improvement
  • Breaking change
  • Documentation

Tests 🛠️

# Here's some code block to paste some code snippets

@maidul98
Copy link
Collaborator

🎉 Snyk checks have passed. No issues have been found so far.

security/snyk check is complete. No issues have been found. (View Details)

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Summary

This PR fixes a critical security bug in the app connection service where organization permission checks were using incorrect parameter ordering when calling permissionService.getOrgPermission(). The issue affected 5 functions across the app connection service: getAppConnectionById, updateAppConnection, deleteAppConnection, connectAppConnectionById, and findAppConnectionUsageById.

The bug involved swapping two key parameters:

  • The 3rd parameter (orgId) should be the organization being accessed (the app connection's organization)
  • The 5th parameter (actorOrgId) should be the actor's organization for scope validation

Previously, these were reversed, causing permission checks to validate against the actor's organization instead of the app connection's organization. This change ensures proper tenant isolation by validating permissions in the correct organizational context.

The fix maintains the existing function signatures and behavior while correcting the internal permission validation logic. This aligns with the codebase's pattern of checking permissions against the resource's organization rather than the actor's organization, which is consistent with how other services like the integration auth service handle similar permission checks.

Confidence score: 5/5

  • This PR is extremely safe to merge with minimal risk of breaking existing functionality
  • Score reflects a straightforward parameter order correction that fixes a critical security vulnerability without changing business logic
  • All affected functions follow the same pattern making the fix consistent and predictable

1 file reviewed, no comments

Edit Code Review Bot Settings | Greptile

@scott-ray-wilson scott-ray-wilson merged commit a1dc9ec into main Sep 22, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants