Tests: ngx_http_json_module tests. by roman-f5 · Pull Request #101 · nginx/nginx-tests

roman-f5 · 2026-08-12T20:10:55Z

Proposed changes

Added json_parser.t covering json_set value extraction (scalars, nested
objects and arrays, compound containers, literals, number types, escapes,
the '$' root selector, source caching, last-match-wins on duplicate keys,
quoted key segments, and binary-safe / Unicode / surrogate-pair keys), and
json_parser_invalid.t covering rejection of malformed json_set paths at
configuration load.

Checklist

Before creating a PR, run through this checklist and mark each as complete:

I have read the contributing guidelines.
I have signed the F5 Contributor License Agreement (CLA).
If applicable, I have added tests that prove my fix is effective or that my feature works.
If applicable, I have checked that any relevant tests pass after adding my changes.
I have updated any relevant documentation (README.md and/or CHANGELOG.md).

Added json_parser.t for json_set value extraction and json_parser_invalid.t for rejection of malformed json_set paths at configuration load.

dekobon · 2026-08-20T12:56:09Z

+
+EOF
+
+$t->try_run('no json_set')->plan(45);


This is good coverage for json_set path/extraction behavior, but it is not yet a parser conformance test.

The current file has 29 runtime extraction cases and only one malformed JSON input ({bad). That means a large class of parser regressions could still land unnoticed: malformed numbers, trailing garbage, invalid UTF-8, lone or mispaired surrogates, BOM handling, unusual whitespace, deep nesting, and other edge cases that are often security-relevant because different parsers disagree on them.

JSONTestSuite is a public conformance corpus used by many JSON parser implementations. It includes:

95 valid cases (y_*)

188 invalid cases (n_*)

35 implementation-defined edge cases (i_*)

22 transform cases

Its value is that it exercises exactly the kinds of parser boundary cases that hand-written happy-path tests usually miss.

I think this PR needs a second layer of tests:

keep json_parser.t for module behavior and path extraction

add a direct parser conformance test around ngx_json_validate() that runs a representative subset, or ideally the full JSONTestSuite corpus

A direct parser test is probably the better long-term place for this because some important JSONTestSuite inputs use raw bytes (invalid UTF-8, BOMs, embedded NULs, malformed escapes) that are awkward or impossible to express reliably through $arg_json and HTTP query-string based tests.

dekobon · 2026-08-20T12:57:02Z

+    json_set $jp_bf $arg_json bf;
+    json_set $jp_nv $arg_json nv;
+
+    json_set $jp_deep $arg_json a.b.c.d.e;


There is no json_max_depth coverage in this file yet. Since the directive directly controls the parser stack bound, it would be useful to add at least:

json_max_depth 1 with a one-level success case

json_max_depth 1 with a two-level rejection case

json_max_depth 0 config rejection

Once the upper-bound fix lands, a huge-value config rejection should be covered too. These are exactly the boundary tests that would catch regressions in depth enforcement and the stack-allocation path.

dekobon · 2026-08-20T12:57:02Z

+like(http_get('/quoted?json={"a.b":"x","foo":{},"a]b":"y","data":{}}'),
+	qr/empty=\[\]/, 'quoted empty key absent');
+
+like(http_post('/allq', '{"\\"asd\\u8898  smth\\"":"hit"}'),


The $request_body coverage here only uses small in-memory bodies. Could we add a spill regression as well? With a low client_body_buffer_size, the same logical JSON body can spill to a temp file and make $request_body-backed json_set values disappear. That is security-relevant for configs that use an extracted field for allow/deny decisions.

A useful matrix would be:

small {"decision":"deny"} body -> extracted value present

same body padded past the spill threshold -> policy result must not silently change

same padded body with a larger body buffer -> extracted value present again

That would catch fail-open behavior caused only by body size / storage mode.

dekobon · 2026-08-20T12:57:02Z

+
+    json_set $jp_loc_name $arg_json name;
+
+    json_set $jp_cache_a $arg_json a;


Could we add regression coverage for destination-variable interference here? All current destinations are fresh $jp_* names, so this file would not catch either sibling-slot clobbering or collisions with built-in prefix variables.

Two small cases would help a lot:

set $x ...; json_set $x ...; json_set $y ...; return "$x$y"; to ensure evaluating $y does not rewrite $x

json_set $http_authorization ...; with and without an Authorization header to make the intended collision behavior explicit

Those are security-relevant because they can change auth-related values based only on evaluation order or destination naming.

dekobon · 2026-08-20T12:57:02Z

+	my $bin = $ENV{TEST_NGINX_BINARY} || 'nginx';
+	my $out = `$bin -t -p $testdir/ -c invalid.conf -e invalid_error.log 2>&1`;
+
+	# ignore the nginx -t exit status: an otherwise-valid config may still


check_conf() ignores the nginx -t exit status and decides only from regex output. That is fine for rejection checks, but it makes accepted-path checks like foo["x"].bar capable of false-passing if nginx -t fails for some unrelated reason. Could we split this into check_conf_rejected() and check_conf_ok() helpers, with the acceptance helper requiring exit status 0?

Correction to my earlier suggestion: requiring exit status 0 is not robust on sanitizer builds, because a valid nginx -t can print syntax is ok and still exit non-zero after LeakSanitizer output.

The core issue is still that accepted-path checks currently pass on any output that lacks invalid json_set path; a better accept helper would require syntax is ok and diag($out) on mismatch.

dekobon · 2026-08-20T14:15:23Z

+
+	# use a dedicated log file so these nginx -t runs never write into the
+	# error.log that the Test::Nginx teardown inspects for alerts
+	my $bin = $ENV{TEST_NGINX_BINARY} || 'nginx';


check_conf() is not using the same nginx binary as the rest of the test in the default workflow. Test::Nginx falls back to ../nginx/objs/nginx, but this falls back to bare nginx from PATH. With TEST_NGINX_BINARY unset I reproduced the HTTP cases running against the local build while 15 config checks ran against /usr/sbin/nginx and failed with unknown directive "json_set"; the acceptance check still passed vacuously.

Could this use $Test::Nginx::NGINX instead? It also avoids the || vs defined mismatch for an empty-but-set env var.

dekobon · 2026-08-20T14:15:23Z

+
+like(http_get('/empty_src'), qr/name=\[\]/, 'empty source');
+
+like(http_get('/loc?json={"name":"locval"}'), qr/loc_name=\[locval\]/,


This does not test a location-level directive: $jp_loc_name is declared in http {} like the other vars, and json_set itself is NGX_HTTP_MAIN_CONF only. /loc is therefore just another name extraction case.

Could this become a deliberate same-path multi-destination test plus a separate check_conf() case asserting that json_set is rejected inside server {} / location {}?

dekobon · 2026-08-20T14:15:23Z

+like(http_get('/toparray?json=[10,20,30]'), qr/t0=\[10\] t1=\[20\]/,
+	'top-level array');
+
+like(http_get('/invalid?json={bad'), qr/name=\[\]/, 'invalid json');


The only invalid JSON case fails before any destination is stored, so it does not exercise the second reset after a later parse failure.

A regression that drops the post-failure reset would still pass this file while leaking name=[John] from inputs like {"name":"John", or {"name":"John","x":"\uDEAD"}.

Could we add one truncated-after-match case and one invalid-sibling-string case?

dekobon · 2026-08-20T14:15:23Z

+like(http_get('/cache?json={"a":"one","b":"two"}'), qr/a=\[one\] b=\[two\]/,
+	'cache shared source');
+
+like(http_get('/skipidx?json=[{"ignored":1},{"wanted":"yes"}]'),


This input does not actually skip element 0: [0] is already configured on the same $arg_json tree via $jp_top0, so the first element is matched and descended.

A skip-on-open regression would leave this green. Could this use a dedicated source/path with only index 1 configured, e.g. arrx[1].wanted, so element 0 is genuinely unconfigured?

dekobon · 2026-08-20T14:15:40Z

+	. '"num_exp":1e10}'), qr/int=\[42\] neg=\[-7\] frac=\[3.14\] exp=\[1e10\]/,
+	'number types');
+
+like(http_get('/escaped?json={"escaped":"a\\nb\\tc"}'), qr/esc=\[a\nb\tc\]/,


All current \u assertions are key-side, where both the config path and the JSON key are decoded by the same unescaper before comparison.

That means a systematically wrong UTF-8 encoding could still pass. Could we add a value-side assertion such as {"e":"\uD83D\uDE00"} -> decoded emoji, and perhaps \u8898, to pin the actual output bytes?

dekobon · 2026-08-20T14:15:40Z

+is(check_path('a[1a]'), 1, 'index with trailing letter rejected');
+is(check_path('a[12'), 1, 'unterminated index rejected');
+
+is(check_path('a["abc'), 1, 'unterminated quoted key rejected');


This rejection block only covers structural path syntax. It misses the quoted-key unescape validator (a["x\q"], a["\u12"], o["x\uD83D"]), the empty-path branch (which reports empty json_set path, so check_path() cannot match it), and non-$ name/source arguments (invalid variable name).

Those are separate config-time rejection paths worth pinning here.

dekobon · 2026-08-20T14:15:40Z

+
+EOF
+
+$t->try_run('no json_set')->plan(45);


try_run('no json_set') turns any startup error in this full config into skip_all, not just an absent module.

Because this config contains the hardest quoted-key / surrogate / NUL cases, a regression that makes one of them fail config load would make CI green by skip. Could this first probe module presence with a minimal one-directive config, then call run() on the full config so real config regressions fail?

dekobon · 2026-08-20T14:15:40Z

+    json_set $jp_dup $arg_json dup;
+    json_set $jp_dup_nested $arg_json obj.k;
+
+    json_set $jp_nul_x $request_body '["a\\u0000x"]';


There are two parser sources in the config ($arg_json and $request_body), but no request evaluates vars from both sources together.

That leaves per-source reset/isolation untested. A mixed location that returns one $arg_json-derived value alongside the $request_body-derived values would catch a regression that resets every source's destinations instead of only the current source.

dekobon · 2026-08-20T14:15:40Z

+	qr/m00=\[1\] m01=\[2\] m10=\[3\] m1=\[\[3,4\]\]/, 'array of arrays');
+
+like(http_get('/numbers?json={"num_int":42,"num_neg":-7,"num_frac":3.14,'
+	. '"num_exp":1e10}'), qr/int=\[42\] neg=\[-7\] frac=\[3.14\] exp=\[1e10\]/,


Two assertions here are weaker than intended:

qr/frac=\[3.14\]/ leaves . unescaped, so frac=[3X14] would still match.

The quoted-empty-key case later only checks empty=[], which would also pass if the whole parse failed.

Please use 3\.14 here and anchor the empty-key case with a present sibling, e.g. rbr=[y] empty=[].

dekobon · 2026-08-20T14:15:49Z

The PR description no longer matches the diff: it says there is a separate json_parser_invalid.t file, but this PR only adds json_parser.t, and it lists $ root selector extraction even though the test correctly asserts that $ is rejected.

Could the description be updated to match the current coverage?

github-project-automation Bot added this to NGINX OSS Unified Workspace Aug 12, 2026

github-project-automation Bot moved this to New in NGINX OSS Unified Workspace Aug 12, 2026

roman-f5 assigned route443 Aug 12, 2026

roman-f5 requested a review from pluknet August 12, 2026 20:13

roman-f5 force-pushed the dev/json-parser branch 3 times, most recently from 347aa85 to 02a6707 Compare August 12, 2026 22:54

sindhushiv moved this from New to In Review in NGINX OSS Unified Workspace Aug 13, 2026

roman-f5 force-pushed the dev/json-parser branch 4 times, most recently from 7199862 to 6284bcb Compare August 18, 2026 18:12

Tests: ngx_http_json_module tests.

f6ba9ea

Added json_parser.t for json_set value extraction and json_parser_invalid.t for rejection of malformed json_set paths at configuration load.

roman-f5 force-pushed the dev/json-parser branch from 6284bcb to f6ba9ea Compare August 18, 2026 18:39

xeioex mentioned this pull request Aug 20, 2026

Added the json parser module. nginx/nginx#1642

Merged

7 tasks

dekobon reviewed Aug 20, 2026

View reviewed changes


		json_set $jp_loc_name $arg_json name;

		json_set $jp_cache_a $arg_json a;


		like(http_get('/empty_src'), qr/name=\[\]/, 'empty source');

		like(http_get('/loc?json={"name":"locval"}'), qr/loc_name=\[locval\]/,


		EOF

		$t->try_run('no json_set')->plan(45);


		EOF

		$t->try_run('no json_set')->plan(45);

Uh oh!

Conversation

roman-f5 commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Checklist

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!

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!

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!

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!

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!

dekobon commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

roman-f5 commented Aug 12, 2026 •

edited

Loading