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

[Bugfix]: 'forbidden_chars' added the condition to drop file #43468

Conversation

arublov
Copy link
Member

@arublov arublov commented Feb 8, 2024

Summary

TODO

  • ...

Checklist

Andrii Rublov and others added 2 commits February 8, 2024 21:25
Signed-off-by: Andrii Rublov <airublev@outlook.com>
…rop-file

Signed-off-by: Andrii Rublov <39839367+arublov@users.noreply.github.com>
toCheck.forEach(char => {
if (forbiddenCharacters.indexOf(char) !== -1) {
showError(t('files', t('files', '"{char}" is not allowed inside a file name.', { char })))
throw '#.. is not allowed inside a file name.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Throwing a string is nothing we should do, instead a proper Error should be used.

Suggested change
throw '#.. is not allowed inside a file name.'
throw new Error(`${char} is not allowed inside a file name.`).

@@ -67,6 +70,14 @@ export const handleDrop = async (data: DataTransfer): Promise<Upload[]> => {
const handleFileUpload = async (file: File, path: string = '') => {
const uploader = getUploader()

const toCheck = file.name.split('')
toCheck.forEach(char => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be the wrong way and probably performance wise pretty bad, instead I would do something like:

const forbidden = forbiddenCharacters.split('')
if (forbidden.some((char) => file.name.includes(char))) {
// ...
}

why? because file name is probably longer than the list of forbidden chars so we reduce the outer loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

@susnux thank you for your comments. Yesterday, I submitted that pull request as a draft because I'm still working on it and it's not the final version yet. Today, I'll have the final version for this task. Still, thank you for your comments.

susnux and others added 9 commits February 9, 2024 22:49
…ud/vue` dependency

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
Signed-off-by: Nextcloud bot <bot@nextcloud.com>
Signed-off-by: Simon L <szaimen@e.mail.de>
Signed-off-by: Simon L <szaimen@e.mail.de>
It is not allowed to use slashes within path parameters, so they would need to be encoded.
But URL encoded slashes are not suported by Apache, so instead replace slash with space.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
…L entities in their names

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
…or uploadPicker

Signed-off-by: Andrii Rublov <airublev@outlook.com>
@arublov arublov closed this Feb 9, 2024
@arublov arublov deleted the fix/forbidden-chars-added-condition-to-drop-file branch February 9, 2024 21:51
arublov pushed a commit to arublov/server that referenced this pull request Feb 9, 2024
…p file and added forbiddenCharacters to UploadPicker

Signed-off-by: Andrii Rublov <airublev@outlook.com>
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.

5 participants