feat: migrate REST → GraphQL, add CI/CD (v0.2.0)#1
Open
alexandresaiz wants to merge 2 commits intomainfrom
Open
feat: migrate REST → GraphQL, add CI/CD (v0.2.0)#1alexandresaiz wants to merge 2 commits intomainfrom
alexandresaiz wants to merge 2 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughTransition from a REST-based MCP server to a GraphQL-native v0.2.0 design: new GraphQL client and tools, simplified OAuth/PKCE state handling, consolidated tool definitions and dispatch, streamlined middleware/audit/rate-limiter, updated CI/deployment, and substantial test-suite rework. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (MCP inspector / UI)
participant Server as MCP Server
participant ClientLib as MoneiGraphQLClient
participant GraphQL as Monei GraphQL API
Client->>Server: POST /call-tool { toolName, args, apiKey? }
Server->>ClientLib: new MoneiGraphQLClient({ apiKey, endpoint })
Server->>ClientLib: execute(graphqlQuery, variables)
ClientLib->>GraphQL: HTTP POST /graphql (Authorization: ApiKey ...)
GraphQL-->>ClientLib: 200 { data / errors }
ClientLib-->>Server: parsed data or thrown error
Server-->>Client: 200 { content } or 4xx/5xx error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MONEI-MCP-Server/.env.example`:
- Around line 16-18: The .env example documents MONEI_API_BASE_URL but the
runtime reads MONEI_GRAPHQL_ENDPOINT (see src/index.ts), causing
misconfiguration; update the environment template to either replace
MONEI_API_BASE_URL with MONEI_GRAPHQL_ENDPOINT (set to the GraphQL URL, e.g.
https://api.monei.com/graphql) or add MONEI_GRAPHQL_ENDPOINT alongside the
existing key and include an explanatory comment so the runtime picks up the
correct variable name referenced in src/index.ts.
In `@MONEI-MCP-Server/.node-version`:
- Line 1: Remove the stale MONEI-MCP-Server subdirectory and its leftover files
(including the subdirectory's .node-version containing "22.14.0" and the v0.1.0
package.json) from the repository, commit the deletion, and ensure the root
.node-version (20) and root package.json v0.2.0 remain as the single source of
truth; also check and update any CI/config references that point to
MONEI-MCP-Server so they no longer reference the removed directory.
In `@MONEI-MCP-Server/railway.json`:
- Around line 5-8: The deploy config uses startCommand "node dist/index.js" but
buildCommand currently skips building, so ensure the build produces the dist
artifacts or change startup to run the source directly; specifically update
buildCommand to run your project's build step (e.g., run the package.json build
script to emit dist) or alter startCommand to point to the compiled output you
actually produce (or to a runtime entry like src/index.js) so that dist/index.js
exists at deploy time; modify the "buildCommand" or "startCommand" entries
accordingly and keep names buildCommand, startCommand, and dist/index.js in
sync.
In `@MONEI-MCP-Server/SECURITY.md`:
- Around line 49-53: The SECURITY.md claims rate limiting and audit logging are
active but src/server.ts currently calls handleToolCall directly; import and
apply the existing middleware from src/middleware/rate-limiter.ts (e.g.,
rateLimiter) and src/middleware/audit-logger.ts (e.g., auditLogger or
auditLogger.log) into the request flow that invokes handleToolCall so requests
pass through rateLimiter and are recorded by auditLogger before/after
handleToolCall, or if you opt not to enable them, update SECURITY.md to remove
or revise the Operational Security bullets to reflect the actual implemented
controls; ensure you reference the existing symbols rateLimiter, auditLogger and
handleToolCall when making the change.
- Around line 36-40: SECURITY.md currently lists only five operations and
misnames capturePayment; update the document to list all eight operations
defined in src/types/index.ts — refundPayment, capturePayment, cancelPayment,
cancelSubscription, updateAccount, deleteWebhook, createApiKey, deleteApiKey —
and provide accurate one-line descriptions for each (e.g., capturePayment:
capture an authorized payment to finalize the charge; cancelPayment: void or
cancel an in-flight authorization; refundPayment: issue refunds;
cancelSubscription: cancel/delete subscriptions; updateAccount: modify account
settings; deleteWebhook: remove webhook endpoints; createApiKey: generate new
API keys; deleteApiKey: revoke existing API keys). Ensure the terminology
matches the function intent and replace the misleading "Charge cards or Bizum"
wording with the precise description for capturePayment.
In `@README.md`:
- Around line 143-145: Update the README middleware list to reflect current
implementation: change the description for src/middleware/security.ts to mention
bodySizeGuard (e.g., "body size guard") instead of "input guard", and update
src/middleware/rate-limiter.ts description to indicate it uses a fixed-window
limiter (e.g., "fixed-window per-account limiter") rather than "sliding window";
ensure the entries reference the actual exported symbol names like bodySizeGuard
and the rate-limiter module so the docs match v0.2.0 behavior.
In `@src/middleware/security.ts`:
- Around line 3-8: The corsHeaders middleware currently sets
Access-Control-Allow-Origin to "*" which opens browser-facing routes to any
origin; update the corsHeaders function to enforce a configured whitelist (e.g.,
from process.env.ALLOWED_ORIGINS or app config), check the incoming
req.headers.origin and if it matches the whitelist set
Access-Control-Allow-Origin to that origin (otherwise omit the header or return
a 403 for disallowed origins), keep Access-Control-Allow-Methods/Headers and
handle OPTIONS responses as now; reference the corsHeaders middleware to locate
and replace the wildcard-origin behavior with origin validation against the
whitelist.
- Around line 20-24: The bodySizeGuard middleware cannot prevent large bodies
because express.json() already consumes the stream and content-length is
untrusted; instead either (A) move the bodySizeGuard registration to run before
express.json() in your middleware chain (so bodySizeGuard sees the raw request
stream) and change bodySizeGuard to count bytes from the incoming request stream
rather than trusting req.headers["content-length"], or (B) replace the custom
guard with express.json({ limit: <maxBytes> }) so express itself enforces the
limit; update references to bodySizeGuard and ensure whichever approach you pick
is applied before the JSON body parser (express.json) in the server setup.
In `@src/tools/payments.ts`:
- Line 95: The call in the "list_payments" case uses the `||` operator so
explicit zero values are treated as falsy and replaced (e.g., `(args.limit as
number) || 20`), losing an intentional `limit: 0`; change these to use nullish
coalescing or an explicit undefined check (e.g., `(args.limit as number) ?? 20`
or `args.limit !== undefined ? args.limit : 20`) and do the same for
`args.offset` so `client.listCharges` receives a preserved 0 when callers pass
it.
In `@src/tools/subscriptions.ts`:
- Line 35: The return in the "list_subscriptions" case uses logical OR which
overwrites explicit zero values; update the arguments passed to
client.listSubscriptions so that size uses nullish coalescing for args.limit and
from uses nullish coalescing for args.offset (i.e., keep explicit 0 from
args.limit/args.offset but fall back to 20 and 0 only when those fields are null
or undefined), referencing the "list_subscriptions" case and the
args.limit/args.offset parameters used for size and from.
In `@tests/middleware/middleware.test.ts`:
- Around line 42-55: The tests for withAudit currently only check return/throw
and not that an audit record was emitted; update both specs to assert the audit
logging was invoked: in the "withAudit wraps successful calls" test, after
calling withAudit("get_account_info", ..., async () => ({ name: "Test" })),
assert the console (or the audit logger) spy was called with an entry containing
the action "get_account_info", the account id "acc_1", the input/context object,
and the successful result; in the "withAudit logs errors and re-throws" test,
after expecting the rejection for withAudit("get_payment", { paymentId: "bad" },
"acc_1", ...), assert the console/audit spy was called with an entry containing
action "get_payment", account "acc_1", the context payload, and the error
message "Not found". Ensure you keep the existing spy setup/restore
(vi.spyOn(console, "log").mockImplementation(...)) and use its call assertions
to validate the audit payload rather than only checking return values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3f1f5de-20d6-432c-9c13-7a68b85a0586
⛔ Files ignored due to path filters (58)
MONEI-MCP-Server/package-lock.jsonis excluded by!**/package-lock.jsondist/api/monei-client.d.tsis excluded by!**/dist/**dist/api/monei-client.d.ts.mapis excluded by!**/dist/**,!**/*.mapdist/api/monei-client.jsis excluded by!**/dist/**dist/api/monei-client.js.mapis excluded by!**/dist/**,!**/*.mapdist/auth/oauth.d.tsis excluded by!**/dist/**dist/auth/oauth.d.ts.mapis excluded by!**/dist/**,!**/*.mapdist/auth/oauth.jsis excluded by!**/dist/**dist/auth/oauth.js.mapis excluded by!**/dist/**,!**/*.mapdist/auth/pkce.d.tsis excluded by!**/dist/**dist/auth/pkce.d.ts.mapis excluded by!**/dist/**,!**/*.mapdist/auth/pkce.jsis excluded by!**/dist/**dist/auth/pkce.js.mapis excluded by!**/dist/**,!**/*.mapdist/auth/session.d.tsis excluded by!**/dist/**dist/auth/session.d.ts.mapis excluded by!**/dist/**,!**/*.mapdist/auth/session.jsis excluded by!**/dist/**dist/auth/session.js.mapis excluded by!**/dist/**,!**/*.mapdist/index.d.tsis excluded by!**/dist/**dist/index.d.ts.mapis excluded by!**/dist/**,!**/*.mapdist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.mapdist/middleware/audit-logger.d.tsis excluded by!**/dist/**dist/middleware/audit-logger.d.ts.mapis excluded by!**/dist/**,!**/*.mapdist/middleware/audit-logger.jsis excluded by!**/dist/**dist/middleware/audit-logger.js.mapis excluded by!**/dist/**,!**/*.mapdist/middleware/rate-limiter.d.tsis excluded by!**/dist/**dist/middleware/rate-limiter.d.ts.mapis excluded by!**/dist/**,!**/*.mapdist/middleware/rate-limiter.jsis excluded by!**/dist/**dist/middleware/rate-limiter.js.mapis excluded by!**/dist/**,!**/*.mapdist/middleware/security.d.tsis excluded by!**/dist/**dist/middleware/security.d.ts.mapis excluded by!**/dist/**,!**/*.mapdist/middleware/security.jsis excluded by!**/dist/**dist/middleware/security.js.mapis excluded by!**/dist/**,!**/*.mapdist/server.d.tsis excluded by!**/dist/**dist/server.d.ts.mapis excluded by!**/dist/**,!**/*.mapdist/server.jsis excluded by!**/dist/**dist/server.js.mapis excluded by!**/dist/**,!**/*.mapdist/tools/account.d.tsis excluded by!**/dist/**dist/tools/account.d.ts.mapis excluded by!**/dist/**,!**/*.mapdist/tools/account.jsis excluded by!**/dist/**dist/tools/account.js.mapis excluded by!**/dist/**,!**/*.mapdist/tools/index.d.tsis excluded by!**/dist/**dist/tools/index.d.ts.mapis excluded by!**/dist/**,!**/*.mapdist/tools/index.jsis excluded by!**/dist/**dist/tools/index.js.mapis excluded by!**/dist/**,!**/*.mapdist/tools/payments.d.tsis excluded by!**/dist/**dist/tools/payments.d.ts.mapis excluded by!**/dist/**,!**/*.mapdist/tools/payments.jsis excluded by!**/dist/**dist/tools/payments.js.mapis excluded by!**/dist/**,!**/*.mapdist/tools/subscriptions.d.tsis excluded by!**/dist/**dist/tools/subscriptions.d.ts.mapis excluded by!**/dist/**,!**/*.mapdist/tools/subscriptions.jsis excluded by!**/dist/**dist/tools/subscriptions.js.mapis excluded by!**/dist/**,!**/*.mapdist/types/index.d.tsis excluded by!**/dist/**dist/types/index.d.ts.mapis excluded by!**/dist/**,!**/*.mapdist/types/index.jsis excluded by!**/dist/**dist/types/index.js.mapis excluded by!**/dist/**,!**/*.mappackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (45)
.github/dependabot.yml.github/workflows/ci.yml.github/workflows/schema-check.yml.gitignore.node-versionMONEI-MCP-Server/.env.exampleMONEI-MCP-Server/.gitignoreMONEI-MCP-Server/.node-versionMONEI-MCP-Server/LICENSEMONEI-MCP-Server/README.mdMONEI-MCP-Server/SECURITY.mdMONEI-MCP-Server/package.jsonMONEI-MCP-Server/railway.jsonMONEI-MCP-Server/tsconfig.jsonMONEI-MCP-Server/vitest.config.tsREADME.mdpackage.jsonrailway.jsonsrc/api/monei-client.tssrc/auth/oauth.tssrc/auth/pkce.tssrc/auth/session.tssrc/index.tssrc/middleware/audit-logger.tssrc/middleware/rate-limiter.tssrc/middleware/security.tssrc/server.tssrc/tools/account.tssrc/tools/index.tssrc/tools/payments.tssrc/tools/subscriptions.tssrc/types/index.tstests/api/monei-client.test.tstests/auth/auth.test.tstests/auth/pkce.test.tstests/auth/scopes.test.tstests/auth/session.test.tstests/middleware/audit-logger.test.tstests/middleware/middleware.test.tstests/middleware/rate-limiter.test.tstests/middleware/security.test.tstests/tools/restrictions.test.tstests/tools/routing.test.tstests/tools/tools.test.tstests/tools/validation.test.ts
💤 Files with no reviewable changes (9)
- tests/auth/scopes.test.ts
- tests/tools/routing.test.ts
- tests/tools/validation.test.ts
- tests/middleware/rate-limiter.test.ts
- tests/auth/pkce.test.ts
- tests/middleware/security.test.ts
- tests/tools/restrictions.test.ts
- tests/auth/session.test.ts
- tests/middleware/audit-logger.test.ts
| @@ -0,0 +1 @@ | |||
| 22.14.0 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check all .node-version files in the repository
fd -H '.node-version' --exec sh -c 'echo "{}:"; cat "{}"'Repository: MONEI/MONEI-MCP-Server
Length of output: 127
🏁 Script executed:
#!/bin/bash
# Check repository structure and package.json files
echo "=== Root package.json (first 30 lines) ==="
head -30 package.json 2>/dev/null || echo "No root package.json"
echo -e "\n=== MONEI-MCP-Server package.json (first 30 lines) ==="
head -30 MONEI-MCP-Server/package.json 2>/dev/null || echo "No MONEI-MCP-Server/package.json"
echo -e "\n=== Git log for .node-version files ==="
git log --oneline -n 5 -- '.node-version' 'MONEI-MCP-Server/.node-version' 2>/dev/null || echo "No git history available"
echo -e "\n=== Repository structure ==="
ls -la | head -20Repository: MONEI/MONEI-MCP-Server
Length of output: 2993
🏁 Script executed:
#!/bin/bash
# Look for CI configuration
find . -maxdepth 2 -type f \( -name '*.yml' -o -name '*.yaml' \) | grep -E '(github|ci|workflow)' | head -5Repository: MONEI/MONEI-MCP-Server
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Check if MONEI-MCP-Server is a submodule or regular directory
echo "=== Check .gitmodules ==="
cat .gitmodules 2>/dev/null || echo "No .gitmodules file"
echo -e "\n=== Check if MONEI-MCP-Server is a git submodule ==="
ls -la MONEI-MCP-Server/.git 2>/dev/null || echo "Not a submodule (no .git in MONEI-MCP-Server)"
echo -e "\n=== Check engines field in both package.json files ==="
echo "Root package.json engines:"
grep -A 2 '"engines"' package.json || echo "No engines field"
echo -e "\nMONEI-MCP-Server/package.json engines:"
grep -A 2 '"engines"' MONEI-MCP-Server/package.json || echo "No engines field"
echo -e "\n=== List all files in MONEI-MCP-Server directory ==="
ls -la MONEI-MCP-Server/Repository: MONEI/MONEI-MCP-Server
Length of output: 1293
🏁 Script executed:
#!/bin/bash
# Check if MONEI-MCP-Server directory is a duplicate/copy or the actual source
echo "=== Compare file counts ==="
echo "Root directory source files (src/):"
find src -type f | wc -l
echo -e "\nMONEI-MCP-Server source files:"
find MONEI-MCP-Server -type f | wc -l
echo -e "\n=== Check git status of MONEI-MCP-Server directory ==="
git status MONEI-MCP-Server/ 2>/dev/null || echo "Directory check failed"
echo -e "\n=== Check git history for MONEI-MCP-Server directory ==="
git log --oneline -n 3 -- MONEI-MCP-Server/ 2>/dev/null | head -5Repository: MONEI/MONEI-MCP-Server
Length of output: 404
Remove the outdated MONEI-MCP-Server subdirectory—it appears to be a leftover from the v0.1.0 to v0.2.0 migration.
The MONEI-MCP-Server/.node-version file (22.14.0) conflicts with the root .node-version (20) because the subdirectory is a stale copy. It contains package.json v0.1.0 while the root specifies v0.2.0, and the migration commit "migrate REST → GraphQL, add CI/CD (v0.2.0)" indicates the root directory is the current source of truth. This directory should be removed to eliminate the version confusion and prevent accidental use of outdated code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MONEI-MCP-Server/.node-version` at line 1, Remove the stale MONEI-MCP-Server
subdirectory and its leftover files (including the subdirectory's .node-version
containing "22.14.0" and the v0.1.0 package.json) from the repository, commit
the deletion, and ensure the root .node-version (20) and root package.json
v0.2.0 remain as the single source of truth; also check and update any CI/config
references that point to MONEI-MCP-Server so they no longer reference the
removed directory.
Comment on lines
+5
to
+8
| "buildCommand": "echo 'skipping build - dist prebuilt'" | ||
| }, | ||
| "deploy": { | ||
| "startCommand": "node dist/index.js", |
There was a problem hiding this comment.
Build is skipped while startup requires dist, which can break deployment.
startCommand runs node dist/index.js, but buildCommand never creates dist. If artifacts are not externally injected, the service will fail to boot.
🔧 Proposed fix
"build": {
"builder": "NIXPACKS",
- "buildCommand": "echo 'skipping build - dist prebuilt'"
+ "buildCommand": "npm ci && npm run build"
},📝 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.
Suggested change
| "buildCommand": "echo 'skipping build - dist prebuilt'" | |
| }, | |
| "deploy": { | |
| "startCommand": "node dist/index.js", | |
| "build": { | |
| "builder": "NIXPACKS", | |
| "buildCommand": "npm ci && npm run build" | |
| }, | |
| "deploy": { | |
| "startCommand": "node dist/index.js", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MONEI-MCP-Server/railway.json` around lines 5 - 8, The deploy config uses
startCommand "node dist/index.js" but buildCommand currently skips building, so
ensure the build produces the dist artifacts or change startup to run the source
directly; specifically update buildCommand to run your project's build step
(e.g., run the package.json build script to emit dist) or alter startCommand to
point to the compiled output you actually produce (or to a runtime entry like
src/index.js) so that dist/index.js exists at deploy time; modify the
"buildCommand" or "startCommand" entries accordingly and keep names
buildCommand, startCommand, and dist/index.js in sync.
Comment on lines
+36
to
+40
| - Refund payments | ||
| - Charge cards or Bizum | ||
| - Card or Bizum payouts | ||
| - Cancel/delete subscriptions | ||
| - Modify account settings |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all restricted operations defined in the codebase
rg -n "RESTRICTED_OPERATIONS" --type=ts -A 20Repository: MONEI/MONEI-MCP-Server
Length of output: 4988
🏁 Script executed:
#!/bin/bash
# Read the SECURITY.md file
cat -n SECURITY.mdRepository: MONEI/MONEI-MCP-Server
Length of output: 2519
SECURITY.md documentation of restricted operations is incomplete and uses misleading terminology.
Lines 36-40 document only 5 restricted operations, but src/types/index.ts:31-40 defines 8:
- refundPayment — matches "Refund payments" ✓
- capturePayment — called "Charge cards or Bizum" (misleading; this is payment authorization/capture, not charging)
- cancelPayment — not documented in SECURITY.md
- cancelSubscription — matches "Cancel/delete subscriptions" ✓
- updateAccount — matches "Modify account settings" ✓
- deleteWebhook — not documented
- createApiKey — not documented
- deleteApiKey — not documented
Update SECURITY.md to list all 8 operations with accurate descriptions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MONEI-MCP-Server/SECURITY.md` around lines 36 - 40, SECURITY.md currently
lists only five operations and misnames capturePayment; update the document to
list all eight operations defined in src/types/index.ts — refundPayment,
capturePayment, cancelPayment, cancelSubscription, updateAccount, deleteWebhook,
createApiKey, deleteApiKey — and provide accurate one-line descriptions for each
(e.g., capturePayment: capture an authorized payment to finalize the charge;
cancelPayment: void or cancel an in-flight authorization; refundPayment: issue
refunds; cancelSubscription: cancel/delete subscriptions; updateAccount: modify
account settings; deleteWebhook: remove webhook endpoints; createApiKey:
generate new API keys; deleteApiKey: revoke existing API keys). Ensure the
terminology matches the function intent and replace the misleading "Charge cards
or Bizum" wording with the precise description for capturePayment.
Comment on lines
+49
to
+53
| ### Operational Security | ||
| - Per-account sliding-window rate limiting | ||
| - Structured audit logging for every tool invocation | ||
| - Session validation on all `/messages` requests | ||
| - Automatic session cleanup on disconnect |
There was a problem hiding this comment.
Documentation claims security controls that are not implemented.
The documented "Operational Security" features are not currently wired into the server:
- Rate limiting (
src/middleware/rate-limiter.ts) exists but is not integrated into the request handler insrc/server.ts - Audit logging (
src/middleware/audit-logger.ts) exists but is not called from the request handler
Per src/server.ts:42-62, handleToolCall is invoked directly without rate limit checks or audit wrapping. Either integrate these middleware into the request flow or update the documentation to reflect the actual security posture.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MONEI-MCP-Server/SECURITY.md` around lines 49 - 53, The SECURITY.md claims
rate limiting and audit logging are active but src/server.ts currently calls
handleToolCall directly; import and apply the existing middleware from
src/middleware/rate-limiter.ts (e.g., rateLimiter) and
src/middleware/audit-logger.ts (e.g., auditLogger or auditLogger.log) into the
request flow that invokes handleToolCall so requests pass through rateLimiter
and are recorded by auditLogger before/after handleToolCall, or if you opt not
to enable them, update SECURITY.md to remove or revise the Operational Security
bullets to reflect the actual implemented controls; ensure you reference the
existing symbols rateLimiter, auditLogger and handleToolCall when making the
change.
Comment on lines
+3
to
+8
| export function corsHeaders(_req: Request, res: Response, next: NextFunction): void { | ||
| res.setHeader("Access-Control-Allow-Origin", "*"); | ||
| res.setHeader("Access-Control-Allow-Methods", "GET, POST, OPTIONS"); | ||
| res.setHeader("Access-Control-Allow-Headers", "Content-Type, Authorization, X-Session-Id"); | ||
| res.setHeader("Access-Control-Max-Age", "86400"); | ||
| next(); |
There was a problem hiding this comment.
Don't open every browser-facing route to every origin.
In src/index.ts, Lines 23-28, this middleware is installed globally, so Access-Control-Allow-Origin: * makes /mcp, /messages, and the OAuth routes callable from any site. That weakens the origin boundary around browser-held session/token traffic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/middleware/security.ts` around lines 3 - 8, The corsHeaders middleware
currently sets Access-Control-Allow-Origin to "*" which opens browser-facing
routes to any origin; update the corsHeaders function to enforce a configured
whitelist (e.g., from process.env.ALLOWED_ORIGINS or app config), check the
incoming req.headers.origin and if it matches the whitelist set
Access-Control-Allow-Origin to that origin (otherwise omit the header or return
a 403 for disallowed origins), keep Access-Control-Allow-Methods/Headers and
handle OPTIONS responses as now; reference the corsHeaders middleware to locate
and replace the wildcard-origin behavior with origin validation against the
whitelist.
Comment on lines
+20
to
24
| export function bodySizeGuard(maxBytes = 512 * 1024) { | ||
| return (req: Request, res: Response, next: NextFunction): void => { | ||
| if (process.env.NODE_ENV !== "production") { | ||
| next(); | ||
| return; | ||
| } | ||
|
|
||
| const proto = req.headers["x-forwarded-proto"] ?? req.protocol; | ||
| if (proto !== "https") { | ||
| res.redirect(301, `https://${req.headers.host}${req.url}`); | ||
| return; | ||
| } | ||
|
|
||
| // HSTS header | ||
| res.setHeader( | ||
| "Strict-Transport-Security", | ||
| "max-age=63072000; includeSubDomains; preload" | ||
| ); | ||
| next(); | ||
| }; | ||
| } | ||
|
|
||
| // ─── Session Validation for /messages ────────────────────── | ||
|
|
||
| /** | ||
| * Validates that the session ID on /messages requests | ||
| * corresponds to an active, authenticated SSE connection. | ||
| */ | ||
| export function createSessionValidator( | ||
| activeSessions: Map<string, unknown> | ||
| ) { | ||
| return (req: Request, res: Response, next: NextFunction): void => { | ||
| const sessionId = req.query.sessionId as string | undefined; | ||
|
|
||
| if (!sessionId) { | ||
| res.status(400).json({ | ||
| error: "Missing sessionId parameter", | ||
| hint: "Connect via /sse first to establish a session.", | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (!activeSessions.has(sessionId)) { | ||
| res.status(401).json({ | ||
| error: "Invalid or expired session", | ||
| hint: "Your session may have expired. Reconnect via /sse.", | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| next(); | ||
| }; | ||
| } | ||
|
|
||
| // ─── Input Sanitization ──────────────────────────────────── | ||
|
|
||
| /** | ||
| * Reject oversized payloads and non-JSON content types on POST. | ||
| */ | ||
| export function inputGuard() { | ||
| return (req: Request, res: Response, next: NextFunction): void => { | ||
| if (req.method === "POST") { | ||
| const contentType = req.headers["content-type"]; | ||
| if (contentType && !contentType.includes("application/json")) { | ||
| res.status(415).json({ error: "Content-Type must be application/json" }); | ||
| return; | ||
| } | ||
| } | ||
| next(); | ||
| }; | ||
| } | ||
|
|
||
| // ─── API Key Validation (development mode) ───────────────── | ||
|
|
||
| /** | ||
| * For development/testing: validate that a MONEI API key is present. | ||
| * In production, OAuth tokens are used instead. | ||
| */ | ||
| export function requireApiKeyInDev() { | ||
| return (_req: Request, res: Response, next: NextFunction): void => { | ||
| if (process.env.NODE_ENV === "production") { | ||
| next(); | ||
| return; | ||
| } | ||
|
|
||
| if (!process.env.MONEI_API_KEY) { | ||
| console.warn( | ||
| "[Security] No MONEI_API_KEY set. Tool calls will fail. Set it in .env" | ||
| ); | ||
| } | ||
| const len = parseInt(req.headers["content-length"] || "0", 10); | ||
| if (len > maxBytes) { res.status(413).json({ error: "Request body too large" }); return; } | ||
| next(); |
There was a problem hiding this comment.
bodySizeGuard cannot stop oversized bodies in the current pipeline.
In src/index.ts, Lines 23-28, express.json() runs before this middleware, so the body has already been read into memory before Line 23 executes. This check also trusts Content-Length, which means chunked requests can bypass it entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/middleware/security.ts` around lines 20 - 24, The bodySizeGuard
middleware cannot prevent large bodies because express.json() already consumes
the stream and content-length is untrusted; instead either (A) move the
bodySizeGuard registration to run before express.json() in your middleware chain
(so bodySizeGuard sees the raw request stream) and change bodySizeGuard to count
bytes from the incoming request stream rather than trusting
req.headers["content-length"], or (B) replace the custom guard with
express.json({ limit: <maxBytes> }) so express itself enforces the limit; update
references to bodySizeGuard and ensure whichever approach you pick is applied
before the JSON body parser (express.json) in the server setup.
| return client.createPayment(input as any); | ||
| } | ||
| case "get_payment": return client.getCharge(args.paymentId as string); | ||
| case "list_payments": return client.listCharges({ search: args.search as string | undefined, status: args.status as string | undefined, size: (args.limit as number) || 20, from: (args.offset as number) || 0 }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the file structure and the specific line in question
cat -n src/tools/payments.ts | sed -n '85,105p'Repository: MONEI/MONEI-MCP-Server
Length of output: 1452
🏁 Script executed:
#!/bin/bash
# Let's also see the full tool definition around list_payments to understand the schema
cat -n src/tools/payments.ts | sed -n '1,100p'Repository: MONEI/MONEI-MCP-Server
Length of output: 6498
Preserve explicit limit: 0 in list_payments.
Line 95 uses (args.limit as number) || 20, so if a caller explicitly passes limit: 0, it will be treated as falsy and replaced with 20. This changes the requested page size and returns a different number of payments than requested.
Suggested fix
- case "list_payments": return client.listCharges({ search: args.search as string | undefined, status: args.status as string | undefined, size: (args.limit as number) || 20, from: (args.offset as number) || 0 });
+ case "list_payments": return client.listCharges({ search: args.search as string | undefined, status: args.status as string | undefined, size: (args.limit as number | undefined) ?? 20, from: (args.offset as number | undefined) ?? 0 });📝 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.
Suggested change
| case "list_payments": return client.listCharges({ search: args.search as string | undefined, status: args.status as string | undefined, size: (args.limit as number) || 20, from: (args.offset as number) || 0 }); | |
| case "list_payments": return client.listCharges({ search: args.search as string | undefined, status: args.status as string | undefined, size: (args.limit as number | undefined) ?? 20, from: (args.offset as number | undefined) ?? 0 }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/payments.ts` at line 95, The call in the "list_payments" case uses
the `||` operator so explicit zero values are treated as falsy and replaced
(e.g., `(args.limit as number) || 20`), losing an intentional `limit: 0`; change
these to use nullish coalescing or an explicit undefined check (e.g.,
`(args.limit as number) ?? 20` or `args.limit !== undefined ? args.limit : 20`)
and do the same for `args.offset` so `client.listCharges` receives a preserved 0
when callers pass it.
| ): Promise<unknown> { | ||
| switch (toolName) { | ||
| case "get_subscription": return client.getSubscription(args.subscriptionId as string); | ||
| case "list_subscriptions": return client.listSubscriptions({ search: args.search as string | undefined, status: args.status as string | undefined, size: (args.limit as number) || 20, from: (args.offset as number) || 0 }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the file structure and read the relevant section
head -n 40 src/tools/subscriptions.ts | cat -nRepository: MONEI/MONEI-MCP-Server
Length of output: 2141
Use nullish coalescing (??) instead of logical OR (||) to preserve explicit zero values.
Line 35 uses (args.limit as number) || 20 and (args.offset as number) || 0. This means a caller-supplied limit: 0 is silently rewritten to 20, changing the caller's explicit intent. The schema defines no minimum constraint for these fields, so 0 is a valid value that should be preserved.
Suggested fix
- case "list_subscriptions": return client.listSubscriptions({ search: args.search as string | undefined, status: args.status as string | undefined, size: (args.limit as number) || 20, from: (args.offset as number) || 0 });
+ case "list_subscriptions": return client.listSubscriptions({ search: args.search as string | undefined, status: args.status as string | undefined, size: (args.limit as number | undefined) ?? 20, from: (args.offset as number | undefined) ?? 0 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/subscriptions.ts` at line 35, The return in the
"list_subscriptions" case uses logical OR which overwrites explicit zero values;
update the arguments passed to client.listSubscriptions so that size uses
nullish coalescing for args.limit and from uses nullish coalescing for
args.offset (i.e., keep explicit 0 from args.limit/args.offset but fall back to
20 and 0 only when those fields are null or undefined), referencing the
"list_subscriptions" case and the args.limit/args.offset parameters used for
size and from.
Comment on lines
+42
to
+55
| it("withAudit wraps successful calls", async () => { | ||
| const spy = vi.spyOn(console, "log").mockImplementation(() => {}); | ||
| const result = await withAudit("get_account_info", {}, "acc_1", async () => ({ name: "Test" })); | ||
| expect(result).toEqual({ name: "Test" }); | ||
| spy.mockRestore(); | ||
| }); | ||
|
|
||
| it("withAudit logs errors and re-throws", async () => { | ||
| const spy = vi.spyOn(console, "log").mockImplementation(() => {}); | ||
| await expect( | ||
| withAudit("get_payment", { paymentId: "bad" }, "acc_1", async () => { throw new Error("Not found"); }) | ||
| ).rejects.toThrow("Not found"); | ||
| spy.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
withAudit tests are false positives for audit behavior.
Both cases validate return/throw but never assert that an audit record was actually emitted. The tests would still pass if logging is removed or broken.
🔧 Proposed fix
it("withAudit wraps successful calls", async () => {
const spy = vi.spyOn(console, "log").mockImplementation(() => {});
const result = await withAudit("get_account_info", {}, "acc_1", async () => ({ name: "Test" }));
expect(result).toEqual({ name: "Test" });
+ expect(spy).toHaveBeenCalledTimes(1);
+ const logged = JSON.parse(spy.mock.calls[0][0] as string);
+ expect(logged.type).toBe("audit");
+ expect(logged.tool).toBe("get_account_info");
+ expect(logged.success).toBe(true);
spy.mockRestore();
});
it("withAudit logs errors and re-throws", async () => {
const spy = vi.spyOn(console, "log").mockImplementation(() => {});
await expect(
withAudit("get_payment", { paymentId: "bad" }, "acc_1", async () => { throw new Error("Not found"); })
).rejects.toThrow("Not found");
+ expect(spy).toHaveBeenCalledTimes(1);
+ const logged = JSON.parse(spy.mock.calls[0][0] as string);
+ expect(logged.type).toBe("audit");
+ expect(logged.tool).toBe("get_payment");
+ expect(logged.success).toBe(false);
+ expect(logged.error).toBe("Not found");
spy.mockRestore();
});As per coding guidelines, "**/*.test.ts: Only comment on: tests that would pass when the code is broken (false positives), or tests with actual runtime errors."
📝 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.
Suggested change
| it("withAudit wraps successful calls", async () => { | |
| const spy = vi.spyOn(console, "log").mockImplementation(() => {}); | |
| const result = await withAudit("get_account_info", {}, "acc_1", async () => ({ name: "Test" })); | |
| expect(result).toEqual({ name: "Test" }); | |
| spy.mockRestore(); | |
| }); | |
| it("withAudit logs errors and re-throws", async () => { | |
| const spy = vi.spyOn(console, "log").mockImplementation(() => {}); | |
| await expect( | |
| withAudit("get_payment", { paymentId: "bad" }, "acc_1", async () => { throw new Error("Not found"); }) | |
| ).rejects.toThrow("Not found"); | |
| spy.mockRestore(); | |
| }); | |
| it("withAudit wraps successful calls", async () => { | |
| const spy = vi.spyOn(console, "log").mockImplementation(() => {}); | |
| const result = await withAudit("get_account_info", {}, "acc_1", async () => ({ name: "Test" })); | |
| expect(result).toEqual({ name: "Test" }); | |
| expect(spy).toHaveBeenCalledTimes(1); | |
| const logged = JSON.parse(spy.mock.calls[0][0] as string); | |
| expect(logged.type).toBe("audit"); | |
| expect(logged.tool).toBe("get_account_info"); | |
| expect(logged.success).toBe(true); | |
| spy.mockRestore(); | |
| }); | |
| it("withAudit logs errors and re-throws", async () => { | |
| const spy = vi.spyOn(console, "log").mockImplementation(() => {}); | |
| await expect( | |
| withAudit("get_payment", { paymentId: "bad" }, "acc_1", async () => { throw new Error("Not found"); }) | |
| ).rejects.toThrow("Not found"); | |
| expect(spy).toHaveBeenCalledTimes(1); | |
| const logged = JSON.parse(spy.mock.calls[0][0] as string); | |
| expect(logged.type).toBe("audit"); | |
| expect(logged.tool).toBe("get_payment"); | |
| expect(logged.success).toBe(false); | |
| expect(logged.error).toBe("Not found"); | |
| spy.mockRestore(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/middleware/middleware.test.ts` around lines 42 - 55, The tests for
withAudit currently only check return/throw and not that an audit record was
emitted; update both specs to assert the audit logging was invoked: in the
"withAudit wraps successful calls" test, after calling
withAudit("get_account_info", ..., async () => ({ name: "Test" })), assert the
console (or the audit logger) spy was called with an entry containing the action
"get_account_info", the account id "acc_1", the input/context object, and the
successful result; in the "withAudit logs errors and re-throws" test, after
expecting the rejection for withAudit("get_payment", { paymentId: "bad" },
"acc_1", ...), assert the console/audit spy was called with an entry containing
action "get_payment", account "acc_1", the context payload, and the error
message "Not found". Ensure you keep the existing spy setup/restore
(vi.spyOn(console, "log").mockImplementation(...)) and use its call assertions
to validate the audit payload rather than only checking return values.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
README.md (1)
172-173:⚠️ Potential issue | 🟡 MinorMiddleware labels don’t match current implementation names.
security.tsnow exposesbodySizeGuard; “input guard” is stale. Keep these labels in sync with actual exported symbols.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 172 - 173, Update the README middleware labels to match the actual exported symbols: replace the stale "input guard" label with the current name(s) exported from security.ts (e.g., bodySizeGuard) so the README reflects the real implementation; ensure the line listing middleware (currently showing "security.ts # CORS, security headers, input guard") is changed to something like "security.ts # CORS, security headers, bodySizeGuard" or include all exported middleware names from security.ts to keep docs and code in sync.
🧹 Nitpick comments (2)
.env.example (1)
6-7: ReorderHOSTandPORTto satisfy dotenv-linter.
dotenv-linterreportsUnorderedKeyhere. Swap these two lines to keep lint output clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example around lines 6 - 7, The .env example has keys out of alphabetical order causing dotenv-linter's UnorderedKey; swap the two environment keys so HOST appears before PORT in the file (update the lines containing the HOST and PORT entries so they are alphabetically ordered: HOST then PORT).README.md (1)
16-18: Add language identifiers to fenced code blocks (MD040).These fenced blocks are missing language tags and trigger markdownlint warnings.
Suggested markdown fix
-``` +```text MCP Server URL: https://mcp.monei.com/mcp-
+text
You: "Generate a €25 payment link for order#1234— customer is alex@example.com"
...-``` +```text src/ ...</details> Also applies to: 26-46, 156-177 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@README.mdaround lines 16 - 18, Add language identifiers to the fenced code
blocks in README.md: update the triple-backtick blocks (including the block
containing "MCP Server URL: https://mcp.monei.com/mcp" and the other example
blocks such as the conversation example, the src/ snippet, and the blocks in the
ranges referenced) to include an appropriate language tag (e.g., text, bash,
json) immediately after the opening ``` so markdownlint MD040 is satisfied;
ensure each fenced block uses the most specific tag that matches its contents.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@Dockerfile:
- Around line 36-37: The HEALTHCHECK currently hardcodes localhost:3000; update
the HEALTHCHECK instruction so it runs in a shell (use the shell form of CMD)
and constructs the health URL using the runtime PORT environment variable with a
fallback to 3000, e.g., have the shell read $PORT (or default to 3000) and call
/health (use wget or curl) so the check honors the container's PORT setting;
edit the HEALTHCHECK line that currently uses wget and localhost:3000
accordingly.In
@README.md:
- Line 59: The README table incorrectly lists the KPI tool as
chargesDateRangeKPI; update the table entry to use the actual exported tool name
get_payments_kpi to match the implementation (get_payments_kpi) so docs and code
align.- Around line 133-142: The README table lists blocked operations that don't
match the actual enforced server-side blocklist; update the README table to
reflect the exact entries in the RESTRICTED_OPERATIONS constant (refer to the
RESTRICTED_OPERATIONS export in src/types/index.ts) so the docs mirror the code,
or if the intention was to block the extra operations (e.g., direct
charge/payout names), add those exact operation names to the
RESTRICTED_OPERATIONS array instead; ensure the README wording and the
RESTRICTED_OPERATIONS symbol remain consistent and use the same operation
identifiers.In
@SECURITY.md:
- Line 16: The SECURITY.md sentence lists “charges” and “payouts” as
hard-blocked categories but the canonical enforced list is the
RESTRICTED_OPERATIONS set; update the sentence to exactly reflect the operations
named in RESTRICTED_OPERATIONS (referencing the constant in src/types/index.ts)
— replace the current category words with the concrete operation names from
RESTRICTED_OPERATIONS or state that the hard-blocked actions are "the exact
operations listed in RESTRICTED_OPERATIONS (see src/types/index.ts)" so the doc
matches the enforced blocklist.- Line 31: The SECURITY.md claim about rate limiting is inaccurate because
checkRateLimit from src/middleware/rate-limiter.ts is never wired into
production paths; either remove the "Per-account sliding window" statement from
SECURITY.md or integrate the middleware by importing checkRateLimit and
registering it on the Express app routes (e.g., the /mcp and /sse handlers)
and/or invoking it inside handleToolCall in src/tools/index.ts so tool calls and
SSE streams enforce the per-account 60 req/min sliding window; update or keep
existing tests to reflect the chosen approach.
Duplicate comments:
In@README.md:
- Around line 172-173: Update the README middleware labels to match the actual
exported symbols: replace the stale "input guard" label with the current name(s)
exported from security.ts (e.g., bodySizeGuard) so the README reflects the real
implementation; ensure the line listing middleware (currently showing
"security.ts # CORS, security headers, input guard") is changed to something
like "security.ts # CORS, security headers, bodySizeGuard" or include all
exported middleware names from security.ts to keep docs and code in sync.
Nitpick comments:
In @.env.example:
- Around line 6-7: The .env example has keys out of alphabetical order causing
dotenv-linter's UnorderedKey; swap the two environment keys so HOST appears
before PORT in the file (update the lines containing the HOST and PORT entries
so they are alphabetically ordered: HOST then PORT).In
@README.md:
- Around line 16-18: Add language identifiers to the fenced code blocks in
README.md: update the triple-backtick blocks (including the block containing
"MCP Server URL: https://mcp.monei.com/mcp" and the other example blocks such as
the conversation example, the src/ snippet, and the blocks in the ranges
referenced) to include an appropriate language tag (e.g., text, bash, json)
immediately after the opening ``` so markdownlint MD040 is satisfied; ensure
each fenced block uses the most specific tag that matches its contents.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `b7bad4e4-504f-4453-87e7-760ca65a496a` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 58b051a9ba13be023d294c452532788fd8522334 and 0e7db8be8301b01a32c174b84b183c7b23d15408. </details> <details> <summary>⛔ Files ignored due to path filters (56)</summary> * `dist/api/monei-client.d.ts` is excluded by `!**/dist/**` * `dist/api/monei-client.d.ts.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/api/monei-client.js` is excluded by `!**/dist/**` * `dist/api/monei-client.js.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/auth/oauth.d.ts` is excluded by `!**/dist/**` * `dist/auth/oauth.d.ts.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/auth/oauth.js` is excluded by `!**/dist/**` * `dist/auth/oauth.js.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/auth/pkce.d.ts` is excluded by `!**/dist/**` * `dist/auth/pkce.d.ts.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/auth/pkce.js` is excluded by `!**/dist/**` * `dist/auth/pkce.js.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/auth/session.d.ts` is excluded by `!**/dist/**` * `dist/auth/session.d.ts.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/auth/session.js` is excluded by `!**/dist/**` * `dist/auth/session.js.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/index.d.ts` is excluded by `!**/dist/**` * `dist/index.d.ts.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/index.js` is excluded by `!**/dist/**` * `dist/index.js.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/middleware/audit-logger.d.ts` is excluded by `!**/dist/**` * `dist/middleware/audit-logger.d.ts.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/middleware/audit-logger.js` is excluded by `!**/dist/**` * `dist/middleware/audit-logger.js.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/middleware/rate-limiter.d.ts` is excluded by `!**/dist/**` * `dist/middleware/rate-limiter.d.ts.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/middleware/rate-limiter.js` is excluded by `!**/dist/**` * `dist/middleware/rate-limiter.js.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/middleware/security.d.ts` is excluded by `!**/dist/**` * `dist/middleware/security.d.ts.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/middleware/security.js` is excluded by `!**/dist/**` * `dist/middleware/security.js.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/server.d.ts` is excluded by `!**/dist/**` * `dist/server.d.ts.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/server.js` is excluded by `!**/dist/**` * `dist/server.js.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/tools/account.d.ts` is excluded by `!**/dist/**` * `dist/tools/account.d.ts.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/tools/account.js` is excluded by `!**/dist/**` * `dist/tools/account.js.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/tools/index.d.ts` is excluded by `!**/dist/**` * `dist/tools/index.d.ts.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/tools/index.js` is excluded by `!**/dist/**` * `dist/tools/index.js.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/tools/payments.d.ts` is excluded by `!**/dist/**` * `dist/tools/payments.d.ts.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/tools/payments.js` is excluded by `!**/dist/**` * `dist/tools/payments.js.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/tools/subscriptions.d.ts` is excluded by `!**/dist/**` * `dist/tools/subscriptions.d.ts.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/tools/subscriptions.js` is excluded by `!**/dist/**` * `dist/tools/subscriptions.js.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/types/index.d.ts` is excluded by `!**/dist/**` * `dist/types/index.d.ts.map` is excluded by `!**/dist/**`, `!**/*.map` * `dist/types/index.js` is excluded by `!**/dist/**` * `dist/types/index.js.map` is excluded by `!**/dist/**`, `!**/*.map` </details> <details> <summary>📒 Files selected for processing (7)</summary> * `.dockerignore` * `.env.example` * `CHANGELOG.md` * `CONTRIBUTING.md` * `Dockerfile` * `README.md` * `SECURITY.md` </details> <details> <summary>✅ Files skipped from review due to trivial changes (3)</summary> * .dockerignore * CONTRIBUTING.md * CHANGELOG.md </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+36
to
+37
| HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=3 \ | ||
| CMD wget -qO- http://localhost:3000/health || exit 1 |
There was a problem hiding this comment.
Make healthcheck honor runtime PORT instead of hardcoding 3000.
Line [37] always checks localhost:3000, but the app reads PORT from env. If PORT is overridden, the container can be marked unhealthy even when the server is fine.
🔧 Proposed fix
-HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=3 \
- CMD wget -qO- http://localhost:3000/health || exit 1
+HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=3 \
+ CMD sh -c 'wget -qO- "http://127.0.0.1:${PORT}/health" >/dev/null || exit 1'📝 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.
Suggested change
| HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=3 \ | |
| CMD wget -qO- http://localhost:3000/health || exit 1 | |
| HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=3 \ | |
| CMD sh -c 'wget -qO- "http://127.0.0.1:${PORT}/health" >/dev/null || exit 1' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 36 - 37, The HEALTHCHECK currently hardcodes
localhost:3000; update the HEALTHCHECK instruction so it runs in a shell (use
the shell form of CMD) and constructs the health URL using the runtime PORT
environment variable with a fallback to 3000, e.g., have the shell read $PORT
(or default to 3000) and call /health (use wget or curl) so the check honors the
container's PORT setting; edit the HEALTHCHECK line that currently uses wget and
localhost:3000 accordingly.
| | **API** | GraphQL native (AppSync) | REST | REST | | ||
| | **Open source** | ✅ MIT | Partial | ❌ | | ||
| | **Safety guardrails** | Hard-blocked restricted ops | Configurable | Configurable | | ||
| | **Analytics built-in** | `chargesDateRangeKPI` tool | Separate API | Separate API | |
There was a problem hiding this comment.
Use the actual KPI tool name (get_payments_kpi).
The table references chargesDateRangeKPI but the implemented tool is get_payments_kpi (src/tools/payments.ts:43-58). Please align docs with the exported tool name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 59, The README table incorrectly lists the KPI tool as
chargesDateRangeKPI; update the table entry to use the actual exported tool name
get_payments_kpi to match the implementation (get_payments_kpi) so docs and code
align.
Comment on lines
+133
to
+142
| | Blocked Operation | Why | | ||
| |---|---| | ||
| | Refund payments | Financial risk — requires manual review | | ||
| | Capture/cancel payments | Requires authorization context | | ||
| | Charge cards/Bizum directly | Requires PCI context + cardholder consent | | ||
| | Card/Bizum payouts | Funds disbursement needs compliance controls | | ||
| | Cancel subscriptions | Destructive — affects recurring revenue | | ||
| | Modify account settings | Security-sensitive configuration | | ||
| | Create/delete API keys | Credential management | | ||
| | Delete webhooks | Can break integrations | |
There was a problem hiding this comment.
Blocked-operations table is broader than the enforced blocklist.
This section lists direct charge and payout blocks, but server-side enforced restrictions are the entries in RESTRICTED_OPERATIONS (src/types/index.ts:28-46), which currently do not include explicit charge/payout operation names. Please align this table with the real enforced operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 133 - 142, The README table lists blocked operations
that don't match the actual enforced server-side blocklist; update the README
table to reflect the exact entries in the RESTRICTED_OPERATIONS constant (refer
to the RESTRICTED_OPERATIONS export in src/types/index.ts) so the docs mirror
the code, or if the intention was to block the extra operations (e.g., direct
charge/payout names), add those exact operation names to the
RESTRICTED_OPERATIONS array instead; ensure the README wording and the
RESTRICTED_OPERATIONS symbol remain consistent and use the same operation
identifiers.
|
|
||
| ### Restricted Operations | ||
| The following operations are **hard-blocked at the server level** and cannot be executed through the MCP server under any circumstances: | ||
| Refunds, charges, payouts, subscription cancellations, and account modifications are **hard-blocked at the server level**. Even manually crafted tool calls are rejected with a clear message pointing to the [MONEI Dashboard](https://dashboard.monei.com). This is not configurable — it's enforced in code. |
There was a problem hiding this comment.
Restricted-action statement should match the enforced blocklist exactly.
This sentence includes “charges” and “payouts” as hard-blocked categories, but the enforced server list is the explicit RESTRICTED_OPERATIONS set in src/types/index.ts:28-46. Please align wording to those concrete operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SECURITY.md` at line 16, The SECURITY.md sentence lists “charges” and
“payouts” as hard-blocked categories but the canonical enforced list is the
RESTRICTED_OPERATIONS set; update the sentence to exactly reflect the operations
named in RESTRICTED_OPERATIONS (referencing the constant in src/types/index.ts)
— replace the current category words with the concrete operation names from
RESTRICTED_OPERATIONS or state that the hard-blocked actions are "the exact
operations listed in RESTRICTED_OPERATIONS (see src/types/index.ts)" so the doc
matches the enforced blocklist.
| - **Body size limits** — Requests over 512KB are rejected | ||
|
|
||
| ### Monitoring | ||
| - **Rate limiting** — Per-account sliding window (60 req/min) prevents abuse |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for checkRateLimit() call sites..."
rg -n -C3 '\bcheckRateLimit\s*\('
echo
echo "Searching for rate-limiter imports..."
rg -n -C3 'from .*rate-limiter|require\(.*rate-limiter'
echo
echo "Searching for middleware wiring in server entry points..."
rg -n -C3 'app\.use|handleToolCall|/mcp|/sse|/oauth' srcRepository: MONEI/MONEI-MCP-Server
Length of output: 8060
Remove the rate limiting claim from SECURITY.md or integrate rate limiting into the request/tool path.
The rate limiter is defined in src/middleware/rate-limiter.ts and has tests, but it is not actually wired into the codebase. The checkRateLimit() function is never imported or called in any production code—only in tests. It is not registered as middleware on the Express app and not invoked during tool calls. Either integrate the rate limiter into the MCP request handlers (e.g., in the /mcp and /sse endpoints, or within handleToolCall() in src/tools/index.ts) or remove the rate limiting claim from SECURITY.md to avoid misleading users about enforced security controls.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SECURITY.md` at line 31, The SECURITY.md claim about rate limiting is
inaccurate because checkRateLimit from src/middleware/rate-limiter.ts is never
wired into production paths; either remove the "Per-account sliding window"
statement from SECURITY.md or integrate the middleware by importing
checkRateLimit and registering it on the Express app routes (e.g., the /mcp and
/sse handlers) and/or invoking it inside handleToolCall in src/tools/index.ts so
tool calls and SSE streams enforce the per-account 60 req/min sliding window;
update or keep existing tests to reflect the chosen approach.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Infrastructure
Documentation
Tests