chore: revert back to FileServer to prevent bytes in memory (#87)

Author: Emyrk

api/api.go

@@ -112,7 +112,7 @@ func New(options *Options) *API {
 	r.Post("/api/extensionquery", api.extensionQuery)
 
 	// Endpoint for getting an extension's files or the extension zip.
-	r.Mount("/files", http.StripPrefix("/files", storage.HTTPFileServer(options.Storage)))
+	r.Mount("/files", http.StripPrefix("/files", options.Storage.FileServer()))
 
 	// VS Code can use the files in the response to get file paths but it will
 	// sometimes ignore that and use requests to /assets with hardcoded types to

go.mod

@@ -9,7 +9,6 @@ require (
 	github.com/go-chi/httprate v0.14.1
 	github.com/google/uuid v1.6.0
 	github.com/lithammer/fuzzysearch v1.1.8
-	github.com/spf13/afero v1.11.0
 	github.com/spf13/cobra v1.8.1
 	github.com/stretchr/testify v1.9.0
 	golang.org/x/mod v0.19.0
@@ -18,6 +17,7 @@ require (
 )
 
 require (
+	cloud.google.com/go/logging v1.8.1 // indirect
 	github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect
 	github.com/cespare/xxhash/v2 v2.3.0 // indirect
 	github.com/charmbracelet/lipgloss v0.7.1 // indirect
@@ -38,6 +38,9 @@ require (
 	golang.org/x/sys v0.17.0 // indirect
 	golang.org/x/term v0.17.0 // indirect
 	golang.org/x/text v0.14.0 // indirect
+	google.golang.org/genproto v0.0.0-20231106174013-bbf56f31fb17 // indirect
+	google.golang.org/genproto/googleapis/api v0.0.0-20231106174013-bbf56f31fb17 // indirect
+	google.golang.org/genproto/googleapis/rpc v0.0.0-20231120223509-83a465c0220f // indirect
 	google.golang.org/protobuf v1.32.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )

go.sum

@@ -5,10 +5,10 @@ cloud.google.com/go/compute v1.23.3 h1:6sVlXXBmbd7jNX0Ipq0trII3e4n1/MsADLK6a+aiV
 cloud.google.com/go/compute v1.23.3/go.mod h1:VCgBUoMnIVIR0CscqQiPJLAG25E3ZRZMzcFZeQ+h8CI=
 cloud.google.com/go/compute/metadata v0.2.3 h1:mg4jlk7mCAj6xXp9UJ4fjI9VUI5rubuGBW5aJ7UnBMY=
 cloud.google.com/go/compute/metadata v0.2.3/go.mod h1:VAV5nSsACxMJvgaAuX6Pk2AawlZn8kiOGuCv6gTkwuA=
-cloud.google.com/go/logging v1.7.0 h1:CJYxlNNNNAMkHp9em/YEXcfJg+rPDg7YfwoRpMU+t5I=
-cloud.google.com/go/logging v1.7.0/go.mod h1:3xjP2CjkM3ZkO73aj4ASA5wRPGGCRrPIAeNqVNkzY8M=
-cloud.google.com/go/longrunning v0.5.1 h1:Fr7TXftcqTudoyRJa113hyaqlGdiBQkp0Gq7tErFDWI=
-cloud.google.com/go/longrunning v0.5.1/go.mod h1:spvimkwdz6SPWKEt/XBij79E9fiTkHSQl/fRUUQJYJc=
+cloud.google.com/go/logging v1.8.1 h1:26skQWPeYhvIasWKm48+Eq7oUqdcdbwsCVwz5Ys0FvU=
+cloud.google.com/go/logging v1.8.1/go.mod h1:TJjR+SimHwuC8MZ9cjByQulAMgni+RkXeI3wwctHJEI=
+cloud.google.com/go/longrunning v0.5.4 h1:w8xEcbZodnA2BbW6sVirkkoC+1gP8wS57EUUgGS0GVg=
+cloud.google.com/go/longrunning v0.5.4/go.mod h1:zqNVncI0BOP8ST6XQD1+VcvuShMmq7+xFSzOL++V0dI=
 github.com/aymanbagabas/go-osc52/v2 v2.0.1 h1:HwpRHbFMcZLEVr42D4p7XBqjyuxQH5SMiErDT4WkJ2k=
 github.com/aymanbagabas/go-osc52/v2 v2.0.1/go.mod h1:uYgXzlJ7ZpABp8OJ+exZzJJhRNQ2ASbcXHWsFqH8hp8=
 github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs=
@@ -56,8 +56,6 @@ github.com/rivo/uniseg v0.2.0/go.mod h1:J6wj4VEh+S6ZtnVlnTBMWIodfgj8LQOQFoIToxlJ
 github.com/rivo/uniseg v0.4.4 h1:8TfxU8dW6PdqD27gjM8MVNuicgxIjxpm4K7x4jp8sis=
 github.com/rivo/uniseg v0.4.4/go.mod h1:FN3SvrM+Zdj16jyLfmOkMNblXMcoc8DfTHruCPUcx88=
 github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
-github.com/spf13/afero v1.11.0 h1:WJQKhtpdm3v2IzqG8VMqrr6Rf3UYpEF239Jy9wNepM8=
-github.com/spf13/afero v1.11.0/go.mod h1:GH9Y3pIexgf1MTIWtNGyogA5MwRIDXGUr+hbWNoBjkY=
 github.com/spf13/cobra v1.8.1 h1:e5/vxKd/rZsfSJMUX1agtjeTDf+qv1/JdBF8gg5k9ZM=
 github.com/spf13/cobra v1.8.1/go.mod h1:wHxEcudfqmLYa8iTfL+OuZPbBZkmvliBWKIezN3kD9Y=
 github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA=

storage/artifactory.go

@@ -7,7 +7,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"io/fs"
 	"net/http"
 	"os"
 	"path"
@@ -16,7 +15,6 @@ import (
 	"sync"
 	"time"
 
-	"github.com/spf13/afero/mem"
 	"golang.org/x/sync/errgroup"
 	"golang.org/x/xerrors"
 
@@ -277,37 +275,25 @@ func (s *Artifactory) AddExtension(ctx context.Context, manifest *VSIXManifest,
 	return s.uri + dir, nil
 }
 
-// Open returns a file from Artifactory.
-// TODO: Since we only extract a subset of files perhaps if the file does not
-// exist we should download the vsix and extract the requested file as a
-// fallback.  Obviously this seems like quite a bit of overhead so we would
-// then emit a warning so we can notice that VS Code has added new asset types
-// that we should be extracting to avoid that overhead.  Other solutions could
-// be implemented though like extracting the VSIX to disk locally and only
-// going to Artifactory for the VSIX when it is missing on disk (basically
-// using the disk as a cache).
-func (s *Artifactory) Open(ctx context.Context, fp string) (fs.File, error) {
-	resp, code, err := s.read(ctx, fp)
-	if code != http.StatusOK || err != nil {
-		switch code {
-		case http.StatusNotFound:
-			return nil, fs.ErrNotExist
-		case http.StatusForbidden:
-			return nil, fs.ErrPermission
-		default:
-			return nil, err
+func (s *Artifactory) FileServer() http.Handler {
+	// TODO: Since we only extract a subset of files perhaps if the file does not
+	// exist we should download the vsix and extract the requested file as a
+	// fallback.  Obviously this seems like quite a bit of overhead so we would
+	// then emit a warning so we can notice that VS Code has added new asset types
+	// that we should be extracting to avoid that overhead.  Other solutions could
+	// be implemented though like extracting the VSIX to disk locally and only
+	// going to Artifactory for the VSIX when it is missing on disk (basically
+	// using the disk as a cache).
+	return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
+		reader, code, err := s.read(r.Context(), r.URL.Path)
+		if err != nil {
+			http.Error(rw, err.Error(), code)
+			return
 		}
-	}
-
-	// TODO: Do no copy the bytes into memory, stream them rather than
-	// storing the entire file into memory.
-	f := mem.NewFileHandle(mem.CreateFile(fp))
-	_, err = io.Copy(f, resp)
-	if err != nil {
-		return nil, err
-	}
-
-	return f, nil
+		defer reader.Close()
+		rw.WriteHeader(http.StatusOK)
+		_, _ = io.Copy(rw, reader)
+	})
 }
 
 func (s *Artifactory) Manifest(ctx context.Context, publisher, name string, version Version) (*VSIXManifest, error) {

storage/local.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"io/fs"
 	"net/http"
 	"os"
 	"path/filepath"
@@ -142,8 +141,8 @@ func (s *Local) AddExtension(ctx context.Context, manifest *VSIXManifest, vsix [
 	return dir, nil
 }
 
-func (s *Local) Open(_ context.Context, fp string) (fs.File, error) {
-	return http.Dir(s.extdir).Open(fp)
+func (s *Local) FileServer() http.Handler {
+	return http.FileServer(http.Dir(s.extdir))
 }
 
 func (s *Local) Manifest(ctx context.Context, publisher, name string, version Version) (*VSIXManifest, error) {

storage/signature.go

@@ -2,14 +2,13 @@ package storage
 
 import (
 	"context"
-	"io/fs"
-	"path/filepath"
+	"net/http"
+	"strconv"
 	"strings"
 
-	"github.com/spf13/afero/mem"
-	"golang.org/x/xerrors"
-
 	"cdr.dev/slog"
+	"github.com/coder/code-marketplace/api/httpapi"
+	"github.com/coder/code-marketplace/api/httpmw"
 
 	"github.com/coder/code-marketplace/extensionsign"
 )
@@ -70,7 +69,7 @@ func (s *Signature) Manifest(ctx context.Context, publisher, name string, versio
 	return manifest, nil
 }
 
-// Open will intercept requests for signed extensions payload.
+// FileServer will intercept requests for signed extensions payload.
 // It does this by looking for 'SigzipFileExtension' or p7s.sig.
 //
 // The signed payload is completely empty. Nothing it actually signed.
@@ -85,18 +84,26 @@ func (s *Signature) Manifest(ctx context.Context, publisher, name string, versio
 //     for linux users.
 //     For windows users, the signature must be valid, and this implementation
 //     will not work.
-func (s *Signature) Open(ctx context.Context, fp string) (fs.File, error) {
-	if s.SigningEnabled() && strings.HasSuffix(filepath.Base(fp), SigzipFileExtension) {
-		// hijack this request, return an empty signature payload
-		signed, err := extensionsign.IncludeEmptySignature()
-		if err != nil {
-			return nil, xerrors.Errorf("sign and zip manifest: %w", err)
-		}
+func (s *Signature) FileServer() http.Handler {
+	return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
+		if s.SigningEnabled() && strings.HasSuffix(r.URL.Path, SigzipFileExtension) {
+			// hijack this request, return an empty signature payload
+			signed, err := extensionsign.IncludeEmptySignature()
+			if err != nil {
+				httpapi.Write(rw, http.StatusInternalServerError, httpapi.ErrorResponse{
+					Message:   "Unable to generate empty signature for extension",
+					Detail:    err.Error(),
+					RequestID: httpmw.RequestID(r),
+				})
+				return
+			}
 
-		f := mem.NewFileHandle(mem.CreateFile(fp))
-		_, err = f.Write(signed)
-		return f, err
-	}
+			rw.Header().Set("Content-Length", strconv.FormatInt(int64(len(signed)), 10))
+			rw.WriteHeader(http.StatusOK)
+			_, _ = rw.Write(signed)
+			return
+		}
 
-	return s.Storage.Open(ctx, fp)
+		s.Storage.FileServer().ServeHTTP(rw, r)
+	})
 }

storage/storage.go

@@ -6,7 +6,6 @@ import (
 	"encoding/xml"
 	"fmt"
 	"io"
-	"io/fs"
 	"net/http"
 	"os"
 	"regexp"
@@ -211,10 +210,9 @@ type Storage interface {
 	// for verification purposes. Extra files can be included, but not required.
 	// All extra files will be placed relative to the manifest outside the vsix.
 	AddExtension(ctx context.Context, manifest *VSIXManifest, vsix []byte, extra ...File) (string, error)
-	// Open mirrors the fs.FS interface of Open, except with a context.
-	// The Open should return files from the extension storage, and used for
-	// serving extensions.
-	Open(ctx context.Context, name string) (fs.File, error)
+	// FileServer provides a handler for fetching extension repository files from
+	// a client.
+	FileServer() http.Handler
 	// Manifest returns the manifest bytes for the provided extension.  The
 	// extension asset itself (the VSIX) will be included on the manifest even if
 	// it does not exist on the manifest on disk.
@@ -237,17 +235,6 @@ type Storage interface {
 	WalkExtensions(ctx context.Context, fn func(manifest *VSIXManifest, versions []Version) error) error
 }
 
-// HTTPFileServer creates an http.Handler that serves files from the provided
-// storage.
-func HTTPFileServer(s Storage) http.Handler {
-	return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
-		http.FileServerFS(&contextFs{
-			ctx:  r.Context(),
-			open: s.Open,
-		}).ServeHTTP(rw, r)
-	})
-}
-
 type File struct {
 	RelativePath string
 	Content      []byte
@@ -442,12 +429,3 @@ func ParseExtensionID(id string) (string, string, string, error) {
 	}
 	return match[0][1], match[0][2], match[0][3], nil
 }
-
-type contextFs struct {
-	ctx  context.Context
-	open func(ctx context.Context, name string) (fs.File, error)
-}
-
-func (c *contextFs) Open(name string) (fs.File, error) {
-	return c.open(c.ctx, name)
-}

storage/storage_test.go

@@ -201,7 +201,7 @@ func testFileServer(t *testing.T, factory storageFactory) {
 			req := httptest.NewRequest("GET", test.path, nil)
 			rec := httptest.NewRecorder()
 
-			server := storage.HTTPFileServer(f.storage)
+			server := f.storage.FileServer()
 			server.ServeHTTP(rec, req)
 
 			resp := rec.Result()

testutil/mockstorage.go

@@ -3,14 +3,10 @@ package testutil
 import (
 	"context"
 	"errors"
-	"io/fs"
 	"net/http"
 	"os"
-	"path/filepath"
 	"sort"
 
-	"github.com/spf13/afero/mem"
-
 	"github.com/coder/code-marketplace/storage"
 )
 
@@ -26,15 +22,6 @@ func NewMockStorage() *MockStorage {
 func (s *MockStorage) AddExtension(ctx context.Context, manifest *storage.VSIXManifest, vsix []byte, extra ...storage.File) (string, error) {
 	return "", errors.New("not implemented")
 }
-func (s *MockStorage) Open(ctx context.Context, path string) (fs.File, error) {
-	if filepath.Base(path) == "nonexistent" {
-		return nil, fs.ErrNotExist
-	}
-
-	f := mem.NewFileHandle(mem.CreateFile(path))
-	_, _ = f.Write([]byte("foobar"))
-	return f, nil
-}
 
 func (s *MockStorage) FileServer() http.Handler {
 	return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {