fix(security): correct RPC simulation shape, warn on secrets, label admin examples - #142
Open
habnark wants to merge 2 commits into
Open
fix(security): correct RPC simulation shape, warn on secrets, label admin examples#142habnark wants to merge 2 commits into
habnark wants to merge 2 commits into
Conversation
…dmin examples
checkWhitelist and the portfolio balance lookup called simulateTransaction
with a raw contract-call operation wrapped in `{ transaction: ... } as any`,
rather than a built Transaction. rpc.Server.simulateTransaction expects a
Transaction directly; both call sites are fixed via a shared
buildSimulationTransaction helper, with regression tests asserting the real
Transaction shape now used.
Also removes the unnecessary signing keypair from the README's read-only
Quickstart (it was labeled "adminKeypair" for a call that needs no signer),
adds an explicit Privileged Operations section for mint/transfer, and
replaces every hardcoded Keypair.fromSecret('S...') in README/docs/examples
with an env-var pattern plus a "never hardcode a real secret" warning.
Updates the reviewer checklist's security section to reference these
examples so future ones are held to the same bar.
Closes #66
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.
Description
Security audit of SDK examples per issue #66: reviewed README, docs, and
examples/migration/for unsafe key handling, admin misuse, incorrect RPCformatting, and compliance claims.
What changed:
ComplianceModule.checkWhitelistandInvestorModule's balance lookup both calledsimulateTransaction({ transaction: call as any } as any)— a{ transaction }wrapper around anunbuilt contract-call operation.
rpc.Server.simulateTransactionactuallytakes a built
Transactiondirectly as its first argument. Both were wrongon two counts (wrapper shape + unbuilt operation), papered over with
as any. Fixed via a new shared helper,src/utils/simulation.ts, used by bothcall sites; added regression tests asserting the real
Transactionshape.Also corrected the same pattern everywhere it was copy-pasted into
examples/migration/*.tsanddocs/migration-guide.md's "before" snippets.Keypair.fromSecret('S...')inREADME.md,docs/migration-guide.md, andexamples/migration/mint-transfer-before-after.tsnow loads fromprocess.env.AEGIS_ISSUER_SECRET!with an explicit "never hardcode a realsecret" warning comment next to it, instead of showing a bare string
literal as the pattern to copy.
unused, misleadingly-named
adminKeypairfor what is actually a read-onlycall (
checkWhitelistneeds no signer). A new "Privileged Operations(Admin / Issuer)" README section, and
sections of
docs/migration-guide.md, now clearly mark where a privilegedsigner is actually required.
docs/api-reference.md,docs/role-discovery.md,docs/contract-events.md, anddocs/admin-action-receipts.md— all already carry accurateprotocol-vs-legal compliance disclaimers. No gap found; no changes needed
here beyond updating the one stale "Open note" in
api-reference.mdthatdescribed the now-fixed
as anyuncertainty.docs/reviewer-checklist.md'sSecurity section now explicitly requires the secret-loading pattern,
admin-labelling convention, and correct
simulateTransactionshape shownabove, and names the audited files as the canonical reference.
Why this approach: the secret/admin issues were fixable as documentation
changes, but the RPC formatting issue turned out to be a real bug in
production
src/code (not just examples) once traced back to its source —fixing it there and reusing one helper across both call sites was the
correct fix, not just adding a caveat comment.
Closes: #66
Evidence Checklist
1. Issue Reference
Closes #66).2. Implementation Summary
src/compliance.ts,src/investor/portfolio.ts— use the new shared simulation-transaction builder instead of the wrapper-object pattern.src/utils/simulation.ts(new) — sharedbuildSimulationTransactionhelper.tests/compliance.test.ts(new) — first direct unit tests forComplianceModule, including the RPC-shape regression test.tests/investor.test.ts— added a regression test for the balance-lookup RPC shape.README.md,docs/migration-guide.md,docs/api-reference.md,docs/reviewer-checklist.md— secret-handling warnings, admin labelling, updated open note, checklist references.examples/migration/*.ts(4 files) — corrected RPC shape in "before" snippets, secret-handling warning in the mint/transfer "after" snippet.3. Tests
checkWhitelist, portfolio balance lookup).docs//examples/changes are documentation-only; no runtime tests apply to them beyond thesrc/regression tests above, which exercise the same code path the examples demonstrate.4. Commands Run
Command output
5. CI Status
CI failure notes
npm run lintfails on this branch, but it also fails identically onmainbefore this PR's changes:
eslintis referenced by thelintscript but isnot installed as a dependency, and no
eslint.config.jsexists in the repo.This is a pre-existing repository gap, not something introduced by this PR —
npm run build,npm test, andnpm run test:compatall pass cleanly.6. Acceptance Criteria Coverage — Traceability Table
README.md,docs/migration-guide.md,examples/migration/*.ts(4 files)README.md,docs/migration-guide.md,examples/migration/mint-transfer-before-after.tsKeypair.fromSecret(...)now reads fromprocess.env.AEGIS_ISSUER_SECRET!with an adjacent "never hardcode a real secret" comment; no bare'S...'literal remains as the pattern to copy.README.md(Privileged Operations (Admin / Issuer)section),docs/migration-guide.md(Minting/Transferring/Setup sections)⚠️ Privileged operationnote; the read-only Quickstart no longer includes an unused signer.src/compliance.ts,src/investor/portfolio.ts,src/utils/simulation.tstests/compliance.test.ts(calls simulateTransaction with a real built Transaction, not a wrapper object),tests/investor.test.ts(calls simulateTransaction with a real built Transaction for the balance query)simulateTransactionnow receives a builtTransactioninstance directly; tests assertpassedArg instanceof Transactionand the correctnetworkPassphrase, replacing the old{ transaction: call as any } as anyshape.docs/api-reference.md,docs/role-discovery.md,docs/contract-events.mdapi-reference.mdthat described the now-fixed RPC-shape uncertainty.docs/reviewer-checklist.mdReviewer Notes
The RPC-formatting fix (#4) is the substantive change here — it's a real bug
in
src/, not just a documentation issue, found while tracing why themigration examples' "before" snippets used a pattern that looked fragile.
ComplianceModulepreviously had no direct unit tests at all (only exercisedindirectly via
InvestorModuleand mocked away inrole.test.ts), which islikely why this went unnoticed;
tests/compliance.test.tscloses that gap.