search

nginx / unit

NGINX Unit - universal web app server - a lightweight and versatile open source server that simplifies the application stack by natively executing application code across eight different programming language runtimes.
https://unit.nginx.org
Apache License 2.0
5.29k stars 325 forks source link

Doesn't work after rails 7.1 update #974

Closed xeron closed 7 months ago

xeron commented 9 months ago

Updated a rails app from 7.0 to 7.1. Getting the following error in unit logs:

2023/10/18 02:02:58 [error] 977151#977151 [unit] #9: Ruby: Wrong header entry 'value' from application
2023/10/18 02:02:58 [error] 977151#977151 [unit] #9: Ruby: Failed to run ruby script

might be related to rack upgrade from 2.x to 3.x. Locking rack to ~> 2.0 fixes the issue.

Unit version: 1.31.0.

tippexs commented 9 months ago

Hi @xeron thanks for reaching out! Are you able to share a simple code example on how to reproduce it? This would be great!

Furthermore - can you help us investigate and fix this issue? It sounds like there was a change from rack 2 to 3. The error you can see in the log is produced in our Ruby Language module.

https://github.com/nginx/unit/blob/5265b7e0580edee488c07e0cc2953d88ae2aed1b/src/ruby/nxt_ruby.c#L892

Going up the call stack real quick we are getting the response headers from rack and checking if these are from type T_HASH. https://github.com/nginx/unit/blob/5265b7e0580edee488c07e0cc2953d88ae2aed1b/src/ruby/nxt_ruby.c#L842

Iterating over all headers and calling nxt_ruby_hash_info. As this fails we are going to fail and rais the error. So the question is what's new in the rack response that is now NOT a T_STRING?

On our website we are not mentioning anything about the version of Rack we are supporting. https://unit.nginx.org/keyfeatures/#supported-app-languages This should be chaged after we have fixed this bug.

tippexs commented 9 months ago

@ac000 I know it is good to look into something different but to me this looks like that we are telling the ruby application we are want the rack version 1.3?


    #define NXT_RUBY_RACK_API_VERSION_MAJOR  1
    #define NXT_RUBY_RACK_API_VERSION_MINOR  3

    version = rb_ary_new();

    rb_ary_push(version, UINT2NUM(NXT_RUBY_RACK_API_VERSION_MAJOR));
    rb_ary_push(version, UINT2NUM(NXT_RUBY_RACK_API_VERSION_MINOR));

    rb_hash_aset(hash_env, rb_str_new2("SCRIPT_NAME"), rb_str_new("", 0));
    rb_hash_aset(hash_env, rb_str_new2("rack.version"), version);
ac000 commented 9 months ago

I seem to have managed to setup a Rails 7.1.1 environment on Fedora 38 and created the basic rails app from here by doing

$ rails new gh-974

My Unit config is simply

{
    "listeners": {
        "[::1]:8080": {
            "pass": "applications/app"
        }
    },

     "applications": {
        "app": {
            "type": "ruby",
            "working_directory": "/home/andrew/src/ruby/gh-974",
            "script": "config.ru"
        }
    }
}

Hitting the app gives...

<!DOCTYPE html>
<html>
<head>
  <title>Ruby on Rails 7.1.1</title>
...
  <ul>
    <li><strong>Rails version:</strong> 7.1.1</li>
    <li><strong>Ruby version:</strong> ruby 3.2.2 (2023-03-30 revision e51014f9c0) [x86_64-linux]</li>
  </ul>
</body>

</html>

This is using Rack 3.0.8 as confirmed by strace(1)

15685 openat(AT_FDCWD, "/home/andrew/.local/share/gem/ruby/gems/rack-3.0.8/lib/rack/head.rb", O_RDONLY|O_NONBLOCK|O_CLOEXEC) = 18
15685 openat(AT_FDCWD, "/home/andrew/.local/share/gem/ruby/gems/rack-3.0.8/lib/rack/head.rb", O_RDONLY) = 18
15685 openat(AT_FDCWD, "/home/andrew/.local/share/gem/ruby/gems/rack-3.0.8/lib/rack/conditional_get.rb", O_RDONLY|O_NONBLOCK|O_CLOEXEC) = 18
15685 openat(AT_FDCWD, "/home/andrew/.local/share/gem/ruby/gems/rack-3.0.8/lib/rack/conditional_get.rb", O_RDONLY) = 18
15685 openat(AT_FDCWD, "/home/andrew/.local/share/gem/ruby/gems/rack-3.0.8/lib/rack/etag.rb", O_RDONLY|O_NONBLOCK|O_CLOEXEC) = 18
15685 openat(AT_FDCWD, "/home/andrew/.local/share/gem/ruby/gems/rack-3.0.8/lib/rack/etag.rb", O_RDONLY) = 18
15685 openat(AT_FDCWD, "/home/andrew/.local/share/gem/ruby/gems/rack-3.0.8/lib/rack/tempfile_reaper.rb", O_RDONLY|O_NONBLOCK|O_CLOEXEC) = 18
15685 openat(AT_FDCWD, "/home/andrew/.local/share/gem/ruby/gems/rack-3.0.8/lib/rack/tempfile_reaper.rb", O_RDONLY) = 18
15685 openat(AT_FDCWD, "/home/andrew/.local/share/gem/ruby/gems/rack-3.0.8/lib/rack/files.rb", O_RDONLY|O_NONBLOCK|O_CLOEXEC) = 18
15685 openat(AT_FDCWD, "/home/andrew/.local/share/gem/ruby/gems/rack-3.0.8/lib/rack/files.rb", O_RDONLY) = 18
ac000 commented 9 months ago

@ac000 I know it is good to look into something different but to me this looks like that we are telling the ruby application we are want the rack version 1.3?

    #define NXT_RUBY_RACK_API_VERSION_MAJOR  1
    #define NXT_RUBY_RACK_API_VERSION_MINOR  3

Not sure why that's hard-coded like that, I guess that was the current version at the time that ruby support was added in 37051b6c15cce7d6ab01c50e1086f8ef0b34e93d.

Although in practice it doesn't seem to cause an issue, it should probably be auto-detected somehow...

tippexs commented 9 months ago

This looks good - Have the same behaviour with a very simple config.ru file looking like this

run do |env|
  [200, {}, ["Hello World"]]
end

from https://github.com/rack/rack

I am using our latest Docker Container unit:ruby. @xeron please share your code as it work in general.

tippexs commented 9 months ago

@ac000 I know it is good to look into something different but to me this looks like that we are telling the ruby application we are want the rack version 1.3?

    #define NXT_RUBY_RACK_API_VERSION_MAJOR  1
    #define NXT_RUBY_RACK_API_VERSION_MINOR  3

Not sure why that's hard-coded like that, I guess that was the current version at the time that ruby support was added in 37051b6.

Although in practice it doesn't seem to cause an issue, it should probably be auto-detected somehow...

Thanks @ac000 for looking into this. We are pasing this along into the context, but I am not sure what the application is doing with it? Will try to find out.

xeron commented 9 months ago

Hey thanks for looking into that. I can't share the app in question but I'll try to create a simple reproducible app later. One specific thing about the app is it sets config.force_ssl = true in production.rb and direct unit request for testing is:

curl -v -H 'X-Forwarded-Proto: https' http://127.0.0.1:3000
xeron commented 9 months ago

@tippexs here's a minimal app which triggers the issue – https://github.com/xeron/gh-974-xeron

Run ./build_and_start.sh to build a unit docker image with the app included and configured then run curl -v -H 'X-Forwarded-Proto: https' http://127.0.0.1:8080.

ac000 commented 9 months ago

@xeron Thanks for the updates.

config.force_ssl = true in production.rb

Looks like that's on by default.

curl -v -H 'X-Forwarded-Proto: https' http://127.0.0.1:3000

Produced no error.

Using your provided application under Fedora 38 also produces no error.

Trying to build the docker image under Debian 11 results in the following error

Step 6/9 : RUN bundle install
 ---> Running in 6a26e3533fda
Bundler 2.4.10 is running, but your lockfile was generated with 2.4.21. Installing Bundler 2.4.21 and restarting using that version.
Fetching gem metadata from https://rubygems.org/.
Fetching bundler 2.4.21
Installing bundler 2.4.21
Fetching gem metadata from https://rubygems.org/.........
Fetching bigdecimal 3.1.4
Fetching concurrent-ruby 1.2.2
Installing bigdecimal 3.1.4 with native extensions
Installing concurrent-ruby 1.2.2
Fetching connection_pool 2.4.1
Installing connection_pool 2.4.1
Fetching minitest 5.20.0
Installing minitest 5.20.0
Fetching builder 3.2.4
Installing builder 3.2.4
Fetching erubi 1.12.0
Installing erubi 1.12.0
Fetching racc 1.7.1
Installing racc 1.7.1 with native extensions
Killed
The command '/bin/sh -c bundle install' returned a non-zero code
docker: Error response from daemon: invalid mount config for typd mount path: './unit/config/' mount path must be absolute.
See 'docker run --help'.
xeron commented 9 months ago

Trying to build the docker image under Debian 11 results in the following error

You can replace ./ with $(pwd)/.

I'm using Docker for Mac so passing absolute paths into the Docker VM is a complicated story.

xeron commented 9 months ago

I pushed the commit which removes the need for volume mount and just copies config.json into the container.

I also published the image so you don't even need to build it:

docker pull ixeron/gh-974-xeron
docker run -it -p 8080:8080 ixeron/gh-974-xeron

And in a separate shell:

curl -v -H 'X-Forwarded-Proto: https' http://127.0.0.1:8080
tippexs commented 9 months ago

I was able to do some more testing with it this morning. It looks like the issue is with SOMETHING that comes from config.force_ssl = true.

My configuration:

{
  "listeners": {
    "*:8080": {
      "pass": "applications/gh-974-xeron"
    },
    "*:8443": {
      "pass": "applications/gh-974-xeron",
      "tls": {
        "certificate": "bundle1"
      }
    }
  },
  "applications": {
    "gh-974-xeron": {
      "type": "ruby",
      "processes": {
        "max": 5,
        "spare": 1,
        "idle_timeout": 60
      },
      "working_directory": "/app",
      "script": "/app/config.ru",
      "environment": {
        "RAILS_ENV": "production"
      }
    }
  }
}

To generate self-signed certs for testing run

openssl req -x509 -nodes -days 365 -newkey rsa:2048 -keyout cert.key -out cert.crt

cat them together and send them to Unit

cat cert.key cert.crt | curl --unix-socket /var/run/control.unit.sock -X PUT --data-binary @- l/certificates/bundle1

My tests WITH config.force_ssl = true

Request HTTP://localhost:8080 w/o any additional header.

root@5589ee6ff083:/app# curl -v  http://localhost:8080/
*   Trying 127.0.0.1:8080...
* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET / HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.74.0
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 301 Moved Permanently
< content-type: text/html; charset=utf-8
< location: https://localhost:8080/
< Server: Unit/1.31.0
< Date: Thu, 19 Oct 2023 06:04:05 GMT
< Transfer-Encoding: chunked
<
* Connection #0 to host localhost left intact
root@5589ee6ff083:/app#

As we force the application to use https, we will be redirected! This works as expected!

Request HTTPS://localhost:8443 w/o any additional header.

Long story short! The same error as we alreday saw.

My tests WITH config.force_ssl = false

Request to plain-text endpoint 8080 will return the index page as well as to the TLS-Endpoint :8443 just fine.

Let's see what the docs are saying about force_ssl. https://guides.rubyonrails.org/configuring.html#config-force-ssl

It implemets the ActionDispatch:SSL https://api.rubyonrails.org/v7.1.0/classes/ActionDispatch/SSL.html

I have a feeling it has something to do with the headers introduced by config.force_ssl. Without this, I am totally able to work with the application. I have also tested to set headers in the post_controller.rb manually but with those and force_ssl = false the application still works.

My post_controller.rb file

  # GET /posts or /posts.json
  def index
    head :created, "X-Test": "PostController"
    head :created, "Strict-Transport-Security": "max-age=31536000; includeSubDomains; preload"
    head :created, "Set-Cookie": "timotest=sessionsomething; Domain=localhost:8443; Secure; HttpOnly"
    @posts = Post.all
  end
tippexs commented 9 months ago

Update: I was able to trace down the issue (at least a little bit more)

according to the documentation force_ssl will use ActionDispatch::SSL with some defaults. So far so good. This does not mean that we are not able to modify them.

With config.ssl_options we have access to this configuration. I have added / removed the defaults as long as the app was working / not working. HSTS and Cookies are fine BUT adding the redirect feature will crash with our lovely error.

Crash:

  # Force all access to the app over SSL, use Strict-Transport-Security, and use secure cookies.
  config.force_ssl = true
  config.ssl_options = {redirect: {host: "localhost", port: 8443}}

Works:

  config.force_ssl = true
  config.ssl_options = {redirect: false}

As soon as the redirect is off it just works!

ac000 commented 9 months ago

I pushed the commit which removes the need for volume mount and just copies config.json into the container.

Unfortunately this now errors with

Step 6/10 : RUN bundle install
 ---> Running in 02adb08fbad3
Bundler 2.4.10 is running, but your lockfile was generated with 2.4.21. Installing Bundler 2.4.21 and restarting using that version.
Fetching gem metadata from https://rubygems.org/.
Fetching bundler 2.4.21
Installing bundler 2.4.21
Fetching gem metadata from https://rubygems.org/.........
Fetching concurrent-ruby 1.2.2
Fetching bigdecimal 3.1.4
Installing bigdecimal 3.1.4 with native extensions
Installing concurrent-ruby 1.2.2
Fetching connection_pool 2.4.1
Installing connection_pool 2.4.1
Fetching minitest 5.20.0
Installing minitest 5.20.0
Fetching builder 3.2.4
Installing builder 3.2.4
Fetching erubi 1.12.0
Installing erubi 1.12.0
Fetching racc 1.7.1
Installing racc 1.7.1 with native extensions
Fetching crass 1.0.6
Installing crass 1.0.6
Fetching nio4r 2.5.9
Installing nio4r 2.5.9 with native extensions
Killed
The command '/bin/sh -c bundle install' returned a non-zero code: 137
Unable to find image 'gh-974-xeron:latest' locally
docker: Error response from daemon: pull access denied for gh-974-xeron, repository does not exist or may require 'docker login': denied: requested access to the resource is denied.
See 'docker run --help'.

I also published the image so you don't even need to build it:

docker pull ixeron/gh-974-xeron
docker run -it -p 8080:8080 ixeron/gh-974-xeron

Thanks, but no-go

$ docker run -it -p 8080:8080 ixeron/gh-974-xeron
WARNING: The requested image's platform (linux/arm64/v8) does not match the detected host platform (linux/amd64) and no specific platform was requested
standard_init_linux.go:219: exec user process caused: exec format error

x86_64 here...

But don't worry, I think I know how to reproduce it natively...

ac000 commented 9 months ago

I cannot seem to reproduce this issue...

With config.force_ssl set to true or false and if I use http for https, I always get the

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="utf-8" />
  <meta name="viewport" content="width=device-width, initial-scale=1">
  <meta name="turbo-visit-control" content="reload">
  <title>Action Controller: Exception caught</title>

page and never the ruby errors...

Although I do always seem to be getting a '500 Internal Server Error'

Do I need to do anything special after editing config/environments/production.rb or is restarting Unit enough?

I don't see that file getting read by Unit at all...

Hmm, bugger, I'm running in development mode!

Now that I switched to production mode, I'm getting a different page

<!DOCTYPE html>
<html>
<head>
  <title>We're sorry, but something went wrong (500)</title>

and application errors in the unit log, but still not those ruby errors.

With config.force_ssl = false I'm still seeing the 500 Internal Server Error.

With it set to true I do now see the re-direct.

With it set to true and trying to hit https direcrly I'm seeing the 500 Internal Server Error.

This is the error I'm getting...

