Prevent panic on /remote.php/dav/files/ #1320

refs · 2020-11-13T11:14:38Z

Currently requests to /remote.php/dav/files/ result in panics since we cannot longer strip the user + destination from the url.

We're reproducing this issue on the ocis storage. I wanted to tackle this at the storage level but sadly the panic happens before reaching the stat call.

I wonder if there is a better way to achieve the same results but instead letting the storage handle the way it interprets paths. Open to suggestions 💃

update-docs · 2020-11-13T11:14:41Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

labkode · 2020-11-13T12:36:43Z

@refs can you provide info of the panic?

phil-davis · 2020-11-13T12:40:11Z

https://cloud.drone.io/cs3org/reva/3145/13/6

runsh: Total unexpected passed scenarios throughout the test run:
apiWebdavOperations/propfind.feature:5

This does "a good thing" - that now-passing scenario should be deleted from the expected-failures files.

phil-davis · 2020-11-13T12:42:14Z

And look at https://cloud.drone.io/cs3org/reva/3145/6/7 tests/acceptance/features/apiOcisSpecific/apiShareWebdavOperations-propfind.feature:6

  Scenario: PROPFIND to "/remote.php/dav/files"                                            # /drone/src/tests/acceptance/features/apiOcisSpecific/apiShareWebdavOperations-propfind.feature:6
    Given user "Alice" has been created with default attributes and without skeleton files # FeatureContext::userHasBeenCreatedWithDefaultAttributesAndWithoutSkeletonFiles()
    When user "Alice" requests "/remote.php/dav/files" with "PROPFIND" using basic auth    # AuthContext::userRequestsURLWithUsingBasicAuth()
    Then the HTTP status code should be "500"                                              # FeatureContext::thenTheHTTPStatusCodeShouldBe()
      HTTP status code 405 is not the expected value 500
      Failed asserting that 405 matches expected '500'.

That bug-demo scenario is now producing a good result. It can be deleted.

refs · 2020-11-23T08:44:23Z

hi @labkode sorry for the radio silence, I was off sick the past week with some string children virus thing ...

The panic I was referring to happens here:

reva/internal/grpc/services/gateway/storageprovider.go

Lines 2017 to 2020 in b2c4af4

    
           // validate that we have always at least two elements 
        
           if len(parts) < 2 { 
        
           	panic(fmt.Sprintf("split: len(parts) < 2: path:%s parts:%+v", p, parts)) 
        
           }

and it is triggered when the URL looks just like in the description of this pull request:

/remote.php/dav/files/

I'm open to better alternatives and not just patch this :)

labkode · 2020-11-23T13:40:42Z

@refs to trigger this condition one has to be on a "Shared folder" that must contain this structure.
If the path is outside the Shared folder (remote.php/dav/files) is not a shared folder this method should never be called.
Is it possible that your config is missing or has a different value for the Shares folder?

Anyways, the panic could have some nice logging to inform of the problem.

butonic · 2020-11-23T14:54:04Z

internal/grpc/services/gateway/storageprovider.go

+	// 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"),


A storage provider determines if the listing of the root folder is allowed based on permissions, not the gateway ...

butonic · 2020-11-23T14:55:28Z

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:


we should check if the username is missing only in the ocs api. no need to push these quirks into storage providers or the cs3 api.

agree, will contain the changes to ocdav only

refs · 2020-11-24T10:21:16Z

@butonic changes are now contained to ocdav.go and its path handling magic 🌸 will put some effort into returning the correct XML and refactoring a little bit.

I thought of a middleware for not allowed methods to prevent from executing unnecessary logic but it might be an early optimization, but the fact that there is no way to catch this early on our router has some room for improvement 🚀

@labkode as per the behavior, it is captured here: https://github.com/owncloud/core/blob/18475dac812064b21dabcc50f25ef3ffe55691a5/tests/acceptance/features/apiWebdavOperations/propfind.feature

honestly that LoC on Reva was running regardless of being on a shared folder or not, the stack trace shed some light on the path taken, I recommend having a look at it. I will dig into my configuration to discard it but we're not having a very complex use case, but we need to be sure.

butonic · 2020-11-24T15:28:35Z

tests/acceptance/features/apiOcisSpecific/apiShareWebdavOperations-propfind.feature

@@ -6,4 +6,4 @@ Feature: PROPFIND
  Scenario: PROPFIND to "/remote.php/dav/files"


If you check the line above you should find the after fixing all issues delete this Scenario and use the one from oC10 core, meaning that AFAICT this complete feature file can be removed, as it is covered by the core tests.

remove tests/acceptance/features/apiOcisSpecific/apiShareWebdavOperations-propfind.feature

phil-davis

Looks good from a test point of view - the related scenario in the core API tests now passes, and the local bug-demo scenario has been deleted.

ishank011 · 2020-11-25T15:11:30Z

@refs can we try to return false instead of raising a panic here?

reva/internal/grpc/services/gateway/storageprovider.go

