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

[romanilyin]

Issue for this PR

Closes #34255

Part of #33107

Related: #30895, #19473

Type of change

• Bug fix
• New feature
• Refactor / code improvement
• Documentation

What does this PR do?

Validates request directory hints before InstanceStore.load.

Query values are consumed once, encoded SDK headers are decoded once, and Windows drive/UNC paths sent to a POSIX/WSL server return a structured 400 instead of being resolved under the server cwd.

The path logic stays behind one small wrapper. This PR does not migrate SQLite/App state, change session matching, resolve symlink identity, or translate Windows paths to WSL paths.

How did you verify your code works?

• Added parser tests for native POSIX/Windows paths and foreign rooted syntax.
• Added HTTP tests for query/header decoding and URL-sensitive directory names.
• Verified /mnt/c/... succeeds on WSL while C:\... and UNC directory hints return 400.
• Verified existing relative-directory and raw-header behavior remains compatible.

Screenshots / recordings

N/A — server/API change.

Checklist

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

[andrei-hasna]

Merge blocker found during review: the PR validates top-level directory query/header hints before InstanceStore.load, but the v2 ServerApi path still has an equivalent directory-hint bypass. packages/server/src/location.ts reads location[directory] and x-opencode-directory, decodes the header, and calls LocationServiceMap.get(...) without ServerDirectory.parse/native-path rejection. Those routes include location-scoped /api/* endpoints such as packages/protocol/src/groups/fs.ts, so a POSIX server can still accept /api/fs/find?location[directory]=C%3A%5CWork%5CRepo without the intended 400. Please apply the same server-native parsing/rejection to LocationMiddleware or a shared parser and add focused tests for location[directory] plus encoded x-opencode-directory on /api/* routes. Local evidence from a clean worktree: bun install --frozen-lockfile passed, focused server tests passed 49/49, and packages/opencode typecheck passed. Note: this account could not submit a formal request-changes review because GitHub reports it lacks explicit repository access.

[romanilyin]

Addressed in 255e714.

Changes:

• Moved ServerDirectory into @opencode-ai/server/server-directory so both v1 instance routing and v2 LocationMiddleware share the same parser.
• packages/server/src/location.ts now validates location[directory] query hints and decoded x-opencode-directory header hints before LocationServiceMap.get(...).
• Invalid v2 hints now return InvalidRequestError with kind: "Query" or kind: "Header" and field: "location[directory]".
• Added focused /api/fs/find coverage for both location[directory]=C%3A%5CWork%5CRepo and encoded x-opencode-directory.

Verification:

• bun test --timeout 60000 test/server/server-directory.test.ts test/server/httpapi-v2-location.test.ts test/server/httpapi-instance-context.test.ts test/server/httpapi-workspace-routing.test.ts test/server/httpapi-sdk.test.ts -> 54 pass
• bun typecheck in packages/server
• bun typecheck in packages/opencode
• bun run generate from packages/client -> no generated diff
• git diff --check
• pre-push bun turbo typecheck -> 29/29 packages passed

[andrei-hasna]

Follow-up safe-merge review on current head 255e714295885819f1a94d069cde8a556ff44bd1:

Validation in an isolated worktree merged into current origin/dev (61a7f6db35e1b8284258de247b889edf14748234) passed:

• bun install --frozen-lockfile
• bun test --timeout 60000 test/server/server-directory.test.ts test/server/httpapi-v2-location.test.ts test/server/httpapi-instance-context.test.ts test/server/httpapi-workspace-routing.test.ts test/server/httpapi-sdk.test.ts -> 54 pass
• bun typecheck in packages/server
• bun typecheck in packages/opencode
• bun run generate in packages/client with no generated diff
• bun turbo typecheck -> 29/29 successful
• git diff --check

Current state: the PR is open/non-draft, checks pass, local merge into latest dev is conflict-free. I could not merge from this account because GitHub reports viewerPermission: READ; mergeStateStatus is still BLOCKED even though mergeable is MERGEABLE and no branch protection rule was visible through the API.

Residual review notes before a maintainer merges: the original v2 LocationMiddleware blocker is addressed in this head. The remaining risk is narrower: POSIX/WSL parsing still intentionally accepts non-rooted Windows-like strings such as C:Work\\Repo and POSIX //server/share style paths, and the fix adds the new single-maintainer dependency @romanilyin/canonicalpath@2026.6.19-1 in request-boundary validation code. Those may be acceptable for this narrow issue, but should be an explicit maintainer decision.

[andrei-hasna]

Thanks for the focused fix. I locally validated the PR's merge result against current dev in a clean worktree and the new/changed server tests pass, and full bun typecheck passes.

I don't think this is merge-ready yet:

1. ServerDirectory.parse() still accepts slash-form UNC-like paths on POSIX/WSL. C:\Work\Repo, C:/Work/Repo, and \\server\share\repo are rejected, but //server/share/repo currently flows through as a native POSIX path before instance/location lookup. Given the PR says Windows drive/UNC hints are rejected, please either reject this form too or add an explicit test/comment showing that POSIX //... compatibility is intentional.

   Local repro against the PR merge:

   //server/share/repo => "//server/share/repo"
   \\server\share\repo => ServerDirectoryParseError foreign
   C:/Work/Repo => ServerDirectoryParseError foreign
   

   Relevant area: packages/server/src/server-directory.ts (parse, hasWindowsUncRoot) and the new server directory/v2 location tests.

2. Required branch ruleset checks have not run successfully. The required contexts are unit (linux), unit (windows), e2e (linux), e2e (windows), and typecheck; the PR head currently only has successful check-standards/check-compliance, while the pull_request workflows show action_required.

Other observed state: PR is open, non-draft, mergeable without content conflicts, but stale by 3 commits from current dev.

[romanilyin]

Addressed the latest review note in 86b443550.

Changes:

• Rebased the branch onto current origin/dev (dfeb1b5051a05b359bd4af711b204d2c0342c5f4), so the PR is no longer behind dev.
• ServerDirectory.parse() now rejects slash-form UNC-like hints such as //server/share/repo on POSIX/WSL, in addition to \\server\share\repo, C:\..., and C:/....
• Added coverage for //server/share/repo in the parser test, the v1/top-level directory middleware path, and v2 /api/fs/find query/header paths.

Verification:

• bun test --timeout 60000 test/server/server-directory.test.ts test/server/httpapi-v2-location.test.ts test/server/httpapi-instance-context.test.ts test/server/httpapi-workspace-routing.test.ts test/server/httpapi-sdk.test.ts -> 54 pass
• bun typecheck in packages/server
• bun typecheck in packages/opencode
• bun run generate from packages/client -> no generated diff
• git diff --check
• pre-push bun turbo typecheck -> 29/29 packages passed

Current GitHub state after push: the PR is mergeable and check-standards/check-compliance are passing. The required unit/e2e/typecheck workflow contexts still have not been started by GitHub on this fork PR, so I cannot clear those from my side beyond keeping the branch current and locally verified.

[andrei-hasna]

Blocking merge on one remaining cross-platform parser issue found by independent review and reproduced locally. I cannot submit this as a formal request-changes review because GitHub says this account lacks explicit repository access.

On the current PR head 86b443550783c5eed4f4a21e708f52d9f8cd18be, ServerDirectory.parse() rejects Windows-rooted hints on POSIX/WSL, but Windows-profile parsing still accepts POSIX-rooted hints and silently converts them into Windows paths. In packages/server/src/server-directory.ts, parseWindows() calls normalize(raw) without constraining the target profile, then toWin32(...) converts /tmp/repo to \tmp\repo. That means a request like ?directory=/tmp/repo to a Windows OpenCode server is not rejected as foreign syntax and can route to a different directory than the client intended.

Local reproduction on the PR merge tree:

/tmp/repo => \tmp\repo
file:///tmp/repo => ServerDirectoryParseError invalid
C:/Work/Repo => C:\Work\Repo
C:\Work\Repo => C:\Work\Repo
C:Work\Repo => ServerDirectoryParseError drive-relative
\\server\share\repo => \\server\share\repo
relative\repo => relative\repo

Required before merge:

• Reject POSIX-rooted paths in the Windows profile unless there is an explicit documented reason to treat them as native Windows current-drive paths.
• Add platform-independent unit coverage using ServerDirectory.parse(input, { kind: "win32" }) for /tmp/repo, file:///tmp/repo, drive-root paths, drive-relative paths, UNC paths, and relative paths.

Validation already run on the local merge result:

• bun install --frozen-lockfile passed.
• From packages/opencode: bun test --timeout 30000 test/server/server-directory.test.ts test/server/httpapi-instance-context.test.ts test/server/httpapi-workspace-routing.test.ts test/server/httpapi-v2-location.test.ts test/server/httpapi-sdk.test.ts -> 54 pass, 0 fail.
• From packages/server: bun typecheck passed.
• From packages/opencode: bun typecheck passed.

GitHub state is still mergeable=MERGEABLE, mergeStateStatus=BLOCKED, no review decision, and only check-standards/check-compliance are passing. I am not merging this head.

[romanilyin]

Addressed the Windows-profile parser blocker in ee7abd5ab.

Changes:

• Rebased the branch onto current origin/dev (58ba99e505a8c898f79e1438bddea35c197bc0af), so the PR is no longer behind dev.
• ServerDirectory.parse(input, { kind: "win32" }) now rejects POSIX-rooted single-slash hints such as /tmp/repo as foreign before normalize(...) can convert them to \tmp\repo.
• Added platform-independent Windows-profile unit coverage for /tmp/repo, file:///tmp/repo, drive-root paths, drive-relative paths, UNC paths, and relative paths.
• No canonicalpath change was needed; this is the OpenCode request-boundary wrapper policy.

Verification:

• bun test --timeout 60000 test/server/server-directory.test.ts -> 7 pass
• bun test --timeout 60000 test/server/server-directory.test.ts test/server/httpapi-v2-location.test.ts test/server/httpapi-instance-context.test.ts test/server/httpapi-workspace-routing.test.ts test/server/httpapi-sdk.test.ts -> 56 pass
• bun typecheck in packages/server
• bun typecheck in packages/opencode
• bun run generate from packages/client -> no generated diff
• git diff --check
• pre-push bun turbo typecheck -> 29/29 packages passed

Current GitHub state after push: PR is mergeable, branch is current with dev, and check-standards/check-compliance are passing. The upstream pull_request workflows are still gated by GitHub fork approval/action_required, which requires maintainer/admin approval.

[andrei-hasna]

Current safe-merge review on 2026-07-05 for head ee7abd5abdaa15eacc76f99a1592964775b37629:

Not merging this head. Current GitHub/API state is still mergeable=MERGEABLE but mergeStateStatus=BLOCKED; PR metadata base is 58ba99e505a8c898f79e1438bddea35c197bc0af, while current dev is b7e4f1ef7433f83ba009eefa2997aeb81017f6ed (dev...head shows 233 base commits ahead / 4 PR commits). Required pull_request workflows for this fork PR are still action_required (test, typecheck, storybook, nix-eval); only check-standards and check-compliance from pr-standards are green.

Local validation in an isolated worktree after merging the PR head into current dev:

• clean content merge, no conflicts
• bun install --frozen-lockfile passed with writable temp/cache env
• bun test --timeout 60000 test/server/server-directory.test.ts test/server/httpapi-v2-location.test.ts test/server/httpapi-instance-context.test.ts test/server/httpapi-workspace-routing.test.ts from packages/opencode -> 37 pass, 0 fail
• bun typecheck from packages/server -> pass
• bun typecheck from packages/opencode -> pass
• git diff --check / git diff --cached --check -> pass

Validation caveat: adding test/server/httpapi-sdk.test.ts to the same focused test command failed 19 SDK tests at startup with the shared logger trying to open /tmp/opencode-test-data-2/share/opencode/log/opencode.log before it exists. The new parser/location assertions passed; I am treating the SDK result as failed local validation evidence rather than merge clearance.

Adversarial review did not find a new foreign-path bypass in the updated parser/location changes. Residual maintainer decisions before merge: the PR intentionally preserves relative directory hints, the HTTP Windows/WSL behavior is only unit-tested via injected parser profiles in this Linux review environment, and the change adds the new runtime dependency @romanilyin/canonicalpath@2026.6.19-1 in request-boundary parsing code.

Required before merge: rebase/update the PR onto current dev, get the required GitHub workflows approved and green, and rerun/resolve the SDK test harness failure if it reproduces in the maintainer CI environment.

[romanilyin]

Updated the branch onto current dev in 85b275a7b (b7e4f1ef7433f83ba009eefa2997aeb81017f6ed base). The PR is now behind_by: 0 and remains mergeable.

Validation after the rebase:

• bun test --timeout 60000 test/server/server-directory.test.ts test/server/httpapi-v2-location.test.ts test/server/httpapi-instance-context.test.ts test/server/httpapi-workspace-routing.test.ts test/server/httpapi-sdk.test.ts from packages/opencode -> 56 pass, 0 fail
• The previously reported SDK logger startup failure with httpapi-sdk.test.ts did not reproduce locally on this head
• bun typecheck from packages/server -> pass
• bun typecheck from packages/opencode -> pass
• bun run generate from packages/client -> no generated diff
• git diff --check and git diff --check origin/dev..HEAD -> pass
• pre-push bun turbo typecheck -> 30/30 packages passed

GitHub state after push: check-standards and check-compliance are green. The fork pull_request workflows (test, typecheck, nix-eval, storybook) are still action_required, so they still need maintainer/admin approval to run.

[romanilyin]

Hi @StarpTech, gentle follow-up on this PR.

The underlying server-side issue still appears to be present on the current dev: LocationMiddleware continues to consume location[directory] / x-opencode-directory without rejecting foreign rooted path syntax.

The branch is now behind dev again, so before rebasing and rerunning the test suite, could you confirm whether maintainers are open to this approach?

I’m also happy to replace the @romanilyin/canonicalpath dependency with a small in-tree parser if the external dependency is the main concern. If the direction is acceptable, I’ll rebase the branch, resolve any drift, and refresh the validation results.

The required fork workflows are still awaiting maintainer approval to run.

[github-actions]

Automated PR Cleanup

Thank you for contributing to opencode.

Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions.

This PR was closed because it matched the following cleanup criteria:

• The PR was created more than 1 month ago
• The PR had fewer than 2 positive reactions
• Positive reactions are counted as thumbs-up, heart, celebration, or rocket reactions on the PR

PRs created within the last month are not affected by this cleanup.

If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate.

Thanks again for taking the time to contribute.

[romanilyin]

This PR was closed by the automated cleanup, but the underlying issue is still active and has since been reproduced again in #36902, which explicitly references this PR.

The current dev still accepts foreign Windows directory hints on Linux/WSL before instance/location lookup. This PR provides the narrow request-boundary validation part of the fix; it does not attempt automatic Windows-to-WSL translation or database migration.

I am still actively maintaining this contribution and can rebase it onto the latest dev, resolve any drift, and rerun the focused and full validation suites.

I’m also willing to replace @romanilyin/canonicalpath with a small in-tree implementation if the external runtime dependency is the main concern.

Could a maintainer please confirm whether this direction is acceptable and reopen the PR for an updated revision?