cubeegress: validate match field types, and make rule_matches fail closed

[dwin-gharibi]

Closes #1458.

Motivation

validate_policy verified that rule.match is a table and stopped there. rule_matches then called
string.upper on each match.method entry and string.lower on match.scheme, so a policy the admin API
had accepted produced an uncaught Lua error inside access_by_lua_block — nginx 500 for every request from
that sandbox, with nothing pointing at the policy as the cause.

What this changes

Two layers, because either alone leaves a gap.

1. CubeEgress/lua/policy.lua — reject malformed policies at install time. validate_policy now
requires match.sni / match.host / match.path / match.scheme to be strings when present, and
match.method to be an array of strings. Errors name the rule index and the field, matching the existing
message style. An empty match = {} is still allowed, as documented.

2. CubeEgress/lua/access_phase.lua — make rule_matches fail closed. A non-string method entry is
skipped rather than passed to string.upper, and a non-string scheme returns no-match rather than being
passed to string.lower. This matters because a policy can reach the shared dict without going through
validate_policy (bootstrap load, or anything written directly), and the data plane should deny rather than
500 in that case.

No comment changes.

Testing

Install-time validation — all 11 cases behave as intended, where master accepted 8 of them:

                                   this branch          master
match.method = [ {} ]              rejected             accepted
match.method = [ 1 ]               rejected             accepted
match.method = "GET"               rejected             accepted
match.scheme = {}                  rejected             accepted
match.scheme = 80                  rejected             accepted
match.host   = {}                  rejected             accepted
match.path   = {}                  rejected             accepted
match.sni    = 12345               rejected             accepted
match.method = {} (empty object)   accepted             accepted
match = {} (empty, allowed)        accepted             accepted
valid full match                   accepted             accepted

with messages like rules[1].match.method[1] must be a string.

Request-path hardening — driving the real rule_matches via its exported _rule_matches hook:

                     master                               this branch
method = [ {} ]      LUA ERROR -> nginx 500               returned false (deny)
scheme = {}          LUA ERROR -> nginx 500               returned false (deny)
method = [ 1 ]       returned false                       returned false
scheme = 80          returned false                       returned false
host/path/sni bad    returned false                       returned false
                     LUA ERRORS: 2                        NO LUA ERRORS

Matching semantics unchanged — the same harness checks the normal cases, and the credential-injection
harness from the HTTP-inject issue is re-run as a regression check:

valid match (should be true)      -> true
non-matching method (false)       -> false
empty match (matches anything)    -> true

1. HTTPS, Host==SNI (legit)              allow=true  injected=sk-live-REAL-OPERATOR-CREDENTIAL
2. HTTPS, Host!=SNI (G4 blocks)          allow=true  injected=<none>
3. HTTP, spoofed Host -> attacker dst_ip allow=true  injected=sk-live-REAL-OPERATOR-CREDENTIAL

Case 3 is the separate plaintext-injection issue, tracked on its own branch — unchanged here, and shown so it
is clear this PR did not alter it.

CI gates: the Lua files are not covered by fmt-check (no Lua formatter configured) or by
unit-test-check. Verification is via LuaJIT against the real modules, as above.

Risk / rollout

A PUT that previously returned 200 for a malformed rule now returns 400. Any control-plane component
pushing such a rule would start seeing failures — but those rules were producing 500s on the data plane
anyway, so surfacing the error at install time is strictly better.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[cubesandboxbot]

Partially redundant with the existing checks in validate_match_tuples (called a few lines below this loop): a non-string scheme is already rejected by port_scheme.normalize_scheme/expand with the same message (rules[i].match.scheme must be a string), and non-string host/sni are already rejected by normalize_identity (rules[i].match host/sni ... is invalid: empty). So the genuinely new install-time rejections here are method (below) and path. Harmless defense-in-depth — but the PR's install-time table claims master "accepted" scheme={}, scheme=80, host={}, sni=12345; those were already 400s on master. The actual install-time behavior change is limited to malformed method/path.

[cubesandboxbot]

The check only verifies type == "table", so an object-shaped table (e.g. {"GET" = true}) or a holey array ({1="GET", 3="POST"}) passes install validation — ipairs iterates nothing, or stops at the first nil gap, so index 3 is never validated. In rule_matches, ipairs then yields nothing or stops early, so the rule silently never matches — a dead rule, despite the "must be an array of strings" message. cjson-decoded JSON arrays are always contiguous, so this is only reachable via hand-built tables and is fail-closed either way; verifying it's a real array (e.g. #method equals the count of integer keys) would make the error message true.

[cubesandboxbot]

Minor asymmetry with the scheme guard at line 145: a non-string method entry is skipped, so a policy that reached the dict without validation with method = [1, "GET"] would still match GET, whereas the scheme guard fails the whole rule on any non-string. If the stated intent is strictly fail-closed ("the data plane should deny rather than 500"), returning false on the first non-string entry would be consistent with the scheme behavior; as written, only the all-garbage case flips from 500 → deny. No effect on valid all-string policies — purely a question of intent on malformed ones.

[cubesandboxbot]

Review: cubeegress match field type validation + fail-closed rule_matches

Overall: looks correct and well-scoped — approved with minor notes. All inline findings are low-severity/informational; no correctness or security defects found in the changed code.

What this does

Two layers of hardening for malformed policy match fields:

1. policy.lua — install-time rejection. validate_policy now requires sni/host/path/scheme to be strings when present, and method to be an array of strings, returning 400 via the admin API instead of letting a policy through that will crash later.
2. access_phase.lua — data-plane fail-closed. rule_matches no longer passes non-strings to string.upper/string.lower, so a policy that reaches the shared dict without going through validate_policy (bootstrap load, direct writes) denies instead of raising a Lua error → nginx 500.

Verified sound

• The request-path fixes are the substantive part and are correct. On master, method = [{}] and scheme = {} raised a Lua error inside access_by_lua_block; the new guards convert both to no-match (deny). Lua coerces numbers in string.upper/lower (string.upper(1) → "1"), which is why the PR's table correctly shows method = [1] and scheme = 80 returning false on master rather than crashing — the new type guards preserve that behavior.
• After this change rule_matches is fully type-safe across all match fields: sni/host via domain_match→lower (nil-safe), path via path_match (type-checked), port/scheme via port_scheme.matches/matches_deny (type-checked), and now method/scheme guarded directly.
• No valid policy is rejected: the enforced types match the documented match-field semantics (all optional, strings/array-of-strings). A previously-installable policy with e.g. method = "GET" was a dead rule on master (never matched), so rejecting it at install loses no working behavior.
• Existing tests in CubeEgress/tests/port_scheme_test.lua exercise both changed functions (policy.validate_policy, access._rule_matches) and all use string-typed fields, so the new validation doesn't break them.

Findings (all low severity)

1. Install-time checks partially overlap existing validation — the new sni/host/scheme string checks are redundant with validate_match_tuples (via port_scheme.normalize_scheme/expand and normalize_identity), which already rejected non-strings for those fields on master. The genuinely new install-time rejections are method and path. Correspondingly, the PR's install-time table overstates what master accepted: scheme={}, scheme=80, host={}, sni=12345 were already 400s on master. The method and path rows are accurate.
2. The method check accepts any table, not just arrays — an object-shaped table or holey array passes validation (the ipairs loop iterates nothing or stops at the first nil gap), producing a rule that silently never matches, despite the "must be an array of strings" message. Only reachable via hand-built tables (cjson arrays are contiguous) and fail-closed either way, but a #method count check would make the message true.
3. Asymmetric data-plane policy for method vs scheme — a non-string method entry is skipped (so method = [1, "GET"] would still match GET), while a non-string scheme fails the whole rule. If strictly fail-closed is the intent, returning false on the first non-string method entry would be consistent with the scheme guard; as written, only the all-garbage case flips from 500 → deny.

Suggestions

• Add regression tests. CubeEgress/tests/port_scheme_test.lua already drives both changed functions and is the natural home for the 11 install-time cases plus the two request-path crash cases (method = [{}], scheme = {}). They aren't run in CI (confirmed: no workflow executes the Lua tests), but they document the intended behavior and prevent regression if the harness is ever wired into CI.
• The "breaking change" risk in the rollout section is real but narrower than stated — only malformed method/path policies actually flip from 200 → 400; the other shapes were already rejected. Either way, 400-at-install beats 500-per-request, so the direction is right.

This review was AI-generated; no human approval is implied.

[chenhengqi]

I personally don't like these runtime type checking. We can't hit these paths from API/SDK side.