Mirror: feat(mcp): re-enable oauth resource parameter and add discovery logging (#5331)#10
Mirror: feat(mcp): re-enable oauth resource parameter and add discovery logging (#5331)#10jeremylongshore merged 6 commits intomainfrom
Conversation
- Re-enable the RFC 8707 resource parameter in the authorization URL - Add warning logs to capture specific failures during RFC 8414 and OIDC metadata discovery to improve observability
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ 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 |
Summary of ChangesHello @jeremylongshore, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the OAuth flow by re-enabling a crucial resource parameter, which improves the specificity of authorization requests. Additionally, it introduces logging for metadata discovery failures, providing better visibility and debuggability for issues related to authorization server configuration. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Failed to generate code suggestions for PR |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request re-enables the RFC 8707 resource parameter in the OAuth flow and adds warning logs for metadata discovery failures. The changes are aligned with the PR's objectives. My review includes a high-severity comment regarding the removal of context about a potential provider-specific issue when re-enabling the resource parameter. I've also included two medium-severity suggestions to update comments to more accurately reflect the new logging behavior in error handling blocks.
| codeChallenge, | ||
| codeChallengeMethod: "S256", | ||
| // resource: serverUrl, // Disabled: Cloudflare doesn't support RFC 8707 resource parameter | ||
| resource: serverUrl, |
There was a problem hiding this comment.
Re-enabling the resource parameter is a good step for broader RFC 8707 compliance. However, the previous comment mentioned a specific issue with Cloudflare. Removing this comment loses valuable context that could be important for future debugging or for users interacting with Cloudflare-based services. To improve maintainability, please consider either:
- Adding a new comment explaining why it's safe to re-enable this (e.g., if the Cloudflare issue is resolved).
- If the issue might still exist for some providers, making the inclusion of the
resourceparameter conditional.
| // Ignore and try next | ||
| console.warn(`Failed to fetch RFC 8414 metadata:`, e) |
There was a problem hiding this comment.
With the addition of console.warn, the error is no longer fully ignored. To avoid confusion, it would be clearer to update the comment to reflect that the error is logged before proceeding.
| // Ignore and try next | |
| console.warn(`Failed to fetch RFC 8414 metadata:`, e) | |
| // Log error and try next discovery method | |
| console.warn(`Failed to fetch RFC 8414 metadata:`, e) |
| // Ignore fetch errors | ||
| console.warn(`Failed to fetch OIDC metadata:`, e) |
There was a problem hiding this comment.
Similar to the previous catch block, the comment // Ignore fetch errors is now slightly misleading since the error is being logged. Please update the comment to reflect the new behavior.
| // Ignore fetch errors | |
| console.warn(`Failed to fetch OIDC metadata:`, e) | |
| // Log fetch errors and continue | |
| console.warn(`Failed to fetch OIDC metadata:`, e) |
jeremylongshore
force-pushed
the
main
branch
from
4231140 to
fa13626
Compare
Review Summary
Checklist
Findings1. Stale Comment (Minor)Location: The comment above the authorization params still says: // Note: We don't include the 'resource' parameter by default as some servers
// (like Cloudflare) don't support RFC 8707 and return internal server errorBut the code now does include the resource parameter: resource: serverUrl,Suggestion: Update or remove the comment to reflect the new behavior. 2. Cloudflare Compatibility (Context)The original Kilo-Org#5297 PR disabled the resource parameter specifically because Cloudflare doesn't support RFC 8707. Re-enabling it will break Cloudflare-hosted MCP servers. This is likely intentional (RFC compliance > single provider compatibility), but worth noting in changelog. Verification
Code Changes// McpAuthorizationDiscovery.ts - Added logging
} catch (e) {
console.warn(`Failed to fetch RFC 8414 metadata:`, e)
}
// ...
} catch (e) {
console.warn(`Failed to fetch OIDC metadata:`, e)
}
// McpOAuthService.ts - Re-enabled resource param
-// resource: serverUrl, // Disabled: Cloudflare doesn't support RFC 8707
+resource: serverUrl,RecommendationAPPROVE — Clean RFC 8707 compliance change with improved observability. The stale comment is minor and can be fixed in a follow-up.
|
Review Journal: kilocode Kilo-Org#5331
SummaryThis PR is a follow-up to the major MCP OAuth implementation in Kilo-Org#5297. It makes two changes: (1) re-enables the RFC 8707 First ImpressionsA 4-line PR with maintainer approval — should be fast. The What I Looked At
AnalysisChange 1: Re-enable resource parameterThe RFC 8707 resource parameter tells the authorization server which API the token is intended for. It was disabled in Kilo-Org#5297 because:
This PR re-enables it, prioritizing RFC compliance over Cloudflare compatibility. This is the right trade-off — RFC 8707 is a security feature that prevents token confusion attacks, and Cloudflare should fix their implementation rather than projects disabling security features. Change 2: Discovery loggingAdding Stale commentThe kiloconnect bot correctly identified that the comment above the authorization params contradicts the new code. The comment says "we don't include the resource parameter" but the code now includes it. Minor issue, can be fixed in follow-up. Bot Review Synthesis
Gemini provided a clean summary but no actionable findings. CodeRabbit hit hourly rate limits from our batch work. Verification
Lessons Learned
RecommendationAPPROVE — Clean, focused enhancement with maintainer sign-off. The stale comment is minor. Review #7 of 75 | Methodology: jeremylongshore/kilocode/.reviews | Reviewed with Claude Code |
2 participants
Mirror of Kilo-Org#5331
This PR mirrors the upstream change for multi-AI review analysis.
Changes
Bot Review Checklist