[release-branch.go1.10] net/url, net/http: reject control characters in URLs #29926

dmitshur · 2019-01-24T22:53:13Z

This is a more conservative version of the reverted CL 99135 (which
was reverted in CL 137716)

The net/url part rejects URLs with ASCII CTLs from being parsed and
the net/http part rejects writing them if a bogus url.URL is
constructed otherwise.

Updates #27302
Updates #22907
Fixes #29922

Change-Id: I09a2212eb74c63db575223277aec363c55421ed8
Reviewed-on: https://go-review.googlesource.com/c/159157
Run-TryBot: Brad Fitzpatrick bradfitz@golang.org
TryBot-Result: Gobot Gobot gobot@golang.org
Reviewed-by: Filippo Valsorda filippo@golang.org

googlebot · 2019-01-24T22:53:15Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2019-01-24T22:53:55Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

gopherbot · 2019-01-24T22:54:59Z

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

Please visit https://go-review.googlesource.com/c/go/+/159478 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-01-24T22:58:11Z

Message from Dmitri Shuralyov:

Patch Set 1:

This is an unusual cherry-pick CL in that I am testing whether it's viable to send cherry-pick CLs via Pull Requests (see #29922 (comment)).

It needs to be more thoroughly reviewed to determine if it's acceptable and follows the cherry-pick requirements as described at https://github.com/golang/go/wiki/MinorReleases#making-cherry-pick-cls.

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

gopherbot · 2019-01-24T22:59:29Z

Message from Dmitri Shuralyov:

Patch Set 2: Run-TryBot+1

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

gopherbot · 2019-01-24T23:00:02Z

Message from Gobot Gobot:

Patch Set 2:

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

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

gopherbot · 2019-01-24T23:02:40Z

Message from Gobot Gobot:

Patch Set 2:

Build is still in progress...
This change failed on openbsd-amd64-64:
See https://storage.googleapis.com/go-build-log/aefd6e9f/openbsd-amd64-64_3de08438.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/159478.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-01-24T23:04:53Z

Message from Gobot Gobot:

Patch Set 2: 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/159478.
After addressing review feedback, remember to publish your drafts!

…in URLs This is a more conservative version of the reverted CL 99135 (which was reverted in CL 137716) The net/url part rejects URLs with ASCII CTLs from being parsed and the net/http part rejects writing them if a bogus url.URL is constructed otherwise. Updates #27302 Updates #22907 Fixes #29922 Change-Id: I09a2212eb74c63db575223277aec363c55421ed8 Reviewed-on: https://go-review.googlesource.com/c/159157 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Filippo Valsorda <filippo@golang.org>

googlebot · 2019-01-25T00:07:06Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

gopherbot · 2019-01-25T00:07:43Z

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

Please visit https://go-review.googlesource.com/c/go/+/159478 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

googlebot · 2019-01-25T00:08:19Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

gopherbot · 2019-01-25T00:19:53Z

Message from Dmitri Shuralyov:

Patch Set 3: Run-TryBot+1

Here's a list of items I've discovered about the process of sending a cherry-pick CL via a PR by going through it:

It requires someone to manually verify CLAs and change the "cla: no" label to "cla: yes" in the PR. See [release-branch.go1.10] net/url, net/http: reject control characters in URLs #29926 (comment). googlebot applies "cla: no" label because it sees that "one or more commits were authored or co-authored by someone other than the pull request submitter."

I think this may not apply if cherry-picking your own CL.

Normally, one isn't supposed to include Change-Id and other such lines in the PR description, but I needed to for the cherry-pick CL. This doesn't recognized by GerritBot, and it ends up repeating the Change-Id line. See https://go-review.googlesource.com/c/go/+/159478/1//COMMIT_MSG, and https://go-review.googlesource.com/c/go/+/159478/1..2//COMMIT_MSG which happened on its own without me updating the PR.

Aside from that, it seems to work okay so far.

Andy, how do you feel about this? Would you prefer we say that cherry-pick CLs via PRs are not supported and shouldn't be made, or should we invest into fixing problem 2 (described above) so that it is possible for people to send cherry-picks via PRs?

I'm leaning towards saying it's not supported now, but we can open a FeatureRequest issue about fixing the experience so it becomes supported.

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

gopherbot · 2019-01-25T00:20:34Z

Message from Gobot Gobot:

Patch Set 3:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=1453f848

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

gopherbot · 2019-01-25T00:22:51Z

Message from Gobot Gobot:

Patch Set 3:

Build is still in progress...
This change failed on openbsd-amd64-64:
See https://storage.googleapis.com/go-build-log/1453f848/openbsd-amd64-64_3d5f90da.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/159478.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-01-25T00:27:46Z

Message from Gobot Gobot:

Patch Set 3: TryBot-Result-1

2 of 19 TryBots failed:
Failed on openbsd-amd64-64: https://storage.googleapis.com/go-build-log/1453f848/openbsd-amd64-64_3d5f90da.log
Failed on freebsd-amd64-12_0: https://storage.googleapis.com/go-build-log/1453f848/freebsd-amd64-12_0_701f3791.log

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

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

gopherbot · 2019-01-25T00:34:51Z

Message from Dmitri Shuralyov:

Patch Set 3:

2 of 19 TryBots failed

The 2 trybot failures are expected due to issue #29265.

I'm about to deploy a fix for it (CL 155463) and I plan to use this CL to verify that the fix works correctly.

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

gopherbot · 2019-01-25T01:27:48Z

Message from Gobot Gobot:

Patch Set 3: TryBot-Result+1

TryBots are happy.

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

gopherbot · 2019-01-25T01:48:24Z

Message from Dmitri Shuralyov:

Patch Set 3:

TryBots are happy.

I re-ran the trybots after deploying the fix for #29265, and they're passing now.

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

gopherbot · 2019-01-28T16:21:39Z

Message from Andrew Bonventre:

Patch Set 3:

Patch Set 3: Run-TryBot+1

Here's a list of items I've discovered about the process of sending a cherry-pick CL via a PR by going through it:

It requires someone to manually verify CLAs and change the "cla: no" label to "cla: yes" in the PR. See [release-branch.go1.10] net/url, net/http: reject control characters in URLs #29926 (comment). googlebot applies "cla: no" label because it sees that "one or more commits were authored or co-authored by someone other than the pull request submitter."

I think this may not apply if cherry-picking your own CL.

Normally, one isn't supposed to include Change-Id and other such lines in the PR description, but I needed to for the cherry-pick CL. This doesn't recognized by GerritBot, and it ends up repeating the Change-Id line. See https://go-review.googlesource.com/c/go/+/159478/1//COMMIT_MSG, and https://go-review.googlesource.com/c/go/+/159478/1..2//COMMIT_MSG which happened on its own without me updating the PR.

Aside from that, it seems to work okay so far.

Andy, how do you feel about this? Would you prefer we say that cherry-pick CLs via PRs are not supported and shouldn't be made, or should we invest into fixing problem 2 (described above) so that it is possible for people to send cherry-picks via PRs?

I'm leaning towards saying it's not supported now, but we can open a FeatureRequest issue about fixing the experience so it becomes supported.

I don't think it's worth supporting now.

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

gopherbot · 2019-01-30T18:55:41Z

Message from Andrew Bonventre:

Patch Set 3: Code-Review+2

If you still want to continue, here, go for it.

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

gopherbot · 2019-01-30T19:11:41Z

Message from Brad Fitzpatrick:

Patch Set 3: Code-Review-2

We're going to backport the later fix instead.

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

gopherbot · 2019-01-31T19:29:00Z

This PR is being closed because golang.org/cl/159478 has been abandoned.

This PR was used to investigate whether it's possible to send a cherry-pick CL via PRs at this time. It turns out not to be. See golang.org/issue/30037 for details.

There may be another fix being backported (per Brad's message above), so abandoning this.

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 Jan 24, 2019

dmitshur 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 Jan 24, 2019

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

dmitshur 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 Jan 25, 2019

dmitshur mentioned this pull request Jan 31, 2019

x/build/cmd/gerritbot: not possible to make cherry-pick CLs via PRs #30037

Open

gopherbot closed this Jan 31, 2019

dmitshur deleted the cherry-pick-29922 branch January 31, 2019 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[release-branch.go1.10] net/url, net/http: reject control characters in URLs #29926

[release-branch.go1.10] net/url, net/http: reject control characters in URLs #29926

dmitshur commented Jan 24, 2019

googlebot commented Jan 24, 2019

googlebot commented Jan 24, 2019

gopherbot commented Jan 24, 2019

gopherbot commented Jan 24, 2019

gopherbot commented Jan 24, 2019

gopherbot commented Jan 24, 2019

gopherbot commented Jan 24, 2019

gopherbot commented Jan 24, 2019

googlebot commented Jan 25, 2019

gopherbot commented Jan 25, 2019

googlebot commented Jan 25, 2019

gopherbot commented Jan 25, 2019

gopherbot commented Jan 25, 2019

gopherbot commented Jan 25, 2019

gopherbot commented Jan 25, 2019

gopherbot commented Jan 25, 2019

gopherbot commented Jan 25, 2019

gopherbot commented Jan 25, 2019

gopherbot commented Jan 28, 2019

gopherbot commented Jan 30, 2019

gopherbot commented Jan 30, 2019

gopherbot commented Jan 31, 2019

[release-branch.go1.10] net/url, net/http: reject control characters in URLs #29926

[release-branch.go1.10] net/url, net/http: reject control characters in URLs #29926

Conversation

dmitshur commented Jan 24, 2019

googlebot commented Jan 24, 2019

googlebot commented Jan 24, 2019

gopherbot commented Jan 24, 2019

gopherbot commented Jan 24, 2019

gopherbot commented Jan 24, 2019

gopherbot commented Jan 24, 2019

gopherbot commented Jan 24, 2019

gopherbot commented Jan 24, 2019

googlebot commented Jan 25, 2019

gopherbot commented Jan 25, 2019

googlebot commented Jan 25, 2019

gopherbot commented Jan 25, 2019

gopherbot commented Jan 25, 2019

gopherbot commented Jan 25, 2019

gopherbot commented Jan 25, 2019

gopherbot commented Jan 25, 2019

gopherbot commented Jan 25, 2019

gopherbot commented Jan 25, 2019

gopherbot commented Jan 28, 2019

gopherbot commented Jan 30, 2019

gopherbot commented Jan 30, 2019

gopherbot commented Jan 31, 2019