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

[RFC] Limit thumb sizes to config presets (2.x) #28

Closed
wants to merge 1 commit into from

Conversation

GDmac
Copy link
Contributor

@GDmac GDmac commented Mar 18, 2016

This adds the option to limit the amount of image sizes that are allowed to be generated.
It looks if config thumbnails presets are set, and then limits the allowed sizes to those presets.

The presets take the following format (defined in app/config/config.yml under thumbnails)

thumbnails:
  presets:
    mythumb : { size: [ 160,  120] }
    myimage : { size: [1000,  750] }
    dynamic_height : { size: [400,  0] }

The sub-array format is choosen in anticipation of bolt/bolt#3703
which might/will allow other parameters (cropping, watermarking) and
which might/will allow usage of presets in twig templates
as parameters for record.image|thumb(preset_name)

when config thumbnail presets are present,
limit allowable sizes to presets.
@GwendolenLynch GwendolenLynch changed the title Limit thumb sizes to config presets (2.x) [RFC] Limit thumb sizes to config presets (2.x) Mar 18, 2016
@GwendolenLynch
Copy link
Contributor

Added to Tuesday's agenda

@bobdenotter
Copy link
Member

This does not include having the 'key' in the URL, like /thumbs/myimage/foo.jpg, correct?

@GDmac
Copy link
Contributor Author

GDmac commented Mar 18, 2016

Affirmative, no key in url. Only test that width-x-height size from url is found in allowed presets.
With BC fallback (allow all) if presets are not defined.

Note: preset key in url is currently NOT in the proposal as i see it !
presets can in the proposal only be used in Twig (record.image|thumb(preset_name))
and then translate to a thumb url. e.g. from above presets,
/path/image.jpg|thumb(mythumb) translates to thumbs/160x120c/path/image.jpg
(edit2: is having keys in the url a requested feature? /thumbs/myimage/foo.jpg )

@CarsonF
Copy link
Member

CarsonF commented Mar 18, 2016

This is a new feature on a stable, released branch 👎

@GDmac
Copy link
Contributor Author

GDmac commented Mar 20, 2016

IMO this is a "security" fix against unlimited thumb requests generating files.
You call it a "new feature", that is a somewhat formal approach to the issue.

@bobdenotter
Copy link
Member

It's really not a security issue. If you are legitimately worried about someone bringing down your server by requesting a ton of thumbnails, then adding this feature will not prevent that. If this is a problem for the site, use varnish, cloudflare or some other CDN to offload the images.

@GDmac GDmac mentioned this pull request Mar 20, 2016
@cdowdy
Copy link

cdowdy commented Mar 21, 2016

It's really not a security issue

@bobdenotter I agree maybe not security but definitely a ddos vector.

Really this is no different than drupals DDos vector[1] they patched ~3 years ago where on demand thumbnail generation can bring a site to a halt through so many requests, fill up the disk space, or max out its CPU.
It is also why liip imagine's bundle implemented hmac[2] like I believe it was @rossriley mentioned in the forums.

Varnish won't "fix" this. These would be dynamic urls, created on request and Varnish wouldn't know about them to cache them, because they don't exist until the request, until after they are generated.

or some other CDN to offload the images.

how does an "editor" or "frontend designer" offload thumbnails to a CDN through bolt currently?

[1] https://www.drupal.org/SA-CORE-2013-002

[2] Liip Imagine's "signer" https://github.com/liip/LiipImagineBundle/blob/master/Imagine/Cache/Signer.php#L23-L32

@GwendolenLynch
Copy link
Contributor

Closed by consensus as #not-for-2.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants