-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feature(app-connections): project-scope app connections #4353
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
base: main
Are you sure you want to change the base?
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
@greptile review this |
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.
Greptile Summary
This PR implements a major architectural change to migrate App Connections from organization-scoped to project-scoped resources. The migration introduces comprehensive support for project-level app connections while maintaining backward compatibility with existing organization-level connections.
The implementation adds a nullable projectId
column to the app connections database table with updated unique constraints, moving from [orgId, name]
uniqueness to [name, projectId]
uniqueness. The backend services are updated to support dual permission checking - organization permissions for legacy connections and project permissions for new ones. The permission system includes new ProjectPermissionAppConnectionActions
with CRUD operations plus a specific 'Connect' action.
Frontend changes include new project-scoped routing across Secret Manager, Certificate Manager, and Secret Scanning layouts, with dedicated App Connections pages for each project type. The UI provides visual distinction between organization-scoped connections (shown with 'Organization' badges) and project-scoped connections through the new AppConnectionOption
component. OAuth flows are updated to handle project context and redirect users to appropriate project-specific pages after authentication.
The migration includes a comprehensive update to over 35 integration documentation files, changing navigation instructions from 'Organization Settings > App Connections' to project-level access and adding required projectId
parameters to all API examples. A migration function allows existing organization-level connections to be moved to project scope while updating dependent resources like secret syncs, rotations, and certificate authorities.
All app connection service functions (Azure, AWS, GitLab, etc.) are updated to support project-scoped encryption using appropriate KMS data keys based on whether a projectId
is present. The implementation follows a transitional approach where both organization and project-scoped connections can coexist during the migration period.
Confidence score: 2/5
- This PR has significant security vulnerabilities and complex migration logic that could cause production issues
- Score lowered due to unsafe OR queries, non-null assertion operators, incorrect permission mappings, and complex transaction handling
- Pay close attention to app-connection-dal.ts (unsafe OR queries), AppConnectionForm.tsx (non-null assertions), GatewayListPage.tsx (wrong permissions), and migration files (constraint handling)
Context used:
Rule - # Greptile Code Review Prompt: OR Query Safety Check (knex.js)
Objective
Flag database queries that use or
conditions without proper grouping, which can break outer filters and cause unintended data exposure.
What to Flag
Look for query builder patterns where or
methods are called directly on the main query object without being wrapped in a subquery or callback function.
Flag these patterns:
.orWhere()
,.orWhereRaw()
,.orWhereILike()
,.orWhereIn()
, etc. called directly on the main query- Multiple chained
or
conditions without proper grouping - Any
or
condition that could bypass security filters or WHERE clauses applied elsewhere
Examples to FLAG:
// ❌ DANGEROUS - or conditions break outer filters
query.where('status', 'active')
.orWhere('name', 'like', '%search%')
.orWhere('email', 'like', '%search%');
// ❌ DANGEROUS - mixed with other conditions
query.where('tenantId', userId)
.where('deleted_at', null)
.orWhere('name', 'like', '%search%')
.orWhereRaw('email ilike ?', ['%search%']);
What NOT to Flag
Do NOT flag or
conditions that are properly grouped within a callback function or subquery.
Examples that are SAFE:
// ✅ SAFE - or conditions grouped in callback
query.where('status', 'active')
.where((qb) => {
qb.where('name', 'like', '%search%')
.orWhere('email', 'like', '%search%');
});
// ✅ SAFE - explicit subquery grouping
query.where('tenantId', userId)
.where('deleted_at', null)
.where(function() {
this.orWhere('name', 'like', '%search%')
.orWhere('email', 'like', '%search%');
});
Detection Pattern
Flag any line containing:
.orWhere*()
methods called directly on a query object- NOT preceded by
.where((
or.where(function
- NOT inside a callback function block
Review Message Template
When flagging, use this message:
⚠️ **Unsafe OR Query Detected**
This query uses `or` conditions directly on the main query object, which can bypass outer filters and security constraints.
**Issue:** OR conditions at the query root level can break tenant isolation, permission checks, or other important filters.
**Fix:** Wrap OR conditions in a grouped WHERE clause:
```javascript
// Instead of this:
query.where('important_filter', value)
.orWhere('field1', condition)
.orWhere('field2', condition);
// Do this:
query.where('important_filter', value)
.where((qb) => {
qb.where('field1', condition)
.orWhere('field2', condition);
});
Security Impact: High - Could expose unauthorized data
## Additional Context
This pattern is particularly dangerous in multi-tenant applications, permission systems, or any query with security-critical WHERE clauses. Always ensure OR conditions are logically grouped to maintain the integrity of outer security filters. ([link](https://app.greptile.com/review/custom-context?memory=c4ca0367-148d-42b9-bcbd-958caf88aa07))
**Rule -** Add validation for string fields that have database column length limits to prevent insertion failures. For username template fields, add a 255 character limit validation. ([link](https://app.greptile.com/review/custom-context?memory=ab5293aa-4452-426c-a274-f034c06d0066))
**Context -** Use 'email auth' instead of 'direct credential login' in documentation to provide clearer terminology for users. ([link](https://app.greptile.com/review/custom-context?memory=8067f41a-5f47-4fe6-8155-04af79d986ba))
<sub>112 files reviewed, 21 comments</sub>
<sub>[Edit Code Review Bot Settings](https://app.greptile.com/review/github) | [Greptile](https://greptile.com?utm_source=greptile_expert&utm_medium=github&utm_campaign=code_reviews&utm_content=infisical_4353)</sub>
...ages/organization/AppConnections/AppConnectionsPage/components/MigrateAppConnectionModal.tsx
Outdated
Show resolved
Hide resolved
...ization/AppConnections/AppConnectionsPage/components/AppConnectionForm/AppConnectionForm.tsx
Show resolved
Hide resolved
...end/src/pages/organization/AppConnections/AppConnectionsPage/components/AppConnectionRow.tsx
Outdated
Show resolved
Hide resolved
backend/src/db/migrations/20250807165059_add-app-connections-project-id-col.ts
Show resolved
Hide resolved
const externalCas = await (tx || db.replicaNode())(TableName.ExternalCertificateAuthority) | ||
.where(`${TableName.ExternalCertificateAuthority}.appConnectionId`, connectionId) | ||
.orWhere(`${TableName.ExternalCertificateAuthority}.dnsAppConnectionId`, connectionId) |
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.
logic:
This query uses orWhere
directly on the main query object, which can bypass outer filters and security constraints.
Issue: OR conditions at the query root level can break tenant isolation, permission checks, or other important filters.
Fix: Wrap OR conditions in a grouped WHERE clause:
// Instead of this:
query.where('filter', value)
.orWhere('field1', condition)
.orWhere('field2', condition);
// Do this:
query.where('filter', value)
.where((qb) => {
qb.where('field1', condition)
.orWhere('field2', condition);
});
Security Impact: High - Could expose unauthorized data
const externalCas = await (tx || db.replicaNode())(TableName.ExternalCertificateAuthority) | |
.where(`${TableName.ExternalCertificateAuthority}.appConnectionId`, connectionId) | |
.orWhere(`${TableName.ExternalCertificateAuthority}.dnsAppConnectionId`, connectionId) | |
const externalCas = await (tx || db.replicaNode())(TableName.ExternalCertificateAuthority) | |
.where((qb) => { | |
qb.where(`${TableName.ExternalCertificateAuthority}.appConnectionId`, connectionId) | |
.orWhere(`${TableName.ExternalCertificateAuthority}.dnsAppConnectionId`, connectionId); | |
}) |
Context Used: Rule - # Greptile Code Review Prompt: OR Query Safety Check (knex.js)
Objective
Flag database queries that use or
conditions without proper grouping, which can break outer filters and cause unintended data exposure.
What to Flag
Look for query builder patterns where or
methods are called directly on the main query object without being wrapped in a subquery or callback function.
Flag these patterns:
.orWhere()
,.orWhereRaw()
,.orWhereILike()
,.orWhereIn()
, etc. called directly on the main query- Multiple chained
or
conditions without proper grouping - Any
or
condition that could bypass security filters or WHERE clauses applied elsewhere
Examples to FLAG:
// ❌ DANGEROUS - or conditions break outer filters
query.where('status', 'active')
.orWhere('name', 'like', '%search%')
.orWhere('email', 'like', '%search%');
// ❌ DANGEROUS - mixed with other conditions
query.where('tenantId', userId)
.where('deleted_at', null)
.orWhere('name', 'like', '%search%')
.orWhereRaw('email ilike ?', ['%search%']);
What NOT to Flag
Do NOT flag or
conditions that are properly grouped within a callback function or subquery.
Examples that are SAFE:
// ✅ SAFE - or conditions grouped in callback
query.where('status', 'active')
.where((qb) => {
qb.where('name', 'like', '%search%')
.orWhere('email', 'like', '%search%');
});
// ✅ SAFE - explicit subquery grouping
query.where('tenantId', userId)
.where('deleted_at', null)
.where(function() {
this.orWhere('name', 'like', '%search%')
.orWhere('email', 'like', '%search%');
});
Detection Pattern
Flag any line containing:
.orWhere*()
methods called directly on a query object- NOT preceded by
.where((
or.where(function
- NOT inside a callback function block
Review Message Template
When flagging, use this message:
⚠️ **Unsafe OR Query Detected**
This query uses `or` conditions directly on the main query object, which can bypass outer filters and security constraints.
**Issue:** OR conditions at the query root level can break tenant isolation, permission checks, or other important filters.
**Fix:** Wrap OR conditions in a grouped WHERE clause:
```javascript
// Instead of this:
query.where('important_filter', value)
.orWhere('field1', condition)
.orWhere('field2', condition);
// Do this:
query.where('important_filter', value)
.where((qb) => {
qb.where('field1', condition)
.orWhere('field2', condition);
});
Security Impact: High - Could expose unauthorized data
## Additional Context
This pattern is particularly dangerous in multi-tenant applications, permission systems, or any query with security-critical WHERE clauses. Always ensure OR conditions are logically grouped to maintain the integrity of outer security filters. ([link](https://app.greptile.com/review/custom-context?memory=c4ca0367-148d-42b9-bcbd-958caf88aa07))
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.
there's no other where conditions so this is safe
f3d2668
to
f4dcb44
Compare
Description 📣
This PR adds support for project scoped app connections and updates docs, permissions, backend and frontend to support these changes.
Also includes org permission check fixes
Type ✨
Tests 🛠️
# Here's some code block to paste some code snippets