Skip to content

Add RootStore #1282

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Add RootStore #1282

wants to merge 8 commits into from

Conversation

cugu
Copy link

@cugu cugu commented Jun 28, 2025

Add a new RootStore DataStore that uses the os.Root type introduced in Go 1.24.

This type ensures that all files accessed are only in the directory passed and protects against path traversal attacks.

Read more abount os.Root here: https://go.dev/blog/osroot

@Acconut
Copy link
Member

Acconut commented Jun 30, 2025

Thank you for this PR! The usage of os.Root looks interesting and could definitely be helpful in some cases. However, I think that it would be better to offer this as an option in the existing filestore instead of duplicating filestore. This would avoid us having to maintain both storages, even though the appear to be practically the same.

@cugu
Copy link
Author

cugu commented Jun 30, 2025

Both stores are incompatible and merging them into one would result in breaking changes, as the filestore allows for absolute path, which the rootstore by design cannot support.

@Acconut
Copy link
Member

Acconut commented Jul 1, 2025

Would it be possible to offer an option in filestore to enable the use of os.Root? Then we can avoid breaking changes and duplicating the storage.

@cugu
Copy link
Author

cugu commented Jul 1, 2025

I don't think this is possible without breaking changes.

The current interface of FileStore is FileStore{Path string} and we need some other field for the FileStore, e.g. FileStore{Path string, FS FS}, which would break initialization without keys like FileStore{"my/path"}

@cugu
Copy link
Author

cugu commented Jul 1, 2025

We probably could share quite a bit of code though between both implementations.

@Acconut
Copy link
Member

Acconut commented Jul 1, 2025

The current interface of FileStore is FileStore{Path string} and we need some other field for the FileStore, e.g. FileStore{Path string, FS FS}, which would break initialization without keys like FileStore{"my/path"}

Good point. In that case, this might be something for the next major release, where we can introduce such changes. I'm quite hesitant to merge the PR as is because it duplicates a lot of code.

@cugu
Copy link
Author

cugu commented Jul 1, 2025

Okay added another way: A baseStore that is shared between FileStore and RootStore which are both just small wrappers. This offers reduced code duplication and backwards compatibility.

Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the updates. This seems to be going in a good direction.

"os"
)

type osFS struct{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can osFS be replaced by os.DirFS (https://pkg.go.dev/os#DirFS)? Then we don't have to implement the interface on our own.

Copy link
Author

@cugu cugu Jul 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. os.DirFS returns a read-only fs.FS, we need writing functions like Create or Remove.


// RootStore is a file based storage backend that uses the
// os.Root type to safely store uploads.
type RootStore struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we actually have to export a RootStore. If we expose a filestore that can work with any io/fs.FS, then users just supply the corresponding file system interface for their root and use then. Makes it more flexible and less generalized.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something along these lines, which is similar to what you have proposed in this PR already:

struct FSStore {
  FS fs.FS
}

// Implementation of FSStore function omitted.

struct FileStore {
  Dir string
}

func (store FileStore) fsStore() FSStore {
  return FSStore{os.DirFS(store.Dir)}
}

func (store FileStore) NewUpload(ctx context.Context, info handler.FileInfo) (handler.Upload, error) {
	return store.fsStore().NewUpload(ctx, info)
}

@cugu
Copy link
Author

cugu commented Jul 4, 2025

Just noticed another minor (?) breaking change: Changing the the FileStore.Path during runtime currently works, while it will not work anymore with any of the changes suggested above.

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.

2 participants