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

feat: sanitize media filenames according to global slug setting #3315

Merged
merged 1 commit into from
Feb 25, 2020

Conversation

stefk
Copy link
Contributor

@stefk stefk commented Feb 24, 2020

Currently, users are allowed to upload images whose filename contains accents, spaces or special characters. This may cause several problems downstream (#857).

It seems that at some point, the work done in #1135 should have fixed that issue (see #857 (comment)) but I can't find any trace of that behaviour in the current codebase.

This PR adds/restores that feature by applying the policy defined in the global slug setting to media inserted via the default media library.

@stefk stefk requested a review from a team February 24, 2020 17:57
@erezrokah erezrokah self-requested a review February 24, 2020 18:54
@erezrokah
Copy link
Contributor

Thanks @stefk, this does look like a regression.

@stefk stefk force-pushed the skl/media-name-encoding branch 3 times, most recently from c6a9031 to c28d42f Compare February 25, 2020 10:26
@erezrokah
Copy link
Contributor

Not sure why tests are failing on your branch, I will look into it soon

@stefk stefk force-pushed the skl/media-name-encoding branch from c28d42f to 6c22e92 Compare February 25, 2020 15:53
@stefk
Copy link
Contributor Author

stefk commented Feb 25, 2020

Thanks @erezrokah. I was also puzzled by the test failure, considering everything was green yesterday and that I had just amended a commit this morning with unrelated changes. Then I noticed that the failure was on editorial_workflow_migration_spec_github_backend.js, which wasn't present in my branch (it comes from the recent #3316). Rebasing solved the build issue, but do you have any idea how was this situation possible in the first place? Why was a spec from the master branch executed on a distinct branch?

@erezrokah
Copy link
Contributor

erezrokah commented Feb 25, 2020

Thanks @erezrokah. I was also puzzled by the test failure, considering everything was green yesterday and that I had just amended a commit this morning with unrelated changes. Then I noticed that the failure was on editorial_workflow_migration_spec_github_backend.js, which wasn't present in my branch (it comes from the recent #3316). Rebasing solved the build issue, but do you have any idea how was this situation possible in first place? Why was a spec from the master branch executed on a distinct branch?

That is a new test and I think it was a bit flaky (hopefully I fixed it).
We actually reverted that PR and re-introduced it so I think you got caught in the middle somewhere.

@stefk stefk force-pushed the skl/media-name-encoding branch 2 times, most recently from 5d2ea78 to 40b0a7a Compare February 25, 2020 17:20
@erezrokah erezrokah merged commit 8874769 into decaporg:master Feb 25, 2020
vladdu pushed a commit to vladdu/netlify-cms that referenced this pull request Jan 26, 2021
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.

2 participants