fix: resolve proxy bugs and part ID prefix mismatch (#19, #20, #21, #23)#24
Open
SoulWayy wants to merge 5 commits into
Open
fix: resolve proxy bugs and part ID prefix mismatch (#19, #20, #21, #23)#24SoulWayy wants to merge 5 commits into
SoulWayy wants to merge 5 commits into
Conversation
- Fix joelhooks#19: Change generatePartId() prefix from 'part-' to 'prt-' - Fix joelhooks#20: Replace request.body ReadableStream with request.text() for POST body forwarding - Fix joelhooks#20: Add 204 No Content handling (return null body instead of crashing) - Fix joelhooks#21: Make directory optional in OpencodeConfig, handle empty directory - Fix joelhooks#23: Forward ALL request headers except hop-by-hop headers (RFC 2616) Tests updated: - Uncommented 204 No Content test - Added header forwarding tests (all headers forwarded, hop-by-hop filtered) - Removed outdated duplex: 'half' checks - All 34 proxy route tests pass, 8 prompt tests pass Closes joelhooks#19, joelhooks#20, joelhooks#21, joelhooks#23
📝 WalkthroughWalkthroughProxy now forwards all non-hop-by-hop request headers (excludes Connection-listed tokens), reads POST/PUT/PATCH bodies via request.text(), returns empty 204 responses with headers, updates header/method tests, always mounts OpencodeSSRPlugin with optional directory, hardens server-spawn validation, and switches part IDs to the prt- prefix. ChangesAPI Proxy Robustness and Configuration Updates
sequenceDiagram
participant Client
participant proxyRequest
participant Backend
Client->>proxyRequest: Request (headers, method, body)
proxyRequest->>proxyRequest: Remove HOP_BY_HOP and Connection-token headers
proxyRequest->>Backend: fetch(targetUrl, { method, headers, bodyText? })
Backend-->>proxyRequest: Response (status, headers, body?)
alt status == 204
proxyRequest-->>Client: NextResponse(null, {status:204, headers})
else
proxyRequest->>proxyRequest: response.text()
proxyRequest-->>Client: Response with body and headers
end
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 3
🤖 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 `@apps/web/src/app/api/opencode/`[port]/[[...path]]/route.test.ts:
- Around line 615-633: The test must fail if the proxy reads an upstream body
for 204 responses: update the mock fetch response used in this test (the object
returned by global.fetch) so its text() is a function that throws or rejects
(e.g. vi.fn().mockImplementation(() => { throw new Error("text() should not be
called for 204"); }) or vi.fn().mockRejectedValue(new Error(...))) instead of
resolving to "", and keep the existing assertions on DELETE and response.text()
accordingly; this ensures the mocked text() will break any implementation that
erroneously calls response.text() for status 204 (target symbols: global.fetch
mock, text(), DELETE handler, NextRequest used in the test).
In `@apps/web/src/app/api/opencode/`[port]/[[...path]]/route.ts:
- Around line 149-150: The 204 branch drops upstream headers by returning new
NextResponse(null, { status: 204 }) without copying response.headers; update the
branch that checks response.status === 204 to create the NextResponse and copy
all headers from response.headers into it (preserve ETag, Cache-Control,
Set-Cookie, etc.) before returning so the proxy remains transparent—use the
existing response object and NextResponse constructor used elsewhere in this
file to transfer headers.
- Around line 125-128: The proxy currently only drops headers in
HOP_BY_HOP_HEADERS when copying request.headers; update the logic to also parse
any Connection header(s) from the incoming request, split on commas, trim and
lower-case each token, add those token names to the deny list, and ensure those
header names (and the Connection header itself) are not forwarded; modify the
loop that copies headers (the code referencing request.headers.entries(),
headers, and HOP_BY_HOP_HEADERS) to build a combined deny set (standard
hop-by-hop plus tokens from Connection) and skip any header whose lower-cased
name appears in that combined set.
🪄 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: 0f62177a-35a6-442d-b01d-20bb99fd2fbb
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
apps/web/src/app/api/opencode/[port]/[[...path]]/route.test.tsapps/web/src/app/api/opencode/[port]/[[...path]]/route.tsapps/web/src/app/session/[id]/session-layout.tsxpackages/core/src/api/prompt.tspackages/react/src/next-ssr-plugin.tsx
- Parse Connection header tokens per RFC 2616 Section 14.10 - Forward upstream headers on 204 No Content responses - Improve 204 test to verify text() is not called
Spawn endpoint: - Add validateDirectory() with realpath, stat, BLOCKED_PREFIXES - Fix child.pid! non-null assertion → guard check - Add AbortSignal.timeout(2000) to findExistingServer fetch Proxy: - Add AbortSignal.timeout(30_000) to proxy fetch - Forward all upstream response headers - Default to application/json when upstream has no content-type Addresses security findings from codebase scan.
- multi-server-sse.ts: Type SSEEvent.payload as SDK Event union
instead of generic {type: string; properties: Record<string,unknown>}
- factory.ts: Remove @ts-expect-error (now type-safe)
- spawn/route.test.ts: Add 20 tests for directory validation,
blocked prefixes, existing server detection, spawn flow, error handling
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 `@apps/web/src/app/api/opencode/servers/spawn/route.ts`:
- Around line 68-93: The code currently checks BLOCKED_PREFIXES only against the
pre-realpath normalizedPath, allowing symlinks to resolve into blocked
locations; update the logic so after obtaining canonical via realpath() you
re-run the blocked-prefix check against a canonicalNormalized value (remove
trailing slashes but preserve "/" as a single slash) and return the same {
valid: false, error: ... } if canonical equals a prefix or startsWith
`${prefix}/`; ensure the normalization helper used for both normalizedPath and
canonicalNormalized preserves root ("/") instead of producing an empty string so
the root prefix check still works.
🪄 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: ece489d2-856a-4967-9a1e-0eb93782afa8
📒 Files selected for processing (5)
apps/web/src/app/api/opencode/[port]/[[...path]]/route.tsapps/web/src/app/api/opencode/servers/spawn/route.test.tsapps/web/src/app/api/opencode/servers/spawn/route.tspackages/core/src/sse/multi-server-sse.tspackages/react/src/factory.ts
💤 Files with no reviewable changes (1)
- packages/react/src/factory.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/sse/multi-server-sse.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/app/api/opencode/[port]/[[...path]]/route.ts
Comment on lines
+68
to
+93
| const absolutePath = isAbsolute(directory) ? directory : resolve(directory) | ||
| const normalizedPath = absolutePath.replace(/\/+$/, "") | ||
|
|
||
| for (const prefix of BLOCKED_PREFIXES) { | ||
| if (normalizedPath === prefix || normalizedPath.startsWith(`${prefix}/`)) { | ||
| return { valid: false, error: `Cannot spawn server in system directory: ${prefix}` } | ||
| } | ||
| } | ||
|
|
||
| let canonical: string | ||
| try { | ||
| canonical = await realpath(absolutePath) | ||
| } catch { | ||
| return { valid: false, error: `Directory does not exist: ${directory}` } | ||
| } | ||
|
|
||
| try { | ||
| const fileStat = await stat(canonical) | ||
| if (!fileStat.isDirectory()) { | ||
| return { valid: false, error: `Path is not a directory: ${directory}` } | ||
| } | ||
| } catch { | ||
| return { valid: false, error: `Cannot access directory: ${directory}` } | ||
| } | ||
|
|
||
| return { valid: true, canonical } |
There was a problem hiding this comment.
Re-run the blocked-prefix check on the canonical path.
realpath() can turn an allowed-looking input into a blocked system path after the current prefix scan has already passed. A symlink like /tmp/project-link -> /etc will still be accepted and used as cwd, which defeats the hardening added here. Re-check BLOCKED_PREFIXES against the canonical path as well, and add a regression test where realpath() resolves into a blocked directory. This same normalization helper should preserve / instead of turning it into an empty string.
Suggested patch
+const normalizeForPrefixCheck = (path: string) => path.replace(/\/+$/, "") || "/"
+
+const getBlockedPrefix = (path: string) =>
+ BLOCKED_PREFIXES.find((prefix) => path === prefix || path.startsWith(`${prefix}/`))
+
async function validateDirectory(
directory: string,
): Promise<{ valid: true; canonical: string } | { valid: false; error: string }> {
if (!directory || typeof directory !== "string") {
return { valid: false, error: "directory is required" }
}
const absolutePath = isAbsolute(directory) ? directory : resolve(directory)
- const normalizedPath = absolutePath.replace(/\/+$/, "")
-
- for (const prefix of BLOCKED_PREFIXES) {
- if (normalizedPath === prefix || normalizedPath.startsWith(`${prefix}/`)) {
- return { valid: false, error: `Cannot spawn server in system directory: ${prefix}` }
- }
- }
+ const blockedBeforeRealpath = getBlockedPrefix(normalizeForPrefixCheck(absolutePath))
+ if (blockedBeforeRealpath) {
+ return {
+ valid: false,
+ error: `Cannot spawn server in system directory: ${blockedBeforeRealpath}`,
+ }
+ }
let canonical: string
try {
canonical = await realpath(absolutePath)
} catch {
return { valid: false, error: `Directory does not exist: ${directory}` }
}
+
+ const blockedCanonical = getBlockedPrefix(normalizeForPrefixCheck(canonical))
+ if (blockedCanonical) {
+ return {
+ valid: false,
+ error: `Cannot spawn server in system directory: ${blockedCanonical}`,
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const absolutePath = isAbsolute(directory) ? directory : resolve(directory) | |
| const normalizedPath = absolutePath.replace(/\/+$/, "") | |
| for (const prefix of BLOCKED_PREFIXES) { | |
| if (normalizedPath === prefix || normalizedPath.startsWith(`${prefix}/`)) { | |
| return { valid: false, error: `Cannot spawn server in system directory: ${prefix}` } | |
| } | |
| } | |
| let canonical: string | |
| try { | |
| canonical = await realpath(absolutePath) | |
| } catch { | |
| return { valid: false, error: `Directory does not exist: ${directory}` } | |
| } | |
| try { | |
| const fileStat = await stat(canonical) | |
| if (!fileStat.isDirectory()) { | |
| return { valid: false, error: `Path is not a directory: ${directory}` } | |
| } | |
| } catch { | |
| return { valid: false, error: `Cannot access directory: ${directory}` } | |
| } | |
| return { valid: true, canonical } | |
| const normalizeForPrefixCheck = (path: string) => path.replace(/\/+$/, "") || "/" | |
| const getBlockedPrefix = (path: string) => | |
| BLOCKED_PREFIXES.find((prefix) => path === prefix || path.startsWith(`${prefix}/`)) | |
| async function validateDirectory( | |
| directory: string, | |
| ): Promise<{ valid: true; canonical: string } | { valid: false; error: string }> { | |
| if (!directory || typeof directory !== "string") { | |
| return { valid: false, error: "directory is required" } | |
| } | |
| const absolutePath = isAbsolute(directory) ? directory : resolve(directory) | |
| const blockedBeforeRealpath = getBlockedPrefix(normalizeForPrefixCheck(absolutePath)) | |
| if (blockedBeforeRealpath) { | |
| return { | |
| valid: false, | |
| error: `Cannot spawn server in system directory: ${blockedBeforeRealpath}`, | |
| } | |
| } | |
| let canonical: string | |
| try { | |
| canonical = await realpath(absolutePath) | |
| } catch { | |
| return { valid: false, error: `Directory does not exist: ${directory}` } | |
| } | |
| const blockedCanonical = getBlockedPrefix(normalizeForPrefixCheck(canonical)) | |
| if (blockedCanonical) { | |
| return { | |
| valid: false, | |
| error: `Cannot spawn server in system directory: ${blockedCanonical}`, | |
| } | |
| } | |
| try { | |
| const fileStat = await stat(canonical) | |
| if (!fileStat.isDirectory()) { | |
| return { valid: false, error: `Path is not a directory: ${directory}` } | |
| } | |
| } catch { | |
| return { valid: false, error: `Cannot access directory: ${directory}` } | |
| } | |
| return { valid: true, canonical } |
🤖 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 `@apps/web/src/app/api/opencode/servers/spawn/route.ts` around lines 68 - 93,
The code currently checks BLOCKED_PREFIXES only against the pre-realpath
normalizedPath, allowing symlinks to resolve into blocked locations; update the
logic so after obtaining canonical via realpath() you re-run the blocked-prefix
check against a canonicalNormalized value (remove trailing slashes but preserve
"/" as a single slash) and return the same { valid: false, error: ... } if
canonical equals a prefix or startsWith `${prefix}/`; ensure the normalization
helper used for both normalizedPath and canonicalNormalized preserves root ("/")
instead of producing an empty string so the root prefix check still works.
- Add PUT/DELETE/PATCH/OPTIONS port validation tests (proxy branch coverage) - Add reserved segment 'servers' test for each HTTP method - Add Connection header with empty values test - Add 204 response with headers test - Add spawn route tests: waitForServer timeout, null pid, non-Error exception - Increase spawn test timeout to 15s for waitForServer polling Coverage improvements: - route.ts branches: 66% → 88% - spawn/route.ts branches: 70% → 85%
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/app/api/opencode/servers/spawn/route.test.ts (1)
335-338: 💤 Low valueRemove unnecessary mock restoration.
mockImplementationOnceautomatically reverts to the default behavior after one call, andvi.clearAllMocks()(line 26) already resets all mocks between tests. The restoration here is redundant and potentially confusing sincemockImplementationexpects an implementation function, not a mock object reference.For consistency with the other error tests (lines 345, 395), consider using
mockReturnValueOnceinstead, which doesn't require manual restoration.♻️ Proposed refactor
it("returns 500 on spawn failure", async () => { const { spawn } = await import("child_process") - const originalSpawn = spawn ;(spawn as ReturnType<typeof vi.fn>).mockImplementationOnce(() => { throw new Error("spawn failed") }) global.fetch = vi.fn().mockResolvedValue({ ok: true, json: vi.fn().mockResolvedValue([]), }) as unknown as typeof fetch const request = new NextRequest("http://localhost:3000/api/opencode/servers/spawn", { method: "POST", body: JSON.stringify({ directory: "/home/user/project" }), }) const response = await POST(request) const data = await response.json() expect(response.status).toBe(500) expect(data.error).toBe("Failed to spawn server") - ;(spawn as ReturnType<typeof vi.fn>).mockImplementation( - originalSpawn as ReturnType<typeof vi.fn>, - ) })🤖 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 `@apps/web/src/app/api/opencode/servers/spawn/route.test.ts` around lines 335 - 338, The test restores the spawn mock with mockImplementation but that's unnecessary and wrong — replace the manual restoration with a one-shot mock that reverts automatically: change the usage of spawn.mockImplementationOnce(originalSpawn) to spawn.mockReturnValueOnce(originalSpawn) (or use mockImplementationOnce with a proper implementation function) and rely on vi.clearAllMocks() for cleanup; update the test around the spawn mock (and any related assertions) to mirror the other error tests that use mockReturnValueOnce to keep behavior consistent.
🤖 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.
Nitpick comments:
In `@apps/web/src/app/api/opencode/servers/spawn/route.test.ts`:
- Around line 335-338: The test restores the spawn mock with mockImplementation
but that's unnecessary and wrong — replace the manual restoration with a
one-shot mock that reverts automatically: change the usage of
spawn.mockImplementationOnce(originalSpawn) to
spawn.mockReturnValueOnce(originalSpawn) (or use mockImplementationOnce with a
proper implementation function) and rely on vi.clearAllMocks() for cleanup;
update the test around the spawn mock (and any related assertions) to mirror the
other error tests that use mockReturnValueOnce to keep behavior consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e15657ae-51ce-4510-a74c-6e767100c27b
📒 Files selected for processing (2)
apps/web/src/app/api/opencode/[port]/[[...path]]/route.test.tsapps/web/src/app/api/opencode/servers/spawn/route.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/app/api/opencode/[port]/[[...path]]/route.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR resolves 4 critical bugs in the proxy layer and part ID generation:
Bug Fixes
Fix Bug: prompt_async returns 400 - part IDs must start with "prt" #19: Part ID prefix mismatch
generatePartId()frompart-toprt-prefix inpackages/core/src/api/prompt.tsFix Bug: API proxy doesn't forward POST body in Bun/Next.js 16 #20: POST body forwarding crash
request.bodyReadableStream withrequest.text()for POST/PUT/PATCHduplex: "half"workaround that failed in bun/Next.js 16new NextResponse(null, { status: 204 })instead of crashingFix Bug: OpencodeSSRPlugin with empty directory from ServerStatus breaks store lookups #21: SSRPlugin directory mismatch
directoryoptional inOpencodeConfiginterfaceSessionLayoutto always renderOpencodeSSRPlugin(not conditionally)directory ?? ""for consistent store key lookupFix Bug: API proxy drops all headers except content-type and x-opencode-directory #23: Header forwarding incomplete
Tests Updated
duplex: "half"checks from POST/PUT/PATCH testsFiles Changed
apps/web/src/app/api/opencode/[port]/[[...path]]/route.ts— proxy implementationapps/web/src/app/api/opencode/[port]/[[...path]]/route.test.ts— proxy testspackages/core/src/api/prompt.ts— part ID generationpackages/react/src/next-ssr-plugin.tsx— SSR plugin directory handlingapps/web/src/app/session/[id]/session-layout.tsx— session layoutVerification
Closes #19, #20, #21, #23
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Chores
Tests