-
Notifications
You must be signed in to change notification settings - Fork 5.6k
fix: updating Geckoboard auth #19610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Thank you so much for submitting this! We've added it to our backlog to review, and our team has been notified. |
|
Thanks for submitting this PR! When we review PRs, we follow the Pipedream component guidelines. If you're not familiar, here's a quick checklist:
|
Walkthrough_makeRequest in the Geckoboard component was changed to stop using axios Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/geckoboard/geckoboard.app.mjs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
🔇 Additional comments (2)
components/geckoboard/geckoboard.app.mjs (2)
38-38: LGTM! Headers destructuring added correctly.The addition of headers destructuring with a default empty object is appropriate for the new header-based authentication approach.
49-53: Headers configuration looks good with one consideration.The Authorization header is correctly formatted with the "Basic" scheme, and adding an explicit Content-Type header improves API consistency.
Minor consideration: The spread operator
...headersis placed after the default headers, which allows callers to override theAuthorizationandContent-Typeheaders. This provides flexibility but could potentially be a security concern if the Authorization header is accidentally overridden.If the intent is to prevent overriding these critical headers, consider reversing the order:
headers: { ...headers, Authorization: `Basic ${token}`, "Content-Type": "application/json", }However, if flexibility is desired (e.g., for testing or special cases), the current implementation is fine.
Please confirm whether allowing callers to override the Authorization header is intentional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/geckoboard/geckoboard.app.mjs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Lint Code Base
- GitHub Check: pnpm publish
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
🔇 Additional comments (1)
components/geckoboard/geckoboard.app.mjs (1)
34-40: LGTM: Headers parameter addition.The addition of
headers = {}to the destructuring correctly enables callers to provide custom headers while maintaining backward compatibility with a safe default.
michelle0927
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Ready for QA!
|
Hi everyone, all test cases are passed! Ready for release! Test reports
|
🔐 Fix Geckoboard API authentication in Pipedream integration
Summary
This PR fixes an authentication issue when calling the Geckoboard Datasets API from Pipedream.
Although Geckoboard uses HTTP Basic Auth with the API key as the username and an empty password, relying on Axios’ auth option caused the generated Authorization header to omit the required colon (:) in some cases. This resulted in 401 Unauthorized responses from the Geckoboard API.
What changed
Replaced Axios auth configuration with an explicit Authorization header.
Manually generate the Basic Auth token using api_key: to guarantee the required colon is always included.
Added explicit Content-Type: application/json header for consistency.
Why this change
Geckoboard strictly requires Basic Authentication in the format:
Authorization: Basic base64(API_KEY:)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.