Skip to content

Commit

Permalink
check name for illegal values during uploads and moves (#1900)
Browse files Browse the repository at this point in the history
  • Loading branch information
David Christofas authored Jul 22, 2021
1 parent 6494cac commit 4475f43
Show file tree
Hide file tree
Showing 11 changed files with 53 additions and 146 deletions.
5 changes: 5 additions & 0 deletions changelog/unreleased/tus-illegal-names.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Enhancement: Check for illegal names while uploading or moving files

The code was not checking for invalid file names during uploads and moves.

https://github.com/cs3org/reva/pull/1900
6 changes: 6 additions & 0 deletions internal/http/services/owncloud/ocdav/copy.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,12 @@ func (s *svc) handlePathCopy(w http.ResponseWriter, r *http.Request, ns string)
w.WriteHeader(http.StatusBadRequest)
return
}
for _, r := range nameRules {
if !r.Test(dst) {
w.WriteHeader(http.StatusBadRequest)
return
}
}
dst = path.Join(ns, dst)

sublog := appctx.GetLogger(ctx).With().Str("src", src).Str("dst", dst).Logger()
Expand Down
7 changes: 6 additions & 1 deletion internal/http/services/owncloud/ocdav/mkcol.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,12 @@ func (s *svc) handlePathMkcol(w http.ResponseWriter, r *http.Request, ns string)
defer span.End()

fn := path.Join(ns, r.URL.Path)

for _, r := range nameRules {
if !r.Test(fn) {
w.WriteHeader(http.StatusBadRequest)
return
}
}
sublog := appctx.GetLogger(ctx).With().Str("path", fn).Logger()

ref := &provider.Reference{Path: fn}
Expand Down
8 changes: 8 additions & 0 deletions internal/http/services/owncloud/ocdav/move.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,14 @@ func (s *svc) handlePathMove(w http.ResponseWriter, r *http.Request, ns string)
w.WriteHeader(http.StatusBadRequest)
return
}

for _, r := range nameRules {
if !r.Test(dstPath) {
w.WriteHeader(http.StatusBadRequest)
return
}
}

dstPath = path.Join(ns, dstPath)

sublog := appctx.GetLogger(ctx).With().Str("src", srcPath).Str("dst", dstPath).Logger()
Expand Down
23 changes: 23 additions & 0 deletions internal/http/services/owncloud/ocdav/ocdav.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,31 @@ const (

var (
errInvalidValue = errors.New("invalid value")

nameRules = [...]nameRule{
nameNotEmpty{},
nameDoesNotContain{chars: "\f\r\n\\"},
}
)

type nameRule interface {
Test(name string) bool
}

type nameNotEmpty struct{}

func (r nameNotEmpty) Test(name string) bool {
return len(strings.TrimSpace(name)) > 0
}

type nameDoesNotContain struct {
chars string
}

func (r nameDoesNotContain) Test(name string) bool {
return !strings.ContainsAny(name, r.chars)
}

func init() {
global.Register("ocdav", New)
}
Expand Down
8 changes: 5 additions & 3 deletions internal/http/services/owncloud/ocdav/tus.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,11 @@ func (s *svc) handlePathTusPost(w http.ResponseWriter, r *http.Request, ns strin

// read filename from metadata
meta := tusd.ParseMetadataHeader(r.Header.Get(HeaderUploadMetadata))
if meta["filename"] == "" {
w.WriteHeader(http.StatusPreconditionFailed)
return
for _, r := range nameRules {
if !r.Test(meta["filename"]) {
w.WriteHeader(http.StatusPreconditionFailed)
return
}
}

// append filename to current dir
Expand Down
19 changes: 0 additions & 19 deletions tests/acceptance/expected-failures-on-OCIS-storage.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,26 +38,10 @@ Basic file management like up and download, move, copy, properties, quota, trash
- [apiVersions/fileVersions.feature:276](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L276)
- [apiVersions/fileVersions.feature:347](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L347)

#### [invalid file-names should not be created using the TUS protocol](https://github.com/owncloud/ocis/issues/1001)
- [apiWebdavUploadTUS/uploadFile.feature:143](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L143)
- [apiWebdavUploadTUS/uploadFile.feature:144](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L144)
- [apiWebdavUploadTUS/uploadFile.feature:146](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L146)
- [apiWebdavUploadTUS/uploadFile.feature:147](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L147)
- [apiWebdavUploadTUS/uploadFile.feature:148](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L148)
- [apiWebdavUploadTUS/uploadFile.feature:150](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L150)

#### [upload a file using TUS resource URL as an other user should not work](https://github.com/owncloud/ocis/issues/1141)
- [apiWebdavUploadTUS/uploadFile.feature:165](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L165)
- [apiWebdavUploadTUS/uploadFile.feature:166](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L166)

#### [renaming to banned name works](https://github.com/owncloud/ocis/issues/1295)
- [apiWebdavMove1/moveFolder.feature:21](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove1/moveFolder.feature#L21)
- [apiWebdavMove1/moveFolder.feature:22](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove1/moveFolder.feature#L22)
- [apiWebdavMove1/moveFolder.feature:34](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove1/moveFolder.feature#L34)
- [apiWebdavMove1/moveFolder.feature:35](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove1/moveFolder.feature#L35)
- [apiWebdavMove1/moveFolder.feature:47](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove1/moveFolder.feature#L47)
- [apiWebdavMove1/moveFolder.feature:48](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove1/moveFolder.feature#L48)

#### [Getting information about a folder overwritten by a file gives 500 error instead of 404](https://github.com/owncloud/ocis/issues/1239)
- [apiWebdavProperties1/copyFile.feature:226](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties1/copyFile.feature#L226)
- [apiWebdavProperties1/copyFile.feature:227](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties1/copyFile.feature#L227)
Expand Down Expand Up @@ -901,9 +885,6 @@ Scenario Outline: Moving a file to a shared folder with no permissions
Scenario Outline: Moving a file to overwrite a file in a shared folder with no permissions
- [apiWebdavMove2/moveFile.feature:213](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove2/moveFile.feature#L213)
- [apiWebdavMove2/moveFile.feature:214](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove2/moveFile.feature#L214)
Scenario Outline: rename a file into an invalid filename
- [apiWebdavMove2/moveFile.feature:234](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove2/moveFile.feature#L234)
- [apiWebdavMove2/moveFile.feature:235](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove2/moveFile.feature#L235)
Scenario Outline: Checking file id after a move between received shares
- [apiWebdavMove2/moveFile.feature:272](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove2/moveFile.feature#L272)
- [apiWebdavMove2/moveFile.feature:273](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove2/moveFile.feature#L273)
Expand Down
17 changes: 0 additions & 17 deletions tests/acceptance/expected-failures-on-OWNCLOUD-storage.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,8 @@ The following scenarios fail on OWNCLOUD storage but not on OCIS storage:
- [apiWebdavUpload1/uploadFile.feature:113](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUpload1/uploadFile.feature#L113)

#### [invalid file-names should not be created using the TUS protocol](https://github.com/owncloud/ocis/issues/1001)
- [apiWebdavUploadTUS/uploadFile.feature:143](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L135)
- [apiWebdavUploadTUS/uploadFile.feature:144](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L136)
- [apiWebdavUploadTUS/uploadFile.feature:145](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L137)
- [apiWebdavUploadTUS/uploadFile.feature:146](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L138)
- [apiWebdavUploadTUS/uploadFile.feature:147](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L139)
- [apiWebdavUploadTUS/uploadFile.feature:148](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L140)
- [apiWebdavUploadTUS/uploadFile.feature:149](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L141)
- [apiWebdavUploadTUS/uploadFile.feature:150](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L142)

### [500 Internal Server Error on Post request for TUS upload](https://github.com/owncloud/ocis/issues/1047)

Expand All @@ -101,14 +95,6 @@ The following scenarios fail on OWNCLOUD storage but not on OCIS storage:
- [apiWebdavUploadTUS/uploadFile.feature:165](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L155)
- [apiWebdavUploadTUS/uploadFile.feature:166](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L156)

#### [renaming to banned name works](https://github.com/owncloud/ocis/issues/1295)
- [apiWebdavMove1/moveFolder.feature:21](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove1/moveFolder.feature#L21)
- [apiWebdavMove1/moveFolder.feature:22](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove1/moveFolder.feature#L22)
- [apiWebdavMove1/moveFolder.feature:34](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove1/moveFolder.feature#L34)
- [apiWebdavMove1/moveFolder.feature:35](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove1/moveFolder.feature#L35)
- [apiWebdavMove1/moveFolder.feature:47](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove1/moveFolder.feature#L47)
- [apiWebdavMove1/moveFolder.feature:48](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove1/moveFolder.feature#L48)

#### [Getting information about a folder overwritten by a file gives 500 error instead of 404](https://github.com/owncloud/ocis/issues/1239)
- [apiWebdavProperties1/copyFile.feature:226](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties1/copyFile.feature#L226)
- [apiWebdavProperties1/copyFile.feature:227](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties1/copyFile.feature#L227)
Expand Down Expand Up @@ -1059,9 +1045,6 @@ Scenario Outline: Moving a file into a shared folder as the sharee and as the sh
Scenario Outline: Moving a file to overwrite a file in a shared folder with no permissions
- [apiWebdavMove2/moveFile.feature:213](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove2/moveFile.feature#L213)
- [apiWebdavMove2/moveFile.feature:214](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove2/moveFile.feature#L214)
Scenario Outline: rename a file into an invalid filename
- [apiWebdavMove2/moveFile.feature:234](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove2/moveFile.feature#L234)
- [apiWebdavMove2/moveFile.feature:235](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove2/moveFile.feature#L235)
Scenario Outline: Checking file id after a move between received shares
- [apiWebdavMove2/moveFile.feature:272](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove2/moveFile.feature#L272)
- [apiWebdavMove2/moveFile.feature:273](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove2/moveFile.feature#L273)
Expand Down
19 changes: 0 additions & 19 deletions tests/acceptance/expected-failures-on-S3NG-storage.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,26 +44,10 @@ Basic file management like up and download, move, copy, properties, quota, trash
- [apiVersions/fileVersions.feature:347](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiVersions/fileVersions.feature#L347)


#### [invalid file-names should not be created using the TUS protocol](https://github.com/owncloud/ocis/issues/1001)
- [apiWebdavUploadTUS/uploadFile.feature:143](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L135)
- [apiWebdavUploadTUS/uploadFile.feature:144](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L136)
- [apiWebdavUploadTUS/uploadFile.feature:146](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L138)
- [apiWebdavUploadTUS/uploadFile.feature:147](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L139)
- [apiWebdavUploadTUS/uploadFile.feature:148](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L140)
- [apiWebdavUploadTUS/uploadFile.feature:150](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L142)

#### [upload a file using TUS resource URL as an other user should not work](https://github.com/owncloud/ocis/issues/1141)
- [apiWebdavUploadTUS/uploadFile.feature:165](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L155)
- [apiWebdavUploadTUS/uploadFile.feature:166](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavUploadTUS/uploadFile.feature#L156)

#### [renaming to banned name works](https://github.com/owncloud/ocis/issues/1295)
- [apiWebdavMove1/moveFolder.feature:21](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove1/moveFolder.feature#L21)
- [apiWebdavMove1/moveFolder.feature:22](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove1/moveFolder.feature#L22)
- [apiWebdavMove1/moveFolder.feature:34](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove1/moveFolder.feature#L34)
- [apiWebdavMove1/moveFolder.feature:35](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove1/moveFolder.feature#L35)
- [apiWebdavMove1/moveFolder.feature:47](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove1/moveFolder.feature#L47)
- [apiWebdavMove1/moveFolder.feature:48](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove1/moveFolder.feature#L48)

#### [Getting information about a folder overwritten by a file gives 500 error instead of 404](https://github.com/owncloud/ocis/issues/1239)
- [apiWebdavProperties1/copyFile.feature:226](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties1/copyFile.feature#L226)
- [apiWebdavProperties1/copyFile.feature:227](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavProperties1/copyFile.feature#L227)
Expand Down Expand Up @@ -890,9 +874,6 @@ Scenario Outline: Moving a file to a shared folder with no permissions
Scenario Outline: Moving a file to overwrite a file in a shared folder with no permissions
- [apiWebdavMove2/moveFile.feature:213](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove2/moveFile.feature#L213)
- [apiWebdavMove2/moveFile.feature:214](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove2/moveFile.feature#L214)
Scenario Outline: rename a file into an invalid filename
- [apiWebdavMove2/moveFile.feature:234](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove2/moveFile.feature#L234)
- [apiWebdavMove2/moveFile.feature:235](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove2/moveFile.feature#L235)
Scenario Outline: Checking file id after a move between received shares
- [apiWebdavMove2/moveFile.feature:272](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove2/moveFile.feature#L272)
- [apiWebdavMove2/moveFile.feature:273](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiWebdavMove2/moveFile.feature#L273)
Expand Down

This file was deleted.

Loading

0 comments on commit 4475f43

Please sign in to comment.