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: Fails to parse URLs with spaces in scope IDs in 1.6beta2 #14002

Closed
calmh opened this issue Jan 18, 2016 · 15 comments
Closed

net/url: Fails to parse URLs with spaces in scope IDs in 1.6beta2 #14002

calmh opened this issue Jan 18, 2016 · 15 comments

Comments

@calmh
Copy link
Contributor

calmh commented Jan 18, 2016

Test program:

package main

import (
    "fmt"
    "net/url"
)

func main() {
    u, err := url.Parse("tcp://[fe80::cefa:ff:fef3:5fcc%25Wireless%20Network%20Connection]:22000")
    if err != nil {
        fmt.Println(err)
    } else {
        fmt.Println("OK:", u)
    }
}

Fine in 1.5.2:

jb@syno:~/tmp $ go version
go version go1.5.2 darwin/amd64
jb@syno:~/tmp $ go run test.go 
OK: tcp://[fe80::cefa:ff:fef3:5fcc%25Wireless%20Network%20Connection]:22000

Errors in 1.6beta2:

jb@syno:~/tmp $ go run test.go 
parse tcp://[fe80::cefa:ff:fef3:5fcc%25Wireless%20Network%20Connection]:22000: invalid URL escape "%20"
jb@syno:~/tmp $ go version
go version devel +66330d8 Wed Jan 13 23:40:13 2016 +0000 darwin/amd64

That's how Windows represents scope IDs.

@bradfitz
Copy link
Contributor

@rsc, you touched this last. Maybe you still remember all the spec rules and Go decisions made?

@bradfitz bradfitz added this to the Go1.6Maybe milestone Jan 20, 2016
@rsc
Copy link
Contributor

rsc commented Jan 24, 2016

We started rejecting spaces in host names, which is generally correct. I guess we have to accept spaces in the %-encoded scope section. Ugh.

@rsc
Copy link
Contributor

rsc commented Jan 24, 2016

CL 18855.

@gopherbot
Copy link

CL https://golang.org/cl/18855 mentions this issue.

@canton7
Copy link

canton7 commented Jan 26, 2016

Windows: putting spaces where they don't belong since Windows NT 3.1.

Purely out of interest: Windows uses the Scope ID of the network interface in question as the zone identifier. This is a long, so zones typically have the value %1, %2, etc.

Using the network interface name on Windows seems to be a Go thing. Presumably there's good reason, although it does make taking URLs formatted by Go and parsing them in other languages slightly tricky.

@bradfitz
Copy link
Contributor

/cc @calmh, @mikioh, @alexbrainman for feedback. Where did the OP's URL/scope ID come from?

@calmh
Copy link
Contributor Author

calmh commented Jan 26, 2016

That's from String() on a net.Addr returned from Read...() on an UDP socket (whatever the exact read method is that returns the packet plus the source address, I'm on mobile currently). We that put that address in as the Host in a url.URL which in turn gets stringified and sent over the network. The other side does url.Parse which results in the above.

@bradfitz
Copy link
Contributor

@rsc, given @calmh's comment, I think your fix above isn't necessary. I think we need to fix the String method instead.

@bradfitz bradfitz reopened this Jan 26, 2016
@rsc
Copy link
Contributor

rsc commented Jan 26, 2016

I'm not changing the String method to emit an encoded URL with literal spaces in it. URLs do not have spaces. Full stop.

@rsc
Copy link
Contributor

rsc commented Jan 26, 2016

There's some notion that we should be using %1 instead of %AdapterName, but I don't know where the 1 comes from and I don't know if it works to send it over the network.

@calmh
Copy link
Contributor Author

calmh commented Jan 26, 2016

As a side note, url.Parse also does not accept spaces in the host name (in the scope ID or otherwise), so that wouldn't be an immediate fix. Perhaps it could be taught to do so, but it seems cleanest to do the escaping as it currently does and then accept that back.

@calmh
Copy link
Contributor Author

calmh commented Jan 26, 2016

Actually the "send over network" part in my description is both incorrect and irrelevant. It's just two packages where the interface between them is in terms of URLs to connect to and it so happens that some of these represent link local v6 addresses.

@rsc
Copy link
Contributor

rsc commented Jan 26, 2016

OK, well, the fact is that zone identifiers are allowed to have spaces, and this code was trying to make sure they were minimally escaped. Since you can't have a space in a URL, the space must be escaped. This code didn't allow that, and now it does. So I think everything here is in order.

There's some question about whether zone identifiers in practice use spaces, but it appears that at least in Go programs on Windows they do. Maybe that's a mistake, or maybe not, but it's too late in the release cycle to make a fundamental change like that in package net. The acceptance of %20 in the zone identifier is trivial, clearly safe, and consistent with RFCs. (The code in question was actually being stricter than the RFCs and still is.)

If someone wants to open a separate bug, for Go 1.7, about maybe using %1 instead of "%Windows Ethernet Adapter" or whatever, that's fine.

@rsc rsc closed this as completed Jan 26, 2016
@canton7
Copy link

canton7 commented Jan 26, 2016

That's from String() on a net.Addr returned from Read...() on an UDP socket

...

I think we need to fix the String method instead.

...

I'm not changing the String method to emit an encoded URL with literal spaces in it.

I think @bradfitz was referring to net.Addr.String(), not URL.String()? Maybe I'm misreading it...

@rsc
Copy link
Contributor

rsc commented Jan 26, 2016

Oh, the net.Addr.String method would make more sense. But the point in my final comment remains: it's fine if someone wants to look into fixing that for Go 1.7, but this seems to work for people in Go 1.6 and the URL change is not technically wrong, and the URL change is also less scary than changing the form of net.Addr.String this close to a release.

@golang golang locked and limited conversation to collaborators Feb 3, 2017
@rsc rsc removed their assignment Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants