-
Notifications
You must be signed in to change notification settings - Fork 2
Allow authenticating with bearer token #3
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
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAccepts Alertmanager webhooks authenticated either via query parameter Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
README.md(2 hunks)src/routes.js(1 hunks)
🔇 Additional comments (3)
src/routes.js (1)
8-23: Good dual authentication implementation.The dual authentication approach (query parameter or Bearer token) correctly addresses the PR objective of keeping credentials out of the Alertmanager UI. The use of the
authorizedflag makes the logic clear and maintainable, and returning 403 for unauthorized requests is appropriate.README.md (2)
22-26: LGTM - Clear configuration guidance.The updated wording clearly explains that configuration works via environment variables for both manual and Docker deployments.
48-51: Good security guidance on authentication methods.The documentation correctly warns users that the query parameter approach exposes credentials in the Alertmanager UI, which directly addresses the issue described in PLAT-34566. The recommendation to prefer the Authorization header is appropriate.
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: 0
🧹 Nitpick comments (1)
src/routes.js (1)
24-35: LGTM! Authentication logic is sound.Both query parameter and bearer token methods use timing-safe comparison correctly. The 403 fallback is appropriate.
Optional defensive improvement: Express may parse repeated query parameters (e.g.,
?secret=a&secret=b) as an array. While unlikely to be exploitable (length check would fail or Buffer conversion would mismatch), you could add explicit type validation:- if (passwordsEqual(req.query.secret, expectedSecret)) { + const querySecret = typeof req.query.secret === 'string' ? req.query.secret : undefined + if (passwordsEqual(querySecret, expectedSecret)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
README.md(2 hunks)src/routes.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧬 Code graph analysis (1)
src/routes.js (2)
src/app.js (1)
routes(3-3)tests/routes.tests.js (2)
req(15-15)res(16-18)
🔇 Additional comments (2)
src/routes.js (2)
4-8: LGTM! Timing-safe comparison correctly implemented.The
passwordsEqualfunction properly guards against null/undefined values and length mismatches before callingcrypto.timingSafeEqual. This addresses the timing attack concern from the previous review.
15-22: LGTM! Secret validation correctly implemented.The guard ensures requests cannot be accidentally authorized when both
req.query.secretandexpectedSecretare undefined. The 500 response appropriately indicates server misconfiguration.
PLAT-34566