net/url: give a proper error message on invalid character in scheme #29460

cgopalan · 2018-12-29T22:45:16Z

googlebot · 2018-12-29T22:45:20Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2018-12-29T22:53:37Z

CLAs look good, thanks!

gopherbot · 2018-12-29T22:54:23Z

This PR (HEAD: d51a00d) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/155922 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

cgopalan · 2018-12-29T22:57:34Z

@agnivade I have made changes based on our discussion in the issue, but at the moment I have one failing test:

--- FAIL: TestFileServerEscapesNames (0.00s)
    fs_test.go:300: test "simple_name": Get: Get http://127.0.0.1:36757/0: failed to parse Location header "0/": parse 0/: URL scheme has invalid character
FAIL
FAIL	net/http	4.252s

Not sure why this is failing since the scheme seems to be fine. Would love to get some pointers on this.

stefanb · 2019-01-03T02:32:02Z

Please note that the negative tests are not actually testing for the expected error message content, just its mere presence.

…ar-url-scheme

cgopalan · 2019-01-14T22:25:41Z

Please note that the negative tests are not actually testing for the expected error message content, just its mere presence.

@stefanb still a WIP. Will correct this.

cgopalan · 2019-01-14T23:11:27Z

@agnivade @stefanb in the failing test I mentioned above, it seems like the 0 in http://127.0.0.1:36757/0 url is being parsed as a location header and as a result 0 is a valid beginning character for a url. Also there are tests that count on / . ? ; being valid starting characters for a url so I had to include them.
Is this the right approach? Would like some guidance here.

agnivade · 2019-01-15T03:58:46Z

The code can do with some improvement. Your (c < 'A' || (c > 'Z' && c < 'a') || c > 'z') is repeated which I wanted to avoid.

We don't typically discuss CLs in github. Gerrit is the place for that. Please feel free to ping Brad on the CL to get guidance.

gopherbot · 2019-01-16T18:11:54Z

Message from Dmitri Shuralyov:

Patch Set 1: Run-TryBot+1

(3 comments)

Thanks for sending the change Chandrakant. I've left some initial review comments.

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-01-16T18:11:55Z

Message from Gobot Gobot:

Patch Set 1:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=81fd1e73

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-01-16T18:16:35Z

Message from Gobot Gobot:

Patch Set 1:

Build is still in progress...
This change failed on linux-amd64:
See https://storage.googleapis.com/go-build-log/81fd1e73/linux-amd64_3d167c8b.log

Consult https://build.golang.org/ to see whether it's a new failure. Other builds still in progress; subsequent failure notices suppressed until final report.

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-01-16T18:21:00Z

Message from Gobot Gobot:

Patch Set 1: TryBot-Result-1

Consult https://build.golang.org/ to see whether they are new failures.

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-01-23T16:29:44Z

Message from Chandrakant G:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

…ar-url-scheme

gopherbot · 2019-02-02T20:12:53Z

This PR (HEAD: 59a4fc2) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/155922 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

…ar-url-scheme

gopherbot · 2019-02-06T01:55:21Z

This PR (HEAD: feefe58) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/155922 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-02-08T17:33:42Z

Message from Chandrakant G:

Patch Set 5:

Any updates on this? Thanks!

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-02-26T15:09:06Z

Message from Chandrakant G:

Patch Set 5:

Patch Set 5:

Any updates on this? Thanks!

Just a friendly ping for review. Thanks!

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-02-26T20:01:29Z

Message from Brad Fitzpatrick:

Patch Set 5:

Patch Set 5:

Patch Set 5:

Any updates on this? Thanks!

Just a friendly ping for review. Thanks!

It was already reviewed. We were waiting on you to update it.

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-02-26T20:13:35Z

Message from Chandrakant G:

Patch Set 5:

Patch Set 5:

Patch Set 5:

Patch Set 5:

Any updates on this? Thanks!

Just a friendly ping for review. Thanks!

It was already reviewed. We were waiting on you to update it.

I have updated the code according to the review from Dmitri - code changes, the PR description and commit messages. The tests also pass. Is there anything else I need to do? Sorry am new to Gerrit, so might have missed something.

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-08-09T08:15:43Z

