From 04001f7ad3422570ec0ef185ac787b0e0b022785 Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Mon, 28 Jun 2021 19:40:04 +0200 Subject: [PATCH] FTP: try to return more specific error codes/messages for some errors We now return 552 code for quota exceeded errors and 553 in the following cases: - filename denied by a filter - no upload permission - no overwrite permission - pre upload hook error Fixes #442 --- common/connection.go | 23 ++++++++++++++++++++++- common/connection_test.go | 4 +++- common/transfer.go | 2 +- ftpd/ftpd_test.go | 5 +++-- ftpd/handler.go | 15 ++++++++------- ftpd/transfer.go | 2 +- go.mod | 2 +- go.sum | 7 ++----- 8 files changed, 41 insertions(+), 19 deletions(-) diff --git a/common/connection.go b/common/connection.go index 6fa02416..69c68e63 100644 --- a/common/connection.go +++ b/common/connection.go @@ -10,6 +10,7 @@ import ( "sync/atomic" "time" + ftpserver "github.com/fclairamb/ftpserverlib" "github.com/pkg/sftp" "github.com/drakkan/sftpgo/v2/dataprovider" @@ -796,7 +797,7 @@ func (c *BaseConnection) GetMaxWriteSize(quotaResult vfs.QuotaCheckResult, isRes return 0, c.GetOpUnsupportedError() } if c.User.Filters.MaxUploadFileSize > 0 && c.User.Filters.MaxUploadFileSize <= fileSize { - return 0, ErrQuotaExceeded + return 0, c.GetQuotaExceededError() } if c.User.Filters.MaxUploadFileSize > 0 { maxUploadSize := c.User.Filters.MaxUploadFileSize - fileSize @@ -1058,6 +1059,26 @@ func (c *BaseConnection) GetOpUnsupportedError() error { } } +// GetQuotaExceededError returns an appropriate storage limit exceeded error for the connection protocol +func (c *BaseConnection) GetQuotaExceededError() error { + switch c.protocol { + case ProtocolFTP: + return ftpserver.ErrStorageExceeded + default: + return ErrQuotaExceeded + } +} + +// IsQuotaExceededError returns true if the given error is a quota exceeded error +func (c *BaseConnection) IsQuotaExceededError(err error) bool { + switch c.protocol { + case ProtocolFTP: + return errors.Is(err, ftpserver.ErrStorageExceeded) + default: + return errors.Is(err, ErrQuotaExceeded) + } +} + // GetGenericError returns an appropriate generic error for the connection protocol func (c *BaseConnection) GetGenericError(err error) error { switch c.protocol { diff --git a/common/connection_test.go b/common/connection_test.go index 85a1ae10..733ef04e 100644 --- a/common/connection_test.go +++ b/common/connection_test.go @@ -259,6 +259,8 @@ func TestErrorsMapping(t *testing.T) { } else { assert.EqualError(t, err, vfs.ErrStorageSizeUnavailable.Error()) } + err = conn.GetQuotaExceededError() + assert.True(t, conn.IsQuotaExceededError(err)) err = conn.GetFsError(fs, nil) assert.NoError(t, err) err = conn.GetOpUnsupportedError() @@ -307,7 +309,7 @@ func TestMaxWriteSize(t *testing.T) { quotaResult.QuotaSize = 0 quotaResult.UsedSize = 0 size, err = conn.GetMaxWriteSize(quotaResult, true, 100, fs.IsUploadResumeSupported()) - assert.EqualError(t, err, ErrQuotaExceeded.Error()) + assert.True(t, conn.IsQuotaExceededError(err)) assert.Equal(t, int64(0), size) size, err = conn.GetMaxWriteSize(quotaResult, true, 10, fs.IsUploadResumeSupported()) diff --git a/common/transfer.go b/common/transfer.go index 3d3db81a..74c64ab4 100644 --- a/common/transfer.go +++ b/common/transfer.go @@ -207,7 +207,7 @@ func (t *BaseTransfer) Close() error { numFiles = 1 } metrics.TransferCompleted(atomic.LoadInt64(&t.BytesSent), atomic.LoadInt64(&t.BytesReceived), t.transferType, t.ErrTransfer) - if t.ErrTransfer == ErrQuotaExceeded && t.File != nil { + if t.File != nil && t.Connection.IsQuotaExceededError(t.ErrTransfer) { // if quota is exceeded we try to remove the partial file for uploads to local filesystem err = t.Fs.Remove(t.File.Name(), false) if err == nil { diff --git a/ftpd/ftpd_test.go b/ftpd/ftpd_test.go index 08a6a4ff..55c671e3 100644 --- a/ftpd/ftpd_test.go +++ b/ftpd/ftpd_test.go @@ -19,6 +19,7 @@ import ( "testing" "time" + ftpserver "github.com/fclairamb/ftpserverlib" "github.com/jlaffaye/ftp" "github.com/rs/zerolog" "github.com/stretchr/testify/assert" @@ -819,11 +820,11 @@ func TestPreUploadHook(t *testing.T) { assert.NoError(t, err) err = ftpUploadFile(testFilePath, testFileName, testFileSize, client, 0) if assert.Error(t, err) { - assert.Contains(t, err.Error(), "permission denied") + assert.Contains(t, err.Error(), ftpserver.ErrFileNameNotAllowed.Error()) } err = ftpUploadFile(testFilePath, testFileName+"1", testFileSize, client, 0) if assert.Error(t, err) { - assert.Contains(t, err.Error(), "permission denied") + assert.Contains(t, err.Error(), ftpserver.ErrFileNameNotAllowed.Error()) } err := client.Quit() assert.NoError(t, err) diff --git a/ftpd/handler.go b/ftpd/handler.go index dc8ca3e7..7af7f9d3 100644 --- a/ftpd/handler.go +++ b/ftpd/handler.go @@ -2,6 +2,7 @@ package ftpd import ( "errors" + "fmt" "io" "os" "path" @@ -318,7 +319,7 @@ func (c *Connection) downloadFile(fs vfs.Fs, fsPath, ftpPath string, offset int6 func (c *Connection) uploadFile(fs vfs.Fs, fsPath, ftpPath string, flags int) (ftpserver.FileTransfer, error) { if !c.User.IsFileAllowed(ftpPath) { c.Log(logger.LevelWarn, "writing file %#v is not allowed", ftpPath) - return nil, c.GetPermissionDeniedError() + return nil, ftpserver.ErrFileNameNotAllowed } filePath := fsPath @@ -329,7 +330,7 @@ func (c *Connection) uploadFile(fs vfs.Fs, fsPath, ftpPath string, flags int) (f stat, statErr := fs.Lstat(fsPath) if (statErr == nil && stat.Mode()&os.ModeSymlink != 0) || fs.IsNotExist(statErr) { if !c.User.HasPerm(dataprovider.PermUpload, path.Dir(ftpPath)) { - return nil, c.GetPermissionDeniedError() + return nil, fmt.Errorf("%w, no upload permission", ftpserver.ErrFileNameNotAllowed) } return c.handleFTPUploadToNewFile(fs, fsPath, filePath, ftpPath) } @@ -346,7 +347,7 @@ func (c *Connection) uploadFile(fs vfs.Fs, fsPath, ftpPath string, flags int) (f } if !c.User.HasPerm(dataprovider.PermOverwrite, path.Dir(ftpPath)) { - return nil, c.GetPermissionDeniedError() + return nil, fmt.Errorf("%w, no overwrite permission", ftpserver.ErrFileNameNotAllowed) } return c.handleFTPUploadToExistingFile(fs, flags, fsPath, filePath, stat.Size(), ftpPath) @@ -356,11 +357,11 @@ func (c *Connection) handleFTPUploadToNewFile(fs vfs.Fs, resolvedPath, filePath, quotaResult := c.HasSpace(true, false, requestPath) if !quotaResult.HasSpace { c.Log(logger.LevelInfo, "denying file write due to quota limits") - return nil, common.ErrQuotaExceeded + return nil, ftpserver.ErrStorageExceeded } if err := common.ExecutePreAction(&c.User, common.OperationPreUpload, resolvedPath, requestPath, c.GetProtocol(), 0, 0); err != nil { c.Log(logger.LevelDebug, "upload for file %#v denied by pre action: %v", requestPath, err) - return nil, c.GetPermissionDeniedError() + return nil, fmt.Errorf("%w, denied by pre-upload action", ftpserver.ErrFileNameNotAllowed) } file, w, cancelFn, err := fs.Create(filePath, 0) if err != nil { @@ -386,7 +387,7 @@ func (c *Connection) handleFTPUploadToExistingFile(fs vfs.Fs, flags int, resolve quotaResult := c.HasSpace(false, false, requestPath) if !quotaResult.HasSpace { c.Log(logger.LevelInfo, "denying file write due to quota limits") - return nil, common.ErrQuotaExceeded + return nil, ftpserver.ErrStorageExceeded } minWriteOffset := int64(0) // ftpserverlib sets: @@ -404,7 +405,7 @@ func (c *Connection) handleFTPUploadToExistingFile(fs vfs.Fs, flags int, resolve } if err := common.ExecutePreAction(&c.User, common.OperationPreUpload, resolvedPath, requestPath, c.GetProtocol(), fileSize, flags); err != nil { c.Log(logger.LevelDebug, "upload for file %#v denied by pre action: %v", requestPath, err) - return nil, c.GetPermissionDeniedError() + return nil, fmt.Errorf("%w, denied by pre-upload action", ftpserver.ErrFileNameNotAllowed) } if common.Config.IsAtomicUploadEnabled() && fs.IsAtomicUploadSupported() { diff --git a/ftpd/transfer.go b/ftpd/transfer.go index 26fb53fc..1a9e5aa9 100644 --- a/ftpd/transfer.go +++ b/ftpd/transfer.go @@ -65,7 +65,7 @@ func (t *transfer) Write(p []byte) (n int, err error) { atomic.AddInt64(&t.BytesReceived, int64(n)) if t.MaxWriteSize > 0 && err == nil && atomic.LoadInt64(&t.BytesReceived) > t.MaxWriteSize { - err = common.ErrQuotaExceeded + err = t.Connection.GetQuotaExceededError() } if err != nil { t.TransferError(err) diff --git a/go.mod b/go.mod index d7d79caf..94e9be3e 100644 --- a/go.mod +++ b/go.mod @@ -11,7 +11,7 @@ require ( github.com/aws/aws-sdk-go v1.38.68 github.com/cockroachdb/cockroach-go/v2 v2.1.1 github.com/eikenb/pipeat v0.0.0-20210603033007-44fc3ffce52b - github.com/fclairamb/ftpserverlib v0.13.3-0.20210614220040-27dccea41813 + github.com/fclairamb/ftpserverlib v0.13.3-0.20210627202858-3bf551007432 github.com/frankban/quicktest v1.13.0 // indirect github.com/go-chi/chi/v5 v5.0.3 github.com/go-chi/jwtauth/v5 v5.0.1 diff --git a/go.sum b/go.sum index 2013335b..3288723e 100644 --- a/go.sum +++ b/go.sum @@ -233,8 +233,8 @@ github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7 github.com/fatih/color v1.7.0 h1:DkWD4oS2D8LGGgTQ6IvwJJXSL5Vp2ffcQg58nFV38Ys= github.com/fatih/color v1.7.0/go.mod h1:Zm6kSWBoL9eyXnKyktHP6abPY2pDugNf5KwzbycvMj4= github.com/fatih/structs v1.1.0/go.mod h1:9NiDSp5zOcgEDl+j00MP/WkGVPOlPRLejGD8Ga6PJ7M= -github.com/fclairamb/ftpserverlib v0.13.3-0.20210614220040-27dccea41813 h1:wXDgbyHQT62eyVff37ocEXSp0N5RiTLdoOmugP/tRRU= -github.com/fclairamb/ftpserverlib v0.13.3-0.20210614220040-27dccea41813/go.mod h1:T7GFWYWSREftxINf3ielPlaQ+aiE04/elEH0zLwkxBA= +github.com/fclairamb/ftpserverlib v0.13.3-0.20210627202858-3bf551007432 h1:UtF6df4idbDpc/x9lMyJT/fLmu6YLRZG6nFyz7qi81Y= +github.com/fclairamb/ftpserverlib v0.13.3-0.20210627202858-3bf551007432/go.mod h1:ATLgn4bHgiM9+vfZbK+rMu/dqgkxO5nk94x/9f8ffDI= github.com/form3tech-oss/jwt-go v3.2.2+incompatible h1:TcekIExNqud5crz4xD2pavyTgWiPvpYe4Xau31I0PRk= github.com/form3tech-oss/jwt-go v3.2.2+incompatible/go.mod h1:pbq4aXjuKjdthFRnoDwaVPLA+WlJuPGy+QneDUgJi2k= github.com/fortytw2/leaktest v1.3.0/go.mod h1:jDsjWgpAGjm2CA7WthBh/CdZYEPF31XHquHwclZch5g= @@ -294,8 +294,6 @@ github.com/gobwas/httphead v0.0.0-20180130184737-2c6c146eadee/go.mod h1:L0fX3K22 github.com/gobwas/pool v0.2.0/go.mod h1:q8bcK0KcYlCgd9e7WYLm9LpyS+YeLd8JVDW6WezmKEw= github.com/gobwas/ws v1.0.2/go.mod h1:szmBTxLgaFppYjEmNtny/v3w89xOydFnnZMcgRRu/EM= github.com/goccy/go-json v0.4.8/go.mod h1:6MelG93GURQebXPDq3khkgXZkazVtN9CRI+MGFi0w8I= -github.com/goccy/go-json v0.7.1 h1:VMhnh5gcc8De8f6m2DLvSqY1x8Jwl3btet+EqMP0QNs= -github.com/goccy/go-json v0.7.1/go.mod h1:6MelG93GURQebXPDq3khkgXZkazVtN9CRI+MGFi0w8I= github.com/goccy/go-json v0.7.2 h1:MY1gMmtCxRpaI8YGpeHCvXUb+FVIo09pnjqF9Rhh274= github.com/goccy/go-json v0.7.2/go.mod h1:6MelG93GURQebXPDq3khkgXZkazVtN9CRI+MGFi0w8I= github.com/godbus/dbus v0.0.0-20190422162347-ade71ed3457e/go.mod h1:bBOAhwG1umN6/6ZUMtDFBMQR8jRg9O75tm9K00oMsK4= @@ -843,7 +841,6 @@ github.com/streadway/amqp v0.0.0-20190827072141-edfb9018d271/go.mod h1:AZpEONHx3 github.com/streadway/handy v0.0.0-20190108123426-d5acb3125c2a/go.mod h1:qNTQ5P5JnDBl6z3cMAg/SywNDC5ABu5ApDIw6lUbRmI= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= -github.com/stretchr/objx v0.2.0 h1:Hbg2NidpLE8veEBkEZTL3CvlkUIVzuU9jDplZO54c48= github.com/stretchr/objx v0.2.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE= github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=