Skip to content

fix(routing): make public share, send and fill links reach the recipient - #610

Closed
remko48 wants to merge 35 commits into
developmentfrom
fix/public-share-link-routing
Closed

fix(routing): make public share, send and fill links reach the recipient#610
remko48 wants to merge 35 commits into
developmentfrom
fix/public-share-link-routing

Conversation

@remko48

@remko48 remko48 commented Sep 3, 2026

Copy link
Copy Markdown
Member

Every recipient link (share / send / fill) was dead for the person it was
sent to. The router moved to createWebHistory, but the link builders
still emitted hash URLs (/apps/keepiq/public#/share/link/<token>); the
fragment is never read under path routing, so the catch-all redirected to
/ and the vault guard bounced account-less recipients onto the lock
screen — asked for a master password they do not have.

The fix lands as a stack, each commit green on its own:

  1. serve the anonymous shell at every /public subpath — a
    publicShell catch-all route (/public/{path}, mirroring the AppHost
    dashboard catch-all) serves the #[PublicPage] shell for every
    recipient path, ahead of the authenticated /{path} fallback.
  2. resolve recipient routes as paths and rescue sent hash links
    routerBase() extends to /apps/keepiq/public so the same manifest
    routes resolve on both shells, and a bootstrap handoff rewrites any
    legacy #/ URL in place (links already in inboxes cannot be
    regenerated). A legacy in-fragment ?k= send key moves to a new #k=
    fragment — never into the real query string, which the browser would
    send to the server on refresh.
  3. emit path links from the recipient link builders — link shares,
    application fill links, fillLink.js and sends; the send access page
    reads the key from the fragment with a legacy-query fallback.
  4. emit path deep links from search, notifications and the PWA
    manifest
    — three more hash builders the sweep found on the
    authenticated shell; their deep links were silently swallowed by the
    catch-all.
  5. test(e2e): cover the anonymous recipient — the first specs to open
    share/send/fill links in a browser context with no session, including
    the legacy hash form and the key-stays-out-of-the-query assertion.
    This gap is why the bug shipped.
  6. fix(l10n) — every recipient-facing string translated into Dutch
    (nl ratchet lowered 382 → 362), plus a new "Invalid password. Please
    try again." key in all 36 locales — the link-share page compares
    nothing against a second entry, so "does not match" was wrong there.
  7. fix(public-pages) — the recipient card centres on the guest layout
    (the app mount shrank inside core's flex row); the revealed secret is
    labelled field rows with per-field copy and masked sensitive values
    (card/identity JSON parsed into fields, no raw JSON); the
    not-found/expired state is a proper error page instead of the raw
    English server message.
  8. style(share-dialog) — the one-time link + password in a
    highlighted panel, real padding and separated sections.

Review note: this branch is stacked on
fix/cards-table-type-icon-strength-badge — only the eight commits above
are new here; the ancestors disappear from the diff once the parent PRs
merge.

…l overlays

"Applicatie registreren" appeared to do nothing: the dialog mounted as a
bare <section role="dialog"> in document flow, below the fold, with no
overlay. PrivateKeyDownloadDialog in the same view was built the same way —
and it shows a one-time private key that cannot be recovered.

Both are now NcDialog overlays with NcTextField/NcTextArea/NcButton, moved
to src/dialogs/ where the modal-isolation gate expects NcDialog components.
The register dialog's type select became a two-option radio group on
purpose: an NcSelect inside an NcDialog either teleports behind the modal
or is clipped by its content box, the exact failure family just fixed in
the move dialog. The private-key dialog uses noClose, so the library's
close button, Esc and outside-click cannot bypass the acknowledgment
checkbox that gates Dismiss — before this, converting to an overlay would
have added three ways around the one-time-key acknowledgment.

No new translatable strings; the hand-rolled Close string is gone. The
stale eslint suppression for the old path is dropped (bare catch instead
of catch(_e)). New unit tests pin the overlay mechanism, the noClose
contract, the acknowledgment gate, and that a refused registration keeps
the dialog open with the server's reason.
Gate-16 flags five methods in the converted dialogs — both open watchers,
onCsrUpload, onCopy and onDownload — as changed without an @SPEC anchor.
The local gate runs before the previous commit stayed green because the
two dialog files were untracked at the time, and git-diff-scoped gates
cannot see untracked files; CI diffs the committed PR and caught them.
The watchers are excluded as open-state plumbing; the CSR upload and the
one-time-key copy/download anchor on the encryptionsuite-via-csr
requirement their neighbours already use.
The "New secret" and "Register application" dashboard tiles landed on the
lock screen: linkType 'app' resolves to a full browser navigation, the
vault master key is memory-only, so every tile click re-locked the vault.
The '#/secrets' hash values were dead weight on top — main.js routes with
createWebHistory and the fragment is never read — and the manifest _note
still claimed hash routing, which made those links look correct on review.

The tiles now use the tile widgets' linkType 'route', which pushes the
value through the host vue-router so the SPA and the unlocked vault
survive the click. They target the create actions rather than the bare
lists: SecretList consumes ?action=create and ApplicationRegisterView
consumes ?action=register — each opens its dialog and strips the marker
from the URL, so a refresh (which round-trips the query through the lock
screen's returnUrl) does not re-open the dialog on every unlock. Watchers
rather than mounted() checks, because CnPageRenderer keeps a view mounted
when only the query changes. The stale _note is corrected.

Two interactions shaped the SecretList side. CnAppRoot closes the active
registry modal on every route change, and stripping the query IS a route
change — so the marker is stripped first and the dialog opened after,
and the spec pins that order. And the library's CnIndexPage also consumes
?action=create, opening its own generic schema-form dialog over the
registry one; :showFormDialog="false" is its documented opt-out, and the
@add path is unaffected because the view listens to it.

Requires @conduction/nextcloud-vue with route-type tile support; until
that release ships, the released library renders these tiles with a dead
href (no navigation), not a broken page.
The create dialog's folder picker was a local NcSelect that prepended a
"Vault root" option — but the root is not a place a secret can live:
top-level folders are vaults, and a rootless secret has nowhere to be
shown in the vault views. The picker is now the shared DestinationSelect
(the move dialogs' picker): every vault and folder, tree-ordered, with
the nav rail's own glyphs and colours, and no root option by design.

Creating without a folder stays blocked instead of posting a null
folderId — the requiredFields spec that used to pin "sends a null folder
when created at the vault root" now pins the refusal.
…y do

"New secret" wore a bare plus and "Register application" a person-with-
plus — the latter reads as "add user". The tiles now carry mdi key-plus
and mdi application-import, both verbatim from vue-material-design-icons
(a hand-recomposed key-plus variant with the badge beside the key was
tried and dropped: stock MDI beats a custom remix). The Documentation
tile was already right and is untouched.
…gate

Gate-16 anchors on the function it sees: the @SPEC docblocks sat on the
watcher KEYS while the checker attributed the changed lines to the inner
handler functions, so both handlers — and openCreateSecret, pulled into
the changed set by a reformat — read as unanchored. The docblocks now sit
on the handlers themselves.
…ble views

Fix-brief bugs 8+9 (stage 5). The List view rendered each secret with its
type icon and password-strength pill, but the cards and table views fell
back to CnIndexPage's generic renderer driven by a name+url schema — the
same secret lost its type and strength the moment the user switched view.

Cards now fill CnIndexPage's #card slot with the SAME row component the
list uses (SecretListItem: favicon/type icon, strength pill, outstanding-
request, blocked and compromised states, copy button) boxed in a card
shell that mirrors CnObjectCard's tokens, with the selection checkbox
re-wired like the existing #list-item override. The table grows Type and
Strength columns in listSchema, rendered through #column-type and
#column-strength cell slots since their values live in the type/health
stores rather than on the row; both are non-sortable — no row field backs
them. Unscored Strength cells render a muted em dash instead of a blank
hole. The type→icon map moves into a shared SecretTypeIcon component so
the three views cannot drift apart again.

One new l10n key ("Strength") ships translated into all 36 locales with
the catalogs regenerated. 8 new unit tests cover the icon map and its
fallback, the column set and order, the sort opt-out, the translated type
label, and the badge-or-dash rule.
"All secrets" opened with a Vaults strip above the list — navigation
posing as contents. It duplicated the nav's folder tree one panel away
and pushed every secret below a screen of vault rows, worst on mobile
where the nav toggle already opens the vault list in one tap (Proton Pass
comparison). The root is a cross-vault query, not a container the user is
inside.

folderRows now returns nothing at the root; inside a vault or folder the
subfolder rows are unchanged — there they ARE the contents of the thing
being looked at. The vault-glyph rendering, its three view helpers and
the strip's level-dependent caption go with it. The folder-sharing e2e
workflow reaches a vault through the nav tree instead of the removed root
rows, and a new unit spec pins both sides of the rule.
… tag

Follow-through on removing the root Vaults strip: the cross-vault context
it carried moves into two Proton-style indicators instead of container
rows.

A new VaultIndicator component renders both shapes from the vault's own
Stage 9 icon and color, so the glyphs cannot drift: a compact icon-only
dot on the rows in All secrets — list, cards, and the table's Name cell —
with a tooltip plus screen-reader-only vault name (never inside a vault,
where the page itself is the location), and an icon+name tag in the
vault's color under the secret's title in the detail sidebar, which
renders wherever the sidebar opens. rootVaultOf() in utils/vaultList.js
walks a secret's folderId up to its top-level vault, guarded against
parentId cycles and dangling parents.

No new translatable strings — the indicators show vault names, which are
data. 15 new unit tests cover the walk, both indicator variants with the
icon fallback, the root-only rule for the dot, and the sidebar's
resolution through nested folders.
The warning shipped with PR #599 in English and Dutch only — it consumed
the parity ratchet's last slack, and the other locales were left to the
tracked translation debt (#180). Backfill German and French now; the
remaining locales follow with the bulk translation pass, where the
long-tail languages get their native read-through.
…'s loose ends

Review follow-ups from #600.

The offline list branch upgraded folderId and search to key-presence
checks but dropped typeId entirely, so a "passkeys only" list silently
showed the whole vault on every refresh while offline — it now mirrors
the online branch's params.typeId with the same presence semantics.

The move dialog's picker preselects a secret's current folder so "you are
here" is visible, which armed Move on open: clicking it fired a PUT that
kept the secret exactly where it was. Move now stays disarmed until the
destination actually changes. And dismissing the dialog mid-transfer used
to be cosmetic — the per-item vault loop kept PUTting invisibly until
every item had moved, then refreshed against whatever the user had
navigated to. A cancelled flag now stops the loop at the next item,
restores the visible list immediately at cancel time, and suppresses the
loop's own trailing refresh; completed transfers close through a separate
path so success never picks up cancel semantics.

The fourth review note — no more vault-root (folderId=null) destination —
is design intent, called out for the release notes rather than changed.

5 new unit tests: the offline type filter (honoured, folder-combined,
explicitly cleared), the disarmed-until-changed rule, and a mid-loop
cancel proving the second item is never sent and the list is restored
exactly once.
…r form

Review follow-ups from #601, strict pass over one-time key material.

The private-key dialog no longer leaves the key lying around: copying
goes through CopyButton, whose timer clears the clipboard again after
30 seconds, and the key renders masked (bullet-filled, line structure
preserved) until an explicit reveal — the dialog opens unprompted after
registration, possibly mid-screenshare. A keydown handler additionally
swallows Escape, since the library's Esc handling has historically
routed through internals rather than the noClose prop the dialog relies
on. Reveal reuses the existing Show/Hide private key strings.

The register dialog now trims description and CSR the way it already
trimmed the name, emits close alongside registered so a cnOpenModal
host that only wires @registered is never left with a stuck dialog, and
gates onSubmit on busy so the form's @submit and the actions-slot
button can never register twice for one gesture.

7 new or updated unit tests pin the auto-clear, the mask/reveal cycle,
the Escape swallow, the trimmed payload, the self-sufficient close, and
the single-registration guarantee.
The SPA is moving its recipient links from hash URLs to paths under the
anonymous shell (/apps/keepiq/public/share/link/<token> and friends), so
a load or refresh of any such path must serve the shell without a
Nextcloud session. A publicShell catch-all route (/public/{path},
mirroring the AppHost dashboard#page / dashboard#catchAll split - a
distinct name, because Symfony silently replaces same-named routes)
serves the same #[PublicPage] template at every subpath. It sits in
$extra, so it precedes the authenticated /{path} fallback.
…links

The router runs createWebHistory, which never reads a URL fragment - yet
the recipient links in the wild are hash URLs
(/apps/keepiq/public#/share/link/<token>): vue-router saw the bare
/public path, the catch-all redirected to /, and the vault guard bounced
account-less recipients onto the lock screen, asking for a master
password they do not have.

Two changes, one per link population:

- routerBase() extends to /apps/keepiq/public on the anonymous shell,
  so the SAME manifest routes (/share/link/:token, /send/:token,
  /share/request/:token) resolve on both shells unchanged.
- Links already sent cannot be regenerated, so a bootstrap handoff
  (src/bootstrap/hash-route-handoff.js) rewrites any legacy #/ URL in
  place with replaceState before the router is created. A legacy
  fragment-mode send key (?k= INSIDE the old SPA fragment) moves to a
  new #k= fragment, never into the real query string, which the browser
  would send to the server on the next refresh.

isPublicSurface() now classifies by path prefix, keeping the hash
prefixes for reads that can happen before the handoff.
The builders of recipient-facing links still emitted the retired hash
form, which the createWebHistory router never reads - recipients got the
lock screen. All four now emit paths under the anonymous shell:

- LinkShareController (share links)
- ApplicationSecretRequestsController (application fill links, a fifth
  site the fix brief's list missed)
- fillLink.js (secret-request fill links)
- ephemeralSend.js (sends)

The ephemeral-send content key rides as a #k= fragment - still never
transmitted - and the access page reads it from the fragment with a
fallback to the legacy route query, so links from before this change
keep decrypting. The send page's card styling rides along with its
fragment-key reader. The stale hash-routing wording in the manifest
note is corrected.
… PWA manifest

Sweeping for the retired hash scheme turned up three more builders
outside the fix brief's list, all on the AUTHENTICATED shell: the
unified-search provider, the notification deep link, and the PWA
manifest's vault shortcut. Their /apps/keepiq/#/secrets/<id> links fell
through the catch-all to the dashboard - the deep link silently
swallowed. They now emit paths; the gated route lands on the lock screen
with the path as returnUrl and resumes there after unlock.
No spec ever opened a share, send, or fill link without a session - the
existing page-surface specs drive the same components on the
authenticated shell with the admin cookie jar, which is precisely why
dead recipient links could ship. This spec mints fixtures through the
authenticated API (the server stores only opaque blobs, so no real
crypto is needed) and opens the links in a browser context with an EMPTY
storage state: the path form, the RETIRED hash form (through the
bootstrap handoff), a legacy fragment-mode send - asserting the content
key stays out of the query string - and a fill link.
…d-password message

Every string an account-less recipient sees on the share access and
ephemeral-send pages now has a Dutch translation (18 keys; the fill page
was already covered). The nl parity ratchet is lowered 382 -> 362 so the
paid-off debt cannot silently return.

Also adds "Invalid password. Please try again.", translated into all 36
locales, for the link-share access page to adopt: that page compares
nothing against a second entry, so the existing "The password does not
match" wording - correct on the lock screen, where it compares the two
typed setup passwords - is wrong there.
…ecret as copyable fields

The recipient card sat pinned in the top-left corner: core's guest
layout makes #content.app-public a flex row, so #keepiq-app shrank to
the card's width and the card's `margin: auto` had no free space to
distribute. The app mount now fills the flex row (app.css) and the
public shell centres its card on both axes with flex + margin:auto,
which stays scrollable when the card outgrows the viewport. All three
recipient views share the same card treatment.

The revealed link share was a bare <dl> that printed a card secret as
its raw JSON composite. It now renders labelled field rows with a copy
button per row (CopyButton, clipboard auto-clear) and masked sensitive
values behind an eye toggle - card/identity composites are parsed via
parsePayload() into Card number / Expiry / CVV / PIN / Cardholder rows
(number, CVV, PIN and BSN masked), additional fields render as masked
rows, and a plain key stays one masked "Secret value" row. The
usage-cap notice moved to the top of the card, under the heading, where
a whole-card status banner belongs.

The terminal not-found/expired state is a recognisable error page
(broken-link icon, "Link not found or expired" heading, translated
explanation) instead of the raw English server message, and a failed
decrypt now says "Invalid password. Please try again." The password
form moved to house-style controls.
The freshly created link and password - which cannot be retrieved again
- render in a highlighted panel so they read as the thing to take away
from the dialog. Sections gained real padding and separators, rows
breathe, the password is proper monospace at a readable size, and the
disabled user-sharing affordance is visually set apart from the working
controls. No copy changes; the folder-sharing e2e selectors are
unchanged.
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/keepiq @ 36b0412

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 ✅ 111/111
npm ✅ 536/536
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman
Playwright ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development
Hydra gates

Quality workflow — 2026-09-03 13:59 UTC

Download the full PDF report from the workflow artifacts.

@remko48
remko48 changed the base branch from development to fix/cards-table-type-icon-strength-badge September 3, 2026 14:29
Comment thread lib/Notification/KeepiqNotifier.php
Comment thread appinfo/routes.php
Comment thread src/router/guards.js
Comment thread tests/views/LinkShareAccess.spec.js
Comment thread src/dialogs/SecretShareDialog.vue

@WilcoLouwerse WilcoLouwerse left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verdict: REQUEST_CHANGES (Strict first review — security-sensitive)

Security posture is solid — #[PublicPage] + #[NoCSRFRequired] correct on PublicShellController, AnonRateLimit(120/60) set, hash-route-handoff.js uses history.replaceState before router init so the content key never lands server-observable, createVaultGuard is fail-closed and name-based, no referer-leak paths in the recipient views, all 37 locales carry the new Invalid password. Please try again. key. The routing/security work itself lands cleanly.

But PHPUnit is genuinely failing across all 6 CI combos on 4 tests in KeepiqNotifierTest — the tests still assert the old hash URLs the modified KeepiqNotifier.php no longer emits. Details on the Notifier inline comment. Must go green before merge.

Rest of the inline notes: three 🟡 (catch-all guardrail, isPublicSurface substring, e2e-vs-component-test wording) + one 🟢 (SecretShareDialog "style-only" description).

Hydra-gate CI failure noted — flagged for tomorrow, orthogonal to the PHPUnit blocker.

@remko48
remko48 changed the base branch from fix/cards-table-type-icon-strength-badge to development September 4, 2026 11:58
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/keepiq @ f566dc2

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 ✅ 111/111
npm ✅ 536/536
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman
Playwright ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development
Hydra gates

Quality workflow — 2026-09-04 12:21 UTC

Download the full PDF report from the workflow artifacts.

Four KeepiqNotifierTest tests failed across all six CI combos, and the
URLs were not why: commit 33bf6c7 already moved every expectation to the
path form. That same commit round-tripped the file through a CP1252
reader, turning all eight em dashes into "a-euro-quote" (c3a2 e282ac
e2809d where e2 8094 belongs). Four of the eight sit inside asserted
message strings, so exactly four tests failed - the other four
occurrences are in comments and were silent.

Restores U+2014 in all eight places. Nothing else about the file changes;
development still carries the correct bytes, so the diff against it is
now empty for these lines.
Hydra gate-16 failed on src/views/EphemeralSendAccess.vue::fragmentKey -
the computed was added with an inline comment but no docblock, so the
changed-method scan found no @SPEC line. Adds the docblock with the
anonymous-recipient anchor the neighbouring mounted() hook already uses.
isPublicSurface() classified any path merely CONTAINING
'/apps/keepiq/public' as a recipient surface, so
/apps/keepiq/publications/..., /apps/keepiq/publicfoo and
/apps/keepiq/secrets/public all answered true. It is not the security
gate - access is authorised server-side by scoped token, and the
locked-vault gate is the name-based isPublicRoute() - but a classifier
that is wrong for a whole family of paths invites being mistaken for one,
and its docblock read as if it were authoritative.

Splits the app-relative remainder off the pathname first, then compares
prefixes against that. The app segment is still LOCATED rather than
anchored, so sub-directory installs (/nextcloud/index.php/apps/keepiq/..)
keep resolving; what it now requires is that the remainder begin at a "/"
boundary, which also rules out another app whose id merely starts with
this one. The docblock says plainly what the function is not.

Specs cover the former false positives, the neighbouring-app id, and the
webroot install the boundary check must not cost.
publicShell#pageCatchAll ('path' => '.+') matches every GET below
/public/ and hands it to a #[PublicPage] controller, so a session-gated
route mistakenly placed there is shadowed: the anonymous shell renders,
the intended handler never runs, and the author sees a working page
rather than a 404. The shell exposes no data, so there is nothing to
exploit today - the hazard is that a routing slip becomes a silent auth
bypass instead of an error.

A comment cannot enforce that, so this adds both: the warning on the
route entry, and a contract test that reads the route table as source
text and asserts every /public route resolves to a method actually
declaring #[PublicPage] (and #[NoCSRFRequired], which a session-less
recipient needs). Injecting dashboard#summary at /public/oops fails it
with the route named. A negative control keeps the check honest.

Not done instead: rejecting requests that carry a valid Nextcloud
session. A logged-in user opening a recipient link is ordinary - the
sharer checking their own link, or a colleague with an account but no
vault suite - and that reject would break it. The rationale is recorded
in the test.
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/keepiq @ a9ad934

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 ✅ 111/111
npm ✅ 536/536
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman
Playwright ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development
Hydra gates

Quality workflow — 2026-09-04 12:50 UTC

Download the full PDF report from the workflow artifacts.

The coverage ratchet blocks this branch on PublicShellController, and the
reason only became visible once PHPUnit itself stopped failing first. The
guard scopes to changed PHP files and keeps head-only method buckets on the
head side, so pageCatchAll() — two statements, added by this branch, with no
test class anywhere for the file — entered the denominator uncovered and pulled
the scoped ratio under the merge base. The other five changed lib/ files are
comment and string edits inside methods that were already covered, so nothing
else moved.

Covering the file rather than the method: page() was untested too, and a shell
at 0% is what let a new public method land uncovered without anything looking
wrong. The assertions pin what would actually strand a recipient — RENDER_AS_BASE
rather than the session-expecting user layout, the wasm-unsafe-eval directive
the Argon2id derivation needs to run at all, and the identity of the two methods
across the three live link shapes, since page() answers /public and pageCatchAll()
answers everything below it and a divergence would break only one half with a
correct-looking route table.
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/keepiq @ 981b77f

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 ✅ 111/111
npm ✅ 536/536
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman
Playwright ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development
Hydra gates

Quality workflow — 2026-09-04 13:11 UTC

Download the full PDF report from the workflow artifacts.

WilcoLouwerse
WilcoLouwerse previously approved these changes Sep 4, 2026

@WilcoLouwerse WilcoLouwerse left a comment

Copy link
Copy Markdown
Contributor

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)

The blocker cleared and both 🟡 concerns landed strong fixes.

  • 🔴 PHPUnit — 6/6 combos green. Root cause was em-dash mojibake in KeepiqNotifierTest expectations (430b6bb), not URL-form mismatch as I had hypothesised.
  • 🟡 Catch-all guardrailc9d7cc3 docblock warning + PublicRouteSurfaceContractTest (253 lines) parsing routes.php and reflecting every /public/* handler for #[PublicPage] + #[NoCSRFRequired], with a negative-control. 2b64bc5 adds PublicShellControllerTest for the shell.
  • 🟡 isPublicSurface substringc9d7cc3 new appRelativePath() with segment-boundary check; three new guards.spec.js describe blocks cover the exact shapes I flagged plus subdirectory-served Nextcloud.
  • 🟡 e2e-wording + 🟢 style-only description — non-blocking, left open for you to resolve.

Hydra Gates ✅, PHPUnit ✅, Newman ✅, all frontend checks ✅. Nothing else surfaced in the compare against eb0a396.

@remko48
remko48 dismissed WilcoLouwerse’s stale review September 4, 2026 13:37

The merge-base changed after approval.

WilcoLouwerse
WilcoLouwerse previously approved these changes Sep 4, 2026

@WilcoLouwerse WilcoLouwerse left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

APPROVE — re-review at 2b64bc5 confirms all blockers cleared. Details in the earlier verdict comment.

@remko48
remko48 dismissed WilcoLouwerse’s stale review September 4, 2026 13:44

The merge-base changed after approval.

@remko48

remko48 commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

Reviews are automaticly declared stale because of git history bug

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