fix(gist): handle proxied raw gist files (#222)#224
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end support for fetching truncated gist file raw content via a new ChangesRaw Gist Content Fetching via Backend Proxy
Sequence Diagram(s)sequenceDiagram
participant HighlightedCode as HighlightedCode (GistDetailModal)
participant Factory as createGitHubApiService()
participant GitHubApiService
participant Backend as POST /proxy/github-raw
participant ProxyService as proxyRequest(preserveRawResponse)
participant GitHub as raw.githubusercontent.com
HighlightedCode->>Factory: createGitHubApiService(githubToken)
Factory-->>HighlightedCode: GitHubApiService (configured with backendUrl)
HighlightedCode->>GitHubApiService: getGistFileRaw(file.raw_url, signal)
GitHubApiService->>Backend: POST {url: raw_url}
Backend->>ProxyService: proxyRequest(raw_url, {GET, preserveRawResponse: true})
ProxyService->>GitHub: GET raw_url (responseType: text)
GitHub-->>ProxyService: raw file text
ProxyService-->>Backend: {status, data: string}
Backend-->>GitHubApiService: raw text response
GitHubApiService-->>HighlightedCode: string content
HighlightedCode->>HighlightedCode: onContentLoaded(filename, content)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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 adds a new backend proxy endpoint /api/proxy/github-raw to fetch raw Gist file contents when they are truncated, updates the frontend GitHubApiService to support routing requests through this proxy with exponential backoff retries, and integrates on-demand fetching and caching of truncated files in the UI. Feedback highlights a critical security vulnerability due to missing authentication on the proxy endpoints, a React stale closure risk in GistDetailModal with onContentLoaded, robustness issues where network errors bypass the retry loop and invalid URLs cause unhandled exceptions, and an opportunity to simplify code by removing a redundant abortRef.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| router.post('/api/proxy/github-raw', async (req, res) => { | ||
| try { | ||
| const db = getDb(); | ||
| const tokenRow = db.prepare('SELECT value FROM settings WHERE key = ?').get('github_token') as { value: string } | undefined; | ||
| if (!tokenRow?.value) { | ||
| res.status(400).json({ error: 'GitHub token not configured', code: 'GITHUB_TOKEN_NOT_CONFIGURED' }); | ||
| return; | ||
| } | ||
|
|
||
| let token: string; | ||
| try { | ||
| token = decrypt(tokenRow.value, config.encryptionKey); | ||
| } catch { | ||
| res.status(500).json({ error: 'Failed to decrypt GitHub token', code: 'GITHUB_TOKEN_DECRYPT_FAILED' }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Security Vulnerability: Missing Authentication on Proxy Endpoints
The new /api/proxy/github-raw endpoint (as well as the existing /api/proxy/github/* endpoint) does not perform any authentication or validation of the backendApiSecret (or any other authorization header).
Since these endpoints decrypt and use the server's configured github_token to make requests to GitHub, any unauthorized client who can access the server can abuse these endpoints to make arbitrary requests to GitHub using your server's credentials.
Recommendation:
Implement a middleware or a check to validate the incoming Authorization header against the configured backend API secret before proceeding with the proxy request.
| const HighlightedCode: React.FC<HighlightedCodeProps> = ({ file, onContentLoaded }) => { | ||
| const codeRef = useRef<HTMLElement>(null); | ||
| const language = inferGistCodeLanguage(file.filename, file.language); | ||
| const content = file.content || ''; | ||
| const githubToken = useAppStore(state => state.githubToken); | ||
| const language2 = useAppStore(state => state.language); | ||
| const t = (zh: string, en: string) => language2 === 'zh' ? zh : en; | ||
|
|
||
| // 截断文件（>1MB）的 content 会被 GitHub gist 详情 API 省略，需要按需拉取 raw_url。 | ||
| const needsRawFetch = !!file.truncated && !file.content && !!file.raw_url; | ||
| const [rawContent, setRawContent] = useState<string | null>(null); | ||
| const [rawError, setRawError] = useState<string | null>(null); | ||
| const [isLoadingRaw, setIsLoadingRaw] = useState(false); | ||
| const [retryTick, setRetryTick] = useState(0); | ||
| const abortRef = useRef<AbortController | null>(null); |
There was a problem hiding this comment.
React Bug: Stale Closure Risk with onContentLoaded
The useEffect that fetches raw content calls onContentLoaded?.(file.filename, text). However, onContentLoaded is not included in the useEffect dependency array.
Because handleContentLoaded in GistDetailModal is recreated on every render (as it is not memoized and closes over the gist state), the useEffect in HighlightedCode will hold a stale reference to onContentLoaded. If updateGist is called with a stale gist object, it can overwrite and revert other updates to the gist in the store.
To fix this without causing the fetch effect to re-run unnecessarily, you should store the latest onContentLoaded callback in a React ref.
Suggested Fix:
Store onContentLoaded in a ref and call onContentLoadedRef.current?.(file.filename, text); inside the fetch effect:
const onContentLoadedRef = useRef(onContentLoaded);
useEffect(() => {
onContentLoadedRef.current = onContentLoaded;
}, [onContentLoaded]);| } | ||
|
|
||
| // Whitelist: only allow known GitHub raw-content hosts | ||
| const parsed = new URL(body.url); |
There was a problem hiding this comment.
Robustness: Unhandled Exception on Invalid URL
If body.url is not a valid URL string, new URL(body.url) will throw an exception. Since this is caught by the global try-catch block, it will result in a 500 Internal Server Error with a generic GITHUB_RAW_PROXY_FAILED code.
It is better to explicitly handle URL parsing errors and return a 400 Bad Request to the client.
| const parsed = new URL(body.url); | |
| let parsed: URL; | |
| try { | |
| parsed = new URL(body.url); | |
| } catch { | |
| res.status(400).json({ error: 'Invalid URL format', code: 'INVALID_URL' }); | |
| return; | |
| } |
| useEffect(() => { | ||
| if (!needsRawFetch || !file.raw_url) return; | ||
| // 切换文件或重试时，取消上一个未完成请求，避免旧响应覆盖新文件。 | ||
| abortRef.current?.abort(); | ||
| const controller = new AbortController(); | ||
| abortRef.current = controller; |
There was a problem hiding this comment.
Code Simplification: Redundant abortRef
The abortRef is redundant because the useEffect cleanup function (return () => controller.abort(); on line 72) is automatically executed by React whenever the dependencies change or when the component unmounts.
You can safely delete const abortRef = useRef<AbortController | null>(null); on line 37 and simplify the effect setup.
| useEffect(() => { | |
| if (!needsRawFetch || !file.raw_url) return; | |
| // 切换文件或重试时，取消上一个未完成请求，避免旧响应覆盖新文件。 | |
| abortRef.current?.abort(); | |
| const controller = new AbortController(); | |
| abortRef.current = controller; | |
| useEffect(() => { | |
| if (!needsRawFetch || !file.raw_url) return; | |
| const controller = new AbortController(); |
| } catch (fetchError) { | ||
| const durationMs = Date.now() - startTime; | ||
| logger.error('githubApi', 'API request network error', { method, endpoint, durationMs, error: fetchError instanceof Error ? fetchError.message : String(fetchError) }); | ||
| throw fetchError; | ||
| } |
There was a problem hiding this comment.
Reliability: Network Errors Bypass Retry Loop
Currently, any network error thrown by fetch (such as temporary connection drops, DNS timeouts, or socket hang-ups) is caught in the catch block and immediately rethrown. This completely bypasses the retry loop, meaning transient network issues are never retried.
To improve reliability, you should only throw the error if attempt === maxRetries. Otherwise, log a warning and allow the loop to continue to the next retry attempt.
} catch (fetchError) {
if (attempt === maxRetries) {
const durationMs = Date.now() - startTime;
logger.error('githubApi', 'API request network error', { method, endpoint, durationMs, error: fetchError instanceof Error ? fetchError.message : String(fetchError) });
throw fetchError;
}
logger.warn('githubApi', 'API request network error, retrying', { method, endpoint, attempt: attempt + 1, error: fetchError instanceof Error ? fetchError.message : String(fetchError) });
const delayMs = Math.min(1000 * Math.pow(2, attempt), 4000);
await new Promise(resolve => setTimeout(resolve, delayMs));
continue;
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
server/tests/routes/githubProxyRoute.test.ts (1)
71-91: ⚡ Quick winAdd negative-path regression tests for
github-raw.Please add cases for invalid URL/blocked host and for client-supplied
Authorizationnot overriding server credentials. Current coverage is only success-path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/tests/routes/githubProxyRoute.test.ts` around lines 71 - 91, The test file for the github-raw route currently only covers the success path. Add two additional test cases to the test suite for the github-raw endpoint: first, add a test that verifies the endpoint properly rejects invalid URLs or blocked hosts (ensure validateUrlMock or proxyRequestMock handles rejection appropriately), and second, add a test that confirms when a client supplies an Authorization header in the request, it does not override the server's configured credentials (verify that the actual request sent through proxyRequestMock uses server credentials, not client-supplied ones). These tests should follow the same pattern as the existing success-path test for the github-raw POST endpoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/routes/proxy.ts`:
- Around line 141-147: The new URL(body.url) call at line 141 can throw an
exception when given a malformed URL, which currently bubbles to an outer catch
block returning a 500 error. Since this is invalid client input, wrap the URL
parsing in a try-catch block that catches the error and immediately returns a
400 status response with an appropriate error message indicating the URL is
malformed, before proceeding with the hostname validation and validateUrl call.
- Around line 150-155: The headers object construction in the proxy route allows
client-provided headers from body.headers to override critical server-defined
headers like Authorization. Reorder the headers object so that the
server-defined headers (Authorization, User-Agent, Accept) are spread last after
...body.headers, ensuring client-provided headers cannot override the
security-critical Authorization bearer token. This prevents callers from
bypassing the server's authentication mechanism in the /api/proxy/github-raw
endpoint.
In `@src/components/GistDetailModal.tsx`:
- Around line 135-147: The handleContentLoaded function captures the gist prop
value at the time of definition, which can cause stale data issues when multiple
raw file contents are loaded sequentially. Instead of using the captured gist
variable directly in the updateGist call, use a state setter function pattern
(callback) that receives the current/latest gist state as a parameter, ensuring
that each call to handleContentLoaded always works with the most recent gist
snapshot when persisting the fetched content.
In `@src/components/GistView.tsx`:
- Around line 224-228: The catch block in the gist detail-fetching logic lacks a
sequence check to guard against stale requests, allowing old failed requests to
show error toasts after a new gist has been opened. Add a sequence verification
check in the catch block (similar to what exists in the success path) before
calling the toast function to ensure that only errors from the current/latest
request are displayed to the user.
In `@src/services/githubApi.ts`:
- Around line 227-230: The retry delay in the retry loop uses a plain setTimeout
that doesn't respect abort signals, causing cancellation to be delayed until the
timer completes. Modify the delay promise at line 229 to be abort-aware by
incorporating the AbortSignal (if available in the function's scope) so that the
delay immediately resolves or rejects when the signal aborts, allowing the retry
mechanism to respond promptly to cancellation requests instead of waiting for
the full timeout duration.
---
Nitpick comments:
In `@server/tests/routes/githubProxyRoute.test.ts`:
- Around line 71-91: The test file for the github-raw route currently only
covers the success path. Add two additional test cases to the test suite for the
github-raw endpoint: first, add a test that verifies the endpoint properly
rejects invalid URLs or blocked hosts (ensure validateUrlMock or
proxyRequestMock handles rejection appropriately), and second, add a test that
confirms when a client supplies an Authorization header in the request, it does
not override the server's configured credentials (verify that the actual request
sent through proxyRequestMock uses server credentials, not client-supplied
ones). These tests should follow the same pattern as the existing success-path
test for the github-raw POST endpoint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f9a2259-5f2c-49e2-83f2-eb27ffb7bd5e
📒 Files selected for processing (11)
server/src/routes/proxy.tsserver/src/services/proxyService.tsserver/tests/routes/githubProxyRoute.test.tssrc/components/GistCard.tsxsrc/components/GistDetailModal.tsxsrc/components/GistView.tsxsrc/services/githubApi.tssrc/services/githubApiFactory.tssrc/store/useAppStore.tssrc/utils/gistUtils.test.tssrc/utils/gistUtils.ts
💤 Files with no reviewable changes (1)
- src/store/useAppStore.ts
- Fix network errors bypassing retry loop in makeRequest (gemini #5) Transient network errors (connection drops, DNS timeouts) now use exponential backoff retry instead of immediate rethrow. - Add abort-aware retry backoff for both network and 5xx retries (coderabbit #5) Signal abort during backoff now cancels immediately instead of waiting for the timer to complete.
Some gists (e.g. karpathy/8627fe009c40f57531cb18360106ce95) consistently return 502 from GitHub's gist detail API. The raw content endpoint (gist.githubusercontent.com) remains accessible for these gists. - openDetail: on 502/503/504, open the modal with cached list data instead of showing an error toast; warn the user via a non-blocking toast that content will load on demand - HighlightedCode: relax needsRawFetch condition to trigger when content is absent and raw_url is available (not only when truncated=true), covering the list-API-cached fallback path where truncated is null
When GitHub gist detail API returns truncated:true (>1MB files), file.content contains only partial data. Previously the needsRawFetch condition only checked for missing content, so truncated files with partial content were never refreshed from raw_url. Changes: - needsRawFetch now triggers on file.truncated || !file.content - rawContent (from raw_url) takes priority over file.content - handleContentLoaded overwrites truncated content in store - mergeGistMetadata prefers cached full content over incoming truncated partial content
The 502 gist fallback used toast(msg, 'warning') but ToastType only supported success|error|info. iconMap['warning'] was undefined, causing <undefined /> render and React error #130. Added warning type with AlertTriangle icon and amber styling.
When the gist detail API returns 5xx (e.g. stable 502 for certain gists), AI analysis would fail entirely. Added getGistForAnalysis() that falls back to fetching file content from raw_url when the detail API is unavailable, ensuring analysis can proceed with cached metadata.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/githubApi.ts`:
- Around line 448-451: The regex pattern `/50[0-9]/` in the isServerFailure
variable assignment only matches HTTP status codes 500-509, but HTTP 5xx errors
extend to 510-599 (including Cloudflare errors like 520-530). Update the regex
pattern to match all 5xx status codes by changing it to `/5[0-9]{2}/`, which
will correctly match any three-digit error code starting with 5.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c334ffce-1084-4202-849d-6098f094e272
📒 Files selected for processing (5)
src/components/GistCard.tsxsrc/components/GistDetailModal.tsxsrc/components/GistView.tsxsrc/components/ui/Toast.tsxsrc/services/githubApi.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/GistDetailModal.tsx
- src/components/GistCard.tsx
- src/components/GistView.tsx
CodeRabbit audit: /50[0-9]/ only matched 500-509, missing 510-599
(e.g. Cloudflare 520-530). Changed to /5\d{2}/ in both
getGistForAnalysis and openDetail fallback.
1 participant
Summary
Textlanguage to highlight.js plaintextFixes #222
Tests
Summary by CodeRabbit