feat(error-tracking): per-team Ed25519 signing-key registration#62750
Open
Gilbert09 wants to merge 3 commits into
Open
feat(error-tracking): per-team Ed25519 signing-key registration#62750Gilbert09 wants to merge 3 commits into
Gilbert09 wants to merge 3 commits into
Conversation
Adds ErrorTrackingSigningKey (team-scoped public-key registry) + CRUD API so customers can register the Ed25519 public key their backend SDK signs exceptions with. The server validates it's an Ed25519 public key and derives a key_id (base64url(sha256(raw_pubkey))[:16]) that matches the SDK's derivation, so ingestion can look the key up by the id stamped on signed events. Public key is write-once; only label/revoked are mutable. cymbal reads this table to verify signatures (separate change).
assign-reviewers-posthog
Bot
requested review from
a team,
ablaszkiewicz,
cat-ph and
hpouillot
This was referenced Jun 10, 2026
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
products/error_tracking/backend/api/signing_keys.py:59-62
**Inconsistent `public_key` behaviour on updates**
Because `public_key` is not listed in `read_only_fields`, DRF's field-level `validate_public_key` still runs during a PATCH/PUT. That means a *valid* PEM sent via PATCH passes validation, `update` silently pops it, and the client receives `200 OK` with the key unchanged — it has no way to know the field was ignored. Conversely, an *invalid* PEM returns `400`. The current test (`"tampered"` is not a valid PEM) only exercises the 400 path; it never reaches the silent-drop branch.
The cleanest fix is to make `public_key` read-only for updates by overriding `get_fields`, so DRF skips both validation and population of the field during any update operation:
```python
def get_fields(self):
fields = super().get_fields()
if self.instance is not None: # update context
fields["public_key"].read_only = True
return fields
```
With that change the `pop` guard in `update` becomes unnecessary and the test should use a valid alternate PEM to confirm the field is truly ignored.
### Issue 2 of 3
products/error_tracking/backend/api/signing_keys.py:45-51
**PEM parsed twice (OnceAndOnlyOnce)**
`validate_public_key` calls `_validate_ed25519_public_key_pem` and discards the returned raw bytes, then `create` calls the same function again purely to obtain those bytes for key-id derivation. Storing the result on the serializer instance during field validation avoids the redundant parse:
```python
def validate_public_key(self, value: str) -> str:
self._raw_pubkey = _validate_ed25519_public_key_pem(value)
return value
def create(self, validated_data):
validated_data["key_id"] = derive_key_id(self._raw_pubkey)
...
```
### Issue 3 of 3
products/error_tracking/backend/api/test/test_signing_keys.py:84-90
**Test doesn't exercise the immutability path it claims to test**
`"tampered"` is not a valid PEM, so the PATCH actually returns `400` because `validate_public_key` raises a `ValidationError` — the `pop` inside `update` is never reached. The test never verifies what happens when a *valid* (but different) public key is submitted. The response status code is also not asserted, so the test would pass even if the server returned `500`.
Using a second valid seed and asserting `HTTP_400_BAD_REQUEST` (or `200` with documented silent-drop semantics, once the behaviour is settled) would make the test actually cover the declared invariant.
Reviews (1): Last reviewed commit: "feat(error-tracking): per-team Ed25519 s..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
MCP UI Apps size report
|
Contributor
|
Size Change: +20.4 kB (+0.02%) Total Size: 82.5 MB 📦 View Changed
ℹ️ View Unchanged
|
- Add a 'Signing keys' section under Error tracking settings: list (key id, label, status, added, last used), add-key modal (label + PEM public key), and revoke. New signingKeysLogic + SigningKeys component, api.errorTracking .signingKeys client, and SettingsMap registration. - Review fixes: make public_key read-only on update (get_fields) so a PATCH can't silently no-op; parse/validate the PEM once and reuse for key_id; the immutability test now submits a valid alternate key and asserts it's ignored.
Contributor
|
👋 Visual changes detected for this PR. Review and approve in PostHog Visual Review If these changes are unexpected, they may be caused by a flaky test or a broken snapshot on master. Don't approve — rerun the job or wait for a fix. |
Contributor
Migration SQL ChangesHey 👋, we've detected some migrations on this PR. Here's the SQL output for each migration, make sure they make sense:
|
Contributor
🔍 Migration Risk AnalysisWe've analyzed your migrations for potential risks. Summary: 1 Safe | 0 Needs Review | 0 Blocked ✅ SafeBrief or no lock, backwards compatible Last updated: 2026-06-10 18:38 UTC (af32ed1) |
Contributor
|
🎭 Playwright report · View test results → ❌ 1 failed test:
🔁 Flake verification failed (--repeat-each=10):
The report only shows the tests under verification. View report → Fix these before merging. These issues are not necessarily caused by your changes. |
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
1 participant
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
For "verified exceptions" (proving a
$exceptiongenuinely came from a customer's backend rather than being forged through the public ingest key), error-tracking ingestion needs to verify a signature against a key the customer controls. This adds the per-project registry of those public keys.Changes
ErrorTrackingSigningKeymodel — a team-scoped registry of Ed25519 public keys (only the public half is stored; the customer's backend holds the private key in its SDK). Mirrors theErrorTrackingSymbolSetpattern./api/environments/:team_id/error_tracking/signing_keys/) followingTeamAndOrgViewSetMixin. On create it validates the PEM is an Ed25519 public key and derives akey_id=base64url(sha256(raw_pubkey))[:16]that matches the SDK's derivation, so a signature's key id resolves to the stored key. Public key is write-once; onlylabel/revokedare mutable.Part of the verified-exceptions feature: PostHog/posthog-python#657 (SDK signs), cymbal verify (next PR).
How tested
Agent-assisted.
pytest products/error_tracking/backend/api/test/test_signing_keys.py(7 tests) passes. The key test asserts the server derives the samekey_idas the SDK's cross-language parity vector (Vkdap1RjR0wChd9d), locking the two derivations together; plus Ed25519/garbage rejection, team-scoping, revoke, and public-key immutability.