Skip to content

fix: use target route workspaceId in permission guard #7865

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
peslyn merged 1 commit into from
Nov 12, 2026
Merged

fix: use target route workspaceId in permission guard #7865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions ui/src/router/admin/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { systemRoutes } from './system'
import { workspaceRoutes } from './workspace'
import { workflowRoutes } from './workflow'
import { getChildRouteList } from './utils'
import { withRouteWorkspace } from '@/utils/resource-context'
const ADMIN_BASE_PATH = window.MaxKB?.prefix || import.meta.env.VITE_BASE_PATH || '/admin/'

NProgress.configure({ minimum: 0.3, showSpinner: false, speed: 500, trickleSpeed: 200 })
Expand Down Expand Up @@ -62,17 +63,17 @@ router.beforeEach(async (to, from) => {
}
}

const workspaceList = user.userInfo?.workspace_list ?? []
if (to.name === 'workspace-home' && workspaceList.length === 0) {
const hasSystemAccess = getChildRouteList('system').some((item) => (item.permission ? item.permission() : true))
if (hasSystemAccess) {
return { name: 'system-home', replace: true }
const workspaceList = user.userInfo?.workspace_list ?? []
if (to.name === 'workspace-home' && workspaceList.length === 0) {
const hasSystemAccess = getChildRouteList('system').some((item) => (item.permission ? item.permission() : true))
if (hasSystemAccess) {
return { name: 'system-home', replace: true }
}
// 没有任何工作空间、也没有系统权限：不做额外处理，保持原有行为
}
// 没有任何工作空间、也没有系统权限：不做额外处理，保持原有行为
}

// 当前路由有权限，放行
if (!to.meta?.permission || (to.meta.permission as (route: RouteLocationNormalizedLoaded) => boolean)(to)) {
if (!to.meta?.permission || withRouteWorkspace(to, () => (to.meta.permission as (route: RouteLocationNormalizedLoaded) => boolean)(to))) {
return true
}

Expand All @@ -98,7 +99,7 @@ if (to.name === 'workspace-home' && workspaceList.length === 0) {
const nextPermission = nextRoute.meta?.permission as (
route?: RouteLocationNormalized | RouteLocationResolvedGeneric | RouteLocationNormalizedLoaded,
) => boolean
if (!nextPermission || nextPermission(nextRoute)) {
if (!nextPermission || withRouteWorkspace(nextRoute, () => nextPermission(nextRoute))) {
return { name: nextName, params: to.params, query: to.query, hash: to.hash, replace: true }
}
}

@shaohuzhang1 shaohuzhang1 Nov 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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).

Expand Down
2 changes: 2 additions & 0 deletions ui/src/utils/UTILS_README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ MkTable、MkFormList 的排序优先使用其公开属性；普通列表再直
资源范围定义见 [ROUTE_README.md](../router/ROUTE_README.md#资源范围判断)。
`resource-context.ts` 提供 getWorkspaceId、getResourceScope 和四种 isXxxResource 判断。
非组件代码通过 getWorkspaceId 获取工作空间；页面／组件通过现有判断读取当前资源范围。
Routing 守卫评估目标路由权限时使用 `withRouteWorkspace(route, fn)` 将该路由的 workspaceId 作为
判定上下文，此时 router.currentRoute 尚未更新为待跳转路由，不能直接读取。
isWorkspaceResource 仅匹配 workspace，不包含 workspace-shared；未配置范围时不推测来源。

## VNode 插槽内容判断
Expand Down
34 changes: 30 additions & 4 deletions ui/src/utils/resource-context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,40 @@
/** 提供当前路由资源范围和工作空间上下文的读取函数。 */

import router from '@/router/admin'
import type { RouteLocationNormalized, RouteLocationNormalizedLoaded, RouteLocationResolvedGeneric } from 'vue-router'

type WorkspaceRouteLike = RouteLocationNormalized | RouteLocationNormalizedLoaded | RouteLocationResolvedGeneric

let hasRouteWorkspace = false
let routeWorkspaceId: string | undefined

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

/**
* 获取当前工作空间 id。
* 当前实现取自路由参数；不在工作空间上下文下时返回 undefined。
* 取自目标路由参数；不在工作空间上下文下时返回 undefined。
*/
export const getWorkspaceId = (): string | undefined => {
const value = router.currentRoute.value.params.workspaceId
return Array.isArray(value) ? value[0] : value
export const getWorkspaceId = (): string | undefined => (hasRouteWorkspace ? routeWorkspaceId : readWorkspaceId(router.currentRoute.value))

/**
* 在给定路由的工作空间上下文内执行回调，期间 getWorkspaceId 使用该路由的 workspaceId。
* 用于路由守卫评估目标路由权限：此时 router.currentRoute 尚未更新为待跳转路由，
* 需要以被评估路由的参数为准，避免读到上一条（或初始 START）路由导致结果错误。
*/
export const withRouteWorkspace = <T>(route: WorkspaceRouteLike, fn: () => T): T => {
const prevHas = hasRouteWorkspace
const prevId = routeWorkspaceId
hasRouteWorkspace = true
routeWorkspaceId = readWorkspaceId(route)
try {
return fn()
} finally {
hasRouteWorkspace = prevHas
routeWorkspaceId = prevId
}
}

/** 判断当前路由是否属于工作空间资源。 */

@shaohuzhang1 shaohuzhang1 Nov 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Expand Down
Loading