Merge pull request #15 from AlexanderGrooff/nested-includes

Author: AlexanderGrooff

.github/workflows/test.yaml

@@ -12,17 +12,26 @@ jobs:
       - uses: actions/checkout@v3
       - run: sudo python setup.py install
       - run: nginx-static-analysis --help
-  integration_test:
+  unit_test:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v3
-      - name: Run general testsuite
-        run: ./runtests.sh
-        shell: bash
+      - uses: actions/setup-python@v3
+      - run: pip install -r requirements/development.txt
+      - run: coverage run -m pytest
+      - run: coverage report
+      - run: coverage xml -o ./coverage.xml
       - name: Upload coverage reports
         uses: codecov/codecov-action@v3
         with:
           files: ./coverage.xml
+  integration_test:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+      - name: Run general testsuite
+        run: ./runtests.sh
+        shell: bash
   linter:
     runs-on: ubuntu-latest
     steps:

example_nginx/servers/example.com.conf

@@ -8,4 +8,5 @@ server {
     location / {
         proxy_pass http://localhost:8003;
     }
+    include /etc/nginx/servers/include/*.conf;
 }

example_nginx/servers/include/nested.conf

@@ -0,0 +1,3 @@
+location /nested {
+    return 418;
+}

nginx_analysis/analysis.py

@@ -9,14 +9,16 @@
     RootNginxConfig,
     filter_unique,
     get_children_recursive,
+    get_parents_recursive,
 )
 
 
 def nginx_to_regex(nginx: str) -> str:
     """
-    Convert a nginx-style regex to a python regex
+    Convert a nginx-style regex to a python regex.
+    Wildcard files are converted to regexes that match any file in the directory.
     """
-    return nginx.replace("*", ".*")
+    return nginx.replace("*", r"[^/]*")
 
 
 def set_parents_in_include(root_config: RootNginxConfig, block_config: NginxLineConfig):
@@ -27,6 +29,7 @@ def set_parents_in_include(root_config: RootNginxConfig, block_config: NginxLine
                 for nested_line_config in nested_file_config.parsed:
                     nested_line_config.parent = block_config
                     block_config.children.append(nested_line_config)
+                    set_parents_in_include(root_config, nested_line_config)
 
 
 def set_parents_of_blocks(root_config: RootNginxConfig, line_config: NginxLineConfig):
@@ -104,44 +107,62 @@ def is_partial_direct_match(
     return None
 
 
-def find_matches_in_children(
+def get_matching_lines_in_children(
     line: NginxLineConfig, filters: List[DirectiveFilter]
 ) -> Tuple[List[NginxLineConfig], List[DirectiveFilter]]:
     """
-    Check if the line matches filters. If it matches, return the line,
-    the line's neighbours and all children. If it doesn't match, check
-    if any of the children match the filters. If they do, return all
-    descendants of the line that match the filters, including the current
-    line.
+    Check if the line matches filters. If it matches, return the line.
+    If it doesn't match, check if any of the children match the filters.
     We also return a list of filters that matched, so we can check if
     all filters were matched eventually.
     If there are no matches, return an empty list for both the matching
     lines and the matched filters.
     """
+    if not filters:
+        # No filters, meaning that the line matches directly
+        return [line], []
+
+    logger.debug(f"Checking if line {line} matches filters: {filters}")
     matched_filter = is_partial_direct_match(line, filters)
     if matched_filter:
         logger.debug(f"Found match in children: {line}")
         all_matched_filters = set([matched_filter])
         # Search for remaining filters in children, as the parent
         # might still be looking for other filters
-        remaining_filters = [f for f in filters if f != matched_filter]
         for child in line.children:
-            _, child_matched_filters = find_matches_in_children(
-                child, remaining_filters
-            )
-            all_matched_filters.update(child_matched_filters)
-        return [line, *line.neighbours, *get_children_recursive(line)], list(
-            all_matched_filters
-        )
+            remaining_filters = [f for f in filters if f not in all_matched_filters]
+            if remaining_filters:
+                logger.debug(
+                    f"Looking for remaining filters in child {child}: {remaining_filters}"
+                )
+                _, child_matched_filters = get_matching_lines_in_children(
+                    child, remaining_filters
+                )
+                all_matched_filters.update(child_matched_filters)
+            else:
+                logger.debug(
+                    f"Found all filters in children of line {line}. We should stop!"
+                )
+                break
+        return [line], list(all_matched_filters)
 
     # All filters must match at least one child
     matched_filters = set()
-    matches = [line]
+    matches = []
     for child in line.children:
-        child_matches, child_matched_filters = find_matches_in_children(child, filters)
-        if child_matches:
-            matches.extend(child_matches)
-            matched_filters.update(child_matched_filters)
+        remaining_filters = [f for f in filters if f not in matched_filters]
+        if remaining_filters:
+            child_matches, child_matched_filters = get_matching_lines_in_children(
+                child, remaining_filters
+            )
+            if child_matches:
+                matches.extend(child_matches)
+                matched_filters.update(child_matched_filters)
+        else:
+            logger.debug(
+                f"Found all filters in children of line {line}. We should stop!"
+            )
+            break
 
     if len(matched_filters) == len(filters):
         logger.debug(
@@ -151,6 +172,25 @@ def find_matches_in_children(
     return [], []
 
 
+def expand_upon_direct_match(
+    match: NginxLineConfig, matched_filters: List[DirectiveFilter]
+) -> List[NginxLineConfig]:
+    """
+    Expand upon matching lines by recursively looking through
+    parents and neighbours. However, we don't want to include any
+    lines that have the same directive as in the filters.
+    F.e., if the filter looks for location=/banaan, we don't want
+    to include other recursive location lines.
+    """
+    matching_lines = []
+    for neighbour in match.neighbours:
+        matching_lines.append(neighbour)
+        matching_lines.extend(get_children_recursive(neighbour))
+    parents = get_parents_recursive(match)
+    matching_lines.extend(parents)
+    return matching_lines
+
+
 def filter_config(
     lines: Iterator[NginxLineConfig],
     filters: List[DirectiveFilter],
@@ -160,10 +200,10 @@ def filter_config(
     """
     matching_lines = []
     for line in lines:
-        child_matches, matched_filters = find_matches_in_children(line, filters)
-        if len(matched_filters) == len(filters):
-            logger.debug(f"Matched all ({len(matched_filters)}) filters: {line}")
-            matching_lines.extend(child_matches)
+        child_matches, _ = get_matching_lines_in_children(line, filters)
+        matching_lines.extend(child_matches)
+        for match in child_matches:
+            matching_lines.extend(expand_upon_direct_match(match, filters))
 
     return filter_unique(matching_lines)
 

nginx_analysis/dataclasses.py

@@ -66,9 +66,12 @@ def match(self, line: "NginxLineConfig") -> bool:
     def __hash__(self) -> int:
         return hash(str(self))
 
-    def __str__(self) -> str:
+    def __repr__(self) -> str:
         return f"{self.directive} -> {self.value}"
 
+    def __str__(self) -> str:
+        return self.__repr__()
+
 
 class NginxLineConfig(BaseModel):
     directive: str
@@ -103,9 +106,12 @@ def __eq__(self, other: Any) -> bool:
     def __hash__(self) -> int:
         return hash(str(self))
 
-    def __str__(self) -> str:
+    def __repr__(self) -> str:
         return f"{self.file}:{self.line} -> {self.directive}"
 
+    def __str__(self) -> str:
+        return self.__repr__()
+
 
 class NginxFileConfig(BaseModel):
     file: Path
@@ -121,9 +127,12 @@ def __eq__(self, other: Any) -> bool:
         comparison_fields = ["file", "status", "errors", "parsed"]
         return compare_objects(self, other, comparison_fields)
 
-    def __str__(self) -> str:
+    def __repr__(self) -> str:
         return f"{self.file}"
 
+    def __str__(self) -> str:
+        return self.__repr__()
+
     @property
     def lines(self) -> Generator[NginxLineConfig, None, None]:
         for line in self.parsed:
@@ -183,3 +192,9 @@ def __eq__(self, other: Any) -> bool:
 
         comparison_fields = ["status", "errors", "config"]
         return compare_objects(self, other, comparison_fields)
+
+    def __repr__(self) -> str:
+        return f"{self.status} {self.root_file}"
+
+    def __str__(self) -> str:
+        return self.__repr__()

runtests.sh

@@ -16,11 +16,4 @@ if [[ ! $(docker-compose ps --services --filter status=running nginx | grep ngin
     docker-compose up -d nginx
 fi
 
-if [[ ! $($NGINX_EXEC command -v pytest) ]]; then
-    $NGINX_EXEC python3 -m pip install pytest
-fi
-
-$NGINX_EXEC coverage run -m pytest
-$NGINX_EXEC coverage report
-$NGINX_EXEC coverage xml -o coverage.xml
 $NGINX_EXEC ./scripts/integration_tests.sh

scripts/integration_tests.sh

@@ -13,23 +13,35 @@ trap uninstall EXIT
 python setup.py install
 NSA=/usr/local/bin/nginx-static-analysis
 
-# Ensure the basic functionality works
+echo "Ensure the basic functionality works"
 $NSA --help || (echo "NOT OK" && exit 1)
 $NSA -d server_name || (echo "NOT OK" && exit 1)
-# Check if only unique lines are returned
+
+echo "Check if only unique lines are returned"
 test $($NSA -d server_name |& grep example.com | wc -l) = 1 || (echo "NOT OK" && exit 1)
-# Garbage in --> no matches
+
+echo "Garbage in --> no matches"
 $NSA -d blabla |& grep "Found no matches" || (echo "NOT OK" && exit 1)
-# Value should show the value in table format
+
+echo "Value should show the value in table format"
 $NSA -d server_name -f server_name=example.com |& grep example.com || (echo "NOT OK" && exit 1)
-# Value should not show other values
+
+echo "Value should not show other values"
 $NSA -d server_name -f server_name=banaan.com |& grep example.com && (echo "NOT OK" && exit 1)
-# Multiple filters should show the most specific match
+
+echo "Multiple filters should show the most specific match"
 $NSA -f location=/ -f server_name=example.com |& grep "/etc/nginx/servers/example.com.conf:8" || (echo "NOT OK" && exit 1)
-# Show directives next to the direct match
+
+echo "Show directives next to the direct match"
 $NSA -f server_name=example.com |& grep "listen" | grep -v "default_server" || (echo "NOT OK" && exit 1)
 $NSA -f server_name=testalex.hypernode.io |& grep "listen" | grep "default_server" || (echo "NOT OK" && exit 1)
-# Don't show nested children of neighbours of direct match
-$NSA -f server_name=testalex.hypernode.io -f location=/ |& grep "404" && (echo "NOT OK" && exit 1)
+
+echo "Don't show nested children of parent neighbours of direct match"
+$NSA -f server_name=testalex.hypernode.io |& grep "/etc/nginx/servers/example.com.conf" && (echo "NOT OK" && exit 1)
+$NSA -f server_name=example.com |& grep "/etc/nginx/servers/testalex.hypernode.io.conf" && (echo "NOT OK" && exit 1)
+
+echo "Show nested includes"
+$NSA -f server_name=testalex.hypernode.io |& grep "/nested" && (echo "NOT OK" && exit 1)
+$NSA -f server_name=example.com |& grep "/nested" || (echo "NOT OK" && exit 1)
 
 echo "All tests passed"

tests/unit/analysis/test_find_matches_in_children.py

@@ -1,4 +1,4 @@
-from nginx_analysis.analysis import find_matches_in_children
+from nginx_analysis.analysis import get_matching_lines_in_children
 from nginx_analysis.dataclasses import DirectiveFilter, NginxLineConfig
 from tests.testcase import TestCase
 
@@ -11,8 +11,12 @@ def test_return_empty_list_if_no_children_and_no_match(self):
             line=1,
         )
         dfilter = DirectiveFilter(directive="location", value="/")
-        self.assertEqual(find_matches_in_children(line, filters=[dfilter]), ([], []))
-        self.assertEqual(find_matches_in_children(line, filters=[dfilter]), ([], []))
+        self.assertEqual(
+            get_matching_lines_in_children(line, filters=[dfilter]), ([], [])
+        )
+        self.assertEqual(
+            get_matching_lines_in_children(line, filters=[dfilter]), ([], [])
+        )
 
     def test_return_empty_list_if_args_dont_match(self):
         line = NginxLineConfig(
@@ -21,8 +25,12 @@ def test_return_empty_list_if_args_dont_match(self):
             line=1,
         )
         dfilter = DirectiveFilter(directive="return", value="123")
-        self.assertEqual(find_matches_in_children(line, filters=[dfilter]), ([], []))
-        self.assertEqual(find_matches_in_children(line, filters=[dfilter]), ([], []))
+        self.assertEqual(
+            get_matching_lines_in_children(line, filters=[dfilter]), ([], [])
+        )
+        self.assertEqual(
+            get_matching_lines_in_children(line, filters=[dfilter]), ([], [])
+        )
 
     def test_return_line_if_direct_match(self):
         line = NginxLineConfig(
@@ -32,7 +40,7 @@ def test_return_line_if_direct_match(self):
         )
         dfilter = DirectiveFilter(directive="return", value="404")
         self.assertEqual(
-            find_matches_in_children(line, filters=[dfilter]), ([line], [dfilter])
+            get_matching_lines_in_children(line, filters=[dfilter]), ([line], [dfilter])
         )
 
     def test_return_line_if_direct_match_for_one_filter(self):
@@ -44,11 +52,11 @@ def test_return_line_if_direct_match_for_one_filter(self):
         dfilter1 = DirectiveFilter(directive="return", value="404")
         dfilter2 = DirectiveFilter(directive="return", value="405")
         self.assertEqual(
-            find_matches_in_children(line, filters=[dfilter1, dfilter2]),
+            get_matching_lines_in_children(line, filters=[dfilter1, dfilter2]),
             ([line], [dfilter1]),
         )
 
-    def test_return_parent_and_children_if_match_child(self):
+    def test_return_child_if_match_child(self):
         parent = NginxLineConfig(
             directive="location",
             args=["/"],
@@ -64,11 +72,11 @@ def test_return_parent_and_children_if_match_child(self):
 
         dfilter = DirectiveFilter(directive="return", value="404")
         self.assertEqual(
-            find_matches_in_children(parent, filters=[dfilter]),
-            ([parent, line], [dfilter]),
+            get_matching_lines_in_children(parent, filters=[dfilter]),
+            ([line], [dfilter]),
         )
 
-    def test_return_parent_and_children_if_match_parent(self):
+    def test_return_parent_if_match_parent(self):
         parent = NginxLineConfig(
             directive="location",
             args=["/"],
@@ -84,6 +92,6 @@ def test_return_parent_and_children_if_match_parent(self):
 
         dfilter = DirectiveFilter(directive="location", value="/")
         self.assertEqual(
-            find_matches_in_children(parent, filters=[dfilter]),
-            ([parent, line], [dfilter]),
+            get_matching_lines_in_children(parent, filters=[dfilter]),
+            ([parent], [dfilter]),
         )

tests/unit/analysis/test_nginx_to_regex.py

@@ -0,0 +1,15 @@
+import re
+
+from nginx_analysis.analysis import nginx_to_regex
+from tests.testcase import TestCase
+
+
+class TestNginxToRegex(TestCase):
+    def test_regular_string_doesnt_change(self):
+        ret = nginx_to_regex("test")
+        self.assertEqual(ret, "test")
+
+    def test_wildcard_only_matches_that_directory(self):
+        regex = nginx_to_regex("*.conf")
+        self.assertTrue(re.match(regex, "test.conf"))
+        self.assertFalse(re.match(regex, "test/test.conf"))