Improve varnish config, add support for URIBAN by regex by MarttiR · Pull Request #495 · wunderio/charts

MarttiR · 2025-08-15T14:58:48Z

Changes

(drupal/varnish): Fix issue with Varnish varying cached static files by cookies
(drupal/varnish, frontend/varnish): Fix URIBAN behavior, add support for URIBAN by regex

Notes

Making a URIBAN request with a header like x-url-invalidate-pattern: ^/sites/default/files/foo/bar\.pdf will purge the cache from entries matching the pattern
Making a URIBAN request with no such header will purge cache from entries which exactly match the Host header and the URL, just as before
For the frontend chart, the BAN/URIBAN handling is moved before the allowed method checks (one would assume the previous placement made purging entirely non-functional)

MarttiR · 2025-08-15T14:59:08Z

The CircleCI failure does not seem related to this PR.

ragnarkurmwunder · 2025-08-18T08:03:42Z


-      if (req.url ~ "\.(png|gif|jpg|tif|tiff|ico|webp|swf|css|js|pdf|doc|xls|ppt|zip)(\?.*)?$") {
-        // Forcing a lookup with static file requests
+      if (req.url ~ "\.(png|gif|jpg|svg|tif|tiff|ico|webp|swf|css|js|pdf|doc|xls|ppt|zip|woff|eot|ttf|bmp|bz2)$") {


Sort the extensions alphabetically?

Done in 9e62b7f.

Also noticed that the Accept-Encoding block tried to skip compressing already compressed files, but as there is significant overlap with the static files handling and the block was after the static files block, it was actually not doing anything for many types of static files. 6859a11 moves it to before the static files block.

…lock

ragnarkurmwunder · 2025-08-25T09:30:40Z

+      if (req.method == "URIBAN") {
+        # Admin port is only exposed to internal network
+        if (!client.ip ~ purge) {
+          return (synth(403, "Not allowed."));


Consider "IP not allowed".
Reasoning: suppose someone gets the error, and has no clue of this code here, it could be a guesswork why the error.

AFAIK, the message set here is only logged in varnishlog, not transmitted via HTTP.

ragnarkurmwunder · 2025-08-25T09:36:19Z

-      if (req.url ~ "\.(png|gif|jpg|tif|tiff|ico|webp|swf|css|js|pdf|doc|xls|ppt|zip)(\?.*)?$") {
-        // Forcing a lookup with static file requests
+      if (req.http.Accept-Encoding) {
+        if (req.url ~ "\.(bz2|eot|gif|gz|ico|jpg|mp3|ogg|pdf|png|svg|swf|tbz|tgz|tif|tiff|ttf|webp|woff|zip)(\?.*)?$") {


Can we use non-capturing regex: (?:...)?

Varnish uses PCRE so it would probably work, I will try it out.

Changed regexps using groups to use non-capturing groups in a3e28a4 and 2294948.

ragnarkurmwunder · 2025-08-25T09:37:43Z

+      }
+
+      if (req.url ~ "\.(bmp|bz2|css|doc|eot|gif|ico|jpg|js|pdf|png|ppt|svg|swf|tif|tiff|ttf|webp|woff|xls|zip)$") {
+        # Static file request do not vary on cookies


Good point. I like this.

ragnarkurmwunder · 2025-08-25T09:53:27Z

-          return (hash);
-        }
-        elseif (req.http.Cookie ~ "(SESS[a-z0-9]+|SSESS[a-z0-9]+)") {
+        if (req.http.Cookie ~ "(SESS[a-z0-9]+|SSESS[a-z0-9]+)") {


simplify?
``"S?SESS[a-z0-9]+="`

Good idea, done in cba1254.

ragnarkurmwunder · 2025-08-25T09:58:01Z

I reviewed it to my capacity, but I didn't have the full understanding of it, but I have it roughly, so I could not really review the algorithmic aspect. I did not test it, but reviewed it visually.

Copilot

Pull Request Overview

This PR improves Varnish caching configuration by fixing static file caching issues and enhancing URIBAN functionality with regex pattern support. The changes address cookie-based cache variations for static files and add more flexible cache invalidation capabilities.

Key changes:

Fixed static file cache variation issue by removing cookies from static file requests
Enhanced URIBAN method to support regex-based cache invalidation via x-url-invalidate-pattern header
Moved BAN/URIBAN handling to occur before method validation checks

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
frontend/templates/varnish-configmap-vcl.yaml	Moved BAN/URIBAN logic earlier in request processing, added regex URIBAN support, improved regex patterns
drupal/templates/varnish-configmap-vcl.yaml	Split BAN/URIBAN into separate handlers, added regex URIBAN support, fixed static file cookie handling, reorganized request processing flow

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-08-26T09:02:46Z

+        # If x-url-invalidate-pattern header is present,
+        # use it to match URLs in stored objects. (ban by regex pattern)
+        if (req.http.x-url-invalidate-pattern) {
+          ban("obj.http.x-url ~ " + req.http.x-url-invalidate-pattern);


The ban statements reference obj.http.x-url but there's no evidence that this header is set during cache storage. The original code used req.url for URIBAN matching. If x-url is not properly set on cached objects, these ban statements will not match any cached entries.

obj.http.x-url is stored in the cached objects here

Copilot · 2025-08-26T09:02:46Z

-          return (hash);
-        }
-        elseif (req.http.Cookie ~ "(SESS[a-z0-9]+|SSESS[a-z0-9]+)") {
+        if (req.http.Cookie ~ "S?SESS[a-z0-9]+=") {


The regex pattern S?SESS[a-z0-9]+= is incorrect. The S? makes the 'S' optional, which would match both 'SESS' and 'ESS' patterns. Based on the original code that checked for (SESS[a-z0-9]+|SSESS[a-z0-9]+), this should be (S?SESS[a-z0-9]+=) or (?:S?SESS[a-z0-9]+=) to properly match both SESS and SSESS cookies.

Suggested change

if (req.http.Cookie ~ "S?SESS[a-z0-9]+=") {

if (req.http.Cookie ~ "(S?SESS[a-z0-9]+=)") {

That's not true, the pattern is correct. This can be verified using regexr.com:

MarttiR added 3 commits August 15, 2025 17:27

(drupal/varnish) Don't use cookies to vary cache for static files

ead552d

(drupal/varnish) Fix URIBAN behavior, add support for URIBAN by regex

11cbd62

(frontend/varnish) Fix URIBAN behavior, add support for URIBAN by regex

b8ef26d

ragnarkurmwunder reviewed Aug 18, 2025

View reviewed changes

MarttiR added 2 commits August 18, 2025 12:09

(drupal/varnish) Sort static file extensions alphabetically

9e62b7f

(drupal/varnish) Move Accept-Encoding block before the static files b…

6859a11

…lock

MarttiR requested a review from ragnarkurmwunder August 19, 2025 09:16

ragnarkurmwunder reviewed Aug 25, 2025

View reviewed changes

ragnarkurmwunder approved these changes Aug 25, 2025

View reviewed changes

MarttiR added 3 commits August 25, 2025 16:23

(drupal/varnish) Use non-capturing groups in regexes

a3e28a4

(frontend/varnish) Use non-capturing groups in regexes

2294948

(drupal/varnish) Simplify session cookie regex

cba1254

Rade333 requested a review from Copilot August 26, 2025 09:00

Copilot AI reviewed Aug 26, 2025

View reviewed changes

Rade333 changed the base branch from master to release/2025-08-26 August 26, 2025 09:18

Rade333 merged commit 8eae1d5 into release/2025-08-26 Aug 26, 2025
2 of 13 checks passed

Rade333 deleted the feature/regex-uriban branch August 26, 2025 09:19

Rade333 mentioned this pull request Aug 26, 2025

Release 2025-08-26 #498

Merged

	if (req.http.Cookie ~ "S?SESS[a-z0-9]+=") {
	if (req.http.Cookie ~ "(S?SESS[a-z0-9]+=)") {

Conversation

MarttiR commented Aug 15, 2025

Changes

Notes

Uh oh!

MarttiR commented Aug 15, 2025

Uh oh!

ragnarkurmwunder Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarttiR Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ragnarkurmwunder Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ragnarkurmwunder commented Aug 25, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ragnarkurmwunder Aug 18, 2025 •

edited

Loading

MarttiR Aug 18, 2025 •

edited

Loading

ragnarkurmwunder Aug 25, 2025 •

edited

Loading