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

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

Closed
wants to merge 1 commit into
base: release-branch.go1.10
from

Conversation

Projects
None yet
4 participants
@dmitshur
Copy link
Member

dmitshur commented Jan 24, 2019

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

This comment has been minimized.

Copy link

googlebot commented Jan 24, 2019

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 googlebot added the cla: no label Jan 24, 2019

@dmitshur dmitshur added cla: yes and removed cla: no labels Jan 24, 2019

@googlebot

This comment has been minimized.

Copy link

googlebot commented Jan 24, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 24, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 24, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 24, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 24, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 24, 2019

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!

[release-branch.go1.10] net/url, net/http: reject control characters …
…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

This comment has been minimized.

Copy link

googlebot commented Jan 25, 2019

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 googlebot added cla: no and removed cla: yes labels Jan 25, 2019

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Jan 25, 2019

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

@dmitshur dmitshur added cla: yes and removed cla: no labels Jan 25, 2019

@googlebot

This comment has been minimized.

Copy link

googlebot commented Jan 25, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 25, 2019

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:

  1. It requires someone to manually verify CLAs and change the "cla: no" label to "cla: yes" in the PR. See #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.

  1. 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

This comment has been minimized.

Copy link

gopherbot commented Jan 25, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 25, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 25, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 25, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 25, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 25, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 28, 2019

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:

  1. It requires someone to manually verify CLAs and change the "cla: no" label to "cla: yes" in the PR. See #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.

  1. 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

This comment has been minimized.

Copy link

gopherbot commented Jan 30, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 30, 2019

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

This comment has been minimized.

Copy link

gopherbot commented Jan 31, 2019

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.

@gopherbot gopherbot closed this Jan 31, 2019

@dmitshur dmitshur deleted the dmitshur:cherry-pick-29922 branch Jan 31, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment