Restrict the usage of http.Client for otel

Makefile

@@ -18,7 +18,6 @@ vendor:
 	go mod download
 	go install github.com/golang/mock/mockgen
 	go install github.com/google/wire/cmd/wire
-	go install github.com/authgear/go-vet-timeunixutc/cmd/govettimeunixutc@1e0ad32ff28a52d3c7aa949f9139b0a068f64090
 	go install golang.org/x/vuln/cmd/govulncheck@latest
 	go install golang.org/x/tools/cmd/goimports@latest
 	go install go.k6.io/xk6/cmd/xk6@latest
@@ -49,7 +48,7 @@ generate:
 .PHONY: test
 test:
 	$(MAKE) -C ./k6 go-test
-	go test ./cmd/... ./pkg/... -timeout 1m30s
+	go test ./devtools/goanalysis/... ./cmd/... ./pkg/... -timeout 1m30s
 
 .PHONY: lint-translation-keys
 lint-translation-keys:
@@ -59,7 +58,6 @@ lint-translation-keys:
 .PHONY: lint
 lint:
 	golangci-lint run ./cmd/... ./pkg/... --timeout 7m
-	govettimeunixutc ./cmd/... ./pkg/...
 	go run ./devtools/translationlinter
 	-go run ./devtools/importlinter api api >.make-lint-expect 2>&1
 	-go run ./devtools/importlinter lib api util >> .make-lint-expect 2>&1
@@ -74,6 +72,7 @@ lint:
 	git diff --exit-code .make-lint-expect > /dev/null 2>&1
 	go run ./devtools/gotemplatelinter --ignore-rule translation-key ./resources/authgear/templates/en/web/authflowv2
 	$(MAKE) lint-translation-keys
+	go run ./devtools/goanalysis ./cmd/... ./pkg/...
 
 .PHONY: sort-translations
 sort-translations:
@@ -82,7 +81,7 @@ sort-translations:
 .PHONY: fmt
 fmt:
 	# Ignore generated files, such as wire_gen.go and *_mock_test.go
-	find ./pkg ./cmd ./e2e -name '*.go' -not -name 'wire_gen.go' -not -name '*_mock_test.go' | sort | xargs goimports -w -format-only -local github.com/authgear/authgear-server
+	find ./devtools ./pkg ./cmd ./e2e -name '*.go' -not -name 'wire_gen.go' -not -name '*_mock_test.go' | sort | xargs goimports -w -format-only -local github.com/authgear/authgear-server
 	$(MAKE) sort-translations
 
 .PHONY: govulncheck

cmd/authgear/background/deps.go

@@ -37,7 +37,8 @@ func NewNoopTaskQueue() NoopTaskQueue {
 // This dummy HTTP request is only used for get/set cookie
 // which does not have any effect at all.
 func NewDummyHTTPRequest() *http.Request {
-	r, _ := http.NewRequest("", "", nil)
+	ctx := context.Background()
+	r, _ := http.NewRequestWithContext(ctx, "", "", nil)
 	return r
 }
 

custombuild/go.mod

@@ -6,7 +6,7 @@ replace github.com/authgear/authgear-server v0.0.0 => ../
 
 require (
 	github.com/authgear/authgear-server v0.0.0
-	github.com/authgear/iamsmart v0.0.0-20240527095947-f47aa4fc0702
+	github.com/authgear/iamsmart v0.0.0-20241101122333-a35f5efd0acd
 	github.com/joho/godotenv v1.5.1
 	go.uber.org/automaxprocs v1.6.0
 	k8s.io/client-go v0.30.6

custombuild/go.sum

@@ -69,8 +69,8 @@ github.com/alicebob/miniredis/v2 v2.33.0 h1:uvTF0EDeu9RLnUEG27Db5I68ESoIxTiXbNUi
 github.com/alicebob/miniredis/v2 v2.33.0/go.mod h1:MhP4a3EU7aENRi9aO+tHfTBZicLqQevyi/DJpoj6mi0=
 github.com/authgear/graphql-go-relay v0.0.0-20240429082917-f56c3cce72ba h1:3KVNeYLx5vbpphhkEMu+ew281AEjZDCqHFxINrgVXsY=
 github.com/authgear/graphql-go-relay v0.0.0-20240429082917-f56c3cce72ba/go.mod h1:Nmot+3hWButYXa8FfWIfTaoaVOmCNUUa/eyrLnMSzYc=
-github.com/authgear/iamsmart v0.0.0-20240527095947-f47aa4fc0702 h1:PdeCJ2RKO1IjWshx+LiGYSFqT1CqCKoS637BY4Qr3R4=
-github.com/authgear/iamsmart v0.0.0-20240527095947-f47aa4fc0702/go.mod h1:Wm0yxpOHcTAHk/iyuMC5+BDUeU0qtGwg2dsXUB8EX7o=
+github.com/authgear/iamsmart v0.0.0-20241101122333-a35f5efd0acd h1:qI3bDdcslb5ywUNLlS5Evc+ku3DxuTCzyIvbMSmhbik=
+github.com/authgear/iamsmart v0.0.0-20241101122333-a35f5efd0acd/go.mod h1:3lLi+xVYSLCO+bcNEQlVtzYfIGtFBQ9makuME5rdEW8=
 github.com/authgear/oauthrelyingparty v1.5.0 h1:Gysu4Y+XfOy/WKPieIOZ4d1iIHsC4Hek2o1kSAtDW2k=
 github.com/authgear/oauthrelyingparty v1.5.0/go.mod h1:0UcO0p5eS9BPLulX6K7TvkXkuPeTn3QhAJ/UQg4fmiQ=
 github.com/aws/aws-sdk-go v1.55.5 h1:KKUZBfBoyqy5d3swXyiC7Q76ic40rYcbqH7qjh59kzU=

devtools/goanalysis/main.go

@@ -0,0 +1,12 @@
+package main
+
+import (
+	"golang.org/x/tools/go/analysis/multichecker"
+
+	"github.com/authgear/authgear-server/devtools/goanalysis/pkg/httpclient"
+	"github.com/authgear/authgear-server/devtools/goanalysis/pkg/timeunixutc"
+)
+
+func main() {
+	multichecker.Main(httpclient.Analyzer, timeunixutc.Analyzer)
+}

devtools/goanalysis/pkg/httpclient/analyzer.go

@@ -0,0 +1,136 @@
+package httpclient
+
+import (
+	"go/ast"
+	"go/types"
+	"strings"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+)
+
+var Analyzer = &analysis.Analyzer{
+	Name: "httpclient",
+	Doc:  "httpclient restricts the way how a http.Client can be used.",
+	Run:  run,
+	// See https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/inspect
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+}
+
+func isATestFile(pass *analysis.Pass, n ast.Node) bool {
+	position := pass.Fset.Position(n.Pos())
+	// position is valid if Line > 0
+	// See https://pkg.go.dev/go/token#Position
+	if position.Line > 0 {
+		if strings.HasSuffix(position.Filename, "_test.go") {
+			return true
+		}
+	}
+	return false
+}
+
+func isOfFilename(pass *analysis.Pass, n ast.Node, filename string) bool {
+	position := pass.Fset.Position(n.Pos())
+	// position is valid if Line > 0
+	// See https://pkg.go.dev/go/token#Position
+	if position.Line > 0 {
+		return position.Filename == filename
+	}
+	return false
+}
+
+func isOfPackagePath(pass *analysis.Pass, packagePath string) bool {
+	return pass.Pkg.Path() == packagePath
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+	traverse := func(n ast.Node) {
+		isTestFile := isATestFile(pass, n)
+		isHttputil := isOfPackagePath(pass, "github.com/authgear/authgear-server/pkg/util/httputil")
+		isExtClientDotGo := isOfFilename(pass, n, "ext_client.go")
+
+		if !isTestFile && !isHttputil && !isExtClientDotGo {
+			if n, ok := n.(*ast.CompositeLit); ok {
+				if typ := pass.TypesInfo.TypeOf(n.Type); typ != nil {
+					if typ.String() == "net/http.Client" {
+						pass.Reportf(n.Pos(), "Constructing http.Client directly is forbidden. Use httputil.NewExternalClient instead.")
+					}
+				}
+			}
+		}
+
+		if !isTestFile {
+			if n, ok := n.(*ast.SelectorExpr); ok {
+				if selObj := pass.TypesInfo.ObjectOf(n.Sel); selObj != nil {
+					if v, ok := selObj.(*types.Var); ok {
+						if v.String() == "var net/http.DefaultClient *net/http.Client" {
+							pass.Reportf(n.Pos(), "Using http.DefaultClient is forbidden. Use httputil.NewExternalClient instead.")
+						}
+					}
+				}
+			}
+		}
+
+		if !isTestFile {
+			if n, ok := n.(*ast.SelectorExpr); ok {
+				if selObj := pass.TypesInfo.ObjectOf(n.Sel); selObj != nil {
+					if f, ok := selObj.(*types.Func); ok {
+						fullName := f.FullName()
+						if fullName == "net/http.NewRequest" {
+							pass.Reportf(n.Pos(), "Calling http.NewRequest is forbidden. Use http.NewRequestWithContext instead.")
+						}
+					}
+				}
+			}
+		}
+
+		if !isTestFile {
+			if n, ok := n.(*ast.SelectorExpr); ok {
+				if selObj := pass.TypesInfo.ObjectOf(n.Sel); selObj != nil {
+					if f, ok := selObj.(*types.Func); ok {
+						fullName := f.FullName()
+						switch fullName {
+						case "net/http.Get":
+							fallthrough
+						case "net/http.Head":
+							fallthrough
+						case "net/http.Post":
+							fallthrough
+						case "net/http.PostForm":
+							pass.Reportf(n.Pos(), "context.Context is lost. Use http.Client.Do instead.")
+						default:
+							break
+						}
+					}
+				}
+			}
+		}
+
+		if !isTestFile {
+			if n, ok := n.(*ast.SelectorExpr); ok {
+				if selObj := pass.TypesInfo.ObjectOf(n.Sel); selObj != nil {
+					if fun, ok := selObj.(*types.Func); ok {
+						fullName := fun.FullName()
+						switch fullName {
+						case "(*net/http.Client).Get":
+							fallthrough
+						case "(*net/http.Client).Head":
+							fallthrough
+						case "(*net/http.Client).Post":
+							fallthrough
+						case "(*net/http.Client).PostForm":
+							pass.Reportf(n.Pos(), "context.Context is lost. Use http.Client.Do instead.")
+						default:
+							break
+						}
+					}
+				}
+			}
+		}
+	}
+
+	inspect.Preorder(nil, traverse)
+	return nil, nil
+}

devtools/goanalysis/pkg/httpclient/analyzer_test.go

@@ -0,0 +1,11 @@
+package httpclient
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+func TestAnalyzer(t *testing.T) {
+	analysistest.Run(t, analysistest.TestData(), Analyzer, "basic")
+}

devtools/goanalysis/pkg/httpclient/testdata/src/basic/basic.go

@@ -0,0 +1,65 @@
+package basic
+
+import (
+	"context"
+	"net/http"
+	aliasedhttp "net/http"
+)
+
+func ConstructingHTTPClient() {
+	_ = http.Client{}         // want `Constructing http.Client directly is forbidden. Use httputil.NewExternalClient instead.`
+	_ = &http.Client{}        // want `Constructing http.Client directly is forbidden. Use httputil.NewExternalClient instead.`
+	_ = aliasedhttp.Client{}  // want `Constructing http.Client directly is forbidden. Use httputil.NewExternalClient instead.`
+	_ = &aliasedhttp.Client{} // want `Constructing http.Client directly is forbidden. Use httputil.NewExternalClient instead.`
+}
+
+func UseHTTPDefaultClient() {
+	_ = http.DefaultClient // want `Using http.DefaultClient is forbidden. Use httputil.NewExternalClient instead.`
+}
+
+func ConstructingHTTPRequest() {
+	_, _ = aliasedhttp.NewRequestWithContext(context.Background(), "GET", "", nil)
+	_, _ = http.NewRequestWithContext(context.Background(), "GET", "", nil)
+
+	_, _ = http.NewRequest("GET", "", nil)        // want `Calling http.NewRequest is forbidden. Use http.NewRequestWithContext instead.`
+	_, _ = aliasedhttp.NewRequest("GET", "", nil) // want `Calling http.NewRequest is forbidden. Use http.NewRequestWithContext instead.`
+}
+
+func UseHTTPDefaultClientImplicitly() {
+	_, _ = http.Get("")           // want `context.Context is lost. Use http.Client.Do instead.`
+	_, _ = http.Head("")          // want `context.Context is lost. Use http.Client.Do instead.`
+	_, _ = http.Post("", "", nil) // want `context.Context is lost. Use http.Client.Do instead.`
+	_, _ = http.PostForm("", nil) // want `context.Context is lost. Use http.Client.Do instead.`
+}
+
+func UseMethodsOtherThanDo() {
+	nonPointerHTTPClient := http.Client{}         // want `Constructing http.Client directly is forbidden. Use httputil.NewExternalClient instead.`
+	_, _ = nonPointerHTTPClient.Get("")           // want `context.Context is lost. Use http.Client.Do instead.`
+	_, _ = nonPointerHTTPClient.Head("")          // want `context.Context is lost. Use http.Client.Do instead.`
+	_, _ = nonPointerHTTPClient.Post("", "", nil) // want `context.Context is lost. Use http.Client.Do instead.`
+	_, _ = nonPointerHTTPClient.PostForm("", nil) // want `context.Context is lost. Use http.Client.Do instead.`
+
+	pointerHTTPClient := &http.Client{}        // want `Constructing http.Client directly is forbidden. Use httputil.NewExternalClient instead.`
+	_, _ = pointerHTTPClient.Get("")           // want `context.Context is lost. Use http.Client.Do instead.`
+	_, _ = pointerHTTPClient.Head("")          // want `context.Context is lost. Use http.Client.Do instead.`
+	_, _ = pointerHTTPClient.Post("", "", nil) // want `context.Context is lost. Use http.Client.Do instead.`
+	_, _ = pointerHTTPClient.PostForm("", nil) // want `context.Context is lost. Use http.Client.Do instead.`
+
+	type EmbeddedStructClient struct {
+		http.Client
+	}
+	embeddedStructClient := EmbeddedStructClient{}
+	_, _ = embeddedStructClient.Get("")           // want `context.Context is lost. Use http.Client.Do instead.`
+	_, _ = embeddedStructClient.Head("")          // want `context.Context is lost. Use http.Client.Do instead.`
+	_, _ = embeddedStructClient.Post("", "", nil) // want `context.Context is lost. Use http.Client.Do instead.`
+	_, _ = embeddedStructClient.PostForm("", nil) // want `context.Context is lost. Use http.Client.Do instead.`
+
+	type EmbeddedPointerClient struct {
+		*http.Client
+	}
+	embeddedPointerClient := EmbeddedPointerClient{}
+	_, _ = embeddedPointerClient.Get("")           // want `context.Context is lost. Use http.Client.Do instead.`
+	_, _ = embeddedPointerClient.Head("")          // want `context.Context is lost. Use http.Client.Do instead.`
+	_, _ = embeddedPointerClient.Post("", "", nil) // want `context.Context is lost. Use http.Client.Do instead.`
+	_, _ = embeddedPointerClient.PostForm("", nil) // want `context.Context is lost. Use http.Client.Do instead.`
+}