From 1743dd67558963b22cf81a92d110dbed565232ff Mon Sep 17 00:00:00 2001 From: Sainyam Kapoor Date: Fri, 13 Nov 2026 18:37:34 +0000 Subject: [PATCH] fix(storage): Allow filesystem reads without write access --- packages/shared/pkg/storage/storage_fs.go | 33 +++++---- .../shared/pkg/storage/storage_fs_test.go | 73 +++++++++++++++++++ 2 files changed, 90 insertions(+), 16 deletions(-) diff --git a/packages/shared/pkg/storage/storage_fs.go b/packages/shared/pkg/storage/storage_fs.go index 4fc101ef06..a0bbdcc489 100644 --- a/packages/shared/pkg/storage/storage_fs.go +++ b/packages/shared/pkg/storage/storage_fs.go @@ -126,7 +126,7 @@ func (o *fsObject) WriteTo(ctx context.Context, dst io.Writer) (n int64, err err start := time.Now() defer func() { RecordReadBlob(ctx, time.Since(start), n, o.path, SourceFS, err) }() - handle, err := o.getHandle(true) + handle, err := o.getHandle() if err != nil { return 0, err } @@ -225,7 +225,7 @@ func (o *fsObject) storeFileCompressed(ctx context.Context, localPath string, cf } func (o *fsObject) openRangeReader(_ context.Context, off, length int64) (RangeReader, error) { - f, err := o.getHandle(true) + f, err := o.getHandle() if err != nil { return nil, err } @@ -246,7 +246,7 @@ func (o *fsObject) Size(ctx context.Context) (_ int64, err error) { start := time.Now() defer func() { RecordReadSize(ctx, time.Since(start), o.objType, SourceFS, err) }() - handle, err := o.getHandle(true) + handle, err := o.getHandle() if err != nil { return 0, err } @@ -293,26 +293,27 @@ func ValidateUploadToken(key []byte, path string, expires int64, token string) b return hmac.Equal([]byte(expected), []byte(token)) } -func (o *fsObject) getHandle(checkExistence bool) (*os.File, error) { - if checkExistence { - info, err := os.Stat(o.path) - if err != nil { - if os.IsNotExist(err) { - return nil, ErrObjectNotExist - } - - return nil, err +func (o *fsObject) getHandle() (*os.File, error) { + handle, err := os.Open(o.path) + if err != nil { + if os.IsNotExist(err) { + return nil, ErrObjectNotExist } - if info.IsDir() { - return nil, fmt.Errorf("path %s is a directory", o.path) - } + return nil, err } - handle, err := os.OpenFile(o.path, os.O_RDWR|os.O_CREATE, 0o644) + info, err := handle.Stat() if err != nil { + handle.Close() + return nil, err } + if info.IsDir() { + handle.Close() + + return nil, fmt.Errorf("path %s is a directory", o.path) + } return handle, nil } diff --git a/packages/shared/pkg/storage/storage_fs_test.go b/packages/shared/pkg/storage/storage_fs_test.go index 3bce8a5948..b9750d259e 100644 --- a/packages/shared/pkg/storage/storage_fs_test.go +++ b/packages/shared/pkg/storage/storage_fs_test.go @@ -1,8 +1,10 @@ package storage import ( + "bytes" "context" "errors" + "io" "os" "path/filepath" "testing" @@ -434,3 +436,74 @@ func TestWriteToNonExistentObject(t *testing.T) { _, err = GetBlob(t.Context(), obj) require.ErrorIs(t, err, ErrObjectNotExist) } + +func TestFSReadOnlyObject(t *testing.T) { + t.Parallel() + if os.Geteuid() == 0 { + t.Skip("root bypasses file permissions") + } + + p := newTempProvider(t) + path := filepath.Join(p.basePath, "object") + const contents = "hello world" + require.NoError(t, os.WriteFile(path, []byte(contents), 0o444)) + require.NoError(t, os.Chmod(p.basePath, 0o555)) + t.Cleanup(func() { + require.NoError(t, os.Chmod(p.basePath, 0o755)) + }) + + t.Run("WriteTo", func(t *testing.T) { + obj, err := p.OpenBlob(t.Context(), "object") + require.NoError(t, err) + var dst bytes.Buffer + n, err := obj.WriteTo(t.Context(), &dst) + require.NoError(t, err) + assert.EqualValues(t, len(contents), n) + assert.Equal(t, contents, dst.String()) + }) + t.Run("Size", func(t *testing.T) { + obj, err := p.OpenSeekable(t.Context(), "object") + require.NoError(t, err) + size, err := obj.Size(t.Context()) + require.NoError(t, err) + assert.EqualValues(t, len(contents), size) + }) + t.Run("OpenRangeReader", func(t *testing.T) { + obj, err := p.OpenSeekable(t.Context(), "object") + require.NoError(t, err) + reader, _, err := obj.OpenRangeReader(t.Context(), 6, 5, nil) + require.NoError(t, err) + defer reader.Close(t.Context()) + data, err := io.ReadAll(reader) + require.NoError(t, err) + assert.Equal(t, "world", string(data)) + }) +} + +func TestFSReadMissingObject(t *testing.T) { + t.Parallel() + p := newTempProvider(t) + obj, err := p.OpenSeekable(t.Context(), "missing") + require.NoError(t, err) + + _, err = obj.Size(t.Context()) + require.ErrorIs(t, err, ErrObjectNotExist) + reader, _, err := obj.OpenRangeReader(t.Context(), 0, 1, nil) + require.ErrorIs(t, err, ErrObjectNotExist) + assert.Nil(t, reader) + assert.NoFileExists(t, filepath.Join(p.basePath, "missing")) +} + +func TestFSReadDirectory(t *testing.T) { + t.Parallel() + p := newTempProvider(t) + require.NoError(t, os.Mkdir(filepath.Join(p.basePath, "directory"), 0o755)) + obj, err := p.OpenSeekable(t.Context(), "directory") + require.NoError(t, err) + + _, err = obj.Size(t.Context()) + require.ErrorContains(t, err, "is a directory") + reader, _, err := obj.OpenRangeReader(t.Context(), 0, 1, nil) + require.ErrorContains(t, err, "is a directory") + assert.Nil(t, reader) +}