[feature] SC-166737/improve app proxy security by restricting where token replacements can go #78
Conversation
…oken replacements can go
Reviewer's GuideThis PR enhances the TeamViewer integration by introducing a settingsInjection configuration to securely inject sensitive credentials into OAuth and API requests, and strengthens type safety by marking the placeholders object as const. Entity relationship diagram for settingsInjection configuration in manifest.jsonerDiagram
SETTINGS_INJECTION {
string client_id
string client_secret
string global_access_token
}
REQUEST_BODY {
string client_id
string client_secret
string refresh_token
}
SETTINGS_INJECTION ||--|| REQUEST_BODY : injects
Class diagram for updated placeholders constant in TeamViewer serviceclassDiagram
class placeholders {
+string GLOBAL_REFRESH_TOKEN
+string ACCESS_TOKEN
+string REFRESH_TOKEN
}
%% The object is now marked as const for type safety
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The settingsInjection objects for the two endpoints are identical—consider extracting them into a shared constant or reference to reduce duplication and ensure consistency across your manifest.
- Double-check that your placeholders (e.g. GLOBAL_REFRESH_TOKEN) align exactly with the body keys ("refresh_token") defined in settingsInjection to avoid runtime mismatches.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The settingsInjection objects for the two endpoints are identical—consider extracting them into a shared constant or reference to reduce duplication and ensure consistency across your manifest.
- Double-check that your placeholders (e.g. GLOBAL_REFRESH_TOKEN) align exactly with the body keys ("refresh_token") defined in settingsInjection to avoid runtime mismatches.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Build for commit bfbab01 deployed to: https://teamviewer-pr-78.ci.next.deskprodemo.com URLs: |
There was a problem hiding this comment.
Pull Request Overview
This pull request aims to improve app proxy security by adding settingsInjection configuration to TeamViewer integration endpoints, along with a TypeScript type safety improvement. However, there are several critical issues with the implementation that will prevent it from working correctly.
Key Issues Identified:
- The OAuth authorize endpoint (GET request) is configured to inject credentials into the request body, which is not supported for GET requests
- The broad API endpoint pattern injects sensitive credentials into all API calls unnecessarily, when only the token endpoint requires them
- The
global_access_tokenJSON object is being injected directly intorefresh_tokenbody parameters without extracting the actual refresh token field
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/services/teamviewer/constants.ts | Added as const assertion to placeholders object for improved type safety |
| manifest.json | Added settingsInjection configuration for OAuth authorize and API endpoints, but with incorrect mappings that will cause authentication failures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "global_access_token": { | ||
| "body": ["refresh_token"] | ||
| } |
There was a problem hiding this comment.
Same issue as line 64-66: The mapping of global_access_token to body parameter refresh_token appears incorrect. The global_access_token setting stores a JSON object containing both accessToken and refreshToken fields, not just the refresh token string.
This configuration will inject the entire JSON string where only the refresh token value is expected, causing OAuth token refresh requests to fail. The settingsInjection configuration needs to support JSON field extraction similar to the placeholder syntax used in the codebase: __global_access_token.json("[refreshToken]")__.
…oken replacements can go
This pull request introduces updates to the TeamViewer integration, focusing on improved configuration and type safety. The main changes are enhancements to how sensitive credentials are injected into requests, and a minor TypeScript improvement for constants.
TeamViewer integration improvements:
settingsInjectionconfiguration for both the OAuth authorization and API endpoints inmanifest.json, allowingclient_id,client_secret, andglobal_access_tokento be injected into the request body from settings.Code quality and type safety:
placeholdersobject insrc/services/teamviewer/constants.tsto useas const, ensuring stricter type safety for the exported constants.Summary by Sourcery
Improve TeamViewer integration by enabling credential injection via settingsInjection in the app proxy manifest and strengthen type safety of placeholder constants
New Features:
Enhancements: