From bd231e56674f92a02eba587c32da9aaab23a8be6 Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Sat, 10 Oct 2026 11:30:31 +0200 Subject: [PATCH] mod_cache: Parse comma separated Cache-Control directives again. cache_strqtok() did not end a token at a comma or whitespace unless an argument followed, so "private, no-store" was read as one token and the response was stored. Stop at any non-token character, after the '=' test. BZ 70028. --- changes-entries/cache-control-comma-list.txt | 5 + modules/cache/cache_util.c | 3 + test/modules/proxy/test_07_cache_control.py | 163 +++++++++++++++++++ 3 files changed, 171 insertions(+) create mode 100644 changes-entries/cache-control-comma-list.txt create mode 100644 test/modules/proxy/test_07_cache_control.py diff --git a/changes-entries/cache-control-comma-list.txt b/changes-entries/cache-control-comma-list.txt new file mode 100644 index 00000000000..3e1865ab172 --- /dev/null +++ b/changes-entries/cache-control-comma-list.txt @@ -0,0 +1,5 @@ + *) mod_cache: Parse comma separated Cache-Control directives again. A + directive without an argument followed by another one, e.g. + "private, no-store", was taken as a single token, so such responses + were stored and reused. + [Arturo Bernal] diff --git a/modules/cache/cache_util.c b/modules/cache/cache_util.c index 33abc969fdb..90712e37956 100644 --- a/modules/cache/cache_util.c +++ b/modules/cache/cache_util.c @@ -977,6 +977,9 @@ apr_status_t cache_strqtok(char *str, char **token, char **arg, char **last) if (arg) *arg = wpos; continue; } + if (TEST_CHAR(*str, T_HTTP_TOKEN_STOP)) { + goto end; + } break; case IN_BETWEEN: diff --git a/test/modules/proxy/test_07_cache_control.py b/test/modules/proxy/test_07_cache_control.py new file mode 100644 index 00000000000..0b56a8aa85c --- /dev/null +++ b/test/modules/proxy/test_07_cache_control.py @@ -0,0 +1,163 @@ +import os +import uuid + +import pytest + +from pyhttpd.conf import HttpdConf +from .env import TCPFaker + + +# Backend answering // with the Cache-Control header lines of +# CASES[index], counting the requests it gets for every path. +class _CacheFaker(TCPFaker): + + def __init__(self, host, port, cases): + super().__init__(host, port) + self.cases = cases + self.hits = {} + + def _make_response(self, data): + head = data.split(b"\r\n\r\n")[0].decode("latin-1").split("\r\n") + path = head[0].split(" ")[1] + self.hits[path] = self.hits.get(path, 0) + 1 + cookie = "" + for line in head[1:]: + if line.lower().startswith("cookie:"): + cookie = line.split(":", 1)[1].strip() + cc, extra = self.cases[int(path.rsplit("/", 1)[1])] + body = f"hit={self.hits[path]} cookie={cookie}\n" + lines = ["HTTP/1.1 200 OK", + "Last-Modified: Thu, 01 Jan 2026 00:00:00 GMT", + 'ETag: "t"', + "Content-Type: text/plain", + f"Content-Length: {len(body)}", + "Connection: close"] + lines += [f"Cache-Control: {v}" for v in cc] + lines += extra + return ("\r\n".join(lines) + "\r\n\r\n" + body).encode() + + +# Cache-Control field lines and further response headers of the backend +NO_STORE = [ + [["no-store"], []], + [["private"], []], + [["private, no-store"], []], + [["private,no-store,max-age=0"], []], + [["private, no-store, max-age=0"], []], + [["private , no-store , max-age=0"], []], + [["max-age=60, private"], []], + [["max-age=60, no-store"], []], + [["no-store, max-age=60"], []], + [["no-store, foo=bar"], []], + [["foo=bar, no-store"], []], + [["foo=\"a, b\", no-store"], []], + [["private", "no-store", "max-age=0"], []], +] +STORED = [ + [["public, max-age=60"], []], + [["max-age=60"], []], + [["max-age=60, must-revalidate"], []], + [["must-revalidate, max-age=60"], []], + [["public, s-maxage=60"], []], + [["max-age=60, stale-while-revalidate=30"], []], + [["max-age=60, stale-if-error=30"], []], + [["foo=\"a, b\", max-age=60"], []], +] +QUALIFIED = [ + [["max-age=60, private=\"X-Secret\""], []], + [["max-age=60, private=\"X-Secret, Set-Cookie\""], []], + [["max-age=60, no-cache=\"X-Secret,Set-Cookie\""], []], +] +USER = [ + [["private, no-store, max-age=0"], []], +] +CASES = NO_STORE + STORED + QUALIFIED + USER +for _c in QUALIFIED: + _c[1] += ["Set-Cookie: a=b", "X-Secret: s", "X-Public: p"] + + +class TestCacheControl: + + @pytest.fixture(autouse=True, scope='class') + def _class_scope(self, env): + faker = _CacheFaker("127.0.0.1", env.http_port2, CASES) + faker.start() + cache_dir = os.path.join(env.gen_dir, "cache-control") + os.makedirs(cache_dir, exist_ok=True) + + conf = HttpdConf(env) + conf.start_vhost(domains=[f"test1.{env.http_tld}"], port=env.http_port, + doc_root="htdocs", with_ssl=False) + conf.add([ + f'CacheRoot "{cache_dir}"', + 'CacheEnable disk "/cc/"', + 'CacheHeader on', + 'CacheStorePrivate Off', + 'CacheStoreNoStore Off', + f"ProxyPass /cc/ http://127.0.0.1:{env.http_port2}/", + ]) + conf.end_vhost() + conf.install() + assert env.apache_restart() == 0 + TestCacheControl.faker = faker + TestCacheControl.run = uuid.uuid4().hex[:12] + yield + faker.stop() + + def get(self, env, index, cookie=None): + url = env.mkurl("http", "test1", f"/cc/{self.run}/{index}") + options = ["-H", f"Cookie: {cookie}"] if cookie else [] + r = env.curl_get(url, options=options) + assert r.response["status"] == 200, f"{r}" + return r.response + + def backend_hits(self, index): + return self.faker.hits.get(f"/{self.run}/{index}", 0) + + # a response that must not be stored is never served from the cache + @pytest.mark.parametrize("index", range(len(NO_STORE))) + def test_proxy_07_001(self, env, index): + cc = ", ".join(CASES[index][0]) + r1 = self.get(env, index) + r2 = self.get(env, index) + assert self.backend_hits(index) == 2, f"Cache-Control: {cc} was cached" + assert not r1["header"].get("x-cache", "").startswith("HIT") + assert not r2["header"].get("x-cache", "").startswith("HIT") + assert r2["body"] == b"hit=2 cookie=\n" + + # a response that may be stored is served from the cache + @pytest.mark.parametrize("index", range(len(NO_STORE), + len(NO_STORE) + len(STORED))) + def test_proxy_07_002(self, env, index): + cc = ", ".join(CASES[index][0]) + r1 = self.get(env, index) + r2 = self.get(env, index) + assert self.backend_hits(index) == 1, f"Cache-Control: {cc} not cached" + assert r2["header"]["x-cache"].startswith("HIT") + assert r2["body"] == r1["body"] + + # fields listed in private="..." and no-cache="..." are not stored, + # whether one or several are listed, the rest of the response is + @pytest.mark.parametrize("index", range(len(NO_STORE) + len(STORED), + len(NO_STORE) + len(STORED) + len(QUALIFIED))) + def test_proxy_07_003(self, env, index): + cc = ", ".join(CASES[index][0]) + r1 = self.get(env, index) + assert r1["header"]["x-secret"] == "s" + assert r1["header"]["set-cookie"] == "a=b" + r2 = self.get(env, index) + assert self.backend_hits(index) == 1, f"Cache-Control: {cc} not cached" + assert r2["header"]["x-cache"].startswith("HIT") + assert r2["header"]["x-public"] == "p" + assert "x-secret" not in r2["header"], f"X-Secret stored: {cc}" + if "Set-Cookie" in cc: + assert "set-cookie" not in r2["header"], f"Set-Cookie stored: {cc}" + + # a user specific response is not given to another request + def test_proxy_07_004(self, env): + index = len(CASES) - 1 + r1 = self.get(env, index, cookie="session=first") + assert r1["body"] == b"hit=1 cookie=session=first\n" + r2 = self.get(env, index) + assert b"session=first" not in r2["body"] + assert self.backend_hits(index) == 2