feat: Add ALLOW_WRITE_OPERATIONS environment variable for write operation control#29
feat: Add ALLOW_WRITE_OPERATIONS environment variable for write operation control#29
Conversation
Add AllowWriteOperations field to Config struct and parse the ALLOW_WRITE_OPERATIONS environment variable to control whether write operations are exposed. Defaults to false for security. Relates to #3
Only register create_order and cancel_order tools when AllowWriteOperations is true. Add logging to indicate whether write operations are enabled or disabled. Relates to #3
Add comprehensive test cases for the AllowWriteOperations flag, covering all valid input values (true, 1, yes) and default behavior when the environment variable is not set. Relates to #3
Add tests to verify that create_order and cancel_order tools are only registered when AllowWriteOperations is true. Also add dedicated test function for write operations control. Relates to #3
Add comprehensive documentation about the write operations control feature, including: - Explanation of default read-only mode - How to enable write operations - Examples for Docker and VS Code configuration - Updated tools table to show which tools require write operations - Added security best practice for using read-only mode by default Relates to #3
Add example configuration for the write operations control feature with clear documentation about default behavior and how to enable it. Relates to #3
WalkthroughThis change introduces a new environment variable, Assessment against linked issues
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
coderabbitai
bot
requested review from
adamhicks,
andrewwormald,
pkachelhoffer and
shivanluno
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
.env.example (1)
12-16: Clarify the default for disabled write operationsHaving
# ALLOW_WRITE_OPERATIONS=falsesuggests uncommenting would set the variable to"false", whereas omitting the variable entirely already keeps the server in read-only mode.
To avoid confusion you might delete the=falseor add a note that leaving the line commented is equivalent.internal/config/config.go (1)
95-106: Factor repeated “truthy env-var” parsing into a helperThe write-ops block duplicates the earlier debug-mode logic. Extracting a small helper such as
parseBoolEnv(key string) boolwould remove repetition and guarantee consistent behaviour going forward.-allowWriteOps := false -if writeOpsEnv := os.Getenv(strings.TrimSpace(EnvAllowWriteOperations)); writeOpsEnv != "" { - allowWriteOps = strings.ToLower(writeOpsEnv) == "true" || - writeOpsEnv == "1" || - strings.ToLower(writeOpsEnv) == "yes" - if allowWriteOps { - fmt.Println("Write operations enabled via environment variable") - } -} +allowWriteOps := parseBoolEnv(EnvAllowWriteOperations) +if allowWriteOps { + fmt.Println("Write operations enabled via environment variable") +}This keeps
Loadfocused on wiring while centralising the normalisation logic.README.md (1)
206-216: Remove trailing punctuation to satisfy markdown-lintHeadings ending with a colon trigger
MD026. Removing the colon keeps the meaning intact and silences CI noise.-### Write Operations Control +### Write Operations Control…and likewise for the “Docker Example” / “VS Code Configuration” sub-headings.
internal/config/config_test.go (2)
152-193: Consider table-driven sub-tests for truthy parsingThe write-ops cases dominate the test table and obscure unrelated scenarios. Splitting them into a dedicated
TestLoad_WriteOps(or looping over[]string{"true","1","yes"}inside a single sub-test) would improve readability and isolate intent, while still exercising the same code paths.
232-234: Missing negative assertion messageIf this check fails you only learn that the values differ, not which env-var triggered it. Append the test case name or
tc.allowWriteOpsEnvto the error for quicker diagnosis.-t.Errorf("Expected AllowWriteOperations to be %v, got %v", tc.expectedAllowWriteOps, cfg.AllowWriteOperations) +t.Errorf("%s: expected AllowWriteOperations=%v, got %v", + tc.name, tc.expectedAllowWriteOps, cfg.AllowWriteOperations)internal/server/server_test.go (1)
181-195: Hard-coded error substrings are brittle across OS/network stacksError strings such as
"lookup tcp/address: unknown port"and"bind: permission denied"vary between operating systems and Go versions. The test may become flaky on CI runners with different kernels or privilege levels.Mitigation:
- errorMsg: "lookup tcp/address: unknown port", + errorMsg: "unknown port", ... - errorMsg: "bind: permission denied", + errorMsg: "permission denied",Or match with
regexp.MatchStringfor portability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.env.example(1 hunks)README.md(2 hunks)internal/config/config.go(3 hunks)internal/config/config_test.go(4 hunks)internal/server/server.go(1 hunks)internal/server/server_test.go(5 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
217-217: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
226-226: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-publish
🔇 Additional comments (3)
internal/server/server.go (1)
73-83: Conditional registration looks goodThe guard cleanly prevents exposure of state-changing endpoints when
AllowWriteOperationsis disabled. Nice use of structured logging for observability.README.md (1)
50-60: Good addition of “Requires Write Operations” columnThis makes the security posture explicit for users.
internal/server/server_test.go (1)
191-194: Using privileged port 80 assumes non-root executionBinding to port 80 only fails when the test process lacks root privileges; otherwise it will hang or pass. Prefer an unprivileged port already in use—for example, start a dummy listener on a random free port, then attempt to bind to it again—to make the failure deterministic.
| for _, tc := range tests { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| lunoClient := luno.NewClient() | ||
| cfg := &config.Config{ | ||
| LunoClient: lunoClient, | ||
| AllowWriteOperations: tc.allowWriteOps, | ||
| } | ||
|
|
||
| server := NewMCPServer("test-write-ops", "1.0.0", cfg) | ||
| require.NotNil(t, server, "NewMCPServer should return non-nil server") | ||
|
|
||
| // Register tools | ||
| registerTools(server, cfg) | ||
|
|
||
| // Note: In a real test, we would verify that the tools are registered correctly | ||
| // by checking the server's tool registry. Since the MCP server doesn't expose | ||
| // this directly, we're demonstrating the test structure here. | ||
| // In practice, you might: | ||
| // 1. Use reflection to inspect the server's internal state | ||
| // 2. Add a method to the server to list registered tools | ||
| // 3. Test by attempting to call the tools and checking for errors | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
TestWriteOperationsControl currently provides no verification
The test only exercises the happy path of server creation and calls registerTools without checking the presence/absence of create_order and cancel_order. This will silently pass even if the feature regresses.
Consider asserting by:
- Exposing
ListTools()on the server for testing. - Using reflection to inspect the private registry under a
//go:build testtag.
Without a real assertion the test offers no value and increases maintenance cost.
🤖 Prompt for AI Agents
In internal/server/server_test.go around lines 149 to 170, the test
TestWriteOperationsControl currently does not verify whether the tools
create_order and cancel_order are correctly registered or not, which means it
can pass silently even if the feature breaks. To fix this, add a method
ListTools() to the server that returns the registered tools, or use reflection
under a //go:build test tag to inspect the private tool registry. Then, in the
test, assert the presence or absence of create_order and cancel_order tools
based on the allowWriteOps flag to ensure the test validates the intended
behavior.
| allowWriteOps bool | ||
| expectedToolCount int | ||
| }{ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
expectedToolCount is computed but never asserted
expectedToolCount is set in every test-case but no assertion uses it later on, effectively making it dead code and giving a false sense of coverage. Either remove the field or, preferably, expose (or temporarily introspect via reflection) the server’s tool registry so that the value can be verified.
🤖 Prompt for AI Agents
In internal/server/server_test.go around lines 30 to 32, the field
expectedToolCount is set in test cases but never asserted, making it dead code.
To fix this, add assertions in the tests that compare expectedToolCount with the
actual count of tools in the server's tool registry. If the tool registry is not
accessible, modify the server code to expose it or use reflection temporarily to
access it for verification.
| registerResources(server, cfg) | ||
| registerTools(server, cfg) | ||
|
|
||
| // Verify that the correct number of tools are registered | ||
| // Note: This assumes the server exposes a way to count tools. | ||
| // Since the actual implementation doesn't expose this, we'll skip this assertion for now. | ||
| // In a real implementation, you might expose a method or use reflection to verify. | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Commented-out assertion hides an unimplemented test
The block explicitly states that the assertion is skipped because the server does not expose the required information. Commented-out code in tests quickly becomes stale. Replace it with an actual assertion (using a helper to count tools via reflection) or delete the unused code altogether to keep the suite clean.
- // Verify that the correct number of tools are registered
- // Note: This assumes the server exposes a way to count tools.
- // Since the actual implementation doesn't expose this, we'll skip this assertion for now.
- // In a real implementation, you might expose a method or use reflection to verify.
+ toolCount := internaltest.CountRegisteredTools(server) // helper you expose in _test build
+ require.Equal(t, tc.expectedToolCount, toolCount, "unexpected number of registered tools")Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/server/server_test.go around lines 117 to 124, the test contains
commented-out code for asserting the number of registered tools, which is
currently skipped due to lack of server support. To fix this, either implement a
helper function that uses reflection to count the registered tools and add an
actual assertion verifying the count, or remove the commented-out assertion code
entirely to keep the test clean and maintainable.
|
Will follow up with manual testing and check of from the list shortly |
| | `get_transaction` | Transactions | Get details of a specific transaction | | ||
| | Tool | Category | Description | Requires Write Operations | | ||
| | ------------------- | ------------------- | ------------------------------------------------- | ------------------------- | | ||
| | `get_ticker` | Market Data | Get current ticker information for a trading pair | No | |
There was a problem hiding this comment.
I have a slight preference for ✅ and ❌ over yes and no, but not that important to change 😄
|
|
||
| ### Write Operations Control | ||
|
|
||
| By default, the MCP server runs in **read-only mode** with write operations (`create_order` and `cancel_order`) disabled for security. To enable write operations, you must explicitly set the `ALLOW_WRITE_OPERATIONS` environment variable. |
There was a problem hiding this comment.
Sorry I know I put ALLOW_WRITE_OPERATIONS in the issue (#3), but I'm just thinking about whether the OPERATIONS is implicit, and whether we can shorted to just ALLOW_WRITE? 🤔
I guess https://github.com/benborla/mcp-server-mysql/ uses:
"ALLOW_INSERT_OPERATION": "false",
"ALLOW_UPDATE_OPERATION": "false",
"ALLOW_DELETE_OPERATION": "false"So maybe this is better to leave as is
There was a problem hiding this comment.
Think I am also in favor more than just ALLOW_WRITE. Thinking maybe we go for ALLOW_WRITE_TOOLS instead? 🤔
There was a problem hiding this comment.
I think users of our MCP server might not necessarily know what a tool is, if they are new to MCP.
Maybe we just stick with ALLOW_WRITE_OPERATIONS! I'm torn between that and ALLOW_WRITE
| ##### Docker Example: | ||
| ```bash | ||
| docker run --rm -i \ | ||
| -e "LUNO_API_KEY_ID=${LUNO_API_KEY_ID}" \ | ||
| -e "LUNO_API_SECRET=${LUNO_API_SECRET}" \ | ||
| -e "ALLOW_WRITE_OPERATIONS=true" \ | ||
| ghcr.io/luno/luno-mcp:latest | ||
| ``` | ||
|
|
||
| ##### VS Code Configuration: | ||
| ```json | ||
| { | ||
| "servers": { | ||
| "luno-docker": { | ||
| "command": "docker", | ||
| "args": [ | ||
| "run", "--rm", "-i", | ||
| "-e", "LUNO_API_KEY_ID=${input:luno_api_key_id}", | ||
| "-e", "LUNO_API_SECRET=${input:luno_api_secret}", | ||
| "-e", "ALLOW_WRITE_OPERATIONS=true", | ||
| "ghcr.io/luno/luno-mcp:latest" | ||
| ], | ||
| "inputs": [ | ||
| {"id": "luno_api_key_id", "type": "promptString", "description": "Luno API Key ID", "password": true}, | ||
| {"id": "luno_api_secret", "type": "promptString", "description": "Luno API Secret", "password": true} | ||
| ] | ||
| } | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Rather than adding another example, I think we can add this new flag to the existing config declarations above. The flag explanation would probably be best to live there too 👍
There was a problem hiding this comment.
I hear you, thought it best there because it speaks more to what this is which is Security Considerations. That said, could move the example up and just have the paragraph explaining the consideration.
There was a problem hiding this comment.
I think it's just a lot easier for users to read if we have a single config for each, with all the options. If users see the config above this one, they might not even know that ALLOW_WRITE_OPERATIONS is a thing.
Also want to keep the README as concise as possible
|
NB: Need to add the flag to the command line options |
2 participants
Summary
ALLOW_WRITE_OPERATIONSenvironment variable to control whether write operations (create_orderandcancel_order) are exposedtrue,1, oryesChanges Made
AllowWriteOperationsfield to theConfigstruct.env.examplewith the new environment variableTest Plan
Closes #3
Summary by CodeRabbit