Correctly look up filename when logged in but inaccessible

[toad]

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 https://github.com/nextcloud/forms/issues/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.

• Resolves: forms bug #2067

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

• Code is properly formatted
• Sign-off message is added to all commits
• [ ] Tests (unit, integration, api and/or acceptance) are included
• [ ] Screenshots before/after for front-end changes
• [x] Documentation (manuals or wiki) has been updated or is not required
• [ ] Backports requested where applicable (ex: critical bugfixes)

[come-nc]

@icewind1991 @artonge I’m interested in your input on this. The most clear explanation is in https://github.com/nextcloud/forms/issues/2067#issuecomment-2366985992

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]

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]

@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 :)

[artonge]

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]

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?

[github-actions[bot]]

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]

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]

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!