Repository navigation
fix(signals): close the remaining scout lifecycle lock bypasses - #115256
trunk-io[bot] merged 2 commits into
Conversation
Gate a first config registration that sets lifecycle_locked, gate skill renames on the lock and move the scout config with the skill, keep the owner claim across archive-before-delete, and pass the request authenticator to refusal telemetry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 253dab3f-6465-44a8-afa1-7a59259f0043
|
😎 Merged successfully - details. |
PostHog Review · Deep · reviewed
|
| Step | Status | Result |
|---|---|---|
| Prepare the diff | done | 7 files, 1 chunk |
| Pick perspectives | done | 3 perspectives |
| Run review passes | done | 7 issues (+1 blind-spot) |
| Merge overlapping findings | done | 8 → 5 |
| Validate findings | done | 1 kept, 4 dismissed |
| Publish | done | 1 posted (view review) |
Also found in comments already on this pull request, so not posted again:
- P1 · Check destination configs before returning for an ordinary skill, raised by
parameterai[bot](comment)
|
Risk: Medium · 1 medium The PR closes three lifecycle-lock bypass routes: registering a locked config at first registration, renaming a locked scout's skill, and deleting a config after its skill was archived (owner now resolved from the most recent archive, which is sound because archiving stamps one Sentinel reviewed |
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved — escalated to a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
The authorization and API changes have no current-head independent review, owning-team authorship, or strong familiarity assurance. Request review covering these changes from @PostHog/team-self-driving and @PostHog/team-context-and-mcp before re-requesting.
- Risky territory without independent assurance: Auth or security-sensitive surface — products/signals/backend/scout_harness/lifecycle_lock.py and views.py change who can lock, rename, and delete scouts, including ownership resolution after skill archival.; Public API contracts — products/skills/backend/api/skills.py adds permission-denied and name-conflict outcomes to the skill rename endpoint, while products/signals/backend/scout_harness/views.py restricts initial registration with a lifecycle lock.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 218L, 6F substantive, 276L/7F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (276L, 7F, single-area, fix) |
| stamphog 2.4.1 | .stamphog/policy.yml @ 6c7e806 · reviewed head 6c7e806 |
🤖 CI report
|
| File | Patch | Uncovered changed lines |
|---|---|---|
products/skills/backend/api/skills.py |
75.0% | 1793, 1795 |
products/signals/backend/apps.py |
83.3% | 86–87 |
products/signals/backend/scout_harness/lifecycle_lock.py |
91.4% | 113, 235, 247 |
🤖 Agents: add a test only if an uncovered line exposes a realistic regression that existing tests miss. Otherwise explain why no new test is needed under "How did you test this code?". Gap list: the patch-coverage artifact on this run (gh run download 281915476031277 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
platform_features |
██░░░░░░░░░░░░░░░░░░ 12.1% |
7 / 58 |
demo |
███████████░░░░░░░░░ 55.9% |
1,508 / 2,700 |
data_tools |
████████████░░░░░░░░ 61.2% |
90 / 147 |
warehouse_sources_queue |
█████████████░░░░░░░ 66.8% |
1,921 / 2,875 |
aeo |
███████████████░░░░░ 76.3% |
617 / 809 |
batch_exports |
████████████████░░░░ 80.1% |
21,383 / 26,689 |
apm |
█████████████████░░░ 84.1% |
1,306 / 1,553 |
engineering_analytics |
█████████████████░░░ 84.9% |
11,355 / 13,374 |
mcp_analytics |
█████████████████░░░ 85.7% |
4,844 / 5,651 |
ml_inference |
█████████████████░░░ 86.3% |
524 / 607 |
warehouse_suggestions |
█████████████████░░░ 87.3% |
2,252 / 2,580 |
posthog_ai |
██████████████████░░ 87.7% |
4,226 / 4,817 |
notebooks |
██████████████████░░ 88.4% |
15,201 / 17,196 |
today |
██████████████████░░ 88.6% |
2,866 / 3,233 |
cdp |
██████████████████░░ 88.7% |
5,081 / 5,729 |
mcp_registry |
██████████████████░░ 89.0% |
1,596 / 1,794 |
product_tours |
██████████████████░░ 89.1% |
1,337 / 1,500 |
dashboards |
██████████████████░░ 89.5% |
7,205 / 8,050 |
webmcp |
██████████████████░░ 89.6% |
240 / 268 |
cohorts |
██████████████████░░ 89.6% |
8,458 / 9,438 |
signals |
██████████████████░░ 89.9% |
69,352 / 77,102 |
data_modeling |
██████████████████░░ 90.1% |
10,856 / 12,043 |
ai_training |
██████████████████░░ 90.4% |
349 / 386 |
tasks |
██████████████████░░ 90.6% |
80,632 / 89,022 |
visual_review |
██████████████████░░ 90.7% |
9,842 / 10,856 |
data_warehouse |
██████████████████░░ 90.8% |
14,831 / 16,339 |
canvas |
██████████████████░░ 90.9% |
7,748 / 8,522 |
exports |
██████████████████░░ 91.0% |
9,748 / 10,717 |
business_knowledge |
██████████████████░░ 91.0% |
8,791 / 9,656 |
autoresearch |
██████████████████░░ 91.1% |
10,356 / 11,368 |
managed_warehouse |
██████████████████░░ 91.2% |
11,053 / 12,114 |
error_tracking |
██████████████████░░ 91.6% |
16,797 / 18,328 |
wizard |
██████████████████░░ 91.8% |
6,008 / 6,548 |
streamlit_apps |
██████████████████░░ 91.9% |
3,139 / 3,415 |
stamphog |
██████████████████░░ 92.0% |
8,243 / 8,964 |
conversations |
██████████████████░░ 92.0% |
29,714 / 32,284 |
early_access_features |
███████████████████░ 92.6% |
1,339 / 1,446 |
managed_migrations |
███████████████████░ 92.6% |
1,630 / 1,760 |
alerts |
███████████████████░ 92.9% |
7,246 / 7,797 |
review_hog |
███████████████████░ 93.0% |
15,757 / 16,947 |
surveys |
███████████████████░ 93.1% |
6,626 / 7,118 |
web_analytics |
███████████████████░ 93.1% |
24,096 / 25,877 |
notifications |
███████████████████░ 93.2% |
1,152 / 1,236 |
approvals |
███████████████████░ 93.4% |
4,526 / 4,848 |
cross_project_dashboards |
███████████████████░ 93.4% |
880 / 942 |
slack_app |
███████████████████░ 93.5% |
14,854 / 15,882 |
context_layer |
███████████████████░ 93.6% |
3,408 / 3,640 |
marketing_analytics |
███████████████████░ 93.8% |
20,074 / 21,402 |
workflows |
███████████████████░ 93.8% |
16,619 / 17,718 |
messaging |
███████████████████░ 93.8% |
4,468 / 4,762 |
billing_alerts |
███████████████████░ 93.9% |
2,090 / 2,226 |
customer_analytics |
███████████████████░ 93.9% |
26,079 / 27,773 |
mcp_store |
███████████████████░ 93.9% |
9,008 / 9,593 |
ai_observability |
███████████████████░ 94.2% |
26,434 / 28,069 |
experiments |
███████████████████░ 94.2% |
33,733 / 35,817 |
replay_vision |
███████████████████░ 94.3% |
29,788 / 31,594 |
legal_documents |
███████████████████░ 94.3% |
2,289 / 2,427 |
logs |
███████████████████░ 94.5% |
15,850 / 16,770 |
actions |
███████████████████░ 94.6% |
973 / 1,029 |
reminders |
███████████████████░ 94.8% |
760 / 802 |
endpoints |
███████████████████░ 94.8% |
9,333 / 9,847 |
skills |
███████████████████░ 94.8% |
7,061 / 7,445 |
growth |
███████████████████░ 94.8% |
11,268 / 11,880 |
annotations |
███████████████████░ 95.0% |
816 / 859 |
tracing |
███████████████████░ 95.2% |
3,631 / 3,815 |
data_catalog |
███████████████████░ 95.5% |
4,419 / 4,628 |
access_control |
███████████████████░ 95.5% |
7,779 / 8,143 |
product_analytics |
███████████████████░ 95.7% |
28,723 / 29,999 |
alerts_platform |
███████████████████░ 95.8% |
5,013 / 5,234 |
revenue_analytics |
███████████████████░ 95.9% |
1,878 / 1,959 |
data_quality |
███████████████████░ 95.9% |
8,569 / 8,933 |
feature_flags |
███████████████████░ 96.2% |
26,900 / 27,965 |
warehouse_sources |
███████████████████░ 96.4% |
453,908 / 470,920 |
security |
███████████████████░ 96.7% |
1,452 / 1,501 |
pulse |
███████████████████░ 97.4% |
2,023 / 2,078 |
metrics |
████████████████████ 97.7% |
4,406 / 4,509 |
analytics_platform |
████████████████████ 98.0% |
2,775 / 2,833 |
field_notes |
████████████████████ 99.4% |
172 / 173 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
Archive and rename now use one refusal helper. The archived-owner lookup reuses the earliest-member-author query from the acting-user lookup and finds the newest archive in one query. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 253dab3f-6465-44a8-afa1-7a59259f0043
There was a problem hiding this comment.
Not approved — escalated to a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
Authorization and API-contract changes have no independent current-head assurance; the discussion status messages are not reviews. Request review covering these changes from @PostHog/team-self-driving and @PostHog/team-context-and-mcp before re-requesting.
- Risky territory without independent assurance: Auth or security-sensitive surface — products/signals/backend/scout_harness/lifecycle_lock.py and views.py change who may lock, rename, and delete scouts, including ownership resolution after archival.; Public API contracts — products/skills/backend/api/skills.py adds permission-denied and name-conflict responses to skill renames and documents the new 403 response.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 271L, 7F substantive, 329L/8F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (329L, 8F, single-area, fix) |
| stamphog 2.4.1 | .stamphog/policy.yml @ 2520b90 · reviewed head 2520b90 |
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| scoped_team = canonical_team(team) | ||
| configs = SignalScoutConfig.objects.for_team(scoped_team.id) | ||
| config = configs.select_for_update().filter(skill_name=skill_name).first() |
There was a problem hiding this comment.
P2 · Keep child-environment renames from moving parent scout configs
rename_skill() updates skill rows for the exact team, but this hook selects configs from the parent team. If the parent and child both contain custom-watch, renaming the child's skill moves the parent's config. The parent's skill keeps its original name, so its scout stops running. The skills endpoint does not apply ScoutCanonicalTeamAccessPermission. A child-scoped credential can therefore trigger this parent write when the config is unlocked. Return without moving parent configs when team.id differs from scoped_team.id. Keep the config and skill lookups bound to the same owning team. Add a regression case with matching parent and child skill names and a child-scoped API key. Assert that renaming the child skill leaves the parent skill and config unchanged.
| if configs.filter(skill_name=new_name).exists(): | ||
| raise ScoutConfigNameTaken( | ||
| "Another scout config already uses this name. Delete that config or pick another name." | ||
| ) | ||
| config.skill_name = new_name | ||
| config.save(update_fields=["skill_name", "updated_at"]) |
There was a problem hiding this comment.
P3 · Validate reserved scout names before moving the config
LLMSkillRenameSerializer accepts runs, findings, and scratchpad because these names are valid for ordinary skills. The new hook also accepts them and moves the scout config. Inbox routing explicitly ignores these names when selecting a scout, so the renamed scout's detail page cannot open. Scout creation and registration already reject them through reserved_scout_name_error(). Call reserved_scout_name_error(new_name) when the source has a scout config, before moving any rows. Propagate a validation error through the hook so the endpoint returns 400. Keep ordinary skill renames valid. Extend rename coverage with runs, findings, and scratchpad.
![parameterai[bot]](https://avatars.githubusercontent.com/in/2665233?s=60&v=4){kind=small}
| stamps one `updated_at` on every version it tombstones, and an older skill that once used the | ||
| name must not give its author the claim. | ||
| """ | ||
| if LLMSkill.objects.filter(team=team, name=skill_name, deleted=False).exists(): |
There was a problem hiding this comment.
The lifecycle lock can be defeated for any locked config whose skill was archived (the orphan state the config-destroy endpoint exists to clean up). Once a live skill exists under the config's skill_name, resolve_scout_lifecycle_owner_id takes the live path and names that skill's earliest author as the claim holder — so whoever re-creates the name becomes the owner, and the config's granted write scopes then run the attacker's prompt on schedule.
How:
- The scout's owner archives its skill (allowed); the locked
SignalScoutConfigstays keyed on the old skill name. - A plain member creates a new skill under that now-free name —
create_skillonly checks live rows (skill_services.py:651), and nothing in the skills product consults scout configs — or renames a non-scout skill onto it:rename_scout_with_skillreturns atif config is None(lifecycle_lock.py:233-234) before theskill_name=new_nameconflict check at line 246. - With a live skill under the name, line 106-107 resolves the claim to the earliest live author — the attacker — beating the archived-author fallback this PR added.
- The attacker now passes every lock gate (
user_holds_scout_lifecycle_claim, line 132): pauses, unlocks, or deletes the victim's locked config. (The scout-create route also skips the gate entirely when the skill was created fresh in the request,views.py:2813, applyingenabled: falsestraight to the adopted orphan config.)
Fix: Resolve the claim for an orphaned locked config from the config's own provenance (archived-author path) rather than whoever currently holds the name, and refuse a skill create/rename onto a name a scout config holds unless the caller holds that config's claim.
React with 👍 if useful or 👎 if not
There was a problem hiding this comment.
Thanks. This path exists on master, so this PR does not add it:
- On master,
user_holds_scout_lifecycle_claimcallsresolve_scout_acting_user_id, which picks the earliest live author (master permalink). Re-creating an archived scout's name already gives the new author the claim. - This PR only adds the archived-author fallback for the case with no live version. When a live version exists, the result stays the same as on master.
- A full fix has to stop
create_skilland every rename from taking a name that an orphan scout config holds, or bind the config to the skill identity instead of the name. That is a separate change in the skills product, outside this PR.
No code change in this PR for this thread. The gap stays open until a separate follow-up lands.
| archived = LLMSkill.objects.filter(team=team, name=skill_name, deleted=True) | ||
| last_archive = archived.filter(updated_at=Subquery(archived.order_by("-updated_at").values("updated_at")[:1])) | ||
| archived_author_id = earliest_member_author_id(team, last_archive) |
There was a problem hiding this comment.
P3 · Use immutable archive metadata to resolve the lifecycle owner
updated_at does not identify an archive operation. normalize_skill_file_paths.Command._apply updates this field on individual skill versions, including archived versions. If it updates a version written by an editor, last_archive excludes the creator's versions and assigns the lifecycle claim to that editor. The editor can then unlock or delete the creator's locked config. Updating an older archive can also restore a former author's claim. Preserve the resolved lifecycle owner when archiving, or record an immutable archive identifier that groups the archived versions. Do not derive authorization from updated_at. Add regression coverage that changes one archived version's updated_at and verifies that the lifecycle owner stays unchanged.
|
/trunk merge |
Problem
signal_scout:writecan still stop a locked scout, or lock a scout before its owner does, because three routes skip the owner-or-admin gate from #106109.lifecycle_locked: truegoes through for any member.auth_kind="other", because the endpoint does not pass its authenticator.Origin
signals-scout-inbox-validationChanges
lifecycle_locked: true. A plain registration withenabledoremitstays open, because nothing is locked yet.run_skill_rename_hooks), next to the archive guard. Signals registers the hook, so the skills product does not import signals.request.successful_authenticatorto the refusal counter.Note
A rename moves the config only. Past runs and notes stay under the old skill name.
How did you test this code?
Test rationale: Three new cases in
TestScoutHarnessConfigLifecycleLockAPI, next to the existing lock tests. Each failed on master and passes with the fix:Also ran locally: the existing lifecycle-lock class, the rename and archive tests in
products/skills/backend/api/test/test_skills.py, ruff, repo-wide mypy, andhogli product:lintfor signals and skills.Not checked:
hogli build:openapifailed in this sandbox in an unrelated dashboards widget step. The rename endpoint now declares a 403 response, so CI's OpenAPI gate is the check for generated types.Release status
Docs update
None.
🤖 Agent context
Autonomy: Fully autonomous
Agent: PostHog Desktop (Claude Code), claude-opus-5-5
Created with PostHog Desktop from this inbox report.
🤖 Generated with Claude Code