feat(web): support separate decryption keys - #139
Conversation
There was a problem hiding this comment.
Thanks for the thorough work here — the receiver-side flow (missing key → manual entry prompt) is exactly the right shape, and keeping the raw key out of React Query cache identities is a good catch. However, I'd like to take a different overall approach, which should significantly shrink this PR:
1. No configuration — always show both options
I don't want this to be deployment/config-driven. Instead, the create-success view should always show three fields:
- The combined URL (key in fragment) — current behavior
- The key-free URL
- The decryption key on its own
The sender chooses how to share at share time, rather than the deployer choosing at build time. A URL without a key already implicitly tells the recipient to enter it separately — the key prompt you built renders whenever the fragment is missing, so no query parameter and no flag are needed. This removes VITE_SEPARATE_DECRYPTION_KEY and with it the Dockerfile.web, docker-compose.yaml, README, config.ts, and config.test.ts changes.
2. Hard-break ?key= support instead of the compatibility layer
Rather than carrying the deprecated query-param format forward, let's remove support for it entirely (planned as its own release/changeset for changelog clarity — happy to coordinate sequencing). Rationale: secrets are TTL-bound so there's no long tail of old links, and a ?key= link has already leaked the key to server logs by the time the client sees it — reading it client-side only masks that. With the new key prompt, an old ?key= link degrades gracefully: the recipient lands on the prompt and can still enter the key manually.
That means legacyKeyBootstrap.ts and the legacy branches in resolveDecryptionKey can go. The one piece worth keeping is a small scrub: if a key query param is present, history.replaceState it away and show a warning toast that the link format is unsafe — without ever reading the value into app state. That's a few lines in main.tsx, not a module.
3. Please trim the unrelated changes
A few things in here are separate concerns that make the diff hard to review — details inline, but broadly: the clipboardCopy rework, the router remount machinery, and the generic createFailed error handling. Happy to see any of those as their own PRs with their own rationale. (Rewording existing locale strings is fine where this feature changes their meaning — e.g. success.description.main must change once auto-copy is removed — just avoid general copy edits beyond that.)
One change I do want to keep from your approach: dropping the automatic copy-to-clipboard on create. Letting the user explicitly choose what to copy is safer and fits the three-field design.
Core of what survives: buildSecretLinks (simplified to always return both URLs), the DecryptionKeyPrompt component and missing-key flow in View.tsx, the new locale keys, and keeping the key out of mutation cache identity. That's a much smaller, focused PR.
| @@ -1,5 +1,6 @@ | |||
| export const config = Object.freeze({ | |||
| API_URL: import.meta.env.VITE_API_URL ?? 'http://localhost:4321', | |||
| SEPARATE_DECRYPTION_KEY: import.meta.env.VITE_SEPARATE_DECRYPTION_KEY === 'true', | |||
There was a problem hiding this comment.
Drop — per the top-level comment, this shouldn't be config-driven. The success view will always show both URL forms plus the key. That removes this flag along with config.test.ts, the Dockerfile.web/docker-compose.yaml build args, and the README configuration section.
| userInitiatedFallback?: boolean; | ||
| } | ||
|
|
||
| export const clipboardCopy = async ( |
There was a problem hiding this comment.
Please revert this file (and its tests). With auto-copy removed entirely, every remaining call site is a user-initiated button click, so the userInitiatedFallback option has no caller that needs it. This was only ever a call-site concern.
| @@ -0,0 +1,101 @@ | |||
| import { stripLegacyKeyFromUrl } from './secretUrl'; | |||
There was a problem hiding this comment.
Remove — we're going to hard-break ?key= support instead (see top-level comment). The module-level import side effect and the setTimeout(0) capture-replay window are a lot of machinery to keep a format alive that already leaked the key by the time it reaches the client. Replace with a small scrub in main.tsx: if key is in the query string, replaceState it away and toast a warning — never read the value.
| const { id } = useParams({ from: '/$id' }); | ||
| const search = useSearch({ from: '/$id' }); | ||
| const navigationKey = useRouterState({ | ||
| select: (state) => state.location.state.__TSR_key, |
There was a problem hiding this comment.
state.location.state.__TSR_key is a private TanStack Router internal and will break on upgrade. With the legacy path gone I don't think the remount machinery (remountDeps + hashchange listener + composite instance key) is needed — ViewPage can read the hash on mount as before. If there's a concrete stale-key-across-navigation repro this is solving, let's look at that case specifically.
| * query parameter. QR codes intentionally remain key-free in both modes | ||
| * because they are commonly downloaded, screenshotted, and retained. | ||
| */ | ||
| export function buildSecretLinks({ |
There was a problem hiding this comment.
Drop the separateDecryptionKey flag; return both forms unconditionally, e.g. { combinedUrl, keylessUrl, qrUrl }. The resolveDecryptionKey legacy-query branches can also go with the ?key= hard break — resolution reduces to "fragment or manual entry."
| onError(error) { | ||
| toast.error(error.message); | ||
| onError() { | ||
| toast.error(t('create.errors.createFailed')); |
There was a problem hiding this comment.
Please keep toast.error(error.message) — swapping in a generic createFailed message loses actionable error info and is out of scope here (drops the new locale key too).
| <QRCodeSVG | ||
| ref={qrCodeRef} | ||
| value={createMutation.data.url} | ||
| value={createMutation.data.qrUrl} |
There was a problem hiding this comment.
Making the QR key-free unconditionally is a real behavior change for existing users — scanning is no longer sufficient to open a secret. I'd keep the QR encoding the combined URL for now; if we want a key-free QR option it should be its own discussion. (Open to revisiting once the three-field UI exists.)
| description: { | ||
| main: 'Your secret has been created and the URL has been copied to your clipboard', | ||
| password: 'Share the URL and password with the desired recipient', | ||
| main: 'Your secret has been created. Share the URL below with the intended recipient.', |
There was a problem hiding this comment.
On the locale changes: rewording existing strings is fine where this feature actually changes behavior — e.g. success.description.main has to change since we are dropping the automatic clipboard copy, and success.description.password reads better with the new share flow. Just keep rewording limited to strings whose meaning is affected by this feature (rather than general copy edits), so the five-locale diff stays reviewable.
| onChange={(e) => { | ||
| setPassword(e.target.value); | ||
| onChange={(event) => { | ||
| passwordRef.current = event.target.value; |
There was a problem hiding this comment.
The controlled→ref conversion of the password input (plus the e → event renames) is churn unrelated to this feature — please revert to keep the diff focused. The a11y additions (labels, aria-describedby, role="alert") are welcome though.
| @@ -0,0 +1,131 @@ | |||
| import { renderToStaticMarkup } from 'react-dom/server'; | |||
There was a problem hiding this comment.
Nit: string-matching renderToStaticMarkup output with every dependency mocked is brittle. Prefer @testing-library/react queries against rendered behavior. Fine as a follow-up.
2 participants
Summary
VITE_SEPARATE_DECRYPTION_KEYsetting, with Docker support, while preserving fragment links by default.#keylinks working and safely scrub deprecated?key=links before router initialization.Fixes #54
Security
Testing
pnpm --filter @crypt.fyi/web test— 6 files, 22 tests passedpnpm --filter @crypt.fyi/web lintpnpm typecheckVITE_SEPARATE_DECRYPTION_KEY=trueproduction web buildsServer integration tests were attempted but could not connect to Redis on this machine; no server source changed.
AI assistance
AI assistance: OpenAI Codex assisted with implementation and review. The final patch and test evidence were reviewed by the submitter, and the exact candidate also received an independent packet-only security and threat-model review with Grok 4.5 (no material findings).