fix(edge): coerce cjson.null pathPrefix to nil for whole-project rules - #1017
Open
apple-ouyang wants to merge 1 commit into
Open
apple-ouyang wants to merge 1 commit into
apple-ouyang wants to merge 1 commit into
Conversation
A whole-project route rule stores path_prefix NULL and serializes as "pathPrefix":null; lua-cjson decodes JSON null to the cjson.null userdata sentinel, not nil. Every nil/""/"/" check in rules_guard's match loop was then false and the guard took #p on userdata, throwing inside access_by_lua -- every request on the host 500s (whole-site outage, 0.8.0): rules_guard.lua:41: attempt to get length of local 'p' (a userdata value) Normalize non-string pathPrefix to nil at the decode boundary in rules_lib.parse; fixing only the match loop would still leave (chosen.pathPrefix or "/") concatenating userdata into the rate-limit key, since cjson.null is truthy. A defensive type check in the guard's match loop keeps un-normalized entries degrading to the catch-all instead of throwing. Adds contract-test coverage asserting both normalizations against the Lua source (the established pattern: no Lua runtime in CI). Fixes oblien#1015
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1015
What was broken
A route rule with
pathPrefix = NULL— a whole-project rule, the default created byopenship edge rules add(no--path) or byPOST /api/projects/:id/route-ruleswithoutpathPrefix— crashed every request on the affected host (500 site-wide), confirmed in production on 0.8.0:Chain:
serializeProjectRulesemits"pathPrefix":null→mgmt_api.luastores the list verbatim inngx.shared.rules→rules_lib.parse()decodes with lua-cjson, where JSON null becomes thecjson.nulluserdata sentinel, notnil→ in the guard's match loopp == nil,p == "",p == "/"are all false →string.sub(uri, 1, #p)takes#pon userdata → throw insideaccess_by_lua→ every request 500s.The fix
rules_lib.lua(parse): coerce a non-stringpathPrefixtonilat the decode boundary. This is the load-bearing fix — fixing only the guard's match loop would still leave(chosen.pathPrefix or "/")concatenating userdata into the rate-limit key (cjson.nullis truthy), crashing the same way for a catch-all rule that carries a rate limit.rules_guard.lua: defensivetype(p) ~= "string"check in the match loop so an un-normalized entry (hand-written dict data, a future producer) degrades to the catch-all instead of throwing on the request path.rules-lua-contract.test.ts: newdescribeasserting both normalizations against the Lua source — the established pattern in this file, since there is no Lua runtime in CI.Verification
luac -pon both modified Lua files: clean.rules_lib.parse+ the guard's match loop under a plain Lua harness withcjson.safe/resty.lrucachestubbed and a userdata stand-in forcjson.null:attempt to get length of a FILE* value (local 'p')— the same crash class as production.nil, catch-all wins, and the rate-limit key renders asrl:host:/:ip:….bun run test(no local checkout of the monorepo); the new tests follow the file's existing source-assertion pattern and were validated as described above.