Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 74 additions & 25 deletions assistant/src/approvals/guardian-decision-primitive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
* 9. Kind-specific resolver dispatch via the resolver registry
*
* Security invariants enforced here:
* - Decision application is identity-bound to expected guardian identity
* - Decision authorization is purely principal-based:
* actor.guardianPrincipalId must match request.guardianPrincipalId
* - Decisions are first-response-wins (CAS-like stale protection)
* - `approve_always` is rejected/downgraded for guardian-on-behalf requests
* - Scoped grant minting only on explicit approve for requests with tool metadata
Expand All @@ -35,6 +36,7 @@ import {
type GuardianApprovalRequest,
updateApprovalDecision,
} from '../memory/channel-guardian-store.js';
import { getActiveBinding } from '../memory/guardian-bindings.js';
import { DAEMON_INTERNAL_ASSISTANT_ID } from '../runtime/assistant-scope.js';
import type {
ApprovalAction,
Expand Down Expand Up @@ -273,6 +275,39 @@ export function mintCanonicalRequestGrant(params: {
return { minted: false };
}

// ---------------------------------------------------------------------------
// Cross-channel principal authorization helper
// ---------------------------------------------------------------------------

/**
* Check whether an actor's guardian principal is authorized to act on a
* request that was bound to a (possibly different) guardian principal.
*
* Authorization passes when:
* 1. The actor's principal directly matches the request's principal, OR
* 2. The actor's principal matches the assistant's canonical (vellum)
* principal — this covers the cross-channel case where a request was
* created with a channel binding's principal (e.g. Telegram) but the
* guardian responds from vellum/desktop.
*
* Returns `true` when the actor is authorized, `false` otherwise.
*/
export function isAuthorizedGuardianPrincipal(
actorPrincipalId: string,
requestPrincipalId: string,
): boolean {
if (actorPrincipalId === requestPrincipalId) {
return true;
}

// Cross-channel fallback: check if the actor matches the assistant's
// canonical (vellum) principal.
const vellumBinding = getActiveBinding(DAEMON_INTERNAL_ASSISTANT_ID, 'vellum');
const canonicalPrincipal = vellumBinding?.guardianPrincipalId;

return !!canonicalPrincipal && actorPrincipalId === canonicalPrincipal;
}

// ---------------------------------------------------------------------------
// Canonical guardian decision primitive
// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -349,44 +384,58 @@ export async function applyCanonicalGuardianDecision(
return { applied: false, reason: 'invalid_action', detail: `invalid action: ${action}` };
}

// 2c. Validate identity: actor must match guardian_external_user_id
// unless the actor is trusted (desktop).
//
// Channel tool-approval requests must always be identity-bound. Treat
// missing guardianExternalUserId as unauthorized (fail-closed) so a
// non-guardian actor can never approve an unbound request.
if (
!actorContext.isTrusted &&
request.kind === 'tool_approval' &&
!request.guardianExternalUserId
) {
// 2c. Principal-based authorization: actor.guardianPrincipalId must match
// request.guardianPrincipalId for any applied decision. This is the single
// authorization gate — there is no trusted bypass.

if (!request.guardianPrincipalId) {
log.warn(
{
event: 'canonical_decision_missing_guardian_binding',
event: 'canonical_decision_missing_request_principal',
requestId,
kind: request.kind,
sourceType: request.sourceType,
},
'Canonical tool approval missing guardian binding; rejecting decision',
'Canonical request missing guardianPrincipalId; rejecting decision',
);
return { applied: false, reason: 'identity_mismatch', detail: 'missing guardian binding' };
return { applied: false, reason: 'identity_mismatch', detail: 'request missing guardianPrincipalId' };
}

if (
request.guardianExternalUserId &&
!actorContext.isTrusted &&
actorContext.externalUserId !== request.guardianExternalUserId
) {
if (!actorContext.guardianPrincipalId) {
log.warn(
{
event: 'canonical_decision_identity_mismatch',
event: 'canonical_decision_missing_actor_principal',
requestId,
actorChannel: actorContext.channel,
},
'Actor missing guardianPrincipalId; rejecting decision',
);
return { applied: false, reason: 'identity_mismatch', detail: 'actor missing guardianPrincipalId' };
}

if (!isAuthorizedGuardianPrincipal(actorContext.guardianPrincipalId, request.guardianPrincipalId)) {
log.warn(
{
event: 'canonical_decision_principal_mismatch',
requestId,
expectedPrincipal: request.guardianPrincipalId,
actualPrincipal: actorContext.guardianPrincipalId,
},
'Actor principal does not match request principal or canonical principal',
);
return { applied: false, reason: 'identity_mismatch', detail: 'principal mismatch' };
}

if (actorContext.guardianPrincipalId !== request.guardianPrincipalId) {
log.info(
{
event: 'canonical_decision_cross_channel_principal_match',
requestId,
expectedGuardian: request.guardianExternalUserId,
actualActor: actorContext.externalUserId,
requestPrincipal: request.guardianPrincipalId,
actorPrincipal: actorContext.guardianPrincipalId,
},
'Actor identity does not match expected guardian',
'Actor principal matches canonical vellum principal (cross-channel authorization)',
);
return { applied: false, reason: 'identity_mismatch' };
}

// 2d. Check expiry
Expand Down
12 changes: 8 additions & 4 deletions assistant/src/approvals/guardian-request-resolvers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ export interface ActorContext {
externalUserId: string | undefined;
/** Channel the decision arrived on. */
channel: string;
/** Whether the actor is a trusted/desktop context. */
isTrusted: boolean;
/** Principal ID for authorization — must match the request's guardianPrincipalId. */
guardianPrincipalId: string | undefined;
}

/** The decision being applied. */
Expand Down Expand Up @@ -374,7 +374,9 @@ const accessRequestResolver: GuardianRequestResolver = {
return {
ok: true,
applied: true,
...(ctx.actor.isTrusted ? { guardianReplyText: `Access denied for ${requesterLabel}.` } : {}),
// Desktop actors (vellum channel) receive inline reply text; channel
// actors get replies delivered via the channel delivery context.
...(ctx.actor.channel === 'vellum' ? { guardianReplyText: `Access denied for ${requesterLabel}.` } : {}),
};
}

Expand Down Expand Up @@ -528,7 +530,9 @@ const accessRequestResolver: GuardianRequestResolver = {
return {
ok: true,
applied: true,
...(ctx.actor.isTrusted ? { guardianReplyText: verificationReplyText } : {}),
// Desktop actors (vellum channel) receive inline reply text; channel
// actors get replies delivered via the channel delivery context.
...(ctx.actor.channel === 'vellum' ? { guardianReplyText: verificationReplyText } : {}),
};
},
};
Expand Down
7 changes: 5 additions & 2 deletions assistant/src/daemon/handlers/guardian-actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
} from '../../approvals/guardian-decision-primitive.js';
import { getCanonicalGuardianRequest } from '../../memory/canonical-guardian-store.js';
import type { ApprovalAction } from '../../runtime/channel-approval-types.js';
import { resolveLocalIpcGuardianContext } from '../../runtime/local-actor-identity.js';
import { listGuardianDecisionPrompts } from '../../runtime/routes/guardian-action-routes.js';
import type { GuardianActionDecision, GuardianActionsPendingRequest } from '../ipc-protocol.js';
import { defineHandlers, log } from './shared.js';
Expand Down Expand Up @@ -45,13 +46,15 @@ export const guardianActionsHandlers = defineHandlers({
}
}

// Resolve the local IPC actor's principal via the vellum guardian binding.
const localGuardianCtx = resolveLocalIpcGuardianContext('vellum');
const canonicalResult = await applyCanonicalGuardianDecision({
requestId: msg.requestId,
action: msg.action as ApprovalAction,
actorContext: {
externalUserId: undefined,
externalUserId: localGuardianCtx.guardianExternalUserId,
channel: 'vellum',
isTrusted: true,
guardianPrincipalId: localGuardianCtx.guardianPrincipalId ?? undefined,
},
userText: undefined,
});
Expand Down
7 changes: 5 additions & 2 deletions assistant/src/daemon/handlers/sessions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -600,13 +600,16 @@ export async function handleUserMessage(
]));

