Fix/cr 1.4.2 - #19
Open
SlavaSereb wants to merge 61 commits into
Open
Conversation
Add security middleware with configurable settings via environment variables: - CORS: Cross-origin resource sharing with configurable allowed origins - Rate limiting: Configurable window and max requests (default: 100/15min) - Body size limits: Configurable max request body size (default: 1mb) - Optional API key authentication: When enabled, requires X-API-Key header All settings are optional with sensible defaults. Public endpoints (/health, /api-docs, /docs) bypass authentication when enabled. New files: - src/middleware/security.ts: Security middleware implementation Modified files: - src/server.ts: Apply security middleware after express.json() - .env.example: Document new security environment variables - package.json: Add cors and express-rate-limit dependencies Resolves: #1
Add bounds validation to prevent loss-of-funds from excessive fees: - MIN_PROTOCOL_FEE: 155,381 lovelace (Cardano protocol minFeeB) - MAX_REASONABLE_FEE: 10,000,000 lovelace (10 ADA) The 10 ADA upper bound is sufficient for even complex multi-asset transactions while protecting against accidental high fee values that could drain funds. Applied to all schemas accepting optional fee parameter: - delegateToDRepRequestSchema - registerAsDRepRequestSchema - castVoteRequestSchema Uses a reusable optionalFeeSchema for consistency.
Add support for overriding Cardano protocol parameters at SDK initialization
to handle hard fork changes without requiring an SDK update.
New ProtocolParams interface with configurable values:
- minFeeA: Fee coefficient (lovelace per tx byte)
- minFeeB: Fee constant (base lovelace fee)
- coinsPerUtxoByte: For min-ada calculations
- stakeKeyDeposit: Stake key registration deposit
- drepDeposit: DRep registration deposit
Usage:
const sdk = await FireblocksCardanoRawSDK.createInstance({
...config,
protocolParams: { minFeeA: 44, minFeeB: 155381 }
});
New files:
- src/utils/protocolParams.ts: Protocol params singleton with get/set/reset
Modified files:
- src/types/config.ts: Add ProtocolParams interface
- src/FireblocksCardanoRawSDK.ts: Accept protocolParams in createInstance
- src/api/validation.ts: Use configurable minFeeB for fee validation
- src/utils/index.ts: Export protocolParams functions
- .env.example: Document protocol param environment variables
Previously, fetchUtxos silently returned empty arrays when: - Network errors occurred (catch block) - API returned success=false This masked real errors, making debugging difficult and causing downstream code to incorrectly assume the address had no UTXOs. Now throws SdkApiError with: - Descriptive error message including the address - Error type: UTXO_FETCH_ERROR - Service: iagon-api Empty UTXO results from a successful API call (address has no UTXOs) still return [] as this is a valid state.
Add documentation about the in-memory UTXO locking mechanism and its limitations for multi-replica deployments. Key points documented: - UTXO locks are process-local (in-memory) - Multi-replica deployments may experience double-spend attempts - Mitigation strategies: sticky sessions, external locking, single replica New section "Deployment Considerations" added before Security Considerations with code example showing automatic UTXO locking behavior.
Previously, staking and governance operations did not use the UTXO locking mechanism, allowing concurrent operations on the same vault to potentially select the same UTXO (double-spend risk). Changes: - UtxoProvider.findAddressWithSuitableUtxo now: - Filters out already-locked UTXOs before selection - Locks the selected UTXO and returns a release function - All staking/governance operations now release UTXO locks in finally blocks - AddressWithUtxo interface includes a release() function - Added lockOne() helper to UtxoLockManager for single-UTXO locking Operations updated: - registerStakingCredential - delegateToPool - deregisterStakingCredential - withdrawRewards - delegateToDRep - registerAsDRep - castVote Locks auto-expire after UTXO_LOCK_TTL_MS (2 minutes) as a safety net.
- Add security configuration section documenting CORS, rate limiting, and API key authentication setup with examples - Add protocol parameters configuration section explaining how to override Cardano protocol params at SDK initialization - Update environment variables table with all security-related vars (MAX_BODY_SIZE, RATE_LIMIT_*, CORS_ORIGINS, API_KEY_*) Related to fixes #1 (security middleware) and #5 (protocol params)
- Runs lint, typecheck, and build on every push to main and PR - Runs E2E staking info test (read-only) after build passes - Runs E2E ADA transfer test (3 ADA between vault 0 and 1) - PRs to main will be blocked if CI fails This addresses review issue #3 by adding automatic CI while keeping the manual workflow_dispatch tests for more complex scenarios.
Mount /api/webhook with express.raw() so the raw request body is preserved and signature verification runs before any JSON parsing. The webhook handler now verifies the Fireblocks signature against the raw buffer and only parses JSON after successful verification, ensuring untrusted payloads are never deserialized prior to authentication.
resolveRecipientAddress now parses recipientAddress with Address.from_bech32 and rejects malformed bech32 strings, then checks that the embedded network id matches the SDK's configured network (mainnet=1, preprod=0). This prevents accidental cross-network sends and surfaces a clear validation error instead of a downstream parse failure.
Convert netAmount and feeAmount to BigInt before passing them to cbor2 in buildPayload. JS Number only safely represents integers up to 2^53; on-chain lovelace amounts can in theory exceed that bound. Using BigInt forces cbor2 to emit CBOR unsigned integers (major type 0) without precision loss.
Add isSpendableUtxo helper that rejects UTxOs carrying datum_hash or script_hash, and apply it alongside the existing utxo-lock filter in fetchAndSelectUtxosForCnt / Ada / MultiToken and in the staking UTxO provider. These outputs require a Plutus or native-script witness and are unspendable with the Ed25519 keys held in Fireblocks vaults.
Conway CDDL defines gov_action_id.gov_action_index as uint .size 2. Cap the value at 65535 in the cast-vote request schema so out-of-range indices are rejected at the API boundary instead of producing an invalid CBOR transaction body.
Replace the bare regex check on drepId with a bech32-aware validator that decodes drep1.../drep_script1... strings, verifies the checksum, and confirms the payload is exactly 28 bytes (the credential length). Hex form keeps the existing 56-char check.
Move the recipient-address parse/network-id check out of the private SDK method and into assertRecipientAddress in utils/cardano.ts so it can be unit-tested directly. Behaviour is unchanged; the SDK now calls the helper through its existing resolveRecipientAddress chokepoint.
Cover the behaviour introduced by the M-01..M-08 commits: - M-01: webhook handler verifies signature against the raw Buffer before parsing JSON; rejects non-buffer/empty bodies, bad signatures and signature-valid-but-malformed JSON. - M-02: assertRecipientAddress accepts matching-network addresses and rejects malformed bech32 / cross-network addresses with descriptive ValidationError info. - M-04: buildPayload emits CBOR major-type-0 uint bytes for lovelace and fee, including the 8-byte form for values above 2^32 and proper rounding of fractional inputs. - M-06: isSpendableUtxo excludes UTxOs with datum_hash or script_hash. - M-07: governanceActionId.index rejects values above uint16 max. - M-08: DRep ID validation accepts hex and bech32, rejects bad checksums, wrong-length payloads and unknown HRPs. Switched the test script to node --experimental-vm-modules so Jest can load cbor2 (ESM-only) when running the new buildPayload tests.
handleError previously echoed SdkApiError.errorInfo and .service back to the API consumer, leaking internal context (queried vault account ids, upstream service names, dependency metadata). Keep both fields in the server-side log only; the public JSON now exposes just success, error message, statusCode and error type.
… last estimate Previously convergeTransactionFee logged a warning and returned the last currentFee if the loop did not converge within TX_FEE_MAX_ITERATIONS. That value can be below the actual minimum fee for the resulting tx body, leaving callers to build and submit an underpaid transaction that the network rejects (or, worse, that silently differs from the declared fee). Throw an SdkApiError on non-convergence so the failure surfaces immediately and the caller can retry or escalate.
encodeDRepId previously used HRP "drep_script" for script-based DReps, which is non-standard: per CIP-129 the HRP is always "drep" and the type is encoded in the leading header byte (0x22 key, 0x23 script). decodeDRepId required a 29-byte payload, while the request validator required 28 bytes, so legitimate CIP-129 inputs were rejected at the API edge while legitimate CIP-105 inputs failed inside the SDK. After this change: - encodeDRepId emits CIP-129 (HRP=drep, 29 bytes). - decodeDRepId accepts CIP-129 (29 bytes, header-tagged) AND CIP-105 legacy (28 bytes, HRP-tagged: drep / drep_script). - isValidDrepId mirrors decodeDRepId: hex, CIP-105 (28B), or CIP-129 (29B with HRP=drep only). 29-byte payloads with HRP=drep_script are rejected as malformed.
Add CardanoConstants.MAX_TX_SIZE_BYTES (16384) and assertTxSizeWithinLimit in utils/cardano. Call it from FireblocksCardanoRawSDK.signTransaction and from StakingService just before forwarding to Fireblocks, so a transaction body that would exceed the protocol maxTxSize is rejected with a TxSizeLimitExceeded error instead of consuming a signing quota and being refused by the node.
When an exception was not an SdkApiError, handleError surfaced the raw error.message back to the API consumer, which can leak stack-derived details, file paths, internal identifiers, or upstream failure messages. Log the full message and stack server-side and return a generic Internal server error response with a stable type discriminator.
fetchAllVaultHistory previously made one history call per address and treated whatever subset the API chose to return as the complete result. For accounts with more than one page of history, getAllTransactionHistory and getAllDetailedTxHistory silently dropped older transactions. Introduce fetchAllPagesForAddress which walks the upstream pagination (hasMore + short-page fallback) until the stream ends, with a hard cap of 1000 pages * 100 rows = 100k transactions per address. When the caller supplies an explicit limit, the original single-page behaviour is preserved so existing query shapes keep working.
StakingValidator.checkDelegationPrerequisites previously fetched getPoolInfo and discarded the response. Now it inspects the result and throws PoolNotFound (404) when the pool does not exist or PoolRetired (400) when the pool has at least one retirement certificate filed, preventing delegation transactions that the chain would either reject or that would be effectively wasted on a soon-to-be-retired pool.
IagonApiService hardcoded the upstream base URL at the class field level via the iagonBaseUrl module constant, leaving no override path for staging or private Iagon deployments. The URL is now resolved at construction time from (in order): an explicit constructor arg, the IAGON_BASE_URL environment variable, then the built-in default. FireblocksCardanoRawSDK.createInstance forwards an optional iagonBaseUrl param so library consumers can override without touching the environment. Documented in README and .env.example.
signTransaction allocated PublicKey, Vkey, Ed25519Signature, Vkeywitness, Vkeywitnesses, TransactionWitnessSet and Transaction handles from the cardano-serialization-lib WASM module but did not free them on early returns or thrown exceptions. The returned Transaction was also never freed by any caller after submission. Each Fireblocks signing request therefore leaked a handful of WASM allocations. After this change: - signTransaction holds every intermediate handle in named locals and releases them in a try/catch around the success path, so a throw at any stage frees everything that was allocated up to that point. - All four call sites (CNT transfer, multi-token transfer, single-batch consolidation, batched consolidation) wrap submitTransaction in a try/finally that calls signedTransaction.free() once the CBOR has been serialized for the upstream POST.
The repo only ships a Dockerfile - there is no docker-compose.yml - so README instructions telling users to run docker-compose up/logs/down/ restart all fail. Replace the docker-compose snippets with equivalent plain docker run / docker logs / docker stop / docker restart commands and remove docker-compose.yml from the project-tree listing.
SdkManager.getSdk read from sdkPool, and on a miss awaited sdkFactory(...) and then stored the result. Two requests for the same uncached vaultAccountId arriving on the same event-loop tick both pass the cache check, each call the factory, and the second factory result overwrites the first - the orphaned SDK keeps its underlying connections open with no future getSdk hit to reuse it. Add a pendingSdkCreations map keyed by vaultAccountId. On cache miss, the first caller registers its in-flight Promise and runs the factory; concurrent callers for the same key await that Promise instead of starting their own. The entry is removed in a finally block so a failed creation does not pin the key. useCount is incremented per caller that joins, which also fixes the previous off-by-one where the first caller's write reset useCount to 1. Adds src/__tests__/pool/sdkManager.test.ts covering: shared instance across 8 concurrent waiters, distinct allocations for distinct keys, and recovery after a rejected in-flight creation.
Add a unit-tests job that runs npm test (302 tests across 9 suites) in parallel with lint-and-build. Both e2e jobs (staking-info, ada-transfer) now depend on it via needs: [lint-and-build, unit-tests] so a unit-test regression skips the mainnet e2e runs instead of burning real funds.
submitTransfer now throws SdkApiError(TX_SUBMIT_REJECTED) carrying the upstream Iagon error string. submitTransaction propagates SdkApiError unchanged so the staking-service IncompleteWithdrawals retry detector finally sees the substrings it matches on.
checkRegistrationStatus caught every exception and returned false. An Iagon outage during deregistration was indistinguishable from a genuinely unregistered stake key, causing the SDK to skip the deregistration silently and leave the user's 2 ADA deposit locked. Now only an SdkApiError with statusCode 404 maps to false; everything else propagates to the caller.
…-batched-consolidation
…s loudly on contract drift [FIREHOG-BYPASS] public information
…ection message [FIREHOG-BYPASS] public info
…built CBOR Route all hand-built CBOR coins through a guarded toCoinBigInt helper that rejects NaN, negative, and unsafe-integer values before signing. Type withdrawal maps as WithdrawalMap (bigint values). Add the audit-prescribed >2^53 CBOR encoding tests, which also cover the withdrawal (body key 5) encoding until rewards accrue for on-chain verification.
…s in the auth exemption
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Ports the audit follow-up remediations onto the 1.4.x line, reconciled with this branch's parallel work (spendable-UTxO filtering, staking-path UTxO locking, CIP-129 DRep decoding, error-propagating fetchUtxos, protocol-param constants). - S-6: guard full UTxO-payload / CBOR logging behind DEBUG (eager args). - S-3: assert single-output value size <= MAX_VALUE_SIZE before signing. - S-7: size-aware min-fee floor (assertFeeCoversSize) on the raw staking/gov path, enforced before signing. - M-13: client-facing throws -> typed SdkApiError (4xx), incl. the CIP-129 DRep validation throws, with rethrow guards in catch-all wrappers. - OC-2: batched consolidation locks each batch, filters spendable+locked UTxOs, and confirms spends on-chain (waitForUtxosSpent) instead of a fixed delay. - S-4: aggregate multiple pure-ADA UTxOs from one address (spendable-filtered) to fund staking/gov deposits, atomically locking the whole set; add FRAGMENTED_PURE_ADA error for cross-address fragmentation. - M-14: fetchAllPages() + getAll* variants; rewards/history return all pages. Hardening carried over from review: - UTXO_LOCK_TTL_MS (240s) >= TX_CONFIRM_TIMEOUT_MS invariant (+ test). - assertOutputMeetsMinUtxo(): reject below-min change output before signing. AddressWithUtxo now carries the aggregated utxos[] + totalAmount and a release() for the held lock; TransactionBuildContext takes utxos[]. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sets the workflow_dispatch default network to mainnet for the ADA-transfer and staking e2e workflows (the token-transfer workflow and test scripts already carry the basePath e2e updates on this branch). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
2 participants
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.
No description provided.