diff --git a/internal/executionplane/artifact_collector.go b/internal/executionplane/artifact_collector.go index ef039b18..78bcdb96 100644 --- a/internal/executionplane/artifact_collector.go +++ b/internal/executionplane/artifact_collector.go @@ -8,13 +8,13 @@ import ( "encoding/json" "fmt" "mime" - "net/url" "os" "path/filepath" "sort" "strings" "github.com/devlikebear/tars/internal/atomicwrite" + "github.com/devlikebear/tars/internal/fileuri" ) const ( @@ -324,17 +324,17 @@ func writeCollectedArtifact(root, kind, name string, raw []byte) (CollectedArtif mediaType = "application/octet-stream" } return CollectedArtifact{ - Kind: kind, Name: filepath.ToSlash(name), URI: (&url.URL{Scheme: "file", Path: path}).String(), + Kind: kind, Name: filepath.ToSlash(name), URI: fileuri.New(path), Digest: fmt.Sprintf("sha256:%x", digest[:]), MediaType: mediaType, SizeBytes: int64(len(raw)), }, nil } func filepathFromURI(raw string) (string, error) { - parsed, err := url.Parse(raw) - if err != nil || parsed.Scheme != "file" || parsed.Host != "" { - return "", fmt.Errorf("executionplane: invalid file artifact URI") + path, err := fileuri.Path(raw) + if err != nil { + return "", fmt.Errorf("executionplane: invalid file artifact URI: %w", err) } - return filepath.FromSlash(parsed.Path), nil + return path, nil } var _ ArtifactCollector = (*FileArtifactCollector)(nil) diff --git a/internal/executionplane/local_environment.go b/internal/executionplane/local_environment.go index be4c7205..0cf89446 100644 --- a/internal/executionplane/local_environment.go +++ b/internal/executionplane/local_environment.go @@ -4,12 +4,12 @@ import ( "context" "encoding/json" "fmt" - "net/url" "os" "path/filepath" "strings" "time" + "github.com/devlikebear/tars/internal/fileuri" "github.com/devlikebear/tars/internal/proofverifier" "github.com/devlikebear/tars/internal/workstore" ) @@ -87,7 +87,7 @@ func (provider *LocalEnvironmentProvider) Sync(ctx context.Context, environment now := provider.now().UTC() return EnvironmentSnapshot{ ID: "snapshot:" + strings.TrimPrefix(digest, "sha256:"), Digest: digest, - URI: (&url.URL{Scheme: "file", Path: recovered.RootDir}).String(), + URI: fileuri.New(recovered.RootDir), MetadataJSON: metadata, CreatedAt: now, }, nil } diff --git a/internal/executionplane/worktree_environment.go b/internal/executionplane/worktree_environment.go index 1160e11c..9a2f9a5a 100644 --- a/internal/executionplane/worktree_environment.go +++ b/internal/executionplane/worktree_environment.go @@ -6,7 +6,6 @@ import ( "crypto/sha256" "encoding/json" "fmt" - "net/url" "os" "os/exec" "path/filepath" @@ -14,6 +13,7 @@ import ( "time" "github.com/devlikebear/tars/internal/atomicwrite" + "github.com/devlikebear/tars/internal/fileuri" "github.com/devlikebear/tars/internal/proofverifier" "github.com/devlikebear/tars/internal/workstore" ) @@ -161,7 +161,7 @@ func (provider *ManagedWorktreeProvider) Sync(ctx context.Context, environment E now := provider.now().UTC() return EnvironmentSnapshot{ ID: "snapshot:" + strings.TrimPrefix(digest, "sha256:"), Digest: digest, - URI: (&url.URL{Scheme: "file", Path: recovered.RootDir}).String(), + URI: fileuri.New(recovered.RootDir), MetadataJSON: metadata, CreatedAt: now, }, nil } diff --git a/internal/fileuri/fileuri.go b/internal/fileuri/fileuri.go new file mode 100644 index 00000000..8d3d6d6e --- /dev/null +++ b/internal/fileuri/fileuri.go @@ -0,0 +1,67 @@ +// Package fileuri converts between local paths and file: URIs. +// +// It exists because url.URL{Scheme: "file", Path: path} is wrong on Windows: +// `C:\dir\artifact.txt` renders as file://C:%5Cdir%5Cartifact.txt, where the +// drive letter becomes the URI host and every separator is percent-encoded. +// Such a URI is not resolvable — the parser here rejects it, and a naive +// consumer that strips the scheme gets a path that cannot be opened. +// +// A file URI needs forward slashes and a leading one ahead of the drive +// letter: file:///C:/dir/artifact.txt. On unix the absolute path already +// starts with a slash, so both forms agree and existing URIs stay valid. +package fileuri + +import ( + "fmt" + "net/url" + "path/filepath" + "strings" +) + +// New returns the file: URI for path. +// +// It takes no error because it cannot fail for its callers. path is made +// absolute so the URI identifies the same file from any working directory, +// and filepath.Abs only fails when the working directory cannot be +// determined — unreachable here, since every caller already holds an absolute +// path. Returning an error anyway would put a branch in four callers that no +// test could ever reach. +func New(path string) string { + resolved := strings.TrimSpace(path) + if absolute, err := filepath.Abs(resolved); err == nil { + resolved = absolute + } + uriPath := filepath.ToSlash(resolved) + if !strings.HasPrefix(uriPath, "/") { + uriPath = "/" + uriPath + } + return (&url.URL{Scheme: "file", Path: uriPath}).String() +} + +// Path returns the local path a file: URI refers to. +// +// A URI with a host is rejected rather than guessed at: it is either a remote +// UNC reference this code does not handle, or the malformed shape New exists +// to avoid. +func Path(raw string) (string, error) { + parsed, err := url.Parse(strings.TrimSpace(raw)) + if err != nil { + return "", fmt.Errorf("fileuri: parse %q: %w", raw, err) + } + if parsed.Scheme != "file" { + return "", fmt.Errorf("fileuri: %q is not a file URI", raw) + } + if parsed.Host != "" { + return "", fmt.Errorf("fileuri: %q has a host", raw) + } + uriPath := parsed.Path + // file:///C:/dir becomes /C:/dir when parsed; the leading slash is part of + // the URI grammar, not of the Windows path. + if len(uriPath) > 2 && uriPath[0] == '/' && uriPath[2] == ':' { + uriPath = uriPath[1:] + } + if uriPath == "" { + return "", fmt.Errorf("fileuri: %q has no path", raw) + } + return filepath.FromSlash(uriPath), nil +} diff --git a/internal/fileuri/fileuri_test.go b/internal/fileuri/fileuri_test.go new file mode 100644 index 00000000..5edb5d09 --- /dev/null +++ b/internal/fileuri/fileuri_test.go @@ -0,0 +1,68 @@ +package fileuri + +import ( + "path/filepath" + "strings" + "testing" +) + +// TestRoundTripPreservesPath is the property every caller depends on: a path +// turned into a URI and back must name the same file. +func TestRoundTripPreservesPath(t *testing.T) { + want := filepath.Join(t.TempDir(), "nested", "artifact.txt") + + uri := New(want) + got, err := Path(uri) + if err != nil { + t.Fatalf("Path(%q): %v", uri, err) + } + if got != want { + t.Fatalf("round trip = %q, want %q", got, want) + } +} + +// TestNewHasNoHostAndForwardSlashes pins the shape that was wrong on Windows, +// where url.URL{Scheme: "file", Path: path} put the drive letter in the host +// and percent-encoded every separator. +func TestNewHasNoHostAndForwardSlashes(t *testing.T) { + uri := New(filepath.Join(t.TempDir(), "artifact.txt")) + if !strings.HasPrefix(uri, "file:///") { + t.Fatalf("uri = %q, want a file:/// prefix", uri) + } + if strings.Contains(uri, "%5C") || strings.Contains(uri, `\`) { + t.Fatalf("uri = %q, want forward slashes only", uri) + } +} + +func TestNewResolvesRelativePaths(t *testing.T) { + t.Chdir(t.TempDir()) + + uri := New("artifact.txt") + path, err := Path(uri) + if err != nil { + t.Fatalf("Path: %v", err) + } + if !filepath.IsAbs(path) { + t.Fatalf("path = %q, want an absolute path", path) + } + if filepath.Base(path) != "artifact.txt" { + t.Fatalf("path = %q, want it to end at the requested file", path) + } +} + +func TestPathRejectsUnusableURIs(t *testing.T) { + cases := map[string]string{ + "other scheme": "https://example.test/artifact.txt", + "host present": "file://example.test/artifact.txt", + "no path": "file://", + "not a uri": "::not a uri::", + "malformed old": `file://C:%5Cdir%5Cartifact.txt`, + } + for name, raw := range cases { + t.Run(name, func(t *testing.T) { + if got, err := Path(raw); err == nil { + t.Fatalf("Path(%q) = %q, want an error", raw, got) + } + }) + } +} diff --git a/internal/tarsserver/work_scheduler_runtime_test.go b/internal/tarsserver/work_scheduler_runtime_test.go index 2b12f2a0..4344e949 100644 --- a/internal/tarsserver/work_scheduler_runtime_test.go +++ b/internal/tarsserver/work_scheduler_runtime_test.go @@ -18,6 +18,7 @@ import ( "github.com/devlikebear/tars/internal/agentruntime" "github.com/devlikebear/tars/internal/config" "github.com/devlikebear/tars/internal/executionplane" + "github.com/devlikebear/tars/internal/fileuri" "github.com/devlikebear/tars/internal/session" "github.com/devlikebear/tars/internal/workerprotocol" "github.com/devlikebear/tars/internal/workscheduler" @@ -639,12 +640,13 @@ func shellQuoteForTest(value string) string { return "'" + strings.ReplaceAll(value, "'", "'\\''") + "'" } +// filepathFromArtifactURI resolves an artifact URI the way production does. +// +// It used to strip "file://" and treat the rest as a path, which quietly +// accepted the malformed URIs this code used to produce on Windows and handed +// back a percent-encoded string that could not be opened. func filepathFromArtifactURI(raw string) (string, error) { - const prefix = "file://" - if !strings.HasPrefix(raw, prefix) { - return "", os.ErrInvalid - } - return strings.TrimPrefix(raw, prefix), nil + return fileuri.Path(raw) } func TestBuildNativeWorkExecutionPlaneSelectsHonestEnvironment(t *testing.T) { diff --git a/internal/workerprotocol/artifact_quarantine.go b/internal/workerprotocol/artifact_quarantine.go index 26cf2569..3a34a86f 100644 --- a/internal/workerprotocol/artifact_quarantine.go +++ b/internal/workerprotocol/artifact_quarantine.go @@ -13,6 +13,7 @@ import ( "unicode/utf8" "github.com/devlikebear/tars/internal/atomicwrite" + "github.com/devlikebear/tars/internal/fileuri" "github.com/devlikebear/tars/internal/secrets" ) @@ -217,7 +218,7 @@ func (quarantine *ArtifactQuarantine) release(ctx context.Context, placementID s func releasedArtifact(path string, artifact WireArtifact) ReleasedArtifact { return ReleasedArtifact{ - Name: artifact.Name, URI: (&url.URL{Scheme: "file", Path: path}).String(), + Name: artifact.Name, URI: fileuri.New(path), Digest: artifact.Digest, MediaType: artifact.MediaType, SizeBytes: int64(len(artifact.Data)), } }