Researcher encrypted results#764
Merged
Merged
Conversation
chrisbendel
commented
Jun 11, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the end-to-end “encrypted results” flow so that researchers can decrypt approved results/logs using their own key, without the server ever storing plaintext. It does this by having the reviewer decrypt artifacts in-browser, then re-wrap each inner file’s AES key to lab recipients and persist those wrapped keys in a new study_job_file_key table.
Changes:
- Add server-side “re-wrap sharing” mechanism (
study_job_file_key) and actions/queries to insert and read wrapped keys. - Rename “Reviewer Key” to role-neutral “Results Key” / “UserKey”, and extend key gating + permissions to lab researchers as well as enclave reviewers.
- Refactor encrypted-file UI and decryption to operate on whole-zip artifacts, support researcher re-wrapped keys, and remove per-file selection (all-or-nothing share).
Reviewed changes
Copilot reviewed 66 out of 70 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/study-flow.spec.ts | Updates e2e flow copy/behavior for results-key wording and all-or-nothing sharing. |
| src/server/storage.ts | Documents encrypted-results archive semantics; removes approved-plaintext storage helper. |
| src/server/results-sharing.ts | New: validates + persists re-wrapped per-recipient AES keys (study_job_file_key). |
| src/server/results-sharing.test.ts | Unit tests for sharing insert validation + idempotency. |
| src/server/db/queries.ts | Renames key lookup; adds lab-key and shared-artifact queries; expands job file metadata. |
| src/server/db/queries.test.ts | Updates tests for renamed key lookup and new shared-artifact query. |
| src/server/actions/user.actions.ts | Renames redirect flag; gates key generation for any org that needs a key. |
| src/server/actions/user.actions.test.ts | Adds test ensuring lab users without keys are redirected to key generation. |
| src/server/actions/user-keys.actions.ts | Renames actions to UserKey; adds SPKI validation and broader revalidation. |
| src/server/actions/user-keys.actions.test.ts | Updates tests for renamed actions and adds invalid-key rejection coverage. |
| src/server/actions/study.actions.ts | Switches from uploading plaintext approved files to persisting wrapped-key rows. |
| src/server/actions/study-job.actions.ts | Approve now inserts wrapped-key rows; adds actions to fetch lab keys and shared artifact ids; researcher fetch path supports wrapped keys. |
| src/server/actions/study-job.actions.test.ts | Updates tests for encrypted-file fetching and researcher wrapped-key filtering. |
| src/server/actions/org.actions.ts | Removes legacy reviewer-key action. |
| src/lib/types.ts | Adds orgNeedsKey; introduces SharedFile schema; adds rawAesKey to decrypted entries. |
| src/lib/types.test.ts | Adds unit tests for orgNeedsKey. |
| src/lib/routes/definitions.ts | Renames route reviewerKey → userKey. |
| src/lib/re-wrap-results.ts | New: client-side re-wrap builder using lab public keys. |
| src/lib/permissions.ts | Renames ability subject ReviewerKey → UserKey; grants key access to lab + enclave org members. |
| src/lib/permissions.test.ts | Updates ability tests to assert UserKey access for reviewer + researcher roles. |
| src/lib/permission-types.ts | Updates CASL subject union to UserKey. |
| src/lib/paths.ts | Updates non-org route prefixes to include user-key. |
| src/lib/file-type-helpers.ts | Removes legacy approved/encrypted conversion helpers; keeps mapping + labels. |
| src/hooks/use-profile-menu-disclosure.ts | Pins Routes.userKey in profile menu. |
| src/hooks/use-encrypted-files-panel.ts | Refactors encrypted-file panel state model to artifact-based view and shared-id lookup. |
| src/hooks/use-decrypt-files.ts | Updates decryption to support whole-zip bodies + wrapped researcher keys; extracts raw AES keys for re-wrap. |
| src/database/types.ts | Adds Kysely model/table entry for studyJobFileKey. |
| src/database/seeds/1743608138837_test_users.ts | Seeds valid test public keys for key-required org members; computes fingerprint inline. |
| src/database/seed-test-public-key.test.ts | Ensures embedded seed PEM matches tests/support/public_key.pem. |
| src/database/migrations/1780000000000_study_job_file_keys.ts | Migration creating study_job_file_key with uniqueness on (artifact, inner path, fingerprint). |
| src/components/study/submitted-code-table.test.tsx | Updates job-file fixtures to include id/path from expanded query. |
| src/components/require-user-key.tsx | Renames + broadens key gate to lab + enclave orgs. |
| src/components/require-user-key.test.tsx | Adds unit tests for researcher key-gating behavior. |
| src/components/require-reviewer-key.test.tsx | Removes legacy reviewer-key gate test. |
| src/components/require-mfa.test.tsx | Removes mocking of deleted legacy reviewer-key org action. |
| src/components/private-key-form.tsx | New shared private-key entry form component (reviewer + researcher). |
| src/components/otp-input.tsx | Adds autofocus to OTP input. |
| src/components/layout/navbar-profile-menu.tsx | Renames nav item label to “Results Key” and points to /user-key. |
| src/components/layout/app-shell.tsx | Switches app shell to new RequireUserKey gate. |
| src/components/job-results.tsx | Replaces plaintext results UI with reusable encrypted panel for researchers. |
| src/components/encrypted-files-panel.tsx | Removes per-file selection; uses shared PrivateKeyForm; updates status icons. |
| src/components/encrypted-files-panel.test.tsx | Reworks tests around whole-zip artifacts + new placeholder copy. |
| src/app/user-key/regenerate-key.tsx | New /user-key regeneration container. |
| src/app/user-key/regenerate-key.test.tsx | Updates regeneration test for renamed action/copy. |
| src/app/user-key/regenerate-key-view.tsx | Updates copy to “Results Key”; role-neutral key semantics. |
| src/app/user-key/regenerate-key-view.stories.tsx | Updates story metadata/title for new page naming. |
| src/app/user-key/page.tsx | Adds /user-key page. |
| src/app/user-key/layout.tsx | Updates layout to use userKeyExistsAction and redirect to /account/keys if missing. |
| src/app/dl/results/[jobId]/[fileName]/route.ts | Marks plaintext download route as legacy-only. |
| src/app/api/job/[jobId]/results/route.test.ts | Adds regression test preventing results overwrite after completion. |
| src/app/account/signin/sign-in-form.tsx | Uses new redirect flag for key generation. |
| src/app/account/signin/mfa.tsx | Uses new redirect flag + invite join key requirement rename. |
| src/app/account/keys/page.tsx | Uses userKeyExistsAction. |
| src/app/account/keys/generate-keys.tsx | Renames copy/actions to “Results Key” and uses new user-key actions. |
| src/app/account/keys/generate-keys.test.tsx | Updates tests for renamed key actions/copy. |
| src/app/account/invitation/[inviteId]/join-team/page.tsx | Uses renamed needsUserKey flag. |
| src/app/account/invitation/[inviteId]/create-account.action.ts | Renames key lookup and gates lab + enclave orgs via orgNeedsKey. |
| src/app/account/invitation/[inviteId]/create-account.action.test.ts | Updates tests for new needsUserKey behavior (including lab org). |
| src/app/[orgSlug]/study/[studyId]/review/study-review-buttons.tsx | Builds re-wrap payload client-side and submits as sharedFiles. |
| src/app/[orgSlug]/study/[studyId]/review/study-results.test.tsx | Updates results tests for encrypted/shared key-based visibility. |
| src/app/[orgSlug]/study/[studyId]/review/study-results-redesign.tsx | Removes legacy key label behavior; continues using encrypted panel. |
| src/app/[orgSlug]/study/[studyId]/review/study-results-redesign.test.tsx | Updates placeholder text and encryption test harness. |
| src/app/[orgSlug]/study/[studyId]/review/job-review-buttons.tsx | Approve now re-wraps and sends sharedFiles instead of plaintext job files. |
| src/app/[orgSlug]/study/[studyId]/review/job-review-buttons.test.tsx | Updates approve test to assert re-wrapped key rows are persisted. |
| src/app/[orgSlug]/dashboard/page.test.tsx | Removes mocking of deleted legacy reviewer-key org action. |
| pnpm-workspace.yaml | Adds hono override to remediate a dev-only transitive vuln flagged by scanning. |
| pnpm-lock.yaml | Locks updated hono and bumps si-encryption git ref. |
| package.json | Bumps pnpm version and updates si-encryption dependency ref. |
| bin/inject-encrypted-results.ts | New dev helper script to inject encrypted results and exercise the flow locally. |
| .npmrc | Adds confirm-modules-purge=false. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nathanstitt
reviewed
Jun 17, 2026
nathanstitt
left a comment
nathanstitt
left a comment
Member
There was a problem hiding this comment.
I see we're still storing the encrypted AES key that decodes the results in the DB for researchers vs in the zip like we do for reviewers.
It seems better to me to have a single way to store this, so either we store in the zip (reviewers), or in the db (researchers). Either has tradeoffs:
- zip lets us have a single blob that is self-contained and easy to pass between systems
- db row lets us potentially revoke access to a single user by deleting the row. However there's also other ways to revoke access, remove from org, or implement some other access rules.
I could be persuaded either way but think we should use the same mechanism for all users.
| @@ -90,15 +90,15 @@ export function NavbarProfileMenu() { | |||
|
|
|||
| <Protect role={AuthRole.Reviewer}> | |||
Member
There was a problem hiding this comment.
Now that lab researchers hold a Results Key too (orgNeedsKey), this nav link is still gated behind <Protect role={AuthRole.Reviewer}> — so a researcher won't see "Results Key" in their profile menu. RequireUserKey covers the first-time generate redirect, but a researcher who already has a key has no menu entry to reach /user-key and regenerate it. Worth widening the Protect (or adding a Researcher-visible entry) so the rotate path is reachable for both roles.
| 'in', | ||
| encryptedFiles.map((f) => f.id), | ||
| ) | ||
| .where('fingerprint', '=', userKey.fingerprint) |
Member
There was a problem hiding this comment.
This seems important to solve:
Researcher access is keyed to userKey.fingerprint. If a researcher rotates their key (the update flow swaps the fingerprint), this filter stops matching the study_job_file_key rows wrapped against the old fingerprint, and they permanently lose access to already-approved results.
Is this intended?
Contributor
Author
There was a problem hiding this comment.
Intended for now. Rotation swaps the fingerprint and orphans the old wrapped rows — the recovery path is the renewal/re-wrap flow (a teammate re-wraps for the new key), which is a separate workstream and not in this PR. Until it lands we're not surfacing self-rotate to researchers (first-time generate still works via the key-required redirect), and I'm filing renewal as the immediate follow-up. Tracking issue: .
| .handler(async ({ studyJob }) => { | ||
| .handler(async ({ studyJob, session, db }) => { | ||
| const userKey = await getUserPublicKey(session.user.id) | ||
| if (!userKey) return [] |
Member
There was a problem hiding this comment.
Small thing: getUserPublicKey does executeTakeFirst() over all of the user's rows, so if more than one userPublicKey row ever exists for a user the fingerprint chosen here is non-deterministic, which would silently mismatch the wrapped-key lookup below. Worth confirming there's a one-row-per-user invariant (unique constraint on user_id) backing this.
Contributor
Author
There was a problem hiding this comment.
Backed by a constraint — user_public_key has unique_user_id (migration 1742320602314), so it's one row per user. executeTakeFirst is deterministic and setUserPublicKeyAction's insert can only ever be first-time. Rotation is update-in-place on that row.
Contributor
Author
Fair push — and you're right that one source of truth matters. The thing is, on the read path it already is one: ResultsReader takes the DB-stored wrapped keys (additionalKeys, wired here as researcherKeys) and splices them into the manifest under the recipient's fingerprint before any decrypt. So decrypt is a single manifest.files[path].keys[fingerprint] lookup for everyone — there's no second code path, the DB rows just hydrate the same manifest the zip would've carried. Where they differ is write timing, not mechanism: Zip manifest = recipients that existed when the job ran in the enclave (reviewers). It's the immutable run artifact — we never rewrite it. To make that intent obvious in the schema, I'll rename study_job_file_key → study_job_file_recipient_key (and the param researcherKeys → recipientKeys) so it reads as "post-hoc recipient keys, any role," not "the researcher special-case." Decompose (pulling reviewer keys out of the zip into the DB too, so there's literally one store) is the other way to unify — but that's the version I just backed out of, and it means rewriting the prod ingest/decrypt path TOA depends on. Happy to revisit if you think single-store is worth that. |
nathanstitt
approved these changes
Jun 24, 2026
nathanstitt
left a comment
nathanstitt
left a comment
Member
There was a problem hiding this comment.
Thanks for your patience on this @chrisbendel - while I still think it'd be slightly better to have everything stored in the zip, I do see your reasoning for storing study-keys in the db. So I'm fine with it for now at least, we can easily revisit later if it does turn out to be an issue, but I doubt it will.
Thanks again!
Contributor
Total coverage
Detailed report58 files with a coverage regression
|
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Pairs with safeinsights/encryption#26
Test plan — researcher encrypted results
Setup — keys first (both at
/account/keys, save PEM)Flow
PENDING-REVIEW, jobCODE-SUBMITTED.CODE-APPROVED+JOB-READY, studyAPPROVED. (Code-approval gate. Do NOT skip — script's RUN-COMPLETE assumes it.)RUN-COMPLETE:FILES-APPROVED+ re-wrap fires.DB check after step 5
Expect 2 rows per researcher fingerprint:
results.csv+run.log. Empty before approve.Spot checks