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

net/url: ParseRequestURI changes behavior from 1.9 with IRC URLs #23657

Closed
bradfitz opened this issue Feb 1, 2018 · 7 comments
Closed

net/url: ParseRequestURI changes behavior from 1.9 with IRC URLs #23657

bradfitz opened this issue Feb 1, 2018 · 7 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Milestone

Comments

@bradfitz
Copy link
Contributor

bradfitz commented Feb 1, 2018

Running Go 1.10 inside of Google revealed that this test now fails in Go 1.10:

https://github.com/asaskevich/govalidator

validator_test.go:698: Expected IsRequestURL("irc://#channel@network") to be true, got false

... due to ba1018b (https://go-review.googlesource.com/87038)

For #23392

Is that a valid URL?

@bradfitz bradfitz added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker labels Feb 1, 2018
@bradfitz bradfitz added this to the Go1.10 milestone Feb 1, 2018
@mvdan mvdan changed the title net/url: net/url: IsRequestURL changes behavior from 1.9 with IRC URLs Feb 1, 2018
@adamdecaf
Copy link
Contributor

adamdecaf commented Feb 2, 2018

Is it assumed that #channel is the authority (username)? Section 3.2 Authority mentions this:

The authority component is preceded by a double slash ("//") and is
terminated by the next slash ("/"), question mark ("?"), or number
sign ("#") character, or by the end of the URI.

Edit: Looking at a couple other languages (ruby, java) these seem to parse that input as a fragment.

scala> import java.net.URI

scala> new URI("irc://#channel@network")
res1: java.net.URI = irc://#channel@network

scala> res1.getFragment
res2: String = channel@network
adam@~$ irb
irb(main):003:0> require 'uri'

irb(main):008:0> URI.parse("irc://#channel@network").fragment   
=> "channel@network"

@rsc rsc changed the title net/url: IsRequestURL changes behavior from 1.9 with IRC URLs net/url: ParseRequestURI changes behavior from 1.9 with IRC URLs Feb 2, 2018
@rsc
Copy link
Contributor

rsc commented Feb 2, 2018

Note that this is about ParseRequestURI (IsRequestURL is in the govalidator package and calls url.ParseRequestURI).
The comparison with java.net.URI and ruby uri are off-center - they are more like url.Parse, not url.ParseRequestURI. url.Parse does accept irc://#channel@network and take the #channel@network as a fragment. But url.ParseRequestURI's whole reason for existing is to not look for fragments.

The relevant docs say:

// ParseRequestURI parses rawurl into a URL structure. It assumes that
// rawurl was received in an HTTP request, so the rawurl is interpreted
// only as an absolute URI or an absolute path.
// The string rawurl is assumed not to have a #fragment suffix.
// (Web browsers strip #fragment before sending the URL to a web server.)

And of course #channel@network is in fact NOT a valid user info. In this program:

package main

import (
	"fmt"
	"net/url"
)

func main() {
	u, err := url.Parse("irc://#channel@network")
	fmt.Printf("%+v %q %v\n", u, u.Fragment, err)
	u, err = url.ParseRequestURI("irc://#channel@network")
	fmt.Printf("%+v %v\n", u, err)
}

url.Parse was happy in Go 1.9 and Go 1.10. url.ParseRequestURI was happy in Go 1.9 but silently converted the URL, when reprinting it with u.String, into irc://%23channel@network. If everyone wants the channel@network to be a fragment, that %23 is going to hurt.

It seems like this change is unfortunate and will break some code, but Go 1.10 is actually doing the right thing by rejecting # in userinfo, and Go 1.9 and earlier were not.

@rsc
Copy link
Contributor

rsc commented Feb 2, 2018

Even govalidator says:

// IsRequestURL check if the string rawurl, assuming
// it was received in an HTTP request, is a valid
// URL confirm to RFC 3986

I don't believe that receiving irc://#channel@network in an HTTP request is valid.

@bradfitz
Copy link
Contributor Author

bradfitz commented Feb 2, 2018

@asaskevich, can you fix govalidator?

@rsc
Copy link
Contributor

rsc commented Feb 2, 2018

Took me a few minutes but I think @bradfitz means fix the govalidator tests not to expect IsRequestURL("irc://#channel@network") to be true.

@magical
Copy link
Contributor

magical commented Feb 2, 2018

Is that even a valid IRC url? The channel usually goes in the path, like irc://network/channel. I've never seen irc://#channel@network.

@rsc
Copy link
Contributor

rsc commented Feb 6, 2018

irc://#channel@network is a fine URL, it's just not a fine request URL, that is, something you'd see in an HTTP request line. I don't see anyone arguing otherwise here, so I'm going to close this as working as intended.

@rsc rsc closed this as completed Feb 6, 2018
stapelberg added a commit to stapelberg/govalidator that referenced this issue Feb 16, 2018
The previously-used URL format is not valid as per either of these two:
https://www.w3.org/Addressing/draft-mirashi-url-irc-01.txt
https://www-archive.mozilla.org/projects/rt-messaging/chatzilla/irc-urls.html

Further, Go’s net/url package no longer parses the syntax as of
golang/go@ba1018b

fixes asaskevich#250
related to golang/go#23657
@golang golang locked and limited conversation to collaborators Feb 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Projects
None yet
Development

No branches or pull requests

5 participants