Skip to content
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

Jump to bottom

x/website: 1.20 release notes should mention updated cookie validation #58485

Closed
wfernandes opened this issue Feb 13, 2023 · 8 comments
Closed

x/website: 1.20 release notes should mention updated cookie validation #58485

wfernandes opened this issue Feb 13, 2023 · 8 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. website
Milestone
Unreleased

Comments

@wfernandes
Copy link
Contributor

What version of Go are you using (go version)?

$ go version
go version go1.19.5 darwin/arm64

Does this issue reproduce with the latest release?

Yes. This issue is a difference in behavior between go1.19.5 and go 1.20

What operating system and processor architecture are you using (go env)?

go env Trimmed Output
$ go env
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOOS="darwin"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/opt/homebrew/Cellar/go/1.19.5/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/homebrew/Cellar/go/1.19.5/libexec/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.19.5"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/34/g9npzgvx7fd1b09b88ljtdtw0000gn/T/go-build2978260838=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

package main

import (
	"fmt"
	"net/http"
)

func main() {

	c := &http.Cookie{Name: "valid"}
	if err := c.Valid(); err != nil {
		fmt.Println(err.Error())
		return
	}
	fmt.Println("valid cookie")
}

https://go.dev/play/p/Epzlq8U-v_N

Upon running the above example code in go1.20 we get the output of valid cookie however when we switch to the previous minor version go1.19, we get the error http: invalid Cookie.Expires.

What did you expect to see?

I expected to see the same behavior between versions since this behavior was not explicitly documented in the go1.20 release notes for net/http.

What did you see instead?

I saw a difference in behavior.
In go1.20 we check Cookie.Expires.IsZero however in go1.19 we don't perform that additional check.
https://github.com/golang/go/blob/release-branch.go1.20/src/net/http/cookie.go#L250
vs.
https://github.com/golang/go/blob/release-branch.go1.19/src/net/http/cookie.go#L249
@wfernandes
Copy link
Contributor Author

Just to be clear, I think the go1.20 version does have the better behavior and I prefer it TBH. However, it would be nice to see the change documented in the go1.20 release notes if possible. Thanks.

@0xmohit
Copy link
Contributor

0xmohit commented Feb 13, 2023

Related: #52989

@seankhliao seankhliao changed the title net/http: validating cookie behavior differs between go1.19 and go1.20 x/website: 1.20 release notes should mention updated cookie validation Feb 13, 2023
@gopherbot gopherbot added this to the Unreleased milestone Feb 13, 2023
@seankhliao
Copy link
Member

feel free to send a CL

@wfernandes
Copy link
Contributor Author

@seankhliao Thanks for the feedback. I would love to make a CL! Could you point me to the repository where I could make this documentation change?

@seankhliao
Copy link
Member

https://go.googlesource.com/website/+/refs/heads/master/_content/doc/go1.20.html

@prattmic prattmic added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 14, 2023
@prattmic
Copy link
Member

cc @amitsaha @neild

@wfernandes wfernandes mentioned this issue Feb 16, 2023
Closed
@wfernandes
Copy link
Contributor Author

@prattmic @seankhliao I created the above PR/CL to update the go1.20 release notes. Please let me know if there are any changes that are needed.

wfernandes added a commit to wfernandes/website that referenced this issue Feb 17, 2023
@wfernandes
_content/doc/go1.20: add release note for new cookie.valid behavior
6355670 
Fixes golang/go#58485

Add a release note documenting that Cookie.Valid ignores the
Cookie.Expires field when empty.

Signed-off-by: Warren Fernandes <warren.f.fernandes@gmail.com>
@wfernandes
Copy link
Contributor Author

I think this issue can be closed since golang/website#193 has been merged.
Also see https://go-review.googlesource.com/c/website/+/468716.

Thanks for the help and direction.

@golang golang locked and limited conversation to collaborators Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. website
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@prattmic @wfernandes @0xmohit @gopherbot @seankhliao