Skip to content

Commit f197aec

Browse files
committed
fix: graceful handling of malformed nginx configs
Instead of crashing with IndexError on malformed directives like `map $single_arg { }`, gixy now catches the exception and displays a helpful error message suggesting the user run `nginx -t` to validate. - Add try/except wrapper in directive_factory for IndexError/TypeError - Display error message in CLI instead of silently failing - Add .claude to .gitignore - Add tests for malformed config handling
1 parent 5b138e4 commit f197aec

File tree

6 files changed

+268
-13
lines changed

6 files changed

+268
-13
lines changed

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ venv/
6464
venv3/
6565
.idea/
6666
.vscode/
67+
.claude/
6768

6869
# Cursor rules stored in private dotfiles
6970
.cursor/rules

gixy/cli/main.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -421,7 +421,8 @@ def main():
421421
else:
422422
with open(path, mode="rb") as fdata:
423423
yoda.audit(path, fdata, is_stdin=False)
424-
except InvalidConfiguration:
424+
except InvalidConfiguration as e:
425+
sys.stderr.write(f"Configuration error: {e}\n")
425426
failed = True
426427
formatter.feed(path, yoda)
427428
failed = failed or sum(yoda.stats.values()) > 0

gixy/directives/block.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
except ImportError:
44
from functools import cached_property
55

6+
from gixy.core.exceptions import InvalidConfiguration
67
from gixy.core.regexp import Regexp
78
from gixy.core.variable import Variable, compile_script
89
from gixy.directives.directive import Directive, MapDirective
@@ -182,7 +183,7 @@ def __init__(self, name, args):
182183
# if ($request_method = POST)
183184
self.variable, self.operand, self.value = args
184185
else:
185-
raise Exception(f'Unknown "if" definition, args: {args!r}')
186+
raise InvalidConfiguration(f'Unknown "if" definition, args: {args!r}')
186187

187188
def __str__(self):
188189
return "{name} ({args}) {{".format(name=self.name, args=" ".join(self.args))

gixy/parser/nginx_parser.py

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -199,16 +199,22 @@ def directive_factory(self, parsed_type, parsed_name, parsed_args):
199199
if not klass:
200200
return None
201201

202-
if klass.is_block:
203-
args = [to_native(v).strip() for v in parsed_args[0]]
204-
children = parsed_args[1]
202+
try:
203+
if klass.is_block:
204+
args = [to_native(v).strip() for v in parsed_args[0]]
205+
children = parsed_args[1]
205206

206-
inst = klass(parsed_name, args)
207-
self.parse_block(children, inst)
208-
return inst
209-
else:
210-
args = [to_native(v).strip() for v in parsed_args]
211-
return klass(parsed_name, args)
207+
inst = klass(parsed_name, args)
208+
self.parse_block(children, inst)
209+
return inst
210+
else:
211+
args = [to_native(v).strip() for v in parsed_args]
212+
return klass(parsed_name, args)
213+
except (IndexError, TypeError) as e:
214+
raise InvalidConfiguration(
215+
f'Failed to parse "{parsed_name}" directive: {e}. '
216+
f'Config may be malformed - run "nginx -t" to validate.'
217+
)
212218

213219
def _get_directive_class(self, parsed_type, parsed_name):
214220
if (
@@ -233,6 +239,11 @@ def _init_directives(self):
233239
self.directives["directive"] = directive.get_overrides()
234240

235241
def _resolve_include(self, args, parent):
242+
if not args:
243+
raise InvalidConfiguration(
244+
'Failed to parse "include" directive: list index out of range. '
245+
'Config may be malformed - run "nginx -t" to validate.'
246+
)
236247
pattern = args[0]
237248
# TODO(buglloc): maybe file providers?
238249
if self.is_dump:

gixy/parser/raw_parser.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ def parse(self, data):
9393
single=True,
9494
strict=False, # Allow directives outside their normal context
9595
check_ctx=False, # Skip context validation
96-
check_args=False, # Skip argument validation
96+
check_args=False, # Don't validate argument counts - gixy is not a config validator
9797
comments=True, # Include comments in the output
9898
)
9999

@@ -117,7 +117,7 @@ def parse_path(self, path):
117117
single=True,
118118
strict=False, # Allow directives outside their normal context
119119
check_ctx=False, # Skip context validation
120-
check_args=False, # Skip argument validation
120+
check_args=False, # Don't validate argument counts - gixy is not a config validator
121121
comments=True, # Include comments in the output
122122
)
123123

Lines changed: 241 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,241 @@
1+
"""Tests for graceful handling of malformed nginx configurations."""
2+
3+
import pytest
4+
5+
from gixy.core.exceptions import InvalidConfiguration
6+
from gixy.directives.block import LocationBlock, MapBlock, IfBlock
7+
from gixy.directives.directive import (
8+
AddHeaderDirective,
9+
SetDirective,
10+
RewriteDirective,
11+
)
12+
from gixy.parser.nginx_parser import NginxParser
13+
14+
15+
def _parse(config):
16+
return NginxParser(cwd="", allow_includes=False).parse_string(config)
17+
18+
19+
class TestMalformedDirectives:
20+
"""Test that malformed directives raise InvalidConfiguration."""
21+
22+
def test_add_header_missing_value(self):
23+
"""add_header requires 2-3 args."""
24+
with pytest.raises(
25+
InvalidConfiguration, match='Failed to parse "add_header" directive'
26+
):
27+
_parse("add_header X-Test;")
28+
29+
def test_add_header_no_args(self):
30+
"""add_header with no args should fail."""
31+
with pytest.raises(
32+
InvalidConfiguration, match='Failed to parse "add_header" directive'
33+
):
34+
_parse("add_header;")
35+
36+
def test_set_missing_value(self):
37+
"""set requires exactly 2 args."""
38+
with pytest.raises(
39+
InvalidConfiguration, match='Failed to parse "set" directive'
40+
):
41+
_parse("set $foo;")
42+
43+
def test_set_no_args(self):
44+
"""set with no args should fail."""
45+
with pytest.raises(
46+
InvalidConfiguration, match='Failed to parse "set" directive'
47+
):
48+
_parse("set;")
49+
50+
def test_auth_request_set_missing_value(self):
51+
"""auth_request_set requires exactly 2 args."""
52+
with pytest.raises(
53+
InvalidConfiguration, match='Failed to parse "auth_request_set" directive'
54+
):
55+
_parse("auth_request_set $foo;")
56+
57+
def test_perl_set_missing_value(self):
58+
"""perl_set requires exactly 2 args."""
59+
with pytest.raises(
60+
InvalidConfiguration, match='Failed to parse "perl_set" directive'
61+
):
62+
_parse("perl_set $foo;")
63+
64+
def test_set_by_lua_missing_value(self):
65+
"""set_by_lua requires 2+ args."""
66+
with pytest.raises(
67+
InvalidConfiguration, match='Failed to parse "set_by_lua" directive'
68+
):
69+
_parse("set_by_lua $foo;")
70+
71+
def test_rewrite_missing_replacement(self):
72+
"""rewrite requires 2-3 args."""
73+
with pytest.raises(
74+
InvalidConfiguration, match='Failed to parse "rewrite" directive'
75+
):
76+
_parse("rewrite ^/old;")
77+
78+
def test_rewrite_no_args(self):
79+
"""rewrite with no args should fail."""
80+
with pytest.raises(
81+
InvalidConfiguration, match='Failed to parse "rewrite" directive'
82+
):
83+
_parse("rewrite;")
84+
85+
86+
class TestMalformedBlocks:
87+
"""Test that malformed blocks raise InvalidConfiguration."""
88+
89+
def test_location_no_args(self):
90+
"""location requires 1-2 args."""
91+
with pytest.raises(
92+
InvalidConfiguration, match='Failed to parse "location" directive'
93+
):
94+
_parse("location {}")
95+
96+
def test_map_missing_destination(self):
97+
"""map requires exactly 2 args - Jim's original bug case."""
98+
with pytest.raises(
99+
InvalidConfiguration, match='Failed to parse "map" directive'
100+
):
101+
_parse("map $uri {}")
102+
103+
def test_map_no_args(self):
104+
"""map with no args should fail."""
105+
with pytest.raises(
106+
InvalidConfiguration, match='Failed to parse "map" directive'
107+
):
108+
_parse("map {}")
109+
110+
def test_include_no_args(self):
111+
"""include requires exactly 1 arg."""
112+
with pytest.raises(
113+
InvalidConfiguration, match='Failed to parse "include" directive'
114+
):
115+
_parse("include;")
116+
117+
def test_if_invalid_args(self):
118+
"""if with 4+ args should fail."""
119+
with pytest.raises(InvalidConfiguration, match='Unknown "if" definition'):
120+
_parse("if ($a = b c d) {}")
121+
122+
123+
class TestValidEdgeCases:
124+
"""Test that valid edge cases still work correctly."""
125+
126+
def test_location_with_modifier(self):
127+
"""location ~ /regex should work."""
128+
tree = _parse("location ~ ^/api { }")
129+
location = tree.children[0]
130+
assert isinstance(location, LocationBlock)
131+
assert location.modifier == "~"
132+
assert location.path == "^/api"
133+
134+
def test_location_without_modifier(self):
135+
"""location /path should work."""
136+
tree = _parse("location /api { }")
137+
location = tree.children[0]
138+
assert isinstance(location, LocationBlock)
139+
assert location.modifier is None
140+
assert location.path == "/api"
141+
142+
def test_location_exact_match(self):
143+
"""location = /exact should work."""
144+
tree = _parse("location = /exact { }")
145+
location = tree.children[0]
146+
assert isinstance(location, LocationBlock)
147+
assert location.modifier == "="
148+
assert location.path == "/exact"
149+
150+
def test_rewrite_with_flag(self):
151+
"""rewrite with flag should work."""
152+
tree = _parse("rewrite ^/old /new permanent;")
153+
rewrite = tree.children[0]
154+
assert isinstance(rewrite, RewriteDirective)
155+
assert rewrite.pattern == "^/old"
156+
assert rewrite.replace == "/new"
157+
assert rewrite.flag == "permanent"
158+
159+
def test_rewrite_without_flag(self):
160+
"""rewrite without flag should work."""
161+
tree = _parse("rewrite ^/old /new;")
162+
rewrite = tree.children[0]
163+
assert isinstance(rewrite, RewriteDirective)
164+
assert rewrite.pattern == "^/old"
165+
assert rewrite.replace == "/new"
166+
assert rewrite.flag is None
167+
168+
def test_add_header_with_always(self):
169+
"""add_header with always flag should work."""
170+
tree = _parse("add_header X-Test value always;")
171+
add_header = tree.children[0]
172+
assert isinstance(add_header, AddHeaderDirective)
173+
assert add_header.header == "x-test"
174+
assert add_header.value == "value"
175+
assert add_header.always is True
176+
177+
def test_add_header_without_always(self):
178+
"""add_header without always flag should work."""
179+
tree = _parse("add_header X-Test value;")
180+
add_header = tree.children[0]
181+
assert isinstance(add_header, AddHeaderDirective)
182+
assert add_header.header == "x-test"
183+
assert add_header.value == "value"
184+
assert add_header.always is False
185+
186+
def test_set_directive(self):
187+
"""set with 2 args should work."""
188+
tree = _parse("set $foo bar;")
189+
set_dir = tree.children[0]
190+
assert isinstance(set_dir, SetDirective)
191+
assert set_dir.variable == "foo"
192+
assert set_dir.value == "bar"
193+
194+
def test_map_block(self):
195+
"""map with 2 args should work."""
196+
tree = _parse("map $uri $mapped { default 0; }")
197+
map_block = tree.children[0]
198+
assert isinstance(map_block, MapBlock)
199+
assert map_block.source == "$uri"
200+
assert map_block.variable == "mapped"
201+
202+
def test_map_empty_source_valid(self):
203+
"""Jim's valid pattern: map "" $myvar should work."""
204+
tree = _parse('map "" $myvar { default ""; }')
205+
map_block = tree.children[0]
206+
assert isinstance(map_block, MapBlock)
207+
assert map_block.source == ""
208+
assert map_block.variable == "myvar"
209+
210+
def test_if_single_variable(self):
211+
"""if ($var) should work."""
212+
tree = _parse("if ($slow) { }")
213+
if_block = tree.children[0]
214+
assert isinstance(if_block, IfBlock)
215+
assert if_block.variable == "$slow"
216+
217+
def test_if_file_check(self):
218+
"""if (-f $file) should work."""
219+
tree = _parse("if (-f $request_filename) { }")
220+
if_block = tree.children[0]
221+
assert isinstance(if_block, IfBlock)
222+
assert if_block.operand == "-f"
223+
assert if_block.value == "$request_filename"
224+
225+
def test_if_comparison(self):
226+
"""if ($var = value) should work."""
227+
tree = _parse("if ($request_method = POST) { }")
228+
if_block = tree.children[0]
229+
assert isinstance(if_block, IfBlock)
230+
assert if_block.variable == "$request_method"
231+
assert if_block.operand == "="
232+
assert if_block.value == "POST"
233+
234+
def test_if_regex(self):
235+
"""if ($var ~ regex) should work."""
236+
tree = _parse("if ($request_uri ~ ^/admin) { }")
237+
if_block = tree.children[0]
238+
assert isinstance(if_block, IfBlock)
239+
assert if_block.variable == "$request_uri"
240+
assert if_block.operand == "~"
241+
assert if_block.value == "^/admin"

0 commit comments

Comments
 (0)