Redirection to 'next' url raises an unsafe error

[pierrejeambrun]

Body

This was reported in #55143 (comment).

I appears that https://hostname.cs-us-east1-rtep.cloudshell.dev/api/v2/auth/login?next=https%3A%2F%2Fhostname.cs-us-east1-rtep.cloudshell.dev%2Fapi%2Fv2%2Fauth%2Flogin is causing an unsafe url error while it shouldn't.

I suspect this might be caused by the query param encoding.

Committer

• I acknowledge that I am a maintainer/committer of the Apache Airflow project.

[pierrejeambrun]

The scope of the resolution should be limited to our is_safe_url function.

[sjyangkevin]

I am interested in looking into it.

[xchwan]

@pierrejeambrun can I take this issue? I guess all we need to do is modify is_safe_url function.

[pierrejeambrun]

@sjyangkevin you're assigned.

I guess all we need to do is modify is_safe_url function.

Yes and update the related test to make sure that it works as expected.

@xchwan there are other issues that are up for a grab. For instance that meta issue #53040 still has some pieces un-assigned, or more generally other good first issues on the repo.

[sjyangkevin]

Hi @pierrejeambrun , and @Steexyz,

I would like to follow up here to get some feedback to see if I am on the right direction, since I wasn't able to get a failure test by using the given URL below.

https://hostname.cs-us-east1-rtep.cloudshell.dev/api/v2/auth/login?next=https%3A%2F%2Fhostname.cs-us-east1-rtep.cloudshell.dev%2Fapi%2Fv2%2Fauth%2Flogin

I also want to share the steps of how I attempted to identify the root cause, would appreciate if you could point out anything wrong below.

First, if the issue is related to the is_safe_url function, I would expect if I properly update the test and use the given URL, the unit test should raise a test failure. So, I first start airflow in breeze environment and use some loggings to understand the parameters including target_url and request. As shown below. The target_url is http://localhost:28080/ and the base url of the request is also http://localhost:28080/. The url of the request http://localhost:28080/api/v2/auth/login?next=http%3A%2F%2Flocalhost%3A28080%2F follows the same pattern as the given erroring url.

Then, I updated the test_is_safe_url to run a test for the localhost case.

• set the url and base_url for the request mock to the value shown while running airflow.
• set airflow configuration section=api and option=base_url to http://0.0.0.0:8080 (I probably should set it to None because this parameter is commented in my airflow configuration. This value is the get from the host and port).
• update the url in pytest.mark.parameterize to "http://localhost:28080/" which is the same as the target_url while running airflow.

The updated test passed. Even though there is encoding for the url in the next param, but the target_url is not encoded when being passed into the is_safe_url function.

After that, I follow the same approach to test for the given URL, and the test also passed.

Below is my airflow config for the section=api.

[api]
# instance_name =
enable_swagger_ui = True
secret_key = GI9UlscGDGy3B/h2k/2HMg==
expose_config = False
expose_stacktrace = False
# base_url =
host = 0.0.0.0
port = 8080
workers = 4
worker_timeout = 120
# log_config =
ssl_cert =
ssl_key =
maximum_page_limit = 100
fallback_page_limit = 50
access_control_allow_headers =
access_control_allow_methods =
access_control_allow_origins =
grid_view_sorting_order = topological
log_fetch_timeout_sec = 5
hide_paused_dags_by_default = False
page_size = 50
default_wrap = False
auto_refresh_interval = 3
require_confirmation_dag_change = False

Another thing probably is worth to notice that, when I access the browser debug tool. The URL in the next parameter is not encoded. But the one mentioned in #55143 (comment) is somehow encoded. I am wondering if there is anyway we could replicate the situation. If it is the case, it might be because the target_url is encoded but the request.base_url is not, and the base_url in the airflow configuration is not encoded (or not set), so it cannot be mapped to the same host.

Then, the solution probably could be to decode that URL before applying the check.

Thanks.

[xchwan]

I have use this url to test, is_safe_url recognize as true. That means the url have "next" is will not check the url after "next=". once url after "next=" is in wrong domain. it pass So, I think it still need to modify. But I'm not sure is it should be considered in this issue.