Lines 2017 to 2020 in b2c4af4

    
           // validate that we have always at least two elements 
        
           if len(parts) < 2 { 
        
           	panic(fmt.Sprintf("split: len(parts) < 2: path:%s parts:%+v", p, parts)) 
        
           }

The next step will be to find the storage provider and since it won't find any mapping for it, it will return the appropriate error. That's because this is raised a lot of other times when the user doesn't provide a correct path from any other clients.

refs · 2020-11-25T15:12:33Z

@refs can we try to return false instead of raising a panic here?

reva/internal/grpc/services/gateway/storageprovider.go

Lines 2017 to 2020 in b2c4af4

// validate that we have always at least two elements

if len(parts) < 2 {

panic(fmt.Sprintf("split: len(parts) < 2: path:%s parts:%+v", p, parts))

}

The next step will be to find the storage provider and since it won't find any mapping for it, it will return the appropriate error. That's because this is raised a lot of other times when the user doesn't provide a correct path from any other clients.

I'd say that's out of the scope of this one :D

ishank011

Oh okay. No worries!

avoid panic on /remote.php/dav/files/

23783b1

refs requested a review from labkode as a code owner November 13, 2020 11:14

exalate-issue-sync bot mentioned this pull request Nov 13, 2020

PROPFIND to https://localhost:9200/remote.php/dav/files gets an error 500 response owncloud/ocis#751

Closed

refs changed the title ~~Avoid panic on /remote.php/dav/files/~~ Prevent panic on /remote.php/dav/files/ Nov 13, 2020

butonic requested changes Nov 23, 2020

View reviewed changes

refs added 2 commits November 24, 2020 10:52

Merge branch 'master' into dav-files-unimplemented

370830e

contain changes only to ocdav.go

24bf5c4

butonic previously approved these changes Nov 24, 2020

View reviewed changes

enhance xml error response

b8222bd

refs dismissed butonic’s stale review via b8222bd November 24, 2020 13:23

refs added 4 commits November 24, 2020 14:34

fix linter

f02d7ab

adjust expected scenario

b6a59b9

add changelog

f972f66

error check write operation

5c0cef9

micbar requested a review from butonic November 24, 2020 14:00

refs added 4 commits November 24, 2020 15:01

Merge branch 'master' into dav-files-unimplemented

3cf4e25

undo [tests-only]

392ea67

move block from propfind -> dav

c777d6f

remove expected to fail scenario

251c49f

butonic requested changes Nov 24, 2020

View reviewed changes

delete apiShareWebdavOperations-propfind.feature

0f5f818

phil-davis reviewed Nov 25, 2020

View reviewed changes

butonic approved these changes Nov 25, 2020

View reviewed changes

ishank011 approved these changes Nov 25, 2020

View reviewed changes

ishank011 merged commit ee35749 into cs3org:master Nov 25, 2020

phil-davis mentioned this pull request Nov 25, 2020

[tests-only] Bump commit id for API tests #1337

Merged

refs mentioned this pull request Nov 25, 2020

Update reva to v1.4.1-0.20201125144025-57da0c27434c owncloud/ocis#951

Merged

micbar mentioned this pull request Dec 16, 2020

[with-benchmarks] Merge back after 1.0.0 Release owncloud/ocis#1117

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent panic on /remote.php/dav/files/ #1320

Prevent panic on /remote.php/dav/files/ #1320

refs commented Nov 13, 2020

update-docs bot commented Nov 13, 2020

labkode commented Nov 13, 2020

phil-davis commented Nov 13, 2020

phil-davis commented Nov 13, 2020

refs commented Nov 23, 2020

labkode commented Nov 23, 2020 •

edited

Loading

butonic Nov 23, 2020

butonic Nov 23, 2020

refs Nov 24, 2020

refs commented Nov 24, 2020

butonic Nov 24, 2020 •

edited

Loading

phil-davis left a comment

ishank011 commented Nov 25, 2020

refs commented Nov 25, 2020

ishank011 left a comment

		@@ -6,4 +6,4 @@ Feature: PROPFIND
		Scenario: PROPFIND to "/remote.php/dav/files"

Prevent panic on /remote.php/dav/files/ #1320

Prevent panic on /remote.php/dav/files/ #1320

Conversation

refs commented Nov 13, 2020

update-docs bot commented Nov 13, 2020

labkode commented Nov 13, 2020

phil-davis commented Nov 13, 2020

phil-davis commented Nov 13, 2020

refs commented Nov 23, 2020

labkode commented Nov 23, 2020 • edited Loading

butonic Nov 23, 2020

Choose a reason for hiding this comment

butonic Nov 23, 2020

Choose a reason for hiding this comment

refs Nov 24, 2020

Choose a reason for hiding this comment

refs commented Nov 24, 2020

butonic Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

phil-davis left a comment

Choose a reason for hiding this comment

ishank011 commented Nov 25, 2020

refs commented Nov 25, 2020

ishank011 left a comment

Choose a reason for hiding this comment

labkode commented Nov 23, 2020 •

edited

Loading

butonic Nov 24, 2020 •

edited

Loading