security(llc): the goal ancestors route scopes only the leaf, so a cross-company parent edge leaks the chain

GET /goals/{goal_id}/ancestors scopes the starting goal and then walks its parent chain with no tenant check, returning every ancestor as a GoalResponse.

# llc/api/goals.py:264-266
await _get_authorized_goal(session, goal_id, ctx)        # scopes the LEAF only
ancestors = await _svc().get_ancestors(session, goal_id)  # company_id omitted -> unscoped walk
return [GoalResponse.model_validate(a) for a in ancestors]

The defence exists and is not switched on. GoalService.get_ancestors gates its tenant check on the argument the route does not pass:

# llc/services/goal.py:270
async def get_ancestors(self, session, goal_id, *, company_id: Optional[str] = None)
# :291
if company_id is not None and str(goal.company_id) != str(company_id):

#13704 names this exact failure

The service docstring, added by that issue:

Scoping only the leaf is not enough — a cross-company parent edge makes the chain itself the leak, and such an edge was creatable until the guard added to create in the same change.

So the walk-stopping behaviour was written for precisely this route's shape, and the route does not enable it.

Why this is reachable rather than theoretical

llc_goals.parent_goal_id is a self-referential FK to llc_goals.id with no company constraint — the database permits a cross-company edge.
Only the service-layer create guard prevents new ones. By fix(memory): L4 GoalAncestry needs a server-side session→work-item binding and a tenant-scoped goal lookup #13704's own account such edges were creatable before that guard, so any installation with goals predating it may hold one.
_get_authorized_goal returns the caller's own goal, so the request looks entirely legitimate at the boundary.

A caller owning goal G whose parent belongs to another company receives that parent, and everything above it, as full GoalResponse objects.

How it was found, which is the part worth generalising

company_id is Optional[str] = None. A caller does not have to pass None to get the unscoped path — omitting the argument does it silently. A search for company_id=None finds nothing; the defect is in the callers that never mention it.

Two live callers omit it:

llc/api/goals.py:265                      get_ancestors(session, goal_id)          <- this issue
llc/scheduler/heartbeat_scheduler.py:1039 revoke_key(session, agent_id=, key_id=)  <- see below

An optional tenant check is off by default, and every call site that predates the parameter keeps the old behaviour without any diff showing it. The same shape appears at llc/services/api_key.py:78 (if company_id is not None: builds the condition list) and at llc/services/goal.py:324.

Acceptance criteria

GET /goals/{goal_id}/ancestors passes the caller's company, so the walk stops at a foreign ancestor rather than traversing it.
A test seeds a goal whose parent belongs to another company, calls the route as the child's owner, and asserts the foreign ancestor is absent from the response — not merely that a warning was logged.
heartbeat_scheduler.py:1039's revoke_key is classified: either it is a trusted internal path where an unscoped revoke is intended, and that is stated, or it passes the company.
Every parameter of the form company_id: Optional[...] = None on a tenant-scoped service method is enumerated, with each call site classified as scoped or deliberately unscoped. The population is the parameter, not the string None.
Decided and recorded: whether a tenant scope may be optional at all. A required parameter makes every unscoped call site a visible diff; an optional one makes them invisible, which is how these two survived fix(memory): L4 GoalAncestry needs a server-side session→work-item binding and a tenant-scoped goal lookup #13704.

Relationship to #15924

#15924 is the seven surviving copies of the IDOR-guard helper. This is narrower and worse: not a duplicated guard, but a guard that exists, is correct, and is not invoked. Found while measuring #15924's severity — the two share a cause, in that both follow from the canonical guard being unreachable from part of the codebase.

Refs #13704, #15924

The mechanism generalises — measured, with the caveat that I found no second live instance

company_id: Optional[str] = None making scoping opt-in is a class, not one
route. Measured across autobot-backend/**/*.py, excluding tests:

functions taking a scope id (company_id/org_id/organization_id/tenant_id)
that DEFAULTS TO None                                              : 54
  of those, the tenant filter applies only inside `if <scope_id>`  :  5

The five:

llc/kb/artifact_ingestor.py:128     ingest(company_id=None)
llc/services/portability.py:583     _goal_titles_exist(company_id=None)
llc/services/routine_service.py:86  list(company_id=None)
llc/services/work_item_service.py:342  get(company_id=None)
memory/trajectory_store.py:439      find_similar_trajectories(tenant_id=None)

