feat(harness): register macrod harnesses with device-code pairing - #6030
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (60)
📒 Files selected for processing (105)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds private and team harness pairing through device codes, persisted credentials, harness token authorization, and harness-scoped runtime connections. Adds database tables, storage APIs, service routes, agent harness bindings, feed reconciliation, and daemon login support. Updates the web settings pages for pairing, listing, removal, and agent harness selection. Adds unit and integration tests for pairing, authorization, persistence, runtime routing, configuration, and UI behavior. Warning Some tools did not complete. Review the errors below. 🔧 Checkov (3.3.11).github/workspace-dep-closures.jsonCheckov timed out on this file apps/web/src/lib/service-clients/service-storage/openapi.jsonCheckov timed out on this file packages/sdk/specs/storage.jsonCheckov timed out on this file Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
a058fa9 to
c30665b
Compare
c30665b to
b7004f9
Compare
b7004f9 to
6423ae3
Compare
63c61c9 to
56236ae
Compare
|
Re: cursor[bot]'s "Team delete blocked by harness FKs" — confirmed and fixed in 56236ae. Verified empirically on a scratch DB before fixing: an Fix, in the squashed migration:
Revalidated: migration up/down/up on a fresh DB, and the team-delete repro with all three blockers present now succeeds, leaving the personal agent unbound. |
| /// | ||
| /// Kept minimal on purpose: the bots domain only needs enough to decide | ||
| /// whether a caller may bind an agent to the harness. | ||
| #[derive(Debug, Clone, PartialEq, Eq)] |
There was a problem hiding this comment.
this is really stinky. why are we calling this "HarnessFacts" why isn't this an enum with variants TeamOwned and TeamOwned?
56236ae to
cdc0282
Compare
| match self { | ||
| Self::User(user) => Some(user), | ||
| Self::Bot(bot) => bot.acting_user.as_ref(), | ||
| Self::Harness(harness) => Some(&harness.acting_user), |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: HIGH
Harness authentication always supplies a verified acting_user, and MacroAuthorization::acting_user() now returns that user for every harness request. document_storage_service and agent-session accept harness bearer tokens on the shared auth stack. Extractors that special-case Bot but fall through to acting_user() (document, chat, channel, history, reminder, foreign-entity, agent-session) therefore authorize the daemon as that user. ActingUser-gated routes do the same. Team harnesses accept x-macro-harness-for-macro-user-id for any current teammate; managed session create skips the harness-binding check.
Impact: A paired or stolen harness token can read and mutate the owner’s (or any current teammate’s) documents, chats, and agent sessions, open managed cloud sessions, and send prompts — far beyond agents bound to that harness.
Reviewed by Cursor Security Reviewer for commit cdc0282. Configure here.
| .context("failed to register this harness's trigger feed")?; | ||
| if let Some(feed) = &initial { | ||
| *signing_secret.write().expect("signing secret lock") = feed.signing_secret.clone(); | ||
| } |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: MEDIUM
After pairing, macrod serves POST /macro-events even when no agents are bound and ensure_feed returns None, leaving the webhook HMAC secret as an empty string. webhook_signature::verify accepts HMAC-SHA256 keyed by that empty secret. Unbinding later clears the feed id but does not clear the in-memory secret; the empty-key window is the initial unbound path (and up to the first reconcile after a bot is bound).
Impact: A network attacker who can reach the daemon port can mint a valid empty-key signature and drive session create/prompt using the paired harness credential.
Reviewed by Cursor Security Reviewer for commit cdc0282. Configure here.
| Json(req): Json<CreatePairingRequest>, | ||
| ) -> Result<(StatusCode, Json<CreatedPairing>), HarnessesHandlerErr> { | ||
| let pairing = state.service.create_pairing(req).await?; | ||
| Ok((StatusCode::CREATED, Json(pairing))) |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: MEDIUM
POST /harness-pairings is unauthenticated by design, and the only throttle is a deployment-wide cap of 32 open pairings (and 4 per requested name) with a 15-minute TTL. There is no per-client or per-IP limit on this handler.
Impact: An anonymous caller can fill those slots and refresh them as they expire, blocking legitimate device pairing for the whole deployment.
Reviewed by Cursor Security Reviewer for commit cdc0282. Configure here.
| } | ||
|
|
||
| #[tracing::instrument(skip(self), err)] | ||
| async fn get_pairing(&self, code: &str) -> Result<PairingDetails, HarnessError> { |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: MEDIUM
get_pairing (and approve_pairing, which logs code and caller) uses tracing::instrument that does not skip the pairing code. That code is the user-facing device secret. create_pairing correctly uses skip_all.
Impact: Anyone with application traces during the 15-minute window can recover codes and approve the pairing onto their own account, binding the victim’s daemon.
Reviewed by Cursor Security Reviewer for commit cdc0282. Configure here.
cdc0282 to
ddb36a1
Compare
| match self { | ||
| Self::User(user) => Some(user), | ||
| Self::Bot(bot) => bot.acting_user.as_ref(), | ||
| Self::Harness(harness) => Some(&harness.acting_user), |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: HIGH
Harness authentication always supplies a verified acting_user, and MacroAuthorization::acting_user() now returns that user for every harness request. Document storage and agent-session accept harness bearer tokens on the shared auth stack. Extractors that fall through to acting_user() (document, chat, channel, history, reminder, webhook, sandbox-size, and agent-session) therefore authorize the daemon as that user. Team harnesses accept x-macro-harness-for-macro-user-id for any current teammate. Managed session create skips harness-binding checks, and session control uses the same acting-user grants.
Impact: A private harness token exercises the owner’s grants on those routes. A team daemon can impersonate any teammate. A paired credential can open and prompt managed/cloud sessions and other sessions that user already owns, not only agents bound to that harness.
Reviewed by Cursor Security Reviewer for commit ddb36a1. Configure here.
| .context("failed to register this harness's trigger feed")?; | ||
| if let Some(feed) = &initial { | ||
| *signing_secret.write().expect("signing secret lock") = feed.signing_secret.clone(); | ||
| } |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: MEDIUM
After pairing, macrod serves POST /macro-events on 0.0.0.0 even when no agents are bound and ensure_feed returns None, leaving the webhook HMAC secret as an empty string. webhook_signature::verify accepts HMAC-SHA256 keyed by that empty secret.
Impact: A network attacker who can reach the daemon port can mint a valid empty-key signature and drive session create/prompt using the paired harness credential.
Reviewed by Cursor Security Reviewer for commit ddb36a1. Configure here.
| Json(req): Json<CreatePairingRequest>, | ||
| ) -> Result<(StatusCode, Json<CreatedPairing>), HarnessesHandlerErr> { | ||
| let pairing = state.service.create_pairing(req).await?; | ||
| Ok((StatusCode::CREATED, Json(pairing))) |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: MEDIUM
POST /harness-pairings is unauthenticated by design, and the only throttle is a deployment-wide cap of 32 open pairings (and 4 per requested name) with a 15-minute TTL. There is no per-client or per-IP limit on this handler.
Impact: An anonymous caller can fill those slots and refresh them as they expire, blocking legitimate device pairing for the whole deployment.
Reviewed by Cursor Security Reviewer for commit ddb36a1. Configure here.
| } | ||
|
|
||
| #[tracing::instrument(skip(self), err)] | ||
| async fn get_pairing(&self, code: &str) -> Result<PairingDetails, HarnessError> { |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: MEDIUM
get_pairing (and approve_pairing, which logs code and caller) uses tracing::instrument that does not skip the pairing code. That code is the user-facing device secret. create_pairing correctly uses skip_all.
Impact: Anyone with application traces during the 15-minute window can recover codes and approve the pairing onto their own account, binding the victim’s daemon.
Reviewed by Cursor Security Reviewer for commit ddb36a1. Configure here.
ddb36a1 to
35d54f1
Compare
35d54f1 to
7fe2598
Compare
7fe2598 to
9df03df
Compare
| (Some(connected_at), Some(disconnected_at)) => connected_at > disconnected_at, | ||
| (Some(_), None) => true, | ||
| (None, _) => false, | ||
| }; |
There was a problem hiding this comment.
Reconnect races connected status
Medium Severity
connected is derived from whichever of last_connected_at and last_disconnected_at is newer. Attach and detach write those timestamps independently with no generation or fencing, so a reconnect can persist disconnect after connect and show a live daemon as disconnected.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 9df03df. Configure here.
| match self { | ||
| Self::User(user) => Some(user), | ||
| Self::Bot(bot) => bot.acting_user.as_ref(), | ||
| Self::Harness(harness) => Some(&harness.acting_user), |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: HIGH
Harness authentication always supplies a verified acting_user, and MacroAuthorization::acting_user() now returns that user for every harness request. Document storage and agent-session accept harness bearer tokens on the shared auth stack. Extractors that special-case Bot but fall through to acting_user() (document, chat, channel, history, reminder, foreign-entity, agent-session) therefore authorize the daemon as that user. ActingUser-gated routes, including sandbox-size, do the same. Team harnesses accept x-macro-harness-for-macro-user-id for any current teammate; managed session create and session control skip harness-binding checks.
Impact: A paired or stolen harness token can read and mutate the owner’s (or any current teammate’s) documents, chats, and agent sessions, open managed cloud sessions, and send prompts — far beyond agents bound to that harness.
Reviewed by Cursor Security Reviewer for commit 9df03df. Configure here.
| credentials.clone(), | ||
| config_path, | ||
| )); | ||
| let signing_secret = Arc::new(std::sync::RwLock::new(String::new())); |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: MEDIUM
After pairing, macrod serves POST /macro-events on 0.0.0.0 even when no agents are bound and ensure_feed returns None, leaving the webhook HMAC secret as an empty string. webhook_signature::verify accepts HMAC-SHA256 keyed by that empty secret.
Impact: A network attacker who can reach the daemon port can mint a valid empty-key signature and drive session create/prompt using the paired harness credential.
Reviewed by Cursor Security Reviewer for commit 9df03df. Configure here.
| (status = 500, body = ErrorResponse), | ||
| ) | ||
| )] | ||
| pub async fn create_pairing_handler<S: HarnessService, Auth: MacroAuthorizationService>( |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: MEDIUM
POST /harness-pairings is unauthenticated by design, and the only throttle is a deployment-wide cap of 32 open pairings (and 4 per requested name) with a 15-minute TTL. There is no per-client or per-IP limit on this handler.
Impact: An anonymous caller can fill those slots and refresh them as they expire, blocking legitimate device pairing for the whole deployment.
Reviewed by Cursor Security Reviewer for commit 9df03df. Configure here.
| } | ||
|
|
||
| #[tracing::instrument(skip(self), err)] | ||
| async fn get_pairing(&self, code: &str) -> Result<PairingDetails, HarnessError> { |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: MEDIUM
get_pairing (and approve_pairing, which logs code and caller) uses tracing::instrument that does not skip the pairing code. That code is the user-facing device secret. create_pairing correctly uses skip_all.
Impact: Anyone with application traces during the 15-minute window can recover codes and approve the pairing onto their own account, binding the victim’s daemon.
Reviewed by Cursor Security Reviewer for commit 9df03df. Configure here.
9df03df to
73613fc
Compare
73613fc to
f7b371f
Compare
| match self { | ||
| Self::User(user) => Some(user), | ||
| Self::Bot(bot) => bot.acting_user.as_ref(), | ||
| Self::Harness(harness) => Some(&harness.acting_user), |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: HIGH
Harness authentication always supplies a verified acting_user, and MacroAuthorization::acting_user() returns that user for every harness request. ActingUser then admits the daemon token as that user, and several entity extractors (document, channel, chat, agent session) only special-case Bot before falling through to acting_user(). A team harness can also set x-macro-harness-for-macro-user-id to any current teammate.
Impact: A valid mhns_… token can create webhooks, control or delete that user’s agent sessions, and read or mutate their documents, chats, and channels. On a team harness, the daemon can impersonate any teammate, not only the pairing approver.
Reviewed by Cursor Security Reviewer for commit f7b371f. Configure here.
| .context("failed to register this bot's trigger feed")?; | ||
| let needs_validation = (!feed.is_valid).then_some(feed.webhook_id); | ||
| (feed.signing_secret, needs_validation) | ||
| .context("failed to register this harness's trigger feed")?; |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: MEDIUM
After pairing, macrod still binds the webhook listener when ensure_feed returns None (no agents bound). The signing secret stays an empty string, and POST /macro-events verifies HMAC against that empty key.
Impact: Anyone who can reach the daemon port can forge a valid signature with an empty HMAC key and submit agent-trigger events until a feed is registered.
Reviewed by Cursor Security Reviewer for commit f7b371f. Configure here.
| Json(req): Json<CreatePairingRequest>, | ||
| ) -> Result<(StatusCode, Json<CreatedPairing>), HarnessesHandlerErr> { | ||
| let pairing = state.service.create_pairing(req).await?; | ||
| Ok((StatusCode::CREATED, Json(pairing))) |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: MEDIUM
POST /harness-pairings is unauthenticated by design, and the only throttle is a deployment-wide cap of 32 open pairings (and a per-name cap). There is no per-IP or per-client limit.
Impact: An unauthenticated caller can fill the global pending-pairing quota and block every user from pairing a daemon until those rows expire.
Reviewed by Cursor Security Reviewer for commit f7b371f. Configure here.
| } | ||
|
|
||
| #[tracing::instrument(skip(self), err)] | ||
| async fn get_pairing(&self, code: &str) -> Result<PairingDetails, HarnessError> { |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: MEDIUM
get_pairing is instrumented with skip(self) only, so the pairing code is recorded on the tracing span. approve_pairing similarly logs code and caller fields.
Impact: Pairing codes in logs or traces can be used to look up pending pairings and, together with a leaked device secret, complete a claim.
Reviewed by Cursor Security Reviewer for commit f7b371f. Configure here.
f7b371f to
c3973b3
Compare
| >( | ||
| State(state): State<CreateSessionState<Opener, Bots, Auth>>, | ||
| caller: MacroAuthorizationExtractor<Auth, UserOrBot>, | ||
| caller: MacroAuthorizationExtractor<Auth, UserBotOrHarness>, |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: HIGH
create_agent_session now accepts harness tokens via UserBotOrHarness, but the managed path (no workspace) still skips every bot/harness bind check and opens a cloud-managed sandbox as the harness acting_user.
A valid mhns_ token can therefore provision Macro-hosted sessions with caller-supplied prompt/instructions. A team harness can also set x-macro-harness-for-macro-user-id to any current teammate and open those sessions as that user. Binding is only enforced later on the external-workspace branch.
Impact: A user-run daemon credential can spawn and attribute managed sandboxes the harness was not meant to operate, including as another teammate.
Reviewed by Cursor Security Reviewer for commit c3973b3. Configure here.
Makes user-run macrod daemons first-class harnesses: a harnesses entity (private or team-owned), device-code pairing that mints a harness credential, agents bound to a registered harness with ownership checks, the runtime gateway rekeyed from bot to harness so one daemon serves every bound agent, and the Settings UI to approve pairings and manage harnesses. The macrod TOML is credential-free; identity comes from pairing, with scope (private/team) requestable from config.
c3973b3 to
9726559
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9726559. Configure here.
| .context("failed to revoke harness tokens")?; | ||
|
|
||
| tx.commit().await.context("failed to commit")?; | ||
| Ok(deleted.rows_affected() == 1) |
There was a problem hiding this comment.
Deleted harness leaves agents bound
Medium Severity
Removing a harness only soft-deletes the row and revokes tokens. Bound agents keep the old harness_id, routing ignores deleted harnesses, and a later macrod login mints a new harness. Agents stay dead until each one is edited, despite copy that they will run again after reconnect.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9726559. Configure here.
| if let UserBotOrHarnessAuthorization::Harness(_) = caller | ||
| && let Some(claimed) = claimed | ||
| { | ||
| return MacroUserIdStr::try_from(claimed) | ||
| .map_err(|_| CreateSessionApiError::UnparseableOwner); | ||
| } |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: MEDIUM
For harness callers, CreateAgentSessionRequest.owner is parsed and used with no check against harness ownership or the verified acting user. Tests lock in that a bound-agent session can be owned by an arbitrary STRANGER. Session create then grants that user owner entity access.
Impact: A holder of the daemon token can attribute and later control sessions as another Macro user, including a teammate when combined with a team harness acting-user claim.
Reviewed by Cursor Security Reviewer for commit 9726559. Configure here.
1 participant
Makes user-run macrod daemons first-class private/team harnesses: device-code pairing mints the daemon's credential, agents bind to a registered harness, the runtime gateway is keyed by harness so one daemon serves every bound agent, and Settings gains the approval and management UI. Stacked on #6005.
Note
High Risk
Adds harness token authentication, pairing/claim flows, and agent-to-harness binding—security-sensitive paths that affect who can run code on user machines.
Overview
Introduces registered macrod harnesses so BYOA daemons pair via a printed code instead of embedding bot tokens in
macro.toml. New cratesharness_id,harness_token, andharnessesback persistence (harnesses,harness_tokens,harness_pairing_requests), token auth inmacro_authorization, and HTTP routes for pairing create/lookup/approve/claim plus harness list/delete. Agents gain an optionalharness_idonagent_configs(alongside the existingharnessslug); create/update/list queries and APIs carry the binding so macrod agents use harnessmacrod+ a specific harness UUID.Settings adds harness management: list connected/disconnected macrod instances, enter/approve pairing (including
?pair=deep link), remove harness (revokes tokens), and the Agents UI lists registered harnesses in the harness picker with free-text default model for macrod. Client types and Orval schemas pick upharness_id; docs for bring-your-own walk through pairing and creating agents on a harness.CI workspace dependency closures are updated to include the new crates across affected packages.
Reviewed by Cursor Bugbot for commit 9726559. Bugbot is set up for automated code reviews on this repo. Configure here.