Skip to content

Commit

Permalink
Catch and warn about a rule crash in S3BucketPublicReadAclAndListStat…
Browse files Browse the repository at this point in the history
…ementRule (#177)

The following line in the rule from this commit is:
`bucket = cfmodel.Resources.get(bucket_name)`
It expected `bucket_name` to be a hashable object eg. string; however, for an
unresolved template at this point is will be a FunctionDict Ref at its most
basic
  • Loading branch information
cogpie committed Jun 3, 2021
1 parent 0472df9 commit da9db3f
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 1 deletion.
4 changes: 4 additions & 0 deletions cfripper/rules/s3_public_access.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,12 @@ def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
re.compile(r"^s3:L.*$")
):
bucket_name = resource.Properties.Bucket
if not isinstance(bucket_name, str):
logger.warning(f"Not adding {type(self).__name__} failure in {logical_id} – try resolving?")
continue
if "UNDEFINED_PARAM_" in bucket_name:
bucket_name = bucket_name[len("UNDEFINED_PARAM_") :]

bucket = cfmodel.Resources.get(bucket_name)
if bucket and bucket.Properties.get("AccessControl") == "PublicRead":
self.add_failure_to_result(
Expand Down
24 changes: 23 additions & 1 deletion tests/rules/test_S3BucketPublicReadAclAndListStatementRule.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from unittest.mock import MagicMock, patch

import pytest

from cfripper.model.enums import RuleGranularity, RuleMode, RuleRisk
Expand All @@ -6,9 +8,18 @@
from tests.utils import compare_lists_of_failures, get_cfmodel_from


def get_s3_read_plus_list():
return get_cfmodel_from("rules/S3BucketPublicReadAclAndListStatementRule/s3_read_plus_list.json")


@pytest.fixture()
def unresolved_s3_read_plus_list():
return get_s3_read_plus_list()


@pytest.fixture()
def s3_read_plus_list():
return get_cfmodel_from("rules/S3BucketPublicReadAclAndListStatementRule/s3_read_plus_list.json").resolve()
return get_s3_read_plus_list().resolve()


def test_s3_read_plus_list(s3_read_plus_list):
Expand Down Expand Up @@ -41,6 +52,17 @@ def test_s3_read_plus_list(s3_read_plus_list):
)


@patch("cfripper.rules.s3_public_access.logger")
def test_s3_read_plus_list_with_unresolved_template(patched_logger: MagicMock, unresolved_s3_read_plus_list):
rule = S3BucketPublicReadAclAndListStatementRule(None)

try:
rule.invoke(unresolved_s3_read_plus_list)
assert patched_logger.warning.call_count == 2
except TypeError:
assert False, "S3BucketPublicReadAclAndListStatementRule crashed"


def test_rule_supports_filter_config(s3_read_plus_list, default_allow_all_config):
rule = S3BucketPublicReadAclAndListStatementRule(default_allow_all_config)
result = rule.invoke(s3_read_plus_list)
Expand Down

0 comments on commit da9db3f

Please sign in to comment.