-
Notifications
You must be signed in to change notification settings - Fork 5
fix(cli): gate /api/shared-memory/publish on curator identity #299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1358,6 +1358,18 @@ | |
| expect(chain.createOnChainContextGraphCalls[2]?.publishAuthority).toBe(ethers.getAddress(chain.signerAddress)); | ||
| expect(chain.createOnChainContextGraphCalls[2]?.participantAgents).toEqual([]); | ||
|
|
||
| // `isContextGraphCurated` must mirror the EVM publish-policy | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Issue: This only pins the helper mapping, not the HTTP regression the PR is fixing.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair — the agent-side test pins the helper but doesn't exercise the HTTP surface. I didn't add a daemon-level integration test here because the existing pattern ( Happy to add a unit-level daemon test using a stubbed |
||
| // mapping above — HTTP routes (see | ||
| // packages/cli/src/daemon/routes/memory.ts) rely on it to decide | ||
| // whether to preflight a curator-only owner gate before | ||
| // `publishFromSharedMemory`. If this ever drifts from the policy | ||
| // the chain adapter actually enforces, non-curator publishes to | ||
| // open CGs will be rejected with 403 even though the contract | ||
| // accepts them (Codex PR#299 review finding). | ||
| expect(await agent.isContextGraphCurated('register-open-policy')).toBe(false); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Issue: these assertions only pin the helper mapping; they don't cover the HTTP behavior that changed in this PR. Please add a daemon-level regression test for |
||
| expect(await agent.isContextGraphCurated('register-curated-policy')).toBe(true); | ||
| expect(await agent.isContextGraphCurated('register-agent-allowlist-policy')).toBe(true); | ||
|
|
||
| await agent.stop().catch(() => {}); | ||
| }); | ||
|
|
||
|
|
@@ -1998,7 +2010,7 @@ | |
|
|
||
| await agent.syncFromPeer('12D3KooWPerContextGraphDeadline111111111111111111111111', ['cg-a', 'cg-b']); | ||
|
|
||
| expect(deadlines).toHaveLength(4); | ||
|
Check failure on line 2013 in packages/agent/test/agent.test.ts
|
||
| expect(deadlines[0]).toBe(1_060_000); | ||
| expect(deadlines[1]).toBe(1_060_000); | ||
| expect(deadlines[2]).toBe(1_120_000); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -445,6 +445,53 @@ export async function handleMemoryRoutes(ctx: RequestContext): Promise<void> { | |
| '"subGraphName" and "publishContextGraphId" cannot be used together', | ||
| }); | ||
| } | ||
|
|
||
| // Policy-aware preflight (spec §2.2): | ||
| // | ||
| // - Curated CGs (on-chain `publishPolicy = EVM_PUBLISH_CURATED`, | ||
| // which `registerContextGraph` sets for private CGs or any CG | ||
| // with an allowlist): only the registered curator may VM-publish. | ||
| // Without this gate, non-curator callers hit the on-chain | ||
| // `isAuthorizedPublisher` revert deep in the stack and the HTTP | ||
| // surface returns 200 with `status=tentative` — masking the | ||
| // authorization failure and leaving phantom tentative metadata | ||
| // on disk. | ||
| // | ||
| // - Open CGs (on-chain `publishPolicy = EVM_PUBLISH_OPEN`): any | ||
| // non-zero collaborator may publish per the contract; we must | ||
| // NOT gate on curator identity here or we'd reject legitimate | ||
| // participant publishes with 403. | ||
| // | ||
| // Preflight applies only to the curated branch; open CGs fall | ||
| // through to the publisher and the chain adapter decides. | ||
| // | ||
| // Known scope gap (Codex PR#299 review, tracked separately): | ||
| // `assertContextGraphOwner` compares against the locally stored | ||
| // `dkg:curator` wallet DID. The on-chain | ||
| // `ContextGraphs.isAuthorizedPublisher` is richer — for PCA | ||
| // curators it live-resolves the NFT owner and any | ||
| // `agentToAccountId`-registered agents. This preflight therefore | ||
| // over-rejects PCA-delegated agents and post-transfer NFT holders | ||
| // whose wallets don't match the stale local curator metadata. The | ||
| // same shape of check is already used by share, invite, rename, | ||
| // and manifest-publish routes — migrating them all to a | ||
| // chain-authoritative preflight is a separate follow-up. Until | ||
| // then, those callers can work around this by publishing from the | ||
| // wallet recorded as the CG's local curator. | ||
| if (await agent.isContextGraphCurated(paranetId)) { | ||
| try { | ||
| await agent.assertContextGraphOwner( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Bug:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Valid concern, acknowledged as a known scope gap rather than a fresh regression:
Migrating all of these routes to a chain-authoritative preflight ( In the meantime, affected callers can work around it by publishing from the wallet recorded as the local curator. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Bug: this preflight uses |
||
| paranetId, | ||
| requestAgentAddress, | ||
| "publish shared memory to Verified Memory", | ||
| ); | ||
| } catch (authErr: unknown) { | ||
| const msg = authErr instanceof Error ? authErr.message : String(authErr); | ||
| const code = /has no registered owner/.test(msg) ? 400 : 403; | ||
| return jsonResponse(res, code, { error: msg }); | ||
| } | ||
| } | ||
|
|
||
| const ctx = createOperationContext("publishFromSWM"); | ||
| tracker.start(ctx, { | ||
| contextGraphId: paranetId, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Bug:
isPrivateContextGraph()is only local metadata state, not the live on-chainpublishPolicythis helper claims to mirror. If a CG's publish policy is updated on chain, or the node has stale discovery data,/api/shared-memory/publishwill take the wrong auth branch and either 403 a valid publish or fall back to the masked200/tentativefailure this PR is trying to prevent. Please read/sync the actual on-chain publish policy instead of inferring it from local access metadata.