Failed to export Submissions: Not allowed to write to file #2067
Comments
chriscroome
added
0. Needs triage
|
Did you restart your php processes? Are you using php-fpm? We had lots of errors in the past after upgrades due to cached code... |
Could you please provide the full object from the log? Including the error message? |
|
@Chartman123 PHP-FPM had been restarted. @susnux here is a full log entry, all I have changed is the IP address and I also ran it through {
"reqId": "oLRtH9XtF8rwtUiZidOT",
"level": 2,
"time": "2024-04-15T19:08:20+00:00",
"remoteAddr": "X.X.X.X",
"user": "--",
"app": "forms",
"method": "POST",
"url": "/ocs/v2.php/apps/forms/api/v2.4/submission/insert",
"message": "Failed to export Submissions: Not allowed to write to file",
"userAgent": "Mozilla/5.0 (X11; Linux x86_64; rv:126.0) Gecko/20100101 Firefox/126.0",
"version": "28.0.4.1",
"data": {
"app": "forms"
}
} |
|
Thank you, but @chriscroome what I meant is from you browser console there should be the network request that fails, could you please provide the response from the server? But for you there should be a more meaningful error message |
|
This is what I have after exporting it and changing the domain name: [ERROR] forms: Error while submitting the form
Object { app: "forms", level: 2, error: {…} }
NcSettingsSection-NSrCZ23G-6nKKlVz9.mjs:2:337806
value https://cloud.example.com/apps/forms/js/NcSettingsSection-NSrCZ23G-6nKKlVz9.mjs:2
value https://cloud.example.com/apps/forms/js/NcSettingsSection-NSrCZ23G-6nKKlVz9.mjs:2
onConfirmedSubmit https://cloud.example.com/apps/forms/js/Submit-PS6lxYc-.mjs:2Is that what you need? |
|
I have also found And this is from the Apache error log: |
|
I could reproduce this on the instance, but there is not much helpful information for further debugging. Where is the file located you want to export submissions to? A groupfolder? Local storage? Is the file or parent folder shared? |
|
I didn't set the form up so I don't know the answers to these questions I'm afraid but I have asked our client if they know the answers. I think I have found the |
|
The issue is still present on the instance mentioned by chriscroome. I was able to make a new test form that leads to the same error message "There was an error submitting the form" whenever a submission is attempted by a user (except the owner of the form, for whom there is not such message). If no spreadsheet is created, or if it is created in the form owner's personal space, no such issue was noticed. |
|
@mariomorvan thanks for sharing this information with us. Are those "normal" shared folders or group folders or something else? |
@Chartman123 thanks for looking into this. Normal share as far as I understand the different kind of shares (we do not have installed group folders) |
|
the logs show the following message associated with today's submission errors: |
|
@mariomorvan @susnux I could reproduce it this way:
What's strange to me is that I don't get the original warning/error message but an error 500. Perhaps something else changed in the meantime. Heres the server logfile attached that I could see: Logfile |
Chartman123
added
1. to develop
|
Strangely I received a from @Chartman123 saying:
However it doesn't appear above 🤷 @mariomorvan is the admin on the Nextcloud instance where this was an issue -- @mariomorvan I have set off a job to update your Nextcloud to version 29.0.5, when it has completed could you test it again to see if this issue is resolved? |
|
Yeah, I deleted my comment again after I found that in my test scenario the file wasn't linked at all... so of course no error message. It still happens in the latest NC versions |
|
I think this is the same error as this one: #2165 Should be doable for 4.3 |
|
We have this issue on Nextcloud v28.0.9 and v29. The spreadsheet is in a group folder to which the logged in user filling in the form does not have access. (Our cloud is set up so people only have access to shared folders - their personal quota is 0.) If the user is not logged in, it works fine. You need to attempt to write to the spreadsheet as the user who owns the form, even if it is being filled in by another user. The attempt to submit is a POST to https:///ocs/v2.php/apps/forms/api/v2.4/submission/insert which returns 500 The Nextcloud log entry says: |
|
forms-bug.permissions-hack.clean.diff.txt What is actually happening here is an interaction between several apps:
However, FileEventListener is part of files_versions; it's not doing any meaningful security check here. In particular, if the user is logged out, it checks the path for a username, and uses that. So if we patch FileEventListener to do the same thing when $user == $owner, it will correctly look up the filename against its real owner, and file_exists() returns true. As I understand it, permissions checks have already been done by the time we get here; files_versions is just hooking into that process, creating version records, and presumably preventing old versions being deleted. And the forms app has already bypassed the permissions checks somehow; as discussed above, it'd be better to update the spreadsheet on a background thread, but right now it works (and implementing that is more work than I'm prepared to do for a workaround since you'll be doing it next year). So my guess is that my patch is safe (though there are some fishy things around it). But I'm not very sure about this! If we can use this, it'd solve some immediate problems and we would not have to wait until next year. Thoughts? |
|
@toad thank you very much for digging into this... If you want you can open a pull request on the server repository (where files_versions is developed) and see what the maintainers think about it. :) |
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.
|
I've created a pull request for a shorter version of the patch: nextcloud/server#48295 Note that this is against server, so I'm not sure whether I need a server ticket? I haven't implemented tests yet; if my assumptions about the security implications are wrong then the whole thing is the wrong approach. |
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>
That’s not incorrect I think, since this owner does have write access to the file. Can you output |
|
Hmmm, I think this has evolved. Filling in from a public link does work for us - because of the owner guessing, I assume. |
|
According to my logging, in the failing case (where the user is logged in and doesn't have access to the spreadsheet which is on a group share), $node->getOwner() was the logged in user's ID. I think this comes from getFileInfo() and ultimately from GroupFolderStorage::getOwner(). Meanwhile node->getPath() is the full path, from the original owner, i.e. "/matthewtoseland/files/uk tech/test/test3 folder/forms_answers/Yet another test form (responses).xlsx" Which is why guessing the owner from the path works (both in this case with my patch, and in the logged out case). |
Addressed in #2337 |
|
HookConnector::write() calls getNodeForPath(). The passed in path is relative at this point ("/uk tech/test/Yet another test form (responses).xlsx"). That calls Filesystem::getView()->getFileInfo() on path. Which calls Files::Storage::Wrapper::getOwner($path), and we eventually end up in GorupFolderStorage::getOwner(), which returns the current logged in user (since it's a group storage). Which means the Node's passed in info sets the owner to the currently logged in user. Even though it has the correct absolute path which starts with the real owner. |
|
Confirmed. The node has path = "/matthewtoseland/files/uk tech/test/test3 folder/New form title (responses).xlsx" and owner = "matthew-test3". |
|
Is there a realistic alternative here? Maybe GroupFolderStorage could check whether the logged in user has access? Or alternatively, is forms really doing something dodgy here? "The logged in user has access to any group share we interact with" seems like a reasonable invariant. Does it holds outside of forms? That line of reasoning means you'd need the originally planned "write updates to the spreadsheet on another thread" change. :( My main concern personally/professionally is whether the workaround patch is safe. We might just turn off versioning to get around it though. |
Only if the user is not logged in. If the user is logged in, the owner ID is the logged in user's ID, because it's on a group share. Which is why I consider my workaround basically reasonable (it's not doing anything more dangerous than a path that already exists), but I'm not confident enough to put it on a live system; we'll probably shut down versioning instead. As for the logging you asked for see my other comments. |
|
In fact, if the user is not logged in, $node->getOwner()?->getUid() is null. Which is why we need to guess the ID from the full path. |
|
Why did you mention HookConnector, the code path goes through a hook? This may be the problem, if the node is serialized to a path which is then turned into a Node again it may lose owner information. If the ownerId in https://github.com/nextcloud/forms/blob/main/lib/Service/SubmissionService.php#L148-L152 is the logged in user id and not the form owner then it’s a problem in forms. It should not try to write in the file from a user without write permission. If not, then it’s either a problem with groupfolders or with the hook/event system. My guess is groupfolders was designed without permissions and this owner situation was never triggered before but the problem is in groupfolders. |
|
Please keep in mind that is not only related to groupfolders but also to normal shares. On my dev instance I don't use groupfolders at all. |
|
@Chartman123 Are you saying it breaks with normal shares? I don't see how it would: if it doesn't work with the user, it checks the owner, and for a normal share, that will work. (But I can't test this right now because all of our files are in group folders) We may be conflating two different bugs with different causes here; the first case (user is logged out) appeared to have been fixed before we started getting the new error message (argument 2 must be of type string). |
|
@come-nc So you're saying forms needs to be rewritten to write spreadsheet updates on a background thread? That's a much bigger change, when can we realistically expect it? If it's going to take months then I need to know whether my patch is relatively safe, or whether we just need to turn off either versioning or forms for now. :( |
|
So the fact that forms works when there is no user logged in (without forking a separate thread with a different logged in user) shows that it already abuses the permissions model? |
|
And yes, it goes through a hook. This is the hook that files_versions uses to create a new revision during a write callback. Which creates a node with the original user's full path, but the logged in user's UID. FileEventsListener then gets a dodgy node. But FileEventsListener already has to workaround odd-looking Node's, because the Node might have a full path but no owner (this happens in the "no user logged in" / public share case). Hence the 1-line workaround to do the same thing in the case where the owner and user UIDs are the same due to it being a group share. But if no other NextCloud code abuses the permissions system like Forms is doing, then the fault is in Forms I guess? |
|
What @susnux said above - "We run on system permissions" - suggests that what Forms is doing - writing to files that the logged in user does not have access to, without having to fork a worker thread and change the logged in user - is perfectly valid? In which case AFAICS the options are:
|
I did not get an answer to this I think. But it matters a bit less if the hook loses owner information anyway I guess? |
I'll get this piece of information this evening :) |
|
A relative path is passed into writeFileToCloud, but what it passes through is the correct full path including the owning user. So at least in the group share case it looks like the the owner must be correct at that point. But it's not passed on into the Node. The Node owner is in the fileInfo, which HookConnector::getNodeForPath() gets from View::getFileInfo(), which gets it from Storage. The owner returned from GroupFolderStorage (always the logged in user) is clearly not helpful for system writes. Is it possible to pass the ownership in as an argument to e.g. write()? We already pass the path in. But that would mean passing it through putContent() etc. Or is it important to get it from Storage? Ownership is a property of the file, not the path. In which case, can we just make GroupFolderStorage return null if it's not a share the current logged in user has access to? Does getOwner() returning null cause a problem elsewhere? It already returns null if we're filling in a form from a public share, which works. |
Please use the 👍 reaction to show that you are affected by the same issue. Please don't comment if you have no relevant information to add!
Describe the bug
A Nextcloud instance that did have a working Forms app now generates an error on form submission.
To Reproduce
Steps to reproduce the behaviour:
Expected behavior
A successfully submission.
Nextcloud (please complete the following information):
Desktop (please complete the following information):
Smartphone (please complete the following information):
Browser log
Nextcloud log
The following is written to the Nextcloud log:
Server logs
There is nothing in the Apache or PHP-FPM logs.
TMPDIR
The
tempdirectoryis writable.Additional context
The error that is produced appears to be here:
forms/lib/Service/SubmissionService.php
Line 226 in 1ba3a14
The change to the server (the forms were working prior to this change), was updating from Debian Bullseye to Debian Bookworm, the instance uses PHP-FPM 8.3.
I have tried to duplicate this on another server with the same configuration but haven't been able to.
The text was updated successfully, but these errors were encountered: