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.

Sign up for GitHub

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

Jump to bottom

fix: Filename validation should only forbid create and update #47185

Merged
merged 5 commits into from
Aug 28, 2024
Merged

fix: Filename validation should only forbid create and update #47185

merged 5 commits into from
Aug 28, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Aug 12, 2024 •
edited
Loading

Summary

  1. Fix error messages to be consistent and follow the design rules (as they are used by our mobile apps)
  2. isForbidden must only check full filename - this is used for other purposes (backwards compatibility + fix issues)
  3. DAV is directly using the storage and skips their validation, similar other places do, they and the frontend check only the permissions of the node. So we need to adjust the permissions (remove create and update) when the node is places within an invalid node.
  4. The view needs to check also the path for invalid nodes but still needs to allow read-only access on invalid names.

Screen recording

Setup - folder before enabling "Windows support":
image

Without this ("before")

Bildschirmaufnahme_20240812_182432.webm

With this ("after")

Bildschirmaufnahme_20240812_182149.webm

Checklist

@susnux susnux added this to the Nextcloud 30 milestone Aug 12, 2024
@susnux susnux changed the title Fix/filename validation fix: Filename validation should only forbid create and update Aug 12, 2024
@come-nc
Copy link
Contributor

come-nc commented Aug 13, 2024

Hum, so the New button is greyed out because the name of the folder is invalid, but how can the user understand this?
There should be an error message somewhere explaining this, no?

come-nc
come-nc reviewed Aug 13, 2024
View reviewed changes
lib/private/Files/View.php Outdated Show resolved Hide resolved
lib/private/Files/View.php Outdated Show resolved Hide resolved
lib/private/Files/View.php Outdated Show resolved Hide resolved
@susnux
Copy link
Contributor Author

susnux commented Aug 13, 2024

Hum, so the New button is greyed out because the name of the folder is invalid, but how can the user understand this?

Its greyed out because the permission does not include "CREATE". Did not change the front end here, could be done in a follow up, but fixing the permissions.

come-nc
come-nc approved these changes Aug 13, 2024
View reviewed changes
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.

Approved as after is better than before. But we should make the situation more clear in the UI in my opinion, or we’ll get confused users about why they do not have write access to the folder, and will look into ACLs and storages and file permissions before thinking about a forbidden name on a parent folder.

@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
Merged
@susnux susnux force-pushed the fix/filename-validation branch 2 times, most recently from 64a602d to fbebd84 Compare August 13, 2024 14:00
@susnux susnux force-pushed the fix/filename-validation branch 2 times, most recently from 56ca480 to 758ed89 Compare August 13, 2024 16:06
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024
@susnux
Copy link
Contributor Author

susnux commented Aug 14, 2024

/backport to stable30

@juliushaertl
Copy link
Member

Test failures seem related, looks like it widely fails on install

@susnux susnux mentioned this pull request Aug 21, 2024
Merged
4 tasks
@susnux susnux force-pushed the fix/filename-validation branch 2 times, most recently from 2d1b6a2 to 0c5f5c1 Compare August 27, 2024 00:06
@susnux susnux requested a review from skjnldsv as a code owner August 27, 2024 00:06
@susnux susnux force-pushed the fix/filename-validation branch 2 times, most recently from 95bbd4e to 43a3a6d Compare August 27, 2024 21:21
@susnux susnux added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 27, 2024
@AndyScherzinger
Copy link
Member

/compile /

@nextcloud nextcloud deleted a comment from susnux Aug 28, 2024
susnux and others added 5 commits August 28, 2024 17:22
@susnux @AndyScherzinger
fix(dav): Pre-check if node can be copied or moved in DAV files plugin
0d41c49 
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux @AndyScherzinger
fix: Renaming does not need update but delete permissions
030c209 
Renaming is basically copy + delete (a move), so no need to update permissions.
Especially if the node is in a invalid directory the node should be moveable but not editable.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux @AndyScherzinger
fix: Allow read-only filename validation to allow reading files
0883669 
Needed to read files with the "Windows compatibility" feature.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux @AndyScherzinger
fix: Also validate parent path in verifyPath
17f011f 
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@nextcloud-command @AndyScherzinger
chore(assets): Recompile assets
9321292 
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@AndyScherzinger AndyScherzinger merged commit ffde0c9 into master Aug 28, 2024
173 checks passed
@AndyScherzinger AndyScherzinger deleted the fix/filename-validation branch August 28, 2024 16:39
@backportbot backportbot bot mentioned this pull request Aug 28, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sorbaugh sorbaugh sorbaugh approved these changes

@come-nc come-nc come-nc approved these changes

@Altahrim Altahrim Awaiting requested review from Altahrim

@juliushaertl juliushaertl Awaiting requested review from juliushaertl

@artonge artonge Awaiting requested review from artonge

@skjnldsv skjnldsv Awaiting requested review from skjnldsv skjnldsv is a code owner

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: files feature: filesystem high
Projects
None yet
Milestone
Nextcloud 31
Development

Successfully merging this pull request may close these issues.

🪟📁📄 Ability to enforce windows compatible file names on the server
7 participants
@susnux @come-nc @juliushaertl @AndyScherzinger @sorbaugh @skjnldsv @nextcloud-command