Skip to content

Harden REST API input validation - #2374

Open
a-shannon wants to merge 14 commits into
ergoplatform:masterfrom
a-shannon:fix/rest-api-input-hardening
Open

a-shannon wants to merge 14 commits into
ergoplatform:masterfrom
a-shannon:fix/rest-api-input-hardening

Conversation

@a-shannon

@a-shannon a-shannon commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Malformed or wrong-length UTXO IDs now return BadRequest. Seed length, fee-estimation parameters, public batch sizes and unconfirmed-transaction paging/hash parameters are checked before expensive processing or mempool scans.

Review

Head 9b3027038bf9588da21a28eb7a84cc1ee152b82c preserves all six API-owning files from 7386692905aea78d883350054b9e900368cfe585 unchanged.

The earlier head 73866929 merged master 5528ef569a41ebccbc8658212e6ee3c97d990b96. Its six-file review increment preserves all three original API production files byte-for-byte. Its shared hash decoder also preserves master's byte-length rejection, and the exact upstream 17-line invalid-length regression is retained alongside the original cases.

Review the added shared prerequisites separately.

Shared prerequisites and integration order

The branch reuses the funded transaction fixtures 7c753910, convergence helper H 0f9f75c1, selected-header observer U bcfcd611, settled-seed/isolation fixture D 71f33c6e, and synchronizer reader fixture R 76e5ec4b, owned by #2535. Continuation/download correction G d62c945a remains owned by #2511; header/body cache wake-up X a772a5cb remains owned by #2549.

Integrate the required shared/production increments before this API correction; H precedes U/D and R precedes G. The whole #2535 branch is not a prerequisite of #2511. GitHub's upstream-master diff includes the cumulative prerequisites until they merge. The observer checks each node's selected header instead of combining retained alternatives; convergence assertions, mining targets and existing deadlines remain intact.

Validation

Normal Test and IntegrationTest compilation passed on the final tree, with 82/82 focused cases: 49 synchronization/history/cache cases and 33 convergence/isolation cases.

Historical validation on 73866929 passed UtxoApiRouteSpec (11), UtilsApiRouteSpec (11) and TransactionApiRouteSpec (27), totaling 49/49. The subsequent funded-fixture composition passed 132/132, including those 49 API cases and 83 shared-fixture cases, because API stubs consume the funded transaction generator. Its six API files and unit-fixture inputs are unchanged in this final composition; the 132 cases were not rerun as part of the new 82-case gate. Independent reviews found no composition blockers.

The shared liveness composition also passed native CI at pinned #2535 head efbd1e917fe74f0f680a2e3c9e4d93595a59b06a (run 34734047921). That is supporting composition evidence, not this PR's exact-head CI or proof of an exclusive cause for historical integration failures. The local gate did not run Docker. Native CI for the updated head is pending; follow the integration tracker.

#2398 independently adds authentication to the binary-proof endpoint. Shared files do not create a merge prerequisite here; an eventual conflict resolution must retain both that authentication and this PR's batch bound.

@kushti

kushti commented Jun 11, 2026

Copy link
Copy Markdown
Member

"Enforce API-key protection on script compile/evaluation routes and UTXO binary proof generation where OpenAPI already declares ApiKeyAuth" - oh, maybe it is better to relax openapi.yaml then ? Not sure those methods should be protected (as no any sensitive results can appear in API calls output)

@a-shannon
a-shannon force-pushed the fix/rest-api-input-hardening branch from 98a2a25 to 0408853 Compare June 12, 2026 11:15
@a-shannon
a-shannon force-pushed the fix/rest-api-input-hardening branch from 0408853 to 72ad036 Compare June 12, 2026 16:35
@a-shannon

Copy link
Copy Markdown
Contributor Author

Agreed. I removed that auth change from this PR. #2374 is now scoped to input-bound / overflow hardening only: NiPoPoW params, tx/utxo/utils validation, and related tests.\n\nI split the OpenAPI/auth mismatch into separate optional PRs so it can be decided independently: #2385 for script routes and #2398 for UTXO binary proofs. If the preferred direction is to keep those endpoints public, those PRs can be closed and OpenAPI relaxed instead.

@a-shannon

Copy link
Copy Markdown
Contributor Author

Follow-up: I also removed the NiPoPoW overflow change from #2374 so the NiPoPoW parameter invariant is owned only by the focused #2379. #2374 is now limited to tx/utxo/utils REST input bounds. Checks after the cleanup: git diff --check; sbt testOnly org.ergoplatform.http.routes.TransactionApiRouteSpec org.ergoplatform.http.routes.UtilsApiRouteSpec org.ergoplatform.http.routes.UtxoApiRouteSpec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants