Fix user fetch network error #10
Conversation
…URLs The VITE_API_URL env var can have a trailing slash which causes double-slash URLs like //api/users/me when paths start with /. This was causing requests to fail or go to incorrect endpoints. Fixes both api.ts (apiFetch utility) and api-client.ts (Eden Treaty client).
The CORS origin callback was incorrectly returning an array of allowed
origins. According to Elysia CORS plugin documentation, the callback must
return a boolean indicating if the request's origin is allowed.
Changes:
- Get incoming origin from request.headers.get("origin")
- Compare against allowed origins list
- Return true/false (boolean) instead of string[]
- Add logging for rejected origins to aid debugging
- Allow requests without origin header (same-origin/non-browser)
This fixes CORS preflight failures where no Access-Control-Allow-Origin
header was being returned.
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 @Dexploarer, 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 focuses on enhancing the robustness and reliability of network communication within the application. It addresses potential network errors, particularly those related to user data fetching, by refining the server's Cross-Origin Resource Sharing (CORS) mechanism and standardizing the client-side API URL construction. These changes aim to ensure that API requests are correctly routed, authorized, and free from common URL-related issues. Highlights
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
|
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| .use(securityHeaders) | ||
| .use( | ||
| cors({ | ||
| origin: (request) => { | ||
| origin: ({ request }) => { | ||
| // Get the incoming origin from the request | ||
| const incomingOrigin = request.headers.get("origin"); | ||
|
|
||
| // No origin header means same-origin request or non-browser client | ||
| if (!incomingOrigin) { | ||
| return true; | ||
| } | ||
|
|
||
| // Build allowed origins list | ||
| const allowedOrigins: string[] = []; | ||
|
|
There was a problem hiding this comment.
Security/Logic Issue:
The allowed origins list is rebuilt for every request, and the check for allowed origins uses strict equality (allowedOrigins.includes(incomingOrigin)). This may not account for subdomains or protocol variations, potentially causing legitimate requests to be rejected. Consider using a more flexible matching strategy, such as pattern matching or regular expressions, to allow for subdomains or protocol differences if required.
Recommended Solution:
Replace strict equality with a more robust check, for example:
const isAllowed = allowedOrigins.some(origin => incomingOrigin === origin || incomingOrigin.endsWith('.' + origin));Or use a library for CORS origin matching if complex rules are needed.
| if (allowedOrigins.length === 0) { | ||
| logger.warn( | ||
| { context: "cors" }, | ||
| "No CORS origins configured - rejecting cross-origin requests", | ||
| { context: "cors", origin: incomingOrigin }, | ||
| "No CORS origins configured - rejecting cross-origin request", | ||
| ); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Error Handling/UX Issue:
When no allowed origins are configured in production, the code logs a warning and returns false, but does not provide a clear error response to the client. This could lead to silent failures for cross-origin requests.
Recommended Solution:
Consider returning a more informative error response or status code to the client when CORS is misconfigured, to aid debugging and improve developer experience. For example, you could throw an error or return a custom message indicating that CORS is not configured.
| signal: signal ?? controller.signal | ||
| }) | ||
| return response | ||
| signal: signal ?? controller.signal, |
There was a problem hiding this comment.
Potential Timeout Bypass Due to Signal Precedence
The code uses signal: signal ?? controller.signal, which means if a caller provides a signal, the internal timeout logic using controller.signal will be ignored. This can result in requests not being aborted after the specified timeout, potentially causing resource leaks or hanging requests.
Recommended Solution:
Consider always using the internal controller.signal for timeout, and if an external signal is provided, listen for its abort event and propagate it to the controller:
if (signal) {
signal.addEventListener('abort', () => controller.abort(signal.reason), { once: true });
}Then always pass controller.signal to fetch.
| const headers: Record<string, string> = {}; | ||
|
|
||
| if (rest.headers) { | ||
| if (rest.headers instanceof Headers) { | ||
| // Headers object - iterate over entries | ||
| rest.headers.forEach((value, key) => { | ||
| headers[key] = value | ||
| }) | ||
| headers[key] = value; | ||
| }); | ||
| } else if (Array.isArray(rest.headers)) { | ||
| // Array of [key, value] tuples | ||
| rest.headers.forEach(([key, value]) => { | ||
| headers[key] = value | ||
| }) | ||
| headers[key] = value; | ||
| }); | ||
| } else { | ||
| // Plain object | ||
| Object.assign(headers, rest.headers) | ||
| Object.assign(headers, rest.headers); | ||
| } | ||
| } |
There was a problem hiding this comment.
Header Key Case Sensitivity and Overwrite Risk
The header merging logic does not normalize header keys to a consistent case, which can result in duplicate headers (e.g., both 'Authorization' and 'authorization') or missed overwrites. This can lead to security issues if the intended header is not set or is overridden unexpectedly.
Recommended Solution:
Normalize all header keys to a consistent case (e.g., lowercase) when merging, and check for the presence of the authorization header in a case-insensitive manner:
const normalizedHeaders = Object.fromEntries(
Object.entries(headers).map(([k, v]) => [k.toLowerCase(), v])
);Then check for 'authorization' only.
PR Compliance Guide 🔍(Compliance updated until commit 1ed10cd)Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label Previous compliance checksCompliance check up to commit 72df026
|
||||||||||||||||||||||||||||||||||||||||||||||
Greptile OverviewGreptile SummaryThis PR fixes network errors affecting user authentication by addressing two critical issues. First, it corrects the CORS origin callback in the Elysia server to return boolean values instead of arrays, which was preventing legitimate cross-origin requests from being processed. Second, it removes trailing slashes from API base URLs across the frontend to prevent malformed double-slash paths (like Important Files Changed
Confidence score: 5/5
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant ElysiaCORS as "Elysia CORS Plugin"
participant APIClient as "API Client"
participant Server as "Elysia Server"
User->>Frontend: "Triggers API request"
Frontend->>APIClient: "Make API call"
Note over APIClient: Remove trailing slash from base URL
APIClient->>APIClient: "Strip trailing '/' to prevent double-slash URLs"
APIClient->>ElysiaCORS: "Send request with Origin header"
ElysiaCORS->>ElysiaCORS: "Extract origin from request headers"
alt No origin header
ElysiaCORS->>ElysiaCORS: "Allow same-origin/non-browser clients"
ElysiaCORS->>Server: "Forward request (allowed)"
else Origin header present
ElysiaCORS->>ElysiaCORS: "Build allowed origins list"
ElysiaCORS->>ElysiaCORS: "Check if origin is in allowed list"
alt Origin allowed
ElysiaCORS->>Server: "Forward request (allowed)"
else Origin rejected
ElysiaCORS->>ElysiaCORS: "Log rejection with origin and allowed list"
ElysiaCORS->>APIClient: "Return CORS error (boolean false)"
end
end
Server->>APIClient: "Return API response"
APIClient->>Frontend: "Return formatted response"
Frontend->>User: "Display result"
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
PR Code Suggestions ✨
Explore these optional code suggestions:
|
|||||||||||
CI Feedback 🧐(Feedback updated until commit 72df026)A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| origin: ({ request }) => { | ||
| // Get the incoming origin from the request | ||
| const incomingOrigin = request.headers.get("origin"); | ||
|
|
||
| // No origin header means same-origin request or non-browser client | ||
| if (!incomingOrigin) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Bug: The @elysiajs/cors origin callback incorrectly destructures ({ request }) instead of receiving the full Context object.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The origin callback for @elysiajs/cors is incorrectly destructuring ({ request }) from its parameter. The callback expects a full Context object, not an object with a top-level request property. This causes request to be undefined, leading to a TypeError: Cannot read property 'headers' of undefined when request.headers.get("origin") is called, which will crash the server when processing CORS requests.
💡 Suggested Fix
Change the origin callback signature from ({ request }) => { ... } to (context) => { ... } and access the request via context.request.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: apps/core/server/api-elysia.ts#L255-L262
Potential issue: The `origin` callback for `@elysiajs/cors` is incorrectly destructuring
`({ request })` from its parameter. The callback expects a full `Context` object, not an
object with a top-level `request` property. This causes `request` to be `undefined`,
leading to a `TypeError: Cannot read property 'headers' of undefined` when
`request.headers.get("origin")` is called, which will crash the server when processing
CORS requests.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 3979100
![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 introduces several improvements to fix a network error, primarily by enhancing the CORS handling on the server and making client-side URL construction more robust. The changes to the CORS logic add better logging and more explicit handling of different request types, which is a great improvement for security and debuggability. On the client side, removing trailing slashes from base URLs prevents potential issues with double slashes. I've identified one high-severity issue in the new CORS implementation where a mismatch in trailing slashes between the browser's Origin header and configured environment variables could lead to legitimate requests being blocked. My review includes a suggestion to address this.
|
|
||
| return allowedOrigins; | ||
| // Check if incoming origin is in allowed list | ||
| const isAllowed = allowedOrigins.includes(incomingOrigin); |
There was a problem hiding this comment.
The current check allowedOrigins.includes(incomingOrigin) is sensitive to trailing slashes. The Origin header from browsers typically does not include a trailing slash (e.g., https://example.com), but an origin configured in env.FRONTEND_URL or env.CORS_ALLOWED_ORIGINS might have one (e.g., https://example.com/). This discrepancy will cause the .includes() check to fail, incorrectly blocking valid cross-origin requests.
To make this check more robust, you should account for the potential trailing slash on the allowed origins during comparison.
const isAllowed = allowedOrigins.some(allowedOrigin => allowedOrigin === incomingOrigin || (allowedOrigin.endsWith('/') && allowedOrigin.slice(0, -1) === incomingOrigin));![codiumai-pr-agent-free[bot]](https://avatars.githubusercontent.com/in/354216?s=60&v=4){kind=small}
Co-authored-by: codiumai-pr-agent-free[bot] <138128286+codiumai-pr-agent-free[bot]@users.noreply.github.com>
| // Check if incoming origin is in allowed list | ||
| const isAllowed = allowedOrigins.includes(incomingOrigin); |
There was a problem hiding this comment.
Suggestion: Prevent potential null value errors
| // Check if incoming origin is in allowed list | |
| const isAllowed = allowedOrigins.includes(incomingOrigin); | |
| // Check if incoming origin is in allowed list | |
| const isAllowed = !!incomingOrigin && allowedOrigins.includes(incomingOrigin); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/core/src/lib/api-client.ts (2)
196-228: Naming conflict: TwoapiFetchfunctions with different signatures.This
apiFetchreturns{ data, error }and auto-parses JSON, whileapps/core/src/utils/api.tsexports anapiFetchthat returnsPromise<Response>with retry/timeout support. Both are public exports with the same name but incompatible contracts.Consider:
- Rename one (e.g.,
apiFetchJsonhere vsapiFetchRawin utils), or- Consolidate into a single implementation with optional response handling
203-205: Redundant conditional: both branches are identical.The ternary condition checks if
API_BASE_URLstarts withhttp, but both branches execute the same code. Simplify to remove the unnecessary conditional.- const url = API_BASE_URL.startsWith("http") - ? `${API_BASE_URL}${endpointPath}` // Absolute URL - : `${API_BASE_URL}${endpointPath}`; // Relative URL (Vite proxy handles it) + const url = `${API_BASE_URL}${endpointPath}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/core/server/api-elysia.ts(2 hunks)apps/core/src/lib/api-client.ts(1 hunks)apps/core/src/utils/api.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>in the Asset-Forge project
Useconstoverlet, never usevar
Use template literals for strings instead of string concatenation
Files:
apps/core/server/api-elysia.tsapps/core/src/lib/api-client.tsapps/core/src/utils/api.ts
apps/core/server/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/core/server/**/*.ts: Research with Deepwiki before adding new endpoints in Elysia
Define API types in service file with TypeBox validation in Elysia endpoints
Implement business logic in service class for API endpoints
Create route handler with TypeBox validation for API endpoints
Add Swagger documentation to new API endpoints
Follow REST conventions for API endpoints
Files:
apps/core/server/api-elysia.ts
**/*.{ts,tsx,js,jsx,json,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Use 2-space indentation for all code
Files:
apps/core/server/api-elysia.tsapps/core/src/lib/api-client.tsapps/core/src/utils/api.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript strict mode everywhere
Noanyorunknowntypes in TypeScript code
Use explicit return types on public methods in TypeScript
Files:
apps/core/server/api-elysia.tsapps/core/src/lib/api-client.tsapps/core/src/utils/api.ts
🧬 Code graph analysis (1)
apps/core/src/utils/api.ts (2)
apps/core/src/lib/api-client.ts (1)
apiFetch(196-228)apps/core/__tests__/helpers/auth.ts (1)
hasAuthHeader(126-129)
🪛 GitHub Actions: Test Suite
apps/core/server/api-elysia.ts
[error] 255-255: TS2339: Property 'request' does not exist on type 'Request'.
🔇 Additional comments (5)
apps/core/src/lib/api-client.ts (1)
23-29: LGTM! Good fix for trailing slash normalization.This correctly prevents double-slash URLs (e.g.,
//api/users/me) by stripping trailing slashes from the base URL before concatenation.apps/core/server/api-elysia.ts (2)
264-311: LGTM! CORS origin validation logic is well-structured.Good implementation with:
- Dynamic allowed origins from environment configuration
- Development mode localhost fallbacks
- Clear warning logs for debugging unauthorized origins
- Correct early return for same-origin/non-browser requests
The logic is sound once the callback signature issue (above) is fixed.
255-262: The review comment is incorrect. The CORS origin callback signature is correct.According to the official
@elysiajs/corsdocumentation, the origin callback receives a Context object that should be destructured. The patternorigin: ({ request }) => { ... }is the correct and documented approach. The review comment's claim that this causes aTS2339error is false, and the proposed "fix" would actually break the code.The current code at lines 255-262 properly destructures the Context object to access the
requestproperty, which is exactly how the library is designed to be used.apps/core/src/utils/api.ts (2)
19-22: LGTM! Consistent trailing slash normalization.This mirrors the fix in
api-client.ts, ensuring consistent URL handling across both API utilities.
24-104: Well-implemented fetch wrapper with proper timeout and retry handling.Good patterns:
- AbortController for request timeouts with proper cleanup in
finally- Case-insensitive auth header check (lines 73-74)
- Flexible header normalization supporting
Headers, arrays, and plain objects- Clean retry integration with
retryWithBackoff
Browser Origin headers never include trailing slashes, but environment variables (FRONTEND_URL, CORS_ALLOWED_ORIGINS) might be configured with them. This mismatch would cause legitimate requests to be rejected. Added normalizeOrigin() helper to strip trailing slashes from configured origins before comparison with the incoming request origin.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/core/src/utils/api.ts (2)
82-86: Timeout bypass when external signal is provided.When the caller provides a
signal, the internal timeout controller is bypassed due tosignal ?? controller.signal. The request will not be aborted aftertimeoutMs, potentially causing hanging requests.The previous review suggested combining both signals. Consider aborting the internal controller when the external signal aborts:
+ if (signal) { + signal.addEventListener('abort', () => controller.abort(signal.reason), { once: true }); + } + const response = await fetch(url, { ...rest, headers, - signal: signal ?? controller.signal, + signal: controller.signal, });
76-80: Partial fix for header case sensitivity.The case-insensitive check for
Authorizationprevents duplicate auth headers from the token store. However, the broader header normalization suggested in a previous review (normalizing all header keys to consistent case) was not implemented. This is acceptable for now since the auth header is the critical case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/core/src/utils/api.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>in the Asset-Forge project
Useconstoverlet, never usevar
Use template literals for strings instead of string concatenation
Files:
apps/core/src/utils/api.ts
**/*.{ts,tsx,js,jsx,json,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Use 2-space indentation for all code
Files:
apps/core/src/utils/api.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript strict mode everywhere
Noanyorunknowntypes in TypeScript code
Use explicit return types on public methods in TypeScript
Files:
apps/core/src/utils/api.ts
🧬 Code graph analysis (1)
apps/core/src/utils/api.ts (2)
apps/core/src/lib/api-client.ts (1)
apiFetch(196-228)apps/core/__tests__/helpers/auth.ts (1)
hasAuthHeader(126-129)
🔇 Additional comments (4)
apps/core/src/utils/api.ts (4)
1-7: LGTM!Imports and interface definition are clean. The
RequestOptionsextension ofRequestInitproperly types the additional options.
12-22: LGTM!The trailing slash normalization correctly prevents double-slash URLs. The environment-based logic for dev vs. production is well-documented.
24-40: LGTM!The URL construction logic correctly handles absolute URLs, relative paths with and without leading slashes, and avoids double-slash issues when combined with the normalized base URL.
93-107: LGTM!The retry logic correctly handles both boolean and object configurations, returns the response on success, and throws an appropriate error after retries are exhausted.
| export async function apiFetch( | ||
| input: string, | ||
| init: RequestOptions = {}, | ||
| ): Promise<Response> { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find api-client.ts file
echo "=== Searching for api-client.ts ==="
find . -name "api-client.ts" -type f 2>/dev/null | head -20
# Find the specific file under review
echo -e "\n=== Searching for the api.ts file under review ==="
find . -path "*/core/src/utils/api.ts" -type f 2>/dev/nullRepository: Dexploarer/hyper-forge
Length of output: 216
🏁 Script executed:
#!/bin/bash
# Read the api-client.ts file to check for apiFetch export
echo "=== Content of api-client.ts ==="
cat -n ./apps/core/src/lib/api-client.ts
echo -e "\n=== Content of api.ts (first 50 lines) ==="
head -50 ./apps/core/src/utils/api.ts
# Search for imports of apiFetch across the codebase
echo -e "\n=== Imports of apiFetch in codebase ==="
rg -n "import.*apiFetch|from.*api" --type=ts --type=tsx -A1 -B1 | head -100Repository: Dexploarer/hyper-forge
Length of output: 11868
🏁 Script executed:
#!/bin/bash
# Search for imports of apiFetch - fix the file type filter
echo "=== Imports of apiFetch in codebase ==="
rg -n "import.*apiFetch" --type=ts -B2 -A2
echo -e "\n=== Files that import from api-client ==="
rg -n "from.*['\"].*api-client" --type=ts
echo -e "\n=== Files that import from utils/api ==="
rg -n "from.*['\"].*utils/api" --type=tsRepository: Dexploarer/hyper-forge
Length of output: 11685
Two apiFetch exports with incompatible signatures create real import confusion.
Verified: PromptService.ts and CDNAdminService.ts import apiFetch from @/lib/api-client (returns Promise<{ data: T | null; error: string | null }>), while 19+ other files import from @/utils/api (returns Promise<Response>). This inconsistency means developers could easily pick the wrong import. Consider renaming one function to clarify intent—e.g., apiFetchWrapped or apiFetchRaw.
🤖 Prompt for AI Agents
In apps/core/src/utils/api.ts around lines 24 to 27, there is an apiFetch
exported that returns Promise<Response>, which conflicts with another apiFetch
exported from @/lib/api-client (returns Promise<{ data: T | null; error: string
| null }>) causing import confusion; rename this function to a clear, distinct
name (for example apiFetchRaw or fetchResponse) and update its export and all
internal imports/usages accordingly to preserve the original Response-returning
behavior, leaving the wrapped api-client export untouched so callers explicitly
choose raw Response vs wrapped data/error form.
PR Type
Bug fix
Description
Fix CORS callback to return boolean instead of array per Elysia spec
Remove trailing slashes from API base URLs to prevent double-slash paths
Add origin validation logging for debugging CORS rejections
Format code with consistent quote style and semicolons
Diagram Walkthrough
File Walkthrough
api-elysia.ts
Fix CORS callback to return booleanapps/core/server/api-elysia.ts
clients
allowed list
api-client.ts
Strip trailing slash from API base URLapps/core/src/lib/api-client.ts
api.ts
Remove trailing slash and format codeapps/core/src/utils/api.ts
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.