From 23783b1a33bbb5a36bd3c71cf4a7965e54974e77 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Fri, 13 Nov 2020 12:12:30 +0100 Subject: [PATCH 01/11] avoid panic on /remote.php/dav/files/ --- internal/grpc/services/gateway/storageprovider.go | 7 +++++++ internal/http/services/owncloud/ocdav/propfind.go | 4 ++++ 2 files changed, 11 insertions(+) diff --git a/internal/grpc/services/gateway/storageprovider.go b/internal/grpc/services/gateway/storageprovider.go index 8537e8580d..ee62ca0e06 100644 --- a/internal/grpc/services/gateway/storageprovider.go +++ b/internal/grpc/services/gateway/storageprovider.go @@ -1315,6 +1315,13 @@ func (s *svc) Stat(ctx context.Context, req *provider.StatRequest) (*provider.St return s.statHome(ctx) } + // webdav endpoint is called without a path. i.e: /remote.php/dav/files + if len(strings.Split(req.Ref.GetPath(), "/")) == 2 { + return &provider.StatResponse{ + Status: status.NewUnimplemented(ctx, nil, "method not allowed"), + }, nil + } + if s.isSharedFolder(ctx, p) { return s.statSharesFolder(ctx) } diff --git a/internal/http/services/owncloud/ocdav/propfind.go b/internal/http/services/owncloud/ocdav/propfind.go index e6fbdd591f..5930e90a17 100644 --- a/internal/http/services/owncloud/ocdav/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind.go @@ -95,6 +95,10 @@ func (s *svc) handlePropfind(w http.ResponseWriter, r *http.Request, ns string) case rpc.Code_CODE_PERMISSION_DENIED: log.Debug().Str("path", fn).Interface("status", res.Status).Msg("permission denied") w.WriteHeader(http.StatusMultiStatus) + // TODO this is not a correct mapping. A new code SHOULD be added to the cs3apis. + case rpc.Code_CODE_UNIMPLEMENTED: + log.Debug().Str("path", fn).Interface("status", res.Status).Msg("method not allowed") + w.WriteHeader(http.StatusMethodNotAllowed) default: log.Error().Str("path", fn).Interface("status", res.Status).Msg("grpc stat request failed") w.WriteHeader(http.StatusInternalServerError) From 24bf5c480512a6de8a651a3a6810fbcf584db6da Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 24 Nov 2020 11:16:23 +0100 Subject: [PATCH 02/11] contain changes only to ocdav.go --- internal/grpc/services/gateway/storageprovider.go | 7 ------- internal/http/services/owncloud/ocdav/propfind.go | 12 ++++++++---- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/internal/grpc/services/gateway/storageprovider.go b/internal/grpc/services/gateway/storageprovider.go index a39493f4b2..399ab087a4 100644 --- a/internal/grpc/services/gateway/storageprovider.go +++ b/internal/grpc/services/gateway/storageprovider.go @@ -1341,13 +1341,6 @@ func (s *svc) Stat(ctx context.Context, req *provider.StatRequest) (*provider.St return s.statHome(ctx) } - // webdav endpoint is called without a path. i.e: /remote.php/dav/files - if len(strings.Split(req.Ref.GetPath(), "/")) == 2 { - return &provider.StatResponse{ - Status: status.NewUnimplemented(ctx, nil, "method not allowed"), - }, nil - } - if s.isSharedFolder(ctx, p) { return s.statSharesFolder(ctx) } diff --git a/internal/http/services/owncloud/ocdav/propfind.go b/internal/http/services/owncloud/ocdav/propfind.go index 5930e90a17..74e142ce1d 100644 --- a/internal/http/services/owncloud/ocdav/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind.go @@ -55,6 +55,14 @@ func (s *svc) handlePropfind(w http.ResponseWriter, r *http.Request, ns string) if depth == "" { depth = "1" } + + // if there is no file in the request url we assume the request url is: "/remote.php/dav/files" + // https://github.com/owncloud/core/blob/18475dac812064b21dabcc50f25ef3ffe55691a5/tests/acceptance/features/apiWebdavOperations/propfind.feature + if r.URL.Path == "/" { + log.Debug().Str("path", r.URL.Path).Msg("method not allowed") + w.WriteHeader(http.StatusMethodNotAllowed) + } + // see https://tools.ietf.org/html/rfc4918#section-10.2 if depth != "0" && depth != "1" && depth != "infinity" { log.Error().Msgf("invalid Depth header value %s", depth) @@ -95,10 +103,6 @@ func (s *svc) handlePropfind(w http.ResponseWriter, r *http.Request, ns string) case rpc.Code_CODE_PERMISSION_DENIED: log.Debug().Str("path", fn).Interface("status", res.Status).Msg("permission denied") w.WriteHeader(http.StatusMultiStatus) - // TODO this is not a correct mapping. A new code SHOULD be added to the cs3apis. - case rpc.Code_CODE_UNIMPLEMENTED: - log.Debug().Str("path", fn).Interface("status", res.Status).Msg("method not allowed") - w.WriteHeader(http.StatusMethodNotAllowed) default: log.Error().Str("path", fn).Interface("status", res.Status).Msg("grpc stat request failed") w.WriteHeader(http.StatusInternalServerError) From b8222bd46ceb98bdb9f88a53f994bd3d19d58988 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 24 Nov 2020 14:23:52 +0100 Subject: [PATCH 03/11] enhance xml error response --- .../http/services/owncloud/ocdav/error.go | 50 +++++++++++++++++++ .../http/services/owncloud/ocdav/propfind.go | 19 ++++++- 2 files changed, 67 insertions(+), 2 deletions(-) create mode 100644 internal/http/services/owncloud/ocdav/error.go diff --git a/internal/http/services/owncloud/ocdav/error.go b/internal/http/services/owncloud/ocdav/error.go new file mode 100644 index 0000000000..42e71735a2 --- /dev/null +++ b/internal/http/services/owncloud/ocdav/error.go @@ -0,0 +1,50 @@ +// Copyright 2018-2020 CERN +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// In applying this license, CERN does not waive the privileges and immunities +// granted to it by virtue of its status as an Intergovernmental Organization +// or submit itself to any jurisdiction. + +package ocdav + +import ( + "encoding/xml" +) + +type code int + +const ( + SabredavMethodNotAllowed code = iota +) + +var ( + codesEnum = []string{ + "Sabre\\DAV\\Exception\\MethodNotAllowed", + } +) + +type exception struct { + code code + message string +} + +func Marshal(e exception) ([]byte, error) { + + return xml.Marshal(&errorXML{ + Xmlnsd: "DAV", + Xmlnss: "http://sabredav.org/ns", + Exception: codesEnum[e.code], + Message: e.message, + }) +} diff --git a/internal/http/services/owncloud/ocdav/propfind.go b/internal/http/services/owncloud/ocdav/propfind.go index 74e142ce1d..03d5ce42b3 100644 --- a/internal/http/services/owncloud/ocdav/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind.go @@ -61,6 +61,17 @@ func (s *svc) handlePropfind(w http.ResponseWriter, r *http.Request, ns string) if r.URL.Path == "/" { log.Debug().Str("path", r.URL.Path).Msg("method not allowed") w.WriteHeader(http.StatusMethodNotAllowed) + b, err := Marshal(exception{ + code: SabredavMethodNotAllowed, + message: "Listing members of this collection is disabled", + }) + if err != nil { + log.Error().Msgf("error marshaling xml response: %s", b) + w.WriteHeader(http.StatusInternalServerError) + return + } + w.Write(b) + return } // see https://tools.ietf.org/html/rfc4918#section-10.2 @@ -647,8 +658,12 @@ type propertyXML struct { // http://www.webdav.org/specs/rfc4918.html#ELEMENT_error type errorXML struct { - XMLName xml.Name `xml:"d:error"` - InnerXML []byte `xml:",innerxml"` + XMLName xml.Name `xml:"d:error"` + Xmlnsd string `xml:"xmlns:d,attr"` + Xmlnss string `xml:"xmlns:s,attr"` + Exception string `xml:"s:exception"` + Message string `xml:"s:message"` + InnerXML []byte `xml:",innerxml"` } var errInvalidPropfind = errors.New("webdav: invalid propfind") From f02d7ab2afeb1ddf025d0a61610d80c2018701c3 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 24 Nov 2020 14:34:44 +0100 Subject: [PATCH 04/11] fix linter --- internal/http/services/owncloud/ocdav/error.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/http/services/owncloud/ocdav/error.go b/internal/http/services/owncloud/ocdav/error.go index 42e71735a2..ac993f4d7d 100644 --- a/internal/http/services/owncloud/ocdav/error.go +++ b/internal/http/services/owncloud/ocdav/error.go @@ -25,6 +25,7 @@ import ( type code int const ( + // SabredavMethodNotAllowed maps to HTTP 405 SabredavMethodNotAllowed code = iota ) @@ -39,8 +40,8 @@ type exception struct { message string } +// Marshal just calls the xml marshaller for a given exception. func Marshal(e exception) ([]byte, error) { - return xml.Marshal(&errorXML{ Xmlnsd: "DAV", Xmlnss: "http://sabredav.org/ns", From b6a59b979322ef36228be896e75549f37a924e58 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 24 Nov 2020 14:44:19 +0100 Subject: [PATCH 05/11] adjust expected scenario --- .../apiOcisSpecific/apiShareWebdavOperations-propfind.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/acceptance/features/apiOcisSpecific/apiShareWebdavOperations-propfind.feature b/tests/acceptance/features/apiOcisSpecific/apiShareWebdavOperations-propfind.feature index c1742ac40d..6b2a07c03d 100644 --- a/tests/acceptance/features/apiOcisSpecific/apiShareWebdavOperations-propfind.feature +++ b/tests/acceptance/features/apiOcisSpecific/apiShareWebdavOperations-propfind.feature @@ -6,4 +6,4 @@ Feature: PROPFIND Scenario: PROPFIND to "/remote.php/dav/files" Given user "Alice" has been created with default attributes and without skeleton files When user "Alice" requests "/remote.php/dav/files" with "PROPFIND" using basic auth - Then the HTTP status code should be "500" + Then the HTTP status code should be "405" From f972f66c5639bf586b36d1f56fa178d64e5dbd5e Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 24 Nov 2020 14:48:14 +0100 Subject: [PATCH 06/11] add changelog --- changelog/unreleased/handle-invalid-webdav-listing.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/unreleased/handle-invalid-webdav-listing.md diff --git a/changelog/unreleased/handle-invalid-webdav-listing.md b/changelog/unreleased/handle-invalid-webdav-listing.md new file mode 100644 index 0000000000..b7a8e23923 --- /dev/null +++ b/changelog/unreleased/handle-invalid-webdav-listing.md @@ -0,0 +1,5 @@ +Bugfix: Do not panic on remote.php/dav/files/ + +Currently requests to /remote.php/dav/files/ result in panics since we cannot longer strip the user + destination from the url. This fixes the server response code and adds an error body to the response. + +https://github.com/cs3org/reva/pull/1320 \ No newline at end of file From 5c0cef94c17c84490928746b615ee883da029481 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 24 Nov 2020 14:51:41 +0100 Subject: [PATCH 07/11] error check write operation --- internal/http/services/owncloud/ocdav/propfind.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/http/services/owncloud/ocdav/propfind.go b/internal/http/services/owncloud/ocdav/propfind.go index 03d5ce42b3..a6dc286c9f 100644 --- a/internal/http/services/owncloud/ocdav/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind.go @@ -70,7 +70,12 @@ func (s *svc) handlePropfind(w http.ResponseWriter, r *http.Request, ns string) w.WriteHeader(http.StatusInternalServerError) return } - w.Write(b) + _, err = w.Write(b) + if err != nil { + log.Error().Msgf("error writing xml response: %s", b) + w.WriteHeader(http.StatusInternalServerError) + return + } return } From 392ea6712e623a12bec6cab90c28231b7cb7c464 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 24 Nov 2020 15:31:59 +0100 Subject: [PATCH 08/11] undo [tests-only] --- .../http/services/owncloud/ocdav/propfind.go | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/propfind.go b/internal/http/services/owncloud/ocdav/propfind.go index a6dc286c9f..2f78fc0613 100644 --- a/internal/http/services/owncloud/ocdav/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind.go @@ -58,26 +58,26 @@ func (s *svc) handlePropfind(w http.ResponseWriter, r *http.Request, ns string) // if there is no file in the request url we assume the request url is: "/remote.php/dav/files" // https://github.com/owncloud/core/blob/18475dac812064b21dabcc50f25ef3ffe55691a5/tests/acceptance/features/apiWebdavOperations/propfind.feature - if r.URL.Path == "/" { - log.Debug().Str("path", r.URL.Path).Msg("method not allowed") - w.WriteHeader(http.StatusMethodNotAllowed) - b, err := Marshal(exception{ - code: SabredavMethodNotAllowed, - message: "Listing members of this collection is disabled", - }) - if err != nil { - log.Error().Msgf("error marshaling xml response: %s", b) - w.WriteHeader(http.StatusInternalServerError) - return - } - _, err = w.Write(b) - if err != nil { - log.Error().Msgf("error writing xml response: %s", b) - w.WriteHeader(http.StatusInternalServerError) - return - } - return - } + //if r.URL.Path == "/" { + // log.Debug().Str("path", r.URL.Path).Msg("method not allowed") + // w.WriteHeader(http.StatusMethodNotAllowed) + // b, err := Marshal(exception{ + // code: SabredavMethodNotAllowed, + // message: "Listing members of this collection is disabled", + // }) + // if err != nil { + // log.Error().Msgf("error marshaling xml response: %s", b) + // w.WriteHeader(http.StatusInternalServerError) + // return + // } + // _, err = w.Write(b) + // if err != nil { + // log.Error().Msgf("error writing xml response: %s", b) + // w.WriteHeader(http.StatusInternalServerError) + // return + // } + // return + //} // see https://tools.ietf.org/html/rfc4918#section-10.2 if depth != "0" && depth != "1" && depth != "infinity" { From c777d6f0209e88478eb9064feb45b264d6e6333a Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 24 Nov 2020 15:51:30 +0100 Subject: [PATCH 09/11] move block from propfind -> dav --- internal/http/services/owncloud/ocdav/dav.go | 23 +++++++++++++++++++ .../http/services/owncloud/ocdav/propfind.go | 23 ------------------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/internal/http/services/owncloud/ocdav/dav.go b/internal/http/services/owncloud/ocdav/dav.go index 52e5c52c5e..0c1ff00996 100644 --- a/internal/http/services/owncloud/ocdav/dav.go +++ b/internal/http/services/owncloud/ocdav/dav.go @@ -93,6 +93,29 @@ func (h *DavHandler) Handler(s *svc) http.Handler { ctx := r.Context() log := appctx.GetLogger(ctx) + // if there is no file in the request url we assume the request url is: "/remote.php/dav/files" + // https://github.com/owncloud/core/blob/18475dac812064b21dabcc50f25ef3ffe55691a5/tests/acceptance/features/apiWebdavOperations/propfind.feature + if r.URL.Path == "/files" { + log.Debug().Str("path", r.URL.Path).Msg("method not allowed") + w.WriteHeader(http.StatusMethodNotAllowed) + b, err := Marshal(exception{ + code: SabredavMethodNotAllowed, + message: "Listing members of this collection is disabled", + }) + if err != nil { + log.Error().Msgf("error marshaling xml response: %s", b) + w.WriteHeader(http.StatusInternalServerError) + return + } + _, err = w.Write(b) + if err != nil { + log.Error().Msgf("error writing xml response: %s", b) + w.WriteHeader(http.StatusInternalServerError) + return + } + return + } + var head string head, r.URL.Path = router.ShiftPath(r.URL.Path) diff --git a/internal/http/services/owncloud/ocdav/propfind.go b/internal/http/services/owncloud/ocdav/propfind.go index 2f78fc0613..30acd20702 100644 --- a/internal/http/services/owncloud/ocdav/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind.go @@ -56,29 +56,6 @@ func (s *svc) handlePropfind(w http.ResponseWriter, r *http.Request, ns string) depth = "1" } - // if there is no file in the request url we assume the request url is: "/remote.php/dav/files" - // https://github.com/owncloud/core/blob/18475dac812064b21dabcc50f25ef3ffe55691a5/tests/acceptance/features/apiWebdavOperations/propfind.feature - //if r.URL.Path == "/" { - // log.Debug().Str("path", r.URL.Path).Msg("method not allowed") - // w.WriteHeader(http.StatusMethodNotAllowed) - // b, err := Marshal(exception{ - // code: SabredavMethodNotAllowed, - // message: "Listing members of this collection is disabled", - // }) - // if err != nil { - // log.Error().Msgf("error marshaling xml response: %s", b) - // w.WriteHeader(http.StatusInternalServerError) - // return - // } - // _, err = w.Write(b) - // if err != nil { - // log.Error().Msgf("error writing xml response: %s", b) - // w.WriteHeader(http.StatusInternalServerError) - // return - // } - // return - //} - // see https://tools.ietf.org/html/rfc4918#section-10.2 if depth != "0" && depth != "1" && depth != "infinity" { log.Error().Msgf("invalid Depth header value %s", depth) From 251c49fdbcd8aebdc07518f56799b2cc5bc88114 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 24 Nov 2020 16:08:50 +0100 Subject: [PATCH 10/11] remove expected to fail scenario --- tests/acceptance/expected-failures-on-OCIS-storage.txt | 4 ---- tests/acceptance/expected-failures-on-OWNCLOUD-storage.txt | 4 ---- 2 files changed, 8 deletions(-) diff --git a/tests/acceptance/expected-failures-on-OCIS-storage.txt b/tests/acceptance/expected-failures-on-OCIS-storage.txt index e26d2bcdf6..5fbeca9d71 100644 --- a/tests/acceptance/expected-failures-on-OCIS-storage.txt +++ b/tests/acceptance/expected-failures-on-OCIS-storage.txt @@ -1067,10 +1067,6 @@ apiWebdavOperations/refuseAccess.feature:22 apiWebdavOperations/refuseAccess.feature:33 apiWebdavOperations/refuseAccess.feature:34 # -# https://github.com/owncloud/core/pull/38035 PROPFIND to https://localhost:9200/remote.php/dav/files gets an error 500 response -# -apiWebdavOperations/propfind.feature:5 -# # https://github.com/owncloud/ocis-reva/issues/39 REPORT request not implemented # apiWebdavOperations/search.feature:42 diff --git a/tests/acceptance/expected-failures-on-OWNCLOUD-storage.txt b/tests/acceptance/expected-failures-on-OWNCLOUD-storage.txt index 5cf750dd04..769135c787 100644 --- a/tests/acceptance/expected-failures-on-OWNCLOUD-storage.txt +++ b/tests/acceptance/expected-failures-on-OWNCLOUD-storage.txt @@ -1038,10 +1038,6 @@ apiWebdavOperations/refuseAccess.feature:22 apiWebdavOperations/refuseAccess.feature:33 apiWebdavOperations/refuseAccess.feature:34 # -# https://github.com/owncloud/core/pull/38035 PROPFIND to https://localhost:9200/remote.php/dav/files gets an error 500 response -# -apiWebdavOperations/propfind.feature:5 -# # https://github.com/owncloud/ocis-reva/issues/39 REPORT request not implemented # apiWebdavOperations/search.feature:42 From 0f5f818abd70a25682f300a8e4c547b636dfb262 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Wed, 25 Nov 2020 09:03:30 +0100 Subject: [PATCH 11/11] delete apiShareWebdavOperations-propfind.feature --- .../apiShareWebdavOperations-propfind.feature | 9 --------- 1 file changed, 9 deletions(-) delete mode 100644 tests/acceptance/features/apiOcisSpecific/apiShareWebdavOperations-propfind.feature diff --git a/tests/acceptance/features/apiOcisSpecific/apiShareWebdavOperations-propfind.feature b/tests/acceptance/features/apiOcisSpecific/apiShareWebdavOperations-propfind.feature deleted file mode 100644 index 6b2a07c03d..0000000000 --- a/tests/acceptance/features/apiOcisSpecific/apiShareWebdavOperations-propfind.feature +++ /dev/null @@ -1,9 +0,0 @@ -@api -Feature: PROPFIND - - @issue-ocis-751 - # after fixing all issues delete this Scenario and use the one from oC10 core - Scenario: PROPFIND to "/remote.php/dav/files" - Given user "Alice" has been created with default attributes and without skeleton files - When user "Alice" requests "/remote.php/dav/files" with "PROPFIND" using basic auth - Then the HTTP status code should be "405"