Skip to content
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 relative paths when sanitizing URLs #41995

Merged
merged 5 commits into from
Sep 4, 2024

Conversation

utkarsharma2
Copy link
Contributor

@utkarsharma2 utkarsharma2 commented Sep 4, 2024

In the initial PR(#41665) we didn't handle the relative path in URL which led to issue(#41977). This PR aims at handling the relative path case when sanitizing URLs

The issue is fixed with current change:
Screenshot 2024-09-04 at 6 53 13 PM

In the initial PR(apache#41665) we didn't handle the relative path in URL which led to issue(apache#41977). This PR aims at handling the relative path case when sanitizing URLs
@boring-cyborg boring-cyborg bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Sep 4, 2024
@utkarsharma2
Copy link
Contributor Author

Not entirely sure where to add test cases, if someone can point me to it?

@potiuk
Copy link
Member

potiuk commented Sep 4, 2024

Not sure if we have some tests for this one ? @pierrejeambrun @bbovenzi ?

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

LGTM now. I never imagined such a simple thing in javascript needs to be done in such way (but it's much more reasonable than the initial way).

@utkarsharma2 utkarsharma2 merged commit 0429bf4 into apache:main Sep 4, 2024
51 checks passed
@utkarsharma2 utkarsharma2 deleted the FixisSanitised branch September 4, 2024 16:58
@eladkal
Copy link
Contributor

eladkal commented Sep 5, 2024

@utkarsharma2 This will target 2.10.2?

@potiuk potiuk added this to the Airflow 2.10.2 milestone Sep 5, 2024
@potiuk
Copy link
Member

potiuk commented Sep 5, 2024

I marked it so - but this was I think what we agreed on, that it is not going to cancel rc1

@utkarsharma2
Copy link
Contributor Author

Thanks @potiuk, yes this belongs to 2.10.2.

@mfatemipour
Copy link
Contributor

Hi, Could we have this in 2.10 ?

@potiuk
Copy link
Member

potiuk commented Dec 19, 2024

It is already released in 2.10.2 - have you checked it @mfatemipour ? Can you please verify it in release notes and upgrade (2.10.4 is now available)/ test locally and confirm that it works? If not, open a new issue describing the problem you have.

Thanks for your help in advance.

@gpadavala
Copy link

gpadavala commented Dec 19, 2024

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

@potiuk
Copy link
Member

potiuk commented Dec 19, 2024

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.

@potiuk potiuk modified the milestones: Airflow 2.10.2, Airflow 2.10.5 Dec 19, 2024
@potiuk
Copy link
Member

potiuk commented Dec 19, 2024

I changed milestone to 2.10.5 and will cherry-pick it for 2.10.5

@potiuk
Copy link
Member

potiuk commented Dec 19, 2024

But it would be great to find what happened :)

potiuk pushed a commit to potiuk/airflow that referenced this pull request Dec 19, 2024
* Handle relative paths when sanitizing URLs

In the initial PR(apache#41665) we didn't handle the relative path in URL which led to issue(apache#41977). This PR aims at handling the relative path case when sanitizing URLs

* Add PR suggestions

* Update code comment
(cherry picked from commit 0429bf4)

Co-authored-by: Utkarsh Sharma <[email protected]>
potiuk added a commit that referenced this pull request Dec 20, 2024
)

* Handle relative paths when sanitizing URLs

In the initial PR(#41665) we didn't handle the relative path in URL which led to issue(#41977). This PR aims at handling the relative path case when sanitizing URLs

* Add PR suggestions

* Update code comment
(cherry picked from commit 0429bf4)

Co-authored-by: Utkarsh Sharma <[email protected]>
@utkarsharma2
Copy link
Contributor Author

utkarsharma2 commented Dec 20, 2024

But it would be great to find what happened :)

I think I missed backporting it, somehow. I will investigate more. :/

Thanks for the backporting it.

@potiuk
Copy link
Member

potiuk commented Dec 20, 2024

I think I missed backporting it, somehow. I will investigate more. :/

Yeah. Could be also some tooling issue of ours :(. I think we should do some 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants