Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix example in expr.xml #485

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from
Open

Fix example in expr.xml #485

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3c7c339
Update expr.xml
nisbet-hubbard Oct 1, 2024
0856423
Use same condition argument
nisbet-hubbard Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/manual/expr.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -761,7 +761,8 @@ Require expr replace(%{REQUEST_METHOD}, 'E', 'O') == 'GET'"
Header set foo-checksum "expr=%{md5:foo}"

# This delays the evaluation of the condition clause compared to <If>
Header always set CustomHeader my-value "expr=%{REQUEST_URI} =~ m#^/special_path\.php$#"
SetEnvIf REQUEST_URI ^/special_path\.php$ special
Header always set CustomHeader my-value env=special

Copy link
Member

@covener covener Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original works fine for me and the replacement doesn't use expressions

$ curl -vk http://localhost/special_path.php 2>&1 |grep CustomHeader
< CustomHeader: my-value
$ curl -vk http://localhost/other_path.php 2>&1 |grep CustomHeader
$

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I find surprising about the existing example and comment is that delaying the evaluation of REQUEST_URI doesn't really make much sense. it should really check something like the CONTENT_TYPE or REQUEST_STATUS.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original works fine for me

Yes, sometimes it does. A case where it doesn’t is when an existing header is set by the application which one would like to override

@header( 'Cache-Control: no-cache' );

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a different result from the replacement directives? It seems like the override case is more likely about "onsuccess" vs "always" then about using an expression or an env condition

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my first thought. So I tested each version with always and implicit onsuccess

Header always set Cache-Control "max-age=604800" "expr=%{REQUEST_URI} =~ m#^/wp\.serviceworker$#"
SetEnvIf Request_URI ^/wp\.serviceworker$ worker
Header always set Cache-Control "max-age=604800" env=worker

Only the second works.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@covener I’ve identified the condition under which REQUEST_URI doesn’t work in expr=.

It’s when the path refers to something that’s not a static file, but dynamically generated by an application. This also applies to <If >. Previously, we observed the same limitation with <FilesMatch >, though I didn’t connect the dots at the time.

If you think this behaviour should be preserved, I suppose we can highlight the rule whenever it applies.

# Add a header to forward client's certificate SAN to some backend
RequestHeader set X-Client-SAN "expr=%{:join PeerExtList('subjectAltName'):}"
Expand Down