[shaohuzhang1]

fix: use target route workspaceId in permission guard

[shaohuzhang1]

The code is syntactically valid, and the synchronous nested save/restore logic in withRouteWorkspace is correct. However, the module-level route override is fragile and can produce incorrect results in async or concurrent scenarios.

Main issues

1. Async/concurrent safety is the biggest problem

   • withRouteWorkspace only works safely for synchronous callbacks.
   • If someone passes an async callback:

     withRouteWorkspace(to, async () => {
       await checkPermission()
       getWorkspaceId() // override has already been restored
     })

     the finally block runs before the promise resolves, so getWorkspaceId() no longer sees the target route.
   • If multiple route guards/navigations interleave, the shared module state can be overwritten.
   • The generic type T allows Promise<T>, so TypeScript will not prevent this misuse.

2. SSR / module-global state risk

   • If this code runs in SSR, module-level state is shared across requests. Any async interleaving can leak workspace context between requests.

3. Exception-safety gap in withRouteWorkspace

   • readWorkspaceId(route) runs after hasRouteWorkspace = true but before the try block. If readWorkspaceId ever throws, the override will not be restored.
   • Move the read before mutation, or put all state changes inside try.

4. Hidden coupling to global router.currentRoute

   • The fallback in getWorkspaceId() is convenient, but inside a route guard it is exactly what you are trying to avoid. If the override is accidentally removed too early, the function silently reads the previous route instead of failing.

5. Minor naming issue

   • hasRouteWorkspace sounds like “the route has a workspace,” but it actually means “a workspace override is active.” Prefer hasRouteWorkspaceOverride.

Recommended approach

Avoid the global override and make the route/workspace explicit:

type WorkspaceRouteLike = Pick<RouteLocationNormalized, 'params'>

const readWorkspaceId = (route: WorkspaceRouteLike): string | undefined => {
  const value = route.params.workspaceId
  return Array.isArray(value) ? value[0] : value
}

export const getWorkspaceId = (
  route: WorkspaceRouteLike = router.currentRoute.value,
): string | undefined => readWorkspaceId(route)

Then use it in guards as:

router.beforeEach((to) => {
  const workspaceId = getWorkspaceId(to)
  // check permissions with workspaceId
})

This removes global mutable state entirely and works correctly with async guards.

If you must keep withRouteWorkspace

At minimum:

• Document that the callback must be synchronous.
• Fix the exception-safety order:

export const withRouteWorkspace = <T>(route: WorkspaceRouteLike, fn: () => T): T => {
  const nextId = readWorkspaceId(route)
  const prevHas = hasRouteWorkspace
  const prevId = routeWorkspaceId

  try {
    hasRouteWorkspace = true
    routeWorkspaceId = nextId
    return fn()
  } finally {
    hasRouteWorkspace = prevHas
    routeWorkspaceId = prevId
  }
}

But even with this fix, the async/concurrent problem remains. The explicit getWorkspaceId(to) approach is safer and simpler.

[shaohuzhang1]

Review

Overall the diff is small and the indentation fix is welcome, but the change adds a non-trivial dependency (withRouteWorkspace) into a security-relevant guard, so a few things are worth verifying before merging.

1. Correctness / security (highest priority)

• The wrapper must be strictly synchronous and return the callback's value. The expression !to.meta?.permission || withRouteWorkspace(to, () => perm(to)) relies on the wrapper returning the boolean result of the inner function. If withRouteWorkspace is async, or is written as function withRouteWorkspace(route, fn) { setContext(route); fn(); restore(); } (returning undefined), then:
  • an async wrapper returns a Promise → always truthy → every permission check passes (auth bypass);
  • a void wrapper returns undefined → falsy → every guarded route is denied, including valid ones.
    Please double-check (and ideally type it as withRouteWorkspace<T>(route, fn: () => T): T).
• Context restoration must be in a finally. If the permission function throws (or if any route permission callback is buggy), a non-finally restore will leak the workspace context into subsequent navigations/checks.
• Validate that the workspace derived from to actually belongs to the user. Since the wrapper presumably derives context from the route (params/query), a crafted URL pointing at another tenant's workspace could make the permission callback evaluate against the wrong/foreign context. The wrapper should treat an unknown/unauthorized workspace as "no context" rather than silently accepting it.
• Routes without a workspace (system routes, error pages, public routes) must be a no-op in withRouteWorkspace. If it throws or sets a bogus context for those, system routes will break.
• Inconsistency: getChildRouteList('system').some((item) => item.permission ? item.permission() : true) is not wrapped. If permission callbacks now depend on resource context, this check can behave differently from the main check. Either wrap it too (with a sensible route/context) or document why it's exempt.

2. Robustness

• The beforeEach has no try/catch around the permission evaluation. A throw from the wrapper (e.g., malformed param) will reject the navigation and surface as an uncaught router error. Consider catching and doing return { name: 'error-...' } / return false, or guarantee the wrapper never throws.
• Error path is asymmetric: if the wrapper fails in the first check, you fall through to the redirect loop, which may itself throw in nextPermission. Make the two call sites behave consistently.
• If the wrapper mutates global/reactive state, confirm that repeated/rapid navigations (canceled navigations) can't interleave. Here it's synchronous, so it should be fine — but this property should be enforced/documented.

3. Types

• nextPermission is typed as (route?: RouteLocationNormalized | RouteLocationResolvedGeneric | RouteLocationNormalizedLoaded) => boolean, but inside the loop it's called with nextRoute of that union type. withRouteWorkspace(nextRoute, ...) must accept the same union, not only RouteLocationNormalizedLoaded. Check the wrapper's signature.
• The cast to.meta.permission as (route: RouteLocationNormalizedLoaded) => boolean is applied to to, which in beforeEach is RouteLocationNormalized. Pre-existing, but the new wrapper makes the mismatch more visible — worth tightening with a shared type/type guard.

4. Duplication / readability

• The pattern appears twice (withRouteWorkspace(route, () => perm(route))). Extract a small helper, e.g.:

  const hasPermission = (route) =>
    !route.meta?.permission || withRouteWorkspace(route, () => (route.meta.permission as PermissionFn)(route))

  Then both call sites become if (hasPermission(to)) return true and if (hasPermission(nextRoute)) return {...}. This also keeps the wrapper contract in one place.
• Consider a comment on the wrapper call explaining why the context is needed (e.g., "permission callbacks read the current workspace from resource-context"), since this is currently non-obvious.

5. Style / minor

• Import placement: @/utils/resource-context is added after the relative imports. Most codebases group alias imports before local relative ones — align with the existing convention in the file.
• The block indentation fix is good; the diff would be easier to review if whitespace-only changes were in a separate commit from the behavioral change.

6. Testing suggestions

• Permission callback returns true / false, and navigations redirect correctly in both cases.
• No workspace in the route → system routes still work (wrapper is a no-op).
• Route with a workspace the user doesn't belong to → denied.
• Permission callback throws → navigation fails gracefully, context is restored.
• Verify context is restored after a failed/permitted navigation (subsequent navigation uses the correct workspace).