* fix(auth): scope session-keyed reads and mutations to the workspace owner
Session-keyed service entry points did not apply per-user scoping, so
knowing another user's session or task ID was enough to reach it:
- GetTaskSession, ListTaskSessions, GetPrimarySession returned rows for
any ID. These feed GET /task-sessions/:id and GET /tasks/:id/sessions,
whose DTO carries session Metadata, the host worktree path, the branch,
and container/executor IDs.
- DismissLastAgentError mutated any session.
- ApproveSession approved a workflow review step on any session, which
advances the task's workflow — the highest-impact of the set.
Authorize via authorizeTaskID off the row already read, rather than
AuthorizeSessionAccess, which would re-read the session on what is a hot
path. Internal identity-less callers (pollers, event bus, office
schedulers, office agent JWTs) and the synthetic identity used while auth
is disabled stay unscoped, and unowned pre-auth rows stay visible.
Batch helpers that take ID lists are deliberately left alone: their
callers derive IDs from an already-authorized task list, so per-ID checks
would add N queries to list views for no gain. Recorded in AGENTS.md.
Also route httpApproveSession through handleNotFound so a denial answers
404 like every other session route instead of a 500 that would confirm
the session exists.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(auth): scope the session-keyed paths that bypass the task service
An adversarial sweep of the previous commit found session-keyed surfaces
reachable with a caller-supplied ID and no ownership check. They bypass
the task service, so scoping its methods did not cover them:
- POST /task-sessions/:id/{set-mode,set-model,set-config-option,
authenticate} and .../processes/:processId/stop resolve their execution
with a bare in-memory lookup, which skips the GetOrEnsure* chokepoint
where the lifecycle per-user check runs. A non-owner could switch
another user's model or mode, re-authenticate their agent, or kill
their process. Guarded by a shared denySessionAccess helper.
- The orchestrator reads sessions through its own repo handle, so
GetTaskSessionStatus and CheckSessionPR inherited nothing from the task
service. CheckSessionPR also installs a PR watch as a side effect.
Added a SetSessionAccessChecker hook mirroring the lifecycle manager's,
wired to taskSvc.AuthorizeSessionAccess. The check runs before
CheckSessionPR's unconfigured-GitHub early return so it applies
regardless of setup, and denials reuse the "session not found" shape so
they cannot be told apart from a missing session.
- GetMessage had no scoping while ListMessages beside it did, so a
caller holding another user's (session_id, message_id) pair could read
their shell output.
This does NOT close the whole surface. A second sweep found the
orchestrator's session-mutating WS actions (session.stop/delete/rename/
set_primary/set_plan_mode/reset_context/recover/launch/ensure,
agent.cancel, permission.respond) and agent.stdin / agent.resize also
unscoped — agent.stdin writes arbitrary input into another user's agent
process. Those touch live session lifecycle and need their own change;
recorded in AGENTS.md so the gap is not mistaken for covered.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(auth): scope every session-keyed WS action, plus a gateway backstop
Completes the sweep. Dispatched WS actions were left to authorize
themselves at the service layer, and a dozen never did — a caller with a
guessed session ID could stop, delete, rename, relaunch or reset another
user's live agent session, cancel their in-flight turn, answer their
permission prompts, or write arbitrary bytes into their agent process.
Two layers, deliberately both:
1. Gateway backstop. Client.authorizeAction checks any dispatched payload
carrying task_id or session_id against the client's identity before
dispatch. Per-method opt-in across ~74 actions in four packages had
been missed repeatedly; this makes a newly added action scoped by
default. Verified safe to apply generically: session_id means a task
session everywhere, and user-shell/terminal actions key off task_id /
task_environment_id, so nothing is misread.
2. Explicit guards, for defense in depth and for non-WS callers:
- orchestrator: StopSession, DeleteSession, SetPrimarySession,
RenameSession, ResetAgentContext, CancelAgent, RespondToPermission,
SetSessionPlanModeByID, RecoverSession, LaunchSession, EnsureSession,
CancelTransientRetry. Added SetTaskAccessChecker for the task-keyed
ones. LaunchSession guards every intent at the single funnel;
SessionID is empty on create, so the task check carries that case.
- agent.stdin / agent.resize now call lifecycleMgr.CheckSessionAccess,
the helper already documented for bare-lookup handlers.
Guards go first in each method, before any repo/executor use.
TestSessionKeyedEntryPointsGuardBeforeDependencies runs all twelve with
nil dependencies and fails on a panic if one is placed too late.
Denials are indistinguishable from a missing resource: WS returns
not-found, CancelTransientRetry reports "nothing cancelled", and
GetTaskSessionStatus reuses its "session not found" shape.
Unwired checkers stay permissive throughout, so identity-less internal
callers (schedulers, event bus, workflow engine) and pre-auth single-user
instances are unaffected — pinned per entry point.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(auth): scope environment-keyed user-shell actions
Third sweep finding, in surface the previous commit's own comment claimed
was covered. The user-shell actions key off task_environment_id and treat
task_id as optional, so a task_id-only backstop missed them:
user_shell.stop with an empty task_id and a foreign task_environment_id
fell through to the legacy runner path and tore down another user's
terminal PTY. That path never reaches
GetOrEnsureExecutionForEnvironment, where the environment check normally
runs, so neither layer caught it.
- Gateway backstop now also reads task_environment_id, gated by a new
AuthPolicy.ActionEnvironment hook wired to
taskSvc.AuthorizeEnvironmentAccess. This covers all three
environment-keyed actions generically, not just the reported one.
- wsUserShellStop guards explicitly via a new
lifecycleMgr.CheckEnvironmentAccess, mirroring CheckSessionAccess, so
the fallback is safe independent of the gateway.
Corrected the comment that wrongly asserted task_environment_id was
already covered, and recorded in AGENTS.md both the exact field names the
backstop reads and why a permissive type on one of them would silently
opt out.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(auth): authorize both IDs on task+session entry points
Review finding (P1). Entry points taking both a task and a session ID
only checked the session, so a caller could satisfy that check with one
of their own sessions while pointing taskID at another user's task:
- CheckSessionPR then used the unchecked taskID for GetTaskPR,
resolveTaskRepo, resolvePrimaryTaskRepositoryID and EnsurePRWatch —
cross-workspace PR disclosure plus a PR-watch write.
- CancelTransientRetry handed it to handleRecoverableFailure, which
writes against that task.
Both now use a new authorizeTaskSessionPair, which authorizes each ID and
additionally requires the pair to be consistent, so the two arguments
cannot describe different tasks. GetTaskSessionStatus already validated
the pair after loading the session and is unchanged.
The consistency half is skipped when the session cannot be loaded: both
IDs are authorized by then, so a mismatch is a caller bug rather than a
cross-user leak, and failing there would change behavior for
identity-less internal callers.
Recorded the rule in AGENTS.md — a two-ID entry point must never use
authorizeSession alone.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(auth): scope AttachWorkspaceSources
Found while rebasing onto main. POST /tasks/:id/workspace-sources and the
add_workspace_sources_kandev MCP tool landed on main during this PR's
review with no ownership check, so a caller could attach repositories and
folders to another user's task and have them materialized into that
task's workspace.
Same class as the rest of this PR, so fixed here rather than left for a
follow-up. The WS dispatch backstop added earlier already covered the
browser path (the payload carries task_id); this closes the HTTP route
and the MCP tool, which do not go through it.
ErrTaskNotFound already maps to 404 in workspaceSourceHTTPStatus, so the
denial surfaces correctly without touching the handler.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(auth): map denied shell-output reads to 404
Review finding. GetMessage became per-user scoped earlier in this PR, so
a denial now arrives as ErrTaskNotFound. httpGetShellOutput only mapped
sql.ErrNoRows to 404, sending everything else to the 500 branch — so a
foreign message answered 500 while a missing one answered 404, which is
an existence signal and inconsistent with the 404-everywhere rule the
other session routes follow.
Kept the sql.ErrNoRows branch explicit rather than replacing it outright:
isNotFound classifies typed sentinels and the "not found" substring, and
"sql: no rows in result set" matches neither, so routing that case
through handleNotFound alone regressed the missing-message response to
500 (caught by TestHTTPShellOutputSnapshot).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(auth): make the shell-output scoping test actually exercise the guard
Both reviewers caught that TestHTTPGetShellOutputDeniesForeignMessageWith404
was vacuous. httpGetShellOutput has a second 404 gate after the service
call — `if !ok || message.TaskSessionID != c.Param("id")` — and
ExtractShellExecOutput returns ok=false for nil metadata. The fixture had
none, so the route answered 404 through that gate whether or not scoping
denied the read.
Confirmed by mutation rather than inspection: with the authorization
removed from GetMessage the test still passed. With valid shell-output
metadata in the fixture it returns 200 and the victim's stdout, so the
test fails as intended; restoring the guard makes it pass again.
Also mutation-tested the sibling denial tests, since this is a defect
class rather than a one-off:
- TestHTTPGetTaskSessionDeniesForeignSessionWith404 — sound, returns 200
with the session DTO when the guard is removed.
- TestProcessRoutesDenyForeignSession — catches removal, though via the
documented nil-lifecycleMgr panic rather than a status assertion.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(auth): add an in-test witness to the shell-output scoping test
Review suggestion. The fixture fix made the test exercise the guard, but
nothing in the test proved the 404 came from authorization rather than the
route's post-service `!ok` gate — which is exactly how it went vacuous in
the first place. The evidence lived in a commit message describing a
manual mutation run, not in the test.
Now asserts both directions, matching the approve-route test: the
non-owner gets 404, and the owner does NOT. If the assertion ever stops
proving authorization, the owner case fails and says so.
Re-verified by mutation: removing the guard from GetMessage fails the test
with 200 and the victim's stdout; restoring it passes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* feat: resolve @prompt_name saved-prompt references in workflow step prompts
Workflow step prompts, the GitHub review-watch task prompt, and the CI
auto-fix prompt now resolve "@name" references to saved custom prompts
(Settings > Prompts) at invocation time, appending the referenced
content as hidden context while keeping the mention visible - matching
how chat messages and message_task already resolve @mentions. Editing
a saved prompt now propagates to every workflow step that references
it without needing to update each step individually.
Adds a Monaco "@" completion provider to the step-prompt and
review-watch editors, alongside the existing "{{placeholder}}"
provider.
* fix: address PR review findings on prompt-reference expansion
- Make AppendReferenceExpansions idempotent: a prompt that already
contains a previously-appended expansion block is returned unchanged,
since the block's own "### @name" headers would otherwise be
re-scanned as fresh mentions on a second call.
- Skip "@name" expansion entirely for CLI-passthrough sessions: their
prompts are written raw to a PTY with no <kandev-system> stripping,
so the hidden block would otherwise leak into the terminal verbatim.
* fix: fail safe on passthrough lookup error in startTask
A session-lookup error while resolving isPassthrough for the Kandev
MCP wrap and prompt-reference expansion previously defaulted to
false (non-passthrough), risking a hidden <kandev-system> block
leaking into a real passthrough session's PTY on a transient error.
Extracted resolveIsPassthroughForLaunch, which now fails safe by
defaulting to true (skip injection) and logs a warning.
* fix: scope expansion idempotency marker to system blocks
The idempotency guard added in a prior commit checked for the
expansion marker anywhere in the prompt, which could falsely
suppress a legitimate first-time expansion if caller-controlled
text happened to contain that literal phrase. Add
sysprompt.HasSystemMarker, mirroring the existing HasKandevContext
pattern, so the marker is only recognized inside a formatter-
produced <kandev-system> block. Also strengthens a passthrough test
assertion that didn't actually distinguish the snapshot path from
the fail-safe default.
* fix: match exact expansion-block prefix in idempotency guard
The prior scoping fix accepted the marker anywhere inside any
<kandev-system> block, which could still false-positive against an
unrelated system block (e.g. a plan-mode prefix or MCP context
wrap) that happens to mention the marker phrase without it being
the block's own content. Check for the exact literal prefix this
package's formatter produces instead: the opening tag immediately
followed by the marker, with nothing in between.
---------
Co-authored-by: Kandev Agent <agent@kandev.dev>
João Salavisa