#7645 correct missing constraints on queue name and acc 8000 eptable charac… #7701

rjmcginness · 2023-02-16T13:33:56Z

fix: correct missing constraints on queue name and acceptable characters

thrau

thanks for the contribution @rjmcginness

this one's a headscratcher to me because we already have a method for asserting the queue name, but it's apparently not in use. we do also have assert methods in the sqs queue model itself, which are used, so i'm not sure why these exceptions wouldn't show up

localstack/localstack/services/sqs/models.py

Lines 462 to 466 in 27b4e32

    
           def _assert_queue_name(self, name): 
        
               if not re.match(r"^[a-zA-Z0-9_-]{1,80}$", name): 
        
                   raise InvalidParameterValue( 
        
                       "Can only include alphanumeric characters, hyphens, or underscores. 1 to 80 in length" 
        
                   )

verifying the queue name directly in create_queue makes sense to me, but we could maybe re-use the method we already have (linked in it a comment). and then remove the validation from the SqsQueue class accordingly. i just want to avoid having the same code in three different ways.

and it would be great to add snapshot validated tests. if that's too much of a bother for you, i'm happy to help.

thrau · 2023-02-16T17:35:48Z

tests/integration/test_sqs.py

+    @pytest.mark.only_localstack
+    def test_queue_name_too_long(self, sqs_create_queue, snapshot):
+        with pytest.raises(InvalidParameterValue) as e:
+            sqs_create_queue(QueueName=(81 * "a"))
+
+        snapshot.match("queue_name_invalid_character", e.value.response)


this seems like something we could test with snapshotting. here are a couple of examples. if you don't have an AWS account you want to test it with, then i'm happy to help with tests.

localstack/tests/integration/test_sqs.py

Lines 1408 to 1422 in 27b4e32

@pytest.mark.aws_validated

def test_fifo_queue_requires_suffix(self, sqs_create_queue):

queue_name = f"invalid-{short_uid()}"

attributes = {"FifoQueue": "true"}

with pytest.raises(Exception) as e:

sqs_create_queue(QueueName=queue_name, Attributes=attributes)

e.match("InvalidParameterValue")

@pytest.mark.aws_validated

def test_standard_queue_cannot_have_fifo_suffix(self, sqs_create_queue):

queue_name = f"queue-{short_uid()}.fifo"

with pytest.raises(Exception) as e:

sqs_create_queue(QueueName=queue_name)

e.match("InvalidParameterValue")

thrau · 2023-02-16T17:35:50Z

localstack/services/sqs/provider.py

+    def _verify_valid_queue_name(self, queue_name: str):
+        name_re = re.compile(sqs_constants.QUEUE_NAME_REGEX + "{" + f"{len(queue_name)}" + "}")
+        if not queue_name or len(queue_name) > 80 or name_re.match(queue_name) is None:
+            error_msg = "Can only include alphanumeric characters, hyphens, or underscores. 1 to 80 in length"
+            raise InvalidParameterValue(error_msg)


we actually already have a method for this:

localstack/localstack/services/sqs/provider.py

Lines 105 to 119 in 44ff8c7

def assert_queue_name(queue_name: str, fifo: bool = False):

if queue_name.endswith(".fifo"):

if not fifo:

# Standard queues with .fifo suffix are not allowed

raise InvalidParameterValue(

"Can only include alphanumeric characters, hyphens, or underscores. 1 to 80 in length"

)

# The .fifo suffix counts towards the 80-character queue name quota.

queue_name = queue_name[:-5] + "_fifo"

# slashes are actually not allowed, but we've allowed it explicitly in localstack

if not re.match(r"^[a-zA-Z0-9/_-]{1,80}$", queue_name):

raise InvalidParameterValue(

"Can only include alphanumeric characters, hyphens, or underscores. 1 to 80 in length"

)

rjmcginness · 2023-02-16T23:00:10Z

@thrau Thank you, Thomas. Yes, makes perfect sense. I can try to do this, though it might not be quick. I will try to investigate why the other method is not working and use that in the place I added the new one.

rjmcginness requested review from thrau and baermat as code owners February 16, 2023 13:33

thrau requested changes Feb 16, 2023

View reviewed changes

rjmcginness closed this Feb 17, 2023

rjmcginness force-pushed the master branch from 17092a2 to ed74b27 Compare February 17, 2023 00:12

dominikschubert temporarily deployed to localstack-ext-tests February 17, 2023 10:23 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

#7645 correct missing constraints on queue name and acc 8000 eptable charac… #7701

#7645 correct missing constraints on queue name and acceptable charac… #7701

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	def _assert_queue_name(self, name):
	if not re.match(r"^[a-zA-Z0-9_-]{1,80}$", name):
	raise InvalidParameterValue(
	"Can only include alphanumeric characters, hyphens, or underscores. 1 to 80 in length"
	)

	@pytest.mark.aws_validated
	def test_fifo_queue_requires_suffix(self, sqs_create_queue):
	queue_name = f"invalid-{short_uid()}"
	attributes = {"FifoQueue": "true"}

	with pytest.raises(Exception) as e:
	sqs_create_queue(QueueName=queue_name, Attributes=attributes)
	e.match("InvalidParameterValue")

	@pytest.mark.aws_validated
	def test_standard_queue_cannot_have_fifo_suffix(self, sqs_create_queue):
	queue_name = f"queue-{short_uid()}.fifo"
	with pytest.raises(Exception) as e:
	sqs_create_queue(QueueName=queue_name)
	e.match("InvalidParameterValue")

	def assert_queue_name(queue_name: str, fifo: bool = False):
	if queue_name.endswith(".fifo"):
	if not fifo:
	# Standard queues with .fifo suffix are not allowed
	raise InvalidParameterValue(
	"Can only include alphanumeric characters, hyphens, or underscores. 1 to 80 in length"
	)
	# The .fifo suffix counts towards the 80-character queue name quota.
	queue_name = queue_name[:-5] + "_fifo"

	# slashes are actually not allowed, but we've allowed it explicitly in localstack
	if not re.match(r"^[a-zA-Z0-9/_-]{1,80}$", queue_name):
	raise InvalidParameterValue(
	"Can only include alphanumeric characters, hyphens, or underscores. 1 to 80 in length"
	)

Uh oh!

#7645 correct missing constraints on queue name and acc 8000 eptable charac… #7701

#7645 correct missing constraints on queue name and acceptable charac… #7701

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!