if (pendingRequestIdsForConversation.length > 0) {
// Resolve the local IPC actor's principal via the vellum guardian binding
// for principal-based authorization in the canonical decision primitive.
const localCtx = resolveLocalIpcGuardianContext(ipcChannel);
const routerResult = await routeGuardianReply({
messageText: messageText.trim(),
channel: ipcChannel,
actor: {
externalUserId: undefined,
externalUserId: localCtx.guardianExternalUserId,
channel: ipcChannel,
isTrusted: true,
guardianPrincipalId: localCtx.guardianPrincipalId ?? undefined,
},
conversationId: msg.sessionId,
pendingRequestIds: pendingRequestIdsForConversation,
Expand Down
4 changes: 2 additions & 2 deletions assistant/src/daemon/session-process.ts
Original file line number Diff line number Diff line change
Expand Up @@ -383,9 +383,9 @@ export async function processMessage(
messageText: trimmedContent,
channel: 'vellum',
actor: {
externalUserId: undefined,
externalUserId: session.guardianContext?.guardianExternalUserId,
channel: 'vellum',
isTrusted: true,
guardianPrincipalId: session.guardianContext?.guardianPrincipalId ?? undefined,
},
conversationId: session.conversationId,
pendingRequestIds: canonicalPendingRequestIdsForConversation,
Expand Down
4 changes: 4 additions & 0 deletions assistant/src/memory/canonical-guardian-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ export function getCanonicalGuardianRequestByCode(code: string): CanonicalGuardi
export interface ListCanonicalGuardianRequestsFilters {
status?: CanonicalRequestStatus;
guardianExternalUserId?: string;
guardianPrincipalId?: string;
requesterExternalUserId?: string;
conversationId?: string;
sourceType?: string;
Expand All @@ -289,6 +290,9 @@ export function listCanonicalGuardianRequests(filters?: ListCanonicalGuardianReq
if (filters?.guardianExternalUserId) {
conditions.push(eq(canonicalGuardianRequests.guardianExternalUserId, filters.guardianExternalUserId));
}
if (filters?.guardianPrincipalId) {
conditions.push(eq(canonicalGuardianRequests.guardianPrincipalId, filters.guardianPrincipalId));
}
if (filters?.conversationId) {
conditions.push(eq(canonicalGuardianRequests.conversationId, filters.conversationId));
}
Expand Down
42 changes: 26 additions & 16 deletions assistant/src/runtime/guardian-reply-router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import {
applyCanonicalGuardianDecision,
type CanonicalDecisionResult,
isAuthorizedGuardianPrincipal,
} from '../approvals/guardian-decision-primitive.js';
import type { ActorContext, ChannelDeliveryContext } from '../approvals/guardian-request-resolvers.js';
import {
Expand Down Expand Up @@ -195,19 +196,26 @@ function findPendingCanonicalRequests(
});
}

// For desktop/trusted actors without an externalUserId, query by
// conversationId so the NL path can discover pending requests.
// Actors without an externalUserId: scope by conversationId so the NL
// path can discover pending requests bound to this conversation.
// Include guardianPrincipalId filter when available so the guardian only
// sees requests they are authorized to act on.
if (conversationId) {
return listCanonicalGuardianRequests({
status: 'pending',
conversationId,
...(actor.guardianPrincipalId ? { guardianPrincipalId: actor.guardianPrincipalId } : {}),
});
Comment thread
noanflaherty marked this conversation as resolved.
}

// Trusted actors without a conversationId: return all pending requests
// so desktop sessions can always discover pending guardian work.
if (actor.isTrusted) {
return listCanonicalGuardianRequests({ status: 'pending' });
// Actors with a guardianPrincipalId but no externalUserId or
// conversationId: query by principal so desktop sessions can still
// discover pending guardian work via their bound principal.
if (actor.guardianPrincipalId) {
return listCanonicalGuardianRequests({
status: 'pending',
guardianPrincipalId: actor.guardianPrincipalId,
});
}

return [];
Expand Down Expand Up @@ -299,22 +307,24 @@ export async function routeGuardianReply(
// silently defaulting to approve_once.
if (!codeResult.remainingText || codeResult.remainingText.trim().length === 0) {
// Identity check: only expose request details to the assigned guardian
// or trusted (desktop) actors. Mirrors the identity check in
// applyCanonicalGuardianDecision to prevent leaking request details
// (toolName, questionText) to unauthorized senders.
// principal. Uses the shared cross-channel principal helper (same logic
// as applyCanonicalGuardianDecision) to prevent leaking request details
// (toolName, questionText) to unauthorized senders while allowing
// cross-channel lookups (e.g. desktop guardian entering a code for a
// Telegram-originated request).
if (
request.guardianExternalUserId &&
!actor.isTrusted &&
actor.externalUserId !== request.guardianExternalUserId
request.guardianPrincipalId &&
actor.guardianPrincipalId &&
!isAuthorizedGuardianPrincipal(actor.guardianPrincipalId, request.guardianPrincipalId)
Comment on lines 315 to +318

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Code-only clarification identity check silently skips when actor principal is missing

The code-only clarification identity gate in guardian-reply-router.ts is weaker than the old code because the condition requires both request.guardianPrincipalId and actor.guardianPrincipalId to be truthy before checking authorization. When actor.guardianPrincipalId is undefined (e.g., a channel guardian whose binding was created before guardianPrincipalId was introduced), the entire check is skipped and request details (toolName, questionText) are exposed.

Comparison with old code

Old code:

if (
  request.guardianExternalUserId &&
  !actor.isTrusted &&
  actor.externalUserId !== request.guardianExternalUserId
)

The old code blocked when actor.externalUserId (always present for channel actors) didn't match the request's guardian, regardless of whether guardianPrincipalId existed.

New code:

if (
  request.guardianPrincipalId &&
  actor.guardianPrincipalId &&
  !isAuthorizedGuardianPrincipal(...)
)

When actor.guardianPrincipalId is falsy (legacy binding with null guardianPrincipalId), the second operand short-circuits the entire condition to false, and the block that returns 'Request not found.' is never reached.

By contrast, the decision gate in applyCanonicalGuardianDecision (guardian-decision-primitive.ts:404) correctly rejects when actor principal is missing. This read-only path should mirror that fail-closed behavior.

Impact: A channel actor on a legacy binding without guardianPrincipalId who enters a cross-channel request code can see clarification details that would have been hidden by the old identity check. Practical impact is limited to read-only info disclosure and only for legacy bindings.

Suggested change
if (
request.guardianExternalUserId &&
!actor.isTrusted &&
actor.externalUserId !== request.guardianExternalUserId
request.guardianPrincipalId &&
actor.guardianPrincipalId &&
!isAuthorizedGuardianPrincipal(actor.guardianPrincipalId, request.guardianPrincipalId)
if (
request.guardianPrincipalId &&
(!actor.guardianPrincipalId || !isAuthorizedGuardianPrincipal(actor.guardianPrincipalId, request.guardianPrincipalId))
) {
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

) {
Comment thread
noanflaherty marked this conversation as resolved.
Comment on lines +313 to 315

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Apply canonical-principal fallback to code-only clarifications

The code-only clarification guard now rejects whenever actor.guardianPrincipalId !== request.guardianPrincipalId, but applyCanonicalGuardianDecision() (same PR) explicitly allows cross-channel authorization when the actor matches the assistant’s canonical vellum principal. In the common case where a request is created with a Telegram/SMS principal and the guardian replies from vellum, decisions are allowed but a code-only message is incorrectly blocked with “Request not found,” so authorized guardians cannot use the clarification flow before deciding. This check should mirror the canonical-principal fallback used by decision application.

Useful? React with 👍 / 👎.

log.warn(
{
event: 'router_code_only_identity_mismatch',
event: 'router_code_only_principal_mismatch',
requestId: request.id,
expectedGuardian: request.guardianExternalUserId,
actualActor: actor.externalUserId,
expectedPrincipal: request.guardianPrincipalId,
actualPrincipal: actor.guardianPrincipalId,
},
'Code-only clarification blocked: actor identity does not match expected guardian',
'Code-only clarification blocked: actor principal does not match request or canonical principal',
);
return {
decisionApplied: false,
Expand Down
22 changes: 12 additions & 10 deletions assistant/src/runtime/routes/conversation-routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ async function tryConsumeCanonicalGuardianReply(params: {
approvalConversationGenerator?: ApprovalConversationGenerator;
/** Verified actor identity from actor-token middleware. */
verifiedActorExternalUserId?: string;
/** Verified actor principal ID for principal-based authorization. */
verifiedActorPrincipalId?: string;
}): Promise<{ consumed: boolean; messageId?: string }> {
const {
conversationId,
Expand All @@ -98,6 +100,7 @@ async function tryConsumeCanonicalGuardianReply(params: {
onEvent,
approvalConversationGenerator,
verifiedActorExternalUserId,
verifiedActorPrincipalId,
} = params;
const trimmedContent = content.trim();

Expand All @@ -114,12 +117,7 @@ async function tryConsumeCanonicalGuardianReply(params: {
actor: {
externalUserId: verifiedActorExternalUserId,
channel: sourceChannel,
// When a verified identity is available, disable the trusted bypass so
// that the identity-match checks in applyCanonicalGuardianDecision
// actually run. Only fall back to isTrusted when no verified identity
// was resolved (defensive — shouldn't happen for vellum since
// verification runs upstream).
isTrusted: !verifiedActorExternalUserId,
guardianPrincipalId: verifiedActorPrincipalId,
},
conversationId,
pendingRequestIds,
Expand Down Expand Up @@ -523,12 +521,15 @@ export async function handleSendMessage(
? smDeps.resolveAttachments(attachmentIds)
: [];

// Resolve the verified actor's external user ID for inline approval
// routing. Uses the guardianExternalUserId from the verified context
// (actor-token or local-fallback) rather than hardcoding undefined.
// Resolve the verified actor's external user ID and principal for inline
// approval routing. Uses the guardianExternalUserId and guardianPrincipalId
// from the verified context (actor-token or local-fallback).
const verifiedActorExternalUserId = actorVerification?.ok
? actorVerification.guardianContext.guardianExternalUserId
: undefined;
: session.guardianContext?.guardianExternalUserId;
const verifiedActorPrincipalId = actorVerification?.ok
? actorVerification.guardianContext.guardianPrincipalId ?? undefined
: session.guardianContext?.guardianPrincipalId ?? undefined;

// Try to consume the message as a canonical guardian approval/rejection reply.
// On failure, degrade to the existing queue/auto-deny path rather than
Expand All @@ -544,6 +545,7 @@ export async function handleSendMessage(
onEvent,
approvalConversationGenerator: deps.approvalConversationGenerator,
verifiedActorExternalUserId,
verifiedActorPrincipalId,
});
if (inlineReplyResult.consumed) {
return Response.json(
Expand Down
Loading