fix: Resolve redirect loop when accessing /dagu through local nginx #720
Conversation
yottahmd
changed the title
basePath and accessing via reverse proxy
yottahmd
changed the title
basePath and accessing via reverse proxy
|
@chrishoage Thanks for your help. I adjusted the HTTP handler a bit again. Let me know if you find any issues! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #720 +/- ##
=======================================
Coverage 61.67% 61.67%
=======================================
Files 53 53
Lines 5260 5260
=======================================
Hits 3244 3244
Misses 1762 1762
Partials 254 254 Continue to review full report in Codecov by Sentry.
|
|
I would consider the configuration that is in the body of this PR a configuration error. Base paths do not match and they need to match. In your nginx config I would expect to see a redirect from /dagu to /dagu/ so that it would handle both routes. I will need to take a closer look when I have time, but I just wouldn't expect the configuration to work with the trailing sash at the end. I think a simpler solution might have been to I will try to behavior test this when I take a closer look. However if all the cases worked in the ticket I opened and you're happy with the code as is then all is well! |
|
I had some time to investigate this evening. This issue was caused from serving nginx on port 80 but exposing the port via 8081 in docker. Changing the port in nginx to 8081 (in my example) allowed the redirect to work correctly If you change both nginx and the exposed docker port to 80 this works server {
listen 80;
server_name dagu;
location /dagu/ {
proxy_pass http://dagu:8080;
proxy_set_header Host $host;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
}
}services:
nginx:
image: nginx:alpine
container_name: nginx
hostname: nginx
ports:
- "80:80"
volumes:
- ./nginx.conf:/etc/nginx/conf.d/default.conf
dagu:
image: dagu
container_name: dagu
hostname: dagu
environment:
- DAGU_BASE_PATH=/daguthe above works with the misconfigured However I'm having difficult reproducing what you are trying to fix. Can you provide more detailed steps to reproduce? Is this only when both nginx and dagu are running on the host? For example, the following still doesn't work for me on main server {
listen 80;
server_name dagu;
location /dagu/ {
proxy_pass http://dagu:8080;
proxy_set_header Host $host;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
}
}services:
nginx:
image: nginx:alpine
container_name: nginx
hostname: nginx
ports:
- "8081:80"
volumes:
- ./nginx.conf:/etc/nginx/conf.d/default.conf
dagu:
image: dagu
container_name: dagu
hostname: dagu
environment:
- DAGU_BASE_PATH=/daguWhen using curl to request and follow redirects it redirects to a different port (the one that the nginx server is exposed under) In any case if you're happy with the changes in |
|
Hey @chrishoage, thank you very much for the detailed investigation! I really appreciate it. I've put together a minimal reproduction setup to demonstrate the redirect loop issue I was encountering before the fix in this PR.
# nginx
worker_processes 1;
events {
worker_connections 1024;
}
http {
server {
listen 80;
server_name localhost;
location /dagu/ {
proxy_pass http://localhost:8080/;
proxy_set_header Host $host;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
}
}
}
dags: /Users/hamadayouta/dev/dagu/.local/dags
basePath: /dagu
port: 8080
$ curl -Lv http://localhost/dagu
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0* Trying [::1]:80...
* connect to ::1 port 80 failed: Connection refused
* Trying 127.0.0.1:80...
* Connected to localhost (127.0.0.1) port 80
> GET /dagu HTTP/1.1
> Host: localhost
> User-Agent: curl/8.4.0
> Accept: */*
>
< HTTP/1.1 303 See Other
< Server: nginx/1.27.2
< Date: Wed, 20 Nov 2024 07:06:38 GMT
< Content-Type: text/html; charset=utf-8
< Content-Length: 32
< Connection: keep-alive
< Location: /dagu
<
* Ignoring the response-body
{ [32 bytes data]
100 32 100 32 0 0 11535 0 --:--:-- --:--:-- --:--:-- 16000
* Connection #0 to host localhost left intact
* Issue another request to this URL: 'http://localhost/dagu'
* Found bundle for host: 0x122405300 [serially]
* Can not multiplex, even if we wanted to
* Re-using existing connection with host localhost
> GET /dagu HTTP/1.1
> Host: localhost
> User-Agent: curl/8.4.0
> Accept: */*
>
< HTTP/1.1 303 See Other
< Server: nginx/1.27.2
< Date: Wed, 20 Nov 2024 07:06:38 GMT
< Content-Type: text/html; charset=utf-8
< Content-Length: 32
< Connection: keep-alive
< Location: /dagu
<
* Ignoring the response-body
{ [32 bytes data]
100 32 100 32 0 0 8576 0 --:--:-- --:--:-- --:--:-- 8576
* Connection #0 to host localhost left intact
* Issue another request to this URL: 'http://localhost/dagu'
* Found bundle for host: 0x122405300 [serially]
* Can not multiplex, even if we wanted to
* Re-using existing connection with host localhost
> GET /dagu HTTP/1.1
> Host: localhost
> User-Agent: curl/8.4.0
> Accept: */*
... (repeated redirects) ...
100 32 100 32 0 0 808 0 --:--:-- --:--:-- --:--:-- 808
* Connection #0 to host localhost left intact
* Maximum (50) redirects followed
curl: (47) Maximum (50) redirects followedWith the fix in this PR, the redirect loop no longer occurs. The request to http://localhost/dagu (without the trailing slash) is now correctly handled and Dagu serves the content. I also created a separate PR (#722) to address the path cleaning you mentioned. Thanks again for your insights! |
|
@yohamta Thank you, I can reproduce. This is still a configuration issue. You should not have an ending See the documentation here https://docs.nginx.com/nginx/admin-guide/web-server/reverse-proxy/ The intent here is that the proxy server proxies the entire request to to the proxied sever, and that server handles the entire request (which is why setting the base path is important) You are instructing all requests to go to
I would personally not try to handle this case at all. As mentioned this is simply a configuration issue with nginx and not a problem with the redirect. The redirect is behaving correctly. If you insist on protecting against misconfigurations of a proxy as we have here I would recommend the following diff. Now - this would still not allow your misconfiguration to work - you'll get a 404 - however perhaps that's more desirable than a redirect loop. I would personally not recommended this, and I would personally use the simpler approach that was originally taken, as the added complexity is there to solve a problem of a broken nginx config. Please let me know if you have any further questions! diff --git a/internal/frontend/middleware/global.go b/internal/frontend/middleware/global.go
index 41767ac..f926e49 100644
--- a/internal/frontend/middleware/global.go
+++ b/internal/frontend/middleware/global.go
@@ -18,7 +18,6 @@ package middleware
import (
"context"
"net/http"
- "path"
"strings"
"github.com/dagu-org/dagu/internal/logger"
@@ -46,7 +45,6 @@ func SetupGlobalMiddleware(handler http.Handler) http.Handler {
)(next)
}
next = prefixChecker(next)
- next = cleanPath(next)
return next
}
@@ -103,37 +101,25 @@ func Setup(opts *Options) {
}
func prefixChecker(next http.Handler) http.Handler {
- handleRequest := func(w http.ResponseWriter, r *http.Request) {
- if strings.HasPrefix(r.URL.Path, "/api") {
- next.ServeHTTP(w, r)
- } else {
- defaultHandler.ServeHTTP(w, r)
- }
- }
return http.HandlerFunc(
func(w http.ResponseWriter, r *http.Request) {
- if basePath == "" || !strings.HasPrefix(r.URL.Path, basePath) {
- handleRequest(w, r)
- return
- }
- if basePath != "" && r.URL.Path == "/" {
+ // If the request does not come from a proxy and the path is the root
+ // path, redirect to the base path when one is set for convenience.
+ if basePath != "" && r.URL.Path == "/" && r.Header.Get("X-Forwarded-For") == "" {
http.Redirect(w, r, basePath, http.StatusSeeOther)
return
}
+
http.StripPrefix(basePath, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
- handleRequest(w, r)
+ if strings.HasPrefix(r.URL.Path, "/api") {
+ next.ServeHTTP(w, r)
+ } else {
+ defaultHandler.ServeHTTP(w, r)
+ }
})).ServeHTTP(w, r)
})
}
-func cleanPath(h http.Handler) http.Handler {
- return http.HandlerFunc(
- func(w http.ResponseWriter, r *http.Request) {
- r.URL.Path = path.Clean(r.URL.Path)
- h.ServeHTTP(w, r)
- })
-}
-
func cors(h http.Handler) http.Handler {
return http.HandlerFunc(
func(w http.ResponseWriter, r *http.Request) { |
|
Thank you for the detailed explanation. It was extremely helpful. I wasn’t aware of the trailing slash behavior in Nginx |
|
@yohamta just to clear up any confusion, that diff I provided was if you feel the need to protect against misconfigurations like you had it. I personally feel that I provided it in case you felt like you must protect against the redirect loop |
|
Oh, thank you for pointing this out! I agree 100% and will refactor this as well. |
This PR resolves a redirect loop issue when accessing Dagu through an Nginx reverse proxy at http://localhost/dagu.
The problem arose from Dagu's basePath configuration and the Nginx proxy settings. Accessing Dagu via the proxy resulted in an infinite redirect loop.
Reproduction Steps:
The issue can be reproduced with the following Nginx and Dagu configurations:
This PR fixes the issue by modifying HTTP handler for handling
basePath. Testing confirms that Dagu is now accessible through the proxy at http://localhost/dagu without redirect loops and also in the docker-compose setup provided in #718.