net/url: http://a,b,c/foo parses and reencodes as http://a%2Cb%2Cc/foo

[rsc]

This affects mongodb, which uses net/url to parse URLs. See @liviosoares's comments on #12023.

[rsc]

@liviosoares, suppose that mongodb://ip1,ip2,ip3 parses and reencodes as mongodb://ip1%2Cip2%2Cip3. Why is that a problem? They mean the same thing. The only way I can see this being a problem is if mongodb is using raw string manipulation instead of url.Parse. In that case it should probably be using url.Parse.

[cespare]

@rsc minor note; I think this particular mongodb issue is unrelated to database/sql (unlike the original issue in #12023).

[rsc]

@cespare Thank you, I have edited the top comment to remove mention of database/sql. In that case, though, I'm even more confused. What's url.Parse'ing and reencoding the URLs and then handing them to something that doesn't url.Parse them?

[rsc]

@mikioh, I'm having a hard time understanding why shouldEscape behaves as it does for mode==escapeHost. Simplifying, it says:

// §2.3 Unreserved characters (alphanum)
if 'A' <= c && c <= 'Z' || 'a' <= c && c <= 'z' || '0' <= c && c <= '9' {
    return false
}

switch c {
case '-', '_', '.', '~': // §2.3 Unreserved characters (mark)
    return false

case '$', '&', '+', ',', '/', ':', ';', '=', '?', '@': // §2.2 Reserved characters (reserved)
    switch mode {
    case encodeHost: // §3.2.1
        // The RFC allows ':'.
        return c != ':'
    }

case '[', ']': // §2.2 Reserved characters (reserved)
    switch mode {
    case encodeHost: // §3.2.1
        // The RFC allows '[', ']'.
        return false
    }
}

// Everything else must be escaped.
return true

So:

shouldEscape('A', escapeHost) = false (first clause)
shouldEscape('-', escapeHost) = false (first branch of switch)
shouldEscape('+', escapeHost) = true (second branch of switch)
shouldEscape(':', escapeHost) = false (second branch of switch)
shouldEscape('[', escapeHost) = false (third branch of switch)

but the relevant parts of RFC 3986 say:

  host        = IP-literal / IPv4address / reg-name
  reg-name    = *( unreserved / pct-encoded / sub-delims )
  unreserved  = ALPHA / DIGIT / "-" / "." / "_" / "~"
  sub-delims    = "!" / "$" / "&" / "'" / "(" / ")"
              / "*" / "+" / "," / ";" / "="

If the host allows all of those sub-delims, why does shouldEscape return true for all of them? The only thing it returns false for is ":", which ironically isn't allowed (but it is part of IPv6 syntax and the :port suffix, which is why it's allowed). But why do we force escaping of the sub-delim case? It seems like we should allow them all to be used unencoded.

That is, return c != ':' should be return false. No?

[mikioh]

But why do we force escaping of the sub-delim case?

My bad, because I didn't want to dive into the long standing issue #5684.

[mikioh]

That is, return c != ':' should be return false. No?

Nope, the line

case '$', '&', '+', ',', '/', ':', ';', '=', '?', '@': // §2.2 Reserved characters (reserved)

includes both gen-delims and sub-delims partially.

[rsc]

Please see https://go-review.googlesource.com/#/c/13254/

[gopherbot]

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