Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do not leak existing spaces #3300

Merged
merged 10 commits into from
Oct 17, 2022

Conversation

butonic
Copy link
Contributor

@butonic butonic commented Sep 30, 2022

We are now returning a not found error for more requests to not leak existence of spaces for users that do not have access to resources.

I stopped for now, because I noticed that for modifying requests we now have to differentiate between existing resources and missing write permissions. It would require an additional stat call in error cases. Something I'd like to prevent.

Furthermore, the ocdav service should not really change status codes IMO.

If you want to expose permission errors, implement a storage provider that does that. If you don't went to leak space / resource existence, do that.

if we only prevent leaking existence in ocdav the CS3 api still knows that ... so ... yeah, this should be done by storage drivers and ocdav should just transparently handle that.

Ironically, the spaces registry originally had no way of finding a space a user had no access to ...

This might actually require touching all three services. I'll dig deeper next week.

And obviously this will freak out the testsuite ... but I agree with @phil-davis oc10 should return 404 as well.

Fixes owncloud/ocis#3561

@lgtm-com
Copy link

lgtm-com bot commented Oct 5, 2022

This pull request fixes 4 alerts when merging 69ad276 into 2dfa067 - view on LGTM.com

fixed alerts:

  • 4 for Uncontrolled data used in path expression

@lgtm-com
Copy link

lgtm-com bot commented Oct 5, 2022

This pull request fixes 4 alerts when merging b7d6cc5 into 2dfa067 - view on LGTM.com

fixed alerts:

  • 4 for Uncontrolled data used in path expression

@butonic
Copy link
Contributor Author

butonic commented Oct 5, 2022

the only failing tests are the ones I changed in owncloud/core#40406 althoug I fear I may have to duplicate them and split between ocis & oc10 ... correct @phil-davis ?

@butonic butonic force-pushed the not-found-for-permission-denied branch from eccbcad to 380378b Compare October 13, 2022 08:26
@lgtm-com
Copy link

lgtm-com bot commented Oct 13, 2022

This pull request fixes 4 alerts when merging 380378b into 11cc78a - view on LGTM.com

fixed alerts:

  • 4 for Uncontrolled data used in path expression

@butonic butonic force-pushed the not-found-for-permission-denied branch from 380378b to a2d2e6f Compare October 13, 2022 09:20
@butonic
Copy link
Contributor Author

butonic commented Oct 13, 2022

hm @aduffeck @dragotin, I ran into a concurrent create dir error in https://drone.cernbox.cern.ch/cs3org/reva/9181/5/3:

• [FAILED] [0.896 seconds]
Decomposed
/drone/src/pkg/storage/utils/decomposedfs/decomposedfs_concurrency_test.go:36
  concurrent
  /drone/src/pkg/storage/utils/decomposedfs/decomposedfs_concurrency_test.go:53
    CreateDir
    /drone/src/pkg/storage/utils/decomposedfs/decomposedfs_concurrency_test.go:93
      [It] handles already existing directories
      /drone/src/pkg/storage/utils/decomposedfs/decomposedfs_concurrency_test.go:100

  Unexpected error:
      <*errors.withStack | 0xc00000ec00>: {
          error: <*errors.withMessage | 0xc0008d05a0>{
              cause: <*errors.errorString | 0xc000769fb0>{
                  s: "unable to acquire a lock on the file",
              },
              msg: "xattrs: Unable to lock file for read",
          },
          stack: [0xd4253a, 0xd5107c, 0xd51668, 0xe044b3, 0xdea7a6, 0xf80838, 0x49ee61],
      }
      xattrs: Unable to lock file for read: unable to acquire a lock on the file
  occurred
  In [It] at: /drone/src/pkg/storage/utils/decomposedfs/decomposedfs_concurrency_test.go:115
------------------------------


Summarizing 1 Failure:
  [FAIL] Decomposed concurrent CreateDir [It] handles already existing directories
  /drone/src/pkg/storage/utils/decomposedfs/decomposedfs_concurrency_test.go:115

again in https://drone.cernbox.cern.ch/cs3org/reva/9207/4/2

@lgtm-com
Copy link

lgtm-com bot commented Oct 13, 2022

This pull request fixes 4 alerts when merging a2d2e6f into 11cc78a - view on LGTM.com

fixed alerts:

  • 4 for Uncontrolled data used in path expression

@butonic butonic force-pushed the not-found-for-permission-denied branch from a2d2e6f to 8d7431c Compare October 13, 2022 09:52
@lgtm-com
Copy link

lgtm-com bot commented Oct 13, 2022

This pull request fixes 4 alerts when merging 8d7431c into 11cc78a - view on LGTM.com

fixed alerts:

  • 4 for Uncontrolled data used in path expression

@lgtm-com
Copy link

lgtm-com bot commented Oct 13, 2022

This pull request fixes 4 alerts when merging c682b55 into 11cc78a - view on LGTM.com

fixed alerts:

  • 4 for Uncontrolled data used in path expression

@lgtm-com
Copy link

lgtm-com bot commented Oct 13, 2022

This pull request fixes 4 alerts when merging 1150199 into 560ba92 - view on LGTM.com

fixed alerts:

  • 4 for Uncontrolled data used in path expression

@lgtm-com
Copy link

lgtm-com bot commented Oct 13, 2022

This pull request fixes 4 alerts when merging 03290c8 into 560ba92 - view on LGTM.com

fixed alerts:

  • 4 for Uncontrolled data used in path expression

@aduffeck
Copy link
Contributor

hm @aduffeck @dragotin, I ran into a concurrent create dir error in https://drone.cernbox.cern.ch/cs3org/reva/9181/5/3:

ugh.... looks like a different issue than the one we fixed recently, that was about numerical result out of range, not xattrs: Unable to lock file for read.

@butonic butonic force-pushed the not-found-for-permission-denied branch from 03290c8 to c9675e3 Compare October 14, 2022 08:01
@lgtm-com
Copy link

lgtm-com bot commented Oct 14, 2022

This pull request fixes 4 alerts when merging c9675e3 into fe227ef - view on LGTM.com

fixed alerts:

  • 4 for Uncontrolled data used in path expression

@lgtm-com
Copy link

lgtm-com bot commented Oct 14, 2022

This pull request fixes 4 alerts when merging 87f6d32 into fe227ef - view on LGTM.com

fixed alerts:

  • 4 for Uncontrolled data used in path expression

@butonic butonic marked this pull request as ready for review October 14, 2022 10:04
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@butonic butonic force-pushed the not-found-for-permission-denied branch from 87f6d32 to 18a6eba Compare October 14, 2022 14:08
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@lgtm-com
Copy link

lgtm-com bot commented Oct 14, 2022

This pull request fixes 4 alerts when merging b6524fe into 9567525 - view on LGTM.com

fixed alerts:

  • 4 for Uncontrolled data used in path expression

Copy link
Member

@micbar micbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, big change. Could not test all the edge cases but the adapted unit and API tests are good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants