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

Correctly look up filename when logged in but inaccessible #48295

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

toad
Copy link

@toad toad commented Sep 23, 2024

Fixes a forms bug by changing how files_versions gets the path for a Node to tolerate the forms app writing files that are not even visible to the logged-in user.

In FileEventsListener::getPathForNode(), if the user is not logged in, and we can't find the file owner, we guess the file owner from the path. Most likely the username in the path has some access to the file, so we can find the filename. This allows writing files when the user is not logged in, for instance where we are filling in a form from a public link.

However, in the case where there is a logged-in user, and the file is on a group share, both the username and the owner will be the same UID. Which is fine as long as we have access to the file.

But in the case where we are actually writing a spreadsheet update for a form submission, the logged in user may not have access to the file. However, it is still a legitimate write by the forms app, so it is safe to use the owner in the path, just as if we were logged out.

I'm not 100% sure about the security implications here and request review. However it provides a simple workaround for our use case:

  • Form with an attached spreadsheet
  • User is logged in
  • User has access to the form but not the spreadsheet
  • The spreadsheet is in a group folder
  • The versioning app is enabled

The result is an error every time, see the forms bug nextcloud/forms#2067 (note that the actual error has changed since the initial report!)

The alternative is to fix the underlying problem in the forms app, which would be a much bigger diff, see the above.

However, if I am right about the security model here, this is a safe workaround, and may actually be correct.

Summary

Fixes a reproducible form submission error due to a conflict between how forms and versioning see permissions.

At this stage I'm asking for review that this is basically the right solution and there are no security implications. I believe it is safe but request review on that assumption!

An alternative fix in forms would be much more work, see their bug.

TODO

  • Confirm that the fix is safe before proceeding with the rest

Checklist

If the user is not logged in, and we can't find the file owner, we guess
the file owner from the path. Most likely the username in the path has
some access to the file, so we can find the filename.

However, in the case where we are logged in, and the file is a group
share, both the username and the owner will be the same UID. Which is
fine as long as we have access to the file.

But in the case where we are actually writing a spreadsheet update for a
form submission, the logged in user may not have access to the file.
However, it is still a legitimate write by the forms app, so it is safe
to use the owner in the path, just as if we were logged out.

I'm not 100% sure about the security implications here and request
review. However it provides a simple workaround for our use case:
- Form with an attached spreadsheet
- User is logged in
- User has access to the form but not the spreadsheet
- The spreadsheet is in a group folder
- The versioning app is enabled

The alternative is to fix the underlying problem in the forms app, which
would be a much bigger diff. See the forms bug here:
nextcloud/forms#2067

However, if I am right about the security model here, this is a safe
workaround, and may actually be correct.

Signed-off-by: Matthew Toseland <matthew@toselandcs.co.uk>
@toad toad force-pushed the mjt-forms-versions-error-forms-bug-2067 branch from 0d7674a to e92817d Compare September 23, 2024 12:16
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

This does not look like something we should do, or not this way.

Do you have a stack trace for the permission error in forms app?
Why does the files write pass but not the versions write 🤔

The issue is specific to groupfolders where there is no owner so getOwner can return a user with no write permission I think. Maybe that’s the root problem to solve, not sure.

@come-nc
Copy link
Contributor

come-nc commented Sep 23, 2024

@icewind1991 @artonge I’m interested in your input on this.
The most clear explanation is in nextcloud/forms#2067 (comment)

Part of the problem is that files_version needs a path to work with, and the event contains a Node instance, and the logic to extract a path from the node fails.
Part of the problem is also that groupfolders return the current user as owner for a node in a groupfolder the user has access to, as a groupfolder has no owner. But if the user has readonly access to the folder, getOwner returns a user which has no write permission on the folder.

It’s not clear to me if the fix for this lies in groupfolders or files_version, or a bit of both.

@toad
Copy link
Author

toad commented Sep 23, 2024

In the error case I was debugging, it's actually in files_exists. If the user has read-only access to the file then it will work. If the user does not have access to the file at all, it fails. Which is not unreasonable - as they said earlier in the ticket, ideally Forms should use a worker thread with its own user. On the other hand I have to assume that running without files_versions doesn't create a massive security hole? Is files_versions actually enforcing security constraints here, or is it just crashing when something legal but confusing happens? Given that it allows you to write files when you're not even logged in, I have to assume the latter.

Anyway, the stack trace is:

{"file":"/var/www/html/lib/private/Files/View.php","line":531,"function":"basicOperation","class":"OC\\Files\\View","type":"->","args":["file_exists",null]},
{"file":"/var/www/html/lib/private/Files/Filesystem.php","line":546,"function":"file_exists","class":"OC\\Files\\View","type":"->","args":[null]},
{"file":"/var/www/html/apps/files_versions/lib/Storage.php","line":191,"function":"file_exists","class":"OC\\Files\\Filesystem","type":"::","args":[null]},
{"file":"/var/www/html/apps/files_versions/lib/Listener/FileEventsListener.php","line":197,"function":"store","class":"OCA\\Files_Versions\\Storage","type":"::","args":[null]},
{"file":"/var/www/html/apps/files_versions/lib/Listener/FileEventsListener.php","line":103,"function":"write_hook","class":"OCA\\Files_Versions\\Listener\\FileEventsListener","type":"->","args":[["OC\\Files\\Node\\File"]]},
{"file":"/var/www/html/lib/private/EventDispatcher/ServiceEventListener.php","line":86,"function":"handle","class":"OCA\\Files_Versions\\Listener\\FileEventsListener","type":"->","args":[["OCP\\Files\\Events\\Node\\BeforeNodeWrittenEvent"]]},
{"file":"/var/www/html/3rdparty/symfony/event-dispatcher/EventDispatcher.php","line":230,"function":"__invoke","class":"OC\\EventDispatcher\\ServiceEventListener","type":"->","args":[["OCP\\Files\\Events\\Node\\BeforeNodeWrittenEvent"],"OCP\\Files\\Events\\Node\\BeforeNodeWrittenEvent",["Symfony\\Component\\EventDispatcher\\EventDispatcher"]]},
{"file":"/var/www/html/3rdparty/symfony/event-dispatcher/EventDispatcher.php","line":59,"function":"callListeners","class":"Symfony\\Component\\EventDispatcher\\EventDispatcher","type":"->","args":[[["Closure"],["Closure"]],"OCP\\Files\\Events\\Node\\BeforeNodeWrittenEvent",["OCP\\Files\\Events\\Node\\BeforeNodeWrittenEvent"]]},
{"file":"/var/www/html/lib/private/EventDispatcher/EventDispatcher.php","line":86,"function":"dispatch","class":"Symfony\\Component\\EventDispatcher\\EventDispatcher","type":"->","args":[["OCP\\Files\\Events\\Node\\BeforeNodeWrittenEvent"],"OCP\\Files\\Events\\Node\\BeforeNodeWrittenEvent"]},
{"file":"/var/www/html/lib/private/EventDispatcher/EventDispatcher.php","line":98,"function":"dispatch","class":"OC\\EventDispatcher\\EventDispatcher","type":"->","args":["OCP\\Files\\Events\\Node\\BeforeNodeWrittenEvent",["OCP\\Files\\Events\\Node\\BeforeNodeWrittenEvent"]]},
{"file":"/var/www/html/lib/private/Files/Node/HookConnector.php","line":93,"function":"dispatchTyped","class":"OC\\EventDispatcher\\EventDispatcher","type":"->","args":[["OCP\\Files\\Events\\Node\\BeforeNodeWrittenEvent"]]},
{"file":"/var/www/html/lib/private/legacy/OC_Hook.php","line":105,"function":"write","class":"OC\\Files\\Node\\HookConnector","type":"->","args":[[true,"/uk tech/test/Yet another test form (responses).xlsx"]]},
{"file":"/var/www/html/lib/private/Files/View.php","line":1282,"function":"emit","class":"OC_Hook","type":"::","args":["OC_Filesystem","write",[true,"/uk tech/test/Yet another test form (responses).xlsx"]]},
{"file":"/var/www/html/lib/private/Files/View.php","line":1154,"function":"runHooks","class":"OC\\Files\\View","type":"->","args":[["update","write"],"/uk tech/test/Yet another test form (responses).xlsx"]},
{"file":"/var/www/html/lib/private/Files/View.php","line":683,"function":"basicOperation","class":"OC\\Files\\View","type":"->","args":["file_put_contents","/matthewtoseland/files/uk tech/test/Yet another test form (responses).xlsx",["update","write"],null]},
{"file":"/var/www/html/lib/private/Files/Node/File.php","line":73,"function":"file_put_contents","class":"OC\\Files\\View","type":"->","args":["/matthewtoseland/files/uk tech/test/Yet another test form (responses).xlsx",null]},
{"file":"/var/www/html/custom_apps/forms/lib/Service/SubmissionService.php","line":224,"function":"putContent","class":"OC\\Files\\Node\\File","type":"->","args":[null]},
{"file":"/var/www/html/custom_apps/forms/lib/Controller/ApiController.php","line":1145,"function":"writeFileToCloud","class":"OCA\\Forms\\Service\\SubmissionService","type":"->","args":[["OCA\\Forms\\Db\\Form",11],"/uk tech/test/Yet another test form (responses).xlsx","xlsx","matthewtoseland"]},
{"file":"/var/www/html/lib/private/AppFramework/Http/Dispatcher.php","line":232,"function":"insertSubmission","class":"OCA\\Forms\\Controller\\ApiController","type":"->","args":[11,[["42"],["44"],["Test test text"]],""]},
{"file":"/var/www/html/lib/private/AppFramework/Http/Dispatcher.php","line":138,"function":"executeController","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->","args":[["OCA\\Forms\\Controller\\ApiController"],"insertSubmission"]},
{"file":"/var/www/html/lib/private/AppFramework/App.php","line":184,"function":"dispatch","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->","args":[["OCA\\Forms\\Controller\\ApiController"],"insertSubmission"]},
{"file":"/var/www/html/lib/private/Route/Router.php","line":331,"function":"main","class":"OC\\AppFramework\\App","type":"::","args":["OCA\\Forms\\Controller\\ApiController","insertSubmission",["OC\\AppFramework\\DependencyInjection\\DIContainer"],["v2.4","ocs.forms.api.insertsubmission"]]},
{"file":"/var/www/html/ocs/v1.php","line":66,"function":"match","class":"OC\\Route\\Router","type":"->","args":["/ocsapp/apps/forms/api/v2.4/submission/insert"]},
{"file":"/var/www/html/ocs/v2.php","line":23,"args":["/var/www/html/ocs/v1.php"],"function":"require_once"}]

