unit icon indicating copy to clipboard operation
unit copied to clipboard

PHP: request to app failed without tailing slash in URI

Open mwoodpatrick opened this issue 2 years ago • 9 comments

I'm getting a 503 error when trying to access a php app I don't see any issues reported in the unit log file and the php module is enabled. Is there any logging I can enable to see why I'm getting the error, any pointers as to what's causing the issue or best way to debug would be appreciated

curl http://localhost/example_php

Error 503

Error 503.

config from unit is { "listeners": { ":80": { "pass": "routes" } }, "routes": [ { "match": { "uri": "/static" }, "action": { "share": "/projects/www/$uri" } }, { "match": { "uri": "/example_php" }, "action": { "pass": "applications/example_php" } } ], "applications": { "example_php": { "type": "php", "processes": 2, "root": "/projects/www/php/phpinfo-app", "index": "index.php" } } } log file from unit 2022/09/10 12:55:13 [info] 2095#2095 discovery started 2022/09/10 12:55:13 [notice] 2095#2095 module: java 11.0.15 "/usr/lib64/unit/modules/java11.unit.so" 2022/09/10 12:55:13 [notice] 2095#2095 module: java 1.8.0_332 "/usr/lib64/unit/modules/java8.unit.so" 2022/09/10 12:55:13 [notice] 2095#2095 module: perl 5.34.1 "/usr/lib64/unit/modules/perl.unit.so" 2022/09/10 12:55:13 [notice] 2095#2095 module: php 8.1.5 "/usr/lib64/unit/modules/php.unit.so" 2022/09/10 12:55:13 [notice] 2095#2095 module: python 3.10.4 "/usr/lib64/unit/modules/python3.10.unit.so" 2022/09/10 12:55:13 [notice] 2095#2095 module: ruby 3.1.2 "/usr/lib64/unit/modules/ruby.unit.so" 2022/09/10 12:55:13 [info] 2091#2091 controller started 2022/09/10 12:55:13 [notice] 2091#2091 process 2095 exited with code 0 2022/09/10 12:55:13 [info] 2097#2097 router started 2022/09/10 12:55:13 [info] 2097#2097 OpenSSL 3.0.5 5 Jul 2022, 30000050 2022/09/10 12:55:13 [info] 2098#2098 "example_php" prototype started 2022/09/10 12:55:13 [info] 2099#2099 "example_php" application started 2022/09/10 12:55:13 [info] 2100#2100 "example_php" application started

mwoodpatrick avatar Sep 10 '22 18:09 mwoodpatrick

Good Morning @mwoodpatrick . This looks like a problem within your PHP code. As you can see from the logs the PHP application started successfully.

A quick thing that is unlikely the problem but can you please change your static file share configuration form /projects/www/$uri to /projects/www$uri.

https://unit.nginx.org/configuration/#handling-actions

According to our documentation, the $uri does contain the leading /, so therefore there is no need to add it again.

May I ask you to add a access.log to your configuration and can you share this as well? https://unit.nginx.org/configuration/#access-log

tippexs avatar Sep 12 '22 05:09 tippexs

If I change the static file path to remove the "/" I get the same result

For the PHP issue the access log shows:

127.0.0.1 - - [12/Sep/2022:08:28:45 -0700] "GET /example_php HTTP/1.1" 503 54 "-" "curl/7.82.0" 127.0.0.1 - - [12/Sep/2022:08:28:49 -0700] "GET /example_php/ HTTP/1.1" 200 85786 "-" "curl/7.82.0"

It seems to work if I terminate the URL with a "/" and if I don't I get the 503 error

I am seeing an error in the log file

2022/09/12 08:28:29 [alert] 19997#19997 failed to store current configuration

even though the access os working anyone know what this issue is or how best to debug

mwoodpatrick avatar Sep 12 '22 15:09 mwoodpatrick

Needing to terminate the URL with a '/' seems to be intended, from the docs...

index | Filename appended to any URI paths ending with a slash; applies if script is omitted.

ac000 avatar Sep 13 '22 22:09 ac000

Thanks for the info, much appreciated. It would be helpful if the log had something clearer than just error 503, this seems rather obscure

mwoodpatrick avatar Sep 14 '22 00:09 mwoodpatrick

I agree, this could probably be handled better.

IIRC (memory is likely a little hazy) in the early days of the web if the last component of a URL was a directory you had to use a trailing '/' to signify it as such.

Over time severs relaxed a little and you could omit the trailing '/' and the server would figure out if it was a directory or file.

I will need to do some tracing and digging through the source to see what exactly Unit is doing here.

ac000 avatar Sep 14 '22 11:09 ac000

@mwoodpatrick Here's a patch against current unit that allows to skip the trailing '/' on PHP URLs that are directories, if you'd like to give it a spin...

(updated to fix a potential buffer overflow)

diff --git a/src/nxt_php_sapi.c b/src/nxt_php_sapi.c
index 68ef07eb..6d2f320e 100644
--- a/src/nxt_php_sapi.c
+++ b/src/nxt_php_sapi.c
@@ -958,11 +958,28 @@ nxt_php_dynamic_request(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r)
 {
     u_char     *p;
     nxt_str_t  path, script_name;
-    nxt_int_t  ret;
+    nxt_int_t  ret, isdir;
 
     path.length = r->path_length;
     path.start = nxt_unit_sptr_get(&r->path);
 
+    isdir = 0;
+    if (path.start[path.length - 1] != '/'
+        && ctx->root->length + path.length < PATH_MAX)
+    {
+        char         tpath[PATH_MAX];
+        struct stat  sb;
+
+        p = nxt_cpymem(tpath, ctx->root->start, ctx->root->length);
+        p = nxt_cpymem(p, path.start, path.length);
+        *p = '\0';
+
+        ret = stat(tpath, &sb);
+        if (ret == 0 && S_ISDIR(sb.st_mode)) {
+            isdir = 1;
+        }
+    }
+
     nxt_str_null(&script_name);
 
     ctx->path_info.start = (u_char *) strstr((char *) path.start, ".php/");
@@ -972,7 +989,7 @@ nxt_php_dynamic_request(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r)
 
         ctx->path_info.length = r->path_length - path.length;
 
-    } else if (path.start[path.length - 1] == '/') {
+    } else if (path.start[path.length - 1] == '/' || isdir == 1) {
         script_name = *ctx->index;
 
     } else {
@@ -988,6 +1005,7 @@ nxt_php_dynamic_request(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r)
 
     ctx->script_filename.length = ctx->root->length
                                   + path.length
+                                  + isdir
                                   + script_name.length;
 
     p = nxt_malloc(ctx->script_filename.length + 1);
@@ -1004,6 +1022,7 @@ nxt_php_dynamic_request(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r)
 
     p = nxt_cpymem(p, ctx->root->start, ctx->root->length);
     p = nxt_cpymem(p, path.start, path.length);
+    p = nxt_cpymem(p, "/", isdir);
 
     if (script_name.length > 0) {
         p = nxt_cpymem(p, script_name.start, script_name.length);

ac000 avatar Sep 16 '22 01:09 ac000

@ac000 linking for visibility. https://github.com/nginx/unit/issues/717 .Looks like the same root cause.

tippexs avatar Sep 16 '22 17:09 tippexs

@ac000 With the patch, requests that don't end with / will have to add one more syscall stat(...). And most requests are like this.

hongzhidao avatar Sep 23 '22 05:09 hongzhidao

@mwoodpatrick Here's an preliminary version of an alternative fix for this issue. It uses a 301 redirect to point to the path with a trailing '/', if it's a directory. This is how the likes of nginx, Apache & lighttpd do it. (It also skips the check on .php URLs).

Hopefully the 301 redirect works for you... (curl -L ...)

diff --git a/src/nxt_php_sapi.c b/src/nxt_php_sapi.c
index 68ef07eb..dc308da2 100644
--- a/src/nxt_php_sapi.c
+++ b/src/nxt_php_sapi.c
@@ -14,6 +14,7 @@
 #include <nxt_router.h>
 #include <nxt_unit.h>
 #include <nxt_unit_request.h>
+#include <nxt_http.h>
 
 
 #if PHP_VERSION_ID >= 50400
@@ -920,6 +921,95 @@ nxt_realpath(const void *c)
 }
 
 
+static char *
+nxt_php_create_location(const nxt_unit_request_info_t *req, uint32_t *size)
+{
+    char        *p, *url;
+    nxt_str_t   server_name, local_port, path, query;
+    const char  *proto;
+
+    proto = (req->request->tls == 1) ? "https://" : "http://";
+
+    server_name.start = nxt_unit_sptr_get(&req->request->server_name);
+    server_name.length = req->request->server_name_length;
+    local_port.start = nxt_unit_sptr_get(&req->request->local_port);
+    local_port.length = req->request->local_port_length;
+    path.start = nxt_unit_sptr_get(&req->request->path);
+    path.length = req->request->path_length;
+    query.start = nxt_unit_sptr_get(&req->request->query);
+    query.length = req->request->query_length;
+
+    url = nxt_malloc(8                  /* max length of proto */
+                     + server_name.length
+                     + 1 + local_port.length
+                     + path.length  + 1 /* for the '/' */
+                     + query.length + 1 /* for the '?' */
+                     + 1);
+    if (nxt_slow_path(url == NULL)) {
+        return NULL;
+    }
+
+    p = nxt_cpymem(url, proto, strlen(proto));
+    p = nxt_cpymem(p, server_name.start, server_name.length);
+
+    if (nxt_strcmp(local_port.start, "80") != 0
+        && nxt_strcmp(local_port.start, "443") != 0)
+    {
+         p = nxt_cpymem(p, ":", 1);
+         p = nxt_cpymem(p, local_port.start, local_port.length);
+    }
+
+    p = nxt_cpymem(p, path.start, path.length);
+    p = nxt_cpymem(p, "/", 1);
+
+    if (query.length > 0) {
+        p = nxt_cpymem(p, "?", 1);
+        p = nxt_cpymem(p, query.start, query.length);
+    }
+
+    *p = '\0';
+
+    *size = p - url;
+
+    return url;
+}
+
+
+static void
+nxt_php_do_err_response(nxt_unit_request_info_t *req,
+                        nxt_http_status_t status)
+{
+    nxt_int_t  ec;
+
+    ec = NXT_UNIT_OK;
+
+    switch (status) {
+    case NXT_HTTP_MOVED_PERMANENTLY:
+        char      *url;
+        uint32_t  size;
+
+        url = nxt_php_create_location(req, &size);
+        if (nxt_slow_path(url == NULL)) {
+            ec = NXT_UNIT_ERROR;
+            goto out_request_done;
+        }
+
+        nxt_unit_response_init(req, status, 1, nxt_length("Location") + size);
+        nxt_unit_response_add_field(req, "Location", nxt_length("Location"),
+                                    url, size);
+        nxt_free(url);
+        break;
+
+    default:
+        ec = NXT_UNIT_ERROR;
+    }
+
+out_request_done:
+
+    nxt_unit_request_done(req, ec);
+}
+
+
 static void
 nxt_php_request_handler(nxt_unit_request_info_t *req)
 {
@@ -963,6 +1053,26 @@ nxt_php_dynamic_request(nxt_php_run_ctx_t *ctx, nxt_unit_r
equest_t *r)
     path.length = r->path_length;
     path.start = nxt_unit_sptr_get(&r->path);
 
+    if (((path.length > 4
+          && nxt_memcmp(path.start + (path.length - 4), ".php", 4) != 0)
+         || path.length < 5)
+        && path.start[path.length - 1] != '/'
+        && ctx->root->length + path.length < PATH_MAX)
+    {
+        char         tpath[PATH_MAX];
+        struct stat  sb;
+
+        p = nxt_cpymem(tpath, ctx->root->start, ctx->root->length);
+        p = nxt_cpymem(p, path.start, path.length);
+        *p = '\0';
+
+        ret = stat(tpath, &sb);
+        if (ret == 0 && S_ISDIR(sb.st_mode)) {
+            nxt_php_do_err_response(ctx->req, NXT_HTTP_MOVED_PERMANENTLY);
+            return;
+        }
+    }
+
     nxt_str_null(&script_name);
 
     ctx->path_info.start = (u_char *) strstr((char *) path.start, ".php/");
diff --git a/src/nxt_router.c b/src/nxt_router.c
index f02bf3f2..56676e57 100644
--- a/src/nxt_router.c
+++ b/src/nxt_router.c
@@ -5254,6 +5254,12 @@ nxt_router_prepare_msg(nxt_task_t *task, nxt_http_request_t *r,
     p = nxt_cpymem(p, nxt_sockaddr_address(r->local), r->local->address_length);
     *p++ = '\0';
 
+    req->local_port_length = nxt_sockaddr_port_length(r->local);
+    nxt_unit_sptr_set(&req->local_port, p);
+    p = nxt_cpymem(p, nxt_sockaddr_port(r->local),
+                   nxt_sockaddr_port_length(r->local));
+    *p++ = '\0';
+
     req->tls = r->tls;
     req->websocket_handshake = r->websocket_handshake;
 
diff --git a/src/nxt_unit_request.h b/src/nxt_unit_request.h
index 5dbf648d..febc599c 100644
--- a/src/nxt_unit_request.h
+++ b/src/nxt_unit_request.h
@@ -19,6 +19,7 @@ struct nxt_unit_request_s {
     uint8_t               version_length;
     uint8_t               remote_length;
     uint8_t               local_length;
+    uint8_t               local_port_length;
     uint8_t               tls;
     uint8_t               websocket_handshake;
     uint8_t               app_target;
@@ -39,6 +40,7 @@ struct nxt_unit_request_s {
     nxt_unit_sptr_t       version;
     nxt_unit_sptr_t       remote;
     nxt_unit_sptr_t       local;
+    nxt_unit_sptr_t       local_port;
     nxt_unit_sptr_t       server_name;
     nxt_unit_sptr_t       target;
     nxt_unit_sptr_t       path;

ac000 avatar Sep 27 '22 01:09 ac000

The fix for this has been merged. a032744

ac000 avatar Nov 02 '22 15:11 ac000