Return GoCardless rate limit information on error #509
Conversation
actual-github-bot
bot
changed the title
matt-fidd
changed the title
matt-fidd
changed the title
WalkthroughThe changes in this pull request focus on enhancing the error handling logic within the Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 0
🧹 Outside diff range and nitpick comments (2)
src/app-gocardless/app-gocardless.js (2)
204-210: Add type checking and documentation for rate limit headers.The header extraction logic is good, but could be more robust with additional safeguards.
Consider these improvements:
- const headers = error.details?.response?.headers ?? {}; + // Extract rate limit headers from the error response + const headers = error.details?.response?.headers; + if (typeof headers !== 'object' || headers === null) { + headers = {}; + } const rateLimitHeaders = Object.fromEntries( Object.entries(headers).filter(([key]) => - key.startsWith('http_x_ratelimit'), + key.toLowerCase().startsWith('http_x_ratelimit'), ), );
212-216: Consider selective inclusion of rate limit headers.Currently, rate limit headers are included in all error responses, even for non-rate-limit related errors. This might lead to unnecessary response payload size.
Consider including rate limit headers only for rate limit related errors:
const sendErrorResponse = (data) => res.send({ status: 'ok', - data: { ...data, details: error.details, rateLimitHeaders }, + data: { + ...data, + details: error.details, + ...(data.error_type === 'RATE_LIMIT_EXCEEDED' && { rateLimitHeaders }), + }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/509.mdis excluded by!**/*.md
📒 Files selected for processing (1)
src/app-gocardless/app-gocardless.js(1 hunks)
🔇 Additional comments (1)
src/app-gocardless/app-gocardless.js (1)
204-216: Verify the impact on error logging.
The changes add new fields to error responses. Let's verify the logging impact and ensure sensitive information isn't exposed.
✅ Verification successful
Let me analyze the error logging impact further by checking the error handling structure and response format.
Error logging changes are safe and well-structured
The changes to include rate limit headers in error responses follow existing patterns and maintain security:
- Error responses have a consistent structure with typed errors (
error_type,error_code) - Rate limit headers are extracted only from response headers, not containing sensitive data
- Error logging uses
inspect()with proper error handling across different error types - The exposed
error.detailsis already part of the existing error response pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential sensitive information in error logging
# Look for console.log statements with error objects
rg -A 5 "console\.log.*error"
# Check for any existing rate limit header logging
rg -i "ratelimit|rate.?limit"
Length of output: 4331
Script:
#!/bin/bash
# Check error response structure and handling
ast-grep --pattern 'sendErrorResponse({
$$$
})'
# Check error details structure
ast-grep --pattern 'error.details'
# Inspect the inspect utility usage
rg -B 2 -A 2 "inspect\(.*error"
Length of output: 3978
Unfortunately we can only get this information from the error object from the nordigen-node package. This just adds a little more info to the error sent to the client.
Complimentary PR: actualbudget/actual#3895