Copilot commented on code in PR #13831:
URL: https://github.com/apache/apisix/pull/13831#discussion_r3792023274
##########
t/plugin/proxy-cache/memory.t:
##########
@@ -1554,3 +1554,68 @@ GET /t
--- response_body
purge=200
leftover=0
+
+
+
+=== TEST 48: a crafted URI cannot collide with another request's Vary variant
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local http = require("resty.http")
+
+ local code, body =
t('/apisix/admin/routes/proxy-cache-vary-collision', ngx.HTTP_PUT, [[{
+ "uri": "/vary-encoding*",
+ "plugins": {
+ "proxy-cache": {
+ "cache_strategy": "memory",
+ "cache_key": ["$host", "$uri"],
+ "cache_zone": "memory_cache",
+ "cache_method": ["GET"],
+ "cache_http_status": [200],
+ "cache_ttl": 300
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1986": 1
+ },
+ "type": "roundrobin"
+ }
+ }]])
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ return
+ end
+
+ local base = "http://127.0.0.1:"; .. ngx.var.server_port
+ local function fetch(path, enc)
+ local res, err = http.new():request_uri(base .. path, {
+ headers = { ["Accept-Encoding"] = enc },
+ })
+ if not res then return nil, err end
+ return res.headers["Apisix-Cache-Status"],
res.body:gsub("%s+$", "")
Review Comment:
`fetch()` assumes `res.body` is always a string; if the upstream returns a
response without a body (or the client gets a HEAD-like response),
`res.body:gsub(...)` will throw and fail the whole test. Other tests in this
file guard `res.body` before trimming whitespace; this one should too.
##########
apisix/plugins/proxy-cache/memory_handler.lua:
##########
@@ -43,6 +43,30 @@ local VARY_INDEX_SUFFIX = "::__vary"
local MAX_VARIANTS = 64
+-- Every shdict key for one cache key is derived from it: the entry itself,
+-- the Vary index, and one entry per variant. The cache key is client
+-- controlled (`cache_key` defaults to `$host` and `$request_uri`), so
+-- deriving them by appending a suffix lets a crafted request produce a cache
+-- key byte-identical to another request's variant key -- the attacker's
Review Comment:
This PR changes the on-disk/in-memory *key derivation*, but `CACHE_VERSION`
stays at 2. That means any pre-upgrade memory-cache entries that happen to be
stored under a key matching the new length-prefixed layout would still be
considered valid (version == 2) and could be served post-upgrade. Bumping
`CACHE_VERSION` would guarantee such entries are bypassed and purged on first
access, regardless of cache_key composition.
##########
apisix/plugins/proxy-cache/memory_handler.lua:
##########
@@ -116,15 +140,14 @@ end
-- cache_control derives them per response), and an expired index must still
Review Comment:
The comment still refers to purging a "legacy base-key entry", but after
this change the stored base entry key is `entry_key(base_key)`
(length-prefixed). Updating the wording will avoid confusion about which key is
actually purged and how the new key layout 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]
