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

Prevent a server error if the name of the ImageField is not a valid cache-key #4

Merged
merged 4 commits into from
Jun 28, 2020

Conversation

sacovo
Copy link
Contributor

@sacovo sacovo commented Jun 26, 2020

If any exception occurs while accessing the cache set the url to an empty string. If the form that contains the ImageField had any validation-errors, the name of the ImageField is not cleaned and could be an invalid key for caches like Memcached.

#3

@matthiask
Copy link
Owner

Hmm, I suspect that you might not want to silently ignore cache errors. Maybe we should first try generating better cache keys?

@sacovo
Copy link
Contributor Author

sacovo commented Jun 26, 2020

Under django.utils.text there is get_valid_filename, that is also used by the file storage backend.

@sacovo sacovo changed the title Catch exception if the name of the ImageField is not a valid cache-key Prevent a server error if the name of the ImageField is not a valid cache-key Jun 26, 2020
@matthiask
Copy link
Owner

Thanks again.

I worry a bit that different images may now be mapped to the same cache key. Hashing the filename should work; it does unfortunately produce opaque cache keys but I think that's acceptable as a small downside.

Something like key = "imagefield-admin-thumb:%s" % hashlib.sha256(value.name).hexdigest() maybe? I'm sorry for the back and forth. I definitely think it's worth it fixing this bug. Thanks for your patience.

@sacovo
Copy link
Contributor Author

sacovo commented Jun 28, 2020

Thanks for your input, hashing also has the advantage that the keys will always be the same length, memchached for example has a limit of 250 bytes for a key.

@matthiask matthiask merged commit 4d09602 into matthiask:master Jun 28, 2020
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.

None yet

2 participants