fix(publisher): drop random fallback wallet + audit Wallet.createRandom#371
fix(publisher): drop random fallback wallet + audit Wallet.createRandom#371branarakic merged 43 commits intomainfrom
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| 'node_modules', 'dist', 'dist-ui', '.git', '.turbo', 'coverage', | ||
| 'test', 'tests', '__tests__', 'cache', 'artifacts', 'typechain', | ||
| ]); | ||
| const SOURCE_EXTS = new Set(['.ts', '.js', '.mjs', '.cjs']); |
There was a problem hiding this comment.
🟡 Issue: SOURCE_EXTS misses .tsx/.jsx, but this repo already ships production code from packages/*/src/**/*.tsx (for example the node UI). That means the new CI gate can still be bypassed by introducing Wallet.createRandom() in a TSX entrypoint even though the script/header/changelog describe a ban across production sources. Include those extensions or narrow the documented scope.
There was a problem hiding this comment.
Addressed in 36163d9 — added .tsx / .jsx to SOURCE_EXTS and to the test-suffix exclusion list. Manually verified the script now flags Wallet.createRandom() in a .tsx source file and still excludes .spec.tsx fixtures.
| this.publisherAddress = this.publisherWallet.address; | ||
| } else { | ||
| // No private key supplied → no signing capability. Every on-chain code | ||
| // path in this class is gated on `if (this.publisherWallet)` and |
There was a problem hiding this comment.
🟡 Issue: This comment now claims every on-chain path in DKGPublisher is gated on publisherWallet, but update() still calls updateKnowledgeCollectionV10 / updateKnowledgeAssets without that guard. That invariant is broader than the code and can mislead future changes. Reword this to the narrower guarantee that publish-time self-sign / authorship-proof paths require publisherWallet.
There was a problem hiding this comment.
Addressed in 36163d9 — reworded to the narrower (accurate) guarantee: only the publish-time self-sign ACK (L1237), on-chain publishDirect signature (L1276), and per-KA authorship proofs (L1444) require publisherWallet. Called out explicitly that update() and the chain.updateKnowledge* paths use the chain adapter's own signer pool, so they don't depend on this field.
Two follow-ups from Codex review on PR #371: 1. scripts/audit-create-random.mjs: SOURCE_EXTS missed .tsx/.jsx, but packages/node-ui and packages/graph-viz ship production code from .tsx files (40+ files in packages/*/src/). The CI gate could be bypassed by introducing Wallet.createRandom() in a tsx entrypoint. Fix: add .tsx/.jsx to SOURCE_EXTS and to the test-suffix exclusion list. Manually verified the script now flags a tsx offense and correctly excludes .spec.tsx fixtures. 2. packages/publisher/src/dkg-publisher.ts: the constructor comment claimed every on-chain path is gated on `if (this.publisherWallet)`, but `update()` calls `chain.updateKnowledgeCollectionV10` / `chain.updateKnowledgeAssets` directly — those use the chain adapter's own signer pool and never touch this wallet. Reworded to the narrower (accurate) guarantee: only the publish-time self-sign ACK, on-chain publishDirect signature, and per-KA authorship proofs need it. Co-authored-by: Cursor <cursoragent@cursor.com>
| // callers that supplied `publisherAddress` separately. See PR #371 for | ||
| // the testnet-blocking incident chain (`ensureProfile` had the same | ||
| // anti-pattern, fixed in PR #366). | ||
| this.publisherAddress = config.publisherAddress ?? '0x' + '0'.repeat(40); |
There was a problem hiding this comment.
🔴 Bug: This branch treats “no publisherPrivateKey” as “no signing capability”, but the configured ChainAdapter already owns the operational signer and exposes signMessage(). With this change, callers that keep the key inside the adapter now always hit the tentative path and can never produce a confirmed publish, even though the adapter can sign the ACK/publisher digests. A safer fix is to fall back to the adapter signer here (and use the eventual onChainResult.publisherAddress for authorship metadata) instead of disabling on-chain publish entirely.
There was a problem hiding this comment.
Acknowledged but deferring to a follow-up — out of scope for this fix. Tracked in #373.
The proposed fallback isn't a one-line substitution: Wallet.signMessage(string|bytes) returns the EIP-191-prefixed eth-signed hex sig that all three call sites consume (ACK self-sign at L1256, on-chain publishDirect at L1320, authorship proofs at L1447), while the ChainAdapter.signMessage(messageHash: Uint8Array): {r, vs} we have today returns an EIP-2098 compact pair over a raw hash — different prefix semantics and different return shape.
In addition, every in-tree caller (the daemon's dkg-agent.ts and publisher-runner.ts) already passes the same op-key as both publisherPrivateKey and into the adapter signer pool — so this PR doesn't regress any actual current call path. The HSM-backed-adapter-only scenario Codex describes isn't a configuration we ship today.
The narrow change in this PR is still the right one in isolation: it stops minting throw-away keys and restores the if (this.publisherWallet) invariant the call sites already check. See #373 for the full design plan.
| 'node_modules', 'dist', 'dist-ui', '.git', '.turbo', 'coverage', | ||
| 'test', 'tests', '__tests__', 'cache', 'artifacts', 'typechain', | ||
| ]); | ||
| const SOURCE_EXTS = new Set(['.ts', '.tsx', '.js', '.jsx', '.mjs', '.cjs']); |
There was a problem hiding this comment.
🟡 Issue: The audit omits .mts/.cts, so this new CI gate does not actually cover all TS/ESM source files in the monorepo. A future Wallet.createRandom() in packages/*/src/**/*.mts would pass even though the script claims to ban the pattern in production code. Add .mts/.cts here and extend TEST_SUFFIXES accordingly.
There was a problem hiding this comment.
Addressed in 59272c8 — added .mts and .cts to both SOURCE_EXTS and TEST_SUFFIXES. Verified with a synthetic .mts fixture that the script now flags the offense.
Codex round 2 on PR #371: the previous round added .tsx/.jsx but I forgot the explicit-ESM/CJS TypeScript variants. A future Wallet.createRandom() in packages/*/src/**/*.mts would slip past the gate. Adds .mts and .cts to SOURCE_EXTS and to the test-suffix exclusion list. Verified with a synthetic .mts fixture that the script now flags the offense. Co-authored-by: Cursor <cursoragent@cursor.com>
| const text = await readFile(absPath, 'utf8'); | ||
| if (!fileHasOffense(text)) continue; | ||
| const relPath = relative(REPO_ROOT, absPath).split('\\').join('/'); | ||
| if (ALLOWLIST.has(relPath)) continue; |
There was a problem hiding this comment.
🔴 Bug: Skipping an allowlisted file entirely turns this audit off for every future Wallet.createRandom() added to that file. If someone later adds a second call in op-wallets.ts, agent-keystore.ts, or helpers.ts, CI will still pass even though only one site is justified today. Allowlist the specific hit(s) instead of the whole path, and fail on any extra matches.
There was a problem hiding this comment.
Addressed in cd1bd76 — ALLOWLIST now pins per-file expected hit counts (1 each for op-wallets.ts, agent-keystore.ts, helpers.ts). A second Wallet.createRandom() added to any of those files now fails CI with Allowlisted for 1 hit … but found 2. Stale entries (file no longer contains a hit) also fail, so exemptions can't outlive their call site. Verified locally with a synthetic test that injects a second call into op-wallets.ts and confirms the audit fails.
| } | ||
|
|
||
| function fileHasOffense(text) { | ||
| for (const line of text.split('\n')) { |
There was a problem hiding this comment.
🔴 Bug: This scan is line-based, so it misses split invocations like ethers.Wallet\n .createRandom() or Wallet /* ... */\n.createRandom(). That makes the new CI guard easy to bypass with formatting alone. Match against the whole file or parse the syntax tree instead of checking one line at a time.
There was a problem hiding this comment.
Addressed in cd1bd76 — replaced the per-line scan with a whole-file scan after // and block comments are blanked to whitespace of the same byte length (so original line numbers still resolve in error output). The pattern is now /\bWallet\s*\.\s*createRandom\s*\(/g, which catches Wallet\n.createRandom(), Wallet/* */.createRandom(), and any other whitespace-separated split. Verified with two synthetic fixtures (split-line + comment-injected) that both now fail CI.
| } else { | ||
| // No private key supplied → no in-process signing capability. The | ||
| // publish-time paths that need a `publisherWallet` to produce a | ||
| // signature (V10 self-sign ACK at L1237, the on-chain `publishDirect` |
There was a problem hiding this comment.
🟡 Issue: These hard-coded source line references will go stale immediately as the file changes, and they already do not match the current locations of the guarded branches. Refer to the relevant publish/update paths by name instead of L1237/L1276/L1444 so the comment stays accurate.
There was a problem hiding this comment.
Addressed in cd1bd76 — replaced the L1237/L1276/L1444 references with named-path references (V10 self-signed ACK fallback when no peer ACKs are collected, on-chain publishDirect publisher signature, per-KA authorship-proof loop in publishFromSharedMemory). Those numbers were already stale on the previous commit and would have gone stale again on the next change; named references stay accurate as the file evolves.
Addresses Codex review feedback on PR #371: - Whole-file scan after stripping `//` and block comments. The previous per-line regex let `Wallet\n .createRandom()` and `Wallet/* */.createRandom()` bypass the audit purely by formatting. The new scan uses the same regex with `\s*` between every token over the comment-stripped buffer, so split invocations are caught while the original line numbers still resolve (comments are blanked to whitespace of the same byte length). - Per-hit allowlist (vs whole-file). Each ALLOWLIST entry now pins the expected hit count, and any extra `Wallet.createRandom()` added to an already-exempt file fails CI even though one site is justified. Stale allowlist entries (file no longer contains a hit) also fail, so exemptions can't outlive their call site. - Replaced the L1237/L1276/L1444 references in the publisher constructor comment with named-path references (V10 self-signed ACK fallback, on-chain `publishDirect` signature, per-KA authorship-proof loop). These line numbers were already stale on the previous commit and would have gone stale again on the next change. Verified locally with synthetic fixtures covering all three new bypasses (split-line, comment-injected, second-hit-in-exempt-file) plus the preserved `.tsx` / `.mts` coverage and stale-allowlist detection. Co-authored-by: Cursor <cursoragent@cursor.com>
| * for an audit that only cares about real `Wallet.createRandom(` calls — | ||
| * such calls cannot live inside string literals. | ||
| */ | ||
| function stripCommentsPreservingPositions(text) { |
There was a problem hiding this comment.
🔴 Bug: This audit is still bypassable because the comment stripper treats // and /* inside string/template literals as real comments. A line like const url = "http://"; Wallet.createRandom() gets blanked before the regex runs, so CI misses the actual call. Since this guard is meant to prevent a security-sensitive regression, parse the file as JS/TS (or at least lex strings/comments correctly) and match real createRandom() call expressions instead of scanning stripped text.
Codex review on PR #371 flagged that the prior comment-stripper treated `//` and `/*` inside string literals as real comments, so a line like `const url = "http://"; Wallet.createRandom();` would blank from the `//` onwards — silently swallowing the real `createRandom()` call after the string. False-negative on a real offending call is the worst failure mode for a security audit (it provides false assurance), so the audit script's bypass-resistance was effectively undermined by formatting. Replace the comment-only stripper with a small state machine that also tracks single-quoted, double-quoted, and template-literal contents. Inside any string state, `//` and `/*` no longer trigger comment mode. String contents themselves are also blanked (not passed through), which prevents the inverse false-positive of a string literal containing `Wallet.createRandom(` matching the regex. Template substitutions (`${ ... }`) are scanned as code so a `${Wallet.createRandom()}` inside a template still gets flagged. Add a node:test unit suite covering: - comment / string / template stripping basics - the PR #371 regression cases (string with `//`, `/*`, escaped quotes, template literal — both same-line and following-line variants) - that calls inside string literals are NOT false-positives - that calls inside template substitutions ARE detected Verified the regression: with the old lexer inlined, the string-bypass test reports 0 hits (BYPASS); with the new lexer, 1 hit (CORRECT). Wire `node --test scripts/audit-create-random.test.mjs` into the lint job in `ci.yml` so a future regression of the lexer would fail CI. Co-authored-by: Cursor <cursoragent@cursor.com>
| continue; | ||
| } | ||
|
|
||
| if (state === 'tpl-substitution') { |
There was a problem hiding this comment.
🔴 Bug: tpl-substitution counts {/} even when they appear inside string literals, so the new audit can miss real Wallet.createRandom() calls inside template substitutions. For example, findHits('const t = ${"}" + Wallet.createRandom()};') returns no hits because the } inside the string closes the substitution early. Since this script is meant to prevent security regressions, it needs to lex strings/comments inside ${...} as well, or at least ignore braces while nested inside string states.
Two follow-ups from Codex review on PR #371: 1. scripts/audit-create-random.mjs: SOURCE_EXTS missed .tsx/.jsx, but packages/node-ui and packages/graph-viz ship production code from .tsx files (40+ files in packages/*/src/). The CI gate could be bypassed by introducing Wallet.createRandom() in a tsx entrypoint. Fix: add .tsx/.jsx to SOURCE_EXTS and to the test-suffix exclusion list. Manually verified the script now flags a tsx offense and correctly excludes .spec.tsx fixtures. 2. packages/publisher/src/dkg-publisher.ts: the constructor comment claimed every on-chain path is gated on `if (this.publisherWallet)`, but `update()` calls `chain.updateKnowledgeCollectionV10` / `chain.updateKnowledgeAssets` directly — those use the chain adapter's own signer pool and never touch this wallet. Reworded to the narrower (accurate) guarantee: only the publish-time self-sign ACK, on-chain publishDirect signature, and per-KA authorship proofs need it. Co-authored-by: Cursor <cursoragent@cursor.com>
Codex round 2 on PR #371: the previous round added .tsx/.jsx but I forgot the explicit-ESM/CJS TypeScript variants. A future Wallet.createRandom() in packages/*/src/**/*.mts would slip past the gate. Adds .mts and .cts to SOURCE_EXTS and to the test-suffix exclusion list. Verified with a synthetic .mts fixture that the script now flags the offense. Co-authored-by: Cursor <cursoragent@cursor.com>
Addresses Codex review feedback on PR #371: - Whole-file scan after stripping `//` and block comments. The previous per-line regex let `Wallet\n .createRandom()` and `Wallet/* */.createRandom()` bypass the audit purely by formatting. The new scan uses the same regex with `\s*` between every token over the comment-stripped buffer, so split invocations are caught while the original line numbers still resolve (comments are blanked to whitespace of the same byte length). - Per-hit allowlist (vs whole-file). Each ALLOWLIST entry now pins the expected hit count, and any extra `Wallet.createRandom()` added to an already-exempt file fails CI even though one site is justified. Stale allowlist entries (file no longer contains a hit) also fail, so exemptions can't outlive their call site. - Replaced the L1237/L1276/L1444 references in the publisher constructor comment with named-path references (V10 self-signed ACK fallback, on-chain `publishDirect` signature, per-KA authorship-proof loop). These line numbers were already stale on the previous commit and would have gone stale again on the next change. Verified locally with synthetic fixtures covering all three new bypasses (split-line, comment-injected, second-hit-in-exempt-file) plus the preserved `.tsx` / `.mts` coverage and stale-allowlist detection. Co-authored-by: Cursor <cursoragent@cursor.com>
Codex review on PR #371 flagged that the prior comment-stripper treated `//` and `/*` inside string literals as real comments, so a line like `const url = "http://"; Wallet.createRandom();` would blank from the `//` onwards — silently swallowing the real `createRandom()` call after the string. False-negative on a real offending call is the worst failure mode for a security audit (it provides false assurance), so the audit script's bypass-resistance was effectively undermined by formatting. Replace the comment-only stripper with a small state machine that also tracks single-quoted, double-quoted, and template-literal contents. Inside any string state, `//` and `/*` no longer trigger comment mode. String contents themselves are also blanked (not passed through), which prevents the inverse false-positive of a string literal containing `Wallet.createRandom(` matching the regex. Template substitutions (`${ ... }`) are scanned as code so a `${Wallet.createRandom()}` inside a template still gets flagged. Add a node:test unit suite covering: - comment / string / template stripping basics - the PR #371 regression cases (string with `//`, `/*`, escaped quotes, template literal — both same-line and following-line variants) - that calls inside string literals are NOT false-positives - that calls inside template substitutions ARE detected Verified the regression: with the old lexer inlined, the string-bypass test reports 0 hits (BYPASS); with the new lexer, 1 hit (CORRECT). Wire `node --test scripts/audit-create-random.test.mjs` into the lint job in `ci.yml` so a future regression of the lexer would fail CI. Co-authored-by: Cursor <cursoragent@cursor.com>
branarakic
force-pushed
the
fix/audit-create-random-and-fix-publisher
branch
from
063c93b to
3e20885
Compare
| * literal `"Wallet.createRandom("` inside a string can't false-positive | ||
| * the regex either. | ||
| * | ||
| * Regex literals (`/foo/g`) are NOT explicitly handled — we'd need full |
There was a problem hiding this comment.
🔴 Bug: This lexer still has a straightforward false-negative path through regex literals. For example, findHits('const r = /\\/\\//; Wallet.createRandom();') returns no hits because the // inside the regex body is treated as a real line comment and blanks the call that follows. Since CI is now relying on this script as the regression guard for random-key generation, that bypass materially weakens the protection. Please either handle regex literals conservatively here or switch the scan to a parser/tokenizer-based approach, and add a regression test for this case.
| const effectiveAccessPolicy = accessPolicy ?? (privateQuads.length > 0 ? 'ownerOnly' : 'public'); | ||
| const normalizedAllowedPeers = [...new Set((allowedPeers ?? []).map((p) => p.trim()).filter(Boolean))]; | ||
| const normalizedPublisherPeerId = publisherPeerId.trim(); | ||
| const { wallet: publisherWallet, address: publisherAddress } = this.requirePublisherWallet('publish'); |
There was a problem hiding this comment.
🔴 Bug: This hard guard regresses the existing tentative-only publish flow. The repo docs and PublishHandler both support publishes with no EVM key/signature, but this now throws before any prepare/store/P2P work happens. Keep the random-wallet fix, but move the wallet requirement down to the self-sign/on-chain branches so walletless publishes can still persist and broadcast tentative data.
| if (privateQuads.length > 0) rejectReservedSubjectPrefixes(privateQuads); | ||
| } | ||
| const ctx: OperationContext = operationCtx ?? createOperationContext('publish'); | ||
| const { address: publisherAddress } = this.requirePublisherWallet('update'); |
There was a problem hiding this comment.
🔴 Bug: update() now requires publisherPrivateKey, but the chain adapter APIs are explicitly built to select the signer themselves (publisherAddress is optional, and updateKnowledgeAssets() can resolve the original publisher on-chain). This blocks valid setups where the signing key lives only inside the adapter/signer pool. Let the adapter attempt the update and only fail if the adapter cannot find an authorized signer.
| const effectiveAccessPolicy = accessPolicy ?? (privateQuads.length > 0 ? 'ownerOnly' : 'public'); | ||
| const normalizedAllowedPeers = [...new Set((allowedPeers ?? []).map((p) => p.trim()).filter(Boolean))]; | ||
| const normalizedPublisherPeerId = publisherPeerId.trim(); | ||
| const { wallet: publisherWallet, address: publisherAddress } = this.requirePublisherWallet('publish'); |
There was a problem hiding this comment.
🔴 Bug: publish() now hard-fails before the adapter is consulted, so callers that intentionally keep the signing key inside a custom ChainAdapter can no longer publish even if they provide a valid publisherAddress. That is a public API regression for adapter-backed or hardware-signer setups. Either route this path through chain.signMessage()/the configured address when no local wallet exists, or keep the old tentative behavior until adapter-backed publish signing is implemented.
| if (privateQuads.length > 0) rejectReservedSubjectPrefixes(privateQuads); | ||
| } | ||
| const ctx: OperationContext = operationCtx ?? createOperationContext('publish'); | ||
| const { address: publisherAddress } = this.requirePublisherWallet('update'); |
There was a problem hiding this comment.
🔴 Bug: update() is stricter than the underlying adapter contract now. updateKnowledgeCollectionV10() already supports selecting the signer from the adapter’s signer pool via publisherAddress and can derive the signatures itself, so requiring publisherPrivateKey here breaks existing address-only/custom-signer callers for no correctness gain. Please allow the adapter-backed path and only throw when neither a local wallet nor an adapter signing path is available.
| const effectiveAccessPolicy = accessPolicy ?? (privateQuads.length > 0 ? 'ownerOnly' : 'public'); | ||
| const normalizedAllowedPeers = [...new Set((allowedPeers ?? []).map((p) => p.trim()).filter(Boolean))]; | ||
| const normalizedPublisherPeerId = publisherPeerId.trim(); | ||
| const publisherSigner = this.requirePublisherSigner('publish'); |
There was a problem hiding this comment.
🔴 Bug: This changes publish() from returning a tentative result into throwing immediately whenever no publisher signer is configured. Existing callers on NoChainAdapter or other identity-less setups will now hard-fail before any local tentative metadata is written. If the fail-closed behavior is intentional, this needs an explicit compatibility/migration path; otherwise move the signer requirement down to the branch that actually signs ACKs / on-chain payloads so the previous tentative flow still works.
| // newlines) between the tokens. This is intentionally a single regex over | ||
| // the comment-stripped file, NOT a per-line scan — that previously let | ||
| // `Wallet\n .createRandom()` bypass the audit by formatting alone. | ||
| const PATTERN = /\bWallet\s*\.\s*createRandom\s*\(/g; |
There was a problem hiding this comment.
🟡 Issue: This audit only catches the literal identifier Wallet, so equivalent call sites like import { Wallet as EthersWallet } from 'ethers'; EthersWallet.createRandom() or const W = Wallet; W.createRandom() still pass CI. Since this script is meant to prevent a high-impact key-loss regression, the matcher needs parser/binding-aware detection (or at least a broader strategy) rather than a single-name regex.
| const publishEpochs = 1; | ||
| let precomputedTokenAmount = 0n; | ||
| if (this.publisherWallet && typeof this.chain.getRequiredPublishTokenAmount === 'function') { | ||
| if (typeof this.chain.getRequiredPublishTokenAmount === 'function') { |
There was a problem hiding this comment.
🔴 Bug: Removing the signer guard here makes even guaranteed-tentative publishes call getRequiredPublishTokenAmount(). In no-key / no-identity setups that used to stay local, an RPC failure will now reject the whole publish before any tentative metadata is written. Gate this precompute behind the actual on-chain path (for example, only when a publisher signer exists and publisherNodeIdentityId > 0n), or fall back cleanly when token estimation is unavailable.
| const effectiveAccessPolicy = accessPolicy ?? (privateQuads.length > 0 ? 'ownerOnly' : 'public'); | ||
| const normalizedAllowedPeers = [...new Set((allowedPeers ?? []).map((p) => p.trim()).filter(Boolean))]; | ||
| const normalizedPublisherPeerId = publisherPeerId.trim(); | ||
| const publisherAddress = this.requirePublisherAddress('publish'); |
There was a problem hiding this comment.
🔴 Bug: This hard-fails every publish() call when no publisher EVM address is configured, which breaks the existing tentative/no-chain flow. DKGAgent.create() only passes publisherPrivateKey: opKeys?.[0], and its public contract still says omitting chainConfig should simply skip on-chain finality. After this change, those callers throw before any local publish work happens. Only require a publisher address/signer on the paths that actually need signing, or plumb an explicit fallback publisher address through the agent config before landing this.
| const normalizedAllowedPeers = [...new Set((allowedPeers ?? []).map((p) => p.trim()).filter(Boolean))]; | ||
| const normalizedPublisherPeerId = publisherPeerId.trim(); | ||
| const publisherAddress = this.publisherAddress ?? this.localTentativePublisherAddress(); | ||
| const canAttemptOnChainPublish = this.publisherNodeIdentityId > 0n && this.getPublisherSigner() !== undefined; |
There was a problem hiding this comment.
🔴 Bug: This no longer enforces the new "fail closed" behavior when the publisher has no signing key. If publisherNodeIdentityId is still 0n, publish() will keep going, write data, and return a tentative result instead of surfacing a configuration error. That means a misconfigured EVM node can silently downgrade to local-only publishes, and the new phase-sequence test below won't actually match runtime behavior. If missing publisher credentials should be fatal, reject before any phase/store work.
There was a problem hiding this comment.
Codex review produced 1 comment(s) but all targeted lines outside the diff and were dropped. Check the workflow logs for details.
| if (privateQuads.length > 0) rejectReservedSubjectPrefixes(privateQuads); | ||
| } | ||
| const ctx: OperationContext = operationCtx ?? createOperationContext('publish'); | ||
| const publisherAddress = this.requirePublisherAddress('update'); |
There was a problem hiding this comment.
🔴 Bug: this makes the default no-chain/tentative publisher impossible to update. publish() now falls back to localTentativePublisherAddress() when no EVM publisher key/address exists, but update() hard-requires this.publisherAddress, which is still undefined in that mode. A caller can successfully create a did:dkg:none/... KC and then never update it with the same DKGPublisher. Reuse the same deterministic fallback here (or persist it at construction time) so no-chain publishes remain updateable.
| const node = new DKGNode(nodeConfig); | ||
| const workspaceOwnedEntities = new Map<string, Map<string, string>>(); | ||
| const writeLocks = new Map<string, Promise<void>>(); | ||
| const publisherAddress = config.publisherAddress ?? ( |
There was a problem hiding this comment.
🔴 Bug: this infers the adapter-backed publisher address only once during DKGAgent.create(). If getSignerAddresses() / signMessage() fails transiently here (cold RPC, hardware signer locked, startup race), publisherAddress is frozen as undefined and every later publish fails with PublisherWalletRequiredError even after the adapter becomes healthy. Please defer this inference to publish/update time, or add a retry path when the first publish is attempted.
| publisherAddress: config.publisherAddress, | ||
| publisherAddressResolver: opKeys?.[0] || config.publisherAddress | ||
| ? undefined | ||
| : () => inferAdapterPublisherAddress(chain), |
There was a problem hiding this comment.
🔴 Bug: This resolver picks one adapter address up front, but the EVM adapter chooses the actual tx signer per context graph via nextAuthorizedSigner(). In multi-signer setups those can diverge, which means the publisher/ACK/authorship signatures come from signer A while the publish tx and resulting UAL are attributed to signer B. Please make the resolved publisher address context-graph-aware or pass the chosen address through to the chain submit path so both sides use the same key.
| } | ||
|
|
||
| async function inferAdapterPublisherAddress(chain: ChainAdapter): Promise<string | undefined> { | ||
| const signerAddresses = (chain as unknown as { getSignerAddresses?: () => unknown }).getSignerAddresses; |
There was a problem hiding this comment.
🟡 Issue: inferAdapterPublisherAddress() no longer probes getSignerAddress(), even though the agent already treats that as the standard single-signer hook elsewhere. Any custom adapter that exposes only getSignerAddress() will now fail address inference and hit PublisherWalletRequiredError despite being able to report its signer. Consider checking getSignerAddress() before falling back to signMessage().
| const seenAllowlistPaths = new Set(); | ||
| for (const pkg of pkgs) { | ||
| if (!pkg.isDirectory() || pkg.name.startsWith('.')) continue; | ||
| for (const subdir of ['src', 'utils']) { |
There was a problem hiding this comment.
🟡 Issue: The audit only walks packages/*/{src,utils}, so CI will miss new Wallet.createRandom() calls in root operational scripts or package-level scripts/ directories. There is already a non-test use in scripts/distribute-publisher-trac.ts, so this leaves a real escape hatch for the regression the audit is supposed to prevent. Widen the scan roots or explicitly allowlist those directories.
| readonly knownBatchContextGraphs: Map<string, string>; | ||
| private publisherNodeIdentityId: bigint; | ||
| private readonly publisherAddress: string; | ||
| private publisherAddress?: string; |
There was a problem hiding this comment.
🔴 Bug: publisherAddress is now mutable instance-wide state, but publish() can yield multiple times before it re-reads the signer via requirePublisherSigner(). Two concurrent publishes on the same DKGPublisher for different context graphs can overwrite this field mid-flight, causing the first call to sign with the second call's address or fail the recovered-address check. Keep the resolved address/signer in per-operation locals and thread that through the publish flow instead of mutating shared state.
| const seen = new Set(); | ||
|
|
||
| for (const alias of walletAliases) { | ||
| const pattern = new RegExp(String.raw`\b${escapeRegExp(alias)}\s*\.\s*createRandom\s*\(`, 'g'); |
There was a problem hiding this comment.
🟡 Issue: this matcher only catches Alias.createRandom(, so equivalent spellings like Wallet?.createRandom() or Wallet['createRandom']() bypass the audit entirely. Because this script is meant to be a hard regression gate for discarded key generation, the detection should cover these variants (or move to an AST-based check) and add regression tests for them.
|
|
||
| for (const pkg of pkgs) { | ||
| if (!pkg.isDirectory() || pkg.name.startsWith('.')) continue; | ||
| for (const subdir of ['src', 'utils', 'scripts']) { |
There was a problem hiding this comment.
🟡 Issue: the scan only walks packages/*/{src,utils,scripts}, but this repo has runtime JS entrypoints outside those folders (for example setup-entry.mjs, openclaw-entry.mjs, and hook files under packages/mcp-dkg). A new Wallet.createRandom() in one of those production files would pass CI unnoticed. Widen the walk to all non-test JS/TS under packages/, or explicitly include the known runtime entrypoint locations.
|
|
||
| if (stripped.startsWith('?.[', i)) { | ||
| i += 3; | ||
| const property = readQuotedProperty(originalText, skipWhitespace(originalText, i)); |
There was a problem hiding this comment.
🔴 Bug: This parser switches back to originalText before reading the bracket property, so comments between [ and the quoted key are not skipped. A call like Wallet[/*gap*/"createRandom"]() currently returns no hits, which lets the new CI audit be bypassed by formatting alone. Parse against the stripped buffer here (and in the plain [ branch below), and add a regression test for the comment-in-brackets form.
| }; | ||
| const canStartRegexLiteral = () => { | ||
| const prev = previousSignificantChar(); | ||
| return prev === '' || '([{=,:;!&|?+-*~^<>'.includes(prev); |
There was a problem hiding this comment.
🔴 Bug: this regex-start heuristic is too narrow for a security gate. Regex literals after keywords like return/throw are treated as normal code, so // or /* inside the regex body re-enters comment mode and can hide a later Wallet.createRandom(). Repro: function f(){ return /\\/\\//; Wallet.createRandom(); } currently produces no hits. Please make regex detection token-aware (or parser-backed) and add a regression test for keyword-prefixed regex literals.
|
|
||
| if (stripped.startsWith('?.[', i)) { | ||
| i += 3; | ||
| const property = readQuotedProperty(originalText, skipWhitespace(originalText, i)); |
There was a problem hiding this comment.
🔴 Bug: bracket-property calls can still bypass the audit when a comment appears between [ and the quoted property. Wallet[/*x*/'createRandom']() and Wallet?.[/*x*/'createRandom']?.() currently return no hits because this path skips whitespace in originalText, not comments in the stripped buffer. Advance using the comment-stripped text here (or explicitly skip comments) before calling readQuotedProperty, and add regression coverage for both bracket forms.
|
|
||
| // Destructuring aliases from ethers: const { Wallet: W } = ethers; | ||
| const destructurePattern = new RegExp( | ||
| String.raw`\b(?:const|let|var)\s*\{([^}]*)\}\s*=\s*ethers\b`, |
There was a problem hiding this comment.
🔴 Bug: alias discovery hardcodes the identifier ethers, so namespace-import aliases evade the CI check. import * as E from 'ethers'; const { Wallet: W } = E; const w = W.createRandom(); currently yields no hits. Since this script is meant to be bypass-resistant, track namespace-import aliases and use them in the destructuring/assignment alias patterns instead of matching only the literal ethers.
|
|
||
| function adapterAdvertisesPublisherSigner(chain: ChainAdapter): boolean { | ||
| if (typeof chain.signMessageAs === 'function') return true; | ||
| if (typeof (chain as unknown as { getOperationalPrivateKey?: unknown }).getOperationalPrivateKey === 'function') return true; |
There was a problem hiding this comment.
🔴 Bug: treating getOperationalPrivateKey() as an "adapter-backed signer" changes precedence for callers that explicitly pass chainConfig.operationalKeys alongside a custom adapter. In that case useLegacyAdapterOperationalKeyFallback becomes false, so publishes stop using the configured key and silently switch back to the adapter's own operational key. That can break CGs where only the configured key is authorized. getOperationalPrivateKey() should stay in the legacy/private-key fallback path; only real adapter-held signing surfaces like signMessageAs() should suppress the explicit operationalKeys signer.
| } | ||
|
|
||
| const kcId = this.nextBatchId++; | ||
| const publisherAddress = params.publisherAddress ?? this.signerAddress; |
There was a problem hiding this comment.
🟡 Issue: the mock now accepts any caller-supplied publisherAddress, even though it only has a single real signer (this.signerAddress). That weakens parity with EVMChainAdapter, which rejects unknown/unavailable publisher addresses, so tests can now pass with signer selections that would fail on a real chain. Consider rejecting non-matching overrides here (or adding explicit mock support for address-specific signing) instead of silently rewriting on-chain attribution.
| } | ||
|
|
||
| if (!txResult.success) { | ||
| const failedPublisherAddress = coercePublisherAddress(txResult.publisherAddress) ?? |
There was a problem hiding this comment.
🔴 Bug: This failed-update path now throws whenever neither txResult.publisherAddress nor the caller hint is populated, but the new ChainAdapter contract only requires publisher attribution for successful updates. Adapters that rely on getLatestMerkleRootPublisher() will now surface an exception instead of the structured { status: 'failed' } result for revert receipts. Reuse the same getLatestMerkleRootPublisher(kcId) fallback here before throwing.
| if (typeof signerAddressGetter === 'function') { | ||
| try { | ||
| const address = normalizeAdapterPublisherAddress( | ||
| await Promise.resolve(signerAddressGetter.call(chain)), |
There was a problem hiding this comment.
🟡 Issue: This adds async getSignerAddress() support here, but the agent still reads getSignerAddress() synchronously in getChainPublishAuthorityAddress() during curated context-graph registration. A custom adapter that follows this new async pattern will publish fine but can still fail registerContextGraph() with the "without chain signer introspection" path for non-default curators. Either keep getSignerAddress() sync-only, or update that registration guard to await async results too.
| const challenge = ethers.getBytes(ethers.id(`dkg-publisher:chain-signer-probe:${expectedAddress.toLowerCase()}`)); | ||
| let signMessageMatches = false; | ||
| try { | ||
| const compact = await this.chain.signMessage(challenge); |
There was a problem hiding this comment.
🟡 Issue: This uses a real chain.signMessage() call as an address probe before we even know whether the publish/update will submit on-chain. For HSM/remote signers that means an extra signature round-trip or user prompt on every call, and a transient probe failure here degrades a usable signer into PublisherWalletRequiredError. Prefer explicit address introspection (or cache a successful probe) instead of spending an actual signature just to discover signer identity.
| throw new Error('Publish rejected: "allowedPeers" is only valid when accessPolicy is "allowList"'); | ||
| } | ||
|
|
||
| if (willAttemptOnChainPublish && chainAdvertisesV10Publish && !publisherSigner) { |
There was a problem hiding this comment.
🔴 Bug: this capability check keys off createKnowledgeAssetsV10 existing, but EVMChainAdapter exposes that method even when isV10Ready() is false (for example on a V9-only or misconfigured chain). In that case a publish with no local signer now throws here instead of taking the existing tentative fallback. Gate this on actual V10 readiness, not just method presence.
| `Failed to sync _meta merkleRoot for kcId=${kcId}: ${err instanceof Error ? err.message : String(err)}`, | ||
| onPhase?.('chain:submit', 'end'); | ||
| onPhase?.('chain', 'end'); | ||
| if (!effectivePublisherAddress) { |
There was a problem hiding this comment.
🔴 Bug: this makes publisherAddress effectively mandatory for every successful update even though TxResult.publisherAddress is still optional in the public ChainAdapter contract. Any external adapter that still returns the old success shape will mine the update and then fail locally here. Either keep a backward-compatible fallback/deprecation path, or make the contract change explicit in a major-version interface update.
| return i; | ||
| } | ||
|
|
||
| function readQuotedProperty(originalText, index) { |
There was a problem hiding this comment.
🔴 Bug: readQuotedProperty() only accepts ' and ", so Wallet[createRandom]() / Wallet?.[createRandom]?.() are missed entirely. Because this script is intended to be a security gate, that leaves a straightforward syntax-level bypass for the banned call. Extend the computed-property parser to handle template-literal keys (at least the no-substitution form), and add a regression test for it.
| // As with named imports below, the module specifier string has been | ||
| // blanked by the lexer, so this intentionally treats namespace imports | ||
| // conservatively rather than allowing `ethers` aliases to bypass the gate. | ||
| const namespaceImportPattern = new RegExp(String.raw`\bimport\s+\*\s+as\s+(${IDENT})\s+from\b`, 'g'); |
There was a problem hiding this comment.
🔴 Bug: the alias collector only seeds namespace aliases from bare ethers and import * as ..., so patterns like import { ethers as E } from 'ethers'; const W = E.Wallet; W.createRandom() or CommonJS require('ethers') aliases in .js/.cjs files evade the audit. Since the scanner explicitly walks JS/CJS sources, this can produce false negatives in CI. Teach collectWalletAliases() to recognize those namespace bindings, or narrow the scanned file set to forms the lexer can analyze soundly.
| } catch { | ||
| // Descriptive SWM graph names stay on the existing tentative/mock path. | ||
| } | ||
| const resolvedPublisherAddress = await this.resolvePublisherAddress(publisherContextGraphId); |
There was a problem hiding this comment.
🔴 Bug: signer resolution happens before refreshChainV10Readiness(). For adapter-backed signing, some adapters only expose a usable signer after their V10 init/probe path runs, so this first probe can return undefined, publisherSigner stays unset, and we throw PublisherWalletRequiredError even though the adapter becomes ready a few lines later. Initialize first, or retry resolvePublisherAddress()/getPublisherSigner() after the readiness probe when the first attempt fails.
| signerAddress?: string; | ||
| }; | ||
| const rawAddress = chainWithSigner.getSignerAddress?.() ?? chainWithSigner.signerAddress; | ||
| const rawAddress = chainWithSigner.getSignerAddress |
There was a problem hiding this comment.
🔴 Bug: this curated-registration probe now awaits getSignerAddress(), but it still treats that one method as the only introspection surface and lets probe failures bubble. Adapters that publish via the new getAuthorizedPublisherAddress() / getSignerAddresses() hooks, or adapters whose async signer probe rejects until initialized, will now fail registration even though publishing works. Reuse the same best-effort publisher-address resolver used elsewhere here and swallow probe errors to undefined.
| onPhase?.('chain:submit', 'end'); | ||
| onPhase?.('chain', 'end'); | ||
| if (!effectivePublisherAddress) { | ||
| throw new Error( |
There was a problem hiding this comment.
🔴 Bug: this turns a successful update into a runtime exception for any existing adapter that still returns the old TxResult shape without publisherAddress and does not implement getLatestMerkleRootPublisher(). Because publisherAddress is still optional in the public interface, out-of-tree adapters will continue to compile and then start failing all confirmed updates here. Either make this a versioned contract change, or keep a backward-compatible fallback until adapters are updated.
| typeof (chain as unknown as { getSignerAddresses?: unknown }).getSignerAddresses === 'function' || | ||
| Boolean(normalizeAdapterPublisherAddress((chain as unknown as { signerAddress?: unknown }).signerAddress)); | ||
|
|
||
| return hasAddressProbe && typeof chain.signMessageAs === 'function'; |
There was a problem hiding this comment.
🔴 Bug: This gate only recognizes delegated publishers when the adapter implements signMessageAs(). Single-signer adapters that expose getSignerAddress()/signMessage() but not signMessageAs() are still capable of delegated signing, yet DKGAgent.create() will treat them as legacy and fall back to chainConfig.operationalKeys[0] when that key is present. That can reintroduce the exact address-mismatch this PR is trying to prevent, because publishes may be signed with the fallback key instead of the adapter's advertised signer. Treat a single resolvable signer address plus signMessage() as adapter-backed too, or avoid the private-key fallback in that case.
| return this.txResult(true); | ||
| return { | ||
| ...this.txResult(true), | ||
| publisherAddress: this.signerAddress, |
There was a problem hiding this comment.
🔴 Bug: The mock update path still hard-codes this.signerAddress as the effective publisher. After this PR, a KC can be created under a different publisherAddress, but a subsequent mock-backed update will rewrite TxResult.publisherAddress (and the emitted event above) back to the default signer. DKGPublisher.update() now trusts that field first, so tests will produce confirmed UALs/metadata with the wrong publisher and drift from the EVM adapter. Return the stored batch publisher (or the validated params.publisherAddress) here, and make the V9 update path do the same.
| } | ||
| const publishAuthority = publishPolicy === EVM_PUBLISH_CURATED | ||
| ? this.getChainPublishAuthorityAddress() | ||
| ? await this.getChainPublishAuthorityAddress(id) |
There was a problem hiding this comment.
🔴 Bug: This now derives curated registration authority through getChainPublishAuthorityAddress(), which falls back to generic chain.signMessage() inference. That breaks adapters where signMessage() uses a different key than the actual publish path (for example, publish still falls back to chainConfig.operationalKeys[0]): registration can reject the real curator or store the wrong publishAuthority. Keep registration authority resolution aligned with the publish fallback rules instead of treating bare signMessage() as authoritative here.
| const entityPrivateQuads = entityPrivateMap.get(rootEntity) ?? []; | ||
| if (entityPrivateQuads.length > 0) { | ||
| await this.privateStore.storePrivateTriples(contextGraphId, rootEntity, entityPrivateQuads, options.subGraphName); | ||
| let effectivePublisherAddress = coercePublisherAddress(txResult.publisherAddress) ?? |
There was a problem hiding this comment.
🔴 Bug: This still upgrades the update to confirmed from non-chain state when the adapter does not report publisherAddress. publisherAddress here is only the caller hint, and the later resolveKnownBatchPublisherAddress() fallback is just whatever UAL was last stored locally. If the adapter ignores the hint or rotates signers, we will persist confirmed metadata with the wrong publisher. If neither TxResult.publisherAddress nor getLatestMerkleRootPublisher() is available, this path should remain tentative instead of reusing local metadata.
| typeof (chain as unknown as { getSignerAddress?: unknown }).getSignerAddress === 'function' || | ||
| typeof (chain as unknown as { getSignerAddresses?: unknown }).getSignerAddresses === 'function' || | ||
| Boolean(normalizeAdapterPublisherAddress((chain as unknown as { signerAddress?: unknown }).signerAddress)); | ||
| const hasSigningProbe = typeof chain.signMessageAs === 'function' || |
There was a problem hiding this comment.
🔴 Bug: signMessage is not a safe substitute for signMessageAs when the adapter advertises multiple possible publisher addresses (getAuthorizedPublisherAddress / getSignerAddresses). This marks those adapters as signer-capable, disables the operationalKeys[0] fallback, and then publish fails later because the generic signer cannot produce a signature for the reserved address. Require signMessageAs for multi-signer surfaces, or only treat plain signMessage as sufficient when there is exactly one deterministic signer address.
| } catch { | ||
| // Local descriptive CG ids cannot be used as adapter context hints. | ||
| } | ||
| return inferAdapterPublisherAddress(this.chain, publisherContextGraphId); |
There was a problem hiding this comment.
🔴 Bug: getChainPublishAuthorityAddress() now goes through inferAdapterPublisherAddress(), which probes getAuthorizedPublisherAddress(). That API is stateful on EVMChainAdapter and advances the signer reservation queue, so this read-only registration check can change future publish signer selection and make curated registration nondeterministic on repeated calls. Use a non-reserving address probe here, or skip getAuthorizedPublisherAddress() entirely for authority checks.
| } catch { | ||
| // Descriptive SWM graph names are valid local/mock update scopes. | ||
| } | ||
| const resolvedPublisherAddress = await this.resolvePublisherAddress(publisherContextGraphId); |
There was a problem hiding this comment.
🔴 Bug: update now resolves publisherAddress through the same publish-time address discovery path, including getAuthorizedPublisherAddress(contextGraphId). That API picks the next authorized signer for a new publish, not the existing KC publisher, so on adapters that cannot read getLatestMerkleRootPublisher() reliably this can feed the wrong signer into updateKnowledgeCollectionV10() and cause NotBatchPublisher failures. For updates, prefer the stored/on-chain KC publisher and otherwise leave publisherAddress undefined so the adapter can resolve the original publisher itself.
| } | ||
| const willAttemptOnChainPublish = this.publisherNodeIdentityId > 0n && publisherContextGraphId !== undefined; | ||
| const chainV10Ready = await this.refreshChainV10Readiness(); | ||
| const resolvedPublisherAddress = await this.resolvePublisherAddress(publisherContextGraphId); |
There was a problem hiding this comment.
🔴 Bug: resolvePublisherAddress() can call getAuthorizedPublisherAddress(), which this PR defines as a real signer reservation. Calling it before the identityId / positive CG id / V10-ready checks means even purely tentative publishes can consume or advance a signer with no tx ever submitted. On signer-pool adapters that will skew or leak the next real publish selection. Defer reserving probes until you know the publish will go on-chain, and use a non-reserving lookup or the local tentative address on the soft path.
| if (matches) this.adapterSignMessagePublisherAddress = expectedAddress; | ||
| return matches; | ||
| } catch { | ||
| this.adapterSignMessageProbeCache.set(cacheKey, false); |
There was a problem hiding this comment.
🔴 Bug: caching false on any signMessage() probe error makes transient failures permanent for that address. A startup/RPC timeout here means every later publish/update in this process will skip adapter-backed signing even after the signer recovers. Only cache successful matches/mismatches, or invalidate negative entries after probe failures.
| try { | ||
| const ual = await resolveUalByBatchId( | ||
| this.store, | ||
| this.graphManager.metaGraphUri(contextGraphId), |
There was a problem hiding this comment.
🔴 Bug: this fallback hard-codes the default _meta graph. KAs published into a subGraphName or custom targetMetaGraphUri will not be found here, so updates can now downgrade to tentative or throw PublisherWalletRequiredError even though the confirmed metadata exists locally. Please thread the actual meta-graph URI/subgraph through this resolver.
| } | ||
| } | ||
| failedPublisherAddress ??= await this.resolveKnownBatchPublisherAddress(contextGraphId, kcId); | ||
| if (!failedPublisherAddress) throw new PublisherWalletRequiredError('update'); |
There was a problem hiding this comment.
🔴 Bug: when the adapter returns success: false but no publisher address, this now throws PublisherWalletRequiredError, which rewrites a real chain/update failure into a misleading config error. That is a regression for cases like unknown/missing batches or rejected updates. Preserve the underlying update failure here instead of converting it to a wallet-required error.
| } | ||
| const willAttemptOnChainPublish = this.publisherNodeIdentityId > 0n && publisherContextGraphId !== undefined; | ||
| const chainV10Ready = await this.refreshChainV10Readiness(); | ||
| const resolvedPublisherAddress = await this.resolvePublisherAddress(publisherContextGraphId); |
There was a problem hiding this comment.
🔴 Bug: resolvePublisherAddress() can call getAuthorizedPublisherAddress() and advance adapter signer-pool state even when this publish is guaranteed to stay tentative (for example publisherNodeIdentityId === 0 or the chain never becomes V10-ready). That means an identity-less/tentative publish can consume a reserved signer and change which signer a later real on-chain publish uses. Gate signer resolution behind the same conditions that actually allow an on-chain publish, or make the tentative path avoid reservation-style probes entirely.
| const publisherAddress = params.publisherAddress | ||
| ? ethers.getAddress(params.publisherAddress) | ||
| : ethers.getAddress(this.signerAddress); | ||
| if (!this.allowedPublisherAddresses.has(publisherAddress.toLowerCase())) { |
There was a problem hiding this comment.
🟡 Issue: this makes the mock reject delegated publishes unless the caller also seeds the publisher address as an identity, which is stricter than the real EVM adapter. On-chain delegated publisher addresses do not need a separate identity registration, so tests/off-chain usage can now fail for flows that would succeed in production. Consider a dedicated mock allowlist for delegated signer addresses instead of tying it to seedIdentity().
| // original publisher while submitting the transaction. | ||
| } | ||
| } | ||
| if (!resolvedPublisherAddress && !localOnlyUpdate) { |
There was a problem hiding this comment.
🔴 Bug: This update path no longer falls back to the locally configured publisher signer when chain state and _meta do not resolve the original publisher. In a fresh process or any store that does not have the prior batch metadata, publisherAddress stays undefined, so the adapter can pick an arbitrary signer and the update can fail with NotBatchPublisher even though the caller supplied the correct publisherPrivateKey/publisherAddress. Keep the configured publisher address (or resolvePublisherAddress(...)) as the final fallback before submitting the update.
| const hasAnyAddressProbe = hasReservingOrMultiAddressProbe || hasSingleAddressProbe; | ||
|
|
||
| if (typeof chain.signMessageAs === 'function') return hasAnyAddressProbe; | ||
| return !hasReservingOrMultiAddressProbe && |
There was a problem hiding this comment.
🔴 Bug: This helper assumes getSignerAddress() + signMessage() means the adapter can act as the publish signer, and that suppresses the existing operationalKeys[0] fallback in DKGAgent.create(). That is not safe: the new tests in this PR already show generic signMessage() can belong to a different key. For single-address adapters with an unrelated signMessage(), publishes now regress from working with chainConfig.operationalKeys[0] to failing at runtime. Only disable the legacy-key fallback after a positive address/signature match, or keep the fallback until publish-time verification succeeds.
| const configuredPublisherAddress = normalizeAdapterPublisherAddress(this.config.publisherAddress); | ||
| if (configuredPublisherAddress) return configuredPublisherAddress; | ||
|
|
||
| const legacyAdapterOperationalKey = this.config.chainConfig?.operationalKeys?.[0]; |
There was a problem hiding this comment.
🔴 Bug: this only looks at chainConfig.operationalKeys[0], but create() now also derives the legacy publisher key from chainAdapter.getOperationalPrivateKey(). For adapters that expose only getOperationalPrivateKey() (for example the new OperationalKeyOnlyPublishChainAdapter path), publish uses that fallback key while curated registration sees no publish authority here, so registerContextGraph() can reject a valid owner or register without the actual authority address. Reuse the same derived adapter key/address here (or persist it on the agent) so registration and publish stay consistent.
1 participant
Summary
Closes the second instance of the anti-pattern that destroyed nine testnet admin keys at registration time on 2026-05-01–02 (see investigation chain on identities 1–9: each created via pre-PR-#366
EVMChainAdapter.ensureProfile()which generatedethers.Wallet.createRandom(), used it once increateProfile, then dropped it without persistence).PR #366 fixed
ensureProfileitself. This PR closes the matching bug inDKGPublisherand adds a CI gate so the pattern cannot creep back.What changes
packages/publisher/src/dkg-publisher.ts— Constructor previously generatedethers.Wallet.createRandom()wheneverchain.chainId !== 'none'and nopublisherPrivateKeywas supplied, then used that ephemeral wallet to sign on-chain publish digests, ACK self-signatures, and authorship proofs. Resulting signatures were attributed (via the separately-suppliedpublisherAddress) to a different address than the one that actually signed — verifiably-junk by construction. Constructor now leavespublisherWalletundefined; every signing call site is already guarded byif (this.publisherWallet)and skips gracefully (verified at lines 1109, 1231, 1270, 1438).scripts/audit-create-random.mjs— Dependency-free Node script that walkspackages/*/src/**/*.{ts,js,mjs,cjs}and fails onWallet.createRandom(outside three explicitly justified call sites:packages/agent/src/op-wallets.ts— first-run admin+op wallet generation, persisted towallets.json(chmod 0600)packages/agent/src/agent-keystore.ts— custodial chat-agent keypair, returned to caller and persisted in keystorepackages/evm-module/utils/helpers.ts— hardhat deploy-script utility, key returned to operatorSkips comment lines (so PR descriptions of historical fixes don't trip it) and test files (which routinely use random keys for fixtures). Runs in <300 ms on every CI build.
.github/workflows/ci.yml— Run the audit immediately after checkout in thebuildjob. Fails fast beforepnpm install.packages/publisher/test/publisher-no-random-wallet.test.ts— Regression test covering all three constructor branches:chainId !== 'none') + no key →publisherWalletisundefinedpublisherWalletisundefinedpublisherWalletis constructed correctlyCHANGELOG.md— entry under[Unreleased]documenting both the fix and the new audit gate.Why an audit script (vs ESLint rule)
This repo has no ESLint configuration anywhere —
turbo lintexists but no package implements the task. Adding ESLint just for one rule is disproportionate; a 70-line dependency-free Node script slotted into the existing CI provides the same regression-prevention with less infrastructure.Test plan
Local verification (already run):
node scripts/audit-create-random.mjs→ exit 0 on the fixed treeethers.Wallet.createRandom()into a non-allowlisted file → audit exits 1 with a precise file/line/snippetpnpm --filter @origintrail-official/dkg-publisher exec vitest run test/publisher-no-random-wallet.test.ts→ 3/3 passpnpm --filter @origintrail-official/dkg-publisher exec vitest run test/share-size-boundary-extra.test.ts test/dkg-publisher.test.ts→ 39/39 pass (closest pre-existing tests to the change)pnpm --filter @origintrail-official/dkg-publisher build→ tsc cleanCI to verify:
Audit Wallet.createRandom usagestep runs and passes inbuildjobDKGPublisherwithout a key — theif (this.publisherWallet)guards mean those publishers cleanly no-op signing)Operational note
Does NOT change the behavior of
loadOpWalletsfirst-run — that path still auto-generates an admin+op-wallet bundle and persists towallets.json. PR #366 already established that pattern; the random key is recoverable because it's saved to disk before any on-chain transaction uses it. The audit allowlist covers it explicitly.Made with Cursor