AlinsRan commented on code in PR #13818:
URL: https://github.com/apache/apisix/pull/13818#discussion_r3780794586
##########
apisix/plugins/data-mask.lua:
##########
@@ -181,7 +182,11 @@ local function mask_json(obj, conf)
end
local index = table_index(node.path[#node.path])
if conf.action == "remove" then
- nested[index] = nil
+ if type(index) == "number" then
+ t_remove(nested, index)
Review Comment:
`jp.nodes()` can return several nodes from the same array, and this loop
walks them in ascending order — each `table.remove` shifts the indexes of the
nodes not yet processed. So this fixes the single-index case but breaks
multi-match removals.
Verified on this branch with `{"items":["a","b","c"]}` and `action: remove`:
| name | master | this PR | expected |
| --- | --- | --- | --- |
| `$.items[1]` | `["a"]` | `["a","c"]` ✅ | `["a","c"]` |
| `$.items[*]` | `[]` | `["b"]` ❌ | `[]` |
| `$.items[0,2]` | `[null,"b"]` | `["b","c"]` ❌ | `["b"]` |
The `$.items[0,2]` row is the one that matters: `"c"` is matched for removal
but survives into the log, while master did remove it — a masking plugin
silently keeping data it was told to drop.
Iterating the nodes in reverse (`for n = #nodes, 1, -1`) makes all three
cases correct; `replace` and `regex` are index-stable, so they are unaffected.
Worth adding `$.items[*]` and `$.items[0,2]` to the tests — the current test
only covers a single index, which is exactly the case that already works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
