Skip to content

Ensure that a variable is used in the path when using request_uri, and do not warn on proxy_pass_normalized if request_uri is used#23

Merged
dvershinin merged 2 commits intodvershinin:masterfrom
MegaManSec:master
Feb 18, 2025
Merged

Conversation

@MegaManSec
Copy link

In my previous PR, I messed up the "valid" proxy_pass directive.

location /1/ {
  rewrite ^ $request_uri;
  rewrite ^/1(/.*) $1 break;
  return 400; # extremely important!
  proxy_pass http://127.0.0.1:8080;
}

will cause the backend server to receive a double-encoded path.

The correct configuration should be:

location /1/ {
  rewrite ^ $request_uri;
  rewrite ^/1(/.*) $1 break;
  return 400; # extremely important!
  proxy_pass http://127.0.0.1:8080/$1; # or http://127.0.0.1:8080$1; -- http://127.0.0.1:8080/ will NOT work and http://127.0.0.1:8080; will NOT work, as they will produce double-encoded paths.
}

This PR fixes that. It ensures that a warning is only shown if proxy_pass is used with a path and $1 is not set to $request_uri.

It also warns if $request_uri is used, but no variable is actually set in the proxy_pass directive (which will now warn on anybody that used my incorrect solution)

Also, turn find_directive_in_scope into find_directives_in_scope, allowing it
to turn into a list.
@dvershinin
Copy link
Owner

@MegaManSec can you please add those configs above to test and fp test?

Also would be great to heavily comment them e.g. with request URL and received by backend URL example, to fully understand what the type of config does.

I had used similar configuration elsewhere and no longer understand what it does with those rewrites and the need for return 400, etc. :-)

@MegaManSec
Copy link
Author

Great idea. Note, I will rebase when I'm done, too.

…uest_uri;', to ensure double-encoding does not occur.

Also add testcases.

Signed-off-by: Joshua Rogers <Joshua@Joshua.Hu>
@sonarqubecloud
Copy link

@MegaManSec
Copy link
Author

All done. Apologies for the spam due to the 20 commits.

@dvershinin dvershinin merged commit 3d4b64d into dvershinin:master Feb 18, 2025
10 checks passed
@dvershinin
Copy link
Owner

@MegaManSec great and thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants