From 8c75da27fc84272b2e14330b75399000379eb3c4 Mon Sep 17 00:00:00 2001 From: Charlotte Brandhorst-Satzkorn <46385858+catzkorn@users.noreply.github.com> Date: Fri, 5 Apr 2024 11:43:13 -0700 Subject: [PATCH] drive: move normalizeShareName into pkg drive and make func public (#11638) This change makes the normalizeShareName function public, so it can be used for validation in control. Updates tailscale/corp#16827 Signed-off-by: Charlotte Brandhorst-Satzkorn --- drive/remote.go | 27 ++++++++++++- .../drive_test.go => drive/remote_test.go | 4 +- ipn/ipnlocal/drive.go | 39 ++++--------------- ipn/ipnlocal/local_test.go | 10 ++--- ipn/localapi/localapi.go | 4 +- 5 files changed, 42 insertions(+), 42 deletions(-) rename ipn/ipnlocal/drive_test.go => drive/remote_test.go (92%) diff --git a/drive/remote.go b/drive/remote.go index 05237d0ce..b41bd054a 100644 --- a/drive/remote.go +++ b/drive/remote.go @@ -7,14 +7,22 @@ package drive import ( "bytes" + "errors" "net/http" + "regexp" "strings" ) var ( // DisallowShareAs forcibly disables sharing as a specific user, only used // for testing. - DisallowShareAs = false + DisallowShareAs = false + ErrDriveNotEnabled = errors.New("Taildrive not enabled") + ErrInvalidShareName = errors.New("Share names may only contain the letters a-z, underscore _, parentheses (), or spaces") +) + +var ( + shareNameRegex = regexp.MustCompile(`^[a-z0-9_\(\) ]+$`) ) // AllowShareAs reports whether sharing files as a specific user is allowed. @@ -103,3 +111,20 @@ type FileSystemForRemote interface { // Close() stops serving the WebDAV content Close() error } + +// NormalizeShareName normalizes the given share name and returns an error if +// it contains any disallowed characters. +func NormalizeShareName(name string) (string, error) { + // Force all share names to lowercase to avoid potential incompatibilities + // with clients that don't support case-sensitive filenames. + name = strings.ToLower(name) + + // Trim whitespace + name = strings.TrimSpace(name) + + if !shareNameRegex.MatchString(name) { + return "", ErrInvalidShareName + } + + return name, nil +} diff --git a/ipn/ipnlocal/drive_test.go b/drive/remote_test.go similarity index 92% rename from ipn/ipnlocal/drive_test.go rename to drive/remote_test.go index dade9583f..e05b23839 100644 --- a/ipn/ipnlocal/drive_test.go +++ b/drive/remote_test.go @@ -1,7 +1,7 @@ // Copyright (c) Tailscale Inc & AUTHORS // SPDX-License-Identifier: BSD-3-Clause -package ipnlocal +package drive import ( "fmt" @@ -29,7 +29,7 @@ func TestNormalizeShareName(t *testing.T) { } for _, tt := range tests { t.Run(fmt.Sprintf("name %q", tt.name), func(t *testing.T) { - got, err := normalizeShareName(tt.name) + got, err := NormalizeShareName(tt.name) if tt.err != nil && err != tt.err { t.Errorf("wanted error %v, got %v", tt.err, err) } else if got != tt.want { diff --git a/ipn/ipnlocal/drive.go b/ipn/ipnlocal/drive.go index 7b98c135e..ef2e6a7cc 100644 --- a/ipn/ipnlocal/drive.go +++ b/ipn/ipnlocal/drive.go @@ -4,10 +4,8 @@ package ipnlocal import ( - "errors" "fmt" "os" - "regexp" "slices" "strings" @@ -24,12 +22,6 @@ const ( DriveLocalPort = 8080 ) -var ( - shareNameRegex = regexp.MustCompile(`^[a-z0-9_\(\) ]+$`) - ErrDriveNotEnabled = errors.New("Taildrive not enabled") - ErrInvalidShareName = errors.New("Share names may only contain the letters a-z, underscore _, parentheses (), or spaces") -) - // DriveSharingEnabled reports whether sharing to remote nodes via Taildrive is // enabled. This is currently based on checking for the drive:share node // attribute. @@ -62,7 +54,7 @@ func (b *LocalBackend) driveAccessEnabledLocked() bool { func (b *LocalBackend) DriveSetServerAddr(addr string) error { fs, ok := b.sys.DriveForRemote.GetOK() if !ok { - return ErrDriveNotEnabled + return drive.ErrDriveNotEnabled } fs.SetFileServerAddr(addr) @@ -75,7 +67,7 @@ func (b *LocalBackend) DriveSetServerAddr(addr string) error { // limited to alphanumeric characters and the underscore _. func (b *LocalBackend) DriveSetShare(share *drive.Share) error { var err error - share.Name, err = normalizeShareName(share.Name) + share.Name, err = drive.NormalizeShareName(share.Name) if err != nil { return err } @@ -91,29 +83,12 @@ func (b *LocalBackend) DriveSetShare(share *drive.Share) error { return nil } -// normalizeShareName normalizes the given share name and returns an error if -// it contains any disallowed characters. -func normalizeShareName(name string) (string, error) { - // Force all share names to lowercase to avoid potential incompatibilities - // with clients that don't support case-sensitive filenames. - name = strings.ToLower(name) - - // Trim whitespace - name = strings.TrimSpace(name) - - if !shareNameRegex.MatchString(name) { - return "", ErrInvalidShareName - } - - return name, nil -} - func (b *LocalBackend) driveSetShareLocked(share *drive.Share) (views.SliceView[*drive.Share, drive.ShareView], error) { existingShares := b.pm.prefs.DriveShares() fs, ok := b.sys.DriveForRemote.GetOK() if !ok { - return existingShares, ErrDriveNotEnabled + return existingShares, drive.ErrDriveNotEnabled } addedShare := false @@ -151,7 +126,7 @@ func (b *LocalBackend) driveSetShareLocked(share *drive.Share) (views.SliceView[ // - share already exists under new name func (b *LocalBackend) DriveRenameShare(oldName, newName string) error { var err error - newName, err = normalizeShareName(newName) + newName, err = drive.NormalizeShareName(newName) if err != nil { return err } @@ -172,7 +147,7 @@ func (b *LocalBackend) driveRenameShareLocked(oldName, newName string) (views.Sl fs, ok := b.sys.DriveForRemote.GetOK() if !ok { - return existingShares, ErrDriveNotEnabled + return existingShares, drive.ErrDriveNotEnabled } found := false @@ -212,7 +187,7 @@ func (b *LocalBackend) DriveRemoveShare(name string) error { // Force all share names to lowercase to avoid potential incompatibilities // with clients that don't support case-sensitive filenames. var err error - name, err = normalizeShareName(name) + name, err = drive.NormalizeShareName(name) if err != nil { return err } @@ -233,7 +208,7 @@ func (b *LocalBackend) driveRemoveShareLocked(name string) (views.SliceView[*dri fs, ok := b.sys.DriveForRemote.GetOK() if !ok { - return existingShares, ErrDriveNotEnabled + return existingShares, drive.ErrDriveNotEnabled } found := false diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index 387604333..aad62bad9 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -2404,13 +2404,13 @@ func TestDriveManageShares(t *testing.T) { { name: "add_bad_name", add: &drive.Share{Name: "$"}, - expect: ErrInvalidShareName, + expect: drive.ErrInvalidShareName, }, { name: "add_disabled", disabled: true, add: &drive.Share{Name: "a"}, - expect: ErrDriveNotEnabled, + expect: drive.ErrDriveNotEnabled, }, { name: "remove", @@ -2439,7 +2439,7 @@ func TestDriveManageShares(t *testing.T) { name: "remove_disabled", disabled: true, remove: "b", - expect: ErrDriveNotEnabled, + expect: drive.ErrDriveNotEnabled, }, { name: "rename", @@ -2474,13 +2474,13 @@ func TestDriveManageShares(t *testing.T) { { name: "rename_bad_name", rename: [2]string{"a", "$"}, - expect: ErrInvalidShareName, + expect: drive.ErrInvalidShareName, }, { name: "rename_disabled", disabled: true, rename: [2]string{"a", "c"}, - expect: ErrDriveNotEnabled, + expect: drive.ErrDriveNotEnabled, }, } diff --git a/ipn/localapi/localapi.go b/ipn/localapi/localapi.go index a387e3156..2c2cc7126 100644 --- a/ipn/localapi/localapi.go +++ b/ipn/localapi/localapi.go @@ -2792,7 +2792,7 @@ func (h *Handler) serveShares(w http.ResponseWriter, r *http.Request) { } err = h.b.DriveSetShare(&share) if err != nil { - if errors.Is(err, ipnlocal.ErrInvalidShareName) { + if errors.Is(err, drive.ErrInvalidShareName) { http.Error(w, "invalid share name", http.StatusBadRequest) return } @@ -2833,7 +2833,7 @@ func (h *Handler) serveShares(w http.ResponseWriter, r *http.Request) { http.Error(w, "share name already used", http.StatusBadRequest) return } - if errors.Is(err, ipnlocal.ErrInvalidShareName) { + if errors.Is(err, drive.ErrInvalidShareName) { http.Error(w, "invalid share name", http.StatusBadRequest) return }