Block redirect-following and reject malformed auth responses across Node/Java/Go SDKs

.github/workflows/generate-check.yml

@@ -8,19 +8,7 @@ name: Generate Check
 on:
   push:
     branches: [main]
-    paths:
-      - "shared/**"
-      - "node/scripts/gen-route-meta.mjs"
-      - "java/scripts/gen-route-meta.mjs"
-      - "go/generated/oapi-codegen-config.yaml"
-      - ".github/workflows/generate-check.yml"
-  pull_request:
-    paths:
-      - "shared/**"
-      - "node/scripts/gen-route-meta.mjs"
-      - "java/scripts/gen-route-meta.mjs"
-      - "go/generated/oapi-codegen-config.yaml"
-      - ".github/workflows/generate-check.yml"
+  pull_request: {}
 
 jobs:
   generate-check:

DEVELOPMENT.md

@@ -39,7 +39,13 @@ cd java && ./gradlew checkstyleMain checkstyleTest spotlessCheck
 cd go && gofmt -l . && go vet ./... && staticcheck ./...
 ```
 
-三端都遵循 Google 官方风格指南：TS 用 `gts`（`npm run lint` / `npm run fix`），Java 用 `google-java-format`（`./gradlew spotlessApply` 自动修复）+ Checkstyle（`google_checks.xml`），Go 用 `gofmt`/`go vet`/`staticcheck`。
+三端都遵循 Google 官方风格指南，具体文档地址：
+
+- **TypeScript**：[Google TypeScript Style Guide](https://google.github.io/styleguide/tsguide.html)，由 [`gts`](https://github.com/google/gts) 落地（`npm run lint` 检查 / `npm run fix` 自动修复）。
+- **Java**：[Google Java Style Guide](https://google.github.io/styleguide/javaguide.html)，由 Checkstyle 的 `google_checks.xml`（随 checkstyle 工具本体分发）做静态检查，`google-java-format` 通过 Spotless 插件（`./gradlew spotlessApply` 自动修复）落地排版规则。
+- **Go**：[Google Go Style Guide](https://google.github.io/styleguide/go/)（含 [Style Guide](https://google.github.io/styleguide/go/guide) / [Style Decisions](https://google.github.io/styleguide/go/decisions) / [Best Practices](https://google.github.io/styleguide/go/best-practices) 三部分），目前落地手段是 `gofmt` + `go vet` + `staticcheck`；仓库暂未接入面向 Google Go Style 的专用 lint 配置（如 golangci-lint），差异需要人工对照上述文档走查。
+
+三端提交前必须跑通对应语言的 lint/format 检查（见上一节命令），不允许绕过。
 
 ## 测试
 

go/auth.go

@@ -170,6 +170,10 @@ func (a *AuthService) exchangeClientCredentials(ctx context.Context) (string, er
 		return "", fmt.Errorf("qdmp: failed to decode auth.token response: %w", err)
 	}
 
+	if result.AccessToken == "" {
+		return "", fmt.Errorf("qdmp: auth.token response has an empty accessToken")
+	}
+
 	expiresAt, err := strconv.ParseInt(result.ExpiresAt, 10, 64)
 	if err != nil {
 		return "", fmt.Errorf("qdmp: auth.token response has a non-numeric expiresAt: %w", err)
@@ -201,6 +205,9 @@ func (a *AuthService) Code2Session(ctx context.Context, code string) (*Code2Sess
 	if err := json.Unmarshal(data, &result); err != nil {
 		return nil, fmt.Errorf("qdmp: failed to decode auth.code2Session response: %w", err)
 	}
+	if result.AccessToken == "" {
+		return nil, fmt.Errorf("qdmp: auth.code2Session response has an empty accessToken")
+	}
 	return &result, nil
 }
 
@@ -223,5 +230,8 @@ func (a *AuthService) RefreshToken(ctx context.Context, refreshToken string) (*R
 	if err := json.Unmarshal(data, &result); err != nil {
 		return nil, fmt.Errorf("qdmp: failed to decode auth.refreshToken response: %w", err)
 	}
+	if result.AccessToken == "" {
+		return nil, fmt.Errorf("qdmp: auth.refreshToken response has an empty accessToken")
+	}
 	return &result, nil
 }

go/auth_empty_access_token_test.go

@@ -0,0 +1,99 @@
+package qdmp_test
+
+// Contract exercised by this file (not yet implemented — see the codex
+// review round1 fix task): AuthService.exchangeClientCredentials (used by
+// GetAccessToken), Code2Session, and RefreshToken currently only validate
+// that expiresAt parses as an integer; none of them validate that
+// accessToken itself is non-empty. A malformed-but-code=0 response
+// (accessToken: "") must be rejected as an error, not treated as a
+// successful exchange and returned/cached as if it were a real token.
+
+import (
+	"context"
+	"net/http"
+	"testing"
+
+	qdmp "github.com/EchoTechFE/qdmp-server-sdk/go"
+)
+
+// TestAuthGetAccessToken_EmptyAccessTokenRejected drives the app-level
+// CLIENT_CREDENTIALS exchange (GetAccessToken -> exchangeClientCredentials)
+// through a response that reports success (code=0) with a valid expiresAt
+// but an empty accessToken. The SDK must return a non-nil error and must
+// not write the empty token through to the TokenStore (an empty "cached"
+// token would be indistinguishable from "no token" on the next read, or
+// worse, would be handed to a caller as if it were usable).
+func TestAuthGetAccessToken_EmptyAccessTokenRejected(t *testing.T) {
+	counter := newRequestCounter(func(w http.ResponseWriter, r *http.Request) {
+		businessEnvelope(w, http.StatusOK, "0", "ok", "req-empty-token-1", map[string]any{
+			"accessToken": "",
+			"expiresAt":   "9999999999",
+		})
+	})
+	srv := startServer(t, counter)
+	store := &customTokenStore{}
+	client, err := qdmp.NewClient(qdmp.ClientOptions{
+		AppID:      "test-app-id",
+		AppSecret:  "test-app-secret-do-not-leak",
+		BaseURL:    srv.URL,
+		TokenStore: store,
+	})
+	if err != nil {
+		t.Fatalf("qdmp.NewClient() error = %v", err)
+	}
+
+	token, err := client.Auth.GetAccessToken(context.Background())
+	if err == nil {
+		t.Fatalf("GetAccessToken() error = nil, want an error because accessToken is empty despite code=0")
+	}
+	if token != "" {
+		t.Fatalf("GetAccessToken() = %q, want the zero value on failure", token)
+	}
+	if store.setHits != 0 {
+		t.Fatalf("TokenStore.Set() called %d times, want 0 (an empty accessToken must never be cached)", store.setHits)
+	}
+}
+
+// TestAuthCode2Session_EmptyAccessTokenRejected is Code2Session's
+// counterpart: a response reporting code=0 with all other fields populated
+// but accessToken empty must still surface as an error, not a
+// *Code2SessionResult carrying an unusable empty AccessToken.
+func TestAuthCode2Session_EmptyAccessTokenRejected(t *testing.T) {
+	srv := startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		businessEnvelope(w, http.StatusOK, "0", "ok", "req-empty-token-2", map[string]any{
+			"accessToken":  "",
+			"refreshToken": "some-refresh-token",
+			"expiresAt":    "1785508950",
+			"openId":       "user-openid-1",
+		})
+	}))
+	client := newTestClient(t, srv.URL)
+
+	sess, err := client.Auth.Code2Session(context.Background(), "some-code")
+	if err == nil {
+		t.Fatalf("Code2Session() error = nil, want an error because accessToken is empty despite code=0")
+	}
+	if sess != nil {
+		t.Fatalf("Code2Session() result = %+v, want nil on failure", sess)
+	}
+}
+
+// TestAuthRefreshToken_EmptyAccessTokenRejected is RefreshToken's
+// counterpart to the two tests above.
+func TestAuthRefreshToken_EmptyAccessTokenRejected(t *testing.T) {
+	srv := startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		businessEnvelope(w, http.StatusOK, "0", "ok", "req-empty-token-3", map[string]any{
+			"accessToken": "",
+			"expiresAt":   "1785508950",
+		})
+	}))
+	client := newTestClient(t, srv.URL)
+
+	result, err := client.Auth.RefreshToken(context.Background(), "a-valid-refresh-token")
+	if err == nil {
+		t.Fatalf("RefreshToken() error = nil, want an error because accessToken is empty despite code=0")
+	}
+	if result != nil {
+		t.Fatalf("RefreshToken() result = %+v, want nil on failure", result)
+	}
+}

go/client.go

@@ -41,7 +41,10 @@ type ClientOptions struct {
 	QdmpVersion string
 	// HTTPClient overrides the *http.Client used for outbound requests.
 	// Defaults to a new client with a defaultHTTPTimeout (30s) request
-	// timeout, so a hung qdmp endpoint can never block a caller forever.
+	// timeout, so a hung qdmp endpoint can never block a caller forever. Its
+	// CheckRedirect is always overridden by NewClient (on a shallow copy,
+	// never mutating the caller's original client) to refuse redirects --
+	// see the comment in NewClient for why this can't be left configurable.
 	HTTPClient *http.Client
 	// TokenStore overrides the app-level access token cache. Defaults to an
 	// in-process memory store. See TokenStore for why a pluggable store
@@ -90,9 +93,28 @@ func NewClient(opts ClientOptions) (*Client, error) {
 	if qdmpVersion == "" {
 		qdmpVersion = defaultQdmpVersion
 	}
+	// Never follow a redirect: a compromised/misconfigured gateway (or an
+	// attacker able to influence a response's Location header) must not be
+	// able to make this client replay the access-token header (and any other
+	// request state) against an arbitrary host. http.ErrUseLastResponse makes
+	// Client.Do return the 3xx response itself instead of chasing Location;
+	// doRequest (see request.go) then rejects any 3xx status explicitly.
+	//
+	// This is enforced even on a caller-supplied ClientOptions.HTTPClient: we
+	// shallow-copy it and override CheckRedirect on the copy (never mutating
+	// the caller's original *http.Client), so injecting a custom transport or
+	// proxy can never silently reintroduce the redirect-leak vulnerability by
+	// omission.
+	checkRedirect := func(req *http.Request, via []*http.Request) error {
+		return http.ErrUseLastResponse
+	}
 	httpClient := opts.HTTPClient
 	if httpClient == nil {
-		httpClient = &http.Client{Timeout: defaultHTTPTimeout}
+		httpClient = &http.Client{Timeout: defaultHTTPTimeout, CheckRedirect: checkRedirect}
+	} else {
+		copied := *httpClient
+		copied.CheckRedirect = checkRedirect
+		httpClient = &copied
 	}
 	store := opts.TokenStore
 	if store == nil {
@@ -153,5 +175,11 @@ func requireAccessToken(uc *UserClient, opName string) error {
 	if uc.accessToken == "" {
 		return fmt.Errorf("qdmp: %s requires an access token: %w", opName, ErrAccessTokenRequired)
 	}
+	for i := 0; i < len(uc.accessToken); i++ {
+		b := uc.accessToken[i]
+		if b <= 0x1F || b == 0x7F {
+			return fmt.Errorf("qdmp: %s: %w", opName, ErrInvalidAccessToken)
+		}
+	}
 	return nil
 }

go/errors.go

@@ -12,6 +12,14 @@ import (
 // sent, whenever this error is returned.
 var ErrAccessTokenRequired = errors.New("qdmp: access token is required")
 
+// ErrInvalidAccessToken is a sentinel error returned when a caller-supplied
+// access token contains a byte that cannot be safely sent as an HTTP header
+// value (any byte <= 0x1F, or 0x7F/DEL — including a bare tab, 0x09, which
+// Go's net/http would otherwise transmit as-is instead of rejecting it). The
+// SDK fails locally, before any HTTP request is sent, whenever this error is
+// returned.
+var ErrInvalidAccessToken = errors.New("qdmp: access token contains a character that cannot be safely sent as an HTTP header value")
+
 // QdmpApiError represents a business-level failure reported by the qdmp
 // OpenAPI, as opposed to a transport-level failure (network error, etc).
 //

go/invalid_access_token_test.go

@@ -0,0 +1,140 @@
+package qdmp_test
+
+// Contract exercised by this file (not yet implemented — see the codex
+// review round1 fix task): requireAccessToken (go/client.go) currently only
+// rejects an empty access token. The fix task adds rejection of any access
+// token containing a control character (byte <= 0x1F, or == 0x7F) — most
+// importantly CR/LF, which (if ever allowed through) would enable HTTP
+// header/request-splitting via the "access-token" header value. This must
+// be caught locally, before any HTTP request is sent, exactly like the
+// existing empty-token check (see TestUserMe_MissingAccessToken_NoRequestSent
+// in user_test.go).
+
+import (
+	"context"
+	"net/http"
+	"strings"
+	"testing"
+)
+
+// TestUserMe_ControlCharacterAccessToken_NoRequestSent verifies that an
+// access token containing a CR/LF pair is rejected locally: the SDK must
+// return a non-nil error without ever reaching the network.
+func TestUserMe_ControlCharacterAccessToken_NoRequestSent(t *testing.T) {
+	const token = "abc\r\ndef"
+	counter := newRequestCounter(func(w http.ResponseWriter, r *http.Request) {
+		t.Fatalf("server should never have been called for an access token containing control characters, got %s %s", r.Method, r.URL.Path)
+	})
+	srv := startServer(t, counter)
+	client := newTestClient(t, srv.URL)
+
+	me, err := client.WithAccessToken(token).User.Me(context.Background())
+	if err == nil {
+		t.Fatalf("User.Me() error = nil, want a local validation error for a CR/LF-containing accessToken")
+	}
+	if me != nil {
+		t.Fatalf("User.Me() result = %+v, want nil on validation failure", me)
+	}
+	if counter.Count() != 0 {
+		t.Fatalf("server hit %d times, want 0 (a control-character accessToken must be rejected locally, before any HTTP request)", counter.Count())
+	}
+}
+
+// TestUserMe_DELCharacterAccessToken_NoRequestSent covers the other edge of
+// the control-character range called out by the fix task: byte 0x7F (DEL)
+// must be rejected the same way as the more obviously dangerous 0x00-0x1F
+// range.
+func TestUserMe_DELCharacterAccessToken_NoRequestSent(t *testing.T) {
+	token := "abc" + string(rune(0x7F)) + "def"
+	counter := newRequestCounter(func(w http.ResponseWriter, r *http.Request) {
+		t.Fatalf("server should never have been called for an access token containing a DEL (0x7F) byte, got %s %s", r.Method, r.URL.Path)
+	})
+	srv := startServer(t, counter)
+	client := newTestClient(t, srv.URL)
+
+	me, err := client.WithAccessToken(token).User.Me(context.Background())
+	if err == nil {
+		t.Fatalf("User.Me() error = nil, want a local validation error for a DEL-containing accessToken")
+	}
+	if me != nil {
+		t.Fatalf("User.Me() result = %+v, want nil on validation failure", me)
+	}
+	if counter.Count() != 0 {
+		t.Fatalf("server hit %d times, want 0 (a control-character accessToken must be rejected locally, before any HTTP request)", counter.Count())
+	}
+}
+
+// TestUserMe_ControlCharacterAccessToken_ErrorDoesNotLeakToken asserts the
+// local validation error's message never echoes the raw (invalid) token
+// content back to the caller, matching the same "never leak the token via
+// Error()" rule already enforced for the empty-token and
+// rejected-by-server paths (see TestUserMe_ErrorDoesNotLeakAccessToken in
+// user_test.go).
+func TestUserMe_ControlCharacterAccessToken_ErrorDoesNotLeakToken(t *testing.T) {
+	const token = "abc\r\ndef"
+	srv := startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		t.Fatalf("server should never have been called for an access token containing control characters, got %s %s", r.Method, r.URL.Path)
+	}))
+	client := newTestClient(t, srv.URL)
+
+	_, err := client.WithAccessToken(token).User.Me(context.Background())
+	if err == nil {
+		t.Fatalf("User.Me() error = nil, want a local validation error for a CR/LF-containing accessToken")
+	}
+	if got := err.Error(); strings.Contains(got, token) {
+		t.Fatalf("err.Error() leaks the raw invalid accessToken content: %q", got)
+	}
+}
+
+// TestUserMe_TabControlCharacterAccessToken_NoRequestSent is the
+// discriminating case for this requirement: Go's own net/http header-value
+// validation (httpguts.ValidHeaderFieldValue) already refuses to send a
+// request whose header value contains CR/LF or a DEL (0x7F) byte — so those
+// two cases can appear to "pass" even with no qdmp-level validation at all,
+// purely as a side effect of the standard library. A lone horizontal tab
+// (0x09) is *not* rejected by net/http (it is treated as legal header
+// whitespace/LWS and is transmitted as-is — confirmed empirically: a GET
+// request with "access-token: abc\tdef" reaches the server body untouched).
+// Since the fix task's contract is "reject any byte <= 0x1F", a tab must
+// still be caught by the SDK's own requireAccessToken check; this test only
+// passes once that local check actually exists.
+func TestUserMe_TabControlCharacterAccessToken_NoRequestSent(t *testing.T) {
+	const token = "abc\tdef"
+	counter := newRequestCounter(func(w http.ResponseWriter, r *http.Request) {
+		t.Fatalf("server should never have been called for an access token containing a tab (0x09) byte, got %s %s", r.Method, r.URL.Path)
+	})
+	srv := startServer(t, counter)
+	client := newTestClient(t, srv.URL)
+
+	me, err := client.WithAccessToken(token).User.Me(context.Background())
+	if err == nil {
+		t.Fatalf("User.Me() error = nil, want a local validation error for a tab-containing accessToken (net/http alone will NOT reject this and would send it over the wire)")
+	}
+	if me != nil {
+		t.Fatalf("User.Me() result = %+v, want nil on validation failure", me)
+	}
+	if counter.Count() != 0 {
+		t.Fatalf("server hit %d times, want 0 (a control-character accessToken, including a bare tab, must be rejected locally, before any HTTP request)", counter.Count())
+	}
+}
+
+// TestUserMe_TabControlCharacterAccessToken_ErrorDoesNotLeakToken is the
+// tab-byte counterpart to TestUserMe_ControlCharacterAccessToken_ErrorDoesNotLeakToken
+// above, for the same reason TestUserMe_TabControlCharacterAccessToken_NoRequestSent
+// exists: it exercises the SDK's own error message, not one manufactured by
+// net/http's transport-level rejection.
+func TestUserMe_TabControlCharacterAccessToken_ErrorDoesNotLeakToken(t *testing.T) {
+	const token = "abc\tdef"
+	srv := startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		t.Fatalf("server should never have been called for an access token containing a tab (0x09) byte, got %s %s", r.Method, r.URL.Path)
+	}))
+	client := newTestClient(t, srv.URL)
+
+	_, err := client.WithAccessToken(token).User.Me(context.Background())
+	if err == nil {
+		t.Fatalf("User.Me() error = nil, want a local validation error for a tab-containing accessToken")
+	}
+	if got := err.Error(); strings.Contains(got, token) {
+		t.Fatalf("err.Error() leaks the raw invalid accessToken content: %q", got)
+	}
+}