fix(docker): Windows paths not being converted into linux while being run in WSL by junaid2005p · Pull Request #366 · SurgeDM/Surge

junaid2005p · 2026-04-13T07:32:13Z

Greptile Summary

This PR fixes Windows absolute paths (drive letters and UNC) sent by WSL clients not being mapped to the server-side Linux download directory. It introduces IsWindowsAbsPath, MapWindowsPathToDefaultDir, and mapClientWindowsPath with segment-name matching and .. traversal rejection, and adds corresponding tests. The change is correct and addresses the previously raised silent-fallthrough concern.

The PR bundles several unrelated improvements — a Docker multi-stage build refactor with a non-root user, updated compose.yml, and a new CI tag-based workflow trigger — alongside the core path fix; these should ideally be separate PRs per the project's contribution guidelines.

Confidence Score: 5/5

Safe to merge — all remaining findings are P2 style/test suggestions with no blocking correctness or security issues.

The core path-mapping logic is correct, traversal attacks are rejected in MapWindowsPathToDefaultDir, and the previously raised silent-fallthrough concern is addressed by the new fallback mechanism. All open comments are non-blocking P2 improvements.

docker/compose.yml — the user field may silently default to 1000:1000 in most environments.

Important Files Changed

Filename	Overview
internal/utils/path.go	Adds IsWindowsAbsPath and MapWindowsPathToDefaultDir with sound logic; POSIX // negative test case missing from companion test file.
cmd/root_downloads.go	Adds mapClientWindowsPath and shouldFallbackUnmappedWindowsPath; correctly falls back to baseDir for unmapped Windows paths; missing end-to-end test for explicit non-RelativeToDefaultDir unmapped path on Linux.
cmd/http_handler_test.go	Refactors path resolution tests to use pre-computed expectedOutputPath; adds Windows path cases and TestShouldFallbackUnmappedWindowsPath; lacks explicit unmapped Windows path test without RelativeToDefaultDir.
internal/utils/path_test.go	Adds TestIsWindowsAbsPath and TestMapWindowsPathToDefaultDir with traversal, case-insensitive, and no-match cases; missing //server/share negative case.
docker/Dockerfile	Clean multi-stage build with non-root surge user, proper volume declarations, and TARGETOS/TARGETARCH build args; jq correctly scoped to downloader stage.
docker/compose.yml	Adds user field for non-root execution but $UID is not exported by bash by default so the value silently defaults to 1000 in most environments.
.github/workflows/build-push-images.yml	Adds tag-based trigger, automatic version extraction from tag name, and a latest-tag step gated on clean semver tags; logic is correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Client sends DownloadRequest] --> B{IsWindowsAbsPath?}
    B -- No --> C{RelativeToDefaultDir?}
    B -- Yes --> D[mapClientWindowsPath]
    C -- Yes --> E[Join baseDir + reqPath]
    C -- No --> F[EnsureAbsPath reqPath]
    D --> G{MapWindowsPathToDefaultDir matches?}
    G -- Yes --> H[Return baseDir/suffix]
    G -- No --> I{shouldFallbackUnmappedWindowsPath?}
    I -- true --> J[Return filepath.Clean baseDir]
    I -- false --> K[Return empty, fall through]
    H --> L[resolveOutputDir returns path]
    J --> L
    E --> L
    F --> L
    K --> C

Prompt To Fix All With AI

This is a comment left during a code review.
Path: docker/compose.yml
Line: 12

Comment:
**`${UID}` is not exported by bash by default**

`$UID` is a bash read-only special variable that is NOT exported to child processes. When Docker Compose is launched normally, it won't see the host user's UID from the environment — `${UID:-1000}` will always resolve to `1000`, defeating the purpose of the mapping. Users need to explicitly export it or set it before running compose:

```bash
UID=$(id -u) GID=$(id -g) docker compose up
```

Or add to a `.env` file alongside `compose.yml`:
```
UID=1000
GID=1000
```

Consider adding a comment or README note explaining this requirement, or using a wrapper script that pre-sets the variables.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: internal/utils/path_test.go
Line: 51-63

Comment:
**Missing negative test for POSIX `//` double-slash path**

The implementation explicitly notes that `//path` is POSIX-legal and must NOT be treated as a Windows UNC path (only `\\` backslash-backslash is UNC). There's no test case confirming `//server/share` returns `false`, which is the critical distinction the code comment calls out.

```suggestion
	tests := []struct {
		path string
		want bool
	}{
		{path: `C:\Users\me\Downloads`, want: true},
		{path: `C:/Users/me/Downloads`, want: true},
		{path: `\\server\share\file.zip`, want: true},
		{path: `/tmp/downloads`, want: false},
		{path: `downloads/subdir`, want: false},
		{path: `//server/share`, want: false},
	}
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: cmd/http_handler_test.go
Line: 155-165

Comment:
**No test for explicit unmapped Windows path on Linux without `RelativeToDefaultDir`**

The "Unmatched Windows Path Falls Back To Default Dir" case only covers the `RelativeToDefaultDir: true` branch. However, `shouldFallbackUnmappedWindowsPath(false, "linux")` also returns `true`, meaning an explicit Windows path that doesn't match the server's download folder name silently falls back to the default dir on Linux. Consider adding a case with `Path: "E:/Torrents/complete"` and no `RelativeToDefaultDir` flag to cover this branch end-to-end.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (8): Last reviewed commit: "fix(utils): reject parent traversal segm..." | Re-trigger Greptile}

Context used:

Rule used - What: Flag commits that bundle unnecessary changes... (source)

…irectory

greptile-apps · 2026-04-13T07:37:52Z

+FROM alpine:3.21 AS downloader

-RUN apk add --no-cache ca-certificates curl jq
+RUN apk add --no-cache ca-certificates curl jq tar


jq installed but no longer used

jq was needed to parse the GitHub API response when auto-fetching the latest version. Since version auto-detection was removed, jq is no longer used anywhere in this stage and can be dropped to keep the image lean.

Suggested change

RUN apk add --no-cache ca-certificates curl jq tar

RUN apk add --no-cache ca-certificates curl tar

Prompt To Fix With AI

This is a comment left during a code review. Path: docker/Dockerfile Line: 3 Comment: **`jq` installed but no longer used** `jq` was needed to parse the GitHub API response when auto-fetching the latest version. Since version auto-detection was removed, `jq` is no longer used anywhere in this stage and can be dropped to keep the image lean. ```suggestion RUN apk add --no-cache ca-certificates curl tar ``` How can I resolve this? If you propose a fix, please make it concise.

…ath projection fails

… root on non-Windows hosts

junaid2005p added 3 commits April 13, 2026 12:54

docker: harden Surge image and align compose with rootless state paths

33c4c2f

fix(utils): detect Windows absolute paths on non Windows hosts

b2e1c1c

fix(cmd): map Windows client download paths onto the server default d…

5227c6a

…irectory

greptile-apps Bot reviewed Apr 13, 2026

View reviewed changes

junaid2005p added 4 commits April 13, 2026 13:21

fix(docker): restore latest version fallback for local image builds

187cfcc

test(utils): add edge case coverage for windows path projection

5e58a9d

ci(docker): publish container images automatically on release tags

f2f461a

fix(utils): address UNC check issue

73719e0

greptile-apps Bot reviewed Apr 13, 2026

View reviewed changes

Comment thread cmd/root_downloads.go Outdated

junaid2005p added 7 commits April 13, 2026 18:08

fix(remote-downloads): fall back to server default dir when windows p…

579b55c

…ath projection fails

ci(docker): make latest tagging explicti for release tag builds

642029a

fix(utils): use first match semantics for windows path projection

e2c6b7d

test(cmd): make download path resolution assertions fully table-driven

c5d53c5

ci(docker): tag latest only for stable release builds or manual opt-in

b26ae6e

fix(remote-downloads): keep unmatched Windows paths inside the server…

be9f2b2

… root on non-Windows hosts

fix(utils): reject parent traversal segments in Windows path projection

dc342e0

junaid2005p requested a review from SuperCoolPencil April 13, 2026 13:57

SuperCoolPencil approved these changes Apr 13, 2026

View reviewed changes

junaid2005p merged commit 8f3fff5 into main Apr 13, 2026
21 checks passed

junaid2005p deleted the docker-path-fix branch April 13, 2026 15:12

SuperCoolPencil mentioned this pull request Apr 16, 2026

Fix WSL download path handling #375

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(docker): Windows paths not being converted into linux while being run in WSL#366

fix(docker): Windows paths not being converted into linux while being run in WSL#366
junaid2005p merged 14 commits intomainfrom
docker-path-fix

junaid2005p commented Apr 13, 2026 •

edited by greptile-apps Bot

Loading

Uh oh!

Uh oh!

greptile-apps Bot Apr 13, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	RUN apk add --no-cache ca-certificates curl jq tar
	RUN apk add --no-cache ca-certificates curl tar

Uh oh!

Conversation

junaid2005p commented Apr 13, 2026 • edited by greptile-apps Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Flowchart

Uh oh!

Uh oh!

greptile-apps Bot Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

junaid2005p commented Apr 13, 2026 •

edited by greptile-apps Bot

Loading