Re: [PR] Handle relative paths when sanitizing URLs [airflow]
potiuk commented on PR #41995: URL: https://github.com/apache/airflow/pull/41995#issuecomment-2556989150 > I think I missed backporting it, somehow. I will investigate more. :/ Yeah. Could be also some tooling issue of ours :(. I think we shoudl give an extra scrutiny for 2.10.5 (with @kaxil on board as well). This is actually the problem with release management tools, that when they work, we start trusting them and stop checking and validating things, bue it's pretty natural that some refactorings introduce some new cases - including edge cases, so once in a while we should pretty manually verify all the steps of release process, otherwise we might miss something. Having this check for 2.10.5 might be a good idea to do it in preparation for 3.0.0 (though there we will have to do extra-extra-scrutiny as there will be MANY NEW THINGS). At least if we find and fix some things in 2.10.5 we will have less to do when 3.0.0 comes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
utkarsharma2 commented on PR #41995: URL: https://github.com/apache/airflow/pull/41995#issuecomment-2556973010 > But it would be great to find what happened :) I think I missed backporting it, somehow. :/ will investigate more. Thanks for the backporting it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
potiuk commented on PR #41995: URL: https://github.com/apache/airflow/pull/41995#issuecomment-2554458054 But it would be great to find what happened :) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
potiuk commented on PR #41995: URL: https://github.com/apache/airflow/pull/41995#issuecomment-2554456705 I changed milestone to 2.10.5 and will cherry-pick it for 2.10.5 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
potiuk commented on PR #41995: URL: https://github.com/apache/airflow/pull/41995#issuecomment-2554453967 > I am on 2.10.3 and this is still happening, on checking it looks like the fix is not in the releases, the code is visible in the main branch but not in the tags 2.10.4 / 2.10.3 Interesting: @utkarsharma2 -> it's marked with 2.10.2 milestone, but yes it seems it's not cherry-picked there. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
gpadavala commented on PR #41995: URL: https://github.com/apache/airflow/pull/41995#issuecomment-2554358777 The fix is not in the release, it seems to be in main branch but not in the tags 2.10.4 / 2.10.3 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
potiuk commented on PR #41995: URL: https://github.com/apache/airflow/pull/41995#issuecomment-2553386763 It is already released in 2.10.2 - have you checked it @mfatemipour ? Can you please verify it in release notes and upgrade/ test locally and confirm that it works? If not, open a new issue describing the problem you have. Thanks for your help in advance. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
mfatemipour commented on PR #41995: URL: https://github.com/apache/airflow/pull/41995#issuecomment-2553032777 Hi, Could we have this in 2.10 ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
mfatemipour commented on code in PR #41995:
URL: https://github.com/apache/airflow/pull/41995#discussion_r1863739240
##
airflow/www/static/js/dag/details/taskInstance/ExtraLinks.tsx:
##
@@ -57,8 +57,15 @@ const ExtraLinks = ({
if (!url) {
return true;
}
-const urlRegex = /^(https?:)/i;
-return urlRegex.test(url);
+const path = new URL(url, "http://localhost";);
Review Comment:
@utkarsharma2
I believe we should use the value from AIRFLOW__WEBSERVER__BASE_URL in
configs, instead of localhost, and this change needs to be backported to
version 2.10
Of course I know this change is already merged, I will create a PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
mfatemipour commented on code in PR #41995:
URL: https://github.com/apache/airflow/pull/41995#discussion_r1863739240
##
airflow/www/static/js/dag/details/taskInstance/ExtraLinks.tsx:
##
@@ -57,8 +57,15 @@ const ExtraLinks = ({
if (!url) {
return true;
}
-const urlRegex = /^(https?:)/i;
-return urlRegex.test(url);
+const path = new URL(url, "http://localhost";);
Review Comment:
@utkarsharma2
I believe we should use the value from AIRFLOW__WEBSERVER__BASE_URL in
configs, instead of localhost, and this change needs to be backported to
version 2.10
Of course I know this change is already merged, I will create a PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
mfatemipour commented on code in PR #41995:
URL: https://github.com/apache/airflow/pull/41995#discussion_r1863739240
##
airflow/www/static/js/dag/details/taskInstance/ExtraLinks.tsx:
##
@@ -57,8 +57,15 @@ const ExtraLinks = ({
if (!url) {
return true;
}
-const urlRegex = /^(https?:)/i;
-return urlRegex.test(url);
+const path = new URL(url, "http://localhost";);
Review Comment:
@utkarsharma2
I believe we should use the value from AIRFLOW__WEBSERVER__BASE_URL in
configs, instead of localhost, and this change needs to be backported to
version 2.10
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
mfatemipour commented on code in PR #41995:
URL: https://github.com/apache/airflow/pull/41995#discussion_r1863739240
##
airflow/www/static/js/dag/details/taskInstance/ExtraLinks.tsx:
##
@@ -57,8 +57,15 @@ const ExtraLinks = ({
if (!url) {
return true;
}
-const urlRegex = /^(https?:)/i;
-return urlRegex.test(url);
+const path = new URL(url, "http://localhost";);
Review Comment:
@utkarsharma2
I believe we should use the value from AIRFLOW__WEBSERVER__BASE_URL instead
of localhost, and this change needs to be backported to version 2.10
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
potiuk commented on PR #41995: URL: https://github.com/apache/airflow/pull/41995#issuecomment-2332169416 I marked it so - but this was I think what we agreed on, that it is not going to cancel rc1 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
eladkal commented on PR #41995: URL: https://github.com/apache/airflow/pull/41995#issuecomment-2332004702 @utkarsharma2 This will target 2.10.2? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
potiuk commented on code in PR #41995:
URL: https://github.com/apache/airflow/pull/41995#discussion_r1744115305
##
airflow/www/static/js/dag/details/taskInstance/ExtraLinks.tsx:
##
@@ -57,8 +57,15 @@ const ExtraLinks = ({
if (!url) {
return true;
}
-const urlRegex = /^(https?:)/i;
-return urlRegex.test(url);
+const path = new URL(url, "http://localhost";);
Review Comment:
Yeah. I think so @bbovenzi ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
utkarsharma2 commented on code in PR #41995:
URL: https://github.com/apache/airflow/pull/41995#discussion_r1744093827
##
airflow/www/static/js/dag/details/taskInstance/ExtraLinks.tsx:
##
@@ -57,8 +57,15 @@ const ExtraLinks = ({
if (!url) {
return true;
}
-const urlRegex = /^(https?:)/i;
-return urlRegex.test(url);
+const path = new URL(url, "http://localhost";);
+// The URL creation will succeed for
+// 1. Absolute/Relative URL
Review Comment:
Yes, I have updated it. PTAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
potiuk commented on code in PR #41995:
URL: https://github.com/apache/airflow/pull/41995#discussion_r1744074896
##
airflow/www/static/js/dag/details/taskInstance/ExtraLinks.tsx:
##
@@ -57,8 +57,15 @@ const ExtraLinks = ({
if (!url) {
return true;
}
-const urlRegex = /^(https?:)/i;
-return urlRegex.test(url);
+const path = new URL(url, "http://localhost";);
Review Comment:
(and we are just doing this to parse the UIRL and see if it's "correct")
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
utkarsharma2 commented on code in PR #41995:
URL: https://github.com/apache/airflow/pull/41995#discussion_r1744075302
##
airflow/www/static/js/dag/details/taskInstance/ExtraLinks.tsx:
##
@@ -57,8 +57,15 @@ const ExtraLinks = ({
if (!url) {
return true;
}
-const urlRegex = /^(https?:)/i;
-return urlRegex.test(url);
+const path = new URL(url, "http://localhost";);
Review Comment:
I think it should be fine without 8080 since we are just validating the URL,
right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
potiuk commented on code in PR #41995:
URL: https://github.com/apache/airflow/pull/41995#discussion_r1744074896
##
airflow/www/static/js/dag/details/taskInstance/ExtraLinks.tsx:
##
@@ -57,8 +57,15 @@ const ExtraLinks = ({
if (!url) {
return true;
}
-const urlRegex = /^(https?:)/i;
-return urlRegex.test(url);
+const path = new URL(url, "http://localhost";);
Review Comment:
(and we are just doing this to parse the UIRL and see if it's "correct" and
produces http protocol
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
bbovenzi commented on code in PR #41995:
URL: https://github.com/apache/airflow/pull/41995#discussion_r1744050012
##
airflow/www/static/js/dag/details/taskInstance/ExtraLinks.tsx:
##
@@ -57,8 +57,15 @@ const ExtraLinks = ({
if (!url) {
return true;
}
-const urlRegex = /^(https?:)/i;
-return urlRegex.test(url);
+const path = new URL(url, "http://localhost";);
Review Comment:
Exactly, it would have to be something like `http://localhost:8080/test`to
be valid
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
potiuk commented on code in PR #41995:
URL: https://github.com/apache/airflow/pull/41995#discussion_r1744045430
##
airflow/www/static/js/dag/details/taskInstance/ExtraLinks.tsx:
##
@@ -57,8 +57,15 @@ const ExtraLinks = ({
if (!url) {
return true;
}
-const urlRegex = /^(https?:)/i;
-return urlRegex.test(url);
+const path = new URL(url, "http://localhost";);
Review Comment:
https://github.com/user-attachments/assets/0745749f-5391-45f5-90d3-cc16e30e5f6a";>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
potiuk commented on code in PR #41995:
URL: https://github.com/apache/airflow/pull/41995#discussion_r1744040814
##
airflow/www/static/js/dag/details/taskInstance/ExtraLinks.tsx:
##
@@ -57,8 +57,15 @@ const ExtraLinks = ({
if (!url) {
return true;
}
-const urlRegex = /^(https?:)/i;
-return urlRegex.test(url);
+const path = new URL(url, "http://localhost";);
Review Comment:
Hmm. I think this won't work for absolute URLs now
```
URL("http://www.apache.org";, "http://localhost";)b x xx
```
-> `Uncaught TypeError: Failed to construct 'URL': Please use the 'new'
operator, this DOM object constructor cannot be called as a function.
at :1:1`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
potiuk commented on code in PR #41995:
URL: https://github.com/apache/airflow/pull/41995#discussion_r1744040814
##
airflow/www/static/js/dag/details/taskInstance/ExtraLinks.tsx:
##
@@ -57,8 +57,15 @@ const ExtraLinks = ({
if (!url) {
return true;
}
-const urlRegex = /^(https?:)/i;
-return urlRegex.test(url);
+const path = new URL(url, "http://localhost";);
Review Comment:
Hmm. I think this won't work for absolute URLs now
```
URL("http://www.apache.org";, "http://localhost";)
```
-> `URL("http://www.apache.org";, "http://localhost";)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
utkarsharma2 commented on code in PR #41995:
URL: https://github.com/apache/airflow/pull/41995#discussion_r1744019178
##
airflow/www/static/js/dag/details/taskInstance/ExtraLinks.tsx:
##
@@ -57,8 +57,27 @@ const ExtraLinks = ({
if (!url) {
return true;
}
-const urlRegex = /^(https?:)/i;
-return urlRegex.test(url);
+try {
+ const path = new URL(url);
+ // The URL creation will succeed for
+ // 1. Absolute URL
+ // 2. URL with javascript:()
+ // URL(url) will fail for relative paths.
+ // eslint-disable-next-line
+ if (path.protocol === "javascript:") {
+return false; // javascript:() is not allowed
+ }
+ if (path.protocol === "http:" || path.protocol === "https:") {
+return true; // Absolute URL are allowed
+ }
+} catch (e: any) {
+ // Relative URL are handled here
+ const path = new URL(url, "http://localhost";);
+ if (path.protocol === "http:") {
Review Comment:
Correct, updated the code, accordingly. PTAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
potiuk commented on PR #41995: URL: https://github.com/apache/airflow/pull/41995#issuecomment-2329221409 Not sure if we have some tests for this one ? @pierrejeambrun @bbovenzi ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Handle relative paths when sanitizing URLs [airflow]
potiuk commented on code in PR #41995:
URL: https://github.com/apache/airflow/pull/41995#discussion_r1743910036
##
airflow/www/static/js/dag/details/taskInstance/ExtraLinks.tsx:
##
@@ -57,8 +57,27 @@ const ExtraLinks = ({
if (!url) {
return true;
}
-const urlRegex = /^(https?:)/i;
-return urlRegex.test(url);
+try {
+ const path = new URL(url);
+ // The URL creation will succeed for
+ // 1. Absolute URL
+ // 2. URL with javascript:()
+ // URL(url) will fail for relative paths.
+ // eslint-disable-next-line
+ if (path.protocol === "javascript:") {
Review Comment:
We likely don't need to check it here - since only http/https are allowed
below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
