-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
Handle invalid parameters (including missing object) in webserver #8171
Comments
@mik-laj Hi, what is your expected fix for this issue? Are you looking for something like this?
Regarding tests, is a URL like |
It should be something similar. The most important thing is that no mushrooms appear, but user-readable error messages. For example: However, if you go to the link: This should be standardized and a clear message should always be displayed to the user. |
For clarity. This contribution does not have to solve all problems in one PR. I will be happy even if one problem is solved. And another person will be able to create more changes and solve more problems. List of all routes
|
Redirects the user back to the main page if an invalid DAG id is provided to the /rendered page instead of crashing to improve the overall user experience. Signed-off-by: Remy Suen <remy.suen@gmail.com>
This is a user experience problem, but it is also a security problem. If we see similar messages, it means that we haven't verified enough input data. Data validation is the basic method of protecting against other serious attacks from the "Injection" family e.g. SQL Injection. Input validation should happen as early as possible in the data flow, preferably as soon as the data is received from the client. However, we do not have any validation for many parameters. |
Hi @mik-laj As discussed let me put my hands on this. Let me know if there's something you would want to provide additional info. |
@2796gaurav We already have one draft. Please read it and comments, and then you will be able to start working on this change. |
Yes sure! |
I also invite you to read our guides: |
@2796gaurav I unassigned you from this ticket so that the next contributor can start working. If you want to continue working, let me know and I will assign you again. |
I wonder if type checks can be used to catch these kinds of issues. There are a lot of # airflow/www/app.py
if TYPE_CHECKING:
from airflow.models.dagbag import DagBag
from airflow.www.security import AirflowSecurityManager
class _AirflowAppBuilder(Protocol):
sm: AirflowSecurityManager
...
class _CurrentApp(Protocol):
dag_bag: DagBag
appbuilder: _AirflowAppBuilder
...
from flask import current_app as _current_app
current_app = cast("_CurrentApp", _current_app) And then all code can import this instead of directly from Flask to be type checked. We can alternatively supply a type stub |
This issue seems to be resolved in the latest version. The following links, given in the original issue, return a reasonable error and valid page without going "nuclear": http://localhost:28080/tries?dag_id=example_automl_text_sentiment2&days=30 |
Description
Hi. The webserver code is very optimistic and negative paths are not checked.
For example:
We have the following code:
airflow/airflow/www/views.py
Lines 595 to 597 in 0dafdd0
It is not checked here whether the DAG object exists. Condition
dag == None
should be added and when it is met, error 404 should be reported.Use case / motivation
Improving the experience of using the webserver and reducing the number of nukulars.
I hope that a thorough review of the entire web server code and completing the tests with negative paths will improve the overall health of the webserver.
If this is done by the Polidea team, it will be an opportunity to get to know the webserver better. My team has not focused on the webserver yet.
it will be an opportunity to find other health problems (e.g. side-effect, missing tests).
Related Issues
N/A
The text was updated successfully, but these errors were encountered: