Prevent return 302 on metadata resource urls - #187
mdthorpe-sc merged 3 commits into
Conversation
|
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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughReplaced upstream redirects for OAuth protected-resource well-known endpoints with local JSON metadata responses. Added Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/server-http.test.ts (1)
201-221: Env manipulation is unnecessary for this test.The
getProtectedResourceMetadatafunction only reads from theconfigobject passed to it, not fromprocess.env.AUTH_SERVER. The save/restore logic for the env var is dead code.♻️ Simplified test
test("builds protected-resource metadata from local server config", () => { - const previousAuthServer = process.env.AUTH_SERVER; - process.env.AUTH_SERVER = "auth.example.com"; - const config = { mcpServerUrl: "http://localhost:9292", authServerIssuerUrl: "https://auth.example.com", }; expect(getProtectedResourceMetadata(config)).toEqual({ resource: "http://localhost:9292/mcp", authorization_servers: ["https://auth.example.com"], scopes_supported: ["openid"], }); - - if (previousAuthServer === undefined) { - delete process.env.AUTH_SERVER; - } else { - process.env.AUTH_SERVER = previousAuthServer; - } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server-http.test.ts` around lines 201 - 221, The test unnecessarily manipulates process.env.AUTH_SERVER even though getProtectedResourceMetadata reads only from the passed config; remove the env save/restore and the assignment lines (the previousAuthServer capture, setting process.env.AUTH_SERVER, and the conditional restore/delete) and keep the config setup and expect assertion as-is so the test solely exercises getProtectedResourceMetadata with the provided config.src/server-http.ts (1)
174-183: Optional: Remove unusedapiBaseUrlfrom the type signature.Since the function no longer handles protected-resource paths,
apiBaseUrlis unused but still required by thePicktype.♻️ Suggested cleanup
export function getWellKnownRedirectUrl( path: string, - config: Pick<ServerConfig, "apiBaseUrl" | "authServerIssuerUrl">, + config: Pick<ServerConfig, "authServerIssuerUrl">, ): string | null {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server-http.ts` around lines 174 - 183, The function getWellKnownRedirectUrl declares config as Pick<ServerConfig, "apiBaseUrl" | "authServerIssuerUrl"> but no longer uses apiBaseUrl; update the type to only require authServerIssuerUrl (e.g., Pick<ServerConfig, "authServerIssuerUrl"> or a direct type with authServerIssuerUrl) so callers and the signature reflect the actual dependency, and adjust any call sites if needed to stop passing apiBaseUrl; keep the function body (and the reference to authServerIssuerUrl) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/server-http.test.ts`:
- Around line 201-221: The test unnecessarily manipulates
process.env.AUTH_SERVER even though getProtectedResourceMetadata reads only from
the passed config; remove the env save/restore and the assignment lines (the
previousAuthServer capture, setting process.env.AUTH_SERVER, and the conditional
restore/delete) and keep the config setup and expect assertion as-is so the test
solely exercises getProtectedResourceMetadata with the provided config.
In `@src/server-http.ts`:
- Around line 174-183: The function getWellKnownRedirectUrl declares config as
Pick<ServerConfig, "apiBaseUrl" | "authServerIssuerUrl"> but no longer uses
apiBaseUrl; update the type to only require authServerIssuerUrl (e.g.,
Pick<ServerConfig, "authServerIssuerUrl"> or a direct type with
authServerIssuerUrl) so callers and the signature reflect the actual dependency,
and adjust any call sites if needed to stop passing apiBaseUrl; keep the
function body (and the reference to authServerIssuerUrl) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8cfd4db4-4fa6-4d2d-be76-8556ad87da07
📒 Files selected for processing (4)
src/auth/oauth-integration.test.tssrc/auth/oauth.test.tssrc/server-http.test.tssrc/server-http.ts
1 participant
Hopefully a fix for Claude
Summary by CodeRabbit
New Features
Tests