@bradfitz @dsnet Wouldnt a TrimSpace before getscheme on this line solve the issue? I can raise a PR for this if thats agreed upon as the fix. Would also write tests of course.

Technically, according to RFC3986-Sec3.1 -

Scheme names consist of a sequence of characters beginning with a
letter and followed by any combination of letters, digits, plus
("+"), period ("."), or hyphen ("-").

So therefore, anything not starting with a letter should be rejected and that would be the right behavior. However, I had a look at the implementations out there in the wild.

NodeJS accepts it-

> require("url").parse('  https://example.org')
Url {
  protocol: 'https:',
  slashes: true,
  auth: null,
  host: 'example.org',
  port: null,
  hostname: 'example.org',
  hash: null,
  search: null,
  query: null,
  pathname: '/',
  path: '/',
  href: 'https://example.org/' }

Curl does not -

curl '   https://example.org' 
curl: (1) Protocol "   https" not supported or disabled in libcurl

Same with wget -

wget '   https://example.org' 
   https://example.org: Scheme missing.

Python also fails to parse-

from urllib.parse import urlparse
>>> o = urlparse('  https://example.org')
>>> o
ParseResult(scheme='', netloc='', path='  https://example.org', params='', query='', fragment='')

Note that scheme and netloc are empty.

In light of the above, let us just return a more descriptive error (with quotes around the url) if we see a space. How about -

parse " http://example.org": URL scheme does not begin with an alpha-numeric character.

That's right, sorry. Then we can simplify it to ".. does not begin with a letter".

simplify it to ".. does not begin with a letter".

Perhaps ".. scheme does not begin with a letter" would be more accurate as "//example.org" is valid to my understanding.

Interestingly, i found

	u, err := url.Parse("    //example.org")
	if err != nil {
		panic(err)
	}
	fmt.Println(u)

is NOT failing, but outputs:

%20%20%20%20//example.org

https://play.golang.org/p/jRk63PWsKG5

Yes, we should return an error on leading bogus chars.

ok I will make a PR for this code change with associated tests.

I have implemented the generic improvement of error messages by consistently quoting the URLs in #29384, but more detailed messages and stricter validation are welcome of course.

@bradfitz what does "bogus chars" translate to besides a space?

Any non-letter, according to the spec.

On further analysis, it seems its not straightforward to implement this because of 2 reasons:

• the characters that are invalid in a scheme could be valid in other parts of the url (like the period and plus characters) so we will need to remember what we processed before. Currently getscheme does not do that, and am wondering if adding that functionality would increase complexity.
• the url package says it should be backward compatible.

To account for the space-specific error, we could add a separate case branch and raise the error message like so:

case c == ' ':   // Add other invalid prefix chars here
	if i == 0 {
		return "", "", errors.New("URL scheme has invalid characters")
	} 

However this seems like its a bandaid fix to me, but I am not sure, so would appreciate some input. Any thoughts on whether this is good, or if there's any other approach?

@stefanb @bradfitz @agnivade ?

I assume the proposed change would go just before default line 450 in the switch block here:

This would make it inconsistent with other validation error cases in the same switch block:

1. the first character validation:

   go/src/net/url/url.go

   Lines 441 to 444 in b50210f

   	case '0' <= c && c <= '9' || c == '+' || c == '-' || c == '.':
   	if i == 0 {
   	return "", rawurl, nil
   	}

2. the invalid character validation
   

   go/src/net/url/url.go

   Lines 450 to 453 in b50210f

   	default:
   	// we have encountered an invalid character,
   	// so there is no valid scheme
   	return "", rawurl, nil

They both just returns scheme as "" and no error whatsoever. It should be consistent in a way so that all reasons for invalid scheme result either in error or "", but not mixed.

It's hard to discuss code changes here. Maybe just send a CL and we can continue it over there.

We probably need to simplify the switch-case a bit. Have an if condition first, which checks for i==0 and non-letter, and return immediately. And integrate the rest of the switch-case accordingly.

We can keep the special c == ':' and i == 0 check, which will preserve the "missing protocol scheme" error, for backward compatibility reasons. But I will let @bradfitz decide that.

Change https://golang.org/cl/155922 mentions this issue: net/url: give a proper error message on invalid character in scheme

Change https://golang.org/cl/185117 mentions this issue: net/url: improve url parsing error messages by quoting