diff --git a/.github/workflows/generate-check.yml b/.github/workflows/generate-check.yml index 745848b..90aafeb 100644 --- a/.github/workflows/generate-check.yml +++ b/.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: diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index 3225988..706f4cd 100644 --- a/DEVELOPMENT.md +++ b/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 检查(见上一节命令),不允许绕过。 ## 测试 diff --git a/go/auth.go b/go/auth.go index 1578675..971c09f 100644 --- a/go/auth.go +++ b/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 } diff --git a/go/auth_empty_access_token_test.go b/go/auth_empty_access_token_test.go new file mode 100644 index 0000000..38de8e8 --- /dev/null +++ b/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) + } +} diff --git a/go/client.go b/go/client.go index 9694c3c..2c8ca98 100644 --- a/go/client.go +++ b/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 } diff --git a/go/errors.go b/go/errors.go index 005c4d8..b7bbb46 100644 --- a/go/errors.go +++ b/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). // diff --git a/go/invalid_access_token_test.go b/go/invalid_access_token_test.go new file mode 100644 index 0000000..4571b85 --- /dev/null +++ b/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) + } +} diff --git a/go/redirect_custom_httpclient_test.go b/go/redirect_custom_httpclient_test.go new file mode 100644 index 0000000..75d721d --- /dev/null +++ b/go/redirect_custom_httpclient_test.go @@ -0,0 +1,68 @@ +package qdmp_test + +// Regression test for a gap found during the Round 1 closing adversarial +// review: NewClient only set CheckRedirect on the *http.Client it built +// itself, so a caller-supplied ClientOptions.HTTPClient (which defaults to +// following redirects, per net/http) silently reintroduced the +// redirect-leak vulnerability the rest of this package guards against. See +// redirect_test.go for the default-client version of this contract. + +import ( + "context" + "net/http" + "sync/atomic" + "testing" + + qdmp "github.com/EchoTechFE/qdmp-server-sdk/go" +) + +// TestDoRequest_CustomHTTPClient_RedirectStillNotFollowed verifies that +// injecting a plain *http.Client (which follows redirects by default) via +// ClientOptions.HTTPClient does not let a 3xx response be chased to the +// redirect target: NewClient must force CheckRedirect on its own copy of +// the supplied client rather than trusting the caller to have configured it +// safely. +func TestDoRequest_CustomHTTPClient_RedirectStillNotFollowed(t *testing.T) { + var srv *httpTestServerPlaceholder + var redirectTargetHits int64 + + mux := http.NewServeMux() + mux.HandleFunc("/user/v1/me", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Location", srv.URL()+"/redirect-target") + w.WriteHeader(http.StatusFound) + }) + mux.HandleFunc("/redirect-target", func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt64(&redirectTargetHits, 1) + t.Errorf("redirect target must never be requested even with a caller-supplied HTTPClient, got %s %s", r.Method, r.URL.Path) + }) + + realSrv := startServer(t, mux) + srv = &httpTestServerPlaceholder{url: realSrv.URL} + + // A stock *http.Client follows redirects by default (nil CheckRedirect). + customClient := &http.Client{} + client, err := qdmp.NewClient(qdmp.ClientOptions{ + AppID: "test-app-id", + AppSecret: "test-app-secret-do-not-leak", + BaseURL: realSrv.URL, + HTTPClient: customClient, + }) + if err != nil { + t.Fatalf("qdmp.NewClient() returned unexpected error: %v", err) + } + + me, err := client.WithAccessToken("sentinel-secret-token").User.Me(context.Background()) + if err == nil { + t.Fatalf("User.Me() error = nil, want a non-nil error because the server responded with a 302 redirect") + } + if me != nil { + t.Fatalf("User.Me() result = %+v, want nil when the request fails", me) + } + if atomic.LoadInt64(&redirectTargetHits) != 0 { + t.Fatalf("redirect target was hit %d times, want 0 (custom HTTPClient must still have redirects disabled)", redirectTargetHits) + } + // The caller's original client must not be mutated in place. + if customClient.CheckRedirect != nil { + t.Fatalf("the caller-supplied *http.Client was mutated in place; NewClient must operate on a shallow copy") + } +} diff --git a/go/redirect_test.go b/go/redirect_test.go new file mode 100644 index 0000000..95e41a4 --- /dev/null +++ b/go/redirect_test.go @@ -0,0 +1,107 @@ +package qdmp_test + +// Contract exercised by this file (not yet implemented — see the codex +// review round1 fix task): the default *http.Client built by NewClient must +// never follow an HTTP redirect (3xx + Location) returned by the qdmp +// OpenAPI. doRequest must surface this as a non-nil error instead of +// silently chasing the Location header, and that error must never echo the +// caller's access-token value. +// +// Regression motivation: the stock http.Client (used whenever +// ClientOptions.HTTPClient is nil) follows redirects by default. A +// compromised/misconfigured gateway (or an attacker able to influence the +// Location header on a response) could redirect the client to an arbitrary +// host, which would replay the access-token header (and any other request +// state) against that host if redirects were followed uncritically. + +import ( + "context" + "net/http" + "strings" + "sync/atomic" + "testing" +) + +// TestDoRequest_RedirectNotFollowed verifies that a 3xx response from the +// qdmp OpenAPI results in a non-nil error and that the SDK never actually +// issues a request to the redirect target (proving the redirect was not +// followed, not just that the eventual error was later swallowed). +func TestDoRequest_RedirectNotFollowed(t *testing.T) { + var srv *httpTestServerPlaceholder + var redirectTargetHits int64 + + mux := http.NewServeMux() + mux.HandleFunc("/user/v1/me", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Location", srv.URL()+"/redirect-target") + w.WriteHeader(http.StatusFound) + }) + mux.HandleFunc("/redirect-target", func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt64(&redirectTargetHits, 1) + t.Errorf("redirect target must never be requested: the SDK must not follow 3xx redirects, got %s %s", r.Method, r.URL.Path) + }) + + realSrv := startServer(t, mux) + srv = &httpTestServerPlaceholder{url: realSrv.URL} + + client := newTestClient(t, realSrv.URL) + + me, err := client.WithAccessToken("sentinel-secret-token").User.Me(context.Background()) + if err == nil { + t.Fatalf("User.Me() error = nil, want a non-nil error because the server responded with a 302 redirect") + } + if me != nil { + t.Fatalf("User.Me() result = %+v, want nil when the request fails", me) + } + if atomic.LoadInt64(&redirectTargetHits) != 0 { + t.Fatalf("redirect target was hit %d times, want 0 (the SDK must not follow redirects)", redirectTargetHits) + } +} + +// TestDoRequest_RedirectErrorDoesNotLeakAccessToken drives the same 3xx +// redirect scenario and additionally asserts the resulting error message +// never echoes the caller's access-token value — a redirect error is still +// an SDK-internal error value and must uphold the same "never leak secrets +// via Error()" rule as every other error path in this package (see +// TestUserMe_ErrorDoesNotLeakAccessToken in user_test.go). +func TestDoRequest_RedirectErrorDoesNotLeakAccessToken(t *testing.T) { + const token = "sentinel-secret-token" + var srv *httpTestServerPlaceholder + + mux := http.NewServeMux() + mux.HandleFunc("/user/v1/me", func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("access-token"); got != token { + t.Fatalf("server never received the expected access-token header, got %q", got) + } + w.Header().Set("Location", srv.URL()+"/redirect-target") + w.WriteHeader(http.StatusFound) + }) + mux.HandleFunc("/redirect-target", func(w http.ResponseWriter, r *http.Request) { + t.Errorf("redirect target must never be requested, got %s %s", r.Method, r.URL.Path) + }) + + realSrv := startServer(t, mux) + srv = &httpTestServerPlaceholder{url: realSrv.URL} + + client := newTestClient(t, realSrv.URL) + + _, err := client.WithAccessToken(token).User.Me(context.Background()) + if err == nil { + t.Fatalf("User.Me() error = nil, want a non-nil error because the server responded with a 302 redirect") + } + if got := err.Error(); strings.Contains(got, token) { + t.Fatalf("err.Error() leaks the accessToken: %q", got) + } +} + +// httpTestServerPlaceholder lets the "/user/v1/me" handler above compute a +// self-referencing "Location" header (pointing back at the same +// httptest.Server) without a chicken-and-egg problem: the handler closure +// only reads srv.URL() at request time, by which point the httptest.Server +// has already been constructed and its real URL assigned. +type httpTestServerPlaceholder struct { + url string +} + +func (p *httpTestServerPlaceholder) URL() string { + return p.url +} diff --git a/go/request.go b/go/request.go index ee81f8c..bf5cfab 100644 --- a/go/request.go +++ b/go/request.go @@ -117,6 +117,16 @@ func (c *Client) doRequest(ctx context.Context, p requestParams) (json.RawMessag } defer resp.Body.Close() + // The default *http.Client never follows redirects (see NewClient's + // CheckRedirect), so a 3xx response here is the raw redirect itself, not + // something already chased to its destination. Reject it explicitly + // rather than trying to parse a redirect response body as a business + // envelope. Deliberately does not echo any request header (access-token + // included) into the error message. + if resp.StatusCode >= 300 && resp.StatusCode < 400 { + return nil, fmt.Errorf("qdmp: unexpected redirect response (http %d)", resp.StatusCode) + } + raw, err := io.ReadAll(resp.Body) if err != nil { return nil, fmt.Errorf("qdmp: failed to read response body (http status %d): %w", resp.StatusCode, err) diff --git a/java/src/main/java/io/github/echotechfe/qdmp/QdmpClient.java b/java/src/main/java/io/github/echotechfe/qdmp/QdmpClient.java index 0d4c011..a121a1e 100644 --- a/java/src/main/java/io/github/echotechfe/qdmp/QdmpClient.java +++ b/java/src/main/java/io/github/echotechfe/qdmp/QdmpClient.java @@ -33,7 +33,8 @@ public final class QdmpClient { * @param config the client configuration */ public QdmpClient(QdmpClientConfig config) { - OkHttpClient httpClient = new OkHttpClient(); + OkHttpClient httpClient = + new OkHttpClient.Builder().followRedirects(false).followSslRedirects(false).build(); QdmpTransport transport = new QdmpTransport( httpClient, config.getBaseUrl(), config.getQdmpVersion(), config.getAppId()); diff --git a/java/src/main/java/io/github/echotechfe/qdmp/QdmpTransport.java b/java/src/main/java/io/github/echotechfe/qdmp/QdmpTransport.java index 5cb7d46..64c3868 100644 --- a/java/src/main/java/io/github/echotechfe/qdmp/QdmpTransport.java +++ b/java/src/main/java/io/github/echotechfe/qdmp/QdmpTransport.java @@ -57,7 +57,12 @@ public QdmpTransport(OkHttpClient httpClient, String baseUrl, String qdmpVersion if (parsed == null) { throw new IllegalArgumentException("invalid baseUrl: " + baseUrl); } - this.httpClient = httpClient; + // Force-disable redirect-following on whatever client we were handed, rather than trusting the + // caller to have configured it safely: this SDK carries secret credentials as request headers, + // and this constructor is reachable directly (it is public only because groups/auth live in + // separate packages), so it must not rely on QdmpClient being the only caller. + this.httpClient = + httpClient.newBuilder().followRedirects(false).followSslRedirects(false).build(); this.baseUrl = parsed; this.qdmpVersion = qdmpVersion; this.appId = appId; @@ -156,6 +161,19 @@ private void addQueryParams(HttpUrl.Builder urlBuilder, Map quer private T execute(Request request, Class dataType) { try (Response response = httpClient.newCall(request).execute()) { + if (response.code() >= 300 && response.code() < 400) { + // Never follow a redirect: this SDK carries secret credentials (access-token / + // x-openapi-access-token) as request headers, and a redirect could point at an + // attacker-controlled host that would then receive them. Surface as a transport + // failure instead -- the message intentionally omits headers/URLs that could carry + // the access token. + throw new QdmpTransportException( + "qdmp request received an HTTP " + + response.code() + + " redirect response, which the" + + " SDK refuses to follow to avoid leaking credentials to an unverified" + + " destination"); + } String bodyString = readBody(response); JsonNode root = bodyString.isEmpty() ? JSON.createObjectNode() : JSON.readTree(bodyString); JsonNode codeNode = root.get("code"); diff --git a/java/src/main/java/io/github/echotechfe/qdmp/auth/QdmpAuth.java b/java/src/main/java/io/github/echotechfe/qdmp/auth/QdmpAuth.java index de0c1b5..465c141 100644 --- a/java/src/main/java/io/github/echotechfe/qdmp/auth/QdmpAuth.java +++ b/java/src/main/java/io/github/echotechfe/qdmp/auth/QdmpAuth.java @@ -95,15 +95,15 @@ public String getAccessToken() { } // The server reported code:"0" (success), but the SDK still has to guard against a - // structurally malformed body: a null `data`, a null `accessToken`, or a non-numeric - // `expiresAt` would otherwise surface as a bare NullPointerException/NumberFormatException - // instead of one of the SDK's typed exceptions. + // structurally malformed body: a null/empty `accessToken` or a non-numeric `expiresAt` would + // otherwise surface as a bare NullPointerException/NumberFormatException instead of one of the + // SDK's typed exceptions, or worse -- silently cache/return an empty access token. private static CachedAppToken toCachedAppToken(AuthToken200ResponseAllOfData data) { - if (data == null || data.getAccessToken() == null || data.getExpiresAt() == null) { - throw new QdmpTransportException( - "qdmp POST /auth/v1/token reported code=\"0\" (success) but returned a malformed body" - + " (missing data/accessToken/expiresAt)"); - } + requireNonMalformed( + data == null, + data == null ? null : data.getAccessToken(), + data == null ? null : data.getExpiresAt(), + "POST /auth/v1/token"); long expiresAtEpochSeconds; try { expiresAtEpochSeconds = Long.parseLong(data.getExpiresAt()); @@ -138,12 +138,19 @@ public AuthToken200ResponseAllOfData code2Session(String code) { .appId(appId) .appSecret(appSecret) .code(code); - return transport.post( - AUTH_TOKEN_ROUTE.getPath(), - AuthScheme.fromWireValue(AUTH_TOKEN_ROUTE.getAuthScheme()), - null, - request, - AuthToken200ResponseAllOfData.class); + AuthToken200ResponseAllOfData data = + transport.post( + AUTH_TOKEN_ROUTE.getPath(), + AuthScheme.fromWireValue(AUTH_TOKEN_ROUTE.getAuthScheme()), + null, + request, + AuthToken200ResponseAllOfData.class); + requireNonMalformed( + data == null, + data == null ? null : data.getAccessToken(), + data == null ? null : data.getExpiresAt(), + "POST /auth/v1/token"); + return data; } /** @@ -160,12 +167,38 @@ public AuthRefresh200ResponseAllOfData refreshToken(String refreshToken) { "refreshToken: \"refreshToken\" is required and must be a non-empty string"); } AuthRefreshRequest request = new AuthRefreshRequest().refreshToken(refreshToken); - return transport.post( - AUTH_REFRESH_ROUTE.getPath(), - AuthScheme.fromWireValue(AUTH_REFRESH_ROUTE.getAuthScheme()), - null, - request, - AuthRefresh200ResponseAllOfData.class); + AuthRefresh200ResponseAllOfData data = + transport.post( + AUTH_REFRESH_ROUTE.getPath(), + AuthScheme.fromWireValue(AUTH_REFRESH_ROUTE.getAuthScheme()), + null, + request, + AuthRefresh200ResponseAllOfData.class); + requireNonMalformed( + data == null, + data == null ? null : data.getAccessToken(), + data == null ? null : data.getExpiresAt(), + "POST /auth/v1/refresh"); + return data; + } + + // Shared guard for all three auth endpoints (CLIENT_CREDENTIALS/code2Session/refreshToken): the + // server reported code:"0" (success), but the SDK still has to protect callers from a + // structurally malformed body -- a null `data` or a null/empty `accessToken`/`expiresAt` must + // never be handed back as a silently-broken session/token object. + private static void requireNonMalformed( + boolean dataIsNull, String accessToken, String expiresAt, String routeDescription) { + if (dataIsNull + || accessToken == null + || accessToken.isEmpty() + || expiresAt == null + || expiresAt.isEmpty()) { + throw new QdmpTransportException( + "qdmp " + + routeDescription + + " reported code=\"0\" (success) but returned a malformed body (missing" + + " data/accessToken/expiresAt)"); + } } private boolean hasSufficientTimeRemaining(CachedAppToken token) { diff --git a/java/src/test/java/io/github/echotechfe/qdmp/QdmpClientRedirectTest.java b/java/src/test/java/io/github/echotechfe/qdmp/QdmpClientRedirectTest.java new file mode 100644 index 0000000..db73e32 --- /dev/null +++ b/java/src/test/java/io/github/echotechfe/qdmp/QdmpClientRedirectTest.java @@ -0,0 +1,114 @@ +package io.github.echotechfe.qdmp; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import io.github.echotechfe.qdmp.errors.QdmpTransportException; +import io.github.echotechfe.qdmp.testsupport.TestClients; +import java.io.IOException; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * {@link QdmpClient} builds its {@link okhttp3.OkHttpClient} with default settings, which follow + * 3xx redirects transparently. That is unsafe for an SDK carrying secret credentials in headers + * (access-token / x-openapi-access-token): a redirect could point at an attacker-controlled host + * that would then receive those headers. The SDK must never follow a redirect -- any 3xx response + * from the server must surface as a {@link QdmpTransportException} instead, without leaking the + * access token that was used for the original request into the exception's message. + */ +class QdmpClientRedirectTest { + + private static final String SENTINEL_TOKEN = "sentinel-secret-token"; + + private MockWebServer server; + + @BeforeEach + void startServer() throws IOException { + server = new MockWebServer(); + server.start(); + } + + @AfterEach + void stopServer() throws IOException { + server.shutdown(); + } + + /** + * A bare {@code new OkHttpClient()} would transparently follow this 302 to the second enqueued + * response and return a perfectly normal 200 success payload -- so this response is queued as a + * trap: if the SDK ever starts following redirects again, this test starts passing for the wrong + * reason (silently returning data) instead of failing, unless we also assert only one request was + * ever sent. + */ + @Test + void redirectResponse_isNotFollowed_throwsQdmpTransportException() { + server.enqueue( + new MockResponse() + .setResponseCode(302) + .setHeader("Location", server.url("/user/v1/me-redirected").toString())); + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{" + + "\"id\":\"user-1\",\"nickname\":\"should-not-be-reached\"}}")); + QdmpClient client = TestClients.create(server); + QdmpContext ctx = QdmpContext.of(SENTINEL_TOKEN); + + assertThatThrownBy(() -> client.user().me(ctx)).isInstanceOf(QdmpTransportException.class); + assertThat(server.getRequestCount()) + .as("the redirect target must never be requested -- the SDK must not follow it") + .isEqualTo(1); + } + + @Test + void redirectResponse_exceptionMessage_doesNotLeakAccessToken() { + server.enqueue( + new MockResponse() + .setResponseCode(302) + .setHeader("Location", server.url("/user/v1/me-redirected").toString())); + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{" + + "\"id\":\"user-1\",\"nickname\":\"should-not-be-reached\"}}")); + QdmpClient client = TestClients.create(server); + QdmpContext ctx = QdmpContext.of(SENTINEL_TOKEN); + + QdmpTransportException exception = + (QdmpTransportException) + org.assertj.core.api.Assertions.catchThrowable(() -> client.user().me(ctx)); + + assertThat(exception).isNotNull(); + assertThat(exception.getMessage()).doesNotContain(SENTINEL_TOKEN); + } + + /** + * Same contract for a different 3xx status code -- any redirect must be rejected, not just 302. + */ + @Test + void permanentRedirectResponse_isNotFollowed_throwsQdmpTransportException() { + server.enqueue( + new MockResponse() + .setResponseCode(301) + .setHeader("Location", server.url("/user/v1/me-redirected").toString())); + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{" + + "\"id\":\"user-1\",\"nickname\":\"should-not-be-reached\"}}")); + QdmpClient client = TestClients.create(server); + QdmpContext ctx = QdmpContext.of(SENTINEL_TOKEN); + + assertThatThrownBy(() -> client.user().me(ctx)).isInstanceOf(QdmpTransportException.class); + assertThat(server.getRequestCount()) + .as("the redirect target must never be requested -- the SDK must not follow it") + .isEqualTo(1); + } +} diff --git a/java/src/test/java/io/github/echotechfe/qdmp/auth/QdmpAuthCodeSessionAndRefreshTest.java b/java/src/test/java/io/github/echotechfe/qdmp/auth/QdmpAuthCodeSessionAndRefreshTest.java index 3189671..b0a671e 100644 --- a/java/src/test/java/io/github/echotechfe/qdmp/auth/QdmpAuthCodeSessionAndRefreshTest.java +++ b/java/src/test/java/io/github/echotechfe/qdmp/auth/QdmpAuthCodeSessionAndRefreshTest.java @@ -7,6 +7,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import io.github.echotechfe.qdmp.QdmpClient; import io.github.echotechfe.qdmp.errors.QdmpApiError; +import io.github.echotechfe.qdmp.errors.QdmpTransportException; import io.github.echotechfe.qdmp.errors.QdmpValidationError; import io.github.echotechfe.qdmp.generated.AuthRefresh200ResponseAllOfData; import io.github.echotechfe.qdmp.generated.AuthToken200ResponseAllOfData; @@ -196,4 +197,72 @@ void refreshToken_invalid_throwsWithCode10007() { .isInstanceOf(QdmpApiError.class) .satisfies(e -> assertThat(((QdmpApiError) e).getCode()).isEqualTo("10007")); } + + /** + * {@code code2Session} must reject a malformed "success" body the same way {@code + * getAccessToken()}'s {@code toCachedAppToken} already does (see {@link + * QdmpAuthGetAccessTokenTest#successCode_withNullData_throwsQdmpTransportException_notBareNpe}): + * code:"0" but no {@code data} at all must never be handed back to the caller as a silent {@code + * null} return value -- it must surface as a {@link QdmpTransportException}. + */ + @Test + void code2Session_successCode_withNullData_throwsQdmpTransportException() { + server.enqueue( + new MockResponse().setResponseCode(200).setBody("{\"code\":\"0\",\"message\":\"ok\"}")); + QdmpClient client = TestClients.create(server); + + assertThatThrownBy(() -> client.auth().code2Session("wx-login-code-abc")) + .isInstanceOf(QdmpTransportException.class); + } + + /** + * Same malformed-success guard as above, but for the more subtle case: {@code data} is present + * but its {@code accessToken} is missing. The caller must never receive a session object whose + * {@code accessToken} is silently {@code null}. + */ + @Test + void code2Session_successCode_withDataMissingAccessToken_throwsQdmpTransportException() { + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{" + + "\"expiresAt\":\"1900000000\",\"refreshToken\":\"user-refresh-token\"," + + "\"openId\":\"open-id-1\"}}")); + QdmpClient client = TestClients.create(server); + + assertThatThrownBy(() -> client.auth().code2Session("wx-login-code-abc")) + .isInstanceOf(QdmpTransportException.class); + } + + /** + * Same guard for {@code refreshToken}: code:"0" but no {@code data} at all must not be returned + * to the caller as a silent {@code null}. + */ + @Test + void refreshToken_successCode_withNullData_throwsQdmpTransportException() { + server.enqueue( + new MockResponse().setResponseCode(200).setBody("{\"code\":\"0\",\"message\":\"ok\"}")); + QdmpClient client = TestClients.create(server); + + assertThatThrownBy(() -> client.auth().refreshToken("some-refresh-token")) + .isInstanceOf(QdmpTransportException.class); + } + + /** + * Same guard for {@code refreshToken}: {@code data} present but {@code accessToken} missing must + * not be returned to the caller as an object whose {@code accessToken} is silently {@code null}. + */ + @Test + void refreshToken_successCode_withDataMissingAccessToken_throwsQdmpTransportException() { + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{\"expiresAt\":\"1900000000\"}}")); + QdmpClient client = TestClients.create(server); + + assertThatThrownBy(() -> client.auth().refreshToken("some-refresh-token")) + .isInstanceOf(QdmpTransportException.class); + } } diff --git a/java/src/test/java/io/github/echotechfe/qdmp/auth/QdmpAuthGetAccessTokenTest.java b/java/src/test/java/io/github/echotechfe/qdmp/auth/QdmpAuthGetAccessTokenTest.java index 5b57f20..f830b25 100644 --- a/java/src/test/java/io/github/echotechfe/qdmp/auth/QdmpAuthGetAccessTokenTest.java +++ b/java/src/test/java/io/github/echotechfe/qdmp/auth/QdmpAuthGetAccessTokenTest.java @@ -71,6 +71,22 @@ void firstCall_sendsClientCredentialsRequest_andReturnsAccessToken() throws Exce assertThat(body.get("appSecret").asText()).isEqualTo(TestClients.APP_SECRET); } + @Test + void emptyAccessToken_isRejected_andNeverCached() throws Exception { + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"requestId\":\"req-1\"," + + "\"data\":{\"accessToken\":\"\",\"expiresAt\":\"1900000000\"}}")); + RecordingTokenStore tokenStore = new RecordingTokenStore(); + QdmpClient client = TestClients.create(server, tokenStore); + + assertThatThrownBy(() -> client.auth().getAccessToken()) + .isInstanceOf(QdmpTransportException.class); + assertThat(tokenStore.getSetCallCount()).isZero(); + } + @Test void numericZeroCode_isTreatedAsSuccessJustLikeStringZero() throws Exception { server.enqueue( diff --git a/node/src/auth.ts b/node/src/auth.ts index dd01d76..9cad22d 100644 --- a/node/src/auth.ts +++ b/node/src/auth.ts @@ -13,11 +13,46 @@ import type {HttpClient} from './http.js'; import type {AppTokenData, RefreshTokenData, SessionData} from './types.js'; import type {StoredToken, TokenStore} from './token-store.js'; import {getRouteMeta} from './generated/route-meta.js'; -import {QdmpValidationError} from './errors.js'; +import {QdmpTransportError, QdmpValidationError} from './errors.js'; /** Refresh this many seconds before the token's real expiresAt. */ const APP_TOKEN_REFRESH_BUFFER_SECONDS = 300; +/** Guards against a malformed "success" response: HTTP 200 + business + * code='0' but with a missing/empty accessToken, or an expiresAt that can't + * be interpreted as a real timestamp. Must be called before the result is + * written to any cache/tokenStore or resolved to the caller — otherwise a + * broken value could poison the cache or silently resolve as + * undefined/empty to callers. */ +function assertWellFormedTokenResponse( + data: {accessToken?: string; expiresAt?: string} | undefined, + context: string, +): void { + if ( + !data || + typeof data.accessToken !== 'string' || + data.accessToken.length === 0 + ) { + throw new QdmpTransportError( + `${context}: server response was a business-success envelope but is missing a non-empty accessToken`, + ); + } + // Match Go's strconv.ParseInt / Java's Long.parseLong: a strict decimal + // integer only. Number(expiresAt) alone is far more permissive than + // either — it silently accepts whitespace-only strings (Number(' ') + // === 0), scientific notation, hex, and decimals, none of which the + // other two SDKs would parse as a valid Unix timestamp. + if ( + typeof data.expiresAt !== 'string' || + !/^\d+$/.test(data.expiresAt) || + !Number.isSafeInteger(Number(data.expiresAt)) + ) { + throw new QdmpTransportError( + `${context}: server response was a business-success envelope but expiresAt could not be parsed as a finite number`, + ); + } +} + export interface AuthModuleConfig { http: HttpClient; appId: string; @@ -88,6 +123,7 @@ export class AuthModule { if (!this.inFlight) { this.inFlight = this.requestClientCredentialsToken() .then(async token => { + assertWellFormedTokenResponse(token, 'auth.getAccessToken'); this.cachedToken = token; await Promise.resolve(this.tokenStore.set(token)); return token.accessToken; @@ -120,7 +156,7 @@ export class AuthModule { ); } const route = getRouteMeta('authToken'); - return this.http.request({ + const session = await this.http.request({ method: route.method, path: route.path, authScheme: route.authScheme, @@ -131,6 +167,8 @@ export class AuthModule { code, }, }); + assertWellFormedTokenResponse(session, 'auth.code2Session'); + return session; } async refreshToken(refreshToken: string): Promise { @@ -140,11 +178,13 @@ export class AuthModule { ); } const route = getRouteMeta('authRefresh'); - return this.http.request({ + const result = await this.http.request({ method: route.method, path: route.path, authScheme: route.authScheme, body: {refreshToken}, }); + assertWellFormedTokenResponse(result, 'auth.refreshToken'); + return result; } } diff --git a/node/src/context.ts b/node/src/context.ts index 4b8ef48..8afd993 100644 --- a/node/src/context.ts +++ b/node/src/context.ts @@ -14,5 +14,13 @@ export function requireAccessToken( 'ctx.accessToken is required for this qdmp-server-sdk call', ); } + for (let i = 0; i < accessToken.length; i++) { + const charCode = accessToken.charCodeAt(i); + if (charCode <= 0x1f || charCode === 0x7f) { + throw new QdmpValidationError( + 'ctx.accessToken contains a character that cannot be safely sent as an HTTP header value', + ); + } + } return accessToken; } diff --git a/node/src/errors.ts b/node/src/errors.ts index d267332..70cb832 100644 --- a/node/src/errors.ts +++ b/node/src/errors.ts @@ -68,3 +68,30 @@ export class QdmpValidationError extends Error { Object.setPrototypeOf(this, QdmpValidationError.prototype); } } + +export interface QdmpTransportErrorOptions { + cause?: unknown; + httpStatus?: number; +} + +/** Thrown for transport-level failures that must never be treated as a + * business response — e.g. an unexpected 3xx redirect, or a malformed + * "success" envelope that fails local shape validation before it is ever + * cached or returned to a caller. Never include header values (accessToken, + * appSecret, refreshToken, Location) in the message. */ +export class QdmpTransportError extends Error { + readonly cause?: unknown; + readonly httpStatus?: number; + + constructor(message: string, options?: QdmpTransportErrorOptions) { + super(message); + this.name = 'QdmpTransportError'; + this.cause = options?.cause; + this.httpStatus = options?.httpStatus; + Object.setPrototypeOf(this, QdmpTransportError.prototype); + } + + override toString(): string { + return this.message; + } +} diff --git a/node/src/http.ts b/node/src/http.ts index 67e212d..6cc9786 100644 --- a/node/src/http.ts +++ b/node/src/http.ts @@ -11,7 +11,7 @@ */ import type {Dispatcher} from 'undici'; -import {QdmpApiError} from './errors.js'; +import {QdmpApiError, QdmpTransportError} from './errors.js'; import type {QdmpAuthScheme, QdmpHttpMethod} from './generated/route-meta.js'; export interface HttpClientConfig { @@ -123,9 +123,29 @@ export class HttpClient { body: options.body !== undefined ? JSON.stringify(options.body) : undefined, dispatcher: this.dispatcher, + // Never auto-follow redirects: a 3xx response is returned to us as-is + // instead of fetch silently re-issuing the request (potentially with + // our access-token / app-secret headers) against a different origin. + redirect: 'manual', }); const httpStatus = response.status; + if (httpStatus >= 300 && httpStatus < 400) { + // Never surface anything derived from the response (e.g. the + // Location header) in this error: the redirect target is + // attacker-influenced and could reflect request data — including a + // token — back into a header value that would otherwise end up in + // this error's message/logs. + try { + await response.body?.cancel(); + } catch { + // best-effort connection cleanup; must not mask the redirect error + } + throw new QdmpTransportError( + `received an unexpected HTTP ${httpStatus} redirect response`, + {httpStatus}, + ); + } let payload: unknown; try { payload = await response.json(); diff --git a/node/src/index.ts b/node/src/index.ts index edb4894..b1d057e 100644 --- a/node/src/index.ts +++ b/node/src/index.ts @@ -1,7 +1,11 @@ export {QdmpClient} from './client.js'; export type {QdmpClientOptions} from './client.js'; export {AuthModule} from './auth.js'; -export {QdmpApiError, QdmpValidationError} from './errors.js'; +export { + QdmpApiError, + QdmpTransportError, + QdmpValidationError, +} from './errors.js'; export type {QdmpErrorCode} from './errors.js'; export {InMemoryTokenStore} from './token-store.js'; export type {StoredToken, TokenStore} from './token-store.js'; diff --git a/node/test/auth-malformed-success.test.ts b/node/test/auth-malformed-success.test.ts new file mode 100644 index 0000000..8ea4097 --- /dev/null +++ b/node/test/auth-malformed-success.test.ts @@ -0,0 +1,164 @@ +/** + * auth 模块畸形"成功"响应测试(对应 codex review round1 发现): + * 服务端返回 HTTP 200 + code='0'(业务上判定为成功),但 data 缺失,或 + * data.accessToken 缺失/空字符串时,调用方必须 reject,而不能: + * - resolve 成 undefined / 空字符串; + * - 把这个坏值写进 tokenStore,污染后续调用的缓存。 + * + * 覆盖 getAccessToken()(CLIENT_CREDENTIALS)与 refreshToken()。 + * + * 现状(本测试写下时): + * - parseEnvelope() 只用 `String(code) === '0'` 判定成功,不检查 data 内容 + * (`body.data ?? {}` 甚至允许 data 完全缺失); + * - AuthModule 的三个方法都直接把 http.request() 的结果透传给调用方,不做 + * accessToken 存在性/非空校验。 + * 因此下面的测试预期是失败的(红色状态):当前实现会 resolve 成 undefined + * 或空字符串,而不是 reject。 + */ +import assert from 'node:assert/strict'; +import {test} from 'node:test'; + +import {QdmpClient} from '../src/client.js'; +import { + businessSuccess, + createMockAgent, + expectFailure, +} from './helpers/mock-http.js'; + +function tokenSuccessData(overrides: Record = {}) { + return { + accessToken: 'app-token-abc', + expiresAt: String(Math.floor(Date.now() / 1000) + 7200), + ...overrides, + }; +} + +void test('auth.getAccessToken: HTTP 200 + {code:0} 但完全没有 data 字段时应 reject,且不得把坏值写进 tokenStore(下一次调用仍会真实重新换取)', async () => { + const {mockAgent, pool} = createMockAgent(); + // 故意不用 businessSuccess() 帮助函数:这里就是要构造"code=0 但没有 data 字段" + // 这种畸形成功响应。 + pool + .intercept({path: '/auth/v1/token', method: 'POST'}) + .reply(200, {code: 0, message: 'ok', requestId: 'req-malformed-1'}); + + const client = new QdmpClient({ + appId: 'app-id-1', + appSecret: 'app-secret-1', + dispatcher: mockAgent, + }); + + await expectFailure(() => client.auth.getAccessToken()); + + // 验证失败之后没有任何坏值被当作"已缓存的新鲜 token"留存下来: + // 只注册这一个新拦截器,若上一次的失败结果被误当成功缓存,这里就不会真的 + // 再发一次 HTTP 请求,disableNetConnect 会让请求因找不到拦截器而报错。 + pool + .intercept({path: '/auth/v1/token', method: 'POST'}) + .reply( + 200, + businessSuccess(tokenSuccessData({accessToken: 'app-token-good'})), + ); + + const token = await client.auth.getAccessToken(); + assert.equal( + token, + 'app-token-good', + '前一次畸形响应失败之后,下一次调用应当真实重新发起 CLIENT_CREDENTIALS 换取,而不是复用坏缓存', + ); + assert.equal( + mockAgent.pendingInterceptors().length, + 0, + '第二次真实请求的拦截器应当被消费', + ); +}); + +void test('auth.getAccessToken: data 存在但 data.accessToken 为空字符串时应 reject,不得把空字符串当作合法 token resolve 给调用方', async () => { + const {mockAgent, pool} = createMockAgent(); + pool.intercept({path: '/auth/v1/token', method: 'POST'}).reply( + 200, + businessSuccess({ + accessToken: '', + expiresAt: String(Math.floor(Date.now() / 1000) + 7200), + }), + ); + + const client = new QdmpClient({ + appId: 'app-id-1', + appSecret: 'app-secret-1', + dispatcher: mockAgent, + }); + + await expectFailure(() => client.auth.getAccessToken()); +}); + +void test('auth.getAccessToken: data.expiresAt 为空字符串时应 reject(Number("")===0 曾让这个畸形值被当成"已过期但合法"的数字通过校验)', async () => { + const {mockAgent, pool} = createMockAgent(); + pool.intercept({path: '/auth/v1/token', method: 'POST'}).reply( + 200, + businessSuccess({ + accessToken: 'app-token-abc', + expiresAt: '', + }), + ); + + const client = new QdmpClient({ + appId: 'app-id-1', + appSecret: 'app-secret-1', + dispatcher: mockAgent, + }); + + await expectFailure(() => client.auth.getAccessToken()); +}); + +void test('auth.getAccessToken: data.expiresAt 为纯空白字符串时应 reject(Number(" ")===0 曾让它被当成合法数字通过校验)', async () => { + const {mockAgent, pool} = createMockAgent(); + pool.intercept({path: '/auth/v1/token', method: 'POST'}).reply( + 200, + businessSuccess({ + accessToken: 'app-token-abc', + expiresAt: ' ', + }), + ); + + const client = new QdmpClient({ + appId: 'app-id-1', + appSecret: 'app-secret-1', + dispatcher: mockAgent, + }); + + await expectFailure(() => client.auth.getAccessToken()); +}); + +void test('auth.getAccessToken: data.expiresAt 为科学计数法(如 "1e100")时应 reject(Go/Java 的严格整数解析都会拒绝这类格式,Node 不应更宽松)', async () => { + const {mockAgent, pool} = createMockAgent(); + pool.intercept({path: '/auth/v1/token', method: 'POST'}).reply( + 200, + businessSuccess({ + accessToken: 'app-token-abc', + expiresAt: '1e100', + }), + ); + + const client = new QdmpClient({ + appId: 'app-id-1', + appSecret: 'app-secret-1', + dispatcher: mockAgent, + }); + + await expectFailure(() => client.auth.getAccessToken()); +}); + +void test('auth.refreshToken: data.accessToken 为空字符串时应 reject,不得把空字符串 resolve 给调用方', async () => { + const {mockAgent, pool} = createMockAgent(); + pool + .intercept({path: '/auth/v1/refresh', method: 'POST'}) + .reply(200, businessSuccess({accessToken: ''})); + + const client = new QdmpClient({ + appId: 'app-1', + appSecret: 'secret-1', + dispatcher: mockAgent, + }); + + await expectFailure(() => client.auth.refreshToken('some-refresh-token')); +}); diff --git a/node/test/context-invalid-token.test.ts b/node/test/context-invalid-token.test.ts new file mode 100644 index 0000000..52138df --- /dev/null +++ b/node/test/context-invalid-token.test.ts @@ -0,0 +1,54 @@ +/** + * requireAccessToken 本地校验测试:拒绝包含控制字符的 accessToken。 + * + * 需求(对应 codex review round1 发现):requireAccessToken(node/src/context.ts) + * 目前只做 truthy 检查。新增行为契约:当传入的 accessToken 字符串包含控制字符 + * (charCode <= 0x1F 或 === 0x7F,比如 \r、\n)时,必须**同步抛出** + * QdmpValidationError,且在发起任何网络请求之前就抛出(不需要 MockAgent 介入)。 + * + * 现状(本测试写下时):requireAccessToken 只做 `!accessToken` 的 truthy 检查, + * 不检测控制字符,因此第 1、2 个测试预期是失败的(红色状态)。第 3 个测试 + * (合法 token 不应报错)用于防止未来实现过度收紧规则,现在应当已经通过。 + */ +import assert from 'node:assert/strict'; +import {test} from 'node:test'; + +import {requireAccessToken} from '../src/context.js'; +import {QdmpValidationError} from '../src/errors.js'; + +void test('requireAccessToken: accessToken 包含 \\r\\n 控制字符时应同步抛出 QdmpValidationError', () => { + assert.throws( + () => requireAccessToken({accessToken: 'sentinel-secret\r\nleak'}), + (err: unknown) => { + assert.ok( + err instanceof QdmpValidationError, + `应当抛出 QdmpValidationError,实际抛出: ${String(err)}`, + ); + return true; + }, + ); +}); + +void test('requireAccessToken: 抛出的 QdmpValidationError.message 不得包含非法 token 的原始内容', () => { + const secretMarker = 'sentinel-secret\r\nleak'; + let caught: unknown; + try { + requireAccessToken({accessToken: secretMarker}); + } catch (err) { + caught = err; + } + assert.ok( + caught instanceof QdmpValidationError, + `应当抛出 QdmpValidationError,实际抛出: ${String(caught)}`, + ); + const message = (caught as QdmpValidationError).message ?? ''; + assert.ok( + !message.includes(secretMarker), + `error.message 不应包含非法 token 原文,实际: ${message}`, + ); +}); + +void test('requireAccessToken: 完全合法(纯 ASCII 可打印字符)的 token 不应抛错', () => { + const validToken = 'valid-ascii-token-ABC123._~-'; + assert.equal(requireAccessToken({accessToken: validToken}), validToken); +}); diff --git a/node/test/transport-redirect.test.ts b/node/test/transport-redirect.test.ts new file mode 100644 index 0000000..51bad19 --- /dev/null +++ b/node/test/transport-redirect.test.ts @@ -0,0 +1,101 @@ +/** + * 传输层安全测试:禁止跟随任意/跨域重定向。 + * + * 需求(对应 codex review round1 发现):HttpClient.request() 收到的响应 + * HTTP status 落在 [300, 400) 区间(任何 3xx 重定向)时,必须**不跟随重定向**, + * 而是 reject 一个 QdmpTransportError 实例,且这个 error 的 message/toString() + * 不能包含 accessToken 原文。 + * + * 现状(本测试写下时):QdmpTransportError 尚不存在于 src/errors.ts,且 + * HttpClient.request() 没有对 3xx 做任何特殊处理(Node 全局 fetch 默认 + * `redirect: 'follow'`),因此这些测试预期是失败的(红色状态)。 + */ +import assert from 'node:assert/strict'; +import {test} from 'node:test'; + +import {QdmpClient} from '../src/client.js'; +import {QdmpTransportError} from '../src/errors.js'; +import {createMockAgent, expectFailure} from './helpers/mock-http.js'; + +void test('HttpClient.request: 收到 302 + Location 指向另一 origin 时必须 reject QdmpTransportError,不能跟随重定向', async () => { + const {mockAgent, pool} = createMockAgent(); + pool.intercept({path: '/user/v1/me', method: 'GET'}).reply(302, '', { + headers: {location: 'https://evil-redirect-target.example.com/steal'}, + }); + + const client = new QdmpClient({ + appId: 'app-id-1', + appSecret: 'app-secret-1', + dispatcher: mockAgent, + }); + + const err = await expectFailure(() => + client.user.me({accessToken: 'sentinel-secret-token'}), + ); + + assert.ok( + err instanceof QdmpTransportError, + `收到 3xx 重定向响应时应当 reject QdmpTransportError,实际抛出: ${String(err)}`, + ); +}); + +void test('HttpClient.request: 3xx 重定向触发的 QdmpTransportError 不得在 message/toString() 中泄露 accessToken 原文', async () => { + const secretToken = 'sentinel-secret-token'; + const {mockAgent, pool} = createMockAgent(); + pool.intercept({path: '/user/v1/me', method: 'GET'}).reply(301, '', { + headers: {location: 'https://another-origin.example.com/anywhere'}, + }); + + const client = new QdmpClient({ + appId: 'app-id-1', + appSecret: 'app-secret-1', + dispatcher: mockAgent, + }); + + const err = await expectFailure(() => + client.user.me({accessToken: secretToken}), + ); + + assert.ok( + err instanceof QdmpTransportError, + `应当 reject QdmpTransportError,实际抛出: ${String(err)}`, + ); + const transportErr = err as QdmpTransportError; + const rendered = `${transportErr.message ?? ''} ${String(transportErr)}`; + assert.ok( + !rendered.includes(secretToken), + `QdmpTransportError 的 message/toString() 不应包含 accessToken 原文,实际: ${rendered}`, + ); +}); + +void test('HttpClient.request: 即使 Location host 本身反射了 accessToken,QdmpTransportError 也不能包含任何 Location 派生内容', async () => { + const secretToken = 'sentinel-secret-token'; + const {mockAgent, pool} = createMockAgent(); + // 服务端(或攻击者控制的网关)把 access-token 反射进 Location 的 host 部分—— + // 错误对象绝不能从响应里派生任何内容,只应报告 HTTP status。 + pool.intercept({path: '/user/v1/me', method: 'GET'}).reply(302, '', { + headers: {location: `https://${secretToken}.evil.example.com/steal`}, + }); + + const client = new QdmpClient({ + appId: 'app-id-1', + appSecret: 'app-secret-1', + dispatcher: mockAgent, + }); + + const err = await expectFailure(() => + client.user.me({accessToken: secretToken}), + ); + + assert.ok(err instanceof QdmpTransportError); + const transportErr = err as QdmpTransportError; + const rendered = `${transportErr.message ?? ''} ${String(transportErr)}`; + assert.ok( + !rendered.includes(secretToken), + `QdmpTransportError 不应包含 Location 派生的任何内容(含反射进 host 的 token),实际: ${rendered}`, + ); + assert.ok( + !rendered.toLowerCase().includes('location'), + `QdmpTransportError 不应包含任何 Location 相关内容,实际: ${rendered}`, + ); +});