Commit 25a7daed authored by Stan Hu's avatar Stan Hu

Fix 400 errors not being logged in multipart middleware

In https://gitlab.com/gitlab-com/gl-infra/production/-/issues/5194, we
saw that Workhorse was logging 400 errors, but we could not see an
associated error in the API logs to explain why this was happening.

According to
https://github.com/rack/rack/blob/master/SPEC.rdoc#label-The+Body, Rack
v2.1+ now requires:

```
The Body must respond to `each` and must only yield String values
```

Since the response returned returned a plain string, we would only see
this mysterious error in the Puma stderr logs:

```
Read: #<NoMethodError: undefined method `each' for <String:0x1234>
```

Making things worse, Puma didn't display the backtrace, and this
exception wasn't caught by Sentry because the exception was happening
outside of the middleware chain.

We fix this by wrapping the string in an array.

Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/336609

Changelog: fixed
parent ccaf08c9
...@@ -177,7 +177,7 @@ module Gitlab ...@@ -177,7 +177,7 @@ module Gitlab
@app.call(env) @app.call(env)
end end
rescue UploadedFile::InvalidPathError => e rescue UploadedFile::InvalidPathError => e
[400, { 'Content-Type' => 'text/plain' }, e.message] [400, { 'Content-Type' => 'text/plain' }, [e.message]]
end end
end end
end end
......
...@@ -77,7 +77,8 @@ RSpec.describe Gitlab::Middleware::Multipart do ...@@ -77,7 +77,8 @@ RSpec.describe Gitlab::Middleware::Multipart do
result = subject result = subject
expect(result[0]).to eq(400) expect(result[0]).to eq(400)
expect(result[2]).to include('insecure path used') expect(result[2]).to be_a(Array)
expect(result[2].first).to include('insecure path used')
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment