fix(sharing): keep a recipient without write permission from editing their copy by SudoThijn · Pull Request #1302 · ConductionNL/keepiq

SudoThijn · 2026-10-20T13:46:05Z

Summary

Closes #1287. The web app let a recipient edit a shared secret they have no write permission on, and saved the change to their own copy only, without saying so. A copy is now view-only when its holder isn't a writer on the source: the write-context grade is not owner, write or manage (a team-folder Viewer, a direct-share recipient, and today a group member). This is the same rule share-save and the browser extension already use.

Web app

src/utils/copyGrade.js holds the rule (isViewOnlyCopy).
SecretDetailSidebar.vue hides Edit, version Restore and "Ask for new values" on a view-only copy, and shows "Shared with you to view only. Ask its owner for edit rights to change it." Move, favourite, archive, share and delete stay. A use-only copy keeps only its own note.
load() fetches the secret and its write-context together and drops a late answer or failure for a secret the sidebar has left, so a view-only flag never carries over and Edit doesn't flash.
SecretEditDialog.vue shows the same note and keeps Save disabled, for a dialog opened before write-context answered.
saveShared throws ShareSaveError('view_only') before sending anything, in place of the own-copy PUT and version-restore fallback.
Offline replay fails a view_only entry without retrying it; Copy value and Discard stay. Edit still shows offline, since the grade isn't known then.
Two new strings, translated in every locale.

Server

ViewOnlyCopyGuard refuses a change of key, login or additionalFields on a view-only copy with ViewOnlyCopyException (code view_only, HTTP 428 over OCS). A field resent with its stored value doesn't count, and other fields such as folderId, name and URL still save.
It runs in SecretService::update (after loadForUpdate, so share-save's not_allowed answer is unchanged), SecretVersionService::restore and SecretRequestService::writeFilledValues. A fill is refused before anything goes to the pending list, and the request stays pending.
SecretVersionController::restore answers 403/404 (428 over OCS) for the refusals, and SecretRequestFillController answers 428 forbidden instead of a 500, also when the copy's source can't be found. That also covers a fill onto a use-only copy or a partner's read-only copy, which answered 500 before.
PUT /api/v1/secrets/{id}/sync refuses an id that is a received copy with 428 forbidden, since a sync starts from the source. Before, a holder's own copy passed as its own source and the sync wrote it alone.
The guard is a required dependency of all three services, and decides from ShareAuthorizationService (owner, delegate or a team-folder write grade, the rule resolveSyncWriter uses), so it stays out of the sync's dependency graph.

Docs

openspec/specs/user-sharing/spec.md: new requirement "View-Only Copies", which also says a sync naming a copy is refused and that a suite migration re-seals a copy without a server check.
folder-permission-grades, secrets-write-ui, secret-version-history, secrets, secret-requests and offline-edit-queue main specs, and the matching parts of openspec/changes/sharing-atomic-share-save/ (D1, D7 and the delta specs).
openspec/parity/capabilities.json (vault-18) and a CHANGELOG Fixed entry.

Tests

vitest: npm test 1818 passed, including new SecretDetailSidebar.viewOnlyCopy.spec.js and copy-grade.spec.js, and updated share-save, offline-queue and edit-dialog specs.
PHPUnit: 2365 tests; new ViewOnlyCopyGuardTest, and additions to the SecretService, SecretVersion, SecretRequest, ShareService (sync onto a copy) and controller tests. The only error is SuppressionHygieneTest, a container permission problem on browser-extension/.wxt; it passes on the host.
phpcs, phpmd, phpstan, eslint, prettier, stylelint and the l10n checks pass. Psalm crashes while scanning composer autoload files, outside project code.
e2e: a new view-only case in tests/e2e/workflows/share-save.spec.ts (no Edit, the note, move and archive still work, a value PUT gets 428 view_only, a folder-only PUT gets 200, values unchanged). It has not been run yet: the local dev instance fails vault enrolment ("HMAC does not match"), and PR CI skips Playwright (e2e-promotion-only), so it needs a manual Code Quality run on this branch before merging.

Notes for review

Group members are view-only until Members of a group a secret is shared with can't save changes for everyone #1285 makes them writers. Whichever of the two merges second updates the "View-Only Copies" wording and the CHANGELOG entry, points the guard at ShareWriteContext::standingOn and keeps assertNotACopy in the rewritten syncUpdate.
A view-only holder can't rename their copy or change its URL, the same as a partner's read-only copy. The server would allow it; it's a product decision whether the web app should.

Follow-ups (not in this PR)

A fill onto a partner's read-only copy stores extra fields in the pending list before it's refused.
Requests for new values can still be created on a view-only copy, and can never be filled.
A fill onto a write-grade holder's copy writes that copy alone.
The offline manifest doesn't carry the grade, so Edit shows offline on a view-only copy.
Recipients get the owner's controls on their copy and can share it onward #1298, A partner's read-only copy can be rolled back through version restore #1299 and A value change by plain PUT reaches one row only, and the mobile app saves only that way #1300 were filed from what turned up while working on this.

…their copy A recipient whose write-context grade isn't owner, write or manage no longer gets Edit, version Restore or "Ask for new values" in the web app, and sees a view-only note instead. `saveShared` refuses such a save with `view_only` instead of falling back to the own-copy PUT, so offline replay and version restore can't split the copy from its source either. The server refuses a change of `key`, `login` or `additionalFields` on such a copy through `ViewOnlyCopyGuard`, in the plain update, the version restore and the request fill, with 428 `view_only`. Other fields still save. Closes #1287

github-actions · 2026-10-20T13:51:56Z

Quality Report — ConductionNL/keepiq @ `9ba1777`

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					✅ 2360/2360 · 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-20 13:51 UTC

Download the full PDF report from the workflow artifacts.

rjzondervan · 2026-10-20T15:53:07Z

+			{ key: 'Never-saved-view-1!', cert: certificate },
+		)
+		const refused = await api(bob.page, 'PUT', `/secrets/${bobCopy}`, { key })
+		expect(refused.status, JSON.stringify(refused.body)).toBe(428)


🟡 Concern — the only end-to-end check of the server refusal has not been run, and PR CI won't run it

The PR says the new view-only case "relies on CI". In code-quality.yml (:112-126, :202-205), Playwright is kept off ordinary development PRs (e2e-promotion-only). On b99e7adb the quality / E2E Tests (Playwright…) check is skipped, and standalone.yml runs only standalone-shell.spec.ts. So nothing runs this case before the merge. That includes the 428/view_only mapping through OcsRefusalMiddleware, which no unit test covers end to end.

Impact: the HTTP contract the specs promise (PUT → 428 view_only, folder-only PUT → 200) and the UI hiding Edit are first exercised on the promotion PR into beta.

Suggested fix: run the case before merging, either locally once enrolment works or with a workflow_dispatch of Code Quality on this branch (code-quality.yml:43), and link the run. Correct the PR body's "it relies on CI".

Done when:

a linked Playwright run on b99e7adb or later shows a direct-share recipient sees their copy as view only and still files and archives it passing

Still open at a739abe: the PR body now says the case has not run and needs a manual Code Quality run, which is accurate. But there is no run yet: the only Code Quality runs on this branch are pull_request runs, where Playwright is skipped. Resolve this once a dispatched run that passes the case is linked.

rjzondervan

Review — Strict, REQUEST_CHANGES at `b99e7ad`

The guard idea is right, and the client side (saveShared refuses before any request, a view_only entry fails offline, the sidebar drops late answers) checks out. Two blockers remain on the server side.

🔴 Blockers

f1: PUT /api/v1/secrets/{copyId}/sync still lets a view-only holder overwrite their own copy. The copy passes as its own sync source, and nothing in that path calls the guard. This is not the gap #1300 describes.
f2: ViewOnlyCopyGuard → ShareSyncService closes a DI cycle once #1294 is merged as well. Without lazy ghosts (PHP 8.3, which info.xml supports), every secret endpoint answers 500, and unit-test CI cannot see it.

🟡 Concerns

f3: the guard is optional (?->), so a container that doesn't inject it lets the write through.
f4: the new e2e case has never run, because PR CI skips E2E (e2e-promotion-only). The body's "relies on CI" does not hold.

🟢 Minor

f5: fill answers 500 (NotFound) or 428 forbidden, not the 403 the body states.
f6: the spec's MUST does not mention the suite-migration re-seal.

Merging with #1294: besides f2, there are text conflicts in openspec/specs/user-sharing/spec.md and tests/e2e/workflows/share-save.spec.ts. ViewOnlyCopyGuardTest.php:242 passes suiteMapper:, which #1294's ShareSyncService no longer accepts.

CI and Hydra Gates are green. The persistence audit was not run.

…required and out of the sync's graph - `PUT /api/v1/secrets/{id}/sync` refuses an id that is a received copy, so a view-only holder can't write their own copy through it. - ViewOnlyCopyGuard decides from ShareAuthorizationService instead of ShareSyncService, which closes the DI cycle with #1294. - SecretService, SecretVersionService and SecretRequestService take the guard as a required parameter. - A fill whose copy can't be resolved answers the neutral refusal instead of 500.

…ion re-seals a view-only copy unchecked

github-actions · 2026-10-21T09:22:11Z

Quality Report — ConductionNL/keepiq @ `94c09fb`

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					✅ 2362/2362 · 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-21 09:22 UTC

Download the full PDF report from the workflow artifacts.

rjzondervan · 2026-10-21T09:22:51Z

+			return;
+		}
+
+		$grade = $this->auth->resolveGrade(secret: $source, userId: $userId);


🟡 Strict blocker — Once #1294 is merged, the guard's write rule disagrees with it, and a group member who may edit is told "view only"

This copies the rule resolveSyncWriter uses on development: owner, delegate, or a team-folder write/manage grade. The comment calls it "the rule a sync is authorized on", which makes keeping the two in step an obligation. #1294 widens that rule. A member of a group the source is shared with in full now writes for everyone (ShareWriteContext::standingOn → writesOn GROUP_WRITE). I checked the tree from git merge-tree --write-tree a739abe2 306dd9b5:

