Upgrade Node.js to 20 LTS, switch to pnpm for dependency management, … #7
Conversation
…and enhance Dockerfile for production readiness - Updated base image in Dockerfile to Node.js 20 LTS. - Replaced npm with pnpm for dependency installation and added support for pnpm workspace. - Removed package-lock.json and updated package.json to reflect new dependencies including express and cors. - Enhanced README with detailed Docker installation instructions and transport mode configurations. - Improved server initialization logic in index.ts to support both HTTP and stdio transports with better logging and error handling.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @bsmi021, 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 delivers a substantial upgrade to the MCP WebScan server, modernizing its core infrastructure by moving to Node.js 20 LTS and pnpm, and significantly enhancing its deployment capabilities through improved Dockerization. A major new feature is the introduction of a robust HTTP transport layer with integrated GitHub OAuth authentication, enabling secure remote access and broader client compatibility. These changes collectively improve the server's performance, security, and ease of deployment and use.
Highlights
- Node.js & Dependency Management Upgrade: Upgraded the Node.js runtime to 20 LTS and transitioned from
npmtopnpmfor dependency management, including support for pnpm workspaces. - Enhanced Dockerization: Significantly improved the Dockerfile for production readiness, incorporating Node.js 20 LTS, pnpm, non-root user execution, security updates, and robust health checks. New Docker Compose configurations are also provided.
- HTTP Transport & Authentication: Introduced a new HTTP transport mode for the MCP server, complementing the existing stdio mode. This includes comprehensive GitHub OAuth authentication, session management, and CORS support.
- Server Architecture Refinement: Reworked server initialization logic in
src/index.tsto dynamically select between HTTP and stdio transports, enhancing logging and error handling. Dedicated transport classes (src/transports/HttpTransport.ts,src/transports/StdioTransport.ts) and authentication middleware (src/auth/AuthMiddleware.ts,src/auth/GitHubOAuth.ts) were added. - Documentation & Examples: Added extensive documentation including a new
CLAUDE.mdfor AI guidance, updatedREADME.mdwith detailed Docker and transport mode instructions, and new.env.example,docker-compose.yml, andsetup-oauth.jsfiles for easier setup and testing. A Python test script (test_tools.py) and its report (MCP_WebScan_HTTP_Transport_Test_Report.md) were also added.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This pull request implements a major upgrade to the MCP WebScan Server, upgrading from Node.js 18 to Node.js 20 LTS, replacing npm with pnpm for dependency management, and adding comprehensive HTTP transport support with GitHub OAuth authentication alongside the existing stdio transport. The changes significantly enhance the server's production readiness and remote accessibility while maintaining backward compatibility.
- Upgraded to Node.js 20 LTS and migrated from npm to pnpm for improved performance and dependency management
- Added dual transport support (stdio + HTTP) with MCP 2025-03-26 Streamable HTTP specification compliance
- Implemented comprehensive GitHub OAuth authentication system for secure remote access
Reviewed Changes
Copilot reviewed 19 out of 22 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updated Node.js to v20, added express/cors dependencies, migrated to pnpm |
| Dockerfile | Enhanced for production with Node.js 20, pnpm, security hardening, and health checks |
| src/index.ts | Refactored to support dual transport modes with configuration-based selection |
| src/transports/HttpTransport.ts | New comprehensive HTTP transport with session management and OAuth integration |
| src/auth/GitHubOAuth.ts | Complete GitHub OAuth 2.0 provider implementation with security best practices |
| setup-oauth.js | OAuth setup helper script for streamlined configuration |
| test_tools.py | Python test client for comprehensive HTTP transport validation |
| docker-compose.yml | Multi-service Docker setup for different deployment scenarios |
src/transports/HttpTransport.ts
Outdated
| import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp.js'; | ||
| import { InitializeRequest } from '@modelcontextprotocol/sdk/types.js'; | ||
|
|
||
| // Helper function to check if a request is an initialize request |
There was a problem hiding this comment.
The isInitializeRequest helper function and its import should be moved to a utilities module or imported from the SDK to avoid code duplication and improve maintainability.
| headers: { | ||
| 'Accept': 'application/json', | ||
| 'Content-Type': 'application/json', | ||
| 'User-Agent': 'webscan-mcp-server/1.0.0' |
There was a problem hiding this comment.
The User-Agent string is hardcoded and should be configurable or derived from package.json to ensure it stays in sync with the actual version.
| } | ||
|
|
||
| // Cache user info (expires in 1 hour) | ||
| this.userTokens.set(accessToken, { |
There was a problem hiding this comment.
Storing access tokens in an in-memory Map creates a potential security risk if the process crashes or memory is dumped. Consider using encrypted storage or a secure token cache with proper cleanup mechanisms.
| private mcpServer: McpServer; | ||
| private authMiddleware: AuthMiddleware; | ||
|
|
||
| constructor(config: TransportConfig, mcpServer: McpServer) { |
There was a problem hiding this comment.
[nitpick] The constructor is doing significant work including setting up middleware and routes. Consider moving the setup logic to separate initialization methods for better testability and separation of concerns.
| console.log('🔐 MCP WebScan Server - GitHub OAuth Setup Helper\n'); | ||
|
|
||
| // Generate a random cookie secret | ||
| const cookieSecret = crypto.randomBytes(32).toString('hex'); |
There was a problem hiding this comment.
The generated cookie secret should be explicitly marked as sensitive information in the output, and users should be warned to keep it secure and not commit it to version control.
|
|
||
| constructor(config: GitHubOAuthConfig) { | ||
| this.config = config; | ||
|
|
||
| // Clean up expired sessions periodically | ||
| setInterval(() => { | ||
| this.cleanupExpiredSessions(); | ||
| }, 60 * 1000); // Check every minute |
There was a problem hiding this comment.
The setInterval for session cleanup runs every minute which may be excessive for low-traffic scenarios. Consider making the cleanup interval configurable or using a more sophisticated cleanup strategy.
| constructor(config: GitHubOAuthConfig) { | |
| this.config = config; | |
| // Clean up expired sessions periodically | |
| setInterval(() => { | |
| this.cleanupExpiredSessions(); | |
| }, 60 * 1000); // Check every minute | |
| private readonly DEFAULT_CLEANUP_INTERVAL = 60 * 1000; // 1 minute | |
| constructor(config: GitHubOAuthConfig) { | |
| this.config = config; | |
| // Clean up expired sessions periodically | |
| const cleanupInterval = config.cleanupInterval ?? this.DEFAULT_CLEANUP_INTERVAL; | |
| setInterval(() => { | |
| this.cleanupExpiredSessions(); | |
| }, cleanupInterval); |
| test_url = "https://www.foxnews.com" | ||
|
|
There was a problem hiding this comment.
The test URL is hardcoded. Consider making it configurable via command line arguments or environment variables to allow testing against different websites.
| test_url = "https://www.foxnews.com" | |
| # Determine the test URL | |
| parser = argparse.ArgumentParser(description="Test MCP WebScan tools.") | |
| parser.add_argument("--url", type=str, help="Target website URL for testing.") | |
| args = parser.parse_args() | |
| test_url = args.url or os.getenv("TEST_URL", "https://www.foxnews.com") | |
| print(f"Using test URL: {test_url}") | |
src/transports/HttpTransport.ts
Outdated
| })); | ||
|
|
||
| // JSON parsing | ||
| this.app.use(express.json({ limit: '4mb' })); // MCP default message size limit |
There was a problem hiding this comment.
The message size limit should be configurable rather than hardcoded, as different deployments may have different requirements based on their use cases.
There was a problem hiding this comment.
Code Review
This pull request is a major upgrade, moving to Node.js 20, adopting pnpm, and introducing a full-featured HTTP transport with OAuth. The changes are well-implemented and follow good practices for containerization and security. My review focuses on ensuring consistency across the new documentation and code, fixing a bug in the Dockerfile's health check, and improving type safety in a few places. Overall, this is a great set of enhancements.
| RUN apk update && apk upgrade && \ | ||
| npm install -g pnpm && \ | ||
| addgroup -g 1001 -S nodejs && \ | ||
| adduser -S mcp -u 1001 |
There was a problem hiding this comment.
The HEALTHCHECK instruction on line 63 uses wget, but wget is not installed in the release stage of the Docker image. The node:20-alpine base image doesn't include wget by default, which will cause the health check to fail and could lead to the container being terminated by an orchestrator. Please add wget to the list of packages to install.
RUN apk update && apk upgrade && \
apk add --no-cache wget && \
npm install -g pnpm && \
addgroup -g 1001 -S nodejs && \
adduser -S mcp -u 1001
| "Bash(npm run build:*)", | ||
| "Bash(npm install:*)", | ||
| "Bash(npm start)", | ||
| "Bash(TRANSPORT_MODE=http npm start)", | ||
| "Bash(TRANSPORT_MODE=http HTTP_PORT=3001 npm start)", |
There was a problem hiding this comment.
This pull request migrates the project to use pnpm. For consistency, these Bash commands should be updated from npm to pnpm.
"Bash(pnpm run build:*)",
"Bash(pnpm install:*)",
"Bash(pnpm start)",
"Bash(TRANSPORT_MODE=http pnpm start)",
"Bash(TRANSPORT_MODE=http HTTP_PORT=3001 pnpm start)"
| npm install | ||
|
|
||
| # Build project (TypeScript compilation) | ||
| npm run build | ||
|
|
||
| # Development mode (with ts-node) | ||
| npm run dev | ||
|
|
||
| # Start production server | ||
| npm start |
There was a problem hiding this comment.
The project has been migrated to pnpm, but the command examples in this document still use npm. To ensure the documentation is accurate and to avoid confusion for developers, please update these commands to use pnpm.
| npm install | |
| # Build project (TypeScript compilation) | |
| npm run build | |
| # Development mode (with ts-node) | |
| npm run dev | |
| # Start production server | |
| npm start | |
| # Install dependencies | |
| pnpm install | |
| # Build project (TypeScript compilation) | |
| pnpm run build | |
| # Development mode (with ts-node) | |
| pnpm run dev | |
| # Start production server | |
| pnpm start |
README.md
Outdated
| For local use with MCP clients like Claude Desktop: | ||
|
|
||
| ```bash | ||
| npm start |
There was a problem hiding this comment.
The project has been migrated to pnpm, but this command and others in the documentation still use npm. Please update them to pnpm for consistency.
| npm start | |
| pnpm start |
| ├── LICENSE | ||
| ├── mcp-consistant-servers-guide.md | ||
| ├── package.json | ||
| ├── package-lock.json |
There was a problem hiding this comment.
The project structure in the documentation lists package-lock.json. Since the project has migrated to pnpm, this should be updated to pnpm-lock.yaml.
| ├── package-lock.json | |
| ├── pnpm-lock.yaml |
| npm install @modelcontextprotocol/sdk | ||
| ``` | ||
|
|
||
| > ⚠️ MCP requires Node.js v18.x or higher to work fine. |
There was a problem hiding this comment.
This documentation states that Node.js v18.x or higher is required. However, the project has been upgraded to Node.js 20, and other documentation (README.md, Dockerfile) reflects this. To maintain consistency, please update this requirement to Node.js v20.x or higher.
| > ⚠️ MCP requires Node.js v18.x or higher to work fine. | |
| > ⚠️ MCP requires Node.js v20.x or higher to work fine. |
|
|
||
| console.log(''); | ||
| console.log('4. ✅ Test the setup:'); | ||
| console.log(' 📍 Run: npm start'); |
There was a problem hiding this comment.
This helper script outputs instructions that reference npm start. Since the project now uses pnpm, this should be updated to pnpm start to ensure the instructions are correct.
| console.log(' 📍 Run: npm start'); | |
| console.log(' 📍 Run: pnpm start'); |
| logger.debug('User authenticated successfully', { | ||
| userId: user.id, | ||
| username: user.login | ||
| } as any); |
There was a problem hiding this comment.
The as any type assertion here is unnecessary and bypasses type checking. The object being passed is already compatible with the logger's expected context type (Record<string, unknown>). Removing the cast will improve type safety.
| logger.debug('User authenticated successfully', { | |
| userId: user.id, | |
| username: user.login | |
| } as any); | |
| logger.debug('User authenticated successfully', { | |
| userId: user.id, | |
| username: user.login | |
| }); |
|
|
||
| } catch (error) { | ||
| console.error("Error initializing webscan MCP server:", error); | ||
| logger.error("Error initializing webscan MCP server:", error as any); |
There was a problem hiding this comment.
Using as any for a caught error is not type-safe. A better approach is to wrap the unknown error in an object, which is compatible with the logger's context type. This ensures that whatever is caught can be safely logged.
| logger.error("Error initializing webscan MCP server:", error as any); | |
| logger.error("Error initializing webscan MCP server:", { error }); |
src/transports/HttpTransport.ts
Outdated
| const isInitializeRequest = (request: any): request is InitializeRequest => { | ||
| return request && request.method === 'initialize'; | ||
| }; |
There was a problem hiding this comment.
This file re-implements the isInitializeRequest type guard. The @modelcontextprotocol/sdk package provides this utility. To improve maintainability and rely on the official SDK implementation, you should import and use the function from the SDK instead of defining it locally.
You can import it with:
import { isInitializeRequest } from '@modelcontextprotocol/sdk/types.js';
…guration - Updated Dockerfile to include wget for health checks and ensure security updates. - Replaced npm commands with pnpm in README for consistency in dependency management. - Added new environment variables for maximum message size and token expiration time in README. - Refactored HttpTransport to utilize configurable message size limit for JSON parsing.
…e error handling in HttpTransport - Removed the mcp-webscan-stdio service from docker-compose.yml as it is no longer needed. - Updated error handling in HttpTransport to specify error type as 'any' for better type safety.
- Removed the version declaration from docker-compose.yml as it is no longer necessary for the current setup.
…ty and future reference. This change maintains the existing configuration while preventing it from being executed.
…and enhance Dockerfile for production readiness