fix(subscriptions): restore exports with sharing disabled#60276
Merged
vdekrijger merged 1 commit intoMay 29, 2026
Merged
Conversation
Contributor
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
posthog/api/sharing.py:795-797
The `request` parameter is accepted but never read inside the method, making it superfluous. The only relevant data is already on `resource`.
```suggestion
def _is_blocked_by_public_sharing_setting(
self, resource: SharingConfiguration | ExportedAsset
) -> bool:
```
### Issue 2 of 3
posthog/api/sharing.py:804-807
**`"subscription_delivery"` string duplicated across three files**
The key `"subscription_delivery"` is written as a bare string literal in `sharing.py`, `ee/tasks/subscriptions/subscription_utils.py`, and `posthog/temporal/subscriptions/activities.py`. A mismatch from a later rename would silently break the bypass allowance without a test failure on the mismatched writer. Defining it once as a constant (e.g., in `exported_asset.py`) and importing it would satisfy OnceAndOnlyOnce.
### Issue 3 of 3
posthog/models/exported_asset.py:235-242
**`has_short_lived_public_render_token` detects remaining lifetime, not original issue window**
The guard `expires_at <= now_ + timedelta(minutes=20)` is true any time the token has ≤ 20 minutes of validity left — including a long-lived content token (365-day default) that happens to be in its final 20 minutes of life. In that narrow window the token would pass the legacy-render-path check and be permitted on `/exporter`. The practical window is 20 minutes out of 525 600 (≈ 0.0038% of the token's lifespan), and `encode_jwt` does not embed `iat`, so there is no clean way to recover the original expiry without a schema change. Worth noting for when the legacy path is eventually retired and this heuristic can be removed.
Reviews (1): Last reviewed commit: "fix(subscriptions): restore exports with..." | Re-trigger Greptile |
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 2 · PR risk: 0/10 |
f319e7e to
7a65b6b
Compare
Contributor
Author
|
Thanks — addressed in the latest push:
Retested: ./bin/hogli test posthog/api/test/test_sharing.py::TestSharingConfigurationSerializerValidation posthog/tasks/exports/test/test_image_exporter.py ee/tasks/test/subscriptions/test_subscriptions_utils.pyResult: |
e7478d4 to
7d874f9
Compare
vdekrijger
commented
May 28, 2026
7d874f9 to
3361925
Compare
Contributor
Author
|
Addressed the latest review comments in the newest push:
Retested: ./bin/hogli test posthog/api/test/test_sharing.py::TestSharingConfigurationSerializerValidation posthog/tasks/exports/test/test_image_exporter.py ee/tasks/test/subscriptions/test_subscriptions_utils.pyResult: . |
3361925 to
00d595f
Compare
Contributor
Author
|
Updated again based on the discussion to simplify the model:
Retested: ./bin/hogli test posthog/api/test/test_sharing.py::TestSharingConfigurationSerializerValidation posthog/tasks/exports/test/test_image_exporter.py ee/tasks/test/subscriptions/test_subscriptions_utils.pyResult: |
00d595f to
a623da2
Compare
Contributor
Author
|
Updated again to use the standard JWT shape we discussed: a single
Retested: ./bin/hogli test posthog/api/test/test_sharing.py::TestSharingConfigurationSerializerValidation posthog/tasks/exports/test/test_image_exporter.py ee/tasks/test/subscriptions/test_subscriptions_utils.pyResult: |
a623da2 to
b9fb1ac
Compare
Contributor
Author
|
Updated to the middle-ground token model:
Retested: ./bin/hogli test posthog/api/test/test_sharing.py::TestSharingConfigurationSerializerValidation posthog/tasks/exports/test/test_image_exporter.py ee/tasks/test/subscriptions/test_subscriptions_utils.pyResult: |
ReeceJones
approved these changes
May 28, 2026
b9fb1ac to
c8bf02a
Compare
Generated-By: PostHog Code Task-Id: 09015dfc-1238-463a-a11c-e3944ff5f975
c8bf02a to
8576950
Compare
MattPua
approved these changes
May 28, 2026
rorylshanks
pushed a commit
that referenced
this pull request
May 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Dashboard and insight subscriptions rely on exported image assets for Slack and email delivery. When an organization disables public shared resources, subscription screenshot generation and delivery should continue to work for explicitly configured recipients without reopening the richer shared/exporter render surface to public asset tokens.
The previous exported asset sharing enforcement treated subscription delivery artifacts the same as general public shared resources, which caused subscription export rendering to fail. A safe fix also needs to avoid letting long-lived public content tokens render the full exporter page.
Changes
EXPORT_ASSET_RENDER_TOKEN_ENABLEDfor safe rollout.is_system=Trueon subscription-generated assets for quota/system accounting only.How did you test this code?
Agent-authored. Automated tests run:
./bin/hogli test posthog/api/test/test_sharing.py::TestSharingConfigurationSerializerValidation posthog/tasks/exports/test/test_image_exporter.py ee/tasks/test/subscriptions/test_subscriptions_utils.pyResult:
36 passed.Also ran:
Result: no whitespace errors.
Local cross-reference: PR branch vs master
Agent-driven manual verification on the demo dev stack (team 1,
Hedgebox Inc.).Repeatable on a fresh local stack — DB state was held constant across both branches,
only the view code differs.
Setup (paste into
python manage.py shell):Setup script
Results — same DB state, same JWTs, only branch differs:
/exporter(page)/exporter(page)/exporter/<file>/exporter/<file>/exporter(page)and the subscription delivery token cannot be repurposed against the render page.
Publish to changelog?
no
Docs update
No docs update needed.
🤖 Agent context
PostHog Code investigated a regression (#59523) in subscription export rendering for organizations with public shared resources disabled. The first implementation allowed system exported assets to render, but review found that public content tokens could be reused against the richer exporter route and that delivered image URLs would remain blocked. The final design uses capability-specific token audiences for render, normal content, and subscription delivery content, restricts tokens to their intended URL surfaces, and adds a rollout flag so web compatibility can deploy before workers switch render token audiences.
Review-swarm findings were addressed before opening this PR. Remaining rollout guidance: deploy this code with
EXPORT_ASSET_RENDER_TOKEN_ENABLEDleft at its defaultfalse, then enable the flag after web pods are rolled out.Local cross-reference verification against master added by Claude Code in a follow-up session: minted public / render / subscription-delivery tokens against an asset on the demo team with
allow_publicly_shared_resources=False, then re-ran the same five curls on both branches with identical DB state. Confirmed the two endpoints the PR is meant to restore (render page + subscription file delivery) flip from 404 to 200, and the three guarded endpoints stay 404 on both — i.e. the fix doesn't widen public sharing.Created with PostHog Code