Skip to content

[CHAIN] feat(ui): add fail-closed Registry access boundaries - #12499

Merged
alejandrobailo merged 1 commit into
feat/prowler-2414-registry-uifrom
feat/prowler-2414-registry-ui-03-access-boundaries
Sep 8, 2026
Merged

[CHAIN] feat(ui): add fail-closed Registry access boundaries#12499
alejandrobailo merged 1 commit into
feat/prowler-2414-registry-uifrom
feat/prowler-2414-registry-ui-03-access-boundaries

Conversation

@Alan-TheGentleman

@Alan-TheGentleman Alan-TheGentleman commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

🔗 Part of Chained PRs

Field Value
Feature Branch feat/prowler-2414-registry-ui
Main PR #12494
Position 3 of 8
Base feat/prowler-2414-registry-ui-02-access-authority
Depends on #12497
Follow-up #12503, Registry DTO and documented-error adapters
Review budget 397 / 400 changed lines
Starts at Fresh server authority without Registry discovery or routes
Ends with Bounded in-memory leases, fail-closed navigation, and independently guarded direct routes

Chain Overview

feat/prowler-2414-registry-ui (#12494 tracker)
└── #12495 PR 1: permissions and flag typing
    └── #12497 PR 2: fresh access authority
        └── 📍 #12499 PR 3: lease, navigation, and route guards
            └── #12503 PR 4: DTO and error adapters
                └── ... PR 8: acceptance hardening
                    └── #12494 tracker -> master

Scope

  • Includes: one in-memory eligibility provider, a Registry access boundary, lease-only sidebar discovery, a New badge, proxy denial, and an independently guarded dynamic /registry page.
  • Excludes: Registry DTO/adapters/data actions, credential handling, catalog traversal, explorer UI, Add/Remove mutations, shared primitive changes, Playwright, backend changes, migrations, deployment, and rollout documentation.

Autonomy

  • Focused and full unit/integration results are recorded.
  • Runtime behavior is covered by browser integration; manual explorer validation remains deferred until the user-facing slices exist.
  • Rollback is the single child commit and excludes unrelated work.
  • The diff contains only this work unit and remains below 400 changed lines.

Context

A fresh server permission check is not enough for a long-lived browser session. Registry navigation and protected content must disappear within a bounded interval after permission or flag rollback, while direct requests must independently fail closed before reading Registry data.

This slice adds that access boundary without introducing any Registry API or credential behavior.

Description

  • Add one shared in-memory Registry eligibility provider around the authenticated application shell.
  • Start in an unknown state and refresh a server-issued lease without trusting JWT permissions or hydrated runtime configuration.
  • Renew visible sessions every 15 seconds and enforce a hard 30-second lease expiry, including while the document is hidden.
  • Invalidate before mount, focus, visibility, pathname, and online refreshes, and ignore stale request generations.
  • Show the New-badged Registry sidebar entry only while the lease is valid.
  • Unmount protected Registry content immediately when eligibility is lost.
  • Guard /registry independently in both the proxy and dynamic server page, redirecting denial to /profile before any Registry read.
  • Keep lease and protected state out of browser storage and URLs.

No npm dependency, Registry endpoint call, backend change, migration, deployment change, or complete explorer UI is introduced.

Steps to review

  1. Review registry-eligibility-provider.tsx for unknown initialization, 15/30-second bounds, invalidate-before-refresh events, and stale-generation suppression.
  2. Confirm sidebar discovery depends only on provider eligibility and that the New badge disappears on lease invalidation.
  3. Confirm registry-access-boundary.tsx unmounts children when eligibility is not current.
  4. Confirm ui/proxy.ts and ui/app/(prowler)/registry/page.tsx independently evaluate fresh access and redirect to /profile before Registry reads.
  5. Run cd ui && pnpm exec vitest run --project unit proxy.test.ts lib/registry/access.server.test.ts components/layout/main-layout/main-layout.test.tsx components/layout/app-sidebar/navigation-config.test.ts components/layout/app-sidebar/app-sidebar-content.test.tsx; the recorded result is 5 files / 29 tests.
  6. Run cd ui && pnpm exec vitest run --project integration components/registry/registry-eligibility-provider.integration.test.tsx components/registry/registry-access-boundary.integration.test.tsx; the recorded result is 2 files / 3 tests.
  7. Run cd ui && pnpm run test:unit && pnpm run test:integration; the recorded results are 3,074 unit and 111 integration tests passing.
  8. Run PREK_NO_CONCURRENCY=1 uv run --directory . prek run; Prettier, ESLint, TypeScript, related tests, and root hooks are recorded as passing.

Checklist

Community Checklist

SDK/CLI

  • Are there new checks included in this PR? No

UI

  • All issue/task requirements work as expected on the UI: the complete user-facing workflow is delivered by later child PRs.
  • This PR adds or updates no npm dependencies.
  • Mobile screenshots/video are deferred because Registry remains default-off and the explorer is not part of this slice.
  • Tablet screenshots/video are deferred because Registry remains default-off and the explorer is not part of this slice.
  • Desktop screenshots/video are deferred because Registry remains default-off and the explorer is not part of this slice.
  • No UI changelog fragment is added for this guarded foundation slice; the PR uses no-changelog, and the user-visible slice will add the feature fragment.

API

  • The API is not changed by this PR.
  • Endpoint output, query analysis, performance evidence, API specs, versions, and API changelog are not applicable.

MCP Server

  • The MCP Server is not changed by this PR.
  • The MCP changelog is not applicable.

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

- Renew in-memory server-issued eligibility leases with bounded freshness
- Hide navigation and unmount protected state on access loss
- Deny direct Registry routes before any Registry data read
@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: ede690d8-caed-4dd3-8452-8114e7b23421

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Alan-TheGentleman Alan-TheGentleman added the no-changelog Skip including change in changelog/release notes label Aug 20, 2026
@Alan-TheGentleman Alan-TheGentleman changed the title feat(ui): add fail-closed Registry access boundaries [CHAIN] feat(ui): add fail-closed Registry access boundaries Aug 20, 2026
@pfe-nazaries

pfe-nazaries commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Review — findings that survived an adversarial pass

Four candidate findings were each handed to a fresh reviewer whose only job was to disprove them. Three were narrowed, one was redirected, none survived as a merge blocker. Reporting only what held up, with the severity the counter-evidence supports.

Scope note first, because it reframes everything below: 841d77cde (layer 08 tip, "drop the client-side Registry eligibility lease") deletes registry-eligibility-provider.tsx, registry-access-boundary.tsx and both integration tests. Since the repo is squash-only and just #12494 targets master, roughly 265 of this PR's 395 added lines never reach master in any form. Both findings below therefore affect layers 03–07 as open PRs, not the shipped product.


1. next build fails on layers 03–07 — the intermediate layers can't be built, served, or manually verified

ui/lib/registry/access.server.ts carries import "server-only" at :1 and an inline "use server" at :20, while ui/components/registry/registry-eligibility-provider.tsx ("use client") imports refreshRegistryEligibility from it. Next doesn't promote a module to an action module on an inline directive, so the whole server-only module is pulled into the browser graph.

./lib/registry/access.server.ts:20:3
It is not allowed to define inline "use server" annotated Server Actions in Client Components.
  • Reproduced independently three times, and on both bundlers (next build and next build --webpack) — there's no flag that makes it build.
  • Confirmed by real builds on 04 (d2fb198a3), 05 (feecafb6b), 06 (5bb370e0b), 07 (99a810123) — all EXIT=1, same frame.
  • Base 487f79eef builds clean (45/45 pages), so it's introduced here. Layers 08/09 build clean, so master is protected.
  • There is no precedent for this shape: of 54 "use server" files in ui/, this is the only one not on line 1.

Not a merge blockermergeStateStatus is CLEAN and no build gate fires on a feature-branch base (ui-tests.yml and ui-container-checks.yml are both gated on master/v5.*). The cost is that five open PRs present code a reviewer can't run. Worth knowing: tsc and healthcheck pass, so this is a bundler failure, not a typecheck one.

A caveat on this chain's CI, since it's why none of this was caught: #12494's checks are green but vacuous — the principal branch's only commit is empty, so tj-actions/changed-files reports any_changed=false and every build step is if:-skipped (ui-tests "passed" in 33s, ui-container-build-and-scan in 34s — not enough for a Next build). Nothing in this chain has been built by CI yet. Those gates will run for real once layers land on the principal branch, and by then layer 08 has removed the offending code — so this is a "don't read those greens as coverage" note, not a second defect.

Repro hygiene: rm -rf ui/.next first — a stale .next/types left by the failed build produces a phantom TS2344 that looks like an unrelated type error.

Measured fix (builds clean, 45/45 pages, /registry + middleware intact — it's what layer 08 already does):

  1. New ui/actions/registry/access.ts, top-of-file "use server", static import { auth } from "@/auth.config"
  2. Delete the inline refreshRegistryEligibility from access.server.ts, leaving it server-only
  3. Repoint the provider's import

The await import("@/auth.config") at :21 dodges no cycle — layer 08's own working action uses a static import.


2. generation <= 1 in RegistryAccessBoundary evicts users the server just approved

registry-access-boundary.tsx:20-24. The provider is mounted app-wide in app/(prowler)/layout.tsx, so generation survives navigation and is already ≥1 after the first refresh. Every refresh(true)[pathname], focus, visibilitychange, online, lease expiry — synchronously sets {UNKNOWN, generation ≥ 2} before awaiting, the grace clause can't cover it, and the [allowed] effect fires router.replace("/profile") while the refresh that returns ELIGIBLE is still in flight.

Verified in a real Next 16.2.11 + React 19.2.7 App Router app (both components copied verbatim, only import specifiers changed), driven with Playwright — not a mocked router:

soft nav:  6800ms history.pushState(/reg)  ->  7141ms history.replaceState(/profile)
           final URL /profile, provider settled "eligible:gen3:eligible"
tab focus: one window focus event, no click, no navigation -> replaceState(/profile) at +24ms

Next does not coalesce or cancel the dispatched navigation. Cold direct load is the only path that works; soft nav, bare tab focus, and 30s in a hidden tab all evict, deterministically (3/3 consecutive soft navs).

A latency sweep bounced at every value tried — 0, 1, 5, 15, 40, 150ms. The one case that survived was a pure microtask resolution (mockResolvedValue, no await) from a fresh page load, where React batched eligible gen1 -> eligible gen2 into a single commit — and even that bounced when run inside a repeated-trial loop. So this is not a latency threshold with a safe zone below it: the existing tests stay green because an instant double in that one mount shape happens to coalesce, which is a fragile artifact rather than a guarantee. A real server action is an HTTP POST and can never resolve in a microtask.

Exposure and harm, stated plainly. The gate is UI_CLOUD_ENABLED && UI_REGISTRY_ENABLED && manage_registry, so OSS and enterprise deployments are not affected at all — this is Prowler Cloud with the Registry flag on. And at this layer the guarded body is {null}, so today's symptom is a self-navigating empty page plus a sidebar entry that blinks out and back on every navigation and tab focus (it's gated on the same isEligible). Functional loss arrives at 06/07 once there's real content behind the boundary. Note a hard load or pasted URL does work — it's unreachable by clicking the sidebar link, and even a hard-loaded tab self-evicts 30s after losing focus.

The fix already exists in this chain: ff2da11ec (layer 08) deletes generation <= 1 and the synchronous if (invalidate) setState({UNKNOWN, ...}), and preserves a still-valid lease across a failed refresh. The ask is to land that down at layer 03, not to redesign anything.


Also, on a different PR

A finding about multi-role users survived scrutiny intact, but it belongs to #12497, not here — ui/lib/auth/current-user.ts isn't in this diff. Raising it there.

Comment on lines +19 to +20
const expiresAt = useRef(Date.now() + initialLeaseDurationMs);
const [now, setNow] = useState(Date.now());

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.

I may be missing some context about the requirements, but i see a lot of timing check if the user can access the registry, why is so crucial to be aware this fast? Should not be enought on each user token refresh and get the current permissions there?

Comment on lines +65 to +78
useEffect(() => {
const refreshVisible = () => {
if (document.visibilityState === "visible") void refresh(true);
};
const refreshOnline = () => void refresh(true);
window.addEventListener("focus", refreshVisible);
window.addEventListener("online", refreshOnline);
document.addEventListener("visibilitychange", refreshVisible);
return () => {
window.removeEventListener("focus", refreshVisible);
window.removeEventListener("online", refreshOnline);
document.removeEventListener("visibilitychange", refreshVisible);
};
}, []);

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.

Following my previous comment/concern, also not in a "React way" implemented i would say

@pfe-nazaries
pfe-nazaries marked this pull request as ready for review August 26, 2026 14:40
@pfe-nazaries
pfe-nazaries requested a review from a team August 26, 2026 14:40
@Alan-TheGentleman

Copy link
Copy Markdown
Contributor Author

Review routing note: do not test the client access-boundary or eligibility-provider snapshot in this PR. Those components are removed by #12520's lease-free, server-authoritative access model. Review only the retained proxy, navigation, and server access contracts here, then validate final Registry behavior in #12524 and #12590.

Base automatically changed from feat/prowler-2414-registry-ui-02-access-authority to feat/prowler-2414-registry-ui September 8, 2026 17:01
@alejandrobailo
alejandrobailo merged commit 07ed513 into feat/prowler-2414-registry-ui Sep 8, 2026
4 checks passed
@alejandrobailo
alejandrobailo deleted the feat/prowler-2414-registry-ui-03-access-boundaries branch September 8, 2026 17:01
@alejandrobailo

Copy link
Copy Markdown
Contributor

Consolidated into #12494. All changes are preserved there. GitHub marked this PR as merged when its commits were pushed to the parent branch; #12494 remains open and has not been merged into master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Skip including change in changelog/release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants