Skip to content

internal/cloudstorage: handle Windows volume paths - #826

Open
arpitjain099 wants to merge 1 commit into
ossf:mainfrom
arpitjain099:fix/cloudstorage-windows-volume
Open

internal/cloudstorage: handle Windows volume paths#826
arpitjain099 wants to merge 1 commit into
ossf:mainfrom
arpitjain099:fix/cloudstorage-windows-volume

Conversation

@arpitjain099

Copy link
Copy Markdown

Summary

internal/cloudstorage.parseBucketAndPrefix passes the raw destination to url.Parse. On a Windows absolute path like C:\path\to\file, url.Parse reads the drive letter as a single-character URL scheme (c), and url.URL.IsAbs() then returns true. That skips the scheme-less local-file branch entirely, so the volume is never recognised as a filesystem path and path.Split operates on the wrong value.

This is what makes the cmd/enumerate_github/marker tests fail on Windows, where t.TempDir() hands back volume-rooted absolute paths (#337).

Reproduction

url.Parse on a volume path:

url.Parse(`C:\foo\bar`)  -> scheme="c", opaque="\foo\bar", IsAbs()=true
url.Parse(`c:/foo/bar`)  -> scheme="c", path="/foo/bar",   IsAbs()=true

Because IsAbs() is true, the value is treated as a cloud bucket with scheme c instead of a local file.

Fix

Detect a single-letter drive followed by a volume separator (hasWindowsVolume) before parsing. When present, build the URL as an absolute local filesystem path, normalising backslashes to forward slashes so path.Split behaves the same regardless of the host OS, and prefixing / so the volume stays in the URL path rather than being parsed as a host.

Real scheme URLs (gs://, s3://, file://, mem://) take the existing url.Parse path and are unchanged.

Before / after

For C:\path\to\file:

  • before: scheme c, bucket mis-parsed, prefix wrong.
  • after: bucket file:///C:/path/to/?metadata=skip, prefix file. This mirrors the existing handling of a POSIX absolute path like /path/to/file (bucket file:///path/to/?metadata=skip, prefix file).

Tests

Added a table-driven TestParseBucketAndPrefixWindowsVolume covering the backslash, forward-slash, and lowercase-drive-letter forms. The case fails on the current source (drive letter parsed as scheme) and passes with the fix. The existing local-path and scheme-URL tests continue to pass.

go test ./internal/cloudstorage/ is green and go build ./... succeeds. These tests run on any OS, so they guard the Windows behaviour from a Linux CI runner.

Closes #337

parseBucketAndPrefix passed the raw path to url.Parse, which interprets a
Windows absolute path such as "C:\path\to\file" as a URL with the
single-letter scheme "c". The local-file branch was then skipped and the
volume was mishandled, so cmd/enumerate_github/marker tests failed on
Windows where t.TempDir() returns volume-rooted absolute paths.

Detect a single-letter drive followed by a volume separator before parsing
and treat it as an absolute local filesystem path, normalising backslashes
to forward slashes so path.Split works on any host OS. Real scheme URLs
(gs://, s3://, file://, mem://) are unaffected.

Adds table-driven tests covering backslash, forward-slash and lowercase
drive-letter forms.

Closes ossf#337

Signed-off-by: arpitjain099 <arpitjain099@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make internal/cloudstorage correctly support windows volumes

1 participant