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

openforms: opt in nginx config client_max_body_size #64

Open
sjoerdie opened this issue Jul 4, 2023 · 5 comments
Open

openforms: opt in nginx config client_max_body_size #64

sjoerdie opened this issue Jul 4, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@sjoerdie
Copy link
Collaborator

sjoerdie commented Jul 4, 2023

There are now 2 (3 if you count ingress) places to configure max upload:

Application:

settings.maxFileUpload

nginx:

nginx.config.clientMaxBodySize

This is confusing, the default values are also not the same.

Change nginx config to opt in.

@sjoerdie sjoerdie added the enhancement New feature or request label Jul 4, 2023
@sergei-maertens
Copy link
Member

we're doing this for security reasons, setting a large allowed max body size for the entire server opens the API/application up to Denial of Service attacks where people push a lot of data to the endpoints which needs to be parsed/processed.

For the upload endpoint, this is deliberately set to a higher value to allow larger uploads for UX reasons. The value passed to the application must be the same from the server, since this is displayed as a help text to form designers (they can also specify maximum attachment file size limits on the file field, but can get confused if they specify a value higher than configured on the server).

iirc the application and nginx use slightly different syntaxes to express file size (20M vs 20mb or something like that?), which made re-using the same variable not as straight forward, but I may be confusing this with Open Zaak.

@sjoerdie
Copy link
Collaborator Author

we're doing this for security reasons, setting a large allowed max body size for the entire server opens the API/application up to Denial of Service attacks where people push a lot of data to the endpoints which needs to be parsed/processed.

For the upload endpoint, this is deliberately set to a higher value to allow larger uploads for UX reasons. The value passed to the application must be the same from the server, since this is displayed as a help text to form designers (they can also specify maximum attachment file size limits on the file field, but can get confused if they specify a value higher than configured on the server).

iirc the application and nginx use slightly different syntaxes to express file size (20M vs 20mb or something like that?), which made re-using the same variable not as straight forward, but I may be confusing this with Open Zaak.

Thank you for clarifying this.

The opt in was not an option anyway I realized later, since nginx will default to its 1M.

Some organization would prefer to handle it by there ingress / load balancers, but they can set it to 0M in the values (Setting size to 0 disables checking of client request body size.).

The syntax is the same (much appreciated) and settings.Maxfileupload: 50M is already used for both the ENV var MAX_FILE_UPLOAD_SIZE for the application and for the nginx endpoint:

      location = /api/v1/formio/fileupload {
        client_max_body_size {{ .Values.settings.maxFileUpload }};
        include conf.d/proxy;

Correct me if i'm wrong, with the current defaults:

nginx.config.clientMaxBodySize: 10M
settings.maxFileUpload: 50M

Users are still able to upload files to a max of 50M?

@sergei-maertens
Copy link
Member

yes, that should be the case. However, that v1 end the endpoint is catching my attention - we're on v2 of the API so maybe that's causing issues? Do you also have a /api/v2/formio/fileupload rule?

@sjoerdie
Copy link
Collaborator Author

I do not.

I will however then replace the v1 endpoint and release a new chart version.

Latest chart version is:

type: application
version: 1.2.1
appVersion: 2.0.5

So technically this is correct, since 2.0.5 is still using the v1 endpoint.

@sergei-maertens
Copy link
Member

sergei-maertens commented Oct 12, 2023

I don't remember the specifics - there was some deprecation going on in there, but since OF v2.x the API endpoints major version was changed too from v1 -> v2. Major version of the application and API stay in sync, see also: https://open-forms.readthedocs.io/en/stable/developers/versioning.html

To be 100% safe, I'd just list them both (or use a regex to target /api/v(1|2)/formio/fileupload)

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

No branches or pull requests

2 participants