Skip to content

feat: Add the new Keepiq browser extension (accounts, unlock, vault browsing) - #1242

Merged
SudoThijn merged 35 commits into
developmentfrom
feature/browser-extension
Oct 16, 2026
Merged

SudoThijn merged 35 commits into
developmentfrom
feature/browser-extension

Conversation

@SudoThijn

@SudoThijn SudoThijn commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Summary

First version of the new Keepiq browser extension, built with WXT so one source ships to Chrome (MV3) and Firefox (MV2). It replaces the old extension setup, which is removed together with its tests, docs media and release workflows.

  • Accounts and unlock (ext-accounts-and-unlock): add up to 5 accounts with a Nextcloud app password, unlock with the master password, lock and log out per account or all at once. Key material never touches disk.
  • Vault browsing (ext-vault-browse): syncs the vault and lists it with search, folder and type filters; copy user name, password or one-time code and open the site.
  • Toolbar icon states and badge spec (ext-toolbar-icon), Keepiq app icon.
  • English and Dutch translations (ext-i18n), following the browser's language.
  • Dev setup: npm run dev launches a browser with the extension loaded, plus a mock login test site.
  • Docs: browser-extension/README.md, ARCHITECTURE.md, SECURITY.md and the user guide in docs/browser-extension/using.md. Autofill, generator, send, settings and editing are specced in browser-extension/openspec/changes/ but not built yet.

How to test

cd browser-extension
npm ci
npm run dev            # or: npm run dev:firefox

That's it: a browser opens with the extension already loaded and the test site on http://localhost:8100.

  1. Open the Keepiq web app once and set a master password (if you haven't already).
  2. In Nextcloud, create an app password under Settings → Security.
  3. Click the Keepiq icon in the toolbar and add an account: your server URL, username and the app password. https is required, except for localhost, 127.0.0.1 and *.test.
  4. Unlock with your master password.
  5. Browse the Vault tab: search, filter by folder or type, copy a field.

The dev profile is kept in browser-extension/.wxt/chrome-data, so the account survives restarts.

Checks

  • ✅ npm test (457 tests), npm run typecheck, npm run lint, npm run build and npm run build:firefox pass locally

Test plan

  • CI passes
  • Add an account, unlock, browse and copy in Chrome and Firefox
  • Lock, unlock again and log out

🤖 Generated with Claude Code

Delete the old extension's tests, workflows (browser-extension, extension-release,
docs-media) and user docs. Move the test Nextcloud harness that mobile-e2e and
standalone use from browser-extension/capture to tests/server. Freeze the autofill
and passkey vectors that the old extension wrote.
WXT 0.21 made web-ext an optional peer dependency. Without it, wxt dev silently
falls back to its manual runner and never launches a browser.
…nd opens

One page per fill and capture case in the ext-autofill specs: iframes, shadow
DOM, multi-step, honeypot fields, framework-controlled inputs, sign-up, change
password and more, so nobody needs a real Nextcloud page to try the extension.
The dev browser keeps a persistent profile in .wxt/chrome-data.
Read it from xdg-settings on Linux and the registry on Windows, and use it for
the target it fits. Elsewhere WXT's own detection applies, and a local
web-ext.config.ts still overrides it.
…tabs

Let web-ext manage the persistent profile through chromiumProfile, so its
chromiumPref reaches the profile Brave opens, and set it to start on a new tab.
Also hide the crash restore prompt that stopping dev would trigger.
Grey when logged out, a lock when locked, and a count of the logins matching the tab when unlocked, as in Bitwarden.
Render public/icon from img/app-store.svg instead of the placeholder art, and base the toolbar icon variants in the ext-toolbar-icon spec on it.
…d-unlock)

Add the Keepiq API client, the crypto module (byte compatible with the web
app, tested against its vectors), the account store, the key store, the
timeout engine and the background message router, with a React popup for
adding accounts, unlocking, logging in again and switching accounts.

Every module and popup component has vitest tests. The popup treats a
reply that is not a Result as an error instead of rendering nothing, and
npm run dev clears the dev profile's service workers so Chromium runs the
current background.js instead of a stale registered one.
Support Nextcloud under a subpath by storing a server URL instead of an
origin, and drop .local from the plain http allow list. A wrong master
password on a cached suite fetches the suites once, so a password changed
in the web app unlocks. The background replies through sendResponse, which
every Chrome accepts, now that WXT ships no polyfill.

Log out asks for confirmation everywhere and the switcher buttons name
their account. The Add account draft lives in storage.session so it
survives the popup closing, and focus returns to the field to fix.

The "Never" key follows the timeout policy and settings changes, the
certificate parser and RSA decrypt reject malformed lengths, a 5xx reads
as the server being unavailable, and a CI workflow runs the extension's
tests, typecheck, lint and both builds.
Share, file, Talk and core links now reduce to the server URL. When the
check after a wrong master password finds the key withheld or no active
suite, that error is shown instead of "Invalid master password".
Fixed 380x600 shell with a header bar, cards on a tinted background and
brand-coloured tokens for light and dark. Inline SVG icons and logo, icon
password toggle, field hints, pill buttons with a busy spinner, banners
with icons, and a shared Confirm. The account switcher gets status dots,
icon actions and an options list; the header back arrow replaces the
Back and Cancel buttons.
Drop the avatar and status chips from the unlock and re-login screens,
the welcome block from Add account and the extra heading and status
dots from the switcher. The unlocked screen stays empty until
ext-vault-browse; Lock lives in the switcher, as in Bitwarden. The popup
keeps a fixed 380x600 size, and the specs now say so.
Sync the vault into one storage.local snapshot per account: the
manifest under 1000 secrets, otherwise the paged list with a second
pass when ties lose rows. A 15-minute probe reads the newest row,
the count and the suite list, so an unchanged vault downloads nothing
while a key rotation or the two-factor block is still noticed. Rows
on a revoked or compromised suite are blocked as the server does.

The popup gets the tab shell with Vault, Generator, Send and Settings,
the vault list with suggestions for the current site, search, folder
and type filters, lazy login subtitles, Launch and copy actions, and
the item detail with masked fields and TOTP codes. Decryption stays
in the background; the clipboard clears through an offscreen document
on Chrome and the background page on Firefox.

Account writes are serialised, and logout, unlock, sync and the
"Immediately" timeout no longer race each other. Server-side causes
are tracked in keepiq#1233, #1234 and #1235.
Add SECURITY.md and ARCHITECTURE.md for developers, and a user guide
and privacy policy for the extension in the docs site, next to the
mobile pages. The README lists the docs and no longer mentions a
release workflow the extension does not have.
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/keepiq @ 5c3083c

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ✅
psalm ✅
phpstan ✅
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-manifest ✅
test-l10n ✅
format ❌
check-l10n-js ✅
check-schema-l10n ✅
composer ✅ ✅ 114/114
npm ✅ ✅ 660/660
app:check-code ⏭️
info.xml ✅
REUSE ✅
lockfile sync ✅
PHPUnit ✅ 2130/2130 · 3 skipped
Newman ✅ 177/177 · 20 skipped
Playwright ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development.
Hydra gates ✅

Quality workflow — 2026-10-09 08:08 UTC

Download the full PDF report from the workflow artifacts.

ensure the AI knows that `npm run dev` is a watcher, and added a safety clause.
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/keepiq @ 282e84b

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ✅
psalm ✅
phpstan ✅
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-manifest ✅
test-l10n ✅
format ❌
check-l10n-js ✅
check-schema-l10n ✅
composer ✅ ✅ 114/114
npm ✅ ✅ 660/660
app:check-code ⏭️
info.xml ✅
REUSE ✅
lockfile sync ✅
PHPUnit ✅ 2130/2130 · 3 skipped
Newman ✅ 177/177 · 20 skipped
Playwright ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development.
Hydra gates ✅

Quality workflow — 2026-10-09 08:36 UTC

Download the full PDF report from the workflow artifacts.

…ev dependencies

Bump brace-expansion and source-map-js, and override shell-quote to ^1.11.0, since web-ext's fx-runner pins the vulnerable 1.10.0. The node-forge advisory has no patched release yet.
…unches

chrome-launcher writes its logs into the kept profile without creating it, so `npm run dev` failed with ENOENT whenever `.wxt/chrome-data` was missing.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/keepiq @ 8534540

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ✅
psalm ✅
phpstan ✅
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-manifest ✅
test-l10n ✅
format ❌
check-l10n-js ✅
check-schema-l10n ✅
composer ✅ ✅ 114/114
npm ✅ ✅ 660/660
app:check-code ⏭️
info.xml ✅
REUSE ✅
lockfile sync ✅
PHPUnit ✅ 2130/2130 · 3 skipped
Newman ✅ 177/177 · 20 skipped
Playwright ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development.
Hydra gates ✅