A group member's write context answers effectiveGrade: 'write', so isViewOnlyCopy (src/utils/copyGrade.js) returns false. The web app then shows Edit, Restore and "Ask for new values" on their copy.

Restore (SecretVersionService::restore) and a fill (SecretRequestService) still go through this guard. It finds no owner, delegate or team-folder grade and throws ViewOnlyCopyException, so they get 428 "Shared with you to view only. Ask its owner for edit rights to change it."

The specs disagree as well. Here, user-sharing "View-Only Copies" says a group-share recipient holds a view-only copy. In fix(sharing): let group members save a shared secret for everyone #1294, sharing-group says that member saves for everyone. This file merges without a text conflict, so nothing forces anyone to reconcile the two rules.

Suggested fix: in whichever of #1294 / #1302 merges second, decide through the write context, not a copy of the rule:

$standing = $this->contexts->standingOn(source: $source, userId: $userId); if ($this->contexts->writesForEveryone(standing: $standing) === false) { throw new ViewOnlyCopyException(); }

ShareWriteContext depends only on ShareTargetMapper, EncryptionSuiteMapper and ShareAuthorizationService, so this creates no cycle. On the merged tree no constructor path leads from the guard back to SecretService, SecretVersionService or SecretRequestService. Also align the "View-Only Copies" requirement with sharing-group: a group-share recipient's copy is view-only only when no full, live group share covers them.

Done when:

on the tree that has both PRs, a ViewOnlyCopyGuardTest case with a group member of a full, live group share passes the guard, and a case where that share is use-only or expired throws ViewOnlyCopyException

git grep -n "WRITE_GRADES" -- lib/Service/ViewOnlyCopyGuard.php returns nothing

rjzondervan

Verdict: REQUEST_CHANGES (Strict, round 2)

Both 🔴s from round 1 are fixed and verified, and so are f3, f5 and f6. Two 🟡s block: f4 (the e2e case still hasn't run) and the new k1 (the guard's write rule disagrees with #1294 once both are merged). Before a merge: both fixed, their threads resolved, and one approval.

Round	Found	Status
1 (`b99e7ad`)	🔴 2 · 🟡 2 · 🟢 2	5 resolved in `16f472f` / `a739abe`; f4 open
2 (`a739abe`), this review	🟡 1	k1 open

Finding	Was	Now	Where
🔴 `/sync` on the copy's own id still writes a view-only copy (f1)	🔴 round 1	resolved in `16f472f`	reply
🔴 `ViewOnlyCopyGuard → ShareSyncService` DI cycle with #1294 (f2)	🔴 round 1	resolved in `16f472f`	reply
🟡 The guard is optional and fails open (f3)	🟡 round 1	resolved in `16f472f`	reply
🟡 The e2e check of the server refusal has never run (f4)	🟡 round 1	still open: the body is accurate now, but no dispatched run is linked	reply
🟢 The guard's `NotFoundException` reaches the filler as a 500 (f5)	🟢 round 1	resolved in `16f472f`	reply
🟢 A suite migration re-seals the copy unchecked (f6)	🟢 round 1	resolved in `a739abe`	reply
🟡 Once #1294 is merged, the guard's write rule disagrees with it (k1)	new	open	comment

Connected issues

#1287: finished by this PR once f4 and k1 are closed; the body says "Closes #1287", but GitHub only closes issues on a merge into the default branch, so close it by hand after the merge into development.
#1285: affected through k1. The group-member write it adds must count as "may write" here.
#1298, #1299, #1300: follow-ups named in the body, not affected by this round.

Details of this round

Scope: 2 commits since b99e7ad (16f472f, a739abe): 7 PHP files, 3 specs, CHANGELOG.md, and 22 test files, most of them only passing the now-required guard. Analysed at Strict depth.
Verified clean: assertNotACopy runs after the use-only and read-only checks and before resolveSyncWriter, and treats a copy with two share rows as a copy. The required guard leaves no ?-> and no ?ViewOnlyCopyGuard in lib. A constructor-graph walk on this head and on git merge-tree --write-tree a739abe2 306dd9b5 finds no path from the guard back to the three services it guards. fill maps NotFoundException to the neutral refusal.
Merging with #1294: lib/Service/ShareSyncService.php now conflicts (syncUpdate moved into ShareSyncGuards there), as do openspec/specs/user-sharing/spec.md and tests/e2e/workflows/share-save.spec.ts. assertNotACopy has to survive the merge and stay before the base-version check, or testASyncThatNamesARecipientCopyIsRefused gets the 409.
CI: green on a739abe, including Hydra Gates and PHPUnit. Playwright is skipped on PR CI (see f4). The persistence audit was not run.

Before the next round: fix rules on (fix-induced 0 + missed-sibling 0 ≥ new 0; copy-missed 1): the next fixes go through the class sweep first.

🤖 Generated with Claude Code