fix: client restriction UI improvements and security hardening#892
fix: client restriction UI improvements and security hardening#892ding113 merged 10 commits intoding113:devfrom
Conversation
… dialog Use useRef to track latest nested form state synchronously, preventing stale state overwrites when AccessRestrictionsSection fires multiple onChange calls within the same event handler. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Patterns with '*' use full-string glob matching (case-insensitive, literal characters). Patterns without '*' retain existing substring matching with normalization for backward compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds granular selection of individual Claude Code sub-clients (CLI, VS Code, SDK-TS, SDK-PY, CLI-SDK, GitHub Action) via a popover dropdown on the Claude Code preset row. Auto-consolidates to parent "claude-code" when all 6 sub-clients are selected. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove ~200 lines of duplicated preset/popover logic from AccessRestrictionsSection by reusing ClientRestrictionsEditor. Fixes i18n bug where sub-client "All" label used wrong translation key (presetClients["sub-all"] instead of subClients?.all). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace globToRegex (regex-based, vulnerable to catastrophic backtracking on patterns like *a*a*a*a*c) with globMatch (two-pointer linear algorithm). Remove globCache since regex compilation is no longer needed. Add adversarial tests: consecutive wildcards, regex metacharacters as literals, and a performance guard for pathological patterns. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough该 PR 为客户端限制引入带星号的 glob 匹配、将预设扩展为可包含子项并实现子项选择 UI；重构并提取客户端限制编辑器组件，新增相关本地化键与测试覆盖，并在检测逻辑中优先支持 glob 模式。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the client restriction features within the dashboard, improving both user experience and security. It addresses critical issues such as a race condition during user creation, expands the flexibility of custom client patterns with glob support, and provides more granular control over specific sub-clients. Furthermore, it refactors the UI for better maintainability and hardens the glob matching logic against potential ReDoS attacks, ensuring a more robust and secure system. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
bot
added
bug
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several significant improvements to the client restriction UI. It addresses a race condition in the user creation dialog by using a useRef to maintain correct state. It also hardens security by replacing a ReDoS-vulnerable regex-based glob implementation with a linear-time two-pointer algorithm. New features include wildcard support for custom client patterns and granular multi-select for sub-clients. The code has also been refactored to eliminate duplication by introducing a shared ClientRestrictionsEditor component. The changes are well-tested and cover UI, logic, and internationalization. My review did not identify any issues of medium or higher severity.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| translations={{ | ||
| allowAction: translations.actions.allow, | ||
| blockAction: translations.actions.block, | ||
| customAllowedLabel: translations.fields.allowedClients.customLabel, | ||
| customAllowedPlaceholder: translations.fields.allowedClients.customPlaceholder, | ||
| customBlockedLabel: translations.fields.blockedClients.customLabel, | ||
| customBlockedPlaceholder: translations.fields.blockedClients.customPlaceholder, | ||
| customHelp: translations.fields.allowedClients.customHelp, | ||
| presetClients: translations.presetClients, | ||
| subClients: translations.subClients, | ||
| nSelected: translations.nSelected, | ||
| }} | ||
| /> |
There was a problem hiding this comment.
blockedClients.customHelp is silently dropped
ClientRestrictionsEditor accepts a single customHelp field that is rendered beneath both the allowed and blocked custom-pattern inputs. Only translations.fields.allowedClients.customHelp is forwarded here; translations.fields.blockedClients.customHelp is ignored.
In practice the two strings are identical in every locale file today, so there is no visible regression. However, the outer AccessRestrictionsSectionProps translations interface declares blockedClients.customHelp as a separate required field, which creates a false expectation that both strings will be shown independently. Either remove blockedClients.customHelp from the interface, or extend ClientRestrictionsEditor's translations type to accept customAllowedHelp / customBlockedHelp separately.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/user/forms/access-restrictions-section.tsx
Line: 96-108
Comment:
**`blockedClients.customHelp` is silently dropped**
`ClientRestrictionsEditor` accepts a single `customHelp` field that is rendered beneath **both** the allowed and blocked custom-pattern inputs. Only `translations.fields.allowedClients.customHelp` is forwarded here; `translations.fields.blockedClients.customHelp` is ignored.
In practice the two strings are identical in every locale file today, so there is no visible regression. However, the outer `AccessRestrictionsSectionProps` translations interface declares `blockedClients.customHelp` as a separate required field, which creates a false expectation that both strings will be shown independently. Either remove `blockedClients.customHelp` from the interface, or extend `ClientRestrictionsEditor`'s `translations` type to accept `customAllowedHelp` / `customBlockedHelp` separately.
How can I resolve this? If you propose a fix, please make it concise.| "cli-sdk": t("userEditSection.subClients.cli-sdk"), | ||
| "gh-action": t("userEditSection.subClients.gh-action"), | ||
| }, | ||
| nSelected: t("userEditSection.nSelected", { count: "{count}" }), |
There was a problem hiding this comment.
Double-template nSelected pattern is fragile
Passing the string literal "{count}" as the ICU count parameter so that the translated string retains the token for a later .replace() call (in client-restrictions-editor.tsx line 112) works today, but it relies on next-intl not escaping or re-interpreting curly-brace content in the substituted value. If the message format or the i18n library version ever changes, the substituted string could come out as "{count}" literally, an HTML-encoded entity, or something else, silently breaking the display.
The same pattern appears in routing-section.tsx line 297.
A more resilient approach would be to pass a callback translation or to retrieve the raw template string and perform the substitution entirely on the client side without involving the i18n substitution step. For example:
// In the hook: return the raw template so the editor can substitute directly
nSelected: t.raw("userEditSection.nSelected") as string,and in ClientRestrictionsEditor:
translations.nSelected?.replace("{count}", String(selected.length)) ?? String(selected.length)(The editor-side logic is already structured this way; it's only the hook that needs to stop passing through the i18n substitution step.)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/user/hooks/use-user-translations.ts
Line: 217
Comment:
**Double-template `nSelected` pattern is fragile**
Passing the string literal `"{count}"` as the ICU `count` parameter so that the translated string retains the token for a later `.replace()` call (in `client-restrictions-editor.tsx` line 112) works today, but it relies on `next-intl` not escaping or re-interpreting curly-brace content in the substituted value. If the message format or the i18n library version ever changes, the substituted string could come out as `"{count}"` literally, an HTML-encoded entity, or something else, silently breaking the display.
The same pattern appears in `routing-section.tsx` line 297.
A more resilient approach would be to pass a callback translation or to retrieve the raw template string and perform the substitution entirely on the client side without involving the i18n substitution step. For example:
```ts
// In the hook: return the raw template so the editor can substitute directly
nSelected: t.raw("userEditSection.nSelected") as string,
```
and in `ClientRestrictionsEditor`:
```ts
translations.nSelected?.replace("{count}", String(selected.length)) ?? String(selected.length)
```
(The editor-side logic is already structured this way; it's only the hook that needs to stop passing through the i18n substitution step.)
How can I resolve this? If you propose a fix, please make it concise.![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR delivers significant improvements to the client restriction UI: race condition fix, glob wildcard support with ReDoS protection, granular sub-client selection, and code consolidation. The implementation is well-structured with comprehensive test coverage (94 tests for client detection/presets combined).
PR Size: L
- Lines changed: 745 (+588/-157)
- Files changed: 22
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Key Implementation Highlights
globMatch algorithm (client-detector.ts:38-60): Uses a two-pointer approach with O(n*m) worst-case complexity, avoiding catastrophic backtracking in V8's NFA engine. The ReDoS protection test validates pathological patterns complete in <50ms.
Race condition fix (create-user-dialog.tsx:137-138, 237-238, 250-251): The useRef pattern correctly tracks latest form state synchronously, preventing the stale closure issue when multiple setValue calls read from form.values.
Auto-consolidation logic (client-presets.ts:119-126): Selecting all 6 sub-clients correctly stores the parent "claude-code" value, preventing data bloat while maintaining semantic equivalence.
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - ReDoS protected
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Comprehensive (94 tests)
- Code clarity - Good
Automated review by Claude AI
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/lib/client-restrictions/client-presets.ts (1)
127-140: 先规范化selectedChildren，再做“全选折叠”。这里直接用
selectedChildren.length === preset.children.length判定全选。只要传进来有重复值或夹带了非法子项，就可能把部分选择误折叠成父级，最终把限制扩大到整个claude-code组。♻️ 建议修改
export function setChildSelection( values: string[], preset: ClientRestrictionPresetOption, selectedChildren: string[] ): string[] { if (!preset.children) return values; const allChildValues = new Set(preset.children.map((c) => c.value)); + const normalizedSelectedChildren = uniqueOrdered( + selectedChildren.filter((v) => allChildValues.has(v)) + ); const filtered = values.filter((v) => v !== preset.value && !allChildValues.has(v)); - if (selectedChildren.length === 0) return filtered; - if (selectedChildren.length === preset.children.length) { + if (normalizedSelectedChildren.length === 0) return filtered; + if (normalizedSelectedChildren.length === preset.children.length) { return [...filtered, preset.value]; } - return [...filtered, ...selectedChildren]; + return [...filtered, ...normalizedSelectedChildren]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/client-restrictions/client-presets.ts` around lines 127 - 140, The setChildSelection function currently checks selectedChildren.length === preset.children.length without normalizing inputs, which can collapse invalid/duplicate selections into the parent; first normalize selectedChildren by deduplicating and intersecting with the valid child values (use preset.children.map(c => c.value) or allChildValues Set) to produce normalizedSelectedChildren, then use normalizedSelectedChildren.length when testing for full selection and when returning [...filtered, ...normalizedSelectedChildren] (or replacing with preset.value if fully selected), ensuring only valid unique child values are considered.src/lib/client-restrictions/client-presets.test.ts (1)
2-13: 这里改成@/别名导入。仓库规范要求
src/下的引用统一走@/，继续用相对路径会让后续移动测试文件时更脆弱。♻️ 建议修改
import { CLIENT_RESTRICTION_PRESET_OPTIONS, getSelectedChildren, isAllChildrenSelected, isPresetClientValue, isPresetSelected, mergePresetAndCustomClients, removePresetValues, setChildSelection, splitPresetAndCustomClients, togglePresetSelection, -} from "./client-presets"; +} from "@/lib/client-restrictions/client-presets";As per coding guidelines: "Use path alias
@/to reference files in./src/directory"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/client-restrictions/client-presets.test.ts` around lines 2 - 13, Update the import in client-presets.test.ts to use the project path alias instead of a relative path: replace the "./client-presets" import with "@/lib/client-restrictions/client-presets" while keeping all imported symbols (CLIENT_RESTRICTION_PRESET_OPTIONS, getSelectedChildren, isAllChildrenSelected, isPresetClientValue, isPresetSelected, mergePresetAndCustomClients, removePresetValues, setChildSelection, splitPresetAndCustomClients, togglePresetSelection) unchanged so tests still reference the same functions/consts.src/components/form/client-restrictions-editor.tsx (1)
156-159: 取消选中"全部"会移除整个预设，可能导致意外行为。根据
setChildSelection的实现（参见src/lib/client-restrictions/client-presets.ts:127-140），当selectedChildren为空数组时，函数会完全移除该预设及其所有子项。这意味着用户取消选中"全部"复选框时，预设会从允许/阻止列表中完全移除。这可能不是用户的预期行为 —— 用户可能只是想取消选中所有子项，但仍保留该预设以便稍后配置。
建议确认这是否为期望的 UX 设计。如果需要保留预设，考虑在
handleChildSelectionChange中添加特殊处理，或在 UI 中提供更明确的提示。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/form/client-restrictions-editor.tsx` around lines 156 - 159, Uncheck-all currently calls handleChildSelectionChange(value, []), which (per setChildSelection in src/lib/client-restrictions/client-presets.ts) removes the entire preset; change the behavior so unchecking "全部" does not pass an empty array sentinel that triggers preset removal—either modify the onCheckedChange to pass a distinct flag (e.g. null/undefined or a {keepPreset:true, children:[]} payload) instead of [] or update handleChildSelectionChange/setChildSelection to treat empty children as "keep preset but no children" rather than deleting the preset; locate the onCheckedChange handler and handleChildSelectionChange and adjust the input contract and callers accordingly to preserve the preset when all children are unchecked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/tag-input.tsx`:
- Line 44: DEFAULT_TAG_PATTERN currently includes '*' allowing wildcard chars
globally for TagInput; remove '*' from the default regex (change
DEFAULT_TAG_PATTERN to /^[a-zA-Z0-9_-]+$/) so the global TagInput validation no
longer accepts '*' and ensure any editor that needs wildcard support explicitly
overrides the TagInput validation prop (e.g., the client-side editor component
that permits provider-* patterns) to provide a permissive regex or validation
function. Locate DEFAULT_TAG_PATTERN and the TagInput usage sites (e.g.,
TagInput component and places that pass a validation prop) and update only those
editor-specific callers to opt into the wildcard pattern instead of keeping '*'
in the global default.
---
Nitpick comments:
In `@src/components/form/client-restrictions-editor.tsx`:
- Around line 156-159: Uncheck-all currently calls
handleChildSelectionChange(value, []), which (per setChildSelection in
src/lib/client-restrictions/client-presets.ts) removes the entire preset; change
the behavior so unchecking "全部" does not pass an empty array sentinel that
triggers preset removal—either modify the onCheckedChange to pass a distinct
flag (e.g. null/undefined or a {keepPreset:true, children:[]} payload) instead
of [] or update handleChildSelectionChange/setChildSelection to treat empty
children as "keep preset but no children" rather than deleting the preset;
locate the onCheckedChange handler and handleChildSelectionChange and adjust the
input contract and callers accordingly to preserve the preset when all children
are unchecked.
In `@src/lib/client-restrictions/client-presets.test.ts`:
- Around line 2-13: Update the import in client-presets.test.ts to use the
project path alias instead of a relative path: replace the "./client-presets"
import with "@/lib/client-restrictions/client-presets" while keeping all
imported symbols (CLIENT_RESTRICTION_PRESET_OPTIONS, getSelectedChildren,
isAllChildrenSelected, isPresetClientValue, isPresetSelected,
mergePresetAndCustomClients, removePresetValues, setChildSelection,
splitPresetAndCustomClients, togglePresetSelection) unchanged so tests still
reference the same functions/consts.
In `@src/lib/client-restrictions/client-presets.ts`:
- Around line 127-140: The setChildSelection function currently checks
selectedChildren.length === preset.children.length without normalizing inputs,
which can collapse invalid/duplicate selections into the parent; first normalize
selectedChildren by deduplicating and intersecting with the valid child values
(use preset.children.map(c => c.value) or allChildValues Set) to produce
normalizedSelectedChildren, then use normalizedSelectedChildren.length when
testing for full selection and when returning [...filtered,
...normalizedSelectedChildren] (or replacing with preset.value if fully
selected), ensuring only valid unique child values are considered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b5fb07c-5a00-47fe-8e51-86b0ca10b111
📒 Files selected for processing (22)
messages/en/dashboard.jsonmessages/en/settings/providers/form/sections.jsonmessages/ja/dashboard.jsonmessages/ja/settings/providers/form/sections.jsonmessages/ru/dashboard.jsonmessages/ru/settings/providers/form/sections.jsonmessages/zh-CN/dashboard.jsonmessages/zh-CN/settings/providers/form/sections.jsonmessages/zh-TW/dashboard.jsonmessages/zh-TW/settings/providers/form/sections.jsonsrc/app/[locale]/dashboard/_components/user/create-user-dialog.tsxsrc/app/[locale]/dashboard/_components/user/forms/access-restrictions-section.tsxsrc/app/[locale]/dashboard/_components/user/forms/user-edit-section.tsxsrc/app/[locale]/dashboard/_components/user/forms/user-form.tsxsrc/app/[locale]/dashboard/_components/user/hooks/use-user-translations.tssrc/app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsxsrc/app/v1/_lib/proxy/client-detector.tssrc/components/form/client-restrictions-editor.tsxsrc/components/ui/tag-input.tsxsrc/lib/client-restrictions/client-presets.test.tssrc/lib/client-restrictions/client-presets.tstests/unit/proxy/client-detector.test.ts
- ja/dashboard.json: fullwidth -> halfwidth parentheses (CI test) - zh-TW/dashboard.json: halfwidth -> fullwidth in subClients (CI test) - tag-input.tsx: remove * from DEFAULT_TAG_PATTERN (was leaking to all consumers) - client-restrictions-editor.tsx: explicit validateTag with * for client inputs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/form/client-restrictions-editor.tsx (1)
249-263: 可选：提取重复的验证正则表达式。
validateTag的正则表达式/^[a-zA-Z0-9_*-]+$/在第 249 行和第 263 行重复出现，可以提取为常量以便统一维护。建议的重构
在文件顶部添加常量：
const CLIENT_TAG_PATTERN = /^[a-zA-Z0-9_*-]+$/;然后在两处使用：
- validateTag={(tag: string) => /^[a-zA-Z0-9_*-]+$/.test(tag)} + validateTag={(tag: string) => CLIENT_TAG_PATTERN.test(tag)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/form/client-restrictions-editor.tsx` around lines 249 - 263, The validateTag regex /^[a-zA-Z0-9_*-]+$/ is duplicated in the validateTag props for the ArrayTagInputField instances (see occurrences around validateTag and the ArrayTagInputField components); extract this pattern into a single exported/locally scoped constant (e.g., CLIENT_TAG_PATTERN) near the top of the module and replace both validateTag inline regexes with a reference that tests against CLIENT_TAG_PATTERN (e.g., tag => CLIENT_TAG_PATTERN.test(tag)) so the pattern is maintained in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/form/client-restrictions-editor.tsx`:
- Around line 249-263: The validateTag regex /^[a-zA-Z0-9_*-]+$/ is duplicated
in the validateTag props for the ArrayTagInputField instances (see occurrences
around validateTag and the ArrayTagInputField components); extract this pattern
into a single exported/locally scoped constant (e.g., CLIENT_TAG_PATTERN) near
the top of the module and replace both validateTag inline regexes with a
reference that tests against CLIENT_TAG_PATTERN (e.g., tag =>
CLIENT_TAG_PATTERN.test(tag)) so the pattern is maintained in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72ac090b-ff2a-4f08-ab62-42a884377572
📒 Files selected for processing (3)
messages/ja/dashboard.jsonmessages/zh-TW/dashboard.jsonsrc/components/form/client-restrictions-editor.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- messages/zh-TW/dashboard.json
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| className="h-7 text-xs gap-1 px-2" | ||
| disabled={disabled} | ||
| > | ||
| {getChildDisplayText(preset)} | ||
| <ChevronDown className="h-3 w-3 opacity-50" /> | ||
| </Button> |
There was a problem hiding this comment.
Sub-client popover trigger remains interactive when preset is inactive
The <Button> that opens the sub-client popover is only disabled={disabled}. When a preset row is neither allowed nor blocked (!isAllowed && !isBlocked), the button is still clickable and opens the popover. Inside, the "All" checkbox is shown as checked={true} and disabled, which is misleading — the state implies "everything is selected" even though the preset is not active.
Adding the same disabled guard used by the inner checkboxes (line 160) to the trigger button would make the intent clearer:
| <Button | |
| variant="outline" | |
| size="sm" | |
| className="h-7 text-xs gap-1 px-2" | |
| disabled={disabled} | |
| > | |
| {getChildDisplayText(preset)} | |
| <ChevronDown className="h-3 w-3 opacity-50" /> | |
| </Button> | |
| <Button | |
| variant="outline" | |
| size="sm" | |
| className="h-7 text-xs gap-1 px-2" | |
| disabled={disabled || (!isAllowed && !isBlocked)} | |
| > |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/form/client-restrictions-editor.tsx
Line: 136-144
Comment:
**Sub-client popover trigger remains interactive when preset is inactive**
The `<Button>` that opens the sub-client popover is only `disabled={disabled}`. When a preset row is neither allowed nor blocked (`!isAllowed && !isBlocked`), the button is still clickable and opens the popover. Inside, the "All" checkbox is shown as `checked={true}` and `disabled`, which is misleading — the state implies "everything is selected" even though the preset is not active.
Adding the same disabled guard used by the inner checkboxes (line 160) to the trigger button would make the intent clearer:
```suggestion
<Button
variant="outline"
size="sm"
className="h-7 text-xs gap-1 px-2"
disabled={disabled || (!isAllowed && !isBlocked)}
>
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in bfcc19f — the popover Button now has disabled={disabled || (!isAllowed && !isBlocked)}, matching the inner checkboxes.
| @@ -145,6 +260,7 @@ export function ClientRestrictionsEditor({ | |||
| maxTagLength={64} | |||
| maxTags={50} | |||
| placeholder={translations.customBlockedPlaceholder} | |||
| validateTag={(tag: string) => /^[a-zA-Z0-9_*-]+$/.test(tag)} | |||
There was a problem hiding this comment.
Tag validator excludes . despite backend glob supporting it as a literal
The validateTag regex ^[a-zA-Z0-9_*-]+$ silently rejects patterns containing a period (.). However, the backend globMatch function deliberately treats . as a literal character — the test "glob should handle regex metacharacters literally" explicitly verifies that foo.bar* matches foo.bar/1.0 but not fooXbar/1.0.
Real-world User-Agent strings can contain dots in tool or SDK names (e.g., my.internal.tool/1.0). If an admin wants to match such a client precisely with a glob pattern like my.internal.*, the UI will silently reject the entry, while the backend would handle it correctly.
The same restriction applies to both the allowed and blocked custom pattern inputs (lines 249 and 263). Consider extending the regex to include .:
| validateTag={(tag: string) => /^[a-zA-Z0-9_.*-]+$/.test(tag)} |
(and the same change at line 263)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/form/client-restrictions-editor.tsx
Line: 249-263
Comment:
**Tag validator excludes `.` despite backend glob supporting it as a literal**
The `validateTag` regex `^[a-zA-Z0-9_*-]+$` silently rejects patterns containing a period (`.`). However, the backend `globMatch` function deliberately treats `.` as a literal character — the test `"glob should handle regex metacharacters literally"` explicitly verifies that `foo.bar*` matches `foo.bar/1.0` but not `fooXbar/1.0`.
Real-world User-Agent strings can contain dots in tool or SDK names (e.g., `my.internal.tool/1.0`). If an admin wants to match such a client precisely with a glob pattern like `my.internal.*`, the UI will silently reject the entry, while the backend would handle it correctly.
The same restriction applies to both the allowed and blocked custom pattern inputs (lines 249 and 263). Consider extending the regex to include `.`:
```suggestion
validateTag={(tag: string) => /^[a-zA-Z0-9_.*-]+$/.test(tag)}
```
(and the same change at line 263)
How can I resolve this? If you propose a fix, please make it concise.…ient patterns - Disable sub-client popover button when preset is neither allowed nor blocked - Extend validateTag regex to accept . and / for real-world UA patterns (e.g. my.tool/1.0) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/form/client-restrictions-editor.tsx (1)
249-263: 把标签校验规则收敛到一个共享定义。同一个正则在允许/阻止输入里各写了一遍，后续再调整通配符规则时很容易只改到一处。建议至少提成单个常量；更理想的是直接复用这次 PR 已更新的共享模式定义，这样 UI 校验和匹配逻辑不会再漂移。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/form/client-restrictions-editor.tsx` around lines 249 - 263, The same regex is duplicated in the validateTag props for ArrayTagInputField (used for both customAllowed and customBlocked); replace the inline /^[a-zA-Z0-9_./*-]+$/ with a single shared constant or the shared pattern introduced in this PR (e.g., TAG_VALIDATE_REGEX or the PR's shared pattern export) and import/use that in both validateTag callbacks so UI validation and matching logic stay consistent; update usages in the ArrayTagInputField instances and any related handlers like handleCustomAllowedChange/onInvalidTag to reference the shared symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/form/client-restrictions-editor.tsx`:
- Around line 40-41: Make subClients and nSelected required props (remove the
optional '?' on subClients and nSelected) and remove all fallback branches that
return hardcoded strings or internal keys (e.g., the "All" fallback, the
child.labelKey fallback, and any bare-number fallback). Update render logic in
the component (where subClients, nSelected, and child.labelKey are used) to
assume those values are present and to pass them through the i18n translation
function instead of falling back to literals; if needed, add a runtime guard
that throws or logs a clear error when a required translation key is missing so
the issue surfaces at runtime rather than silently degrading. Ensure all
user-facing text uses the i18n API and remove any hardcoded display text
occurrences referenced in this file (including the other noted locations).
---
Nitpick comments:
In `@src/components/form/client-restrictions-editor.tsx`:
- Around line 249-263: The same regex is duplicated in the validateTag props for
ArrayTagInputField (used for both customAllowed and customBlocked); replace the
inline /^[a-zA-Z0-9_./*-]+$/ with a single shared constant or the shared pattern
introduced in this PR (e.g., TAG_VALIDATE_REGEX or the PR's shared pattern
export) and import/use that in both validateTag callbacks so UI validation and
matching logic stay consistent; update usages in the ArrayTagInputField
instances and any related handlers like handleCustomAllowedChange/onInvalidTag
to reference the shared symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c023d94-ced6-4fc1-85b3-abb6ad339e14
📒 Files selected for processing (1)
src/components/form/client-restrictions-editor.tsx
| if (pattern.includes("*")) { | ||
| return globMatch(pattern, ua); | ||
| } |
There was a problem hiding this comment.
Glob patterns are anchored at start, but help text doesn't document this asymmetry
When a pattern contains *, globMatch performs a full-string, start-anchored match (the pattern starts matching from position 0). When a pattern lacks *, substring matching uses .includes() and can match anywhere.
Concretely:
"codex-cli"(no*) matches"my-codex-cli/1.0"✓ (substring anywhere)"codex-*"(glob) does NOT match"my-codex-cli/1.0"✗ (must start at position 0)
The customHelp text in all 5 locales (dashboard.json and sections.json) says only "With *: glob pattern matching" without noting it must match from the start. An admin writing codex-* expecting to match all UAs containing "codex-" will silently get no matches on UAs with prefixes. The correct pattern would be *codex-*.
Update the help text to explicitly document anchoring, e.g.:
"Without
*: case-insensitive substring match ('-'/'_' ignored). With*: glob pattern matching anchored at start (use*foo*to match anywhere)."
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/client-detector.ts
Line: 134-136
Comment:
**Glob patterns are anchored at start, but help text doesn't document this asymmetry**
When a pattern contains `*`, `globMatch` performs a **full-string, start-anchored match** (the pattern starts matching from position 0). When a pattern lacks `*`, substring matching uses `.includes()` and can match anywhere.
Concretely:
- `"codex-cli"` (no `*`) matches `"my-codex-cli/1.0"` ✓ (substring anywhere)
- `"codex-*"` (glob) does **NOT** match `"my-codex-cli/1.0"` ✗ (must start at position 0)
The `customHelp` text in all 5 locales (`dashboard.json` and `sections.json`) says only "With `*`: glob pattern matching" without noting it must match from the start. An admin writing `codex-*` expecting to match all UAs containing "codex-" will silently get no matches on UAs with prefixes. The correct pattern would be `*codex-*`.
Update the help text to explicitly document anchoring, e.g.:
> "Without `*`: case-insensitive substring match ('-'/'_' ignored). With `*`: glob pattern matching anchored at start (use `*foo*` to match anywhere)."
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 8d4ad51 — customHelp text in all 5 locales now documents glob anchoring behavior explicitly.
- Make subClients and nSelected required in component interfaces (all callers already provide them; removes hardcoded "All" fallbacks per i18n rules) - Update customHelp in all 10 i18n files to document glob anchoring behavior (use *foo* to match anywhere) - Update test fixtures with required translation props Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
messages/ja/dashboard.json (1)
1038-1042: 把创建表单里的 glob 帮助文案也补齐。同文件的
userManagement.userEditSection.fields.allowedClients已经解释了*的语义，这里却仍只有 placeholder。这样创建/编辑两个入口的提示会继续不一致。建议补齐同一套帮助文案
"allowedClients": { "label": "許可されたクライアント", "description": "このアカウントを使用できるCLI/IDEクライアントを制限します。空の場合は制限なし。", "customLabel": "カスタムクライアントパターン", - "customPlaceholder": "パターンを入力(例: 'my-ide'、'codex-*')" + "customPlaceholder": "パターンを入力(例: 'my-ide'、'codex-*')", + "customHelp": "'*' なし：大文字小文字を区別しない部分一致('-' と '_' は無視)。'*' あり：UA の先頭からの glob パターンマッチ(*foo* で任意の位置に一致; 大文字小文字を区別しない、文字はそのまま一致)。" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@messages/ja/dashboard.json` around lines 1038 - 1042, The allowedClients block in messages/ja/dashboard.json is missing the detailed help text (only a placeholder exists) causing inconsistency with userManagement.userEditSection.fields.allowedClients; update the allowedClients entry (keys: "allowedClients", "customLabel", "customPlaceholder") to include the same explanatory help/description about glob semantics (e.g., that '*' matches any substring and examples like 'my-ide' or 'codex-*') so the create form and edit form show consistent guidance.messages/ru/dashboard.json (1)
1040-1045: 把创建用户表单里的 glob 说明也同步过来。同文件的
userManagement.userEditSection.fields.allowedClients已补上customHelp，这里只更新了customPlaceholder。这样创建/编辑两个入口的提示会漂移，管理员在创建用户时仍看不到*的匹配规则。建议补齐同一套帮助文案
"allowedClients": { "label": "Разрешённые клиенты", "description": "Ограничьте, какие CLI/IDE клиенты могут использовать эту учётную запись. Пусто = без ограничений.", "customLabel": "Пользовательские шаблоны клиентов", - "customPlaceholder": "Введите шаблон (например, 'my-ide', 'codex-*')" + "customPlaceholder": "Введите шаблон (например, 'my-ide', 'codex-*')", + "customHelp": "Без '*': поиск подстроки без учёта регистра ('-' и '_' игнорируются). С '*': glob-шаблон от начала UA (используйте *foo* для поиска в любом месте; без учёта регистра, символы буквальные)." },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@messages/ru/dashboard.json` around lines 1040 - 1045, The allowedClients block only defines customPlaceholder but is missing the same customHelp used for the edit form, causing inconsistent guidance; update the allowedClients object (the same key "allowedClients" under messages) to include a customHelp field matching userManagement.userEditSection.fields.allowedClients (explain glob patterns and that '*' is supported, e.g. mention "Поддерживаются шаблоны glob, например '*' и 'codex-*'") so create and edit forms show identical help text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@messages/ja/dashboard.json`:
- Around line 1038-1042: The allowedClients block in messages/ja/dashboard.json
is missing the detailed help text (only a placeholder exists) causing
inconsistency with userManagement.userEditSection.fields.allowedClients; update
the allowedClients entry (keys: "allowedClients", "customLabel",
"customPlaceholder") to include the same explanatory help/description about glob
semantics (e.g., that '*' matches any substring and examples like 'my-ide' or
'codex-*') so the create form and edit form show consistent guidance.
In `@messages/ru/dashboard.json`:
- Around line 1040-1045: The allowedClients block only defines customPlaceholder
but is missing the same customHelp used for the edit form, causing inconsistent
guidance; update the allowedClients object (the same key "allowedClients" under
messages) to include a customHelp field matching
userManagement.userEditSection.fields.allowedClients (explain glob patterns and
that '*' is supported, e.g. mention "Поддерживаются шаблоны glob, например '*' и
'codex-*'") so create and edit forms show identical help text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a026bbb2-a9f0-427a-8c2c-e0babae36e75
📒 Files selected for processing (14)
messages/en/dashboard.jsonmessages/en/settings/providers/form/sections.jsonmessages/ja/dashboard.jsonmessages/ja/settings/providers/form/sections.jsonmessages/ru/dashboard.jsonmessages/ru/settings/providers/form/sections.jsonmessages/zh-CN/dashboard.jsonmessages/zh-CN/settings/providers/form/sections.jsonmessages/zh-TW/dashboard.jsonmessages/zh-TW/settings/providers/form/sections.jsonsrc/app/[locale]/dashboard/_components/user/forms/access-restrictions-section.tsxsrc/app/[locale]/dashboard/_components/user/forms/user-edit-section.tsxsrc/components/form/client-restrictions-editor.test.tsxsrc/components/form/client-restrictions-editor.tsx
✅ Files skipped from review due to trivial changes (1)
- src/components/form/client-restrictions-editor.test.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- src/app/[locale]/dashboard/_components/user/forms/user-edit-section.tsx
- src/app/[locale]/dashboard/_components/user/forms/access-restrictions-section.tsx
- messages/zh-CN/settings/providers/form/sections.json
- messages/en/dashboard.json
- messages/ja/settings/providers/form/sections.json
- src/components/form/client-restrictions-editor.tsx
- messages/zh-TW/dashboard.json
- messages/zh-TW/settings/providers/form/sections.json
- messages/zh-CN/dashboard.json
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/form/client-restrictions-editor.tsx (1)
249-249: 考虑使用 useCallback 缓存 validateTag 函数。当前每次渲染都会创建新的箭头函数。如果
ArrayTagInputField内部对该 prop 有依赖比较，可能导致不必要的重渲染。可选优化
+import { useCallback, useMemo } from "react"; -import { useMemo } from "react"; // 在组件内部添加: + const validateTag = useCallback((tag: string) => CLIENT_TAG_PATTERN.test(tag), []); // 然后使用: - validateTag={(tag: string) => CLIENT_TAG_PATTERN.test(tag)} + validateTag={validateTag}Also applies to: 263-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/form/client-restrictions-editor.tsx` at line 249, The validateTag arrow function passed to ArrayTagInputField is recreated on every render; wrap it with React.useCallback to memoize it and avoid unnecessary re-renders. Replace the inline prop validateTag={(tag: string) => CLIENT_TAG_PATTERN.test(tag)} (and the similar occurrence around the other reference) with a const validateTag = useCallback((tag: string) => CLIENT_TAG_PATTERN.test(tag), [CLIENT_TAG_PATTERN]) and pass validateTag to ArrayTagInputField so the reference remains stable across renders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/form/client-restrictions-editor.tsx`:
- Line 249: The validateTag arrow function passed to ArrayTagInputField is
recreated on every render; wrap it with React.useCallback to memoize it and
avoid unnecessary re-renders. Replace the inline prop validateTag={(tag: string)
=> CLIENT_TAG_PATTERN.test(tag)} (and the similar occurrence around the other
reference) with a const validateTag = useCallback((tag: string) =>
CLIENT_TAG_PATTERN.test(tag), [CLIENT_TAG_PATTERN]) and pass validateTag to
ArrayTagInputField so the reference remains stable across renders.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9f4cd89-8998-4640-80d9-0fbae5806b48
📒 Files selected for processing (1)
src/components/form/client-restrictions-editor.tsx
| if (selectedChildren.length === preset.children.length) { | ||
| return [...filtered, preset.value]; | ||
| } |
There was a problem hiding this comment.
Consolidation check uses length equality, not set equality
selectedChildren.length === preset.children.length only verifies the count of selected children, not that they are the correct child values. If a caller passes an array of the right length but with wrong (or duplicate) values, the function incorrectly consolidates to the parent value.
For example, if selectedChildren = ["a", "b", "c", "d", "e", "f"] (6 invalid values), setChildSelection would still return [preset.value] ("claude-code"), silently corrupting the stored restriction list.
The current callers always pass valid values from getSelectedChildren, but this function is exported and is called from handleChildSelectionChange without further validation. A safer check would verify that all expected children are present:
const allChildValues = preset.children.map((c) => c.value);
const allChildValuesSet = new Set(allChildValues);
const filteredSelected = selectedChildren.filter((v) => allChildValuesSet.has(v));
const deduped = [...new Set(filteredSelected)];
// ...
if (deduped.length === preset.children.length) {
return [...filtered, preset.value];
}
return [...filtered, ...deduped];Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/client-restrictions/client-presets.ts
Line: 136-138
Comment:
**Consolidation check uses length equality, not set equality**
`selectedChildren.length === preset.children.length` only verifies the *count* of selected children, not that they are the correct child values. If a caller passes an array of the right length but with wrong (or duplicate) values, the function incorrectly consolidates to the parent value.
For example, if `selectedChildren = ["a", "b", "c", "d", "e", "f"]` (6 invalid values), `setChildSelection` would still return `[preset.value]` (`"claude-code"`), silently corrupting the stored restriction list.
The current callers always pass valid values from `getSelectedChildren`, but this function is exported and is called from `handleChildSelectionChange` without further validation. A safer check would verify that all expected children are present:
```ts
const allChildValues = preset.children.map((c) => c.value);
const allChildValuesSet = new Set(allChildValues);
const filteredSelected = selectedChildren.filter((v) => allChildValuesSet.has(v));
const deduped = [...new Set(filteredSelected)];
// ...
if (deduped.length === preset.children.length) {
return [...filtered, preset.value];
}
return [...filtered, ...deduped];
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
All callers pass values from getSelectedChildren which filters against preset.children. Invalid/duplicate values cannot reach this path. Adding set equality is defensive against a scenario the call chain already prevents.
| if (selected.length === 0 || selected.length === preset.children.length) { | ||
| return translations.subClients.all; |
There was a problem hiding this comment.
selected.length === 0 branch is unreachable dead code
When activeList is non-null, isPresetSelected has already confirmed that at least one alias of the preset is present in the list. getSelectedChildren then returns either all children (when preset.value is present) or the subset of children explicitly listed — in either case the result cannot be empty. The selected.length === 0 condition on this line is therefore never true when control reaches it.
Consider removing the selected.length === 0 || guard to make the intent clearer:
| if (selected.length === 0 || selected.length === preset.children.length) { | |
| return translations.subClients.all; | |
| if (selected.length === preset.children.length) { |
If keeping it for defensive safety, a comment explaining why it is unreachable would aid future readers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/form/client-restrictions-editor.tsx
Line: 102-103
Comment:
**`selected.length === 0` branch is unreachable dead code**
When `activeList` is non-null, `isPresetSelected` has already confirmed that at least one alias of the preset is present in the list. `getSelectedChildren` then returns either all children (when `preset.value` is present) or the subset of children explicitly listed — in either case the result cannot be empty. The `selected.length === 0` condition on this line is therefore never true when control reaches it.
Consider removing the `selected.length === 0 ||` guard to make the intent clearer:
```suggestion
if (selected.length === preset.children.length) {
```
If keeping it for defensive safety, a comment explaining why it is unreachable would aid future readers.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Correct analysis — the branch is unreachable under current logic. Keeping as a defensive guard that prevents subtle bugs if getSelectedChildren behavior changes during refactoring.
| latestUserRef.current = form.values.user || defaultValues.user; | ||
| latestKeyRef.current = form.values.key || defaultValues.key; |
There was a problem hiding this comment.
Side effect (ref mutation) in render body may behave unexpectedly under Strict Mode
Mutating a ref directly in the render body is technically allowed in React but is a side effect that will run twice in React 18 Strict Mode (development builds). While the mutation itself is idempotent here, mixing it with the handler-side update (latestUserRef.current = next at lines 251/272) means there are two paths that set the ref's value. If a re-render is triggered between two rapid handler calls, the render-body assignment will overwrite the in-flight handler-set value with the stale form value before the form state has been committed, reintroducing the same stale-read issue this PR is meant to fix.
A more robust pattern is to rely solely on the handler-side assignment and read form.values directly for the display variables:
// Remove these two lines from the render body:
// latestUserRef.current = form.values.user || defaultValues.user;
// latestKeyRef.current = form.values.key || defaultValues.key;The handler already sets latestRef.current = next before calling form.setValue, so the ref stays ahead of the React state and the race condition is still resolved.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/user/create-user-dialog.tsx
Line: 229-230
Comment:
**Side effect (ref mutation) in render body may behave unexpectedly under Strict Mode**
Mutating a ref directly in the render body is technically allowed in React but is a side effect that will run twice in React 18 Strict Mode (development builds). While the mutation itself is idempotent here, mixing it with the handler-side update (`latestUserRef.current = next` at lines 251/272) means there are two paths that set the ref's value. If a re-render is triggered between two rapid handler calls, the render-body assignment will overwrite the in-flight handler-set value with the stale form value before the form state has been committed, reintroducing the same stale-read issue this PR is meant to fix.
A more robust pattern is to rely solely on the handler-side assignment and read `form.values` directly for the display variables:
```ts
// Remove these two lines from the render body:
// latestUserRef.current = form.values.user || defaultValues.user;
// latestKeyRef.current = form.values.key || defaultValues.key;
```
The handler already sets `latestRef.current = next` before calling `form.setValue`, so the ref stays ahead of the React state and the race condition is still resolved.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
React 18/19 automatic batching guarantees no re-render between the two onChange calls in handleAllowToggle/handleBlockToggle (same synchronous event handler). The render-body sync is needed for correctness after external form changes (reset, other handlers).
| subClients: Record<string, string>; | ||
| nSelected: string; |
There was a problem hiding this comment.
subClients type allows missing required keys without compile-time errors
subClients: Record<string, string> accepts a translations object that silently omits required keys (all, cli, vscode, sdk-ts, sdk-py, cli-sdk, gh-action). If a locale integration ever passes an incomplete map, translations.subClients.all (line 103) and translations.subClients[child.labelKey] (line 109) return undefined at runtime, rendering as empty labels with no TypeScript warning.
Consider tightening the type by listing all expected keys explicitly, while still accepting extras for forward-compatibility:
subClients: Record<"all" | "cli" | "vscode" | "sdk-ts" | "sdk-py" | "cli-sdk" | "gh-action", string>
& Record<string, string>;This way TypeScript will error at the call site if any required sub-client label is missing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/form/client-restrictions-editor.tsx
Line: 42-43
Comment:
**`subClients` type allows missing required keys without compile-time errors**
`subClients: Record<string, string>` accepts a translations object that silently omits required keys (`all`, `cli`, `vscode`, `sdk-ts`, `sdk-py`, `cli-sdk`, `gh-action`). If a locale integration ever passes an incomplete map, `translations.subClients.all` (line 103) and `translations.subClients[child.labelKey]` (line 109) return `undefined` at runtime, rendering as empty labels with no TypeScript warning.
Consider tightening the type by listing all expected keys explicitly, while still accepting extras for forward-compatibility:
```ts
subClients: Record<"all" | "cli" | "vscode" | "sdk-ts" | "sdk-py" | "cli-sdk" | "gh-action", string>
& Record<string, string>;
```
This way TypeScript will error at the call site if any required sub-client label is missing.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Record<string, string> is the standard pattern for i18n translation maps in this project. Coupling the component interface to specific preset children values would require updating the type in 3+ places whenever a sub-client is added. Key completeness is enforced by the i18n system (next-intl).
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
2 participants
Summary
This PR addresses 5 critical issues in the client restriction UI and security hardening:
Problem
Client restriction UI in the dashboard had several issues:
Race condition in create-user-dialog — When toggling Allow/Block checkboxes for client restrictions, the second `form.setValue("user", ...)` call read stale state from `form.values.user`, overwriting the first update. This made checkboxes appear non-functional when creating a new user.
No wildcard support for custom client patterns — Admins could only use exact substring matching (e.g., `codex-cli`). There was no way to express patterns like `codex-*` to match any client starting with `codex-`.
No granular control over Claude Code sub-clients — The `claude-code` preset was all-or-nothing. Admins couldn't selectively allow/block specific sub-clients (CLI, VS Code, SDK-TS, SDK-PY, CLI-SDK, GH Action).
Code duplication between dashboard and provider forms — `access-restrictions-section.tsx` (~320 lines) duplicated the preset/popover logic from `client-restrictions-editor.tsx`, leading to an i18n bug where the sub-client "All" label used the wrong translation key (`presetClients["sub-all"]` instead of `subClients?.all`).
ReDoS vulnerability in glob matching — The initial glob implementation used `RegExp` with `.` segments. Pathological patterns like `aaaaaaaac` caused catastrophic backtracking in V8's NFA engine.
Solution
Commit 1: `76a94569` — Fix race condition
Commit 2: `245d8858` — Add wildcard glob support
Commit 3: `9832dcc1` — Add popover multi-select for sub-clients
Commit 4: `03dd979f` — Refactor to shared editor component
Commit 5: `cfd0d222` — Replace regex glob with linear matcher
Related Issues & PRs
Follow-up to:
Enhances:
Files Changed (22 files, +588/-157)
Verification
Description enhanced by Claude AI
Greptile Summary
This PR delivers five targeted improvements to the client restriction system: a
useRef-based race condition fix in the create-user-dialog, wildcard glob support via a linear two-pointerglobMatchreplacing the ReDoS-vulnerable regex approach, granular sub-client popover control for Claude Code variants, a major code-deduplication refactor ofaccess-restrictions-section.tsxto delegate to the sharedClientRestrictionsEditor, and updated i18n across five locales.Key findings:
setChildSelectionconsolidation check is count-only (client-presets.tsline 136): whenselectedChildren.length === preset.children.length, the function consolidates to the parent value without verifying the children are actually the valid preset children. Callers currently always pass valid values, but the exported function has no guard against a wrong-count-matching array.subClientstranslations type is under-specified (client-restrictions-editor.tsxline 42): typed asRecord<string, string>instead of a union of required keys, so missing translations (e.g."all","cli") will silently render asundefinedrather than a compile-time error.create-user-dialog.tsxlines 229–230): mutatinglatestUserRef.currentandlatestKeyRef.currentdirectly in the render body is a side effect that React Strict Mode will execute twice. Since the handlers also update the same ref synchronously before callingform.setValue, the render-body assignment is redundant and could overwrite an in-flight handler update if a re-render is triggered between two rapid events.getChildDisplayText(client-restrictions-editor.tsxline 102): theselected.length === 0branch is structurally unreachable — any non-nullactiveListguarantees at least one child is returned bygetSelectedChildren.Confidence Score: 4/5
Record<string,string>typing, and a side-effect ref pattern — rather than outright functional bugs. None will cause visible regressions in the current call paths, but the exportedsetChildSelectionhelper and the ref-mutation pattern carry latent risk worth addressing.src/lib/client-restrictions/client-presets.ts(setChildSelection consolidation logic) andsrc/components/form/client-restrictions-editor.tsx(subClients type definition)Important Files Changed
globMatchreplacing regex-based glob; dispatches to it when pattern contains*; logic is correct and performance-safe.ClientRestrictionChild,childrenarray to claude-code preset, and helper functions.setChildSelectionconsolidation check uses count equality rather than content equality, risking incorrect parent consolidation if called with wrong-but-correct-count values.subClientstype isRecord<string,string>(weak typing); dead code branch ingetChildDisplayText; logic for child selection transitions is otherwise correct.useRef; ref update in render body is a side effect that may conflict with handler-side ref updates under concurrent rendering.ClientRestrictionsEditor; i18n bug with wrong translation key for "All" label is fixed.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Admin enters custom pattern] --> B{Pattern contains '*'?} B -- No --> C[normalize: lowercase + remove -/_] C --> D[UA.includes normalizedPattern] B -- Yes --> E[globMatch - two-pointer, O n*m] E --> F[Full-string match anchored at start of UA] F --> G{Match?} D --> G G -- No match in allowlist --> H[allowlist_miss → blocked] G -- Match in blocklist --> I[blocklist_hit → blocked] G -- Match in allowlist --> J[allowed] G -- No restriction --> K[no_restriction → allowed] L[UI: Preset checkbox Allow/Block] --> M{Preset has children?} M -- Yes claude-code --> N[Sub-client popover] N --> O{All selected?} O -- Yes --> P[Store parent value 'claude-code'] O -- No --> Q[Store individual child values] M -- No --> R[togglePresetSelection adds/removes preset value] S[create-user-dialog handler] --> T[Read latestRef.current] T --> U[Compute next state] U --> V[Update latestRef.current synchronously] V --> W[form.setValue triggers re-render]Comments Outside Diff (1)
tests/unit/proxy/client-detector.test.ts, line 241 (link)Stale test description — should say "4-of-4" not "3-of-3"
confirmClaudeCodeSignalsrequires all 4 signals to confirm:x-app-cli,ua-prefix,betas-present, andmetadata-user-id(line 107). The test description "3-of-3" is outdated.Prompt To Fix With AI
Last reviewed commit: 20c5900