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

Jump to bottom

Allow empty authority for unknown schemes #4349

Closed
wants to merge 6 commits into from
Closed

Allow empty authority for unknown schemes #4349

wants to merge 6 commits into from

Conversation

jfinkhaeuser
Copy link
Contributor

I discussed this briefly with @bagder

According to RFC3986 Section 3.2.2 on page 21

If the URI scheme defines a default for host, then that default
applies when the host subcomponent is undefined or when the
registered name is empty (zero length). For example, the "file" URI
scheme is defined so that no authority, an empty host, and
"localhost" all mean the end-user's machine, whereas the "http"
scheme considers a missing authority or empty host invalid.

cURL implements this behaviour for the file scheme, but doesn't - and cannot - implement this behaviour for schemes it does not know. For such schemes, passing a new flag that permits empty authority sections is desirable.

This PR adds such a CURLU_NO_AUTHORITY flag, as well as test cases.

@jfinkhaeuser
Add new flag, allowing empty authority/host parts. This is intended
f21bc07 
for use with unknown schemes (i.e. not "file:///") to override cURL's
demand that an authority exists.
@jfinkhaeuser
Add test cases for the new flag - without it, setting empty host
7bef217 
parts should fail. With it, it should work. The flag effectively
disables the hostname check if it's set, and the hostname is empty.
@jfinkhaeuser
Circumvent hostname checks when the new flag is set and the hostname is
37aac75 
empty.
@bagder bagder added the URL label Sep 13, 2019
@jfinkhaeuser
Copy link
Contributor Author

I've got to run - if there are tests failing, or there should be changes, I'll look at them in a few days, probably. Locally everything looked fine. And obviously if you'd like changes, I'm happy to provide them.

@bagder
Copy link
Member

bagder commented Sep 13, 2019

I think this is a sensible way to make the parser handle such URLs so I'm in favor of merging once things are green. Still missing: documentation of the new flag and symbols-in-versions update.

@jfinkhaeuser
Copy link
Contributor Author

Makes sense - will probably get to those changes tomorrow.

@bagder
Copy link
Member

bagder commented Sep 15, 2019 •
edited

The two builds using boringssl fail due to other reasons, not your fault!

@jfinkhaeuser
Copy link
Contributor Author

Excellent, then let me know if you want any other changes, otherwise I'm done.

bagder
bagder approved these changes Sep 19, 2019
View reviewed changes
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

Will merge asap!

@bagder bagder closed this in 0a4ecbd Sep 19, 2019
@bagder
Copy link
Member

bagder commented Sep 19, 2019

Thanks!

@jfinkhaeuser
Copy link
Contributor Author

No worries, glad to be of help!

@lock lock bot locked as resolved and limited conversation to collaborators Dec 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bagder bagder bagder approved these changes

Assignees
No one assigned
Labels
URL
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jfinkhaeuser @bagder