Skip to content

feat: Add ALLOW_WRITE_OPERATIONS environment variable for write operation control #29

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
solethus wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat: Add ALLOW_WRITE_OPERATIONS environment variable for write operation control #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9d2beb6
feat(config): Add ALLOW_WRITE_OPERATIONS configuration
solethus Jun 18, 2025
198eac7
feat(server): Conditionally register write operation tools
solethus Jun 18, 2025
70403da
test(config): Add tests for ALLOW_WRITE_OPERATIONS configuration
solethus Jun 18, 2025
7dece45
test(server): Add tests for conditional write operations registration
solethus Jun 18, 2025
fdc6dfd
docs(README): Document ALLOW_WRITE_OPERATIONS feature
solethus Jun 18, 2025
6cefa05
docs(.env.example): Add ALLOW_WRITE_OPERATIONS environment variable
solethus Jun 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .env.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,8 @@ LUNO_API_SECRET=your_api_secret
# Optional: Enable debug mode (outputs additional API information)
# Set to "true", "1", or "yes" to enable
# LUNO_API_DEBUG=false

# Optional: Enable write operations (create_order, cancel_order)
# By default, the server runs in read-only mode for security
# Set to "true", "1", or "yes" to enable write operations
# ALLOW_WRITE_OPERATIONS=false
65 changes: 54 additions & 11 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,17 @@ The server requires your Luno API key and secret. These can be obtained from you

## Available Tools

| Tool | Category | Description |
| ------------------- | ------------------- | ------------------------------------------------- |
| `get_ticker` | Market Data | Get current ticker information for a trading pair |
| `get_order_book` | Market Data | Get the order book for a trading pair |
| `list_trades` | Market Data | List recent trades for a currency pair |
| `get_balances` | Account Information | Get balances for all accounts |
| `create_order` | Trading | Create a new buy or sell order |
| `cancel_order` | Trading | Cancel an existing order |
| `list_orders` | Trading | List open orders |
| `list_transactions` | Transactions | List transactions for an account |
| `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 |
Copy link
Contributor

@echarrod echarrod Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a slight preference for ✅ and ❌ over yes and no, but not that important to change 😄

| `get_order_book` | Market Data | Get the order book for a trading pair | No |
| `list_trades` | Market Data | List recent trades for a currency pair | No |
| `get_balances` | Account Information | Get balances for all accounts | No |
| `create_order` | Trading | Create a new buy or sell order | **Yes** |
| `cancel_order` | Trading | Cancel an existing order | **Yes** |
| `list_orders` | Trading | List open orders | No |
| `list_transactions` | Transactions | List transactions for an account | No |
| `get_transaction` | Transactions | Get details of a specific transaction | No |

## Examples

Expand Down Expand Up @@ -203,12 +203,55 @@ This configuration will make VS Code run the Docker container. Ensure Docker is

This tool requires API credentials that have access to your Luno account. Be cautious when using API keys, especially ones with withdrawal permissions. It's recommended to create API keys with only the permissions needed for your specific use case.

### 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.
Copy link
Contributor

@echarrod echarrod Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@solethus solethus Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think I am also in favor more than just ALLOW_WRITE. Thinking maybe we go for ALLOW_WRITE_TOOLS instead? 🤔

Copy link
Contributor

@echarrod echarrod Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


#### Enabling Write Operations

Set the environment variable to one of the following values:
- `ALLOW_WRITE_OPERATIONS=true`
- `ALLOW_WRITE_OPERATIONS=1`
- `ALLOW_WRITE_OPERATIONS=yes`

##### 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}
]
}
}
}
```
Comment on lines +217 to +246
Copy link
Contributor

@echarrod echarrod Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Author

@solethus solethus Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@echarrod echarrod Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@solethus solethus Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense.


### Best Practices for API Credentials

1. **Create Limited-Permission API Keys**: Only grant the permissions absolutely necessary for your use case
2. **Never Commit Credentials to Version Control**: Ensure `.env` files are always in your `.gitignore`
3. **Rotate API Keys Regularly**: Periodically regenerate your API keys to limit the impact of potential leaks
4. **Monitor API Usage**: Regularly check your Luno account for any unauthorized activity
5. **Use Read-Only Mode by Default**: Only enable write operations when specifically needed

### Contributing

Expand Down
29 changes: 24 additions & 5 deletions internal/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,11 @@ import (

const (
// Environment variables
EnvLunoAPIKeyID = "LUNO_API_KEY_ID"
EnvLunoAPIKeySecret = "LUNO_API_SECRET"
EnvLunoAPIDomain = "LUNO_API_DOMAIN"
EnvLunoAPIDebug = "LUNO_API_DEBUG"
EnvLunoAPIKeyID = "LUNO_API_KEY_ID"
EnvLunoAPIKeySecret = "LUNO_API_SECRET"
EnvLunoAPIDomain = "LUNO_API_DOMAIN"
EnvLunoAPIDebug = "LUNO_API_DEBUG"
EnvAllowWriteOperations = "ALLOW_WRITE_OPERATIONS"

// Default Luno API domain
DefaultLunoDomain = "api.luno.com"
Expand All @@ -26,6 +27,9 @@ const (
type Config struct {
// Luno client
LunoClient sdk.LunoClient

// AllowWriteOperations controls whether write operations (create_order, cancel_order) are exposed
AllowWriteOperations bool
}

// Mask a string to show only the first 4 characters and replace the rest with asterisks
Expand Down Expand Up @@ -87,8 +91,23 @@ func Load(domainOverride string) (*Config, error) {
}

client.SetDebug(debugMode)

// Check if write operations are allowed via environment variable
allowWriteOps := false
if writeOpsEnv := os.Getenv(strings.TrimSpace(EnvAllowWriteOperations)); writeOpsEnv != "" {
// Enable write operations if environment variable is set to "true", "1", or "yes"
allowWriteOps = strings.ToLower(writeOpsEnv) == "true" ||
writeOpsEnv == "1" ||
strings.ToLower(writeOpsEnv) == "yes"

if allowWriteOps {
fmt.Println("Write operations enabled via environment variable")
}
}

return &Config{
LunoClient: client,
LunoClient: client,
AllowWriteOperations: allowWriteOps,
}, nil
}

Expand Down
76 changes: 64 additions & 12 deletions internal/config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,30 +68,35 @@ func TestLoad(t *testing.T) {
originalAPISecret := os.Getenv(EnvLunoAPIKeySecret)
originalAPIDomain := os.Getenv(EnvLunoAPIDomain)
originalAPIDebug := os.Getenv(EnvLunoAPIDebug)
originalAllowWriteOps := os.Getenv(EnvAllowWriteOperations)

defer func() {
// Restore original environment
setEnvVar(EnvLunoAPIKeyID, originalAPIKeyID)
setEnvVar(EnvLunoAPIKeySecret, originalAPISecret)
setEnvVar(EnvLunoAPIDomain, originalAPIDomain)
setEnvVar(EnvLunoAPIDebug, originalAPIDebug)
setEnvVar(EnvAllowWriteOperations, originalAllowWriteOps)
}()

tests := []struct {
name string
apiKeyID string
apiSecret string
domainEnv string
domainOverride string
debugEnv string
expectedError string
expectedDomain string
name string
apiKeyID string
apiSecret string
domainEnv string
domainOverride string
debugEnv string
allowWriteOpsEnv string
expectedError string
expectedDomain string
expectedAllowWriteOps bool
}{
{
name: "valid credentials with defaults",
apiKeyID: "test_key_id",
apiSecret: "test_secret",
expectedDomain: DefaultLunoDomain,
name: "valid credentials with defaults",
apiKeyID: "test_key_id",
apiSecret: "test_secret",
expectedDomain: DefaultLunoDomain,
expectedAllowWriteOps: false,
},
{
name: "missing api key id",
Expand Down Expand Up @@ -144,6 +149,48 @@ func TestLoad(t *testing.T) {
apiSecret: "test_secret",
debugEnv: "false",
},
{
name: "write operations enabled with true",
apiKeyID: "test_key_id",
apiSecret: "test_secret",
allowWriteOpsEnv: "true",
expectedAllowWriteOps: true,
},
{
name: "write operations enabled with 1",
apiKeyID: "test_key_id",
apiSecret: "test_secret",
allowWriteOpsEnv: "1",
expectedAllowWriteOps: true,
},
{
name: "write operations enabled with yes",
apiKeyID: "test_key_id",
apiSecret: "test_secret",
allowWriteOpsEnv: "yes",
expectedAllowWriteOps: true,
},
{
name: "write operations disabled with false",
apiKeyID: "test_key_id",
apiSecret: "test_secret",
allowWriteOpsEnv: "false",
expectedAllowWriteOps: false,
},
{
name: "write operations disabled with empty",
apiKeyID: "test_key_id",
apiSecret: "test_secret",
allowWriteOpsEnv: "",
expectedAllowWriteOps: false,
},
{
name: "write operations disabled with invalid value",
apiKeyID: "test_key_id",
apiSecret: "test_secret",
allowWriteOpsEnv: "invalid",
expectedAllowWriteOps: false,
},
}

for _, tc := range tests {
Expand All @@ -153,6 +200,7 @@ func TestLoad(t *testing.T) {
setEnvVar(EnvLunoAPIKeySecret, tc.apiSecret)
setEnvVar(EnvLunoAPIDomain, tc.domainEnv)
setEnvVar(EnvLunoAPIDebug, tc.debugEnv)
setEnvVar(EnvAllowWriteOperations, tc.allowWriteOpsEnv)

cfg, err := Load(tc.domainOverride)

Expand Down Expand Up @@ -180,6 +228,10 @@ func TestLoad(t *testing.T) {
if cfg.LunoClient == nil {
t.Error("Expected LunoClient to be non-nil")
}

if cfg.AllowWriteOperations != tc.expectedAllowWriteOps {
t.Errorf("Expected AllowWriteOperations to be %v, got %v", tc.expectedAllowWriteOps, cfg.AllowWriteOperations)
}
})
}
}
Expand Down
16 changes: 11 additions & 5 deletions internal/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,17 @@ func registerTools(server *mcpserver.MCPServer, cfg *config.Config) {
server.AddTool(orderBookTool, tools.HandleGetOrderBook(cfg))

// Add trading tools
createOrderTool := tools.NewCreateOrderTool()
server.AddTool(createOrderTool, tools.HandleCreateOrder(cfg))

cancelOrderTool := tools.NewCancelOrderTool()
server.AddTool(cancelOrderTool, tools.HandleCancelOrder(cfg))
// Only add write operation tools if explicitly allowed
if cfg.AllowWriteOperations {
slog.Info("Write operations enabled - registering create_order and cancel_order tools")
createOrderTool := tools.NewCreateOrderTool()
server.AddTool(createOrderTool, tools.HandleCreateOrder(cfg))

cancelOrderTool := tools.NewCancelOrderTool()
server.AddTool(cancelOrderTool, tools.HandleCancelOrder(cfg))
} else {
slog.Info("Write operations disabled - create_order and cancel_order tools will not be available")
}

listOrdersTool := tools.NewListOrdersTool()
server.AddTool(listOrdersTool, tools.HandleListOrders(cfg))
Expand Down
Loading
Loading