I, [2023-10-19T16:16:37.320683 #22293]  INFO -- : [43b4a089-103e-40a7-8038-b6fb64db5248] Started GET "/" for ::1 at 2023-10-19 16:16:37 +0100
I, [2023-10-19T16:16:37.325048 #22293]  INFO -- : [43b4a089-103e-40a7-8038-b6fb64db5248] Processing by PostsController#index as */*
I, [2023-10-19T16:16:37.360861 #22293]  INFO -- : [43b4a089-103e-40a7-8038-b6fb64db5248]   Rendered layout layouts/application.html.erb (Duration: 20.0ms | Allocations: 2508)
I, [2023-10-19T16:16:37.361673 #22293]  INFO -- : [43b4a089-103e-40a7-8038-b6fb64db5248] Completed 500 Internal Server Error in 36ms (ActiveRecord: 9.3ms | Allocations: 4097)
E, [2023-10-19T16:16:37.363671 #22293] ERROR -- : [43b4a089-103e-40a7-8038-b6fb64db5248]   
[43b4a089-103e-40a7-8038-b6fb64db5248] ActionView::Template::Error (SQLite3::SQLException: no such table: posts):
[43b4a089-103e-40a7-8038-b6fb64db5248]     3: <h1>Posts</h1>
[43b4a089-103e-40a7-8038-b6fb64db5248]     4: 
[43b4a089-103e-40a7-8038-b6fb64db5248]     5: <div id="posts">
[43b4a089-103e-40a7-8038-b6fb64db5248]     6:   <% @posts.each do |post| %>
[43b4a089-103e-40a7-8038-b6fb64db5248]     7:     <%= render post %>
[43b4a089-103e-40a7-8038-b6fb64db5248]     8:     <p>
[43b4a089-103e-40a7-8038-b6fb64db5248]     9:       <%= link_to "Show this post", post %>
[43b4a089-103e-40a7-8038-b6fb64db5248]

In trying a multitude of combinations I have yet to see the ruby error...

Switching back to my simple test app...

I don't have any routes setup so get 404's, but it seems to be working .

ac000 commented 9 months ago

If someone wants to try this debug patch, it might give some clue...

diff --git a/src/ruby/nxt_ruby.c b/src/ruby/nxt_ruby.c
index bcb48f6b..554d4d08 100644
--- a/src/ruby/nxt_ruby.c
+++ b/src/ruby/nxt_ruby.c
@@ -888,13 +888,78 @@ nxt_ruby_hash_info(VALUE r_key, VALUE r_value, VALUE arg)

         goto fail;
     }
+    printf("RDBG: r_key : %s\n", StringValuePtr(r_key));
+
+    printf("RDBG: TYPE(r_value) : ");
+    switch (TYPE(r_value)) {
+    case T_NIL:
+        printf("NIL\n");
+        break;
+    case T_OBJECT:
+        printf("OBJECT\n");
+        break;
+    case T_CLASS:
+        printf("CLASS\n");
+        break;
+    case T_MODULE:
+        printf("MODULE\n");
+        break;
+    case T_FLOAT:
+        printf("FLOAT\n");
+        break;
+    case T_STRING:
+        printf("STRING\n");
+        break;
+    case T_REGEXP:
+        printf("REGEXP\n");
+        break;
+    case T_ARRAY:
+        printf("ARRAY\n");
+        break;
+    case T_HASH:
+        printf("HASH\n");
+        break;
+    case T_STRUCT:
+        printf("STRUCT\n");
+        break;
+    case T_BIGNUM:
+        printf("BIGNUM\n");
+        break;
+    case T_FIXNUM:
+        printf("FIXNUM\n");
+        break;
+    case T_COMPLEX:
+        printf("COMPLEX\n");
+        break;
+    case T_RATIONAL:
+        printf("RATIONAL\n");
+        break;
+    case T_FILE:
+        printf("FILE\n");
+        break;
+    case T_TRUE:
+    case T_FALSE:
+        printf("TRUE/FALSE\n");
+        break;
+    case T_DATA:
+        printf("DATA\n");
+        break;
+    case T_SYMBOL:
+        printf("SYMBOL\n");
+        break;
+    default:
+        printf("UNKNOWN\n");
+    }

     if (nxt_slow_path(TYPE(r_value) != T_STRING)) {
         nxt_unit_req_error(headers_info->req,
                            "Ruby: Wrong header entry 'value' from application");

+        printf("RDBG: r_value : -\n");
         goto fail;
     }
+    printf("RDBG: r_value : %s\n", StringValuePtr(r_value));
+

     value = RSTRING_PTR(r_value);
     value_end = value + RSTRING_LEN(r_value);

Just copy and paste the above into a file. e.g rdbg.patch in the Unit repository root then do

$ git apply rdbg.patch

If you get no output, it's all good. Then rebuild and install the ruby language module

$ make install

is all that's needed. Of course it kind of assumes you've already built and installed Unit from source...

Restart Unit, make a request and you should get something like (but I guess you'll get something different...)

RDBG: r_key : content-type
RDBG: TYPE(r_value) : STRING
RDBG: r_value : text/html; charset=UTF-8
RDBG: r_key : content-length
RDBG: TYPE(r_value) : STRING
RDBG: r_value : 1722
RDBG: r_key : x-request-id
RDBG: TYPE(r_value) : STRING
RDBG: r_value : fa0c3d93-23b6-47d3-a3e7-0338eee2727a
RDBG: r_key : x-runtime
RDBG: TYPE(r_value) : STRING
RDBG: r_value : 0.013644
RDBG: r_key : strict-transport-security
RDBG: TYPE(r_value) : STRING
RDBG: r_value : max-age=63072000; includeSubDomains

printed to stdout.

ac000 commented 9 months ago

I can reproduce the error message with the following

unit config

{
    "listeners": {
        "[::1]:8080": {
            "pass": "applications/app"
        },
    },

     "applications": {
        "app": {
            "type": "ruby",
            "working_directory": "/home/andrew/src/ruby",
            "script": "974.ru"
        }
    }
}

974.ru

app = Proc.new do |env|                                                         
    ["200", {                                                                   
        "Content-Type" => "text/plain",                                         
        "X-Empty-Header" => nil,
    }, ["Hello World"]]                                                         
end                                                                             

run app
$ curl http://localhost:8080/
<!DOCTYPE html><title>Error 503</title><p>Error 503.
RDBG: r_key : Content-Type
RDBG: TYPE(r_value) : STRING
RDBG: r_value : text/plain
RDBG: r_key : X-Empty-Header
RDBG: TYPE(r_value) : NIL
RDBG: r_value : -
2023/10/20 11:24:56 [error] 24829#24829 [unit] #10: Ruby: Wrong header entry 'value' from application
2023/10/20 11:24:56 [error] 24829#24829 [unit] #10: Ruby: Failed to run ruby script

You could be getting anything that isn't a T_STRING, but I think a T_NIL is the most likely.

xeron commented 9 months ago

As soon as the redirect is off it just works!

Just FYI redirect is not the problem in my case because in my test I'm doing the request using curl -v -H 'X-Forwarded-Proto: https' http://127.0.0.1:8080 which asks rails to return a response over HTTP but for HTTPS proxied client.

In my actual production the rails is behind the nginx which does SSL offload and adds X-Forwarded-Proto: https.

But don't worry, I think I know how to reproduce it natively...

@ac000 sorry I don't have time to make my example work under every OS / Docker setup. It was created on Intel MacBook using Docker for Mac and I know it works there, other OSes or Docker setups are out of my scope.

ac000 commented 9 months ago

@ac000 sorry I don't have time to make my example work under every OS / Docker setup. It was created on Intel MacBook using Docker for Mac and I know it works there, other OSes or Docker setups are out of my scope.

No problem. The reproducer is in fact really simple...

If yourself or @tippexs could try the debug patch to confirm you are seeing an empty header that would be helpful.

ac000 commented 9 months ago

Or if you're feeling lucky! You could just try this patch

diff --git a/src/ruby/nxt_ruby.c b/src/ruby/nxt_ruby.c
index bcb48f6b..2aaac797 100644
--- a/src/ruby/nxt_ruby.c
+++ b/src/ruby/nxt_ruby.c
@@ -889,15 +889,20 @@ nxt_ruby_hash_info(VALUE r_key, VALUE r_value, VALUE arg)
         goto fail;
     }

-    if (nxt_slow_path(TYPE(r_value) != T_STRING)) {
+    if (nxt_slow_path(TYPE(r_value) != T_STRING && TYPE(r_value) != T_NIL)) {
         nxt_unit_req_error(headers_info->req,
                            "Ruby: Wrong header entry 'value' from application");

         goto fail;
     }

-    value = RSTRING_PTR(r_value);
-    value_end = value + RSTRING_LEN(r_value);
+    if (TYPE(r_value) == T_STRING) {
+        value = RSTRING_PTR(r_value);
+        value_end = value + RSTRING_LEN(r_value);
+    } else {
+        value = "";
+        value_end = value;
+    }

     pos = value;

@@ -941,8 +946,13 @@ nxt_ruby_hash_add(VALUE r_key, VALUE r_value, VALUE arg)
     headers_info = (void *) (uintptr_t) arg;
     rc = &headers_info->rc;

-    value = RSTRING_PTR(r_value);
-    value_end = value + RSTRING_LEN(r_value);
+    if (TYPE(r_value) == T_STRING) {
+        value = RSTRING_PTR(r_value);
+        value_end = value + RSTRING_LEN(r_value);
+    } else {
+        value = "";
+        value_end = value;
+    }

     key_len = RSTRING_LEN(r_key);
xeron commented 9 months ago

@ac000 BTW I just realized your build of my sample app fails because of the memory usage. Seems like docker doesn't have enough memory for bundle install command.

Anyway, I applied your patch from https://github.com/nginx/unit/issues/974#issuecomment-1771309585 and rerun the test:

I, [2023-10-22T20:57:28.596104 #56]  INFO -- : [0cb91252-f64f-4051-9250-4b061c2bd283] Started GET "/" for 172.17.0.1 at 2023-10-22 20:57:28 +0000
I, [2023-10-22T20:57:28.596678 #56]  INFO -- : [0cb91252-f64f-4051-9250-4b061c2bd283] Processing by PostsController#index as */*
I, [2023-10-22T20:57:28.601112 #56]  INFO -- : [0cb91252-f64f-4051-9250-4b061c2bd283]   Rendered layout layouts/application.html.erb (Duration: 3.1ms | Allocations: 4556)
I, [2023-10-22T20:57:28.601211 #56]  INFO -- : [0cb91252-f64f-4051-9250-4b061c2bd283] Completed 200 OK in 4ms (Views: 2.9ms | ActiveRecord: 0.6ms | Allocations: 6239)
RDBG: r_key : x-frame-options
RDBG: TYPE(r_value) : STRING
RDBG: r_value : SAMEORIGIN
RDBG: r_key : x-xss-protection
RDBG: TYPE(r_value) : STRING
RDBG: r_value : 0
RDBG: r_key : x-content-type-options
RDBG: TYPE(r_value) : STRING
RDBG: r_value : nosniff
RDBG: r_key : x-permitted-cross-domain-policies
RDBG: TYPE(r_value) : STRING
RDBG: r_value : none
RDBG: r_key : referrer-policy
RDBG: TYPE(r_value) : STRING
RDBG: r_value : strict-origin-when-cross-origin
RDBG: r_key : link
RDBG: TYPE(r_value) : STRING
RDBG: r_value : </assets/application-e0cf9d8fcb18bf7f909d8d91a5e78499f82ac29523d475bf3a9ab265d5e2b451.css>; rel=preload; as=style; nopush,</assets/es-module-shims.min-4ca9b3dd5e434131e3bb4b0c1d7dff3bfd4035672a5086deec6f73979a49be73.js>; rel=preload; as=script; nopush
RDBG: r_key : content-type
RDBG: TYPE(r_value) : STRING
RDBG: r_value : text/html; charset=utf-8
RDBG: r_key : vary
RDBG: TYPE(r_value) : STRING
RDBG: r_value : Accept
RDBG: r_key : etag
RDBG: TYPE(r_value) : STRING
RDBG: r_value : W/"a6e66637d9726009ee1f0f7b5c56aac4"
RDBG: r_key : cache-control
RDBG: TYPE(r_value) : STRING
RDBG: r_value : max-age=0, private, must-revalidate
RDBG: r_key : set-cookie
RDBG: TYPE(r_value) : ARRAY
2023/10/22 20:57:28 [error] 56#56 [unit] #8: Ruby: Wrong header entry 'value' from application
RDBG: r_value : -
2023/10/22 20:57:28 [error] 56#56 [unit] #8: Ruby: Failed to run ruby script

Quick search through rack PRs points to https://github.com/rack/rack/pull/1793 and https://github.com/rack/rack/issues/2129.

Seems like starting with rack 3.0.0 header could be represented by a String or an Array of Strings.

ac000 commented 9 months ago

On Sun, 22 Oct 2023 13:58:39 -0700 Ivan Larionov @.***> wrote:

RDBG: r_key : set-cookie RDBG: TYPE(r_value) : ARRAY

OK, that's unexpected.

Seems like starting with rack 3.0.0 header could be represented by a String or an Array > of Strings.

Yes, you're right as indicated here.

Thanks for the pointer.

tippexs commented 9 months ago

Thanks for testing @xeron Was able to test the same thing! Looks like we have found the root cause! Great work team!

ac000 commented 9 months ago

This patch should make things work

It handles empty response headers and array response fields.

E.g The following

app = Proc.new do |env|                                                         
    ["200", {                                                                   
        "Content-Type" => "text/plain",                                         
        "X-Empty-Header" => nil,
        "X-Array-Header" => ["Item-1", nil, "Item-3", "Item-4"],
    }, ["Hello World\n"]]                                                         
end                                                                             

run app

produces

$ curl -v http://localhost:8080/
*   Trying 127.0.0.1:8080...
* connect to 127.0.0.1 port 8080 failed: Connection refused
*   Trying [::1]:8080...
* Connected to localhost (::1) port 8080 (#0)
> GET / HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/8.0.1
> Accept: */*
> 
< HTTP/1.1 200 OK
< Content-Type: text/plain
< X-Empty-Header: 
< X-Array-Header: Item-1; ; Item-3; Item-4
< Server: Unit/1.31.1
< Date: Mon, 23 Oct 2023 13:28:46 GMT
< Transfer-Encoding: chunked
< 
Hello World
* Connection #0 to host localhost left intact

You'll notice it also handles nil array values.

diff --git a/src/ruby/nxt_ruby.c b/src/ruby/nxt_ruby.c
index bcb48f6b..55f9966e 100644
--- a/src/ruby/nxt_ruby.c
+++ b/src/ruby/nxt_ruby.c
@@ -874,6 +874,18 @@ nxt_ruby_rack_result_headers(nxt_unit_request_info_t *req, VALUE result,
 }

+#define NXT_RUBY_SET_HDR_VALUE(r_value, value, value_end)                     \
+    do {                                                                      \
+        if (TYPE(r_value) == T_STRING) {                                      \
+            value = RSTRING_PTR(r_value);                                     \
+            value_end = value + RSTRING_LEN(r_value);                         \
+        } else {                                                              \
+            value = "";                                                       \
+            value_end = value;                                                \
+        }                                                                     \
+    } while (0);
+
+
 static int
 nxt_ruby_hash_info(VALUE r_key, VALUE r_value, VALUE arg)
 {
@@ -889,16 +901,44 @@ nxt_ruby_hash_info(VALUE r_key, VALUE r_value, VALUE arg)
         goto fail;
     }

-    if (nxt_slow_path(TYPE(r_value) != T_STRING)) {
+    if (nxt_slow_path(TYPE(r_value) != T_STRING
+                      && TYPE(r_value) != T_ARRAY
+                      && TYPE(r_value) != T_NIL)) {
         nxt_unit_req_error(headers_info->req,
                            "Ruby: Wrong header entry 'value' from application");

         goto fail;
     }

-    value = RSTRING_PTR(r_value);
-    value_end = value + RSTRING_LEN(r_value);
+    if (TYPE(r_value) == T_ARRAY) {
+        int     i;
+        int     arr_len = RARRAY_LEN(r_value);
+        VALUE   item;
+        size_t  len = 0;

+        for (i = 0; i < arr_len; i++) {
+            item = rb_ary_entry(r_value, i);
+            if (TYPE(item) != T_STRING && TYPE(item) != T_NIL) {
+                nxt_unit_req_error(headers_info->req,
+                                   "Ruby: Wrong header entry in 'value' array "
+                                   "from application");
+                goto fail;
+            }
+
+            if (TYPE(item) == T_STRING) {
+                len += RSTRING_LEN(item);
+            }
+
+            len += 2;    /* +2 for '; ' */
+        }
+
+        headers_info->fields++;
+        headers_info->size += RSTRING_LEN(r_key) + len - 2;
+
+        return ST_CONTINUE;
+    }
+
+    NXT_RUBY_SET_HDR_VALUE(r_value, value, value_end);
     pos = value;

     for ( ;; ) {
@@ -941,11 +981,57 @@ nxt_ruby_hash_add(VALUE r_key, VALUE r_value, VALUE arg)
     headers_info = (void *) (uintptr_t) arg;
     rc = &headers_info->rc;

-    value = RSTRING_PTR(r_value);
-    value_end = value + RSTRING_LEN(r_value);
-
     key_len = RSTRING_LEN(r_key);

+    if (TYPE(r_value) == T_ARRAY) {
+        int     i;
+        int     arr_len = RARRAY_LEN(r_value);
+        char    *field, *p;
+        VALUE   item;
+        size_t  len = 0;
+
+        for (i = 0; i < arr_len; i++) {
+            item = rb_ary_entry(r_value, i);
+
+            if (TYPE(item) == T_STRING) {
+                len += RSTRING_LEN(item);
+            }
+
+            len += 2;    /* +2 for '; ' */
+        }
+
+        field = nxt_malloc(len);
+        if (field == NULL) {
+            goto fail;
+        }
+
+        p = field;
+
+        for (i = 0; i < arr_len; i++) {
+            item = rb_ary_entry(r_value, i);
+            if (TYPE(item) == T_STRING) {
+                p = nxt_cpymem(p, RSTRING_PTR(item), RSTRING_LEN(item));
+            }
+
+            p = nxt_cpymem(p, "; ", 2);
+        }
+
+        len -= 2;
+
+        *rc = nxt_unit_response_add_field(headers_info->req,
+                                          RSTRING_PTR(r_key), key_len,
+                                          field, len);
+        nxt_free(field);
+
+        if (nxt_slow_path(*rc != NXT_UNIT_OK)) {
+            goto fail;
+        }
+
+        return ST_CONTINUE;
+    }
+
+    NXT_RUBY_SET_HDR_VALUE(r_value, value, value_end);
+
     pos = value;

     for ( ;; ) {
tippexs commented 9 months ago

Will apply and give it a spin! Should add some pytest for this as well! But LGTM! @xeron great first issue! Thanks!!

andrey-zelenkov commented 8 months ago

"X-Empty-Header" => nil,

Looks like this is one should be invalid since spec require explicitly String or Array:

Header values must be either a String instance, or an Array of String instances, such that each String instance must not contain characters below 037.

Also I tried similar example with the rackup and got following error from linter:

Rack::Lint::LintError: a header value must be a String or Array of Strings, but the value of 'x-empty' is a NilClass

P.S. but for some reason it doesn't complain about nil in array of headers.

ac000 commented 8 months ago

"X-Empty-Header" => nil,

Looks like this is one should be invalid since spec require explicitly String or Array:

Which 'spec' are you reading?

According to section 3.2 of RFC 7230 AIUI it is allowed by the following

field-value = *( field-content / obs-fold )

I.e 0 or more field-content and I don't see anything there that distinguishes between request/response headers.

Also in the past I have been required to send empty headers to the UK's Make Tax Digital service.

After discussing a missing header with us, you can omit the header or submit it with an empty value. You must not include a placeholder value, for example null or undefined.

(I don't think omitting it was originally an option).

Header values must be either a String instance, or an Array of String instances, such that each String instance must not contain characters below 037.

Also I tried similar example with the rackup and got following error from linter:

Rack::Lint::LintError: a header value must be a String or Array of Strings, but the value of 'x-empty' is a NilClass

Seems overly restrictive...

P.S. but for some reason it doesn't complain about nil in array of headers.

Anyway this is probably better discussed in the PR.

andrey-zelenkov commented 8 months ago

Anyway this is probably better discussed in the https://github.com/nginx/unit/pull/998.

Sure, will reply you there.

ac000 commented 7 months ago

The fix for this has been merged.