node: storage program read RPCs, /health, rate-limit headers, ACL hardening - #797
Merged
tcsenpai merged 14 commits intoMay 6, 2026
Merged
Conversation
… fix, capability detection plan
Bundles testnet's storage-program-read PR work, adapted to stabilisation's
modular handler-registry architecture. This commit lands:
- 9 read RPC handlers for the StorageProgram subsystem:
getStorageProgram, getStorageProgramAll, getStorageProgramFields,
getStorageProgramFieldType, getStorageProgramItem,
getStorageProgramValue, getStorageProgramsByOwner,
hasStorageProgramField, searchStoragePrograms
- getTransactionStatus RPC for transaction lifecycle tracking
(pending / included / unknown), with sha256-hex regex validation
and case-insensitive normalised lookup
- shared module storageProgramShared.ts with the validate-resolve-
require-jsonObject envelope and a withFieldRead higher-order helper
that dedupes the simple field-read handlers (getValue, getFieldType)
- 404 dispatcher fix: registry-miss path now returns HTTP 404 with a
structured {error, message} body instead of HTTP 200 with a
Python-dict-shaped string. SDK clients can now detect unsupported
methods.
- SDK capability detection implementation plan in history/planning/
ACL semantics are preserved: each handler enforces checkReadPermission
via the shared getAccessibleProgram resolver, with anonymous callers
limited to public programs.
…ites
Registers the 10 new RPC handlers in the modular dispatch registry, adds
the Chain / Mempool / sharedState building blocks they (and the upcoming
/health endpoint) depend on:
- handlers/storageProgramHandlers.ts: registers the 9 storage RPCs
- handlers/transactionHandlers.ts: adds getTransactionStatus
- handlers/index.ts: spreads storageProgramHandlers into the registry
- blockchain/chainTypes.ts: TxStatus discriminated type
- blockchain/chainTransactions.ts: getTransactionStatus(hash) — checks
mempool first, then transactions table, returns pending/included/
unknown
- blockchain/chain.ts: re-exports TxStatus, exposes
Chain.getTransactionStatus on the facade
- blockchain/mempool.ts: count() for cheap mempool size lookups (used
by /health) and findByHash() for case-insensitive hash lookup (used
by getTransactionStatus)
- utilities/sharedState.ts: nodeStartTime + getUptimeSeconds() for the
/health endpoint
…rdening
- GET /health: reports {version, version_name, accepting, mempool_size,
uptime_s}. Returns HTTP 503 when the node is not accepting traffic
(not synced) or when the mempool DB is unreachable, so LB/k8s probes
can detect unhealthy nodes by status code alone. Mempool.count() is
isolated in a try/catch so a transient DB outage doesn't 500 the
probe.
- Rate limiter: /health and /version are exempt from the global rate
limiter middleware so liveness/readiness probes can't be 429-throttled
under load (k8s/ALB hit /health from a single source IP that would
otherwise blow the per-IP quota).
- Rate limiter: getCurrentLimits(ip) exposes the current window state
(limit, remaining, resetEpochSeconds) so server_rpc can surface
X-RateLimit-{Limit,Remaining,Reset} headers on POST / responses. SDK
clients can use these to self-throttle.
- bunServer.jsonResponse: extraHeaders parameter, with Content-Type
forced last so caller-supplied headers can't accidentally override
the JSON content type.
…nymous bypass
Closes the security and correctness defects flagged in PR review:
ACL bypass (security, was P1)
─────────────────────────────
The pattern `!requesterAddress || requesterAddress === owner` treated
anonymous callers (undefined or empty-string requester) as the owner
and skipped checkReadPermission entirely, leaking owner/restricted
programs to unauthenticated callers. This shape was repeated in the
HTTP route handler (listByOwnerHandler) and the new RPC handler. Both
now require a strictly defined non-empty requester for the owner
fast-path; everything else falls through to the SQL ACL filter.
Post-pagination filter (correctness)
────────────────────────────────────
searchStorageProgramsByName paginated at the SQL layer
(take(limit).skip(offset)) and then ACL-filtered the result in JS.
This produced two visible defects:
- short pages: limit=10 could return 3 if 7 of the rows in the SQL
window were restricted
- invisible accessible rows: anything past offset+limit in the SQL
result was never fetched, so subsequent pages couldn't reveal it
Now ACL filtering happens in the WHERE clause via a jsonb predicate
that mirrors checkReadPermission exactly:
- public + not blacklisted (anonymous always sees public —
blacklist needs an identity to test)
- owner mode + requester is the owner
- restricted + requester is the owner (overrides blacklist)
- restricted + requester in acl.allowed and not blacklisted
- restricted + requester is a member of a group with read permission
The database returns exactly the requested page of accessible rows.
Performance
───────────
- Owner fast-path: when requesterAddress === owner, skips the jsonb
predicate entirely and uses the existing owner index
(idx_gcr_storageprogram_owner). Same plan as before for the common
owner-lists-own-programs case.
- Anonymous fast-path: predicate collapses to acl->>'mode' = 'public'
— single text comparison, no jsonb_each subquery, no parameter bind.
- Containment uses jsonb @> operators which Postgres can short-circuit
on absent keys. No GIN index added: the query is already gated by
ILIKE / owner / isDeleted predicates, so the ACL predicate runs on
a small post-prune candidate set; a GIN on `acl` would add write
amplification on every storage-program write (a hot path) for no
win on this query.
- JS-side mapping is now strictly less work: only accessible rows are
hydrated through the ORM and run through toStorageProgramListItem.
Updated all four call sites:
- src/libs/network/routines/nodecalls/getStorageProgramsByOwner.ts
- src/libs/network/routines/nodecalls/searchStoragePrograms.ts
- src/libs/network/manageGCRRoutines.ts
- src/features/storageprogram/routes.ts (HTTP, both byOwner and search)
Tests: regression coverage for the previously-broken short-page case,
the owner fast-path, anonymous-only-sees-public, and parameter binding
(SQL-injection safety).
Contributor
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughStorage-program ACL enforcement moved into SQL with new read-side RPC handlers and shared helpers; transaction status lookup API added; health endpoint, uptime tracking, and rate-limit headers implemented; mempool helpers added; and an SDK capability-detection planning doc was introduced. ChangesStorage Program Access Control
Transaction Status Tracking
Health Monitoring & Rate Limiting
SDK Capability Detection Planning (doc-only)
Sequence Diagram(s)sequenceDiagram
participant Client as Client (SDK)
participant RPC as RPC Handler (/ POST)
participant Routine as Node-call Routine
participant Query as QueryBuilder
participant DB as Database
Client->>RPC: call getStorageProgramByOwner(request)
RPC->>Routine: invoke getStorageProgramsByOwner(data)
Routine->>Query: build QueryBuilder with readReachablePredicate(requesterAddress)
Query->>DB: execute SQL (LIMIT/OFFSET)
DB-->>Query: rows
Query-->>Routine: accessible rows
Routine-->>RPC: rpc(200, mappedList)
RPC-->>Client: HTTP response with body + X-RateLimit-* headers
(uses rgba(100,150,240,0.5) style by default in visualization layers) Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Review Summary by QodoStorage program read RPCs, transaction status, health endpoint, and ACL hardening
WalkthroughsDescription• Implements 9 storage-program read RPCs with modular handler registry • Adds getTransactionStatus RPC for transaction lifecycle tracking • Fixes ACL bypass vulnerability in storage program access control • Adds /health endpoint with rate-limit headers and infrastructure • Moves ACL filtering to SQL layer to prevent pagination bugs Diagramflowchart LR
A["9 Storage Program<br/>Read RPCs"] -->|registered via| B["storageProgramHandlers<br/>module"]
C["getTransactionStatus<br/>RPC"] -->|registered via| D["transactionHandlers<br/>module"]
B -->|dispatch to| E["Modular Handler<br/>Registry"]
D -->|dispatch to| E
E -->|routes to| F["RPC Handlers"]
F -->|enforce ACL via| G["SQL Predicate<br/>readReachablePredicate"]
G -->|prevents| H["Post-pagination<br/>filtering bugs"]
I["Unknown Message<br/>Dispatcher"] -->|returns| J["404 with<br/>structured error"]
K["/health Endpoint"] -->|reports| L["version, accepting,<br/>mempool_size, uptime"]
M["Rate Limiter"] -->|exempts| K
M -->|adds headers| N["X-RateLimit-*<br/>headers"]
File Changes1. src/features/storageprogram/routes.ts
|
Contributor
Code Review by Qodo
1.
|
Greptile SummaryThis PR lands 9 storage-program read RPCs, a
Confidence Score: 5/5Safe to merge; the ACL bypass and pagination fixes are well-tested and the new RPC surface is well-structured. The two highest-risk changes — the SQL ACL predicate and the owner/anonymous fast-paths — both have dedicated regression tests that verify the correct branch is taken and that requesterAddress is bound as a parameter rather than interpolated. The only inconsistency found (empty-string requester not coerced in five single-item handlers) has no access-control impact because checkReadPermission treats empty string identically to anonymous. The five standalone single-item handlers (getStorageProgram.ts, getStorageProgramAll.ts, getStorageProgramFields.ts, getStorageProgramItem.ts, hasStorageProgramField.ts) apply a looser requester-address guard than the list handlers and withFieldRead. Important Files Changed
Sequence DiagramsequenceDiagram
participant SDK as SDK Client
participant RL as RateLimiter
participant RPC as server_rpc (POST /)
participant MNC as manageNodeCall
participant HR as handlerRegistry
participant SP as storageProgramHandlers
participant GCR as GCRStorageProgramRoutines
participant DB as PostgreSQL
SDK->>RL: POST / {message, data}
RL-->>RL: /health or /version? bypass
RL-->>RL: increment IP counter
RPC->>MNC: processPayload(payload, sender)
MNC->>HR: handlerRegistry[message]
HR->>SP: storageProgramHandlers[message](data)
SP->>GCR: getStorageProgramsByOwner / searchStorageProgramsByName
GCR->>DB: SELECT WHERE owner AND isDeleted=false AND ACL predicate LIMIT OFFSET
DB-->>GCR: rows
GCR-->>SP: GCRStorageProgram[]
SP-->>MNC: RPCResponse
MNC-->>RPC: RPCResponse
RPC-->>RL: getCurrentLimits(clientIP)
RL-->>RPC: {limit, remaining, resetEpochSeconds}
RPC-->>SDK: 200 + X-RateLimit-* headers
SDK->>RL: GET /health
RL-->>SDK: bypass (no rate limit)
RPC->>RPC: getSharedState (accepting, version)
RPC->>DB: Mempool.count()
DB-->>RPC: mempoolSize
RPC-->>SDK: 200/503 {version, accepting, mempool_size, uptime_s}
Reviews (3): Last reviewed commit: "node: use own-property check in withFiel..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/libs/network/routines/nodecalls/storageProgramShared.ts (2)
57-63: 💤 Low valueInconsistent shape vs the other
rpc*helpers.
rpcBadRequestandrpcPermissionDeniedboth put the human message inerrorand the machine code inerrorCode.rpcInternalErrorinstead sets both to"INTERNAL_ERROR"and stuffs the actual message intoextra. SDK clients reading the standard(error, errorCode)envelope from this helper geterror === errorCode, which loses the original throw context.Also, the non-
Errorbranch loses information entirely by collapsing to"Unknown error"—String(error)would preserve at least the throwable's coerced form.If the intent is to deliberately not leak internal details to the client, that's defensible — but in that case the shape divergence is worth a
// intentional: don't leak server-side detailscomment so a reader doesn't "fix" it.♻️ Suggested fix if leak-prevention isn't the intent
export function rpcInternalError(error: unknown): RPCResponse { + const message = + error instanceof Error ? error.message : String(error) return rpc( 500, - { error: "INTERNAL_ERROR", errorCode: "INTERNAL_ERROR" }, - `Error: ${error instanceof Error ? error.message : "Unknown error"}`, + { error: message, errorCode: "INTERNAL_ERROR" }, ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libs/network/routines/nodecalls/storageProgramShared.ts` around lines 57 - 63, rpcInternalError currently returns a different shape than rpcBadRequest/rpcPermissionDenied (it sets both error and errorCode to "INTERNAL_ERROR" and puts the real message into the extra field) and it collapses non-Error throwables to "Unknown error"; update rpcInternalError to follow the same envelope as the other helpers by returning error: <human message> and errorCode: "INTERNAL_ERROR" (use the actual message for the human message), convert non-Error values with String(error) to preserve information, and if you intentionally want to avoid leaking details add an explicit comment like "// intentional: don't leak server-side details" above rpcInternalError; reference the rpcInternalError function and the rpc(...) call/ RPCResponse shape when making the change.
247-255: ⚡ Quick winEmpty-string
requesterAddressis not normalized toundefinedhere either.If a caller passes
data.requesterAddress = "", the typeof guard lets it through and it propagates intogetAccessibleProgram→checkReadPermission(program, ""). For most ACL modes the result matches anonymous behavior, but forpublicmode theif (requesterAddress && acl.blacklisted?.includes(...))check is skipped because""is falsy — same data-visibility but a different code path than a true anonymous caller.This mirrors the exact inconsistency flagged in
src/features/storageprogram/routes.ts. A length check keeps the contract uniform across the RPC and HTTP surfaces:♻️ Suggested normalization
const requesterAddress = - typeof data?.requesterAddress === "string" + typeof data?.requesterAddress === "string" && + data.requesterAddress.length > 0 ? data.requesterAddress : undefined🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libs/network/routines/nodecalls/storageProgramShared.ts` around lines 247 - 255, The requesterAddress string is accepted even when empty, causing inconsistent behavior vs anonymous callers; update the normalization where requesterAddress is set in storageProgramShared (the block assigning requesterAddress from data.requesterAddress) to treat empty strings as undefined (e.g., only accept when typeof data.requesterAddress === "string" AND data.requesterAddress.length > 0) so that downstream functions like getAccessibleProgram and checkReadPermission receive undefined for anonymous callers rather than an empty string.tests/storageprogram/routines.test.ts (1)
1095-1150: 💤 Low valueLGTM — solid coverage of the three branches, with one small test rigor opportunity.
The non-owner test at Lines 1133-1150 is the important one for SQL-injection safety since it asserts the requester is bound as a parameter (not concatenated).
Minor: the anonymous test at Lines 1116-1131 only asserts
'public'is present in someandWherecall. Since the non-anonymous predicate also contains'public', this assertion would pass for non-anonymous too. The sibling search test at Lines 1183-1202 already uses the stricter "includes 'public' AND NOT includes :requesterAddress" pattern — the same shape would tighten this test:♻️ Suggested test rigor improvement
- // Anonymous gets the public-only branch, no requesterAddress param. - expect(findAndWhereCall(qb, "'public'")).toBeDefined() + // Anonymous gets the public-only branch, no requesterAddress param. + const hasPublicOnly = qb.andWhere.mock.calls.some( + (c: unknown[]) => + typeof c[0] === "string" && + (c[0] as string).includes("'public'") && + !(c[0] as string).includes(":requesterAddress"), + ) + expect(hasPublicOnly).toBe(true)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/storageprogram/routines.test.ts` around lines 1095 - 1150, The anonymous-branch test for getStorageProgramsByOwner should be tightened to ensure the ACL predicate is *public-only* (no requester parameter); update the anonymous test that uses makeMockQueryBuilder()/repo.createQueryBuilder and findAndWhereCall to assert the found andWhere contains "'public'" AND that findAndWhereCall(qb, ":requesterAddress") is undefined (or that its params do not include requesterAddress) so the test fails if the non-anonymous predicate is used; modify only the assertions in that test (keep use of repo, qb, and findAndWhereCall).src/libs/blockchain/gcr/gcr_routines/GCRStorageProgramRoutines.ts (1)
941-979: 💤 Low valueLGTM — predicate is parameterized and logic mirrors
checkReadPermission.The five ACL branches correspond 1:1 with the JS implementation, including owner-overrides-blacklist in restricted mode and "no blacklist for anonymous public". The use of
to_jsonb(:requesterAddress::text)with TypeORM's named-param binding keeps this safe against SQL injection regardless of caller-supplied requester strings, which is what the regression test attests/storageprogram/routines.test.ts:1133-1150exercises.One minor note:
aliasis interpolated directly into the SQL. It is only ever called with the default"sp"today, but if a future caller passes user-derived input it would be unsafe. Worth either aprivateinvariant comment, or asserting the alias against a small allow-list before interpolation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libs/blockchain/gcr/gcr_routines/GCRStorageProgramRoutines.ts` around lines 941 - 979, The SQL builds use the alias parameter in readReachablePredicate (alias) which is interpolated directly into the query and could be unsafe if a caller ever supplies user-derived input; fix by constraining/sanitizing alias before interpolation (e.g., assert alias is one of an allow-list like 'sp' (or a small set you expect) or validate it against a safe identifier regex /^[A-Za-z_][A-Za-z0-9_]*$/ and throw if it fails), and add a short private-invariant comment above readReachablePredicate documenting that alias must be validated/not come from user input; implement the check at the top of readReachablePredicate and keep the rest of the SQL generation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/storageprogram/routes.ts`:
- Around line 632-657: The parsing that maps an empty identity post-colon
segment (e.g. "ed25519:") to undefined is implemented in listByOwnerHandler but
missing in getRequesterAddress and searchByNameHandler; update
getRequesterAddress to mirror the same logic (split on ":" and return splits[1]
only if non-empty, otherwise undefined) and modify searchByNameHandler to call
getRequesterAddress (or reuse the same normalization logic) instead of inlining
splits[1] so that requesterAddress becomes undefined for empty segments and the
SQL ACL anonymous branch is used consistently (refer to functions
getRequesterAddress, listByOwnerHandler, and searchByNameHandler).
In `@src/libs/network/routines/nodecalls/getStorageProgramItem.ts`:
- Around line 53-55: The check "field in obj" in getStorageProgramItem.ts can
return true for inherited properties; change the presence test to an
own-property check (e.g., use Object.prototype.hasOwnProperty.call(obj, field)
or Object.hasOwn(obj, field)) when evaluating obj and field before returning
rpc(404, ...). Update the conditional that currently reads "if (!(field in
obj))" to use the own-property test so only actual JSON storage keys trigger
existence, leaving the rest of the logic (obj, field, rpc(404,...)) unchanged.
In `@src/libs/network/routines/nodecalls/getStorageProgramsByOwner.ts`:
- Around line 56-65: The current code in getStorageProgramsByOwner fetches the
entire ACL-filtered owner set then paginates in memory; change the call to push
pagination into the DB by updating
GCRStorageProgramRoutines.getStorageProgramsByOwner to accept limit and offset
(e.g., add parameters limit, offset) and apply .take(limit).skip(offset) /
equivalent in its query, then call it from this routine passing the existing
repository, requesterAddress and the limit/offset values so the repository-level
query returns only the paginated results which you then map with
toStorageProgramListItem.
In `@src/libs/network/routines/nodecalls/hasStorageProgramField.ts`:
- Around line 50-52: The existence check uses the `in` operator which matches
inherited keys; update the logic in hasStorageProgramField (the block computing
`exists`) to use an own-property check instead: keep the `isJsonObject` guard
and replace `field in (program.data as Record<string, unknown>)` with a safe
hasOwnProperty call such as `Object.prototype.hasOwnProperty.call(program.data,
field)` (or equivalent) to avoid false positives from prototype properties.
---
Nitpick comments:
In `@src/libs/blockchain/gcr/gcr_routines/GCRStorageProgramRoutines.ts`:
- Around line 941-979: The SQL builds use the alias parameter in
readReachablePredicate (alias) which is interpolated directly into the query and
could be unsafe if a caller ever supplies user-derived input; fix by
constraining/sanitizing alias before interpolation (e.g., assert alias is one of
an allow-list like 'sp' (or a small set you expect) or validate it against a
safe identifier regex /^[A-Za-z_][A-Za-z0-9_]*$/ and throw if it fails), and add
a short private-invariant comment above readReachablePredicate documenting that
alias must be validated/not come from user input; implement the check at the top
of readReachablePredicate and keep the rest of the SQL generation unchanged.
In `@src/libs/network/routines/nodecalls/storageProgramShared.ts`:
- Around line 57-63: rpcInternalError currently returns a different shape than
rpcBadRequest/rpcPermissionDenied (it sets both error and errorCode to
"INTERNAL_ERROR" and puts the real message into the extra field) and it
collapses non-Error throwables to "Unknown error"; update rpcInternalError to
follow the same envelope as the other helpers by returning error: <human
message> and errorCode: "INTERNAL_ERROR" (use the actual message for the human
message), convert non-Error values with String(error) to preserve information,
and if you intentionally want to avoid leaking details add an explicit comment
like "// intentional: don't leak server-side details" above rpcInternalError;
reference the rpcInternalError function and the rpc(...) call/ RPCResponse shape
when making the change.
- Around line 247-255: The requesterAddress string is accepted even when empty,
causing inconsistent behavior vs anonymous callers; update the normalization
where requesterAddress is set in storageProgramShared (the block assigning
requesterAddress from data.requesterAddress) to treat empty strings as undefined
(e.g., only accept when typeof data.requesterAddress === "string" AND
data.requesterAddress.length > 0) so that downstream functions like
getAccessibleProgram and checkReadPermission receive undefined for anonymous
callers rather than an empty string.
In `@tests/storageprogram/routines.test.ts`:
- Around line 1095-1150: The anonymous-branch test for getStorageProgramsByOwner
should be tightened to ensure the ACL predicate is *public-only* (no requester
parameter); update the anonymous test that uses
makeMockQueryBuilder()/repo.createQueryBuilder and findAndWhereCall to assert
the found andWhere contains "'public'" AND that findAndWhereCall(qb,
":requesterAddress") is undefined (or that its params do not include
requesterAddress) so the test fails if the non-anonymous predicate is used;
modify only the assertions in that test (keep use of repo, qb, and
findAndWhereCall).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de30184e-db82-4e6c-9d8c-6b6e947f62fb
📒 Files selected for processing (28)
history/planning/sdk-capability-detection.mdsrc/features/storageprogram/routes.tssrc/libs/blockchain/chain.tssrc/libs/blockchain/chainTransactions.tssrc/libs/blockchain/chainTypes.tssrc/libs/blockchain/gcr/gcr_routines/GCRStorageProgramRoutines.tssrc/libs/blockchain/mempool.tssrc/libs/network/bunServer.tssrc/libs/network/handlers/index.tssrc/libs/network/handlers/storageProgramHandlers.tssrc/libs/network/handlers/transactionHandlers.tssrc/libs/network/manageGCRRoutines.tssrc/libs/network/manageNodeCall.tssrc/libs/network/middleware/rateLimiter.tssrc/libs/network/routines/nodecalls/getStorageProgram.tssrc/libs/network/routines/nodecalls/getStorageProgramAll.tssrc/libs/network/routines/nodecalls/getStorageProgramFieldType.tssrc/libs/network/routines/nodecalls/getStorageProgramFields.tssrc/libs/network/routines/nodecalls/getStorageProgramItem.tssrc/libs/network/routines/nodecalls/getStorageProgramValue.tssrc/libs/network/routines/nodecalls/getStorageProgramsByOwner.tssrc/libs/network/routines/nodecalls/getTransactionStatus.tssrc/libs/network/routines/nodecalls/hasStorageProgramField.tssrc/libs/network/routines/nodecalls/searchStoragePrograms.tssrc/libs/network/routines/nodecalls/storageProgramShared.tssrc/libs/network/server_rpc.tssrc/utilities/sharedState.tstests/storageprogram/routines.test.ts
readReachablePredicate's group-membership branch called jsonb_each on sp.acl->'groups' unconditionally. Postgres errors with "cannot call jsonb_each on a non-object" when the operand is null or any non-object jsonb value, which would 500 list/search queries for any restricted program persisted with groups: null (allowed by the schema — StorageProgramACL.groups is optional and checkReadPermission only iterates when truthy). Add a jsonb_typeof guard before the EXISTS clause so the branch only evaluates when groups is an object, mirroring the JS check. Closes myc #47.
`field in obj` matches inherited prototype keys, so a request for
field='toString' or '__proto__' against any program would incorrectly
report the field as present. TypeORM's jsonb columns hydrate to plain
objects with Object.prototype in their chain, so every JS object has
those keys.
Observable effects of the bug:
- hasStorageProgramField returned {field: 'toString', exists: true}
when the user has no such field — confirms a non-existent key
- getStorageProgramItem with field='toString' passed the existence
check, then failed Array.isArray() and returned the wrong error
code (INVALID_FIELD_TYPE 400 instead of FIELD_NOT_FOUND 404)
Replace `field in obj` with Object.prototype.hasOwnProperty.call(obj,
field) in both handlers so only real user-written keys count.
Closes myc #50.
The withFieldRead higher-order helper accepted requesterAddress=""
while the sibling handlers (searchStoragePrograms,
getStorageProgramsByOwner) gate on .length > 0. Real-world ACL impact
is nil — no real program has owner='' or allowed.includes('') — but
the inconsistency is a footgun and could matter if a future ACL branch
treats empty string differently from undefined.
Closes myc #51.
The anonymous test asserted only that some andWhere call contained 'public', but the non-anonymous predicate ALSO contains 'public' (the public-mode + blacklist branch). The assertion would have passed even if the wrong branch ran. Use the same shape the search test already uses: assert that some andWhere call contains 'public' AND does NOT bind :requesterAddress. Closes myc #52.
readReachablePredicate interpolates `alias` directly into the SQL
strings (`${a}.acl->>'mode'`, etc.). The function is private and only
called internally with the default "sp", so there is no current
exploitable surface — but the parameter exists, and a future caller
passing user-derived input would be a SQL-injection vector.
Add a defense-in-depth identifier-shape check at the top of the
function; throw on anything outside `[A-Za-z_][A-Za-z0-9_]*`. The
throw branch is unreachable for current callers.
Closes myc #53.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libs/blockchain/gcr/gcr_routines/GCRStorageProgramRoutines.ts`:
- Around line 941-990: readReachablePredicate currently treats only undefined as
anonymous but checkReadPermission treats an empty string "" as anonymous too;
update the anonymous check in readReachablePredicate (function
readReachablePredicate) to treat "" the same as undefined (e.g., change the
conditional that tests requesterAddress to also consider requesterAddress ===
''), and ensure the anonymous branch still returns the anonymous SQL and empty
params so empty-string callers follow the same ACL path as checkReadPermission.
In `@src/libs/network/routines/nodecalls/storageProgramShared.ts`:
- Around line 268-274: The shared field reader uses the prototype-inclusive
check `field in obj` which can return inherited names; update the check in the
reader (the logic around `const obj = program.data as Record<string, unknown>`
inside withFieldRead/shared field reader) to use an own-property test such as
Object.prototype.hasOwnProperty.call(obj, field) (or Object.hasOwn(obj, field)
if you prefer modern API) and ensure obj is a non-null object before checking so
that inherited properties like toString/__proto__ are not reported as real
storage fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e8414f0-80ef-4639-b5ac-0300162e3fa7
📒 Files selected for processing (5)
src/libs/blockchain/gcr/gcr_routines/GCRStorageProgramRoutines.tssrc/libs/network/routines/nodecalls/getStorageProgramItem.tssrc/libs/network/routines/nodecalls/hasStorageProgramField.tssrc/libs/network/routines/nodecalls/storageProgramShared.tstests/storageprogram/routines.test.ts
Previously the routine fetched the full ACL-filtered owner result set
and the RPC handler sliced in JS. For owners with hundreds or
thousands of programs this materialised the entire set per request,
wasting DB bandwidth and JS heap.
Apply LIMIT/OFFSET at the SQL layer in both the owner fast-path
(repository.find) and the ACL QueryBuilder branch, mirroring the
sibling searchStorageProgramsByName.
Default limit is 200 today (matches the RPC handler's existing clamp),
clamped to [1, 200]. Documented as scheduled to drop to 100 in a
future release — callers that rely on the implicit cap should pass
explicit pagination.
Updated all four call sites:
- src/libs/network/routines/nodecalls/getStorageProgramsByOwner.ts
(drops the JS slice; passes {limit, offset})
- src/features/storageprogram/routes.ts (HTTP listByOwnerHandler)
accepts ?limit=&offset= query params, defaults to 200
- src/libs/network/manageGCRRoutines.ts (legacy gcr_routine path):
accepts an optional params[2] options object for {limit, offset}
The existing owner-fast-path test still passes because
expect.objectContaining doesn't require exact key match. Test mock
helper now mocks take/skip unconditionally so anonymous/non-owner
tests don't trip on the new SQL pagination calls.
Closes myc #48.
`getRequesterAddress` returned the post-colon segment unconditionally,
so an `identity: "ed25519:"` header (empty after the prefix) yielded
`""` instead of `undefined`. Two route handlers (listByOwnerHandler
and searchByNameHandler) re-implemented the parse inline without that
guard, and only listByOwnerHandler had previously been patched
locally.
Audit of all 7 callers shows the result feeds into
`getAccessibleProgram` -> `checkReadPermission`, which treats `""`
falsy in every branch:
- public mode: blacklist check is gated on `requesterAddress &&`
- owner mode: `requesterAddress === program.owner` (empty never
matches a real owner)
- restricted mode: early-returns on `if (!requesterAddress)`
So returning undefined for an empty post-colon segment is
behavior-preserving for every existing caller while bringing the
helper in line with the SQL ACL contract elsewhere.
Also drop the inline parses in both handlers and reuse the helper.
Closes myc #49.
…nDenied
The previous shape collapsed both `error` and `errorCode` to the
literal "INTERNAL_ERROR" and stuffed the actual exception message into
the `extra` field. SDK clients reading the standard {error, errorCode}
envelope thus saw error === errorCode and lost the original throw
context — and the SDK's `StorageProgramResponse` type only declares
`error?` and `errorCode?`, never `extra`, so the message was
effectively dead data.
Bring the helper in line with the other rpc* helpers: `error` carries
the human message, `errorCode` carries the stable machine identifier.
For non-Error throws fall back to `String(error)` instead of "Unknown
error" so plain objects/numbers/null still produce useful diagnostics.
This is a wire-shape change for 500 responses but every consumer of
the helpers reads the standardised envelope, so existing failure paths
that were ignoring `extra` start surfacing the real message in the
expected slot.
Closes myc #54.
…te boundary The SQL ACL primitive strict-equalled `undefined` for the anonymous branch, but `checkReadPermission` treats `""` as anonymous in every branch via falsy checks. All current callers already gate on `.length > 0` before passing through, so the divergence isn't reachable today — but the function should be self-protecting against future callers, matching the same defense-in-depth rationale used for the alias regex check. Add a one-line normalization at the top of the function and bind the normalized value as the SQL parameter. Closes myc #58.
#50 fixed the prototype-matching bug in the standalone handlers (getStorageProgramItem, hasStorageProgramField) but missed the withFieldRead higher-order helper, which propagated the same bug to the two handlers built on it: getStorageProgramValue and getStorageProgramFieldType. Effect of the original bug: - getStorageProgramValue with field='toString' returned the Object.prototype.toString function reference - getStorageProgramFieldType with field='toString' returned {type: 'object'} both confirming a non-existent user field exists. Replace `field in obj` with Object.prototype.hasOwnProperty.call — same fix as #50. Closes myc #59.
|
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces #796 (closed — wrong base branch).
Lands the storage-program read RPC family,
/healthendpoint, rate-limitclient signal, and the security/correctness review fixes from PR #796 —
all adapted to stabilisation's modular handler-registry architecture.
What's in this PR
Features
getStorageProgram,getStorageProgramAll,getStorageProgramFields,getStorageProgramFieldType,getStorageProgramItem,getStorageProgramValue,getStorageProgramsByOwner,hasStorageProgramField,searchStoragePrograms) registered via a newstorageProgramHandlersmodulein the modular dispatch registry.
getTransactionStatusRPC for transaction lifecycle tracking (pending /included / unknown), with sha256-hex regex validation and case-insensitive
normalised lookup.
GET /healthendpoint reporting{version, version_name, accepting, mempool_size, uptime_s}. Returns HTTP 503 when the node isn't acceptingtraffic or the mempool DB is unreachable, so LB/k8s probes can detect
unhealthy nodes by status code alone.
X-RateLimit-{Limit,Remaining,Reset}headers on POST/responses soSDK clients can self-throttle.
/healthand/versionexempt from the global rate limiter so probescan't be 429-throttled under load.
Fixes (security / correctness)
{error, message}instead of HTTP 200 with a Python-dict-shaped string. SDK clients can now
detect unsupported methods.
!requesterAddress || ... === ownerpatterntreated anonymous (undefined or empty-string) callers as the owner and
leaked owner/restricted programs. Fixed in both the new RPC handler and
the older HTTP route handler.
searchStorageProgramsByNamepaginated atthe SQL layer then JS-filtered, producing short pages and hidden
accessible rows. Now uses a jsonb WHERE predicate that mirrors
checkReadPermissionexactly, so LIMIT/OFFSET produce full pages.Hardening
jsonResponseacceptsextraHeadersand forces Content-Type last socaller-supplied headers can't override the JSON content type.
errorobject as a separate argumentinstead of string-concatenating, preserving non-Error throws as
diagnostic data instead of
[object Object].Performance — SQL ACL filter design notes
The post-pagination ACL fix is the only place where the perf surface
materially changed. Highlights:
requesterAddress === ownerskips the jsonbpredicate entirely, uses
idx_gcr_storageprogram_owner. Same plan asbefore for the common owner-lists-own-programs case.
acl->>'mode' = 'public'— single text comparison, no jsonb_each subquery, no parameter bind.
acl: the query is already gated by ILIKE /owner / isDeleted, so the predicate runs on a small post-prune candidate
set. Adding a GIN would slow every storage-program write (a hot path)
for no win on this query.
through the ORM and mapped through
toStorageProgramListItem.Verification
result.program!non-null-assertion warnings (project-wide style)
tsc --noEmiton changed files: clean — pre-existing errors inchainBlocks.ts,chainTransactions.ts, FHE/ZK tests are notintroduced by this work (verified by stash-and-recheck)
Tests
Regression tests added for:
repo.findnot QueryBuilder):requesterAddressparameter)History
Originally targeted
testnetas PR #796. Closed and reopened againststabilisationper direction that stabilisation will replace testnet.The original testnet branch (
claude/checkout-branches-WQCIg) ispreserved at commit
8c9f6628for history.Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests