Repository: cloudwego/hertz
Pull request #988 (2023-11-06 03:09:43)

optimize(hz): thriftgo option transfer
#### What type of PR is this?
<!--
Add one of the following kinds:

build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
docs: Documentation only changes
feat: A new feature
optimize: A new optimization
fix: A bug fix
perf: A code change that improves performance
refactor: A code change that neither fixes a bug nor adds a feature
style: Changes that do not affect the meaning of the code (white space, formatting, missing semi-colons, etc)
test: Adding missing tests or correcting existing tests
chore: Changes to the build process or auxiliary tools and libraries such as documentation generation
-->
optimize
#### Check the PR title.
<!--
The description of the title will be attached in Release Notes, 
so please describe it from user-oriented, what this PR does / why we need it.
Please check your PR title with the below requirements:
-->
- [x] This PR title match the format: \<type\>(optional scope): \<description\>
- [x] The description of this PR title is user-oriented and clear enough for others to understand.
- [x] Attach the PR updating the user documentation if the current PR requires user awareness at the usage level. [User docs repo](https://github.com/cloudwego/cloudwego.github.io)


#### (Optional) Translate the PR title into Chinese.
优化透传 thriftgo 选项优先级的问题

#### (Optional) More detailed description for this PR(en: English/zh: Chinese).
<!--
Provide more detailed info for review(e.g., it's recommended to provide perf data if this is a perf type PR).
-->
en: optimize thriftgo option transfer
zh(optional): 优化 thriftgo 透传 option 的优先级问题；thriftgo 的 option 以后写入的为准，之前写法把 hz 默认携带的 option 放到了后面，导致用户无法覆盖这些 option

#### (Optional) Which issue(s) this PR fixes:
<!--
Automatically closes linked issue when PR is merged.
Eg: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
-->

#### (Optional) The PR that updates user documentation:
<!--
If the current PR requires user awareness at the usage level, please submit a PR to update user docs. [User docs repo](https://github.com/cloudwego/cloudwego.github.io)
-->

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/cloudwego/hertz/pull/988?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`e02c162`)](https://app.codecov.io/gh/cloudwego/hertz/commit/e02c16214bb98372093a3c7817287c208801de7f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego) 81.86% compared to head [(`7abf87b`)](https://app.codecov.io/gh/cloudwego/hertz/pull/988?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego) 81.86%.

> :exclamation: Current head 7abf87b differs from pull request most recent head c034a0b. Consider uploading reports for the commit c034a0b to get more accurate results

<details><summary>Additional details and impacted files</summary>


```diff
@@           Coverage Diff            @@
##           develop     #988   +/-   ##
========================================
  Coverage    81.86%   81.86%           
========================================
  Files           98       98           
  Lines         9932     9932           
========================================
  Hits          8131     8131           
  Misses        1311     1311           
  Partials       490      490           
```




</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/cloudwego/hertz/pull/988?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloudwego).


Patch:
diff --git a/.gitattributes b/.gitattributes
new file mode 100644
index 0000000..8c2820f
--- /dev/null
+++ b/.gitattributes
@@ -0,0 +1,7 @@
+# Handle line endings automatically for files detected as text
+# and leave all files detected as binary untouched.
+* text=auto eol=lf
+
+# Force bash scripts to always use LF line endings so that if a repo is accessed
+# in Unix via a file share from Windows, the scripts will work.
+*.sh text eol=lf
diff --git a/.github/workflows/vulncheck.yml b/.github/workflows/vulncheck.yml
new file mode 100644
index 0000000..9b88498
--- /dev/null
+++ b/.github/workflows/vulncheck.yml
@@ -0,0 +1,35 @@
+name: Run govulncheck
+
+on:
+  push:
+    branches:
+      - develop
+    paths:
+      - "**"
+      - "!**.md"
+  pull_request:
+    paths:
+      - "**"
+      - "!**.md"
+
+jobs:
+  vulncheck-check:
+    runs-on: ubuntu-latest
+    env:
+      GO111MODULE: on
+    steps:
+      - name: Fetch Repository
+        uses: actions/checkout@v4
+
+      - name: Install Go
+        uses: actions/setup-go@v4
+        with:
+          go-version: "stable"
+          check-latest: true
+          cache: false
+
+      - name: Install Govulncheck
+        run: go install golang.org/x/vuln/cmd/govulncheck@latest
+
+      - name: Run Govulncheck
+        run: govulncheck ./...
diff --git a/cmd/hz/config/cmd.go b/cmd/hz/config/cmd.go
index 983946c..fc83c57 100644
--- a/cmd/hz/config/cmd.go
+++ b/cmd/hz/config/cmd.go
@@ -173,6 +173,7 @@ func BuildPluginCmd(args *Argument) (*exec.Cmd, error) {
 }
 
 func (arg *Argument) GetThriftgoOptions() (string, error) {
+	defaultOpt := "reserve_comments,gen_json_tag=false,"
 	prefix, err := arg.ModelPackagePrefix()
 	if err != nil {
 		return "", err
@@ -181,6 +182,6 @@ func (arg *Argument) GetThriftgoOptions() (string, error) {
 	if arg.JSONEnumStr {
 		arg.ThriftOptions = append(arg.ThriftOptions, "json_enum_as_text")
 	}
-	gas := "go:" + strings.Join(arg.ThriftOptions, ",") + ",reserve_comments,gen_json_tag=false"
+	gas := "go:" + defaultOpt + strings.Join(arg.ThriftOptions, ",")
 	return gas, nil
 }
diff --git a/pkg/protocol/http1/server.go b/pkg/protocol/http1/server.go
index 66cb01e..ef19cd7 100644
--- a/pkg/protocol/http1/server.go
+++ b/pkg/protocol/http1/server.go
@@ -369,11 +369,6 @@ func (s Server) Serve(c context.Context, conn network.Conn) (err error) {
 			return
 		}
 
-		// general case
-		if s.EnableTrace {
-			traceCtl.DoFinish(cc, ctx, err)
-		}
-
 		if connectionClose {
 			return errShortConnection
 		}
@@ -382,6 +377,10 @@ func (s Server) Serve(c context.Context, conn network.Conn) (err error) {
 		if s.IdleTimeout == 0 {
 			return
 		}
+		// general case
+		if s.EnableTrace {
+			traceCtl.DoFinish(cc, ctx, err)
+		}
 
 		ctx.ResetWithoutConn()
 	}
diff --git a/pkg/protocol/uri.go b/pkg/protocol/uri.go
index 6bba7a9..4fd8788 100644
--- a/pkg/protocol/uri.go
+++ b/pkg/protocol/uri.go
@@ -49,6 +49,7 @@ import (
 	"github.com/cloudwego/hertz/internal/bytesconv"
 	"github.com/cloudwego/hertz/internal/bytestr"
 	"github.com/cloudwego/hertz/internal/nocopy"
+	"github.com/cloudwego/hertz/pkg/common/hlog"
 )
 
 // AcquireURI returns an empty URI instance from the pool.
@@ -373,6 +374,34 @@ func (u *URI) Parse(host, uri []byte) {
 	u.parse(host, uri, false)
 }
 
+// Maybe rawURL is of the form scheme:path.
+// (Scheme must be [a-zA-Z][a-zA-Z0-9+-.]*)
+// If so, return scheme, path; else return nil, rawURL.
+func getScheme(rawURL []byte) (scheme, path []byte) {
+	for i := 0; i < len(rawURL); i++ {
+		c := rawURL[i]
+		switch {
+		case 'a' <= c && c <= 'z' || 'A' <= c && c <= 'Z':
+		// do nothing
+		case '0' <= c && c <= '9' || c == '+' || c == '-' || c == '.':
+			if i == 0 {
+				return nil, rawURL
+			}
+		case c == ':':
+			if i == 0 {
+				hlog.Errorf("error happened when try to parse the rawURL(%s): missing protocol scheme", rawURL)
+				return nil, nil
+			}
+			return rawURL[:i], rawURL[i+1:]
+		default:
+			// we have encountered an invalid character,
+			// so there is no valid scheme
+			return nil, rawURL
+		}
+	}
+	return nil, rawURL
+}
+
 func (u *URI) parse(host, uri []byte, isTLS bool) {
 	u.Reset()
 
@@ -455,20 +484,14 @@ func stringContainsCTLByte(s []byte) bool {
 }
 
 func splitHostURI(host, uri []byte) ([]byte, []byte, []byte) {
-	n := bytes.Index(uri, bytestr.StrSlashSlash)
-	if n < 0 {
-		return bytestr.StrHTTP, host, uri
-	}
-	scheme := uri[:n]
-	if bytes.IndexByte(scheme, '/') >= 0 {
+	scheme, path := getScheme(uri)
+
+	if scheme == nil {
 		return bytestr.StrHTTP, host, uri
 	}
-	if len(scheme) > 0 && scheme[len(scheme)-1] == ':' {
-		scheme = scheme[:len(scheme)-1]
-	}
-	n += len(bytestr.StrSlashSlash)
-	uri = uri[n:]
-	n = bytes.IndexByte(uri, '/')
+
+	uri = path[len(bytestr.StrSlashSlash):]
+	n := bytes.IndexByte(uri, '/')
 	if n < 0 {
 		// A hack for bogus urls like foobar.com?a=b without
 		// slash after host.
@@ -587,6 +610,9 @@ func (u *URI) updateBytes(newURI, buf []byte) []byte {
 		if len(u.scheme) > 0 {
 			schemeOriginal = append([]byte(nil), u.scheme...)
 		}
+		if n == 0 {
+			newURI = bytes.Join([][]byte{u.scheme, bytestr.StrColon, newURI}, nil)
+		}
 		u.Parse(nil, newURI)
 		if len(schemeOriginal) > 0 && len(u.scheme) == 0 {
 			u.scheme = append(u.scheme[:0], schemeOriginal...)
diff --git a/pkg/route/engine.go b/pkg/route/engine.go
index 168c79b..6ab2de6 100644
--- a/pkg/route/engine.go
+++ b/pkg/route/engine.go
@@ -89,6 +89,8 @@ var (
 	default404Body = []byte("404 page not found")
 	default405Body = []byte("405 method not allowed")
 	default400Body = []byte("400 bad request")
+
+	requiredHostBody = []byte("missing required Host header")
 )
 
 type hijackConn struct {
@@ -721,6 +723,13 @@ func (engine *Engine) ServeHTTP(c context.Context, ctx *app.RequestContext) {
 	}
 
 	rPath := string(ctx.Request.URI().Path())
+
+	// align with https://datatracker.ietf.org/doc/html/rfc2616#section-5.2
+	if len(ctx.Request.Host()) == 0 && ctx.Request.Header.IsHTTP11() && bytesconv.B2s(ctx.Request.Method()) != consts.MethodConnect {
+		serveError(c, ctx, consts.StatusBadRequest, requiredHostBody)
+		return
+	}
+
 	httpMethod := bytesconv.B2s(ctx.Request.Header.Method())
 	unescape := false
 	if engine.options.UseRawPath {


Test patch:
diff --git a/pkg/app/server/hertz_test.go b/pkg/app/server/hertz_test.go
index 5baa3ec..036df76 100644
--- a/pkg/app/server/hertz_test.go
+++ b/pkg/app/server/hertz_test.go
@@ -259,7 +259,7 @@ func TestNotAbsolutePath(t *testing.T) {
 	go engine.Run()
 	time.Sleep(200 * time.Microsecond)
 
-	s := "POST ?a=b HTTP/1.1\r\nContent-Length: 5\r\nContent-Type: foo/bar\r\n\r\nabcdef4343"
+	s := "POST ?a=b HTTP/1.1\r\nHost: a.b.c\r\nContent-Length: 5\r\nContent-Type: foo/bar\r\n\r\nabcdef4343"
 	zr := mock.NewZeroCopyReader(s)
 
 	ctx := app.NewContext(0)
@@ -270,7 +270,7 @@ func TestNotAbsolutePath(t *testing.T) {
 	assert.DeepEqual(t, consts.StatusOK, ctx.Response.StatusCode())
 	assert.DeepEqual(t, ctx.Request.Body(), ctx.Response.Body())
 
-	s = "POST a?a=b HTTP/1.1\r\nContent-Length: 5\r\nContent-Type: foo/bar\r\n\r\nabcdef4343"
+	s = "POST a?a=b HTTP/1.1\r\nHost: a.b.c\r\nContent-Length: 5\r\nContent-Type: foo/bar\r\n\r\nabcdef4343"
 	zr = mock.NewZeroCopyReader(s)
 
 	ctx = app.NewContext(0)
@@ -291,7 +291,7 @@ func TestNotAbsolutePathWithRawPath(t *testing.T) {
 	go engine.Run()
 	time.Sleep(200 * time.Microsecond)
 
-	s := "POST ?a=b HTTP/1.1\r\nContent-Length: 5\r\nContent-Type: foo/bar\r\n\r\nabcdef4343"
+	s := "POST ?a=b HTTP/1.1\r\nHost: a.b.c\r\nContent-Length: 5\r\nContent-Type: foo/bar\r\n\r\nabcdef4343"
 	zr := mock.NewZeroCopyReader(s)
 
 	ctx := app.NewContext(0)
@@ -302,7 +302,7 @@ func TestNotAbsolutePathWithRawPath(t *testing.T) {
 	assert.DeepEqual(t, consts.StatusBadRequest, ctx.Response.StatusCode())
 	assert.DeepEqual(t, default400Body, ctx.Response.Body())
 
-	s = "POST a?a=b HTTP/1.1\r\nContent-Length: 5\r\nContent-Type: foo/bar\r\n\r\nabcdef4343"
+	s = "POST a?a=b HTTP/1.1\r\nHost: a.b.c\r\nContent-Length: 5\r\nContent-Type: foo/bar\r\n\r\nabcdef4343"
 	zr = mock.NewZeroCopyReader(s)
 
 	ctx = app.NewContext(0)
diff --git a/pkg/protocol/http1/server_test.go b/pkg/protocol/http1/server_test.go
index e34bb3f..5f8f782 100644
--- a/pkg/protocol/http1/server_test.go
+++ b/pkg/protocol/http1/server_test.go
@@ -359,6 +359,52 @@ func TestExpect100ContinueHandler(t *testing.T) {
 	assert.DeepEqual(t, "", string(response.Body()))
 }
 
+type mockController struct {
+	FinishTimes int
+}
+
+func (m *mockController) Append(col tracer.Tracer) {}
+
+func (m *mockController) DoStart(ctx context.Context, c *app.RequestContext) context.Context {
+	return ctx
+}
+
+func (m *mockController) DoFinish(ctx context.Context, c *app.RequestContext, err error) {
+	m.FinishTimes++
+}
+
+func (m *mockController) HasTracer() bool { return true }
+
+func (m *mockController) reset() { m.FinishTimes = 0 }
+
+func TestTraceDoFinishTimes(t *testing.T) {
+	server := &Server{}
+	server.eventStackPool = pool
+	server.EnableTrace = true
+	reqCtx := &app.RequestContext{}
+	controller := &mockController{}
+	server.Core = &mockCore{
+		ctxPool: &sync.Pool{New: func() interface{} {
+			ti := traceinfo.NewTraceInfo()
+			ti.Stats().SetLevel(2)
+			reqCtx.SetTraceInfo(&mockTraceInfo{ti})
+			return reqCtx
+		}},
+		controller: controller,
+	}
+	// for disableKeepAlive case
+	server.DisableKeepalive = true
+	err := server.Serve(context.TODO(), mock.NewConn("GET /aaa HTTP/1.1\nHost: foobar.com\n\n"))
+	assert.True(t, errors.Is(err, errs.ErrShortConnection))
+	assert.DeepEqual(t, 1, controller.FinishTimes)
+	// for IdleTimeout==0 case
+	server.IdleTimeout = 0
+	controller.reset()
+	err = server.Serve(context.TODO(), mock.NewConn("GET /aaa HTTP/1.1\nHost: foobar.com\n\n"))
+	assert.True(t, errors.Is(err, errs.ErrShortConnection))
+	assert.DeepEqual(t, 1, controller.FinishTimes)
+}
+
 type mockCore struct {
 	ctxPool     *sync.Pool
 	controller  tracer.Controller
diff --git a/pkg/protocol/uri_test.go b/pkg/protocol/uri_test.go
index e245faf..3578dda 100644
--- a/pkg/protocol/uri_test.go
+++ b/pkg/protocol/uri_test.go
@@ -42,6 +42,7 @@
 package protocol
 
 import (
+	"bytes"
 	"path/filepath"
 	"reflect"
 	"runtime"
@@ -468,3 +469,35 @@ func TestParseURI(t *testing.T) {
 	uri := string(ParseURI(expectURI).FullURI())
 	assert.DeepEqual(t, expectURI, uri)
 }
+
+func TestSplitHostURI(t *testing.T) {
+	cases := []struct {
+		host, uri                      []byte
+		wantScheme, wantHost, wantPath []byte
+	}{
+		{
+			[]byte("example.com"), []byte("/foobar"),
+			[]byte("http"), []byte("example.com"), []byte("/foobar"),
+		},
+		{
+			[]byte("example2.com"), []byte("http://example2.com"),
+			[]byte("http"), []byte("example2.com"), []byte("/"),
+		},
+		{
+			[]byte("example2.com"), []byte("http://example3.com"),
+			[]byte("http"), []byte("example3.com"), []byte("/"),
+		},
+		{
+			[]byte("example3.com"), []byte("https://foobar.com?a=b"),
+			[]byte("https"), []byte("foobar.com"), []byte("?a=b"),
+		},
+	}
+
+	for _, c := range cases {
+		gotScheme, gotHost, gotPath := splitHostURI(c.host, c.uri)
+		if !bytes.Equal(gotScheme, c.wantScheme) || !bytes.Equal(gotHost, c.wantHost) || !bytes.Equal(gotPath, c.wantPath) {
+			t.Errorf("splitHostURI(%q, %q) == (%q, %q, %q), want (%q, %q, %q)",
+				c.host, c.uri, gotScheme, gotHost, gotPath, c.wantScheme, c.wantHost, c.wantPath)
+		}
+	}
+}