,"File":"/var/www/html/lib/private/Files/View.php","Line":1138},"message":"OC\\Files\\View::basicOperation(): Argument #2 ($path) must be of type string, null given, called in /var/www/html/lib/private/Files/View.php on line 531 in file '/var/www/html/lib/private/Files/View.php' line 1138","exception":{},"CustomMessage":"OC\\Files\\View::basicOperation(): Argument #2 ($path) must be of type string, null given, called in /var/www/html/lib/private/Files/View.php on line 531 in file '/var/www/html/lib/private/Files/View.php' line 1138"}}

@Chartman123
Copy link

@toad We'll investigate further in Forms together with @come-nc if there's some problem in getting the correct path/Node. So perhaps there will be a real fix instead of doing some kind of workaround :)

@solracsf solracsf marked this pull request as draft September 24, 2024 06:11
@artonge
Copy link
Contributor

artonge commented Sep 25, 2024

The getPathForNode method is only here to bridge between the "modern" event parameters providing Node objects, and the legacy Storage class which uses paths. So there is indeed no security implication, the goal is only to have a path so that Storage can do its thing.

There might be a better way to implement getPathForNode, but couldn't find one at the time, and the method ended up in the weird owner hunt that it currently is. So any additional weirdness will not be surprising, and probably shouldn't be rejected :).

EDIT: Also happy to have a better way to get the path. @icewind1991 might know.

@toad
Copy link
Author

toad commented Sep 27, 2024

The getPathForNode method is only here to bridge between the "modern" event parameters providing Node objects, and the legacy Storage class which uses paths. So there is indeed no security implication, the goal is only to have a path so that Storage can do its thing.

There might be a better way to implement getPathForNode, but couldn't find one at the time, and the method ended up in the weird owner hunt that it currently is. So any additional weirdness will not be surprising, and probably shouldn't be rejected :).

EDIT: Also happy to have a better way to get the path. @icewind1991 might know.

So that Storage can do what exactly? As I understand it, permissions checks apply at a higher level. Versions does not replace higher level permissions checks, it just hooks in after they've taken place, creates some metadata for the new version (which will be written to disk anyway), and presumably prevents the old one being deleted?

So it's safe to deploy this as a temporary fix, even though it's probably not the right fix?

Copy link
Contributor

github-actions bot commented Oct 8, 2024

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@sorbaugh
Copy link
Contributor

Hi @toad , from what I read it seems the the preferred approach here is to migrate from hooks to events for this part in forms. We'd rather not merge something temporary as that risks committing less attention to the real source of the issue, and we'd rather work on that. Nevertheless your PR brought our attention to this issue, so thank you very much for that!

@toad
Copy link
Author

toad commented Oct 11, 2024

Hi @toad , from what I read it seems the the preferred approach here is to migrate from hooks to events for this part in forms. We'd rather not merge something temporary as that risks committing less attention to the real source of the issue, and we'd rather work on that. Nevertheless your PR brought our attention to this issue, so thank you very much for that!

Is there a public ticket I can follow to monitor the status of the correct fix? Thanks!

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

Successfully merging this pull request may close these issues.

Failed to export Submissions: Not allowed to write to file
6 participants