fix(auth): scope export renderer JWT tokens with dedicated audience#51714
fix(auth): scope export renderer JWT tokens with dedicated audience#51714
Conversation
Export pages for session recordings and heatmaps were generating IMPERSONATED_USER JWTs that granted full API access as the export creator. Replace with a new EXPORT_RENDERER audience that is only accepted on viewsets that explicitly opt in (session recordings and heatmaps), preventing privilege escalation from export tokens. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use fresh APIClient instances instead of self.client.logout() to avoid DRF session state affecting status codes. Replace insights endpoint (which has sharing auth that changes 401→403) with user API endpoint. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Prompt To Fix All With AIThis is a comment left during a code review.
Path: posthog/jwt.py
Line: 13-14
Comment:
**Missing inline comments on audience constants**
The PR removed the explanatory comment from `IMPERSONATED_USER` and the new `EXPORT_RENDERER` value has no comment either. Per the project convention of adding inline comments to clarify behaviour around API keys, both values should document their purpose:
```suggestion
IMPERSONATED_USER = "posthog:impersonted_user" # Used by background jobs acting on behalf of a user (e.g. async exports)
EXPORT_RENDERER = "posthog:export_renderer" # Scoped to the Chromium export renderer; only accepted by viewsets that explicitly opt in
```
**Rule Used:** Add inline comments to clarify the purpose of sign... ([source](https://app.greptile.com/review/custom-context?memory=4d5b48c5-045d-4693-9cd9-4081bb19508b))
**Learnt From**
[PostHog/posthog#32083](https://github.com/PostHog/posthog/pull/32083)
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: posthog/auth.py
Line: 433-438
Comment:
**Expired token raises hard 401 instead of falling through**
`jwt.ExpiredSignatureError` is not a subclass of `jwt.DecodeError` — it extends `jwt.InvalidClaimError`. This means an expired `EXPORT_RENDERER` token is caught by the bare `except Exception` block and raises `AuthenticationFailed`, short-circuiting the entire DRF authenticator chain.
In practice this is fine for the export renderer (the token is the only credential), but it's inconsistent with the pattern in `JwtAuthentication` where a malformed token quietly falls through (`return None`) to allow later authenticators (e.g. `SessionAuthentication`) to succeed. If a real user happens to hold a stale token in their `Authorization` header but is otherwise authenticated via session, they would receive an opaque 401.
Consider explicitly returning `None` for `jwt.ExpiredSignatureError` to match the "fall through" approach, or at minimum add it to the explicit-return-None block alongside `InvalidAudienceError`:
```suggestion
except jwt.DecodeError:
return None
except jwt.InvalidAudienceError:
return None
except jwt.ExpiredSignatureError:
return None
except Exception:
raise AuthenticationFailed(detail="Token invalid.")
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: posthog/test/test_export_renderer_auth.py
Line: 22-70
Comment:
**Prefer parameterised tests**
The project convention is to always prefer parameterised tests. The four "accepted/rejected on endpoint" test cases follow identical structure and can be collapsed into two parameterised tests using the `parameterized` library (already used elsewhere in the test suite), removing the duplication.
For example, the two acceptance tests can become a single `@parameterized.expand` test taking the target URL as a parameter, and similarly the two rejection tests can be merged. This also makes it cheap to add new opt-in viewsets to the coverage in the future.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "update comments" |
|
|
||
| keyword = "Bearer" | ||
|
|
||
| def authenticate(self, request: Union[HttpRequest, Request]) -> Optional[tuple[Any, None]]: |
There was a problem hiding this comment.
I believe this JWT is only intended for read-only actions. If so, you should verify that the request method is either GET or HEAD like in SharingAccessTokenAuthentication below. And also add some tests to verify POST/PUT/PATCH/DELETE are rejected.
There was a problem hiding this comment.
Scoped down to just GET and HEAD.
There was a problem hiding this comment.
Confirmed that exportToken, is only used with fetch, listSnapshotSources, and getSnapshots.
posthog/auth.py
Outdated
| info = decode_jwt(token, PosthogJwtAudience.EXPORT_RENDERER) | ||
| user = User.objects.get(pk=info["id"]) | ||
| return user, None | ||
| except (jwt.DecodeError, jwt.InvalidAudienceError, jwt.ExpiredSignatureError): |
There was a problem hiding this comment.
jwt.ExpiredSignatureError should probably raise AuthenticationFailed, rather than passing through to the next authenticator.
|
🎭 Playwright report · View test results →
These issues are not necessarily caused by your changes. |
…51714) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Problem
The SharingViewerPageViewSet generates IMPERSONATED_USER JWT tokens for session recording and heatmap exports. These tokens grant full API access as the export creator, but the export renderer only needs to read recording snapshots and heatmap data.
Changes
Introduces a new
EXPORT_RENDERERJWT audience that is only accepted on viewsets that explicitly opt in, replacingIMPERSONATED_USERHow did you test this code?
posthog/test/test_export_renderer_auth.pyPublish to changelog?
No
Docs update
No docs changes needed.