Merge pull request #4740 from butonic/disallow-reserved-names

disallow reserved names

changelog/unreleased/disallow-reserved-names.md

@@ -0,0 +1,5 @@
+Bugfix: Disallow reserved filenames
+
+We now disallow the reserved `..` and `.` filenames. They are only allowed as destinations of move or copy operations.
+
+https://github.com/cs3org/reva/pull/4740

internal/http/services/owncloud/ocdav/copy.go

@@ -88,14 +88,14 @@ func (s *svc) handlePathCopy(w http.ResponseWriter, r *http.Request, ns string)
 		return
 	}
 
-	if err := ValidateName(path.Base(src), s.nameValidators); err != nil {
+	if err := ValidateName(filename(src), s.nameValidators); err != nil {
 		w.WriteHeader(http.StatusBadRequest)
 		b, err := errors.Marshal(http.StatusBadRequest, "source failed naming rules", "")
 		errors.HandleWebdavError(appctx.GetLogger(ctx), w, b, err)
 		return
 	}
 
-	if err := ValidateName(path.Base(dst), s.nameValidators); err != nil {
+	if err := ValidateDestination(filename(dst), s.nameValidators); err != nil {
 		w.WriteHeader(http.StatusBadRequest)
 		b, err := errors.Marshal(http.StatusBadRequest, "destination failed naming rules", "")
 		errors.HandleWebdavError(appctx.GetLogger(ctx), w, b, err)

internal/http/services/owncloud/ocdav/mkcol.go

@@ -39,10 +39,10 @@ func (s *svc) handlePathMkcol(w http.ResponseWriter, r *http.Request, ns string)
 	ctx, span := appctx.GetTracerProvider(r.Context()).Tracer(tracerName).Start(r.Context(), "mkcol")
 	defer span.End()
 
-	fn := path.Join(ns, r.URL.Path)
-	if err := ValidateName(path.Base(fn), s.nameValidators); err != nil {
+	if err := ValidateName(filename(r.URL.Path), s.nameValidators); err != nil {
 		return http.StatusBadRequest, err
 	}
+	fn := path.Join(ns, r.URL.Path)
 	sublog := appctx.GetLogger(ctx).With().Str("path", fn).Logger()
 
 	client, err := s.gatewaySelector.Next()

internal/http/services/owncloud/ocdav/move.go

@@ -60,14 +60,14 @@ func (s *svc) handlePathMove(w http.ResponseWriter, r *http.Request, ns string)
 		return
 	}
 
-	if err := ValidateName(path.Base(srcPath), s.nameValidators); err != nil {
+	if err := ValidateName(filename(srcPath), s.nameValidators); err != nil {
 		w.WriteHeader(http.StatusBadRequest)
 		b, err := errors.Marshal(http.StatusBadRequest, "source failed naming rules", "")
 		errors.HandleWebdavError(appctx.GetLogger(ctx), w, b, err)
 		return
 	}
 
-	if err := ValidateName(path.Base(dstPath), s.nameValidators); err != nil {
+	if err := ValidateDestination(filename(dstPath), s.nameValidators); err != nil {
 		w.WriteHeader(http.StatusBadRequest)
 		b, err := errors.Marshal(http.StatusBadRequest, "destination naming rules", "")
 		errors.HandleWebdavError(appctx.GetLogger(ctx), w, b, err)

internal/http/services/owncloud/ocdav/ocdav.go

@@ -394,3 +394,8 @@ func (s *svc) referenceIsChildOf(ctx context.Context, selector pool.Selectable[g
 	pp := path.Join(parentPathRes.Path, parent.Path) + "/"
 	return strings.HasPrefix(cp, pp), nil
 }
+
+// filename returns the base filename from a path and replaces any slashes with an empty string
+func filename(p string) string {
+	return strings.Trim(path.Base(p), "/")
+}

internal/http/services/owncloud/ocdav/put.go

@@ -113,8 +113,15 @@ func (s *svc) handlePathPut(w http.ResponseWriter, r *http.Request, ns string) {
 	defer span.End()
 
 	fn := path.Join(ns, r.URL.Path)
-
 	sublog := appctx.GetLogger(ctx).With().Str("path", fn).Logger()
+
+	if err := ValidateName(filename(r.URL.Path), s.nameValidators); err != nil {
+		w.WriteHeader(http.StatusBadRequest)
+		b, err := errors.Marshal(http.StatusBadRequest, err.Error(), "")
+		errors.HandleWebdavError(&sublog, w, b, err)
+		return
+	}
+
 	space, status, err := spacelookup.LookUpStorageSpaceForPath(ctx, s.gatewaySelector, fn)
 	if err != nil {
 		sublog.Error().Err(err).Str("path", fn).Msg("failed to look up storage space")
@@ -135,20 +142,13 @@ func (s *svc) handlePut(ctx context.Context, w http.ResponseWriter, r *http.Requ
 		return
 	}
 
-	length, err := getContentLength(w, r)
+	length, err := getContentLength(r)
 	if err != nil {
 		log.Error().Err(err).Msg("error getting the content length")
 		w.WriteHeader(http.StatusBadRequest)
 		return
 	}
 
-	if err := ValidateName(filepath.Base(ref.Path), s.nameValidators); err != nil {
-		w.WriteHeader(http.StatusBadRequest)
-		b, err := errors.Marshal(http.StatusBadRequest, err.Error(), "")
-		errors.HandleWebdavError(&log, w, b, err)
-		return
-	}
-
 	client, err := s.gatewaySelector.Next()
 	if err != nil {
 		log.Error().Err(err).Msg("error selecting next gateway client")
@@ -411,6 +411,13 @@ func (s *svc) handleSpacesPut(w http.ResponseWriter, r *http.Request, spaceID st
 		return
 	}
 
+	if err := ValidateName(filename(ref.Path), s.nameValidators); err != nil {
+		w.WriteHeader(http.StatusBadRequest)
+		b, err := errors.Marshal(http.StatusBadRequest, err.Error(), "")
+		errors.HandleWebdavError(&sublog, w, b, err)
+		return
+	}
+
 	s.handlePut(ctx, w, r, &ref, sublog)
 }
 
@@ -432,7 +439,7 @@ func checkPreconditions(w http.ResponseWriter, r *http.Request, log zerolog.Logg
 	return true
 }
 
-func getContentLength(w http.ResponseWriter, r *http.Request) (int64, error) {
+func getContentLength(r *http.Request) (int64, error) {
 	length, err := strconv.ParseInt(r.Header.Get(net.HeaderContentLength), 10, 64)
 	if err != nil {
 		// Fallback to Upload-Length

internal/http/services/owncloud/ocdav/tus.go

@@ -58,21 +58,15 @@ func (s *svc) handlePathTusPost(w http.ResponseWriter, r *http.Request, ns strin
 
 	// read filename from metadata
 	meta := tusd.ParseMetadataHeader(r.Header.Get(net.HeaderUploadMetadata))
-	if err := ValidateName(path.Base(meta["filename"]), s.nameValidators); err != nil {
-		w.WriteHeader(http.StatusPreconditionFailed)
-		return
-	}
 
 	// append filename to current dir
-	fn := path.Join(ns, r.URL.Path, meta["filename"])
-
-	sublog := appctx.GetLogger(ctx).With().Str("path", fn).Logger()
-	// check tus headers?
-
 	ref := &provider.Reference{
-		// FIXME ResourceId?
-		Path: fn,
+		// a path based request has no resource id, so we can only provide a path. The gateway has te figure out which provider is responsible
+		Path: path.Join(ns, r.URL.Path, meta["filename"]),
 	}
+
+	sublog := appctx.GetLogger(ctx).With().Str("path", r.URL.Path).Str("filename", meta["filename"]).Logger()
+
 	s.handleTusPost(ctx, w, r, meta, ref, sublog)
 }
 
@@ -82,19 +76,15 @@ func (s *svc) handleSpacesTusPost(w http.ResponseWriter, r *http.Request, spaceI
 
 	// read filename from metadata
 	meta := tusd.ParseMetadataHeader(r.Header.Get(net.HeaderUploadMetadata))
-	if err := ValidateName(path.Base(meta["filename"]), s.nameValidators); err != nil {
-		w.WriteHeader(http.StatusPreconditionFailed)
-		return
-	}
-
-	sublog := appctx.GetLogger(ctx).With().Str("spaceid", spaceID).Str("path", r.URL.Path).Logger()
 
 	ref, err := spacelookup.MakeStorageSpaceReference(spaceID, path.Join(r.URL.Path, meta["filename"]))
 	if err != nil {
 		w.WriteHeader(http.StatusBadRequest)
 		return
 	}
 
+	sublog := appctx.GetLogger(ctx).With().Str("spaceid", spaceID).Str("path", r.URL.Path).Str("filename", meta["filename"]).Logger()
+
 	s.handleTusPost(ctx, w, r, meta, &ref, sublog)
 }
 
@@ -116,6 +106,10 @@ func (s *svc) handleTusPost(ctx context.Context, w http.ResponseWriter, r *http.
 		w.WriteHeader(http.StatusPreconditionFailed)
 		return
 	}
+	if err := ValidateName(filename(meta["filename"]), s.nameValidators); err != nil {
+		w.WriteHeader(http.StatusPreconditionFailed)
+		return
+	}
 
 	// Test if the target is a secret filedrop
 	var isSecretFileDrop bool

internal/http/services/owncloud/ocdav/validation.go

@@ -27,6 +27,11 @@ func ValidatorsFromConfig(c *config.Config) []Validator {
 
 // ValidateName will validate a file or folder name, returning an error when it is not accepted
 func ValidateName(name string, validators []Validator) error {
+	return ValidateDestination(name, append(validators, notReserved()))
+}
+
+// ValidateDestination will validate a file or folder destination name (which can be . or ..), returning an error when it is not accepted
+func ValidateDestination(name string, validators []Validator) error {
 	for _, v := range validators {
 		if err := v(name); err != nil {
 			return fmt.Errorf("name validation failed: %w", err)
@@ -35,6 +40,15 @@ func ValidateName(name string, validators []Validator) error {
 	return nil
 }
 
+func notReserved() Validator {
+	return func(s string) error {
+		if s == ".." || s == "." {
+			return errors.New(". and .. are reserved names")
+		}
+		return nil
+	}
+}
+
 func notEmpty() Validator {
 	return func(s string) error {
 		if strings.TrimSpace(s) == "" {

tests/acceptance/expected-failures-on-OCIS-storage.md

@@ -251,5 +251,21 @@ _The below features have been added after I last categorized them. AFAICT they a
 - [coreApiWebdavMove1/moveFolder.feature:254](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavMove1/moveFolder.feature#L254)
 - [coreApiWebdavMove1/moveFolder.feature:259](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavMove1/moveFolder.feature#L259)
 
+### [expected failures due to introduction of `..` mask](https://github.com/cs3org/reva/pull/4740)
+
+- [coreApiWebdavProperties/createFileFolder.feature:206](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L206)
+- [coreApiWebdavProperties/createFileFolder.feature:209](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L209)
+- [coreApiWebdavProperties/createFileFolder.feature:210](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L210)
+- [coreApiWebdavProperties/createFileFolder.feature:212](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L212)
+- [coreApiWebdavProperties/createFileFolder.feature:213](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L213)
+- [coreApiWebdavProperties/createFileFolder.feature:218](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L218)
+- [coreApiWebdavProperties/createFileFolder.feature:220](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L220)
+- [coreApiWebdavProperties/createFileFolder.feature:221](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L221)
+- [coreApiWebdavProperties/createFileFolder.feature:230](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L230)
+- [coreApiWebdavProperties/createFileFolder.feature:233](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L233)
+- [coreApiWebdavProperties/createFileFolder.feature:234](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L234)
+- [coreApiWebdavProperties/createFileFolder.feature:236](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L236)
+- [coreApiWebdavProperties/createFileFolder.feature:237](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L237)
+
 - Note: always have an empty line at the end of this file.
 The bash script that processes this file may not process a scenario reference on the last line.

tests/acceptance/expected-failures-on-POSIX-storage.md

@@ -403,5 +403,21 @@ _The below features have been added after I last categorized them. AFAICT they a
 - [coreApiWebdavMove1/moveFolder.feature:276](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavMove1/moveFolder.feature#L276)
 - [coreApiWebdavMove1/moveFolder.feature:281](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavMove1/moveFolder.feature#L281)
 
+### expected failures due to introduction of `..` mask
+
+- [coreApiWebdavProperties/createFileFolder.feature:206](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L206)
+- [coreApiWebdavProperties/createFileFolder.feature:209](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L209)
+- [coreApiWebdavProperties/createFileFolder.feature:210](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L210)
+- [coreApiWebdavProperties/createFileFolder.feature:212](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L212)
+- [coreApiWebdavProperties/createFileFolder.feature:213](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L213)
+- [coreApiWebdavProperties/createFileFolder.feature:218](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L218)
+- [coreApiWebdavProperties/createFileFolder.feature:220](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L220)
+- [coreApiWebdavProperties/createFileFolder.feature:221](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L221)
+- [coreApiWebdavProperties/createFileFolder.feature:230](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L230)
+- [coreApiWebdavProperties/createFileFolder.feature:233](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L233)
+- [coreApiWebdavProperties/createFileFolder.feature:234](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L234)
+- [coreApiWebdavProperties/createFileFolder.feature:236](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L236)
+- [coreApiWebdavProperties/createFileFolder.feature:237](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L237)
+
 - Note: always have an empty line at the end of this file.
 The bash script that processes this file may not process a scenario reference on the last line.

tests/acceptance/expected-failures-on-S3NG-storage.md

@@ -254,5 +254,21 @@ _The below features have been added after I last categorized them. AFAICT they a
 - [coreApiWebdavMove1/moveFolder.feature:254](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavMove1/moveFolder.feature#L254)
 - [coreApiWebdavMove1/moveFolder.feature:259](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavMove1/moveFolder.feature#L259)
 
+### [create file folder issues resulting from masked `..`](https://github.com/cs3org/reva/pull/4740)
+
+- [coreApiWebdavProperties/createFileFolder.feature:206](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L206)
+- [coreApiWebdavProperties/createFileFolder.feature:209](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L209)
+- [coreApiWebdavProperties/createFileFolder.feature:210](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L210)
+- [coreApiWebdavProperties/createFileFolder.feature:212](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L212)
+- [coreApiWebdavProperties/createFileFolder.feature:213](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L213)
+- [coreApiWebdavProperties/createFileFolder.feature:218](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L218)
+- [coreApiWebdavProperties/createFileFolder.feature:220](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L220)
+- [coreApiWebdavProperties/createFileFolder.feature:221](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L221)
+- [coreApiWebdavProperties/createFileFolder.feature:230](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L230)
+- [coreApiWebdavProperties/createFileFolder.feature:233](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L233)
+- [coreApiWebdavProperties/createFileFolder.feature:234](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L234)
+- [coreApiWebdavProperties/createFileFolder.feature:236](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L236)
+- [coreApiWebdavProperties/createFileFolder.feature:237](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiWebdavProperties/createFileFolder.feature#L237)
+
 Note: always have an empty line at the end of this file.
 The bash script that processes this file may not process a scenario reference on the last line.