Message from Daniel Martí:

Patch Set 9:

Sorry, I missed this in spring.

Perhaps take a look at https://go-review.googlesource.com/c/go/+/185117? That's only updating the issue, not fixing it, but I think it can serve as a base for a much simpler fix.

What I meant with my earlier comment is that we shouldn't loop over the url bytes twice. That's a lot of extra code, and it's also inefficient.

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-10-15T03:41:47Z

Message from Go Bot:

Patch Set 1:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=81fd1e73

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-10-15T03:41:48Z

Message from Go Bot:

Patch Set 1:

Build is still in progress...
This change failed on linux-amd64:
See https://storage.googleapis.com/go-build-log/81fd1e73/linux-amd64_3d167c8b.log

Consult https://build.golang.org/ to see whether it's a new failure. Other builds still in progress; subsequent failure notices suppressed until final report.

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-10-15T03:41:49Z

Message from Go Bot:

Patch Set 1: TryBot-Result-1

Consult https://build.golang.org/ to see whether they are new failures.

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-11-08T11:23:09Z

Message from Chandrakant G:

Patch Set 5:

Patch Set 5:

Any updates on this? Thanks!

Just a friendly ping for review. Thanks!

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-11-08T11:23:10Z

Message from Brad Fitzpatrick:

Patch Set 5:

Patch Set 5:

Patch Set 5:

Any updates on this? Thanks!

Just a friendly ping for review. Thanks!

It was already reviewed. We were waiting on you to update it.

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-11-08T11:23:10Z

Message from Chandrakant G:

Patch Set 5:

Patch Set 5:

Patch Set 5:

Patch Set 5:

Any updates on this? Thanks!

Just a friendly ping for review. Thanks!

It was already reviewed. We were waiting on you to update it.

I have updated the code according to the review from Dmitri - code changes, the PR description and commit messages. The tests also pass. Is there anything else I need to do? Sorry am new to Gerrit, so might have missed something.

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-11-08T11:23:11Z

Message from Dmitri Shuralyov:

Patch Set 5:

It looks like GerritBot is failing to import the commit message from the PR at #29460 (comment) into this CL for some reason.

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-11-08T11:23:12Z

Message from Dmitri Shuralyov:

Patch Set 5:

It looks like GerritBot is failing to import the commit message from the PR at #29460 (comment) into this CL for some reason.

Never mind, that's not what's happening. I misread the description because GitHub renders a URL like "#29261" as just "#29261" in its UI.

Chandrakant, you'll need to update the description of your PR to say exactly "#29261" rather than the long URL (per https://go-review.googlesource.com/c/go/+/155922/1//COMMIT_MSG#9).

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-11-08T11:23:13Z

Message from Chandrakant G:

Patch Set 7:

Patch Set 5:

It looks like GerritBot is failing to import the commit message from the PR at #29460 (comment) into this CL for some reason.

Never mind, that's not what's happening. I misread the description because GitHub renders a URL like "#29261" as just "#29261" in its UI.

Chandrakant, you'll need to update the description of your PR to say exactly "#29261" rather than the long URL (per https://go-review.googlesource.com/c/go/+/155922/1//COMMIT_MSG#9).

It seems like it always shows the long URL no matter how I update it. Are there any other PRs that I can reference to see how its done?

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-11-08T11:23:14Z

Message from Dmitri Shuralyov:

Patch Set 8:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-11-08T11:23:15Z

Message from Brad Fitzpatrick:

Patch Set 8:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-11-08T11:23:16Z

Message from Chandrakant G:

Patch Set 8:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-11-08T11:23:16Z

Message from Chandrakant G:

Patch Set 8:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-11-08T11:23:17Z

Message from Chandrakant G:

Patch Set 9:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-11-08T11:23:18Z

Message from Daniel Martí:

Patch Set 9: Code-Review-1

(1 comment)

I think this could be done in a simpler way, without looping over the input string twice.

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-11-08T11:23:19Z

Message from Chandrakant G:

Patch Set 9:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-11-08T11:23:20Z

Message from Chandrakant G:

Patch Set 9:

Patch Set 9:

(1 comment)

Waiting for feedback on my question. Friendly ping - thanks!

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2021-11-08T11:23:21Z

Message from Daniel Martí:

Patch Set 9:

Sorry, I missed this in spring.

Perhaps take a look at https://go-review.googlesource.com/c/go/+/185117? That's only updating the issue, not fixing it, but I think it can serve as a base for a much simpler fix.

What I meant with my earlier comment is that we shouldn't loop over the url bytes twice. That's a lot of extra code, and it's also inefficient.

Please don’t reply on this GitHub thread. Visit golang.org/cl/155922.
After addressing review feedback, remember to publish your drafts!

googlebot added the cla: no Used by googlebot to label PRs as having an invalid CLA. The text of this label should not change. label Dec 29, 2018

Proper error message on invalid character in scheme

d51a00d

cgopalan force-pushed the invalid-char-url-scheme branch from 7990c1f to d51a00d Compare December 29, 2018 22:53

googlebot added cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. and removed cla: no Used by googlebot to label PRs as having an invalid CLA. The text of this label should not change. labels Dec 29, 2018

Merge branch 'master' of https://github.com/golang/go into invalid-ch…

df100b7

…ar-url-scheme

cgopalan added 2 commits February 2, 2019 14:17

Merge branch 'master' of https://github.com/golang/go into invalid-ch…

f19fd79

…ar-url-scheme

Added fix and tests

59a4fc2

cgopalan changed the title ~~net/url: Proper error message on invalid character in scheme~~ net/url: give a proper error message on invalid character in scheme Feb 2, 2019

cgopalan added 2 commits February 5, 2019 20:53

Fixed test

c0dee3f

Merge branch 'master' of https://github.com/golang/go into invalid-ch…

feefe58

…ar-url-scheme

gopherbot force-pushed the master branch from 53bd915 to 6139019 Compare October 3, 2019 21:09

gopherbot force-pushed the master branch 8 times, most recently from 4a7ed1f to 0f992b9 Compare November 5, 2019 02:57

gopherbot force-pushed the master branch from a408139 to 5764653 Compare September 14, 2020 21:26

heschi closed this Dec 15, 2021

net/url: give a proper error message on invalid character in scheme #29460

net/url: give a proper error message on invalid character in scheme #29460

Conversation

cgopalan commented Dec 29, 2018 • edited by bradfitz Loading

googlebot commented Dec 29, 2018

googlebot commented Dec 29, 2018

gopherbot commented Dec 29, 2018

cgopalan commented Dec 29, 2018

stefanb commented Jan 3, 2019 • edited Loading

cgopalan commented Jan 14, 2019

cgopalan commented Jan 14, 2019

agnivade commented Jan 15, 2019

gopherbot commented Jan 16, 2019

gopherbot commented Jan 16, 2019

gopherbot commented Jan 16, 2019

gopherbot commented Jan 16, 2019

gopherbot commented Jan 23, 2019

gopherbot commented Feb 2, 2019

gopherbot commented Feb 6, 2019

gopherbot commented Feb 8, 2019

gopherbot commented Feb 26, 2019

gopherbot commented Feb 26, 2019

gopherbot commented Feb 26, 2019

gopherbot commented Aug 9, 2019

gopherbot commented Oct 15, 2020

gopherbot commented Oct 15, 2020

gopherbot commented Oct 15, 2020

gopherbot commented Nov 8, 2021

gopherbot commented Nov 8, 2021

gopherbot commented Nov 8, 2021

gopherbot commented Nov 8, 2021

gopherbot commented Nov 8, 2021

gopherbot commented Nov 8, 2021

gopherbot commented Nov 8, 2021

gopherbot commented Nov 8, 2021

gopherbot commented Nov 8, 2021

gopherbot commented Nov 8, 2021

gopherbot commented Nov 8, 2021

gopherbot commented Nov 8, 2021

gopherbot commented Nov 8, 2021

gopherbot commented Nov 8, 2021

gopherbot commented Nov 8, 2021

cgopalan commented Dec 29, 2018 •

edited by bradfitz

Loading

stefanb commented Jan 3, 2019 •

edited

Loading