sync#1
Conversation
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: samanhappy <2755122+samanhappy@users.noreply.github.com>
…413) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: samanhappy <2755122+samanhappy@users.noreply.github.com> Co-authored-by: samanhappy <samanhappy@gmail.com>
…etadata endpoints (#438)
…ings for non-admin users (#440)
…ation management (#447)
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
We failed to fetch the diff for pull request #1
You can try again by commenting this pull request with @sourcery-ai review, or contact us for help.
|
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:
📝 WalkthroughWalkthroughAdds DB-backed DAO layer and DB-mode boot path, a full OAuth2 authorization server (incl. RFC 7591 dynamic client registration and callback handling), a registry proxy/marketplace, many backend controllers/routes, broad frontend UIs (settings, users, activity, imports, registry), Docker/entrypoint adjustments, localization updates, and supporting docs/examples. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant App as MCPHub (App)
participant OAuth as OAuth Server
participant DB as Postgres DB
Browser->>App: GET /oauth/authorize (start auth)
App->>OAuth: Render consent / issue PKCE challenge
Browser->>OAuth: User approves (consent), OAuth returns code+state
OAuth->>Browser: Redirect to /oauth/callback?code&state
Browser->>App: GET /oauth/callback?code&state
App->>OAuth: Exchange code for tokens (finishAuth)
OAuth->>App: Return tokens
App->>DB: Persist tokens/clients (saveToken/createClient) (rgba(34,139,34,0.5))
App->>App: Rebuild transports / refresh server info (rgba(70,130,180,0.5))
App->>Browser: Return success page or redirect
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||
Summary of ChangesHello @Xtothepowerofinfinity, 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 introduces a suite of powerful features designed to enhance MCPHub's security, flexibility, and user experience. It transforms MCPHub into a robust OAuth 2.0 Authorization Server, while simultaneously strengthening its integration with upstream OAuth-protected MCP servers. A major improvement is the comprehensive environment variable expansion within Highlights
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
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||||||||||||
![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 is a massive and impressive pull request that introduces a wide range of significant features and improvements to MCPHub. The implementation of a full OAuth 2.0 client and server is a major undertaking and appears to be well-executed, following relevant RFCs. The addition of session persistence with keep-alive pings and transparent session rebuild is a fantastic enhancement for reliability.
Key highlights of this PR include:
- Comprehensive OAuth 2.0 support: Both for upstream MCP servers (as a client) and for external applications (as an authorization server). The dynamic client registration (RFC 7591) and PKCE support are excellent additions.
- Session Persistence: The SSE keep-alive mechanism and session rebuild on restart will greatly improve the user experience and server robustness.
- Configuration Enhancements: The recursive environment variable expansion and the new configuration options for OAuth and other features provide much-needed flexibility and security.
- Code Quality and Refactoring: Many parts of the codebase have been refactored for better maintainability, such as moving path utilities and parameter conversion to separate modules, and converting the service registry to use async/await with ES modules.
- Fixes and UI improvements: Numerous bug fixes, like URL parameter decoding, and frontend enhancements, like the new hooks and improved forms, round out this substantial update.
- Comprehensive Testing: The addition of extensive tests for the new features is commendable and crucial for a change of this size.
I have a couple of suggestions for improvement, mainly concerning potential race conditions in configuration management and some minor code cleanup. Overall, this is an excellent contribution that significantly advances the capabilities and maturity of the project.
| settings.oauthClients.push(client); | ||
| saveSettings(settings); |
There was a problem hiding this comment.
The current implementation of saveSettings (and by extension, functions like createOAuthClient, updateOAuthClient, deleteOAuthClient, saveToken, and revokeToken) performs a read-modify-write operation on the mcp_settings.json file without any locking mechanism. This can lead to race conditions and data loss if multiple concurrent requests attempt to modify the settings file. For example, if two requests try to add an OAuth client at the same time, one of the additions could be overwritten.
To make this operation atomic and safe for concurrent access, I recommend implementing a file-locking mechanism around the read and write operations in saveSettings. A library like proper-lockfile could be used for this. This would likely require making saveSettings and its callers asynchronous.
| const separatorIndex = stateValue.indexOf(':'); | ||
| if (separatorIndex > 0) { | ||
| return stateValue.slice(0, separatorIndex); | ||
| } |
There was a problem hiding this comment.
This fallback logic for parsing the state parameter by looking for a colon seems to be from a previous implementation. The current state generation in mcpOAuthProvider.ts creates a URL-safe base64 encoded JSON object, which doesn't contain a colon in this manner. If this format is no longer supported, this block can be removed to simplify the code. If it's kept for backward compatibility, a comment explaining this would be beneficial.
Co-authored-by: samanhappy <samanhappy@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/copilot-instructions.md (1)
46-106: Update line 46 port comment from 3001 to 3000The backend actually runs on port 3000 by default (configured in
src/config/index.tsline 12:port: process.env.PORT || 3000). The comment on line 46 incorrectly states "Backend on :3001", which contradicts the manual validation curl commands on lines 96-107 that correctly use port 3000. Update line 46 to read:pnpm dev # Backend on :3000, Frontend on :5173frontend/src/services/promptService.ts (1)
62-69: Function signature allows optionalserverbut all callers provide it; consider enforcing type safety or handling the undefined caseThe
getPromptfunction signature acceptsserver?: string(optional), but directly interpolates it into the URL:/mcp/${server}/prompts/.... Ifserveris undefined, this produces/mcp/undefined/prompts/...instead of a valid route.While the only current call site (PromptCard) always provides a non-empty server string (it's a required prop in the interface), the optional type signature creates a misleading API contract. Additionally, the server parameter should be URL-encoded consistently with promptName.
Either make
server: stringrequired in the function signature to match the backend's validation (/mcp/:serverName/prompts/:promptNamerequires both), or handle the undefined case explicitly:const serverSegment = server ? `/${encodeURIComponent(server)}` : ''; const response = await apiPost( `/mcp${serverSegment}/prompts/${encodeURIComponent(request.promptName)}`, { name: request.promptName, arguments: request.arguments, }, );
♻️ Duplicate comments (2)
src/controllers/oauthCallbackController.ts (1)
82-103: Backward compatibility fallback for state parsing.The colon-delimiter fallback (lines 97-100) maintains backward compatibility with older state formats. A previous review flagged this; keeping it with this comment is acceptable.
src/models/OAuth.ts (1)
54-69: Race condition in client creation persists (as noted in past review).The
createOAuthClientfunction performs a read-modify-write operation without locking. As noted in the past review, concurrent client creation could lead to data loss. This is a known issue with the current settings persistence approach.
🟡 Minor comments (14)
.github/copilot-instructions.md-145-149 (1)
145-149: FixfetchInterceptorfilename extension in project structure sectionThe critical frontend files list references
frontend/src/utils/fetchInterceptor.js, but the actual file is TypeScript (fetchInterceptor.ts), and imports use the compiled.jsonly at runtime.Suggest updating the doc entry to:
-- `frontend/src/utils/fetchInterceptor.js` - Backend API interaction +- `frontend/src/utils/fetchInterceptor.ts` - Backend API interactionto match the source file name.
frontend/src/components/ServerForm.tsx-489-494 (1)
489-494: Non-null assertion on potentially undefinedopenapi.Using
prev.openapi!assumesopenapiis always defined, but it could beundefinedif the form state was never properly initialized. This could cause runtime errors.- onChange={() => - setFormData((prev) => ({ - ...prev, - openapi: { ...prev.openapi!, inputMode: 'url' }, - })) - } + onChange={() => + setFormData((prev) => ({ + ...prev, + openapi: { ...(prev.openapi || {}), inputMode: 'url' }, + })) + }Apply the same pattern to other occurrences using the non-null assertion on
prev.openapi!.Also applies to: 506-511
frontend/src/components/ServerForm.tsx-35-43 (1)
35-43:descriptionfield added but not inEnvVartype.The
getInitialServerEnvVarsfunction adds adescriptionfield (line 41) to each object, but theEnvVarinterface only haskeyandvalueproperties. This field is unused and inconsistent with the type.const getInitialServerEnvVars = (data: Server | null): EnvVar[] => { if (!data || !data.config || !data.config.env) return []; return Object.entries(data.config.env).map(([key, value]) => ({ key, value, - description: '', // You can set a default description if needed })); };frontend/src/hooks/useUserData.ts-13-37 (1)
13-37: Missingtin dependency array forfetchUsers.The
tfunction fromuseTranslationis used on line 18 but not included in theuseCallbackdependency array. This could lead to stale translations if the language changes.- }, []); + }, [t]);frontend/src/hooks/useUserData.ts-69-83 (1)
69-83: Inconsistent return types indeleteUser.The function returns
result(object) on lines 74 and 78, but returnsfalse(boolean) on line 81. This inconsistency makes it difficult for consumers to handle the response reliably.const deleteUser = async (username: string) => { try { const result = await apiDelete(`/users/${username}`); if (!result?.success) { setError(result?.message || t('users.deleteError')); - return result; + return false; } triggerRefresh(); - return result; + return true; } catch (err) { setError(err instanceof Error ? err.message : 'Failed to delete user'); return false; } };docs/api-reference/system.mdx-29-34 (1)
29-34: Document optionalserverNameparameter for MCP settings exportThe implementation of the MCP settings export handler supports an optional
serverNamequery to export a single server’s configuration, but this page only describes the full export.Consider adding a short note under Export MCP Settings like:
- Optional query:
?serverName=<name>– returns JSON containing only that server undermcpServers.This keeps the API reference aligned with the controller behavior and frontend usage.
Also applies to: 96-104
README.zh.md-173-189 (1)
173-189: Add language identifier to code fence.The code fence starting at line 177 is missing a language identifier, which affects syntax highlighting and markdown linting.
Apply this diff:
您可以将智能路由与分组筛选结合使用,仅在特定服务器分组内搜索: -``` +```text # 仅在生产服务器中搜索 http://localhost:3000/mcp/$smart/production # 仅在开发服务器中搜索 http://localhost:3000/mcp/$smart/development</blockquote></details> <details> <summary>examples/oauth-dynamic-registration-config.json-21-21 (1)</summary><blockquote> `21-21`: **Invalid bcrypt password hash placeholder.** The password hash `"$2b$10$abcdefghijklmnopqrstuv"` is not a valid bcrypt hash (bcrypt hashes are 60 characters long). For an example file, consider using a valid bcrypt hash of a well-known test password with a clear comment. Apply this diff to use a valid example hash: ```diff { "username": "admin", - "password": "$2b$10$abcdefghijklmnopqrstuv", + "password": "$2b$10$YourValidBcryptHashHere1234567890123456789012345678", + "// Note: Replace with actual bcrypt hash. Example above hashes 'changeme'": "", "isAdmin": true }Committable suggestion skipped: line range outside the PR's diff.
frontend/src/hooks/useRegistryData.ts-28-80 (1)
28-80:searchQuerymissing fromfetchRegistryServersdependencies but used in fallback.The function uses
searchQueryas a fallback (line 40) whensearchparameter is undefined, butsearchQueryis not in the dependency array. This could cause stale closure issues.const fetchRegistryServers = useCallback( async (cursor?: string, search?: string) => { // ... implementation }, - [t, serversPerPage], + [t, serversPerPage, searchQuery], );Alternatively, always require the
searchparameter explicitly to avoid relying on closed-over state.frontend/src/hooks/useRegistryData.ts-125-134 (1)
125-134:changeServersPerPagehas race condition with state updates.The function calls
fetchRegistryServersimmediately aftersetServersPerPage, but React state updates are async. The fetch may use the oldserversPerPagevalue.const changeServersPerPage = useCallback( - async (newServersPerPage: number) => { - setServersPerPage(newServersPerPage); - setCurrentPage(1); - setCursorHistory([]); - setAllServers([]); - await fetchRegistryServers(undefined, searchQuery); - }, - [searchQuery, fetchRegistryServers], + (newServersPerPage: number) => { + setServersPerPage(newServersPerPage); + setCurrentPage(1); + setCursorHistory([]); + setAllServers([]); + // Fetch will be triggered by useEffect watching serversPerPage + }, + [], );Or pass
newServersPerPagedirectly to the fetch function.Committable suggestion skipped: line range outside the PR's diff.
src/controllers/oauthCallbackController.ts-267-267 (1)
267-267: Settingstatus = 'connected'before actual connection attempt.Line 267 sets
serverInfo.status = 'connected'before the client connection attempt (lines 277-317). If connection fails, the status remains 'connected' which is misleading.- // Update server status to indicate OAuth is complete - serverInfo.status = 'connected'; if (serverInfo.oauth) { serverInfo.oauth.authorizationUrl = undefined; serverInfo.oauth.state = undefined; serverInfo.oauth.codeVerifier = undefined; } // Check if client needs to be connected const isClientConnected = serverInfo.client && serverInfo.client.getServerCapabilities(); if (!isClientConnected) { // Client is not connected yet, connect it if (serverInfo.client && serverInfo.transport) { console.log(`Connecting client with refreshed transport for: ${serverInfo.name}`); try { await serverInfo.client.connect(serverInfo.transport, serverInfo.options); + serverInfo.status = 'connected'; console.log(`Client connected successfully for: ${serverInfo.name}`); // ... rest } catch (connectError) { console.error(`Error connecting client for ${serverInfo.name}:`, connectError); + serverInfo.status = 'disconnected'; // ... } } + } else { + serverInfo.status = 'connected'; }Committable suggestion skipped: line range outside the PR's diff.
frontend/src/components/RegistryServerCard.tsx-28-40 (1)
28-40: Date parsing edge case.
new Date(dateString)doesn't throw for invalid strings—it returns anInvalid Dateobject. Thecatchblock won't trigger for malformed dates; instead, the function will returnNaN/NaN/NaN.const formatDate = (dateString?: string) => { if (!dateString) return ''; try { const date = new Date(dateString); + if (isNaN(date.getTime())) return ''; const year = date.getFullYear(); const month = (date.getMonth() + 1).toString().padStart(2, '0'); const day = date.getDate().toString().padStart(2, '0'); return `${year}/${month}/${day}`; } catch { return ''; } };frontend/src/components/RegistryServerCard.tsx-64-66 (1)
64-66: Add keyboard accessibility for the clickable card.The card is clickable via
onClickbut lacks keyboard navigation support. Addrole="button",tabIndex, andonKeyDownfor accessibility compliance.<div className="bg-white border border-gray-200 rounded-xl p-4 hover:shadow-lg hover:border-blue-400 hover:-translate-y-1 transition-all duration-300 cursor-pointer group relative overflow-hidden h-full flex flex-col" onClick={handleClick} + role="button" + tabIndex={0} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + handleClick(); + } + }} >frontend/src/pages/MarketPage.tsx-717-722 (1)
717-722: Registry pagination display has string concatenation issue.The total count display concatenates a string
'+'with a number which may produce unexpected results in the translation. The translation key expects a number fortotal, but receives a string.? t('registry.showing', { from: (currentPage - 1) * serversPerPage + 1, to: (currentPage - 1) * serversPerPage + servers.length, - total: allServers.length + (registryHasNextPage ? '+' : ''), + total: `${allServers.length}${registryHasNextPage ? '+' : ''}`, })Or update the translation to handle this case properly.
| // Build initial data for ServerForm | ||
| const getInitialFormData = () => { | ||
| if (!selectedOption || !selectedInstallType) return null; | ||
| console.log('Building initial form data for:', selectedOption); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove debug console.log statement.
This appears to be a development artifact that should be removed before merging.
const getInitialFormData = () => {
if (!selectedOption || !selectedInstallType) return null;
- console.log('Building initial form data for:', selectedOption);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/components/RegistryServerDetail.tsx around line 134, remove the
development debug statement console.log('Building initial form data for:',
selectedOption);; simply delete that line (or replace it with appropriate
logging mechanism if persistent logs are required) to avoid leaking debug output
in production.
| const exportMCPSettings = async (serverName?: string) => { | ||
| setLoading(true); | ||
| setError(null); | ||
| try { | ||
| return await apiGet(`/mcp-settings/export?serverName=${serverName ? serverName : ''}`); | ||
| } catch (error) { | ||
| console.error('Failed to export MCP settings:', error); | ||
| const errorMessage = error instanceof Error ? error.message : 'Failed to export MCP settings'; | ||
| setError(errorMessage); | ||
| showToast(errorMessage); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
exportMCPSettings doesn't return a value on error.
The function implicitly returns undefined when catching an error, but the caller in ServerCard.tsx expects a result with .data. This will cause a runtime error.
const exportMCPSettings = async (serverName?: string) => {
setLoading(true);
setError(null);
try {
return await apiGet(`/mcp-settings/export?serverName=${serverName ? serverName : ''}`);
} catch (error) {
console.error('Failed to export MCP settings:', error);
const errorMessage = error instanceof Error ? error.message : 'Failed to export MCP settings';
setError(errorMessage);
showToast(errorMessage);
+ return null;
} finally {
setLoading(false);
}
};Then update the caller to check for null before accessing .data.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const exportMCPSettings = async (serverName?: string) => { | |
| setLoading(true); | |
| setError(null); | |
| try { | |
| return await apiGet(`/mcp-settings/export?serverName=${serverName ? serverName : ''}`); | |
| } catch (error) { | |
| console.error('Failed to export MCP settings:', error); | |
| const errorMessage = error instanceof Error ? error.message : 'Failed to export MCP settings'; | |
| setError(errorMessage); | |
| showToast(errorMessage); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }; | |
| const exportMCPSettings = async (serverName?: string) => { | |
| setLoading(true); | |
| setError(null); | |
| try { | |
| return await apiGet(`/mcp-settings/export?serverName=${serverName ? serverName : ''}`); | |
| } catch (error) { | |
| console.error('Failed to export MCP settings:', error); | |
| const errorMessage = error instanceof Error ? error.message : 'Failed to export MCP settings'; | |
| setError(errorMessage); | |
| showToast(errorMessage); | |
| return null; | |
| } finally { | |
| setLoading(false); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In frontend/src/hooks/useSettingsData.ts around lines 602 to 615,
exportMCPSettings currently swallows errors and returns undefined on failure
which causes callers (e.g., ServerCard.tsx) to crash when accessing .data;
modify exportMCPSettings to always return a consistent shape (e.g., null or an
object) on error — catch the error, setError/showToast as now, then return null
(or a defined error result) so callers can handle it; then update the
ServerCard.tsx caller to null-check the result before accessing .data (e.g., if
(!result) { handle error } or guard result.data) to avoid runtime exceptions.
| if (!data.success) { | ||
| setError(data.message || t('users.fetchError')); | ||
| return; | ||
| } | ||
|
|
||
| if (data && data.success && Array.isArray(data.data)) { | ||
| setUsers(data.data); | ||
| } else { | ||
| console.error('Invalid user data format:', data); | ||
| setUsers([]); | ||
| } | ||
|
|
||
| setError(null); |
There was a problem hiding this comment.
Redundant success check and incorrect error state clearing.
The success check on line 22 duplicates the check on line 17. Additionally, setError(null) on line 29 executes even after setting an error on line 18 (when !data.success), immediately clearing the error.
if (!data.success) {
setError(data.message || t('users.fetchError'));
return;
}
- if (data && data.success && Array.isArray(data.data)) {
+ if (Array.isArray(data.data)) {
setUsers(data.data);
+ setError(null);
} else {
console.error('Invalid user data format:', data);
setUsers([]);
+ setError(t('users.invalidDataFormat'));
}
-
- setError(null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!data.success) { | |
| setError(data.message || t('users.fetchError')); | |
| return; | |
| } | |
| if (data && data.success && Array.isArray(data.data)) { | |
| setUsers(data.data); | |
| } else { | |
| console.error('Invalid user data format:', data); | |
| setUsers([]); | |
| } | |
| setError(null); | |
| if (!data.success) { | |
| setError(data.message || t('users.fetchError')); | |
| return; | |
| } | |
| if (Array.isArray(data.data)) { | |
| setUsers(data.data); | |
| setError(null); | |
| } else { | |
| console.error('Invalid user data format:', data); | |
| setUsers([]); | |
| setError(t('users.invalidDataFormat')); | |
| } |
🤖 Prompt for AI Agents
In frontend/src/hooks/useUserData.ts around lines 17 to 29, the code redundantly
checks data.success twice and always calls setError(null) at the end which
clears errors set in the early-return path; remove the second data.success check
and restructure the branches so the early-return when !data.success exits before
any downstream logic, and only call setError(null) inside the successful branch
after setting users (or set an error and empty users on invalid format),
ensuring you don't clear errors after they've been set.
| if (client.metadata || contacts || logo_uri || client_uri || policy_uri || tos_uri) { | ||
| updates.metadata = { | ||
| ...client.metadata, | ||
| contacts, | ||
| logo_uri, | ||
| client_uri, | ||
| policy_uri, | ||
| tos_uri, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Undefined values may unintentionally clear existing metadata fields.
When destructuring contacts, logo_uri, etc. from req.body, undefined values will overwrite existing metadata. Only fields explicitly provided should be updated.
- if (client.metadata || contacts || logo_uri || client_uri || policy_uri || tos_uri) {
- updates.metadata = {
- ...client.metadata,
- contacts,
- logo_uri,
- client_uri,
- policy_uri,
- tos_uri,
- };
- }
+ if (client.metadata || contacts || logo_uri || client_uri || policy_uri || tos_uri) {
+ updates.metadata = {
+ ...client.metadata,
+ ...(contacts !== undefined && { contacts }),
+ ...(logo_uri !== undefined && { logo_uri }),
+ ...(client_uri !== undefined && { client_uri }),
+ ...(policy_uri !== undefined && { policy_uri }),
+ ...(tos_uri !== undefined && { tos_uri }),
+ };
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (client.metadata || contacts || logo_uri || client_uri || policy_uri || tos_uri) { | |
| updates.metadata = { | |
| ...client.metadata, | |
| contacts, | |
| logo_uri, | |
| client_uri, | |
| policy_uri, | |
| tos_uri, | |
| }; | |
| } | |
| if (client.metadata || contacts || logo_uri || client_uri || policy_uri || tos_uri) { | |
| updates.metadata = { | |
| ...client.metadata, | |
| ...(contacts !== undefined ? { contacts } : {}), | |
| ...(logo_uri !== undefined ? { logo_uri } : {}), | |
| ...(client_uri !== undefined ? { client_uri } : {}), | |
| ...(policy_uri !== undefined ? { policy_uri } : {}), | |
| ...(tos_uri !== undefined ? { tos_uri } : {}), | |
| }; | |
| } |
🤖 Prompt for AI Agents
In src/controllers/oauthDynamicRegistrationController.ts around lines 435 to
444, the code unconditionally sets contacts, logo_uri, client_uri, policy_uri,
and tos_uri into updates.metadata which lets undefined values overwrite existing
client.metadata; change the logic to merge client.metadata with only the
explicitly provided fields by building a new metadata object that starts from
client.metadata and then conditionally assigns each property only if the
corresponding request field is !== undefined (or present), e.g. check each field
and add it to the metadata map before assigning updates.metadata, so existing
metadata keys are preserved when a field is omitted.
…y innovations Major upstream features added: - OAuth 2.0 Authorization Server with RFC 7591 - Environment variable expansion - User management with RBAC - Registry integration - Session persistence with SSE keepalive - Enhanced smart routing - Vector search improvements Local XInfty features preserved: - X^∞ - Quiet_Revolution documentation - SERVER_STARTUP_DELAY environment variable - copyServer() functionality - Mistral API support Build and core tests passing.
Co-authored-by: samanhappy <samanhappy@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/middlewares/auth.ts (1)
61-86: Based on my verification, I can now provide the rewritten review comment:Bearer auth must set user context to avoid downstream failures.
The
validateBearerAuthcheck (lines 61-64) runs before OAuth validation and successfully authenticates the request without settingreq.user. However, the subsequentuserContextMiddlewareand downstream controllers (e.g.,authController.tsline 180,userController.tsline 23) expectreq.userto be defined, causing runtime errors like "Cannot read property 'username' of undefined".For consistency with the OAuth (lines 78-82) and JWT (line 104) authentication paths, the bearer auth path should also set
req.userwith username and admin status. Additionally, when both bearer auth and OAuth are enabled, bearer auth takes precedence—verify this is intentional, as it may bypass OAuth user context enrichment.src/controllers/serverController.ts (1)
552-916: Refactor: Extract functions from updateSystemConfig.This function is 364 lines long with very high cyclomatic complexity, making it difficult to maintain, test, and understand. The function handles validation, initialization, updates, and persistence all in one place.
Consider extracting helper functions:
Validation helpers:
validateRoutingUpdate(routing)validateSmartRoutingUpdate(smartRouting)validateOAuthServerUpdate(oauthServer)Initialization helpers:
initializeDefaultSystemConfig()ensureDefaultSections(systemConfig)Update handlers:
applyRoutingUpdates(systemConfig, routing)applySmartRoutingUpdates(systemConfig, smartRouting)applyOAuthServerUpdates(systemConfig, oauthServer)Business logic:
shouldSyncEmbeddings(prev, current)Example refactor structure:
export const updateSystemConfig = async (req: Request, res: Response): Promise<void> => { try { const updates = extractUpdates(req.body); if (!hasValidUpdates(updates)) { res.status(400).json({ success: false, message: 'Invalid system configuration' }); return; } const systemConfig = await getOrInitializeSystemConfig(); const needsSync = applyUpdates(systemConfig, updates); await saveSystemConfig(systemConfig); res.json({ success: true, data: systemConfig, message: 'System configuration updated' }); if (needsSync) { syncAllServerToolsEmbeddings().catch(console.error); } } catch (error) { res.status(500).json({ success: false, message: 'Internal server error' }); } };This would improve:
- Testability (each function can be unit tested)
- Readability (clear separation of concerns)
- Maintainability (easier to modify individual sections)
- Reusability (helpers can be used elsewhere)
🧹 Nitpick comments (28)
frontend/src/contexts/ServerContext.tsx (1)
283-314: Consider stronger typing for the return value.The function returns a well-defined shape but the interface declares
Promise<any>at line 30. For better type safety:interface ServerContextType { // ... - handleServerEdit: (server: Server) => Promise<any>; + handleServerEdit: (server: Server) => Promise<{ + name: string; + status: string; + tools: any[]; + config: Record<string, any>; + } | null>; // ... }src/controllers/openApiController.ts (2)
114-122: Tool name resolution and schema-based arg coercion look correctBuilding
fullToolNamewithgetNameSeparator()and preferring the matched tool’snamekeeps this endpoint in sync with whatever naming convention the server actually uses, while still supporting both prefixed and unprefixed tool names. GrabbinginputSchemafrom the matched tool and runningconvertParametersToTypesoverargsis a clean way to normalize query/body values without breaking tools that lack a schema (they get the raw args back).Only minor note: the inline comment on Line 133 still says “without server prefix as it gets added by handleCallToolRequest”, but
toolNamemay now already include a prefix when taken fromtool.name. Consider updating that comment to avoid future confusion; behavior itself looks fine.Also applies to: 126-129, 131-136
211-225: Async group lookup fix is good; also return/await the server fallbackAwaiting
getGroupByIdOrName(name)is the right fix and makes the existence check behave as intended.However, when the group is missing you call
getServerOpenAPISpec(req, res);and thenreturn;without awaiting/returning the Promise. That means any async errors thrown insidegetServerOpenAPISpecwon’t be caught by this try/catch and may surface as unhandled rejections instead of a structured 5xx response.You can fix this by returning the Promise directly:
- const group = await getGroupByIdOrName(name); - if (!group) { - getServerOpenAPISpec(req, res); - return; - } + const group = await getGroupByIdOrName(name); + if (!group) { + return getServerOpenAPISpec(req, res); + }This keeps behavior the same while propagating any errors through this controller’s async flow.
README.md (1)
248-254: Specify languages on new fenced code blocks to satisfy markdownlint (MD040)The
$smart/{group}HTTP and SSE examples use bare triple‑backtick fences without a language. To keep markdownlint happy and improve highlighting, consider marking them, e.g.:```bash # Search only within production servers http://localhost:3000/mcp/$smart/production ...# Search within a specific group http://localhost:3000/sse/$smart/{group} ...Also applies to: 299-305
README.zh.md (1)
177-183: Add languages to fenced code blocks for$smart/{group}examples (MD040)The new 智能路由示例和 SSE 示例使用了不带语言标记的代码块（```）。为通过 markdownlint 并改善高亮，建议加上语言，例如：
```bash # 在所有服务器中搜索 http://localhost:3000/mcp/$smart# 在特定分组中搜索 http://localhost:3000/sse/$smart/{group}Also applies to: 207-213, 259-264
src/middlewares/userContext.ts (2)
47-60: SSE user context cleanup is robust; consider reusing pattern for non‑SSE middlewareThe
cleanup+attachCleanupHandlerspattern for SSE/MCP endpoints (handling bothfinishandclosewith an idempotent guard) is a nice improvement and should prevent context from lingering on long‑lived or abruptly closed connections.You may want to mirror this pattern in
userContextMiddlewareso aborted non‑SSE requests (where onlyclosefires) also guaranteeclearCurrentUser()is called.Also applies to: 72-88
101-148: Clarify production intent ofContextAwareDataServiceImpland avoid console logging of user data
ContextAwareDataServiceImpl.getUserDataFromContextcurrently returns hard‑coded “Admin level/User level data from context” blobs and logsdataType+user.usernametoconsole.log. That’s fine as an example or test shim, but if this is on a production path:
- Replace the stubbed payloads with real data access or move this into a dedicated service.
- Prefer a structured logger over
console.log, and consider whether including usernames in logs fits your PII/logging policy.If it is intended purely as an example/demo, adding a short comment to that effect would help future readers.
docs/zh/configuration/database-configuration.mdx (1)
213-229: Add languages to error snippets and verify docs URL consistency
Several fenced blocks showing error messages (e.g., “connect ECONNREFUSED…”, “password authentication failed…”, migration failures) use bare triple‑backticks. To satisfy markdownlint (MD040) and improve readability, consider marking them as
text:```text Error: connect ECONNREFUSED 127.0.0.1:5432The support section links to
https://mcphub.io/docs, whereas the main README points tohttps://docs.mcphubx.com/. It’s worth double‑checking which URL is canonical and aligning them to avoid confusing users.Also applies to: 245-247, 260-262, 269-276, 326-328
README.fr.md (1)
60-89: DB mode description matches implementation; only minor French/style nitsThe explanation of
DB_URLauto‑enabling DB mode andUSE_DB=falseoverriding it is consistent with the runtime logic (useDatabaseinsrc/index.tsandinitializeDaoFactory). From a behavior standpoint this section is accurate.If you want to polish the French a bit later, you could:
- Clarifier que
USE_DBattend littéralement les valeurstrue/false(chaîne) pour éviter toute ambiguïté.- Légèrement alléger les listes, par ex. « Configuration stockée dans une base de données professionnelle… » → « Configuration stockée dans une base de données robuste… ».
Nothing blocking here; this can ship as‑is.
src/db/entities/User.ts (1)
1-33: Entity structure looks good; consider hiding password by defaultThe entity layout (UUID id, unique username, timestamps) is consistent with the rest of the DB model.
For security, consider preventing the password hash from being selected in ordinary queries so it can’t accidentally leak through generic repository access or debug logs:
- @Column({ type: 'varchar', length: 255 }) + @Column({ type: 'varchar', length: 255, select: false }) password: string;Auth code that needs the hash can explicitly add the column in its queries. This is a low‑effort hardening that often pays off.
src/db/entities/Group.ts (1)
1-36: Group entity is fine; consider uniqueness onnameandserversnullabilityThe entity matches the rest of the DB model and should work as-is. Two small modeling tweaks to consider:
- If group names are expected to be unique (they’re used in path segments like
/mcp/{group}), addingunique: trueonnamewill let the DB enforce that invariant instead of relying solely on app logic.serversissimple-jsonand non-nullable; if you allow empty groups, making it nullable or giving it a default[]can avoid insert quirks whenserversis omitted.Example:
- @Column({ type: 'varchar', length: 255 }) + @Column({ type: 'varchar', length: 255, unique: true }) name: string; - @Column({ type: 'simple-json' }) - servers: Array<string | { name: string; tools?: string[] | 'all' }>; + @Column({ type: 'simple-json', nullable: true }) + servers?: Array<string | { name: string; tools?: string[] | 'all' }>;Only adjust if this matches your intended constraints.
docker-compose.db.yml (2)
1-1: Theversionkey is deprecated in Docker Compose V2.Docker Compose V2 ignores this field. You can safely remove it, though it won't cause issues if left in place.
-version: "3.8" - services:
28-30: Consider adding SSL/TLS for database connections in the documentation.For production deployments, the DB_URL should include
?sslmode=requireor?sslmode=verify-full. The current configuration works for local development but the commented example could guide users toward secure defaults.# Database connection (setting DB_URL automatically enables database mode) DB_URL: "postgresql://mcphub:${DB_PASSWORD:-mcphub_password}@postgres:5432/mcphub" + + # For production with SSL: + # DB_URL: "postgresql://mcphub:${DB_PASSWORD}@postgres:5432/mcphub?sslmode=require"src/middlewares/auth.ts (2)
72-72: Redundant token comparison.The
getToken(accessToken)function already returns the token only when valid and not expired, so the additionaloauthToken.accessToken === accessTokencheck is unnecessary.- if (oauthToken && oauthToken.accessToken === accessToken) { + if (oauthToken) {
74-75: Consider using a static import instead of dynamic import.The dynamic import adds latency to every OAuth-authenticated request. Since
findUserByUsernameis likely always available, a static import at the top of the file would be more efficient.Add to imports at the top:
import { findUserByUsername } from '../models/User.js';Then simplify the usage:
- const { findUserByUsername } = await import('../models/User.js'); - const user = await findUserByUsername(oauthToken.username); + const user = await findUserByUsername(oauthToken.username);docs/configuration/database-configuration.mdx (2)
80-81: Add health check condition todepends_onfor consistency.The example here differs from
docker-compose.db.ymlwhich usescondition: service_healthy. Without the health check, MCPHub may attempt to connect before PostgreSQL is ready.depends_on: - - postgres + postgres: + condition: service_healthyAlso add a healthcheck to the postgres service in this example for completeness.
113-117: Consider least-privilege database permissions.
GRANT ALL PRIVILEGESis convenient for development but overly permissive for production. Consider documenting more restrictive grants.-- More restrictive alternative for production: GRANT CONNECT ON DATABASE mcphub TO mcphub; GRANT USAGE ON SCHEMA public TO mcphub; GRANT SELECT, INSERT, UPDATE, DELETE ON ALL TABLES IN SCHEMA public TO mcphub; GRANT USAGE, SELECT ON ALL SEQUENCES IN SCHEMA public TO mcphub; ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO mcphub;src/db/repositories/index.ts (1)
1-6: Minor inconsistency in import style.
VectorEmbeddingRepositoryuses default import while others use named imports. Consider aligning for consistency, though this is purely stylistic.src/dao/DatabaseDaoFactory.ts (2)
34-67: Simplify by removing non-null assertions.The non-null assertions (
!) after the null checks are safe but unnecessary. Assigning and returning directly within theifblock is cleaner.getUserDao(): UserDao { if (!this.userDao) { this.userDao = new UserDaoDbImpl(); } - return this.userDao!; + return this.userDao; }Apply similarly to other getter methods. Since TypeScript's control flow analysis understands that
this.userDaois definitely assigned after theifblock, the assertion is redundant.
69-78: Consider adding a static reset for the singleton instance in tests.The
resetInstances()method clears cached DAOs but doesn't reset the singleton itself. For complete test isolation, you may also need to reset the staticinstance.public resetInstances(): void { this.userDao = null; this.serverDao = null; this.groupDao = null; this.systemConfigDao = null; this.userConfigDao = null; } + + /** + * Reset singleton instance (for testing only) + */ + public static resetInstance(): void { + DatabaseDaoFactory.instance = undefined as any; + }src/dao/SystemConfigDaoDbImpl.ts (2)
15-55: Consider extracting the entity-to-domain mapping to reduce duplication.The same mapping logic (lines 17-26, 31-40, 45-54) is repeated in
get(),update(), andreset(). This violates DRY and increases maintenance burden.+ private mapToSystemConfig(config: any): SystemConfig { + return { + routing: config.routing, + install: config.install, + smartRouting: config.smartRouting, + mcpRouter: config.mcpRouter, + nameSeparator: config.nameSeparator, + oauth: config.oauth, + oauthServer: config.oauthServer, + enableSessionRebuild: config.enableSessionRebuild, + }; + } + async get(): Promise<SystemConfig> { const config = await this.repository.get(); - return { - routing: config.routing as any, - install: config.install as any, - smartRouting: config.smartRouting as any, - mcpRouter: config.mcpRouter as any, - nameSeparator: config.nameSeparator, - oauth: config.oauth as any, - oauthServer: config.oauthServer as any, - enableSessionRebuild: config.enableSessionRebuild, - }; + return this.mapToSystemConfig(config); }This also allows removing the
as anycasts if the types are properly aligned between the entity and domain model.
57-59: Return type inconsistency with interface contract.The
SystemConfigDaointerface declaresgetSectionto returnPromise<SystemConfig[K] | undefined>, but the repository'sgetSectionreturnsSystemConfig[K](non-nullable per the snippet). The cast toanymasks this, but the DAO should align with its interface contract by handling potentialundefinedvalues explicitly.src/dao/GroupDaoDbImpl.ts (2)
54-69: Passing undefined fields may cause unintended behavior.When
entity.name,entity.description, etc. areundefined, they are explicitly set in the update payload. Depending on howrepository.mergehandlesundefined, this could unintentionally clear existing values. Consider filtering out undefined keys:async update(id: string, entity: Partial<IGroup>): Promise<IGroup | null> { - const group = await this.repository.update(id, { - name: entity.name, - description: entity.description, - servers: entity.servers as any, - owner: entity.owner, - }); + const updateData: Partial<IGroup> = {}; + if (entity.name !== undefined) updateData.name = entity.name; + if (entity.description !== undefined) updateData.description = entity.description; + if (entity.servers !== undefined) updateData.servers = entity.servers as any; + if (entity.owner !== undefined) updateData.owner = entity.owner; + const group = await this.repository.update(id, updateData);
109-124: Direct entity mutation before persisting may cause issues.Line 119 mutates
group.serversdirectly on the fetched entity. While it works here, this pattern can cause subtle bugs with TypeORM's change tracking or if the entity is used elsewhere. Consider building a new array instead:if (!serverExists) { - group.servers.push(serverName); - await this.update(groupId, { servers: group.servers as any }); + const updatedServers = [...group.servers, serverName]; + await this.update(groupId, { servers: updatedServers as any }); }src/db/repositories/SystemConfigRepository.ts (1)
51-57: Non-atomic reset may leave inconsistent state.The
reset()method deletes then callsget()to recreate defaults. A concurrent request between these operations could observe a missing config or trigger duplicate creation attempts. If atomicity is important, wrap in a transaction.src/dao/UserDaoDbImpl.ts (2)
52-59: Consider increasing bcrypt salt rounds.The current implementation uses 10 salt rounds, which is the minimum recommended value. Modern security best practices (OWASP) suggest using 12-14 rounds for better protection against brute-force attacks.
Apply this diff to increase salt rounds to 12:
+const BCRYPT_SALT_ROUNDS = 12; + async createWithHashedPassword( username: string, password: string, isAdmin: boolean, ): Promise<IUser> { - const hashedPassword = await bcrypt.hash(password, 10); + const hashedPassword = await bcrypt.hash(password, BCRYPT_SALT_ROUNDS); return await this.create({ username, password: hashedPassword, isAdmin }); } async updatePassword(username: string, newPassword: string): Promise<boolean> { - const hashedPassword = await bcrypt.hash(newPassword, 10); + const hashedPassword = await bcrypt.hash(newPassword, BCRYPT_SALT_ROUNDS); const result = await this.update(username, { password: hashedPassword }); return result !== null; }Also applies to: 94-98
86-92: Consider mitigating timing attacks in credential validation.The method returns
falseimmediately when a user doesn't exist, but performsbcrypt.compare()when the user exists. This creates a timing difference that could theoretically be exploited to enumerate valid usernames. While bcrypt's inherent slowness (~100ms) makes this less practical, defense-in-depth suggests running a dummy comparison even for non-existent users.Apply this diff to add timing attack mitigation:
async validateCredentials(username: string, password: string): Promise<boolean> { const user = await this.findByUsername(username); if (!user) { + // Run a dummy hash comparison to prevent timing attacks + await bcrypt.compare(password, '$2b$10$AAAAAAAAAAAAAAAAAAAAAA'); return false; } return await bcrypt.compare(password, user.password); }src/db/entities/Server.ts (1)
29-57: Consider database-specific JSON types for better query support.Multiple columns use
simple-jsontype, which usesJSON.stringify/parseand doesn't support querying nested fields. If using PostgreSQL, consider usingjsonbtype for better performance and query capabilities. However, if SQLite compatibility is required,simple-jsonis appropriate.For PostgreSQL deployments, consider:
-@Column({ type: 'simple-json', nullable: true }) +@Column({ type: 'jsonb', nullable: true }) tools?: Record<string, { enabled: boolean; description?: string }>; -@Column({ type: 'simple-json', nullable: true }) +@Column({ type: 'jsonb', nullable: true }) prompts?: Record<string, { enabled: boolean; description?: string }>; -@Column({ type: 'simple-json', nullable: true }) +@Column({ type: 'jsonb', nullable: true }) options?: Record<string, any>;Note: This would require conditional logic based on database type or separate entity definitions per database.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
.env.example(1 hunks)README.fr.md(1 hunks)README.md(6 hunks)README.zh.md(5 hunks)docker-compose.db.yml(1 hunks)docs/configuration/database-configuration.mdx(1 hunks)docs/docs.json(6 hunks)docs/zh/configuration/database-configuration.mdx(1 hunks)frontend/src/contexts/ServerContext.tsx(5 hunks)src/config/DaoConfigService.ts(2 hunks)src/controllers/authController.ts(5 hunks)src/controllers/groupController.ts(23 hunks)src/controllers/openApiController.ts(4 hunks)src/controllers/serverController.ts(15 hunks)src/controllers/userController.ts(13 hunks)src/dao/DaoFactory.ts(1 hunks)src/dao/DatabaseDaoFactory.ts(1 hunks)src/dao/GroupDaoDbImpl.ts(1 hunks)src/dao/ServerDaoDbImpl.ts(1 hunks)src/dao/SystemConfigDaoDbImpl.ts(1 hunks)src/dao/UserConfigDaoDbImpl.ts(1 hunks)src/dao/UserDaoDbImpl.ts(1 hunks)src/dao/index.ts(1 hunks)src/db/entities/Group.ts(1 hunks)src/db/entities/Server.ts(1 hunks)src/db/entities/SystemConfig.ts(1 hunks)src/db/entities/User.ts(1 hunks)src/db/entities/UserConfig.ts(1 hunks)src/db/entities/index.ts(1 hunks)src/db/repositories/GroupRepository.ts(1 hunks)src/db/repositories/ServerRepository.ts(1 hunks)src/db/repositories/SystemConfigRepository.ts(1 hunks)src/db/repositories/UserConfigRepository.ts(1 hunks)src/db/repositories/UserRepository.ts(1 hunks)src/db/repositories/index.ts(1 hunks)src/index.ts(1 hunks)src/middlewares/auth.ts(4 hunks)src/middlewares/userContext.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .env.example
🚧 Files skipped from review as they are similar to previous changes (1)
- src/controllers/authController.ts
🧰 Additional context used
🧬 Code graph analysis (21)
src/db/entities/Group.ts (5)
src/db/entities/Server.ts (1)
Entity(12-64)src/db/entities/SystemConfig.ts (1)
Entity(7-41)src/db/entities/User.ts (1)
Entity(12-31)src/db/entities/UserConfig.ts (1)
Entity(12-31)src/db/entities/index.ts (1)
Group(12-12)
src/db/entities/User.ts (5)
src/db/entities/Group.ts (1)
Entity(12-34)src/db/entities/Server.ts (1)
Entity(12-64)src/db/entities/SystemConfig.ts (1)
Entity(7-41)src/db/entities/UserConfig.ts (1)
Entity(12-31)src/db/entities/index.ts (1)
User(12-12)
src/middlewares/auth.ts (3)
src/services/oauthServerService.ts (1)
isOAuthServerEnabled(331-333)src/models/OAuth.ts (1)
getToken(222-243)src/models/User.ts (1)
findUserByUsername(32-41)
src/config/DaoConfigService.ts (2)
src/dao/DaoFactory.ts (10)
getUserDao(44-49)getUserDao(133-135)getServerDao(51-56)getServerDao(137-139)getGroupDao(58-63)getGroupDao(141-143)getSystemConfigDao(65-70)getSystemConfigDao(145-147)getUserConfigDao(72-77)getUserConfigDao(149-151)src/dao/DatabaseDaoFactory.ts (5)
getUserDao(34-39)getServerDao(41-46)getGroupDao(48-53)getSystemConfigDao(55-60)getUserConfigDao(62-67)
src/dao/SystemConfigDaoDbImpl.ts (3)
src/dao/SystemConfigDao.ts (1)
SystemConfigDao(7-32)src/db/repositories/SystemConfigRepository.ts (1)
SystemConfigRepository(9-76)src/types/index.ts (1)
SystemConfig(152-176)
src/dao/UserConfigDaoDbImpl.ts (4)
src/dao/UserConfigDao.ts (1)
UserConfigDao(7-54)src/db/repositories/UserConfigRepository.ts (1)
UserConfigRepository(8-82)src/db/entities/index.ts (1)
UserConfig(12-12)src/types/index.ts (1)
UserConfig(178-181)
src/controllers/openApiController.ts (3)
src/config/index.ts (1)
getNameSeparator(183-186)src/utils/parameterConversion.ts (1)
convertParametersToTypes(14-93)src/services/groupService.ts (1)
getGroupByIdOrName(28-43)
src/dao/GroupDaoDbImpl.ts (3)
src/dao/GroupDao.ts (1)
GroupDao(9-39)src/db/repositories/GroupRepository.ts (1)
GroupRepository(8-93)src/types/index.ts (1)
IGroup(16-22)
src/dao/DatabaseDaoFactory.ts (10)
src/dao/DaoFactory.ts (1)
DaoFactory(10-16)src/dao/UserDao.ts (1)
UserDao(9-34)src/dao/ServerDao.ts (1)
ServerDao(8-44)src/dao/GroupDao.ts (1)
GroupDao(9-39)src/dao/SystemConfigDao.ts (1)
SystemConfigDao(7-32)src/dao/UserConfigDao.ts (1)
UserConfigDao(7-54)src/dao/UserDaoDbImpl.ts (1)
UserDaoDbImpl(9-108)src/dao/GroupDaoDbImpl.ts (1)
GroupDaoDbImpl(8-154)src/dao/SystemConfigDaoDbImpl.ts (1)
SystemConfigDaoDbImpl(8-68)src/dao/UserConfigDaoDbImpl.ts (1)
UserConfigDaoDbImpl(8-79)
src/controllers/userController.ts (4)
src/dao/DaoFactory.ts (2)
getSystemConfigDao(65-70)getSystemConfigDao(145-147)src/dao/DatabaseDaoFactory.ts (1)
getSystemConfigDao(55-60)src/services/userService.ts (5)
getAllUsers(5-8)getUserByUsername(11-15)getAdminCount(109-113)deleteUser(75-93)getUserCount(103-106)src/utils/passwordValidation.ts (1)
validatePasswordStrength(15-42)
src/db/entities/Server.ts (5)
src/db/entities/Group.ts (1)
Entity(12-34)src/db/entities/SystemConfig.ts (1)
Entity(7-41)src/db/entities/User.ts (1)
Entity(12-31)src/db/entities/UserConfig.ts (1)
Entity(12-31)src/db/entities/index.ts (1)
Server(12-12)
src/db/repositories/GroupRepository.ts (5)
src/db/repositories/index.ts (1)
GroupRepository(13-13)src/db/entities/index.ts (1)
Group(12-12)src/db/connection.ts (1)
getAppDataSource(67-69)src/db/repositories/ServerRepository.ts (1)
count(68-70)src/db/repositories/UserRepository.ts (1)
count(68-70)
src/middlewares/userContext.ts (1)
src/utils/oauthBearer.ts (1)
resolveOAuthUserFromAuthHeader(31-44)
src/index.ts (1)
src/utils/migration.ts (1)
initializeDatabaseMode(143-175)
src/db/entities/SystemConfig.ts (1)
src/types/index.ts (1)
SystemConfig(152-176)
src/dao/ServerDaoDbImpl.ts (2)
src/dao/ServerDao.ts (2)
ServerDao(8-44)ServerConfigWithName(49-51)src/db/repositories/ServerRepository.ts (1)
ServerRepository(8-92)
src/db/repositories/UserConfigRepository.ts (4)
src/db/repositories/index.ts (1)
UserConfigRepository(15-15)src/db/entities/index.ts (1)
UserConfig(12-12)src/types/index.ts (1)
UserConfig(178-181)src/db/connection.ts (1)
getAppDataSource(67-69)
src/controllers/groupController.ts (1)
src/services/groupService.ts (11)
getAllGroups(20-25)getGroupByIdOrName(28-43)createGroup(46-84)updateGroup(87-124)updateGroupServers(128-160)deleteGroup(163-171)addServerToGroup(174-213)removeServerFromGroup(216-236)getServerConfigsInGroup(258-262)getServerConfigInGroup(247-255)updateServerToolsInGroup(265-306)
src/dao/UserDaoDbImpl.ts (3)
src/dao/UserDao.ts (1)
UserDao(9-34)src/db/repositories/UserRepository.ts (1)
UserRepository(8-78)src/types/index.ts (1)
IUser(9-13)
src/db/repositories/UserRepository.ts (8)
src/db/repositories/index.ts (1)
UserRepository(11-11)src/db/entities/index.ts (1)
User(12-12)src/db/connection.ts (1)
getAppDataSource(67-69)src/dao/GroupDaoDbImpl.ts (1)
count(79-81)src/dao/ServerDaoDbImpl.ts (1)
count(71-73)src/dao/UserDaoDbImpl.ts (1)
count(82-84)src/db/repositories/GroupRepository.ts (1)
count(83-85)src/db/repositories/ServerRepository.ts (1)
count(68-70)
src/db/entities/index.ts (1)
src/types/index.ts (2)
SystemConfig(152-176)UserConfig(178-181)
🪛 LanguageTool
docs/configuration/database-configuration.mdx
[style] ~196-~196: ‘exactly the same’ might be wordy. Consider a shorter alternative.
Context: ...system/config` The web dashboard works exactly the same way, but now stores changes in the data...
(EN_WORDINESS_PREMIUM_EXACTLY_THE_SAME)
README.zh.md
[uncategorized] ~205-~205: 您的意思是“由"于"”吗？
Context: ...PHub 设置中启用智能路由 分组限定的智能路由： 您可以将智能路由与分组筛选结合使用，仅在特定服务器分组内搜索： ``` # 仅在生产服务器中搜索...
(YU7_YU8)
README.fr.md
[typographical] ~62-~62: Caractère d’apostrophe incorrect.
Context: ...PostgreSQL comme alternative au fichier mcp_settings.json. Le mode base de données offre une pers...
(APOS_INCORRECT)
[style] ~62-~62: Un verbe peut dynamiser votre phrase.
Context: ...ance et une évolutivité améliorées pour les environnements de production et les déploiements d'entrep...
(PROP_NOMINALES_EN_VERBALES)
[grammar] ~64-~64: Il y a peut-être une erreur ici
Context: ...déploiements d'entreprise. Avantages principaux : - ✅ Meilleure persistance - Configurat...
(QB_NEW_FR)
[grammar] ~66-~66: Envisagez un remplacement
Context: ...paux :** - ✅ Meilleure persistance - Configuration stockée dans une base de données profes...
(QB_NEW_FR_OTHER_ERROR_IDS_REPLACEMENT_PUNCTUATION_DASH_–)
[grammar] ~67-~67: Envisagez un remplacement
Context: ...des données - ✅ Haute disponibilité - Profitez des capacités de réplication et de basc...
(QB_NEW_FR_OTHER_ERROR_IDS_REPLACEMENT_PUNCTUATION_DASH_–)
[grammar] ~68-~68: Envisagez un remplacement
Context: ... données - ✅ Prêt pour l'entreprise - Répond aux exigences de gestion des données et...
(QB_NEW_FR_OTHER_ERROR_IDS_REPLACEMENT_PUNCTUATION_DASH_–)
[grammar] ~69-~69: Envisagez un remplacement
Context: ...rise - ✅ Sauvegarde et récupération - Utilisez des outils et stratégies de sauvegarde ...
(QB_NEW_FR_OTHER_ERROR_IDS_REPLACEMENT_PUNCTUATION_DASH_–)
[style] ~69-~69: Cette structure peut être modifiée afin de devenir plus percutante.
Context: ... des outils et stratégies de sauvegarde de base de données matures **Variables d'envir...
(DE_BASE3)
[grammar] ~71-~71: Il y a peut-être une erreur ici
Context: ...de base de données matures Variables d'environnement : bash # Définissez simplement DB_URL pour activer automatiquement le mode base de données DB_URL=postgresql://user:password@host:5432/mcphub # Ou contrôlez explicitement avec USE_DB (optionnel, remplace la détection automatique) # USE_DB=true > Note : Vous n'avez qu'à définir `DB_UR...
(QB_NEW_FR)
[grammar] ~83-~83: Il y a peut-être une erreur ici
Context: ...ion/database-configuration.mdx) complet pour : - Instructions de configuration détaillées...
(QB_NEW_FR)
[style] ~86-~86: Utilisé hors du contexte militaire, on préfèrera d’autres tournures à « baser sur ».
Context: ...ées - Migration depuis la configuration basée sur fichiers - Procédures de sauvegarde et ...
(BASER_SUR)
🪛 markdownlint-cli2 (0.18.1)
README.zh.md
207-207: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
README.md
248-248: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
| /** | ||
| * Switch to database-backed DAOs based on environment variable | ||
| * This is synchronous and should be called during app initialization | ||
| */ | ||
| export function initializeDaoFactory(): void { | ||
| // If USE_DB is explicitly set, use its value; otherwise, auto-detect based on DB_URL presence | ||
| const useDatabase = | ||
| process.env.USE_DB !== undefined ? process.env.USE_DB === 'true' : !!process.env.DB_URL; | ||
| if (useDatabase) { | ||
| console.log('Using database-backed DAO implementations'); | ||
| // Dynamic import to avoid circular dependencies | ||
| // eslint-disable-next-line @typescript-eslint/no-var-requires | ||
| const DatabaseDaoFactoryModule = require('./DatabaseDaoFactory.js'); | ||
| setDaoFactory(DatabaseDaoFactoryModule.DatabaseDaoFactory.getInstance()); | ||
| } else { | ||
| console.log('Using file-based DAO implementations'); | ||
| setDaoFactory(JsonFileDaoFactory.getInstance()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check package.json to understand module system configuration
cat package.json | head -30Repository: XInfty/ct-mcphub
Length of output: 844
🏁 Script executed:
#!/bin/bash
# View the full DaoFactory.ts file to understand context and imports
wc -l src/dao/DaoFactory.tsRepository: XInfty/ct-mcphub
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Check src/index.ts for duplicated logic
fd index.ts --type f src/Repository: XInfty/ct-mcphub
Length of output: 270
🏁 Script executed:
#!/bin/bash
# Check tsconfig.json to understand compilation target
cat tsconfig.jsonRepository: XInfty/ct-mcphub
Length of output: 623
🏁 Script executed:
#!/bin/bash
# Read the full DaoFactory.ts file to understand the context and usage pattern
cat -n src/dao/DaoFactory.tsRepository: XInfty/ct-mcphub
Length of output: 4156
🏁 Script executed:
#!/bin/bash
# Check src/index.ts to see if there's duplicated useDatabase logic
cat -n src/index.tsRepository: XInfty/ct-mcphub
Length of output: 525
🏁 Script executed:
#!/bin/bash
# Search for initializeDaoFactory function
rg -n "initializeDaoFactory" src/Repository: XInfty/ct-mcphub
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for useDatabase to find the logic mentioned in the review
rg -n "useDatabase" src/Repository: XInfty/ct-mcphub
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check the server.js file (imported in index.ts) to see if logic is there
fd server.ts --type f src/Repository: XInfty/ct-mcphub
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Check git status and branch
git statusRepository: XInfty/ct-mcphub
Length of output: 295
🏁 Script executed:
#!/bin/bash
# Check recent commits to understand the state
git log --oneline -10Repository: XInfty/ct-mcphub
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Look at lines 110-131 of DaoFactory.ts specifically (the end of the file)
sed -n '105,131p' src/dao/DaoFactory.ts | cat -nRepository: XInfty/ct-mcphub
Length of output: 836
🏁 Script executed:
#!/bin/bash
# Check if there are any git diffs showing pending changes
git diff --stat HEADRepository: XInfty/ct-mcphub
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for DatabaseDaoFactory to see if it exists
rg -n "DatabaseDaoFactory" src/Repository: XInfty/ct-mcphub
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for process.env.USE_DB or DB_URL to find initialization logic
rg -n "process\.env\.(USE_DB|DB_URL)" src/Repository: XInfty/ct-mcphub
Length of output: 184
🏁 Script executed:
#!/bin/bash
# Check server.ts for initialization logic
cat -n src/server.ts | head -50Repository: XInfty/ct-mcphub
Length of output: 2087
🏁 Script executed:
#!/bin/bash
# Check the full git log for DaoFactory.ts to see if it was recently changed
git log --oneline src/dao/DaoFactory.ts | head -10Repository: XInfty/ct-mcphub
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Check diff of DaoFactory.ts in the last commit
git show HEAD:src/dao/DaoFactory.ts | wc -lRepository: XInfty/ct-mcphub
Length of output: 63
🏁 Script executed:
#!/bin/bash
# Search for any commented code or TODOs related to require or DatabaseDaoFactory
rg -n "(TODO|FIXME|require|DatabaseDaoFactory)" src/dao/DaoFactory.tsRepository: XInfty/ct-mcphub
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if there are other branches or pending changes
git branch -aRepository: XInfty/ct-mcphub
Length of output: 196
🏁 Script executed:
#!/bin/bash
# Check the coderabbit_1 branch to see proposed changes
git show coderabbit_1:src/dao/DaoFactory.ts 2>/dev/null | tail -40Repository: XInfty/ct-mcphub
Length of output: 1388
🏁 Script executed:
#!/bin/bash
# Check if there's a diff between HEAD and coderabbit_1
git diff HEAD coderabbit_1 -- src/dao/DaoFactory.ts | head -100Repository: XInfty/ct-mcphub
Length of output: 1258
🏁 Script executed:
#!/bin/bash
# Look at the context around lines 110-128 in coderabbit_1
git show coderabbit_1:src/dao/DaoFactory.ts 2>/dev/null | sed -n '105,135p' | cat -nRepository: XInfty/ct-mcphub
Length of output: 1368
🏁 Script executed:
#!/bin/bash
# Check if DatabaseDaoFactory.ts exists
fd DatabaseDaoFactory.ts src/Repository: XInfty/ct-mcphub
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for any other DB_URL or USE_DB usage to check for duplication
rg -n "DB_URL|USE_DB" src/ --type tsRepository: XInfty/ct-mcphub
Length of output: 184
🏁 Script executed:
#!/bin/bash
# Check if initializeDaoFactory is called anywhere in the PR branch
git show coderabbit_1:src/index.ts 2>/dev/null | head -30Repository: XInfty/ct-mcphub
Length of output: 958
🏁 Script executed:
#!/bin/bash
# Verify the tsconfig module compilation output by checking what the dist output would be
# Check if there are build artifacts or scripts that show compiled output format
cat tsconfig.json | rg "module|target"Repository: XInfty/ct-mcphub
Length of output: 199
🏁 Script executed:
#!/bin/bash
# Search more broadly for any DatabaseDaoFactory file or reference
rg -i "databasedao" src/ --type tsRepository: XInfty/ct-mcphub
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if there's any TypeScript/ESM import() usage in the codebase as a pattern example
rg -n "import(" src/ --type ts | head -5Repository: XInfty/ct-mcphub
Length of output: 124
Bare require will fail at runtime in ESM mode, and DB-mode detection logic is duplicated
Two critical issues in the proposed initializeDaoFactory function:
-
Bare
requireincompatible with NodeNext module systemWith
tsconfig.jsonconfigured as"module": "NodeNext", TypeScript compiles to ESM (import/export statements). A barerequireis undefined at runtime and will throwReferenceError: require is not definedwhenuseDatabaseis true. Additionally,DatabaseDaoFactory.tsdoes not exist in the codebase, so the require target cannot be resolved.Fix: Use dynamic
import()and makeinitializeDaoFactoryasync:export async function initializeDaoFactory(): Promise<void> { // ... existing logic ... if (useDatabase) { const { DatabaseDaoFactory } = await import('./DatabaseDaoFactory.js'); setDaoFactory(DatabaseDaoFactory.getInstance()); } }
-
Duplicated DB-mode detection logic
The exact same
useDatabaseexpression appears in bothsrc/index.ts(lines 10-13) and the proposed code insrc/dao/DaoFactory.ts(lines 12-13). This duplication creates maintenance risk—any future changes to the logic must be applied in two places or behavior will drift between initialization points.Fix: Extract the logic into a shared utility function (e.g.,
src/utils/databaseMode.ts):export function shouldUseDatabase(): boolean { return process.env.USE_DB !== undefined ? process.env.USE_DB === 'true' : !!process.env.DB_URL; }
🤖 Prompt for AI Agents
In src/dao/DaoFactory.ts around lines 110 to 128, the current
initializeDaoFactory uses a bare require (which fails under NodeNext/ESM) and
duplicates the DB-mode detection logic from src/index.ts; change
initializeDaoFactory to be async and use dynamic import() with the correct
runtime module path (ensure you import the actual DatabaseDaoFactory module that
exists, using the .js runtime extension), and move the USE_DB/DB_URL detection
into a shared utility (e.g., src/utils/databaseMode.ts with a
shouldUseDatabase() function) so both src/index.ts and DaoFactory call that
helper instead of duplicating the expression; update all call sites to await the
async initializeDaoFactory and to import the helper.
| async findByServer(serverName: string): Promise<IGroup[]> { | ||
| const allGroups = await this.repository.findAll(); | ||
| return allGroups | ||
| .filter((g) => | ||
| g.servers.some((s) => (typeof s === 'string' ? s === serverName : s.name === serverName)), | ||
| ) | ||
| .map((g) => ({ | ||
| id: g.id, | ||
| name: g.name, | ||
| description: g.description, | ||
| servers: g.servers as any, | ||
| owner: g.owner, | ||
| })); | ||
| } |
There was a problem hiding this comment.
Inefficient in-memory filtering; consider a database-level query.
findByServer loads all groups into memory and filters in JavaScript. This doesn't scale well as the number of groups grows. Consider adding a repository method that queries the database directly (e.g., using JSON operators if supported by your DB) or indexing server associations.
| async create(entity: Omit<IUser, 'id'>): Promise<IUser> { | ||
| const user = await this.repository.create({ | ||
| username: entity.username, | ||
| password: entity.password, | ||
| isAdmin: entity.isAdmin || false, | ||
| }); | ||
| return { | ||
| username: user.username, | ||
| password: user.password, | ||
| isAdmin: user.isAdmin, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Risk of storing unhashed passwords via create().
The create() method accepts a password field directly without hashing it. If a caller mistakenly uses create() instead of createWithHashedPassword(), plain-text passwords could be stored in the database.
Consider one of these solutions:
- Make
create()private and force all creation throughcreateWithHashedPassword() - Add password hashing logic to
create()as well - Add explicit validation/warning if password doesn't look hashed
Apply this diff to add basic protection:
async create(entity: Omit<IUser, 'id'>): Promise<IUser> {
+ // Warn if password doesn't look hashed (bcrypt hashes start with $2)
+ if (entity.password && !entity.password.startsWith('$2')) {
+ console.warn('Warning: Possibly unhashed password passed to UserDaoDbImpl.create()');
+ }
const user = await this.repository.create({
username: entity.username,
password: entity.password,
isAdmin: entity.isAdmin || false,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async create(entity: Omit<IUser, 'id'>): Promise<IUser> { | |
| const user = await this.repository.create({ | |
| username: entity.username, | |
| password: entity.password, | |
| isAdmin: entity.isAdmin || false, | |
| }); | |
| return { | |
| username: user.username, | |
| password: user.password, | |
| isAdmin: user.isAdmin, | |
| }; | |
| } | |
| async create(entity: Omit<IUser, 'id'>): Promise<IUser> { | |
| // Warn if password doesn't look hashed (bcrypt hashes start with $2) | |
| if (entity.password && !entity.password.startsWith('$2')) { | |
| console.warn('Warning: Possibly unhashed password passed to UserDaoDbImpl.create()'); | |
| } | |
| const user = await this.repository.create({ | |
| username: entity.username, | |
| password: entity.password, | |
| isAdmin: entity.isAdmin || false, | |
| }); | |
| return { | |
| username: user.username, | |
| password: user.password, | |
| isAdmin: user.isAdmin, | |
| }; | |
| } |
| async get(): Promise<SystemConfig> { | ||
| let config = await this.repository.findOne({ where: { id: this.DEFAULT_ID } }); | ||
|
|
||
| // Create default if doesn't exist | ||
| if (!config) { | ||
| config = this.repository.create({ | ||
| id: this.DEFAULT_ID, | ||
| routing: {}, | ||
| install: {}, | ||
| smartRouting: {}, | ||
| mcpRouter: {}, | ||
| nameSeparator: '-', | ||
| oauth: {}, | ||
| oauthServer: {}, | ||
| enableSessionRebuild: false, | ||
| }); | ||
| config = await this.repository.save(config); | ||
| } | ||
|
|
||
| return config; | ||
| } |
There was a problem hiding this comment.
Potential race condition on first access.
If multiple concurrent requests call get() before the default config exists, the check-then-create pattern (lines 21-37) could result in duplicate insert attempts or constraint violations if id has a unique constraint. Consider using upsert or a transaction with locking for safer initialization.
async get(): Promise<SystemConfig> {
- let config = await this.repository.findOne({ where: { id: this.DEFAULT_ID } });
-
- // Create default if doesn't exist
- if (!config) {
- config = this.repository.create({
- id: this.DEFAULT_ID,
- routing: {},
- install: {},
- smartRouting: {},
- mcpRouter: {},
- nameSeparator: '-',
- oauth: {},
- oauthServer: {},
- enableSessionRebuild: false,
- });
- config = await this.repository.save(config);
- }
-
- return config;
+ let config = await this.repository.findOne({ where: { id: this.DEFAULT_ID } });
+ if (config) return config;
+
+ // Use upsert to handle race conditions
+ await this.repository.upsert({
+ id: this.DEFAULT_ID,
+ routing: {},
+ install: {},
+ smartRouting: {},
+ mcpRouter: {},
+ nameSeparator: '-',
+ oauth: {},
+ oauthServer: {},
+ enableSessionRebuild: false,
+ }, ['id']);
+
+ return (await this.repository.findOne({ where: { id: this.DEFAULT_ID } }))!;
}🤖 Prompt for AI Agents
In src/db/repositories/SystemConfigRepository.ts around lines 20 to 40, the
current check-then-create flow can race when multiple callers hit get() before
the default row exists; replace it with an atomic upsert or perform the create
inside a transaction with a row-level lock to avoid duplicate inserts: either
use the ORM's upsert/insert...onConflict behavior (repository.upsert or query
builder insert with onConflict to do nothing and then select) so the call
becomes idempotent, or open a transaction (QueryRunner), SELECT the row FOR
UPDATE, create and save only if still missing, then commit; ensure the method
returns the existing or newly created config in all cases.
| async update(username: string, configData: Partial<UserConfig>): Promise<UserConfig> { | ||
| let config = await this.get(username); | ||
|
|
||
| if (!config) { | ||
| // Create new config if doesn't exist | ||
| config = this.repository.create({ | ||
| username, | ||
| routing: {}, | ||
| additionalConfig: {}, | ||
| ...configData, | ||
| }); | ||
| } else { | ||
| // Merge with existing config | ||
| config = this.repository.merge(config, configData); | ||
| } | ||
|
|
||
| return await this.repository.save(config); | ||
| } |
There was a problem hiding this comment.
Race condition on concurrent config creation.
Similar to SystemConfigRepository, the check-then-create pattern in update() can cause duplicate entries or constraint violations if two requests simultaneously try to create a config for the same new user. Consider using upsert for atomicity.
🤖 Prompt for AI Agents
In src/db/repositories/UserConfigRepository.ts around lines 37 to 54, the
current check-then-create pattern can race when two requests create the same
user config; replace it with an atomic upsert: call the repository's upsert (or
use QueryBuilder INSERT ... ON CONFLICT) keyed on username, passing username
plus merged configData, then reload and return the saved entity; ensure the
upsert updates the routing/additionalConfig fields (merge existing and incoming
keys if needed) and use a follow-up find/get to return the persisted UserConfig.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
locales/fr.json (1)
734-734: Fix corrupted French text with mixed Chinese character.Line 734 contains "和" (Chinese character meaning "and") mixed into French text. Should read "et" in French for consistency.
- "groupIdAndServerNameRequired": "L'ID de groupe和le nom du serveur sont requis", + "groupIdAndServerNameRequired": "L'ID de groupe et le nom du serveur sont requis",
🧹 Nitpick comments (10)
src/db/entities/Server.ts (2)
29-60: Consider using native JSON columns for PostgreSQL.The
simple-jsontype usesJSON.stringify/JSON.parsewhich can be less efficient than nativejsonorjsonbtypes in PostgreSQL. If targeting PostgreSQL, consider using the native types for better performance and query capabilities.Example for PostgreSQL:
- @Column({ type: 'simple-json', nullable: true }) + @Column({ type: 'jsonb', nullable: true }) args?: string[]; - @Column({ type: 'simple-json', nullable: true }) + @Column({ type: 'jsonb', nullable: true }) env?: Record<string, string>; - @Column({ type: 'simple-json', nullable: true }) + @Column({ type: 'jsonb', nullable: true }) headers?: Record<string, string>; - @Column({ type: 'simple-json', nullable: true }) + @Column({ type: 'jsonb', nullable: true }) tools?: Record<string, { enabled: boolean; description?: string }>; - @Column({ type: 'simple-json', nullable: true }) + @Column({ type: 'jsonb', nullable: true }) prompts?: Record<string, { enabled: boolean; description?: string }>; - @Column({ type: 'simple-json', nullable: true }) + @Column({ type: 'jsonb', nullable: true }) options?: Record<string, any>; - @Column({ type: 'simple-json', nullable: true }) + @Column({ type: 'jsonb', nullable: true }) oauth?: Record<string, any>;Note: If supporting multiple databases,
simple-jsonprovides better compatibility.
17-42: Consider adding indexes for frequently queried fields.Fields like
owner,enabled, andtypeare likely to be queried frequently. Adding indexes would improve query performance.Add index decorators:
import { Entity, Column, PrimaryGeneratedColumn, CreateDateColumn, UpdateDateColumn, Index, } from 'typeorm';Then add indexes to relevant columns:
+ @Index() @Column({ type: 'varchar', length: 50, nullable: true }) type?: string; + @Index() @Column({ type: 'boolean', default: true }) enabled: boolean; + @Index() @Column({ type: 'varchar', length: 255, nullable: true }) owner?: string;frontend/src/components/ServerForm.tsx (5)
5-11: Consider typing the payload parameter.The
onSubmitcallback acceptsanyas the payload type. For better type safety, consider defining a specific payload interface that matches the structure being constructed inhandleSubmit.
75-76: Redundant env var initialization.The
formData.envis initialized viagetInitialServerEnvVars(initialData)at line 75, but thenenvVarsstate is initialized separately with the same logic at lines 145-148. TheformData.envfield appears unused since the form usesenvVarsstate directly.Consider removing
envfromformDatainitialization or usingformData.envconsistently:env: getInitialServerEnvVars(initialData), + headers: initialData && initialData.config && initialData.config.headers + ? Object.entries(initialData.config.headers).map(([key, value]) => ({ key, value })) + : [], - headers: [],Then remove the separate
envVarsandheaderVarsstates, usingformData.envandformData.headersinstead.Also applies to: 145-149
607-616: Consider extracting a helper for OpenAPI field updates.The pattern
url: prev.openapi?.url || ''is repeated in every OpenAPI field onChange handler (~15 times). Consider extracting a helper function similar tohandleOAuthChange:+ const handleOpenAPIChange = <K extends keyof NonNullable<ServerFormData['openapi']>>( + field: K, + value: NonNullable<ServerFormData['openapi']>[K], + ) => { + setFormData((prev) => ({ + ...prev, + openapi: { + ...prev.openapi, + [field]: value, + }, + })); + };Then use it in handlers:
onChange={(e) => handleOpenAPIChange('securityType', e.target.value as any)}
1024-1101: Large commented-out code block.This section contains ~80 lines of commented-out OAuth form fields. If these are planned for future implementation, consider:
- Removing for now and tracking in an issue
- Or uncommenting if they're ready for use
The
handleSubmitlogic already handles these fields, so they would work if uncommented.Would you like me to open an issue to track the completion of these OAuth form fields?
1-1373: Component is functional but consider modularization for maintainability.At ~1370 lines, this component handles significant complexity. The current structure works, but for long-term maintainability, consider extracting:
OpenAPIConfigSectioncomponentOAuthConfigSectioncomponentKeepAliveConfigSectioncomponentThis is optional and can be deferred.
src/dao/ServerDaoDbImpl.ts (3)
1-12: Instantiation approach is straightforward; consider DI later for testabilityDirectly constructing
ServerRepositoryin the constructor is fine for now and keeps this implementation simple. If you find yourself needing more flexible testing or multiple data sources, you might later swap this to accept the repository (or a factory) via dependency injection, but that’s optional at this stage.
90-93: Consider a repository‑levelfindByTypeto avoid full‑table scans
findByTypecurrently does:const allServers = await this.repository.findAll(); return allServers.filter((s) => s.type === type).map((s) => this.mapToServerConfig(s));This is fine for small datasets, but it pulls the entire table into memory and relies on JS filtering, which won’t benefit from a DB index on
type.If/when
Servercounts grow, consider adding afindByTypemethod onServerRepositorythat performsWHERE type = :typein the DB and then calling that here instead:// in ServerRepository async findByType(type: string): Promise<Server[]> { return this.repository.find({ where: { type } }); } // here async findByType(type: string): Promise<ServerConfigWithName[]> { const servers = await this.repository.findByType(type); return servers.map((s) => this.mapToServerConfig(s)); }
116-150: Type mapping is clear; add a small guard aroundtypeif you want stronger safetyThe mapper cleanly isolates the DB/entity shape from
ServerConfigWithName. Two minor points you might consider:
- Runtime guard for
typeinstead of a blind assertionRight now:
type: server.type as 'stdio' | 'sse' | 'streamable-http' | 'openapi' | undefined,If the DB ever contains an unexpected string (e.g. a future enum value or bad migration), the type system will happily accept it even though it’s out of the declared union.
You can cheaply validate and coerce invalid values to
undefined:- return { - name: server.name, - type: server.type as 'stdio' | 'sse' | 'streamable-http' | 'openapi' | undefined, + const allowedTypes = ['stdio', 'sse', 'streamable-http', 'openapi'] as const; + const type = + server.type && allowedTypes.includes(server.type as (typeof allowedTypes)[number]) + ? (server.type as (typeof allowedTypes)[number]) + : undefined; + + return { + name: server.name, + type, url: server.url, // ...
- Optionally tighten types for
options/oauth
optionsandoauthare currentlyRecord<string, any>, which is fine but loses the stronger typing you likely have onServerConfig. If you want the compiler to help more here, you could import and use the precise types fromServerConfiginstead ofany, but that’s a nice‑to‑have rather than a blocker.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/src/components/ServerForm.tsx(23 hunks)frontend/src/types/index.ts(5 hunks)locales/en.json(14 hunks)locales/fr.json(14 hunks)locales/tr.json(1 hunks)locales/zh.json(14 hunks)src/dao/ServerDaoDbImpl.ts(1 hunks)src/db/entities/Server.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/components/ServerForm.tsx (1)
frontend/src/types/index.ts (3)
Server(206-218)EnvVar(235-238)ServerFormData(241-296)
src/db/entities/Server.ts (6)
src/db/entities/Group.ts (1)
Entity(12-34)src/db/entities/SystemConfig.ts (1)
Entity(7-41)src/db/entities/User.ts (1)
Entity(12-31)src/db/entities/UserConfig.ts (1)
Entity(12-31)frontend/src/types/index.ts (1)
Server(206-218)src/db/entities/index.ts (1)
Server(12-12)
src/dao/ServerDaoDbImpl.ts (2)
src/dao/ServerDao.ts (2)
ServerDao(8-44)ServerConfigWithName(49-51)src/db/repositories/ServerRepository.ts (1)
ServerRepository(8-92)
🪛 Checkov (3.2.334)
locales/tr.json
[medium] 567-568: Basic Auth Credentials
(CKV_SECRET_4)
🔇 Additional comments (21)
locales/tr.json (1)
1-795: Turkish localization complete and well-formed.The Turkish translations comprehensively cover all UI namespaces including OAuth, registry, and session management features. Structure and key coverage align with other locale files.
locales/en.json (1)
1-795: English localization comprehensive and well-structured.Additions cover OAuth authorization flows, dynamic client registration, JSON import, and registry features with clear user-facing text. All new keys properly integrated with existing namespaces.
locales/fr.json (1)
1-795: French localization structure sound; all sections present except for the line 734 defect.Additions comprehensively cover OAuth, JSON import, and registry features with professional French translations. Fix the corrupted character on line 734 and this file will be production-ready.
locales/zh.json (1)
1-796: Chinese localization comprehensive and well-executed.All new OAuth server, registry, and session management features properly translated with professional Chinese text. Structure aligns with other locale files and key coverage is complete.
src/db/entities/Server.ts (3)
1-7: LGTM!Standard TypeORM imports are correctly specified.
69-69: LGTM!Default export follows the pattern used in other entity files.
41-42: File does not exist in repository.The review comment references
src/db/entities/Server.ts, but this file does not exist in the repository. Thesrc/db/entities/directory only containsVectorEmbedding.tsandindex.ts. The review cannot be verified against a non-existent file.Likely an incorrect or invalid review comment.
frontend/src/types/index.ts (5)
1-2: LGTM!The addition of
'oauth_required'to theServerStatusunion type is appropriate for indicating servers that require OAuth authorization flow.
117-162: LGTM!The OAuth configuration structure is comprehensive and well-organized, supporting:
- Client credentials (clientId, clientSecret)
- Token management (accessToken, refreshToken, scopes)
- RFC 7591 dynamic registration with metadata
- PKCE authorization flow (pendingAuthorization with codeVerifier, state)
- RFC 8707 resource parameter
214-217: LGTM!The
Server.oauthfield appropriately captures runtime OAuth state (authorization URL and CSRF state) separate from the full configuration inServerConfig.oauth.
255-268: LGTM!The
keepAliveandoauthstructures inServerFormDataare appropriately designed for form handling. Thescopesfield being a string (vs. array inServerConfig) is correct since it's parsed into an array during form submission.
371-514: LGTM!The Registry types are well-structured and comprehensive, supporting:
- Variable configuration with choices, defaults, and secret handling
- Package definitions with environment variables, arguments, and transports
- Server metadata with icons, repository info, and official status
- Cursor-based pagination via
RegistryMetadata.nextCursorfrontend/src/components/ServerForm.tsx (4)
225-236: LGTM!The
handleOptionsChangefunction correctly handles the three option fields with appropriate typing and immutable state updates.
374-384: Verify: OAuth config excluded from stdio servers.The
oauthConfigis included in the payload for SSE/streamable-http server types (line 378), but not for stdio servers. Ensure this is intentional, as the PR mentions "OAuth authentication for upstream MCP servers" which could potentially include stdio servers that spawn processes connecting to OAuth-protected endpoints.
317-326: LGTM!The OpenAPI schema parsing is properly wrapped in try/catch with a descriptive error message. The conditional URL/schema handling based on input mode is correct.
1273-1351: LGTM!The Keep-Alive section is well-implemented:
- Properly gated to SSE/streamable-http server types only
- Sensible interval constraints (5s-5min)
- Good use of i18n fallbacks for new translation keys
- Consistent UI pattern with other collapsible sections
src/dao/ServerDaoDbImpl.ts (5)
14-22: Read methods (findAll/findById) look goodThe
findAll/findByIdwrappers are thin and consistent, and centralizing the entity→DTO conversion throughmapToServerConfigis a good pattern to keep the rest of the codebase insulated from the DB entity shape.
24-43: Create implementation andenableddefaulting are reasonableMapping from
ServerConfigWithNameinto the repository payload is clear, and defaultingenabledtotruewhen omitted is explicit and easy to reason about. Assuming that aligns with the existing in‑memory DAO semantics, this looks good.If you haven’t already, it’s worth double‑checking that other
ServerDaoimplementations also defaultenabledthe same way so behavior stays consistent across storage backends.
68-88: Wrapper methods for delete/exists/count/owner/enabled are fineThese thin wrappers over
ServerRepository(delete,exists,count,findByOwner,findEnabled) are straightforward and consistently reusemapToServerConfigwhere needed. No issues here.
95-114: Boolean helpers (setEnabled/updateTools/updatePrompts) are consistentThe boolean return contracts based on
server !== null/result !== nullare clear and match the idea of “did we find and update the record?”. Once the coreupdatemethod’s partial‑update semantics are made robust, these helpers will inherit the improved behavior without further changes.
45-66: File path and code pattern do not exist in the repositoryThe review references
src/dao/ServerDaoDbImpl.ts(lines 45-66), but this file does not exist. The actual file issrc/dao/ServerDao.ts. More importantly, the problematic code pattern shown in the review—anupdate()method that constructs a full object literal with enumerated fields liketype: entity.type, url: entity.url, ...—does not exist in the codebase.The actual
update()method (lines 133–151) safely handles partial updates:async update( name: string, updates: Partial<ServerConfigWithName>, ): Promise<ServerConfigWithName | null> { const servers = await this.getAll(); const index = servers.findIndex((server) => server.name === name); if (index === -1) { return null; } // Don't allow name changes const { name: _, ...allowedUpdates } = updates; const updatedServer = this.updateEntity(servers[index], allowedUpdates); servers[index] = updatedServer; await this.saveAll(servers); return updatedServer; }The implementation uses destructuring to filter out the name field and then merges via the spread operator in
updateEntity(). Additionally, this is a JSON file-based DAO, not a TypeORM database implementation, so TypeORM'smerge()behavior is not relevant.
| @Column({ type: 'varchar', length: 500, nullable: true }) | ||
| command?: string; |
There was a problem hiding this comment.
Consider increasing the command length limit.
A 500-character limit for command may be insufficient for absolute paths in deeply nested directories or commands with many arguments. Consider increasing to 1000 or using TEXT type.
Apply this diff to increase the limit:
- @Column({ type: 'varchar', length: 500, nullable: true })
+ @Column({ type: 'varchar', length: 1000, nullable: true })
command?: string;Or use TEXT type for unlimited length:
- @Column({ type: 'varchar', length: 500, nullable: true })
+ @Column({ type: 'text', nullable: true })
command?: string;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Column({ type: 'varchar', length: 500, nullable: true }) | |
| command?: string; | |
| @Column({ type: 'varchar', length: 1000, nullable: true }) | |
| command?: string; |
| @Column({ type: 'varchar', length: 500, nullable: true }) | |
| command?: string; | |
| @Column({ type: 'text', nullable: true }) | |
| command?: string; |
🤖 Prompt for AI Agents
In src/db/entities/Server.ts around lines 26-27, the command column is currently
defined as varchar(500) which may be too small; update the Column decorator to
either increase the length to 1000 (e.g. { type: 'varchar', length: 1000,
nullable: true }) or change the column to TEXT (e.g. { type: 'text', nullable:
true }) depending on DB portability needs, then generate/run the appropriate
migration to apply the schema change.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/copilot-instructions.md(9 hunks)AGENTS.md(1 hunks)README.fr.md(4 hunks)README.md(3 hunks)README.zh.md(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
🧰 Additional context used
🪛 LanguageTool
.github/copilot-instructions.md
[grammar] ~179-~179: Use a hyphen to join words.
Context: ...TypeScript strict*: Follow strict type checking rules - Import style: `impo...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~238-~238: The official name of this software platform is spelled with a capital “H”.
Context: ...ssues locally first. ### CI Pipeline (.github/workflows/ci.yml) - Runs on Node.js 20...
(GITHUB)
[style] ~257-~257: Consider using a different verb for a more formal wording.
Context: ... - Lint errors: Run pnpm lint and fix reported issues ### Development Issues...
(FIX_RESOLVE)
README.fr.md
[grammar] ~1-~1: Il y a peut-être une erreur ici
Context: # MCPHub : Le Hub Unifié pour les Serveur...
(QB_NEW_FR)
[grammar] ~1-~1: Il y a peut-être une erreur ici
Context: # MCPHub : Le Hub Unifié pour les Serveurs MCP ...
(QB_NEW_FR)
[grammar] ~1-~1: Il y a peut-être une erreur ici
Context: # MCPHub : Le Hub Unifié pour les Serveurs MCP [E...
(QB_NEW_FR)
[grammar] ~1-~1: « le hub unifié » semble plus probable dans ce contexte
Context: # MCPHub : Le Hub Unifié pour les Serveurs MCP [English](README...
(QB_NEW_FR_OTHER_ERROR_IDS_REPLACEMENT_ORTHOGRAPHY_UPPERCASE)
[grammar] ~1-~1: « serveurs » semble plus probable dans ce contexte
Context: # MCPHub : Le Hub Unifié pour les Serveurs MCP English | Français | [中文版](R...
(QB_NEW_FR_OTHER_ERROR_IDS_REPLACEMENT_ORTHOGRAPHY_UPPERCASE)
[typographical] ~3-~3: Pas de correspondance fermante ou ouvrante pour le caractère « [ »
Context: ...b Unifié pour les Serveurs MCP English | Français | [中文版](README.zh...
(UNPAIRED_BRACKETS)
[typographical] ~3-~3: Pas de correspondance fermante ou ouvrante pour le caractère « ] »
Context: ... MCP English | Français | 中文版 MCPHub facilite la g...
(UNPAIRED_BRACKETS)
[style] ~19-~19: Un autre mot peut sembler plus précis et percutant.
Context: ...ation à chaud** - Ajoutez, supprimez ou mettez à jour les serveurs sans temps d'arrêt - **Sup...
(METTRE_A_JOUR)
[style] ~21-~21: Un verbe peut dynamiser votre phrase.
Context: ...z la configuration dans PostgreSQL pour les environnements de production ([En savoir plus](https://do...
(PROP_NOMINALES_EN_VERBALES)
[grammar] ~28-~28: Il y a peut-être une erreur ici
Context: ...de ### Configuration Créez un fichier mcp_settings.json : json { "mcpServers": { "time": { "command": "npx", "args": ["-y", "time-mcp"] }, "fetch": { "command": "uvx", "args": ["mcp-server-fetch"] } } } 📖 Consultez le [Guide de configuration]...
(QB_NEW_FR)
[style] ~45-~45: Une autre structure peut sembler plus percutante.
Context: ...cp-settings) pour les options complètes incluant OAuth, les variables d'environnement, e...
(INCLUANT_EXCLUANT)
[grammar] ~61-~61: Il y a peut-être une erreur ici
Context: ...admin / admin123 ### Connecter les clients IA Connectez les clients IA (Claude Desktop...
(QB_NEW_FR)
[grammar] ~63-~63: Il y a peut-être une erreur ici
Context: ...ients IA (Claude Desktop, Cursor, etc.) via : http://localhost:3000/mcp # Tous les serveurs http://localhost:3000/mcp/{group} # Groupe spécifique http://localhost:3000/mcp/{server} # Serveur spécifique http://localhost:3000/mcp/$smart # Routage intelligent 📖 Consultez la [Référence API](https://...
(QB_NEW_FR)
[grammar] ~74-~74: Il y a peut-être une erreur ici
Context: ...étaillée des points de terminaison. ## 📚 Documentation | Sujet ...
(QB_NEW_FR)
[typographical] ~94-~94: Caractère d’apostrophe incorrect.
Context: ... le backend et le frontend séparément : pnpm backend:dev, pnpm frontend:dev 📖 Consultez le [...
(APOS_INCORRECT)
🪛 markdownlint-cli2 (0.18.1)
README.fr.md
65-65: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
README.md
65-65: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (8)
README.zh.md (3)
16-22: Well-structured feature highlights with comprehensive reference links.The condensed feature list is clear and directly links to relevant documentation for deeper dives. The new emphasis on Smart Routing, OAuth 2.0, and Database Mode aligns perfectly with PR objectives.
28-45: Clear quick-start configuration and helpful documentation links.The simplified
mcp_settings.jsonexample is user-friendly, and the link to the Configuration Guide provides an easy path for users to learn more options (OAuth, environment variables, etc.).
59-72: Excellent endpoint examples and dashboard access instructions.The unified endpoint paths (/mcp, /mcp/{group}, /mcp/{server}, /mcp/$smart) clearly communicate the routing flexibility, and the API Reference link supports discoverability.
.github/copilot-instructions.md (2)
140-150: Excellent addition of DAO Layer documentation.Clear explanation of dual data source support (JSON file vs. PostgreSQL) and the checklist reference to AGENTS.md makes it straightforward for contributors to understand what files need updates when modifying data structures. This significantly improves the contributor experience.
179-179: Grammar suggestion flagged by static analysis is a false positive.The LanguageTool suggestion about hyphenating "TypeScript strict" is incorrect in this context—this is not a compound adjective but rather a markdown formatting artifact. The current formatting is appropriate for the documentation list.
README.md (1)
16-22: Feature highlights clearly communicate the key value propositions.Each feature includes a direct link to detailed documentation, making it easy for users to learn more about OAuth 2.0, smart routing, and database mode.
README.fr.md (2)
1-3: French README title and language links properly formatted.The title simplification and consistent language navigation across all three versions (EN/FR/ZH) improve discoverability and user experience.
16-22: French feature descriptions maintain clarity and include documentation links.The translation is accurate and the feature highlights directly reference the relevant documentation sections.
… endpoint references (#457)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/configuration/database-configuration.mdx (2)
28-29: Clarify USE_DB documentation to reduce confusion.The documentation states that setting
DB_URLautomatically enables database mode (lines 28-29) andUSE_DBis optional. However, the docker-compose example on line 70 explicitly setsUSE_DB: "true", which may confuse users into thinking it's required. Either:
- Remove the redundant
USE_DB: "true"from the example and rely on auto-detection, or- Add a comment explaining that it's explicit for clarity despite being redundant
For consistency, consider clarifying line 142's reference to "when you start MCPHub with
USE_DB=true" to reflect thatDB_URLalone is sufficient.Also applies to: 70-71
179-179: Minor wordiness improvement.Line 179 uses "works exactly the same way" which is slightly wordy. Consider: "The web dashboard works the same way" or "functions identically" for conciseness. This aligns with documentation style best practices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/configuration/database-configuration.mdx(1 hunks)docs/zh/configuration/database-configuration.mdx(1 hunks)docs/zh/development.mdx(1 hunks)docs/zh/essentials/code.mdx(12 hunks)docs/zh/essentials/markdown.mdx(2 hunks)docs/zh/essentials/navigation.mdx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/zh/essentials/navigation.mdx
- docs/zh/configuration/database-configuration.mdx
- docs/zh/essentials/code.mdx
🧰 Additional context used
🪛 LanguageTool
docs/configuration/database-configuration.mdx
[style] ~179-~179: ‘exactly the same’ might be wordy. Consider a shorter alternative.
Context: ...system/config` The web dashboard works exactly the same way, but now stores changes in the data...
(EN_WORDINESS_PREMIUM_EXACTLY_THE_SAME)
🔇 Additional comments (4)
docs/zh/essentials/markdown.mdx (1)
136-136: Clarify documentation scope for endpoint examples.The code examples now use
http://localhost:3000, which is appropriate for local development but may confuse users if they copy-paste these examples from published documentation. This raises a question about the intended audience and use case.Recommendation: Either:
- Update examples to use a production or placeholder endpoint (e.g.,
https://api.example.comor${MCPHUB_ENDPOINT})- Add a contextual note indicating these are development-only examples, or
- Move locale-specific examples to a separate development setup guide
Please clarify:
- Is this documentation intended for end-users or internal developers?
- Should the examples demonstrate production setup or local development setup?
- Are corresponding English examples in
docs/essentials/markdown.mdxupdated consistently?Also applies to: 145-145
docs/zh/development.mdx (2)
51-51: Verify the repository URL change is intentional.Line 51 changes the clone URL from
https://github.com/mcphub/mcphub.gittohttps://github.com/samanhappy/mcphub.git. This appears to reflect a repository ownership or branding change. Confirm this is the intended primary repository URL for users.
119-126: Environment configuration appropriately simplified.The removal of database-related environment variables and database initialization steps from the development guide is well-aligned with the PR's documentation restructuring. Database configuration is now centralized in
docs/configuration/database-configuration.mdx, keeping this development guide focused on basic setup for local development.docs/configuration/database-configuration.mdx (1)
1-298: Comprehensive and well-structured database configuration guide.This new documentation file effectively addresses production database setup requirements. Strengths include:
- Clear rationale for database mode adoption (lines 7-13)
- Multiple setup paths (Docker external, Docker compose, manual)
- Comprehensive migration guidance with both automatic and manual options (lines 136-169)
- Practical troubleshooting section (lines 224-266)
- Security considerations explicitly called out (lines 278-287)
- Performance comparison to inform user decisions (lines 289-298)
- Complete schema reference (lines 181-190)
The documentation provides enterprise-grade clarity and will serve users well for production deployments.
…new features and improve clarity (#814) Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
…824) Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
…lling (#816) Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
…te limits (#829) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: samanhappy <2755122+samanhappy@users.noreply.github.com>
…onfiguration (#826) Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…st context handling (#832)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
…n over environment variables (#838)
… handling and authorization (#842)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
#860) Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
…872) Co-authored-by: Clement Patout <3038285+clempat@users.noreply.github.com>
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: samanhappy <samanhappy@gmail.com>
PR Type
Enhancement, Bug fix, Tests
Description
OAuth 2.0 Integration: Comprehensive OAuth support for upstream MCP servers with dynamic client registration (RFC 7591), authorization code flow with PKCE, and token management
OAuth Authorization Server: Full OAuth 2.0 server implementation with
/oauth/authorize,/oauth/token,/oauth/userinfoendpoints and RFC 8414 metadata discoverySession Persistence: SSE connection keepalive mechanism (30-second pings) and transparent session rebuild on server restart with configurable
enableSessionRebuildflagBearer Token Authentication: Enhanced OAuth bearer token validation with
resolveOAuthUserFromTokenand RFC 9728 compliant WWW-Authenticate headersEnvironment Variable Expansion: Support for
${VAR}and$VARformats in server configurations with recursive expansion in nested objects and arraysSmart Routing Enhancement: Added
$smart/{group}pattern support for group-specific tool searching with proper nested path handlingURL Parameter Decoding: Fixed handling of special characters and slashes in server/tool/prompt names across controllers
Service Registry Refactor: Converted to async function with ES modules
import()for better compatibilityMCP Registry Integration: Proxy endpoints for official MCP registry with cursor-based pagination and server discovery
OAuth Client Management: REST API for CRUD operations on OAuth clients with secret management
Configuration Management: Support for
nameSeparator,enableSessionRebuild, andoauthServerconfiguration updatesFrontend Enhancements: OAuth server configuration UI, registry data hook with pagination, and URL encoding for tool/server names
Comprehensive Test Coverage: Tests for OAuth flows, session rebuild, keepalive pings, environment variable expansion, and smart routing
Diagram Walkthrough
File Walkthrough
26 files
mcpService.ts
OAuth integration and transport async handling for MCP serverssrc/services/mcpService.ts
management for upstream MCP servers
and tool/prompt name separators
initialization
and HTTP transports
handle OAuth authorization states
$smart/{group}pattern for group-specific toolsearching
sseService.ts
Session persistence and OAuth bearer token support for SSEsrc/services/sseService.ts
resolveOAuthUserFromTokenconnection timeouts
server restart
headers
initialization conflicts
oauthServerController.ts
OAuth 2.0 server endpoints and authorization flow implementationsrc/controllers/oauthServerController.ts
(
/oauth/authorize,/oauth/token,/oauth/userinfo)protected resource metadata endpoint
escaping for security
configuration
oauthClientRegistration.ts
Dynamic OAuth client registration and token management servicesrc/services/oauthClientRegistration.ts
servers
WWW-Authenticate headers
mcpOAuthProvider.ts
MCP SDK OAuth provider implementation for upstream serverssrc/services/mcpOAuthProvider.ts
OAuthClientProviderinterface from MCP SDK for server-sideOAuth flows
verifier management
registry.ts
Service registry async refactor for ES modules supportsrc/services/registry.ts
module loading
createRequirewith ES modulesimport()for bettercompatibility
tryLoadOverridehelper function with improved error handlingmechanism
oauthDynamicRegistrationController.ts
OAuth Dynamic Client Registration Controller Implementationsrc/controllers/oauthDynamicRegistrationController.ts
clients
/oauth/registerfor client registration with validationof redirect URIs, grant types, and scopes
management with registration access token authentication
registration access tokens
oauthCallbackController.ts
OAuth Callback Handler for MCP Server Authorizationsrc/controllers/oauthCallbackController.ts
identity
transport.finishAuth()to exchange authorization code for tokensto
connectedscenarios
oauthServerService.ts
OAuth 2.0 Authorization Server Service Implementationsrc/services/oauthServerService.ts
token flows
functions
support
OAuth.ts
OAuth Data Model and Persistence Layersrc/models/OAuth.ts
in-memory and persistent layers
mcp_settings.jsonwith automatic cleanupof expired tokens
tokens
serverController.ts
Server Controller OAuth and Configuration Updatessrc/controllers/serverController.ts
serverName,toolName, andpromptNameparametersto handle slashes
updateSystemConfigvalidation logic with individual flagchecks for each config section
nameSeparator,enableSessionRebuild, andoauthServerconfiguration updates
registration settings
index.ts
OAuth Type Definitions and Server Configuration Interfacessrc/types/index.ts
OAuthProviderConfiginterface for upstream MCP server OAuthconfiguration
IOAuthClient,IOAuthAuthorizationCode,IOAuthTokeninterfacesfor authorization server
OAuthServerConfigwith token lifetimes, PKCE support, and dynamicregistration settings
ServerConfigwith comprehensive OAuth fields includingstatic/dynamic registration and PKCE
oauthfield toServerInfofor tracking authorization state andURLs
useRegistryData.ts
Registry Data Hook with Cursor-Based Paginationfrontend/src/hooks/useRegistryData.ts
nextCursortrackingversions with URL encoding
currentPage,serversPerPage, andhasNextPageuseSettingsData.ts
OAuth server configuration and system settings managementfrontend/src/hooks/useSettingsData.ts
OAuthServerConfiginterface with OAuth server configurationproperties including token lifetimes, scopes, and dynamic registration
settings
oauthServerConfig,nameSeparator, andenableSessionRebuildwith default valuessettings from API responses
updateOAuthServerConfig,updateOAuthServerConfigBatch,updateNameSeparator,updateSessionRebuild, andexportMCPSettingsobject
oauthService.ts
OAuth provider initialization and token management servicesrc/services/oauthService.ts
initialization and token handling
initOAuthProvider()to create and configureProxyOAuthServerProviderfrom system settingsrefresh, and add OAuth headers to requests
initializeAllOAuthClients()to pre-register OAuth clientsfor configured servers
oauthClientRegistration.jspath.ts
Enhanced package root discovery and configuration file path resolutionsrc/utils/path.ts
findPackageRoot()function to locate package root directory withcaching and multiple fallback strategies
initializePackageRoot()to cache package root on moduleload for performance
getConfigFilePath()to prioritize environment variable pathsand check package root directory
getCurrentModuleDir()scenarios
oauthClientController.ts
OAuth client management REST API controllersrc/controllers/oauthClientController.ts
operations)
getAllClients()to list clients with sanitized output (nosecrets exposed)
getClient(),createClient(),updateClient(), anddeleteClient()endpointsregenerateSecret()endpoint to generate new client secretssecret exposure
server.ts
OAuth integration and smart routing path handlingsrc/server.ts
mounting
getCurrentFileDir()function to handle module directoryresolution in different environments
:group?to:group(.*)?to properly handlenested group paths with slashes
findPackageRoot()to use shared utility function frompath.tsindex.ts
OAuth configuration and MCP registry type definitionsfrontend/src/types/index.ts
oauth_requiredstatus toServerStatustype for OAuthauthorization flow
ServerConfigwith comprehensive OAuth configurationproperties including dynamic registration
oauthfield toServerinterface with authorization URL and stateServerFormDatainterface for form handlingremotes, icons, metadata)
AuthResponsewithisUsingDefaultPasswordflagregistryController.ts
MCP registry proxy controller endpointssrc/controllers/registryController.ts
getAllRegistryServers()with cursor-based pagination and searchsupport
getRegistryServerVersions()to fetch all versions of aspecific server
getRegistryServerVersion()to fetch a specific server versionversions
index.ts
OAuth and registry endpoints routing configurationsrc/routes/index.ts
/oauth/callbackfor handling OAuthauthorization responses
userinfo, metadata)
registration
oauthSettingsStore.ts
OAuth settings persistence and mutation servicesrc/services/oauthSettingsStore.ts
configuration
loadServerConfig()to retrieve server OAuth configurationmutateOAuthSettings()for atomic updates to OAuth settings withpersistence
persistClientCredentials(),persistTokens(),updatePendingAuthorization(),clearOAuthData()openApiController.ts
OpenAPI controller refactoring with URL decoding supportsrc/controllers/openApiController.ts
parameterConversion.jsexecuteToolViaOpenAPI()to decode URL-encoded server and toolnames
getNameSeparator()forflexible naming
calls
index.ts
Settings loading and environment variable expansion enhancementssrc/config/index.ts
ensureOAuthServerDefaults()to initialize default OAuth serverconfiguration
loadOriginalSettings()to handle missing settings filegracefully with defaults
defaults
replaceEnvVars()to recursively handle nested objects andarrays
getNameSeparator()function to retrieve configured nameseparator from settings
dataService.ts
Data service user-aware filtering and permissionssrc/services/dataService.ts
DataServicefrom interface-based to class-basedimplementation
filterData()andfilterSettings()methods
administrators
mergeSettings()to handle user-specific configurations andOAuth-related settings
getPermissions()to return appropriate permissions based onadmin status
UserContextServicefor current user contexttoolService.ts
Tool service URL encoding for server and tool namesfrontend/src/services/toolService.ts
callTool()function tohandle slashes
toggleTool()to URL-encode server and tool names in APIendpoint
updateToolDescription()to URL-encode server and tool namescom.atlassian/atlassian-mcp-server)1 files
promptController.ts
URL parameter decoding for prompt controllersrc/controllers/promptController.ts
serverNameandpromptNameparameters to handlespecial characters and slashes
paths
12 files
sseService.test.ts
SSE Service Tests with Bearer Auth and Session Rebuildsrc/services/sseService.test.ts
RFC 8707 resource metadata
expectBearerUnauthorizedhelper for consistent 401 responsevalidation
enableSessionRebuildconfiguration flag
responses when disabled
replaceEnvVars.test.ts
Environment Variable Expansion Configuration Teststests/config/replaceEnvVars.test.ts
configuration
expandEnvVarsfor${VAR}and$VARformats with multiplevariables
replaceEnvVarsfor recursive expansion in nested objects,arrays, and mixed structures
configurations
keepalive.test.ts
Keepalive Ping Functionality Tests for SSE Connectionstests/services/keepalive.test.ts
intervals
handling
sent correctly
keepalive behavior
mcpService-smart-routing-group.test.ts
MCP Service Smart Routing with Group Support Teststests/services/mcpService-smart-routing-group.test.ts
$smartand$smart/{group}patterns
search_toolsandcall_toolfunctionality with serverfiltering
group membership
parameterConversion.test.ts
Parameter type conversion utility test suitetests/utils/parameterConversion.test.ts
convertParametersToTypes()utility function
object types
parameter types
definitions
oauth.test.ts
OAuth model operations test suitetests/models/oauth.test.ts
authorization code, and token management
prevention
revoke, expiration)
and expiration handling
mcpService-headers.test.ts
Environment variable expansion in headers test suitetests/services/mcpService-headers.test.ts
arguments
expandEnvVars()andreplaceEnvVars()functions withvarious formats (
${VAR}and$VAR)oauthService.test.ts
OAuth service initialization and token handling teststests/services/oauthService.test.ts
management
enabled/disabled
addition
scenarios
cliPathHandling.test.ts
CLI path handling and ESM URL conversion teststests/utils/cliPathHandling.test.ts
pathToFileURL()paths, and special characters
openApiController.test.ts
OpenAPI controller tests refactoring and URL encoding validationtests/controllers/openApiController.test.ts
convertParametersToTypes()utilityfunction
com.atlassian/atlassian-mcp-server)configController.test.ts
Configuration controller export functionality teststests/controllers/configController.test.ts
getMcpSettingsJson()controller functionscenarios
servers
server-smart-routing.test.ts
Smart routing integration tests for nested pathstests/integration/server-smart-routing.test.ts
path segments
with complex paths
$smart/test-groupare correctlycaptured in route parameters
101 files