MarketplacePackagePage decides "not an admin" only after the package load, so a non-admin sees a load-failure card instead of a refusal

Found while implementing #5557 (PR #5582), and deliberately left out of its scope — that card fenced MarketplacePackagePage.tsx as read-only.

Observation

packages/app-shell/src/console/marketplace/MarketplacePackagePage.tsx orders its early returns like this (line numbers on origin/main at f2158ec86):

519:  if (!marketplaceEnabled) return ( MarketplaceDisabled )
521:  if (loading) { ...skeleton... }
531:  if (error || !data) { ...destructive card... }
593:  if (!isAdmin) return ( MarketplaceAccessDenied )

The admin guard sits after both the loading branch and the load-failure branch. Two consequences on a runtime where the marketplace IS enabled (features.marketplace: true):

A non-admin who opens a package URL that fails to load gets the destructive "Failed to load package" card, carrying the server's own error message — not the refusal the page would otherwise give them. They are handed a diagnosis about a surface they are not allowed to use.
The page issues both of its requests on behalf of a viewer it is about to refuse — getMarketplacePackage and getCloudInstallationInfo fire during the load it will never show. Marketplace package detail page still red-errors on a runtime with no marketplace — the disabled state stops at the catalog page #5533 established for this same page that a request it knows it will not use should be skipped rather than fired and discarded.

Measured, not inferred: this is exactly why the sibling-consistency case in PR #5582 has to hand the detail page a package that resolves before it can compare refusals. With the 404 the marketplace-off cases use, the detail page never reaches its admin guard at all.

Not claimed

That the refusal itself is wrong, or that the catalog page's ordering (#5557) is implicated — that one is about features.marketplace and lands ahead of everything. This is only about where !isAdmin sits relative to the load, on a runtime that has a marketplace.

Whether it is worth changing is a judgement call for triage: the server remains the real authority on what a non-admin may fetch, and marketplace catalog data is not especially sensitive, so this may be deliberate. Filing it because it was measured, not because a severity was decided.

Refs

Marketplace catalog page tells a NON-ADMIN "access denied" on a runtime that has no marketplace at all #5557 / PR Marketplace catalog page answers "no marketplace here" before "not an admin" (#5557) #5582 — the catalog-page half, where this was found
Marketplace package detail page still red-errors on a runtime with no marketplace — the disabled state stops at the catalog page #5533 — the detail page's disabled state, which established the skip-the-request discipline for this page

Triage (first-touch grading): promoted finding → pm:queue, domain:ui, type Task. Rationale: both halves are covered by standing discipline, so no ruling is needed — #5533 already established for this exact page that a request the page knows it will discard should be skipped, and handing a refused viewer the server's load-failure diagnosis instead of the refusal is an ordering defect against that same posture. The fix is a guard reorder (decide !isAdmin before firing the loads) plus the refusal-first render; the server stays the authority — this only stops the client doing work it will throw away. Not security-sensitive per the card's own assessment (catalog data, server-side authz intact), so no decision-box detour.

Dispatch notes: MarketplacePackagePage.tsx was fenced read-only by in-flight #5557 (PR #5582) — serialize after that PR lands, and re-verify the line numbers on the merged ref. Size/model suggestion: S–M.

Generated by Claude Code

Claim — domain:ui execution seat (#5560), round 8.

Session: session_012u2pRjcqAYtoEjgr3wwhnK
Branch: claude/issue-5583-marketplace-admin-guard-order
Label: pm:queue → pm:dispatched

Serialization dependency cleared: #5557 / PR #5582 have landed, so this is unblocked. Per the triage note the dev re-verifies the cited line numbers on the merged ref before editing rather than trusting the card's :593 / :521 / :531.

Generated by Claude Code

fix(app-shell): decide the marketplace admin refusal before the packa…

docs(changeset): declare the marketplace detail guard-order fix

{
  "issue": 5583,
  "status": "done",
  "branch": "claude/issue-5583-marketplace-admin-guard-order",
  "pr": "https://github.com/objectstack-ai/objectui/pull/5621",
  "pr_url": "https://github.com/objectstack-ai/objectui/pull/5621",
  "premise_still_valid": true,
  "files_changed": [
    "packages/app-shell/src/console/marketplace/MarketplacePackagePage.tsx",
    "packages/app-shell/src/console/marketplace/__tests__/MarketplacePackagePage.guardOrder.test.tsx (new)",
    "packages/app-shell/src/console/marketplace/__tests__/MarketplacePage.guardOrder.test.tsx (fixture triage)",
    ".changeset/marketplace-detail-admin-guard-order-5583.md (new)"
  ],
  "what_changed": "Moved `if (!isAdmin) return <MarketplaceAccessDenied />` from after the loading and `error || !data` branches to immediately after the `!marketplaceEnabled` branch, and gated both fetch effects (`getMarketplacePackage`, `getCloudInstallationInfo`) on `isAdmin` in addition to `marketplaceEnabled`, adding `isAdmin` to both dependency arrays. `loading` stays seeded from `marketplaceEnabled` alone — deliberately NOT `marketplaceEnabled && isAdmin` — because `activeMember` resolves asynchronously, so an org-role admin renders once as a non-admin and a `false` seed would paint the destructive card for a frame before the effect could raise the flag.",
  "summary": "The admin guard now runs ahead of both the loading and the load-failure branches, so a non-admin on a marketplace-enabled runtime gets MarketplaceAccessDenied on the first commit instead of the skeleton and (on a failed load) the destructive 'Failed to load package' card carrying the server's own error message. Both marketplace requests are skipped for a refused viewer, extending the skip-the-request discipline #5533 established on this page. Draft PR #5621; two findings filed unassigned (#5619, #5620).",
  "evidence": {
    "verified_line_numbers": {
      "note": "Re-verified against merged origin/main at 8c87f0583 (with #5557/PR #5582 landed) by locating each branch by its CODE, not by trusting the card. They had NOT drifted — all three cited numbers were exact.",
      "marketplace_disabled": {
        "cited": 519,
        "measured_on_8c87f0583": 519,
        "after_fix": 538
      },
      "loading": {
        "cited": 521,
        "measured_on_8c87f0583": 521,
        "after_fix": 558
      },
      "error": {
        "cited": 531,
        "measured_on_8c87f0583": 531,
        "after_fix": 568
      },
      "guard": {
        "cited": 593,
        "measured_on_8c87f0583": 593,
        "after_fix": 556
      }
    },
    "guard_dependencies": "`isAdmin = useIsWorkspaceAdmin()` (packages/auth/src/useIsWorkspaceAdmin.ts) derives ONLY from `useAuth()`'s `activeMember` and `user`. It reads none of `data`, `loading` or `error` — the page's own fetch state — so the guard was never waiting on the fetch for its input, only sequenced behind it. Measured second-order dependency: `ConnectedShell` (console/ConsoleShell.tsx:239-266) gates children on `if (!adapter || isAuthLoading) return <LoadingFallback />`, so `user` (legs 2 and 3 of the hook) IS resolved before this page mounts; `activeMember` (leg 1) is NOT covered by that gate — AuthProvider.refreshActiveMember runs from its own effect keyed on `activeOrganization` and never raises `isLoading`.",
    "behaviour_change_or_pure_reorder": "BOTH, and the two must be stated separately. (a) PURE REORDER with respect to this page's own load state: the guard has zero data dependency on `data`/`loading`/`error`, so nothing it reads comes into existence only after the fetch resolves. (b) REAL BEHAVIOUR CHANGE in what a viewer sees, which is the card's point: a non-admin now gets the refusal on the first commit instead of the skeleton and instead of the error card; on a load that SUCCEEDS the outcome was already access-denied, so only the detour is removed. Two requests are no longer issued. (c) ONE PRE-EXISTING COUPLING REMOVED, reported not hidden: the package fetch was incidentally masking the `activeMember` resolution window with a skeleton — for as long as an unrelated request happened to take. That was never an auth gate, and the sibling MarketplacePage has had no such mask since #5557. Its consequence is handled locally by keeping the `loading` seed at `marketplaceEnabled` alone; the general residual is filed as #5619."
  },
  "changeset": {
    "present": true,
    "file": ".changeset/marketplace-detail-admin-guard-order-5583.md",
    "bump": "patch @object-ui/app-shell",
    "why": "MEASURED, two legs built at the real dist/ path and compared uncompressed (no gzip header in the number). Before (origin/main source): 5,891,126 bytes across 862 files. After: 5,893,632 bytes across 862 files. Exactly one emitted file differs — dist/console/marketplace/MarketplacePackagePage.js, 42,591 -> 45,097 (+2,506 bytes). ZERO .d.ts files differ, so the dispatch criterion read literally ('does dist/*.d.ts move?') answers NO while the shipped JS plainly does move; flagged in open_questions. Independently decisive either way: check-changeset-presence guards SOURCE of a versioned package and @object-ui/app-shell@17.6.0 is one — the gate reports '3 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)'."
  },
  "tests": {
    "command": "All vitest from the REPO ROOT (package-cwd is refused by the repo guard, #3378), every heavy command through scripts/pm/os-verify-lock.sh. Union run AFTER the final commit, at 25be3f582: `pnpm vitest run --maxWorkers=2 packages/app-shell/src/console/marketplace/__tests__/ packages/app-shell/src/console/__tests__/AppContent.pseudoRouteSegments.test.tsx packages/i18n/src/__tests__/marketplace-preview-namespace-3546.test.tsx`",
    "result": "`Test Files 8 passed (8)` · `Tests 122 passed (122)`, VITEST_EXIT=0, wrapper `VERDICT command-exit 0`. Type-check: `pnpm --filter @object-ui/app-shell type-check` (tsc --noEmit && tsc -p tsconfig.test.json) -> `VERDICT command-exit 0`, with the script name echoed in the output so this is not a zero-match pnpm filter. Dependency closure built first: `pnpm --workspace-concurrency=2 --filter '@object-ui/app-shell^...' build` -> `VERDICT command-exit 0`.",
    "ablation": "REVERSE VERIFICATION, run from the COMMITTED state (fix at 3b10d9c2c) so restoration had a recovery point, with `trap '... git checkout HEAD -- $F' EXIT INT TERM`. Mutation: `git checkout origin/main -- MarketplacePackagePage.tsx`. CONFIRMED ON DISK BEFORE MEASURING, anchored on the exact text moved — guard back at line 593; `grep -c 'if (!isAdmin) return;'` = 0 (injected effect gates gone); `grep -c 'marketplaceEnabled, isAdmin]'` = 0 (dep arrays reverted); guard once again inside the `categoryLabel`..`return (` span = 1. NO REBUILD WAS NEEDED OR DONE, and that is a property of this pair, not an omission: the suites import the component by RELATIVE SOURCE path (`../MarketplacePackagePage`), so no package `exports` -> dist/ hop is interposed and vitest compiles the mutated source directly. The dist/ two-leg build reported under `changeset` is a separate measurement and did rebuild each leg. Result, in the predicted direction: `Tests 5 failed | 10 passed (15)` — the four new non-admin/flip cases red plus the sibling suite's re-scoped detail leg red, while all 10 controls (every admin leg and every marketplace-off leg) stayed GREEN. That green/red split is the discriminating pattern; a mutation that merely broke the page would have taken the controls with it. Tree confirmed restored byte-identically afterwards (`git status --short` empty, `git diff HEAD --stat` 0 lines).",
    "superset_argument": "packages/app-shell has ~487 test files and its full suite exceeds the container's 10-minute foreground cap, so the run is a PROVABLE SUPERSET derived by reading the code, not a sample. Only suites that RENDER the component can observe this change. Non-test importers, enumerated by grep: (1) src/index.ts — a bare re-export, no render and no module-level side effect; (2) AppContent.tsx — a `lazy()` route at `system/marketplace/:packageId`. The only test that drives that route, AppContent.pseudoRouteSegments.test.tsx, `vi.mock`s the component out to a stub div, so the real module never loads there — it was run anyway, proving the mock still resolves. That leaves the three suites in console/marketplace/__tests__/ that import it directly; all six files in that directory were run. Added beyond the render set: packages/i18n/.../marketplace-preview-namespace-3546.test.tsx, which reads this file as TEXT (path constant, static t() call-site scan) and so is sensitive to the edit even though it renders nothing.",
    "lint_scope": "NOT a narrowing: `eslint .` was run inside packages/app-shell, which is the exact command `turbo run lint` (= root `pnpm lint`) invokes for this package. 919 files reported by eslint's own JSON output, 0 errors, 2549 warnings — all pre-existing; the 11 warnings on MarketplacePackagePage.tsx sit at lines 209/273/340/368/372/373/412/495/521/587/627 and none falls inside any hunk this PR adds (101-102, 105-115, 176-179, 189, 200-203, 216, 540-557). Invariance for the other 45 packages: no shared eslint config changed and no file outside packages/app-shell changed, so their lint runs read identical inputs; `check-lint-coverage` independently reports 46/46 linted, 0 with outstanding errors."
  },
  "gates": [
    {
      "name": "vitest (targeted superset, repo root)",
      "status": "pass",
      "verdict_line": "Test Files 8 passed (8) / Tests 122 passed (122)"
    },
    {
      "name": "@object-ui/app-shell type-check",
      "status": "pass",
      "verdict_line": "os-verify-lock: VERDICT command-exit 0"
    },
    {
      "name": "@object-ui/app-shell lint (eslint .)",
      "status": "pass",
      "verdict_line": "919 files, 0 errors (2549 pre-existing warnings)"
    },
    {
      "name": "check-lint-coverage",
      "status": "pass",
      "verdict_line": "lint coverage: 46/46 packages linted, 0 with outstanding errors (0 total)."
    },
    {
      "name": "check-control-bytes",
      "status": "pass",
      "verdict_line": "check-control-bytes: OK (scanned 4672 tracked text file(s); skipped 85 binary)."
    },
    {
      "name": "check-changeset-presence",
      "status": "pass",
      "verdict_line": "3 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)."
    },
    {
      "name": "check-changeset-no-major",
      "status": "pass",
      "verdict_line": "No changeset declares a `major` bump."
    },
    {
      "name": "check-changeset-fixed",
      "status": "pass",
      "verdict_line": "privatePackages declared: version=true, tag=false."
    },
    {
      "name": "check-i18n-call-site-keys",
      "status": "pass",
      "verdict_line": "Every in-scope call-site key resolves against the en pack (2918 keys)..."
    },
    {
      "name": "CI (full farm on PR #5621)",
      "status": "in_progress",
      "verdict_line": "Reported at draft-PR time per the standing contract; CI convergence is the PM's read, not awaited here."
    }
  ],
  "open_questions": [
    {
      "question": "The dispatch's changeset criterion — 'does `dist/*.d.ts` move?' — read literally answers NO here, while the shipped JS moves by +2,506 bytes. Which reading governs?",
      "options": [
        "A. The criterion means 'do emitted bytes move' and .d.ts was shorthand for the common case; a changeset is owed whenever any dist/ file differs.",
        "B. The criterion is literal and .d.ts-only; a changeset would not be owed on emitted grounds here."
      ],
      "recommendation": "A, and it does not change this PR either way: check-changeset-presence.mjs guards SOURCE of a versioned package, so @object-ui/app-shell@17.6.0 owes a changeset regardless of the emitted delta, and the change is user-visible behaviour. Worth restating in future dispatch text as 'does any emitted dist/ file differ' so the two readings stop diverging.",
      "blocking": false
    },
    {
      "question": "Of the +2,506 emitted bytes, the large majority is comment text this package's build preserves (the fix itself is ~6 statements). Is that the intended trade for this package?",
      "options": [
        "A. Keep — house style in this file is heavily commented source, and the surrounding module already emits 42 KB with comments intact.",
        "B. Trim the added comments to reduce shipped bytes for @object-ui/app-shell."
      ],
      "recommendation": "A. The file's existing comments carry the #5533 / #5557 rulings and are the reason the ordering question was answerable at all; shrinking them for bytes would trade the durable record for ~2 KB uncompressed in a 5.9 MB dist. Flagging it because the dispatch asked for byte deltas and this is where they came from.",
      "blocking": false
    },
    {
      "question": "Note on a dispatch instruction that could not be followed as written: `node scripts/pm/dispatch-gates.mjs` (the framework repo's gate-derivation script) resolves its repo root from its own location, so run from the objectui worktree it computed a merge base in objectstack (112a8c673) and exited 2 with 'this branch changes nothing against origin/main'. It has no objectui counterpart.",
      "options": [
        "A. Accept the hand-derived gate set (taken from .github/workflows/lint.yml + the per-package turbo lint/type-check scripts + the changed paths), as listed in `gates`.",
        "B. Port a dispatch-gates equivalent into objectui so the derivation is mechanical there too."
      ],
      "recommendation": "A for this PR — the derivation is stated explicitly above so it is reviewable — and B as a separate card if objectui dispatches keep citing that script. Not filed as an issue since it is PM tooling rather than a product defect; say the word and I will file it.",
      "blocking": false
    },
    {
      "question": "Tooling note for the PM's report scan, measured just now: the `<!-- os-dev-report -->` marker DID survive storage on the report comment — verified by reading the comment raw over the REST API. But `mcp__github__issue_read(get_comments)` renders the body with HTML-ish nodes stripped, so the marker reads as ABSENT through that tool, and JSX in code spans (e.g. the branch spellings in the PR body) disappears the same way. Two writes were nearly made on that false signal.",
      "options": [
        "A. Scan for the marker over the raw REST body (or accept both the HTML-comment and a literal `os-dev-report` first line), and treat the MCP comment reader as lossy for anything angle-bracketed.",
        "B. Have devs emit the marker as literal text `os-dev-report` unconditionally, so it is visible through either reader."
      ],
      "recommendation": "A, plus B as belt-and-braces if the scan already runs through the MCP reader — B costs nothing. Flagging it because the standing dev instructions say to rewrite the comment when the marker 'did not survive', and through the MCP reader it always looks that way even when it did. Related platform behaviour seen on every write: a bare `_Generated by [Claude Code](https://claude.ai/code)_` footer is appended server-side on top of the session-URL footer, so bodies end up carrying two — not another agent editing the PR.",
      "blocking": false
    }
  ],
  "out_of_scope_findings": [
    "filed as #5619: `useIsWorkspaceAdmin` collapses 'not resolved yet' into `false` — `activeMember` resolves asynchronously outside ConnectedShell's `isAuthLoading` gate, so an org-role admin renders once as a non-admin across all 11 call sites (refusals on the two marketplace pages, hidden affordances in AppSidebar/AppHeader/UnifiedSidebar/HomePage). Predates this PR; MarketplacePage has had the unmasked shape since #5557. Unassigned, unlabelled, left for PM triage.",
    "filed as #5620: MarketplacePackagePage's THIRD request, `listLocalInstalls` (a real fetch to ${API_BASE}/install-local), is gated on `features.installLocal` alone, so it still fires on a marketplace-off runtime and for a refused viewer, and its result reaches only the unreachable content branch. Same class as #5533, different flag. NOT taken as a bounded in-place fix: the correct shape is not pinned by existing evidence — features.installLocal is a separate deployment axis and #5533 gated the two marketplace requests while leaving this one, so whether that was deliberate is not readable from the tree."
  ]
}

Generated by Claude Code

fix(app-shell): decide the marketplace admin refusal before the packa…

Navigation Menu

Repository navigation

MarketplacePackagePage decides "not an admin" only after the package load, so a non-admin sees a load-failure card instead of a refusal #5583

Observation

Not claimed

Refs

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

MarketplacePackagePage decides "not an admin" only after the package load, so a non-admin sees a load-failure card instead of a refusal #5583

Description

Observation

Not claimed

Refs

Activity

huangyiirene commented on Aug 21, 2026

os-sales commented on Aug 21, 2026

os-sales commented on Aug 21, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions