Add a check for trailing slash in webserver base_url #31833

hussein-awala · 2023-06-10T14:32:04Z

^ 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.

Signed-off-by: Hussein Awala <hussein@awala.fr>

pierrejeambrun · 2023-06-10T14:50:47Z

airflow/models/taskinstance.py

@@ -758,7 +758,7 @@ def generate_command(
    def log_url(self) -> str:
        """Log URL for TaskInstance."""
        iso = quote(self.execution_date.isoformat())
-        base_url = conf.get_mandatory_value("webserver", "BASE_URL")
+        base_url = conf.get_mandatory_value("webserver", "BASE_URL").rstrip("/")


I feel like this could lead to some errors in the future.

It's hard for someone to guess that retrieving the base_url from the config is a special case and needs to strip trailing '/'. (I can easily imagine someone doing conf.get_mandatory_value("webserver", "BASE_URL") to access the value)

Maybe we can put that in the config accessor ? (So we don't need to bother, the equivalent of @validates/@pre_process for marshmallow field, that does some preprocesssing on the value before storing it)

Otherwise looking good

I will check how can we do that for a specific config

uranusjr · 2023-06-12T07:06:20Z

Two thoughts

Should we just use urljoin instead of raw string formatting to handle the slashes instead?
If we want to eliminate the possibility, I’d prefer an startup-time check to ensure the config is set without a trailing slash. If a trailing slash is found, Airflow can refuse to launch and prompt the user to correct the value.

hussein-awala · 2023-06-17T15:20:57Z

Should we just use urljoin instead of raw string formatting to handle the slashes instead?

it's possible for the changes in airflow/models/taskinstance.py but not for the one in airflow/www/extensions/init_wsgi_middlewares.py since we don't join it to something else, but we provide it as an argument to a 3rd party lib.

If we want to eliminate the possibility, I’d prefer an startup-time check to ensure the config is set without a trailing slash. If a trailing slash is found, Airflow can refuse to launch and prompt the user to correct the value.

I will try this one, and I don't think it's a braking change since it doesn't work with a trailing slash anyway

Signed-off-by: Hussein Awala <hussein@awala.fr>

hussein-awala · 2023-06-17T19:47:15Z

@pierrejeambrun @uranusjr could you recheck it please?

pierrejeambrun

This is a bugfix but could break some user configs by preventing webservers from starting.

Should we go with a warning first before actually refusing such configuration ?

(No strong opinion, not sure how we should handle this actually)

hussein-awala · 2023-06-18T15:20:18Z

@pierrejeambrun As I mentioned in my previous comment:

I don't think it's a breaking change since it doesn't work with a trailing slash anyway.

Therefore, if users utilize a base_url with a trailing slash, they will encounter an issue, and comprehending the cause might not be straightforward. IMO, raising an exception is a secure approach since it does not break any existing functionality, but it just explain the problem reason. WDYT?

pierrejeambrun

Sorry I missed that in your comment.

I did a few tests, looking good

airflow/www/extensions/init_wsgi_middlewares.py

Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>

* Remove right trailing / from webserver base_url Signed-off-by: Hussein Awala <hussein@awala.fr> * use url join instead of removing trailing slash Signed-off-by: Hussein Awala <hussein@awala.fr> * raise an exception when base_url contains a trailing slash Signed-off-by: Hussein Awala <hussein@awala.fr> * Update airflow/www/extensions/init_wsgi_middlewares.py Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com> --------- Signed-off-by: Hussein Awala <hussein@awala.fr> Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com> (cherry picked from commit fe4a6c8)

pankajkoti · 2023-08-01T09:38:10Z

airflow/www/extensions/init_wsgi_middlewares.py

@@ -37,8 +38,11 @@ def _root_app(env: WSGIEnvironment, resp: StartResponse) -> Iterable[bytes]:

 def init_wsgi_middleware(flask_app: Flask) -> None:
    """Handle X-Forwarded-* headers and base_url support."""
+    webserver_base_url = conf.get_mandatory_value("webserver", "BASE_URL", fallback="")
+    if webserver_base_url.endswith("/"):
+        raise AirflowConfigException("webserver.base_url conf cannot have a trailing slash.")


I am wondering here that why can the base URL not have a trailing /?

This is due to how web servers rewrite URLs when you deploy a webapp under a prefix. Say you deploy under https://mydomain/airflow. When you visit https://mydomain/airflow/home in the browser (as an example; any client applies), the proxy server (e.g. NGINX) would strip the prefix, and forward the rest of the URL to the app, so in Python code (e.g. Airflow) we can just handle /home without needing to consider what prefix the entirety of Airflow is deployed under.

Now you may say, hey, why can’t either the proxy server, the gateway protocol (WSGI in Python’s case), or the web framework (Flask for Airflow’s case) be smarter and just strip or add the slash in between as appropriate? And you would be right! Some of them actually can do this (NGINX has merge_slashes, for example), but not everyone does since the slash is technically significant and does change the URL’s meaning (even if that meaning can be nonsensical in the prefix), and that extra check is not free. So it’s better to avoid fighting the tools and just stick to the technically correct configuration.

okay, I am coming from here on how the python urllib.parse.urljoin handles joining base URL and relative URLs.

Wouldn't https://my.astronomer.run/path qualify as a valid base URL? But looks like the urljoin just ignores the path when it does not end with a trailing /. If it was https://my.astronomer.run/path/, urljoin does not strip the path and makes the join as expected.

I think we can handle more cases if users need it. This PR was a quick fix to prevent circular redirections.

I’m sure we can make this work with a trailing slash but requires a bit more work.

(Sorry, forgot to reply.)

Python’s urljoin implements the logic in <a href="..."> tags; I don’t recall the correct term for this, but it is the URL resolution logic when you click on links in a browser. URLs have two kinds, folder and file (not the right terms, but easier to understand). The URL would not have a trailing slash if you are in a file view; the trailing slash indicates a folder view. When you’re in a file, a relative path like foo indicates another file in the same folder. When you’re in a folder view, however, foo means the foo entry in the folder. This means the resolution logic would change depending on whether the URL has a trailing slash or not.

Note that Python’s local path joining logic (both pathlib and os.path) is different and does not consider the trailing slash; it instead matches the common logic used to join format in shell scripts.

But the prefix is an entirely different thing, and is intended to only be joined with simple string concatenation and skips all the folder/file logic because it is not semantically possible to support combining the prefix with absolute paths and relative paths. The only proper way to prepend a prefix is prefix + path (or other string concatenation methods such as f"{prefix}{path}", of course).

understood, thank you @uranusjr for the detailed explanation and propsoal.

It is observed that urljoin is not yielding expected results for the task instance's log_url which needs to be a concatenation of the webserver base_url and specified relative url. The current usage of urljoin does not seem to be the right way to achieve this based on what urljoin is meant for and how it works. So, we use simple string concatenation to yield the desired result. More context in the comment apache#31833 (comment) closes: apache#32996

It is observed that urljoin is not yielding expected results for the task instance's log_url which needs to be a concatenation of the webserver base_url and specified relative url. The current usage of urljoin does not seem to be the right way to achieve this based on what urljoin is meant for and how it works. So, we use simple string concatenation to yield the desired result. More context in the comment #31833 (comment) closes: #32996

It is observed that urljoin is not yielding expected results for the task instance's log_url which needs to be a concatenation of the webserver base_url and specified relative url. The current usage of urljoin does not seem to be the right way to achieve this based on what urljoin is meant for and how it works. So, we use simple string concatenation to yield the desired result. More context in the comment #31833 (comment) closes: #32996 (cherry picked from commit baa1bc0)

It is observed that urljoin is not yielding expected results for the task instance's log_url which needs to be a concatenation of the webserver base_url and specified relative url. The current usage of urljoin does not seem to be the right way to achieve this based on what urljoin is meant for and how it works. So, we use simple string concatenation to yield the desired result. More context in the comment apache/airflow#31833 (comment) closes: #32996 (cherry picked from commit baa1bc0438baa05d358b236eec3c343438d8d53c) GitOrigin-RevId: 868d1389461822cf5d54faaff3ce19913fc7f08e

Remove right trailing / from webserver base_url

601649c

Signed-off-by: Hussein Awala <hussein@awala.fr>

hussein-awala requested review from ryanahamilton, ashb, bbovenzi, pierrejeambrun, kaxil and XD-DENG as code owners June 10, 2023 14:32

boring-cyborg bot added the area:webserver Webserver related Issues label Jun 10, 2023

hussein-awala added the type:bug-fix Changelog: Bug Fixes label Jun 10, 2023

hussein-awala added this to the Airlfow 2.6.3 milestone Jun 10, 2023

pierrejeambrun reviewed Jun 10, 2023

View reviewed changes

hussein-awala added 2 commits June 17, 2023 17:22

use url join instead of removing trailing slash

0480977

Signed-off-by: Hussein Awala <hussein@awala.fr>

raise an exception when base_url contains a trailing slash

e75175f

Signed-off-by: Hussein Awala <hussein@awala.fr>

hussein-awala requested a review from pierrejeambrun June 17, 2023 19:46

hussein-awala requested a review from uranusjr June 17, 2023 19:47

pierrejeambrun reviewed Jun 18, 2023

View reviewed changes

pierrejeambrun approved these changes Jun 18, 2023

View reviewed changes

uranusjr reviewed Jun 19, 2023

View reviewed changes

airflow/www/extensions/init_wsgi_middlewares.py Outdated Show resolved Hide resolved

Update airflow/www/extensions/init_wsgi_middlewares.py

f697a33

Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>

uranusjr approved these changes Jun 19, 2023

View reviewed changes

hussein-awala changed the title ~~Remove right trailing / from webserver base_url~~ Add a check for trailing slash in webserver base_url Jun 19, 2023

hussein-awala merged commit fe4a6c8 into apache:main Jun 19, 2023
42 checks passed

ephraimbuddy mentioned this pull request Jul 7, 2023

Status of testing of Apache Airflow 2.6.3rc1 #32432

Closed

53 tasks

wolfier mentioned this pull request Aug 1, 2023

Task instance log_url is overwrites existing path in base_url #32996

Closed

2 tasks

pankajkoti reviewed Aug 1, 2023

View reviewed changes

uranusjr mentioned this pull request Aug 3, 2023

Add a utility to safely join URLs using urllib.parse.urljoin #33062

Closed

pankajkoti mentioned this pull request Aug 3, 2023

Use string concatenation to prefix base URL for task instance's log_url #33063

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a check for trailing slash in webserver base_url #31833

Add a check for trailing slash in webserver base_url #31833

hussein-awala commented Jun 10, 2023

pierrejeambrun Jun 10, 2023 •

edited

hussein-awala Jun 17, 2023

uranusjr commented Jun 12, 2023

hussein-awala commented Jun 17, 2023

hussein-awala commented Jun 17, 2023

pierrejeambrun left a comment •

edited

hussein-awala commented Jun 18, 2023

pierrejeambrun left a comment

pankajkoti Aug 1, 2023 •

edited

uranusjr Aug 1, 2023 •

edited

pankajkoti Aug 1, 2023 •

edited

pierrejeambrun Aug 1, 2023

uranusjr Aug 3, 2023

pankajkoti Aug 3, 2023 •

edited

Add a check for trailing slash in webserver base_url #31833

Add a check for trailing slash in webserver base_url #31833

Conversation

hussein-awala commented Jun 10, 2023

pierrejeambrun Jun 10, 2023 • edited

Choose a reason for hiding this comment

hussein-awala Jun 17, 2023

Choose a reason for hiding this comment

uranusjr commented Jun 12, 2023

hussein-awala commented Jun 17, 2023

hussein-awala commented Jun 17, 2023

pierrejeambrun left a comment • edited

Choose a reason for hiding this comment

hussein-awala commented Jun 18, 2023

pierrejeambrun left a comment

Choose a reason for hiding this comment

pankajkoti Aug 1, 2023 • edited

Choose a reason for hiding this comment

uranusjr Aug 1, 2023 • edited

Choose a reason for hiding this comment

pankajkoti Aug 1, 2023 • edited

Choose a reason for hiding this comment

pierrejeambrun Aug 1, 2023

Choose a reason for hiding this comment

uranusjr Aug 3, 2023

Choose a reason for hiding this comment

pankajkoti Aug 3, 2023 • edited

Choose a reason for hiding this comment

pierrejeambrun Jun 10, 2023 •

edited

pierrejeambrun left a comment •

edited

pankajkoti Aug 1, 2023 •

edited

uranusjr Aug 1, 2023 •

edited

pankajkoti Aug 1, 2023 •

edited

pankajkoti Aug 3, 2023 •

edited