external auth support

[roymiloh]

Exposing a getAccessToken function, that returns an access token, depending on the integration type. The LLM is instructed to generate the following:

base44.asServiceRole.connectors.getAccessToken(integration_type)

The endpoint requires a service role, and returns the access token of the contributor who connected his account first (we currently allow one account per app).

[claude]

Claude finished @roymiloh's task —— View job

---

Code Review: External Auth Support

Review Summary

I've reviewed the PR implementing external auth support. The feature adds a getAccessToken method that retrieves access tokens for external integrations, requiring service role authentication.

Review Checklist

• Check for CLAUDE.md instructions
• Review code quality and best practices
• Identify potential bugs or issues
• Assess performance considerations
• Analyze security concerns
• Evaluate test coverage

🐛 Critical Bug Found

There's a critical bug in src/modules/external-auth.ts:34-35:

// @ts-expect-error
return response.access_token;

The code is returning response.access_token, but based on Axios conventions, the response data should be accessed via response.data.access_token. This will cause a runtime error.

Fix required:

return response.data;  // or response.data.access_token if you only want the token string

🔐 Security Concerns

1. Service Role Authorization: Good that the module requires service role authentication. However, consider adding explicit validation in the module itself rather than relying solely on client configuration.

2. Integration Type Validation: While there's basic string validation, consider:

   • Validating against a whitelist of allowed integration types
   • Sanitizing the integration type parameter to prevent path traversal attacks

📋 Code Quality & Best Practices

1. Type Safety: The use of @ts-expect-error is a red flag. Fix the actual type issue instead of suppressing it.

2. Type Definition: ExternalAuthIntegrationType is defined as a generic string. Consider using a more specific type:

   export type ExternalAuthIntegrationType = 'github' | 'google' | 'slack' | string;

3. Error Handling: The module should handle specific API errors and provide meaningful error messages for common failure scenarios (e.g., no connected account, invalid integration type).

⚡ Performance Considerations

1. Caching: Consider implementing token caching with appropriate TTL to reduce API calls, especially if tokens have long validity periods.

2. Request Optimization: No performance issues identified with the current implementation.

🧪 Test Coverage

Missing Tests: No test files were added for this new functionality. This is a significant gap that should be addressed:

1. Add unit tests for createExternalAuthModule in tests/unit/external-auth.test.js
2. Add E2E tests if applicable
3. Test scenarios should include:
   • Valid token retrieval
   • Invalid integration type handling
   • Missing service role authorization
   • API error responses
   • Type validation

💡 Additional Recommendations

1. Documentation: Add JSDoc comments explaining:

   • Which integration types are supported
   • Token expiration behavior
   • Example usage

2. Return Type Clarity: The function returns ExternalAuthAccessTokenResponse but the implementation suggests it might only return the token string. Clarify the intended return type.

3. Consistent Error Messages: The error message could be more specific about valid integration types if you have a known set.

Conclusion

The PR adds useful functionality but has a critical bug that must be fixed before merging. Additionally, the lack of test coverage is concerning for a security-sensitive feature. I recommend addressing the bug, adding comprehensive tests, and considering the security enhancements suggested above.

[github-actions]

🚀 Package Preview Available!

---

Install this PR's preview build with npm:

npm i @base44-preview/sdk@0.8.3-pr.40.f27d5be

Prefer not to change any import paths? Install using npm alias so your code still imports @base44/sdk:

npm i "@base44/sdk@npm:@base44-preview/sdk@0.8.3-pr.40.f27d5be"

Or add it to your package.json dependencies:

{
  "dependencies": {
    "@base44/sdk": "npm:@base44-preview/sdk@0.8.3-pr.40.f27d5be"
  }
}

• 📦 Preview Package: @base44-preview/sdk@0.8.3-pr.40.f27d5be
• 🔗 View this commit on GitHub

---

_{Preview published to npm registry — try new features instantly!}

[claude]

Claude finished @roymiloh's task —— View job

---

Code Review: External Auth Support (app-connections module)

Review Tasks:

• Read all changed files to understand the implementation
• Check if previous critical bug was fixed
• Assess code quality and best practices
• Identify potential bugs or issues
• Analyze performance considerations
• Evaluate security concerns
• Check test coverage
• Provide comprehensive feedback

🚨 Critical Bug Still Present

The critical bug identified in my previous review has NOT been fixed. In src/modules/app-connections.ts:33-34:

// @ts-expect-error
return response.access_token;

This code will fail at runtime because:

1. axios.get<AppConnectionAccessTokenResponse>() returns an AxiosResponse object
2. The actual data is in response.data, not response
3. You're trying to access response.access_token which doesn't exist on AxiosResponse

Required fix:

return response.data;

📋 Code Quality Issues

1. TypeScript Error Suppression: Using @ts-expect-error is a code smell. Fix the actual issue instead of suppressing it.

2. Module Naming Inconsistency: The module has been renamed from "external-auth" to "app-connections", but the PR description still refers to externalAuth.getAccessToken(). Update documentation to reflect the actual implementation.

3. Type Definition: AppConnectionIntegrationType is still defined as a generic string. Consider using a more specific type or enum for known integration types.

🐛 Potential Bugs

1. Return Type Mismatch: The function signature promises to return AppConnectionAccessTokenResponse (an object with access_token property), but the implementation tries to return just the token string.

2. Error Handling: No specific error handling for common scenarios like:

   • No connected account for the integration type
   • Invalid/unknown integration type
   • Network/API errors

🔐 Security Considerations

1. Good: Service role authentication is properly enforced through the client architecture.

2. Missing: No validation against allowed integration types. Consider adding a whitelist to prevent potential security issues.

3. Token Exposure: Ensure access tokens are not logged or exposed in error messages.

⚡ Performance Considerations

1. No Caching: Tokens are fetched on every call. Consider implementing caching with appropriate TTL based on token expiration times.

2. No Rate Limiting: Consider implementing rate limiting or request throttling for token requests.

🧪 Test Coverage

Critical Gap: No tests have been added for this new functionality. The project has a clear testing structure with both unit and E2E tests, but no tests were added for the app-connections module.

Missing test coverage:

• Unit tests in tests/unit/app-connections.test.js
• E2E tests in tests/e2e/app-connections.test.js

Test scenarios to cover:

• Successful token retrieval
• Invalid integration type handling
• Missing service role authorization
• API error responses
• Parameter validation

💡 Recommendations

1. Fix the critical bug immediately - the code won't work as is.

2. Add comprehensive tests before merging this security-sensitive feature.

3. Improve error messages to help developers debug integration issues.

4. Add JSDoc comments with examples and supported integration types.

5. Consider adding a method to list available/connected integrations.

Summary

This PR is not ready to merge due to the critical runtime bug that will cause the feature to fail completely. The same bug from the previous review remains unfixed. Additionally, the complete lack of test coverage for a security-sensitive feature is concerning. Please address these issues before proceeding.

[claude]

Claude finished @roymiloh's task —— View job

---

Code Review: External Auth Support

Review Tasks:

• Read all changed files to understand the implementation
• Check if previous critical bug was fixed
• Assess code quality and best practices
• Identify potential bugs or issues
• Analyze performance considerations
• Evaluate security concerns
• Check test coverage
• Provide comprehensive feedback

🚨 Critical Runtime Bug Still Present

The critical bug identified in previous reviews has NOT been fixed. In src/modules/app-connections.ts:33-34:

// @ts-expect-error
return response.access_token;

This code will fail at runtime because:

1. axios.get<AppConnectionAccessTokenResponse>() returns an AxiosResponse<AppConnectionAccessTokenResponse> object
2. The actual data is in response.data, not directly on response
3. You're trying to access response.access_token which doesn't exist on the AxiosResponse type

Required fix:

return response.data;

This is a blocking issue - the feature will not work at all until this is fixed.

📋 Code Quality Issues

1. TypeScript Error Suppression

Using @ts-expect-error is a code smell. You're suppressing a legitimate TypeScript error instead of fixing the actual issue. This defeats the purpose of using TypeScript for type safety.

2. Inconsistent Module Naming

The module has been renamed from "external-auth" to "app-connections", but:

• The API endpoint still uses /external-auth/: /apps/${appId}/external-auth/tokens/${integrationType}
• The PR title still refers to "external auth support"
• This inconsistency could lead to confusion

3. Generic Type Definition

AppConnectionIntegrationType is defined as a generic string. Consider using a more specific type:

// Better approach - enumerate known integration types
export type AppConnectionIntegrationType = 'github' | 'slack' | 'google' | string;

// Or use an enum for stricter typing
export enum AppConnectionIntegrationType {
  GITHUB = 'github',
  SLACK = 'slack',
  GOOGLE = 'google'
}

🐛 Additional Bugs

Return Type Mismatch

The function signature promises to return AppConnectionAccessTokenResponse (an object with access_token property), but the implementation tries to return just the token string. This creates a type mismatch that will confuse API consumers.

🔐 Security Concerns

1. Service Role Enforcement ✅

Good: The module is correctly added to serviceRoleModules only, ensuring it requires service role authentication.

2. Missing Input Validation ⚠️

While there's basic string validation, consider:

• Validating against a whitelist of allowed integration types
• Sanitizing the integration type to prevent potential path traversal attacks
• Adding length limits to prevent overly long strings

3. Error Information Leakage

The module should handle specific API errors gracefully and avoid exposing sensitive information in error messages.

⚡ Performance Considerations

1. No Caching

Tokens are fetched on every call. Consider implementing:

• Token caching with appropriate TTL based on token expiration
• Cache invalidation mechanism for revoked tokens

2. No Rate Limiting

Consider implementing client-side rate limiting or request throttling to prevent API abuse.

🧪 Test Coverage - Critical Gap

No tests exist for this new functionality. This is a significant concern for a security-sensitive feature. The project has a clear testing structure:

• Unit tests in tests/unit/
• E2E tests in tests/e2e/

Required test coverage:

1. Unit tests (tests/unit/app-connections.test.js):

   • Successful token retrieval
   • Invalid integration type handling
   • Parameter validation
   • Error response handling

2. E2E tests (tests/e2e/app-connections.test.js):

   • Full flow with real API calls
   • Service role authorization verification
   • Error scenarios

💡 Recommendations

1. Immediate Actions Required

• Fix the critical bug - the feature is completely broken
• Add comprehensive test coverage before merging
• Remove @ts-expect-error and fix the type issue properly

2. Code Improvements

// Better error handling example
async getAccessToken(
  integrationType: AppConnectionIntegrationType
): Promise<AppConnectionAccessTokenResponse> {
  // Validate input
  if (!integrationType || typeof integrationType !== "string") {
    throw new Error("Integration type is required and must be a string");
  }
  
  // Optional: Validate against known types
  const allowedTypes = ['github', 'slack', 'google'];
  if (!allowedTypes.includes(integrationType)) {
    console.warn(`Unknown integration type: ${integrationType}`);
  }
  
  try {
    const response = await axios.get<AppConnectionAccessTokenResponse>(
      `/apps/${appId}/external-auth/tokens/${integrationType}`
    );
    
    return response.data; // Fix: Access data property
  } catch (error) {
    // Handle specific error cases
    if (error.response?.status === 404) {
      throw new Error(`No connection found for integration type: ${integrationType}`);
    }
    throw error;
  }
}

3. Documentation

Add JSDoc comments explaining:

• Which integration types are supported
• Token expiration behavior
• Example usage
• Error scenarios

Summary

This PR adds useful functionality but is not ready to merge due to:

1. Critical bug that makes the feature non-functional
2. No test coverage for a security-sensitive feature
3. Poor TypeScript practices (using @ts-expect-error)

The same critical bug from previous reviews remains unfixed despite being clearly identified. Please address these issues, especially the runtime error, before proceeding.

[netanelgilad]

looks good.
are these our formal user facing naming for the feature?
app connections
integration type

we are going to start documenting the SDK externally and it's best if the naming we use here would correlate to what we have in the product itself as much as possible

[claude]

Claude finished @roymiloh's task —— View job

---

Code Review: External Auth Support

Review Tasks:

• Read all changed files to understand the implementation
• Check if previous critical bug was fixed
• Assess code quality and best practices
• Identify potential bugs or issues
• Analyze performance considerations
• Evaluate security concerns
• Check test coverage
• Provide comprehensive feedback

🚨 Critical Runtime Bug Still Present

The critical bug identified in all three previous reviews remains unfixed. In src/modules/connectors.ts:33-34:

// @ts-expect-error
return response.access_token;

This code will fail at runtime because:

1. axios.get<ConnectorAccessTokenResponse>() returns an AxiosResponse<ConnectorAccessTokenResponse> object
2. The response data is in response.data, not directly on response
3. You're trying to access response.access_token which doesn't exist on the AxiosResponse type

Required fix:

return response.data;

This is a blocking issue - the feature will not work until this is fixed.

📋 Code Quality Issues

1. TypeScript Error Suppression

Using @ts-expect-error is poor practice. You're suppressing a legitimate TypeScript error that indicates a real runtime bug. This defeats the purpose of using TypeScript for type safety.

2. Module Naming Evolution

The module has evolved from "external-auth" → "app-connections" → "connectors", but:

• The API endpoint still uses /external-auth/: /apps/${appId}/external-auth/tokens/${integrationType}
• This inconsistency between the module name and API endpoint could cause confusion

3. Generic Type Definition

ConnectorIntegrationType is defined as a generic string. Consider using a more specific type:

// Enumerate known integration types for better type safety
export type ConnectorIntegrationType = 'github' | 'slack' | 'google' | string;

🐛 Additional Bugs

Return Type Mismatch

The function signature promises to return ConnectorAccessTokenResponse (an object with access_token property), but the implementation attempts to return just the token string. This creates confusion about what the API actually returns.

🔐 Security Considerations

1. Service Role Enforcement ✅

Good: The module is correctly added only to serviceRoleModules in client.ts:131, ensuring it requires service role authentication.

2. Input Validation ⚠️

While there's basic type validation, consider:

• Validating against a whitelist of allowed integration types
• Adding length limits to prevent overly long strings
• Sanitizing the integration type to prevent potential security issues in the URL path

3. Error Handling

The module lacks specific error handling. Consider handling:

• 404 errors when no connector exists for the integration type
• 403 errors for unauthorized access
• Providing meaningful error messages without exposing sensitive information

⚡ Performance Considerations

1. No Caching

Tokens are fetched on every call. If tokens have reasonable TTLs, consider implementing:

• Client-side token caching with appropriate expiration handling
• Cache invalidation mechanism for revoked tokens

2. No Rate Limiting

Consider implementing client-side rate limiting to prevent excessive API calls.

🧪 Test Coverage - Critical Gap

No tests exist for this new functionality. This is concerning for a security-sensitive feature. The project has a clear testing structure with both unit and E2E tests, but no tests were added for the connectors module.

Required test coverage:

1. Unit tests (tests/unit/connectors.test.js):

   • Successful token retrieval
   • Invalid integration type handling
   • Error response handling
   • Parameter validation

2. E2E tests (tests/e2e/connectors.test.js):

   • Full flow with real API calls
   • Service role authorization verification

💡 Recommendations

1. Immediate Actions Required

• Fix the critical bug - the feature is completely broken without this fix
• Add comprehensive test coverage before merging
• Remove @ts-expect-error and fix the underlying issue

2. Improved Implementation

async getAccessToken(
  integrationType: ConnectorIntegrationType
): Promise<ConnectorAccessTokenResponse> {
  if (!integrationType || typeof integrationType !== "string") {
    throw new Error("Integration type is required and must be a string");
  }
  
  try {
    const response = await axios.get<ConnectorAccessTokenResponse>(
      `/apps/${appId}/external-auth/tokens/${integrationType}`
    );
    
    return response.data; // FIX: Access the data property
  } catch (error) {
    if (axios.isAxiosError(error) && error.response?.status === 404) {
      throw new Error(`No connector found for integration type: ${integrationType}`);
    }
    throw error;
  }
}

3. Documentation

Add JSDoc comments with:

• Supported integration types
• Token expiration behavior
• Example usage
• Error scenarios

Summary

This PR is not ready to merge due to:

1. Critical runtime bug that makes the feature non-functional (4th review, still unfixed)
2. No test coverage for a security-sensitive feature
3. Poor TypeScript practices (using @ts-expect-error)

The same critical bug has been identified in all previous reviews but remains unfixed. Please address these issues, especially the runtime error, before proceeding.

Response to Reviewer's Question

@netanelgilad asked about the naming conventions. The current implementation uses:

• Module name: "connectors"
• Function: getAccessToken
• Parameter: "integrationType"

However, the API endpoint still references "external-auth". For consistency with user-facing documentation, consider aligning all naming to either "connectors" or "app connections" throughout the codebase, including the API endpoints.