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

Underscores in file names are not sanitize on upload but on renaming #2164

Closed
qwerdee opened this issue Oct 2, 2019 · 12 comments

Comments

@qwerdee
Copy link

@qwerdee qwerdee commented Oct 2, 2019

Describe the bug
File with an underscore like "image_02.png" can be uploaded without being sanitized. When renaming them underscores are replaced with a dash.

Expected behavior
The same rules should apply to file names when uploading and renaming. So either "image_02.png" should be changed to "image-02.png" on upload or underscores should be allowed in filenames when renaming them.

Screenshots
image

Kirby Version
3.2.5

@neildaniels

This comment has been minimized.

Copy link
Contributor

@neildaniels neildaniels commented Oct 2, 2019

Sounds similar to #1738

@lukasbestle

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented Oct 3, 2019

@distantnative It looks like the fix you wrote back in May only made dots allowed but didn't change that the sanitization doesn't already happen on upload. Do you still remember why?

@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Oct 5, 2019

@lukasbestle I think back then we decided that the bug is not that no sanitization happens on upload, but that the dot should also be allowed when renaming => dots can be in the names. So the question here again is: shall underscores be part of filenames? If yes, we can add it as allowed character in the slug helper call, if not we need to fix it in the backend on upload.

@lukasbestle

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented Oct 5, 2019

I think that underscores should be allowed as there are sometimes fixed file naming schemes in companies that use underscores. Because they can be in URLs without problems, I don't see why they should be filtered out anyway.

I also think that the renaming for other invalid chars should happen on upload though. Having the names sanitized just when renaming but not on upload is pretty inconsistent.

@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Oct 5, 2019

@lukasbestle I believe they are sanitized on upload. Aren't they?

@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Oct 5, 2019

And as for a fix, I think it's as simple as adding the underscore here to the allowed characters string: https://github.com/getkirby/kirby/blob/develop/panel/src/components/Dialogs/FileRenameDialog.vue#L71

@lukasbestle

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented Oct 5, 2019

I believe they are sanitized on upload. Aren't they?

Isn't that exactly the bug that has been reported in this issue? :D

@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Oct 5, 2019

The issue is reporting that underscores are replaced during rename but not on upload. We just said that underscores should be allowed. So then upload works fine and only rename is the problem (underscore has to be added as allowed character at file rename). All wanted sanitization should happen during upload to my knowledge.

@lukasbestle

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented Oct 6, 2019

Well, but if the behavior for underscores currently differs between upload and rename, that tells us that the sanitization code for upload and rename is not shared, but either different or no sanitization happens on upload at all.

Even if we fix the underscore issue, this issue will remain for other not allowed chars.

@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Oct 6, 2019

It’s pretty easy actually: on upload the backend sanitizes the name, renames are handled by the JS slug helper. I wouldn’t change that behavior. But we have to identify where they differ, underscore is one thing where they currently do. So let’s add that one as allowed character on rename? Or what is your larger argument?

@lukasbestle

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented Oct 6, 2019

That‘s what I meant. We should make sure that the frontend and backend code share the same rules of what is allowed and what isn‘t.

@bastianallgeier

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Oct 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.