You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by ab...@apache.org on 2026/10/19 10:04:36 UTC
svn commit: r1939267 - in httpd/httpd/trunk: modules/cache test/modules/proxy
Author: abernal
Date: Mon Oct 19 10:04:36 2026
New Revision: 1939267
Log:
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.
This is a trunk-only regression from r1856507, which rewrote the
tokenizer as a state machine and dropped the T_HTTP_TOKEN_STOP check in
the token state that r1856493 and r1856500 still had.
PR: 70028
GitHub: closes #821
Added:
httpd/httpd/trunk/test/modules/proxy/test_07_cache_control.py
Modified:
httpd/httpd/trunk/modules/cache/cache_util.c
Modified: httpd/httpd/trunk/modules/cache/cache_util.c
==============================================================================
--- httpd/httpd/trunk/modules/cache/cache_util.c Mon Oct 19 09:28:59 2026 (r1939266)
+++ httpd/httpd/trunk/modules/cache/cache_util.c Mon Oct 19 10:04:36 2026 (r1939267)
@@ -977,6 +977,9 @@ apr_status_t cache_strqtok(char *str, ch
if (arg) *arg = wpos;
continue;
}
+ if (TEST_CHAR(*str, T_HTTP_TOKEN_STOP)) {
+ goto end;
+ }
break;
case IN_BETWEEN:
Added: httpd/httpd/trunk/test/modules/proxy/test_07_cache_control.py
==============================================================================
--- /dev/null 00:00:00 1970 (empty, because file is newly added)
+++ httpd/httpd/trunk/test/modules/proxy/test_07_cache_control.py Mon Oct 19 10:04:36 2026 (r1939267)
@@ -0,0 +1,163 @@
+import os
+import uuid
+
+import pytest
+
+from pyhttpd.conf import HttpdConf
+from .env import TCPFaker
+
+
+# Backend answering /<run>/<index> 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
Re: svn commit: r1939267 - in httpd/httpd/trunk: modules/cache test/modules/proxy
Posted by Arturo Bernal <ab...@apache.org>.
Hi Joe,
Thanks for catching this. I reproduced the failure and traced it to a race
between the first response completing and the cache entry being committed,
unrelated to the Cache-Control parsing change.
I've opened PR #864 <https://github.com/apache/httpd/pull/864> to address
the test flakiness. The fix has been validated locally, including the proxy
test suite.
Regards,
Arturo
On Tue, Oct 20, 2026 at 11:35 AM Joe Orton <jo...@apache.org> wrote:
> On Mon, Oct 19, 2026 at 10:04:36AM -0000, abernal@apache.org wrote:
> > Author: abernal
> > Date: Mon Oct 19 10:04:36 2026
> > New Revision: 1939267
> >
> > Log:
> > 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.
> >
> > This is a trunk-only regression from r1856507, which rewrote the
> > tokenizer as a state machine and dropped the T_HTTP_TOKEN_STOP check in
> > the token state that r1856493 and r1856500 still had.
>
> This one has tripped on Windows in this build -
>
> https://github.com/apache/httpd/actions/runs/39301887856/job/118228919357#step:11:1375
>
> FAILED
> modules/proxy/test_07_cache_control.py::TestCacheControl::test_proxy_07_002[17]
> - AssertionError: Cache-Control: public, s-maxage=60 not cached
> assert 2 == 1
> + where 2 = backend_hits(17)
> + where backend_hits =
> <modules.proxy.test_07_cache_control.TestCacheControl object at
> 0x0000017B8F932960>.backend_hits
> !!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures
> !!!!!!!!!!!!!!!!!!!!!!!!!!
>
> Regards, Joe
>
Re: svn commit: r1939267 - in httpd/httpd/trunk: modules/cache test/modules/proxy
Posted by Joe Orton <jo...@apache.org>.
On Mon, Oct 19, 2026 at 10:04:36AM -0000, abernal@apache.org wrote:
> Author: abernal
> Date: Mon Oct 19 10:04:36 2026
> New Revision: 1939267
>
> Log:
> 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.
>
> This is a trunk-only regression from r1856507, which rewrote the
> tokenizer as a state machine and dropped the T_HTTP_TOKEN_STOP check in
> the token state that r1856493 and r1856500 still had.
This one has tripped on Windows in this build -
https://github.com/apache/httpd/actions/runs/39301887856/job/118228919357#step:11:1375
FAILED modules/proxy/test_07_cache_control.py::TestCacheControl::test_proxy_07_002[17] - AssertionError: Cache-Control: public, s-maxage=60 not cached
assert 2 == 1
+ where 2 = backend_hits(17)
+ where backend_hits = <modules.proxy.test_07_cache_control.TestCacheControl object at 0x0000017B8F932960>.backend_hits
!!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!
Regards, Joe