Each has the shape this issue names: the argument is optional, and omitting it
skips the filter rather than failing.

I checked the most dangerous-looking one and it is not exploitable

WorkItemService.get() filters only if company_id is not None, and
WorkItemService.update() (:347) calls self.get(session, work_item_id)
without it — an in-repo caller silently opting out, on a write path.

The routes are safe. Both get_work_item (work_items.py:515) and
update_work_item (:530) fetch unscoped and then compare by hand:

existing = await _service().get(session, work_item_id)
if existing is None or str(existing.company_id) != str(ctx.org_id):
    raise HTTPException(status_code=404, detail="Work item not found")

So there is no live IDOR here, and I am not reporting one. Saying that plainly
because the pattern reads alarming and the conclusion is that it currently holds.

What it does show

The safe behaviour is achieved by every caller re-implementing it, and
llc/deps.py already carries the consolidated remedy — load_authorized(),
whose docstring says it exists precisely to replace the per-router
"select by id, then compare the row's owning company to ctx.org_id" copies
(#12184). These two routes are two more of those copies, written after the
consolidation.

That is the real cost of an optional scope parameter: it does not make the
system insecure, it makes security the caller's job, and then the same
three-line idiom is rewritten at every call site with no guard that it was.
#15930's ancestors walk is what happens when one of those rewrites is missed.

Suggestion, sized to the finding

Not a sweep of all 54 — most are legitimately optional (a scope id that narrows
an admin query is not a security control). The narrow version:

Make the five conditional-filter functions take the scope id as
required, and let callers that genuinely want unscoped access say so with
an explicit sentinel rather than by omission. An opt-out you have to type is
a decision; an opt-out you get by forgetting is not.
Repoint the two work_items.py sites at load_authorized(), which is
the already-consolidated version of what they hand-roll.
A guard over llc/services/ asserting no tenant filter is conditional on
an optional parameter — discovered by shape, floored at the count found, so
the sixth one fails on arrival.

Item 3 is the one that stops this recurring; 1 and 2 are today's instances.

Method note: the 54/5 split is an AST heuristic of mine, and I verified one of
the five by reading rather than trusting it. The other four I have not read —
worth confirming before acting on any of them, since a conditional filter can be
correct when the unscoped path is an admin one.

Taking this — branch issue-15930-goal-ancestors-scope. Claiming on the issue so any session reading it before starting sees it.

Scope note before I write anything: AC5 ("whether a tenant scope may be optional at all") is a repository-wide convention question, not a fix. I will make the route safe, classify revoke_key, and complete the parameter census — and I will frame AC5 with the evidence rather than deciding it, the way #15907 and #15912's AC1 went. A ruling that every tenant scope becomes required is a change to every such signature and their call sites; that is not mine to make from one route's fix.

fix(llc): the route must NOT pass company_id — it empties an admin's …

security(llc): transition_status enforced no ownership at all (#15952)

fix(llc): GH#8252's blocked-by rule had never fired (#15931) (#15939)

security(llc): scope the goal ancestry walk, and stop it defaulting o…

#15944 merged as of today, so this is now verifiable against base. Three of five met, one met by an argued deviation, one explicitly not decided — leaving this open.

#	criterion	verdict	evidence on merged base
1	the route passes the caller's company	deliberately not done	`goals.py` calls `_svc().get_ancestors(session, goal_id)` with a comment saying so: the service derives scope from the starting goal — the goal just authorised — so for a normal caller the two agree, and for a platform admin `ctx.org_id` would empty the chain. The criterion's effect is achieved; its mechanism is not. Recording it as a deviation rather than ticking it.
2	a test seeds a foreign parent, calls the route as the child's owner, asserts the ancestor is absent from the response	partially met	The scenario is proven at service level (`test_a_foreign_ancestor_is_absent_from_the_walk`, and `test_the_walk_is_scoped_even_when_the_caller_omits_the_company` — which is the production call shape). Route-level tests do exist here via `TestClient` on `/api/llc/goals/{id}/ancestors`, but they cover the admin chain and the non-admin 404, not this scenario. The described test is one fixture away, and the harness is already there.
3	`revoke_key` classified	met	`heartbeat_scheduler.py:1039` now passes `company_id=company_id`. Classified and fixed: an optional tenant scope that is never omitted cannot be omitted by accident.
4	every optional tenant-scope parameter enumerated, call sites classified	met — enumeration below
5	decided and recorded whether a tenant scope may be optional at all	NOT met	#15944's own body says "AC5 is framed, not decided".

Criterion 4 — the enumeration

Population is the parameter, not the string None. Twelve tenant-scope parameters across llc/ are declared optional:

llc/kb/artifact_ingestor.py:84    ArtifactIngestor.ingest(company_id=None)
llc/kb/artifact_ingestor.py:188   ArtifactIngestor.ingest_all_pending(company_id=None)
llc/kb/diary_writer.py:76         AgentDiaryKbWriter.write_from_run(company_id=None)
llc/kb/diary_writer.py:229        AgentDiaryKbWriter._write_patterns(company_id=None)
llc/services/api_key.py:61        ApiKeyService.revoke_key(company_id=None)
llc/services/goal.py:269          GoalService.get_ancestors(company_id=None)
llc/services/goal.py:325          GoalService.get_goal_ancestry_for_work_item(company_id=None)
llc/services/portability.py:564   PortabilityService._agent_names_exist(company_id=None)
llc/services/portability.py:579   PortabilityService._goal_titles_exist(company_id=None)
llc/services/routine_service.py:77 RoutineService.list(company_id=None)
llc/services/work_item_service.py:324 WorkItemService.get(company_id=None)
llc/services/work_item_service.py:838 WorkItemService.transition_status(company_id=None)

The one with real reach is WorkItemService.get, whose docstring is explicit that an omitted scope applies no tenant filter. llc/api/work_items.py calls it from 26 HTTP routes without passing the scope. All 26 are safe: each re-implements the check inline as if item is None or str(item.company_id) != str(ctx.org_id).

Two things worth recording about how that number was reached, because both are the failure this issue is about:

My first sweep matched the method names and reported 14,597 unscoped call sites. get and list are dict methods; the population was the wrong one entirely. That number is withdrawn, not corrected — it never measured anything.
The refined sweep then reported one unguarded route, suggest_acceptance_criteria(). It is guarded. It compares against a local company_id assigned from ctx.org_id two lines earlier, so my text pattern missed it. Reading the function is what settled it, not the sweep.

Why criterion 5 is the one that matters

The evidence for 4 is the argument for 5. The tenant check for WorkItemService.get is written out by hand 26 times because the parameter is optional. Every one is currently correct. Nothing makes the 27th correct, nothing makes its absence visible in a diff, and that is exactly how the two defects behind this issue survived #13704.

PR #15944 (merged to Dev_new_gui) references this issue with a close keyword.

security(llc): scope the goal ancestry walk, and stop it defaulting off (#15930)

If this issue is fully resolved, close it manually. If work remains, no action is needed.

Criterion 5 is now filed as #15989, so the decision outlives this issue rather than sitting in a verification comment. It carries the enumeration, the 26 hand-written checks, and both withdrawn measurements.

This issue's remaining gap is criterion 2 — the foreign-parent scenario proven through the route rather than at service level. The TestClient harness already exists in test_goal_ancestry_scope_15930.py, so that is one fixture.

test(llc): prove the ancestry leak is closed at the route, not the se…

Criterion 2 is addressed in PR #15988 (5ec5a405e4), on that branch rather than a new one because it already holds the llc/tests/ scope.

Two tests through the mounted router: a foreign parent must be absent for the child's own owner, and an own-company chain must still return in full — the second stops the first being satisfied by a route that returns [] unconditionally.

Mutation-proven, naming the tests rather than the totals:

mutation	tests that failed
remove the derived-scope default (the original defect)	`test_the_walk_is_scoped_even_when_the_caller_omits_the_company`, `test_the_route_omits_a_foreign_ancestor_for_the_childs_own_owner`
route always returns an empty chain	2 failed, the contrast among them

With that, criteria 1–4 are met (criterion 1 by the argued deviation recorded above) and criterion 5 is now #15989, so this issue is closable once #15988 merges — I'll verify against merged base and close then, not before.

test(llc): pin checkout_next's candidate window, and prove the ancest…

#15988 merged. Criterion 2 is now met on base; criteria 1–4 verified, criterion 5 lives on as #15989. Closing.

#	criterion	verdict	evidence on merged base
1	the route passes the caller's company	met by an argued deviation	`goals.py` deliberately does not pass it, with the reason in the code: `get_ancestors` derives scope from the starting goal — the goal just authorised — so for a normal caller the two agree, and for a platform admin `ctx.org_id` would empty the chain. The criterion's effect is achieved; its mechanism is not, and the deviation is recorded rather than silent.
2	a test seeds a foreign parent, calls the route as the child's owner, asserts the ancestor is absent from the response	met	`test_the_route_omits_a_foreign_ancestor_for_the_childs_own_owner` — through the mounted router, asserting on the response body. Paired with `test_the_route_returns_an_own_company_chain_in_full`, so a route returning `[]` unconditionally cannot pass.
3	`revoke_key` classified	met	`heartbeat_scheduler.py:1039` passes `company_id=company_id`. Classified and fixed: an optional tenant scope that is never omitted cannot be omitted by accident.
4	every optional tenant-scope parameter enumerated, call sites classified	met	Twelve parameters enumerated; the one with reach is `WorkItemService.get`, called from 26 HTTP routes without the scope, all 26 guarded by an inline `str(item.company_id) != str(ctx.org_id)`.
5	decided and recorded whether a tenant scope may be optional at all	moved to #15989	#15944's own body said "framed, not decided". It is now its own issue, with the enumeration, the 26 hand-written copies, and both withdrawn measurements.

On criterion 2, and why I did not tick it two rounds ago. The scenario was already proven at the service layer, which is the layer the defect hid behind: get_ancestors looked scoped because a caller could pass company_id, and the route did not. Service-level evidence is the same kind that kept this invisible, so the criterion asking for the boundary was right and the substance being covered was not sufficient.

Mutation-proven, naming the tests rather than reporting totals:

mutation	tests that failed
remove the derived-scope default (the original defect)	`test_the_walk_is_scoped_even_when_the_caller_omits_the_company`, `test_the_route_omits_a_foreign_ancestor_for_the_childs_own_owner`
route always returns an empty chain	2 failed, the contrast among them

Naming them mattered: a "2 failed" line is true even when the new test is inert and two others carry it, which is exactly what happened to me on #15962 the same day.

Criterion 5 is where the value now sits. The evidence for criterion 4 is the argument for it: that tenant check is written out by hand 26 times because the parameter is optional. All 26 are correct today; nothing makes the 27th correct, and nothing makes its absence visible in a diff.

merge: resolve base into #15962, and route four converted guards thro…

fix(guards): an empty enumeration must let the checker refuse audibly (…

guard(repo): five guards walked into other checkouts of this reposito…

Navigation Menu

Repository navigation

security(llc): the goal ancestors route scopes only the leaf, so a cross-company parent edge leaks the chain #15930

#13704 names this exact failure

Why this is reachable rather than theoretical

How it was found, which is the part worth generalising

Acceptance criteria

Relationship to #15924

The mechanism generalises — measured, with the caveat that I found no second live instance

I checked the most dangerous-looking one and it is not exploitable

What it does show

Suggestion, sized to the finding

Criterion 4 — the enumeration

Why criterion 5 is the one that matters

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

security(llc): the goal ancestors route scopes only the leaf, so a cross-company parent edge leaks the chain #15930

Description

#13704 names this exact failure

Why this is reachable rather than theoretical

How it was found, which is the part worth generalising

Acceptance criteria

Relationship to #15924

Activity

mrveiss commented on Sep 7, 2026

The mechanism generalises — measured, with the caveat that I found no second live instance

I checked the most dangerous-looking one and it is not exploitable

What it does show

Suggestion, sized to the finding

mrveiss commented on Sep 7, 2026

mrveiss commented on Sep 7, 2026

Criterion 4 — the enumeration

Why criterion 5 is the one that matters

github-actions commented on Sep 7, 2026

mrveiss commented on Sep 7, 2026

mrveiss commented on Sep 7, 2026

mrveiss commented on Sep 7, 2026

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions