From 8f33d6ef02202dd67c45c2cf3af759a0e45e9a73 Mon Sep 17 00:00:00 2001 From: lbb00 Date: Sat, 1 Aug 2026 19:19:29 +0800 Subject: [PATCH] Harden token/error handling, enforce HTTPS-or-loopback, and pin supply-chain versions across Node/Java/Go SDKs Second round of codex-audited security fixes on top of the redirect/token-validation work already in this PR: - Tighten Java token/header validation to a printable-ASCII whitelist and enforce it inside QdmpTransport too, closing a direct-construction bypass of QdmpContext - Reject non-numeric, negative, and already-expired expiresAt values on all three auth exchange paths (cached app token and one-shot code2Session/refreshToken) in Go/Java, and validate refreshToken/openId are non-empty across all three languages - Replace Java's code2Session/refreshToken generated-DTO return types (which leaked tokens via auto-generated toString()) with hand-written Code2SessionResult/RefreshTokenResult, matching Go's existing types - Require HTTPS by default in all three SDKs, using a real IP-literal loopback check (the previous string-prefix check could be bypassed by a hostname like "127.attacker.com") - Sanitize server-controlled message/code/requestId fields (control-character stripping + length caps) before they reach any error string, and cap response body reads at 10MB - Prevent accidental debug-logging of tokens: Go's Code2SessionResult/RefreshTokenResult now implement fmt.Stringer/GoStringer/slog.LogValuer, Node's results carry a redacted util.inspect renderer, Java's toString() also escapes control characters in openId - Fix a Node ordering bug where a failed TokenStore write could still leave a token locally cached as if persisted - Pin/verify supply-chain inputs: SHA-256-verify the downloaded openapi-generator-cli jar, pin Go's oapi-codegen to v2.8.0 via `go run`, add Gradle wrapper's distributionSha256Sum, pin staticcheck's version ## Test plan - Node: 91/91 tests, typecheck, lint clean - Java: full suite + checkstyle/spotless clean - Go: 81/81 tests (-race), gofmt/vet clean --- .github/workflows/generate-check.yml | 5 - .github/workflows/go-ci.yml | 2 +- DEVELOPMENT.md | 4 +- README.md | 2 +- go/auth.go | 133 ++++++++++- go/auth_code2session_fields_test.go | 105 +++++++++ go/auth_expiresat_validation_test.go | 160 +++++++++++++ go/auth_result_stringer_redaction_test.go | 215 ++++++++++++++++++ go/auth_stale_expiresat_test.go | 134 +++++++++++ go/auth_test.go | 28 ++- go/client.go | 53 +++++ go/client_baseurl_scheme_test.go | 123 ++++++++++ go/client_loopback_ip_literal_test.go | 104 +++++++++ go/errors.go | 25 ++ go/errors_code_safety_test.go | 73 ++++++ go/errors_message_safety_test.go | 83 +++++++ go/request.go | 29 ++- go/request_body_size_limit_test.go | 53 +++++ java/gradle/wrapper/gradle-wrapper.properties | 1 + .../echotechfe/qdmp/QdmpClientConfig.java | 73 ++++++ .../github/echotechfe/qdmp/QdmpContext.java | 25 +- .../github/echotechfe/qdmp/QdmpTransport.java | 56 ++++- .../qdmp/auth/Code2SessionResult.java | 98 ++++++++ .../github/echotechfe/qdmp/auth/QdmpAuth.java | 99 ++++++-- .../qdmp/auth/RefreshTokenResult.java | 52 +++++ .../echotechfe/qdmp/errors/QdmpApiError.java | 35 ++- .../QdmpClientConfigBaseUrlSchemeTest.java | 90 ++++++++ .../QdmpContextWhitelistValidationTest.java | 55 +++++ .../QdmpTransportRawTokenValidationTest.java | 104 +++++++++ .../QdmpTransportResponseSizeLimitTest.java | 54 +++++ ...mpAuthCode2SessionFieldValidationTest.java | 94 ++++++++ .../QdmpAuthCodeSessionAndRefreshTest.java | 12 +- .../auth/QdmpAuthExpiresAtValidationTest.java | 151 ++++++++++++ .../qdmp/auth/QdmpAuthGetAccessTokenTest.java | 25 ++ .../qdmp/auth/QdmpAuthResultTypesTest.java | 147 ++++++++++++ .../errors/QdmpApiErrorSanitizationTest.java | 93 ++++++++ node/src/auth.ts | 119 +++++++++- node/src/client.ts | 60 +++++ node/src/errors.ts | 58 ++++- node/src/http.ts | 77 ++++++- node/test/auth-expired-fresh-token.test.ts | 109 +++++++++ .../auth-result-inspect-redaction.test.ts | 142 ++++++++++++ node/test/auth-session-fields.test.ts | 90 ++++++++ .../auth-tokenstore-write-failure.test.ts | 105 +++++++++ node/test/client-baseurl-scheme.test.ts | 135 +++++++++++ node/test/errors-code-sanitization.test.ts | 133 +++++++++++ node/test/errors-message-sanitization.test.ts | 85 +++++++ node/test/http-response-size-limit.test.ts | 42 ++++ shared/scripts/regenerate-all.sh | 60 ++++- 49 files changed, 3727 insertions(+), 83 deletions(-) create mode 100644 go/auth_code2session_fields_test.go create mode 100644 go/auth_expiresat_validation_test.go create mode 100644 go/auth_result_stringer_redaction_test.go create mode 100644 go/auth_stale_expiresat_test.go create mode 100644 go/client_baseurl_scheme_test.go create mode 100644 go/client_loopback_ip_literal_test.go create mode 100644 go/errors_code_safety_test.go create mode 100644 go/errors_message_safety_test.go create mode 100644 go/request_body_size_limit_test.go create mode 100644 java/src/main/java/io/github/echotechfe/qdmp/auth/Code2SessionResult.java create mode 100644 java/src/main/java/io/github/echotechfe/qdmp/auth/RefreshTokenResult.java create mode 100644 java/src/test/java/io/github/echotechfe/qdmp/QdmpClientConfigBaseUrlSchemeTest.java create mode 100644 java/src/test/java/io/github/echotechfe/qdmp/QdmpContextWhitelistValidationTest.java create mode 100644 java/src/test/java/io/github/echotechfe/qdmp/QdmpTransportRawTokenValidationTest.java create mode 100644 java/src/test/java/io/github/echotechfe/qdmp/QdmpTransportResponseSizeLimitTest.java create mode 100644 java/src/test/java/io/github/echotechfe/qdmp/auth/QdmpAuthCode2SessionFieldValidationTest.java create mode 100644 java/src/test/java/io/github/echotechfe/qdmp/auth/QdmpAuthExpiresAtValidationTest.java create mode 100644 java/src/test/java/io/github/echotechfe/qdmp/auth/QdmpAuthResultTypesTest.java create mode 100644 java/src/test/java/io/github/echotechfe/qdmp/errors/QdmpApiErrorSanitizationTest.java create mode 100644 node/test/auth-expired-fresh-token.test.ts create mode 100644 node/test/auth-result-inspect-redaction.test.ts create mode 100644 node/test/auth-session-fields.test.ts create mode 100644 node/test/auth-tokenstore-write-failure.test.ts create mode 100644 node/test/client-baseurl-scheme.test.ts create mode 100644 node/test/errors-code-sanitization.test.ts create mode 100644 node/test/errors-message-sanitization.test.ts create mode 100644 node/test/http-response-size-limit.test.ts diff --git a/.github/workflows/generate-check.yml b/.github/workflows/generate-check.yml index 90aafeb..2cd9e17 100644 --- a/.github/workflows/generate-check.yml +++ b/.github/workflows/generate-check.yml @@ -34,11 +34,6 @@ jobs: run: npm ci working-directory: node - - name: install oapi-codegen - run: | - go install github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen@latest - echo "$(go env GOPATH)/bin" >> "$GITHUB_PATH" - - name: regenerate everything from shared/openapi.yaml run: bash shared/scripts/regenerate-all.sh diff --git a/.github/workflows/go-ci.yml b/.github/workflows/go-ci.yml index a012998..ddac13c 100644 --- a/.github/workflows/go-ci.yml +++ b/.github/workflows/go-ci.yml @@ -48,6 +48,6 @@ jobs: - name: staticcheck uses: dominikh/staticcheck-action@v1 with: - version: latest + version: "2026.1" working-directory: go install-go: false diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index 706f4cd..cb67c8a 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -7,8 +7,8 @@ - Node.js ≥ 22 - JDK ≥ 17（CI 用 temurin 17） - Go（版本见 `go/go.mod`） -- [`oapi-codegen`](https://github.com/oapi-codegen/oapi-codegen)：`go install github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen@latest`，装完确保 `$(go env GOPATH)/bin` 在 `PATH` 上 -- `openapi-generator-cli`（Java model 生成用）：`shared/scripts/regenerate-all.sh` 会在 `OPENAPI_GENERATOR_CLI_JAR` 未指向已存在的 jar 时自动下载到 `/tmp/tools/openapi-generator-cli.jar`，无需手动安装 +- [`oapi-codegen`](https://github.com/oapi-codegen/oapi-codegen)：无需手动安装，`shared/scripts/regenerate-all.sh` 通过 `go run .../oapi-codegen@v2.8.0` 按固定版本直接调用，Go 工具链会对该 module 做 sum.golang.org 校验 +- `openapi-generator-cli`（Java model 生成用）：`shared/scripts/regenerate-all.sh` 会在 `OPENAPI_GENERATOR_CLI_JAR` 未指向已存在的 jar 时自动下载到本地缓存并校验 SHA-256，无需手动安装；若校验失败会重新下载一次，仍失败则直接报错退出 ## 代码生成 diff --git a/README.md b/README.md index 19ab11f..4916bc6 100644 --- a/README.md +++ b/README.md @@ -51,7 +51,7 @@ QdmpClient qdmp = new QdmpClient( .build()); // 用 code 换用户 session（SDK 不缓存，自行持久化） -AuthToken200ResponseAllOfData session = qdmp.auth().code2Session(code); +Code2SessionResult session = qdmp.auth().code2Session(code); // QdmpContext.of(accessToken) 是唯一入口，空/非法 token 在构造期就失败 QdmpContext ctx = QdmpContext.of(session.getAccessToken()); diff --git a/go/auth.go b/go/auth.go index 971c09f..26780cc 100644 --- a/go/auth.go +++ b/go/auth.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "log/slog" "net/http" "strconv" "sync" @@ -38,6 +39,77 @@ type RefreshTokenResult struct { ExpiresAt string `json:"expiresAt"` } +// String implements fmt.Stringer so that accidental debug-printing (e.g. +// fmt.Println(result), fmt.Printf("%v"/"%+v", result), or most structured +// loggers that fall back to %v/%+v) never leaks the raw AccessToken/ +// RefreshToken values. This is a value-receiver method, so it is also picked +// up for *Code2SessionResult (a pointer's method set includes all +// value-receiver methods of the pointed-to type). +// +// This must never affect json.Marshal(result): encoding/json only consults +// the json.Marshaler interface (MarshalJSON), not fmt.Stringer, so callers +// that persist the real values via JSON — the entire point of +// Code2Session/RefreshToken existing — are unaffected. Direct field access +// (result.AccessToken) is likewise untouched. +func (r Code2SessionResult) String() string { + return fmt.Sprintf( + "Code2SessionResult{AccessToken:%q, RefreshToken:%q, ExpiresAt:%q, OpenID:%q}", + "[REDACTED]", "[REDACTED]", r.ExpiresAt, r.OpenID, + ) +} + +// String implements fmt.Stringer — see Code2SessionResult.String for why. +func (r RefreshTokenResult) String() string { + return fmt.Sprintf( + "RefreshTokenResult{AccessToken:%q, ExpiresAt:%q}", + "[REDACTED]", r.ExpiresAt, + ) +} + +// GoString implements fmt.GoStringer. The %#v verb does NOT consult +// fmt.Stringer — it falls back to Go-syntax struct reflection, which would +// print every exported field (including the raw AccessToken/RefreshToken) +// even with String() defined above. GoStringer is the only way to also +// redact under %#v. +func (r Code2SessionResult) GoString() string { + return fmt.Sprintf( + "qdmp.Code2SessionResult{AccessToken:%q, RefreshToken:%q, ExpiresAt:%q, OpenID:%q}", + "[REDACTED]", "[REDACTED]", r.ExpiresAt, r.OpenID, + ) +} + +// GoString implements fmt.GoStringer — see Code2SessionResult.GoString for why. +func (r RefreshTokenResult) GoString() string { + return fmt.Sprintf( + "qdmp.RefreshTokenResult{AccessToken:%q, ExpiresAt:%q}", + "[REDACTED]", r.ExpiresAt, + ) +} + +// LogValue implements slog.LogValuer. Structured loggers built on log/slog +// (e.g. slog.JSONHandler, slog.TextHandler) do NOT consult fmt.Stringer — +// slog.JSONHandler JSON-marshals values directly, which would still emit +// the raw AccessToken/RefreshToken fields even with String()/GoString() +// already redacting the fmt-based paths above. LogValuer is slog's own +// redaction hook; it has no effect on json.Marshal (persistence stays +// exactly as before). +func (r Code2SessionResult) LogValue() slog.Value { + return slog.GroupValue( + slog.String("AccessToken", "[REDACTED]"), + slog.String("RefreshToken", "[REDACTED]"), + slog.String("ExpiresAt", r.ExpiresAt), + slog.String("OpenID", r.OpenID), + ) +} + +// LogValue implements slog.LogValuer — see Code2SessionResult.LogValue for why. +func (r RefreshTokenResult) LogValue() slog.Value { + return slog.GroupValue( + slog.String("AccessToken", "[REDACTED]"), + slog.String("ExpiresAt", r.ExpiresAt), + ) +} + // authInflight represents one in-flight app-level token exchange that // concurrent GetAccessToken callers can wait on instead of each starting // their own HTTP call (single-flight). @@ -174,15 +246,45 @@ func (a *AuthService) exchangeClientCredentials(ctx context.Context) (string, er return "", fmt.Errorf("qdmp: auth.token response has an empty accessToken") } - expiresAt, err := strconv.ParseInt(result.ExpiresAt, 10, 64) + expiresAt, err := parseExpiresAt(result.ExpiresAt) if err != nil { - return "", fmt.Errorf("qdmp: auth.token response has a non-numeric expiresAt: %w", err) + return "", fmt.Errorf("qdmp: auth.token response has an invalid expiresAt: %w", err) + } + // A non-negative expiresAt can still be already expired, or expiring + // within tokenRefreshBufferSeconds (e.g. "1", 1970-01-01, or "a few + // seconds from now") — parseExpiresAt only rejects negative/non-numeric + // values. Accepting such a value here would cache it and hand it back to + // this call's own caller as if it were a usably-fresh token, even though + // cachedValid uses the same buffer and would immediately (and correctly) + // treat that same cached entry as not-fresh on the very next call. Apply + // the identical predicate here so "fresh enough to hand out" means the + // same thing on both the write and read paths. + if expiresAt-tokenRefreshBufferSeconds <= time.Now().Unix() { + return "", fmt.Errorf("qdmp: auth.token response's expiresAt (%d) leaves less than the required %ds before expiry", expiresAt, tokenRefreshBufferSeconds) } a.store.Set(TokenEntry{AccessToken: result.AccessToken, ExpiresAt: expiresAt}) return result.AccessToken, nil } +// parseExpiresAt parses a wire expiresAt string (see Code2SessionResult's +// doc comment for why it is a string on the wire) as a non-negative absolute +// Unix-seconds timestamp. Anything that fails to parse as a base-10 int64, +// or that parses to a negative value, is rejected outright: a +// negative/corrupted expiresAt must never be treated as a usable expiry +// (whether cached or returned directly to a one-shot caller), rather than +// silently limping along. +func parseExpiresAt(s string) (int64, error) { + v, err := strconv.ParseInt(s, 10, 64) + if err != nil { + return 0, fmt.Errorf("non-numeric expiresAt %q: %w", s, err) + } + if v < 0 { + return 0, fmt.Errorf("negative expiresAt %q", s) + } + return v, nil +} + // Code2Session performs a one-shot AUTHORIZATION_CODE exchange. Never // cached — see the AuthService doc comment. func (a *AuthService) Code2Session(ctx context.Context, code string) (*Code2SessionResult, error) { @@ -208,6 +310,24 @@ func (a *AuthService) Code2Session(ctx context.Context, code string) (*Code2Sess if result.AccessToken == "" { return nil, fmt.Errorf("qdmp: auth.code2Session response has an empty accessToken") } + if result.RefreshToken == "" { + return nil, fmt.Errorf("qdmp: auth.code2Session response has an empty refreshToken") + } + if result.OpenID == "" { + return nil, fmt.Errorf("qdmp: auth.code2Session response has an empty openId") + } + expiresAt, err := parseExpiresAt(result.ExpiresAt) + if err != nil { + return nil, fmt.Errorf("qdmp: auth.code2Session response has an invalid expiresAt: %w", err) + } + // A non-negative expiresAt can still already be in the past (e.g. "1", + // 1970-01-01) — parseExpiresAt only rejects negative/non-numeric values. + // Handing back a session whose access token is already dead on arrival + // would let a caller persist (or start relying on) a permanently-broken + // session, same reasoning as the app-level cached path. + if expiresAt <= time.Now().Unix() { + return nil, fmt.Errorf("qdmp: auth.code2Session response has an already-expired expiresAt (%d)", expiresAt) + } return &result, nil } @@ -233,5 +353,14 @@ func (a *AuthService) RefreshToken(ctx context.Context, refreshToken string) (*R if result.AccessToken == "" { return nil, fmt.Errorf("qdmp: auth.refreshToken response has an empty accessToken") } + expiresAt, err := parseExpiresAt(result.ExpiresAt) + if err != nil { + return nil, fmt.Errorf("qdmp: auth.refreshToken response has an invalid expiresAt: %w", err) + } + // See the identical check in Code2Session: a non-negative but + // already-expired expiresAt must not be handed back as a usable result. + if expiresAt <= time.Now().Unix() { + return nil, fmt.Errorf("qdmp: auth.refreshToken response has an already-expired expiresAt (%d)", expiresAt) + } return &result, nil } diff --git a/go/auth_code2session_fields_test.go b/go/auth_code2session_fields_test.go new file mode 100644 index 0000000..2a82b9d --- /dev/null +++ b/go/auth_code2session_fields_test.go @@ -0,0 +1,105 @@ +package qdmp_test + +// Contract exercised by this file (not yet implemented): +// +// Code2SessionResult (auth.go ~line 22) declares RefreshToken and OpenID +// fields, but Code2Session (auth.go ~line 188) only ever checks +// result.AccessToken == "". A response reporting code=0 with a populated +// accessToken/expiresAt but a missing or empty refreshToken/openId must +// still surface as an error: the caller has no usable user-level session +// without a refreshToken to renew it later, or an openId to know whose +// session it is. + +import ( + "context" + "net/http" + "testing" +) + +// TestAuthCode2Session_MissingRefreshTokenRejected drives a response that +// omits the refreshToken field entirely (as opposed to sending it empty). +func TestAuthCode2Session_MissingRefreshTokenRejected(t *testing.T) { + srv := startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + businessEnvelope(w, http.StatusOK, "0", "ok", "req-norefresh-1", map[string]any{ + "accessToken": "user-level-access-token", + "expiresAt": "1785508950", + "openId": "user-openid-1", + // refreshToken deliberately omitted. + }) + })) + 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 refreshToken is missing from the response") + } + if sess != nil { + t.Fatalf("Code2Session() result = %+v, want nil on failure", sess) + } +} + +// TestAuthCode2Session_EmptyRefreshTokenRejected is the same failure mode +// but with an explicit empty string rather than an omitted field. +func TestAuthCode2Session_EmptyRefreshTokenRejected(t *testing.T) { + srv := startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + businessEnvelope(w, http.StatusOK, "0", "ok", "req-norefresh-2", map[string]any{ + "accessToken": "user-level-access-token", + "refreshToken": "", + "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 refreshToken is an empty string") + } + if sess != nil { + t.Fatalf("Code2Session() result = %+v, want nil on failure", sess) + } +} + +// TestAuthCode2Session_MissingOpenIDRejected drives a response that omits +// the openId field entirely. +func TestAuthCode2Session_MissingOpenIDRejected(t *testing.T) { + srv := startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + businessEnvelope(w, http.StatusOK, "0", "ok", "req-noopenid-1", map[string]any{ + "accessToken": "user-level-access-token", + "refreshToken": "user-level-refresh-token", + "expiresAt": "1785508950", + // openId deliberately omitted. + }) + })) + 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 openId is missing from the response") + } + if sess != nil { + t.Fatalf("Code2Session() result = %+v, want nil on failure", sess) + } +} + +// TestAuthCode2Session_EmptyOpenIDRejected is the same failure mode but with +// an explicit empty string rather than an omitted field. +func TestAuthCode2Session_EmptyOpenIDRejected(t *testing.T) { + srv := startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + businessEnvelope(w, http.StatusOK, "0", "ok", "req-noopenid-2", map[string]any{ + "accessToken": "user-level-access-token", + "refreshToken": "user-level-refresh-token", + "expiresAt": "1785508950", + "openId": "", + }) + })) + 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 openId is an empty string") + } + if sess != nil { + t.Fatalf("Code2Session() result = %+v, want nil on failure", sess) + } +} diff --git a/go/auth_expiresat_validation_test.go b/go/auth_expiresat_validation_test.go new file mode 100644 index 0000000..bc37197 --- /dev/null +++ b/go/auth_expiresat_validation_test.go @@ -0,0 +1,160 @@ +package qdmp_test + +// Contract exercised by this file (not yet implemented): +// +// AuthService.Code2Session, AuthService.RefreshToken, and the cached +// GetAccessToken (via exchangeClientCredentials) currently only check that +// AccessToken is non-empty; none of them validate that ExpiresAt actually +// parses as a valid, non-negative integer. exchangeClientCredentials (see +// auth.go ~line 177) does call strconv.ParseInt on it, but happily accepts a +// negative value, which would make cachedValid's +// `entry.ExpiresAt-tokenRefreshBufferSeconds <= now` comparison misbehave +// (a negative ExpiresAt is always "expired", but a poisoned/corrupted +// response should be rejected outright rather than silently limping along). +// +// This file requires: any expiresAt that fails to parse as a non-negative +// base-10 int64 must produce a non-nil error from Code2Session/RefreshToken/ +// GetAccessToken, and — critically for GetAccessToken — a rejected exchange +// must never be written through to the TokenStore, so the very next call +// still issues a real HTTP request instead of treating the poisoned result as +// a fresh cache hit. + +import ( + "context" + "net/http" + "sync/atomic" + "testing" +) + +// TestAuthCode2Session_NonNumericExpiresAtRejected drives Code2Session +// through a response with an accessToken/refreshToken/openId all populated, +// but an expiresAt that isn't a number at all. +func TestAuthCode2Session_NonNumericExpiresAtRejected(t *testing.T) { + srv := startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + businessEnvelope(w, http.StatusOK, "0", "ok", "req-badexp-c2s-1", map[string]any{ + "accessToken": "tok", + "refreshToken": "r", + "expiresAt": "garbage", + "openId": "o", + }) + })) + 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 expiresAt %q does not parse as a number", "garbage") + } + if sess != nil { + t.Fatalf("Code2Session() result = %+v, want nil on failure", sess) + } +} + +// TestAuthCode2Session_NegativeExpiresAtRejected drives Code2Session through +// a response whose expiresAt parses fine as an integer but is negative — a +// value that must never be treated as a usable absolute Unix-seconds expiry. +func TestAuthCode2Session_NegativeExpiresAtRejected(t *testing.T) { + srv := startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + businessEnvelope(w, http.StatusOK, "0", "ok", "req-badexp-c2s-2", map[string]any{ + "accessToken": "tok", + "refreshToken": "r", + "expiresAt": "-100", + "openId": "o", + }) + })) + 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 expiresAt is negative (-100)") + } + if sess != nil { + t.Fatalf("Code2Session() result = %+v, want nil on failure", sess) + } +} + +// TestAuthRefreshToken_NonNumericExpiresAtRejected is RefreshToken's +// counterpart to TestAuthCode2Session_NonNumericExpiresAtRejected. +func TestAuthRefreshToken_NonNumericExpiresAtRejected(t *testing.T) { + srv := startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + businessEnvelope(w, http.StatusOK, "0", "ok", "req-badexp-rt-1", map[string]any{ + "accessToken": "tok", + "expiresAt": "garbage", + }) + })) + 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 expiresAt %q does not parse as a number", "garbage") + } + if result != nil { + t.Fatalf("RefreshToken() result = %+v, want nil on failure", result) + } +} + +// TestAuthRefreshToken_NegativeExpiresAtRejected is RefreshToken's +// counterpart to TestAuthCode2Session_NegativeExpiresAtRejected. +func TestAuthRefreshToken_NegativeExpiresAtRejected(t *testing.T) { + srv := startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + businessEnvelope(w, http.StatusOK, "0", "ok", "req-badexp-rt-2", map[string]any{ + "accessToken": "tok", + "expiresAt": "-100", + }) + })) + 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 expiresAt is negative (-100)") + } + if result != nil { + t.Fatalf("RefreshToken() result = %+v, want nil on failure", result) + } +} + +// TestAuthGetAccessToken_NegativeExpiresAtRejectedAndNotCached is the most +// important test in this file: it proves a rejected (negative-expiresAt) +// app-level token exchange is never written through to the TokenStore. The +// mock server returns a poisoned response (expiresAt: "-100") on its first +// hit, then a legitimate response on every hit after. If the SDK incorrectly +// cached the poisoned exchange as "fresh" (e.g. by caching before validating, +// or by treating the error path as success), the second GetAccessToken call +// would either wrongly succeed with the poisoned token or short-circuit +// without ever hitting the server again. Both would defeat this assertion. +func TestAuthGetAccessToken_NegativeExpiresAtRejectedAndNotCached(t *testing.T) { + var calls int64 + srv := startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := atomic.AddInt64(&calls, 1) + if n == 1 { + businessEnvelope(w, http.StatusOK, "0", "ok", "req-badexp-gat-1", map[string]any{ + "accessToken": "token-from-poisoned-response", + "expiresAt": "-100", + }) + return + } + businessEnvelope(w, http.StatusOK, "0", "ok", "req-badexp-gat-2", map[string]any{ + "accessToken": "token-from-good-response", + "expiresAt": "9999999999", + }) + })) + client := newTestClient(t, srv.URL) + + token, err := client.Auth.GetAccessToken(context.Background()) + if err == nil { + t.Fatalf("first GetAccessToken() error = nil, want an error because expiresAt is negative (-100)") + } + if token != "" { + t.Fatalf("first GetAccessToken() = %q, want the zero value on failure", token) + } + + token, err = client.Auth.GetAccessToken(context.Background()) + if err != nil { + t.Fatalf("second GetAccessToken() error = %v, want nil", err) + } + if token != "token-from-good-response" { + t.Fatalf("second GetAccessToken() = %q, want %q", token, "token-from-good-response") + } + if got := atomic.LoadInt64(&calls); got != 2 { + t.Fatalf("server hit %d times, want exactly 2 (the first poisoned response must not have been cached as a fresh token, forcing a real second exchange)", got) + } +} diff --git a/go/auth_result_stringer_redaction_test.go b/go/auth_result_stringer_redaction_test.go new file mode 100644 index 0000000..98689a1 --- /dev/null +++ b/go/auth_result_stringer_redaction_test.go @@ -0,0 +1,215 @@ +package qdmp_test + +// Regression test for a real security finding (Fable5 design-consistency +// review): Code2SessionResult and RefreshTokenResult are plain structs with +// no fmt.Stringer implementation, so fmt.Println(result)/fmt.Printf("%v"/ +// "%+v", result) and most structured loggers (which fall back to %v/%+v) +// would print AccessToken/RefreshToken in the clear — an easy way for an +// integrator's incidental debug log line to leak a live user session token. +// +// The fix (auth.go, Code2SessionResult.String / RefreshTokenResult.String) +// must redact AccessToken/RefreshToken in the %v/%+v/Println path while +// leaving json.Marshal and direct field access completely untouched: the +// entire reason Code2Session/RefreshToken exist is so the caller can +// persist the real values into its own session store, so over-redacting the +// serialization path would make the SDK unusable. + +import ( + "bytes" + "encoding/json" + "fmt" + "log/slog" + "strings" + "testing" + + qdmp "github.com/EchoTechFE/qdmp-server-sdk/go" +) + +const ( + sentinelAccessToken = "sentinel-access-token-do-not-leak-xyz123" + sentinelRefreshToken = "sentinel-refresh-token-do-not-leak-xyz123" +) + +func TestCode2SessionResult_StringRedactsTokens(t *testing.T) { + result := &qdmp.Code2SessionResult{ + AccessToken: sentinelAccessToken, + RefreshToken: sentinelRefreshToken, + ExpiresAt: "1785508950", + OpenID: "open-id-1", + } + + for _, format := range []string{"%v", "%+v"} { + out := fmt.Sprintf(format, result) + if strings.Contains(out, sentinelAccessToken) { + t.Fatalf("fmt.Sprintf(%q, *Code2SessionResult) = %q, leaked the raw accessToken", format, out) + } + if strings.Contains(out, sentinelRefreshToken) { + t.Fatalf("fmt.Sprintf(%q, *Code2SessionResult) = %q, leaked the raw refreshToken", format, out) + } + if !strings.Contains(out, "[REDACTED]") { + t.Fatalf("fmt.Sprintf(%q, *Code2SessionResult) = %q, want it to contain the [REDACTED] placeholder", format, out) + } + // Non-secret fields must still be visible. + if !strings.Contains(out, "1785508950") || !strings.Contains(out, "open-id-1") { + t.Fatalf("fmt.Sprintf(%q, *Code2SessionResult) = %q, want ExpiresAt/OpenID still printed", format, out) + } + } + + // Same assertions for the value type (not just the pointer), and via + // fmt.Println's default formatting. + valueOut := fmt.Sprintf("%v", *result) + if strings.Contains(valueOut, sentinelAccessToken) || strings.Contains(valueOut, sentinelRefreshToken) { + t.Fatalf("fmt.Sprintf(%%v, Code2SessionResult value) = %q, leaked a raw token", valueOut) + } +} + +// TestCode2SessionResult_GoStringRedactsTokens is a regression test for a +// follow-up finding from codex's convergence-check review: fmt.Stringer's +// String() is NOT consulted by the %#v verb (Go-syntax representation) — +// only fmt.GoStringer is. Without a GoString() method, fmt.Printf("%#v", +// result) would still print the raw AccessToken/RefreshToken fields even +// though %v/%+v are already redacted above. +func TestCode2SessionResult_GoStringRedactsTokens(t *testing.T) { + result := &qdmp.Code2SessionResult{ + AccessToken: sentinelAccessToken, + RefreshToken: sentinelRefreshToken, + ExpiresAt: "1785508950", + OpenID: "open-id-1", + } + + out := fmt.Sprintf("%#v", result) + if strings.Contains(out, sentinelAccessToken) || strings.Contains(out, sentinelRefreshToken) { + t.Fatalf("fmt.Sprintf(%%#v, *Code2SessionResult) = %q, leaked a raw token", out) + } + if !strings.Contains(out, "[REDACTED]") { + t.Fatalf("fmt.Sprintf(%%#v, *Code2SessionResult) = %q, want the [REDACTED] placeholder", out) + } +} + +func TestRefreshTokenResult_GoStringRedactsTokens(t *testing.T) { + result := &qdmp.RefreshTokenResult{ + AccessToken: sentinelAccessToken, + ExpiresAt: "1785508950", + } + + out := fmt.Sprintf("%#v", result) + if strings.Contains(out, sentinelAccessToken) { + t.Fatalf("fmt.Sprintf(%%#v, *RefreshTokenResult) = %q, leaked the raw accessToken", out) + } + if !strings.Contains(out, "[REDACTED]") { + t.Fatalf("fmt.Sprintf(%%#v, *RefreshTokenResult) = %q, want the [REDACTED] placeholder", out) + } +} + +// TestCode2SessionResult_SlogRedactsTokens is a regression test for a +// follow-up finding from codex's second convergence-check review: +// fmt.Stringer/fmt.GoStringer are not consulted by log/slog's handlers — +// slog.JSONHandler in particular JSON-marshals values directly, so +// slog.Info("session", "value", result) would still emit the raw +// AccessToken/RefreshToken even with String()/GoString() already redacting +// the plain fmt.Printf paths. Only slog.LogValuer (LogValue()) is consulted +// by slog itself. +func TestCode2SessionResult_SlogRedactsTokens(t *testing.T) { + result := &qdmp.Code2SessionResult{ + AccessToken: sentinelAccessToken, + RefreshToken: sentinelRefreshToken, + ExpiresAt: "1785508950", + OpenID: "open-id-1", + } + + handlers := []struct { + name string + new func(*bytes.Buffer) slog.Handler + }{ + {"JSONHandler", func(buf *bytes.Buffer) slog.Handler { return slog.NewJSONHandler(buf, nil) }}, + {"TextHandler", func(buf *bytes.Buffer) slog.Handler { return slog.NewTextHandler(buf, nil) }}, + } + for _, h := range handlers { + var buf bytes.Buffer + logger := slog.New(h.new(&buf)) + logger.Info("session", "value", result) + out := buf.String() + if strings.Contains(out, sentinelAccessToken) || strings.Contains(out, sentinelRefreshToken) { + t.Fatalf("slog with %s leaked a raw token: %s", h.name, out) + } + if !strings.Contains(out, "[REDACTED]") { + t.Fatalf("slog with %s output should contain the [REDACTED] placeholder, got: %s", h.name, out) + } + } +} + +func TestRefreshTokenResult_StringRedactsTokens(t *testing.T) { + result := &qdmp.RefreshTokenResult{ + AccessToken: sentinelAccessToken, + ExpiresAt: "1785508950", + } + + for _, format := range []string{"%v", "%+v"} { + out := fmt.Sprintf(format, result) + if strings.Contains(out, sentinelAccessToken) { + t.Fatalf("fmt.Sprintf(%q, *RefreshTokenResult) = %q, leaked the raw accessToken", format, out) + } + if !strings.Contains(out, "[REDACTED]") { + t.Fatalf("fmt.Sprintf(%q, *RefreshTokenResult) = %q, want it to contain the [REDACTED] placeholder", format, out) + } + if !strings.Contains(out, "1785508950") { + t.Fatalf("fmt.Sprintf(%q, *RefreshTokenResult) = %q, want ExpiresAt still printed", format, out) + } + } +} + +// TestCode2SessionResult_JSONMarshalNotRedacted is the reverse-direction +// guard: json.Marshal is how the caller persists the real session into its +// own DB/session store, so it must keep returning the real values — the +// String()/fmt.Stringer redaction must not leak into encoding/json, which +// only consults json.Marshaler (not fmt.Stringer). +func TestCode2SessionResult_JSONMarshalNotRedacted(t *testing.T) { + result := &qdmp.Code2SessionResult{ + AccessToken: sentinelAccessToken, + RefreshToken: sentinelRefreshToken, + ExpiresAt: "1785508950", + OpenID: "open-id-1", + } + + data, err := json.Marshal(result) + if err != nil { + t.Fatalf("json.Marshal(Code2SessionResult) returned unexpected error: %v", err) + } + out := string(data) + if !strings.Contains(out, sentinelAccessToken) { + t.Fatalf("json.Marshal(Code2SessionResult) = %q, want it to contain the real accessToken for the caller to persist", out) + } + if !strings.Contains(out, sentinelRefreshToken) { + t.Fatalf("json.Marshal(Code2SessionResult) = %q, want it to contain the real refreshToken for the caller to persist", out) + } + + // Direct field access must also stay the real plaintext value. + if result.AccessToken != sentinelAccessToken { + t.Fatalf("result.AccessToken = %q, want the raw sentinel value %q", result.AccessToken, sentinelAccessToken) + } + if result.RefreshToken != sentinelRefreshToken { + t.Fatalf("result.RefreshToken = %q, want the raw sentinel value %q", result.RefreshToken, sentinelRefreshToken) + } +} + +// TestRefreshTokenResult_JSONMarshalNotRedacted mirrors the above for +// RefreshTokenResult. +func TestRefreshTokenResult_JSONMarshalNotRedacted(t *testing.T) { + result := &qdmp.RefreshTokenResult{ + AccessToken: sentinelAccessToken, + ExpiresAt: "1785508950", + } + + data, err := json.Marshal(result) + if err != nil { + t.Fatalf("json.Marshal(RefreshTokenResult) returned unexpected error: %v", err) + } + out := string(data) + if !strings.Contains(out, sentinelAccessToken) { + t.Fatalf("json.Marshal(RefreshTokenResult) = %q, want it to contain the real accessToken for the caller to persist", out) + } + + if result.AccessToken != sentinelAccessToken { + t.Fatalf("result.AccessToken = %q, want the raw sentinel value %q", result.AccessToken, sentinelAccessToken) + } +} diff --git a/go/auth_stale_expiresat_test.go b/go/auth_stale_expiresat_test.go new file mode 100644 index 0000000..e373aa4 --- /dev/null +++ b/go/auth_stale_expiresat_test.go @@ -0,0 +1,134 @@ +package qdmp_test + +// Contract exercised by this file (not yet implemented): +// +// exchangeClientCredentials (auth.go ~line 150-184) validates the wire +// expiresAt via parseExpiresAt, which only rejects a non-numeric string or a +// negative value (see auth.go's parseExpiresAt doc comment). It does not +// reject a non-negative expiresAt that is nonetheless already expired +// relative to the current time — e.g. "1", which parses fine as the +// perfectly valid-looking absolute Unix-seconds timestamp 1 (1970-01-01 +// 00:00:01 UTC), a moment far in the past for any real "now". Such a value +// sails through parseExpiresAt (it is not negative) and is written straight +// through to the TokenStore via a.store.Set(...) and returned as if it were +// a fresh, valid token to the very first caller that triggered the exchange +// — even though cachedValid's own `entry.ExpiresAt-tokenRefreshBufferSeconds +// <= now` check would immediately (and correctly) treat that same cached +// entry as expired on the *next* call. In other words: a poisoned/stale +// response is accepted and handed to the caller once, on the very call that +// fetched it, before the cache even gets a chance to reject it. +// +// New requirement: exchangeClientCredentials must also reject (return a +// non-nil error, and never call a.store.Set) whenever the parsed expiresAt +// is <= time.Now().Unix() at the moment the exchange completes — a stale +// expiry must never be treated as a usable, freshly-issued token, exactly +// like the existing negative-expiresAt rejection in +// auth_expiresat_validation_test.go. + +import ( + "context" + "net/http" + "strconv" + "sync/atomic" + "testing" + "time" +) + +// TestAuthGetAccessToken_StaleExpiresAtRejectedAndNotCached mirrors +// TestAuthGetAccessToken_NegativeExpiresAtRejectedAndNotCached in +// auth_expiresat_validation_test.go, but with a non-negative, already- +// expired expiresAt ("1") instead of a negative one ("-100"). The mock +// server returns the poisoned response on its first hit, then a legitimate +// response on every hit after. If the SDK incorrectly accepted/cached the +// poisoned exchange as "fresh" (since expiresAt="1" is not negative and +// today's parseExpiresAt only rejects negative/non-numeric values), the +// first GetAccessToken call would wrongly succeed with the stale token +// and/or the second call would short-circuit without ever hitting the +// server again. Both would defeat the assertions below. +func TestAuthGetAccessToken_StaleExpiresAtRejectedAndNotCached(t *testing.T) { + var calls int64 + srv := startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := atomic.AddInt64(&calls, 1) + if n == 1 { + businessEnvelope(w, http.StatusOK, "0", "ok", "req-staleexp-gat-1", map[string]any{ + "accessToken": "token-from-poisoned-response", + "expiresAt": "1", + }) + return + } + businessEnvelope(w, http.StatusOK, "0", "ok", "req-staleexp-gat-2", map[string]any{ + "accessToken": "token-from-good-response", + "expiresAt": "9999999999", + }) + })) + client := newTestClient(t, srv.URL) + + token, err := client.Auth.GetAccessToken(context.Background()) + if err == nil { + t.Fatalf("first GetAccessToken() error = nil, want an error because expiresAt=\"1\" is already expired (1970-01-01), even though it is not negative") + } + if token != "" { + t.Fatalf("first GetAccessToken() = %q, want the zero value on failure", token) + } + + token, err = client.Auth.GetAccessToken(context.Background()) + if err != nil { + t.Fatalf("second GetAccessToken() error = %v, want nil", err) + } + if token != "token-from-good-response" { + t.Fatalf("second GetAccessToken() = %q, want %q", token, "token-from-good-response") + } + if got := atomic.LoadInt64(&calls); got != 2 { + t.Fatalf("server hit %d times, want exactly 2 (the first stale-expiresAt response must not have been cached as a fresh token, forcing a real second exchange)", got) + } +} + +// TestAuthGetAccessToken_ExpiresWithinBufferRejectedAndNotCached is a +// regression test for a follow-up finding from codex's convergence-check +// review: the fix above only rejected expiresAt <= now, but cachedValid's +// own freshness predicate is stricter — it requires +// entry.ExpiresAt-tokenRefreshBufferSeconds > now (i.e. more than 300 +// seconds of remaining validity), not just "not yet expired". A response +// expiring in, say, 100 seconds (not negative, not even in the past) would +// therefore pass the old check, get cached and handed to the very first +// caller as "fresh" — only for cachedValid to immediately (and correctly) +// call that same entry stale on the *next* call. The write path must use +// the identical predicate as the read path. +func TestAuthGetAccessToken_ExpiresWithinBufferRejectedAndNotCached(t *testing.T) { + var calls int64 + soonExpiresAt := strconv.FormatInt(time.Now().Add(100*time.Second).Unix(), 10) + srv := startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := atomic.AddInt64(&calls, 1) + if n == 1 { + businessEnvelope(w, http.StatusOK, "0", "ok", "req-buffer-gat-1", map[string]any{ + "accessToken": "token-from-poisoned-response", + "expiresAt": soonExpiresAt, + }) + return + } + businessEnvelope(w, http.StatusOK, "0", "ok", "req-buffer-gat-2", map[string]any{ + "accessToken": "token-from-good-response", + "expiresAt": "9999999999", + }) + })) + client := newTestClient(t, srv.URL) + + token, err := client.Auth.GetAccessToken(context.Background()) + if err == nil { + t.Fatalf("first GetAccessToken() error = nil, want an error because expiresAt is only 100s away (less than the 300s refresh buffer)") + } + if token != "" { + t.Fatalf("first GetAccessToken() = %q, want the zero value on failure", token) + } + + token, err = client.Auth.GetAccessToken(context.Background()) + if err != nil { + t.Fatalf("second GetAccessToken() error = %v, want nil", err) + } + if token != "token-from-good-response" { + t.Fatalf("second GetAccessToken() = %q, want %q", token, "token-from-good-response") + } + if got := atomic.LoadInt64(&calls); got != 2 { + t.Fatalf("server hit %d times, want exactly 2 (the first within-buffer response must not have been cached as fresh, forcing a real second exchange)", got) + } +} diff --git a/go/auth_test.go b/go/auth_test.go index 5ef36cc..d410603 100644 --- a/go/auth_test.go +++ b/go/auth_test.go @@ -17,6 +17,7 @@ import ( "encoding/json" "io" "net/http" + "strconv" "strings" "sync" "testing" @@ -47,7 +48,11 @@ func TestAuthGetAccessToken_Success(t *testing.T) { "accessToken": "app-level-access-token", // expiresAt is deliberately a *string* on the wire — this is a // real, confirmed fact (see shared/openapi.yaml), not a guess. - "expiresAt": "1785508950", + // Computed relative to time.Now() (rather than a hardcoded + // absolute timestamp) so this test keeps exercising "a genuinely + // fresh token" indefinitely, instead of silently starting to + // fail once real wall-clock time catches up to a fixed constant. + "expiresAt": strconv.FormatInt(time.Now().Add(time.Hour).Unix(), 10), }) }) srv := startServer(t, counter) @@ -142,6 +147,12 @@ func TestAuthGetAccessToken_ConcurrentSingleFlight(t *testing.T) { // are strings, and a naive numeric-conversion implementation would corrupt // this value silently. func TestAuthCode2Session_Success(t *testing.T) { + // Computed relative to time.Now() (rather than a hardcoded absolute + // timestamp) so this test keeps exercising "a genuinely fresh session" + // indefinitely, instead of silently starting to fail once real + // wall-clock time catches up to a fixed constant (as happened once + // already with a previous hardcoded value here). + wantExpiresAt := strconv.FormatInt(time.Now().Add(time.Hour).Unix(), 10) counter := newRequestCounter(func(w http.ResponseWriter, r *http.Request) { var body map[string]any if err := json.NewDecoder(r.Body).Decode(&body); err != nil { @@ -156,7 +167,7 @@ func TestAuthCode2Session_Success(t *testing.T) { businessEnvelope(w, http.StatusOK, "0", "ok", "req-2", map[string]any{ "accessToken": "user-level-access-token", "refreshToken": "user-level-refresh-token", - "expiresAt": "1785508950", + "expiresAt": wantExpiresAt, "openId": "user-openid-1", }) }) @@ -173,8 +184,8 @@ func TestAuthCode2Session_Success(t *testing.T) { if sess.RefreshToken != "user-level-refresh-token" { t.Fatalf("RefreshToken = %q", sess.RefreshToken) } - if sess.ExpiresAt != "1785508950" { - t.Fatalf("ExpiresAt = %q, want the exact wire string %q (must not be converted to a number)", sess.ExpiresAt, "1785508950") + if sess.ExpiresAt != wantExpiresAt { + t.Fatalf("ExpiresAt = %q, want the exact wire string %q (must not be converted to a number)", sess.ExpiresAt, wantExpiresAt) } if sess.OpenID != "user-openid-1" { t.Fatalf("OpenID = %q", sess.OpenID) @@ -317,6 +328,9 @@ func TestAuthGetAccessToken_CachedOutsideBuffer(t *testing.T) { // TestAuthRefreshToken_HTTP200BusinessFailure above (which only covers the // failure path). func TestAuthRefreshToken_Success(t *testing.T) { + // See TestAuthCode2Session_Success for why this is computed relative to + // time.Now() instead of a hardcoded absolute timestamp. + wantExpiresAt := strconv.FormatInt(time.Now().Add(time.Hour).Unix(), 10) counter := newRequestCounter(func(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost || r.URL.Path != "/auth/v1/refresh" { t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) @@ -330,7 +344,7 @@ func TestAuthRefreshToken_Success(t *testing.T) { } businessEnvelope(w, http.StatusOK, "0", "ok", "req-refresh-1", map[string]any{ "accessToken": "renewed-access-token", - "expiresAt": "1785508950", + "expiresAt": wantExpiresAt, }) }) srv := startServer(t, counter) @@ -343,8 +357,8 @@ func TestAuthRefreshToken_Success(t *testing.T) { if result.AccessToken != "renewed-access-token" { t.Fatalf("AccessToken = %q, want %q", result.AccessToken, "renewed-access-token") } - if result.ExpiresAt != "1785508950" { - t.Fatalf("ExpiresAt = %q, want the exact wire string %q", result.ExpiresAt, "1785508950") + if result.ExpiresAt != wantExpiresAt { + t.Fatalf("ExpiresAt = %q, want the exact wire string %q", result.ExpiresAt, wantExpiresAt) } if counter.Count() != 1 { t.Fatalf("server hit %d times, want 1", counter.Count()) diff --git a/go/client.go b/go/client.go index 2c8ca98..0e61389 100644 --- a/go/client.go +++ b/go/client.go @@ -2,7 +2,10 @@ package qdmp import ( "fmt" + "net" "net/http" + "net/url" + "strings" "time" ) @@ -89,6 +92,9 @@ func NewClient(opts ClientOptions) (*Client, error) { if baseURL == "" { baseURL = defaultBaseURL } + if err := validateBaseURLScheme(baseURL); err != nil { + return nil, err + } qdmpVersion := opts.QdmpVersion if qdmpVersion == "" { qdmpVersion = defaultQdmpVersion @@ -132,6 +138,53 @@ func NewClient(opts ClientOptions) (*Client, error) { return c, nil } +// validateBaseURLScheme rejects a plain-http BaseURL pointed at a +// non-loopback host. appId/appSecret and access-token values are sent as +// plain headers/body fields (see request.go doRequest), so a plain-http +// BaseURL to a real host would expose them in the clear to anyone able to +// observe the network path. http:// remains allowed for loopback/local +// addresses (127.0.0.0/8, or "localhost", case-insensitive) since this +// repository's own test suite points BaseURL at an httptest.Server, whose +// .URL is exactly "http://127.0.0.1:PORT". +func validateBaseURLScheme(baseURL string) error { + u, err := url.Parse(baseURL) + if err != nil { + return fmt.Errorf("qdmp: ClientOptions.BaseURL is not a valid URL: %w", err) + } + if u.Scheme != "http" { + return nil + } + if isLoopbackHost(u.Host) { + return nil + } + return fmt.Errorf("qdmp: ClientOptions.BaseURL %q uses a plain http:// scheme for a non-loopback host; use https:// to avoid sending appSecret/access-token in the clear", baseURL) +} + +// isLoopbackHost reports whether host (as found in a url.URL.Host, which may +// carry a ":port" suffix) refers to a loopback/local address: the exact +// hostname "localhost" (case-insensitive), or a genuine loopback IP literal +// (127.0.0.1, any other bare 127.x.x.x dotted-quad, or the IPv6 "::1"). +// +// This deliberately does NOT do a string-prefix check like +// strings.HasPrefix(h, "127."): that would also match ordinary domain names +// that merely start with the string "127." (e.g. "127.attacker.com" or +// "127.0.0.1.evil.com"), which are not IP addresses at all and could resolve +// via DNS to any attacker-controlled host. net.ParseIP only parses h as an +// IP-address literal — it never performs a DNS lookup — so this stays a pure +// string-literal check with no network access. +func isLoopbackHost(host string) bool { + h := host + if hostOnly, _, err := net.SplitHostPort(host); err == nil { + h = hostOnly + } + h = strings.ToLower(h) + if h == "localhost" { + return true + } + ip := net.ParseIP(h) + return ip != nil && ip.IsLoopback() +} + // UserClient is a qdmp client derived from Client via WithAccessToken, // carrying a single access token (user-level or, where explicitly opted // into, app-level) that every business group method on it uses. This is the diff --git a/go/client_baseurl_scheme_test.go b/go/client_baseurl_scheme_test.go new file mode 100644 index 0000000..74804da --- /dev/null +++ b/go/client_baseurl_scheme_test.go @@ -0,0 +1,123 @@ +package qdmp_test + +// Contract exercised by this file (not yet implemented): +// +// NewClient (client.go ~line 80) currently does not inspect +// ClientOptions.BaseURL's scheme at all — a plain "http://" BaseURL pointed +// at a real host is accepted exactly like "https://". Since the access-token +// and appId/appSecret are sent as plain headers/body fields (see request.go +// doRequest), a plain-http BaseURL to a non-loopback host would leak them to +// anyone able to observe the network path. +// +// New requirement: NewClient must reject (return a non-nil error, no panic) +// any BaseURL whose scheme is "http" (not "https") UNLESS the host is a +// loopback/local address — 127.0.0.1, any 127.x.x.x address, or +// "localhost" (case-insensitive) — which must continue to work unchanged, +// since every existing test in this package points BaseURL at an +// httptest.Server whose .URL is exactly "http://127.0.0.1:PORT". + +import ( + "testing" + + qdmp "github.com/EchoTechFE/qdmp-server-sdk/go" +) + +// TestNewClient_RejectsPlainHTTPForNonLoopbackHost is the core regression +// test: a plain-http BaseURL pointed at a real (non-loopback) host must +// never be accepted, since it would send appSecret/access-token headers in +// the clear over the network. +func TestNewClient_RejectsPlainHTTPForNonLoopbackHost(t *testing.T) { + client, err := qdmp.NewClient(qdmp.ClientOptions{ + AppID: "a", + AppSecret: "s", + BaseURL: "http://evil.example.com", + }) + if err == nil { + t.Fatalf("NewClient() error = nil, want an error for a plain-http, non-loopback BaseURL") + } + if client != nil { + t.Fatalf("NewClient() client = %+v, want nil when BaseURL validation fails", client) + } +} + +// TestNewClient_AllowsPlainHTTPForLoopbackIP verifies the 127.0.0.1 loopback +// exception this repository's own test suite depends on +// (httptest.NewServer's .URL is exactly this shape). +func TestNewClient_AllowsPlainHTTPForLoopbackIP(t *testing.T) { + client, err := qdmp.NewClient(qdmp.ClientOptions{ + AppID: "a", + AppSecret: "s", + BaseURL: "http://127.0.0.1:12345", + }) + if err != nil { + t.Fatalf("NewClient() error = %v, want nil for a loopback (127.0.0.1) http:// BaseURL", err) + } + if client == nil { + t.Fatalf("NewClient() client = nil, want a non-nil client") + } +} + +// TestNewClient_AllowsPlainHTTPForOther127Address verifies the exception +// extends to the whole 127.0.0.0/8 loopback block, not just the single +// address 127.0.0.1. +func TestNewClient_AllowsPlainHTTPForOther127Address(t *testing.T) { + client, err := qdmp.NewClient(qdmp.ClientOptions{ + AppID: "a", + AppSecret: "s", + BaseURL: "http://127.5.6.7:8080", + }) + if err != nil { + t.Fatalf("NewClient() error = %v, want nil for a 127.x.x.x http:// BaseURL", err) + } + if client == nil { + t.Fatalf("NewClient() client = nil, want a non-nil client") + } +} + +// TestNewClient_AllowsPlainHTTPForLocalhost verifies the "localhost" hostname +// exception. +func TestNewClient_AllowsPlainHTTPForLocalhost(t *testing.T) { + client, err := qdmp.NewClient(qdmp.ClientOptions{ + AppID: "a", + AppSecret: "s", + BaseURL: "http://localhost:12345", + }) + if err != nil { + t.Fatalf("NewClient() error = %v, want nil for a localhost http:// BaseURL", err) + } + if client == nil { + t.Fatalf("NewClient() client = nil, want a non-nil client") + } +} + +// TestNewClient_AllowsPlainHTTPForLocalhostCaseInsensitive verifies the +// "localhost" exception is matched case-insensitively. +func TestNewClient_AllowsPlainHTTPForLocalhostCaseInsensitive(t *testing.T) { + client, err := qdmp.NewClient(qdmp.ClientOptions{ + AppID: "a", + AppSecret: "s", + BaseURL: "http://LOCALHOST:12345", + }) + if err != nil { + t.Fatalf("NewClient() error = %v, want nil for a case-insensitive LOCALHOST http:// BaseURL", err) + } + if client == nil { + t.Fatalf("NewClient() client = nil, want a non-nil client") + } +} + +// TestNewClient_AllowsHTTPS verifies a normal https:// BaseURL to a +// non-loopback host still works unchanged. +func TestNewClient_AllowsHTTPS(t *testing.T) { + client, err := qdmp.NewClient(qdmp.ClientOptions{ + AppID: "a", + AppSecret: "s", + BaseURL: "https://example.com", + }) + if err != nil { + t.Fatalf("NewClient() error = %v, want nil for an https:// BaseURL", err) + } + if client == nil { + t.Fatalf("NewClient() client = nil, want a non-nil client") + } +} diff --git a/go/client_loopback_ip_literal_test.go b/go/client_loopback_ip_literal_test.go new file mode 100644 index 0000000..4761409 --- /dev/null +++ b/go/client_loopback_ip_literal_test.go @@ -0,0 +1,104 @@ +package qdmp_test + +// Contract exercised by this file (not yet implemented): +// +// isLoopbackHost (client.go ~line 166-172) decides whether a plain "http://" +// BaseURL is allowed for a given host by doing a naive string check: +// +// h == "localhost" || h == "127.0.0.1" || strings.HasPrefix(h, "127.") +// +// strings.HasPrefix(h, "127.") is not an IP-literal check — it is satisfied +// by any hostname that merely starts with the four characters "127.", even +// when the host is not an IP address at all. A domain name such as +// "127.attacker.com" trivially passes this check today, even though it is a +// perfectly ordinary DNS name that could resolve to any attacker-controlled +// IP address anywhere on the internet. Since validateBaseURLScheme only +// treats http:// as safe when isLoopbackHost is true, this lets an attacker- +// chosen BaseURL smuggle a plain-http, non-loopback destination past the +// scheme check, leaking appId/appSecret/access-token headers in plaintext to +// that remote host. +// +// New requirement: isLoopbackHost (via validateBaseURLScheme/NewClient) must +// only recognize genuine loopback IP literals (127.0.0.1 and other bare +// 127.x.x.x dotted-quad addresses with no additional labels) or the exact +// hostname "localhost" (case-insensitive) — never a domain name that merely +// starts with the string "127." or "127.0.0.1" followed by more labels. + +import ( + "testing" + + qdmp "github.com/EchoTechFE/qdmp-server-sdk/go" +) + +// TestNewClient_RejectsNonLoopbackDomainStartingWith127 is the core +// regression test: "127.attacker.com" is a domain name, not an IP address — +// it must be rejected for a plain-http BaseURL exactly like any other +// non-loopback host, since DNS could resolve it to a remote attacker- +// controlled server. +func TestNewClient_RejectsNonLoopbackDomainStartingWith127(t *testing.T) { + client, err := qdmp.NewClient(qdmp.ClientOptions{ + AppID: "a", + AppSecret: "s", + BaseURL: "http://127.attacker.com", + }) + if err == nil { + t.Fatalf("NewClient() error = nil, want an error for a plain-http BaseURL whose host is the domain name 127.attacker.com (not a loopback IP literal)") + } + if client != nil { + t.Fatalf("NewClient() client = %+v, want nil when BaseURL validation fails", client) + } +} + +// TestNewClient_RejectsLoopbackIPWithExtraDomainLabels is a second shape of +// the same bug: "127.0.0.1.evil.com" has the exact string "127.0.0.1" as a +// prefix, but it is a five-label domain name, not the loopback IP literal +// 127.0.0.1 — it must resolve via DNS like any other domain and must not be +// treated as loopback. +func TestNewClient_RejectsLoopbackIPWithExtraDomainLabels(t *testing.T) { + client, err := qdmp.NewClient(qdmp.ClientOptions{ + AppID: "a", + AppSecret: "s", + BaseURL: "http://127.0.0.1.evil.com", + }) + if err == nil { + t.Fatalf("NewClient() error = nil, want an error for a plain-http BaseURL whose host is the domain name 127.0.0.1.evil.com (not a loopback IP literal)") + } + if client != nil { + t.Fatalf("NewClient() client = %+v, want nil when BaseURL validation fails", client) + } +} + +// TestNewClient_StillAllowsPlainHTTPForLoopbackIP is a sanity check that the +// fix for the above must not break: a genuine loopback IP literal must +// continue to be accepted with a plain http:// BaseURL, since this +// repository's own test suite depends on it (httptest.NewServer's .URL is +// exactly "http://127.0.0.1:PORT"). +func TestNewClient_StillAllowsPlainHTTPForLoopbackIP(t *testing.T) { + client, err := qdmp.NewClient(qdmp.ClientOptions{ + AppID: "a", + AppSecret: "s", + BaseURL: "http://127.0.0.1:12345", + }) + if err != nil { + t.Fatalf("NewClient() error = %v, want nil for a genuine loopback (127.0.0.1) http:// BaseURL", err) + } + if client == nil { + t.Fatalf("NewClient() client = nil, want a non-nil client") + } +} + +// TestNewClient_StillAllowsPlainHTTPForLocalhost is a sanity check that the +// fix for the above must not break the "localhost" hostname exception. +func TestNewClient_StillAllowsPlainHTTPForLocalhost(t *testing.T) { + client, err := qdmp.NewClient(qdmp.ClientOptions{ + AppID: "a", + AppSecret: "s", + BaseURL: "http://localhost:12345", + }) + if err != nil { + t.Fatalf("NewClient() error = %v, want nil for a localhost http:// BaseURL", err) + } + if client == nil { + t.Fatalf("NewClient() client = nil, want a non-nil client") + } +} diff --git a/go/errors.go b/go/errors.go index b7bbb46..c6ad592 100644 --- a/go/errors.go +++ b/go/errors.go @@ -56,3 +56,28 @@ func (e *QdmpApiError) Error() string { } return fmt.Sprintf("qdmp: request failed (code=%s, httpStatus=%d): %s", e.Code, e.HTTPStatus, e.Message) } + +// maxErrorFieldLength bounds how much of a server-supplied Message/ +// RequestID sanitizeForErrorMessage will keep, so a misbehaving upstream +// can't make every log line built from a *QdmpApiError unbounded. +const maxErrorFieldLength = 2000 + +// sanitizeForErrorMessage neutralizes control characters (including raw +// CR/LF, which could otherwise be used to forge a fake extra "log line" when +// this value is later written to a log — classic log injection) and bounds +// the length of server-supplied text. It is applied to QdmpApiError's +// Message/RequestID fields themselves (see doRequest in request.go), rather +// than only inside Error(), so a caller reading these exported fields +// directly also only ever sees the sanitized value. +func sanitizeForErrorMessage(s string) string { + runes := []rune(s) + if len(runes) > maxErrorFieldLength { + runes = runes[:maxErrorFieldLength] + } + for i, r := range runes { + if r < 0x20 || r == 0x7F { + runes[i] = ' ' + } + } + return string(runes) +} diff --git a/go/errors_code_safety_test.go b/go/errors_code_safety_test.go new file mode 100644 index 0000000..75debf3 --- /dev/null +++ b/go/errors_code_safety_test.go @@ -0,0 +1,73 @@ +package qdmp_test + +// Contract exercised by this file (not yet implemented): +// +// QdmpApiError's Message and RequestID fields are already sanitized via +// sanitizeForErrorMessage at construction time, in doRequest (request.go +// ~line 160-169), before the error is ever handed back to a caller — this +// defends against log injection (a malicious/buggy upstream embedding a raw +// CR/LF to forge a fake extra "log line") and unbounded length. The Code +// field, constructed on the very same struct literal right next to Message +// and RequestID, is not sanitized the same way — it is assigned the raw +// output of normalizeCode(env.Code) verbatim. +// +// Code is documented (errors.go, QdmpApiError's doc comment) as an +// intentionally open string, not a closed enum/int, because the qdmp API is +// known to use non-numeric-looking business codes in one envelope shape. +// normalizeCode (request.go) explicitly supports the wire code field being +// encoded as either a bare JSON number or a JSON string (see its +// `trimmed[0] == '"'` branch) — so a malicious/buggy upstream can send a +// string-form code containing embedded raw CR/LF bytes (or an unbounded +// length), and that string flows untouched through normalizeCode straight +// into QdmpApiError.Code, and from there into Error()'s output — the exact +// same log-injection risk Message/RequestID are already protected against. +// +// New requirement: Code must be sanitized the same way as Message/RequestID +// at construction time (in doRequest, via sanitizeForErrorMessage or +// equivalent), so both Error()'s formatted output and the exported Code +// field itself are free of raw CR/LF bytes and bounded in length. + +import ( + "context" + "errors" + "net/http" + "strings" + "testing" + + qdmp "github.com/EchoTechFE/qdmp-server-sdk/go" +) + +// TestQdmpApiError_CodeCRLFNotEchoedRaw drives a business failure whose +// wire `code` field is a JSON *string* (not a bare number) containing an +// embedded CRLF sequence designed to look like a second, fake log line if +// echoed raw into a log file. Passing a Go string as businessEnvelope's code +// argument makes it JSON-encode as a quoted string, exactly matching the +// string-form business codes normalizeCode's `trimmed[0] == '"'` branch is +// built to support. Both Error()'s formatted output and the exported Code +// field on the underlying *qdmp.QdmpApiError must be free of any raw \r or +// \n byte. +func TestQdmpApiError_CodeCRLFNotEchoedRaw(t *testing.T) { + const injectedCode = "1\r\nFAKE injected log line" + srv := startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + businessEnvelope(w, http.StatusOK, injectedCode, "some failure", "req-crlf-code-1", nil) + })) + client := newTestClient(t, srv.URL) + + _, err := client.WithAccessToken("some-token").User.Me(context.Background()) + if err == nil { + t.Fatalf("User.Me() error = nil, want an error because code=%q is not \"0\"", injectedCode) + } + + got := err.Error() + if strings.ContainsRune(got, '\r') || strings.ContainsRune(got, '\n') { + t.Fatalf("err.Error() = %q, must not contain a raw CR or LF byte from the server-supplied code (log-injection risk)", got) + } + + var apiErr *qdmp.QdmpApiError + if !errors.As(err, &apiErr) { + t.Fatalf("errors.As(err, &apiErr) = false, want true: err should be a *qdmp.QdmpApiError, got %T", err) + } + if strings.ContainsRune(apiErr.Code, '\r') || strings.ContainsRune(apiErr.Code, '\n') { + t.Fatalf("apiErr.Code = %q, must not contain a raw CR or LF byte from the server-supplied code (the exported field itself must be sanitized at construction time, not just cosmetically hidden in Error()'s formatting)", apiErr.Code) + } +} diff --git a/go/errors_message_safety_test.go b/go/errors_message_safety_test.go new file mode 100644 index 0000000..cdf0736 --- /dev/null +++ b/go/errors_message_safety_test.go @@ -0,0 +1,83 @@ +package qdmp_test + +// Contract exercised by this file (not yet implemented): +// +// QdmpApiError.Error() (errors.go ~line 53) interpolates the server-supplied +// Message and RequestID fields into its output via plain fmt.Sprintf, with +// no escaping/sanitization or length bound. A malicious or buggy upstream +// response could therefore inject raw CR/LF bytes into a message that later +// gets written to a log file (classic log-injection: a fake extra "log +// line"), or send an unbounded message to make every log line built from +// this error unbounded too. +// +// New requirement: QdmpApiError.Error() must neutralize raw \r / \n bytes +// coming from the server-supplied Message/RequestID (so log-injection via +// this error is not possible) and must bound the overall length of the +// string it returns. + +import ( + "context" + "net/http" + "strings" + "testing" +) + +// TestQdmpApiError_MessageCRLFNotEchoedRaw drives a business failure whose +// message contains an embedded CRLF sequence designed to look like a second, +// fake log line if echoed raw into a log file. Error() must not contain a +// literal \r or \n anywhere in its output. +func TestQdmpApiError_MessageCRLFNotEchoedRaw(t *testing.T) { + const injected = "line1\r\nFAKE injected log line" + srv := startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + businessEnvelope(w, http.StatusOK, float64(20000), injected, "req-crlf-msg-1", nil) + })) + client := newTestClient(t, srv.URL) + + _, err := client.WithAccessToken("some-token").User.Me(context.Background()) + if err == nil { + t.Fatalf("User.Me() error = nil, want an error because code=20000") + } + got := err.Error() + if strings.ContainsRune(got, '\r') || strings.ContainsRune(got, '\n') { + t.Fatalf("err.Error() = %q, must not contain a raw CR or LF byte from the server-supplied message (log-injection risk)", got) + } +} + +// TestQdmpApiError_RequestIDCRLFNotEchoedRaw is the same log-injection +// scenario, but via the RequestID field instead of Message. +func TestQdmpApiError_RequestIDCRLFNotEchoedRaw(t *testing.T) { + const injectedRequestID = "req-1\r\nFAKE injected log line" + srv := startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + businessEnvelope(w, http.StatusOK, float64(20000), "some failure", injectedRequestID, nil) + })) + client := newTestClient(t, srv.URL) + + _, err := client.WithAccessToken("some-token").User.Me(context.Background()) + if err == nil { + t.Fatalf("User.Me() error = nil, want an error because code=20000") + } + got := err.Error() + if strings.ContainsRune(got, '\r') || strings.ContainsRune(got, '\n') { + t.Fatalf("err.Error() = %q, must not contain a raw CR or LF byte from the server-supplied requestId (log-injection risk)", got) + } +} + +// TestQdmpApiError_LongMessageIsBounded drives a business failure whose +// message is ~200000 characters long and asserts the resulting Error() +// string has a bounded length, proving the server-supplied message is +// truncated rather than echoed verbatim and unbounded. +func TestQdmpApiError_LongMessageIsBounded(t *testing.T) { + longMessage := strings.Repeat("a", 200000) + srv := startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + businessEnvelope(w, http.StatusOK, float64(20000), longMessage, "req-long-msg-1", nil) + })) + client := newTestClient(t, srv.URL) + + _, err := client.WithAccessToken("some-token").User.Me(context.Background()) + if err == nil { + t.Fatalf("User.Me() error = nil, want an error because code=20000") + } + if got := len(err.Error()); got >= 5000 { + t.Fatalf("len(err.Error()) = %d, want a bounded/truncated length (< 5000); the ~200000-char server-supplied message must not be echoed verbatim", got) + } +} diff --git a/go/request.go b/go/request.go index bf5cfab..afcc388 100644 --- a/go/request.go +++ b/go/request.go @@ -25,6 +25,11 @@ const ( authSchemeGenai authScheme = "genai" ) +// maxResponseBodyBytes bounds how much of an HTTP response body doRequest +// will ever read into memory, protecting against a misbehaving/compromised +// gateway (or a man-in-the-middle) returning an arbitrarily large body. +const maxResponseBodyBytes = 10 * 1024 * 1024 // 10MB + // requestParams describes one outbound call to request(). It intentionally // has no notion of "token missing" handling: callers (the business group // methods) are responsible for the fail-fast-locally check before ever @@ -127,10 +132,20 @@ func (c *Client) doRequest(ctx context.Context, p requestParams) (json.RawMessag return nil, fmt.Errorf("qdmp: unexpected redirect response (http %d)", resp.StatusCode) } - raw, err := io.ReadAll(resp.Body) + // Cap how much of the response body we will ever buffer: a misbehaving + // or compromised gateway (or a man-in-the-middle) returning an + // arbitrarily large body must not be able to exhaust memory before the + // SDK even attempts to JSON-decode it. Reading one byte past the limit + // lets us distinguish "exactly at the limit" from "over the limit" + // without buffering an unbounded amount either way. + limited := io.LimitReader(resp.Body, maxResponseBodyBytes+1) + raw, err := io.ReadAll(limited) if err != nil { return nil, fmt.Errorf("qdmp: failed to read response body (http status %d): %w", resp.StatusCode, err) } + if len(raw) > maxResponseBodyBytes { + return nil, fmt.Errorf("qdmp: response body exceeds the maximum allowed size of %d bytes (http status %d)", maxResponseBodyBytes, resp.StatusCode) + } var env rawEnvelope if err := json.Unmarshal(raw, &env); err != nil { @@ -144,9 +159,15 @@ func (c *Client) doRequest(ctx context.Context, p requestParams) (json.RawMessag if code != "0" { return nil, &QdmpApiError{ - Code: code, - Message: env.Message, - RequestID: env.RequestID, + // Sanitized here (not only in Error()) so a caller reading + // Code/Message/RequestID directly off the struct also never sees + // raw CR/LF or an unbounded server-supplied string. Code is a + // server-controlled open string (see normalizeCode and + // QdmpApiError's doc comment), so it needs exactly the same + // treatment as Message/RequestID. + Code: sanitizeForErrorMessage(code), + Message: sanitizeForErrorMessage(env.Message), + RequestID: sanitizeForErrorMessage(env.RequestID), HTTPStatus: resp.StatusCode, } } diff --git a/go/request_body_size_limit_test.go b/go/request_body_size_limit_test.go new file mode 100644 index 0000000..aec1ec8 --- /dev/null +++ b/go/request_body_size_limit_test.go @@ -0,0 +1,53 @@ +package qdmp_test + +// Contract exercised by this file (not yet implemented): +// +// doRequest (request.go ~line 130) reads the entire response body via +// io.ReadAll(resp.Body) with no size limit whatsoever. A misbehaving or +// compromised qdmp gateway (or a man-in-the-middle) returning an +// arbitrarily large response body would make the SDK buffer it entirely in +// memory before even attempting to JSON-decode it — an easy memory- +// exhaustion vector for anything driving qdmp SDK calls against an +// untrusted or unreliable endpoint. +// +// New requirement: doRequest must enforce an upper bound on how much of the +// response body it will read, surfacing a non-nil error for a response that +// exceeds it rather than buffering the whole thing and only failing later +// (or, as this test demonstrates, not failing at all) at JSON-decode time. + +import ( + "context" + "net/http" + "strings" + "testing" +) + +// TestDoRequest_OversizedResponseBodyRejected drives a real business call +// (User.Me) against a mock server that returns an ~11MB response that is +// still perfectly valid JSON matching the business envelope + UserMeResult +// shape (a huge but otherwise legitimate avatar URL string) — chosen +// deliberately so that, absent a size cap, doRequest would decode it +// successfully and return err == nil, proving there is currently no limit +// on how much of the response body gets buffered/parsed. With a size cap in +// place this must instead produce a non-nil error. +func TestDoRequest_OversizedResponseBodyRejected(t *testing.T) { + const oversized = 11 * 1024 * 1024 // 11MB + hugeAvatar := strings.Repeat("a", oversized) + + srv := startServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + businessEnvelope(w, http.StatusOK, "0", "ok", "req-huge-1", map[string]any{ + "id": "user-huge", + "nickname": "n", + "avatar": hugeAvatar, + }) + })) + client := newTestClient(t, srv.URL) + + me, err := client.WithAccessToken("some-token").User.Me(context.Background()) + if err == nil { + t.Fatalf("User.Me() error = nil, want an error because the response body (%d bytes) exceeds the SDK's size limit; it decoded successfully as a valid oversized envelope, proving no size cap is currently enforced", oversized) + } + if me != nil { + t.Fatalf("User.Me() result = %+v, want nil when the response is rejected as oversized", me) + } +} diff --git a/java/gradle/wrapper/gradle-wrapper.properties b/java/gradle/wrapper/gradle-wrapper.properties index df6a6ad..bd82f36 100644 --- a/java/gradle/wrapper/gradle-wrapper.properties +++ b/java/gradle/wrapper/gradle-wrapper.properties @@ -1,5 +1,6 @@ distributionBase=GRADLE_USER_HOME distributionPath=wrapper/dists +distributionSha256Sum=bafc141b619ad6350fd975fc903156dd5c151998cc8b058e8c1044ab5f7b031f distributionUrl=https\://services.gradle.org/distributions/gradle-9.5.1-bin.zip networkTimeout=10000 retries=0 diff --git a/java/src/main/java/io/github/echotechfe/qdmp/QdmpClientConfig.java b/java/src/main/java/io/github/echotechfe/qdmp/QdmpClientConfig.java index 80e8cef..68ad6fc 100644 --- a/java/src/main/java/io/github/echotechfe/qdmp/QdmpClientConfig.java +++ b/java/src/main/java/io/github/echotechfe/qdmp/QdmpClientConfig.java @@ -2,6 +2,8 @@ import io.github.echotechfe.qdmp.auth.InMemoryTokenStore; import io.github.echotechfe.qdmp.auth.TokenStore; +import java.net.URI; +import java.net.URISyntaxException; import java.time.Clock; import java.util.Objects; @@ -64,6 +66,55 @@ public Clock getClock() { return clock; } + // Package-private (not private) so QdmpTransport can apply this exact same loopback/localhost + // check to a baseUrl passed directly to its own public constructor -- that constructor is + // reachable without ever going through this builder (see the comment on QdmpTransport's + // constructor), so it must not rely on this class being the only caller. + // + // Deliberately a strict dotted-decimal IPv4 literal check, not a String#startsWith("127.") + // prefix match: a hostname like "127.attacker.com" starts with "127." but is not an IP literal + // at all -- it is an ordinary domain name that DNS can resolve to any attacker-controlled + // server, which would defeat the whole point of this loopback exception. It also deliberately + // does NOT delegate to InetAddress.getByName(host).isLoopbackAddress(): that method performs a + // real DNS resolution, which is both unwanted here (we only want to know whether the string + // itself is an IPv4 loopback literal, not where a hostname happens to resolve today) and would + // itself be a vector for the same class of bug. + static boolean isLoopbackOrLocalhost(String host) { + if (host == null) { + return false; + } + if ("localhost".equalsIgnoreCase(host)) { + return true; + } + return isIpv4LoopbackLiteral(host); + } + + private static boolean isIpv4LoopbackLiteral(String host) { + String[] octets = host.split("\\.", -1); + if (octets.length != 4 || !"127".equals(octets[0])) { + return false; + } + for (String octet : octets) { + if (!isValidIpv4Octet(octet)) { + return false; + } + } + return true; + } + + private static boolean isValidIpv4Octet(String octet) { + if (octet.isEmpty() || octet.length() > 3) { + return false; + } + for (int i = 0; i < octet.length(); i++) { + if (!Character.isDigit(octet.charAt(i))) { + return false; + } + } + int value = Integer.parseInt(octet); + return value >= 0 && value <= 255; + } + /** Builder for {@link QdmpClientConfig}. */ public static final class Builder { @@ -118,7 +169,29 @@ public QdmpClientConfig build() { Objects.requireNonNull(qdmpVersion, "qdmpVersion must not be null"); Objects.requireNonNull(tokenStore, "tokenStore must not be null"); Objects.requireNonNull(clock, "clock must not be null"); + requireHttpsOrLoopback(baseUrl); return new QdmpClientConfig(this); } + + // Every business/auth call carries the app secret and/or an access token as a request header, + // so an http:// base URL (attacker-controlled or just misconfigured) would send those + // credentials in plaintext over the network. loopback/localhost is exempted because + // okhttp3.mockwebserver.MockWebServer, used throughout this SDK's own test suite, always + // listens on http://127.0.0.1:PORT. + private static void requireHttpsOrLoopback(String baseUrl) { + URI uri; + try { + uri = new URI(baseUrl); + } catch (URISyntaxException e) { + throw new IllegalArgumentException("invalid baseUrl: " + baseUrl, e); + } + if (!"http".equalsIgnoreCase(uri.getScheme())) { + return; + } + if (!isLoopbackOrLocalhost(uri.getHost())) { + throw new IllegalArgumentException( + "baseUrl must use https:// unless the host is loopback/localhost, got: " + baseUrl); + } + } } } diff --git a/java/src/main/java/io/github/echotechfe/qdmp/QdmpContext.java b/java/src/main/java/io/github/echotechfe/qdmp/QdmpContext.java index 1d565d3..df0aa15 100644 --- a/java/src/main/java/io/github/echotechfe/qdmp/QdmpContext.java +++ b/java/src/main/java/io/github/echotechfe/qdmp/QdmpContext.java @@ -36,19 +36,22 @@ public static QdmpContext of(String accessToken) { return new QdmpContext(accessToken); } - // Rejects control characters and anything outside ISO-8859-1 before the token ever reaches - // QdmpTransport's OkHttp Request.Builder#header calls. Without this gate, a token with e.g. an - // embedded newline or a non-Latin1 character would be rejected by OkHttp's own header - // validation -- but OkHttp only redacts Authorization/Cookie/Proxy-Authorization/Set-Cookie in - // that exception's message, so "access-token"/"x-openapi-access-token" would otherwise have the - // full (invalid) token value echoed straight into the thrown IllegalArgumentException. The - // message below intentionally never includes the token itself. - private static void requireHeaderSafe(String accessToken) { + // Whitelists printable ASCII (0x20-0x7E) before the token ever reaches QdmpTransport's OkHttp + // Request.Builder#header calls, rejecting everything else -- including tab (0x09) and the whole + // Latin-1 Supplement range (0x80-0xFF, e.g. accented characters). A blacklist that only rejects + // control characters and non-Latin-1 code points is not strict enough: OkHttp's own header + // validation is stricter still (tab or 0x20-0x7E only), and for header names outside its + // hardcoded redaction list (Authorization/Cookie/Proxy-Authorization/Set-Cookie -- + // "access-token"/"x-openapi-access-token" are not on it) it echoes the *entire* invalid header + // value into its thrown IllegalArgumentException message. Whitelisting here means that path is + // never reached. Package-private (not private) so QdmpTransport can reuse the exact same check + // for tokens passed directly to it, bypassing QdmpContext entirely. The message below + // intentionally never includes the token itself. + static void requireHeaderSafe(String accessToken) { for (int i = 0; i < accessToken.length(); i++) { char c = accessToken.charAt(i); - boolean isControlCharacter = c <= 0x1f || c == 0x7f; - boolean isOutsideLatin1 = c > 0xff; - if (isControlCharacter || isOutsideLatin1) { + boolean isPrintableAscii = c >= 0x20 && c <= 0x7e; + if (!isPrintableAscii) { throw new IllegalArgumentException( "accessToken contains a character that cannot be safely sent as an HTTP header value" + " (this message intentionally omits the token itself)"); 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 64c3868..003c5df 100644 --- a/java/src/main/java/io/github/echotechfe/qdmp/QdmpTransport.java +++ b/java/src/main/java/io/github/echotechfe/qdmp/QdmpTransport.java @@ -6,6 +6,8 @@ import io.github.echotechfe.qdmp.errors.QdmpApiError; import io.github.echotechfe.qdmp.errors.QdmpTransportException; import java.io.IOException; +import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; import java.util.List; import java.util.Map; import okhttp3.HttpUrl; @@ -15,6 +17,8 @@ import okhttp3.RequestBody; import okhttp3.Response; import okhttp3.ResponseBody; +import okio.Buffer; +import okio.BufferedSource; /** * Internal HTTP transport shared by {@link io.github.echotechfe.qdmp.auth.QdmpAuth} and every @@ -33,6 +37,10 @@ public final class QdmpTransport { private static final MediaType JSON_MEDIA_TYPE = MediaType.get("application/json; charset=utf-8"); + // A malicious or malfunctioning endpoint could otherwise return an arbitrarily large response + // body, which readBody would buffer entirely into memory before even attempting to parse it. + private static final long MAX_RESPONSE_BODY_BYTES = 10L * 1024 * 1024; + // Generated response models have no @JsonIgnoreProperties and openapi.yaml itself flags several // `data` schemas as inferred/unverified -- the server is free to add fields we didn't predict. // Forward compatibility requires tolerating unknown JSON properties instead of throwing. @@ -57,6 +65,19 @@ public QdmpTransport(OkHttpClient httpClient, String baseUrl, String qdmpVersion if (parsed == null) { throw new IllegalArgumentException("invalid baseUrl: " + baseUrl); } + // QdmpClientConfig.Builder#build() rejects an http:// baseUrl unless the host is + // loopback/localhost, but that check only runs if the caller goes through QdmpClientConfig. + // This constructor is public (only because groups/auth live in separate packages from this + // class) and is reachable directly, so a caller bypassing QdmpClientConfig entirely could + // otherwise construct a transport pointed at an http:// host and send appSecret/access-token + // headers in plaintext. Re-run the identical check here, sharing QdmpClientConfig's loopback + // literal check, for the same defense-in-depth reason the followRedirects(false) call below + // does not rely on QdmpClient being the only caller. + if ("http".equalsIgnoreCase(parsed.scheme()) + && !QdmpClientConfig.isLoopbackOrLocalhost(parsed.host())) { + throw new IllegalArgumentException( + "baseUrl must use https:// unless the host is loopback/localhost, got: " + baseUrl); + } // 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 @@ -130,10 +151,12 @@ private HttpUrl.Builder pathUrlBuilder(String path) { private void applyAuthHeaders(Request.Builder builder, AuthScheme scheme, String accessToken) { switch (scheme) { case STANDARD: + requireHeaderSafeToken(accessToken); builder.header("access-token", accessToken); builder.header("x-echo-qdmp-version", qdmpVersion); break; case GENAI: + requireHeaderSafeToken(accessToken); builder.header("x-openapi-access-token", accessToken); builder.header("x-openapi-app-id", appId); break; @@ -143,6 +166,20 @@ private void applyAuthHeaders(Request.Builder builder, AuthScheme scheme, String } } + // QdmpTransport is public (only because groups/auth live in separate packages from this class), + // so it is reachable directly by a caller that bypasses QdmpContext's validation entirely and + // passes a bare String accessToken straight to get/post. Without this check, such a caller could + // push a token containing e.g. an embedded newline straight into OkHttp's Request.Builder#header + // call: OkHttp's own header validation would then reject it, but -- since neither + // "access-token" nor "x-openapi-access-token" are on OkHttp's hardcoded redaction list + // (Authorization/Cookie/Proxy-Authorization/Set-Cookie) -- it echoes the entire raw (invalid) + // header value, including the token, into the thrown IllegalArgumentException's message. Running + // the same whitelist QdmpContext#of already enforces before ever calling builder.header(...) + // means that OkHttp code path -- and its token-leaking exception message -- is never reached. + private static void requireHeaderSafeToken(String accessToken) { + QdmpContext.requireHeaderSafe(accessToken); + } + private void addQueryParams(HttpUrl.Builder urlBuilder, Map query) { for (Map.Entry entry : query.entrySet()) { Object value = entry.getValue(); @@ -196,6 +233,23 @@ private T execute(Request request, Class dataType) { private String readBody(Response response) throws IOException { ResponseBody body = response.body(); - return body == null ? "" : body.string(); + if (body == null) { + return ""; + } + BufferedSource source = body.source(); + // Reads at most MAX_RESPONSE_BODY_BYTES + 1 bytes from the underlying connection -- enough to + // detect an oversized body without ever buffering the rest of it into memory. + source.request(MAX_RESPONSE_BODY_BYTES + 1); + Buffer buffer = source.buffer(); + if (buffer.size() > MAX_RESPONSE_BODY_BYTES) { + throw new QdmpTransportException( + "qdmp response body exceeded the maximum allowed size of " + + MAX_RESPONSE_BODY_BYTES + + " bytes; refusing to buffer it into memory"); + } + MediaType contentType = body.contentType(); + Charset charset = + contentType == null ? StandardCharsets.UTF_8 : contentType.charset(StandardCharsets.UTF_8); + return buffer.readString(charset); } } diff --git a/java/src/main/java/io/github/echotechfe/qdmp/auth/Code2SessionResult.java b/java/src/main/java/io/github/echotechfe/qdmp/auth/Code2SessionResult.java new file mode 100644 index 0000000..a48dfd5 --- /dev/null +++ b/java/src/main/java/io/github/echotechfe/qdmp/auth/Code2SessionResult.java @@ -0,0 +1,98 @@ +package io.github.echotechfe.qdmp.auth; + +/** + * The full user session returned by {@link QdmpAuth#code2Session(String)}: access token, refresh + * token, expiry, and open ID. + * + *

Hand-written (not generated) so that {@link #toString()} can omit the actual token values -- + * unlike the openapi-generator DTOs, whose generated {@code toString()} prints every field verbatim + * and would otherwise write plaintext credentials into anything that logs this object (e.g. {@code + * logger.info("session={}", session)}). + */ +public final class Code2SessionResult { + + private final String accessToken; + private final String refreshToken; + private final long expiresAtEpochSeconds; + private final String openId; + + /** + * Creates a new session result. + * + * @param accessToken the user-level access token + * @param refreshToken the refresh token, exchangeable via {@link QdmpAuth#refreshToken(String)} + * @param expiresAtEpochSeconds the absolute expiry of {@code accessToken}, as Unix seconds (not + * milliseconds, and not a relative "seconds remaining" duration) + * @param openId the user's open ID, unique within this app + */ + public Code2SessionResult( + String accessToken, String refreshToken, long expiresAtEpochSeconds, String openId) { + this.accessToken = accessToken; + this.refreshToken = refreshToken; + this.expiresAtEpochSeconds = expiresAtEpochSeconds; + this.openId = openId; + } + + /** + * Returns the user-level access token. + * + * @return the access token + */ + public String getAccessToken() { + return accessToken; + } + + /** + * Returns the refresh token. + * + * @return the refresh token + */ + public String getRefreshToken() { + return refreshToken; + } + + /** + * Returns the absolute expiry of the access token, as Unix seconds. + * + * @return the expiry, in epoch seconds + */ + public long getExpiresAtEpochSeconds() { + return expiresAtEpochSeconds; + } + + /** + * Returns the user's open ID. + * + * @return the open ID + */ + public String getOpenId() { + return openId; + } + + @Override + public String toString() { + return "Code2SessionResult{accessToken=[REDACTED], refreshToken=[REDACTED]," + + " expiresAtEpochSeconds=" + + expiresAtEpochSeconds + + ", openId=" + + sanitizeForToString(openId) + + "}"; + } + + // openId is only validated as non-empty (see QdmpAuth.requireNonEmptyField) -- it can still + // contain server-controlled control characters (e.g. embedded CR/LF). This class exists + // specifically so logging it is safe, so toString() must never let openId forge extra log lines + // the way an unsanitized value could. getOpenId() intentionally still returns the raw value -- + // this only affects the debug rendering. + private static String sanitizeForToString(String value) { + if (value == null) { + return null; + } + StringBuilder sanitized = new StringBuilder(value.length()); + for (int i = 0; i < value.length(); i++) { + char c = value.charAt(i); + sanitized.append(c < 0x20 || c == 0x7f ? ' ' : c); + } + return sanitized.toString(); + } +} 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 465c141..d4a3e4d 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 @@ -89,31 +89,35 @@ public String getAccessToken() { request, AuthToken200ResponseAllOfData.class); CachedAppToken fresh = toCachedAppToken(data); + // A non-negative but already-past (or about-to-expire) expiresAt passes + // toCachedAppToken/parseExpiresAt's numeric validation, but a token that is already stale + // the moment it arrives must never be cached or handed back as if it were usable -- apply + // the exact same freshness bar used to decide whether a *cached* token still needs + // refreshing. + if (!hasSufficientTimeRemaining(fresh)) { + throw new QdmpTransportException( + "qdmp POST /auth/v1/token returned an access token that is already expired or too" + + " close to expiry (expiresAt=\"" + + data.getExpiresAt() + + "\"); refusing to cache or return it"); + } tokenStore.set(fresh); return fresh.getAccessToken(); } } // The server reported code:"0" (success), but the SDK still has to guard against a - // 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. + // structurally malformed body: a null/empty `accessToken` or a non-numeric/negative `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 or negative-lifetime + // access token. private static CachedAppToken toCachedAppToken(AuthToken200ResponseAllOfData data) { 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()); - } catch (NumberFormatException e) { - throw new QdmpTransportException( - "qdmp POST /auth/v1/token returned a non-numeric expiresAt: \"" - + data.getExpiresAt() - + "\"", - e); - } + long expiresAtEpochSeconds = parseExpiresAt(data.getExpiresAt(), "POST /auth/v1/token"); return new CachedAppToken(data.getAccessToken(), expiresAtEpochSeconds); } @@ -126,7 +130,7 @@ private static CachedAppToken toCachedAppToken(AuthToken200ResponseAllOfData dat * @return the full session: access token, refresh token, expiry, and open ID * @throws QdmpValidationError if {@code code} is {@code null} or empty */ - public AuthToken200ResponseAllOfData code2Session(String code) { + public Code2SessionResult code2Session(String code) { if (code == null || code.isEmpty()) { throw new QdmpValidationError( "code2Session: \"code\" is required for grantType=AUTHORIZATION_CODE and must be a" @@ -150,7 +154,12 @@ public AuthToken200ResponseAllOfData code2Session(String code) { data == null ? null : data.getAccessToken(), data == null ? null : data.getExpiresAt(), "POST /auth/v1/token"); - return data; + long expiresAtEpochSeconds = parseExpiresAt(data.getExpiresAt(), "POST /auth/v1/token"); + requireNonEmptyField(data.getRefreshToken(), "refreshToken", "POST /auth/v1/token"); + requireNonEmptyField(data.getOpenId(), "openId", "POST /auth/v1/token"); + requireNotAlreadyExpired(expiresAtEpochSeconds, "POST /auth/v1/token"); + return new Code2SessionResult( + data.getAccessToken(), data.getRefreshToken(), expiresAtEpochSeconds, data.getOpenId()); } /** @@ -161,7 +170,7 @@ public AuthToken200ResponseAllOfData code2Session(String code) { * @return the new access token and its expiry * @throws QdmpValidationError if {@code refreshToken} is {@code null} or empty */ - public AuthRefresh200ResponseAllOfData refreshToken(String refreshToken) { + public RefreshTokenResult refreshToken(String refreshToken) { if (refreshToken == null || refreshToken.isEmpty()) { throw new QdmpValidationError( "refreshToken: \"refreshToken\" is required and must be a non-empty string"); @@ -179,7 +188,9 @@ public AuthRefresh200ResponseAllOfData refreshToken(String refreshToken) { data == null ? null : data.getAccessToken(), data == null ? null : data.getExpiresAt(), "POST /auth/v1/refresh"); - return data; + long expiresAtEpochSeconds = parseExpiresAt(data.getExpiresAt(), "POST /auth/v1/refresh"); + requireNotAlreadyExpired(expiresAtEpochSeconds, "POST /auth/v1/refresh"); + return new RefreshTokenResult(data.getAccessToken(), expiresAtEpochSeconds); } // Shared guard for all three auth endpoints (CLIENT_CREDENTIALS/code2Session/refreshToken): the @@ -201,6 +212,60 @@ private static void requireNonMalformed( } } + // Shared numeric guard for all three auth endpoints: `expiresAt` must parse as a non-negative + // long. A non-numeric value would otherwise surface as a bare NumberFormatException, and a + // negative value -- while technically parseable -- describes an already-expired (or nonsensical) + // token and must never be cached/returned as if it were valid. + private static long parseExpiresAt(String expiresAt, String routeDescription) { + long parsed; + try { + parsed = Long.parseLong(expiresAt); + } catch (NumberFormatException e) { + throw new QdmpTransportException( + "qdmp " + routeDescription + " returned a non-numeric expiresAt: \"" + expiresAt + "\"", + e); + } + if (parsed < 0) { + throw new QdmpTransportException( + "qdmp " + routeDescription + " returned a negative expiresAt: \"" + expiresAt + "\""); + } + return parsed; + } + + // code2Session's Javadoc promises "the full session: access token, refresh token, expiry, and + // open ID" -- unlike accessToken/expiresAt (checked by requireNonMalformed), refreshToken/openId + // were never validated at all, so a response missing either field would silently hand back a + // session object with a null/empty refresh token or open ID. + private static void requireNonEmptyField( + String value, String fieldName, String routeDescription) { + if (value == null || value.isEmpty()) { + throw new QdmpTransportException( + "qdmp " + + routeDescription + + " reported code=\"0\" (success) but returned a malformed body (missing/empty " + + fieldName + + ")"); + } + } + + // code2Session/refreshToken are one-shot exchanges the SDK never caches (unlike the app-level + // CLIENT_CREDENTIALS path, guarded by hasSufficientTimeRemaining's stricter + // REFRESH_BUFFER_SECONDS + // buffer), so there is no cache-freshness policy to apply here -- but a non-negative expiresAt + // can + // still already be in the past (e.g. "1", 1970-01-01; parseExpiresAt only rejects negative/ + // non-numeric values). Handing back a session/token that is already dead on arrival would let a + // caller persist (or start relying on) a permanently-broken session. + private void requireNotAlreadyExpired(long expiresAtEpochSeconds, String routeDescription) { + if (expiresAtEpochSeconds <= clock.instant().getEpochSecond()) { + throw new QdmpTransportException( + "qdmp " + + routeDescription + + " returned an already-expired expiresAt: " + + expiresAtEpochSeconds); + } + } + private boolean hasSufficientTimeRemaining(CachedAppToken token) { long remaining = token.getExpiresAtEpochSeconds() - clock.instant().getEpochSecond(); return remaining > REFRESH_BUFFER_SECONDS; diff --git a/java/src/main/java/io/github/echotechfe/qdmp/auth/RefreshTokenResult.java b/java/src/main/java/io/github/echotechfe/qdmp/auth/RefreshTokenResult.java new file mode 100644 index 0000000..9aaed0a --- /dev/null +++ b/java/src/main/java/io/github/echotechfe/qdmp/auth/RefreshTokenResult.java @@ -0,0 +1,52 @@ +package io.github.echotechfe.qdmp.auth; + +/** + * The new access token returned by {@link QdmpAuth#refreshToken(String)}. + * + *

Hand-written (not generated) so that {@link #toString()} can omit the actual token value -- + * unlike the openapi-generator DTOs, whose generated {@code toString()} prints every field verbatim + * and would otherwise write plaintext credentials into anything that logs this object (e.g. {@code + * logger.info("result={}", result)}). + */ +public final class RefreshTokenResult { + + private final String accessToken; + private final long expiresAtEpochSeconds; + + /** + * Creates a new refresh result. + * + * @param accessToken the new user-level access token + * @param expiresAtEpochSeconds the absolute expiry of {@code accessToken}, as Unix seconds (not + * milliseconds, and not a relative "seconds remaining" duration) + */ + public RefreshTokenResult(String accessToken, long expiresAtEpochSeconds) { + this.accessToken = accessToken; + this.expiresAtEpochSeconds = expiresAtEpochSeconds; + } + + /** + * Returns the new user-level access token. + * + * @return the access token + */ + public String getAccessToken() { + return accessToken; + } + + /** + * Returns the absolute expiry of the access token, as Unix seconds. + * + * @return the expiry, in epoch seconds + */ + public long getExpiresAtEpochSeconds() { + return expiresAtEpochSeconds; + } + + @Override + public String toString() { + return "RefreshTokenResult{accessToken=[REDACTED], expiresAtEpochSeconds=" + + expiresAtEpochSeconds + + "}"; + } +} diff --git a/java/src/main/java/io/github/echotechfe/qdmp/errors/QdmpApiError.java b/java/src/main/java/io/github/echotechfe/qdmp/errors/QdmpApiError.java index 5157170..4a31be9 100644 --- a/java/src/main/java/io/github/echotechfe/qdmp/errors/QdmpApiError.java +++ b/java/src/main/java/io/github/echotechfe/qdmp/errors/QdmpApiError.java @@ -17,6 +17,11 @@ public final class QdmpApiError extends RuntimeException { private static final long serialVersionUID = 1L; + // Bounds how much of a server-controlled string (message/requestId) this SDK will hold onto and + // echo back via toString()/getMessage() -- a malicious or buggy upstream could otherwise return + // hundreds of KB in a field that ends up written to logs in full. + private static final int MAX_SANITIZED_FIELD_LENGTH = 2000; + private final String code; private final String requestId; private final int httpStatus; @@ -32,12 +37,36 @@ public final class QdmpApiError extends RuntimeException { * -- see class Javadoc) */ public QdmpApiError(String code, String message, String requestId, int httpStatus) { - super(message); - this.code = code; - this.requestId = requestId; + super(sanitize(message)); + this.code = sanitize(code); + this.requestId = sanitize(requestId); this.httpStatus = httpStatus; } + // code/message/requestId are taken verbatim from the server's response body -- a malicious or + // man-in-the-middle response containing e.g. raw \r\n could otherwise inject fake log lines into + // anything that logs this exception via toString()/getMessage(). code is an intentionally open + // string (see class Javadoc), not a closed enum, so it is just as attacker-influenced as + // message/requestId and must go through the exact same sanitize() gate. Stripping ASCII control + // characters (< 0x20) and DEL (0x7f), and bounding the length, applies to all three fields before + // they are ever stored, so every accessor (getCode(), getMessage(), the requestId field, + // toString()) sees the sanitized value. + private static String sanitize(String value) { + if (value == null) { + return null; + } + String truncated = + value.length() > MAX_SANITIZED_FIELD_LENGTH + ? value.substring(0, MAX_SANITIZED_FIELD_LENGTH) + : value; + StringBuilder sanitized = new StringBuilder(truncated.length()); + for (int i = 0; i < truncated.length(); i++) { + char c = truncated.charAt(i); + sanitized.append(c < 0x20 || c == 0x7f ? ' ' : c); + } + return sanitized.toString(); + } + /** * Returns the business status code reported by the server. * diff --git a/java/src/test/java/io/github/echotechfe/qdmp/QdmpClientConfigBaseUrlSchemeTest.java b/java/src/test/java/io/github/echotechfe/qdmp/QdmpClientConfigBaseUrlSchemeTest.java new file mode 100644 index 0000000..73bd7dd --- /dev/null +++ b/java/src/test/java/io/github/echotechfe/qdmp/QdmpClientConfigBaseUrlSchemeTest.java @@ -0,0 +1,90 @@ +package io.github.echotechfe.qdmp; + +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import org.junit.jupiter.api.Test; + +/** + * {@link QdmpClientConfig.Builder#baseUrl(String)} currently accepts any syntactically valid URL + * without inspecting its scheme -- {@code http://} and {@code https://} are treated identically. + * Since every business/auth call carries the app secret and/or an access token as a request header, + * configuring an {@code http://} base URL (attacker-controlled or just misconfigured) would send + * those credentials in plaintext over the network. + * + *

The required fix: {@link QdmpClientConfig.Builder#build()} must reject a base URL whose scheme + * is {@code http} unless the host is loopback/localhost (127.0.0.1, "localhost" case-insensitively, + * or any {@code 127.*} address) -- the loopback exception exists because {@link + * okhttp3.mockwebserver.MockWebServer}, used throughout this test suite, always listens on {@code + * http://127.0.0.1:PORT}. + */ +class QdmpClientConfigBaseUrlSchemeTest { + + @Test + void build_withHttpNonLoopbackBaseUrl_throwsIllegalArgumentException() { + assertThatThrownBy( + () -> + QdmpClientConfig.builder() + .appId("a") + .appSecret("s") + .baseUrl("http://evil.example.com") + .build()) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + void build_withHttpLoopbackIpBaseUrl_doesNotThrow() { + assertThatCode( + () -> + QdmpClientConfig.builder() + .appId("a") + .appSecret("s") + .baseUrl("http://127.0.0.1:12345") + .build()) + .doesNotThrowAnyException(); + } + + @Test + void build_withHttpLocalhostBaseUrl_doesNotThrow() { + assertThatCode( + () -> + QdmpClientConfig.builder() + .appId("a") + .appSecret("s") + .baseUrl("http://localhost:12345") + .build()) + .doesNotThrowAnyException(); + } + + @Test + void build_withHttpsBaseUrl_doesNotThrow() { + assertThatCode( + () -> + QdmpClientConfig.builder() + .appId("a") + .appSecret("s") + .baseUrl("https://example.com") + .build()) + .doesNotThrowAnyException(); + } + + /** + * {@code isLoopbackOrLocalhost} currently checks {@code lowerHost.startsWith("127.")}, a bare + * string-prefix test rather than validating that the host is a genuine dotted-decimal IPv4 + * literal. {@code 127.attacker.com} is a syntactically valid hostname -- not an IP address at all + * -- that DNS could resolve to any attacker-controlled server, yet it passes this check today and + * is treated as "loopback", letting an {@code http://} base URL past the HTTPS requirement and + * sending app secret/access token headers in plaintext to that attacker host. + */ + @Test + void build_withHttp127PrefixedNonIpHostname_throwsIllegalArgumentException() { + assertThatThrownBy( + () -> + QdmpClientConfig.builder() + .appId("a") + .appSecret("s") + .baseUrl("http://127.attacker.com") + .build()) + .isInstanceOf(IllegalArgumentException.class); + } +} diff --git a/java/src/test/java/io/github/echotechfe/qdmp/QdmpContextWhitelistValidationTest.java b/java/src/test/java/io/github/echotechfe/qdmp/QdmpContextWhitelistValidationTest.java new file mode 100644 index 0000000..78f6802 --- /dev/null +++ b/java/src/test/java/io/github/echotechfe/qdmp/QdmpContextWhitelistValidationTest.java @@ -0,0 +1,55 @@ +package io.github.echotechfe.qdmp; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import org.junit.jupiter.api.Test; + +/** + * {@code QdmpContext.of(accessToken)}'s current {@code requireHeaderSafe} check is a blacklist: it + * only rejects control characters ({@code <= 0x1F}, {@code == 0x7F}) and anything above {@code + * 0xFF}, which means it lets through both {@code 0x09} (tab) and the entire Latin-1 Supplement + * range ({@code 0x80-0xFF}, e.g. {@code é}, U+00E9). + * + *

That matters because OkHttp 4.12's own {@code Headers} validation is stricter than this SDK's + * check (it only accepts tab or {@code 0x20-0x7E}), and for header names outside its hardcoded + * redaction list (Authorization/Cookie/Proxy-Authorization/Set-Cookie -- neither {@code + * access-token} nor {@code x-openapi-access-token} is on it), OkHttp echoes the *entire* invalid + * header value into its thrown {@link IllegalArgumentException} message. A token containing e.g. + * {@code é} or a tab currently sails past {@link QdmpContext#of}, then gets rejected by OkHttp with + * the full token value verbatim in the exception message -- a credential leak into logs/traces. + * + *

The required fix is a whitelist: only printable ASCII ({@code 0x20-0x7E}) may pass. This test + * class documents that requirement directly against {@link QdmpContext#of}. + */ +class QdmpContextWhitelistValidationTest { + + @Test + void of_withLatin1SupplementCharacter_throwsIllegalArgumentException_withoutLeakingToken() { + String tokenWithLatin1Supplement = "prefixéSECRET"; + + assertThatThrownBy(() -> QdmpContext.of(tokenWithLatin1Supplement)) + .isInstanceOf(IllegalArgumentException.class) + .satisfies( + e -> { + assertThat(e.getMessage()).doesNotContain("SECRET"); + assertThat(e.getMessage()).doesNotContain("é"); + }); + } + + @Test + void of_withEmbeddedTab_throwsIllegalArgumentException_withoutLeakingToken() { + String tokenWithTab = "prefix\tSECRET"; + + assertThatThrownBy(() -> QdmpContext.of(tokenWithTab)) + .isInstanceOf(IllegalArgumentException.class) + .satisfies(e -> assertThat(e.getMessage()).doesNotContain("SECRET")); + } + + @Test + void of_withPlainAsciiToken_isAccepted() { + QdmpContext ctx = QdmpContext.of("app-token-abc-123"); + + assertThat(ctx.getAccessToken()).isEqualTo("app-token-abc-123"); + } +} diff --git a/java/src/test/java/io/github/echotechfe/qdmp/QdmpTransportRawTokenValidationTest.java b/java/src/test/java/io/github/echotechfe/qdmp/QdmpTransportRawTokenValidationTest.java new file mode 100644 index 0000000..082ce6c --- /dev/null +++ b/java/src/test/java/io/github/echotechfe/qdmp/QdmpTransportRawTokenValidationTest.java @@ -0,0 +1,104 @@ +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.generated.AuthToken200ResponseAllOfData; +import java.io.IOException; +import okhttp3.OkHttpClient; +import okhttp3.mockwebserver.MockWebServer; +import org.assertj.core.api.Assertions; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * {@link QdmpTransport} is a {@code public final class} with a public constructor and public {@code + * get}/{@code post} methods that accept a bare {@code String accessToken} straight from the caller + * -- {@link QdmpContext}'s header-safety validation ({@link QdmpContext#of(String)}/{@code + * requireHeaderSafe}) is never consulted on this path. A caller that bypasses {@link QdmpContext} + * and constructs {@link QdmpTransport} directly (it is public only because business groups and auth + * live in separate packages, not because direct use is supported/safe) can therefore push a token + * containing arbitrary characters -- including raw control characters -- straight into OkHttp's + * header machinery. + * + *

Empirically confirmed against okhttp 4.12.0: {@code new + * Request.Builder().header("access-token", "raw\nSECRET-TOKEN-DO-NOT-LEAK")} throws {@code + * IllegalArgumentException: Unexpected char 0x0a at 3 in access-token value: raw\nSECRET-TOKEN...}, + * i.e. OkHttp's own header validation embeds the *entire* raw (invalid) header value -- including + * the secret token -- into its exception message for header names outside its hardcoded redaction + * list (Authorization/Cookie/Proxy-Authorization/Set-Cookie; neither {@code access-token} nor + * {@code x-openapi-access-token} is on it). So merely failing before the request is sent is not + * enough: bypassing {@link QdmpContext} and calling {@link QdmpTransport} directly with an unsafe + * token today still throws, but with the token leaked into the exception message. {@link + * QdmpTransport} must therefore perform the same header-safety validation {@link QdmpContext#of} + * does, with the same token-free exception message, rather than letting OkHttp's own validation + * (and its token-echoing message) be reached at all. + */ +class QdmpTransportRawTokenValidationTest { + + private static final String SENTINEL = "SECRET-TOKEN-DO-NOT-LEAK"; + + private MockWebServer server; + + @BeforeEach + void startServer() throws IOException { + server = new MockWebServer(); + server.start(); + } + + @AfterEach + void stopServer() throws IOException { + server.shutdown(); + } + + @Test + void get_withRawControlCharacterToken_failsBeforeAnyRequestIsSent_withoutLeakingToken() { + QdmpTransport transport = + new QdmpTransport(new OkHttpClient(), server.url("/").toString(), "1.0", "app-id"); + String unsafeToken = "raw\n" + SENTINEL; + + Throwable thrown = + Assertions.catchThrowable( + () -> + transport.get( + "/user/v1/me", + AuthScheme.STANDARD, + unsafeToken, + null, + AuthToken200ResponseAllOfData.class)); + + assertThat(thrown) + .as("an unsafe raw token bypassing QdmpContext must still be rejected") + .isInstanceOf(RuntimeException.class); + assertThat(server.getRequestCount()) + .as("an unsafe raw token must be rejected locally before any HTTP request is attempted") + .isEqualTo(0); + assertThat(String.valueOf(thrown.getMessage())) + .as( + "the exception message must never leak the raw token -- OkHttp's own header" + + " validation would otherwise echo it verbatim for a header name like" + + " access-token that isn't on OkHttp's redaction list") + .doesNotContain(SENTINEL); + } + + /** + * {@link QdmpTransport}'s public constructor only rejects a base URL that {@link + * okhttp3.HttpUrl#parse} cannot parse at all -- it never checks the scheme. {@link + * QdmpClientConfig.Builder#build()} enforces https:// (except for loopback/localhost) precisely + * because every business/auth call carries the app secret and/or an access token as a request + * header, but a caller that bypasses {@link QdmpClient}/{@link QdmpClientConfig} and constructs + * {@link QdmpTransport} directly (it is public only because groups/auth live in separate + * packages, not because direct use is supported/safe) can hand it a plain {@code http://} base + * URL, and every subsequent {@code get}/{@code post} call would then send those credentials in + * plaintext. {@link QdmpTransport}'s constructor must therefore enforce the same + * https-or-loopback rule {@link QdmpClientConfig.Builder#build()} does, rather than relying + * entirely on {@link QdmpClientConfig} being the only path that ever constructs it. + */ + @Test + void constructor_withHttpNonLoopbackBaseUrl_throwsIllegalArgumentException() { + assertThatThrownBy( + () -> new QdmpTransport(new OkHttpClient(), "http://evil.example.com", "1.0", "app-id")) + .isInstanceOf(IllegalArgumentException.class); + } +} diff --git a/java/src/test/java/io/github/echotechfe/qdmp/QdmpTransportResponseSizeLimitTest.java b/java/src/test/java/io/github/echotechfe/qdmp/QdmpTransportResponseSizeLimitTest.java new file mode 100644 index 0000000..4966a03 --- /dev/null +++ b/java/src/test/java/io/github/echotechfe/qdmp/QdmpTransportResponseSizeLimitTest.java @@ -0,0 +1,54 @@ +package io.github.echotechfe.qdmp; + +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +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; + +/** + * {@code QdmpTransport#readBody} currently calls {@code body.string()} with no size limit at all -- + * a malicious or malfunctioning endpoint can return an arbitrarily large response body and the SDK + * will buffer the entire thing into memory before even attempting to parse it. This test drives a + * real {@code code2Session} call against a response whose body is otherwise perfectly well-formed + * (valid JSON, valid accessToken/expiresAt/refreshToken/openId) except for one padding field that + * balloons it to roughly 11 MB, to prove the requirement is about size, not malformed content: + * today this call succeeds (no size cap exists), which is exactly the gap this test should catch + * until a response-size limit is enforced and the oversized response is rejected instead of parsed. + */ +class QdmpTransportResponseSizeLimitTest { + + private MockWebServer server; + + @BeforeEach + void startServer() throws IOException { + server = new MockWebServer(); + server.start(); + } + + @AfterEach + void stopServer() throws IOException { + server.shutdown(); + } + + @Test + void oversizedResponseBody_isRejected_ratherThanFullyBufferedAndParsed() { + String padding = "a".repeat(11 * 1024 * 1024); + String body = + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{\"accessToken\":\"acc\"," + + "\"expiresAt\":\"1900000000\",\"refreshToken\":\"ref\",\"openId\":\"open-1\"," + + "\"padding\":\"" + + padding + + "\"}}"; + server.enqueue(new MockResponse().setResponseCode(200).setBody(body)); + QdmpClient client = TestClients.create(server); + + assertThatThrownBy(() -> client.auth().code2Session("some-code")) + .as("an ~11MB response body must be rejected, not fully buffered into memory and parsed") + .isInstanceOf(RuntimeException.class); + } +} diff --git a/java/src/test/java/io/github/echotechfe/qdmp/auth/QdmpAuthCode2SessionFieldValidationTest.java b/java/src/test/java/io/github/echotechfe/qdmp/auth/QdmpAuthCode2SessionFieldValidationTest.java new file mode 100644 index 0000000..8c9daa6 --- /dev/null +++ b/java/src/test/java/io/github/echotechfe/qdmp/auth/QdmpAuthCode2SessionFieldValidationTest.java @@ -0,0 +1,94 @@ +package io.github.echotechfe.qdmp.auth; + +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import io.github.echotechfe.qdmp.QdmpClient; +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 QdmpAuth#code2Session}'s Javadoc promises "the full session: access token, refresh token, + * expiry, and open ID", but the current implementation's malformed-body guard ({@code + * requireNonMalformed}) only checks {@code accessToken}/{@code expiresAt} -- it never looks at + * {@code data.refreshToken} or {@code data.openId} at all. A response missing either field is + * currently handed back to the caller as a session object with a silently-{@code null} (or empty) + * refresh token / open ID, contradicting the documented contract. Both fields must be validated the + * same way {@code accessToken}/{@code expiresAt} already are: missing or empty must throw. + */ +class QdmpAuthCode2SessionFieldValidationTest { + + private MockWebServer server; + + @BeforeEach + void startServer() throws IOException { + server = new MockWebServer(); + server.start(); + } + + @AfterEach + void stopServer() throws IOException { + server.shutdown(); + } + + @Test + void code2Session_missingRefreshToken_throws() { + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{\"accessToken\":\"acc\"," + + "\"expiresAt\":\"1900000000\",\"openId\":\"open-1\"}}")); + QdmpClient client = TestClients.create(server); + + assertThatThrownBy(() -> client.auth().code2Session("some-code")) + .isInstanceOf(RuntimeException.class); + } + + @Test + void code2Session_emptyRefreshToken_throws() { + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{\"accessToken\":\"acc\"," + + "\"expiresAt\":\"1900000000\",\"refreshToken\":\"\"," + + "\"openId\":\"open-1\"}}")); + QdmpClient client = TestClients.create(server); + + assertThatThrownBy(() -> client.auth().code2Session("some-code")) + .isInstanceOf(RuntimeException.class); + } + + @Test + void code2Session_missingOpenId_throws() { + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{\"accessToken\":\"acc\"," + + "\"expiresAt\":\"1900000000\",\"refreshToken\":\"ref\"}}")); + QdmpClient client = TestClients.create(server); + + assertThatThrownBy(() -> client.auth().code2Session("some-code")) + .isInstanceOf(RuntimeException.class); + } + + @Test + void code2Session_emptyOpenId_throws() { + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{\"accessToken\":\"acc\"," + + "\"expiresAt\":\"1900000000\",\"refreshToken\":\"ref\",\"openId\":\"\"}}")); + QdmpClient client = TestClients.create(server); + + assertThatThrownBy(() -> client.auth().code2Session("some-code")) + .isInstanceOf(RuntimeException.class); + } +} 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 b0a671e..9869652 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 @@ -9,8 +9,6 @@ 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; import io.github.echotechfe.qdmp.testsupport.TestClients; import java.io.IOException; import okhttp3.mockwebserver.MockResponse; @@ -54,10 +52,10 @@ void code2Session_success_sendsAuthorizationCodeGrant_andReturnsFullSession() th + "\"refreshToken\":\"user-refresh-token\",\"openId\":\"open-id-1\"}}")); QdmpClient client = TestClients.create(server); - AuthToken200ResponseAllOfData session = client.auth().code2Session("wx-login-code-abc"); + Code2SessionResult session = client.auth().code2Session("wx-login-code-abc"); assertThat(session.getAccessToken()).isEqualTo("user-access-token"); - assertThat(session.getExpiresAt()).isEqualTo("1900000000"); + assertThat(session.getExpiresAtEpochSeconds()).isEqualTo(1900000000L); assertThat(session.getRefreshToken()).isEqualTo("user-refresh-token"); assertThat(session.getOpenId()).isEqualTo("open-id-1"); RecordedRequest request = server.takeRequest(); @@ -84,8 +82,8 @@ void code2Session_isNeverCached_repeatedCallsAlwaysHitTheNetwork() throws Except + "\"openId\":\"o-2\"}}")); QdmpClient client = TestClients.create(server); - AuthToken200ResponseAllOfData first = client.auth().code2Session("code-1"); - AuthToken200ResponseAllOfData second = client.auth().code2Session("code-2"); + Code2SessionResult first = client.auth().code2Session("code-1"); + Code2SessionResult second = client.auth().code2Session("code-2"); assertThat(first.getAccessToken()).isEqualTo("tok-1"); assertThat(second.getAccessToken()).isEqualTo("tok-2"); @@ -113,7 +111,7 @@ void refreshToken_success_returnsNewAccessToken() throws Exception { + "\"expiresAt\":\"1900000000\"}}")); QdmpClient client = TestClients.create(server); - AuthRefresh200ResponseAllOfData result = client.auth().refreshToken("some-refresh-token"); + RefreshTokenResult result = client.auth().refreshToken("some-refresh-token"); assertThat(result.getAccessToken()).isEqualTo("new-token"); RecordedRequest request = server.takeRequest(); diff --git a/java/src/test/java/io/github/echotechfe/qdmp/auth/QdmpAuthExpiresAtValidationTest.java b/java/src/test/java/io/github/echotechfe/qdmp/auth/QdmpAuthExpiresAtValidationTest.java new file mode 100644 index 0000000..c0f3794 --- /dev/null +++ b/java/src/test/java/io/github/echotechfe/qdmp/auth/QdmpAuthExpiresAtValidationTest.java @@ -0,0 +1,151 @@ +package io.github.echotechfe.qdmp.auth; + +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import io.github.echotechfe.qdmp.QdmpClient; +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; + +/** + * {@code code2Session}/{@code refreshToken} currently validate {@code data.expiresAt} only via + * {@code requireNonMalformed}, which checks for {@code null}/empty -- it never parses the string as + * a number. That is inconsistent with the CLIENT_CREDENTIALS caching path ({@code + * toCachedAppToken}), which does call {@code Long.parseLong(data.getExpiresAt())} and turns a + * {@link NumberFormatException} into a typed {@link + * io.github.echotechfe.qdmp.errors.QdmpTransportException}. A non-numeric or negative {@code + * expiresAt} must be rejected on every one of the three auth response paths, not just + * CLIENT_CREDENTIALS -- and {@code toCachedAppToken}'s existing {@code Long.parseLong} call must + * also be tightened to reject negative values, which it currently accepts silently (an existing + * bug, not just a gap in the new code2Session/refreshToken paths). + */ +class QdmpAuthExpiresAtValidationTest { + + private MockWebServer server; + + @BeforeEach + void startServer() throws IOException { + server = new MockWebServer(); + server.start(); + } + + @AfterEach + void stopServer() throws IOException { + server.shutdown(); + } + + @Test + void code2Session_nonNumericExpiresAt_throws() { + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{\"accessToken\":\"acc\"," + + "\"expiresAt\":\"garbage\",\"refreshToken\":\"ref\"," + + "\"openId\":\"open-1\"}}")); + QdmpClient client = TestClients.create(server); + + assertThatThrownBy(() -> client.auth().code2Session("some-code")) + .isInstanceOf(RuntimeException.class); + } + + @Test + void code2Session_negativeExpiresAt_throws() { + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{\"accessToken\":\"acc\"," + + "\"expiresAt\":\"-100\",\"refreshToken\":\"ref\",\"openId\":\"open-1\"}}")); + QdmpClient client = TestClients.create(server); + + assertThatThrownBy(() -> client.auth().code2Session("some-code")) + .isInstanceOf(RuntimeException.class); + } + + @Test + void refreshToken_nonNumericExpiresAt_throws() { + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{\"accessToken\":\"acc\"," + + "\"expiresAt\":\"garbage\"}}")); + QdmpClient client = TestClients.create(server); + + assertThatThrownBy(() -> client.auth().refreshToken("some-refresh-token")) + .isInstanceOf(RuntimeException.class); + } + + @Test + void refreshToken_negativeExpiresAt_throws() { + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{\"accessToken\":\"acc\"," + + "\"expiresAt\":\"-100\"}}")); + QdmpClient client = TestClients.create(server); + + assertThatThrownBy(() -> client.auth().refreshToken("some-refresh-token")) + .isInstanceOf(RuntimeException.class); + } + + /** + * Regression test (codex convergence-check finding): a non-negative but already-expired {@code + * expiresAt} (e.g. {@code "1"}, 1970-01-01) passes {@code parseExpiresAt} (which only rejects + * negative/non-numeric values) and must still be rejected by {@code requireNotAlreadyExpired} -- + * otherwise a caller could persist or start relying on a session/token that is already dead on + * arrival. + */ + @Test + void code2Session_alreadyExpiredExpiresAt_throws() { + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{\"accessToken\":\"acc\"," + + "\"expiresAt\":\"1\",\"refreshToken\":\"ref\",\"openId\":\"open-1\"}}")); + QdmpClient client = TestClients.create(server); + + assertThatThrownBy(() -> client.auth().code2Session("some-code")) + .isInstanceOf(RuntimeException.class); + } + + @Test + void refreshToken_alreadyExpiredExpiresAt_throws() { + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{\"accessToken\":\"acc\"," + + "\"expiresAt\":\"1\"}}")); + QdmpClient client = TestClients.create(server); + + assertThatThrownBy(() -> client.auth().refreshToken("some-refresh-token")) + .isInstanceOf(RuntimeException.class); + } + + /** + * Existing bug in the CLIENT_CREDENTIALS caching path: {@code toCachedAppToken} calls {@code + * Long.parseLong(data.getExpiresAt())}, which happily accepts a negative number instead of + * rejecting it -- so a malformed/negative expiry currently gets cached and returned to the caller + * as if it were valid. + */ + @Test + void getAccessToken_negativeExpiresAt_throws() { + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{\"accessToken\":\"app-token\"," + + "\"expiresAt\":\"-100\"}}")); + QdmpClient client = TestClients.create(server); + + assertThatThrownBy(() -> client.auth().getAccessToken()).isInstanceOf(RuntimeException.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 f830b25..c7a521f 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 @@ -295,6 +295,31 @@ void successCode_withNonNumericExpiresAt_throwsQdmpTransportException_notBareNfe .isNotInstanceOf(NumberFormatException.class); } + /** + * A fresh {@code POST /auth/v1/token} response with code:"0" (success) but an {@code expiresAt} + * that is already in the past (here, 1 second past the Unix epoch -- non-negative, so {@link + * QdmpAuth}'s existing negative-expiresAt guard does not catch it) must not be cached/returned as + * if it were a usable token: with {@code REFRESH_BUFFER_SECONDS=300}, any real clock reports zero + * (in fact deeply negative) time remaining before expiry. The freshly-fetched token is never + * checked against that same freshness requirement before being cached, so today it is persisted + * into the {@link TokenStore} and handed back to the caller as if valid. + */ + @Test + void freshResponse_withAlreadyExpiredExpiresAt_isRejected_andNeverCached() throws Exception { + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":" + + "{\"accessToken\":\"stale-token\",\"expiresAt\":\"1\"}}")); + RecordingTokenStore tokenStore = new RecordingTokenStore(); + QdmpClient client = TestClients.create(server, tokenStore); + + assertThatThrownBy(() -> client.auth().getAccessToken()) + .isInstanceOf(QdmpTransportException.class); + assertThat(tokenStore.getSetCallCount()).isZero(); + } + @Test void failure_doesNotLeakAppSecretInExceptionMessageOrToString() { String secretAppSecret = "S3cr3t-App-Secret-Must-Not-Leak"; diff --git a/java/src/test/java/io/github/echotechfe/qdmp/auth/QdmpAuthResultTypesTest.java b/java/src/test/java/io/github/echotechfe/qdmp/auth/QdmpAuthResultTypesTest.java new file mode 100644 index 0000000..ef5c1a4 --- /dev/null +++ b/java/src/test/java/io/github/echotechfe/qdmp/auth/QdmpAuthResultTypesTest.java @@ -0,0 +1,147 @@ +package io.github.echotechfe.qdmp.auth; + +import static org.assertj.core.api.Assertions.assertThat; + +import io.github.echotechfe.qdmp.QdmpClient; +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; + +/** + * {@code code2Session}/{@code refreshToken} currently return the raw openapi-generator DTOs {@code + * AuthToken200ResponseAllOfData}/{@code AuthRefresh200ResponseAllOfData}. Those generated classes' + * {@code toString()} is produced by the openapi-generator template and prints every field verbatim, + * including {@code accessToken}/{@code refreshToken} -- so a caller doing the extremely common + * {@code logger.info("session={}", session)} would write the plaintext token straight into logs. + * Generated files can't be hand-edited (they get overwritten by the next {@code ./gradlew codegen} + * run), so the fix is to introduce hand-written result types, matching the naming already used by + * the Go SDK: {@link Code2SessionResult} (accessToken/refreshToken/expiresAtEpochSeconds/openId) + * and {@link RefreshTokenResult} (accessToken/expiresAtEpochSeconds), neither of which may print + * the actual token values from {@code toString()}. + */ +class QdmpAuthResultTypesTest { + + private static final String SENTINEL_ACCESS_TOKEN = "sentinel-access-token-xyz"; + private static final String SENTINEL_REFRESH_TOKEN = "sentinel-refresh-token-abc"; + + private MockWebServer server; + + @BeforeEach + void startServer() throws IOException { + server = new MockWebServer(); + server.start(); + } + + @AfterEach + void stopServer() throws IOException { + server.shutdown(); + } + + @Test + void code2Session_returnsCode2SessionResult_withMatchingGetters() { + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{\"accessToken\":\"" + + SENTINEL_ACCESS_TOKEN + + "\",\"expiresAt\":\"1900000000\",\"refreshToken\":\"" + + SENTINEL_REFRESH_TOKEN + + "\",\"openId\":\"open-id-1\"}}")); + QdmpClient client = TestClients.create(server); + + Code2SessionResult session = client.auth().code2Session("some-code"); + + assertThat(session.getAccessToken()).isEqualTo(SENTINEL_ACCESS_TOKEN); + assertThat(session.getRefreshToken()).isEqualTo(SENTINEL_REFRESH_TOKEN); + assertThat(session.getExpiresAtEpochSeconds()).isEqualTo(1900000000L); + assertThat(session.getOpenId()).isEqualTo("open-id-1"); + } + + @Test + void code2Session_toString_doesNotLeakAccessTokenOrRefreshToken() { + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{\"accessToken\":\"" + + SENTINEL_ACCESS_TOKEN + + "\",\"expiresAt\":\"1900000000\",\"refreshToken\":\"" + + SENTINEL_REFRESH_TOKEN + + "\",\"openId\":\"open-id-1\"}}")); + QdmpClient client = TestClients.create(server); + + Code2SessionResult session = client.auth().code2Session("some-code"); + + assertThat(session.toString()).doesNotContain(SENTINEL_ACCESS_TOKEN); + assertThat(session.toString()).doesNotContain(SENTINEL_REFRESH_TOKEN); + } + + /** + * Regression test (codex convergence-check finding): {@code openId} is only validated as + * non-empty (see {@code QdmpAuth.requireNonEmptyField}), so a server-controlled value containing + * embedded CR/LF would, without this fix, survive verbatim into {@code toString()} and forge + * extra log lines via {@code logger.info("session={}", session)} -- the exact log-injection class + * of bug already fixed for message/code/requestId elsewhere. {@code getOpenId()} must still + * return the raw value unchanged; only the debug rendering is sanitized. + */ + @Test + void code2Session_toString_doesNotLeakRawCrLfInOpenId() { + String injectedOpenId = "open-id-1\\r\\nFAKE injected log line"; + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{\"accessToken\":\"" + + SENTINEL_ACCESS_TOKEN + + "\",\"expiresAt\":\"1900000000\",\"refreshToken\":\"" + + SENTINEL_REFRESH_TOKEN + + "\",\"openId\":\"" + + injectedOpenId + + "\"}}")); + QdmpClient client = TestClients.create(server); + + Code2SessionResult session = client.auth().code2Session("some-code"); + + assertThat(session.toString()).doesNotContain("\r").doesNotContain("\n"); + // Reverse-direction guard: getOpenId() must still return the raw value untouched. + assertThat(session.getOpenId()).isEqualTo("open-id-1\r\nFAKE injected log line"); + } + + @Test + void refreshToken_returnsRefreshTokenResult_withMatchingGetters() { + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{\"accessToken\":\"" + + SENTINEL_ACCESS_TOKEN + + "\",\"expiresAt\":\"1900000000\"}}")); + QdmpClient client = TestClients.create(server); + + RefreshTokenResult result = client.auth().refreshToken("some-refresh-token"); + + assertThat(result.getAccessToken()).isEqualTo(SENTINEL_ACCESS_TOKEN); + assertThat(result.getExpiresAtEpochSeconds()).isEqualTo(1900000000L); + } + + @Test + void refreshToken_toString_doesNotLeakAccessToken() { + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":\"0\",\"message\":\"ok\",\"data\":{\"accessToken\":\"" + + SENTINEL_ACCESS_TOKEN + + "\",\"expiresAt\":\"1900000000\"}}")); + QdmpClient client = TestClients.create(server); + + RefreshTokenResult result = client.auth().refreshToken("some-refresh-token"); + + assertThat(result.toString()).doesNotContain(SENTINEL_ACCESS_TOKEN); + } +} diff --git a/java/src/test/java/io/github/echotechfe/qdmp/errors/QdmpApiErrorSanitizationTest.java b/java/src/test/java/io/github/echotechfe/qdmp/errors/QdmpApiErrorSanitizationTest.java new file mode 100644 index 0000000..c7061a2 --- /dev/null +++ b/java/src/test/java/io/github/echotechfe/qdmp/errors/QdmpApiErrorSanitizationTest.java @@ -0,0 +1,93 @@ +package io.github.echotechfe.qdmp.errors; + +import static org.assertj.core.api.Assertions.assertThat; + +import io.github.echotechfe.qdmp.QdmpClient; +import io.github.echotechfe.qdmp.testsupport.TestClients; +import java.io.IOException; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import org.assertj.core.api.Assertions; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * {@link QdmpApiError#toString()} currently interpolates the server-reported {@code message} (and + * {@code requestId}) verbatim, with no escaping or length bound. A server (or a man-in-the-middle, + * or simply a buggy upstream) that returns a {@code message} containing raw {@code \r\n} can inject + * fake log lines into anything that logs the exception via {@code toString()}/{@code getMessage()}, + * and an unbounded-length {@code message} (e.g. hundreds of KB) would be written to logs in full. + * Both the control-character injection and the length must be sanitized/bounded before they reach + * {@code toString()}/{@code getMessage()}. + */ +class QdmpApiErrorSanitizationTest { + + private MockWebServer server; + + @BeforeEach + void startServer() throws IOException { + server = new MockWebServer(); + server.start(); + } + + @AfterEach + void stopServer() throws IOException { + server.shutdown(); + } + + @Test + void message_withEmbeddedCrLf_isSanitized_inToStringAndGetMessage() { + String injectedMessage = "line1\r\nFAKE injected log line"; + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":10002,\"message\":\"" + + injectedMessage.replace("\r", "\\r").replace("\n", "\\n") + + "\",\"requestId\":\"req-1\"}")); + QdmpClient client = TestClients.create(server); + + QdmpApiError error = + (QdmpApiError) Assertions.catchThrowable(() -> client.auth().getAccessToken()); + + assertThat(error).isNotNull(); + assertThat(error.toString()).doesNotContain("\r").doesNotContain("\n"); + assertThat(String.valueOf(error.getMessage())).doesNotContain("\r").doesNotContain("\n"); + } + + @Test + void veryLongMessage_isBounded_inToString() { + String hugeMessage = "x".repeat(200_000); + server.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"code\":10002,\"message\":\"" + hugeMessage + "\",\"requestId\":\"req-1\"}")); + QdmpClient client = TestClients.create(server); + + QdmpApiError error = + (QdmpApiError) Assertions.catchThrowable(() -> client.auth().getAccessToken()); + + assertThat(error).isNotNull(); + assertThat(error.toString().length()) + .as("QdmpApiError#toString() must bound the server-controlled message length") + .isLessThan(10_000); + } + + /** + * {@link QdmpApiError}'s constructor sanitizes {@code message} and {@code requestId} via {@code + * sanitize(...)} before storing them, but stores {@code code} completely verbatim -- {@code code} + * is just as server-controlled (see class Javadoc: "the qdmp API is known to return codes outside + * the documented set") and is echoed by both {@link QdmpApiError#getCode()} and {@link + * QdmpApiError#toString()}, so an embedded {@code \r\n} in {@code code} can inject fake log lines + * exactly like an unsanitized {@code message} would. + */ + @Test + void code_withEmbeddedCrLf_isSanitized_inToStringAndGetCode() { + QdmpApiError error = new QdmpApiError("1\r\nFAKE injected", "msg", null, 200); + + assertThat(error.toString()).doesNotContain("\r").doesNotContain("\n"); + assertThat(error.getCode()).doesNotContain("\r").doesNotContain("\n"); + } +} diff --git a/node/src/auth.ts b/node/src/auth.ts index 9cad22d..f879a87 100644 --- a/node/src/auth.ts +++ b/node/src/auth.ts @@ -9,6 +9,8 @@ * - refreshToken(): one-shot refresh of a caller-supplied refreshToken. * Never cached either. */ +import {inspect} from 'node:util'; + import type {HttpClient} from './http.js'; import type {AppTokenData, RefreshTokenData, SessionData} from './types.js'; import type {StoredToken, TokenStore} from './token-store.js'; @@ -18,6 +20,34 @@ import {QdmpTransportError, QdmpValidationError} from './errors.js'; /** Refresh this many seconds before the token's real expiresAt. */ const APP_TOKEN_REFRESH_BUFFER_SECONDS = 300; +/** + * Attaches a non-enumerable [util.inspect.custom] renderer to `value` so + * that `console.log(value)`/`util.inspect(value)` — an easy accidental + * debug-print path — never shows the raw accessToken/refreshToken. This is + * deliberately done via Object.defineProperty on an already-constructed, + * already-typed value rather than as an object-literal property: adding + * `[inspect.custom]: ...` directly to a `SessionData`/`RefreshTokenData` + * literal would trip TypeScript's excess-property check (neither interface + * declares a symbol-keyed member). + * + * This must never affect JSON.stringify(value) or direct property access + * (value.accessToken): JSON.stringify only consults toJSON(), not + * util.inspect.custom, and the defined property here is non-enumerable so + * it does not add a spurious key to the object either. Persisting the real + * values is the entire point of code2Session()/refreshToken() existing, so + * that path must stay untouched. + */ +function withRedactedInspect( + value: T, + render: () => string, +): T { + Object.defineProperty(value, inspect.custom, { + value: render, + enumerable: false, + }); + return value; +} + /** 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 @@ -53,6 +83,56 @@ function assertWellFormedTokenResponse( } } +/** Guards against a syntactically well-formed but already-expired-on-arrival + * expiresAt in a one-shot (never-cached) code2Session()/refreshToken() + * response. assertWellFormedTokenResponse() only checks that expiresAt + * parses as a non-negative safe integer — a value like "1" (1970-01-01) + * passes that check but describes a session/token that is already dead, and + * must not be handed back to the caller as if it were usable (they might + * persist or start relying on it). Unlike getAccessToken()'s cached path, + * this only rejects an already-past expiresAt, not one merely within the + * app-token refresh buffer — these one-shot results are never cached by the + * SDK, so there is no cache-freshness policy to apply here. */ +function assertNotAlreadyExpired( + data: {expiresAt?: string}, + context: string, +): void { + const nowSeconds = Math.floor(Date.now() / 1000); + if (Number(data.expiresAt) <= nowSeconds) { + throw new QdmpTransportError( + `${context}: server response was a business-success envelope but expiresAt is already in the past`, + ); + } +} + +/** Guards against a malformed code2Session() response: HTTP 200 + business + * code='0' but with a missing/empty refreshToken or openId. Unlike + * accessToken/expiresAt (validated by assertWellFormedTokenResponse() and + * shared with getAccessToken()/refreshToken()), refreshToken/openId are only + * present in the AUTHORIZATION_CODE grant's response, so this check is + * intentionally separate and only ever called from code2Session(). Must be + * called before the result is resolved to the caller — otherwise an empty + * openId could silently end up persisted as a user identifier. */ +function assertWellFormedSessionResponse( + data: {refreshToken?: string; openId?: string} | undefined, + context: string, +): void { + if ( + !data || + typeof data.refreshToken !== 'string' || + data.refreshToken.length === 0 + ) { + throw new QdmpTransportError( + `${context}: server response was a business-success envelope but is missing a non-empty refreshToken`, + ); + } + if (typeof data.openId !== 'string' || data.openId.length === 0) { + throw new QdmpTransportError( + `${context}: server response was a business-success envelope but is missing a non-empty openId`, + ); + } +} + export interface AuthModuleConfig { http: HttpClient; appId: string; @@ -124,8 +204,24 @@ export class AuthModule { this.inFlight = this.requestClientCredentialsToken() .then(async token => { assertWellFormedTokenResponse(token, 'auth.getAccessToken'); - this.cachedToken = token; + // A token that is syntactically well-formed can still be + // expired-on-arrival (e.g. a server bug returns an `expiresAt` + // far in the past). Such a token must never be cached or handed + // back to the caller as if it were freshly usable. + if (!this.isFresh(token)) { + throw new QdmpTransportError( + 'auth.getAccessToken: server response was a business-success envelope but the returned token is already expired (or within the refresh buffer) on arrival', + ); + } + // Only trust the local mirror once the store write actually + // succeeds — assigning this.cachedToken before awaiting + // tokenStore.set() would let a concurrent/later caller (which + // checks this.cachedToken directly, not this call's own promise) + // read back a value that was never durably persisted, if the + // store write goes on to reject. On rejection here, cachedToken + // is left untouched and this call's own promise rejects too. await Promise.resolve(this.tokenStore.set(token)); + this.cachedToken = token; return token.accessToken; }) .finally(() => { @@ -168,7 +264,19 @@ export class AuthModule { }, }); assertWellFormedTokenResponse(session, 'auth.code2Session'); - return session; + assertWellFormedSessionResponse(session, 'auth.code2Session'); + assertNotAlreadyExpired(session, 'auth.code2Session'); + return withRedactedInspect( + session, + // JSON.stringify (not raw template interpolation) for the non-secret + // fields: expiresAt is our own validated digit-only string, but openId + // is server-controlled and only validated as "non-empty" — an + // embedded CR/LF there would otherwise become a raw line break in + // this debug rendering, the same log-injection class of bug already + // fixed for message/code/requestId elsewhere. + () => + `SessionData { accessToken: '[REDACTED]', refreshToken: '[REDACTED]', expiresAt: ${JSON.stringify(session.expiresAt)}, openId: ${JSON.stringify(session.openId)} }`, + ); } async refreshToken(refreshToken: string): Promise { @@ -185,6 +293,11 @@ export class AuthModule { body: {refreshToken}, }); assertWellFormedTokenResponse(result, 'auth.refreshToken'); - return result; + assertNotAlreadyExpired(result, 'auth.refreshToken'); + return withRedactedInspect( + result, + () => + `RefreshTokenData { accessToken: '[REDACTED]', expiresAt: ${JSON.stringify(result.expiresAt)} }`, + ); } } diff --git a/node/src/client.ts b/node/src/client.ts index f488ba7..aa1e462 100644 --- a/node/src/client.ts +++ b/node/src/client.ts @@ -1,6 +1,7 @@ import type {Dispatcher} from 'undici'; import {AuthModule} from './auth.js'; +import {QdmpValidationError} from './errors.js'; import {GenaiGroup} from './groups/genai.js'; import {IslandGroup} from './groups/island.js'; import {MarkGroup} from './groups/mark.js'; @@ -14,6 +15,64 @@ import {InMemoryTokenStore, type TokenStore} from './token-store.js'; /** The qdmp OpenAPI host (see shared/openapi.yaml `servers[0]`). */ const DEFAULT_BASE_URL = 'https://openapi.qiandao.com'; +/** Matches a dotted-decimal IPv4 literal: exactly four groups of 1-3 digits + * separated by dots. This only tests the string SHAPE — range-checking each + * octet (0-255) and confirming the first octet is `127` is done separately + * in `isLoopbackHost()`. Intentionally does not accept anything else (no + * hex/octal forms, no hostnames) — a hostname like `127.attacker.com` must + * NOT match this, since its second "octet" is not a pure decimal string. */ +const IPV4_LITERAL_PATTERN = /^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/; + +/** True if `hostname` is a syntactically valid dotted-decimal IPv4 literal + * (all four octets in 0-255) whose first octet is exactly 127 — i.e. a + * genuine loopback IP address, not merely a string that starts with the + * text "127.". Deliberately does NOT perform any DNS resolution: this must + * stay a pure string/literal check, since resolving the hostname here would + * make the safety decision depend on network state instead of the literal + * the caller wrote. */ +function isLoopbackIpv4Literal(hostname: string): boolean { + const match = IPV4_LITERAL_PATTERN.exec(hostname); + if (!match) { + return false; + } + const octets = match.slice(1, 5).map(Number); + if (!octets.every(octet => octet >= 0 && octet <= 255)) { + return false; + } + return octets[0] === 127; +} + +/** Loopback/localhost hosts (case-insensitive) that are allowed to use + * plain http:// — e.g. a local dev/mock backend. Anything else on http:// + * would send appId/appSecret/accessToken in the clear. + * + * Must reject anything that is merely a string starting with "127." but is + * not itself a genuine IPv4 literal — e.g. the hostname `127.attacker.com` + * is a DNS name (not an IP) that can resolve to any attacker-controlled + * remote host, and must NOT be treated as loopback. */ +function isLoopbackHost(hostname: string): boolean { + const host = hostname.toLowerCase(); + return host === 'localhost' || isLoopbackIpv4Literal(host); +} + +/** Fail fast at construction time — never wait for the first real network + * request — if baseUrl is http:// against a non-loopback host. */ +function assertSafeBaseUrl(baseUrl: string): void { + let parsed: URL; + try { + parsed = new URL(baseUrl); + } catch { + throw new QdmpValidationError( + `QdmpClient: options.baseUrl "${baseUrl}" is not a valid URL`, + ); + } + if (parsed.protocol === 'http:' && !isLoopbackHost(parsed.hostname)) { + throw new QdmpValidationError( + `QdmpClient: options.baseUrl "${baseUrl}" uses plain http:// against a non-loopback host; use https:// (loopback/localhost is exempt for local dev/test)`, + ); + } +} + export interface QdmpClientOptions { appId: string; appSecret: string; @@ -40,6 +99,7 @@ export class QdmpClient { constructor(options: QdmpClientOptions) { const baseUrl = options.baseUrl ?? DEFAULT_BASE_URL; + assertSafeBaseUrl(baseUrl); const qdmpVersion = options.qdmpVersion ?? '1.0'; const http = new HttpClient({baseUrl, dispatcher: options.dispatcher}); diff --git a/node/src/errors.ts b/node/src/errors.ts index 70cb832..2a8331b 100644 --- a/node/src/errors.ts +++ b/node/src/errors.ts @@ -14,6 +14,40 @@ export type QdmpErrorCode = string | number; +/** Upper bound on how much of a server-supplied string is echoed into an + * Error's message — a server (or a compromised intermediary) must never be + * able to force an unbounded string into our caller's memory/logs via + * Error.message. */ +const MAX_SANITIZED_MESSAGE_LENGTH = 500; + +/** Matches the first C0 control character (0x00-0x1F) or DEL (0x7F) in a + * string — most notably CR and LF. */ +const CONTROL_CHAR_PATTERN = new RegExp( + '[' + + String.fromCharCode(...Array.from({length: 32}, (_, i) => i)) + + String.fromCharCode(127) + + ']', +); + +/** Sanitizes a server-supplied string before it is ever interpolated into an + * Error's message. A malicious/compromised server could embed CR/LF into + * `message` to forge fake extra log lines once a caller writes + * error.message to a single-line log (log injection). Replacing the control + * character in place would still leave every byte the attacker chose intact + * and visible right next to it, so instead this truncates the string at the + * first control character — nothing the attacker placed after an injected + * CR/LF is ever included, forged log line or not. Also enforces a hard + * length cap so an arbitrarily long message can't bloat/DoS a caller's + * memory or logs. */ +function sanitizeForErrorMessage(value: string): string { + const controlCharIndex = value.search(CONTROL_CHAR_PATTERN); + const withoutControlChars = + controlCharIndex === -1 ? value : value.slice(0, controlCharIndex); + return withoutControlChars.length > MAX_SANITIZED_MESSAGE_LENGTH + ? withoutControlChars.slice(0, MAX_SANITIZED_MESSAGE_LENGTH) + '…' + : withoutControlChars; +} + export interface QdmpApiErrorInit { code: QdmpErrorCode; message: string; @@ -30,13 +64,31 @@ export class QdmpApiError extends Error { readonly requestId?: string; constructor(init: QdmpApiErrorInit) { + // `code` is an intentionally open `string | number` union (see file + // header) — when it's a string, it is just as server-controlled as + // `message` and must go through the same sanitization before being + // interpolated into this Error's message. A numeric `code` can't carry + // CR/LF or excessive length, so it's used as-is. + const sanitizedCode = + typeof init.code === 'string' + ? sanitizeForErrorMessage(init.code) + : init.code; super( - `qdmp API error (code=${init.code}, httpStatus=${init.httpStatus}): ${init.message}`, + `qdmp API error (code=${sanitizedCode}, httpStatus=${init.httpStatus}): ${sanitizeForErrorMessage(init.message)}`, ); this.name = 'QdmpApiError'; - this.code = init.code; + // Store the same sanitized value used above for the message, not the + // raw init.code — this.code and toJSON().code are public surfaces a + // caller may log/serialize directly, bypassing .message entirely. + this.code = sanitizedCode; this.httpStatus = init.httpStatus; - this.requestId = init.requestId; + // requestId is just as server-controlled as message/code — sanitize it + // too, since callers may log it directly (or via toJSON()) without ever + // going through Error.message. + this.requestId = + init.requestId !== undefined + ? sanitizeForErrorMessage(init.requestId) + : undefined; // Restore the prototype chain — needed because targets below ES2015 // (and some transpilers) don't do this automatically when extending // built-ins like Error. diff --git a/node/src/http.ts b/node/src/http.ts index 6cc9786..98bca67 100644 --- a/node/src/http.ts +++ b/node/src/http.ts @@ -19,6 +19,57 @@ export interface HttpClientConfig { dispatcher?: Dispatcher; } +/** Upper bound on how many response body bytes we'll read before giving up. + * The qdmp OpenAPI response is a business JSON envelope, not a bulk data + * transfer — an oversized body (whether from a misbehaving server or a + * malicious intermediary) should never be fully buffered into memory just to + * find out it doesn't fit. Content-Length can't be trusted for this (it may + * be absent, or simply wrong), so the actual bytes read off the stream are + * counted as they arrive. */ +const MAX_RESPONSE_BODY_BYTES = 10 * 1024 * 1024; + +/** Reads `response.body` chunk-by-chunk, counting bytes as they arrive so a + * body exceeding MAX_RESPONSE_BODY_BYTES is rejected — and the underlying + * stream cancelled — well before it would ever be handed to JSON.parse(). */ +async function readBoundedJsonBody( + response: Response, + httpStatus: number, +): Promise { + const body = response.body; + if (!body) { + // No body stream (e.g. some 204-like responses); treat as empty. + return JSON.parse(''); + } + const reader = body.getReader(); + const chunks: Uint8Array[] = []; + let totalBytes = 0; + for (;;) { + const {done, value} = await reader.read(); + if (done) { + break; + } + totalBytes += value.byteLength; + if (totalBytes > MAX_RESPONSE_BODY_BYTES) { + await reader.cancel().catch(() => { + // best-effort connection cleanup; must not mask the size error + }); + throw new QdmpTransportError( + `response body exceeded the maximum allowed size of ${MAX_RESPONSE_BODY_BYTES} bytes`, + {httpStatus}, + ); + } + chunks.push(value); + } + const combined = new Uint8Array(totalBytes); + let offset = 0; + for (const chunk of chunks) { + combined.set(chunk, offset); + offset += chunk.byteLength; + } + const text = new TextDecoder('utf-8').decode(combined); + return JSON.parse(text); +} + export type QueryValue = string | number | boolean | string[] | undefined; export interface HttpRequestOptions { @@ -87,14 +138,29 @@ function parseEnvelope(payload: unknown, httpStatus: number): T { }); } const body = payload as Record; - const code = body.code as string | number | undefined; - if (code === undefined) { + const rawCode: unknown = body.code; + if (rawCode === undefined) { throw new QdmpApiError({ code: 'QDMP_SDK_INVALID_RESPONSE_SHAPE', message: 'response body is missing the "code" field', httpStatus, }); } + // `body.code` was previously narrowed with `as string | number` — a type + // assertion, not a runtime check. A server (or compromised intermediary) + // returning e.g. an array/object for "code" would sail through that + // assertion, bypass QdmpApiError's string-only sanitization (which only + // triggers for typeof === 'string'), and have its unbounded + // Array.prototype.toString()/Object.prototype.toString() output + // interpolated straight into the thrown error's message/.code/.toJSON(). + if (typeof rawCode !== 'string' && typeof rawCode !== 'number') { + throw new QdmpApiError({ + code: 'QDMP_SDK_INVALID_RESPONSE_SHAPE', + message: 'response body\'s "code" field is neither a string nor a number', + httpStatus, + }); + } + const code = rawCode; const message = typeof body.message === 'string' ? body.message : ''; if (String(code) !== '0') { const requestId = @@ -148,8 +214,11 @@ export class HttpClient { } let payload: unknown; try { - payload = await response.json(); - } catch { + payload = await readBoundedJsonBody(response, httpStatus); + } catch (err) { + if (err instanceof QdmpTransportError) { + throw err; + } throw new QdmpApiError({ code: 'QDMP_SDK_INVALID_JSON_RESPONSE', message: 'response body could not be parsed as JSON', diff --git a/node/test/auth-expired-fresh-token.test.ts b/node/test/auth-expired-fresh-token.test.ts new file mode 100644 index 0000000..a11274f --- /dev/null +++ b/node/test/auth-expired-fresh-token.test.ts @@ -0,0 +1,109 @@ +/** + * auth 模块"已过期即到达（expired-on-arrival）"token 测试。 + * + * 需求：服务端返回 HTTP 200 + 业务 code='0'（判定为成功）且 accessToken/ + * expiresAt 语法上都合法，但 expiresAt 是一个早已过期的时间戳（例如服务端 + * bug 返回 "1"，即 1970 年 Unix epoch 之后 1 秒）时，refreshAppToken() 必须 + * reject，而不能把这个"语法合法但语义上已过期"的 token 当作新鲜 token + * resolve 给调用方、写进 this.cachedToken / tokenStore。 + * + * 现状（本测试写下时）：src/auth.ts 的 assertWellFormedTokenResponse()（约 + * 27-54行）只检查 accessToken 非空、expiresAt 是否是语法上合法的非负整数 + * 字符串，完全不检查它是否已经过期。isFresh(token)（约127-131行）本可以做 + * 这个"是否新鲜"的判断（expiresAt - now > 300秒缓冲），但 refreshAppToken() + * （约147-164行）在拿到刚请求回来的 token 后，只调用了 + * assertWellFormedTokenResponse()，从未对这个新 token 调用 isFresh() 校验， + * 就直接写入 this.cachedToken / tokenStore 并 resolve 给调用方。因此下面的 + * 测试预期是失败的（红色状态）：当前实现会 resolve 成 'app-token-abc'，而不是 + * 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'; + +void test('auth.getAccessToken: HTTP 200 + {code:0} 但 expiresAt 是早已过期的时间戳（"1"，1970年）时应 reject，且不得把这个已过期 token 当作新鲜缓存写入（下一次调用仍会真实重新换取）', async () => { + const {mockAgent, pool} = createMockAgent(); + pool + .intercept({path: '/auth/v1/token', method: 'POST'}) + .reply( + 200, + businessSuccess({accessToken: 'app-token-abc', expiresAt: '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({ + accessToken: 'app-token-good', + expiresAt: String(Math.floor(Date.now() / 1000) + 7200), + }), + ); + + const token = await client.auth.getAccessToken(); + assert.equal( + token, + 'app-token-good', + '前一次已过期 token 被拒绝之后，下一次调用应当真实重新发起 CLIENT_CREDENTIALS 换取，而不是复用坏缓存', + ); + assert.equal( + mockAgent.pendingInterceptors().length, + 0, + '第二次真实请求的拦截器应当被消费', + ); +}); + +// 回归测试（codex 收敛检查轮发现）：code2Session/refreshToken 这两个"一次性、 +// 不缓存"的路径同样只校验 expiresAt 是否语法合法（非负整数字符串），没有校验 +// 它是否早已过期——一次性会话/token 虽然不走本 SDK 的缓存，但如果早已过期就 +// 返回给调用方，调用方可能会把这个"看起来成功"的死会话持久化下来。 +void test('auth.code2Session: expiresAt 是早已过期的时间戳（"1"）时应 reject', async () => { + const {mockAgent, pool} = createMockAgent(); + pool.intercept({path: '/auth/v1/token', method: 'POST'}).reply( + 200, + businessSuccess({ + accessToken: 'user-access-token', + refreshToken: 'user-refresh-token', + expiresAt: '1', + openId: 'open-id-1', + }), + ); + + const client = new QdmpClient({ + appId: 'app-id-1', + appSecret: 'app-secret-1', + dispatcher: mockAgent, + }); + + await expectFailure(() => client.auth.code2Session('wx-login-code-abc')); +}); + +void test('auth.refreshToken: expiresAt 是早已过期的时间戳（"1"）时应 reject', async () => { + const {mockAgent, pool} = createMockAgent(); + pool + .intercept({path: '/auth/v1/refresh', method: 'POST'}) + .reply(200, businessSuccess({accessToken: 'new-token', expiresAt: '1'})); + + const client = new QdmpClient({ + appId: 'app-id-1', + appSecret: 'app-secret-1', + dispatcher: mockAgent, + }); + + await expectFailure(() => client.auth.refreshToken('some-refresh-token')); +}); diff --git a/node/test/auth-result-inspect-redaction.test.ts b/node/test/auth-result-inspect-redaction.test.ts new file mode 100644 index 0000000..a193d37 --- /dev/null +++ b/node/test/auth-result-inspect-redaction.test.ts @@ -0,0 +1,142 @@ +/** + * 回归测试：code2Session()/refreshToken() 返回值的调试打印脱敏。 + * + * 背景（Fable5 设计一致性审查发现的真实安全问题）：code2Session()/ + * refreshToken() 返回的普通 object literal（类型见 src/types.ts 的 + * SessionData/RefreshTokenData）一旦被 console.log(session)/util.inspect(session) + * 这类"意外调试打印"路径打印，就会把 accessToken/refreshToken 明文暴露出去。 + * + * 修复（src/auth.ts 的 withRedactedInspect()）给返回对象挂了一个不可枚举的 + * [util.inspect.custom] 方法，只影响 util.inspect()/console.log() 这条路径。 + * + * 本测试同时覆盖两个方向： + * 1. util.inspect(session) 拿到的字符串表示不能包含真实的 accessToken/refreshToken。 + * 2. 反向验证：JSON.stringify(session) 和直接属性访问（session.accessToken / + * session.refreshToken）必须仍然是原始明文值——因为 code2Session()/ + * refreshToken() 存在的整个目的就是让调用方把这些真实值持久化到自己的 + * 数据库/session store，如果连 JSON.stringify 都被脱敏了 SDK 就没法用了。 + */ +import assert from 'node:assert/strict'; +import {test} from 'node:test'; +import {inspect} from 'node:util'; + +import {QdmpClient} from '../src/client.js'; +import {businessSuccess, createMockAgent} from './helpers/mock-http.js'; + +const SENTINEL_ACCESS_TOKEN = 'sentinel-access-token-do-not-leak-xyz123'; +const SENTINEL_REFRESH_TOKEN = 'sentinel-refresh-token-do-not-leak-xyz123'; + +function makeClient( + mockAgent: ReturnType['mockAgent'], +) { + return new QdmpClient({ + appId: 'app-id-1', + appSecret: 'app-secret-1', + dispatcher: mockAgent, + }); +} + +void test('auth.code2Session: util.inspect()/console.log() 路径不得暴露 accessToken/refreshToken 明文', async () => { + const {mockAgent, pool} = createMockAgent(); + pool.intercept({path: '/auth/v1/token', method: 'POST'}).reply( + 200, + businessSuccess({ + accessToken: SENTINEL_ACCESS_TOKEN, + refreshToken: SENTINEL_REFRESH_TOKEN, + expiresAt: String(Math.floor(Date.now() / 1000) + 7200), + openId: 'open-id-1', + }), + ); + + const client = makeClient(mockAgent); + const session = await client.auth.code2Session('some-auth-code'); + + const rendered = inspect(session); + assert.ok( + !rendered.includes(SENTINEL_ACCESS_TOKEN), + `util.inspect(session) leaked the raw accessToken: ${rendered}`, + ); + assert.ok( + !rendered.includes(SENTINEL_REFRESH_TOKEN), + `util.inspect(session) leaked the raw refreshToken: ${rendered}`, + ); + assert.ok( + rendered.includes('[REDACTED]'), + `util.inspect(session) should contain the [REDACTED] placeholder: ${rendered}`, + ); + + // Reverse-direction guard: JSON.stringify (how the caller persists the + // session) and direct property access must both stay real plaintext. + const json = JSON.stringify(session); + assert.ok( + json.includes(SENTINEL_ACCESS_TOKEN), + `JSON.stringify(session) must still contain the real accessToken for the caller to persist: ${json}`, + ); + assert.ok( + json.includes(SENTINEL_REFRESH_TOKEN), + `JSON.stringify(session) must still contain the real refreshToken for the caller to persist: ${json}`, + ); + assert.equal(session.accessToken, SENTINEL_ACCESS_TOKEN); + assert.equal(session.refreshToken, SENTINEL_REFRESH_TOKEN); +}); + +void test('auth.refreshToken: util.inspect()/console.log() 路径不得暴露 accessToken 明文', async () => { + const {mockAgent, pool} = createMockAgent(); + pool.intercept({path: '/auth/v1/refresh', method: 'POST'}).reply( + 200, + businessSuccess({ + accessToken: SENTINEL_ACCESS_TOKEN, + expiresAt: String(Math.floor(Date.now() / 1000) + 7200), + }), + ); + + const client = makeClient(mockAgent); + const result = await client.auth.refreshToken('some-refresh-token'); + + const rendered = inspect(result); + assert.ok( + !rendered.includes(SENTINEL_ACCESS_TOKEN), + `util.inspect(result) leaked the raw accessToken: ${rendered}`, + ); + assert.ok( + rendered.includes('[REDACTED]'), + `util.inspect(result) should contain the [REDACTED] placeholder: ${rendered}`, + ); + + const json = JSON.stringify(result); + assert.ok( + json.includes(SENTINEL_ACCESS_TOKEN), + `JSON.stringify(result) must still contain the real accessToken for the caller to persist: ${json}`, + ); + assert.equal(result.accessToken, SENTINEL_ACCESS_TOKEN); +}); + +// 回归测试（codex 收敛检查轮发现）：openId 只校验非空字符串，服务端返回一个 +// 嵌入 \r\n 的 openId 时，withRedactedInspect() 的渲染函数如果直接把它拼进 +// 模板字符串，util.inspect(session) 的结果就会出现原始换行——伪造出看似 +// 独立的新日志行，和 message/code/requestId 已经修过的日志注入是同一类问题。 +void test('auth.code2Session: openId 里嵌入的 \\r\\n 不得原样出现在 util.inspect() 的调试渲染中（防日志注入）', async () => { + const injectedOpenId = 'open-id-1\r\nFAKE injected log line'; + const {mockAgent, pool} = createMockAgent(); + pool.intercept({path: '/auth/v1/token', method: 'POST'}).reply( + 200, + businessSuccess({ + accessToken: SENTINEL_ACCESS_TOKEN, + refreshToken: SENTINEL_REFRESH_TOKEN, + expiresAt: String(Math.floor(Date.now() / 1000) + 7200), + openId: injectedOpenId, + }), + ); + + const client = makeClient(mockAgent); + const session = await client.auth.code2Session('some-auth-code'); + + const rendered = inspect(session); + assert.ok( + !rendered.includes('\r'), + `util.inspect(session) 不应包含 openId 中原始的 \\r 字符，实际: ${JSON.stringify(rendered)}`, + ); + // 反向验证：JSON.stringify 仍然携带真实的（哪怕含控制字符的）openId，因为 + // 这是调用方持久化 session 的正常路径，不应该被这个调试渲染修复影响到。 + assert.equal(session.openId, injectedOpenId); +}); diff --git a/node/test/auth-session-fields.test.ts b/node/test/auth-session-fields.test.ts new file mode 100644 index 0000000..08726e7 --- /dev/null +++ b/node/test/auth-session-fields.test.ts @@ -0,0 +1,90 @@ +/** + * auth.code2Session() 字段完整性校验测试。 + * + * 需求：code2Session() 除了要求 accessToken/expiresAt 合法（已由 + * assertWellFormedTokenResponse() 覆盖，见 auth-malformed-success.test.ts）， + * 还必须校验业务层返回的 refreshToken/openId 非空 —— 这两个字段是 + * SessionData 的一部分（见 src/types.ts），调用方（宿主后端）需要拿 + * refreshToken 去后续换取新 token、拿 openId 去关联本地用户，一旦 + * 静默 resolve 出 undefined/空字符串，后续业务逻辑会在错误的地方失败 + * （比如把空字符串当 openId 存进数据库）。 + * + * 现状（本测试写下时）：AuthModule.code2Session()（src/auth.ts 约152-172行） + * 只调用 assertWellFormedTokenResponse(session, ...) 校验 accessToken/expiresAt， + * 完全不检查 session.refreshToken / session.openId 是否存在或非空。 + * 因此下面的测试预期是失败的（红色状态）：当前实现会把畸形 session 直接 + * resolve 给调用方，而不是 reject。 + */ +import {test} from 'node:test'; + +import {QdmpClient} from '../src/client.js'; +import { + businessSuccess, + createMockAgent, + expectFailure, +} from './helpers/mock-http.js'; + +function sessionData( + overrides: Record = {}, +): Record { + return { + accessToken: 'session-access-abc', + refreshToken: 'session-refresh-abc', + expiresAt: String(Math.floor(Date.now() / 1000) + 7200), + openId: 'open-id-abc', + ...overrides, + }; +} + +function makeClient( + mockAgent: ReturnType['mockAgent'], +) { + return new QdmpClient({ + appId: 'app-id-1', + appSecret: 'app-secret-1', + dispatcher: mockAgent, + }); +} + +void test('auth.code2Session: refreshToken 字段完全缺失时应 reject', async () => { + const {mockAgent, pool} = createMockAgent(); + // sessionData({refreshToken: undefined}) 展开后 refreshToken 的值是 + // undefined，JSON.stringify（undici MockAgent 序列化 reply body 时会用到） + // 会直接丢弃 undefined 值的 key，等价于"响应里完全没有这个字段"。 + pool + .intercept({path: '/auth/v1/token', method: 'POST'}) + .reply(200, businessSuccess(sessionData({refreshToken: undefined}))); + + const client = makeClient(mockAgent); + await expectFailure(() => client.auth.code2Session('some-auth-code')); +}); + +void test('auth.code2Session: refreshToken 为空字符串时应 reject，不得把空字符串当作合法 refreshToken resolve 给调用方', async () => { + const {mockAgent, pool} = createMockAgent(); + pool + .intercept({path: '/auth/v1/token', method: 'POST'}) + .reply(200, businessSuccess(sessionData({refreshToken: ''}))); + + const client = makeClient(mockAgent); + await expectFailure(() => client.auth.code2Session('some-auth-code')); +}); + +void test('auth.code2Session: openId 字段完全缺失时应 reject', async () => { + const {mockAgent, pool} = createMockAgent(); + pool + .intercept({path: '/auth/v1/token', method: 'POST'}) + .reply(200, businessSuccess(sessionData({openId: undefined}))); + + const client = makeClient(mockAgent); + await expectFailure(() => client.auth.code2Session('some-auth-code')); +}); + +void test('auth.code2Session: openId 为空字符串时应 reject，不得把空字符串当作合法 openId resolve 给调用方', async () => { + const {mockAgent, pool} = createMockAgent(); + pool + .intercept({path: '/auth/v1/token', method: 'POST'}) + .reply(200, businessSuccess(sessionData({openId: ''}))); + + const client = makeClient(mockAgent); + await expectFailure(() => client.auth.code2Session('some-auth-code')); +}); diff --git a/node/test/auth-tokenstore-write-failure.test.ts b/node/test/auth-tokenstore-write-failure.test.ts new file mode 100644 index 0000000..9d85db5 --- /dev/null +++ b/node/test/auth-tokenstore-write-failure.test.ts @@ -0,0 +1,105 @@ +/** + * 回归测试（codex 收敛检查轮发现）：TokenStore.set() 写入失败时，本地 + * cachedToken 镜像不应当被当作"已经成功持久化"来使用。 + * + * 背景：refreshAppToken()（src/auth.ts）里，`this.cachedToken = token` 曾经 + * 在 `await this.tokenStore.set(token)` 之前就赋值。如果 set() 之后 reject + * （例如一个 Redis-backed TokenStore 因网络问题写入失败），触发这次刷新的 + * 调用本身会因为 await 到 rejected promise 而失败——但 `this.cachedToken` + * 已经被设成了这个"实际上没能持久化"的值。此后（哪怕就在同一个 event loop + * tick 之后）到达的并发/后续调用会看到 `this.cachedToken` 且判定为 fresh， + * 从而复用这个从未真正写入 store 的值，表现得像是这次写入成功了一样——而 + * 其他共享同一个远程 TokenStore 的实例完全不知道这个新 token。 + * + * 修复：把 `this.cachedToken = token` 移到 `await tokenStore.set(token)` + * 成功之后，写入失败时 cachedToken 保持不变。 + */ +import assert from 'node:assert/strict'; +import {test} from 'node:test'; + +import {QdmpClient} from '../src/client.js'; +import type {StoredToken, TokenStore} from '../src/token-store.js'; +import { + businessSuccess, + createMockAgent, + expectFailure, +} from './helpers/mock-http.js'; + +class RejectOnceTokenStore implements TokenStore { + private token: StoredToken | undefined; + private rejectNextSet: boolean; + setCallCount = 0; + + constructor(rejectFirstSet: boolean) { + this.rejectNextSet = rejectFirstSet; + } + + get(): StoredToken | undefined { + return this.token; + } + + async set(token: StoredToken): Promise { + this.setCallCount++; + if (this.rejectNextSet) { + this.rejectNextSet = false; + throw new Error( + 'simulated TokenStore.set() failure (e.g. Redis unreachable)', + ); + } + this.token = token; + } + + clear(): void { + this.token = undefined; + } +} + +void test('auth.getAccessToken: tokenStore.set() 第一次写入失败时，该次调用必须 reject，且不得把这个未持久化的值当作本地新鲜缓存复用', async () => { + const {mockAgent, pool} = createMockAgent(); + const tokenStore = new RejectOnceTokenStore(/* rejectFirstSet= */ true); + + pool.intercept({path: '/auth/v1/token', method: 'POST'}).reply( + 200, + businessSuccess({ + accessToken: 'token-from-failed-write', + expiresAt: String(Math.floor(Date.now() / 1000) + 7200), + }), + ); + + const client = new QdmpClient({ + appId: 'app-id-1', + appSecret: 'app-secret-1', + dispatcher: mockAgent, + tokenStore, + }); + + await expectFailure(() => client.auth.getAccessToken()); + assert.equal( + tokenStore.setCallCount, + 1, + 'tokenStore.set() 应当被调用了一次（即使它失败了）', + ); + + // 紧接着的下一次调用：如果 cachedToken 被错误地提前赋值，这里会直接 + // resolve 成 "token-from-failed-write" 而不发起任何真实请求 + // （disableNetConnect 会让请求因找不到拦截器而报错，从而暴露这个 bug）。 + pool.intercept({path: '/auth/v1/token', method: 'POST'}).reply( + 200, + businessSuccess({ + accessToken: 'token-from-good-write', + expiresAt: String(Math.floor(Date.now() / 1000) + 7200), + }), + ); + + const token = await client.auth.getAccessToken(); + assert.equal( + token, + 'token-from-good-write', + '第一次写入失败之后，下一次调用应当真实重新发起 CLIENT_CREDENTIALS 换取，而不是复用从未成功持久化的本地缓存', + ); + assert.equal( + mockAgent.pendingInterceptors().length, + 0, + '第二次真实请求的拦截器应当被消费', + ); +}); diff --git a/node/test/client-baseurl-scheme.test.ts b/node/test/client-baseurl-scheme.test.ts new file mode 100644 index 0000000..68e85ed --- /dev/null +++ b/node/test/client-baseurl-scheme.test.ts @@ -0,0 +1,135 @@ +/** + * QdmpClient 构造器 baseUrl scheme 校验测试。 + * + * 需求：new QdmpClient({..., baseUrl})，若 baseUrl 的 scheme 是 http（而非 + * https）且 host 不是 loopback/localhost，构造器必须**同步抛异常**——不需要 + * 等到真正发起网络请求才失败（fail fast，和本项目 Go/Java 版 SDK 的对应设计 + * 保持一致：appSecret/appId 等敏感信息绝不应该有机会被明文发往非 HTTPS + * origin）。但 127.0.0.1 / localhost（不区分大小写）/ 127. 开头的地址即使是 + * http 也必须放行，因为本地开发/测试环境常见 http://localhost:xxxx 这种 + * mock/自建后端。 + * + * 现状（本测试写下时）：QdmpClient 构造器（src/client.ts 约41-44行）对 + * options.baseUrl 不做任何 scheme 检查，直接透传给 HttpClient。因此第一个 + * 测试（要求非 loopback 的 http baseUrl 必须抛异常）预期是失败的（红色状态） + * ——当前实现不会抛。其余测试（loopback/https/默认值都不应抛异常）用于防止 + * 未来实现过度收紧规则，现在应当已经通过。 + */ +import assert from 'node:assert/strict'; +import {test} from 'node:test'; + +import {QdmpClient} from '../src/client.js'; + +void test('new QdmpClient: baseUrl 为非 loopback 的 http origin 时必须同步抛异常', () => { + assert.throws( + () => + new QdmpClient({ + appId: 'a', + appSecret: 's', + baseUrl: 'http://evil.example.com', + }), + /./, + 'baseUrl 是 http 且 host 既不是 127.0.0.1 也不是 localhost 时，构造器应当同步抛异常，而不是等到发起请求才失败', + ); +}); + +void test('new QdmpClient: baseUrl 为 http://127.0.0.1: 时不应抛异常（loopback 例外）', () => { + assert.doesNotThrow( + () => + new QdmpClient({ + appId: 'a', + appSecret: 's', + baseUrl: 'http://127.0.0.1:12345', + }), + ); +}); + +void test('new QdmpClient: baseUrl 为 http://localhost: 时不应抛异常（不区分大小写）', () => { + assert.doesNotThrow( + () => + new QdmpClient({ + appId: 'a', + appSecret: 's', + baseUrl: 'http://localhost:12345', + }), + ); + assert.doesNotThrow( + () => + new QdmpClient({ + appId: 'a', + appSecret: 's', + baseUrl: 'http://LOCALHOST:12345', + }), + 'localhost 的大小写不应影响 loopback 例外判定', + ); +}); + +void test('new QdmpClient: baseUrl 为 https origin 时不应抛异常', () => { + assert.doesNotThrow( + () => + new QdmpClient({ + appId: 'a', + appSecret: 's', + baseUrl: 'https://example.com', + }), + ); +}); + +void test('new QdmpClient: 不传 baseUrl（使用默认值）时不应抛异常', () => { + assert.doesNotThrow( + () => + new QdmpClient({ + appId: 'a', + appSecret: 's', + }), + ); +}); + +/** + * 补充测试（安全回归）：isLoopbackHost()（src/client.ts 约21-26行）用 + * `host.startsWith('127.')` 判定 loopback 例外，这是对 hostname 字符串做前缀 + * 匹配，而不是校验它确实是一个点分十进制的 IPv4 字面量。因此域名 + * `127.attacker.com`（不是 IP，是一个可以被攻击者解析到任意远程主机的 DNS + * 名）会被误判为 loopback，从而放行 http://（明文）请求，appSecret/token 会 + * 被发往攻击者控制的远程主机。 + * + * 现状（本测试写下时）：下面第一个测试断言构造器必须同步抛异常，预期是失败的 + * （红色状态）——当前实现不会抛，因为 '127.attacker.com'.startsWith('127.') + * 为 true。其余测试断言真正的 127.x.x.x IPv4 字面量仍然不应抛异常（这是 + * loopback 例外本该覆盖的合法场景），这些现在应当已经通过，用于防止未来修复 + * 过度收紧规则。 + */ +void test('new QdmpClient: baseUrl 为 http://127.attacker.com（域名，非 IP 字面量）时必须同步抛异常', () => { + assert.throws( + () => + new QdmpClient({ + appId: 'a', + appSecret: 's', + baseUrl: 'http://127.attacker.com', + }), + /./, + 'hostname 是域名 127.attacker.com（可解析到任意攻击者控制的主机），不是真正的 127.0.0.1 loopback IP 字面量，不应被 startsWith("127.") 误判为 loopback 而放行明文 http', + ); +}); + +void test('new QdmpClient: baseUrl 为 http://127.5.6.7（真正的 127.x.x.x IPv4 loopback 字面量）时不应抛异常', () => { + assert.doesNotThrow( + () => + new QdmpClient({ + appId: 'a', + appSecret: 's', + baseUrl: 'http://127.5.6.7:12345', + }), + ); +}); + +void test('new QdmpClient: baseUrl 为 http://127.0.0.1（无端口）时不应抛异常', () => { + assert.doesNotThrow( + () => + new QdmpClient({ + appId: 'a', + appSecret: 's', + baseUrl: 'http://127.0.0.1', + }), + ); +}); diff --git a/node/test/errors-code-sanitization.test.ts b/node/test/errors-code-sanitization.test.ts new file mode 100644 index 0000000..10bca42 --- /dev/null +++ b/node/test/errors-code-sanitization.test.ts @@ -0,0 +1,133 @@ +/** + * QdmpApiError code 字段安全处理测试。 + * + * 需求：`QdmpErrorCode` 是有意开放的 `string | number` union（见 + * src/errors.ts 文件头注释），服务端可以返回任意字符串作为 code（例如网关层 + * 小整数、未文档化的 code），因此 code 和 message 一样，都是"服务端可控、会 + * 被原样拼进 Error.message"的字段。message 已经过 sanitizeForErrorMessage() + * 处理（截断首个 C0/DEL 控制字符，并限制最大长度 500），但 code 没有—— + * QdmpApiError 构造函数（src/errors.ts 约66-69行）里 `init.code` 被直接 + * 拼进 `super(...)` 模板字符串，未经任何处理。因此服务端可以在一个 + * STRING 类型的 code 字段里嵌入 \r\n（例如 "1\r\nFAKE injected"），这段内容 + * 会原样出现在 QdmpApiError.message / toString() 里，一旦调用方把 + * error.message 写进单行日志，就会伪造出看似独立的新日志行（日志注入）。 + * + * 现状（本测试写下时）：QdmpApiError 构造函数只对 init.message 调用 + * sanitizeForErrorMessage()，对 init.code 完全不做任何处理。因此下面的测试 + * 预期是失败的（红色状态）：error.message / toString() 中会原样包含 code + * 里注入的 \r 字符。 + */ +import assert from 'node:assert/strict'; +import {test} from 'node:test'; + +import {QdmpClient} from '../src/client.js'; +import {QdmpApiError} from '../src/errors.js'; +import { + businessFailure, + createMockAgent, + expectFailure, +} from './helpers/mock-http.js'; + +void test('QdmpApiError: code 字段中嵌入的 \\r\\n 不得原样出现在 error.message / toString() 中（防日志注入）', async () => { + const injectedCode = '1\r\nFAKE injected'; + const {mockAgent, pool} = createMockAgent(); + pool + .intercept({path: '/auth/v1/token', method: 'POST'}) + .reply(200, businessFailure(injectedCode, 'ok')); + + const client = new QdmpClient({ + appId: 'app-id-1', + appSecret: 'app-secret-1', + dispatcher: mockAgent, + }); + + const err = await expectFailure(() => client.auth.getAccessToken()); + + assert.ok( + err instanceof QdmpApiError, + `应当 reject QdmpApiError，实际抛出: ${String(err)}`, + ); + const apiErr = err as QdmpApiError; + const rendered = `${apiErr.message}\n${apiErr.toString()}`; + assert.ok( + !rendered.includes('\r'), + `QdmpApiError 的 message/toString() 不应包含 code 字段中原始的 \\r 字符，实际: ${JSON.stringify(rendered)}`, + ); + assert.ok( + !rendered.includes('FAKE injected') || !rendered.includes('\n'), + 'QdmpApiError 不应让服务端 code 字段里的换行符原样保留，从而伪造出独立的新日志行', + ); + + // 回归测试（codex 收敛检查轮发现）：构造函数只在拼 message 时对 code + // 做了清洗，`this.code`/`toJSON().code` 这两个调用方可能直接读取/序列化的 + // 字段之前存的是未清洗的 init.code 原始值，同样会造成日志注入。 + const rawFieldsRendered = `${String(apiErr.code)}\n${JSON.stringify(apiErr.toJSON().code)}`; + assert.ok( + !rawFieldsRendered.includes('\r'), + `QdmpApiError.code / toJSON().code 不应包含原始的 \\r 字符，实际: ${JSON.stringify(rawFieldsRendered)}`, + ); +}); + +void test('QdmpApiError: requestId 字段中嵌入的 \\r\\n 不得原样出现在 .requestId / toJSON() 中（防日志注入，调用方可能直接读字段而不经过 message）', async () => { + const injectedRequestId = 'req-1\r\nFAKE injected'; + const {mockAgent, pool} = createMockAgent(); + pool.intercept({path: '/auth/v1/token', method: 'POST'}).reply( + 200, + businessFailure('10003', 'bad code', { + requestId: injectedRequestId, + }), + ); + + const client = new QdmpClient({ + appId: 'app-id-1', + appSecret: 'app-secret-1', + dispatcher: mockAgent, + }); + + const err = await expectFailure(() => client.auth.getAccessToken()); + + assert.ok(err instanceof QdmpApiError); + const apiErr = err as QdmpApiError; + assert.ok( + apiErr.requestId === undefined || !apiErr.requestId.includes('\r'), + `QdmpApiError.requestId 不应包含原始的 \\r 字符，实际: ${JSON.stringify(apiErr.requestId)}`, + ); + const jsonRequestId = apiErr.toJSON().requestId; + assert.ok( + typeof jsonRequestId !== 'string' || !jsonRequestId.includes('\r'), + 'toJSON() 里的 requestId 同样不应包含原始的 \\r 字符', + ); +}); + +void test('HttpClient.request: 响应体的 code 字段是数组/对象（既不是 string 也不是 number）时应 reject，而不是把整个数组/对象无界地拼进 error message', async () => { + const {mockAgent, pool} = createMockAgent(); + // `body.code as string | number` 曾经只是 TS 的类型断言，不是运行时校验： + // 服务端返回一个数组形态的 code，会绕过 QdmpApiError 构造函数里"只在 + // typeof === 'string' 时才清洗"的判断，把 Array.prototype.toString() 的 + // 输出原样、无长度上限地拼进 message/.code/.toJSON()。 + pool.intercept({path: '/auth/v1/token', method: 'POST'}).reply(200, { + code: Array.from({length: 100}, (_, i) => `malformed-code-part-${i}`), + message: 'ok', + }); + + const client = new QdmpClient({ + appId: 'app-id-1', + appSecret: 'app-secret-1', + dispatcher: mockAgent, + }); + + const err = await expectFailure(() => client.auth.getAccessToken()); + assert.ok( + err instanceof QdmpApiError, + `应当 reject QdmpApiError，实际抛出: ${String(err)}`, + ); + const apiErr = err as QdmpApiError; + assert.ok( + apiErr.message.length < 1000, + `code 字段是数组时，QdmpApiError.message 长度应当有界，实际长度: ${apiErr.message.length}`, + ); + assert.ok( + typeof apiErr.code === 'string' || typeof apiErr.code === 'number', + `QdmpApiError.code 应当是 string 或 number（对畸形 code 归一化为一个安全的哨兵值），实际: ${JSON.stringify(apiErr.code)}`, + ); +}); diff --git a/node/test/errors-message-sanitization.test.ts b/node/test/errors-message-sanitization.test.ts new file mode 100644 index 0000000..0641579 --- /dev/null +++ b/node/test/errors-message-sanitization.test.ts @@ -0,0 +1,85 @@ +/** + * QdmpApiError message/requestId 安全处理测试。 + * + * 需求：服务端返回的业务失败信封里的 message 字段会被 QdmpApiError 原样拼进 + * Error.message（见 src/errors.ts 约32-35行的 `super(...)` 调用），如果服务端 + * （或中间代理）返回的 message 里嵌入了 \r\n 等控制字符，一旦调用方把 + * error.message 原样写进单行日志，就可能伪造出看似独立的新日志行（日志注入）。 + * 同理，一个异常超长的 message 会被原样塞进 Error.message，缺乏长度上限。 + * + * 因此新要求： + * 1. QdmpApiError.message（含 toString()）中不得包含服务端 message 原始的 + * \r / \n 字符； + * 2. QdmpApiError.message 的长度必须有上限（不是无限透传服务端的任意长度 + * 字符串）。 + * + * 现状（本测试写下时）：QdmpApiError 构造函数把 init.message 未经任何转义/ + * 截断地拼进 `super(...)`。因此下面两个测试预期都是失败的（红色状态）。 + */ +import assert from 'node:assert/strict'; +import {test} from 'node:test'; + +import {QdmpClient} from '../src/client.js'; +import {QdmpApiError} from '../src/errors.js'; +import { + businessFailure, + createMockAgent, + expectFailure, +} from './helpers/mock-http.js'; + +function makeClient( + mockAgent: ReturnType['mockAgent'], +) { + return new QdmpClient({ + appId: 'app-id-1', + appSecret: 'app-secret-1', + dispatcher: mockAgent, + }); +} + +void test('QdmpApiError: message 中嵌入的 \\r\\n 不得原样出现在 error.message / toString() 中（防日志注入）', async () => { + const injectedMessage = 'line1\r\nFAKE injected log line'; + const {mockAgent, pool} = createMockAgent(); + pool + .intercept({path: '/auth/v1/token', method: 'POST'}) + .reply(200, businessFailure('40001', injectedMessage)); + + const client = makeClient(mockAgent); + const err = await expectFailure(() => client.auth.getAccessToken()); + + assert.ok( + err instanceof QdmpApiError, + `应当 reject QdmpApiError，实际抛出: ${String(err)}`, + ); + const apiErr = err as QdmpApiError; + const rendered = `${apiErr.message}\n${apiErr.toString()}`; + assert.ok( + !rendered.includes('\r'), + `QdmpApiError 的 message/toString() 不应包含原始 \\r 字符，实际: ${JSON.stringify(rendered)}`, + ); + assert.ok( + !rendered.includes('FAKE injected log line') || !rendered.includes('\n'), + 'QdmpApiError 不应让服务端 message 里的换行符原样保留，从而伪造出独立的新日志行', + ); +}); + +void test('QdmpApiError: 服务端返回超长 message（约20万字符）时，error.message 的长度必须有界（不能无限透传）', async () => { + const hugeMessage = 'A'.repeat(200_000); + const {mockAgent, pool} = createMockAgent(); + pool + .intercept({path: '/auth/v1/token', method: 'POST'}) + .reply(200, businessFailure('40002', hugeMessage)); + + const client = makeClient(mockAgent); + const err = await expectFailure(() => client.auth.getAccessToken()); + + assert.ok( + err instanceof QdmpApiError, + `应当 reject QdmpApiError，实际抛出: ${String(err)}`, + ); + const apiErr = err as QdmpApiError; + assert.ok( + apiErr.message.length < 5000, + `QdmpApiError.message 长度应当有界（预期远小于原始 20 万字符），实际长度: ${apiErr.message.length}`, + ); +}); diff --git a/node/test/http-response-size-limit.test.ts b/node/test/http-response-size-limit.test.ts new file mode 100644 index 0000000..f4aa8ce --- /dev/null +++ b/node/test/http-response-size-limit.test.ts @@ -0,0 +1,42 @@ +/** + * HttpClient.request() 响应体大小上限测试。 + * + * 需求：一个恶意或异常的服务端/中间代理返回一个体积极大的响应体时， + * HttpClient.request() 不应无限制地把整个 body 读进内存并 JSON.parse—— + * 这会给调用方进程带来内存耗尽（OOM）风险。响应体大小应当有一个合理的上限 + * （预期实现量级在 10MB 左右），超过上限时应当 reject，而不是成功解析出一个 + * 巨大的对象。 + * + * 本测试用一个约 11MB 的字符串字段构造响应体，安全超出预期的 10MB 量级， + * 同时又不至于让测试本身跑得很慢/占用过多内存。 + * + * 现状（本测试写下时）：HttpClient.request()（src/http.ts 约149-158行）对 + * `response.json()` 的响应体大小没有任何限制，因此这个测试预期是失败的 + * （红色状态）：当前实现会成功 resolve 出这个 11MB 的巨大对象，而不是 reject。 + */ +import {test} from 'node:test'; + +import {QdmpClient} from '../src/client.js'; +import { + businessSuccess, + createMockAgent, + expectFailure, +} from './helpers/mock-http.js'; + +void test('HttpClient.request: 响应体约 11MB 时应 reject，而不是成功解析出巨大的 JSON 对象', async () => { + const oversizedValue = 'x'.repeat(11 * 1024 * 1024); + const {mockAgent, pool} = createMockAgent(); + pool + .intercept({path: '/user/v1/me', method: 'GET'}) + .reply(200, businessSuccess({id: oversizedValue, nickname: 'oversized'})); + + const client = new QdmpClient({ + appId: 'app-id-1', + appSecret: 'app-secret-1', + dispatcher: mockAgent, + }); + + await expectFailure(() => + client.user.me({accessToken: 'valid-looking-access-token'}), + ); +}); diff --git a/shared/scripts/regenerate-all.sh b/shared/scripts/regenerate-all.sh index 0914de7..8cd1eb0 100755 --- a/shared/scripts/regenerate-all.sh +++ b/shared/scripts/regenerate-all.sh @@ -4,10 +4,12 @@ # .github/workflows/generate-check.yml to detect drift (re-run this, then # `git diff --exit-code` the generated paths). # -# Requires on PATH: node, go, oapi-codegen (go install -# github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen@latest), and java -# (for the openapi-generator-cli jar, downloaded on demand below if -# OPENAPI_GENERATOR_CLI_JAR isn't already set to an existing jar path). +# Requires on PATH: node, go, and java (for the openapi-generator-cli jar, +# downloaded and SHA-256-verified on demand below if OPENAPI_GENERATOR_CLI_JAR +# isn't already set to an existing jar path). oapi-codegen does NOT need to be +# preinstalled: it's invoked via `go run @` below, so +# the version is pinned in this file and Go verifies the module's checksum +# against sum.golang.org on first fetch. set -euo pipefail REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" @@ -20,12 +22,45 @@ echo "==> node: openapi-typescript + route-meta/error-codes mirror" (cd node && npx openapi-typescript ../shared/openapi.yaml -o src/generated/schema.d.ts && node scripts/gen-route-meta.mjs) echo "==> java: openapi-generator-cli (models only) + route-meta/error-codes mirror" -OPENAPI_GENERATOR_CLI_JAR="${OPENAPI_GENERATOR_CLI_JAR:-/tmp/tools/openapi-generator-cli.jar}" -if [ ! -f "$OPENAPI_GENERATOR_CLI_JAR" ]; then - echo " downloading openapi-generator-cli 7.16.0 to $OPENAPI_GENERATOR_CLI_JAR" - mkdir -p "$(dirname "$OPENAPI_GENERATOR_CLI_JAR")" - curl -sSL -o "$OPENAPI_GENERATOR_CLI_JAR" \ - https://repo1.maven.org/maven2/org/openapitools/openapi-generator-cli/7.16.0/openapi-generator-cli-7.16.0.jar +OPENAPI_GENERATOR_CLI_VERSION="7.16.0" +# Verified against the artifact's published Maven Central SHA-1 as of writing; +# pinning our own SHA-256 here since Maven Central doesn't publish one. +OPENAPI_GENERATOR_CLI_SHA256="6999b18cece5b58f5d5b246fef5a43bdf61239491c4f0ed6513214e0f6e8464b" + +verify_openapi_generator_cli_jar() { + local jar="$1" + [ "$(shasum -a 256 "$jar" | awk '{print $1}')" = "$OPENAPI_GENERATOR_CLI_SHA256" ] +} + +if [ -n "${OPENAPI_GENERATOR_CLI_JAR:-}" ]; then + # Caller explicitly pointed us at a jar (a local mirror, or a different + # generator version) -- trust it as given, the same as this SDK's other + # explicit-override conventions. We have no known-good hash for an + # arbitrary caller-chosen jar. Fail now, before the `rm -rf + # java/generated/src` below, if a stale/typo'd override points at nothing + # -- otherwise this turns a recoverable "tool not found" into "generated + # sources deleted, then the build fails". + if [ ! -f "$OPENAPI_GENERATOR_CLI_JAR" ]; then + echo "OPENAPI_GENERATOR_CLI_JAR=$OPENAPI_GENERATOR_CLI_JAR does not exist" >&2 + exit 1 + fi + echo " using caller-supplied OPENAPI_GENERATOR_CLI_JAR=$OPENAPI_GENERATOR_CLI_JAR (not checksum-verified)" +else + OPENAPI_GENERATOR_CLI_JAR="/tmp/tools/openapi-generator-cli-${OPENAPI_GENERATOR_CLI_VERSION}.jar" + if [ -f "$OPENAPI_GENERATOR_CLI_JAR" ] && ! verify_openapi_generator_cli_jar "$OPENAPI_GENERATOR_CLI_JAR"; then + echo " cached $OPENAPI_GENERATOR_CLI_JAR failed SHA-256 verification, discarding and re-downloading" + rm -f "$OPENAPI_GENERATOR_CLI_JAR" + fi + if [ ! -f "$OPENAPI_GENERATOR_CLI_JAR" ]; then + echo " downloading openapi-generator-cli $OPENAPI_GENERATOR_CLI_VERSION to $OPENAPI_GENERATOR_CLI_JAR" + mkdir -p "$(dirname "$OPENAPI_GENERATOR_CLI_JAR")" + curl --fail -sSL -o "$OPENAPI_GENERATOR_CLI_JAR" \ + "https://repo1.maven.org/maven2/org/openapitools/openapi-generator-cli/${OPENAPI_GENERATOR_CLI_VERSION}/openapi-generator-cli-${OPENAPI_GENERATOR_CLI_VERSION}.jar" + fi + if ! verify_openapi_generator_cli_jar "$OPENAPI_GENERATOR_CLI_JAR"; then + echo "openapi-generator-cli-${OPENAPI_GENERATOR_CLI_VERSION}.jar failed SHA-256 verification (expected $OPENAPI_GENERATOR_CLI_SHA256), refusing to run it" >&2 + exit 1 + fi fi rm -rf java/generated/src java -jar "$OPENAPI_GENERATOR_CLI_JAR" generate \ @@ -36,7 +71,8 @@ java -jar "$OPENAPI_GENERATOR_CLI_JAR" generate \ --additional-properties=hideGenerationTimestamp=true,dateLibrary=java8,openApiNullable=false node java/scripts/gen-route-meta.mjs -echo "==> go: oapi-codegen (models only)" -oapi-codegen -config go/generated/oapi-codegen-config.yaml shared/openapi.yaml +echo "==> go: oapi-codegen (models only, pinned to v2.8.0)" +go run github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen@v2.8.0 \ + -config go/generated/oapi-codegen-config.yaml shared/openapi.yaml echo "==> done"