Repository navigation
fix(api): keep user sessions off node operations; sign as the user - #1167
Conversation
On a multi-user node, the default user capabilities overlap the ones that guard node-wide calls. A user token could generate, lock or unlock the node's agent, change its entanglement proofs, export or import data at any host path, read provider API keys from ai.models and change the node's model settings. agent.sign signed with the node's main key, and agent.updateProfile set the node's DM language. - api/guards.rs: refuse_user_session() returns 403 for user sessions. - agent.generate/lock/unlock, entanglement calls, runtime.exportData and runtime.importData, ai.discoverModels/addModel/updateModel/removeModel/ setDefaultModel: refuse user sessions. - ai.models: user sessions get the models without provider keys. - agent.sign: user sessions sign with their own key. - agent.updateProfile: a user's DM language goes to the user's profile, with the DID from the user's key; a perspective-only update keeps it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (14)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThis change adds session-aware agent profile updates, signing, and language publishing. It also rejects selected node-level API operations from user sessions, limits access to compute logs and provider keys, and centralizes V8 test initialization. ChangesSession identity and API boundaries
V8 test initialization
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~55 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant UserSession
participant update_profile
participant AgentService
participant publish_agent_updated
UserSession->>update_profile: submit profile update
update_profile->>AgentService: load_user_agent_profile
AgentService-->>update_profile: stored profile
update_profile->>AgentService: store_user_agent_profile
update_profile->>publish_agent_updated: publish updated agent
Suggested reviewers: Merge Risk: 🟡 Moderate · up to Under those mode conditions, a user token can access operations intended for the node operator. Merge the user-detection prerequisite or close this authorization gap before merging. Pre-merge checks |
|
data-bot-coasys
left a comment
There was a problem hiding this comment.
Good fix, and the tests use the real default user capabilities and check the op name in the 403, so they fail on the old code.
One blocker: ai.getDefaultModel still returns the provider key to user sessions (inline). The rest are comments.
Merge order: #1168 first. Until it lands, this guard does nothing when multi-user mode reads as off (inline on guards.rs). No textual conflict with #1168 or #1164. This PR conflicts with #1169 in runtime_ws.rs and api/tests/mod.rs. It does not touch the flow engine or the #1058/#1162 cache writes.
…ions ai.getDefaultModel returned db.get_model(id) as stored, and users hold AI_READ, so a user session still got the default model's provider key. It now goes through models_for_session, like ai.models. Also: - agent.updateProfile returns the error when the user's profile cannot be read, instead of treating it as "no DM language" and dropping it. - The MCP profile perspective update keeps the user's DM language instead of writing None. Implements review 5327106839 on #1167. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@HexaField the changes from my review are in #1172 (stacked on this branch). |
fix(ai): strip the provider key from ai.getDefaultModel for user sessions (review fixes for #1167)
Review follow-up (Data): the guard list missed node-wide calls that the default user capabilities reach. - Refuse user sessions: language.publish (reads any host path, publishes as the node), language.remove, language.writeSettings, agent.add/ deleteTrustedAgents (the node's trust list decides whose languages run), agent.getEntanglementProofs, runtime friends list/add/remove, the outbox and sendFriendMessage (the main agent's), link-language templates add/ remove, runtime.addHcAgentInfos, hosting.wallet and walletHistory. - language.applyTemplate stays open (users create neighbourhoods with it) but publishes as the calling user (guards::session_agent_context). - runtime.computeLog: a user session reads only its own log. - agent.updateProfile: a user's update loads once, merges both fields, stores and publishes once, returns load errors, and announces agent-updated. AgentSignature::from_message goes through from_message_for_context. - Tests: node_operation_tests covers every new guard, applyTemplate as the caller, the compute-log scope and the profile paths.
rust-tests stopped at "Checkout code": the runner's cached clone already held a local branch with this name. Every other job passed on 361afdb.
data-bot-coasys
left a comment
There was a problem hiding this comment.
Re-reviewed 9fc51c9..7b612e5. Every call I listed now refuses user sessions, and applyTemplate signs as the session. user_sessions_cannot_call_node_operations uses the real default user capabilities and checks the op name in the 403, so it fails if any guard is dropped. No test in tests/js calls these ops from a user client, and the REST routes (openai_compat) expose none of them.
Merge order stands: #1168 first. The guard is still separate from #1164's refuse_app_management_by_users. That's fine as agreed: whichever PR merges second switches to one helper. Two follow-ups inline, neither blocking.
…he user neighbourhood_publish_from_perspective cloned the link language template through execute_on_language with no agent context, so a user's clone carried the node's DID, as language.applyTemplate did before this PR. clone_link_language now takes the tool's agent context and publishes through execute_on_language_with_context. - New test mcp_clones_a_link_language_as_the_calling_agent checks the author for a user and for the operator. - TestLanguageLanguage shares one process-wide languages directory, since the directory can only be set once and two tests now install it. Addresses Data's review on #1167.
data-bot-coasys
left a comment
There was a problem hiding this comment.
Approved. I re-reviewed only the delta since my last review (7b612e5..e9dfb4e, one commit). There is no merge from dev in the delta.
My follow-up is addressed. clone_link_language moved out of the handler unchanged, except that it takes the tool's agent_context and publishes through execute_on_language_with_context. The tool passes the agent_context it got from get_agent_context(), so a user's MCP neighbourhood now publishes its link language as the user. mcp_clones_a_link_language_as_the_calling_agent checks the author for a user and for the operator. The shared OnceLock languages directory is right: set_languages_directory can be set only once per process, and bundles save under their hash.
The notification thread is covered by #1169.
Merge order still stands: #1168 first. The guard reads user_email_from_token, and only #1168 makes it return sub in every mode.
One nit inline. I reviewed by reading. I did not run the tests. CI is green on e9dfb4e.
🤖 Generated with Claude Code
- ai_ws.rs: dev's import list and `*Params` contracts, plus this branch's `refuse_user_session` import and model-key tests. - runtime_ws.rs: dev made `runtime.sendFriendMessage` a params-checked not-implemented stub; the user-session guard now runs before it. - tests: dev's dispatcher checks params against each method's contract before the handler, so node_operation_tests now sends valid params and the 403 must come from the guard (export/import use `type`).
data-bot-coasys
left a comment
There was a problem hiding this comment.
Re-approving at 9032b75. The only change since my approval at 2ce0bcd is the dev merge (2f0a1d9, with #1193). The port keeps every guard: the code has 42 call sites of refuse_user_session, model_for_session and session_agent_context before the merge and 42 after. The PR's own diff differs in three files. In ai_ws.rs, the import list changed and the test module now sits after #1193's contract structs. In runtime_ws.rs, the comment above the sendFriendMessage guard changed, because dev made that handler not_implemented. The third file is node_operation_tests.rs.
The test change is the important one, and it is correct. #1193's dispatcher checks params against the contract before the handler runs, so the old {} params would now fail with 400 before they reach the guard. The merge gives each call params that its contract accepts, so the guard is what answers (inline).
Dev added three methods in the meantime, and none of them needs this guard: expression.getRaw is a read, runtime.setUnytMembraneProof requires the admin credential (stricter than this guard), and events.watch/events.unwatch set a per-connection filter.
Merge order is unchanged. #1168 goes first, because this guard relies on user_email being set whatever the multi-user setting says. After that, #1200 needs this same merge. CI is 14/14 green.
I resolved my earlier threads, which my previous approvals had already settled. The helper dedupe with #1164 and the MCP test nit moved to #1346.
|
Sibling WE PR: coasys/we#281, stacked on we#227 and paired with this PR. With this PR, a user session on a multi-user node gets 403 from Tested by hand on a MacBook: WE Electron as the operator, against an executor built from this PR (2528ea0). The operator flows work. The managed-user pass in WE web is in progress. — Hex, Josh's assistant ⬡ |
#1168's branch took dev's #1167 with its own resolution (70fb832). A plain merge here used the old dev base and conflicted in 17 hunks, because both branches carry #1168's code in different forms. Built instead from #1168's tree plus exactly #1169's own changes (the diff 2a8f318..06da93b, 19 files, +1492/-355), re-applied on top: the result differs from #1168's head by that delta and nothing else. Two import/module-list conflicts kept both sides.
With #1167 on dev, refuse_user_session runs before this PR's admin-credential check in runtime.importData, so a managed user's 403 carries the user-session message. The test now expects each caller's own refusal.
#1167 (now on dev) builds RequestContext in three test helpers; this PR adds connection_id to it.
Brings in 119 dev commits, including the security PRs #1164, #1167, #1168, #1169, #1208 and #1355. Conflicts: - rust-executor/src/perspectives/perspective_instance.rs - add_links / update_link: dev's check_consensus_rule_links (#1144) runs before the branch's graph routing (resolve_write_graph, and update_link's stored_copy + "the replacement stays in the old link's graph"). - all_notifications_for_perspective_id: dropped, as on dev; delivery goes through dev's notifications_to_fire (#1169). - Trigger reads keep the branch's rule (a trigger reads as its notification's owner), now keyed by dev's stored Notification.owner_did instead of the removed user_email. - rust-executor/src/api/events_ws.rs - notification-triggered: dev's matches_notification_owner (the stored notification.ownerDid). The branch's own version did the same from a top-level ownerDid field, which is removed from NotificationTriggeredEvent again. Dev's rule has no admin escape hatch; it is kept as dev has it. - exception-occurred: dev's exception_visible. - query-subscription-update: the branch's subscriber-only rule (subscriberDid) stays, on the shared lazy DID cell. The ownership gate is now dev's async matches_perspective_owner, which waits for the perspective handle instead of try_lock. - LazyDid is built from dev's session context. - matches_owner keeps the branch's Local-link rule for sessions without a DID (link, oldLink, newLink). Tests adapted to dev's notification API: add_notification takes the owner DID and grant, Notification has owner_did instead of user_email, and only a granted notification of a perspective owner fires.
What
This PR keeps user sessions away from calls that act on the node itself. On a multi-user node, these calls now refuse a user session with 403: the calls that change the node's agent, keystore, trusted agents or entanglement proofs,
runtime.exportDataandruntime.importData, the AI model settings, the calls that publish, remove or configure languages, the node's friends, outbox and link-language templates,runtime.addHcAgentInfos, and the hosting wallet.ai.modelsandai.getDefaultModelreturn models to a user session without provider keys.agent.signandlanguage.applyTemplatesign with the user's own key.agent.updateProfilewrites a user's DM language to the user's profile. A user reads only their ownruntime.computeLog.Why
The default user capabilities (AGENT_CREATE, AGENT_UPDATE, AGENT_SIGN, AGENT_READ, AI_CREATE, AI_READ) overlap the capabilities that guard node-wide calls. A user token therefore passed the capability check on calls that change the node's agent, its wallet state, its files and its model settings, and on calls that read provider keys.
agent.signsigned with the node's main key, so a user could get signatures that other agents attribute to the node.agent.updateProfilechanged the node's DM language, not the user's.language.applyTemplatepublished a user's language under the node's DID, andruntime.computeLogreturned any user's log.How
agent.generate,agent.lock,agent.unlockagent.addTrustedAgents,agent.deleteTrustedAgentsagent.addEntanglementProofs,agent.deleteEntanglementProofs,agent.entanglementProofPreflight,agent.getEntanglementProofsruntime.exportData,runtime.importDataai.discoverModels,ai.addModel,ai.updateModel,ai.removeModel,ai.setDefaultModellanguage.publish,language.remove,language.writeSettingsruntime.friends,runtime.addFriends,runtime.removeFriends,runtime.outbox,runtime.sendFriendMessageruntime.addLinkLanguageTemplates,runtime.removeLinkLanguageTemplates,runtime.addHcAgentInfoshosting.wallet,hosting.walletHistoryai.models,ai.getDefaultModelapi.apiKeyagent.sign,language.applyTemplate, the MCP neighbourhood tool's link-language cloneruntime.computeLogagent.updateProfilewithdmLanguageagent.updateProfilewithpublicPerspectiveonlyAn operator session means the admin credential, or an app token without a user. Single-user nodes have no user sessions, so nothing changes for them.
A profile update with both fields loads, stores and publishes the user's profile once. A profile that fails to load fails the update, so the update never drops the DM language. The MCP perspective update keeps the user's DM language too.
Tests
api::tests::node_operation_tests::user_sessions_cannot_call_node_operationsapi::tests::node_operation_tests::a_user_session_signs_as_the_user_not_the_nodeapi::tests::node_operation_tests::the_operator_still_signs_as_the_nodeapi::tests::node_operation_tests::apply_template_publishes_as_the_calling_sessionapi::tests::node_operation_tests::mcp_clones_a_link_language_as_the_calling_agentapi::tests::node_operation_tests::a_user_sets_their_own_dm_language_not_the_nodesapi::tests::node_operation_tests::one_profile_update_with_both_fields_stores_and_announces_onceapi::tests::node_operation_tests::a_profile_that_fails_to_load_fails_the_updateapi::tests::node_operation_tests::a_user_reads_only_their_own_compute_logapi::tests::node_operation_tests::the_default_model_hides_its_key_from_users_onlyai.getDefaultModelthrough the handler mapapi::ai_ws::model_key_tests(3)ai.modelsorai.getDefaultModel; operators keep themapi::guards::tests(2)The
ai.getDefaultModelfix, the profile-load error and the MCP DM-language fix come from #1172, the follow-up to Data's review.Merge notes. Merge #1168 first. On
dev,user_email_from_tokenreturns no user when multi-user mode reads as off, so this guard sees no user session; #1168 makes it returnsubin every mode.session_agent_contextrepeatsAgentContext::from_requestfrom #1168, and #1164 repeatsrefuse_user_sessionasrefuse_app_management_by_users; the PR that merges second switches to one helper. This PR conflicts with #1169 inruntime_ws.rsandapi/tests/mod.rs.Local run:
cargo test --release --lib -- --test-threads=1→ 2114 passed, 0 failed, 41 ignored.@lucksus @data-bot-coasys
Summary by CodeRabbit