Feat/clean dropbox oauth#737
Conversation
…xt/plain Bug: 498990498
…er Action API compatibility
Refactored the getAccessTokenFromCode method in dropbox.ts to use gaxios and transmit OAuth parameters (code, client_secret, etc.) in the HTTP request body encoded as application/x-www-form-urlencoded, instead of passing them as URL query parameters in a POST request
There was a problem hiding this comment.
Code Review
This pull request enhances OAuth state management and security across several actions. Key updates include adding support for both encrypted and unencrypted state payloads in Airtable, migrating Dropbox token exchange to a more secure form-encoded body request using gaxios to prevent secret leakage in logs, and fixing a bug where state_url was not correctly mapped from the request root. Additionally, the oauthMaybeEncryptTokens method now returns raw objects instead of strings when encryption is disabled to ensure gaxios sets the correct Content-Type header. Review feedback identifies critical safety issues in the Airtable implementation where null checks are missing after token extraction and suggests simplifying the authentication check logic to improve robustness.
| const stateJson = await this.oauthExtractTokensFromStateJson(request.params.state_json, request.webhookId); | ||
| tokens = airtable_tokens_1.AirtableTokens.fromJson(stateJson); | ||
| accessToken = tokens.access_token; | ||
| const encrypted = await this.oauthMaybeEncryptTokens(new airtable_tokens_1.AirtableTokens(tokens.refresh_token, accessToken, tokens.redirectUri), request.webhookId); | ||
| state.data = typeof encrypted === "string" ? encrypted : JSON.stringify(encrypted); |
There was a problem hiding this comment.
The call to AirtableTokens.fromJson(stateJson) is unsafe because oauthExtractTokensFromStateJson returns null if decryption fails or the JSON is malformed. This will cause a crash when attempting to access properties on null. You should ensure stateJson is truthy before proceeding.
| const stateJson = await this.oauthExtractTokensFromStateJson(request.params.state_json, request.webhookId); | |
| tokens = airtable_tokens_1.AirtableTokens.fromJson(stateJson); | |
| accessToken = tokens.access_token; | |
| const encrypted = await this.oauthMaybeEncryptTokens(new airtable_tokens_1.AirtableTokens(tokens.refresh_token, accessToken, tokens.redirectUri), request.webhookId); | |
| state.data = typeof encrypted === "string" ? encrypted : JSON.stringify(encrypted); | |
| const stateJson = await this.oauthExtractTokensFromStateJson(request.params.state_json, request.webhookId); | |
| if (stateJson) { | |
| tokens = airtable_tokens_1.AirtableTokens.fromJson(stateJson); | |
| accessToken = tokens.access_token; | |
| const encrypted = await this.oauthMaybeEncryptTokens(new airtable_tokens_1.AirtableTokens(tokens.refresh_token, accessToken, tokens.redirectUri), request.webhookId); | |
| state.data = typeof encrypted === "string" ? encrypted : JSON.stringify(encrypted); | |
| } |
| const stateJson = await this.oauthExtractTokensFromStateJson(request.params.state_json, request.webhookId); | ||
| tokens = airtable_tokens_1.AirtableTokens.fromJson(stateJson); | ||
| accessToken = tokens.access_token; |
There was a problem hiding this comment.
Similar to the issue in the execute method, AirtableTokens.fromJson(stateJson) will throw an error if stateJson is null. A check for a truthy stateJson is required here as well.
| const stateJson = await this.oauthExtractTokensFromStateJson(request.params.state_json, request.webhookId); | |
| tokens = airtable_tokens_1.AirtableTokens.fromJson(stateJson); | |
| accessToken = tokens.access_token; | |
| const stateJson = await this.oauthExtractTokensFromStateJson(request.params.state_json, request.webhookId); | |
| if (stateJson) { | |
| tokens = airtable_tokens_1.AirtableTokens.fromJson(stateJson); | |
| accessToken = tokens.access_token; | |
| } |
| const parsedState = JSON.parse(request.params.state_json); | ||
| if (parsedState.cid && parsedState.payload) { | ||
| const stateJson = await this.oauthExtractTokensFromStateJson(request.params.state_json, request.webhookId); | ||
| return !!stateJson; | ||
| } | ||
| else { | ||
| // Keeping the literal old code to ensure no regressions for unencrypted payloads | ||
| const tokens = airtable_tokens_1.AirtableTokens.fromJson(parsedState); | ||
| return !!tokens.access_token; | ||
| } |
There was a problem hiding this comment.
The JSON.parse call on request.params.state_json is unsafe as it is not wrapped in a try...catch block, which could lead to unhandled exceptions if the input is malformed. Furthermore, the logic can be significantly simplified by relying on oauthExtractTokensFromStateJson, which already handles both encrypted and unencrypted states safely and returns the parsed object.
| const parsedState = JSON.parse(request.params.state_json); | |
| if (parsedState.cid && parsedState.payload) { | |
| const stateJson = await this.oauthExtractTokensFromStateJson(request.params.state_json, request.webhookId); | |
| return !!stateJson; | |
| } | |
| else { | |
| // Keeping the literal old code to ensure no regressions for unencrypted payloads | |
| const tokens = airtable_tokens_1.AirtableTokens.fromJson(parsedState); | |
| return !!tokens.access_token; | |
| } | |
| const stateJson = await this.oauthExtractTokensFromStateJson(request.params.state_json, request.webhookId); | |
| if (stateJson) { | |
| const tokens = airtable_tokens_1.AirtableTokens.fromJson(stateJson); | |
| return !!tokens.access_token; | |
| } |
Refactored the getAccessTokenFromCode method in dropbox.ts to use gaxios and transmit OAuth parameters (code, client_secret, etc.) in the HTTP request body encoded as application/x-www-form-urlencoded, instead of passing them as URL query parameters in a POST request