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

Trust admins to upload large files #1661

Closed
markstos opened this issue Jul 5, 2022 · 1 comment
Closed

Trust admins to upload large files #1661

markstos opened this issue Jul 5, 2022 · 1 comment

Comments

@markstos
Copy link
Contributor

markstos commented Jul 5, 2022

Currently, admins are not allowed to upload files larger than 50 MB without manually modifing their Nginx config by hand to modify the client_max_body_size directive. To support the self-hosting indieweb movement, Ghost should support content creators who wish to support videos and other content larger than 50 MB, as long as this change doesn't compromise security or stablity.

History of Ghost's use of Nginx's client_max_body_size directive

The current value of 50mb was added in 2017 to support larger image and theme upload sizes via #231. At the time, there was no discussion of video. There was also no discussion security implications for tuning this value, nor for the possibility of having different values for different location blocks.

What's the ideal value for client_max_body_size ?

Ideally, this directive is to be slightly larger than the max values you'll need. If it's set too high, it can be a vector of denial of service of attacks, allowing malicious users to send a high volume of junk traffic. The tuning of this directive is discussed more here:

The best way to handle this is to keep the value low for most paths and then only allow high values for specific URLs that are protected by authentication. For Ghost, most from the public are GET requests with tiny payloads, or simple POST/PUT requests, like signing up as a member. For all those cases, a value much lower than 50 Mb would work-- 1 Mb would be sufficient.

On the other hand, a trusted admin may wish to upload a 1 GB video for self-hosting. From a security perspective, this adds no significant additional risk-- the admin is already trusted and has access to destroy all the data in the site already, trusting an admin to upload a large file doesn't really add any additional risk.

The biggest "risk" would come from the least trusted users who have the ability to upload photos and videos: "Contributors".

Recommended Changes

Nginx allows for a more nuanced approach to setting client_max_body_size because it can set in a location context as well as the server context. Ghost could use that feature to allow unlimited upload sizes to /ghost/api/admin/media/upload/, which is only accessible to trusted, authenticated users. This is the endpoint where videos get uploaded where 50 Mb is too small. A single location block declared using a regex syntax could cover all the locations where trusted users could usefully upload larger files.

At the same time, Ghost could consider dialing the 50 mb limit back down to 1mb or some lower value for the majority of endpoints that don't receive file uploads.

@ErisDS
Copy link
Member

ErisDS commented Jul 27, 2022

I think, quite simply, the client_max_body_size should be a lot higher than it currently is now that Ghost has support for video, audio and other files. I'd happily accept a PR that changes client_max_body_size to a higher number, with some ref material for what is a reasonable number - although the best ref material is prob Ghost(Pro)s own top limit, which is 1gb: https://ghost.org/pricing/

Restricting upload sizes by user role is a product issue, i.e. something that would need to be implemented into the core of Ghost rather than into Ghost-CLI / nginx. I can see the use case for it and it would likely need to be implemented using the limit service & configured via JSON config.

@ErisDS ErisDS closed this as completed in f75a3d0 Aug 1, 2022
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

No branches or pull requests

2 participants