Repository navigation
fix(signals): close the remaining scout lifecycle lock bypasses #115256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,8 @@ | |
|
|
||
| from collections.abc import Mapping | ||
|
|
||
| from django.db.models import Subquery | ||
|
|
||
| import structlog | ||
| from prometheus_client import Counter | ||
|
|
||
|
|
@@ -25,7 +27,8 @@ | |
| from posthog.user_permissions import UserPermissions | ||
|
|
||
| from products.signals.backend.models import SignalScoutConfig | ||
| from products.signals.backend.scout_harness.skill_loader import resolve_scout_acting_user_id | ||
| from products.signals.backend.scout_harness.skill_loader import earliest_member_author_id, resolve_scout_acting_user_id | ||
| from products.skills.backend.models.skills import LLMSkill | ||
|
|
||
| logger = structlog.get_logger(__name__) | ||
|
|
||
|
|
@@ -48,6 +51,10 @@ class ScoutLifecycleLocked(Exception): | |
| """ | ||
|
|
||
|
|
||
| class ScoutConfigNameTaken(Exception): | ||
| """A skill rename would move a scout's config onto a name another config already holds.""" | ||
|
|
||
|
|
||
| def resolve_auth_kind(authenticator: object) -> str: | ||
| """How a caller authenticated, as a bounded metric label. | ||
|
|
||
|
|
@@ -87,6 +94,25 @@ def lock_protected_changes(requested: object, *, config: SignalScoutConfig | Non | |
| return changed if config.lifecycle_locked or "lifecycle_locked" in changed else [] | ||
|
|
||
|
|
||
| def resolve_scout_lifecycle_owner_id(team: Team, skill_name: str, config: SignalScoutConfig) -> int | None: | ||
| """The person the lock protects: the acting user, also after the skill is archived. | ||
|
|
||
| The delete flow archives the skill before it deletes the config. The acting-user lookup reads | ||
| live versions only, so without this fallback the skill author loses the claim between the two | ||
| steps and gets a refusal for their own scout. Only the most recent archive counts: an archive | ||
| 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(): | ||
| return resolve_scout_acting_user_id(team, skill_name, config) | ||
| 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) | ||
|
Comment on lines
+108
to
+110
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| if archived_author_id is not None: | ||
| return archived_author_id | ||
| return resolve_scout_acting_user_id(team, skill_name, config) | ||
|
|
||
|
|
||
| def user_holds_scout_lifecycle_claim( | ||
| *, | ||
| team: Team, | ||
|
|
@@ -103,7 +129,7 @@ def user_holds_scout_lifecycle_claim( | |
| level = UserPermissions(user=user, team=team).current_team.effective_membership_level | ||
| if level is not None and level >= OrganizationMembership.Level.ADMIN: | ||
| return True | ||
| return resolve_scout_acting_user_id(team, skill_name, config) == user.pk | ||
| return resolve_scout_lifecycle_owner_id(team, skill_name, config) == user.pk | ||
|
|
||
|
|
||
| def record_lifecycle_refusal( | ||
|
|
@@ -128,6 +154,33 @@ def record_lifecycle_refusal( | |
| ) | ||
|
|
||
|
|
||
| def _refuse_without_claim( | ||
| *, | ||
| team: Team, | ||
| skill_name: str, | ||
| config: SignalScoutConfig, | ||
| user: User, | ||
| authenticator: object, | ||
| action: str, | ||
| verb: str, | ||
| ) -> None: | ||
| """Raise `ScoutLifecycleLocked` for a locked scout unless the caller holds its claim.""" | ||
| if not config.lifecycle_locked: | ||
| return | ||
| if user_holds_scout_lifecycle_claim(team=team, skill_name=skill_name, config=config, user=user): | ||
| return | ||
| record_lifecycle_refusal( | ||
| team=team, | ||
| skill_name=skill_name, | ||
| action=action, | ||
| auth_kind=resolve_auth_kind(authenticator), | ||
| user_id=user.pk, | ||
| ) | ||
| raise ScoutLifecycleLocked( | ||
| f"This scout is locked, so only the person its runs act as or a project admin can {verb} it." | ||
| ) | ||
|
|
||
|
|
||
| def assert_can_archive_scout_skill( | ||
| *, | ||
| team: Team, | ||
|
|
@@ -145,17 +198,54 @@ def assert_can_archive_scout_skill( | |
| """ | ||
| scoped_team = canonical_team(team) | ||
| config = SignalScoutConfig.objects.for_team(scoped_team.id).filter(skill_name=skill_name).first() | ||
| if config is None or not config.lifecycle_locked: | ||
| return | ||
| if user_holds_scout_lifecycle_claim(team=scoped_team, skill_name=skill_name, config=config, user=user): | ||
| if config is None: | ||
| return | ||
| record_lifecycle_refusal( | ||
| _refuse_without_claim( | ||
| team=scoped_team, | ||
| skill_name=skill_name, | ||
| config=config, | ||
| user=user, | ||
| authenticator=authenticator, | ||
| action="archive_skill", | ||
| auth_kind=resolve_auth_kind(authenticator), | ||
| user_id=user.pk, | ||
| ) | ||
| raise ScoutLifecycleLocked( | ||
| "This scout is locked, so only the person its runs act as or a project admin can delete it." | ||
| verb="delete", | ||
| ) | ||
|
|
||
|
|
||
| def rename_scout_with_skill( | ||
| *, | ||
| team: Team, | ||
| skill_name: str, | ||
| new_name: str, | ||
| user: User | None, | ||
| authenticator: object = None, | ||
| ) -> None: | ||
| """Move a scout's config to its skill's new name, behind the same gate as archiving. | ||
|
|
||
| The config is keyed on the skill name. A rename that left it behind would orphan the config | ||
| and stop the scout, so a rename removes a scout as surely as an archive does. Call it inside | ||
| the rename transaction, before the skill rows move: the owner lookup reads the old name. | ||
|
|
||
| A no-op for a skill that is not a scout. A caller with no acting user is a system path, so it | ||
| is not gated. | ||
| """ | ||
| scoped_team = canonical_team(team) | ||
| configs = SignalScoutConfig.objects.for_team(scoped_team.id) | ||
| config = configs.select_for_update().filter(skill_name=skill_name).first() | ||
|
Comment on lines
+214
to
+216
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 config is None: | ||
| return | ||
| if user is not None: | ||
| _refuse_without_claim( | ||
| team=scoped_team, | ||
| skill_name=skill_name, | ||
| config=config, | ||
| user=user, | ||
| authenticator=authenticator, | ||
| action="rename_skill", | ||
| verb="rename", | ||
| ) | ||
| 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"]) | ||
|
Comment on lines
+234
to
+239
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_idtakes 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:
SignalScoutConfigstays keyed on the old skill 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.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. This path exists on master, so this PR does not add it:
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.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.