From 34fc212d98d60b73b2c4042606d121a405686aa5 Mon Sep 17 00:00:00 2001 From: ruby childs Date: Tue, 11 Aug 2026 15:02:28 -0400 Subject: [PATCH 001/231] feat(feature-flags): ship disable-and-archive dialog to everyone Removes the feature-flag-disable-and-archive-experiment gate so the disable confirmation dialog with "Disable only" (primary) and a danger-styled "Disable and archive" (secondary) shows for all users. Drops the control-variant fallback plumbing (openControlDialog) now that there is a single dialog, and removes the experiment flag constant. The disable/cancel/archive telemetry and the archive action wiring are unchanged. Generated-By: PostHog Desktop Task-Id: f8cd35e9-bf10-497a-9493-71ebfe6b617d --- frontend/src/lib/constants.tsx | 1 - .../featureFlagConfirmationLogic.ts | 8 +- .../featureFlagDisableDialog.test.ts | 94 +++++-------------- .../featureFlagDisableDialog.tsx | 26 +---- .../feature-flags/featureFlagLogic.test.ts | 13 +-- .../feature-flags/featureFlagsLogic.test.ts | 13 +-- .../scenes/feature-flags/featureFlagsLogic.ts | 22 ++--- 7 files changed, 42 insertions(+), 135 deletions(-) diff --git a/frontend/src/lib/constants.tsx b/frontend/src/lib/constants.tsx index 8c1e2fcfece3..9990629ef89b 100644 --- a/frontend/src/lib/constants.tsx +++ b/frontend/src/lib/constants.tsx @@ -176,7 +176,6 @@ export const FEATURE_FLAGS = { // UX flags, used to control the UX of the app CMD_K_NAV_EXPERIMENT: 'cmd-k-nav-experiment', // owner: @rafaeelaudibert #team-platform-ux multivariate=control,search-bar,footer-hint,tools-row,footer-callout - surfaces the Cmd+K command menu more prominently in the left nav: search-bar = full-width search field below the nav header, footer-hint = extra Search row in the nav footer, tools-row = Search row after the Tools item in the Project section, footer-callout = dismissible callout card in the nav ad slot for users a few days after signup CREATE_BUTTON_NAV_EXPERIMENT: 'create-button-nav-experiment', // owner: #team-platform-ux multivariate=control,test — adds a Create dropdown to the top of the Browse tab in the left nav - FEATURE_FLAG_DISABLE_AND_ARCHIVE_EXPERIMENT: 'feature-flag-disable-and-archive-experiment', // owner: #team-feature-flags multivariate=control,test, adds a destructive "Disable and archive" option alongside "Disable only" in the disable-flag confirmation dialog, control keeps the plain disable confirmation INSIGHT_NOTIFICATION_ENTRYPOINT: 'insight-notification-entrypoint', // owner: #team-product-analytics multivariate=control,notifications,get-updates,monitor — tests notification-entry-point copy against the prominent Subscribe button STARRED_REORDER: 'starred-reorder', // owner: #team-platform-ux, drag-and-drop reorder of starred shortcuts in the side panel UX_HIDE_PROJECT_NOTICE: 'ux-hide-project-notice', // owner: #team-platform-ux, hides the project notice banner across all scenes diff --git a/frontend/src/scenes/feature-flags/featureFlagConfirmationLogic.ts b/frontend/src/scenes/feature-flags/featureFlagConfirmationLogic.ts index 9f898e31e639..a111f01db115 100644 --- a/frontend/src/scenes/feature-flags/featureFlagConfirmationLogic.ts +++ b/frontend/src/scenes/feature-flags/featureFlagConfirmationLogic.ts @@ -148,16 +148,14 @@ export function checkFeatureFlagConfirmation( onCancel: onCancelModal, }) - // Disabling can offer "Disable and archive" behind the disable-and-archive experiment. - // Deliberately below the confirmation gate: a flag with dependents, or a team that set up - // its own confirmation, gets that modal instead. Losing an experiment exposure beats - // archiving a flag others read from. + // Disabling offers "Disable and archive" alongside "Disable only". Deliberately below the + // confirmation gate: a flag with dependents, or a team that set up its own confirmation, + // gets that modal instead. if (!updatedFlag.active && onDisableAndArchive) { openFeatureFlagDisableDialog({ source: 'feature-flag-detail', onDisable: onConfirm, onDisableAndArchive, - openControlDialog: openStatusConfirmationModal, }) return true } diff --git a/frontend/src/scenes/feature-flags/featureFlagDisableDialog.test.ts b/frontend/src/scenes/feature-flags/featureFlagDisableDialog.test.ts index cdfa4e840658..796c08ba43b2 100644 --- a/frontend/src/scenes/feature-flags/featureFlagDisableDialog.test.ts +++ b/frontend/src/scenes/feature-flags/featureFlagDisableDialog.test.ts @@ -2,34 +2,20 @@ import posthog from 'posthog-js' import { LemonDialog } from '@posthog/lemon-ui' -import { FEATURE_FLAGS } from 'lib/constants' -import { featureFlagLogic as enabledFeaturesLogic } from 'lib/logic/featureFlagLogic' - -import { initKeaTests } from '~/test/init' - import { FeatureFlagDisableDialogOption, openFeatureFlagDisableDialog } from './featureFlagDisableDialog' jest.mock('posthog-js') -const EXPERIMENT_KEY = FEATURE_FLAGS.FEATURE_FLAG_DISABLE_AND_ARCHIVE_EXPERIMENT - describe('openFeatureFlagDisableDialog', () => { - let flagsLogic: ReturnType let onDisable: jest.Mock let onDisableAndArchive: jest.Mock - let openControlDialog: jest.Mock let openDialog: jest.SpyInstance - const setVariant = (variant: string | boolean): void => { - flagsLogic.actions.setFeatureFlags([EXPERIMENT_KEY], { [EXPERIMENT_KEY]: variant }) - } - const open = (): void => openFeatureFlagDisableDialog({ source: 'feature-flags-list', onDisable, onDisableAndArchive, - openControlDialog, }) const optionCapturesOf = (option: FeatureFlagDisableDialogOption): any[][] => @@ -38,78 +24,40 @@ describe('openFeatureFlagDisableDialog', () => { ) beforeEach(() => { - initKeaTests() - flagsLogic = enabledFeaturesLogic() - flagsLogic.mount() onDisable = jest.fn() onDisableAndArchive = jest.fn() - openControlDialog = jest.fn() openDialog = jest.spyOn(LemonDialog, 'open').mockImplementation(() => {}) ;(posthog.capture as jest.Mock).mockClear() }) afterEach(() => { - flagsLogic.unmount() jest.restoreAllMocks() }) - describe('variant routing', () => { - it('offers "Disable and archive" to the test variant', () => { - setVariant('test') - open() - - expect(openControlDialog).not.toHaveBeenCalled() - expect(openDialog.mock.calls[0][0].primaryButton.children).toBe('Disable only') - expect(openDialog.mock.calls[0][0].secondaryButton).toMatchObject({ - children: 'Disable and archive', - status: 'danger', - }) - }) - - it.each<[string, string | boolean]>([ - ['control', 'control'], - ['an unset flag', false], - ['an unexpected variant', 'holdout'], - ])("falls back to the caller's own dialog for %s", (_label, variant) => { - setVariant(variant) - open() + it('makes "Disable only" the primary action and "Disable and archive" a danger secondary', () => { + open() - expect(openDialog).not.toHaveBeenCalled() - expect(openControlDialog).toHaveBeenCalledTimes(1) + expect(openDialog.mock.calls[0][0].primaryButton.children).toBe('Disable only') + expect(openDialog.mock.calls[0][0].secondaryButton).toMatchObject({ + children: 'Disable and archive', + status: 'danger', }) }) - describe('option telemetry', () => { - // A fresh dialog per case, so each one can assert the other callback stayed untouched. - it.each<[FeatureFlagDisableDialogOption, 'primaryButton' | 'secondaryButton' | 'tertiaryButton']>([ - ['disable', 'primaryButton'], - ['disable_and_archive', 'secondaryButton'], - ['cancel', 'tertiaryButton'], - ])('reports %s and runs only its own callback', (option, button) => { - setVariant('test') - open() - - openDialog.mock.calls[0][0][button].onClick() - - expect(optionCapturesOf(option)).toEqual([ - ['feature flag disable confirmation option selected', { source: 'feature-flags-list', option }], - ]) - expect(onDisableAndArchive).toHaveBeenCalledTimes(option === 'disable_and_archive' ? 1 : 0) - expect(onDisable).toHaveBeenCalledTimes(option === 'disable' ? 1 : 0) - }) - - it('wraps the control dialog callbacks so control reports the same options', () => { - setVariant('control') - open() - const [confirm, cancel] = openControlDialog.mock.calls[0] - - confirm() - expect(optionCapturesOf('disable')).toHaveLength(1) - expect(onDisable).toHaveBeenCalledTimes(1) - - cancel() - expect(optionCapturesOf('cancel')).toHaveLength(1) - expect(onDisableAndArchive).not.toHaveBeenCalled() - }) + // A fresh dialog per case, so each one can assert the other callback stayed untouched. + it.each<[FeatureFlagDisableDialogOption, 'primaryButton' | 'secondaryButton' | 'tertiaryButton']>([ + ['disable', 'primaryButton'], + ['disable_and_archive', 'secondaryButton'], + ['cancel', 'tertiaryButton'], + ])('reports %s and runs only its own callback', (option, button) => { + open() + + openDialog.mock.calls[0][0][button].onClick() + + expect(optionCapturesOf(option)).toEqual([ + ['feature flag disable confirmation option selected', { source: 'feature-flags-list', option }], + ]) + expect(onDisableAndArchive).toHaveBeenCalledTimes(option === 'disable_and_archive' ? 1 : 0) + expect(onDisable).toHaveBeenCalledTimes(option === 'disable' ? 1 : 0) }) }) diff --git a/frontend/src/scenes/feature-flags/featureFlagDisableDialog.tsx b/frontend/src/scenes/feature-flags/featureFlagDisableDialog.tsx index 64848e9504a8..22566da7de69 100644 --- a/frontend/src/scenes/feature-flags/featureFlagDisableDialog.tsx +++ b/frontend/src/scenes/feature-flags/featureFlagDisableDialog.tsx @@ -2,9 +2,6 @@ import posthog from 'posthog-js' import { LemonDialog } from '@posthog/lemon-ui' -import { FEATURE_FLAGS } from 'lib/constants' -import { featureFlagLogic as enabledFeaturesLogic } from 'lib/logic/featureFlagLogic' - export type FeatureFlagDisableDialogSource = 'feature-flags-list' | 'feature-flag-detail' export type FeatureFlagDisableDialogOption = 'disable' | 'disable_and_archive' | 'cancel' @@ -17,31 +14,19 @@ export function reportFeatureFlagDisableDialogOptionSelected( } /** - * Opens the disable confirmation dialog for a feature flag. The test variant of the - * disable-and-archive experiment gets "Disable only" as the primary CTA alongside a destructive - * "Disable and archive" option; control keeps each caller's pre-existing dialog, with this dialog's own - * option-selected telemetry wrapped around the caller's confirm/cancel. The experiment flag is - * read here rather than at render so the exposure lines up with the dialog actually opening. + * Opens the disable confirmation dialog for a feature flag. "Disable only" is the primary action; + * "Disable and archive" sits alongside it as a destructive secondary, so the more destructive + * option reads as destructive and isn't the default click. */ export function openFeatureFlagDisableDialog({ source, onDisable, onDisableAndArchive, - openControlDialog, }: { source: FeatureFlagDisableDialogSource onDisable: () => void onDisableAndArchive: () => void - /** The pre-experiment dialog, shown to the control variant. Called with confirm/cancel - * callbacks that already report the selected option — the caller only needs to wire them - * into its own dialog's primary/secondary buttons. */ - openControlDialog: (onConfirm?: () => void, onCancel?: () => void) => void }): void { - const inTestVariant = - enabledFeaturesLogic.findMounted()?.values.featureFlags[ - FEATURE_FLAGS.FEATURE_FLAG_DISABLE_AND_ARCHIVE_EXPERIMENT - ] === 'test' - posthog.capture('feature flag disable confirmation shown', { source }) const selectDisable = (): void => { @@ -54,11 +39,6 @@ export function openFeatureFlagDisableDialog({ onDisableAndArchive() } - if (!inTestVariant) { - openControlDialog(selectDisable, selectCancel) - return - } - LemonDialog.open({ title: 'Disable this flag?', description: diff --git a/frontend/src/scenes/feature-flags/featureFlagLogic.test.ts b/frontend/src/scenes/feature-flags/featureFlagLogic.test.ts index 642e88ba0132..8a877d3bb5e1 100644 --- a/frontend/src/scenes/feature-flags/featureFlagLogic.test.ts +++ b/frontend/src/scenes/feature-flags/featureFlagLogic.test.ts @@ -11,11 +11,9 @@ import { expectLogic, partial } from 'kea-test-utils' import posthog from 'posthog-js' import api from 'lib/api' -import { FEATURE_FLAGS } from 'lib/constants' import { dayjs } from 'lib/dayjs' import { LemonDialog } from 'lib/lemon-ui/LemonDialog' import { lemonToast } from 'lib/lemon-ui/LemonToast/LemonToast' -import { featureFlagLogic as enabledFeaturesLogic } from 'lib/logic/featureFlagLogic' import { eventUsageLogic } from 'lib/utils/eventUsageLogic' import { urls } from 'scenes/urls' @@ -2023,24 +2021,21 @@ describe('featureFlagLogic', () => { expect(dialogOpenSpy).toHaveBeenCalledTimes(1) const dialogProps = dialogOpenSpy.mock.calls[0][0] - expect(dialogProps.title).toBe('Disable feature flag "test-flag"?') - expect(dialogProps.primaryButton?.children).toBe('Disable flag') + expect(dialogProps.title).toBe('Disable this flag?') + expect(dialogProps.primaryButton?.children).toBe('Disable only') dialogOpenSpy.mockRestore() }) // onDisableAndArchive is optional at every hop between this listener and // checkFeatureFlagConfirmation, so dropping it anywhere still compiles and would silently - // put test-variant users back on the control dialog. - it('offers disable and archive to the test variant, archiving via the disable confirmation', async () => { + // fall back to the plain status confirmation without the archive option. + it('offers disable and archive, archiving via the disable confirmation', async () => { const dialogOpenSpy = jest.spyOn(LemonDialog, 'open').mockImplementation(() => {}) jest.spyOn(api, 'update').mockResolvedValueOnce({ ...MOCK_FEATURE_FLAG, archived: true, active: false, }) - enabledFeaturesLogic.actions.setFeatureFlags([FEATURE_FLAGS.FEATURE_FLAG_DISABLE_AND_ARCHIVE_EXPERIMENT], { - [FEATURE_FLAGS.FEATURE_FLAG_DISABLE_AND_ARCHIVE_EXPERIMENT]: 'test', - }) logic.actions.setFeatureFlag({ ...MOCK_FEATURE_FLAG, active: true }) await expectLogic(logic, () => logic.actions.toggleFeatureFlagActive(false)).toFinishAllListeners() diff --git a/frontend/src/scenes/feature-flags/featureFlagsLogic.test.ts b/frontend/src/scenes/feature-flags/featureFlagsLogic.test.ts index 483945943ddd..9d2e55f1fc32 100644 --- a/frontend/src/scenes/feature-flags/featureFlagsLogic.test.ts +++ b/frontend/src/scenes/feature-flags/featureFlagsLogic.test.ts @@ -3,9 +3,7 @@ import { expectLogic } from 'kea-test-utils' import posthog from 'posthog-js' import api from 'lib/api' -import { FEATURE_FLAGS } from 'lib/constants' import { LemonDialog } from 'lib/lemon-ui/LemonDialog' -import { featureFlagLogic as enabledFeaturesLogic } from 'lib/logic/featureFlagLogic' import { showApprovalRequiredToast } from 'scenes/approvals/ApprovalRequiredBanner' import { NEW_FLAG } from 'scenes/feature-flags/featureFlagLogic' import { @@ -431,16 +429,11 @@ describe('updateFeatureFlagArchived', () => { expect(logic.values.featureFlagsUpdating[1]).toBeUndefined() }) - // The list arm of the disable-and-archive experiment: the row toggle has to reach - // updateFeatureFlagArchived with the list's own via, not the archive dialog's. - it('archives via the disable confirmation when the test variant picks it', async () => { + // The list arm of the disable-and-archive dialog: picking "Disable and archive" from the row + // toggle has to reach updateFeatureFlagArchived with the list's own via, not the archive dialog's. + it('archives via the disable confirmation when disable and archive is picked', async () => { const openDialog = jest.spyOn(LemonDialog, 'open').mockImplementation(() => {}) jest.spyOn(api, 'update').mockResolvedValueOnce({ id: 1, key: 'test-flag', archived: true, active: false }) - const flagsLogic = enabledFeaturesLogic() - flagsLogic.mount() - flagsLogic.actions.setFeatureFlags([FEATURE_FLAGS.FEATURE_FLAG_DISABLE_AND_ARCHIVE_EXPERIMENT], { - [FEATURE_FLAGS.FEATURE_FLAG_DISABLE_AND_ARCHIVE_EXPERIMENT]: 'test', - }) logic.actions.toggleFeatureFlagActive(1, false) expect(openDialog.mock.calls[0][0].secondaryButton?.children).toBe('Disable and archive') diff --git a/frontend/src/scenes/feature-flags/featureFlagsLogic.ts b/frontend/src/scenes/feature-flags/featureFlagsLogic.ts index 88cfbafa0bf7..d9dbc58e222c 100644 --- a/frontend/src/scenes/feature-flags/featureFlagsLogic.ts +++ b/frontend/src/scenes/feature-flags/featureFlagsLogic.ts @@ -640,35 +640,30 @@ export const featureFlagsLogic = kea([ updateFlagActive: ({ id, active }) => { actions.updateFeatureFlag({ id, payload: { active } }) }, - // Mirrors featureFlagLogic's listener of the same name, so both surfaces of the - // disable-and-archive experiment are driven from a logic rather than from the row component. + // Mirrors featureFlagLogic's listener of the same name, so both the list and the detail + // view drive the toggle from a logic rather than from the row component. toggleFeatureFlagActive: ({ id, active }) => { const applyUpdate = (payload: Partial): void => { actions.updateFeatureFlag({ id, payload }) } - const openControlDialog = (onConfirm?: () => void, onCancel?: () => void): void => { + + if (active) { LemonDialog.open({ - title: `${active ? 'Enable' : 'Disable'} this flag?`, - description: `This flag will be immediately ${ - active ? 'rolled out to' : 'rolled back from' - } the users matching the release conditions.`, + title: 'Enable this flag?', + description: + 'This flag will be immediately rolled out to the users matching the release conditions.', primaryButton: { children: 'Confirm', type: 'primary', - onClick: onConfirm ?? (() => applyUpdate({ active })), + onClick: () => applyUpdate({ active: true }), size: 'small', }, secondaryButton: { children: 'Cancel', type: 'tertiary', size: 'small', - onClick: onCancel, }, }) - } - - if (active) { - openControlDialog() return } @@ -677,7 +672,6 @@ export const featureFlagsLogic = kea([ onDisable: () => applyUpdate({ active: false }), onDisableAndArchive: () => actions.updateFeatureFlagArchived({ id, archived: true, via: 'disable-confirmation' }), - openControlDialog, }) }, setFeatureFlagsFilters: async (_, breakpoint) => { From bdb89731b85bc5be9b8a1710922d46d522f59596 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 20 Aug 2026 12:52:14 +0200 Subject: [PATCH 002/231] feat(access-control): let orgs restrict MCP access to read-only Adds AccessCeiling, an org-scoped per-channel cap (channel, optional resource, max_level), with channel_ceiling()/classify_channel() in the access_control facade. APIScopePermission denies write-scoped actions for MCP-channel requests (classified by the MCP server user agent on token auth) when the org caps the channel at viewer, before the wildcard-scope early return so *-scoped tokens are clamped too. Gated on the organization security settings feature. No settings UI yet; rows are the API. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01LEer2y9uGMkADPjMfWG8WK --- posthog/permissions.py | 36 ++++++++ .../access_control/backend/facade/ceilings.py | 54 ++++++++++++ .../backend/migrations/0002_accessceiling.py | 74 ++++++++++++++++ .../backend/migrations/max_migration.txt | 2 +- .../access_control/backend/models/__init__.py | 3 +- .../backend/models/access_ceiling.py | 58 +++++++++++++ .../backend/tests/test_access_ceilings.py | 84 +++++++++++++++++++ 7 files changed, 309 insertions(+), 2 deletions(-) create mode 100644 products/access_control/backend/facade/ceilings.py create mode 100644 products/access_control/backend/migrations/0002_accessceiling.py create mode 100644 products/access_control/backend/models/access_ceiling.py create mode 100644 products/access_control/backend/tests/test_access_ceilings.py diff --git a/posthog/permissions.py b/posthog/permissions.py index bbcac70d574c..6b7e453a5029 100644 --- a/posthog/permissions.py +++ b/posthog/permissions.py @@ -721,6 +721,9 @@ def has_permission(self, request, view) -> bool: self.message = "This action does not support personal API key access" return False + if not self._check_channel_ceiling(request, view, required_scopes): + return False + if is_psak: self._check_project_secret_api_key_team(request, view) else: @@ -752,6 +755,39 @@ def has_permission(self, request, view) -> bool: return True + def _check_channel_ceiling(self, request, view, required_scopes: list[str]) -> bool: + """Deny write-scoped actions when the organization caps this request's channel below + editor. Runs before the wildcard-scope early return on purpose: a `*` token must not + bypass an organization-level restriction. Reads the same policy the access-control + facade composes into object decisions, so the two enforcement points cannot drift.""" + from products.access_control.backend.facade.ceilings import ( # noqa: PLC0415 — imported lazily to keep the products app out of this module's import cycle at django.setup() + WRITE_CAPPED_LEVELS, + channel_ceiling, + classify_channel, + ) + + channel = classify_channel(request) + if channel is None: + return True + if not any(scope.endswith(":write") for scope in required_scopes): + return True + try: + org = get_organization_from_view(view) + except ValueError: + return True + if not org.is_feature_available(AvailableFeature.ORGANIZATION_SECURITY_SETTINGS): + return True + + scope_object = self._get_scope_object(request, view) + cap = channel_ceiling(org, channel, scope_object if scope_object != "INTERNAL" else None) + if cap in WRITE_CAPPED_LEVELS: + self.message = ( + "Your organization restricts MCP access to read-only. " + "An organization admin can change this in your organization settings." + ) + return False + return True + def _check_project_secret_api_key_team(self, request, view) -> None: psak = request.successful_authenticator.project_secret_api_key try: diff --git a/products/access_control/backend/facade/ceilings.py b/products/access_control/backend/facade/ceilings.py new file mode 100644 index 000000000000..dc3db8b794b7 --- /dev/null +++ b/products/access_control/backend/facade/ceilings.py @@ -0,0 +1,54 @@ +"""Per-channel access ceilings: org-wide caps on what any principal can do through one pathway. + +The first consumer is `APIScopePermission`, which denies write-scoped actions when the request's +channel is capped below editor. When the access-control facade's `decide()` lands, it composes the +same cap into object-level decisions; both read this module, so the two enforcement points cannot +disagree about what the organization configured. +""" + +from typing import TYPE_CHECKING, Any + +from posthog.auth import OAuthAccessTokenAuthentication, PersonalAPIKeyAuthentication + +from products.access_control.backend.models.access_ceiling import AccessCeiling + +if TYPE_CHECKING: + from posthog.models.organization import Organization + +# The outbound identity of services/mcp (see its oauth-constants.ts). Channel classification is +# governance of the pathway, not a defense against a hostile key holder: the same credential used +# outside MCP keeps its own scopes, and tightening the credential itself is the mint-time follow-up. +MCP_USER_AGENT_MARKER = "posthog/mcp-server" + +WRITE_CAPPED_LEVELS = {AccessCeiling.MaxLevel.NONE, AccessCeiling.MaxLevel.VIEWER} + + +def classify_channel(request: Any) -> str | None: + """The access pathway this request arrived through, or None for pathways without policies.""" + authenticator = getattr(request, "successful_authenticator", None) + if isinstance(authenticator, PersonalAPIKeyAuthentication | OAuthAccessTokenAuthentication): + user_agent = request.headers.get("User-Agent") or "" + if MCP_USER_AGENT_MARKER in user_agent: + return AccessCeiling.Channel.MCP + return None + + +def channel_ceiling( + organization: "Organization", channel: str | None, resource: str | None = None +) -> AccessCeiling.MaxLevel | None: + """The max level this organization allows through `channel`, or None when unrestricted. + + A row naming `resource` overrides the wildcard row. One query per call; callers on hot + paths should check the channel first, since channel=None short-circuits. + """ + if channel is None: + return None + rows = AccessCeiling.objects.filter( + organization=organization, channel=channel, resource__in=[resource, None] if resource else [None] + ).values_list("resource", "max_level") + by_resource = dict(rows) + if resource is not None and resource in by_resource: + return AccessCeiling.MaxLevel(by_resource[resource]) + if None in by_resource: + return AccessCeiling.MaxLevel(by_resource[None]) + return None diff --git a/products/access_control/backend/migrations/0002_accessceiling.py b/products/access_control/backend/migrations/0002_accessceiling.py new file mode 100644 index 000000000000..e49168340490 --- /dev/null +++ b/products/access_control/backend/migrations/0002_accessceiling.py @@ -0,0 +1,74 @@ +# Generated by Django 5.2.17 on 2026-08-20 10:49 + +import django.db.models.deletion +from django.conf import settings +from django.db import migrations, models + +import posthog.uuidt + + +class Migration(migrations.Migration): + dependencies = [ + ("access_control", "0001_initial"), + ("posthog", "1310_provisioning_rate_limit_overrides"), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.CreateModel( + name="AccessCeiling", + fields=[ + ( + "id", + models.UUIDField( + default=posthog.uuidt.uuid7, + editable=False, + primary_key=True, + serialize=False, + ), + ), + ("channel", models.CharField(choices=[("mcp", "Mcp")], max_length=32)), + ("resource", models.CharField(blank=True, max_length=64, null=True)), + ( + "max_level", + models.CharField( + choices=[ + ("none", "None"), + ("viewer", "Viewer"), + ("editor", "Editor"), + ], + max_length=32, + ), + ), + ("created_at", models.DateTimeField(auto_now_add=True)), + ("updated_at", models.DateTimeField(auto_now=True)), + ( + "created_by", + models.ForeignKey( + db_constraint=False, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + to=settings.AUTH_USER_MODEL, + ), + ), + ( + "organization", + models.ForeignKey( + db_constraint=False, + on_delete=django.db.models.deletion.CASCADE, + related_name="access_ceilings", + to="posthog.organization", + ), + ), + ], + options={ + "constraints": [ + models.UniqueConstraint( + fields=("organization", "channel", "resource"), + name="unique_ceiling_per_org_channel_resource", + nulls_distinct=False, + ) + ], + }, + ), + ] diff --git a/products/access_control/backend/migrations/max_migration.txt b/products/access_control/backend/migrations/max_migration.txt index cbab66dde92a..ca3c6133d7f2 100644 --- a/products/access_control/backend/migrations/max_migration.txt +++ b/products/access_control/backend/migrations/max_migration.txt @@ -1 +1 @@ -0001_initial +0002_accessceiling diff --git a/products/access_control/backend/models/__init__.py b/products/access_control/backend/models/__init__.py index 332add9baf81..a105a3665831 100644 --- a/products/access_control/backend/models/__init__.py +++ b/products/access_control/backend/models/__init__.py @@ -1,3 +1,4 @@ +from .access_ceiling import AccessCeiling from .property_access_control import PropertyAccessControl -__all__ = ["PropertyAccessControl"] +__all__ = ["AccessCeiling", "PropertyAccessControl"] diff --git a/products/access_control/backend/models/access_ceiling.py b/products/access_control/backend/models/access_ceiling.py new file mode 100644 index 000000000000..7ae76c0f7e3d --- /dev/null +++ b/products/access_control/backend/models/access_ceiling.py @@ -0,0 +1,58 @@ +from django.db import models + +from posthog.models.utils import UUIDModel + + +class AccessCeiling(UUIDModel): + """An organization-wide cap on what any principal can do through one access pathway. + + Ceilings are not grants. The grants system (AccessControl rows) answers "what may this + principal do"; a ceiling answers "how wide is this pathway", and the effective access is + the minimum of the two. A ceiling therefore applies to every member, admins included: + exceptions are future subject-specific ceiling rows that widen the cap, never grants. + + Absence of a row means the channel is unrestricted. `resource=None` caps every resource; + a row naming a resource overrides the wildcard row for that resource. + """ + + class Channel(models.TextChoices): + MCP = "mcp" + + class MaxLevel(models.TextChoices): + # The grants vocabulary, minus levels a cap never needs. "none" disables the + # channel; "viewer" makes it read-only. + NONE = "none" + VIEWER = "viewer" + EDITOR = "editor" + + class Meta: + constraints = [ + models.UniqueConstraint( + fields=["organization", "channel", "resource"], + name="unique_ceiling_per_org_channel_resource", + nulls_distinct=False, + ) + ] + + # db_constraint=False: posthog_organization is a hot table, and creating a real FK + # constraint takes a lock on it that queues behind live writes. + organization = models.ForeignKey( + "posthog.Organization", + on_delete=models.CASCADE, + related_name="access_ceilings", + db_constraint=False, + ) + + channel: models.CharField = models.CharField(max_length=32, choices=Channel.choices) + # An APIScopeObject name, or None to cap every resource. + resource: models.CharField = models.CharField(max_length=64, null=True, blank=True) + max_level: models.CharField = models.CharField(max_length=32, choices=MaxLevel.choices) + + created_by = models.ForeignKey( + "posthog.User", + on_delete=models.SET_NULL, + null=True, + db_constraint=False, + ) + created_at: models.DateTimeField = models.DateTimeField(auto_now_add=True) + updated_at: models.DateTimeField = models.DateTimeField(auto_now=True) diff --git a/products/access_control/backend/tests/test_access_ceilings.py b/products/access_control/backend/tests/test_access_ceilings.py new file mode 100644 index 000000000000..b5a469c57e55 --- /dev/null +++ b/products/access_control/backend/tests/test_access_ceilings.py @@ -0,0 +1,84 @@ +from posthog.test.base import APIBaseTest, BaseTest + +from parameterized import parameterized + +from posthog.constants import AvailableFeature +from posthog.models.personal_api_key import PersonalAPIKey +from posthog.models.utils import generate_random_token_personal, hash_key_value + +from products.access_control.backend.facade.ceilings import MCP_USER_AGENT_MARKER, channel_ceiling +from products.access_control.backend.models import AccessCeiling + + +class TestChannelCeilingResolution(BaseTest): + def test_resource_row_overrides_wildcard_row(self) -> None: + AccessCeiling.objects.create(organization=self.organization, channel="mcp", resource=None, max_level="viewer") + AccessCeiling.objects.create( + organization=self.organization, channel="mcp", resource="feature_flag", max_level="editor" + ) + + assert channel_ceiling(self.organization, "mcp", "dashboard") == AccessCeiling.MaxLevel.VIEWER + assert channel_ceiling(self.organization, "mcp", "feature_flag") == AccessCeiling.MaxLevel.EDITOR + assert channel_ceiling(self.organization, "mcp") == AccessCeiling.MaxLevel.VIEWER + + def test_no_rows_and_no_channel_mean_unrestricted(self) -> None: + assert channel_ceiling(self.organization, "mcp", "dashboard") is None + assert channel_ceiling(self.organization, None, "dashboard") is None + + +class TestMCPReadOnlyEnforcement(APIBaseTest): + """The regression these guard: a write-scoped token arriving through the MCP pathway must be + denied when the org caps the channel, including `*`-scoped tokens, while reads and non-MCP + requests stay untouched. No existing test exercises the ceiling path at all.""" + + def setUp(self) -> None: + super().setUp() + self.organization.available_product_features = [ + { + "key": AvailableFeature.ORGANIZATION_SECURITY_SETTINGS, + "name": AvailableFeature.ORGANIZATION_SECURITY_SETTINGS, + } + ] + self.organization.save() + self.key_value = generate_random_token_personal() + PersonalAPIKey.objects.create( + label="mcp test", + user=self.user, + secure_value=hash_key_value(self.key_value), + scopes=["*"], + ) + self.client.logout() + + def _request(self, method: str, body: dict | None = None, mcp: bool = True): + return getattr(self.client, method)( + f"/api/projects/{self.team.id}/feature_flags/", + body or {}, + HTTP_AUTHORIZATION=f"Bearer {self.key_value}", + headers={"User-Agent": f"cursor/1.0 {MCP_USER_AGENT_MARKER}; version: 1.0.0"} if mcp else None, + ) + + def test_capped_channel_denies_writes_allows_reads(self) -> None: + AccessCeiling.objects.create(organization=self.organization, channel="mcp", max_level="viewer") + + denied = self._request("post", {"key": "flag-via-mcp", "name": "flag"}) + assert denied.status_code == 403 + assert "read-only" in denied.json()["detail"] + + assert self._request("get").status_code == 200 + + @parameterized.expand([("no_ceiling_row", True), ("not_mcp_user_agent", False)]) + def test_writes_pass_without_a_matching_ceiling(self, _name: str, mcp: bool) -> None: + if not mcp: + AccessCeiling.objects.create(organization=self.organization, channel="mcp", max_level="viewer") + + response = self._request("post", {"key": f"flag-{_name}", "name": "flag"}, mcp=mcp) + assert response.status_code == 201 + + def test_resource_exception_lets_that_resource_write(self) -> None: + AccessCeiling.objects.create(organization=self.organization, channel="mcp", max_level="viewer") + AccessCeiling.objects.create( + organization=self.organization, channel="mcp", resource="feature_flag", max_level="editor" + ) + + response = self._request("post", {"key": "flag-excepted", "name": "flag"}) + assert response.status_code == 201 From d2acaa5747c7e84cf25129ce891adf2a449b322e Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 20 Aug 2026 13:23:50 +0200 Subject: [PATCH 003/231] feat(access-control): keep the whole ceiling decision in the facade APIScopePermission now only translates DRF vocabulary and applies the verdict; classification, the entitlement gate, row lookup and the denial copy live in ceiling_denial_for_request. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01LEer2y9uGMkADPjMfWG8WK --- posthog/permissions.py | 33 ++++++++----------- .../access_control/backend/facade/ceilings.py | 25 ++++++++++++++ 2 files changed, 38 insertions(+), 20 deletions(-) diff --git a/posthog/permissions.py b/posthog/permissions.py index 6b7e453a5029..fc0488963506 100644 --- a/posthog/permissions.py +++ b/posthog/permissions.py @@ -756,35 +756,28 @@ def has_permission(self, request, view) -> bool: return True def _check_channel_ceiling(self, request, view, required_scopes: list[str]) -> bool: - """Deny write-scoped actions when the organization caps this request's channel below - editor. Runs before the wildcard-scope early return on purpose: a `*` token must not - bypass an organization-level restriction. Reads the same policy the access-control - facade composes into object decisions, so the two enforcement points cannot drift.""" + """Edge adapter for the access-control channel-ceiling policy: translate DRF vocabulary + (view, required scopes) into the facade's terms and apply its verdict. Runs before the + wildcard-scope early return on purpose: a `*` token must not bypass an organization-level + restriction. All policy lives in the facade so later enforcement points cannot drift.""" from products.access_control.backend.facade.ceilings import ( # noqa: PLC0415 — imported lazily to keep the products app out of this module's import cycle at django.setup() - WRITE_CAPPED_LEVELS, - channel_ceiling, - classify_channel, + ceiling_denial_for_request, ) - channel = classify_channel(request) - if channel is None: - return True - if not any(scope.endswith(":write") for scope in required_scopes): - return True try: org = get_organization_from_view(view) except ValueError: return True - if not org.is_feature_available(AvailableFeature.ORGANIZATION_SECURITY_SETTINGS): - return True scope_object = self._get_scope_object(request, view) - cap = channel_ceiling(org, channel, scope_object if scope_object != "INTERNAL" else None) - if cap in WRITE_CAPPED_LEVELS: - self.message = ( - "Your organization restricts MCP access to read-only. " - "An organization admin can change this in your organization settings." - ) + denial = ceiling_denial_for_request( + request, + org, + resource=scope_object if scope_object != "INTERNAL" else None, + writes=any(scope.endswith(":write") for scope in required_scopes), + ) + if denial is not None: + self.message = denial return False return True diff --git a/products/access_control/backend/facade/ceilings.py b/products/access_control/backend/facade/ceilings.py index dc3db8b794b7..16052b4e55f4 100644 --- a/products/access_control/backend/facade/ceilings.py +++ b/products/access_control/backend/facade/ceilings.py @@ -9,6 +9,7 @@ from typing import TYPE_CHECKING, Any from posthog.auth import OAuthAccessTokenAuthentication, PersonalAPIKeyAuthentication +from posthog.constants import AvailableFeature from products.access_control.backend.models.access_ceiling import AccessCeiling @@ -33,6 +34,30 @@ def classify_channel(request: Any) -> str | None: return None +def ceiling_denial_for_request( + request: Any, organization: "Organization", resource: str | None, writes: bool +) -> str | None: + """The complete channel-ceiling decision: a user-facing denial message when this request's + pathway is capped below what the action needs, or None to allow. + + Owns classification, the entitlement gate, row lookup and the copy, so enforcement points + (today `APIScopePermission`, later the facade's `decide()`) contain no policy of their own.""" + if not writes: + return None + channel = classify_channel(request) + if channel is None: + return None + if not organization.is_feature_available(AvailableFeature.ORGANIZATION_SECURITY_SETTINGS): + return None + cap = channel_ceiling(organization, channel, resource) + if cap in WRITE_CAPPED_LEVELS: + return ( + "Your organization restricts MCP access to read-only. " + "An organization admin can change this in your organization settings." + ) + return None + + def channel_ceiling( organization: "Organization", channel: str | None, resource: str | None = None ) -> AccessCeiling.MaxLevel | None: From 7fabd47a66360ed3559535f9aa18eab942a0196f Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 20 Aug 2026 18:18:53 +0200 Subject: [PATCH 004/231] feat(access-control): enforce channel ceilings as their own permission class ChannelCeilingPermission joins the mixin stack (including the dangerously_get_permissions branch, like domain enforcement) instead of living inside APIScopePermission. DRF evaluates permission classes with AND semantics, so the wildcard-scope early return can no longer matter, and session-authenticated channels become cappable later. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01LEer2y9uGMkADPjMfWG8WK --- posthog/api/routing.py | 15 ++++-- posthog/permissions.py | 29 ----------- .../backend/facade/permissions.py | 48 +++++++++++++++++++ 3 files changed, 59 insertions(+), 33 deletions(-) create mode 100644 products/access_control/backend/facade/permissions.py diff --git a/posthog/api/routing.py b/posthog/api/routing.py index 12dc9db8fd7f..70eb5c5ee538 100644 --- a/posthog/api/routing.py +++ b/posthog/api/routing.py @@ -42,6 +42,8 @@ from posthog.scopes import APIScopeObjectOrNotSupported from posthog.user_permissions import UserPermissions +from products.access_control.backend.facade.permissions import ChannelCeilingPermission + if TYPE_CHECKING: _GenericViewSet = GenericViewSet else: @@ -252,9 +254,9 @@ def get_permissions(self): except NotImplementedError: pass else: - # Domain enforcement is a tenant boundary, not an authorization level: views that - # shape their own permission chain cannot opt out of it. - return [*dangerously_defined, VerifiedDomainEnforcementPermission()] + # Domain enforcement and channel ceilings are tenant boundaries, not authorization + # levels: views that shape their own permission chain cannot opt out of them. + return [*dangerously_defined, ChannelCeilingPermission(), VerifiedDomainEnforcementPermission()] if isinstance(self.request.successful_authenticator, InternalAPIAuthentication): return [IsAuthenticated()] @@ -267,7 +269,12 @@ def get_permissions(self): # NOTE: We define these here to make it hard _not_ to use them. If you want to override them, you have to # override the entire method. - permission_classes: list = [IsAuthenticated, APIScopePermission, AccessControlPermission] + permission_classes: list = [ + IsAuthenticated, + APIScopePermission, + ChannelCeilingPermission, + AccessControlPermission, + ] if self._is_team_view or self._is_project_view: permission_classes.append(TeamMemberAccessPermission) diff --git a/posthog/permissions.py b/posthog/permissions.py index fc0488963506..bbcac70d574c 100644 --- a/posthog/permissions.py +++ b/posthog/permissions.py @@ -721,9 +721,6 @@ def has_permission(self, request, view) -> bool: self.message = "This action does not support personal API key access" return False - if not self._check_channel_ceiling(request, view, required_scopes): - return False - if is_psak: self._check_project_secret_api_key_team(request, view) else: @@ -755,32 +752,6 @@ def has_permission(self, request, view) -> bool: return True - def _check_channel_ceiling(self, request, view, required_scopes: list[str]) -> bool: - """Edge adapter for the access-control channel-ceiling policy: translate DRF vocabulary - (view, required scopes) into the facade's terms and apply its verdict. Runs before the - wildcard-scope early return on purpose: a `*` token must not bypass an organization-level - restriction. All policy lives in the facade so later enforcement points cannot drift.""" - from products.access_control.backend.facade.ceilings import ( # noqa: PLC0415 — imported lazily to keep the products app out of this module's import cycle at django.setup() - ceiling_denial_for_request, - ) - - try: - org = get_organization_from_view(view) - except ValueError: - return True - - scope_object = self._get_scope_object(request, view) - denial = ceiling_denial_for_request( - request, - org, - resource=scope_object if scope_object != "INTERNAL" else None, - writes=any(scope.endswith(":write") for scope in required_scopes), - ) - if denial is not None: - self.message = denial - return False - return True - def _check_project_secret_api_key_team(self, request, view) -> None: psak = request.successful_authenticator.project_secret_api_key try: diff --git a/products/access_control/backend/facade/permissions.py b/products/access_control/backend/facade/permissions.py new file mode 100644 index 000000000000..f5215882b65f --- /dev/null +++ b/products/access_control/backend/facade/permissions.py @@ -0,0 +1,48 @@ +"""DRF enforcement point for access-control policies owned by this product. + +`TeamAndOrgViewSetMixin.get_permissions` composes `ChannelCeilingPermission` into every viewset's +stack, so a new endpoint gets ceiling enforcement without knowing ceilings exist. DRF evaluates +permission classes with AND semantics: this class is an independent vote and cannot be bypassed by +another class's internal early return (a `*`-scoped token passing `APIScopePermission` is still +capped here). +""" + +from typing import Any + +from posthog.permissions import ScopeBasePermission, get_organization_from_view + +from products.access_control.backend.facade.ceilings import ceiling_denial_for_request, classify_channel + + +class ChannelCeilingPermission(ScopeBasePermission): + """Denies actions that exceed the organization's cap for the request's access pathway. + + Subclasses ScopeBasePermission only for `_get_required_scopes`, so this class derives + an action's read/write nature the same way APIScopePermission does and the two can't + disagree about what counts as a write.""" + + def has_permission(self, request: Any, view: Any) -> bool: + # Cheap exit first: almost every request has no classified channel, and classification + # is a couple of isinstance checks with no query. + if classify_channel(request) is None: + return True + + scope_object = getattr(view, "scope_object", None) + if scope_object is None: + return True + try: + organization = get_organization_from_view(view) + except ValueError: + return True + + required_scopes = self._get_required_scopes(request, view) or [] + denial = ceiling_denial_for_request( + request, + organization, + resource=scope_object if scope_object != "INTERNAL" else None, + writes=any(scope.endswith(":write") for scope in required_scopes), + ) + if denial is not None: + self.message = denial + return False + return True From f78fe0c10de9ffba7683d15374a39abd70d86560 Mon Sep 17 00:00:00 2001 From: Reece Jones Date: Fri, 21 Aug 2026 08:47:57 -0400 Subject: [PATCH 005/231] fix: make sure property access control is enforced across all supported tables --- posthog/hogql/printer/clickhouse.py | 4 +- .../test/test_property_access_control.py | 53 +++++++++++++++++++ posthog/hogql/restricted_properties.py | 31 ++++++++++- 3 files changed, 85 insertions(+), 3 deletions(-) diff --git a/posthog/hogql/printer/clickhouse.py b/posthog/hogql/printer/clickhouse.py index abccc7b9d5f6..1def4d690fa1 100644 --- a/posthog/hogql/printer/clickhouse.py +++ b/posthog/hogql/printer/clickhouse.py @@ -35,7 +35,7 @@ from posthog.hogql.helpers.timestamp_visitor import parse_zoned_datetime_string from posthog.hogql.printer.base import BasePrinter, get_channel_definition_dict, resolve_field_type from posthog.hogql.printer.hogql import HogQLPrinter -from posthog.hogql.restricted_properties import restricted_property_keys_for_table_type +from posthog.hogql.restricted_properties import RESTRICTABLE_JSON_BLOB_COLUMNS, restricted_property_keys_for_table_type from posthog.hogql.type_system import parse_sql_runtime_type from posthog.hogql.visitor import GetFieldsTraverser, clone_expr @@ -583,7 +583,7 @@ def _maybe_apply_json_drop_keys(self, type: ast.FieldType, field_sql: str) -> st # would incorrectly skip JSONDropKeys wrapping for the aliased ``properties`` column. # ``person_properties`` is the underlying DB column for ``EventsPersonSubTable.properties`` # (PoE mode); it is also a JSON blob that must be stripped of restricted person-property keys. - if resolved_field.name not in ("properties", "person_properties"): + if resolved_field.name not in RESTRICTABLE_JSON_BLOB_COLUMNS: return field_sql keys_to_drop = restricted_property_keys_for_table_type(type.table_type, self.context) diff --git a/posthog/hogql/printer/test/test_property_access_control.py b/posthog/hogql/printer/test/test_property_access_control.py index 38283c9dde27..d4f33588e406 100644 --- a/posthog/hogql/printer/test/test_property_access_control.py +++ b/posthog/hogql/printer/test/test_property_access_control.py @@ -350,6 +350,59 @@ def test_event_restriction_does_not_affect_person_properties_blob(self): sql = self._compile_select("SELECT person.properties FROM events") assert "JSONDropKeys" not in sql + def _restrict_group_property(self, name: str = "arr", group_type_index: int = 0) -> None: + group_prop = PropertyDefinition.objects.create( + team=self.team, + name=name, + property_type="Numeric", + type=PropertyDefinition.Type.GROUP, + group_type_index=group_type_index, + ) + PropertyAccessControl.objects.create( + team=self.team, + property_definition=group_prop, + access_level=PropertyAccessLevel.NONE.value, + ) + + @parameterized.expand([("groups",), ("raw_groups",)]) + def test_groups_tables_properties_blob_strips_restricted_keys(self, table_name: str): + # The groups blob (`group_properties`) was read unscrubbed, so `SELECT properties FROM groups` returned a group + # property an admin had restricted. + self._restrict_group_property() + sql, values = self._compile_select_with_values(f"SELECT properties FROM {table_name}") + assert "JSONDropKeys" in sql + assert "arr" not in sql + self._assert_value_present(values, "arr") + + def test_denied_group_property_read_is_not_extracted(self): + # `SELECT properties.arr FROM groups` used to compile to a real JSON read of the restricted key. + self._restrict_group_property() + sql, values = self._compile_select_with_values("SELECT properties.arr FROM groups") + # The lazy groups table hoists the read into its argMax subquery, where the restricted value is a constant NULL. + assert "JSONExtract" not in sql + assert "'arr'" not in sql + assert not any("arr" in str(v) for v in values.values()) + + @parameterized.expand( + [ + ("groups_lazy_join", "SELECT group_0.properties FROM events"), + ("group_on_events", "SELECT goe_0.properties FROM events"), + ] + ) + def test_group_properties_blob_on_events_strips_restricted_keys(self, _case_name: str, query: str): + # Group properties reachable from events — through the groups lazy join and through the group-on-events blob + # columns — must be scrubbed too, or the restriction is one join away from bypassed. + self._restrict_group_property() + sql, values = self._compile_select_with_values(query) + assert "JSONDropKeys" in sql + assert "arr" not in sql + self._assert_value_present(values, "arr") + + def test_group_restriction_does_not_affect_event_properties_blob(self): + self._restrict_group_property() + sql = self._compile_select("SELECT properties FROM events") + assert "JSONDropKeys" not in sql + def test_restrictions_do_not_affect_non_event_or_person_tables(self): PropertyAccessControl.objects.create( team=self.team, diff --git a/posthog/hogql/restricted_properties.py b/posthog/hogql/restricted_properties.py index cf54e7500da9..53ba8b54526f 100644 --- a/posthog/hogql/restricted_properties.py +++ b/posthog/hogql/restricted_properties.py @@ -2,11 +2,27 @@ from posthog.hogql import ast from posthog.hogql.context import HogQLContext -from posthog.hogql.database.schema.events import EventsPersonSubTable, EventsTable +from posthog.hogql.database.schema.events import EventsGroupSubTable, EventsPersonSubTable, EventsTable +from posthog.hogql.database.schema.groups import GroupsTable, RawGroupsTable from posthog.hogql.database.schema.persons import PersonsTable, RawPersonsTable +from posthog.constants import GROUP_TYPES_LIMIT + logger = structlog.get_logger(__name__) +# JSON blob columns that hold a restrictable property class, so the printer knows which blob reads to wrap in +# JSONDropKeys. Everything here must be covered by a branch in `restricted_property_keys_for_table_type`, and vice +# versa — a blob whose table type maps to a property class but whose column is missing here is read unscrubbed. +RESTRICTABLE_JSON_BLOB_COLUMNS: frozenset[str] = frozenset( + { + "properties", # events.properties, persons.properties, groups.group_properties reads via the HogQL name + "person_properties", # EventsPersonSubTable (PoE mode) + "group_properties", # groups / raw_groups + # EventsGroupSubTable (group-on-events mode) exposes each group type's blob on the events table. + *(f"group{index}_properties" for index in range(GROUP_TYPES_LIMIT)), + } +) + def restricted_property_keys_for_table_type(table_type: ast.Type, context: HogQLContext) -> set[str]: """Top-level property names restricted by property-level access control for a table, or an empty set. @@ -34,13 +50,26 @@ def restricted_property_keys_for_table_type(table_type: ast.Type, context: HogQL logger.warning("restricted_property_table_resolution_failed", table_type=type(table_type).__name__) return set() + # EventsPersonSubTable and EventsGroupSubTable are virtual tables over `events`, not EventsTable subclasses, but + # they carry person/group properties — match them before the EventsTable branch either way. if isinstance(table, EventsPersonSubTable): prop_def_type = PropertyDefinition.Type.PERSON + elif isinstance(table, EventsGroupSubTable): + prop_def_type = PropertyDefinition.Type.GROUP elif isinstance(table, EventsTable): prop_def_type = PropertyDefinition.Type.EVENT elif isinstance(table, (PersonsTable, RawPersonsTable)): prop_def_type = PropertyDefinition.Type.PERSON + elif isinstance(table, (GroupsTable, RawGroupsTable)): + # Group property definitions are per group type index, but `context.restricted_properties` carries only + # (name, type) — so a restricted name is declined for every group type. Over-restricting one group type's + # property on another is the fail-closed direction; the alternative leaks the restricted value. + prop_def_type = PropertyDefinition.Type.GROUP else: + # PropertyDefinition.Type.SESSION is deliberately absent: the sessions tables expose each session property as + # its own column rather than a JSON blob, so there is nothing for this function's callers to scrub. Restricting + # a session property therefore has no query-time effect yet — enforcing it needs field-level denial, not a + # blob-key drop. return set() return {name for name, ptype in context.restricted_properties if ptype == prop_def_type} From 85877af408abc7dbbf177d5229828c1069a7f7f4 Mon Sep 17 00:00:00 2001 From: Reece Jones Date: Fri, 21 Aug 2026 08:48:20 -0400 Subject: [PATCH 006/231] fix(hogql): scope restricted group properties Generated-By: PostHog Desktop Task-Id: d95a2f15-384e-4099-8837-73058dddf994 --- posthog/hogql/ACCESS_CONTROL.md | 6 ++-- posthog/hogql/context.py | 4 +-- posthog/hogql/database/schema/events.py | 3 ++ posthog/hogql/printer/clickhouse.py | 9 +++++- .../test/test_property_access_control.py | 20 +++++++++++++ posthog/hogql/printer/utils.py | 8 +++-- posthog/hogql/restricted_properties.py | 24 +++++++++++---- .../test/test_events_predicate_pushdown.py | 2 +- .../transforms/test/test_property_types.py | 14 ++++----- posthog/hogql_queries/query_runner.py | 10 ++++--- .../backend/property_access_control.py | 30 +++++++++++++------ 11 files changed, 95 insertions(+), 35 deletions(-) diff --git a/posthog/hogql/ACCESS_CONTROL.md b/posthog/hogql/ACCESS_CONTROL.md index 1a7316db3f2f..c4231f9312e3 100644 --- a/posthog/hogql/ACCESS_CONTROL.md +++ b/posthog/hogql/ACCESS_CONTROL.md @@ -166,7 +166,7 @@ execute_hogql_query(query=..., team=team, bypass_warehouse_access_control=True) ## 3. Property access control -Hides sensitive event and person properties (e.g. `email`) from query results. +Hides sensitive event, person, and group properties (e.g. `email`) from query results. Rules live in the `PropertyAccessControl` model (`products/access_control/backend/models/property_access_control.py`). Property access control is a paid feature, available on the Scale and Enterprise plans: it needs the `PROPERTY_ACCESS_CONTROL` entitlement, and without it resolution short-circuits to no restrictions. @@ -179,6 +179,8 @@ They're masked when the query is printed to ClickHouse SQL, so a restricted read - **Explicit reads** (`properties.email`) are replaced with `NULL`, and the resolver refuses to back them with a materialized column — `ClickHousePropertyResolver` in `posthog/hogql/transforms/clickhouse_property_resolution.py`. - **Whole-blob reads** (`SELECT properties` or `SELECT *`) have the restricted keys stripped from the returned JSON via `JSONDropKeys(...)` — `ClickHousePrinter._maybe_apply_json_drop_keys()` in `posthog/hogql/printer/clickhouse.py`. +Group restrictions retain their group type index, so a same-named property on another group type stays readable. The masking also applies to the Postgres-backed `system.groups.group_properties` field. + The restriction set is loaded once per query in `prepare_ast_for_printing()` and cached per `(team_id, user_id)` for the request lifetime. ### No user: default rules apply @@ -193,7 +195,7 @@ Otherwise a denied user gets served an allowed user's cached rows. The cache key is derived from `get_cache_payload()`: -- `QueryRunner.get_cache_payload()` adds `restricted_properties` (sorted `(name, type)` pairs) when the user has property restrictions. +- `QueryRunner.get_cache_payload()` adds `restricted_properties` (sorted `(name, type, group_type_index)` tuples) when the user has property restrictions. - `AnalyticsQueryRunner.get_cache_payload()` adds `restricted_resources` (denied scopes) and `restricted_objects` (denied object IDs per scope) for levels 1 and 2. Two things keep cache hit rates high: diff --git a/posthog/hogql/context.py b/posthog/hogql/context.py index 532b83b9c909..c4f01a8fdd0f 100644 --- a/posthog/hogql/context.py +++ b/posthog/hogql/context.py @@ -130,10 +130,10 @@ class HogQLContext: # filter and holding lazy introspection objects, so tables resolving to the same bound walk the # database only once while surface-specific catalog metadata is fetched only when requested. information_schema_introspection: Optional[Any] = field(default=None, compare=False, repr=False) - # Property-level access control: set of (property_name, PropertyDefinition.Type) tuples + # Property-level access control: (property_name, PropertyDefinition.Type, group_type_index) tuples # that the current user is denied access to. Populated before type resolution so that # FieldType.get_child() can raise QueryError for restricted properties. - restricted_properties: Optional[set[tuple[str, int]]] = None + restricted_properties: Optional[set[tuple[str, int, int | None]]] = None # Per-query cache of CTE synthetic tables, keyed by id() of the CTE's SelectQueryType. Value pins a # strong ref to the keyed type so its id can't be reused while cached; lookups verify identity. diff --git a/posthog/hogql/database/schema/events.py b/posthog/hogql/database/schema/events.py index 529115782043..7c0be9230f66 100644 --- a/posthog/hogql/database/schema/events.py +++ b/posthog/hogql/database/schema/events.py @@ -58,6 +58,8 @@ def to_printed_hogql(self): class EventsGroupSubTable(VirtualTable): + group_index: int = 0 + def __init__(self, group_index: int): super().__init__( fields={ @@ -66,6 +68,7 @@ def __init__(self, group_index: int): "properties": StringJSONDatabaseField(name=f"group{group_index}_properties", nullable=False), } ) + self.group_index = group_index def avoid_asterisk_fields(self): return [] diff --git a/posthog/hogql/printer/clickhouse.py b/posthog/hogql/printer/clickhouse.py index 1def4d690fa1..39a3a8c84a57 100644 --- a/posthog/hogql/printer/clickhouse.py +++ b/posthog/hogql/printer/clickhouse.py @@ -586,7 +586,14 @@ def _maybe_apply_json_drop_keys(self, type: ast.FieldType, field_sql: str) -> st if resolved_field.name not in RESTRICTABLE_JSON_BLOB_COLUMNS: return field_sql - keys_to_drop = restricted_property_keys_for_table_type(type.table_type, self.context) + group_type_index = None + group_column_match = re.fullmatch(r"group(\d+)_properties", resolved_field.name) + if group_column_match: + group_type_index = int(group_column_match.group(1)) + + keys_to_drop = restricted_property_keys_for_table_type( + type.table_type, self.context, group_type_index=group_type_index + ) if not keys_to_drop: return field_sql diff --git a/posthog/hogql/printer/test/test_property_access_control.py b/posthog/hogql/printer/test/test_property_access_control.py index d4f33588e406..ffff4443d341 100644 --- a/posthog/hogql/printer/test/test_property_access_control.py +++ b/posthog/hogql/printer/test/test_property_access_control.py @@ -383,6 +383,26 @@ def test_denied_group_property_read_is_not_extracted(self): assert "'arr'" not in sql assert not any("arr" in str(v) for v in values.values()) + def test_system_groups_properties_blob_strips_restricted_keys(self): + self._restrict_group_property() + sql, values = self._compile_select_with_values("SELECT group_properties FROM system.groups") + assert "JSONDropKeys" in sql + assert "arr" not in sql + self._assert_value_present(values, "arr") + + def test_denied_system_group_property_read_is_not_extracted(self): + self._restrict_group_property() + sql, values = self._compile_select_with_values("SELECT group_properties.arr FROM system.groups") + assert "JSONExtract" not in sql + assert "'arr'" not in sql + assert not any("arr" in str(value) for value in values.values()) + + def test_group_restriction_only_applies_to_matching_group_type(self): + self._restrict_group_property(group_type_index=0) + sql = self._compile_select("SELECT goe_1.properties.arr FROM events") + assert "JSONExtract" in sql + assert "arr" in sql + @parameterized.expand( [ ("groups_lazy_join", "SELECT group_0.properties FROM events"), diff --git a/posthog/hogql/printer/utils.py b/posthog/hogql/printer/utils.py index 06d70bd04a68..1d07fd78444e 100644 --- a/posthog/hogql/printer/utils.py +++ b/posthog/hogql/printer/utils.py @@ -158,14 +158,16 @@ def prepare_ast_for_printing( # load_property_metadata) — keeping it behind the call is what lets the printer package import # without django.setup(). from products.access_control.backend.property_access_control import ( # noqa: PLC0415 - get_restricted_properties_for_team, + get_restricted_properties_with_group_type_index_for_team, ) with context.timings.measure("load_restricted_properties"): if context.team is not None and context.team.pk == context.team_id: - context.restricted_properties = get_restricted_properties_for_team(user=context.user, team=context.team) + context.restricted_properties = get_restricted_properties_with_group_type_index_for_team( + user=context.user, team=context.team + ) else: - context.restricted_properties = get_restricted_properties_for_team( + context.restricted_properties = get_restricted_properties_with_group_type_index_for_team( user=context.user, team_id=context.team_id ) diff --git a/posthog/hogql/restricted_properties.py b/posthog/hogql/restricted_properties.py index 53ba8b54526f..2356ba75c0f3 100644 --- a/posthog/hogql/restricted_properties.py +++ b/posthog/hogql/restricted_properties.py @@ -2,6 +2,7 @@ from posthog.hogql import ast from posthog.hogql.context import HogQLContext +from posthog.hogql.database.postgres_table import PostgresTable from posthog.hogql.database.schema.events import EventsGroupSubTable, EventsPersonSubTable, EventsTable from posthog.hogql.database.schema.groups import GroupsTable, RawGroupsTable from posthog.hogql.database.schema.persons import PersonsTable, RawPersonsTable @@ -24,7 +25,9 @@ ) -def restricted_property_keys_for_table_type(table_type: ast.Type, context: HogQLContext) -> set[str]: +def restricted_property_keys_for_table_type( + table_type: ast.Type, context: HogQLContext, *, group_type_index: int | None = None +) -> set[str]: """Top-level property names restricted by property-level access control for a table, or an empty set. Single source of truth shared by the ClickHouse printer (which JSONDropKeys-wraps the blob) and the property @@ -56,14 +59,14 @@ def restricted_property_keys_for_table_type(table_type: ast.Type, context: HogQL prop_def_type = PropertyDefinition.Type.PERSON elif isinstance(table, EventsGroupSubTable): prop_def_type = PropertyDefinition.Type.GROUP + group_type_index = table.group_index elif isinstance(table, EventsTable): prop_def_type = PropertyDefinition.Type.EVENT elif isinstance(table, (PersonsTable, RawPersonsTable)): prop_def_type = PropertyDefinition.Type.PERSON - elif isinstance(table, (GroupsTable, RawGroupsTable)): - # Group property definitions are per group type index, but `context.restricted_properties` carries only - # (name, type) — so a restricted name is declined for every group type. Over-restricting one group type's - # property on another is the fail-closed direction; the alternative leaks the restricted value. + elif isinstance(table, (GroupsTable, RawGroupsTable)) or ( + isinstance(table, PostgresTable) and table.postgres_table_name == "posthog_group" + ): prop_def_type = PropertyDefinition.Type.GROUP else: # PropertyDefinition.Type.SESSION is deliberately absent: the sessions tables expose each session property as @@ -72,4 +75,13 @@ def restricted_property_keys_for_table_type(table_type: ast.Type, context: HogQL # blob-key drop. return set() - return {name for name, ptype in context.restricted_properties if ptype == prop_def_type} + return { + name + for name, ptype, restricted_group_type_index in context.restricted_properties + if ptype == prop_def_type + and ( + prop_def_type != PropertyDefinition.Type.GROUP + or group_type_index is None + or restricted_group_type_index == group_type_index + ) + } diff --git a/posthog/hogql/transforms/test/test_events_predicate_pushdown.py b/posthog/hogql/transforms/test/test_events_predicate_pushdown.py index 1044be850aec..94195d3208f4 100644 --- a/posthog/hogql/transforms/test/test_events_predicate_pushdown.py +++ b/posthog/hogql/transforms/test/test_events_predicate_pushdown.py @@ -146,7 +146,7 @@ def _print(push_down: bool) -> str: enable_select_queries=True, modifiers=HogQLQueryModifiers(pushDownPredicates=push_down), ) - context.restricted_properties = {("email", PropertyDefinition.Type.EVENT)} + context.restricted_properties = {("email", PropertyDefinition.Type.EVENT, None)} query, _ = prepare_and_print_ast(parse_select(select), context, "clickhouse") return pretty_print_in_tests(query, self.team.pk) diff --git a/posthog/hogql/transforms/test/test_property_types.py b/posthog/hogql/transforms/test/test_property_types.py index 5c043d8bd588..a47b02234d97 100644 --- a/posthog/hogql/transforms/test/test_property_types.py +++ b/posthog/hogql/transforms/test/test_property_types.py @@ -298,7 +298,7 @@ def _events_schema_snapshot(self): def _plan_where_comparison( self, select: str, - restricted_properties: set[tuple[str, int]] | None = None, + restricted_properties: set[tuple[str, int, int | None]] | None = None, ) -> PropertyComparisonPlan: context, resolved = self._resolve_select(select, restricted_properties=restricted_properties) comparison = cast(ast.CompareOperation, resolved.where) @@ -309,7 +309,7 @@ def _plan_where_comparison( def _resolve_select( self, select: str, - restricted_properties: set[tuple[str, int]] | None = None, + restricted_properties: set[tuple[str, int, int | None]] | None = None, ) -> tuple[HogQLContext, ast.SelectQuery]: """Resolve types and build the property-swapper registry without preparing further. @@ -329,7 +329,7 @@ def _resolve_select( def _prepare_select( self, select: str, - restricted_properties: set[tuple[str, int]] | None = None, + restricted_properties: set[tuple[str, int, int | None]] | None = None, ) -> tuple[HogQLContext, ast.SelectQuery]: expr = parse_select(select) context = HogQLContext(team_id=self.team.pk, team=self.team, enable_select_queries=True) @@ -503,7 +503,7 @@ def test_property_comparison_planner_respects_restricted_property_materializatio with materialized("events", "$browser", is_nullable=True, create_minmax_index=True): plan = self._plan_where_comparison( "select count() from events where properties.$browser < 'm'", - restricted_properties={("$browser", PropertyDefinition.Type.EVENT)}, + restricted_properties={("$browser", PropertyDefinition.Type.EVENT, None)}, ) assert plan.access.source.kind == PropertySourceKind.JSON @@ -756,7 +756,7 @@ def _print_select(self, select: str) -> str: class TestJSONExtractToMaterializedColumn(ClickhouseTestMixin, BaseTest): - def _print_select(self, select: str, restricted_properties: set[tuple[str, int]] | None = None): + def _print_select(self, select: str, restricted_properties: set[tuple[str, int, int | None]] | None = None): expr = parse_select(select) context = HogQLContext(team_id=self.team.pk, enable_select_queries=True) if restricted_properties is not None: @@ -951,8 +951,8 @@ def test_new_events_schema_jsonextract_respects_restricted_properties(self): "JSONHas(properties, 'secret') " "from events", restricted_properties={ - ("secret", PropertyDefinition.Type.EVENT), - ("email", PropertyDefinition.Type.EVENT), + ("secret", PropertyDefinition.Type.EVENT, None), + ("email", PropertyDefinition.Type.EVENT, None), }, ) diff --git a/posthog/hogql_queries/query_runner.py b/posthog/hogql_queries/query_runner.py index 481e25f47118..d67f610ca062 100644 --- a/posthog/hogql_queries/query_runner.py +++ b/posthog/hogql_queries/query_runner.py @@ -2549,16 +2549,18 @@ def read(name: str, fn: Callable[[], Any]) -> dict | str: "customer_analytics": read("customer_analytics_config", lambda: self.team.customer_analytics_config), } - def _get_property_access_restrictions(self) -> list[tuple[str, int]] | None: - """Returns a sorted list of restricted (property_name, type) pairs for the current user, or None if no restrictions. + def _get_property_access_restrictions(self) -> list[tuple[str, int, int | None]] | None: + """Returns sorted restricted property metadata for the current user, or None if unrestricted. The underlying ``get_restricted_properties_for_team`` memoizes per request, so rendering a dashboard with N insights issues one PropertyAccessControl lookup per (team, user) pair instead of N. """ - from products.access_control.backend.property_access_control import get_restricted_properties_for_team + from products.access_control.backend.property_access_control import ( + get_restricted_properties_with_group_type_index_for_team, + ) - restricted = get_restricted_properties_for_team(user=self.user, team=self.team) + restricted = get_restricted_properties_with_group_type_index_for_team(user=self.user, team=self.team) if not restricted: return None return sorted(restricted) diff --git a/products/access_control/backend/property_access_control.py b/products/access_control/backend/property_access_control.py index 92cec6ef86ec..334e431b11b8 100644 --- a/products/access_control/backend/property_access_control.py +++ b/products/access_control/backend/property_access_control.py @@ -36,7 +36,7 @@ # (`task_prerun` / `task_postrun`); callers running outside those boundaries (management # commands, ad-hoc scripts, code paths we haven't instrumented) simply pay the query cost # rather than risk stale authorization data. -_restriction_cache_var: ContextVar[dict[tuple[int, int | None], set[tuple[str, int]]] | None] = ContextVar( +_restriction_cache_var: ContextVar[dict[tuple[int, int | None], set[tuple[str, int, int | None]]] | None] = ContextVar( "property_access_restriction_cache", default=None ) @@ -97,6 +97,7 @@ def _invalidate_restriction_cache_on_change(**_kwargs: object) -> None: "get_non_writable_property_names", "get_property_access_level", "get_restricted_properties_for_team", + "get_restricted_properties_with_group_type_index_for_team", "get_restricted_property_names", "is_property_access_control_enabled", "strip_restricted_properties", @@ -276,14 +277,14 @@ def get_non_writable_property_names( return non_writable -def get_restricted_properties_for_team( +def get_restricted_properties_with_group_type_index_for_team( *, user: User | SyntheticUser | SharedLinkUser | None, team: Team | None = None, team_id: int | None = None, -) -> set[tuple[str, int]]: +) -> set[tuple[str, int, int | None]]: """ - Returns the set of (property_name, property_type) pairs that are restricted for the given user on the team. + Returns the set of (property_name, property_type, group_type_index) tuples that are restricted for the given user. This is designed to be called once per query to batch-load all restrictions rather than checking one property at a time. @@ -298,7 +299,7 @@ def get_restricted_properties_for_team( :param team_id: The team's id, for callers that don't have the instance loaded. Pass exactly one of ``team`` and ``team_id``. - :returns: A set of (property_name, property_definition_type) tuples that are restricted. + :returns: Restricted property metadata. ``group_type_index`` is only set for group properties. """ # Shared-link user and synthetic user have no membership to resolve restrictions against; # treat them as userless so only the default rules apply. @@ -321,7 +322,7 @@ def get_restricted_properties_for_team( # Short-circuit: no PROPERTY_ACCESS_CONTROL means no property access control rules exist if not is_property_access_control_enabled(team=team, team_id=team_id): - empty_no_feature: set[tuple[str, int]] = set() + empty_no_feature: set[tuple[str, int, int | None]] = set() if cache is not None: cache[cache_key] = empty_no_feature return empty_no_feature @@ -333,7 +334,7 @@ def get_restricted_properties_for_team( ) if not rules.exists(): - empty: set[tuple[str, int]] = set() + empty: set[tuple[str, int, int | None]] = set() if cache is not None: cache[cache_key] = empty return empty @@ -367,7 +368,7 @@ def get_restricted_properties_for_team( RoleMembership.objects.filter(organization_member=membership).values_list("role_id", flat=True) ) - restricted: set[tuple[str, int]] = set() + restricted: set[tuple[str, int, int | None]] = set() for _prop_def_id, prop_rules in rules_by_property.items(): prop_def = prop_rules[0].property_definition @@ -377,13 +378,24 @@ def get_restricted_properties_for_team( user_role_ids=user_role_ids, ) if prop_def is not None and not level.grants_access(): - restricted.add((prop_def.name, prop_def.type)) + restricted.add((prop_def.name, prop_def.type, prop_def.group_type_index)) if cache is not None: cache[cache_key] = restricted return restricted +def get_restricted_properties_for_team( + *, + user: User | SyntheticUser | SharedLinkUser | None, + team: Team | None = None, + team_id: int | None = None, +) -> set[tuple[str, int]]: + """Return restricted property names and types for callers that do not need group index scope.""" + restrictions = get_restricted_properties_with_group_type_index_for_team(user=user, team=team, team_id=team_id) + return {(name, property_type) for name, property_type, _group_type_index in restrictions} + + def _resolve_access_level( rules: list[PropertyAccessControl], *, From 618d33afa4ca4808dd6cf5b32f0874d29d410a52 Mon Sep 17 00:00:00 2001 From: Reece Jones Date: Fri, 21 Aug 2026 09:26:35 -0400 Subject: [PATCH 007/231] fix(hogql): use named property restrictions Generated-By: PostHog Desktop Task-Id: d95a2f15-384e-4099-8837-73058dddf994 --- posthog/hogql/ACCESS_CONTROL.md | 2 +- posthog/hogql/context.py | 7 +++--- posthog/hogql/property_access_types.py | 8 +++++++ posthog/hogql/restricted_properties.py | 8 +++---- .../test/test_events_predicate_pushdown.py | 5 ++++- .../transforms/test/test_property_types.py | 17 ++++++++------ posthog/hogql_queries/query_runner.py | 18 +++++++++++++-- .../backend/property_access_control.py | 22 +++++++++++++------ .../test_attribution_table_query_runner.py | 5 +++-- 9 files changed, 65 insertions(+), 27 deletions(-) create mode 100644 posthog/hogql/property_access_types.py diff --git a/posthog/hogql/ACCESS_CONTROL.md b/posthog/hogql/ACCESS_CONTROL.md index c4231f9312e3..deda19b1426b 100644 --- a/posthog/hogql/ACCESS_CONTROL.md +++ b/posthog/hogql/ACCESS_CONTROL.md @@ -195,7 +195,7 @@ Otherwise a denied user gets served an allowed user's cached rows. The cache key is derived from `get_cache_payload()`: -- `QueryRunner.get_cache_payload()` adds `restricted_properties` (sorted `(name, type, group_type_index)` tuples) when the user has property restrictions. +- `QueryRunner.get_cache_payload()` adds named property restriction records, including the group type index, when the user has property restrictions. - `AnalyticsQueryRunner.get_cache_payload()` adds `restricted_resources` (denied scopes) and `restricted_objects` (denied object IDs per scope) for levels 1 and 2. Two things keep cache hit rates high: diff --git a/posthog/hogql/context.py b/posthog/hogql/context.py index c4f01a8fdd0f..696e39c09b0b 100644 --- a/posthog/hogql/context.py +++ b/posthog/hogql/context.py @@ -3,6 +3,7 @@ from typing import TYPE_CHECKING, Any, Literal, Optional from posthog.hogql.constants import LimitContext +from posthog.hogql.property_access_types import RestrictedProperty from posthog.hogql.timings import HogQLTimings from posthog.clickhouse.workload import Workload @@ -30,7 +31,7 @@ def _default_modifiers() -> "HogQLQueryModifiers": return HogQLQueryModifiers() -@dataclass +@dataclass(frozen=False) class HogQLFieldAccess: input: list[str] type: Optional[Literal["event", "event.properties", "person", "person.properties"]] @@ -38,7 +39,7 @@ class HogQLFieldAccess: sql: str -@dataclass +@dataclass(frozen=False) class HogQLContext: """Context given to a HogQL expression printer""" @@ -133,7 +134,7 @@ class HogQLContext: # Property-level access control: (property_name, PropertyDefinition.Type, group_type_index) tuples # that the current user is denied access to. Populated before type resolution so that # FieldType.get_child() can raise QueryError for restricted properties. - restricted_properties: Optional[set[tuple[str, int, int | None]]] = None + restricted_properties: Optional[set[RestrictedProperty]] = None # Per-query cache of CTE synthetic tables, keyed by id() of the CTE's SelectQueryType. Value pins a # strong ref to the keyed type so its id can't be reused while cached; lookups verify identity. diff --git a/posthog/hogql/property_access_types.py b/posthog/hogql/property_access_types.py new file mode 100644 index 000000000000..8ae0f0cc224d --- /dev/null +++ b/posthog/hogql/property_access_types.py @@ -0,0 +1,8 @@ +from posthog.dataclasses import frozen + + +@frozen +class RestrictedProperty: + name: str + property_type: int + group_type_index: int | None = None diff --git a/posthog/hogql/restricted_properties.py b/posthog/hogql/restricted_properties.py index 2356ba75c0f3..7f172bde929a 100644 --- a/posthog/hogql/restricted_properties.py +++ b/posthog/hogql/restricted_properties.py @@ -76,12 +76,12 @@ def restricted_property_keys_for_table_type( return set() return { - name - for name, ptype, restricted_group_type_index in context.restricted_properties - if ptype == prop_def_type + restriction.name + for restriction in context.restricted_properties + if restriction.property_type == prop_def_type and ( prop_def_type != PropertyDefinition.Type.GROUP or group_type_index is None - or restricted_group_type_index == group_type_index + or restriction.group_type_index == group_type_index ) } diff --git a/posthog/hogql/transforms/test/test_events_predicate_pushdown.py b/posthog/hogql/transforms/test/test_events_predicate_pushdown.py index 94195d3208f4..85e2af1b6c11 100644 --- a/posthog/hogql/transforms/test/test_events_predicate_pushdown.py +++ b/posthog/hogql/transforms/test/test_events_predicate_pushdown.py @@ -37,6 +37,7 @@ from posthog.hogql.database.schema.util.where_clause_extractor import EventsPredicatePushdownExtractor from posthog.hogql.parser import parse_select from posthog.hogql.printer.utils import prepare_and_print_ast +from posthog.hogql.property_access_types import RestrictedProperty from posthog.hogql.query import execute_hogql_query from posthog.hogql.resolver import resolve_types from posthog.hogql.test.utils import pretty_print_in_tests @@ -146,7 +147,9 @@ def _print(push_down: bool) -> str: enable_select_queries=True, modifiers=HogQLQueryModifiers(pushDownPredicates=push_down), ) - context.restricted_properties = {("email", PropertyDefinition.Type.EVENT, None)} + context.restricted_properties = { + RestrictedProperty(name="email", property_type=PropertyDefinition.Type.EVENT) + } query, _ = prepare_and_print_ast(parse_select(select), context, "clickhouse") return pretty_print_in_tests(query, self.team.pk) diff --git a/posthog/hogql/transforms/test/test_property_types.py b/posthog/hogql/transforms/test/test_property_types.py index a47b02234d97..8537013b8bbd 100644 --- a/posthog/hogql/transforms/test/test_property_types.py +++ b/posthog/hogql/transforms/test/test_property_types.py @@ -36,6 +36,7 @@ from posthog.hogql.database.database import Database from posthog.hogql.parser import parse_select from posthog.hogql.printer import prepare_and_print_ast +from posthog.hogql.property_access_types import RestrictedProperty from posthog.hogql.property_planner import ( PropertyComparisonPlan, PropertyLiteralConversion, @@ -298,7 +299,7 @@ def _events_schema_snapshot(self): def _plan_where_comparison( self, select: str, - restricted_properties: set[tuple[str, int, int | None]] | None = None, + restricted_properties: set[RestrictedProperty] | None = None, ) -> PropertyComparisonPlan: context, resolved = self._resolve_select(select, restricted_properties=restricted_properties) comparison = cast(ast.CompareOperation, resolved.where) @@ -309,7 +310,7 @@ def _plan_where_comparison( def _resolve_select( self, select: str, - restricted_properties: set[tuple[str, int, int | None]] | None = None, + restricted_properties: set[RestrictedProperty] | None = None, ) -> tuple[HogQLContext, ast.SelectQuery]: """Resolve types and build the property-swapper registry without preparing further. @@ -329,7 +330,7 @@ def _resolve_select( def _prepare_select( self, select: str, - restricted_properties: set[tuple[str, int, int | None]] | None = None, + restricted_properties: set[RestrictedProperty] | None = None, ) -> tuple[HogQLContext, ast.SelectQuery]: expr = parse_select(select) context = HogQLContext(team_id=self.team.pk, team=self.team, enable_select_queries=True) @@ -503,7 +504,9 @@ def test_property_comparison_planner_respects_restricted_property_materializatio with materialized("events", "$browser", is_nullable=True, create_minmax_index=True): plan = self._plan_where_comparison( "select count() from events where properties.$browser < 'm'", - restricted_properties={("$browser", PropertyDefinition.Type.EVENT, None)}, + restricted_properties={ + RestrictedProperty(name="$browser", property_type=PropertyDefinition.Type.EVENT) + }, ) assert plan.access.source.kind == PropertySourceKind.JSON @@ -756,7 +759,7 @@ def _print_select(self, select: str) -> str: class TestJSONExtractToMaterializedColumn(ClickhouseTestMixin, BaseTest): - def _print_select(self, select: str, restricted_properties: set[tuple[str, int, int | None]] | None = None): + def _print_select(self, select: str, restricted_properties: set[RestrictedProperty] | None = None): expr = parse_select(select) context = HogQLContext(team_id=self.team.pk, enable_select_queries=True) if restricted_properties is not None: @@ -951,8 +954,8 @@ def test_new_events_schema_jsonextract_respects_restricted_properties(self): "JSONHas(properties, 'secret') " "from events", restricted_properties={ - ("secret", PropertyDefinition.Type.EVENT, None), - ("email", PropertyDefinition.Type.EVENT, None), + RestrictedProperty(name="secret", property_type=PropertyDefinition.Type.EVENT), + RestrictedProperty(name="email", property_type=PropertyDefinition.Type.EVENT), }, ) diff --git a/posthog/hogql_queries/query_runner.py b/posthog/hogql_queries/query_runner.py index d67f610ca062..2c17085b94f8 100644 --- a/posthog/hogql_queries/query_runner.py +++ b/posthog/hogql_queries/query_runner.py @@ -2549,7 +2549,7 @@ def read(name: str, fn: Callable[[], Any]) -> dict | str: "customer_analytics": read("customer_analytics_config", lambda: self.team.customer_analytics_config), } - def _get_property_access_restrictions(self) -> list[tuple[str, int, int | None]] | None: + def _get_property_access_restrictions(self) -> list[dict[str, str | int | None]] | None: """Returns sorted restricted property metadata for the current user, or None if unrestricted. The underlying ``get_restricted_properties_for_team`` memoizes per request, @@ -2563,7 +2563,21 @@ def _get_property_access_restrictions(self) -> list[tuple[str, int, int | None]] restricted = get_restricted_properties_with_group_type_index_for_team(user=self.user, team=self.team) if not restricted: return None - return sorted(restricted) + return [ + { + "name": restriction.name, + "property_type": restriction.property_type, + "group_type_index": restriction.group_type_index, + } + for restriction in sorted( + restricted, + key=lambda restriction: ( + restriction.name, + restriction.property_type, + restriction.group_type_index if restriction.group_type_index is not None else -1, + ), + ) + ] def get_cache_key(self) -> str: return generate_cache_key(self.team.pk, f"query_{bytes.decode(to_json(self.get_cache_payload()))}") diff --git a/products/access_control/backend/property_access_control.py b/products/access_control/backend/property_access_control.py index 334e431b11b8..ee3ff6b922a5 100644 --- a/products/access_control/backend/property_access_control.py +++ b/products/access_control/backend/property_access_control.py @@ -12,6 +12,8 @@ from celery.signals import task_postrun, task_prerun +from posthog.hogql.property_access_types import RestrictedProperty + from posthog.constants import AvailableFeature from posthog.models import OrganizationMembership from posthog.models.team import Team @@ -36,7 +38,7 @@ # (`task_prerun` / `task_postrun`); callers running outside those boundaries (management # commands, ad-hoc scripts, code paths we haven't instrumented) simply pay the query cost # rather than risk stale authorization data. -_restriction_cache_var: ContextVar[dict[tuple[int, int | None], set[tuple[str, int, int | None]]] | None] = ContextVar( +_restriction_cache_var: ContextVar[dict[tuple[int, int | None], set[RestrictedProperty]] | None] = ContextVar( "property_access_restriction_cache", default=None ) @@ -282,7 +284,7 @@ def get_restricted_properties_with_group_type_index_for_team( user: User | SyntheticUser | SharedLinkUser | None, team: Team | None = None, team_id: int | None = None, -) -> set[tuple[str, int, int | None]]: +) -> set[RestrictedProperty]: """ Returns the set of (property_name, property_type, group_type_index) tuples that are restricted for the given user. This is designed to be called once per query to batch-load all restrictions rather than checking one property @@ -322,7 +324,7 @@ def get_restricted_properties_with_group_type_index_for_team( # Short-circuit: no PROPERTY_ACCESS_CONTROL means no property access control rules exist if not is_property_access_control_enabled(team=team, team_id=team_id): - empty_no_feature: set[tuple[str, int, int | None]] = set() + empty_no_feature: set[RestrictedProperty] = set() if cache is not None: cache[cache_key] = empty_no_feature return empty_no_feature @@ -334,7 +336,7 @@ def get_restricted_properties_with_group_type_index_for_team( ) if not rules.exists(): - empty: set[tuple[str, int, int | None]] = set() + empty: set[RestrictedProperty] = set() if cache is not None: cache[cache_key] = empty return empty @@ -368,7 +370,7 @@ def get_restricted_properties_with_group_type_index_for_team( RoleMembership.objects.filter(organization_member=membership).values_list("role_id", flat=True) ) - restricted: set[tuple[str, int, int | None]] = set() + restricted: set[RestrictedProperty] = set() for _prop_def_id, prop_rules in rules_by_property.items(): prop_def = prop_rules[0].property_definition @@ -378,7 +380,13 @@ def get_restricted_properties_with_group_type_index_for_team( user_role_ids=user_role_ids, ) if prop_def is not None and not level.grants_access(): - restricted.add((prop_def.name, prop_def.type, prop_def.group_type_index)) + restricted.add( + RestrictedProperty( + name=prop_def.name, + property_type=prop_def.type, + group_type_index=prop_def.group_type_index, + ) + ) if cache is not None: cache[cache_key] = restricted @@ -393,7 +401,7 @@ def get_restricted_properties_for_team( ) -> set[tuple[str, int]]: """Return restricted property names and types for callers that do not need group index scope.""" restrictions = get_restricted_properties_with_group_type_index_for_team(user=user, team=team, team_id=team_id) - return {(name, property_type) for name, property_type, _group_type_index in restrictions} + return {(restriction.name, restriction.property_type) for restriction in restrictions} def _resolve_access_level( diff --git a/products/marketing_analytics/backend/hogql_queries/test_attribution_table_query_runner.py b/products/marketing_analytics/backend/hogql_queries/test_attribution_table_query_runner.py index c14352c15165..119c99d20040 100644 --- a/products/marketing_analytics/backend/hogql_queries/test_attribution_table_query_runner.py +++ b/products/marketing_analytics/backend/hogql_queries/test_attribution_table_query_runner.py @@ -18,6 +18,7 @@ from posthog.hogql import ast from posthog.hogql.printer import prepare_and_print_ast +from posthog.hogql.property_access_types import RestrictedProperty from posthog.hogql.visitor import TraversingVisitor from posthog.models import PropertyDefinition @@ -880,10 +881,10 @@ def test_property_denied_to_this_user_is_never_read(self, _name: str, restricted def restrictions_for(*, user, **_kwargs) -> set: if user is None or not restricted: return set() - return {("plan", PropertyDefinition.Type.EVENT)} + return {RestrictedProperty(name="plan", property_type=PropertyDefinition.Type.EVENT)} with patch( - "products.access_control.backend.property_access_control.get_restricted_properties_for_team", + "products.access_control.backend.property_access_control.get_restricted_properties_with_group_type_index_for_team", side_effect=restrictions_for, ): prepare_and_print_ast(runner.to_query(), context=context, dialect="clickhouse") From afff8c5f999550942c696110c07b6ff1b4605fea Mon Sep 17 00:00:00 2001 From: Daniel Visca Date: Fri, 21 Aug 2026 10:28:37 -0400 Subject: [PATCH 008/231] fix(mcp-analytics): stratified session sampling for intent clusters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The intent-clustering corpus was a uniform ~0.5% session sample dominated by the highest-volume tools, which erased low/mid-volume tools (logs/tracing/metrics) from clustering — they collapsed to single-session clusters with capture ranks in the 30s and discovery rates of 0–1.7% that read as "nobody uses this tool". This makes the corpus representative instead of proportional: - per-tool session buckets + stratified selection so each tool keeps a floor of sessions (MIN_SESSIONS_PER_TOOL) instead of being sampled out - a per-tool attributed-call cap (MAX_CALLS_PER_TOOL) so a dominant tool can't occupy the whole intent corpus - raise the corpus ceiling (MAX_CORPUS_SESSIONS 2000 → 6000) and TOP_N (500 → 1000) so floors are actually reachable - mark the snapshot as sampled + carry a sampling_warning in computed_with so the UI can stop presenting per-tool sample stats as population totals Falls back to the prior uniform sample when per-tool data is unavailable, so a capture gap never blocks a run. Generated-By: PostHog Desktop Task-Id: 440c7419-eefb-4213-ac28-5d3eb6f9c3ca --- .../intent_clustering/activities.py | 40 ++++- .../intent_clustering/constants.py | 5 +- .../backend/intent_clustering.py | 165 +++++++++++++++++- .../backend/tests/test_intent_clustering.py | 108 ++++++++++++ 4 files changed, 311 insertions(+), 7 deletions(-) diff --git a/posthog/temporal/mcp_analytics/intent_clustering/activities.py b/posthog/temporal/mcp_analytics/intent_clustering/activities.py index ed78cb141a15..18b138709a84 100644 --- a/posthog/temporal/mcp_analytics/intent_clustering/activities.py +++ b/posthog/temporal/mcp_analytics/intent_clustering/activities.py @@ -109,12 +109,48 @@ async def compute_intent_clusters_activity(inputs: IntentClusteringWorkflowInput snapshot = await _mark_computing(team, user) try: - session_ids = await database_sync_to_async(intent_clustering.sample_corpus_sessions)( - team, lookback_days=inputs.lookback_days + # Stratified sampling: fetch per-tool session buckets and choose + # corpus session ids so every tool keeps a floor, instead of a + # uniform sample that erases low/mid-volume tools (logs/tracing/ + # metrics). Falls back to the uniform sample when per-tool data + # is unavailable so a schema/capture gap never blocks a run. + intent_session_ids = await database_sync_to_async(intent_clustering.sample_corpus_sessions)( + team, lookback_days=inputs.lookback_days, max_sessions=intent_clustering.MAX_CORPUS_SESSIONS ) + try: + tools_by_session = await database_sync_to_async(intent_clustering.fetch_tools_by_session)( + team, lookback_days=inputs.lookback_days + ) + except Exception: + logger.warning( + "mcpa.intent_clustering.tool_buckets_unavailable_falling_back_to_uniform", + team_id=inputs.team_id, + ) + session_ids = intent_session_ids + else: + eligible = set(intent_session_ids) + stratified = intent_clustering.stratify_session_ids( + {tool: sids & eligible for tool, sids in tools_by_session.items()}, + min_sessions_per_tool=intent_clustering.MIN_SESSIONS_PER_TOOL, + max_total_sessions=intent_clustering.MAX_CORPUS_SESSIONS, + ) + # Keep the intent-bearing sessions the stratifier selected, in a + # stable order for the downstream IN-tuple queries. + session_ids = sorted(stratified) or intent_session_ids + call_rows = await database_sync_to_async(intent_clustering.fetch_session_calls)( team, session_ids, lookback_days=inputs.lookback_days ) + # Cap dominant tools so they can't occupy the whole intent corpus. + call_rows, per_tool_cap_report = intent_clustering.cap_per_tool_call_volume( + call_rows, max_calls_per_tool=intent_clustering.MAX_CALLS_PER_TOOL + ) + if per_tool_cap_report: + logger.info( + "mcpa.intent_clustering.capped_overrepresented_tools", + team_id=inputs.team_id, + per_tool=per_tool_cap_report, + ) records, calls_by_session, corpus_stats = intent_clustering.build_call_corpus( call_rows, top_n=inputs.top_n ) diff --git a/posthog/temporal/mcp_analytics/intent_clustering/constants.py b/posthog/temporal/mcp_analytics/intent_clustering/constants.py index 0c6830761102..9abcde1cffbd 100644 --- a/posthog/temporal/mcp_analytics/intent_clustering/constants.py +++ b/posthog/temporal/mcp_analytics/intent_clustering/constants.py @@ -21,7 +21,10 @@ # Sampling ------------------------------------------------------------------ DEFAULT_LOOKBACK_DAYS = 7 -DEFAULT_TOP_N_INTENTS = 500 +# Matches the pipeline default in products/mcp_analytics/backend/intent_clustering.py. +# Raised to 1000 with stratified sampling: per-tool floors + the per-tool call cap +# keep the extra intents spread across tools instead of long-tail exec/scout noise. +DEFAULT_TOP_N_INTENTS = 1000 MIN_INTENTS_FOR_CLUSTERING = 2 # Workflow + activity envelopes -------------------------------------------- diff --git a/products/mcp_analytics/backend/intent_clustering.py b/products/mcp_analytics/backend/intent_clustering.py index 1caba22daee2..471f66a22dcd 100644 --- a/products/mcp_analytics/backend/intent_clustering.py +++ b/products/mcp_analytics/backend/intent_clustering.py @@ -148,10 +148,23 @@ class WindowStats: # Intent corpus ----------------------------------------------------------- -# Bound on sessions sampled from ClickHouse for the corpus. Keeps the IN-tuple -# in the per-session queries below at a sane size; a larger sample mostly adds -# long-tail singleton intents past DEFAULT_TOP_N_INTENTS anyway. -MAX_CORPUS_SESSIONS = 2000 +# Bound on sessions sampled from ClickHouse for the corpus. Raised alongside +# stratified sampling: with per-tool floors, a larger pool means each tool can +# actually reach its floor, and the per-tool call cap (below) stops the extra +# sessions from belonging only to the dominant tool. The IN-tuple stays sane +# because per-session queries chunk the ids. +MAX_CORPUS_SESSIONS = 6000 + +# Each tool keeps at least this many sessions in the corpus, so a low/mid-volume +# tool (logs/tracing/metrics) survives sampling instead of being erased by the +# dominant exec/scout traffic. ~400 is the statistical floor for reading a +# discovery/capture rate to ±5%. +MIN_SESSIONS_PER_TOOL = 400 + +# No tool may contribute more than this many attributed calls to the corpus. +# Stops one dominant tool from occupying the entire intent space; the freed +# budget is what lets mid/low tools cluster into real themes rather than noise. +MAX_CALLS_PER_TOOL = 1500 # execute_hogql_query injects LIMIT 100 into any query without an explicit # LIMIT — far below what the per-session queries return at production scale @@ -415,6 +428,133 @@ def fetch_window_stats(team: Team, lookback_days: int = DEFAULT_LOOKBACK_DAYS) - ) +# Sessions that recorded at least one intent, with the set of effective tools +# each session used, bucketed per tool. This is the input to stratified +# sampling: choosing session ids per tool rather than uniformly across the +# window. Without the per-tool bucket, a uniform sample of a dominant-tool +# window silently erases every low/mid-volume tool (logs/tracing/metrics). +_SESSION_TOOLS_SQL = """ +SELECT + $session_id AS session_id, + left({tool_expr}, {max_tool_len}) AS tool +FROM events +WHERE event = {event} + AND timestamp >= now() - INTERVAL {lookback_days} DAY + AND $session_id != '' + AND notEmpty({tool_expr_where}) +GROUP BY session_id, tool +LIMIT {max_rows} +""" + + +def fetch_tools_by_session( + team: Team, + lookback_days: int = DEFAULT_LOOKBACK_DAYS, +) -> dict[str, set[str]]: + """Return ``{tool: {session_ids}}`` for the sampling window. + + Only sessions that recorded at least one intent are eligible for the + corpus, so this is filtered the same way as ``sample_corpus_sessions`` and + the caller intersects with the intent-bearing sample. Keyed by tool so the + stratifier can guarantee per-tool floors. + """ + query = parse_select( + _SESSION_TOOLS_SQL, + placeholders={ + "event": ast.Constant(value=MCP_TOOL_CALL_EVENT), + "tool_expr": parse_expr(EFFECTIVE_TOOL_SQL), + "tool_expr_where": parse_expr(EFFECTIVE_TOOL_SQL), + "lookback_days": ast.Constant(value=lookback_days), + "max_tool_len": ast.Constant(value=MAX_TOOL_NAME_LENGTH), + "max_rows": ast.Constant(value=MAX_QUERY_ROWS), + }, + ) + out: dict[str, set[str]] = defaultdict(set) + for row in _run_corpus_query(team, query): + session_id, tool = str(row[0] or ""), str(row[1] or "") + if session_id and tool: + out[tool].add(session_id) + return dict(out) + + +def stratify_session_ids( + tool_sessions: dict[str, set[str]], + min_sessions_per_tool: int, + max_total_sessions: int, +) -> set[str]: + """Choose corpus sessions so no tool is erased by the dominant tools. + + The prior uniform sample drew sessions proportionally to traffic, so in a + window where ``exec``/scout dominate, a low/mid-volume tool's handful of + sessions is statistically dropped and the tool becomes invisible to + clustering. This guarantees each tool keeps up to ``min_sessions_per_tool`` + sessions (its full set when smaller), then fills any remaining budget with + the highest-volume tools, capped at ``max_total_sessions``. + + Deterministic: per-tool session ids take the cityHash-style prefix of a + sorted order, so reruns re-hit the embedding cache. + """ + selected: set[str] = set() + # Tools ordered by ascending volume so scarce tools secure their floor before + # dominant tools consume the shared budget. + for tool in sorted(tool_sessions, key=lambda t: (len(tool_sessions[t]), t)): + sessions = sorted(tool_sessions[tool]) + floor = sessions[:min_sessions_per_tool] + selected.update(floor) + if len(selected) >= max_total_sessions: + break + + if len(selected) <= max_total_sessions: + return selected + + # Over budget: trim the largest tools' contribution back toward the floor, + # never below it, until the total fits. Deterministic about which ids drop. + over = len(selected) - max_total_sessions + for tool in sorted(tool_sessions, key=lambda t: (-len(tool_sessions[t]), t)): + if over <= 0: + break + contributed = sorted(tool_sessions[tool]) + droppable = [sid for sid in contributed if sid in selected][min_sessions_per_tool:] + for sid in droppable[:over]: + selected.discard(sid) + over -= 1 + return selected + + +def cap_per_tool_call_volume( + rows: list[tuple[str, str, str, bool]], + max_calls_per_tool: int, +) -> tuple[list[tuple[str, str, str, bool]], dict[str, dict[str, int]]]: + """Down-sample an over-represented tool's raw call rows before attribution. + + Row-level (pre-attribution) so intents and LOCF see the capped population. + Deterministic: keeps an even stride across the tool's rows so the surviving + calls still span the tool's whole session/intent range rather than a prefix. + Returns ``(kept_rows, per_tool_report)`` where each over-capped tool reports + how many calls were ``kept`` vs ``dropped``. + """ + tool_row_indexes: dict[str, list[int]] = defaultdict(list) + for idx, (_, tool, _, _) in enumerate(rows): + tool_row_indexes[tool].append(idx) + + keep_indexes: set[int] = set() + report: dict[str, dict[str, int]] = {} + for tool, indexes in tool_row_indexes.items(): + total = len(indexes) + if total <= max_calls_per_tool: + keep_indexes.update(indexes) + continue + # Even stride keeps breadth across the tool's calls. + stride = total / max_calls_per_tool + kept_positions = {int(i * stride) for i in range(max_calls_per_tool)} + kept = {indexes[pos] for pos in kept_positions} + keep_indexes.update(kept) + report[tool] = {"kept": len(kept), "dropped": total - len(kept)} + + kept_rows = [row for idx, row in enumerate(rows) if idx in keep_indexes] + return kept_rows, report + + def fetch_tool_descriptions( team: Team, tools: Collection[str], lookback_days: int = DEFAULT_LOOKBACK_DAYS ) -> dict[str, str]: @@ -1170,6 +1310,16 @@ def build_snapshot( "dropped_tools": dropped_tools, "dropped_overlap_pairs": dropped_pairs, "description_coverage_pct": _pct(described_tools, len(tools)) if tools else None, + # Representation honesty: these per-tool numbers come from a *balanced + # sample*, never the population. Downstream surfaces must warn before + # treating a tool's capture/discovery rate as its true traffic share. + "sampled": True, + "corpus_strategy": "stratified_by_tool", + "sampling_warning": ( + "Intent clusters are computed from a stratified sample of sessions " + "(per-tool floors, dominant tools capped). Per-tool capture and " + "discovery rates are sample statistics, not population totals." + ), } return { @@ -1222,5 +1372,12 @@ def empty_snapshot( "dropped_tools": 0, "dropped_overlap_pairs": 0, "description_coverage_pct": None, + "sampled": True, + "corpus_strategy": "stratified_by_tool", + "sampling_warning": ( + "Intent clusters are computed from a stratified sample of sessions " + "(per-tool floors, dominant tools capped). Per-tool capture and " + "discovery rates are sample statistics, not population totals." + ), }, } diff --git a/products/mcp_analytics/backend/tests/test_intent_clustering.py b/products/mcp_analytics/backend/tests/test_intent_clustering.py index 9c61e9899001..4e11f5094b4d 100644 --- a/products/mcp_analytics/backend/tests/test_intent_clustering.py +++ b/products/mcp_analytics/backend/tests/test_intent_clustering.py @@ -75,6 +75,10 @@ def _unit(vec: list[float]) -> np.ndarray: return arr / np.linalg.norm(arr) +def _snapshot_record(intent: str, tool: str, count: int) -> IntentRecord: + return IntentRecord(intent_text=intent, frequency=count, tool_counts={tool: count}) + + # cluster_embeddings ------------------------------------------------------ @@ -258,6 +262,20 @@ def test_medoid_is_used_as_cluster_label(self) -> None: assert snapshot["clusters"][0]["label"] == "center" + def test_meta_marks_snapshot_as_sampled_not_population(self) -> None: + # The page presents per-tool numbers as if they were the population; the + # snapshot must carry the sampling/balance metadata so the UI can warn. + records = [_snapshot_record("i1", "exec", 3), _snapshot_record("i2", "query-apm-spans", 1)] + labels = np.array([0, 1], dtype=np.int64) + embeddings = np.array([_unit([1.0, 0.0]), _unit([0.0, 1.0])], dtype=np.float32) + + snapshot = build_snapshot(records, labels, embeddings, calls_by_session={}) + + meta = snapshot["computed_with"] + assert meta["sampled"] is True + assert "corpus_strategy" in meta + assert "sampling_warning" in meta + def test_misaligned_inputs_raise(self) -> None: records = [IntentRecord(intent_text="a", frequency=1, tool_counts={"tool_a": 1})] with pytest.raises(AssertionError): @@ -480,6 +498,96 @@ def test_top_n_keeps_highest_call_count_intents_and_reports_kept_calls(self) -> assert stats.kept_calls == 5 +# stratified corpus --------------------------------------------------------- + + +class TestStratifySessionIds: + """The uniform 0.5% session sample is what erases low/mid-volume tools (the + APM logs/tracing/metrics complaint). Stratifying the *session ids* before + the corpus SQL guarantees every tool keeps a floor of sessions, so no tool + is silently dropped from clustering just because exec/scout dominate.""" + + def test_every_tool_keeps_a_floor_of_sessions(self) -> None: + from products.mcp_analytics.backend.intent_clustering import stratify_session_ids + + tool_sessions: dict[str, set[str]] = { + "exec": {f"exec-s{i}" for i in range(2000)}, + "query-apm-spans": {f"apm-s{i}" for i in range(30)}, + } + union = set().union(*tool_sessions.values()) + + selected = stratify_session_ids(tool_sessions, min_sessions_per_tool=400, max_total_sessions=2000) + + # The 30-session APM tool must survive even though a uniform sample of + # ~2000/2030 will statistically drop most of them. + assert {sid for sid in selected if sid.startswith("apm-s")} == set(tool_sessions["query-apm-spans"]) + assert selected.issubset(union) + + def test_total_respects_max_total_sessions(self) -> None: + from products.mcp_analytics.backend.intent_clustering import stratify_session_ids + + tool_sessions = { + f"tool_{t}": {f"tool_{t}-s{i}" for i in range(600)} for t in range(10) + } + + selected = stratify_session_ids(tool_sessions, min_sessions_per_tool=400, max_total_sessions=2000) + + assert len(selected) <= 2000 + + def test_selection_is_deterministic(self) -> None: + from products.mcp_analytics.backend.intent_clustering import stratify_session_ids + + tool_sessions = {f"tool_{t}": {f"tool_{t}-s{i}" for i in range(50)} for t in range(5)} + + first = stratify_session_ids(tool_sessions, min_sessions_per_tool=20, max_total_sessions=80) + second = stratify_session_ids(tool_sessions, min_sessions_per_tool=20, max_total_sessions=80) + + assert first == second + + def test_scarce_tool_is_kept_entirely(self) -> None: + from products.mcp_analytics.backend.intent_clustering import stratify_session_ids + + tool_sessions = {"query-metrics": {f"m-s{i}" for i in range(4)}, "exec": {f"e-s{i}" for i in range(3000)}} + + selected = stratify_session_ids(tool_sessions, min_sessions_per_tool=400, max_total_sessions=2000) + + assert {sid for sid in selected if sid.startswith("m-s")} == tool_sessions["query-metrics"] + + +class TestCapPerToolCallVolume: + """After sampling, a single high-volume tool (exec) must not be allowed to + occupy the whole intent corpus; cap its attributed calls so mid/low tools + retain enough signal to cluster.""" + + def test_caps_overrepresented_tool_and_reports_it(self) -> None: + from products.mcp_analytics.backend.intent_clustering import cap_per_tool_call_volume + + rows = ( + [("s1", "exec", "operate", False)] * 100 + + [("s2", "query-logs", "tail logs", False)] * 2 + ) + + capped_rows, per_tool = cap_per_tool_call_volume(rows, max_calls_per_tool=10) + + per_tool_count = {} + for _, tool, _, _ in capped_rows: + per_tool_count[tool] = per_tool_count.get(tool, 0) + 1 + assert per_tool_count["exec"] <= 10 + # low-volume tool is untouched + assert per_tool_count["query-logs"] == 2 + assert per_tool["exec"]["dropped"] >= 90 + + def test_deterministic_about_which_calls_survive(self) -> None: + from products.mcp_analytics.backend.intent_clustering import cap_per_tool_call_volume + + rows = [("s1", "exec", f"op {i}", False) for i in range(50)] + + first, _ = cap_per_tool_call_volume(rows, max_calls_per_tool=10) + second, _ = cap_per_tool_call_volume(rows, max_calls_per_tool=10) + + assert first == second + + # compute_cluster_flows ---------------------------------------------------- From 221147ae84f4672f20a98bc39a5c00a68bb9ef1a Mon Sep 17 00:00:00 2001 From: Daniel Visca Date: Fri, 21 Aug 2026 10:45:13 -0400 Subject: [PATCH 009/231] chore(mcp-analytics): annotate per_tool_count for mypy var-annotated Static typing failed on the new cap test: an inferred empty dict needs an explicit annotation. Generated-By: PostHog Desktop Task-Id: 440c7419-eefb-4213-ac28-5d3eb6f9c3ca --- products/mcp_analytics/backend/tests/test_intent_clustering.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/products/mcp_analytics/backend/tests/test_intent_clustering.py b/products/mcp_analytics/backend/tests/test_intent_clustering.py index 4e11f5094b4d..58b997c76cce 100644 --- a/products/mcp_analytics/backend/tests/test_intent_clustering.py +++ b/products/mcp_analytics/backend/tests/test_intent_clustering.py @@ -569,7 +569,7 @@ def test_caps_overrepresented_tool_and_reports_it(self) -> None: capped_rows, per_tool = cap_per_tool_call_volume(rows, max_calls_per_tool=10) - per_tool_count = {} + per_tool_count: dict[str, int] = {} for _, tool, _, _ in capped_rows: per_tool_count[tool] = per_tool_count.get(tool, 0) + 1 assert per_tool_count["exec"] <= 10 From bdf6df39b3fb76abe8a8667e9f7b2cacf2916adc Mon Sep 17 00:00:00 2001 From: Daniel Visca Date: Fri, 21 Aug 2026 10:56:59 -0400 Subject: [PATCH 010/231] chore(mcp-analytics): format test_intent_clustering with repo's ruff 0.15.20 CI's ruff format --check (0.15.20) rejected two hand-wrapped literals my local 0.14.11 accepted. Reformatted with the pinned version. Generated-By: PostHog Desktop Task-Id: 440c7419-eefb-4213-ac28-5d3eb6f9c3ca --- .../backend/tests/test_intent_clustering.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/products/mcp_analytics/backend/tests/test_intent_clustering.py b/products/mcp_analytics/backend/tests/test_intent_clustering.py index 58b997c76cce..2ee801a871a2 100644 --- a/products/mcp_analytics/backend/tests/test_intent_clustering.py +++ b/products/mcp_analytics/backend/tests/test_intent_clustering.py @@ -526,9 +526,7 @@ def test_every_tool_keeps_a_floor_of_sessions(self) -> None: def test_total_respects_max_total_sessions(self) -> None: from products.mcp_analytics.backend.intent_clustering import stratify_session_ids - tool_sessions = { - f"tool_{t}": {f"tool_{t}-s{i}" for i in range(600)} for t in range(10) - } + tool_sessions = {f"tool_{t}": {f"tool_{t}-s{i}" for i in range(600)} for t in range(10)} selected = stratify_session_ids(tool_sessions, min_sessions_per_tool=400, max_total_sessions=2000) @@ -562,10 +560,7 @@ class TestCapPerToolCallVolume: def test_caps_overrepresented_tool_and_reports_it(self) -> None: from products.mcp_analytics.backend.intent_clustering import cap_per_tool_call_volume - rows = ( - [("s1", "exec", "operate", False)] * 100 - + [("s2", "query-logs", "tail logs", False)] * 2 - ) + rows = [("s1", "exec", "operate", False)] * 100 + [("s2", "query-logs", "tail logs", False)] * 2 capped_rows, per_tool = cap_per_tool_call_volume(rows, max_calls_per_tool=10) From 15a4dde7b39b96150c62b1de5f3b447c719993be Mon Sep 17 00:00:00 2001 From: Daniel Visca Date: Fri, 21 Aug 2026 11:08:44 -0400 Subject: [PATCH 011/231] refactor(mcp-analytics): return ToolCallCapResult dataclass from cap_per_tool_call_volume semgrep tuple-return-prefer-dataclass blocked the 2-tuple return of the new cap helper. Return a @frozen ToolCallCapResult (kept_rows + per_tool_report) instead and update the activity + tests to the named fields. Generated-By: PostHog Desktop Task-Id: 440c7419-eefb-4213-ac28-5d3eb6f9c3ca --- .../intent_clustering/activities.py | 7 ++++--- .../mcp_analytics/backend/intent_clustering.py | 17 +++++++++++++---- .../backend/tests/test_intent_clustering.py | 10 +++++----- 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/posthog/temporal/mcp_analytics/intent_clustering/activities.py b/posthog/temporal/mcp_analytics/intent_clustering/activities.py index 18b138709a84..d5e41dac21e3 100644 --- a/posthog/temporal/mcp_analytics/intent_clustering/activities.py +++ b/posthog/temporal/mcp_analytics/intent_clustering/activities.py @@ -142,14 +142,15 @@ async def compute_intent_clusters_activity(inputs: IntentClusteringWorkflowInput team, session_ids, lookback_days=inputs.lookback_days ) # Cap dominant tools so they can't occupy the whole intent corpus. - call_rows, per_tool_cap_report = intent_clustering.cap_per_tool_call_volume( + cap_result = intent_clustering.cap_per_tool_call_volume( call_rows, max_calls_per_tool=intent_clustering.MAX_CALLS_PER_TOOL ) - if per_tool_cap_report: + call_rows = cap_result.kept_rows + if cap_result.per_tool_report: logger.info( "mcpa.intent_clustering.capped_overrepresented_tools", team_id=inputs.team_id, - per_tool=per_tool_cap_report, + per_tool=cap_result.per_tool_report, ) records, calls_by_session, corpus_stats = intent_clustering.build_call_corpus( call_rows, top_n=inputs.top_n diff --git a/products/mcp_analytics/backend/intent_clustering.py b/products/mcp_analytics/backend/intent_clustering.py index 471f66a22dcd..088d74c358a1 100644 --- a/products/mcp_analytics/backend/intent_clustering.py +++ b/products/mcp_analytics/backend/intent_clustering.py @@ -44,6 +44,7 @@ from posthog.api.embedding_worker import EmbeddingResponse, async_generate_embedding from posthog.clickhouse.query_tagging import Feature, Product, tags_context +from posthog.dataclasses import frozen from posthog.models.team.team import Team from posthog.sync import database_sync_to_async @@ -521,17 +522,25 @@ def stratify_session_ids( return selected +@frozen +class ToolCallCapResult: + """Outcome of ``cap_per_tool_call_volume``: the kept rows plus, for each + over-capped tool, how many of its calls were kept vs dropped.""" + + kept_rows: list[tuple[str, str, str, bool]] + per_tool_report: dict[str, dict[str, int]] + + def cap_per_tool_call_volume( rows: list[tuple[str, str, str, bool]], max_calls_per_tool: int, -) -> tuple[list[tuple[str, str, str, bool]], dict[str, dict[str, int]]]: +) -> ToolCallCapResult: """Down-sample an over-represented tool's raw call rows before attribution. Row-level (pre-attribution) so intents and LOCF see the capped population. Deterministic: keeps an even stride across the tool's rows so the surviving calls still span the tool's whole session/intent range rather than a prefix. - Returns ``(kept_rows, per_tool_report)`` where each over-capped tool reports - how many calls were ``kept`` vs ``dropped``. + Each over-capped tool reports how many calls were ``kept`` vs ``dropped``. """ tool_row_indexes: dict[str, list[int]] = defaultdict(list) for idx, (_, tool, _, _) in enumerate(rows): @@ -552,7 +561,7 @@ def cap_per_tool_call_volume( report[tool] = {"kept": len(kept), "dropped": total - len(kept)} kept_rows = [row for idx, row in enumerate(rows) if idx in keep_indexes] - return kept_rows, report + return ToolCallCapResult(kept_rows=kept_rows, per_tool_report=report) def fetch_tool_descriptions( diff --git a/products/mcp_analytics/backend/tests/test_intent_clustering.py b/products/mcp_analytics/backend/tests/test_intent_clustering.py index 2ee801a871a2..19f6f7dd1941 100644 --- a/products/mcp_analytics/backend/tests/test_intent_clustering.py +++ b/products/mcp_analytics/backend/tests/test_intent_clustering.py @@ -562,23 +562,23 @@ def test_caps_overrepresented_tool_and_reports_it(self) -> None: rows = [("s1", "exec", "operate", False)] * 100 + [("s2", "query-logs", "tail logs", False)] * 2 - capped_rows, per_tool = cap_per_tool_call_volume(rows, max_calls_per_tool=10) + result = cap_per_tool_call_volume(rows, max_calls_per_tool=10) per_tool_count: dict[str, int] = {} - for _, tool, _, _ in capped_rows: + for _, tool, _, _ in result.kept_rows: per_tool_count[tool] = per_tool_count.get(tool, 0) + 1 assert per_tool_count["exec"] <= 10 # low-volume tool is untouched assert per_tool_count["query-logs"] == 2 - assert per_tool["exec"]["dropped"] >= 90 + assert result.per_tool_report["exec"]["dropped"] >= 90 def test_deterministic_about_which_calls_survive(self) -> None: from products.mcp_analytics.backend.intent_clustering import cap_per_tool_call_volume rows = [("s1", "exec", f"op {i}", False) for i in range(50)] - first, _ = cap_per_tool_call_volume(rows, max_calls_per_tool=10) - second, _ = cap_per_tool_call_volume(rows, max_calls_per_tool=10) + first = cap_per_tool_call_volume(rows, max_calls_per_tool=10) + second = cap_per_tool_call_volume(rows, max_calls_per_tool=10) assert first == second From 9aef4624c78af94e8958b66a77f5a7cede119bd1 Mon Sep 17 00:00:00 2001 From: Daniel Visca Date: Fri, 21 Aug 2026 11:27:11 -0400 Subject: [PATCH 012/231] fix(mcp-analytics): per-tool candidate sampling + budget-constrained stratifier Address review feedback on the stratified sampler and its ClickHouse query: - Drop the uniform-hash pre-sample that gated stratification (greptile P1): the per-tool bucket query now draws directly from intent-bearing sessions, so a low-volume tool outside the top hash-ordered sessions is no longer removed before its floor can recover it. - Rewrite the sampler to be budget-constrained (greptile P1, graphite): floors are apportioned across tools (min of requested floor and an even budget share) so later/equal-volume tools are never skipped and the result always fits max_total_sessions; remainder goes to the highest-volume tools. - Bound sender-controlled grouping in _SESSION_TOOLS_SQL (veria): cap the candidate-session pool and the distinct tools per session before the GROUP BY materializes, so many unique tool names can't fan out ClickHouse aggregation. Adds regression tests covering the over-budget trim, late-tool skipping, and shared-session overshoot. Generated-By: PostHog Desktop Task-Id: 440c7419-eefb-4213-ac28-5d3eb6f9c3ca --- .../intent_clustering/activities.py | 35 +++--- .../backend/intent_clustering.py | 110 ++++++++++++------ .../backend/tests/test_intent_clustering.py | 42 +++++++ 3 files changed, 135 insertions(+), 52 deletions(-) diff --git a/posthog/temporal/mcp_analytics/intent_clustering/activities.py b/posthog/temporal/mcp_analytics/intent_clustering/activities.py index d5e41dac21e3..1a9d12c5d79a 100644 --- a/posthog/temporal/mcp_analytics/intent_clustering/activities.py +++ b/posthog/temporal/mcp_analytics/intent_clustering/activities.py @@ -109,34 +109,37 @@ async def compute_intent_clusters_activity(inputs: IntentClusteringWorkflowInput snapshot = await _mark_computing(team, user) try: - # Stratified sampling: fetch per-tool session buckets and choose - # corpus session ids so every tool keeps a floor, instead of a - # uniform sample that erases low/mid-volume tools (logs/tracing/ - # metrics). Falls back to the uniform sample when per-tool data - # is unavailable so a schema/capture gap never blocks a run. - intent_session_ids = await database_sync_to_async(intent_clustering.sample_corpus_sessions)( - team, lookback_days=inputs.lookback_days, max_sessions=intent_clustering.MAX_CORPUS_SESSIONS - ) + # Stratified sampling: bucket intent-bearing sessions per tool + # and choose corpus ids so every tool keeps a floor, instead of + # a uniform sample that erases low/mid-volume tools (logs/ + # tracing/metrics). Falls back to the uniform hash sample only + # when the per-tool buckets can't be fetched, so a capture or + # schema gap never blocks a run. try: tools_by_session = await database_sync_to_async(intent_clustering.fetch_tools_by_session)( - team, lookback_days=inputs.lookback_days + team, + lookback_days=inputs.lookback_days, + max_candidate_sessions=intent_clustering.MAX_CORPUS_SESSIONS, ) except Exception: logger.warning( "mcpa.intent_clustering.tool_buckets_unavailable_falling_back_to_uniform", team_id=inputs.team_id, ) - session_ids = intent_session_ids - else: - eligible = set(intent_session_ids) + tools_by_session = None + + if tools_by_session: stratified = intent_clustering.stratify_session_ids( - {tool: sids & eligible for tool, sids in tools_by_session.items()}, + tools_by_session, min_sessions_per_tool=intent_clustering.MIN_SESSIONS_PER_TOOL, max_total_sessions=intent_clustering.MAX_CORPUS_SESSIONS, ) - # Keep the intent-bearing sessions the stratifier selected, in a - # stable order for the downstream IN-tuple queries. - session_ids = sorted(stratified) or intent_session_ids + session_ids = sorted(stratified) + + if not tools_by_session or not session_ids: + session_ids = await database_sync_to_async(intent_clustering.sample_corpus_sessions)( + team, lookback_days=inputs.lookback_days, max_sessions=intent_clustering.MAX_CORPUS_SESSIONS + ) call_rows = await database_sync_to_async(intent_clustering.fetch_session_calls)( team, session_ids, lookback_days=inputs.lookback_days diff --git a/products/mcp_analytics/backend/intent_clustering.py b/products/mcp_analytics/backend/intent_clustering.py index 088d74c358a1..105c87b26569 100644 --- a/products/mcp_analytics/backend/intent_clustering.py +++ b/products/mcp_analytics/backend/intent_clustering.py @@ -167,6 +167,12 @@ class WindowStats: # budget is what lets mid/low tools cluster into real themes rather than noise. MAX_CALLS_PER_TOOL = 1500 +# Sender-controlled tool names only ever expand the ``_SESSION_TOOLS_SQL`` +# grouping. One session honestly uses a handful of distinct tools, so bound how +# many distinct tools a single session can contribute to the per-tool buckets — +# an attacker emitting thousands of unique names can't fan out the aggregation. +MAX_DISTINCT_TOOLS_PER_SESSION_BUCKET = 500 + # execute_hogql_query injects LIMIT 100 into any query without an explicit # LIMIT — far below what the per-session queries return at production scale # (one-plus rows per corpus session). Cap explicitly at the HogQL per-query @@ -429,11 +435,18 @@ def fetch_window_stats(team: Team, lookback_days: int = DEFAULT_LOOKBACK_DAYS) - ) -# Sessions that recorded at least one intent, with the set of effective tools -# each session used, bucketed per tool. This is the input to stratified -# sampling: choosing session ids per tool rather than uniformly across the -# window. Without the per-tool bucket, a uniform sample of a dominant-tool -# window silently erases every low/mid-volume tool (logs/tracing/metrics). +# Intent-bearing sessions bucketed by effective tool. Both dimensions are +# sender-controlled, so each is bounded *before* the GROUP BY materializes +# aggregation state: the inner queries cap the number of intent-bearing +# sessions (cityHash sample, same scheme as ``sample_corpus_sessions``) and the +# per-session distinct tool count, so an attacker submitting many unique tool +# names can't fan out the outer group. Only intent-bearing sessions enter the +# buckets, which is what lets stratified sampling reach a low/mid-volume tool +# directly instead of through an independent uniform sample. +# +# ``cityHash64`` here and in ``sample_corpus_sessions`` is a fast pseudo-random +# ordering, not a security boundary — the memory/CPU protection comes from the +# numeric caps, not the hash. _SESSION_TOOLS_SQL = """ SELECT $session_id AS session_id, @@ -441,9 +454,20 @@ def fetch_window_stats(team: Team, lookback_days: int = DEFAULT_LOOKBACK_DAYS) - FROM events WHERE event = {event} AND timestamp >= now() - INTERVAL {lookback_days} DAY - AND $session_id != '' + AND $session_id IN ( + SELECT $session_id + FROM events + WHERE event = {event} + AND timestamp >= now() - INTERVAL {lookback_days} DAY + AND $session_id != '' + AND coalesce(toString(properties.$mcp_intent), '') != '' + GROUP BY $session_id + ORDER BY cityHash64($session_id) + LIMIT {max_candidate_sessions} + ) AND notEmpty({tool_expr_where}) GROUP BY session_id, tool +LIMIT {max_distinct_tools} BY session_id LIMIT {max_rows} """ @@ -451,13 +475,14 @@ def fetch_window_stats(team: Team, lookback_days: int = DEFAULT_LOOKBACK_DAYS) - def fetch_tools_by_session( team: Team, lookback_days: int = DEFAULT_LOOKBACK_DAYS, + max_candidate_sessions: int = MAX_CORPUS_SESSIONS, ) -> dict[str, set[str]]: - """Return ``{tool: {session_ids}}`` for the sampling window. + """Return ``{tool: {intent-bearing session_ids}}`` for the sampling window. - Only sessions that recorded at least one intent are eligible for the - corpus, so this is filtered the same way as ``sample_corpus_sessions`` and - the caller intersects with the intent-bearing sample. Keyed by tool so the - stratifier can guarantee per-tool floors. + The candidate pool is the deterministic cityHash sample of intent-bearing + sessions (``max_candidate_sessions``), so every tool's bucket is drawn from + that pool directly; the stratifier then guarantees per-tool floors without + any independent uniform sample gating it. """ query = parse_select( _SESSION_TOOLS_SQL, @@ -467,6 +492,8 @@ def fetch_tools_by_session( "tool_expr_where": parse_expr(EFFECTIVE_TOOL_SQL), "lookback_days": ast.Constant(value=lookback_days), "max_tool_len": ast.Constant(value=MAX_TOOL_NAME_LENGTH), + "max_candidate_sessions": ast.Constant(value=max_candidate_sessions), + "max_distinct_tools": ast.Constant(value=MAX_DISTINCT_TOOLS_PER_SESSION_BUCKET), "max_rows": ast.Constant(value=MAX_QUERY_ROWS), }, ) @@ -492,33 +519,44 @@ def stratify_session_ids( sessions (its full set when smaller), then fills any remaining budget with the highest-volume tools, capped at ``max_total_sessions``. - Deterministic: per-tool session ids take the cityHash-style prefix of a - sorted order, so reruns re-hit the embedding cache. + Deterministic: per-tool session ids take the sorted-prefix, so reruns + re-hit the same ids and the embedding cache. + + Every tool is visited: the budget is apportioned across tools rather than + consumed by the first ones, so a later (or alphabetically later equal-volume) + tool is never skipped, and the result is always within ``max_total_sessions`` + regardless of how the floors interact with the budget. """ + tools = sorted(tool_sessions) + if not tools or max_total_sessions <= 0: + return set() + + # The budget cannot always give every tool its full floor (many tools x + # floor swamps max_total_sessions), so the effective per-tool floor is + # min(the request, an even share of the budget). Even-share is the only + # allocation that never starves a tool while always fitting the budget. + budget_floor = max(1, max_total_sessions // len(tools)) + floor_size = max(1, min(min_sessions_per_tool, budget_floor)) + selected: set[str] = set() - # Tools ordered by ascending volume so scarce tools secure their floor before - # dominant tools consume the shared budget. - for tool in sorted(tool_sessions, key=lambda t: (len(tool_sessions[t]), t)): - sessions = sorted(tool_sessions[tool]) - floor = sessions[:min_sessions_per_tool] - selected.update(floor) - if len(selected) >= max_total_sessions: - break - - if len(selected) <= max_total_sessions: - return selected - - # Over budget: trim the largest tools' contribution back toward the floor, - # never below it, until the total fits. Deterministic about which ids drop. - over = len(selected) - max_total_sessions - for tool in sorted(tool_sessions, key=lambda t: (-len(tool_sessions[t]), t)): - if over <= 0: - break - contributed = sorted(tool_sessions[tool]) - droppable = [sid for sid in contributed if sid in selected][min_sessions_per_tool:] - for sid in droppable[:over]: - selected.discard(sid) - over -= 1 + # Ascending volume secures scarce tools' floors before dominant tools add + # their own (shared ids are de-duped through ``selected``). + for tool in sorted(tools, key=lambda t: (len(tool_sessions[t]), t)): + selected.update(sorted(tool_sessions[tool])[:floor_size]) + + # Spend the remainder on the highest-volume tools, which hold the bulk of + # the window's calls, keeping the whole result within budget. + room = max_total_sessions - len(selected) + if room > 0: + for tool in sorted(tools, key=lambda t: (-len(tool_sessions[t]), t)): + if room <= 0: + break + for sid in sorted(tool_sessions[tool]): + if room <= 0: + break + if sid not in selected: + selected.add(sid) + room -= 1 return selected diff --git a/products/mcp_analytics/backend/tests/test_intent_clustering.py b/products/mcp_analytics/backend/tests/test_intent_clustering.py index 19f6f7dd1941..6f8f497847c0 100644 --- a/products/mcp_analytics/backend/tests/test_intent_clustering.py +++ b/products/mcp_analytics/backend/tests/test_intent_clustering.py @@ -551,6 +551,48 @@ def test_scarce_tool_is_kept_entirely(self) -> None: assert {sid for sid in selected if sid.startswith("m-s")} == tool_sessions["query-metrics"] + def test_budget_apportions_floors_and_stays_within_total(self) -> None: + # Regression for the over-budget trimming bug: 3 x 250 = 750 must come + # back under a 700 budget, and no tool may be skipped. + from products.mcp_analytics.backend.intent_clustering import stratify_session_ids + + tool_sessions = {f"tool_{t}": {f"tool_{t}-s{i}" for i in range(300)} for t in range(3)} + + selected = stratify_session_ids(tool_sessions, min_sessions_per_tool=250, max_total_sessions=700) + + assert len(selected) <= 700 + for t in range(3): + assert any(sid.startswith(f"tool_{t}-s") for sid in selected), f"tool_{t} was skipped" + + def test_late_tool_is_not_skipped_when_floors_fill_the_budget(self) -> None: + # Regression for the break that skipped every tool once earlier floors + # hit the budget: a late, higher-volume tool must still get a floor. + from products.mcp_analytics.backend.intent_clustering import stratify_session_ids + + tool_sessions: dict[str, set[str]] = { + "alpha": {f"a-s{i}" for i in range(500)}, + "beta": {f"b-s{i}" for i in range(500)}, + "zeta": {f"z-s{i}" for i in range(1000)}, + } + + selected = stratify_session_ids(tool_sessions, min_sessions_per_tool=400, max_total_sessions=900) + + assert any(sid.startswith("z-s") for sid in selected) + assert len(selected) <= 900 + + def test_never_exceeds_budget_when_many_tools_share_many_sessions(self) -> None: + # 10 tools each with 400 shared sessions: sum of floors (4000) far + # exceeds the budget even after de-dup (shared ids), so any overshoot + # must be trimmed. + from products.mcp_analytics.backend.intent_clustering import stratify_session_ids + + shared = {f"s{i}" for i in range(400)} + tool_sessions = {f"tool_{t}": set(shared) for t in range(10)} + + selected = stratify_session_ids(tool_sessions, min_sessions_per_tool=400, max_total_sessions=1500) + + assert len(selected) <= 1500 + class TestCapPerToolCallVolume: """After sampling, a single high-volume tool (exec) must not be allowed to From b2f0a7e843fa2fd5be9224c2d9cc6e4ec66a5924 Mon Sep 17 00:00:00 2001 From: Daniel Visca Date: Fri, 21 Aug 2026 11:52:07 -0400 Subject: [PATCH 013/231] test(mcp-analytics): update DEFAULT_TOP_N_INTENTS expectations; add fetch_tools_by_session e2e test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Django test runs failed on the stale parse-inputs defaults (500 → 1000) in test_workflow.py and test_coordinator.py after TOP_N was raised. Also adds ClickHouse-backed coverage for the new fetch_tools_by_session bucket query, so the rewritten _SESSION_TOOLS_SQL (subquery + BY-session cap) is exercised end-to-end, not just compiled. Generated-By: PostHog Desktop Task-Id: 440c7419-eefb-4213-ac28-5d3eb6f9c3ca --- .../intent_clustering/tests/test_coordinator.py | 2 +- .../intent_clustering/tests/test_workflow.py | 4 ++-- .../backend/tests/test_intent_clustering.py | 15 +++++++++++++++ 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/posthog/temporal/mcp_analytics/intent_clustering/tests/test_coordinator.py b/posthog/temporal/mcp_analytics/intent_clustering/tests/test_coordinator.py index fdabbc6fb945..152bf69979e2 100644 --- a/posthog/temporal/mcp_analytics/intent_clustering/tests/test_coordinator.py +++ b/posthog/temporal/mcp_analytics/intent_clustering/tests/test_coordinator.py @@ -126,7 +126,7 @@ class TestCoordinatorParseInputs: @pytest.mark.parametrize( "args, expected_lookback, expected_top_n, expected_max_concurrent", [ - ([], 7, 500, 4), + ([], 7, 1000, 4), (["14", "200", "2"], 14, 200, 2), ], ) diff --git a/posthog/temporal/mcp_analytics/intent_clustering/tests/test_workflow.py b/posthog/temporal/mcp_analytics/intent_clustering/tests/test_workflow.py index e23465e0faf0..90971ec53e79 100644 --- a/posthog/temporal/mcp_analytics/intent_clustering/tests/test_workflow.py +++ b/posthog/temporal/mcp_analytics/intent_clustering/tests/test_workflow.py @@ -87,11 +87,11 @@ class TestParseInputs: "raw_payload, expected_team_id, expected_lookback_days, expected_top_n, expected_user_id", [ # Empty input falls back to dataclass defaults. - ([], 0, 7, 500, None), + ([], 0, 7, 1000, None), # Full JSON payload overrides every field. (['{"team_id": 99, "lookback_days": 3, "top_n": 50, "user_id": 5}'], 99, 3, 50, 5), # Partial payload preserves dataclass defaults for omitted fields. - (['{"team_id": 99}'], 99, 7, 500, None), + (['{"team_id": 99}'], 99, 7, 1000, None), ], ) def test_parse_inputs_cases( diff --git a/products/mcp_analytics/backend/tests/test_intent_clustering.py b/products/mcp_analytics/backend/tests/test_intent_clustering.py index 6f8f497847c0..20e266142614 100644 --- a/products/mcp_analytics/backend/tests/test_intent_clustering.py +++ b/products/mcp_analytics/backend/tests/test_intent_clustering.py @@ -1144,6 +1144,21 @@ def test_advertised_catalog_is_bounded_per_list_and_per_session(self) -> None: assert len(advertised["session-chatty"]) <= MAX_ADVERTISED_LIST_EVENTS_PER_SESSION assert len(advertised["session-union"]) == MAX_ADVERTISED_TOOLS_PER_SESSION + def test_tools_by_session_buckets_intent_bearing_sessions_by_effective_tool(self) -> None: + # session-a carries an intent; session-quiet does not, so its tools must + # not buckify it. The exec wrapper resolves to the inner effective tool. + self._seed_tool_call("session-a", "query-logs", intent="tail error logs") + self._seed_tool_call("session-a", "exec", intent="tail error logs", exec_tool_name="query-apm-spans") + self._seed_tool_call("session-quiet", "query-metrics") + flush_persons_and_events() + + buckets = intent_clustering.fetch_tools_by_session(self.team) + + assert buckets.get("query-logs") == {"session-a"} + # the exec-wrapped call buckets under its inner tool + assert buckets.get("query-apm-spans") == {"session-a"} + assert "session-quiet" not in buckets.get("query-metrics", set()) + def test_window_stats_count_calls_intents_and_sessions(self) -> None: self._seed_tool_call("session-a", "execute_sql", intent="find slow queries") self._seed_tool_call("session-a", "query_trends") From b378986693521303bcaaa699543de944c0d3a4bc Mon Sep 17 00:00:00 2001 From: Frank Hamand Date: Fri, 21 Aug 2026 16:59:17 +0100 Subject: [PATCH 014/231] perf(ci): sparse-checkout paths-filter in change-detection jobs The `changes` gating jobs check out all 45k tracked files and then run `./.github/actions/paths-filter`, which on pull_request events diffs via the GitHub API and touches neither the working tree nor git history. The checkout exists only so the local action is on disk. Measured on real runs: these jobs spend 22-44s in checkout on GitHub-hosted runners and 11-13s on depot, while a sparse checkout of a few paths costs 0-7s. They gate every downstream job, so that time sits on the critical path of every CI run. Cone mode is off because it also materializes all 70 repo-root files (21.5 MB, `.test_durations` alone 18.5 MB). `ci-e2e-playwright.yml` already did this; the rest now match it. Generated-By: PostHog Desktop Task-Id: 4dbde566-5725-4f1c-b1b5-7dcabe4a38c9 --- .../skills/authoring-ci-workflows/SKILL.md | 52 ++++++++++++++++--- .depot/workflows/ci-backend.yml | 2 + .github/workflows/cd-sandbox-base-image.yml | 3 ++ .github/workflows/ci-agent-proxy.yml | 3 ++ .github/workflows/ci-agent-skills.yml | 3 ++ .github/workflows/ci-backend.yml | 2 + .github/workflows/ci-dagster.yml | 4 ++ .github/workflows/ci-frontend.yml | 6 +++ .github/workflows/ci-hobby.yml | 3 ++ .github/workflows/ci-hog.yml | 3 ++ .github/workflows/ci-llm-gateway.yml | 3 ++ .github/workflows/ci-mcp-ui-apps.yml | 3 ++ .github/workflows/ci-mcp.yml | 3 ++ ...ci-migrations-service-separation-check.yml | 3 ++ .../ci-ml-mirror-image-scrub-container.yml | 3 ++ .github/workflows/ci-nodejs-container.yml | 3 ++ .github/workflows/ci-oauth-proxy.yml | 3 ++ .github/workflows/ci-proto.yml | 2 + .github/workflows/ci-python.yml | 3 ++ .../ci-recording-rasterizer-container.yml | 3 ++ .github/workflows/ci-rust.yml | 2 + .github/workflows/ci-security.yaml | 2 + .github/workflows/ci-storybook.yml | 6 +++ .github/workflows/container-images-ci.yml | 3 ++ 24 files changed, 115 insertions(+), 8 deletions(-) diff --git a/.agents/skills/authoring-ci-workflows/SKILL.md b/.agents/skills/authoring-ci-workflows/SKILL.md index 68a118f850e1..5509d0e66b0f 100644 --- a/.agents/skills/authoring-ci-workflows/SKILL.md +++ b/.agents/skills/authoring-ci-workflows/SKILL.md @@ -121,19 +121,49 @@ Four rules for the gate body: `WF007` enforces 1, 4, and the `always()` condition, and it takes the dependency list from `needs:` as well as the step body, so a job you wired into `needs:` and then forgot to test is reported rather than silently trusted. The half of rule 2 it cannot check is whether you named the right jobs in `needs:` to begin with: "reporting job" and "coverage job" look identical to a linter, so that one is on you and the reviewer. -## Checkout / clone — shallow by default +## Checkout / clone — sparse first, then shallow -Full clones are slow and hang on degraded runners; blobs dominate clone size and are lazily fetchable. -Default to shallow; go deep only for real merge-base or version math, and even then bound the depth and filter blobs. +This repo is 45k tracked files and 4.6 GiB of packed objects, so **what you materialize costs more than how much history you fetch**. +Measured checkout-step durations, from the GitHub API on real runs: + +| Pattern | depot-ubuntu-24.04 | GitHub-hosted ubuntu | +| ----------------------------------------------- | ------------------ | -------------------- | +| `sparse-checkout` of a few paths, cone mode off | 0–7s | 0–7s | +| plain checkout (depth 1) | 11–13s | 22–44s | +| `fetch-depth: 1000` + `filter: blob:none` | 53–59s | — | + +- **Biggest lever: check out only the paths the job reads.** + Sparse-checkout is not just for single files — a job that runs a local composite action, reads a JSON config, or lints one directory should name those paths and nothing else. + + ```yaml + - uses: actions/checkout@ # v6 + with: + sparse-checkout: | + .github/actions/paths-filter + .github/clickhouse-versions.json + sparse-checkout-cone-mode: false + ``` + +- **Always set `sparse-checkout-cone-mode: false`.** + Cone mode additionally materializes every file in the repo root — here 70 files and 21.5 MB, `.test_durations` alone 18.5 MB — which is most of what you were trying to avoid. + Cone mode also only takes whole directories, so it drags in all of `bin/` when you wanted one script. + +- **`filter: blob:none` is counterproductive if the job then materializes the tree.** + It removes blobs from the fetch, but `git checkout` immediately lazy-fetches every blob in HEAD in a second round trip, which is slower than having fetched them in the pack. + That lazy fetch also intermittently fails its per-blob credential lookup with `could not read Username for github.com` (#59779, blocked a merge until retried). + Pair `blob:none` with `sparse-checkout` so the lazy fetch is a handful of blobs, or drop it and take the plain depth-1 checkout. - **Default:** plain `actions/checkout` (depth 1). Add nothing. -- **Diffing against the PR base:** bounded depth + blobless, then an explicit, scoped fetch (the sanctioned pattern, from `ci-backend.yml`): + +- **Diffing against the PR base:** you need real history, so bound the depth, filter blobs, **and** sparse-checkout the files the job reads: ```yaml - uses: actions/checkout@ # v6 with: fetch-depth: 1000 filter: blob:none + sparse-checkout: .github/actions/paths-filter + sparse-checkout-cone-mode: false - name: Fetch PR base for affected diff if: github.event_name == 'pull_request' env: @@ -141,13 +171,19 @@ Default to shallow; go deep only for real merge-base or version math, and even t run: git fetch --no-tags --depth=1000 --filter=blob:none origin "$BASE_REF:refs/remotes/origin/$BASE_REF" ``` -- **One file (e.g. `.nvmrc` before `setup-node`):** `sparse-checkout` it instead of cloning the repo. + A sparse working tree does not affect `git merge-base`, `git diff ...`, `git log --name-status`, `git ls-tree`, `git ls-files`, or `git show :` — those read the object database or the index. + Only commands that compare against the worktree (`git diff HEAD`, `git status`) see the skip-worktree entries. + +- **`changes` / paths-filter gating jobs:** on `pull_request` the vendored `.github/actions/paths-filter` diffs via the GitHub API and never touches the tree. + The only reason to check out is that a local action must exist on disk, so sparse-checkout `.github/actions/paths-filter` plus any file the job's own steps read. + **Never pass `base: HEAD` to paths-filter from a sparse job** — that routes it to `git diff HEAD`, which a sparse worktree makes return nothing, so every downstream job silently skips green. + - **Foot-gun:** `git fetch --deepen=N` with **no refspec** falls back to the wildcard `refs/heads/*` and pulls _every branch_. Always pass an explicit, `--no-tags`, `--filter=blob:none` refspec scoped to the base ref. (Bumping `actions/checkout`'s own `fetch-depth` is safe — it uses a scoped `refs/pull/N/merge` refspec.) - The linter rejects `fetch-depth: 0` unless you add `filter: blob:none`, use `sparse-checkout`, or justify it with `# hogli-lint: allow-full-depth-checkout -- `. - Genuinely full-history jobs: repo mirroring (`foss-sync.yml`), tag/submodule version math (`release-cli.yml`). - Most base-diff jobs should use bounded `1000 + blob:none`. + Genuinely full-history jobs: repo mirroring (`foss-sync.yml`), tag/submodule version math (`release-cli.yml`, `desktop-tag.yml`). + Most base-diff jobs should use bounded `1000 + blob:none` **plus** a sparse set. ## Pinning and tool versions @@ -250,7 +286,7 @@ Roll out a new blocking lint the same way: ship `continue-on-error`, clear the i - [ ] Triggers scoped: trigger `paths:` where the whole workflow is skippable; a required check must still fire on every PR (never paths-gate it into never dispatching). - [ ] Canonical `concurrency:` block (per-SHA push arm if it publishes on push). - [ ] `timeout-minutes` on every job (except reusable-caller jobs). -- [ ] Checkout is shallow, or bounded `1000 + blob:none` for base diffing. +- [ ] Checkout names only the paths the job reads (`sparse-checkout` + cone mode off), or is shallow; bounded `1000 + blob:none` only for base diffing. - [ ] Third-party actions SHA-pinned; Node from `.nvmrc`; `setup-uv` version pinned. - [ ] External fetches retry (`--retry-all-errors`), except where a repeat has a side effect. - [ ] High-volume API calls on a dedicated App token with `|| github.token` fork fallback. diff --git a/.depot/workflows/ci-backend.yml b/.depot/workflows/ci-backend.yml index 71660b2b91ad..bec2e95cc898 100644 --- a/.depot/workflows/ci-backend.yml +++ b/.depot/workflows/ci-backend.yml @@ -249,6 +249,8 @@ jobs: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: clean: false + sparse-checkout: .depot/actions/paths-filter + sparse-checkout-cone-mode: false - name: Clean up data directories with container permissions run: | # Use docker to clean up files created by containers diff --git a/.github/workflows/cd-sandbox-base-image.yml b/.github/workflows/cd-sandbox-base-image.yml index 4f97cd5ef476..8cac4c2b5936 100644 --- a/.github/workflows/cd-sandbox-base-image.yml +++ b/.github/workflows/cd-sandbox-base-image.yml @@ -39,6 +39,9 @@ jobs: sandbox_image: ${{ steps.filter.outputs.sandbox_image }} steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + sparse-checkout: .github/actions/paths-filter + sparse-checkout-cone-mode: false - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 id: app-token diff --git a/.github/workflows/ci-agent-proxy.yml b/.github/workflows/ci-agent-proxy.yml index 67bc37ee4cbb..3f44ab20a84c 100644 --- a/.github/workflows/ci-agent-proxy.yml +++ b/.github/workflows/ci-agent-proxy.yml @@ -23,6 +23,9 @@ jobs: agent_proxy: ${{ steps.filter.outputs.agent_proxy || 'true' }} steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + sparse-checkout: .github/actions/paths-filter + sparse-checkout-cone-mode: false - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 id: app-token diff --git a/.github/workflows/ci-agent-skills.yml b/.github/workflows/ci-agent-skills.yml index c5dce4f13cd2..be12e62ff3cb 100644 --- a/.github/workflows/ci-agent-skills.yml +++ b/.github/workflows/ci-agent-skills.yml @@ -26,6 +26,9 @@ jobs: skills: ${{ steps.filter.outputs.skills || 'true' }} steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + sparse-checkout: .github/actions/paths-filter + sparse-checkout-cone-mode: false - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 id: app-token diff --git a/.github/workflows/ci-backend.yml b/.github/workflows/ci-backend.yml index 81a28ded22a6..18ae2802b9de 100644 --- a/.github/workflows/ci-backend.yml +++ b/.github/workflows/ci-backend.yml @@ -116,6 +116,8 @@ jobs: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: clean: false + sparse-checkout: .github/actions/paths-filter + sparse-checkout-cone-mode: false - name: Clean up data directories with container permissions run: | # Use docker to clean up files created by containers diff --git a/.github/workflows/ci-dagster.yml b/.github/workflows/ci-dagster.yml index 0ef7d65685b1..c9c2fffb86f9 100644 --- a/.github/workflows/ci-dagster.yml +++ b/.github/workflows/ci-dagster.yml @@ -88,6 +88,10 @@ jobs: fetch-depth: 1000 filter: blob:none clean: false + sparse-checkout: | + .github/actions/paths-filter + .github/clickhouse-versions.json + sparse-checkout-cone-mode: false - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 id: app-token diff --git a/.github/workflows/ci-frontend.yml b/.github/workflows/ci-frontend.yml index b42cbec01029..2f812fe5e1ad 100644 --- a/.github/workflows/ci-frontend.yml +++ b/.github/workflows/ci-frontend.yml @@ -65,6 +65,12 @@ jobs: # For pull requests it's not necessary to check out the code, but we # also want this to run on master, so we need to check out - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + sparse-checkout: | + .github/actions/paths-filter + bin/frontend-exclude-filter + pnpm-workspace.yaml + sparse-checkout-cone-mode: false - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 id: app-token diff --git a/.github/workflows/ci-hobby.yml b/.github/workflows/ci-hobby.yml index 54e082191cfa..1c45e5b87b95 100644 --- a/.github/workflows/ci-hobby.yml +++ b/.github/workflows/ci-hobby.yml @@ -42,6 +42,9 @@ jobs: label_removed: ${{ steps.check-label.outputs.label_removed }} steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + sparse-checkout: .github/actions/paths-filter + sparse-checkout-cone-mode: false - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 id: app-token diff --git a/.github/workflows/ci-hog.yml b/.github/workflows/ci-hog.yml index 2ec32eff2182..a6f72b33eb34 100644 --- a/.github/workflows/ci-hog.yml +++ b/.github/workflows/ci-hog.yml @@ -33,6 +33,9 @@ jobs: # For pull requests it's not necessary to checkout the code, but we # also want this to run on master so we need to checkout - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + sparse-checkout: .github/actions/paths-filter + sparse-checkout-cone-mode: false - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 id: app-token diff --git a/.github/workflows/ci-llm-gateway.yml b/.github/workflows/ci-llm-gateway.yml index ad9fc3a024b7..a9409a07594e 100644 --- a/.github/workflows/ci-llm-gateway.yml +++ b/.github/workflows/ci-llm-gateway.yml @@ -34,6 +34,9 @@ jobs: llm_gateway: ${{ steps.filter.outputs.llm_gateway || 'true' }} steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + sparse-checkout: .github/actions/paths-filter + sparse-checkout-cone-mode: false - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 id: app-token diff --git a/.github/workflows/ci-mcp-ui-apps.yml b/.github/workflows/ci-mcp-ui-apps.yml index 0e5f533386e1..2a39c5f10736 100644 --- a/.github/workflows/ci-mcp-ui-apps.yml +++ b/.github/workflows/ci-mcp-ui-apps.yml @@ -22,6 +22,9 @@ jobs: ui-apps: ${{ steps.filter.outputs.ui-apps || 'true' }} steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + sparse-checkout: .github/actions/paths-filter + sparse-checkout-cone-mode: false - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 id: app-token diff --git a/.github/workflows/ci-mcp.yml b/.github/workflows/ci-mcp.yml index dac3bbf1bb47..6c736234c29a 100644 --- a/.github/workflows/ci-mcp.yml +++ b/.github/workflows/ci-mcp.yml @@ -41,6 +41,9 @@ jobs: mcp: ${{ steps.filter.outputs.mcp || 'true' }} steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + sparse-checkout: .github/actions/paths-filter + sparse-checkout-cone-mode: false # MCP integration tests boot the full PostHog backend (Django web server, # Celery worker, migrations, demo data), so the filter stays broad: any diff --git a/.github/workflows/ci-migrations-service-separation-check.yml b/.github/workflows/ci-migrations-service-separation-check.yml index 3f5cace4e61a..6a855fe27d54 100644 --- a/.github/workflows/ci-migrations-service-separation-check.yml +++ b/.github/workflows/ci-migrations-service-separation-check.yml @@ -21,6 +21,9 @@ jobs: timeout-minutes: 5 steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + sparse-checkout: .github/actions/paths-filter + sparse-checkout-cone-mode: false # Author-attested escape hatch for DB-noop migrations (e.g. SeparateDatabaseAndState # state-only renames) that ship safely alongside service code. The check is path-only diff --git a/.github/workflows/ci-ml-mirror-image-scrub-container.yml b/.github/workflows/ci-ml-mirror-image-scrub-container.yml index 20252fe3dcaa..5fc4ca6b7132 100644 --- a/.github/workflows/ci-ml-mirror-image-scrub-container.yml +++ b/.github/workflows/ci-ml-mirror-image-scrub-container.yml @@ -25,6 +25,9 @@ jobs: steps: - name: Check out uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + sparse-checkout: .github/actions/paths-filter + sparse-checkout-cone-mode: false - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 id: app-token diff --git a/.github/workflows/ci-nodejs-container.yml b/.github/workflows/ci-nodejs-container.yml index 2adc8719bbe2..ccf355fd38a8 100644 --- a/.github/workflows/ci-nodejs-container.yml +++ b/.github/workflows/ci-nodejs-container.yml @@ -44,6 +44,9 @@ jobs: steps: - name: Check out uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + sparse-checkout: .github/actions/paths-filter + sparse-checkout-cone-mode: false - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 id: app-token diff --git a/.github/workflows/ci-oauth-proxy.yml b/.github/workflows/ci-oauth-proxy.yml index 520ca9cb89f3..9c37c7cd15d8 100644 --- a/.github/workflows/ci-oauth-proxy.yml +++ b/.github/workflows/ci-oauth-proxy.yml @@ -22,6 +22,9 @@ jobs: oauth-proxy: ${{ steps.filter.outputs.oauth-proxy || 'true' }} steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + sparse-checkout: .github/actions/paths-filter + sparse-checkout-cone-mode: false - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 id: app-token diff --git a/.github/workflows/ci-proto.yml b/.github/workflows/ci-proto.yml index 6ba702975bf2..0a978c317834 100644 --- a/.github/workflows/ci-proto.yml +++ b/.github/workflows/ci-proto.yml @@ -25,6 +25,8 @@ jobs: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: clean: false + sparse-checkout: .github/actions/paths-filter + sparse-checkout-cone-mode: false - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 id: app-token if: github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository diff --git a/.github/workflows/ci-python.yml b/.github/workflows/ci-python.yml index afd52270d67d..b45e942b9f50 100644 --- a/.github/workflows/ci-python.yml +++ b/.github/workflows/ci-python.yml @@ -31,6 +31,9 @@ jobs: # For pull requests it's not necessary to checkout the code, but we # also want this to run on master so we need to checkout - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + sparse-checkout: .github/actions/paths-filter + sparse-checkout-cone-mode: false - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 id: app-token diff --git a/.github/workflows/ci-recording-rasterizer-container.yml b/.github/workflows/ci-recording-rasterizer-container.yml index 1be91c7dd1e9..6e8295c68858 100644 --- a/.github/workflows/ci-recording-rasterizer-container.yml +++ b/.github/workflows/ci-recording-rasterizer-container.yml @@ -46,6 +46,9 @@ jobs: steps: - name: Check out uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + sparse-checkout: .github/actions/paths-filter + sparse-checkout-cone-mode: false - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 id: app-token diff --git a/.github/workflows/ci-rust.yml b/.github/workflows/ci-rust.yml index 012b1fd45ccd..f50c0ae20c75 100644 --- a/.github/workflows/ci-rust.yml +++ b/.github/workflows/ci-rust.yml @@ -34,6 +34,8 @@ jobs: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: clean: false + sparse-checkout: .github/actions/paths-filter + sparse-checkout-cone-mode: false - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 id: app-token if: github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository diff --git a/.github/workflows/ci-security.yaml b/.github/workflows/ci-security.yaml index f628294a7f18..c1acbec85fdd 100644 --- a/.github/workflows/ci-security.yaml +++ b/.github/workflows/ci-security.yaml @@ -40,6 +40,8 @@ jobs: with: clean: false persist-credentials: false + sparse-checkout: .github/actions/paths-filter + sparse-checkout-cone-mode: false - name: Force all scans for oversized pull requests id: oversized if: github.event_name == 'pull_request' && github.event.pull_request.changed_files > 3000 diff --git a/.github/workflows/ci-storybook.yml b/.github/workflows/ci-storybook.yml index 9cbda31f8647..e4ea9c08cb36 100644 --- a/.github/workflows/ci-storybook.yml +++ b/.github/workflows/ci-storybook.yml @@ -75,6 +75,12 @@ jobs: # ./.github/actions/paths-filter steps, which must exist on disk (on # pull_request events they still diff via the API, not the checkout). - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + sparse-checkout: | + .github/actions/paths-filter + bin/frontend-exclude-filter + pnpm-workspace.yaml + sparse-checkout-cone-mode: false - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 id: app-token diff --git a/.github/workflows/container-images-ci.yml b/.github/workflows/container-images-ci.yml index 2b0f2f02deb9..8e9a2da7fa86 100644 --- a/.github/workflows/container-images-ci.yml +++ b/.github/workflows/container-images-ci.yml @@ -38,6 +38,9 @@ jobs: dockerfiles_files: ${{ steps.filter.outputs.dockerfiles_files }} steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + sparse-checkout: .github/actions/paths-filter + sparse-checkout-cone-mode: false - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 id: app-token if: github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository From 60d801ced6ad305c086f43e3c971020a624d4af6 Mon Sep 17 00:00:00 2001 From: Frank Hamand Date: Fri, 21 Aug 2026 16:59:19 +0100 Subject: [PATCH 015/231] fix(ci): keep docker-compose.base.yml in dagster sparse checkout The changes job reads docker-compose.base.yml at the merge base to build its migration cache key. With blob:none plus a sparse set that omits the file, that read becomes a lazy blob fetch, which can fail quietly and yield a wrong key. Listing the file puts the blob on disk during checkout instead. Generated-By: PostHog Desktop Task-Id: 6a7344ab-5db8-418f-9243-6c06114b330d --- .agents/skills/authoring-ci-workflows/SKILL.md | 2 ++ .github/workflows/ci-dagster.yml | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/.agents/skills/authoring-ci-workflows/SKILL.md b/.agents/skills/authoring-ci-workflows/SKILL.md index 5509d0e66b0f..dbc2003810c2 100644 --- a/.agents/skills/authoring-ci-workflows/SKILL.md +++ b/.agents/skills/authoring-ci-workflows/SKILL.md @@ -173,6 +173,8 @@ Measured checkout-step durations, from the GitHub API on real runs: A sparse working tree does not affect `git merge-base`, `git diff ...`, `git log --name-status`, `git ls-tree`, `git ls-files`, or `git show :` — those read the object database or the index. Only commands that compare against the worktree (`git diff HEAD`, `git status`) see the skip-worktree entries. + One caveat when `blob:none` is also set: `git show :` still needs that blob, and a sparse checkout never downloaded it, so the read becomes a lazy fetch that can fail. + Name any file a step reads that way in the sparse set — `ci-dagster.yml` does this for `docker-compose.base.yml`, whose contents feed a cache key. - **`changes` / paths-filter gating jobs:** on `pull_request` the vendored `.github/actions/paths-filter` diffs via the GitHub API and never touches the tree. The only reason to check out is that a local action must exist on disk, so sparse-checkout `.github/actions/paths-filter` plus any file the job's own steps read. diff --git a/.github/workflows/ci-dagster.yml b/.github/workflows/ci-dagster.yml index c9c2fffb86f9..ca0f9533e6e2 100644 --- a/.github/workflows/ci-dagster.yml +++ b/.github/workflows/ci-dagster.yml @@ -83,6 +83,10 @@ jobs: # fetch-depth=1000 + blob:none mirrors ci-backend's turbo-discover so # HEAD^2 (PR branch tip) is reachable for the merge-base step below # without the cost of fetching blobs. + # docker-compose.base.yml is in the sparse set for the schema-key step, which + # reads it at the merge base. Checking it out puts the blob on disk, so that + # `git show` resolves locally rather than through a lazy fetch that could fail + # quietly and leave the migration cache key wrong. - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: fetch-depth: 1000 @@ -91,6 +95,7 @@ jobs: sparse-checkout: | .github/actions/paths-filter .github/clickhouse-versions.json + docker-compose.base.yml sparse-checkout-cone-mode: false - uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 From d096e7d20159b5b0bdbabc78f09ba72d30cd2ec3 Mon Sep 17 00:00:00 2001 From: Frank Hamand Date: Fri, 21 Aug 2026 16:59:21 +0100 Subject: [PATCH 016/231] fix(ci): give the agent skills build room for its migrations The job runs Django migrations from scratch on ubuntu-latest with no schema cache. Measured across three recent runs that step took 22m21s, 27m50s and 28m34s against a 30-minute job budget, so it cut out mid-migrate on two of them. Build skills, the step the job exists for, takes about 10 seconds either way. Raising the budget to 45 minutes leaves headroom over the worst time seen. Generated-By: PostHog Desktop Task-Id: 6a7344ab-5db8-418f-9243-6c06114b330d --- .github/workflows/cd-sandbox-base-image.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/cd-sandbox-base-image.yml b/.github/workflows/cd-sandbox-base-image.yml index 8cac4c2b5936..ca34499bb96a 100644 --- a/.github/workflows/cd-sandbox-base-image.yml +++ b/.github/workflows/cd-sandbox-base-image.yml @@ -76,7 +76,10 @@ jobs: contains(github.event.pull_request.labels.*.name, 'build-tasks-sandbox-image') ) runs-on: ubuntu-latest - timeout-minutes: 30 + # `Run migrations` builds the schema from scratch on ubuntu-latest with no schema cache and + # takes 22-29 minutes, so a 30-minute budget left no headroom and cut the job off mid-migrate + # on two of three recent runs. `Build skills`, the step this job exists for, takes 10 seconds. + timeout-minutes: 45 permissions: contents: read From 0e7177da36431cc53d6625656c90db80260cf264 Mon Sep 17 00:00:00 2001 From: Alex Date: Fri, 21 Aug 2026 21:33:20 +0200 Subject: [PATCH 017/231] feat(access-control): rename the model family to surface access limits AccessCeiling becomes SurfaceAccessLimit, channel becomes surface, and the permission class becomes WithinSurfaceLimits, so the vocabulary reads without a glossary: a surface is how the request arrived (MCP now; personal API keys, share links, impersonation later) and a limit is the max level the org allows through it. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01LEer2y9uGMkADPjMfWG8WK --- posthog/api/routing.py | 8 +- .../access_control/backend/facade/ceilings.py | 79 ------------------ .../backend/facade/permissions.py | 18 ++--- .../backend/facade/surface_limits.py | 80 +++++++++++++++++++ ...sceiling.py => 0002_surfaceaccesslimit.py} | 12 +-- .../backend/migrations/max_migration.txt | 2 +- .../access_control/backend/models/__init__.py | 4 +- ...ess_ceiling.py => surface_access_limit.py} | 31 +++---- ...lings.py => test_surface_access_limits.py} | 48 +++++------ 9 files changed, 143 insertions(+), 139 deletions(-) delete mode 100644 products/access_control/backend/facade/ceilings.py create mode 100644 products/access_control/backend/facade/surface_limits.py rename products/access_control/backend/migrations/{0002_accessceiling.py => 0002_surfaceaccesslimit.py} (86%) rename products/access_control/backend/models/{access_ceiling.py => surface_access_limit.py} (54%) rename products/access_control/backend/tests/{test_access_ceilings.py => test_surface_access_limits.py} (53%) diff --git a/posthog/api/routing.py b/posthog/api/routing.py index 70eb5c5ee538..a237a6113672 100644 --- a/posthog/api/routing.py +++ b/posthog/api/routing.py @@ -42,7 +42,7 @@ from posthog.scopes import APIScopeObjectOrNotSupported from posthog.user_permissions import UserPermissions -from products.access_control.backend.facade.permissions import ChannelCeilingPermission +from products.access_control.backend.facade.permissions import WithinSurfaceLimits if TYPE_CHECKING: _GenericViewSet = GenericViewSet @@ -254,9 +254,9 @@ def get_permissions(self): except NotImplementedError: pass else: - # Domain enforcement and channel ceilings are tenant boundaries, not authorization + # Domain enforcement and surface limits are tenant boundaries, not authorization # levels: views that shape their own permission chain cannot opt out of them. - return [*dangerously_defined, ChannelCeilingPermission(), VerifiedDomainEnforcementPermission()] + return [*dangerously_defined, WithinSurfaceLimits(), VerifiedDomainEnforcementPermission()] if isinstance(self.request.successful_authenticator, InternalAPIAuthentication): return [IsAuthenticated()] @@ -272,7 +272,7 @@ def get_permissions(self): permission_classes: list = [ IsAuthenticated, APIScopePermission, - ChannelCeilingPermission, + WithinSurfaceLimits, AccessControlPermission, ] diff --git a/products/access_control/backend/facade/ceilings.py b/products/access_control/backend/facade/ceilings.py deleted file mode 100644 index 16052b4e55f4..000000000000 --- a/products/access_control/backend/facade/ceilings.py +++ /dev/null @@ -1,79 +0,0 @@ -"""Per-channel access ceilings: org-wide caps on what any principal can do through one pathway. - -The first consumer is `APIScopePermission`, which denies write-scoped actions when the request's -channel is capped below editor. When the access-control facade's `decide()` lands, it composes the -same cap into object-level decisions; both read this module, so the two enforcement points cannot -disagree about what the organization configured. -""" - -from typing import TYPE_CHECKING, Any - -from posthog.auth import OAuthAccessTokenAuthentication, PersonalAPIKeyAuthentication -from posthog.constants import AvailableFeature - -from products.access_control.backend.models.access_ceiling import AccessCeiling - -if TYPE_CHECKING: - from posthog.models.organization import Organization - -# The outbound identity of services/mcp (see its oauth-constants.ts). Channel classification is -# governance of the pathway, not a defense against a hostile key holder: the same credential used -# outside MCP keeps its own scopes, and tightening the credential itself is the mint-time follow-up. -MCP_USER_AGENT_MARKER = "posthog/mcp-server" - -WRITE_CAPPED_LEVELS = {AccessCeiling.MaxLevel.NONE, AccessCeiling.MaxLevel.VIEWER} - - -def classify_channel(request: Any) -> str | None: - """The access pathway this request arrived through, or None for pathways without policies.""" - authenticator = getattr(request, "successful_authenticator", None) - if isinstance(authenticator, PersonalAPIKeyAuthentication | OAuthAccessTokenAuthentication): - user_agent = request.headers.get("User-Agent") or "" - if MCP_USER_AGENT_MARKER in user_agent: - return AccessCeiling.Channel.MCP - return None - - -def ceiling_denial_for_request( - request: Any, organization: "Organization", resource: str | None, writes: bool -) -> str | None: - """The complete channel-ceiling decision: a user-facing denial message when this request's - pathway is capped below what the action needs, or None to allow. - - Owns classification, the entitlement gate, row lookup and the copy, so enforcement points - (today `APIScopePermission`, later the facade's `decide()`) contain no policy of their own.""" - if not writes: - return None - channel = classify_channel(request) - if channel is None: - return None - if not organization.is_feature_available(AvailableFeature.ORGANIZATION_SECURITY_SETTINGS): - return None - cap = channel_ceiling(organization, channel, resource) - if cap in WRITE_CAPPED_LEVELS: - return ( - "Your organization restricts MCP access to read-only. " - "An organization admin can change this in your organization settings." - ) - return None - - -def channel_ceiling( - organization: "Organization", channel: str | None, resource: str | None = None -) -> AccessCeiling.MaxLevel | None: - """The max level this organization allows through `channel`, or None when unrestricted. - - A row naming `resource` overrides the wildcard row. One query per call; callers on hot - paths should check the channel first, since channel=None short-circuits. - """ - if channel is None: - return None - rows = AccessCeiling.objects.filter( - organization=organization, channel=channel, resource__in=[resource, None] if resource else [None] - ).values_list("resource", "max_level") - by_resource = dict(rows) - if resource is not None and resource in by_resource: - return AccessCeiling.MaxLevel(by_resource[resource]) - if None in by_resource: - return AccessCeiling.MaxLevel(by_resource[None]) - return None diff --git a/products/access_control/backend/facade/permissions.py b/products/access_control/backend/facade/permissions.py index f5215882b65f..8bd997b66dae 100644 --- a/products/access_control/backend/facade/permissions.py +++ b/products/access_control/backend/facade/permissions.py @@ -1,30 +1,30 @@ """DRF enforcement point for access-control policies owned by this product. -`TeamAndOrgViewSetMixin.get_permissions` composes `ChannelCeilingPermission` into every viewset's -stack, so a new endpoint gets ceiling enforcement without knowing ceilings exist. DRF evaluates +`TeamAndOrgViewSetMixin.get_permissions` composes `WithinSurfaceLimits` into every viewset's +stack, so a new endpoint gets surface-limit enforcement without knowing limits exist. DRF evaluates permission classes with AND semantics: this class is an independent vote and cannot be bypassed by another class's internal early return (a `*`-scoped token passing `APIScopePermission` is still -capped here). +limited here). """ from typing import Any from posthog.permissions import ScopeBasePermission, get_organization_from_view -from products.access_control.backend.facade.ceilings import ceiling_denial_for_request, classify_channel +from products.access_control.backend.facade.surface_limits import classify_surface, limit_denial_for_request -class ChannelCeilingPermission(ScopeBasePermission): - """Denies actions that exceed the organization's cap for the request's access pathway. +class WithinSurfaceLimits(ScopeBasePermission): + """Denies actions that exceed the organization's limit for the request's access surface. Subclasses ScopeBasePermission only for `_get_required_scopes`, so this class derives an action's read/write nature the same way APIScopePermission does and the two can't disagree about what counts as a write.""" def has_permission(self, request: Any, view: Any) -> bool: - # Cheap exit first: almost every request has no classified channel, and classification + # Cheap exit first: almost every request has no classified surface, and classification # is a couple of isinstance checks with no query. - if classify_channel(request) is None: + if classify_surface(request) is None: return True scope_object = getattr(view, "scope_object", None) @@ -36,7 +36,7 @@ def has_permission(self, request: Any, view: Any) -> bool: return True required_scopes = self._get_required_scopes(request, view) or [] - denial = ceiling_denial_for_request( + denial = limit_denial_for_request( request, organization, resource=scope_object if scope_object != "INTERNAL" else None, diff --git a/products/access_control/backend/facade/surface_limits.py b/products/access_control/backend/facade/surface_limits.py new file mode 100644 index 000000000000..6e0b91f1bde5 --- /dev/null +++ b/products/access_control/backend/facade/surface_limits.py @@ -0,0 +1,80 @@ +"""Per-surface access limits: org-wide caps on what any principal can do through one access +surface (the MCP server today; personal API keys, share links and impersonation later). + +The first consumer is `WithinSurfaceLimits` in facade/permissions.py, which denies write-scoped +actions when the request's surface is limited below editor. When the access-control facade's `decide()` lands, it composes the same limit into +object-level decisions; both read this module, so enforcement points cannot disagree about what +the organization configured. +""" + +from typing import TYPE_CHECKING, Any + +from posthog.auth import OAuthAccessTokenAuthentication, PersonalAPIKeyAuthentication +from posthog.constants import AvailableFeature + +from products.access_control.backend.models.surface_access_limit import SurfaceAccessLimit + +if TYPE_CHECKING: + from posthog.models.organization import Organization + +# The outbound identity of services/mcp (see its oauth-constants.ts). Surface classification is +# governance of the pathway, not a defense against a hostile key holder: the same credential used +# outside MCP keeps its own scopes, and tightening the credential itself is the mint-time follow-up. +MCP_USER_AGENT_MARKER = "posthog/mcp-server" + +WRITE_LIMITED_LEVELS = {SurfaceAccessLimit.MaxLevel.NONE, SurfaceAccessLimit.MaxLevel.VIEWER} + + +def classify_surface(request: Any) -> str | None: + """The access surface this request arrived through, or None for surfaces without policies.""" + authenticator = getattr(request, "successful_authenticator", None) + if isinstance(authenticator, PersonalAPIKeyAuthentication | OAuthAccessTokenAuthentication): + user_agent = request.headers.get("User-Agent") or "" + if MCP_USER_AGENT_MARKER in user_agent: + return SurfaceAccessLimit.Surface.MCP + return None + + +def limit_denial_for_request( + request: Any, organization: "Organization", resource: str | None, writes: bool +) -> str | None: + """The complete surface-limit decision: a user-facing denial message when this request's + surface is limited below what the action needs, or None to allow. + + Owns classification, the entitlement gate, row lookup and the copy, so enforcement points + (today `WithinSurfaceLimits`, later the facade's `decide()`) contain no policy of their own.""" + if not writes: + return None + surface = classify_surface(request) + if surface is None: + return None + if not organization.is_feature_available(AvailableFeature.ORGANIZATION_SECURITY_SETTINGS): + return None + limit = surface_limit(organization, surface, resource) + if limit in WRITE_LIMITED_LEVELS: + return ( + "Your organization restricts MCP access to read-only. " + "An organization admin can change this in your organization settings." + ) + return None + + +def surface_limit( + organization: "Organization", surface: str | None, resource: str | None = None +) -> SurfaceAccessLimit.MaxLevel | None: + """The max level this organization allows through `surface`, or None when unrestricted. + + A row naming `resource` overrides the wildcard row. One query per call; callers on hot + paths should classify the surface first, since surface=None short-circuits. + """ + if surface is None: + return None + rows = SurfaceAccessLimit.objects.filter( + organization=organization, surface=surface, resource__in=[resource, None] if resource else [None] + ).values_list("resource", "max_level") + by_resource = dict(rows) + if resource is not None and resource in by_resource: + return SurfaceAccessLimit.MaxLevel(by_resource[resource]) + if None in by_resource: + return SurfaceAccessLimit.MaxLevel(by_resource[None]) + return None diff --git a/products/access_control/backend/migrations/0002_accessceiling.py b/products/access_control/backend/migrations/0002_surfaceaccesslimit.py similarity index 86% rename from products/access_control/backend/migrations/0002_accessceiling.py rename to products/access_control/backend/migrations/0002_surfaceaccesslimit.py index e49168340490..21976433f85b 100644 --- a/products/access_control/backend/migrations/0002_accessceiling.py +++ b/products/access_control/backend/migrations/0002_surfaceaccesslimit.py @@ -1,4 +1,4 @@ -# Generated by Django 5.2.17 on 2026-08-20 10:49 +# Generated by Django 5.2.17 on 2026-08-21 19:30 import django.db.models.deletion from django.conf import settings @@ -16,7 +16,7 @@ class Migration(migrations.Migration): operations = [ migrations.CreateModel( - name="AccessCeiling", + name="SurfaceAccessLimit", fields=[ ( "id", @@ -27,7 +27,7 @@ class Migration(migrations.Migration): serialize=False, ), ), - ("channel", models.CharField(choices=[("mcp", "Mcp")], max_length=32)), + ("surface", models.CharField(choices=[("mcp", "Mcp")], max_length=32)), ("resource", models.CharField(blank=True, max_length=64, null=True)), ( "max_level", @@ -56,7 +56,7 @@ class Migration(migrations.Migration): models.ForeignKey( db_constraint=False, on_delete=django.db.models.deletion.CASCADE, - related_name="access_ceilings", + related_name="surface_access_limits", to="posthog.organization", ), ), @@ -64,8 +64,8 @@ class Migration(migrations.Migration): options={ "constraints": [ models.UniqueConstraint( - fields=("organization", "channel", "resource"), - name="unique_ceiling_per_org_channel_resource", + fields=("organization", "surface", "resource"), + name="unique_limit_per_org_surface_resource", nulls_distinct=False, ) ], diff --git a/products/access_control/backend/migrations/max_migration.txt b/products/access_control/backend/migrations/max_migration.txt index ca3c6133d7f2..028284704100 100644 --- a/products/access_control/backend/migrations/max_migration.txt +++ b/products/access_control/backend/migrations/max_migration.txt @@ -1 +1 @@ -0002_accessceiling +0002_surfaceaccesslimit diff --git a/products/access_control/backend/models/__init__.py b/products/access_control/backend/models/__init__.py index a105a3665831..bc69cd8caa7c 100644 --- a/products/access_control/backend/models/__init__.py +++ b/products/access_control/backend/models/__init__.py @@ -1,4 +1,4 @@ -from .access_ceiling import AccessCeiling from .property_access_control import PropertyAccessControl +from .surface_access_limit import SurfaceAccessLimit -__all__ = ["AccessCeiling", "PropertyAccessControl"] +__all__ = ["PropertyAccessControl", "SurfaceAccessLimit"] diff --git a/products/access_control/backend/models/access_ceiling.py b/products/access_control/backend/models/surface_access_limit.py similarity index 54% rename from products/access_control/backend/models/access_ceiling.py rename to products/access_control/backend/models/surface_access_limit.py index 7ae76c0f7e3d..938a0a4831b3 100644 --- a/products/access_control/backend/models/access_ceiling.py +++ b/products/access_control/backend/models/surface_access_limit.py @@ -3,24 +3,25 @@ from posthog.models.utils import UUIDModel -class AccessCeiling(UUIDModel): - """An organization-wide cap on what any principal can do through one access pathway. +class SurfaceAccessLimit(UUIDModel): + """An organization-wide cap on what any principal can do through one access surface, such as + the MCP server, a personal API key, or a public share link. - Ceilings are not grants. The grants system (AccessControl rows) answers "what may this - principal do"; a ceiling answers "how wide is this pathway", and the effective access is - the minimum of the two. A ceiling therefore applies to every member, admins included: - exceptions are future subject-specific ceiling rows that widen the cap, never grants. + Limits are not grants. The grants system (AccessControl rows) answers "what may this + principal do"; a limit answers "how much this surface allows", and the effective access is + the minimum of the two. A limit therefore applies to every member, admins included: + exceptions are future subject-specific limit rows that widen it, never grants. - Absence of a row means the channel is unrestricted. `resource=None` caps every resource; + Absence of a row means the surface is unrestricted. `resource=None` limits every resource; a row naming a resource overrides the wildcard row for that resource. """ - class Channel(models.TextChoices): + class Surface(models.TextChoices): MCP = "mcp" class MaxLevel(models.TextChoices): - # The grants vocabulary, minus levels a cap never needs. "none" disables the - # channel; "viewer" makes it read-only. + # The grants vocabulary, minus levels a limit never needs. "none" disables the + # surface; "viewer" makes it read-only. NONE = "none" VIEWER = "viewer" EDITOR = "editor" @@ -28,8 +29,8 @@ class MaxLevel(models.TextChoices): class Meta: constraints = [ models.UniqueConstraint( - fields=["organization", "channel", "resource"], - name="unique_ceiling_per_org_channel_resource", + fields=["organization", "surface", "resource"], + name="unique_limit_per_org_surface_resource", nulls_distinct=False, ) ] @@ -39,12 +40,12 @@ class Meta: organization = models.ForeignKey( "posthog.Organization", on_delete=models.CASCADE, - related_name="access_ceilings", + related_name="surface_access_limits", db_constraint=False, ) - channel: models.CharField = models.CharField(max_length=32, choices=Channel.choices) - # An APIScopeObject name, or None to cap every resource. + surface: models.CharField = models.CharField(max_length=32, choices=Surface.choices) + # An APIScopeObject name, or None to limit every resource. resource: models.CharField = models.CharField(max_length=64, null=True, blank=True) max_level: models.CharField = models.CharField(max_length=32, choices=MaxLevel.choices) diff --git a/products/access_control/backend/tests/test_access_ceilings.py b/products/access_control/backend/tests/test_surface_access_limits.py similarity index 53% rename from products/access_control/backend/tests/test_access_ceilings.py rename to products/access_control/backend/tests/test_surface_access_limits.py index b5a469c57e55..2a86203e0631 100644 --- a/products/access_control/backend/tests/test_access_ceilings.py +++ b/products/access_control/backend/tests/test_surface_access_limits.py @@ -6,30 +6,32 @@ from posthog.models.personal_api_key import PersonalAPIKey from posthog.models.utils import generate_random_token_personal, hash_key_value -from products.access_control.backend.facade.ceilings import MCP_USER_AGENT_MARKER, channel_ceiling -from products.access_control.backend.models import AccessCeiling +from products.access_control.backend.facade.surface_limits import MCP_USER_AGENT_MARKER, surface_limit +from products.access_control.backend.models import SurfaceAccessLimit -class TestChannelCeilingResolution(BaseTest): +class TestSurfaceLimitResolution(BaseTest): def test_resource_row_overrides_wildcard_row(self) -> None: - AccessCeiling.objects.create(organization=self.organization, channel="mcp", resource=None, max_level="viewer") - AccessCeiling.objects.create( - organization=self.organization, channel="mcp", resource="feature_flag", max_level="editor" + SurfaceAccessLimit.objects.create( + organization=self.organization, surface="mcp", resource=None, max_level="viewer" + ) + SurfaceAccessLimit.objects.create( + organization=self.organization, surface="mcp", resource="feature_flag", max_level="editor" ) - assert channel_ceiling(self.organization, "mcp", "dashboard") == AccessCeiling.MaxLevel.VIEWER - assert channel_ceiling(self.organization, "mcp", "feature_flag") == AccessCeiling.MaxLevel.EDITOR - assert channel_ceiling(self.organization, "mcp") == AccessCeiling.MaxLevel.VIEWER + assert surface_limit(self.organization, "mcp", "dashboard") == SurfaceAccessLimit.MaxLevel.VIEWER + assert surface_limit(self.organization, "mcp", "feature_flag") == SurfaceAccessLimit.MaxLevel.EDITOR + assert surface_limit(self.organization, "mcp") == SurfaceAccessLimit.MaxLevel.VIEWER - def test_no_rows_and_no_channel_mean_unrestricted(self) -> None: - assert channel_ceiling(self.organization, "mcp", "dashboard") is None - assert channel_ceiling(self.organization, None, "dashboard") is None + def test_no_rows_and_no_surface_mean_unrestricted(self) -> None: + assert surface_limit(self.organization, "mcp", "dashboard") is None + assert surface_limit(self.organization, None, "dashboard") is None class TestMCPReadOnlyEnforcement(APIBaseTest): - """The regression these guard: a write-scoped token arriving through the MCP pathway must be - denied when the org caps the channel, including `*`-scoped tokens, while reads and non-MCP - requests stay untouched. No existing test exercises the ceiling path at all.""" + """The regression these guard: a write-scoped token arriving through the MCP surface must be + denied when the org limits it, including `*`-scoped tokens, while reads and non-MCP requests + stay untouched. No existing test exercises the surface-limit path at all.""" def setUp(self) -> None: super().setUp() @@ -57,8 +59,8 @@ def _request(self, method: str, body: dict | None = None, mcp: bool = True): headers={"User-Agent": f"cursor/1.0 {MCP_USER_AGENT_MARKER}; version: 1.0.0"} if mcp else None, ) - def test_capped_channel_denies_writes_allows_reads(self) -> None: - AccessCeiling.objects.create(organization=self.organization, channel="mcp", max_level="viewer") + def test_limited_surface_denies_writes_allows_reads(self) -> None: + SurfaceAccessLimit.objects.create(organization=self.organization, surface="mcp", max_level="viewer") denied = self._request("post", {"key": "flag-via-mcp", "name": "flag"}) assert denied.status_code == 403 @@ -66,18 +68,18 @@ def test_capped_channel_denies_writes_allows_reads(self) -> None: assert self._request("get").status_code == 200 - @parameterized.expand([("no_ceiling_row", True), ("not_mcp_user_agent", False)]) - def test_writes_pass_without_a_matching_ceiling(self, _name: str, mcp: bool) -> None: + @parameterized.expand([("no_limit_row", True), ("not_mcp_user_agent", False)]) + def test_writes_pass_without_a_matching_limit(self, _name: str, mcp: bool) -> None: if not mcp: - AccessCeiling.objects.create(organization=self.organization, channel="mcp", max_level="viewer") + SurfaceAccessLimit.objects.create(organization=self.organization, surface="mcp", max_level="viewer") response = self._request("post", {"key": f"flag-{_name}", "name": "flag"}, mcp=mcp) assert response.status_code == 201 def test_resource_exception_lets_that_resource_write(self) -> None: - AccessCeiling.objects.create(organization=self.organization, channel="mcp", max_level="viewer") - AccessCeiling.objects.create( - organization=self.organization, channel="mcp", resource="feature_flag", max_level="editor" + SurfaceAccessLimit.objects.create(organization=self.organization, surface="mcp", max_level="viewer") + SurfaceAccessLimit.objects.create( + organization=self.organization, surface="mcp", resource="feature_flag", max_level="editor" ) response = self._request("post", {"key": "flag-excepted", "name": "flag"}) From 8559e39bd0b033e4a45223f9b495af6ecd6e3780 Mon Sep 17 00:00:00 2001 From: Alex Date: Fri, 21 Aug 2026 21:57:54 +0200 Subject: [PATCH 018/231] chore(access-control): rewrite docstrings in simplified technical english One statement per sentence, no semicolons, explicit subjects, hedges kept. Also removes the test class docstring per the no-doc-comments-in-tests house rule. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01LEer2y9uGMkADPjMfWG8WK --- posthog/api/routing.py | 2 +- .../backend/facade/permissions.py | 20 +++++++-------- .../backend/facade/surface_limits.py | 25 +++++++++++-------- .../backend/models/surface_access_limit.py | 24 +++++++++--------- .../tests/test_surface_access_limits.py | 4 --- 5 files changed, 37 insertions(+), 38 deletions(-) diff --git a/posthog/api/routing.py b/posthog/api/routing.py index a237a6113672..a1a8692a8c8f 100644 --- a/posthog/api/routing.py +++ b/posthog/api/routing.py @@ -255,7 +255,7 @@ def get_permissions(self): pass else: # Domain enforcement and surface limits are tenant boundaries, not authorization - # levels: views that shape their own permission chain cannot opt out of them. + # levels. Views that shape their own permission chain cannot remove them. return [*dangerously_defined, WithinSurfaceLimits(), VerifiedDomainEnforcementPermission()] if isinstance(self.request.successful_authenticator, InternalAPIAuthentication): diff --git a/products/access_control/backend/facade/permissions.py b/products/access_control/backend/facade/permissions.py index 8bd997b66dae..c5efd73ba24b 100644 --- a/products/access_control/backend/facade/permissions.py +++ b/products/access_control/backend/facade/permissions.py @@ -1,10 +1,10 @@ -"""DRF enforcement point for access-control policies owned by this product. +"""DRF enforcement point for the access-control policies this product owns. -`TeamAndOrgViewSetMixin.get_permissions` composes `WithinSurfaceLimits` into every viewset's -stack, so a new endpoint gets surface-limit enforcement without knowing limits exist. DRF evaluates -permission classes with AND semantics: this class is an independent vote and cannot be bypassed by -another class's internal early return (a `*`-scoped token passing `APIScopePermission` is still -limited here). +`TeamAndOrgViewSetMixin.get_permissions` puts `WithinSurfaceLimits` into every viewset's stack. +A new endpoint gets surface-limit enforcement without knowledge that limits exist. DRF combines +permission classes with AND semantics, so this class is an independent vote. Another class's +internal early return cannot bypass it: a `*`-scoped token that passes `APIScopePermission` is +still limited here. """ from typing import Any @@ -17,13 +17,13 @@ class WithinSurfaceLimits(ScopeBasePermission): """Denies actions that exceed the organization's limit for the request's access surface. - Subclasses ScopeBasePermission only for `_get_required_scopes`, so this class derives - an action's read/write nature the same way APIScopePermission does and the two can't + This class subclasses ScopeBasePermission only for `_get_required_scopes`. It derives an + action's read or write nature the same way `APIScopePermission` does, so the two cannot disagree about what counts as a write.""" def has_permission(self, request: Any, view: Any) -> bool: - # Cheap exit first: almost every request has no classified surface, and classification - # is a couple of isinstance checks with no query. + # Cheap exit first: almost every request has no classified surface. Classification + # is two isinstance checks and no query. if classify_surface(request) is None: return True diff --git a/products/access_control/backend/facade/surface_limits.py b/products/access_control/backend/facade/surface_limits.py index 6e0b91f1bde5..294f43874eda 100644 --- a/products/access_control/backend/facade/surface_limits.py +++ b/products/access_control/backend/facade/surface_limits.py @@ -17,16 +17,17 @@ if TYPE_CHECKING: from posthog.models.organization import Organization -# The outbound identity of services/mcp (see its oauth-constants.ts). Surface classification is -# governance of the pathway, not a defense against a hostile key holder: the same credential used -# outside MCP keeps its own scopes, and tightening the credential itself is the mint-time follow-up. +# The outbound identity of services/mcp (see its oauth-constants.ts). Surface classification +# governs the pathway. It is not a defense against a hostile key holder: the same credential +# keeps its own scopes outside MCP. A later change can reduce the credential's scopes at mint +# time. MCP_USER_AGENT_MARKER = "posthog/mcp-server" WRITE_LIMITED_LEVELS = {SurfaceAccessLimit.MaxLevel.NONE, SurfaceAccessLimit.MaxLevel.VIEWER} def classify_surface(request: Any) -> str | None: - """The access surface this request arrived through, or None for surfaces without policies.""" + """The access surface of this request, or None for surfaces without policies.""" authenticator = getattr(request, "successful_authenticator", None) if isinstance(authenticator, PersonalAPIKeyAuthentication | OAuthAccessTokenAuthentication): user_agent = request.headers.get("User-Agent") or "" @@ -38,11 +39,12 @@ def classify_surface(request: Any) -> str | None: def limit_denial_for_request( request: Any, organization: "Organization", resource: str | None, writes: bool ) -> str | None: - """The complete surface-limit decision: a user-facing denial message when this request's - surface is limited below what the action needs, or None to allow. + """The complete surface-limit decision. Returns a user-facing denial message when the + request's surface is limited below what the action needs. Returns None to allow. - Owns classification, the entitlement gate, row lookup and the copy, so enforcement points - (today `WithinSurfaceLimits`, later the facade's `decide()`) contain no policy of their own.""" + This function owns classification, the entitlement gate, the row lookup and the copy. + Enforcement points (`WithinSurfaceLimits` today, the facade's `decide()` later) contain + no policy of their own.""" if not writes: return None surface = classify_surface(request) @@ -62,10 +64,11 @@ def limit_denial_for_request( def surface_limit( organization: "Organization", surface: str | None, resource: str | None = None ) -> SurfaceAccessLimit.MaxLevel | None: - """The max level this organization allows through `surface`, or None when unrestricted. + """The max level this organization allows through `surface`, or None when the surface has + no limit. - A row naming `resource` overrides the wildcard row. One query per call; callers on hot - paths should classify the surface first, since surface=None short-circuits. + A row that names `resource` overrides the wildcard row. Each call makes one query. Callers + on hot paths can classify the surface first, because surface=None returns with no query. """ if surface is None: return None diff --git a/products/access_control/backend/models/surface_access_limit.py b/products/access_control/backend/models/surface_access_limit.py index 938a0a4831b3..dadd66d3b7ce 100644 --- a/products/access_control/backend/models/surface_access_limit.py +++ b/products/access_control/backend/models/surface_access_limit.py @@ -4,24 +4,24 @@ class SurfaceAccessLimit(UUIDModel): - """An organization-wide cap on what any principal can do through one access surface, such as - the MCP server, a personal API key, or a public share link. + """An organization-wide cap on what any principal can do through one access surface. + Example surfaces: the MCP server, a personal API key, a public share link. - Limits are not grants. The grants system (AccessControl rows) answers "what may this - principal do"; a limit answers "how much this surface allows", and the effective access is - the minimum of the two. A limit therefore applies to every member, admins included: - exceptions are future subject-specific limit rows that widen it, never grants. + A limit is not a grant. AccessControl rows answer "what may this principal do". A limit + answers "how much this surface allows". The effective access is the minimum of the two. + A limit applies to every member, including admins. Only a future subject-specific limit + row can widen a limit. A grant cannot. - Absence of a row means the surface is unrestricted. `resource=None` limits every resource; - a row naming a resource overrides the wildcard row for that resource. + No row means the surface has no limit. A row with `resource=None` limits every resource. + A row that names a resource overrides the wildcard row for that resource. """ class Surface(models.TextChoices): MCP = "mcp" class MaxLevel(models.TextChoices): - # The grants vocabulary, minus levels a limit never needs. "none" disables the - # surface; "viewer" makes it read-only. + # The grants vocabulary without the levels a limit never needs. "none" disables + # the surface. "viewer" makes it read-only. NONE = "none" VIEWER = "viewer" EDITOR = "editor" @@ -35,8 +35,8 @@ class Meta: ) ] - # db_constraint=False: posthog_organization is a hot table, and creating a real FK - # constraint takes a lock on it that queues behind live writes. + # db_constraint=False because posthog_organization is a hot table. A real FK + # constraint takes a lock on it, and that lock queues behind live writes. organization = models.ForeignKey( "posthog.Organization", on_delete=models.CASCADE, diff --git a/products/access_control/backend/tests/test_surface_access_limits.py b/products/access_control/backend/tests/test_surface_access_limits.py index 2a86203e0631..4e55edcfcb74 100644 --- a/products/access_control/backend/tests/test_surface_access_limits.py +++ b/products/access_control/backend/tests/test_surface_access_limits.py @@ -29,10 +29,6 @@ def test_no_rows_and_no_surface_mean_unrestricted(self) -> None: class TestMCPReadOnlyEnforcement(APIBaseTest): - """The regression these guard: a write-scoped token arriving through the MCP surface must be - denied when the org limits it, including `*`-scoped tokens, while reads and non-MCP requests - stay untouched. No existing test exercises the surface-limit path at all.""" - def setUp(self) -> None: super().setUp() self.organization.available_product_features = [ From 72f1f05436d63738b168533fd0d147b068069c31 Mon Sep 17 00:00:00 2001 From: Alex Date: Fri, 21 Aug 2026 21:59:33 +0200 Subject: [PATCH 019/231] chore(access-control): say why SurfaceAccessLimit skips CreatedMetaFields Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01LEer2y9uGMkADPjMfWG8WK --- products/access_control/backend/models/surface_access_limit.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/products/access_control/backend/models/surface_access_limit.py b/products/access_control/backend/models/surface_access_limit.py index dadd66d3b7ce..865a5c52b460 100644 --- a/products/access_control/backend/models/surface_access_limit.py +++ b/products/access_control/backend/models/surface_access_limit.py @@ -49,6 +49,8 @@ class Meta: resource: models.CharField = models.CharField(max_length=64, null=True, blank=True) max_level: models.CharField = models.CharField(max_length=32, choices=MaxLevel.choices) + # Not CreatedMetaFields: its created_by FK carries a real DB constraint on posthog_user, + # a hot table, and Django cannot override a field inherited from an abstract base. created_by = models.ForeignKey( "posthog.User", on_delete=models.SET_NULL, From 24129cb4df45023ebc0ef46e82161605541dee69 Mon Sep 17 00:00:00 2001 From: Alex Date: Fri, 21 Aug 2026 22:06:07 +0200 Subject: [PATCH 020/231] feat(access-control): make the all-resources wildcard explicit resource="*" replaces a nullable resource: a null one column away from max_level="none" read as two different nones. Also drops the nulls_distinct special case from the unique constraint. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01LEer2y9uGMkADPjMfWG8WK --- .../backend/facade/permissions.py | 3 ++- .../backend/facade/surface_limits.py | 20 ++++++++----------- .../migrations/0002_surfaceaccesslimit.py | 5 ++--- .../backend/models/surface_access_limit.py | 9 +++++---- .../tests/test_surface_access_limits.py | 3 +-- 5 files changed, 18 insertions(+), 22 deletions(-) diff --git a/products/access_control/backend/facade/permissions.py b/products/access_control/backend/facade/permissions.py index c5efd73ba24b..df219ceb9711 100644 --- a/products/access_control/backend/facade/permissions.py +++ b/products/access_control/backend/facade/permissions.py @@ -12,6 +12,7 @@ from posthog.permissions import ScopeBasePermission, get_organization_from_view from products.access_control.backend.facade.surface_limits import classify_surface, limit_denial_for_request +from products.access_control.backend.models import SurfaceAccessLimit class WithinSurfaceLimits(ScopeBasePermission): @@ -39,7 +40,7 @@ def has_permission(self, request: Any, view: Any) -> bool: denial = limit_denial_for_request( request, organization, - resource=scope_object if scope_object != "INTERNAL" else None, + resource=scope_object if scope_object != "INTERNAL" else SurfaceAccessLimit.ALL_RESOURCES, writes=any(scope.endswith(":write") for scope in required_scopes), ) if denial is not None: diff --git a/products/access_control/backend/facade/surface_limits.py b/products/access_control/backend/facade/surface_limits.py index 294f43874eda..a6c9a2f2d352 100644 --- a/products/access_control/backend/facade/surface_limits.py +++ b/products/access_control/backend/facade/surface_limits.py @@ -36,9 +36,7 @@ def classify_surface(request: Any) -> str | None: return None -def limit_denial_for_request( - request: Any, organization: "Organization", resource: str | None, writes: bool -) -> str | None: +def limit_denial_for_request(request: Any, organization: "Organization", resource: str, writes: bool) -> str | None: """The complete surface-limit decision. Returns a user-facing denial message when the request's surface is limited below what the action needs. Returns None to allow. @@ -62,22 +60,20 @@ def limit_denial_for_request( def surface_limit( - organization: "Organization", surface: str | None, resource: str | None = None + organization: "Organization", surface: str, resource: str = SurfaceAccessLimit.ALL_RESOURCES ) -> SurfaceAccessLimit.MaxLevel | None: """The max level this organization allows through `surface`, or None when the surface has no limit. - A row that names `resource` overrides the wildcard row. Each call makes one query. Callers - on hot paths can classify the surface first, because surface=None returns with no query. + A row that names `resource` overrides the `"*"` wildcard row. Each call makes one query. """ - if surface is None: - return None + wildcard = SurfaceAccessLimit.ALL_RESOURCES rows = SurfaceAccessLimit.objects.filter( - organization=organization, surface=surface, resource__in=[resource, None] if resource else [None] + organization=organization, surface=surface, resource__in={resource, wildcard} ).values_list("resource", "max_level") by_resource = dict(rows) - if resource is not None and resource in by_resource: + if resource in by_resource: return SurfaceAccessLimit.MaxLevel(by_resource[resource]) - if None in by_resource: - return SurfaceAccessLimit.MaxLevel(by_resource[None]) + if wildcard in by_resource: + return SurfaceAccessLimit.MaxLevel(by_resource[wildcard]) return None diff --git a/products/access_control/backend/migrations/0002_surfaceaccesslimit.py b/products/access_control/backend/migrations/0002_surfaceaccesslimit.py index 21976433f85b..66d332d3be6a 100644 --- a/products/access_control/backend/migrations/0002_surfaceaccesslimit.py +++ b/products/access_control/backend/migrations/0002_surfaceaccesslimit.py @@ -1,4 +1,4 @@ -# Generated by Django 5.2.17 on 2026-08-21 19:30 +# Generated by Django 5.2.17 on 2026-08-21 20:05 import django.db.models.deletion from django.conf import settings @@ -28,7 +28,7 @@ class Migration(migrations.Migration): ), ), ("surface", models.CharField(choices=[("mcp", "Mcp")], max_length=32)), - ("resource", models.CharField(blank=True, max_length=64, null=True)), + ("resource", models.CharField(default="*", max_length=64)), ( "max_level", models.CharField( @@ -66,7 +66,6 @@ class Migration(migrations.Migration): models.UniqueConstraint( fields=("organization", "surface", "resource"), name="unique_limit_per_org_surface_resource", - nulls_distinct=False, ) ], }, diff --git a/products/access_control/backend/models/surface_access_limit.py b/products/access_control/backend/models/surface_access_limit.py index 865a5c52b460..875e5dfbf62e 100644 --- a/products/access_control/backend/models/surface_access_limit.py +++ b/products/access_control/backend/models/surface_access_limit.py @@ -12,8 +12,9 @@ class SurfaceAccessLimit(UUIDModel): A limit applies to every member, including admins. Only a future subject-specific limit row can widen a limit. A grant cannot. - No row means the surface has no limit. A row with `resource=None` limits every resource. + No row means the surface has no limit. A row with `resource="*"` limits every resource. A row that names a resource overrides the wildcard row for that resource. + `max_level="none"` removes all access through the surface. """ class Surface(models.TextChoices): @@ -31,7 +32,6 @@ class Meta: models.UniqueConstraint( fields=["organization", "surface", "resource"], name="unique_limit_per_org_surface_resource", - nulls_distinct=False, ) ] @@ -45,8 +45,9 @@ class Meta: ) surface: models.CharField = models.CharField(max_length=32, choices=Surface.choices) - # An APIScopeObject name, or None to limit every resource. - resource: models.CharField = models.CharField(max_length=64, null=True, blank=True) + # An APIScopeObject name, or "*" to limit every resource. + ALL_RESOURCES = "*" + resource: models.CharField = models.CharField(max_length=64, default=ALL_RESOURCES) max_level: models.CharField = models.CharField(max_length=32, choices=MaxLevel.choices) # Not CreatedMetaFields: its created_by FK carries a real DB constraint on posthog_user, diff --git a/products/access_control/backend/tests/test_surface_access_limits.py b/products/access_control/backend/tests/test_surface_access_limits.py index 4e55edcfcb74..086c491709a4 100644 --- a/products/access_control/backend/tests/test_surface_access_limits.py +++ b/products/access_control/backend/tests/test_surface_access_limits.py @@ -23,9 +23,8 @@ def test_resource_row_overrides_wildcard_row(self) -> None: assert surface_limit(self.organization, "mcp", "feature_flag") == SurfaceAccessLimit.MaxLevel.EDITOR assert surface_limit(self.organization, "mcp") == SurfaceAccessLimit.MaxLevel.VIEWER - def test_no_rows_and_no_surface_mean_unrestricted(self) -> None: + def test_no_rows_mean_unrestricted(self) -> None: assert surface_limit(self.organization, "mcp", "dashboard") is None - assert surface_limit(self.organization, None, "dashboard") is None class TestMCPReadOnlyEnforcement(APIBaseTest): From 321e7c4dfa8ea0193785d2c433197c2a7452b9e9 Mon Sep 17 00:00:00 2001 From: Alex Date: Fri, 21 Aug 2026 23:18:16 +0200 Subject: [PATCH 021/231] chore(access-control): finish the simplified-english pass on the facade modules The surface_limits module docstring kept its pre-rewrite wording. Function docstrings now start with a verb, and the user-agent comment loses the mint-time jargon. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01LEer2y9uGMkADPjMfWG8WK --- .../backend/facade/permissions.py | 2 +- .../backend/facade/surface_limits.py | 43 +++++++++++-------- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/products/access_control/backend/facade/permissions.py b/products/access_control/backend/facade/permissions.py index df219ceb9711..0ba93d0c8991 100644 --- a/products/access_control/backend/facade/permissions.py +++ b/products/access_control/backend/facade/permissions.py @@ -1,7 +1,7 @@ """DRF enforcement point for the access-control policies this product owns. `TeamAndOrgViewSetMixin.get_permissions` puts `WithinSurfaceLimits` into every viewset's stack. -A new endpoint gets surface-limit enforcement without knowledge that limits exist. DRF combines +A new endpoint gets surface-limit enforcement automatically. DRF combines permission classes with AND semantics, so this class is an independent vote. Another class's internal early return cannot bypass it: a `*`-scoped token that passes `APIScopePermission` is still limited here. diff --git a/products/access_control/backend/facade/surface_limits.py b/products/access_control/backend/facade/surface_limits.py index a6c9a2f2d352..eeb1181ec2dc 100644 --- a/products/access_control/backend/facade/surface_limits.py +++ b/products/access_control/backend/facade/surface_limits.py @@ -1,10 +1,15 @@ -"""Per-surface access limits: org-wide caps on what any principal can do through one access -surface (the MCP server today; personal API keys, share links and impersonation later). +"""Per-surface access limits. -The first consumer is `WithinSurfaceLimits` in facade/permissions.py, which denies write-scoped -actions when the request's surface is limited below editor. When the access-control facade's `decide()` lands, it composes the same limit into -object-level decisions; both read this module, so enforcement points cannot disagree about what -the organization configured. +An access surface is a path that requests use to reach PostHog. Examples: the MCP server, +a personal API key, a public share link, an impersonated session. A surface limit is an +organization-wide cap on what any principal can do through one surface. The MCP server is +the only surface with limits today. + +The class `WithinSurfaceLimits` in facade/permissions.py is the first consumer. It denies +write actions when the organization limits the request's surface below editor. The +access-control facade's `decide()` will also read this module later, to apply the same +limit to object-level decisions. All enforcement points read one module, so they cannot +disagree about the configured limits. """ from typing import TYPE_CHECKING, Any @@ -17,17 +22,18 @@ if TYPE_CHECKING: from posthog.models.organization import Organization -# The outbound identity of services/mcp (see its oauth-constants.ts). Surface classification -# governs the pathway. It is not a defense against a hostile key holder: the same credential -# keeps its own scopes outside MCP. A later change can reduce the credential's scopes at mint -# time. +# The outbound identity of services/mcp (see its oauth-constants.ts). Surface +# classification controls the pathway. It is not a defense against a hostile key holder. +# The same credential keeps its full scopes outside MCP. A future change can reduce the +# credential's scopes when the token is created. MCP_USER_AGENT_MARKER = "posthog/mcp-server" WRITE_LIMITED_LEVELS = {SurfaceAccessLimit.MaxLevel.NONE, SurfaceAccessLimit.MaxLevel.VIEWER} def classify_surface(request: Any) -> str | None: - """The access surface of this request, or None for surfaces without policies.""" + """Returns the access surface of this request. Returns None for paths that have no + surface policies.""" authenticator = getattr(request, "successful_authenticator", None) if isinstance(authenticator, PersonalAPIKeyAuthentication | OAuthAccessTokenAuthentication): user_agent = request.headers.get("User-Agent") or "" @@ -37,12 +43,13 @@ def classify_surface(request: Any) -> str | None: def limit_denial_for_request(request: Any, organization: "Organization", resource: str, writes: bool) -> str | None: - """The complete surface-limit decision. Returns a user-facing denial message when the - request's surface is limited below what the action needs. Returns None to allow. + """Makes the full surface-limit decision for one request. Returns a denial message for + the user when the organization limits the request's surface below what the action needs. + Returns None to allow the request. - This function owns classification, the entitlement gate, the row lookup and the copy. - Enforcement points (`WithinSurfaceLimits` today, the facade's `decide()` later) contain - no policy of their own.""" + This function contains all the policy: surface classification, the feature-entitlement + check, the row lookup, and the message text. Enforcement points (`WithinSurfaceLimits` + today, the facade's `decide()` later) apply the result and add no policy of their own.""" if not writes: return None surface = classify_surface(request) @@ -62,8 +69,8 @@ def limit_denial_for_request(request: Any, organization: "Organization", resourc def surface_limit( organization: "Organization", surface: str, resource: str = SurfaceAccessLimit.ALL_RESOURCES ) -> SurfaceAccessLimit.MaxLevel | None: - """The max level this organization allows through `surface`, or None when the surface has - no limit. + """Returns the max level this organization allows through `surface`. Returns None when + the surface has no limit. A row that names `resource` overrides the `"*"` wildcard row. Each call makes one query. """ From 0d14d46485ed328ae3a459fe2140b1047ced5e88 Mon Sep 17 00:00:00 2001 From: Alex Date: Fri, 21 Aug 2026 23:19:19 +0200 Subject: [PATCH 022/231] chore(access-control): rename the permission class to SurfaceAccessLimitPermission The repo's stack convention is the Permission suffix, and the exact model-name prefix makes one grep find the storage, the policy and the enforcement together. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01LEer2y9uGMkADPjMfWG8WK --- posthog/api/routing.py | 6 +++--- products/access_control/backend/facade/permissions.py | 4 ++-- products/access_control/backend/facade/surface_limits.py | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/posthog/api/routing.py b/posthog/api/routing.py index a1a8692a8c8f..591d7268e0e5 100644 --- a/posthog/api/routing.py +++ b/posthog/api/routing.py @@ -42,7 +42,7 @@ from posthog.scopes import APIScopeObjectOrNotSupported from posthog.user_permissions import UserPermissions -from products.access_control.backend.facade.permissions import WithinSurfaceLimits +from products.access_control.backend.facade.permissions import SurfaceAccessLimitPermission if TYPE_CHECKING: _GenericViewSet = GenericViewSet @@ -256,7 +256,7 @@ def get_permissions(self): else: # Domain enforcement and surface limits are tenant boundaries, not authorization # levels. Views that shape their own permission chain cannot remove them. - return [*dangerously_defined, WithinSurfaceLimits(), VerifiedDomainEnforcementPermission()] + return [*dangerously_defined, SurfaceAccessLimitPermission(), VerifiedDomainEnforcementPermission()] if isinstance(self.request.successful_authenticator, InternalAPIAuthentication): return [IsAuthenticated()] @@ -272,7 +272,7 @@ def get_permissions(self): permission_classes: list = [ IsAuthenticated, APIScopePermission, - WithinSurfaceLimits, + SurfaceAccessLimitPermission, AccessControlPermission, ] diff --git a/products/access_control/backend/facade/permissions.py b/products/access_control/backend/facade/permissions.py index 0ba93d0c8991..bd1c9357c573 100644 --- a/products/access_control/backend/facade/permissions.py +++ b/products/access_control/backend/facade/permissions.py @@ -1,6 +1,6 @@ """DRF enforcement point for the access-control policies this product owns. -`TeamAndOrgViewSetMixin.get_permissions` puts `WithinSurfaceLimits` into every viewset's stack. +`TeamAndOrgViewSetMixin.get_permissions` puts `SurfaceAccessLimitPermission` into every viewset's stack. A new endpoint gets surface-limit enforcement automatically. DRF combines permission classes with AND semantics, so this class is an independent vote. Another class's internal early return cannot bypass it: a `*`-scoped token that passes `APIScopePermission` is @@ -15,7 +15,7 @@ from products.access_control.backend.models import SurfaceAccessLimit -class WithinSurfaceLimits(ScopeBasePermission): +class SurfaceAccessLimitPermission(ScopeBasePermission): """Denies actions that exceed the organization's limit for the request's access surface. This class subclasses ScopeBasePermission only for `_get_required_scopes`. It derives an diff --git a/products/access_control/backend/facade/surface_limits.py b/products/access_control/backend/facade/surface_limits.py index eeb1181ec2dc..681ed847dc9c 100644 --- a/products/access_control/backend/facade/surface_limits.py +++ b/products/access_control/backend/facade/surface_limits.py @@ -5,7 +5,7 @@ organization-wide cap on what any principal can do through one surface. The MCP server is the only surface with limits today. -The class `WithinSurfaceLimits` in facade/permissions.py is the first consumer. It denies +The class `SurfaceAccessLimitPermission` in facade/permissions.py is the first consumer. It denies write actions when the organization limits the request's surface below editor. The access-control facade's `decide()` will also read this module later, to apply the same limit to object-level decisions. All enforcement points read one module, so they cannot @@ -48,7 +48,7 @@ def limit_denial_for_request(request: Any, organization: "Organization", resourc Returns None to allow the request. This function contains all the policy: surface classification, the feature-entitlement - check, the row lookup, and the message text. Enforcement points (`WithinSurfaceLimits` + check, the row lookup, and the message text. Enforcement points (`SurfaceAccessLimitPermission` today, the facade's `decide()` later) apply the result and add no policy of their own.""" if not writes: return None From 6597f141d0e81f116163a02f3465794d6f8ded55 Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Fri, 21 Aug 2026 21:16:16 +0100 Subject: [PATCH 023/231] fix(dev): deny git-config and hook writes from the dev sandbox Sandboxed dependency code could set core.hooksPath or core.fsmonitor in a git config file and have that command run later in the developer's own terminal, outside Seatbelt. Close the write routes that reach it. The profile denies the repo's git config files, the .git node, Homebrew's system gitconfig and the directory holding it, .husky, .flox/env, and the main repo's bin/. The wrapper resolves at render time the physical paths a worktree or a symlinked .git needs, since Seatbelt matches physical paths, and adds the git dir's commondir, which repoints git at another common dir carrying its own config and hooks. One shared predicate decides whether a path is safe to render inside an SBPL literal. The wrapper pins its own PATH to the system directories before the sandbox exists and runs /usr/bin/sandbox-exec by absolute path, so sandboxed code cannot plant a binary that the wrapper then executes unsandboxed. core.hooksPath and blame.ignoreRevsFile are seeded from the flox activation, which runs unsandboxed. husky reports success even when its git config write fails, so without the seed a fresh clone would install no hooks and say nothing. The seed reads the local scope, so a global core.hooksPath does not suppress it. .husky/_/husky.sh stays writable because husky rewrites it on every install and throws if it cannot; the checked-in hooks are protected, and the self-test pins that trade-off so closing it cannot break installs silently. Sibling worktrees and [include]-chained config files stay open, tracked in #79300. Generated-By: PostHog Desktop Task-Id: 7ad0d5f8-020a-4917-8108-a1d66b8ac2c2 --- .flox/env/on-activate.sh | 17 ++++ bin/dev-sandbox | 118 +++++++++++++++++++------ bin/dev-sandbox-selftest | 181 +++++++++++++++++++++++++++++++++++++-- bin/dev-sandbox.sb | 67 +++++++++++++++ 4 files changed, 349 insertions(+), 34 deletions(-) diff --git a/.flox/env/on-activate.sh b/.flox/env/on-activate.sh index e88fadbc1e1d..f0aec9ae4144 100755 --- a/.flox/env/on-activate.sh +++ b/.flox/env/on-activate.sh @@ -312,6 +312,23 @@ _UV_SKIP=0 _PHROCS_SKIP=0 [[ -n "$_PHROCS_BAKED" && -n "$_PHROCS_CURRENT" && "$_PHROCS_BAKED" == "$_PHROCS_CURRENT" ]] && _PHROCS_SKIP=1 +# Seed repo-local git settings here, because package.json's postinstall runs inside +# the sandbox below, which write-denies .git/config. The postinstall still tries +# blame.ignoreRevsFile for clones that never activate flox (.claude/hooks/setup-cloud.sh +# and friends); under the sandbox that attempt no-ops and this one is what lands. +# Idempotent — the --get short-circuits once the value is set. +git -C "$FLOX_ENV_PROJECT" config --get blame.ignoreRevsFile >/dev/null 2>&1 || + git -C "$FLOX_ENV_PROJECT" config blame.ignoreRevsFile .git-blame-ignore-revs >/dev/null 2>&1 || + true +# Same for husky's core.hooksPath, which `prepare` sets during the sandboxed pnpm +# install below. husky checks only whether git spawned, not how it exited, so the +# denied write leaves a fresh clone with no hooks and an install that claims success. +# --local, not --get: a global core.hooksPath would satisfy a merged --get and skip +# the seed, leaving the repo pointed at the developer's global hooks dir instead. +git -C "$FLOX_ENV_PROJECT" config --local --get core.hooksPath >/dev/null 2>&1 || + git -C "$FLOX_ENV_PROJECT" config core.hooksPath .husky >/dev/null 2>&1 || + true + # Sandbox the automatic installs below by default on macOS (opt out with # POSTHOG_DEV_SANDBOX=0). .env.local isn't loaded at flox-activate time, so check # it directly — but only when the live env is unset, so shell env keeps precedence. diff --git a/bin/dev-sandbox b/bin/dev-sandbox index cea3b9fb4795..208d339aca96 100755 --- a/bin/dev-sandbox +++ b/bin/dev-sandbox @@ -22,13 +22,25 @@ set -euo pipefail +# Everything below runs BEFORE the sandbox exists, with the developer's full +# privileges. The inherited $PATH is not trustworthy at that point: it keeps the +# repo's own node_modules/.bin (see the PATH sanitizing near the end, which +# deliberately preserves repo bins for the child), and sandboxed dependency code +# can write there. A bare `sed`/`mktemp` would then resolve to a binary a previous +# sandboxed run planted, and run unsandboxed as the developer. So pin PATH to the +# system directories for this script's own use and hand the inherited one to the +# child, which is the only consumer that needs it. +inherited_path="$PATH" +PATH="/usr/bin:/bin:/usr/sbin:/sbin" + cmd="${1:?bin/dev-sandbox: expected a command string as the first argument}" project="$(cd "$(dirname "$0")/.." && pwd -P)" # canonical repo root (Seatbelt matches physical paths) base_profile="$project/bin/dev-sandbox.sb" -run_unsandboxed() { exec /bin/bash -c "$cmd"; } +# Passthrough runs the developer's own command, so it gets the real PATH back. +run_unsandboxed() { PATH="$inherited_path" exec /bin/bash -c "$cmd"; } -if [[ "$(uname -s)" != "Darwin" ]] || ! command -v sandbox-exec >/dev/null 2>&1 || [[ ! -f "$base_profile" ]]; then +if [[ "$(uname -s)" != "Darwin" ]] || [[ ! -x /usr/bin/sandbox-exec ]] || [[ ! -f "$base_profile" ]]; then run_unsandboxed fi @@ -53,6 +65,23 @@ fi # the physical path Seatbelt enforces against (/var/folders -> /private/var/...). tmpdir="$(cd "${TMPDIR:-/tmp}" 2>/dev/null && pwd -P || echo "${TMPDIR:-/tmp}")" +# Held in variables because matching a literal backslash in a `case` pattern +# otherwise needs a shellcheck pragma (SC1003). +_dq='"' _bs=$'\\' + +# Can this path be rendered inside an SBPL (literal "…")? One owner, so every +# emitter agrees on the answer. Quotes and backslashes are the cases that matter, +# for two different reasons: a double quote closes the literal early and fails the +# whole profile to parse (loud, but the wrapper then runs unsandboxed), while a +# backslash is eaten as an SBPL string escape, so the deny silently binds to a +# different path than intended and the real one stays writable. Tabs and newlines +# are legal in filenames and render correctly, so they are not rejected. Empty means +# a cd/pwd -P failed upstream, leaving nothing to protect. +unrenderable_path() { # $1 = path + case "$1" in "" | *"$_dq"* | *"$_bs"*) return 0 ;; esac + return 1 +} + # The base profile allows the git config files by literal path, but Seatbelt # matches physical paths — when one is a symlink (dotfile managers), the read of # its target is still denied and cargo/libgit2 fails hard ("failed to stat @@ -65,10 +94,53 @@ for _cfg in "$HOME/.gitconfig" "$HOME/.gitconfig.local" "$HOME/.config/git/confi [[ -n "$_resolved" && "$_resolved" != "$_cfg" ]] && gitcfg_targets+=("$_resolved") done -cache_key="$(printf '%s\0' "$project" "$mainrepo" "$HOME" "${SSH_AUTH_SOCK:-}" ${gitcfg_targets[@]+"${gitcfg_targets[@]}"} | /usr/bin/shasum -a256 | cut -c1-16)" +# The profile write-denies the repo's own git config by PROJECT/MAINREPO literal, +# which reaches neither a worktree's real gitdir (it lives under the MAIN repo, at +# .git/worktrees//) nor a checkout whose .git is a symlink, since Seatbelt +# matches physical paths. Collect both config files there so the render can deny +# them too. Write-deny only: reads are already allowed via the MAINREPO subpath. +# $mainrepo is $project unless this is a worktree, where $project/.git is the +# redirect file (nothing to deny inside it) and $gitdir is the real git dir. +# commondir belongs to this surface too — git resolves it to the common git dir and +# then reads that directory's config AND hooks/, so rewriting it reaches the escape +# the .git/hooks denies close off. It is not worktree-only: git honors a commondir +# in any git dir, including a plain clone's. +# Only this checkout's git dirs are covered — the main repo's and, in a worktree, +# this worktree's own. Sibling worktrees of the same main repo have the same exposure +# but reaching them means globbing .git/worktrees/, a directory sandboxed code can +# write to, which is an unbounded input to the profile; and their working trees are +# writable through $HOME regardless. Both belong to the general fix in #79300, not to +# a hand-enumerated list here. So this loop stays bounded: at most two git dirs. +gitcfg_wdeny=() +for _gd in "$mainrepo/.git" ${gitdir:+"$gitdir"}; do + [[ -d "$_gd" ]] || continue + _raw="$_gd" + _gd="$(cd "$_gd" 2>/dev/null && pwd -P || true)" # physical path: Seatbelt matches that + # Dropping the whole render over one git dir would leave the developer with no + # sandbox at all, so skip just this deny — but say so, since the config it would + # have protected stays writable. + if unrenderable_path "$_gd"; then + echo "⚠️ dev sandbox: cannot deny writes to $_raw/config (path unresolvable or unrenderable)" >&2 + continue + fi + gitcfg_wdeny+=("$_gd/config" "$_gd/config.worktree" "$_gd/commondir") +done + +cache_key="$(printf '%s\0' "$project" "$mainrepo" "$HOME" "${SSH_AUTH_SOCK:-}" ${gitcfg_targets[@]+"${gitcfg_targets[@]}"} "--" ${gitcfg_wdeny[@]+"${gitcfg_wdeny[@]}"} | /usr/bin/shasum -a256 | cut -c1-16)" rendered="$tmpdir/posthog-dev-sandbox-$cache_key.sb" stamp="$rendered.ok" # touched once the rendered profile passes the init preflight +# One SBPL rule over N path literals. A rule with no filters matches EVERYTHING — +# a bare `(deny file-write*)` denies every write — so an empty list emits nothing. +emit_rule() { # emit_rule ... + local op="$1" + shift + [[ $# -gt 0 ]] || return 0 + printf '(%s' "$op" + printf ' (literal "%s")' "$@" + printf ')\n' +} + # Render = base profile + ancestor read-allows ($HOME down to each repo root, so # the OS/node/getcwd can lstat+readdir into the repo) + the SSH-agent socket deny. # Parameter expansion (no dirname forks) since this is on the per-service path. @@ -77,7 +149,6 @@ stamp="$rendered.ok" # touched once the rendered profile passes the init preflig render_profile() { local -a ancestors=() local r p _a tmp sock_dir - local _dq='"' _bs=$'\\' for r in "$project" "$mainrepo"; do p="${r%/*}" while [[ "$p" != "$HOME" && "$p" != "/" && -n "$p" ]]; do @@ -86,32 +157,26 @@ render_profile() { done done ancestors+=("$HOME") - # Paths are emitted as SBPL string literals; a " or \ would corrupt the - # profile (quoted expansions match literally, dodging glob/escape ambiguity). + # These paths carry the read-allows the stack needs, so unlike a single git-dir + # deny they cannot be dropped individually — fail the render and run without a + # sandbox, loudly, rather than emit a profile missing part of what it grants. for _a in "${ancestors[@]}" "$rendered" "$stamp" ${gitcfg_targets[@]+"${gitcfg_targets[@]}"}; do - case "$_a" in - *"$_dq"* | *"$_bs"*) - echo "⚠️ dev sandbox: path contains a quote/backslash, running WITHOUT sandbox: $_a" >&2 + if unrenderable_path "$_a"; then + echo "⚠️ dev sandbox: path is unrenderable, running WITHOUT sandbox: $_a" >&2 return 1 - ;; - esac + fi done tmp="$(mktemp "$tmpdir/posthog-dev-sandbox.XXXXXX")" || return 1 if { cat "$base_profile" - printf '(allow file-read*' - printf ' (literal "%s")' "${ancestors[@]}" - printf ')\n' + emit_rule 'allow file-read*' "${ancestors[@]}" # Physical targets of symlinked git configs (resolved above): readable like # the configs themselves, write-denied for the same hooksPath escape reason. - if [[ ${#gitcfg_targets[@]} -gt 0 ]]; then - printf '(allow file-read*' - printf ' (literal "%s")' "${gitcfg_targets[@]}" - printf ')\n' - printf '(deny file-write*' - printf ' (literal "%s")' "${gitcfg_targets[@]}" - printf ')\n' - fi + emit_rule 'allow file-read*' ${gitcfg_targets[@]+"${gitcfg_targets[@]}"} + emit_rule 'deny file-write*' ${gitcfg_targets[@]+"${gitcfg_targets[@]}"} + # Physical git-dir config paths (collected above), which the profile's + # PROJECT/MAINREPO literals miss for a worktree or a symlinked .git. + emit_rule 'deny file-write*' ${gitcfg_wdeny[@]+"${gitcfg_wdeny[@]}"} # Deny the SSH agent socket (path varies per agent: Secretive, 1Password, # launchd, gpg). Canonicalize its dir — Seatbelt matches the kernel's # physical path (e.g. /tmp -> /private/tmp); fall back to the raw path. @@ -128,7 +193,7 @@ render_profile() { # them would bypass the sandbox on the next cache hit, which re-runs neither # render nor preflight. The legit render here runs outside the sandbox, so # it is unaffected. Literals are canonical (TMPDIR resolved above). - printf '(deny file-write* (literal "%s") (literal "%s"))\n' "$rendered" "$stamp" + emit_rule 'deny file-write*' "$rendered" "$stamp" } >"$tmp" 2>/dev/null && mv -f "$tmp" "$rendered"; then rm -f "$stamp" # new contents: force a fresh init preflight return 0 @@ -144,7 +209,10 @@ if [[ ! -f "$rendered" || "$base_profile" -nt "$rendered" || "$project/bin/dev-s render_profile || run_unsandboxed fi -sandbox=(sandbox-exec -f "$rendered" -D HOME="$HOME" -D PROJECT="$project" -D MAINREPO="$mainrepo") +# Absolute path: the final exec below hands the child's PATH to `env`, and that PATH +# deliberately keeps the repo's own writable bins — so a bare name here would let +# sandboxed code plant the very binary that creates the sandbox. +sandbox=(/usr/bin/sandbox-exec -f "$rendered" -D HOME="$HOME" -D PROJECT="$project" -D MAINREPO="$mainrepo") # Init preflight, once per rendered profile (the stamp records that it passed). # Fail OPEN if it can't initialize (e.g. the profile won't compile on this OS). @@ -172,7 +240,7 @@ sandboxed_path="" _oldifs="$IFS" set -f # PATH entries are literal paths; never glob-expand them IFS=':' -for _pdir in $PATH; do +for _pdir in $inherited_path; do # the developer's real PATH, not this script's pinned one case "$_pdir" in "$project"/* | "$mainrepo"/*) ;; # repo bins (node_modules/.bin) — keep "$HOME"/* | "") continue ;; # read-denied $HOME shim dir / empty — drop diff --git a/bin/dev-sandbox-selftest b/bin/dev-sandbox-selftest index adf490367ca2..783a5997867f 100755 --- a/bin/dev-sandbox-selftest +++ b/bin/dev-sandbox-selftest @@ -15,12 +15,18 @@ set -uo pipefail # intentionally not -e: run every check, then report cd "$(dirname "$0")/.." || exit 1 SANDBOX="bin/dev-sandbox" +repo="$(pwd -P)" # canonical repo root, as Seatbelt sees it fail=0 pass() { printf ' \033[32mok\033[0m %s\n' "$1"; } die() { printf ' \033[31mFAIL\033[0m %s\n' "$1" fail=1 } +# The checks below build throwaway repos at several different points. One trap owns +# all of them, so an interrupt mid-run doesn't strand a git worktree in TMPDIR. +_tmpdirs=() +cleanup_tmpdirs() { [[ ${#_tmpdirs[@]} -eq 0 ]] || rm -rf "${_tmpdirs[@]}"; } +trap cleanup_tmpdirs EXIT if [[ "$(uname -s)" != "Darwin" ]] || ! command -v sandbox-exec >/dev/null 2>&1; then echo "Sandbox is a no-op on this platform — nothing to test." @@ -161,13 +167,19 @@ fi # 11. Execute-later write targets are read-only inside the sandbox (poisoning # them would run code OUTSIDE the sandbox). Probe files only — never real ones. -check_write_blocked() { - if $SANDBOX "echo x > '$1'" >/dev/null 2>&1; then +# The probes differ in how they attempt the write; they share this verdict. +# Returns non-zero when the write got through, so a caller can clean up after +# whatever its probe just created. +report_write_blocked() { # $1 = probe exit status, $2 = label + if [[ "$1" -eq 0 ]]; then die "writable, should be blocked: $2" - rm -f "$1" - else - pass "write blocked: $2" + return 1 fi + pass "write blocked: $2" +} +check_write_blocked() { + $SANDBOX "echo x > '$1'" >/dev/null 2>&1 + report_write_blocked $? "$2" || rm -f "$1" } check_write_blocked "bin/.dev_sandbox_selftest_probe" "repo bin/" mkdir -p .git/hooks 2>/dev/null || true @@ -175,18 +187,150 @@ check_write_blocked ".git/hooks/.dev_sandbox_selftest_probe" ".git/hooks" if [[ -d "$HOME/.cargo/bin" ]]; then check_write_blocked "$HOME/.cargo/bin/.dev_sandbox_selftest_probe" "cargo bin" fi +# For files that really exist and must not be damaged by the probe: opens for +# append and writes nothing, so Seatbelt still has to decide on write access while +# a missing deny cannot corrupt the file. +check_write_blocked_existing() { + $SANDBOX ": >> '$1'" >/dev/null 2>&1 + report_write_blocked $? "$2" +} +# Repo-local git config reaches the same escape as .git/hooks above: core.hooksPath +# moves the hook directory elsewhere, and core.fsmonitor is a command git runs on +# ordinary read commands. --git-common-dir resolves to the main repo's .git in a +# worktree, which is the file git actually reads there. +gitcommon="$(git rev-parse --path-format=absolute --git-common-dir 2>/dev/null || true)" +# Absolute, but not symlink-resolved — Seatbelt matches the physical path. +[[ -n "$gitcommon" ]] && gitcommon="$(cd "$gitcommon" 2>/dev/null && pwd -P || true)" +if [[ -n "$gitcommon" && -f "$gitcommon/config" ]]; then + check_write_blocked_existing "$gitcommon/config" "repo .git/config (core.hooksPath / core.fsmonitor escape)" +else + echo " -- repo .git/config not found, skipping" +fi +# The scripts that actually run outside the sandbox. core.hooksPath points git at +# .husky, so the .git/hooks deny above guards a directory git never reads once that +# is set; these are the real hooks. .flox/env holds the activation scripts, and +# on-activate.sh is what builds this sandbox in the first place. +# .husky/_/husky.sh is deliberately absent from this list — husky's installer +# rewrites it on every sandboxed install, so the profile re-allows it. See the note +# in bin/dev-sandbox.sb. +for unsandboxed_exec in .husky/pre-commit .husky/pre-push .husky/post-checkout .flox/env/on-activate.sh; do + if [[ ! -f "$repo/$unsandboxed_exec" ]]; then + echo " -- $unsandboxed_exec absent, skipping" + continue + fi + check_write_blocked_existing "$repo/$unsandboxed_exec" "$unsandboxed_exec (runs unsandboxed)" +done +# Counterpart to the .husky checks above, so the suite does not read as if husky +# hooks were fully sealed. The checked-in hooks each source .husky/_/husky.sh, and +# that file stays writable BY DESIGN — husky's installer rewrites it on every +# sandboxed install and throws if it cannot. So the .husky denies stop tampering +# with the hook files, not execution through the sourced helper. This asserts the +# door is knowingly open; closing it (and breaking pnpm install) turns this red. +# See bin/dev-sandbox.sb and the follow-up in #79300. +if [[ -f "$repo/.husky/_/husky.sh" ]]; then + if $SANDBOX ": >> '$repo/.husky/_/husky.sh'" >/dev/null 2>&1; then + pass "known-open: .husky/_/husky.sh writable by design (husky reinstalls it)" + else + die ".husky/_/husky.sh is denied — this breaks the sandboxed pnpm install (husky throws)" + fi +else + echo " -- .husky/_/husky.sh absent, skipping known-open check" +fi +# Homebrew's system-scope gitconfig is owned by the developer, not root, and a brew +# git reads it on every invocation — the same escape from a file outside $HOME. +for sysgitcfg in /opt/homebrew/etc/gitconfig /usr/local/etc/gitconfig; do + if [[ ! -f "$sysgitcfg" ]]; then + echo " -- $sysgitcfg absent, skipping" + continue + fi + check_write_blocked_existing "$sysgitcfg" "system gitconfig ($sysgitcfg)" +done +# The .git node itself: in a worktree it is a file holding `gitdir: `, so +# rewriting it repoints git at an attacker-controlled directory that brings its own +# config. Build a throwaway worktree rather than probing this checkout's own .git — +# appending to a directory fails on its own, so on a plain clone that probe would +# report "blocked" without the sandbox having anything to do with it. +wt_root="$(cd "$(mktemp -d)" && pwd -P)" +_tmpdirs+=("$wt_root") +# Runs the throwaway worktree's own copy of the wrapper, so the deny under test is +# the one that copy renders. The cd matters: from the outer repo the child's cwd is +# read-denied, and a getcwd failure would look like the write being blocked. +wt_write_blocked() { # $1 = shell write expression, $2 = label + (cd "$wt_root/wt" && TMPDIR="$wt_root/tmp" ./bin/dev-sandbox "$1") >/dev/null 2>&1 + report_write_blocked $? "$2" +} +# The counterpart: a write the dev stack depends on, which must NOT be blocked. +# Every other check here asserts something is refused, so an over-broad deny would +# land green — this is what catches one. +wt_write_allowed() { # $1 = shell write expression, $2 = label + if (cd "$wt_root/wt" && TMPDIR="$wt_root/tmp" ./bin/dev-sandbox "$1") >/dev/null 2>&1; then + pass "write allowed: $2" + else + die "write blocked, should be allowed: $2" + fi +} +if git init -q "$wt_root/origin" 2>/dev/null && + git -C "$wt_root/origin" -c user.email=t@example.com -c user.name=t -c commit.gpgsign=false \ + commit -q --allow-empty -m init 2>/dev/null && + git -C "$wt_root/origin" worktree add -q "$wt_root/wt" 2>/dev/null && + [[ -f "$wt_root/wt/.git" ]]; then + mkdir -p "$wt_root/wt/bin" "$wt_root/tmp" + cp -p "$repo/bin/dev-sandbox" "$repo/bin/dev-sandbox.sb" "$wt_root/wt/bin/" + wt_write_blocked ": >> .git" "worktree .git redirect file (gitdir redirect escape)" + # The worktree's real git dir lives under the MAIN repo, which no + # PROJECT-relative rule reaches; the wrapper denies it at runtime. + wt_gitdir="$(cd "$wt_root/wt" && git rev-parse --path-format=absolute --git-dir 2>/dev/null || true)" + if [[ -n "$wt_gitdir" ]]; then + wt_write_blocked "echo x > '$wt_gitdir/config.worktree'" "per-worktree config.worktree" + # commondir repoints git at another common git dir, which brings its own + # config AND hooks/ — so it reaches the escape from a file the config denies + # do not name. Append-only: the real one holds "../.." and must keep working. + wt_write_blocked ": >> '$wt_gitdir/commondir'" "per-worktree commondir (common-dir repoint escape)" + fi + # Positive control: the .git node deny is a literal, not a subpath, so ordinary + # entries inside a real .git directory must stay writable or the dev stack breaks. + wt_write_allowed "echo x > '$wt_root/origin/.git/DEV_SANDBOX_PROBE'" \ + "entry inside a real .git dir (the deny is the node, not a subpath)" +else + echo " -- could not build a throwaway worktree, skipping .git redirect checks" +fi +# A checkout whose .git is a symlink to a git dir elsewhere, which is what dotfile +# managers produce. Seatbelt matches the physical path, so the profile's +# PROJECT/.git/config literal cannot fire here and the wrapper's runtime +# canonicalization is the only thing denying the write. Without it this passes +# silently while the developer's config stays writable. +sym_root="$(cd "$(mktemp -d)" && pwd -P)" +_tmpdirs+=("$sym_root") +if git init -q "$sym_root/repo" 2>/dev/null && + mv "$sym_root/repo/.git" "$sym_root/realgit" 2>/dev/null && + ln -s "$sym_root/realgit" "$sym_root/repo/.git" 2>/dev/null; then + mkdir -p "$sym_root/repo/bin" "$sym_root/tmp" + cp -p "$repo/bin/dev-sandbox" "$repo/bin/dev-sandbox.sb" "$sym_root/repo/bin/" + (cd "$sym_root/repo" && TMPDIR="$sym_root/tmp" ./bin/dev-sandbox ": >> '$sym_root/realgit/config'") >/dev/null 2>&1 + report_write_blocked $? "symlinked .git, config at the physical target" +else + echo " -- could not build a symlinked-.git checkout, skipping" +fi # 12. The rendered profile + its preflight stamp are read-only inside the sandbox. # Otherwise a dependency could overwrite them and have a poisoned profile # loaded on the next cache hit (which re-runs neither render nor preflight), -# bypassing the sandbox entirely. Earlier checks have already rendered it. -rendered_profiles=("${TMPDIR:-/tmp}"/posthog-dev-sandbox-*.sb) +# bypassing the sandbox entirely. +# A profile write-denies only itself, so render into a private TMPDIR: the only +# profile there is the one these checks run under. Globbing the shared TMPDIR +# would also turn up profiles left by other worktrees — writable by this sandbox, +# and so read as a failure — and clearing those would evict their live cache. +prof_tmp="$(cd "$(mktemp -d)" && pwd -P)" +_tmpdirs+=("$prof_tmp") +export TMPDIR="$prof_tmp" # check 13 below reuses this same private profile +$SANDBOX 'true' >/dev/null 2>&1 +rendered_profiles=("$prof_tmp"/posthog-dev-sandbox-*.sb) rendered_profile="${rendered_profiles[0]}" # literal glob if none matched -> -f false below if [[ -f "$rendered_profile" ]]; then check_write_blocked "$rendered_profile" "rendered profile (cache poisoning)" check_write_blocked "$rendered_profile.ok" "rendered profile stamp (cache poisoning)" else - die "rendered profile not found (expected after earlier sandbox runs)" + die "rendered profile not found in $prof_tmp (expected from the render just above)" fi # 13. PATH is sanitized: read-denied $HOME dirs are dropped from the child PATH so a @@ -196,7 +340,6 @@ fi # ahead of the real binary breaks every bare spawn (e.g. pnpm lifecycle scripts). # Assert a $HOME dir is dropped while system + repo bins survive. String-only, so # the probe dir need not exist. -repo="$(pwd -P)" shim="$HOME/.dev_sandbox_selftest_shim/bin" child_path="$(PATH="$shim:/usr/bin:/bin:$repo/bin" $SANDBOX 'printf %s "$PATH"' 2>/dev/null)" if [[ ":$child_path:" == *":$shim:"* ]]; then @@ -207,6 +350,26 @@ else pass "PATH sanitized: denied \$HOME dir dropped, system + repo bins kept" fi +# 14. The passthrough hands the caller's real PATH back. On Linux that is the +# wrapper's entire job (there is no Seatbelt), and this suite exits before here +# on non-Darwin — so reach the same branch on macOS by running a copy of the +# wrapper with no profile beside it. If the restore breaks, every sandboxed +# service on a Linux dev box starts with only the four pinned system dirs. +pass_dir="$(cd "$(mktemp -d)" && pwd -P)" +_tmpdirs+=("$pass_dir") +mkdir -p "$pass_dir/bin" +cp -p "$repo/bin/dev-sandbox" "$pass_dir/bin/" # no dev-sandbox.sb -> passthrough branch +# $HOME-rooted so the marker also fails the sandboxed branch's PATH sanitizer: if +# passthrough ever handed back the pinned/sanitized PATH instead of the caller's, +# a system-dir marker would survive and hide the regression, but this one is dropped. +marker="$HOME/.dev_sandbox_selftest_pathmarker" # string-only, need not exist +child_path="$(PATH="$marker:$PATH" "$pass_dir/bin/dev-sandbox" 'printf %s "$PATH"' 2>/dev/null)" +if [[ ":$child_path:" == *":$marker:"* ]]; then + pass "passthrough restores the caller's PATH" +else + die "passthrough PATH left pinned to system dirs (got: $child_path)" +fi + echo if [[ $fail -eq 0 ]]; then echo "PASS — all dev-sandbox self-tests passed" diff --git a/bin/dev-sandbox.sb b/bin/dev-sandbox.sb index ee3ce3b76a46..99ffc5396d49 100644 --- a/bin/dev-sandbox.sb +++ b/bin/dev-sandbox.sb @@ -110,9 +110,76 @@ ;; Execute-later targets that run OUTSIDE the sandbox if poisoned. The repo ;; is otherwise writable (build output, caches); these specific paths are not. (subpath (string-append (param "PROJECT") "/bin")) ; sandbox files + scripts (bin/wait-for-docker runs unsandboxed) + (subpath (string-append (param "MAINREPO") "/bin")) ; worktree main-repo scripts — same execute-later route + ;; The flox activation scripts. They run unsandboxed on every `flox activate`, + ;; and on-activate.sh is what builds this sandbox — poisoning it is a shortcut + ;; past everything else here. Only .flox/env is denied: run/, cache/ and log/ + ;; are generated and stay writable. + (subpath (string-append (param "PROJECT") "/.flox/env")) + (subpath (string-append (param "MAINREPO") "/.flox/env")) + ;; The husky hooks. core.hooksPath points git at .husky, so these are the hooks + ;; that actually run — .git/hooks above is the directory git never reads once + ;; that is set. Same unsandboxed execution, one step more direct. + (subpath (string-append (param "PROJECT") "/.husky")) + (subpath (string-append (param "MAINREPO") "/.husky")) (subpath (string-append (param "PROJECT") "/.git/hooks")) ; git hooks run unsandboxed (subpath (string-append (param "MAINREPO") "/.git/hooks")) ; worktree main-repo hooks + ;; Repo-local git config, which reaches the same escape as the hook denies above + ;; by another route: core.hooksPath relocates the hook directory they protect, + ;; and core.fsmonitor is a command git runs on ordinary read commands. Either one + ;; executes in the developer's own terminal, outside Seatbelt. config.worktree is + ;; the same file for a repo with extensions.worktreeConfig set. + ;; The repo seeds its own entries here (blame.ignoreRevsFile, and core.hooksPath + ;; for husky) from the flox activation, which runs unsandboxed. package.json's + ;; postinstall still attempts blame.ignoreRevsFile, for clones that never activate + ;; flox; sandboxed, that attempt no-ops against these denies and the activation is + ;; what lands. cargo's git-dependency writes go to ~/.cargo/git/db instead. + ;; husky's `prepare` still retries the core.hooksPath write on every sandboxed + ;; install and prints git's "Operation not permitted"; that is noise, because the + ;; activation already seeded the value. Do not relax these to silence it — husky + ;; ignores git's exit status, so the deny would fail silently rather than loudly. + ;; bin/dev-sandbox appends more of these at render time: a worktree's real gitdir + ;; lives under MAINREPO/.git/worktrees//, sibling worktrees have their own, + ;; and a symlinked .git resolves elsewhere — Seatbelt matches physical paths. + ;; Those runtime denies are not redundant with the literals below. + (literal (string-append (param "PROJECT") "/.git/config")) + (literal (string-append (param "PROJECT") "/.git/config.worktree")) + (literal (string-append (param "MAINREPO") "/.git/config")) + (literal (string-append (param "MAINREPO") "/.git/config.worktree")) + ;; The .git node itself. In a worktree it is a file holding `gitdir: `, and + ;; rewriting it repoints git at a directory the attacker controls, which brings + ;; its own config and so the same escape. Denying the literal does not affect + ;; entries inside a real .git directory — those are their own paths, and the dev + ;; stack writes them freely. + (literal (string-append (param "PROJECT") "/.git")) + (literal (string-append (param "MAINREPO") "/.git")) (literal (string-append (param "HOME") "/.gitconfig")) ; core.hooksPath / credential.helper = !cmd (literal (string-append (param "HOME") "/.gitconfig.local")) ; included by ~/.gitconfig — same escape if writable (subpath (string-append (param "HOME") "/.config/git")) ; XDG git config — same hooksPath/helper escape + ;; Homebrew's system-scope gitconfig, read by a brew-installed git on every + ;; invocation and owned by the developer's account — unlike the Xcode CLT one, + ;; which is root-owned and so already out of reach. Same hooksPath/fsmonitor + ;; escape. Both prefixes: Apple Silicon, then Intel. + (literal "/opt/homebrew/etc/gitconfig") + (literal "/usr/local/etc/gitconfig") + ;; …and the directories holding them. Seatbelt matches the resolved path, so + ;; renaming the parent and dropping a symlink in its place moves the file out + ;; from under the two literals above while everything in it keeps working. + ;; Denying the node blocks that swap; writes to files inside are unaffected, + ;; same as the .git node denies above. + (literal "/opt/homebrew/etc") + (literal "/usr/local/etc") (subpath (string-append (param "HOME") "/.cargo/bin"))) ; binaries on PATH + +;; husky's installer writes .husky/_/ (a gitignored husky.sh plus a .gitignore) on +;; every sandboxed `pnpm install`, and it throws when that write fails, which would +;; fail the install. Re-allow just that directory: last match wins, so this reopens +;; _/ and nothing else inside the .husky deny above. +;; Known limit, deliberate: the checked-in hooks source _/husky.sh, so this leaves +;; one path to the same unsandboxed execution. Closing it means moving husky's +;; install out of the sandbox, and a fresh clone has no _/husky.sh until pnpm +;; install creates it — the same restructuring #79300 tracks for the config chain. +;; The three checked-in hooks are protected either way. +(allow file-write* + (subpath (string-append (param "PROJECT") "/.husky/_")) + (subpath (string-append (param "MAINREPO") "/.husky/_"))) From bbbb6a8f875f249461b78d481684cbc2107c256e Mon Sep 17 00:00:00 2001 From: Alex Date: Fri, 21 Aug 2026 23:37:24 +0200 Subject: [PATCH 024/231] chore(access-control): register SurfaceAccessLimit in the org-scoped IDOR rules The coverage check requires every org-scoped model in the semgrep taint rules, so lookups without an organization filter get flagged. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01LEer2y9uGMkADPjMfWG8WK --- .semgrep/rules/security/idor-team-scoped-models.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.semgrep/rules/security/idor-team-scoped-models.yaml b/.semgrep/rules/security/idor-team-scoped-models.yaml index 29a1194d73d2..1f75160e65ca 100644 --- a/.semgrep/rules/security/idor-team-scoped-models.yaml +++ b/.semgrep/rules/security/idor-team-scoped-models.yaml @@ -838,6 +838,7 @@ rules: |Role |RoleExternalReference |RoleMembership + |SurfaceAccessLimit )$ - focus-metavariable: $SINK pattern-sanitizers: @@ -910,6 +911,7 @@ rules: |Role |RoleExternalReference |RoleMembership + |SurfaceAccessLimit )$ - pattern-not: $M.objects. ... .$METHOD2(..., organization=$O, ...) - pattern-not: $M.objects. ... .$METHOD2(..., organization_id=$O, ...) From 1f3922b32d5d2cfd49b133fa65b1449daf334832 Mon Sep 17 00:00:00 2001 From: Alex Date: Fri, 21 Aug 2026 23:41:04 +0200 Subject: [PATCH 025/231] fix(access-control): drop a stale resource=None from the wildcard test The column is not nullable since the wildcard change; the row now takes the "*" default. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01LEer2y9uGMkADPjMfWG8WK --- .../backend/tests/test_surface_access_limits.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/products/access_control/backend/tests/test_surface_access_limits.py b/products/access_control/backend/tests/test_surface_access_limits.py index 086c491709a4..ea6bf4321013 100644 --- a/products/access_control/backend/tests/test_surface_access_limits.py +++ b/products/access_control/backend/tests/test_surface_access_limits.py @@ -12,9 +12,7 @@ class TestSurfaceLimitResolution(BaseTest): def test_resource_row_overrides_wildcard_row(self) -> None: - SurfaceAccessLimit.objects.create( - organization=self.organization, surface="mcp", resource=None, max_level="viewer" - ) + SurfaceAccessLimit.objects.create(organization=self.organization, surface="mcp", max_level="viewer") SurfaceAccessLimit.objects.create( organization=self.organization, surface="mcp", resource="feature_flag", max_level="editor" ) From 59a7de19223b823d23264360d629149d36affb28 Mon Sep 17 00:00:00 2001 From: Alex Date: Fri, 21 Aug 2026 23:44:01 +0200 Subject: [PATCH 026/231] fix(access-control): a none limit denies reads too, and drop Any from signatures The writes early-return ran before the row lookup, so a disabled surface still served reads, against the model's documented semantics. Adds the regression test and types the permission and policy entry points with concrete request and view classes. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01LEer2y9uGMkADPjMfWG8WK --- .../backend/facade/permissions.py | 7 +++-- .../backend/facade/surface_limits.py | 29 ++++++++++++------- .../tests/test_surface_access_limits.py | 12 +++++++- 3 files changed, 34 insertions(+), 14 deletions(-) diff --git a/products/access_control/backend/facade/permissions.py b/products/access_control/backend/facade/permissions.py index bd1c9357c573..0dbbb96f8004 100644 --- a/products/access_control/backend/facade/permissions.py +++ b/products/access_control/backend/facade/permissions.py @@ -7,7 +7,10 @@ still limited here. """ -from typing import Any +from django.http import HttpRequest + +from rest_framework.request import Request +from rest_framework.views import APIView from posthog.permissions import ScopeBasePermission, get_organization_from_view @@ -22,7 +25,7 @@ class SurfaceAccessLimitPermission(ScopeBasePermission): action's read or write nature the same way `APIScopePermission` does, so the two cannot disagree about what counts as a write.""" - def has_permission(self, request: Any, view: Any) -> bool: + def has_permission(self, request: HttpRequest | Request, view: APIView) -> bool: # Cheap exit first: almost every request has no classified surface. Classification # is two isinstance checks and no query. if classify_surface(request) is None: diff --git a/products/access_control/backend/facade/surface_limits.py b/products/access_control/backend/facade/surface_limits.py index 681ed847dc9c..51cb8ddfa455 100644 --- a/products/access_control/backend/facade/surface_limits.py +++ b/products/access_control/backend/facade/surface_limits.py @@ -5,14 +5,18 @@ organization-wide cap on what any principal can do through one surface. The MCP server is the only surface with limits today. -The class `SurfaceAccessLimitPermission` in facade/permissions.py is the first consumer. It denies -write actions when the organization limits the request's surface below editor. The +The class `SurfaceAccessLimitPermission` in facade/permissions.py is the first consumer. It +denies actions that need more than the surface's limit allows. The access-control facade's `decide()` will also read this module later, to apply the same limit to object-level decisions. All enforcement points read one module, so they cannot disagree about the configured limits. """ -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING + +from django.http import HttpRequest + +from rest_framework.request import Request from posthog.auth import OAuthAccessTokenAuthentication, PersonalAPIKeyAuthentication from posthog.constants import AvailableFeature @@ -28,10 +32,8 @@ # credential's scopes when the token is created. MCP_USER_AGENT_MARKER = "posthog/mcp-server" -WRITE_LIMITED_LEVELS = {SurfaceAccessLimit.MaxLevel.NONE, SurfaceAccessLimit.MaxLevel.VIEWER} - -def classify_surface(request: Any) -> str | None: +def classify_surface(request: HttpRequest | Request) -> str | None: """Returns the access surface of this request. Returns None for paths that have no surface policies.""" authenticator = getattr(request, "successful_authenticator", None) @@ -42,23 +44,28 @@ def classify_surface(request: Any) -> str | None: return None -def limit_denial_for_request(request: Any, organization: "Organization", resource: str, writes: bool) -> str | None: +def limit_denial_for_request( + request: HttpRequest | Request, organization: "Organization", resource: str, writes: bool +) -> str | None: """Makes the full surface-limit decision for one request. Returns a denial message for the user when the organization limits the request's surface below what the action needs. - Returns None to allow the request. + Returns None to allow the request. A `"none"` limit denies reads and writes both. This function contains all the policy: surface classification, the feature-entitlement check, the row lookup, and the message text. Enforcement points (`SurfaceAccessLimitPermission` today, the facade's `decide()` later) apply the result and add no policy of their own.""" - if not writes: - return None surface = classify_surface(request) if surface is None: return None if not organization.is_feature_available(AvailableFeature.ORGANIZATION_SECURITY_SETTINGS): return None limit = surface_limit(organization, surface, resource) - if limit in WRITE_LIMITED_LEVELS: + if limit == SurfaceAccessLimit.MaxLevel.NONE: + return ( + "Your organization has disabled MCP access. " + "An organization admin can change this in your organization settings." + ) + if writes and limit == SurfaceAccessLimit.MaxLevel.VIEWER: return ( "Your organization restricts MCP access to read-only. " "An organization admin can change this in your organization settings." diff --git a/products/access_control/backend/tests/test_surface_access_limits.py b/products/access_control/backend/tests/test_surface_access_limits.py index ea6bf4321013..ff3656614adf 100644 --- a/products/access_control/backend/tests/test_surface_access_limits.py +++ b/products/access_control/backend/tests/test_surface_access_limits.py @@ -1,5 +1,7 @@ from posthog.test.base import APIBaseTest, BaseTest +from django.http import HttpResponse + from parameterized import parameterized from posthog.constants import AvailableFeature @@ -44,7 +46,7 @@ def setUp(self) -> None: ) self.client.logout() - def _request(self, method: str, body: dict | None = None, mcp: bool = True): + def _request(self, method: str, body: dict | None = None, mcp: bool = True) -> HttpResponse: return getattr(self.client, method)( f"/api/projects/{self.team.id}/feature_flags/", body or {}, @@ -69,6 +71,14 @@ def test_writes_pass_without_a_matching_limit(self, _name: str, mcp: bool) -> No response = self._request("post", {"key": f"flag-{_name}", "name": "flag"}, mcp=mcp) assert response.status_code == 201 + def test_disabled_surface_denies_reads_too(self) -> None: + SurfaceAccessLimit.objects.create(organization=self.organization, surface="mcp", max_level="none") + + read = self._request("get") + assert read.status_code == 403 + assert "disabled" in read.json()["detail"] + assert self._request("post", {"key": "mcp-e2e-disabled", "name": "e2e"}).status_code == 403 + def test_resource_exception_lets_that_resource_write(self) -> None: SurfaceAccessLimit.objects.create(organization=self.organization, surface="mcp", max_level="viewer") SurfaceAccessLimit.objects.create( From 79859115353f46777c6b8ff66b756cb54db91884 Mon Sep 17 00:00:00 2001 From: Adam Leith Date: Mon, 24 Aug 2026 11:39:01 +0100 Subject: [PATCH 027/231] fix(desktop): show starting state on session dot Generated-By: PostHog Desktop Task-Id: cfa3e600-1fd9-4d9a-9a79-a96105041948 --- .../core/src/sessions/sessionService.ts | 14 +++++++++++++- .../packages/core/src/sessions/sessionStore.ts | 18 ++++++++++++++++++ .../src/sessions/sessionStoreEviction.test.ts | 15 +++++++++++++++ .../canvas/components/ChannelItemRow.test.tsx | 5 +++++ .../canvas/hooks/useChannelTaskStatus.ts | 3 +++ .../sessions/sessionServiceHost.test.ts | 5 +++++ .../ui/src/features/sessions/useSession.ts | 7 +++++++ .../components/items/taskStatusVocabulary.ts | 6 ++++-- 8 files changed, 70 insertions(+), 3 deletions(-) diff --git a/products/desktop/packages/core/src/sessions/sessionService.ts b/products/desktop/packages/core/src/sessions/sessionService.ts index 7930cd052d23..ca71ec97a8e7 100644 --- a/products/desktop/packages/core/src/sessions/sessionService.ts +++ b/products/desktop/packages/core/src/sessions/sessionService.ts @@ -367,6 +367,8 @@ export interface SessionTrpc { export interface ISessionStore { setSession(session: AgentSession): void; removeSession(taskRunId: string): void; + setTaskStarting?(taskId: string): void; + clearTaskStarting?(taskId: string): void; updateSession(taskRunId: string, updates: Partial): void; appendEvents( taskRunId: string, @@ -1898,6 +1900,7 @@ export class SessionService { const { task } = params; const taskId = task.id; this.taskCreationMarks.delete(taskId); + this.d.store.clearTaskStarting?.(taskId); this.localRepoPaths.set(taskId, params.repoPath); this.sessionLastUsedAt.set(taskId, Date.now()); void this.evictIdleSessions(taskId); @@ -5222,6 +5225,7 @@ export class SessionService { runtimeOptions = getCloudRuntimeOptions(session, previousRun); try { + this.markTaskCreationInFlight(session.taskId); // Backend derives the snapshot from resumeFromRunId and restores the sandbox. updatedTask = await authCredentials.client.runTaskInCloud( session.taskId, @@ -5261,11 +5265,13 @@ export class SessionService { throw error; } } catch (error) { + this.clearTaskCreationInFlight(session.taskId); rollbackOptimisticPrompt(); throw error; } const newRun = updatedTask.latest_run; if (!newRun?.id) { + this.clearTaskCreationInFlight(session.taskId); rollbackOptimisticPrompt(); throw new Error("Failed to create resume run"); } @@ -8209,6 +8215,12 @@ export class SessionService { public markTaskCreationInFlight(taskId: string): void { this.taskCreationMarks.set(taskId, Date.now()); + this.d.store.setTaskStarting?.(taskId); + } + + private clearTaskCreationInFlight(taskId: string): void { + this.taskCreationMarks.delete(taskId); + this.d.store.clearTaskStarting?.(taskId); } private isTaskCreationInFlight(taskId: string): boolean { @@ -8217,7 +8229,7 @@ export class SessionService { const expired = Date.now() - markedAt > SessionService.TASK_CREATION_IN_FLIGHT_TTL_MS; if (expired) { - this.taskCreationMarks.delete(taskId); + this.clearTaskCreationInFlight(taskId); return false; } return true; diff --git a/products/desktop/packages/core/src/sessions/sessionStore.ts b/products/desktop/packages/core/src/sessions/sessionStore.ts index 90bbda9957de..6f3c8aeecf46 100644 --- a/products/desktop/packages/core/src/sessions/sessionStore.ts +++ b/products/desktop/packages/core/src/sessions/sessionStore.ts @@ -25,12 +25,15 @@ export interface SessionState { sessions: Record; /** Index mapping taskId -> taskRunId for O(1) lookups */ taskIdIndex: Record; + /** Task ids whose first/resumed agent session is being created. */ + startingTaskIds: Record; } export const sessionStore = createStore()( immer(() => ({ sessions: {}, taskIdIndex: {}, + startingTaskIds: {}, })), ); @@ -94,6 +97,7 @@ export const sessionStoreSetters = { state.sessions[session.taskRunId] = session; state.taskIdIndex[session.taskId] = session.taskRunId; + delete state.startingTaskIds[session.taskId]; }); }, @@ -102,11 +106,24 @@ export const sessionStoreSetters = { const session = state.sessions[taskRunId]; if (session) { delete state.taskIdIndex[session.taskId]; + delete state.startingTaskIds[session.taskId]; } delete state.sessions[taskRunId]; }); }, + setTaskStarting: (taskId: string) => { + sessionStore.setState((state) => { + state.startingTaskIds[taskId] = true; + }); + }, + + clearTaskStarting: (taskId: string) => { + sessionStore.setState((state) => { + delete state.startingTaskIds[taskId]; + }); + }, + updateSession: (taskRunId: string, updates: Partial) => { sessionStore.setState((state) => { if (state.sessions[taskRunId]) { @@ -436,6 +453,7 @@ export const sessionStoreSetters = { sessionStore.setState((state) => { state.sessions = {}; state.taskIdIndex = {}; + state.startingTaskIds = {}; }); }, }; diff --git a/products/desktop/packages/core/src/sessions/sessionStoreEviction.test.ts b/products/desktop/packages/core/src/sessions/sessionStoreEviction.test.ts index ff04eccb72fc..4e37a235d63f 100644 --- a/products/desktop/packages/core/src/sessions/sessionStoreEviction.test.ts +++ b/products/desktop/packages/core/src/sessions/sessionStoreEviction.test.ts @@ -24,6 +24,21 @@ function seedWithEvents() { afterEach(() => sessionStoreSetters.removeSession(RUN)); describe("evictEvents / restoreEvents", () => { + it("clears a starting marker when the session arrives", () => { + sessionStoreSetters.setTaskStarting(TASK); + + sessionStoreSetters.setSession({ + taskRunId: RUN, + taskId: TASK, + events: [], + messageQueue: [], + pendingPermissions: new Map(), + status: "connected", + } as unknown as AgentSession); + + expect(sessionStore.getState().startingTaskIds[TASK]).toBeUndefined(); + }); + it("evictEvents frees the transcript and resets the line cursor", () => { seedWithEvents(); expect(sessionStore.getState().sessions[RUN].events).toHaveLength(1); diff --git a/products/desktop/packages/ui/src/features/canvas/components/ChannelItemRow.test.tsx b/products/desktop/packages/ui/src/features/canvas/components/ChannelItemRow.test.tsx index 9773aa64ed7b..711cd0384aa1 100644 --- a/products/desktop/packages/ui/src/features/canvas/components/ChannelItemRow.test.tsx +++ b/products/desktop/packages/ui/src/features/canvas/components/ChannelItemRow.test.tsx @@ -118,6 +118,11 @@ describe("ChannelItemRow", () => { // rather than the status: starting, live but stalled, or something to read. it.each([ ["a permission prompt", { needsPermission: true }, "Needs your input"], + [ + "an agent session being created", + { isAgentSessionStarting: true }, + "Starting", + ], ["a streaming agent", { isGenerating: true }, "Working"], [ // A background run is one-shot and unattended, so its in_progress really diff --git a/products/desktop/packages/ui/src/features/canvas/hooks/useChannelTaskStatus.ts b/products/desktop/packages/ui/src/features/canvas/hooks/useChannelTaskStatus.ts index 14c29d99a4d0..78475f73bb3e 100644 --- a/products/desktop/packages/ui/src/features/canvas/hooks/useChannelTaskStatus.ts +++ b/products/desktop/packages/ui/src/features/canvas/hooks/useChannelTaskStatus.ts @@ -1,6 +1,7 @@ import type { ChannelItemModel } from "@posthog/core/canvas/channelItems"; import type { Task } from "@posthog/shared/domain-types"; import { useChannelTaskData } from "@posthog/ui/features/canvas/hooks/useChannelTaskData"; +import { useTaskSessionStarting } from "@posthog/ui/features/sessions/useSession"; import type { TaskStatusInput } from "@posthog/ui/features/sidebar/components/items/taskStatusVocabulary"; import { useTaskPrStatus } from "@posthog/ui/features/sidebar/useTaskPrStatus"; import { useWorkspace } from "@posthog/ui/features/workspace/useWorkspace"; @@ -32,6 +33,7 @@ export function useTaskStatusInput( ): TaskStatusInput | null { const taskData = useChannelTaskData(task); const workspace = useWorkspace(task?.id); + const isAgentSessionStarting = useTaskSessionStarting(task?.id); const { prState, hasDiff, prUrl } = useTaskPrStatus({ // An empty id is the hook's own "nothing to look up", so this asks for no // query rather than one it throws away. @@ -56,6 +58,7 @@ export function useTaskStatusInput( slackThreadUrl: taskData.slackThreadUrl, prState, hasDiff, + isAgentSessionStarting, // The url is the early signal: a cloud run writes it the moment it opens the // PR, long before (or without ever) resolving the PR's state. A local run // has no cloud url, so the one the host cached against the task stands in. diff --git a/products/desktop/packages/ui/src/features/sessions/sessionServiceHost.test.ts b/products/desktop/packages/ui/src/features/sessions/sessionServiceHost.test.ts index 30c0fc7597a7..6bf15401f008 100644 --- a/products/desktop/packages/ui/src/features/sessions/sessionServiceHost.test.ts +++ b/products/desktop/packages/ui/src/features/sessions/sessionServiceHost.test.ts @@ -67,6 +67,8 @@ const mockTrpcOs = vi.hoisted(() => ({ const mockSessionStoreSetters = vi.hoisted(() => ({ setSession: vi.fn(), removeSession: vi.fn(), + setTaskStarting: vi.fn(), + clearTaskStarting: vi.fn(), updateSession: vi.fn(), updateCloudStatus: vi.fn(), appendEvents: vi.fn(), @@ -7680,6 +7682,9 @@ describe("SessionService", () => { ); expect(result.stopReason).toBe("queued"); + expect(mockSessionStoreSetters.setTaskStarting).toHaveBeenCalledWith( + "task-123", + ); expect(mockAuthenticatedClient.runTaskInCloud).toHaveBeenCalledWith( "task-123", "feature/codex-run", diff --git a/products/desktop/packages/ui/src/features/sessions/useSession.ts b/products/desktop/packages/ui/src/features/sessions/useSession.ts index 9b2e9bb9cc20..4770381c960e 100644 --- a/products/desktop/packages/ui/src/features/sessions/useSession.ts +++ b/products/desktop/packages/ui/src/features/sessions/useSession.ts @@ -198,3 +198,10 @@ export const useSessionHandoffInProgress = ( return s.sessions[taskRunId]?.handoffInProgress ?? false; }); }; + +export const useTaskSessionStarting = (taskId: string | undefined): boolean => { + return useSessionStore((s) => { + if (!taskId) return false; + return s.startingTaskIds[taskId] === true; + }); +}; diff --git a/products/desktop/packages/ui/src/features/sidebar/components/items/taskStatusVocabulary.ts b/products/desktop/packages/ui/src/features/sidebar/components/items/taskStatusVocabulary.ts index efdaa8c2979f..81902ee43e64 100644 --- a/products/desktop/packages/ui/src/features/sidebar/components/items/taskStatusVocabulary.ts +++ b/products/desktop/packages/ui/src/features/sidebar/components/items/taskStatusVocabulary.ts @@ -24,6 +24,7 @@ import { SlackMark } from "@posthog/ui/primitives/SlackMark"; */ export type TaskStatusInput = TaskIconProps & { prUrl?: string | null; + isAgentSessionStarting?: boolean; }; /** @@ -146,13 +147,14 @@ export function taskDot(props: TaskStatusInput): TaskDot { // status, so it can sit there for hours after the agent is done with it. const isStartingCloudRun = props.taskRunStatus === "queued" && props.workspaceMode === "cloud"; - if (props.isGenerating || isStartingCloudRun) { + const isStarting = props.isAgentSessionStarting || isStartingCloudRun; + if (props.isGenerating || isStarting) { return { tone: "yellow", style: "solid", pulse: false, spinner: true, - label: props.isGenerating ? "Working" : "Starting", + label: props.isGenerating && !isStarting ? "Working" : "Starting", }; } // Only a background run's status is a claim about work. An interactive run is From a4e5fcd891e3fd41f19f20ecb07fd2b4442a274b Mon Sep 17 00:00:00 2001 From: Adam Leith Date: Mon, 24 Aug 2026 12:53:11 +0100 Subject: [PATCH 028/231] fix(desktop): keep session dot spinning during start Generated-By: PostHog Desktop Task-Id: cfa3e600-1fd9-4d9a-9a79-a96105041948 --- .../core/src/sessions/sessionService.ts | 5 ++++- .../sessions/sessionServiceRecovery.test.ts | 18 ++++++++++++++---- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/products/desktop/packages/core/src/sessions/sessionService.ts b/products/desktop/packages/core/src/sessions/sessionService.ts index ca71ec97a8e7..0fb09045b5c9 100644 --- a/products/desktop/packages/core/src/sessions/sessionService.ts +++ b/products/desktop/packages/core/src/sessions/sessionService.ts @@ -1900,7 +1900,6 @@ export class SessionService { const { task } = params; const taskId = task.id; this.taskCreationMarks.delete(taskId); - this.d.store.clearTaskStarting?.(taskId); this.localRepoPaths.set(taskId, params.repoPath); this.sessionLastUsedAt.set(taskId, Date.now()); void this.evictIdleSessions(taskId); @@ -1914,9 +1913,13 @@ export class SessionService { // Check for existing connected session const existingSession = this.d.store.getSessionByTaskId(taskId); if (existingSession?.status === "connected") { + this.d.store.clearTaskStarting?.(taskId); this.d.log.info("Already connected to task", { taskId }); return; } + if (task.latest_run?.environment !== "cloud") { + this.d.store.setTaskStarting?.(taskId); + } if (existingSession?.status === "connecting") { this.d.log.info("Session already in connecting state", { taskId }); return; diff --git a/products/desktop/packages/core/src/sessions/sessionServiceRecovery.test.ts b/products/desktop/packages/core/src/sessions/sessionServiceRecovery.test.ts index cb48522111b7..9c0d86226b2b 100644 --- a/products/desktop/packages/core/src/sessions/sessionServiceRecovery.test.ts +++ b/products/desktop/packages/core/src/sessions/sessionServiceRecovery.test.ts @@ -45,6 +45,8 @@ function createHarness({ spyConnect = true } = {}) { getSessionByTaskId: (taskId: string) => Object.values(sessions).find((s) => s.taskId === taskId), removeSession: vi.fn(), + setTaskStarting: vi.fn(), + clearTaskStarting: vi.fn(), updateSession: vi.fn(), }; const log = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }; @@ -73,7 +75,7 @@ function createHarness({ spyConnect = true } = {}) { const connectToTask = spyConnect ? vi.spyOn(service, "connectToTask").mockResolvedValue(undefined) : undefined; - return { service, sessions, connectToTask, log }; + return { service, sessions, connectToTask, log, store }; } function reconcile( @@ -162,14 +164,22 @@ describe("SessionService run-less local task recovery", () => { expect(connectToTask).toHaveBeenCalledTimes(1); }); - it("clears the in-flight mark when a connect starts", async () => { - const { service, sessions } = createHarness({ spyConnect: false }); + it("keeps the starting marker while clearing the recovery in-flight mark", async () => { + const { service, sessions, store } = createHarness({ spyConnect: false }); const task = makeTask(); - sessions[`run-${task.id}`] = makeSession(task.id); + sessions[`run-${task.id}`] = { + ...makeSession(task.id), + status: "connecting", + }; service.markTaskCreationInFlight(task.id); + vi.mocked(store.setTaskStarting).mockClear(); + vi.mocked(store.clearTaskStarting).mockClear(); await service.connectToTask({ task, repoPath: "/repo" }); + expect(store.setTaskStarting).toHaveBeenCalledWith(task.id); + expect(store.clearTaskStarting).not.toHaveBeenCalled(); + const connectToTask = vi .spyOn(service, "connectToTask") .mockResolvedValue(undefined); From 91e5f200541fb8711b6fb48e3c6e8f8c2339a2cb Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Mon, 24 Aug 2026 08:42:16 -0700 Subject: [PATCH 029/231] fix(dev): pin the wrapper's interpreter and deny the Homebrew prefix The shebang resolved bash through the caller's PATH, which keeps repo dirs sandboxed code can write, so a planted bash ran as the developer before the wrapper's own PATH pin could take effect. It is a fixed /bin/bash now. The profile denied Homebrew's etc/gitconfig but left bin/, sbin/ and Cellar/ writable. Those are developer-owned and on PATH, so planting a binary there reaches unsandboxed execution without the git-config indirection. Two self-test cases cover them: a bash planted ahead on PATH must not run, and the Homebrew bin directories must refuse a write. Both were checked red against the previous behavior. Write-side gaps that survive these denies, the git-dir and repo-root node swaps, the activation PATH, and the shared profile cache, are in #87967. Generated-By: PostHog Desktop Task-Id: 7ad0d5f8-020a-4917-8108-a1d66b8ac2c2 --- bin/dev-sandbox | 8 +++++++- bin/dev-sandbox-selftest | 28 ++++++++++++++++++++++++++++ bin/dev-sandbox.sb | 9 +++++++++ 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/bin/dev-sandbox b/bin/dev-sandbox index 208d339aca96..e8d2e2d341a4 100755 --- a/bin/dev-sandbox +++ b/bin/dev-sandbox @@ -1,4 +1,4 @@ -#!/usr/bin/env bash +#!/bin/bash # Run a dev-stack command inside a macOS Seatbelt sandbox (see bin/dev-sandbox.sb). # # Usage: bin/dev-sandbox '' @@ -19,6 +19,12 @@ # - non-macOS / sandbox-exec missing / profile missing / render fails -> passthrough # - sandbox-exec cannot initialize the profile -> fail OPEN, loud warning # (a working sandbox that merely *denies* a path is left to surface normally) +# +# The shebang is a fixed /bin/bash, not `/usr/bin/env bash`: the kernel resolves it +# before any line of this script runs, by searching the PATH of whoever invokes this +# file directly — the developer's inherited PATH, which keeps repo-writable dirs (see +# below). `env bash` would let a bash planted by a prior sandboxed run in one of those +# dirs execute as the developer, with nothing here yet in a position to stop it. set -euo pipefail diff --git a/bin/dev-sandbox-selftest b/bin/dev-sandbox-selftest index 783a5997867f..0d4a60883201 100755 --- a/bin/dev-sandbox-selftest +++ b/bin/dev-sandbox-selftest @@ -187,6 +187,13 @@ check_write_blocked ".git/hooks/.dev_sandbox_selftest_probe" ".git/hooks" if [[ -d "$HOME/.cargo/bin" ]]; then check_write_blocked "$HOME/.cargo/bin/.dev_sandbox_selftest_probe" "cargo bin" fi +# Homebrew's bin/sbin/Cellar: developer-owned and on PATH, same execute-later route +# as cargo bin above — no git-config indirection needed to reach it. +for hb_bin in /opt/homebrew/bin /usr/local/bin; do + if [[ -d "$hb_bin" ]]; then + check_write_blocked "$hb_bin/.dev_sandbox_selftest_probe" "Homebrew bin ($hb_bin)" + fi +done # For files that really exist and must not be damaged by the probe: opens for # append and writes nothing, so Seatbelt still has to decide on write access while # a missing deny cannot corrupt the file. @@ -370,6 +377,27 @@ else die "passthrough PATH left pinned to system dirs (got: $child_path)" fi +# 15. The shebang resolves a fixed interpreter, not one found by searching the +# caller's PATH. `env bash` would let a bash planted by a prior sandboxed run +# (e.g. in node_modules/.bin or .flox/cache/venv/bin, both repo-writable) run +# BEFORE this script's own PATH pin — or any other line — has a chance to stop +# it. Plant a fake bash ahead of the real one and confirm it never runs. +fake_bin="$(cd "$(mktemp -d)" && pwd -P)" +_tmpdirs+=("$fake_bin") +marker="$fake_bin/ran" +cat >"$fake_bin/bash" <