Dev Merge #100
Dev Merge #100
Conversation
Updated APICall, AccessTokenManager, and OAuth.helper to support both legacy and new OAuth token storage structures. Added logic to distinguish between structures, improved parameter extraction for OAuth1 signatures, and ensured correct token updates and retrievals for both formats. Also replaced usage of oauthService with oauth_con_id for more robust identification.
Removed unnecessary console.log statements from OAuth.helper.ts for cleaner output. Updated AccessTokenManager to store expiration timestamp as a string if available, improving consistency in token expiration handling.
AccessTokenManager now correctly persists rotated refresh tokens and handles expiration timestamps with nullish checks. OAuth.helper.ts avoids logging sensitive OAuth config data in plaintext and refines token data structure updates to better preserve existing fields.
Corrects the assignment of expires_in to preserve 0 and undefined values, and updates the in-memory refresh token if the provider rotates it during access token refresh.
Refactor OAuth token handling for new structure support
WalkthroughIntroduces oauth_con_id config; reroutes OAuth token/header handling to use it. Adds dual-structure token support (old vs new) across retrieval, refresh, and storage. Refactors OAuth1/2 parameter extraction and signing. Updates AccessTokenManager constructor and save logic. Removes rootUrl from OAuth flows. Bumps core and sdk versions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as APICall.class
participant Helper as OAuth.helper
participant Vault as Vault Store
participant ATM as AccessTokenManager
participant API as Target API
Caller->>Helper: handleOAuthHeaders(agent, config, reqConfig, logger)
Helper->>Vault: retrieveOAuthTokens(oauth_con_id)
Vault-->>Helper: { tokensData, keyId, isNewStructure }
alt OAuth2 (service != oauth1)
Helper->>ATM: new AccessTokenManager(..., tokensData, ..., isNewStructure)
ATM->>API: POST token_url (refresh or client credentials)
API-->>ATM: { access_token, refresh_token?, expires_in }
ATM->>Vault: save updatedData (structure-aware)
ATM-->>Helper: Bearer token
Helper-->>Caller: headers{ Authorization: Bearer ... }
else OAuth1
Helper->>Helper: Build base URL, collect params/body hash
Helper-->>Caller: headers{ Authorization: OAuth ... }
end
Caller->>API: Request with merged headers
API-->>Caller: Response
sequenceDiagram
autonumber
participant Helper as OAuth.helper
participant Vault as Vault Store
participant CC as ClientCredentials Flow
note over Helper: Client credentials path
Helper->>Vault: get tokensData (new/old structure)
alt New structure
Helper->>CC: request token using auth_settings
CC-->>Helper: access_token, expires_in
Helper->>Vault: update auth_data.primary/expires_in<br/>and auth_settings
else Old structure
Helper->>CC: request token using oauth_info
CC-->>Helper: access_token, expires_in
Helper->>Vault: update primary/expires_in and oauth_info
end
Helper-->>Helper: Return access_token
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/core/src/Components/APICall/APICall.class.ts (2)
100-100: Bug: headers transformed into an array; axios expects an object.
Using concat() yields an array of header objects, breaking axios.request. Merge as an object.Apply:
- reqConfig.headers = (await parseHeaders(input, config, agent)).concat({ ...headers }); + reqConfig.headers = { ...(await parseHeaders(input, config, agent)), ...headers };
100-122: Ensure consistent headers representation
- ForkedAgent.class.ts:39 uses
agentRequest.headers = []- APICall.class.ts:121 uses
reqConfig.headers = reqConfig.headers.concat(...)
Unifyheadersas either an object map or an array of header entries and update both sites plus the related type definitions accordingly.packages/core/src/Components/APICall/OAuth.helper.ts (2)
272-333: Standardize handleOAuthHeaders to return only OAuth header patch.
Currently OAuth1 branch returns merged headers while OAuth2 returns only Authorization; make it consistent so callers can merge once.- const oauthHeader = buildOAuth1Header( + const oauthHeader = buildOAuth1Header( reqConfig.url, reqConfig.method, { consumerKey: oAuthConfig.consumerKey, consumerSecret: oAuthConfig.consumerSecret, token: oauthTokens.primaryToken, tokenSecret: oauthTokens.secondaryToken, }, additionalParams ); - - headers = { ...reqConfig.headers, ...oauthHeader }; + headers = oauthHeader; // return only OAuth header patch logger.debug('OAuth1 access token check success.');Follow-up in APICall.class.ts already merges with existing headers via object spread.
378-380: Add timeout to token fetch.
Prevent hangs on identity endpoints.- const response = await axios.post(tokenURL, params.toString(), { - headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, - }); + const response = await axios.post( + tokenURL, + params.toString(), + { headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, timeout: 10000 } + );
🧹 Nitpick comments (1)
packages/core/src/Components/APICall/APICall.class.ts (1)
127-131: Redact sensitive headers in debug logs.
reqConfig may include Authorization; avoid logging it verbatim.- logger.debug('Making API call', { ...reqConfig, data: dataForDebug }); + const { headers, ...rest } = reqConfig; + logger.debug('Making API call', { ...rest, headers: '[redacted]', data: dataForDebug });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
packages/sdk/src/Components/generated/APICall.tsis excluded by!**/generated/**
📒 Files selected for processing (5)
packages/core/package.json(1 hunks)packages/core/src/Components/APICall/APICall.class.ts(2 hunks)packages/core/src/Components/APICall/AccessTokenManager.ts(2 hunks)packages/core/src/Components/APICall/OAuth.helper.ts(8 hunks)packages/sdk/package.json(1 hunks)
🔇 Additional comments (5)
packages/core/package.json (1)
3-3: LGTM: version bump only.
No issues spotted. Ensure CHANGELOG reflects 1.5.51.packages/sdk/package.json (1)
3-3: LGTM: version bump only.
No issues spotted. Ensure release notes updated to 1.0.46.packages/core/src/Components/APICall/AccessTokenManager.ts (2)
112-139: Token save path looks correct for dual structures.
Merge logic preserves new (auth_data) and old layouts; refresh_token rotation handled. Good.
147-156: State refresh after save is consistent.
Updates in-memory tokens and preserves 0 for expires_in. Good.packages/core/src/Components/APICall/OAuth.helper.ts (1)
24-135: OAuth1 param extraction hardening looks solid.
Covers query, form, body hash, and multipart cases with sensible logging. Good.
| if (config?.data?.oauth_con_id !== '' && config?.data?.oauth_con_id !== 'None') { | ||
| const additionalParams = extractAdditionalParamsForOAuth1(reqConfig); | ||
| const oauthHeaders = await generateOAuthHeaders(agent, config, reqConfig, logger, additionalParams, rootUrl); | ||
| const oauthHeaders = await generateOAuthHeaders(agent, config, reqConfig, logger, additionalParams); | ||
| //reqConfig.headers = { ...reqConfig.headers, ...oauthHeaders }; | ||
| reqConfig.headers = reqConfig.headers.concat({ ...oauthHeaders }); | ||
| } |
There was a problem hiding this comment.
Fix OAuth gating and header merge.
Undefined oauth_con_id currently passes the condition; also avoid concat on headers.
- if (config?.data?.oauth_con_id !== '' && config?.data?.oauth_con_id !== 'None') {
+ if (typeof config?.data?.oauth_con_id === 'string' && config.data.oauth_con_id.trim() && config.data.oauth_con_id !== 'None') {
const additionalParams = extractAdditionalParamsForOAuth1(reqConfig);
- const oauthHeaders = await generateOAuthHeaders(agent, config, reqConfig, logger, additionalParams);
- //reqConfig.headers = { ...reqConfig.headers, ...oauthHeaders };
- reqConfig.headers = reqConfig.headers.concat({ ...oauthHeaders });
+ const oauthHeaders = await generateOAuthHeaders(agent, config, reqConfig, logger, additionalParams);
+ reqConfig.headers = { ...(reqConfig.headers || {}), ...oauthHeaders };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (config?.data?.oauth_con_id !== '' && config?.data?.oauth_con_id !== 'None') { | |
| const additionalParams = extractAdditionalParamsForOAuth1(reqConfig); | |
| const oauthHeaders = await generateOAuthHeaders(agent, config, reqConfig, logger, additionalParams, rootUrl); | |
| const oauthHeaders = await generateOAuthHeaders(agent, config, reqConfig, logger, additionalParams); | |
| //reqConfig.headers = { ...reqConfig.headers, ...oauthHeaders }; | |
| reqConfig.headers = reqConfig.headers.concat({ ...oauthHeaders }); | |
| } | |
| if (typeof config?.data?.oauth_con_id === 'string' && config.data.oauth_con_id.trim() && config.data.oauth_con_id !== 'None') { | |
| const additionalParams = extractAdditionalParamsForOAuth1(reqConfig); | |
| const oauthHeaders = await generateOAuthHeaders(agent, config, reqConfig, logger, additionalParams); | |
| reqConfig.headers = { ...(reqConfig.headers || {}), ...oauthHeaders }; | |
| } |
🤖 Prompt for AI Agents
In packages/core/src/Components/APICall/APICall.class.ts around lines 117-122,
the condition allows undefined oauth_con_id to pass and headers are being
concatenated as if they were an array; update the gating to check for a truthy,
non-empty, non-'None' value (e.g. config?.data?.oauth_con_id &&
config.data.oauth_con_id !== 'None') and replace the concat with an object merge
that ensures headers is an object (e.g. reqConfig.headers = {
...(reqConfig.headers || {}), ...oauthHeaders }) so OAuth headers are only added
when present and merged correctly.
| export const retrieveOAuthTokens = async (agent, config) => { | ||
| let tokenKey: any = null; | ||
| try { | ||
| tokenKey = `OAUTH_${config.componentId ?? config.id}_TOKENS`; | ||
| tokenKey = config?.data?.oauth_con_id; | ||
|
|
||
| try { |
There was a problem hiding this comment.
Guard when oauth_con_id is missing.
Avoid vault get with undefined key.
export const retrieveOAuthTokens = async (agent, config) => {
let tokenKey: any = null;
try {
- tokenKey = config?.data?.oauth_con_id;
+ tokenKey = config?.data?.oauth_con_id;
+ if (!tokenKey || tokenKey === 'None') {
+ throw new Error('oauth_con_id is required to retrieve OAuth tokens');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const retrieveOAuthTokens = async (agent, config) => { | |
| let tokenKey: any = null; | |
| try { | |
| tokenKey = `OAUTH_${config.componentId ?? config.id}_TOKENS`; | |
| tokenKey = config?.data?.oauth_con_id; | |
| try { | |
| export const retrieveOAuthTokens = async (agent, config) => { | |
| let tokenKey: any = null; | |
| try { | |
| tokenKey = config?.data?.oauth_con_id; | |
| if (!tokenKey || tokenKey === 'None') { | |
| throw new Error('oauth_con_id is required to retrieve OAuth tokens'); | |
| } | |
| try { | |
| // ... existing inner try block code | |
| } | |
| // ... | |
| } catch (err) { | |
| // ... | |
| } | |
| }; |
🤖 Prompt for AI Agents
In packages/core/src/Components/APICall/OAuth.helper.ts around lines 176 to 181,
the code uses config?.data?.oauth_con_id directly and passes it to the vault get
which can be undefined; add a guard so you do not call the vault with an
undefined key: read tokenKey = config?.data?.oauth_con_id and if it is
null/undefined (or empty string) then return a sensible value or throw a
descriptive error (or log and return) instead of proceeding to vault.get; ensure
subsequent code assumes tokenKey is a non-empty string (narrow the type) and add
a unit/test or comment that vault.get will only be called with a valid key.
| updatedData.oauth_info = { | ||
| oauth_keys_prefix: `OAUTH_${config?.data?.oauth_con_id?.split('_')[1]}`, | ||
| service: 'oauth2_client_credentials', | ||
| tokenURL, | ||
| clientID, | ||
| clientSecret | ||
| }; |
There was a problem hiding this comment.
Avoid 'OAUTH_undefined' when deriving oauth_keys_prefix (old structure).
Compute prefix safely as done for new structure.
- updatedData.oauth_info = {
- oauth_keys_prefix: `OAUTH_${config?.data?.oauth_con_id?.split('_')[1]}`,
+ const parts = String(config?.data?.oauth_con_id ?? '').split('_');
+ const prefixSuffix = parts.length > 1 ? parts[1] : parts[0];
+ updatedData.oauth_info = {
+ oauth_keys_prefix: prefixSuffix ? `OAUTH_${prefixSuffix}` : undefined,
service: 'oauth2_client_credentials',
tokenURL,
clientID,
clientSecret
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| updatedData.oauth_info = { | |
| oauth_keys_prefix: `OAUTH_${config?.data?.oauth_con_id?.split('_')[1]}`, | |
| service: 'oauth2_client_credentials', | |
| tokenURL, | |
| clientID, | |
| clientSecret | |
| }; | |
| // Safely derive the OAuth key prefix to avoid "OAUTH_undefined" | |
| const parts = String(config?.data?.oauth_con_id ?? '').split('_'); | |
| const prefixSuffix = parts.length > 1 ? parts[1] : parts[0]; | |
| updatedData.oauth_info = { | |
| oauth_keys_prefix: prefixSuffix ? `OAUTH_${prefixSuffix}` : undefined, | |
| service: 'oauth2_client_credentials', | |
| tokenURL, | |
| clientID, | |
| clientSecret | |
| }; |
🤖 Prompt for AI Agents
In packages/core/src/Components/APICall/OAuth.helper.ts around lines 425 to 431,
the oauth_keys_prefix may become "OAUTH_undefined" when config.data.oauth_con_id
doesn't follow the expected old structure; compute the suffix safely and avoid
using split result directly. Extract a safeSuffix: if config?.data?.oauth_con_id
is falsy use an empty string, otherwise try splitting and use the second segment
if present, else fall back to the whole oauth_con_id (or empty string), then set
oauth_keys_prefix to `OAUTH_${safeSuffix}` so "undefined" is never inserted.
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores