Implement project access guards #23
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive project access control system for the Backlog MCP server to enhance security by restricting operations to an allow-list of projects. The implementation includes guard services, error handling, and configuration management improvements.
- Adds
ProjectGuardServiceto enforce project access restrictions with configurable read/write guard policies - Introduces
ProjectAccessForbiddenErrorfor structured error responses when access is denied - Centralizes configuration management from
index.tsinto a dedicatedconfig.tsmodule
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/config.ts | New centralized configuration module with project access control options |
| src/guards/ProjectGuardService.ts | Core service for managing and enforcing project access rules |
| src/handlers/transformers/wrapWithProjectGuard.ts | Wrapper function to apply project guards to tool handlers |
| src/errors/ProjectAccessForbiddenError.ts | Custom error class for project access violations |
| src/registerTools.ts | Updated to integrate project guard wrapper with tool registration |
| src/index.ts | Refactored to use centralized config and initialize guard service |
| src/types/result.ts | Extended ErrorLike type to support error codes and data |
| src/backlog/backlogErrorHandler.ts | Added handling for ProjectAccessForbiddenError |
| package.json | Updated dev script to use tsx for faster development workflow |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
| const allowed = candidates.find((id) => guardService.isAllowed(id)); | ||
| return allowed ? [allowed] : [candidates[0]]; | ||
| } |
There was a problem hiding this comment.
Missing semicolon after the function definition. This will cause a syntax error.
| } | |
| }; |
Copilot uses AI. Check for mistakes.
| targetProjectIds = [project.id]; | ||
| } | ||
|
|
||
| for(const targetProjectId of targetProjectIds) { |
There was a problem hiding this comment.
Missing space after 'for' keyword. Should be 'for (const targetProjectId of targetProjectIds)'.
| for(const targetProjectId of targetProjectIds) { | |
| for (const targetProjectId of targetProjectIds) { |
Copilot uses AI. Check for mistakes.
| // Copyright (c) 2025 Nulab inc. | ||
| // Licensed under the MIT License. | ||
|
|
||
| import { ReadGuardPolicy, WriteGuardPolicy } from '../config.js'; |
There was a problem hiding this comment.
The imported types ReadGuardPolicy and WriteGuardPolicy are not used in this file. This import should be removed as it's unnecessary.
| import { ReadGuardPolicy, WriteGuardPolicy } from '../config.js'; |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
@trknhr Thanks for working on this and sharing the draft!
I was interested to see what type of solution you came up with and had a few comments that might be worth dropping in. Curious to hear what you think.
| registerTools(server, toolsetGroup, mcpOption, guardService, backlog); | ||
|
|
||
| // Register dynamic tool management tools if enabled | ||
| if (config.dynamicToolsets) { | ||
| const registrar = createToolRegistrar(server, toolsetGroup, mcpOption, guardService, backlog); |
There was a problem hiding this comment.
createToolRegistrar internally calls registerTools which means when using dynamicToolsets, registerTools is called twice with the same arguments. Could this call be cached and reused or are they different calls?
There was a problem hiding this comment.
It looks like the helper function definitions isNilOrEmpty and coerceToNumericIds could be moved out of the function scope of wrapWithProjectGuard to improve the clarity a bit
| return []; | ||
| } | ||
| const allowed = candidates.find((id) => guardService.isAllowed(id)); | ||
| return allowed ? [allowed] : [candidates[0]]; |
There was a problem hiding this comment.
Should we still be choosing the first candidate if there are no allowed projectIds found?
| const resolveCandidate = (candidates: number[]): number[] => { | ||
| if (candidates.length === 0) { | ||
| return []; | ||
| } | ||
| const allowed = candidates.find((id) => guardService.isAllowed(id)); | ||
| return allowed ? [allowed] : [candidates[0]]; | ||
| } | ||
|
|
||
| let targetProjectIds: number[] = resolveCandidate(primaryCandidates); |
There was a problem hiding this comment.
We might not need the helper function resolveCandidate:
| const resolveCandidate = (candidates: number[]): number[] => { | |
| if (candidates.length === 0) { | |
| return []; | |
| } | |
| const allowed = candidates.find((id) => guardService.isAllowed(id)); | |
| return allowed ? [allowed] : [candidates[0]]; | |
| } | |
| let targetProjectIds: number[] = resolveCandidate(primaryCandidates); | |
| let targetProjectIds: number[] = primaryCandidates.filter(guardService.isAllowed) |
2 participants
This update introduces a robust Project Access Control feature to enhance security by restricting the server's operations to an allow-list of Backlog projects. It also includes significant improvements to the development workflow and configuration management.
Key Features & Enhancements
🛡️ Project Access Guards
BACKLOG_ALLOWED_PROJECT_IDS: A comma-separated list of project IDs.BACKLOG_ALLOWED_PROJECT_KEYS: A comma-separated list of project keys (e.g., "BLG", "MYPROJ"). These keys are resolved to their corresponding IDs at startup.ProjectGuardServicehas been implemented to manage and enforce these access rules.ProjectAccessForbiddenError, is thrown when access is denied, providing clear, structured error data in the MCP response.⚙️ Developer Experience & Refactoring
devscript has been updated fromts-nodetotsx. This provides significantly faster startup and hot-reloading capabilities, improving the development feedback loop.yargsandenv-var) has been consolidated fromindex.tsinto a new, dedicatedsrc/config.tsmodule. This makes configuration clearer and easier to manage.