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.

Sign up for GitHub

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

Jump to bottom

String: Parse fragment from URL #92237

Merged
merged 1 commit into from
Oct 1, 2024
Merged

String: Parse fragment from URL #92237

merged 1 commit into from
Oct 1, 2024

Conversation

timothyqiu
Copy link
Member

Fixes #92229

@timothyqiu timothyqiu added bug topic:network cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels May 22, 2024
@timothyqiu timothyqiu added this to the 4.3 milestone May 22, 2024
@timothyqiu timothyqiu requested review from a team as code owners May 22, 2024 02:56
@akien-mga akien-mga changed the title Parse fragment from URL String: Parse fragment from URL May 22, 2024
@timothyqiu timothyqiu modified the milestones: 4.3, 4.4 Jul 22, 2024
@AThousandShips AThousandShips added the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Jul 25, 2024
Faless
Faless reviewed Sep 27, 2024
View reviewed changes
Copy link
Collaborator

@Faless Faless left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks good, but I wonder if we should parse the fragment before the scheme.

As far as I understand RFC 3986 section 3.5 and Appendix A specify that the URL fragment can contain both : and / characters.

This means that the following URI is valid:

google.com/#goto=http://redirect_url/

But our current implementation will take the scheme as google.com/#goto=http://.

@timothyqiu timothyqiu requested review from a team as code owners September 27, 2024 11:53
@timothyqiu
Copy link
Member Author

http_://example.com is currently considered valid, which is not the case according to RFC 3986. Adding scheme validation seems to solve both problems.

Also added some tests for parse_url().

Faless
Faless approved these changes Sep 28, 2024
View reviewed changes
Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

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

LGTM, nice work! 🚀

@akien-mga akien-mga merged commit cf1d910 into godotengine:master Oct 1, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga mentioned this pull request Oct 1, 2024
Merged
@timothyqiu timothyqiu deleted the url-fragment branch October 2, 2024 10:28
@jss2a98aj jss2a98aj mentioned this pull request Nov 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Faless Faless Faless approved these changes

Assignees
No one assigned
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:core topic:network
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.

HTTPRequests do not correctly handle URL fragments
4 participants
@timothyqiu @akien-mga @Faless @AThousandShips