From 460044ae3cce9bd2d66623800566624c645c4fab Mon Sep 17 00:00:00 2001 From: zzz Date: Mon, 9 Nov 2026 22:15:44 +0800 Subject: [PATCH] fix(sdk): reject control characters in application-relative paths --- sdk/go/client.go | 4 +- sdk/go/client_test.go | 97 ++++++++++++++++++++++++++++ sdk/python/src/fastcas/client.py | 2 +- sdk/python/tests/test_store.py | 75 ++++++++++++++++++++- sdk/typescript/src/browser.ts | 4 +- sdk/typescript/src/server.ts | 2 +- sdk/typescript/test/browser.test.mjs | 73 ++++++++++++++++++++- 7 files changed, 249 insertions(+), 8 deletions(-) create mode 100644 sdk/go/client_test.go diff --git a/sdk/go/client.go b/sdk/go/client.go index d9f7511..80caea0 100644 --- a/sdk/go/client.go +++ b/sdk/go/client.go @@ -241,7 +241,9 @@ func (c *Client) begin(ctx context.Context, purpose string, options BeginOptions if path == "" { path = "/" } - if !strings.HasPrefix(path, "/") || strings.HasPrefix(path, "//") || strings.ContainsAny(path, "\\\r\n") { + if !strings.HasPrefix(path, "/") || strings.HasPrefix(path, "//") || strings.ContainsFunc(path, func(character rune) bool { + return character == '\\' || character < 0x20 || character == 0x7f + }) { return "", errors.New("application-relative return path required") } p, _, err := c.discovery(ctx) diff --git a/sdk/go/client_test.go b/sdk/go/client_test.go new file mode 100644 index 0000000..d902f4f --- /dev/null +++ b/sdk/go/client_test.go @@ -0,0 +1,97 @@ +package fastcas + +import ( + "context" + "fmt" + "io" + "net/http" + "strings" + "testing" +) + +type pathTransactions struct{ values []Transaction } + +func (store *pathTransactions) Put(_ context.Context, transaction Transaction) error { + store.values = append(store.values, transaction) + return nil +} + +func (store *pathTransactions) Take(_ context.Context, _ string) (*Transaction, error) { + return nil, nil +} + +type pathTransport struct{ requests int } + +func (transport *pathTransport) RoundTrip(request *http.Request) (*http.Response, error) { + transport.requests++ + body := `{"id":"intent","nonce":"nonce"}` + switch request.URL.Path { + case "/.well-known/openid-configuration": + body = `{"issuer":"https://auth.example","authorization_endpoint":"https://auth.example/authorize","token_endpoint":"https://auth.example/token","jwks_uri":"https://auth.example/jwks","userinfo_endpoint":"https://auth.example/userinfo","id_token_signing_alg_values_supported":["RS256"]}` + case "/api/v1/link-intents": + default: + return nil, fmt.Errorf("unexpected request: %s", request.URL) + } + return &http.Response{StatusCode: http.StatusOK, Header: http.Header{"Content-Type": {"application/json"}}, Body: io.NopCloser(strings.NewReader(body)), Request: request}, nil +} + +func TestBeginReturnPaths(test *testing.T) { + invalidPaths := []string{"https://attacker.test", "//attacker.test", "/\\attacker.test"} + for character := rune(0); character <= 127; character++ { + if character >= 32 && character != 127 { + continue + } + invalidPaths = append(invalidPaths, "/"+string(character)+"/attacker.test", "/projects?query="+string(character), "/projects#"+string(character)) + } + validPaths := []string{"", "/", "/projects?sort=recent&tag=a#details", "/项目/😀?q=中文#片段", "/encoded/%00%09%0A%0D%1F%7F%2F%5C%E4%B8%AD", "/unicode/\u0085\u2028"} + for _, purpose := range []string{"login", "link", "register"} { + test.Run(purpose, func(test *testing.T) { + store := &pathTransactions{} + transport := &pathTransport{} + client, err := New(Config{Issuer: "https://auth.example", ClientID: "app", ClientSecret: "test-secret", RedirectURI: "https://app.example/callback", HTTPClient: &http.Client{Transport: transport}}, store) + if err != nil { + test.Fatal(err) + } + begin := func(path string) (string, error) { + options := BeginOptions{BrowserBinding: strings.Repeat("b", 32), ReturnTo: path, LocalAccountRef: "local", LocalSessionID: "session"} + switch purpose { + case "link": + return client.BeginLink(context.Background(), options) + case "register": + return client.BeginRegistration(context.Background(), "new-local", options) + default: + return client.BeginLogin(context.Background(), options) + } + } + for _, path := range invalidPaths { + test.Run(fmt.Sprintf("reject/%q", path), func(test *testing.T) { + transactionsBefore, requestsBefore := len(store.values), transport.requests + if _, err := begin(path); err == nil || err.Error() != "application-relative return path required" { + test.Errorf("expected return path validation error for %q, got %v", path, err) + } + if len(store.values) != transactionsBefore || transport.requests != requestsBefore { + test.Error("invalid return path reached HTTP or transaction storage") + } + }) + } + for _, path := range validPaths { + test.Run(fmt.Sprintf("accept/%q", path), func(test *testing.T) { + authorization, err := begin(path) + if err != nil { + test.Fatal(err) + } + if !strings.HasPrefix(authorization, "https://auth.example/authorize?") { + test.Fatalf("unexpected authorization URL: %s", authorization) + } + want := path + if want == "" { + want = "/" + } + if got := store.values[len(store.values)-1].ReturnTo; got != want { + test.Errorf("return path = %q, want %q", got, want) + } + }) + } + }) + } +} diff --git a/sdk/python/src/fastcas/client.py b/sdk/python/src/fastcas/client.py index 3567a9b..015c7a7 100644 --- a/sdk/python/src/fastcas/client.py +++ b/sdk/python/src/fastcas/client.py @@ -146,7 +146,7 @@ def begin_link(self, browser_binding: str, *, local_account_ref: str, local_sess def _begin(self, purpose, binding, return_to, scopes, local_ref="", session="") -> str: if len(binding) < 32: raise FastCASError("binding_required", "Fresh browser binding required") - if not return_to.startswith("/") or return_to.startswith("//") or any(c in return_to for c in "\\\r\n"): + if not return_to.startswith("/") or return_to.startswith("//") or "\\" in return_to or any(ord(character) < 32 or ord(character) == 127 for character in return_to): raise FastCASError("invalid_return_to", "Application-relative return path required") endpoint = self.discovery()["authorization_endpoint"] tx = dict(state=secrets.token_urlsafe(32), nonce=secrets.token_urlsafe(32), verifier=secrets.token_urlsafe(32), binding_hash=_hash(binding), purpose=purpose, return_to=return_to, expires_at=time.time()+300) diff --git a/sdk/python/tests/test_store.py b/sdk/python/tests/test_store.py index 3690486..7c433b2 100644 --- a/sdk/python/tests/test_store.py +++ b/sdk/python/tests/test_store.py @@ -1,10 +1,26 @@ +import asyncio import concurrent.futures import tempfile import time import unittest from pathlib import Path +from unittest.mock import Mock -from fastcas import Configuration, FastCAS, FastCASError, SQLiteTransactionStore +import httpx + +from fastcas import AsyncFastCAS, Configuration, FastCAS, FastCASError, SQLiteTransactionStore + + +CONTROLS = [chr(code) for code in [*range(32), 127]] +INVALID_PATHS = ["https://attacker.test", "//attacker.test", "/\\attacker.test", *[ + path for control in CONTROLS for path in (f"/{control}/attacker.test", f"/projects?query={control}", f"/projects#{control}") +]] +VALID_PATHS = ["/", "/projects?sort=recent&tag=a#details", "/项目/😀?q=中文#片段", "/encoded/%00%09%0A%0D%1F%7F%2F%5C%E4%B8%AD", "/unicode/\u0085\u2028"] +BEGIN_METHODS = [ + ("begin_login", {}), + ("begin_link", dict(local_account_ref="local", local_session_id="session")), + ("begin_registration", dict(new_local_account_ref="new-local")), +] class StoreTests(unittest.TestCase): @@ -23,5 +39,62 @@ def test_insecure_remote_configuration_rejected(self): FastCAS(Configuration("http://remote.test", "app", "http://localhost/callback", allow_loopback_http=True), SQLiteTransactionStore(Path(directory)/"transactions.sqlite")) +class ReturnPathTests(unittest.TestCase): + def setUp(self): + self.store = Mock() + self.requests = [] + + def respond(request): + self.requests.append(request) + if request.url.path == "/.well-known/openid-configuration": + return httpx.Response(200, json=dict( + issuer="https://auth.example", authorization_endpoint="https://auth.example/authorize", + token_endpoint="https://auth.example/token", jwks_uri="https://auth.example/jwks", + userinfo_endpoint="https://auth.example/userinfo", + )) + self.assertEqual(request.url.path, "/api/v1/link-intents") + return httpx.Response(200, json=dict(id="intent", nonce="nonce")) + + self.http = httpx.Client(transport=httpx.MockTransport(respond)) + self.addCleanup(self.http.close) + self.config = Configuration("https://auth.example", "app", "https://app.example/callback", client_secret="test-secret") + self.client = FastCAS(self.config, self.store, http=self.http) + + def test_rejects_ascii_controls_before_http_or_storage(self): + for method, options in BEGIN_METHODS: + for path in INVALID_PATHS: + with self.subTest(method=method, path=repr(path)): + with self.assertRaises(FastCASError) as raised: + getattr(self.client, method)("b" * 32, return_to=path, **options) + self.assertEqual(raised.exception.code, "invalid_return_to") + self.store.put.assert_not_called() + self.assertEqual(self.requests, []) + + def test_preserves_valid_paths_and_default(self): + for method, options in BEGIN_METHODS: + for path in VALID_PATHS: + with self.subTest(method=method, path=path): + authorization = getattr(self.client, method)("b" * 32, return_to=path, **options) + self.assertTrue(authorization.startswith("https://auth.example/authorize?")) + self.assertEqual(self.store.put.call_args.args[0]["return_to"], path) + getattr(self.client, method)("b" * 32, **options) + self.assertEqual(self.store.put.call_args.args[0]["return_to"], "/") + + def test_async_facade_rejects_ascii_controls(self): + client = AsyncFastCAS(self.config, self.store, http=self.http) + + async def exercise(): + for method, options in BEGIN_METHODS: + for path in INVALID_PATHS: + with self.subTest(method=method, path=repr(path)): + with self.assertRaises(FastCASError) as raised: + await getattr(client, method)("b" * 32, return_to=path, **options) + self.assertEqual(raised.exception.code, "invalid_return_to") + + asyncio.run(exercise()) + self.store.put.assert_not_called() + self.assertEqual(self.requests, []) + + if __name__ == "__main__": unittest.main() diff --git a/sdk/typescript/src/browser.ts b/sdk/typescript/src/browser.ts index e130a24..b83e40b 100644 --- a/sdk/typescript/src/browser.ts +++ b/sdk/typescript/src/browser.ts @@ -1,11 +1,11 @@ /** Browser helpers never hold provider credentials or upstream refresh tokens. */ export function loginURL(path = "/api/auth/fastcas/login", returnTo = "/"): string { - if (!path.startsWith("/") || path.startsWith("//") || /[\\\r\n]/.test(path) || !returnTo.startsWith("/") || returnTo.startsWith("//") || /[\\\r\n]/.test(returnTo)) throw new Error("Application-relative paths required"); + if (!path.startsWith("/") || path.startsWith("//") || /[\\\x00-\x1f\x7f]/.test(path) || !returnTo.startsWith("/") || returnTo.startsWith("//") || /[\\\x00-\x1f\x7f]/.test(returnTo)) throw new Error("Application-relative paths required"); const separator = path.includes("?") ? "&" : "?"; return `${path}${separator}returnTo=${encodeURIComponent(returnTo)}`; } export async function accountStatus(path = "/api/auth/fastcas/status", signal?: AbortSignal): Promise { - if (!path.startsWith("/") || path.startsWith("//") || /[\\\r\n]/.test(path)) throw new Error("Application-relative path required"); + if (!path.startsWith("/") || path.startsWith("//") || /[\\\x00-\x1f\x7f]/.test(path)) throw new Error("Application-relative path required"); const response = await fetch(path, { credentials: "same-origin", signal, headers: { accept: "application/json" } }); if (!response.ok) throw new Error("Unable to load account connection status"); return response.json() as Promise; diff --git a/sdk/typescript/src/server.ts b/sdk/typescript/src/server.ts index 5dd4044..252eae3 100644 --- a/sdk/typescript/src/server.ts +++ b/sdk/typescript/src/server.ts @@ -37,7 +37,7 @@ function basicComponent(value: string): string { return new URLSearchParams({ va function equal(left: string, right: string): boolean { const a = Buffer.from(left); const b = Buffer.from(right); return a.length === b.length && timingSafeEqual(a, b); } function safeReturnTo(value?: string): string { if (!value) return "/"; - if (!value.startsWith("/") || value.startsWith("//") || /[\\\r\n]/.test(value)) throw new FastCASError("invalid_return_to", "Use an application-relative return path"); + if (!value.startsWith("/") || value.startsWith("//") || /[\\\x00-\x1f\x7f]/.test(value)) throw new FastCASError("invalid_return_to", "Use an application-relative return path"); return value; } function checkURL(value: string, insecure: boolean): URL { diff --git a/sdk/typescript/test/browser.test.mjs b/sdk/typescript/test/browser.test.mjs index 6f46849..cce7632 100644 --- a/sdk/typescript/test/browser.test.mjs +++ b/sdk/typescript/test/browser.test.mjs @@ -1,7 +1,13 @@ import test from "node:test"; import assert from "node:assert/strict"; -import { loginURL } from "../dist/browser.js"; -import { FastCAS } from "../dist/server.js"; +import { loginURL, accountStatus } from "../dist/browser.js"; +import { FastCAS, FastCASError } from "../dist/server.js"; + +const controls = [...Array.from({ length: 32 }, (_, code) => String.fromCharCode(code)), "\x7f"]; +const invalidPaths = ["https://attacker.test", "//attacker.test", "/\\attacker.test", ...controls.flatMap(control => [ + `/${control}/attacker.test`, `/projects?query=${control}`, `/projects#${control}`, +])]; +const validPaths = ["/", "/projects?sort=recent&tag=a#details", "/项目/😀?q=中文#片段", "/encoded/%00%09%0A%0D%1F%7F%2F%5C%E4%B8%AD", "/unicode/\u0085\u2028"]; test("browser redirects remain application relative", () => { assert.equal(loginURL(undefined, "/projects/one"), "/api/auth/fastcas/login?returnTo=%2Fprojects%2Fone"); @@ -12,3 +18,66 @@ test("configuration does not accept non-loopback insecure issuers", () => { assert.throws(() => new FastCAS({ issuer: "http://attacker.example", clientId: "app", redirectUri: "http://127.0.0.1/callback", allowLoopbackHTTP: true }, store)); assert.throws(() => new FastCAS({ issuer: "https://auth.example/", clientId: "app", redirectUri: "https://app.example/callback" }, store)); }); + +test("loginURL rejects ASCII controls in the login path", () => { + for (const path of invalidPaths) assert.throws(() => loginURL(path), /Application-relative paths required/, JSON.stringify(path)); +}); + +test("loginURL rejects ASCII controls in returnTo", () => { + for (const path of invalidPaths) assert.throws(() => loginURL(undefined, path), /Application-relative paths required/, JSON.stringify(path)); +}); + +test("accountStatus rejects ASCII controls before fetching", async context => { + const request = context.mock.method(globalThis, "fetch", async () => Response.json({ connected: true })); + for (const path of invalidPaths) await assert.rejects(accountStatus(path), /Application-relative path required/, JSON.stringify(path)); + assert.equal(request.mock.callCount(), 0); +}); + +test("browser helpers preserve valid paths and defaults", async context => { + const request = context.mock.method(globalThis, "fetch", async () => Response.json({ connected: true })); + for (const path of validPaths) { + assert.equal(loginURL(path), `${path}${path.includes("?") ? "&" : "?"}returnTo=%2F`); + assert.equal(new URL(loginURL(undefined, path), "https://app.test").searchParams.get("returnTo"), path); + assert.deepEqual(await accountStatus(path), { connected: true }); + assert.equal(request.mock.calls.at(-1).arguments[0], path); + } + assert.equal(loginURL(), "/api/auth/fastcas/login?returnTo=%2F"); + await accountStatus(); + assert.equal(request.mock.calls.at(-1).arguments[0], "/api/auth/fastcas/status"); +}); + +function serverFixture() { + const transactions = []; + const requests = []; + const issuer = "https://auth.example"; + const client = new FastCAS({ issuer, clientId: "app", clientSecret: "test-secret", redirectUri: "https://app.example/callback", fetch: async (url, options) => { + requests.push({ url: String(url), method: options?.method }); + if (String(url) === `${issuer}/.well-known/openid-configuration`) return Response.json({ + issuer, authorization_endpoint: `${issuer}/authorize`, token_endpoint: `${issuer}/token`, + userinfo_endpoint: `${issuer}/userinfo`, jwks_uri: `${issuer}/jwks`, response_types_supported: ["code"], + subject_types_supported: ["public"], id_token_signing_alg_values_supported: ["RS256"], + }); + assert.equal(String(url), `${issuer}/api/v1/link-intents`); + return Response.json({ id: "intent", nonce: "nonce" }); + } }, { async put(transaction) { transactions.push(transaction); }, async take() {} }); + return { client, transactions, requests }; +} + +for (const method of ["beginLogin", "beginLink", "beginRegistration"]) { + const options = { browserBinding: "b".repeat(32), localAccountRef: "local", localSessionId: "session", newLocalAccountRef: "new-local" }; + test(`${method} rejects ASCII controls without storing a transaction or creating an intent`, async () => { + const { client, transactions, requests } = serverFixture(); + for (const returnTo of invalidPaths) await assert.rejects(client[method]({ ...options, returnTo }), error => error instanceof FastCASError && error.code === "invalid_return_to", JSON.stringify(returnTo)); + assert.equal(transactions.length, 0); + assert.ok(requests.every(request => request.url.endsWith("/.well-known/openid-configuration"))); + }); + + test(`${method} preserves valid returnTo values and defaults`, async () => { + const { client, transactions } = serverFixture(); + for (const returnTo of [...validPaths, undefined, ""]) { + const authorization = await client[method]({ ...options, returnTo }); + assert.equal(authorization.origin, "https://auth.example"); + assert.equal(transactions.at(-1).returnTo, returnTo || "/"); + } + }); +}