fix(oauth): remove 8-character minimum on client secret#1920
fix(oauth): remove 8-character minimum on client secret#1920nuthalapativarun wants to merge 2 commits intoMCPJam:mainfrom
Conversation
The OAuth spec recommends secrets of at least 8 characters for security but does not require it. Some identity providers issue shorter secrets that users cannot change. Lower the minimum to 1 (non-empty) so that valid shorter client secrets are accepted. Fixes MCPJam#1723
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughClient secret validation in the server form hook was relaxed: the validator no longer enforces a minimum length and now only rejects secrets that are explicitly empty or whitespace (trimmed to 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts`:
- Around line 216-221: The guard in validateClientSecret is unreachable because
`value && value.length < 1` can never be true for strings; either remove the
dead branch and return null explicitly, or better: mirror validateClientId and
defensively trim the input to catch whitespace-only secrets (e.g., treat
value.trim().length === 0 as empty) and return "Client Secret cannot be empty if
provided"; keep references to validateClientSecret (and callers
AddServerModal.tsx and ServerDetailModal.tsx) when making this change so
validation behavior remains consistent with server-side
z.string().trim().min(1).optional().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a09290e-940c-439b-ba43-fef913a7f498
📒 Files selected for processing (1)
mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts
…ntSecret The previous `value && value.length < 1` condition was logically impossible for strings — a truthy string always has length ≥ 1. Replace with a trim check that mirrors validateClientId and catches whitespace-only secrets.
94ed8ca to
1190220
Compare
Summary
Fixes #1723
The OAuth spec recommends client secrets of at least 8 characters for security, but does not mandate it. Some identity providers issue shorter secrets that users cannot control.
The validation in
use-server-form.tswas enforcing a hard minimum of 8 characters:Changed to only reject empty strings (the field is already optional — if provided it must be non-empty):
Testing
banana, 6 chars)All 3 existing
use-server-formtests pass.