feat(decrypt): show private signing attributes on the download page#257
Conversation
Recipients can now see (and crypto verifies) the sender's name, mobile number, and date of birth alongside the email — the attributes the sender disclosed under the *private* signing identity at sign time. Previously the download page only ever showed the public email even when the sender disclosed more. Three things were needed: 1. Work around privacybydesign/irmamobile#360. The irma app drops optional attributes when the empty `[]` alternative precedes the real one inside a discon. Reorder `SIGN_ATTRIBUTES` so the empty alternative is last in each disjunction, and expand the per- attribute `optional: true` shorthand by hand into `[[attr], []]` for the same reason. Bumping pg-js or any other client wouldn't help — the bug is on the wallet side. Revert once the irmamobile fix has shipped to the app stores. 2. Pick up the corresponding pg-js fix that captures the verified private signing identity returned by `unsealer.unseal(...)`. (Already landed upstream — bump pinned through the lockfile once the new pg-js version is released.) 3. Render the disclosed non-email attributes on the download page with their existing i18n labels (`filesharing.attributes.<type>`) under the already-translated `verifiedExtra` heading, in both the pre-decrypt Ready panel and the post-decrypt Done panel. The filtering/labeling logic lives in a tiny pure helper (`verifiedAttributesFor`) that is unit-tested with vitest. Also adds vitest as a devDep with a minimal config scoped to `src/**` so the project finally has a unit-test framework alongside the existing Playwright e2e setup. New `npm run test:unit` script.
|
Dobby has received the request! Routing to the right specialist now... |
![dobby-coder[bot]](https://avatars.githubusercontent.com/in/3365457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review
Surfaces non-email signed attributes via a small pure helper + tests. Correct for the gemeente/mobile/dateofbirth path, but SIGN_ATTRIBUTES also permits name disclosure via passport/idcard/drivinglicence (firstName + lastName each) whose filesharing.attributes.* labels are missing in src/lib/locales/{en,nl}.json — those rows will render the raw key as the <dt>. Tests pass.
Rule compliance
PR body references issue #28 ("Show optional sender attributes in email and download flow") implicitly but lacks a closing keyword — per pr-close-issue-keywords, add Closes #28 on its own line so merge auto-closes the tracker.
) * feat(decrypt): add trust-confirmation gate before files reach disk The download page used to auto-trigger browser downloads the moment decryption finished. Recipients now see a Confirm panel — same Done layout (banner + file list + pill chips of verified attributes) plus two neutral buttons — and decide whether to keep the files. Decline discards the in-memory blob; accept triggers the download as before. When the sender disclosed nothing beyond their email, a strong warning band appears above the buttons. Email alone is a weak claim (anyone with control of the mailbox could have signed), so the gate is the right moment to surface that risk. UX polish that came out of the same round: - Widen the column to 350px and let the Yivi QR fill it instead of capping at 330px — the column now visibly wraps the QR. - New `--pg-success` brand token (light/dark) used only for the accept button's hover/focus tint; decline tints to the existing `--pg-input-error`. - /debug/download-flow: ALL_STATES gains `Confirm` + `Discarded`, scenarios route through Confirm and pause there for the developer to click one of the trust buttons, force-state highlight tracks the live `downloadState` in real time, the page locks to one viewport height, and the global footer is hidden on `/debug/*`. Test: - New `isWeakSenderIdentity` helper alongside `verifiedAttributesFor`, unit-tested for null, email-only, with-private-attrs, and empty-value-attribute cases. * fix(decrypt): address review on trust-confirmation gate - Confirm gate showed doneMessageComplete ("files have been downloaded and decrypted") before files reach disk. Add a readyToDownload message and use it there; the banner no longer contradicts the gate and CTA. - Migrate the Ready (QR-scan) sender-section to the attr-chips markup. It still referenced the removed verifiedExtra i18n key and attr-list CSS, so disclosed attributes rendered as a literal key in an unstyled <dl>. - Scope isWeakSenderIdentity to senders that actually verified an email. A missing/unsigned sender no longer triggers the email-only warning, whose copy does not apply when there is no email to caveat. * feat(decrypt): warn on unsigned files and time-lock their download An unsigned file (no verifiable sender at all) is the weakest case of all, yet it previously showed no caution. Add a dedicated, stronger warning for it and force the recipient to read before they can accept: - isUnsignedSender() helper (sender has no verified email) + unit tests. - Confirm gate now branches: unsigned => louder trustWarnUnsigned band (thicker border, more saturated fill); email-only keeps the existing trustWarnEmailOnly band; verified senders show none. - For the unsigned case only, the download button starts disabled and fills left-to-right over 5s (TRUST_UNLOCK_MS) before activating, so the user cannot click through without pausing on the warning. Decline stays enabled throughout. Other cases remain instantly clickable. - New i18n key trustWarnUnsigned (en + nl). - debug/download-flow: replace the email-only toggle with a three-way sender-identity selector (strong / email-only / unsigned) and mirror the warning branch + time-locked button so the preview stays faithful; also sync its Confirm banner to readyToDownload and hide sender sections when there is no email. * refactor(decrypt): replace unsigned time-lock with a confirmation modal The 5s greyed/progress-bar download button read as broken UI. Replace it with an explicit confirmation step, and refine the warnings per review: - New shared UnsignedConfirmModal: on an unsigned file, "Download files" opens a modal (red warning, Cancel / Download anyway, Esc + click- outside to close, Cancel focused on open) instead of a single click. Signed files still download on one click. Used by both the download page and the debug preview. - Remove the time-lock state/effect/timer and the locked-button CSS. - Email-only warning is now orange (new --pg-warning token); unsigned stays red but is no longer bold, so severity reads through colour. - Strip em-dashes from both warning messages (en + nl). - New i18n keys: trustConfirmHeader / trustConfirmAccept / trustConfirmCancel.
|
/dobby what is the status of this PR, has it been superseded? Or is it still necessary? |
|
Short answer: still necessary, not superseded. #257 is the only PR carrying this work — the only other open PRs are the release-please train (#229) and this one. What's actually in the branch now
Dependencies
Before it can land
So: keep it — it's the live home of this feature. It just needs a rebase and the pending lockfile bump to be mergeable. |
|
/dobby fix the conflicts. the sign_attributes bug has been fixed in irmago 1.0. Also fix all your comments |
Resolve conflicts and address review feedback: - +layout.svelte: keep the debug footer gate around main's new ExternalLinkIcon footer. - global.scss: keep the new --pg-warning tokens alongside main's updated --pg-success values. - package.json/lock: keep test:unit, drop the removed test script, bump @e4a/pg-js to ^2.1.4 (picks up the pg-js#89 verified-sender fix). - signAttributes.ts: revert the irmamobile#360 ordering workaround now that the sign_attributes bug is fixed in irmago 1.0 (back to the empty [] alternative first and the optional: true shorthand). - verifiedAttributes.ts: drop the unused labelKey field (chips render values only) and match the email attribute by the .email.email suffix instead of a loose substring. - download & debug pages: derive verifiedAttributes once instead of calling verifiedAttributesFor on every render. - vitest.config.ts: drop the sveltekit() plugin for a minimal node config. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Merged
|
There was a problem hiding this comment.
Rules Dobby 2 — consolidated review (cycle 1)
Per-rule compliance sweep (all rules in the memory repo, checked via sub-agents): clean — no rule violations in this diff. Two correctness findings carry over from the code review and are confirmed against HEAD:
1. test:unit fails on a clean checkout (see inline comment on package.json). "test:unit": "vitest run" has no svelte-kit sync in front. Reproduced from a clean npm ci: it hard-fails with [TSCONFIG_ERROR] Tsconfig not found and runs 0 tests, because the root tsconfig.json extends the not-yet-generated .svelte-kit/tsconfig.json. After svelte-kit sync it passes (14 tests). So the test-plan checkbox "test:unit — tests pass" and the vitest.config.ts comment claiming the config "avoids depending on svelte-kit sync having run first" are both false. Fix: "test:unit": "svelte-kit sync && vitest run" (or point vitest at a standalone tsconfig).
2. CI never runs test:unit. .github/workflows/ci.yml has jobs for svelte-check, lint, nginx, release-please, build, and finalize — no vitest step. (Reported here rather than inline: ci.yml isn't touched by this PR, so the reviews API can't anchor a comment to it.) The 14 new unit tests never execute in CI, so regressions in verifiedAttributesFor — and the clean-checkout failure above — go uncaught. Add an npm run test:unit step (after svelte-kit sync) if these tests should gate PRs.
Sending back to PR Dobby to fix.
test:unit was `vitest run` with no `svelte-kit sync` in front, so on a clean checkout it hard-failed with `[TSCONFIG_ERROR] Tsconfig not found` (the root tsconfig extends the not-yet-generated .svelte-kit/tsconfig.json) and ran 0 tests. Prefix `svelte-kit sync` so it works from a bare `npm ci` and correct the vitest.config.ts comment that claimed otherwise. Addresses review feedback on PR #257. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Rules Dobby 2 — consolidated review (cycle 2)
Per-rule compliance sweep (every rule in the memory repo + the postguard-website repo notes, checked via Haiku sub-agents in parallel): clean — no rule violations in this diff. WCAG-AA (new modal/warnings/buttons have role, aria-*, focus management, :focus-visible, and non-colour-only signalling), vitest-mock-hoisting (no vi.mock in the new tests), and the @e4a/pg-js 2.1.4 dep-bump gotchas all pass.
Merging with the code review, one finding remains — a stale doc comment (see inline). The conflicts are resolved (PR is mergeable) and the sign_attributes path now works against pg-js 2.1.4, so this is the last item.
Requesting changes so the PR Dobby can reword the docstring; everything else is good.
Adds a test-unit job that runs npm run test:unit on every PR. The script is self-contained (svelte-kit sync && vitest run), so a bare npm ci is enough — no separate sync step needed.
The docstring claimed the unsigned-sender gate "time-locks the download button", but there is no time delay anywhere in the code. The gate shows the strongest trust warning and routes the download through an extra confirmation modal (UnsignedConfirmModal). Reword to match behaviour. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@rubenhensen addressed your three points:
Verification: |
|
/dobby review |
There was a problem hiding this comment.
Rules Dobby 2 — consolidated review (cycle 3)
Per-rule compliance sweep — all 95 memory-repo rules + the 2 postguard-website repo notes, checked via Haiku sub-agents in parallel: clean, no rule violations in this diff.
Merged with Review Dobby 2's findings: zero carried-over findings. All three items from the earlier cycles are confirmed fixed against HEAD:
- Merge conflicts resolved — PR is
MERGEABLE. sign_attributesback to the clean form —signAttributes.tsuses the{ optional: true }shorthand with the empty[]alternative first; the irmamobile#360[[attr],[]]workaround is fully reverted, matching the irmago 1.0 fix.isUnsignedSenderdocstring corrected — no longer claims a "time-lock"; now describes the extraUnsignedConfirmModalconfirmation gate.
Also still-good: test:unit runs svelte-kit sync && vitest run (passes from a clean checkout), CI now runs test:unit as a PR gate, WCAG-AA on the new modal/warnings, and the @e4a/pg-js ^2.1.4 bump is used in source.
Approving. 🧹
1 participant
Summary
Surfaces the sender's verified private signing attributes — name, mobile number, date of birth — on the download page, alongside the existing public email. Previously the recipient only ever saw the sender's email, even when the sender disclosed more at sign time.
Two pieces:
unsealer.unseal(...)instead of discarding it.verifiedAttributesFor) that is unit-tested; the email (public signing identity) is filtered out by its.email.emailsuffix and rendered separately.Also adds Vitest as a devDep with a minimal config scoped to
src/**so the project finally has a unit-test framework alongside the Playwright e2e setup. Newnpm run test:unitscript (svelte-kit sync && vitest run, so it works from a barenpm ci).Test plan
npm run test:unit— 14 tests pass forverifiedAttributesFor(from a cleannpm ci).npm run check— typecheck clean (0 errors, 0 warnings)./senddisclosing email + name + mobile + dateofbirth via Yivi. Open the download link in another session. Both the Ready (inspect) and Done panels show the private attributes as chips.Depends on encryption4all/postguard-js#89.
Closes the review feedback on this PR (unit-test bootstrap ordering, email-suffix match,
$derivedreuse, value-only chips).