Skip to content

Commit 43b3cac

Browse files
committed
fix(amazon-sns-sqs-mcp-server): add tag validation to permission mutator tools
Three permission mutator tools (add_sns_permission, add_sqs_permission, remove_sqs_permission) bypassed the mcp_server_version tag validation that guards all other mutative operations. This allows untagged resources to be modified without the safety check. Add 'validator': is_mutative_action_allowed to the tool configuration for all three tools, consistent with other mutative operations. Also adds @BenAtAmazon to CODEOWNERS for amazon-sns-sqs-mcp-server. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.
1 parent ebcafae commit 43b3cac

5 files changed

Lines changed: 112 additions & 4 deletions

File tree

.github/CODEOWNERS

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ NOTICE @awslabs/mcp-admi
2626
/src/amazon-qbusiness-anonymous-mcp-server @awslabs/mcp-admins @awslabs/mcp-maintainers @abhjaw
2727
/src/amazon-qindex-mcp-server @awslabs/mcp-admins @awslabs/mcp-maintainers @tkoba-aws @akhileshamara
2828
/src/amazon-translate-mcp-server @awslabs/mcp-admins @awslabs/mcp-maintainers @goviha01 @rahullks
29-
/src/amazon-sns-sqs-mcp-server @awslabs/mcp-admins @awslabs/mcp-maintainers @kenliao94 @hashimsharkh
29+
/src/amazon-sns-sqs-mcp-server @awslabs/mcp-admins @awslabs/mcp-maintainers @kenliao94 @hashimsharkh @BenAtAmazon
3030
/src/aurora-dsql-mcp-server @awslabs/mcp-admins @awslabs/mcp-maintainers @anwesham-lab @benjscho @pkale @amaksimo @praba2210 @spencercorwin
3131
/src/aws-api-mcp-server @awslabs/mcp-admins @awslabs/mcp-maintainers @awslabs/aws-api-mcp @rshevchuk-git @PCManticore @iddv @arnewouters @bidesh
3232
/src/aws-appsync-mcp-server @awslabs/mcp-admins @awslabs/mcp-maintainers @phani-srikar @maxi114 @neelmurt

src/amazon-sns-sqs-mcp-server/awslabs/amazon_sns_sqs_mcp_server/sns.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,10 @@ def register_sns_tools(mcp: FastMCP, disallow_resource_creation: bool = False):
124124

125125
# Create the tool configuration dictionary
126126
tool_configuration = {
127-
'add_permission': {'name_override': 'add_sns_permission'},
127+
'add_permission': {
128+
'name_override': 'add_sns_permission',
129+
'validator': is_mutative_action_allowed,
130+
},
128131
'remove_permission': {'name_override': 'remove_sns_permission'},
129132
'create_topic': {'func_override': create_topic_override},
130133
'delete_topic': {'validator': is_mutative_action_allowed},

src/amazon-sns-sqs-mcp-server/awslabs/amazon_sns_sqs_mcp_server/sqs.py

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -91,8 +91,14 @@ def register_sqs_tools(mcp: FastMCP, disallow_resource_creation: bool = False):
9191

9292
# Create the tool configuration dictionary
9393
tool_configuration = {
94-
'add_permission': {'name_override': 'add_sqs_permission'},
95-
'remove_permission': {'name_override': 'remove_sqs_permission'},
94+
'add_permission': {
95+
'name_override': 'add_sqs_permission',
96+
'validator': is_mutative_action_allowed,
97+
},
98+
'remove_permission': {
99+
'name_override': 'remove_sqs_permission',
100+
'validator': is_mutative_action_allowed,
101+
},
96102
'create_queue': {'func_override': create_queue_override},
97103
'delete_queue': {'validator': is_mutative_action_allowed},
98104
'set_queue_attributes': {'validator': is_mutative_action_allowed},

src/amazon-sns-sqs-mcp-server/tests/test_sns.py

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,3 +314,36 @@ def test_unsubscribe_validator(self):
314314
)
315315
assert result is False
316316
assert message == 'mutating a resource without the mcp_server_version tag is not allowed'
317+
318+
@patch('boto3.client')
319+
@patch('awslabs.amazon_sns_sqs_mcp_server.sns.AWSToolGenerator')
320+
def test_add_permission_has_validator(self, mock_aws_tool_generator, mock_boto3_client):
321+
"""Test that add_permission tool has tag validation to prevent mutations on untagged resources.
322+
323+
This is a security-critical test: without the validator, add_sns_permission can
324+
grant cross-account access to any SNS topic regardless of whether it was created
325+
by the MCP server (i.e., tagged with mcp_server_version).
326+
"""
327+
mock_mcp = MagicMock()
328+
tool_config_capture = {}
329+
330+
def mock_generator(
331+
service_name,
332+
service_display_name,
333+
mcp,
334+
tool_configuration,
335+
skip_param_documentation,
336+
mcp_server_version=MCP_SERVER_VERSION,
337+
):
338+
nonlocal tool_config_capture
339+
tool_config_capture = tool_configuration
340+
return MagicMock()
341+
342+
mock_aws_tool_generator.side_effect = mock_generator
343+
register_sns_tools(mock_mcp)
344+
345+
# Verify add_permission has a validator
346+
assert 'add_permission' in tool_config_capture
347+
assert 'validator' in tool_config_capture['add_permission'], \
348+
'add_permission must have a validator to prevent mutations on untagged resources'
349+
assert tool_config_capture['add_permission']['validator'] == is_mutative_action_allowed

src/amazon-sns-sqs-mcp-server/tests/test_sqs.py

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,3 +307,69 @@ def test_is_mutative_action_allowed_exception(self):
307307
)
308308
assert result is False
309309
assert message == 'Test exception'
310+
311+
@patch('boto3.client')
312+
@patch('awslabs.amazon_sns_sqs_mcp_server.sqs.AWSToolGenerator')
313+
def test_add_permission_has_validator(self, mock_aws_tool_generator, mock_boto3_client):
314+
"""Test that add_permission tool has tag validation to prevent mutations on untagged resources.
315+
316+
This is a security-critical test: without the validator, add_sqs_permission can
317+
grant cross-account access to any SQS queue regardless of whether it was created
318+
by the MCP server (i.e., tagged with mcp_server_version).
319+
"""
320+
mock_mcp = MagicMock()
321+
tool_config_capture = {}
322+
323+
def mock_generator(
324+
service_name,
325+
service_display_name,
326+
mcp,
327+
tool_configuration,
328+
skip_param_documentation,
329+
mcp_server_version=MCP_SERVER_VERSION,
330+
):
331+
nonlocal tool_config_capture
332+
tool_config_capture = tool_configuration
333+
return MagicMock()
334+
335+
mock_aws_tool_generator.side_effect = mock_generator
336+
register_sqs_tools(mock_mcp)
337+
338+
# Verify add_permission has a validator
339+
assert 'add_permission' in tool_config_capture
340+
assert 'validator' in tool_config_capture['add_permission'], \
341+
'add_permission must have a validator to prevent mutations on untagged resources'
342+
assert tool_config_capture['add_permission']['validator'] == is_mutative_action_allowed
343+
344+
@patch('boto3.client')
345+
@patch('awslabs.amazon_sns_sqs_mcp_server.sqs.AWSToolGenerator')
346+
def test_remove_permission_has_validator(self, mock_aws_tool_generator, mock_boto3_client):
347+
"""Test that remove_permission tool has tag validation to prevent mutations on untagged resources.
348+
349+
This is a security-critical test: without the validator, remove_sqs_permission can
350+
remove permissions from any SQS queue regardless of whether it was created
351+
by the MCP server (i.e., tagged with mcp_server_version).
352+
"""
353+
mock_mcp = MagicMock()
354+
tool_config_capture = {}
355+
356+
def mock_generator(
357+
service_name,
358+
service_display_name,
359+
mcp,
360+
tool_configuration,
361+
skip_param_documentation,
362+
mcp_server_version=MCP_SERVER_VERSION,
363+
):
364+
nonlocal tool_config_capture
365+
tool_config_capture = tool_configuration
366+
return MagicMock()
367+
368+
mock_aws_tool_generator.side_effect = mock_generator
369+
register_sqs_tools(mock_mcp)
370+
371+
# Verify remove_permission has a validator
372+
assert 'remove_permission' in tool_config_capture
373+
assert 'validator' in tool_config_capture['remove_permission'], \
374+
'remove_permission must have a validator to prevent mutations on untagged resources'
375+
assert tool_config_capture['remove_permission']['validator'] == is_mutative_action_allowed

0 commit comments

Comments
 (0)