fix: align export proxy auth contracts#403
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello, 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 refines the authentication and base URL handling within the web API proxy layer. It standardizes the fallback mechanism for backend API URLs and ensures that protected endpoints, specifically the paper export functionality, correctly propagate user session authentication. These changes enhance the robustness and consistency of API interactions by aligning with established environment variable contracts and improving secure access to backend services. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
📝 WalkthroughWalkthroughThe pull request introduces authentication header utilities for server-side session token application with fallback support, including unit tests and integration with the papers export endpoint. The changes add session metadata handling, Authorization header preservation logic, and comprehensive test coverage for both authentication utilities and the export proxy endpoint. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Vercel Preview
|
|
![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 effectively aligns authentication contracts for the export proxy by updating the base URL fallback logic and ensuring authentication headers are forwarded correctly. The changes improve type safety with the new readSessionMetadata utility, and the new tests provide good coverage. My review includes a couple of suggestions to refactor the tests to use more idiomatic Vitest mocking patterns, which will improve their maintainability.
| afterEach(() => { | ||
| if (originalBackendBaseUrl === undefined) { | ||
| delete process.env.BACKEND_BASE_URL | ||
| } else { | ||
| process.env.BACKEND_BASE_URL = originalBackendBaseUrl | ||
| } | ||
|
|
||
| if (originalPaperbotApiBaseUrl === undefined) { | ||
| delete process.env.PAPERBOT_API_BASE_URL | ||
| } else { | ||
| process.env.PAPERBOT_API_BASE_URL = originalPaperbotApiBaseUrl | ||
| } | ||
| }) |
There was a problem hiding this comment.
The manual restoration of environment variables in afterEach is verbose and can be simplified using Vitest's vi.stubEnv and vi.unstubAllEnvs. This makes the test setup cleaner and less prone to errors.
Your test case can be updated to use vi.stubEnv to set variables, and this afterEach block can be simplified to just call vi.unstubAllEnvs().
Example of updated test case:
it("falls back to PAPERBOT_API_BASE_URL when BACKEND_BASE_URL is unset", () => {
delete process.env.BACKEND_BASE_URL;
vi.stubEnv("PAPERBOT_API_BASE_URL", "https://paperbot-api.example.com");
expect(backendBaseUrl()).toBe("https://paperbot-api.example.com");
}); afterEach(() => {
vi.unstubAllEnvs()
})| afterEach(() => { | ||
| global.fetch = originalFetch | ||
| }) |
There was a problem hiding this comment.
Instead of manually saving and restoring global.fetch, it's better to use Vitest's built-in mocking capabilities. You can use vi.spyOn(global, 'fetch') to mock fetch within your test, and then call vi.restoreAllMocks() here to automatically clean up.
This would look like this in your test:
const fetchSpy = vi.spyOn(global, 'fetch').mockResolvedValue(
new Response("bibtex-body", {
status: 200,
headers: {
"content-type": "application/x-bibtex",
"content-disposition": "attachment; filename=papers.bib",
},
}),
);This avoids direct manipulation of global objects and the type casting.
| afterEach(() => { | |
| global.fetch = originalFetch | |
| }) | |
| afterEach(() => { | |
| vi.restoreAllMocks() | |
| }) |
There was a problem hiding this comment.
Pull request overview
Aligns the Next.js web proxy layer’s auth/header behavior with the documented env contract and ensures the papers export proxy forwards the active user session to the protected backend export endpoint.
Changes:
- Forward backend auth headers in the
/api/papers/exportproxy route viawithBackendAuth. - Update
backendBaseUrl()to fall back toPAPERBOT_API_BASE_URLwhenBACKEND_BASE_URLis unset. - Add focused Vitest coverage for the auth header helper and export proxy behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| web/src/app/api/papers/export/route.ts | Forwards backend auth headers when proxying export requests. |
| web/src/app/api/papers/export/route.test.ts | Tests that export proxy calls withBackendAuth and forwards returned headers to fetch. |
| web/src/app/api/_utils/auth-headers.ts | Adds PAPERBOT_API_BASE_URL fallback and safer session metadata extraction for auth header forwarding. |
| web/src/app/api/_utils/auth-headers.test.ts | Tests base URL fallback and auth header selection precedence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/app/api/_utils/auth-headers.ts (1)
35-40:⚠️ Potential issue | 🔴 CriticalDo not trust client
Authorizationby default in proxy auth helper.Line 37–39 returns early with caller-controlled credentials, which lets requests choose upstream identity instead of enforcing the current server session. This weakens auth boundaries for every route that uses this helper (including the shared research proxy base).
🔒 Suggested hardening (secure default + explicit opt-in)
export async function withBackendAuth( req: Request, base: HeadersInit = {}, + options: { allowIncomingAuthorization?: boolean } = {}, ): Promise<HeadersInit> { const headers = new Headers(base) // Prefer client-provided Authorization header if present const incoming = req.headers.get("authorization") - if (incoming) { + if (options.allowIncomingAuthorization && incoming) { headers.set("authorization", incoming) return headers }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/api/_utils/auth-headers.ts` around lines 35 - 40, The helper currently returns early and trusts caller-controlled incoming auth via req.headers.get("authorization") (variables: incoming, headers), which lets clients override the server session; change this to a secure default: do not set or return the incoming Authorization header unless an explicit opt-in flag is passed (e.g., allowClientAuthorization) to the function and the header is validated against the server session or a strict whitelist; remove the early return, use the server-derived session/token as the default value, and only override headers.set("authorization", incoming) when the explicit flag is true and validation succeeds so upstream identity cannot be chosen by the client.
🧹 Nitpick comments (1)
web/src/app/api/papers/export/route.test.ts (1)
18-18: UseglobalThisinstead ofglobalfor fetch stubbing/restoration.Replace all three instances of
global.fetchwithglobalThis.fetch(lines 18, 25, 43). TheglobalThisreference is the standardized, environment-agnostic way to access the global object and aligns with TypeScript and linter best practices.♻️ Suggested update
- const originalFetch = global.fetch + const originalFetch = globalThis.fetch ... - global.fetch = originalFetch + globalThis.fetch = originalFetch ... - global.fetch = fetchMock as typeof fetch + globalThis.fetch = fetchMock as typeof fetch🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/api/papers/export/route.test.ts` at line 18, Replace usages of global.fetch with globalThis.fetch in the test where fetch is saved, stubbed and restored: update the declaration that captures the original fetch (originalFetch), the place where fetch is mocked/assigned for the test, and where it is restored after tests; specifically change references in the setup/teardown around originalFetch and any direct global.fetch assignments to use globalThis.fetch to be environment-agnostic and satisfy TypeScript/linter expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@web/src/app/api/_utils/auth-headers.ts`:
- Around line 35-40: The helper currently returns early and trusts
caller-controlled incoming auth via req.headers.get("authorization") (variables:
incoming, headers), which lets clients override the server session; change this
to a secure default: do not set or return the incoming Authorization header
unless an explicit opt-in flag is passed (e.g., allowClientAuthorization) to the
function and the header is validated against the server session or a strict
whitelist; remove the early return, use the server-derived session/token as the
default value, and only override headers.set("authorization", incoming) when the
explicit flag is true and validation succeeds so upstream identity cannot be
chosen by the client.
---
Nitpick comments:
In `@web/src/app/api/papers/export/route.test.ts`:
- Line 18: Replace usages of global.fetch with globalThis.fetch in the test
where fetch is saved, stubbed and restored: update the declaration that captures
the original fetch (originalFetch), the place where fetch is mocked/assigned for
the test, and where it is restored after tests; specifically change references
in the setup/teardown around originalFetch and any direct global.fetch
assignments to use globalThis.fetch to be environment-agnostic and satisfy
TypeScript/linter expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 47f09541-1284-4b04-8d84-c083b456a6e0
📒 Files selected for processing (4)
web/src/app/api/_utils/auth-headers.test.tsweb/src/app/api/_utils/auth-headers.tsweb/src/app/api/papers/export/route.test.tsweb/src/app/api/papers/export/route.ts
Summary
web/src/app/api/_utils/auth-headers.tsfall back toPAPERBOT_API_BASE_URL, matching the rest of the web proxy layer and documented env contractweb/src/app/api/papers/export/route.tsso the protected export endpoint can use the current user sessionValidation
cd web && npm install --no-audit --no-fundcd web && npx vitest run src/app/api/_utils/auth-headers.test.ts src/app/api/papers/export/route.test.tscd web && npx eslint src/app/api/_utils/auth-headers.ts src/app/api/_utils/auth-headers.test.ts src/app/api/papers/export/route.ts src/app/api/papers/export/route.test.tsSummary by CodeRabbit
Tests
Bug Fixes