fix(server): reject foreign directory hints before location lookup by romanilyin · Pull Request #59414 · anomalyco/opencode

romanilyin · 2026-10-26T22:04:21Z

Issue for this PR

Closes #54617. Part of #54615. Replaces #54619 after development moved from v2 to dev.

Type of change

Bug fix
New feature
Refactor / code improvement
Documentation

What does this PR do?

Return typed HTTP 400 for Windows drive/backslash-UNC hints sent to a POSIX server, and for invalid directory syntax, before Location lookup. Cover middleware, global forms, debug eviction, PTY connect and MCP client connect.

Preserve native path spelling: POSIX //... and ///..., Windows drive/UNC and drive-root-relative paths, relative hints, query precedence and single decoding. Windows validation discards the normalized value, leaving existing Location/session identity handling unchanged.

Keep the pinned MIT dependency @romanilyin/[email protected] for shared Windows syntax validation. I maintain this package; retaining one implementation lets subsequent path-contract stages in #54615 reuse its fixes instead of maintaining a copied parser. This PR does not migrate stored keys. #59162 separately validates locations in session create/import bodies.

How did you verify your code works?

WSL / Bun 1.4.4: 36 tests / 249 assertions passed; the two changed test files also passed 10 repeats (170 tests / 1,140 assertions). Native Windows: 35 tests passed with one existing PTY websocket skip; the final adjusted HTTP fixture was rechecked (6 tests / 54 assertions). Server checks passed with Bun and the pinned TypeScript compiler, and root bun run check passed (35/35 tasks). The tracked App declaration symlink was temporarily materialized for the root check and its original bytes restored.
HTTP rejection coverage uses one server fixture and 10 requests across five routes and two transports; parsing cases remain in unit tests.

Visual

<svg xmlns="http://www.w3.org/2000/svg" width="240" height="72" viewBox="0 0 240 72">
  <style>@media (prefers-reduced-motion:no-preference){.gate{animation:pulse 2s ease-in-out infinite}@keyframes pulse{50%{opacity:.45}}}</style>
  <g fill="none" stroke="currentColor"><rect x="6" y="18" width="93" height="36" rx="6"/><path d="M104 36h28m-5-4 5 4-5 4"/><rect class="gate" x="140" y="18" width="94" height="36" rx="6"/></g>
  <g fill="currentColor" font-family="monospace" font-size="11" text-anchor="middle"><text x="52" y="40">C:\Work</text><text x="187" y="40">400 Invalid</text></g>
</svg>

Screenshots / recordings

Not applicable; HTTP request validation.

Checklist

I have tested my changes locally
I have not included unrelated changes in this PR

opencode-agent

On Linux this fixes the reported problem, but it also starts rejecting valid POSIX paths that begin with //. I ran the #54617 reproduction from source on the base and on this branch. Drive and UNC hints in the query and the header went from 404 LocationNotFoundError to 400 InvalidRequestError with the right kind/field, and native paths still return 200. However, location[directory]=//tmp/<dir> returned 200 on the base and now returns 400 (inline comment). The new tests pass (20/20) and tsc -b for the server is clean. The Windows server branch is untested here and by you.

Public API: every route behind the location middleware (and the global form, debug eviction, PTY connect and client-MCP routes) can now return 400 InvalidRequestError for location[directory] / x-opencode-directory. InvalidRequestError is added to the protocol client's LocationMiddleware error union. This is backwards compatible for callers sending native paths. Breaking changes:

Drive and UNC hints to a POSIX server: 404 → 400.
//-prefixed POSIX paths: 200 → 400.
/-rooted hints to a Windows server: now 400.
A Windows server now normalises hints (e.g. C:/x → C:\x) before lookup.

Other concerns:

New dependency: it is published by the PR author and pinned to a prerelease version. On POSIX the outcome comes entirely from the in-tree regexes, so the package mostly matters on Windows, where it rewrites the hint. I'd prefer the small in-tree version you offered.
Tests: location.test.ts is large for what it checks. Please trim it to the core cases.

#59162 changes the same code and handles relative paths the opposite way (it rejects them, this keeps them), so the two need reconciling.

Related: #54615, #52205, #59164

opencode-agent · 2026-10-26T22:07:01Z

+  return /^[A-Za-z]:(?:$|[^\\/])/.test(value)
+}
+
+function hasWindowsUncRoot(value: string) {


On a POSIX server // followed by any character is a valid path (Linux treats //tmp/x as /tmp/x). Before this change location[directory]=//tmp/<dir> returned 200. Now it returns 400 "not native to this OpenCode server". Clients that join paths loosely would break. On POSIX I'd only treat backslash UNC (\\server\share) and the //?/ / //./ device forms as foreign, and leave plain //... alone.

Fixed in 493321a. The adapter now preserves all forward-slash-rooted POSIX/WSL hints, including //tmp, //tmp/x, ///tmp/x and //server/share. Their spelling alone cannot distinguish a POSIX path from a forward-slash UNC representation. Drive roots and backslash UNC roots are still rejected on POSIX. Unit coverage and real HTTP query/header controls cover the regression.

opencode-agent · 2026-10-26T22:07:01Z

+  return raw as Directory
+}
+
+function parseWindows(raw: string): Directory {


On Windows this branch changes behaviour in two ways that no test covers end to end. /-rooted hints are now rejected. Drive/UNC hints are also rewritten through normalize/toWin32 before LocationServiceMap.get, so C:/x and C:\x now share a cache key, and the stored location directory may differ from what the client sent. That needs checking against how sessions and projects store directories on Windows. This is also the only place where the external @romanilyin/canonicalpath package affects the result. If the normalisation stays, an in-tree version would avoid a new runtime dependency on a prerelease package.

Fixed the identity change in 493321a: Windows validation returns the original hint, discarding the normalized value. Drive casing, separators, UNC spellings, ordinary relative paths and drive-root-relative /... / \... hints are preserved before existing Location handling. This PR no longer introduces key rewriting.

Native Windows / Bun 1.4.4: 35 tests passed across the five affected/regression files, with one existing Windows-only PTY websocket skip. After correcting the HTTP fixture to use the current drive rather than the TEMP drive, its final native run also passed (6 tests / 54 assertions).

I am keeping the pinned MIT @romanilyin/canonicalpath dependency for shared Windows syntax validation and the subsequent stages in #54615. I maintain the package, and keeping one parser lets those stages receive the same fixes instead of maintaining a copied implementation. Identity migration remains separate from this boundary fix.

opencode-agent · 2026-10-26T22:07:01Z

+  expect(ref).toMatchObject({ kind: source === "query" ? "Query" : "Header", field: "location[directory]" })
+})
+
+it.live("rejects foreign directories before all request-based location lookups", () =>


This test and the two after it boot full servers and loop over 5 endpoints × 4 paths × 2 sources, which is a lot for checking one boundary. One request per route kind (middleware, debug, PTY, MCP) with a single drive path in the query and the header, plus a native-path control, would cover the fix. The requestRef unit tests above already cover the parsing cases.

Simplified in 493321a: one server fixture instead of three, with 10 rejection requests (five entry points × query/header) instead of 40. Native-path controls and the SDK error assertion reuse that fixture; parsing, exact spelling, precedence and decoding cases stay in unit tests. This retains coverage of each distinct requestRef consumer without repeating the full path matrix over HTTP.

benjaminkincses

hasWindowsUncRoot in server-directory.ts rejects //tmp but lets ///tmp through. same dir on linux, different verdict. the UNC check needs a server segment, not just // plus one char. //server/share is UNC, //tmp is not.

romanilyin · 2026-10-30T13:25:43Z

Thanks @Dextheking1 — the regression was in the OpenCode adapter. 493321a fixes it and preserves incoming path spelling on Windows as well. On POSIX, even //server/share can be a native path, so the adapter now leaves all forward-slash roots unchanged instead of guessing UNC from that prefix.

WSL / Bun 1.4.4: 36 tests / 249 assertions passed; the two changed test files also passed 10 repeats (170 tests / 1,140 assertions). Native Windows: 35 tests passed with one existing PTY websocket skip; the final adjusted HTTP fixture was rechecked (6 tests / 54 assertions). Server checks passed with Bun and the pinned TypeScript compiler, and root bun run check passed (35/35 tasks). The tracked App declaration symlink was temporarily materialized for the root check and its original bytes restored.

I have also reduced the HTTP test matrix and kept the canonicalpath dependency for the staged work in #54615. The current scope of #59162 is session create/import body validation; query/header relative hints remain supported here, so the earlier overlap no longer applies.

fix(server): reject foreign directory hints before location lookup

6ffba0b

opencode-agent Bot reviewed Oct 26, 2026

View reviewed changes

benjaminkincses reviewed Oct 27, 2026

View reviewed changes

LeizhenZhang mentioned this pull request Oct 27, 2026

fix(server): reject unusable session location directories with 400 #59162

Open

3 of 6 tasks

fix(server): preserve native directory hint spelling

493321a

Navigation Menu

Repository navigation

fix(server): reject foreign directory hints before location lookup - #59414

fix(server): reject foreign directory hints before location lookup#59414
romanilyin wants to merge 2 commits into
anomalyco:devfrom
romanilyin:canonicalpath-dev

romanilyin commented Oct 26, 2026 •

edited

Loading

Uh oh!

opencode-agent Bot left a comment

Uh oh!

opencode-agent Bot Oct 26, 2026

Uh oh!

romanilyin Oct 30, 2026

Uh oh!

opencode-agent Bot Oct 26, 2026

Uh oh!

romanilyin Oct 30, 2026

Uh oh!

opencode-agent Bot Oct 26, 2026

Uh oh!

romanilyin Oct 30, 2026

Uh oh!

benjaminkincses left a comment

Uh oh!

romanilyin commented Oct 30, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

romanilyin commented Oct 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue for this PR

Type of change

What does this PR do?

How did you verify your code works?

Visual

Screenshots / recordings

Checklist

Uh oh!

opencode-agent Bot left a comment

Choose a reason for hiding this comment

Uh oh!

opencode-agent Bot Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

romanilyin Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

opencode-agent Bot Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

romanilyin Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

opencode-agent Bot Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

romanilyin Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

benjaminkincses left a comment

Choose a reason for hiding this comment

Uh oh!

romanilyin commented Oct 30, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

romanilyin commented Oct 26, 2026 •

edited

Loading