Skip to content

Conversation

@xpgcrx
Copy link
Contributor

@xpgcrx xpgcrx commented Dec 24, 2023

Add flask config to limit the allowed payload.
closes: #35196

I set it to limit to 1MB.
Before and after adding this config, I tried sending a string larger than 1MB from the Edit User page.
Before adding the config, it was possible to send; after adding, I confirmed that the attached error page was received.

request_entity_too_large_2023-12-24 14 35 41

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Dec 24, 2023
@xpgcrx xpgcrx marked this pull request as ready for review December 24, 2023 06:16
@eladkal eladkal changed the title Add flask config: MAX_CONTENT_LENGTH (#35196) Add flask config: MAX_CONTENT_LENGTH Dec 24, 2023
Copy link
Contributor

@dirrao dirrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about triggering a DAG with input configuration? Does it require more than 1MB? If we move this configuration to the web server section in airflow cfg, then we can set based on our requirements. For user profile information, is it feasible to restrict the size of form fields rather than relying on the defaults?

@xpgcrx
Copy link
Contributor Author

xpgcrx commented Dec 24, 2023

Thank you for your comment.

How about triggering a DAG with input configuration?

Sorry, what does this mean?

Does it require more than 1MB?

I'm not really sure what would be an appropriate value. Do you have any ideas?

If we move this configuration to the web server section in airflow cfg, then we can set based on our requirements.

Does this mean instead of hardcoding the value, we can set an item in airflow.cfg and get that value as a flask config?

For user profile information, is it feasible to restrict the size of form fields rather than relying on the defaults?

Certainly, I feel it's feasible. Sorry, could you please clarify what you mean by "rather than relying on the defaults"?

@dirrao
Copy link
Contributor

dirrao commented Dec 24, 2023

Thank you for your comment.

How about triggering a DAG with input configuration?

Sorry, what does this mean?

Does it require more than 1MB?

I'm not really sure what would be an appropriate value. Do you have any ideas?

We can pass the JSON payload while triggering the DAG from the Airflow UI. JSON payload can be arbitrary length. It is much bigger. I haven't come across a use case where we pass more than 1MB. However, I am just checking.

If we move this configuration to the web server section in airflow cfg, then we can set based on our requirements.

Does this mean instead of hardcoding the value, we can set an item in airflow.cfg and get that value as a flask config?

Trying to see if can avoid hardcoding in the code.

For user profile information, is it feasible to restrict the size of form fields rather than relying on the defaults?

Certainly, I feel it's feasible. Sorry, could you please clarify what you mean by "rather than relying on the defaults"?

I mean relying on the same default value for multiple forms in the Airflow UI.

@xpgcrx xpgcrx force-pushed the limit-allowed-payload branch from afc81e8 to cd38156 Compare December 26, 2023 13:17
@potiuk
Copy link
Member

potiuk commented Dec 30, 2023

@hussein-awala ?

@eladkal eladkal added this to the Airflow 2.9.0 milestone Dec 30, 2023
@eladkal eladkal added the type:improvement Changelog: Improvements label Dec 30, 2023
@potiuk potiuk modified the milestones: Airflow 2.9.0, Airflow 2.8.1 Dec 30, 2023
Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was checking to see if it accepts float or if we need to round the calculated value, but it looks like Flask implicitly does that.

LGTM, just a small nit, IMHO allowed_payload_size is a better name.

@eladkal eladkal added type:bug-fix Changelog: Bug Fixes and removed type:improvement Changelog: Improvements labels Dec 30, 2023
@xpgcrx xpgcrx force-pushed the limit-allowed-payload branch from a1e667e to eb324e7 Compare December 30, 2023 13:12
Copy link
Contributor

@dirrao dirrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hussein-awala hussein-awala merged commit 84063e7 into apache:main Dec 30, 2023
ephraimbuddy pushed a commit that referenced this pull request Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:webserver Webserver related Issues type:bug-fix Changelog: Bug Fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A long user name in "edit user" can break the webserver

5 participants