From f167335c9013d3b128cd486039a872432387e93c Mon Sep 17 00:00:00 2001 From: shitikyan Date: Wed, 15 Jul 2026 17:44:36 +0400 Subject: [PATCH] fix(storage): close the read-ACL bypass on the HTTP + gcr_routine surfaces too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #970 fixed the manageNodeCall read path, but the same plaintext storage data is reachable via two other mounted, public surfaces that still trusted an unsigned, caller-supplied requester — so the High-severity bypass stayed open: 1. HTTP routes (`src/features/storageprogram/routes.ts`, mounted at server_rpc.ts). `getRequesterAddress` read the raw `identity` header and passed it straight to the ACL. The auth middleware only verifies a signature when BOTH `identity` and `signature` headers are present; a caller who sends `identity` alone naming an allowlisted address was let through and granted access. Now the requester is the key the middleware VERIFIED for the request (`getAuthContext(req).publicKey`) — an unsigned header sets no context and reads as anonymous. 2. `gcr_routine` (`manageGCRRoutines.ts`, cases getStorageProgram / getStorageProgramsByOwner / searchStoragePrograms). These took the requester from `params[1]`/`params[2]` — a caller-supplied value. Now they derive it from the verified `sender` the RPC layer already authenticated; anonymous (empty sender) resolves to undefined, so restricted programs stay hidden. Both surfaces already had a verified identity available (unlike the nodecall path, which is why #970 needed a per-request signed envelope) — the bug was that they ignored it in favour of an unsigned value. Tests (jest, run in-band): - readauth-surfaces.test.ts (new): getRequesterAddress ignores a raw identity header, returns only the middleware-verified key — on the REAL auth context. - gcrroutine-readauth.test.ts (new): the ACL requester is the verified sender, never the params value; empty sender reads anonymous. - routes.test.ts: the owner-mode test now authenticates via the verified context, plus a new "unverified raw identity header -> 403" bypass case; full suite green (25). SDK follow-up (same as #970): storage-read clients must authenticate the request (identity + signature headers) rather than pass a requesterAddress arg. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/features/storageprogram/routes.ts | 18 +++-- src/libs/network/manageGCRRoutines.ts | 14 +++- .../gcrroutine-readauth.test.ts | 76 +++++++++++++++++++ .../storageprogram/readauth-surfaces.test.ts | 67 ++++++++++++++++ tests/storageprogram/routes.test.ts | 41 +++++++++- 5 files changed, 201 insertions(+), 15 deletions(-) create mode 100644 tests/storageprogram/gcrroutine-readauth.test.ts create mode 100644 tests/storageprogram/readauth-surfaces.test.ts diff --git a/src/features/storageprogram/routes.ts b/src/features/storageprogram/routes.ts index f853dee63..5ac3f1eee 100644 --- a/src/features/storageprogram/routes.ts +++ b/src/features/storageprogram/routes.ts @@ -18,6 +18,7 @@ import log from "@/utilities/logger" import Datasource from "@/model/datasource" import { GCRStorageProgram } from "@/model/entities/GCRv2/GCR_StorageProgram" import { GCRStorageProgramRoutines } from "@/libs/blockchain/gcr/gcr_routines/GCRStorageProgramRoutines" +import { getAuthContext } from "@/libs/network/authContext" // ============================================================================ // Response Types @@ -114,14 +115,15 @@ interface StorageProgramGranularResponse { * behaviour with the SQL ACL filter and other call sites that distinguish * `""` from `undefined`. */ -function getRequesterAddress(req: Request): string | undefined { - const identity = req.headers.get("identity") - if (!identity || identity.length === 0) { - return undefined - } - const splits = identity.split(":") - const candidate = splits.length > 1 ? splits[1] : identity - return candidate && candidate.length > 0 ? candidate : undefined +// Exported for tests. +export function getRequesterAddress(req: Request): string | undefined { + // The requester is the key the auth middleware VERIFIED for this request — + // never the raw `identity` header. The middleware only populates the auth + // context after checking the request signature (and 401s on a bad one), so + // a caller who merely names an allowlisted address in the header, with no + // signature, has no auth context and reads as anonymous. Trusting the raw + // header here was the storage read-ACL bypass. + return getAuthContext(req).publicKey ?? undefined } function getValueType( diff --git a/src/libs/network/manageGCRRoutines.ts b/src/libs/network/manageGCRRoutines.ts index 20ed9d84e..d83634d4c 100644 --- a/src/libs/network/manageGCRRoutines.ts +++ b/src/libs/network/manageGCRRoutines.ts @@ -28,6 +28,14 @@ export default async function manageGCRRoutines( // Handle the payload const { method, params } = payload + // The requester for every storage-program read ACL check is the key the RPC + // layer VERIFIED for this call (`sender`), never a caller-supplied params + // value: naming an allowlisted address without proving key ownership was the + // storage read-ACL bypass. Empty sender (anonymous) resolves to undefined, + // so restricted programs stay hidden unless the caller signed the request. + const verifiedRequester = + typeof sender === "string" && sender.length > 0 ? sender : undefined + switch (method) { // SECTION XM Identity Management @@ -290,7 +298,7 @@ export default async function manageGCRRoutines( // REVIEW: Get storage program by address case "getStorageProgram": { const storageAddress = params[0] - const requesterAddress = params[1] // Optional identity for ACL check + const requesterAddress = verifiedRequester // verified signer, not caller-supplied if (!storageAddress) { response.result = 400 @@ -362,7 +370,7 @@ export default async function manageGCRRoutines( // REVIEW: Get storage programs by owner case "getStorageProgramsByOwner": { const owner = params[0] - const requesterAddress = params[1] // Optional identity for ACL filtering + const requesterAddress = verifiedRequester // verified signer, not caller-supplied const options = params[2] || {} // Optional { limit, offset } if (!owner) { @@ -425,7 +433,7 @@ export default async function manageGCRRoutines( case "searchStoragePrograms": { const query = params[0] const options = params[1] || {} // { limit, offset, exactMatch } - const requesterAddress = params[2] // Optional identity for ACL filtering + const requesterAddress = verifiedRequester // verified signer, not caller-supplied if (!query || (typeof query === "string" && query.trim() === "")) { response.result = 400 diff --git a/tests/storageprogram/gcrroutine-readauth.test.ts b/tests/storageprogram/gcrroutine-readauth.test.ts new file mode 100644 index 000000000..d17c74bcf --- /dev/null +++ b/tests/storageprogram/gcrroutine-readauth.test.ts @@ -0,0 +1,76 @@ +import { describe, expect, it, jest, beforeAll } from "@jest/globals" + +// `gcr_routine` storage reads must ACL-filter on the VERIFIED `sender` the RPC +// layer passed in — never a caller-supplied `params` value. Trivial stubs for +// the dispatcher's unrelated import chain; the storage routines are spied so we +// can assert exactly which requester reached the SQL filter. +jest.mock("@/utilities/logger", () => ({ + __esModule: true, + default: { info: jest.fn(), debug: jest.fn(), warning: jest.fn(), error: jest.fn() }, +})) +jest.mock("@kynesyslabs/demosdk/types", () => ({ __esModule: true })) +jest.mock("@/libs/network/server_rpc", () => ({ __esModule: true, emptyResponse: { result: 200, response: null, require_reply: false, extra: null } })) +jest.mock("@/libs/blockchain/gcr/gcr_routines/identityManager", () => ({ __esModule: true, default: {} })) +jest.mock("@/libs/blockchain/gcr/gcr_routines/IncentiveManager", () => ({ __esModule: true, IncentiveManager: {} })) +jest.mock("@/libs/blockchain/gcr/gcr_routines/ensureGCRForUser", () => ({ __esModule: true, default: {} })) +jest.mock("@/features/incentive/referrals", () => ({ __esModule: true, Referrals: {} })) +jest.mock("@/libs/blockchain/gcr/gcr", () => ({ __esModule: true, default: {} })) +jest.mock("@/libs/identity/providers/nomisIdentityProvider", () => ({ __esModule: true, NomisIdentityProvider: {} })) +jest.mock("@/libs/identity/tools/humanpassport", () => ({ __esModule: true, default: {} })) +jest.mock("@/libs/identity/providers/ethosIdentityProvider", () => ({ __esModule: true, EthosIdentityProvider: {} })) +jest.mock("@/libs/communications/broadcastManager", () => ({ __esModule: true, BroadcastManager: {} })) +jest.mock("@/model/entities/GCRv2/GCR_StorageProgram", () => ({ __esModule: true, GCRStorageProgram: class {} })) +jest.mock("@/model/datasource", () => ({ + __esModule: true, + default: { getInstance: async () => ({ getDataSource: () => ({ getRepository: () => ({}) }) }) }, +})) + +const listSpy = jest.fn(async () => []) +const searchSpy = jest.fn(async () => []) +jest.mock("@/libs/blockchain/gcr/gcr_routines/GCRStorageProgramRoutines", () => ({ + __esModule: true, + GCRStorageProgramRoutines: { + getStorageProgramsByOwner: listSpy, + searchStorageProgramsByName: searchSpy, + }, +})) + +let manageGCRRoutines: typeof import("@/libs/network/manageGCRRoutines")["default"] +const VERIFIED = "aa".repeat(32) +const ATTACKER = "bb".repeat(32) // an allowlisted address the caller merely names + +beforeAll(async () => { + ;({ default: manageGCRRoutines } = await import("@/libs/network/manageGCRRoutines")) +}) + +describe("gcr_routine storage reads: ACL requester = verified sender", () => { + it("getStorageProgramsByOwner filters on the verified sender, not params", async () => { + await manageGCRRoutines(VERIFIED, { + method: "getStorageProgramsByOwner", + params: ["owner-x", ATTACKER, {}], + } as never) + // 3rd arg to the routine is the requesterAddress that hits the SQL ACL. + expect(listSpy).toHaveBeenCalled() + expect(listSpy.mock.calls[0][2]).toBe(VERIFIED) + expect(listSpy.mock.calls[0][2]).not.toBe(ATTACKER) + }) + + it("searchStoragePrograms filters on the verified sender, not params", async () => { + await manageGCRRoutines(VERIFIED, { + method: "searchStoragePrograms", + params: ["query", {}, ATTACKER], + } as never) + expect(searchSpy).toHaveBeenCalled() + const opts = searchSpy.mock.calls[0][2] as { requesterAddress?: string } + expect(opts.requesterAddress).toBe(VERIFIED) + expect(opts.requesterAddress).not.toBe(ATTACKER) + }) + + it("an anonymous (empty) sender reads as undefined even if params name an address", async () => { + await manageGCRRoutines("", { + method: "getStorageProgramsByOwner", + params: ["owner-x", ATTACKER, {}], + } as never) + expect(listSpy.mock.calls.at(-1)?.[2]).toBeUndefined() + }) +}) diff --git a/tests/storageprogram/readauth-surfaces.test.ts b/tests/storageprogram/readauth-surfaces.test.ts new file mode 100644 index 000000000..efdbcd319 --- /dev/null +++ b/tests/storageprogram/readauth-surfaces.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, it, jest } from "@jest/globals" + +// The two read surfaces #970 did not cover — the HTTP route layer and the +// `gcr_routine` dispatcher — must derive the ACL requester from a VERIFIED +// identity, never a caller-supplied value. These lock that in. +// +// Mock only the heavy/side-effectful imports so the modules load without a DB +// or SDK. authContext is left REAL: the whole point is that the HTTP handler +// reads the middleware-verified context, so the test drives that real context. +jest.mock("@/utilities/logger", () => ({ + __esModule: true, + default: { info: jest.fn(), debug: jest.fn(), warning: jest.fn(), error: jest.fn() }, +})) +jest.mock("@kynesyslabs/demosdk", () => ({ __esModule: true, types: {}, storage: {} })) +jest.mock("@/model/datasource", () => ({ __esModule: true, default: { getInstance: jest.fn() } })) +jest.mock("@/libs/network/bunServer", () => ({ + __esModule: true, + jsonResponse: (body: unknown, status = 200) => ({ body, status }), +})) + +let getRequesterAddress: typeof import("@/features/storageprogram/routes")["getRequesterAddress"] +let setAuthContext: typeof import("@/libs/network/authContext")["setAuthContext"] + +const KEY = "aa".repeat(32) // raw public-key hex — the ACL address form + +beforeAll(async () => { + ;({ getRequesterAddress } = await import("@/features/storageprogram/routes")) + ;({ setAuthContext } = await import("@/libs/network/authContext")) +}) + +function reqNamingAllowed(): Request { + // A caller that NAMES an allowlisted address in the raw `identity` header, + // with no signature — the exact bypass attempt. + return new Request("http://node/storage-program/stor-" + "0".repeat(40), { + headers: { identity: `ed25519:${KEY}` }, + }) +} + +describe("HTTP read surface: getRequesterAddress", () => { + it("ignores the raw identity header — an unsigned caller is anonymous", () => { + // No auth context set (middleware never verified a signature), so even + // though the header names an allowed address, the requester is anonymous. + expect(getRequesterAddress(reqNamingAllowed())).toBeUndefined() + }) + + it("returns the requester only from the middleware-verified context", () => { + const req = reqNamingAllowed() + setAuthContext(req, { + verified: true, + identity: `ed25519:${KEY}`, + publicKey: KEY, + algorithm: "ed25519", + }) + expect(getRequesterAddress(req)).toBe(KEY) + }) + + it("stays anonymous when the context carries no public key", () => { + const req = reqNamingAllowed() + setAuthContext(req, { + verified: false, + identity: null, + publicKey: null, + algorithm: null, + }) + expect(getRequesterAddress(req)).toBeUndefined() + }) +}) diff --git a/tests/storageprogram/routes.test.ts b/tests/storageprogram/routes.test.ts index a34de2f6c..b705aa969 100644 --- a/tests/storageprogram/routes.test.ts +++ b/tests/storageprogram/routes.test.ts @@ -1,6 +1,7 @@ import { beforeAll, beforeEach, describe, expect, it, jest } from "@jest/globals" import { readFileSync } from "fs" import path from "path" +import { setAuthContext } from "@/libs/network/authContext" // Mock logger jest.mock("@/utilities/logger", () => ({ @@ -119,22 +120,37 @@ beforeEach(() => { function makeRequest( urlPath: string, headers: Record = {}, + authenticatedAs?: string, ): Request { - return new Request(`http://localhost:53550${urlPath}`, { + const req = new Request(`http://localhost:53550${urlPath}`, { headers: new Headers(headers), }) + if (authenticatedAs) { + // Simulate the auth middleware after a VERIFIED signature: only then is + // the verified key placed in the request's auth context. A raw + // `identity` header on its own sets nothing and grants nothing — that + // was the read-ACL bypass. + setAuthContext(req, { + verified: true, + identity: `ed25519:${authenticatedAs}`, + publicKey: authenticatedAs, + algorithm: "ed25519", + }) + } + return req } async function callRoute( pattern: string, urlPath: string, headers: Record = {}, + authenticatedAs?: string, ): Promise<{ status: number; body: any }> { const handler = capturedRoutes.get(pattern) if (!handler) { throw new Error(`Route not found: ${pattern}. Available: ${Array.from(capturedRoutes.keys()).join(", ")}`) } - const response = await handler(makeRequest(urlPath, headers)) + const response = await handler(makeRequest(urlPath, headers, authenticatedAs)) const body = await response.json() return { status: response.status, body } } @@ -214,7 +230,7 @@ describe("StorageProgram HTTP Routes", () => { expect(body.errorCode).toBe("PERMISSION_DENIED") }) - it("200: owner-mode program with owner identity", async () => { + it("200: owner-mode program with a verified owner", async () => { mockRepository.findOneBy.mockResolvedValue( entityFixtures.owner_json_program, ) @@ -222,11 +238,28 @@ describe("StorageProgram HTTP Routes", () => { const { status, body } = await callRoute( "/storage-program/*", "/storage-program/stor-owner-only-789", - { identity: "owner_addr_1" }, + {}, + "owner_addr_1", // verified by the middleware, not a raw header ) expect(status).toBe(200) expect(body.success).toBe(true) }) + + it("403: owner-mode named via an unverified raw identity header (bypass closed)", async () => { + mockRepository.findOneBy.mockResolvedValue( + entityFixtures.owner_json_program, + ) + + // The old bypass: name the owner in the `identity` header, no + // signature. With no verified auth context it is anonymous → 403. + const { status, body } = await callRoute( + "/storage-program/*", + "/storage-program/stor-owner-only-789", + { identity: "owner_addr_1" }, + ) + expect(status).toBe(403) + expect(body.errorCode).toBe("PERMISSION_DENIED") + }) }) // =========================================================================