fix: address security, UX, and hygiene findings from codebase audit#25
Open
LeJamon wants to merge 2 commits into
Open
fix: address security, UX, and hygiene findings from codebase audit#25LeJamon wants to merge 2 commits into
LeJamon wants to merge 2 commits into
Conversation
Security - Pipe wallet seeds to JS modules via stdin instead of an argv tempfile path (eliminates /proc/<pid>/cmdline exposure and stale temp files). - Mask seeds on vault/escrow success output (previously cleartext). - Bump PBKDF2 iterations 100k -> 600k (OWASP 2023); store iteration count in keystore for forward compat; legacy v1 keystores still decrypt. - Wallet resolver no longer misclassifies names like "swap-vault" as seeds. - Tighten file modes: project .wallets/ 0700, ledger-daemon PID 0600, cached JS modules 0644 (were 0755/0644 inconsistently). - Ledger daemon stop now polls and SIGKILLs if SIGTERM is ignored. Correctness / UX - "bedrock init --primitives a,b,c" now parses comma-separated values (was rejecting commas despite the docs). - Config loader walks upward to find bedrock.toml so commands work from subdirectories like contract/. - "bedrock node logs" implemented via Docker SDK (--follow, --tail). - Docker platform derived from runtime.GOARCH instead of hardcoded amd64. - --verbose persistent flag actually wired through; removes three "TODO: get from global flag" sites in call/deploy/escrow. - Unify --network default to "local" across deploy/call/vault/escrow/ modify/delete/user_delete/clawback (was a mix of alphanet/local). Error handling - Surface "rustup target add" failures instead of swallowing. - Log daemon cmd.Wait() errors to the daemon log. - Route ledger-service startup warnings to stderr. Docs / hygiene - Fix README: clone URL (xrpl-bedrock -> XRPL-Commons), Go 1.21 -> 1.24, node logs documented as implemented, --network default updated. - Run "go mod tidy" (drops unused btcsuite/btcutil). - .gitignore: /bedrock binary, tasks/, coverage outputs. Tests / CI (was 0 tests, now 21) - .github/workflows/ci.yaml: go vet, go test -race, mod-tidy check, golangci-lint on ubuntu + macos. - .golangci.yml: errcheck, govet, ineffassign, staticcheck, unused, gofmt, goimports, misspell. - pkg/wallet: encrypt/decrypt round-trip, wrong-password, duplicate prevention, legacy v1 keystore upgrade path, name validation, resolver behavior for seed vs. keystore-name. - pkg/config: defaults, primitive inference, upward walk, missing-config. - pkg/jade: drops <-> XRP precision (incl. > int64 range). - internal/cli: maskSeed property test (never leaks the middle).
- --network default is now `local` for deploy/call (was alphanet) - bedrock node logs is implemented; document --follow and --tail - Go version requirement bumped 1.21 -> 1.24 across all guides - wallet.md notes PBKDF2 600k iterations, per-keystore iteration count, 0700 wallets dir / 0600 keystore mode, and seed-vs-name resolution - CLAUDE.md docker image name now consistent with the rest of the docs; mention auto-detected linux/arm64 vs linux/amd64
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
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.
Addresses the punch list from a code-audit pass. Single PR because most items are small (≤20 LOC) and they reinforce each other; the commit message lists every change for reviewers who want a checklist.
Summary
Security
/proc/<pid>/cmdlineleak and the "what if the temp file lingers" failure mode. All 15 embedded JS modules updated to read stdin when invoked with-.vault deploy/escrow deploysuccess output now usesmaskSeed()(was cleartext).deployalready did.Iterationsfield on the keystore so we can bump again later without breaking forward compat; v1 keystores without the field still decrypt via legacy fallback.swap-vaultare no longer misclassified as raw seeds..wallets/0700, ledger-daemon PID 0600, cached JS modules 0644.bedrock node stoppolls for exit and SIGKILLs after 2s if SIGTERM is ignored, instead of silently removing the PID file.Correctness / UX
bedrock init --primitives contract,escrow,vaultactually splits on commas (was rejecting commas despite the docs).bedrock build/deploy/ etc. now work from a subdirectory — config loader walks upward forbedrock.tomland chdirs to the project root.bedrock node logsis implemented via the Docker SDK (was a "not yet implemented" stub). Supports--followand--tail.runtime.GOARCHinstead of hardcodedlinux/amd64— Apple Silicon no longer pulls an unused amd64 image when an arm64 one is available locally.--verbosepersistent flag is now wired through; removes three// TODO: get from global flagsites incall.go/deploy.go/escrow.go.--networkdefault unified tolocalacross the mutation commands (deploy,call,vault,escrow,modify,delete,user_delete,clawback) — used to be a mix.Error handling
rustup target addfailures now surface instead of being swallowed.cmd.Wait()errors are written to the daemon log.Docs / hygiene
xrpl-bedrock/bedrock→XRPL-Commons/Bedrock, Go 1.21 → 1.24,node logsdocumented as implemented,--networkdefault updated.go mod tidy(drops unusedbtcsuite/btcutil)..gitignore:/bedrockbinary,tasks/, coverage outputs,.vscode/.Tests / CI
The repo had zero
_test.gofiles and no test job. This PR adds:.github/workflows/ci.yaml—go vet,go test -race,go mod tidycheck, andgolangci-linton ubuntu + macos..golangci.yml— errcheck, govet, ineffassign, staticcheck, unused, gofmt, goimports, misspell.pkg/wallet,pkg/config,pkg/jade,internal/clicovering the highest-leverage paths (encrypt/decrypt round-trip, v1 → v2 keystore upgrade, resolver heuristic, upward config walk, drops/XRP precision incl. >int64 range,maskSeedproperty test).Test plan
go build ./...go vet ./...go test -race ./...(21 tests pass)go mod tidycleanrequire('@xrpl-commons/xrpl')step, confirming argv→stdin plumbing works.bedrock init,bedrock node start/stop/logs,bedrock vault deploy,bedrock escrow deployagainst a local node (reviewer's environment).