Feat/2026 hardening 690 694 696 697 - #703
Merged
Nanle-code merged 4 commits intoJul 29, 2026
Merged
Conversation
Soroban charges for what a transaction actually does, and simulateTransaction is the only source of those numbers. StarForge now parses the full resource accounting out of a simulation response and reports it wherever a fee is shown. New `utils::simulation_resources`: - parses CPU instructions, linear memory, ledger footprint (decoded from the `transactionData` XDR), minimum resource fee, and the restore preamble; - derives a submittable fee = resource + restore + margin + inclusion, with overflow-checked arithmetic and a validated 0-1000% margin range; - accepts numeric fields as JSON numbers or strings (stellar-rpc emits u64 as strings) and rejects negative, fractional, and non-numeric values instead of coercing them to zero; - caps input at 8 MiB before the JSON parser runs and decodes XDR with an explicit depth limit. Wiring: - `starforge simulate resources` reports the numbers from a saved response (--file) or a live simulation (--contract/--function), with --margin, --inclusion-fee, and --json; - `starforge cost resources` prices a simulation and checks it against the configured budgets, with --enforce to gate CI; - `starforge deploy --simulate/--dry-run` prints CPU, memory, footprint, and a recommended fee inline. Behaviour change: SimulationResult::fee previously reported `cost.cpuInsns`, an instruction count rather than a fee. It now reports `minResourceFee`, falling back to 100000 stroops only when the server reports no resource accounting at all. The struct gained an optional `resources` field, defaulted for serde so older serialised values still deserialise. Compatibility, security, and migration notes: docs/SIMULATION_RESOURCES.md. Tests: 19 unit tests in the new module plus 3 in utils::soroban covering the primary flow, the margin/overflow/zero-fee boundaries, and the malformed, host-error, transport-error, and unsupported-endpoint failure paths.
Adds tests/config_property_tests.rs, which generates hundreds of internally consistent configurations and asserts two families of properties: - preservation: TOML and JSON round trips are exact, crossing formats is stable, serialization is deterministic, merging an empty overlay is the identity, merging is idempotent, and a merged config still round trips; - rejection: malformed *combinations* are refused rather than repaired - an active network that is not configured, a wallet on an unknown network, a duplicate wallet name, a non-HTTP endpoint, a malformed StrKey, an overlay that collides with a stored wallet, and an unknown overlay key. Supporting changes to utils::config: - pure parse/serialize helpers (parse_config_str, parse_config_json, to_toml_string, to_json_string) so round trips are testable without disk; - ConfigOverlay + merge_configs, with documented precedence: scalars are overlay-wins, feature_flags and plugin_trust are replaced wholesale so a half-merged policy is impossible, networks merge by key, wallets append and reject name collisions, and version/install_id always come from the base. The overlay uses deny_unknown_fields so `netwrok = "mainnet"` is a hard error rather than a silently ignored typo; - Config fields reordered so all scalars precede tables, which is what TOML serialization requires; deserialization is by key so existing files load unchanged; - PartialEq derives on Config and friends so round-trip equality is assertable; - validate_config now rejects duplicate wallet names. Fixes a purity bug: validate_network_exists fell through to validate_network, which calls load() - so validating an in-memory Config opened, migrated, and wrote the user's real database, and the same value could validate differently on two machines. It now consults only the supplied Config and the built-in network names. Migration note in docs/CONFIGURATION.md. Also ignores *.proptest-regressions, which are regenerated shrink artifacts.
Every invocation now carries exactly one correlation ID, attached to a root command span entered in main(). Retries, network requests, plugin calls, and deployment steps all log underneath it, so a single invocation can be reconstructed from an aggregated log stream even when several StarForge processes run concurrently in the same pipeline. New `utils::correlation`: - resolve() with precedence --correlation-id > $STARFORGE_CORRELATION_ID > generated (UUIDv4). An explicitly supplied value that fails validation is fatal (exit 2) rather than a silent fallback, because generating a different ID would break exactly the log join the caller asked for; - validation: 8-64 characters of [A-Za-z0-9_-], trimmed, with a multi-byte character reported as an invalid character rather than a length problem; - span helpers for the four correlated activities: retry_span, network_span, plugin_span, deploy_step_span, each tagged with a `kind` field; - init() is first-call-wins, so a re-entrant context (the REPL, a plugin host) cannot fragment one invocation across several IDs. Not leaking secrets is the point of the exercise: - a supplied ID that looks like key material (a Stellar S... seed, an encrypted salt:nonce:ciphertext bundle, a long mixed-case base64 blob) is rejected outright, since the ID lands on every log line; - span attributes go through sanitize(): secret-looking values become [REDACTED], control characters are replaced so a crafted value cannot forge an extra log record, and values are truncated at 120 characters; - URLs go through redact_url(), which keeps scheme, host, and path and drops the query string and any user:password@ userinfo. JSON logging now emits the current span and the full span list, so nested events carry the ID too. The human formatter already prints span context. Tests: 18 unit tests plus tests/correlation_logging.rs, which drives a real tracing subscriber and inspects the emitted JSON - asserting that every nested event shares one ID, that each span kind appears, that an event outside any span still logs cleanly, and that a secret key, basic-auth credentials, and an API key in a query string never reach the log. Docs: docs/CORRELATION_IDS.md.
`wallet import --file` and `backup restore` read files that came from outside
the tool, so their parsing is a trust boundary. It now lives in
utils::wallet_import - separated from prompting, disk access, and the config
store - where it can be unit-tested, property-tested, and fuzzed.
Enforced on untrusted input:
- size gates run before the JSON parser (4 MiB per document, 8 MiB per
envelope, 1000 wallets, 64-character names), so an oversized file cannot
drive a large allocation;
- backup documents must declare version "1", carry at least one wallet, use
unique names, and hold well-formed StrKeys;
- encrypted envelopes must have 3, 5, or 6 base64 parts, a 16-byte salt, a
12-byte nonce, a ciphertext of at least one AES-GCM tag, and non-zero KDF
parameters - all checked before the passphrase prompt, so a corrupt file
fails fast instead of after an Argon2 derivation;
- wallet names carrying control, bidi, or zero-width characters are rejected;
non-ASCII names are accepted with a homograph warning rather than refused,
since earlier releases allowed any Unicode alphanumeric;
- rejections quote the wallet name and reason, never the key material.
Fixes two classification bugs. Encryption detection was
`raw.matches(':').count() == 2`, which only recognises the 3-part bundle: a
backup encrypted with custom Argon2 parameters has 5 or 6 parts and was handed
to the JSON parser, failing with a misleading "Invalid backup JSON format".
And a JSON document containing exactly two colons could trigger a passphrase
prompt for a passphrase that does not exist. Classification now follows the
bundle grammar and never treats a document starting with { or [ as a bundle.
Three fuzz harnesses drive the parsers: fuzz_wallet_backup_parse (bytes),
fuzz_wallet_import_envelope (envelope structure and classification), and
fuzz_wallet_backup_structured (near-valid documents via arbitrary::Arbitrary,
to reach the semantic checks the byte-level harness rarely hits). Each asserts
totality plus the invariants any accepted value must satisfy. Ships with a
dictionary and 13 seed corpus entries covering valid, watch-only, empty,
bad-version, truncated, bidi-name, 3/5/6-part, truncated-ciphertext, and
non-base64 inputs.
cargo fuzz needs nightly, so tests/wallet_import_property_tests.rs asserts the
same invariants with proptest on stable: 16 tests over the primary flow, the
exact ciphertext/salt/nonce/name-length boundaries, and the failure paths
(malformed JSON, malformed StrKeys, deceptive names, oversized input, never
panicking on arbitrary or lossy-UTF-8 bytes, never echoing a secret).
Docs: docs/WALLET_IMPORT_SECURITY.md, plus FUZZING_GUIDE.md, README.md, and
docs/COMMAND_REFERENCE.md updated for all four roadmap issues.
|
@nonso7 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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.
Description
Brief description of what this PR does and why.
Closes #(issue number)
Type of Change
Changes Made
Testing
How has this been tested?
Describe the tests you ran and how to reproduce them.
Test Coverage
Describe what scenarios have been tested:
Code Quality Checklist
cargo fmt)cargo clippy -- -D warnings)Breaking Changes
If checked, describe the breaking changes and migration path:
Documentation
Screenshots (if applicable)
Add screenshots or GIFs for UI changes.
Additional Context
Add any other context about the PR here.
Note: Make sure all tests pass locally before submitting:
cargo test cargo fmt --all cargo clippy -- -D warningsCloses #690
Closes #694
Closes #696
Closes #697