Repository navigation
fix(server): reject unusable session location directories with 400 - #59162
LeizhenZhang wants to merge 1 commit into
Conversation
github-actions
Bot
added
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix works. With the requests from #59164, the base returns an empty 500 for both the NUL-byte query directory and the relative POST /api/session location. With this PR, both return a 400 InvalidRequestError. Absolute directories and requests with no directory still return 200. The new test fails on the base and passes here, and the existing session create/import/form/instances tests pass.
Public API:
- Session create and import payloads:
locationchanges fromLocation.PublicRefto the newLocation.Input, which requires an absolute POSIX, drive or UNC path without NUL bytes. In the generated client,SessionCreateInput.locationandSessionImportInput.locationchange type the same way. - Schema exports:
@opencode/schema/locationaddsDirectoryandInput. - Location query and header:
location[directory]andx-opencode-directorynow return 400 for relative or NUL-containing directories. - Compatibility: compatible for callers that send absolute paths, which as far as I can see includes the CLI and client. However,
?location[directory]=.returned 200 on dev and now returns 400, and a relativex-opencode-directoryheader changes from 404 to 400. Please confirm this change is intended.
Concerns:
- Routes without the check:
pty.connect(handlers/pty.ts) andmcp.client(handlers/mcp.ts) still read the directory withrequestRefdirectly, so the new check never runs there.handlers/debug.tsdoes the same. Routing those throughrequestLocationwould cover every location entry point. - Overlap with #54619: that open PR validates the same entry points but keeps relative paths working, so the two approaches should be reconciled.
Fixes #59164
456d5b4 to
07e72fa
Compare
LeizhenZhang
changed the title
|
Thanks for the review. I narrowed this PR to the request body (head
|
There was a problem hiding this comment.
The new commits narrow the PR to the session create and import payloads, and that part works. With the #59164 script, a relative or NUL-byte location.directory in POST /api/session returns an empty 500 on dev and a 400 InvalidRequestError with this PR, while an absolute directory still returns 200. The new test and the session create/import tests pass, and bun run generate leaves the committed client files unchanged.
The earlier points are addressed by keeping the query and header middleware as they are and leaving that to #59414. Relative hints like ?location[directory]=. keep working, and the overlap with #59414 is gone.
Public API:
- Session create and import payloads: the
locationfield changes fromLocation.PublicRefto the newLocation.Input. The directory must now be an absolute POSIX, drive or UNC path without NUL bytes, and other values get a 400 where they used to get a 500. The generated client'sSessionCreateInput.locationandSessionImportInput.locationchange type the same way. - Schema exports:
@opencode/schema/locationaddsDirectoryandInput. - Compatibility: compatible for callers that send absolute paths, which the CLI and client do.
Concern: the description now says Closes #59164, but the issue's first case (GET /api/agent?location[directory]=%00) still returns an empty 500 with this PR. That case is left to #59414. Merging this would close the issue while half of it is still open. Please use "Part of #59164" until #59414 lands.
Session create and import accepted any string as location.directory. A relative path or one with a NUL byte reached the database and filesystem layers and the client got an empty HTTP 500. The two payloads now use Location.Input, whose directory must be a POSIX, drive or UNC absolute path without NUL bytes, so a bad body location is a 400 InvalidRequestError. Location hints in the query and header are left as they are.
07e72fa to
b1bfbe6
Compare
|
Rebased onto current |
|
On b1bfbe6, check, check-test, check-bot-scope and check-nix remain |
Issue for this PR
Part of #59164
Type of change
What does this PR do?
POST /api/sessionand session import accepted any string aslocation.directory. A relative path or one with a NUL byte reached the database and filesystem layers, and the client got an empty HTTP 500. The two payloads now useLocation.Input, whosedirectorymust be a POSIX, drive or UNC absolute path without NUL bytes, so a bad body location is a 400InvalidRequestError.Location hints in the query string and the
x-opencode-directoryheader are left as they are. #59414 covers those entry points (middleware, forms, debug, PTY, MCP client) and keeps relative hints such as?location[directory]=.working, so this PR stays out of its way.How did you verify your code works?
Rebased onto
devat986221afon 2026-10-30. The generated client type now follows the experimental import namespace introduced ondev.packages/server/test/location-input.test.ts: a NUL byte and a relative directory in thePOST /api/sessionbody. Both return 500 on dev and fail, both return 400 with the fix.session-createandsession-importtests pass (9 tests together with the new file).bun run generateinpackages/client: onlySessionCreateInput.locationandExperimentalSessionImportInput.locationchange, fromLocation.PublicReftoLocation.Input.bun run typecheckin schema, protocol, server and client, prettier on the touched files, and the rootbun run check(35/35) passed. On rebased headb1bfbe6, the two new server tests pass,check:generatedpasses, and schema, protocol, client and server typechecks pass. The full root check has not been rerun on this head.Checklist
Added by the opencode bot (AI-generated).
Part of #59164
Public API: session create/import
location.directorymust be an absolute path without NUL bytes (new Location.Input / Location.Directory); bad values get a 400 instead of a 500