Quality workflow — 2026-10-12 10:24 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/keepiq @ 475b86f

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ✅
psalm ✅
phpstan ✅
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-manifest ✅
test-l10n ✅
format ❌
check-l10n-js ✅
check-schema-l10n ✅
composer ✅ ✅ 114/114
npm ✅ ✅ 660/660
app:check-code ⏭️
info.xml ✅
REUSE ✅
lockfile sync ✅
PHPUnit ✅ 2130/2130 · 3 skipped
Newman ✅ 177/177 · 20 skipped
Playwright ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development.
Hydra gates ✅

Quality workflow — 2026-10-12 12:14 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/keepiq @ c988b29

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ✅
psalm ✅
phpstan ✅
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-manifest ✅
test-l10n ✅
format ❌
check-l10n-js ✅
check-schema-l10n ✅
composer ✅ ✅ 114/114
npm ✅ ✅ 660/660
app:check-code ⏭️
info.xml ✅
REUSE ✅
lockfile sync ✅
PHPUnit ✅ 2136/2136 · 3 skipped
Newman ✅ 177/177 · 20 skipped
Playwright ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development.
Hydra gates ✅

Quality workflow — 2026-10-12 13:10 UTC

Download the full PDF report from the workflow artifacts.

@remko48 remko48 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST CHANGES (Standard)

The new WXT extension is carefully built where it matters most. The KDF, envelope and RSA chunking match the web app byte for byte, the key lives in storage.session only, there is a guard for an unlock racing a logout, and API calls run from the background with credentials: 'omit'. Two things block the merge: one breaks an existing Keepiq security requirement, and one is the PR's own red CI. Each finding is an inline comment below, except the one about commit messages, which has no line to anchor on.

  • 🔴 Blocker — The extension ignores useOnly, so it shows and copies the value of use-only shares (inline, ItemCard.tsx)
  • 🔴 Blocker — The PR's own edit to crypto-vectors-kotlin.spec.js fails prettier, so CI is red (inline)
  • 🟡 Concern — A content script that does nothing yet asks for every website at install (inline, content.ts)
  • 🟡 Concern — The repo's records still describe the deleted extension (inline, tests/server/setup.sh)
  • 🟡 Concern — The Firefox add-on ID changes to a personal handle (inline, wxt.config.ts)
  • 🟡 Concern — No commit on the branch follows the Conventional Commit format (below)
  • 🟢 Minor — The store description promises features this version lacks (inline, locales/en.yml)
  • 🟢 Minor — Copied passwords are never cleared (inline, clipboard.ts)

🟡 Concern — No commit on the branch follows the Conventional Commit format

All 26 commits lack a type prefix, for example "replaced old bad setup with my setup", "updated claude.md" and "Implement accounts, unlock and lock (ext-accounts-and-unlock)". The PR is merged with a merge commit, so these enter history as they are, and the feat: title never becomes a commit.

Impact: the feature gets no version bump or changelog entry, and history keeps messages like "my setup".

Suggested fix: reword or squash on the branch before merging.

What was verified

  • Read the full diff outside browser-extension/: workflows, root package.json, vitest.config.js, vectors, and the move of tests/server.
  • Read the extension's core: src/crypto/*, src/vault/{key-store,unlock,sync,timeout,store,match,url}.ts, src/accounts/*, src/api/client.ts, src/background/*, src/clipboard.ts, the entrypoints, wxt.config.ts, package.json and .npmrc.
  • Checked that the KDF (PBKDF2-SHA256, 600k iterations) and the envelope layout match src/crypto/aes.js and src/crypto/envelope.js.
  • Ran the Hydra gates on feature/browser-extension (52c3754): 85 of 86 applicable gates green. Gate 4 (composer-audit) did not run.
  • Reproduced the prettier failure locally, and read the logs of the failing CI jobs.

Not run or not reviewed

  • The extension's npm test, typecheck, lint and builds. I relied on the green check-and-build CI job.
  • Loading the extension in Chrome or Firefox. Chrome's permissions.contains behaviour with content-script matches is reasoned, not tested.
  • The popup UI components, popup.css, the test files, the nested openspec/ documents and the committed .claude/ skills, beyond spot checks.
  • The failing ios job (mobile-e2e accessibility audit). It ran because tests/server/** changed. I assume it is unrelated to this diff, but did not confirm that.

Comment thread browser-extension/entrypoints/popup/components/ItemCard.tsx Outdated
Comment thread tests/vitest/crypto-vectors-kotlin.spec.js Outdated
import type { ContentToBackground } from '@/src/messages'

export default defineContentScript({
matches: ['*://*/*'],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Concern — A content script that does nothing yet asks for every website at install

This matches *://*/*, and today it only sends page_ready, which the background drops. Content-script matches count as host access, so Chrome and Firefox show "read and change all your data on all websites" at install. That cancels the per-server grant ADR-003 designs around (optional_host_permissions in wxt.config.ts). I also expect Chrome to count those matches as granted hosts, which would make the permissions.contains guard in src/background/router.ts:49 always pass. I reasoned this from the diff and did not reproduce it.

Impact: a wider install prompt and a harder store review than this version needs, and the "nothing is sent to a server the user declined" check may no longer check anything.

Suggested fix: leave the content script out until ext-autofill ships, or register it at runtime (registration: 'runtime') once the user grants it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're keeping the content script as it is, on purpose.

ext-autofill needs it. If we leave it out now and add it later, everyone who already installed the extension suddenly gets a new "all websites" permission prompt on an update, which looks far more suspicious than asking once at install. This version also isn't going to be published as it is.

On the guard: we checked the browser sources, and permissions.contains still reflects the user's per-server grant on both builds.

  • Chrome MV3: PermissionsContainsFunction::Run (extensions/browser/api/permissions/permissions_api.cc) requires explicit_hosts to contain the origin, and UnpackOriginPermissions puts our origin in optional_explicit_hosts. Content-script matches only ever become scriptable_hosts, so contains is false until the user grants the server.
  • Firefox MV2: contains checks extension.allowedOrigins (toolkit/components/extensions/parent/ext-permissions.js), which holds manifest host permissions (none), the extension's own URL and optional origins the user granted. Content-script matches are never added.

One caveat: in Firefox MV3 the install-time origins do include content-script matches, so the guard would need another look if Firefox ever moves to MV3. Today it builds as MV2.

Comment thread tests/server/setup.sh
BASE="http://localhost:${PORT}"
CONTAINER="${PROJECT}-nc-1"
OUT="${APP_DIR}/browser-extension/capture/out"
OUT="${APP_DIR}/tests/server/out"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Concern — The repo's records still describe the deleted extension

The test server moved here from browser-extension/capture, but "Remove references to the old browser extension" (321f529) missed these:

  • mobile/README.md:97,100 and mobile/e2e/server.mjs:9,44 still point to browser-extension/capture. mobile/README.md:41 runs tests/vitest/autofill-vectors.spec.js, which this PR deletes.
  • openspec/parity/capabilities.json (around lines 1396, 2364, 3804, 6632 and 8702) still cites deleted files as evidence that passkey creation, biometric unlock, use-only fill and the store release are delivered.
  • Open root changes (clients-extension-firefox-and-safari-builds, vault-website-addresses task 1.5, health-site-security-checks task 3.1) have tasks against browser-extension/src/lib/match.js and build.mjs, which no longer exist.
  • The Kotlin core names the deleted JS as its reference implementation (SiteMatch.kt, PinUnlock.kt, ItemCodec.kt, …). crypto-vectors-kotlin.spec.js still has a describe block "passkeys between the phone and the browser extension".

Impact: the parity record claims features that no longer exist in any browser client, and the mobile README commands fail.

Suggested fix: downgrade the affected entries in capabilities.json, fix the mobile README and server.mjs paths, and decide whether the open root changes move to the new extension's specs or close.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partly fixed, partly left on purpose.

  • Fixed in 159fb53 (and 0689bce): openspec/parity/capabilities.json no longer cites the deleted extension. All 21 dead evidence paths are gone, and the browser rows are rated on what the new extension does: autofill, TOTP fill, passkeys, generator and send are back to specified against their ext-* changes, and use-only, unlock and browsing are partial where only part ships.
  • Left as is: the mobile and Kotlin references. The mobile apps have nothing to do with the browser extension, so this PR changes only what their tests need, which is nothing: mobile-e2e.yml and standalone.yml already use tests/server, and the android and ios jobs pass. The leftovers are comments and README notes, not something that runs.
  • Left as is: the open root changes. They belong to the old extension's work and get resolved as the new extension's specs take over those slices.

Comment thread browser-extension/wxt.config.ts Outdated
Comment thread browser-extension/locales/en.yml Outdated
# The source catalog. Every key here must exist in every other catalog; locales.test.ts checks it.
# The BCP 47 tag of this catalog, for dates, numbers and `<html lang>`.
language: en
extensionDescription: Browser extension for Keepiq, the encrypted secrets manager for Nextcloud. Fill in logins, passkeys and one-time codes on any site, and save new ones as you go. Everything is encrypted — your master password and your secrets never reach the server.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Minor — The store description promises features this version lacks

This text (also in package.json) says "Fill in logins, passkeys and one-time codes on any site, and save new ones as you go". This version only browses and copies.

Impact: store reviewers treat a description that overclaims as misleading.

Suggested fix: describe what ships now, and widen the text with ext-autofill.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're keeping the promise in this text on purpose. The store description is marketing for the product, not a changelog of what this build does, and we don't want to rewrite it every time a feature lands. This version also won't go to a store as it is.

Your comment did surface a real problem: the text was 248 characters (255 in Dutch), and Chrome caps the manifest description at 132. 5543af4 shortens it to 117 in both languages with the same message: "Keepiq for your browser. Fill in logins, passkeys and one-time codes from your Nextcloud vault, end-to-end encrypted."

Comment thread browser-extension/src/clipboard.ts Outdated
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/keepiq @ dd28a4d

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ✅
psalm ✅
phpstan ✅
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-manifest ✅
test-l10n ✅
format ❌
check-l10n-js ✅
check-schema-l10n ✅
composer ✅ ✅ 114/114
npm ✅ ✅ 660/660
app:check-code ⏭️
info.xml ✅
REUSE ✅
lockfile sync ✅
PHPUnit ✅ 2321/2321 · 3 skipped
Newman ✅ 177/177 · 20 skipped
Playwright ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development.
Hydra gates ✅

Quality workflow — 2026-10-15 15:09 UTC

Download the full PDF report from the workflow artifacts.

The background gives a use-only copy's holder its login and refuses its
key and additional fields with `use_only`, so the rule holds whatever
the popup asks. The popup offers only the username to copy and shows a
use-only item's login, site and a note instead of its value.
A copy with no stored delay, or an invalid one, now clears after 30 s; a stored 0 means never.

Every copy also cancels a clear still pending from an earlier copy, so that clear can't wipe a newer copy made with Never.
The old extension is deleted, and its files were still cited as proof that fill, save prompts, passkeys, biometric unlock, use-only fill, the generator and sends were built.

Rows the new extension doesn't deliver now read no and specified, pointing at the ext-* change or root spec that describes them. Rows it does deliver (accounts, lock, account linking, browsing, the offline snapshot, the Chrome and Firefox builds) cite its files instead. Rows where the old extension only supplied part of the evidence keep their web app evidence; crypto-09 and sharing-24 drop to partial.
@SudoThijn
SudoThijn force-pushed the feature/browser-extension branch from c9a71dd to 0689bce Compare October 16, 2026 08:18
@SudoThijn

Copy link
Copy Markdown
Contributor Author

@remko48 on the commit-message concern from your review: every commit on the branch now follows the Conventional Commit format, merges included. Only the messages were rewritten (force-pushed); the trees and the merges from development are identical, so nothing in the diff changed.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/keepiq @ a7ce2c2

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ✅
psalm ✅
phpstan ✅
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-manifest ✅
test-l10n ✅
format ✅
check-l10n-js ✅
check-schema-l10n ✅
composer ✅ ✅ 114/114
npm ✅ ✅ 660/660
app:check-code ⏭️
info.xml ✅
REUSE ✅
lockfile sync ✅
PHPUnit ✅ 2321/2321 · 3 skipped
Newman ✅ 177/177 · 20 skipped
Playwright ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development.
Hydra gates ✅

Quality workflow — 2026-10-16 08:27 UTC

Download the full PDF report from the workflow artifacts.

@remko48 remko48 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE (Standard, re-review round 1)

Re-reviewed at 0689bce. Both blockers are fixed, with tests. Of the other findings, three are fixed, and three were declined or partly declined with reasons I can accept. Two small points remain as inline comments, and neither holds the PR.

Fixed

  • 🔴 Use-only values (70abe72, 5966797, 0689bce): the background now enforces the rule. item.decrypt answers use_only for any field beyond login (USE_ONLY_FIELDS), and a batch leaves the row out. The card offers only "Copy username", and the detail shows the login, the site and a note, with no TOTP code or payload. useOnly does reach the snapshot: OfflineManifestService spreads $secret->jsonSerialize(), which carries it. Tests cover the refusal, the batch, toItemMeta and both views. The updatedAt gap for a share switched to use-only later is documented in SECURITY.md. It would help to link the server-side follow-up issue from there once it exists.
  • 🔴 Prettier (525aea2): the format check is green.
  • 🟡 Firefox ID (a3ce65f): keepiq@conduction.nl again.
  • 🟡 Commit messages: all 25 branch commits are now Conventional Commits. I checked the rewrite: the rewritten pre-merge commit fbd4bf2c has the same tree as the reviewed 52c3754, and the merge from development (533059f) matches git's automatic resolution exactly.
  • 🟢 Clipboard (f9ab717): 30 seconds by default, and 0 means Never. Each copy now replaces the pending clear, which also fixes an earlier clear wiping a newer copy.
  • 🟡 Parity record (159fb53): capabilities.json is valid JSON and no longer cites any deleted extension file.

Accepted as left

  • 🟡 Content script on *://*/*: the reasoning holds. Adding host access in a later update would disable the extension in Chrome until the user approves the new permission, which is worse than asking once at install. The source walk-through of permissions.contains (explicit hosts vs scriptable hosts in Chrome, allowedOrigins in Firefox MV2) answers my doubt about the guard.
  • 🟢 Store description: fine as product copy, given this build won't go to a store as it is. The cut to 117 characters (Chrome caps it at 132) was a real catch.
  • 🟡 Open root changes: fine to resolve them as the ext-* specs take over.

Remaining (inline)

  • 🟢 Minor — The mobile README still points at paths this PR removed (on tests/server/compose.yaml)
  • 🟢 Minor — The Firefox MV3 caveat on the permission guard exists only in this thread (on router.ts)

What was verified

  • Read the full diff of the 9 fix commits (533059f..0689bce), including the new tests.
  • Checked that the force-push only changed messages (same tree) and that the merge from development was clean.
  • Hydra gates on 0689bce: 85 of 86 applicable gates green. Gate 4 (composer-audit) did not run.
  • CI on 0689bce is green, including the android, ios and format jobs.

Not run

  • The extension's own tests and builds were not run locally, and it was not loaded in a browser.
  • The Chromium and Firefox source citations on permissions.contains were taken as given, not checked against the sources.

Comment thread tests/server/compose.yaml
#
# docker compose -p kq-media -f browser-extension/capture/compose.yaml up -d
# bash browser-extension/capture/setup.sh kq-media . <openregister dir>
# docker compose -p kq-test -f tests/server/compose.yaml up -d

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Minor — The mobile README still points at paths this PR removed

This file moved here from browser-extension/capture/compose.yaml, but mobile/README.md:97,100 and mobile/e2e/server.mjs:9,44 still name the old path. mobile/README.md:41 still lists tests/vitest/autofill-vectors.spec.js, which this PR deletes. These aren't code, but a developer who follows the README gets a missing file.

Suggested fix: a one-line path swap in each place. It can go in a follow-up if you'd rather keep this PR to the extension.

const normalized = normalizeServerUrl(serverUrl)
if (!normalized.ok) throw new Failure(normalized.code)
// The popup asks for the permission; nothing is sent to a server the user declined.
if (!(await browser.permissions.contains({ origins: [`${normalized.origin}/*`] }))) throw new Failure('permission_denied')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Minor — The Firefox MV3 caveat on the permission guard exists only in this thread

This guard holds because Firefox builds MV2. If Firefox moves to MV3, its install-time origins would include the content-script matches (your note on the content.ts thread), and contains would pass for every server.

Suggested fix: add a short comment next to the guard, so whoever switches the manifest version sees it.

@SudoThijn
SudoThijn merged commit 85f8dc4 into development Oct 16, 2026
55 checks passed
@SudoThijn
SudoThijn deleted the feature/browser-extension branch October 16, 2026 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants