fix(cli): gate /api/shared-memory/publish on curator identity#299
fix(cli): gate /api/shared-memory/publish on curator identity#299branarakic wants to merge 3 commits intomainfrom
Conversation
Spec §2.2 says only the CG's registered curator may promote SWM to
Verified Memory, but the daemon had no caller-level auth check on
`/api/shared-memory/publish` (or the legacy `/api/workspace/enshrine`
alias). Non-curator callers reached the publisher, hit on-chain
`UnauthorizedPublisher` revert, and got HTTP 200 with
`status=tentative` — masking the authorization failure and leaving
phantom tentative metadata on disk.
Add the same owner-check the project-manifest-publish route already
uses (`agent.assertContextGraphOwner`), mapping the thrown error to
403 (not the owner) or 400 (CG has no registered owner yet). The
helper is the single source of truth for "caller is curator" across
the daemon, so the semantics here stay in lockstep with share,
invite, rename, and manifest-publish gates.
Fixes devnet-test §4e ("Non-curator publish should be rejected with
4xx, got HTTP 200").
Made-with: Cursor
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // spec-conformant rejection (matches the project-manifest-publish | ||
| // gate in context-graph.ts). | ||
| try { | ||
| await agent.assertContextGraphOwner( |
There was a problem hiding this comment.
🔴 Bug: assertContextGraphOwner() makes this route curator-only for every publish, but the actual publish contract is policy-based, not owner-only. registerContextGraph() maps open/public context graphs to publishPolicy = open, and ContextGraphs.isAuthorizedPublisher() explicitly allows any non-zero collaborator there. With this preflight, valid publishes from non-curator participants will now be rejected with 403 before the on-chain authorization logic runs. Please make the check policy-aware (only owner-gate curated CGs, or delegate to the same isAuthorizedPublisher logic) and add a regression test for open-CG publish success.
Codex review on #299 flagged that the preflight `assertContextGraphOwner` rejected every non-curator publish, including legitimate publishes to open context graphs where `ContextGraphs.isAuthorizedPublisher` accepts any non-zero collaborator. Make the HTTP gate policy-aware: - `DKGAgent.isContextGraphCurated(cgId)` exposes the existing private `isPrivateContextGraph` check (invite-only allowlist → curated). - `handleMemoryRoutes` only preflights the owner check when the CG is curated; open CGs proceed to the publisher and let the chain adapter arbitrate (which is what §2.2 of the publish spec describes). - Extend the EVM-policy-mapping unit test to pin `isContextGraphCurated` against the three publish-policy variants it already constructs, so this can't silently drift from the on-chain gate again. Devnet §4e (non-curator → 4xx) still holds because curated CGs are still gated; open-CG collaborator publishes (e.g. §4a-d) no longer hit a spurious 403. Made-with: Cursor
| // through to the publisher and the chain adapter decides. | ||
| if (await agent.isContextGraphCurated(paranetId)) { | ||
| try { | ||
| await agent.assertContextGraphOwner( |
There was a problem hiding this comment.
🔴 Bug: assertContextGraphOwner is stricter than the contract's publish authorization. Curated CGs are authorized on-chain via isAuthorizedPublisher, which also covers Safe/PCA ownership and registered agents, and follows CG NFT transfers; this local check only compares against the stored dkg:curator/creator metadata. That means a valid publisher can be rejected with 403 here even though the publish would succeed on-chain. Preflight should use on-chain publish-policy/authorization (or only run when local owner metadata is known to match chain authority).
There was a problem hiding this comment.
Valid concern, acknowledged as a known scope gap rather than a fresh regression:
assertContextGraphOwneris the same helper already used by/api/paranet/:id/share,invite-peer,rename, and the manifest-publish route. They all compare against the locally storeddkg:curatorwallet DID, so this PR doesn't introduce a new mismatch with on-chain authorization — it propagates the existing stance to publish.- The preflight fires only on the curated branch. Open CGs fall through to the chain adapter unchanged.
- For curated CGs this over-rejects two narrow scenarios: (a) PCA-delegated agents whose wallets aren't the stored curator but are registered via
agentToAccountId, and (b) post-NFT-transfer owners whose wallets don't match the stale local curator metadata. In both cases the chain would accept; the HTTP gate returns 403.
Migrating all of these routes to a chain-authoritative preflight (ContextGraphs.isAuthorizedPublisher via the adapter, with the caller wallet resolved through the same path the publisher uses) is the right long-term fix but is a cross-cutting change that touches every owner-gated route plus the multi-wallet resolution path. That's bigger than this PR's intended scope ("ship the curator gate, stop there") so I've filed it as a follow-up and added a code comment in memory.ts documenting the gap.
In the meantime, affected callers can work around it by publishing from the wallet recorded as the local curator.
| 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.
🟡 Issue: This only pins the helper mapping, not the HTTP regression the PR is fixing. memory.ts can still regress on request auth/status handling and this test will stay green. Add a daemon/API test for /api/shared-memory/publish that proves curated CGs fail with 403/400 before tentative metadata is written, while open CGs still allow non-owner publishes.
There was a problem hiding this comment.
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 (daemon-http-behavior-extra.test.ts) requires a real Hardhat node + registered curated CG + real EVMChainAdapter, which is a ~60s-per-test setup and more scope than this minimal route change. The devnet pass against scripts/devnet-test.sh §4e is the integration signal I'm leaning on here — it'll turn red if the handler regresses on request-auth handling.
Happy to add a unit-level daemon test using a stubbed DKGAgent (override isContextGraphCurated + assertContextGraphOwner, fire handleMemoryRoutes directly against a mock IncomingMessage/ServerResponse) if that's the bar — it wouldn't prove end-to-end but would lock in the HTTP branching logic. Let me know and I'll push it in a follow-up commit.
Codex PR#299 review flagged that assertContextGraphOwner is stricter than the contract's isAuthorizedPublisher for curated CGs: PCA delegation and post-NFT-transfer owners would be rejected with 403 here even though the chain accepts them. Chain-authoritative preflight is the right long-term fix but is cross-cutting (same helper is used by share/invite/rename/manifest-publish). Document the gap in-place so the next reader knows it's a known scope boundary, not a fresh regression. Made-with: Cursor
| * appropriate before handing off to the publisher. | ||
| */ | ||
| async isContextGraphCurated(contextGraphId: string): Promise<boolean> { | ||
| return this.isPrivateContextGraph(contextGraphId); |
There was a problem hiding this comment.
🔴 Bug: isPrivateContextGraph() is only local metadata state, not the live on-chain publishPolicy this helper claims to mirror. If a CG's publish policy is updated on chain, or the node has stale discovery data, /api/shared-memory/publish will take the wrong auth branch and either 403 a valid publish or fall back to the masked 200/tentative failure this PR is trying to prevent. Please read/sync the actual on-chain publish policy instead of inferring it from local access metadata.
There was a problem hiding this comment.
Same scope-gap as the sibling memory.ts thread above — isContextGraphCurated reads local metadata (the same _meta graph that records dkg:accessPolicy at create/register time), not the live on-chain publishPolicy. In practice this matches today: registerContextGraph writes both atomically (the local policy is what was just minted on-chain), and there's no on-chain mutator for publishPolicy post-creation in ContextGraphs.sol. Stale-discovery-data risk is bounded to nodes that haven't synced _meta for the CG — same exposure as every other curated-policy route. Chain-authoritative read on every publish call is the right long-term shape and is part of the same follow-up filed for the assertContextGraphOwner migration; doing it here would expand this PR past its stated scope (close the 200/tentative masked-failure bug).
| // wallet recorded as the CG's local curator. | ||
| if (await agent.isContextGraphCurated(paranetId)) { | ||
| try { | ||
| await agent.assertContextGraphOwner( |
There was a problem hiding this comment.
🔴 Bug: this preflight uses assertContextGraphOwner(), which only compares the caller against the locally stored dkg:curator. The contract's curated publish auth is broader than that (ownerOf(accountId) and agentToAccountId(...) in PCA mode, plus ownership changes after NFT transfer), so this branch will now reject publishes that ContextGraphs.isAuthorizedPublisher would accept. The preflight needs to be chain-authoritative, not a local owner-DID check.
There was a problem hiding this comment.
Acknowledged — same explicit scope boundary as the earlier PCA/delegation thread. Documented in commit 69a0dfde directly above this preflight (L468–480 of memory.ts):
Known scope gap (Codex PR#299 review, tracked separately):
assertContextGraphOwnercompares against the locally storeddkg:curatorwallet DID. The on-chainContextGraphs.isAuthorizedPublisheris richer — for PCA curators it live-resolves the NFT owner and anyagentToAccountId-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.
The net of this PR is still strictly better than HEAD: the prior behaviour returned 200/tentative for every unauthorized publish (the testnet incident this PR closes). The remaining over-rejection only affects PCA mode, which (per the Phase-9 status I can see in spec) is not yet exercised on testnet — so the worst-case regression in PCA mode is the same 403 you'd hit if you tried any other already-curator-gated route (share, invite, rename) from a PCA-delegated agent today. Net behaviour for testnet operators improves; chain-authoritative migration is filed as a follow-up rather than blocking this fix.
| // 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.
🟡 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 /api/shared-memory/publish covering one open CG and one curated CG so future refactors can't silently reintroduce the 200/tentative path or start 403'ing legitimate open publishes.
There was a problem hiding this comment.
Acknowledged. Daemon-level coverage for /api/shared-memory/publish would require either (a) spinning the full daemon worker + a real Hardhat-backed EVMChainAdapter + on-chain CG creation per CG variant (the daemon-http-behavior-extra.test.ts model — ~30–60s/test of real-chain bring-up), or (b) mocking the full RequestContext (25+ deps including publisherControl, tracker, memoryManager, fileStore, embeddingProvider…) which loses most of the regression value the test would otherwise have.
Neither is cheap, and the auth decision the test would lock down is going to be rewritten anyway when the chain-authoritative preflight migration lands (the same follow-up tracked for the sibling assertContextGraphOwner thread). Writing the test now against the local-curator semantics and immediately rewriting it for the chain-authoritative semantics doubles the work for a regression that the existing helper-mapping test (agent.test.ts L1369) already prevents at the agent boundary.
Proposing: defer to the chain-authoritative migration PR, where the daemon-level publish-route test gets written once against the final shape. If reviewers want the test before that, happy to do option (a) as a separate commit on this PR — flag explicitly and I'll add it.
|
Filed #375 to track the chain-authoritative preflight migration referenced in the inline replies above (Codex PR-review threads on |
1 participant
Summary
Spec §2.2 requires that only a context graph's registered curator may promote SWM to Verified Memory, but the daemon had no caller-level auth check on
/api/shared-memory/publish(or the legacy/api/workspace/enshrinealias). Non-curator callers reached the publisher, hit an on-chainUnauthorizedPublisherrevert, and got back HTTP 200 withstatus=tentative— masking the authorization failure and leaving phantom tentative metadata on disk.This PR adds the same owner-check the project-manifest-publish route already uses (
agent.assertContextGraphOwner), mapping the thrown error to:POST /api/context-graph/registerfirst)The helper is the single source of truth for "caller is curator" across the daemon, so the semantics here stay in lockstep with share, invite, rename, and manifest-publish gates.
Motivation
Running
scripts/devnet-test.shagainst latestmainproduces:```
[FAIL] Non-curator publish should be rejected with 4xx, got HTTP 200 status=tentative:
{"kcId":"0","status":"tentative", ...}
```
That's devnet §4e (spec §2.2 negative assertion — the publish-authority guard must produce an explicit 4xx rejection). Today the guard only fires on-chain, and the late revert is swallowed into the tentative response. After this PR the daemon rejects at the HTTP boundary with a clear error message.
What this does NOT fix
The curator's own publishes are also currently failing (devnet §4f / §5 / §10 / §22c) because
registerContextGraphpassespublishAuthority=ZeroAddressto the contract — a separate chain-wiring bug that deserves its own PR with contract-level review. This PR only closes the non-curator gap.Test plan
Backwards compatibility
Made with Cursor