Skip to content

Commit 19cefe5

Browse files
authored
[#225]: feature: http bad request code (400) instead of 500
2 parents 94f81a4 + 3b5e751 commit 19cefe5

15 files changed

Lines changed: 571 additions & 321 deletions

.github/workflows/linux.yml

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ jobs:
4242
echo "dir=$(composer config cache-files-dir)" >> $GITHUB_OUTPUT
4343
4444
- name: Init Composer Cache # Docs: <https://git.io/JfAKn#php---composer>
45-
uses: actions/cache@v3
45+
uses: actions/cache@v4
4646
with:
4747
path: ${{ steps.composer-cache.outputs.dir }}
4848
key: ${{ runner.os }}-composer-${{ matrix.php }}-${{ hashFiles('**/composer.json') }}
@@ -52,7 +52,7 @@ jobs:
5252
run: cd tests/php_test_files && composer update --prefer-dist --no-progress --ansi
5353

5454
- name: Init Go modules Cache # Docs: <https://git.io/JfAKn#go---modules>
55-
uses: actions/cache@v3
55+
uses: actions/cache@v4
5656
with:
5757
path: ~/go/pkg/mod
5858
key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }}
@@ -95,25 +95,35 @@ jobs:
9595
uses: actions/upload-artifact@v4
9696
with:
9797
name: coverage
98-
path: ./tests/coverage-ci/httpu.out
98+
path: ./tests/coverage-ci
9999

100100
codecov:
101101
name: Upload codecov
102102
runs-on: ubuntu-latest
103103
needs:
104104
- http_test
105-
106105
timeout-minutes: 60
107106
steps:
107+
- name: Check out code
108+
uses: actions/checkout@v6
108109
- name: Download code coverage results
109110
uses: actions/download-artifact@v8
111+
with:
112+
name: coverage
113+
path: coverage
110114
- run: |
111115
echo 'mode: atomic' > summary.txt
112-
tail -q -n +2 *.out >> summary.txt
113-
sed -i '2,${/roadrunner/!d}' summary.txt
114-
116+
tail -q -n +2 coverage/*.out >> summary.txt
117+
awk '
118+
NR == 1 { print; next }
119+
/^github\.com\/roadrunner-server\/http\/v5\// {
120+
sub(/^github\.com\/roadrunner-server\/http\/v5\//, "", $0)
121+
print
122+
}
123+
' summary.txt > summary.filtered.txt
124+
mv summary.filtered.txt summary.txt
115125
- name: upload to codecov
116126
uses: codecov/codecov-action@v5 # Docs: <https://github.com/codecov/codecov-action>
117127
with:
118-
file: summary.txt
128+
files: summary.txt
119129
fail_ci_if_error: false

.golangci.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ linters:
3838
threshold: 100
3939
goconst:
4040
min-len: 2
41-
min-occurrences: 3
41+
min-occurrences: 10
4242
gocyclo:
4343
min-complexity: 20
4444
godot:

go.work.sum

Lines changed: 135 additions & 0 deletions
Large diffs are not rendered by default.

handler/handler.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
stderr "errors"
66
"html/template"
7+
"io"
78
"net/http"
89
"sync"
910
"time"
@@ -140,6 +141,8 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
140141
status := http.StatusInternalServerError
141142
if _, ok := stderr.AsType[*http.MaxBytesError](err); ok {
142143
status = http.StatusRequestEntityTooLarge
144+
} else if stderr.Is(err, io.EOF) || stderr.Is(err, io.ErrUnexpectedEOF) {
145+
status = http.StatusBadRequest
143146
}
144147
http.Error(w, errors.E(op, err).Error(), status)
145148
h.log.Error(

handler/handler_test.go

Lines changed: 236 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,236 @@
1+
package handler
2+
3+
import (
4+
"context"
5+
"crypto/tls"
6+
"fmt"
7+
"net/http"
8+
"net/http/httptest"
9+
"os"
10+
"strings"
11+
"testing"
12+
13+
"github.com/roadrunner-server/errors"
14+
"github.com/roadrunner-server/http/v5/api"
15+
"github.com/roadrunner-server/http/v5/config"
16+
"github.com/roadrunner-server/pool/payload"
17+
"github.com/roadrunner-server/pool/pool"
18+
staticPool "github.com/roadrunner-server/pool/pool/static_pool"
19+
"github.com/roadrunner-server/pool/worker"
20+
"go.uber.org/zap"
21+
)
22+
23+
// mockPool satisfies api.Pool. Only Exec is exercised by the tests below.
24+
type mockPool struct{ execErr error }
25+
26+
func (m *mockPool) Workers() []*worker.Process { return nil }
27+
func (m *mockPool) RemoveWorker(_ context.Context) error { return nil }
28+
func (m *mockPool) AddWorker() error { return nil }
29+
func (m *mockPool) Exec(_ context.Context, _ *payload.Payload, _ chan struct{}) (chan *staticPool.PExec, error) {
30+
return nil, m.execErr
31+
}
32+
func (m *mockPool) Reset(_ context.Context) error { return nil }
33+
func (m *mockPool) Destroy(_ context.Context) {}
34+
35+
func newTestHandler(t *testing.T, cfg *config.Config, p api.Pool) *Handler {
36+
t.Helper()
37+
h, err := NewHandler(cfg, p, zap.NewNop())
38+
if err != nil {
39+
t.Fatal(err)
40+
}
41+
return h
42+
}
43+
44+
func defaultCfg() *config.Config {
45+
return &config.Config{
46+
MaxRequestSize: 1024,
47+
InternalErrorCode: 500,
48+
Uploads: &config.Uploads{
49+
Dir: os.TempDir(),
50+
Forbidden: map[string]struct{}{},
51+
Allowed: map[string]struct{}{},
52+
},
53+
}
54+
}
55+
56+
// ── Group A: ServeHTTP errors before pool.Exec (nil pool is safe) ────────────
57+
58+
func TestServeHTTP_InvalidMultipart_Returns400(t *testing.T) {
59+
h := newTestHandler(t, defaultCfg(), nil)
60+
61+
// Boundary declared in header but body has no valid multipart parts → EOF.
62+
req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(""))
63+
req.Header.Set("Content-Type", "multipart/form-data; boundary=1111")
64+
65+
rr := httptest.NewRecorder()
66+
h.ServeHTTP(rr, req)
67+
68+
if rr.Code != http.StatusBadRequest {
69+
t.Errorf("expected 400, got %d", rr.Code)
70+
}
71+
}
72+
73+
func TestServeHTTP_StreamBody_MaxBytesExceeded_Returns413(t *testing.T) {
74+
h := newTestHandler(t, defaultCfg(), nil)
75+
76+
req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader("this body is too long"))
77+
req.Header.Set("Content-Type", "application/json")
78+
79+
rr := httptest.NewRecorder()
80+
// Wrap body so that reading more than 5 bytes returns *http.MaxBytesError.
81+
req.Body = http.MaxBytesReader(rr, req.Body, 5)
82+
83+
h.ServeHTTP(rr, req)
84+
85+
if rr.Code != http.StatusRequestEntityTooLarge {
86+
t.Errorf("expected 413, got %d", rr.Code)
87+
}
88+
}
89+
90+
func TestServeHTTP_TruncatedMultipart_Returns400(t *testing.T) {
91+
h := newTestHandler(t, defaultCfg(), nil)
92+
93+
// Multipart body with an open part but no closing boundary → ErrUnexpectedEOF.
94+
body := "--1111\r\nContent-Disposition: form-data; name=\"f\"\r\n\r\nval"
95+
req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(body))
96+
req.Header.Set("Content-Type", "multipart/form-data; boundary=1111")
97+
98+
rr := httptest.NewRecorder()
99+
h.ServeHTTP(rr, req)
100+
101+
if rr.Code != http.StatusBadRequest {
102+
t.Errorf("expected 400, got %d", rr.Code)
103+
}
104+
}
105+
106+
// ── Group B: Direct calls to handleError, URI, FetchIP ───────────────────────
107+
108+
func TestHandleError_NoFreeWorkers_SetsNoWorkersHeader(t *testing.T) {
109+
h := newTestHandler(t, defaultCfg(), nil)
110+
111+
rr := httptest.NewRecorder()
112+
h.handleError(rr, errors.E(errors.NoFreeWorkers))
113+
114+
if got := rr.Header().Get(noWorkers); got != trueStr {
115+
t.Errorf("expected No-Workers: true, got %q", got)
116+
}
117+
if rr.Code != 500 {
118+
t.Errorf("expected status 500, got %d", rr.Code)
119+
}
120+
}
121+
122+
func TestHandleError_CustomInternalCode(t *testing.T) {
123+
cfg := defaultCfg()
124+
cfg.InternalErrorCode = 503
125+
h := newTestHandler(t, cfg, nil)
126+
127+
rr := httptest.NewRecorder()
128+
h.handleError(rr, fmt.Errorf("boom"))
129+
130+
if rr.Code != 503 {
131+
t.Errorf("expected 503, got %d", rr.Code)
132+
}
133+
}
134+
135+
func TestHandleError_DebugMode_WritesEscapedError(t *testing.T) {
136+
cfg := defaultCfg()
137+
cfg.Pool = &pool.Config{Debug: true}
138+
h := newTestHandler(t, cfg, nil)
139+
140+
rr := httptest.NewRecorder()
141+
h.handleError(rr, fmt.Errorf("boom<script>"))
142+
143+
body := rr.Body.String()
144+
if strings.Contains(body, "<script>") {
145+
t.Error("response body contains unescaped <script> tag (XSS risk)")
146+
}
147+
if !strings.Contains(body, "&lt;script&gt;") {
148+
t.Errorf("expected HTML-escaped error in body, got: %q", body)
149+
}
150+
}
151+
152+
func TestURI_PlainHTTP(t *testing.T) {
153+
r := httptest.NewRequest(http.MethodGet, "/path?q=1", nil)
154+
r.Host = "example.com"
155+
156+
got := URI(r)
157+
want := "http://example.com/path?q=1"
158+
if got != want {
159+
t.Errorf("URI() = %q, want %q", got, want)
160+
}
161+
}
162+
163+
func TestURI_TLSRequest_HTTPSScheme(t *testing.T) {
164+
r := httptest.NewRequest(http.MethodGet, "/path?q=1", nil)
165+
r.Host = "example.com"
166+
r.TLS = &tls.ConnectionState{}
167+
168+
got := URI(r)
169+
want := "https://example.com/path?q=1"
170+
if got != want {
171+
t.Errorf("URI() = %q, want %q", got, want)
172+
}
173+
}
174+
175+
func TestURI_StripsCRLFInjection(t *testing.T) {
176+
r := httptest.NewRequest(http.MethodGet, "/path", nil)
177+
r.Host = "example.com"
178+
// Inject CRLF into the raw query — a classic HTTP response-splitting vector.
179+
r.URL.RawQuery = "param=value\r\nX-Injected: true"
180+
181+
got := URI(r)
182+
if strings.ContainsAny(got, "\r\n") {
183+
t.Errorf("URI() result contains CRLF characters: %q", got)
184+
}
185+
}
186+
187+
func TestFetchIP_StripPortFromIPv4(t *testing.T) {
188+
got := FetchIP("127.0.0.1:8080", zap.NewNop())
189+
if got != "127.0.0.1" {
190+
t.Errorf("FetchIP() = %q, want %q", got, "127.0.0.1")
191+
}
192+
}
193+
194+
func TestFetchIP_BareIPv6_NoPort(t *testing.T) {
195+
// "::1" contains colons but is not host:port — SplitHostPort fails,
196+
// ParseIP succeeds.
197+
got := FetchIP("::1", zap.NewNop())
198+
if got != "::1" {
199+
t.Errorf("FetchIP() = %q, want %q", got, "::1")
200+
}
201+
}
202+
203+
// ── Group C: mockPool tests ───────────────────────────────────────────────────
204+
205+
func TestServeHTTP_PoolExecError_Returns500(t *testing.T) {
206+
mp := &mockPool{execErr: fmt.Errorf("worker died")}
207+
h := newTestHandler(t, defaultCfg(), mp)
208+
209+
req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(`{"key":"val"}`))
210+
req.Header.Set("Content-Type", "application/json")
211+
212+
rr := httptest.NewRecorder()
213+
h.ServeHTTP(rr, req)
214+
215+
if rr.Code != 500 {
216+
t.Errorf("expected 500, got %d", rr.Code)
217+
}
218+
}
219+
220+
func TestServeHTTP_NoFreeWorkers_SetsHeader(t *testing.T) {
221+
mp := &mockPool{execErr: errors.E(errors.NoFreeWorkers)}
222+
h := newTestHandler(t, defaultCfg(), mp)
223+
224+
req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(`{"key":"val"}`))
225+
req.Header.Set("Content-Type", "application/json")
226+
227+
rr := httptest.NewRecorder()
228+
h.ServeHTTP(rr, req)
229+
230+
if got := rr.Header().Get(noWorkers); got != trueStr {
231+
t.Errorf("expected No-Workers: true, got %q", got)
232+
}
233+
if rr.Code != 500 {
234+
t.Errorf("expected 500, got %d", rr.Code)
235+
}
236+
}

handler/uploads.go

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -44,15 +44,13 @@ func (u *Uploads) MarshalJSON() ([]byte, error) {
4444
// will be handled individually.
4545
func (u *Uploads) Open(log *zap.Logger, dir string, forbid, allow map[string]struct{}) {
4646
var wg sync.WaitGroup
47-
for i := range u.list {
48-
wg.Add(1)
49-
go func(f *FileUpload) {
50-
defer wg.Done()
47+
for _, f := range u.list {
48+
wg.Go(func() {
5149
err := f.Open(dir, forbid, allow)
5250
if err != nil && log != nil {
5351
log.Error("error opening the file", zap.Error(err))
5452
}
55-
}(u.list[i])
53+
})
5654
}
5755

5856
wg.Wait()

tests/configs/.rr-http-otel.yaml

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -11,23 +11,13 @@ server:
1111
http:
1212
address: 127.0.0.1:43239
1313
max_request_size: 1024
14-
middleware: [ gzip, otel ]
14+
middleware: [ gzip, tracetestOtel ]
1515
pool:
1616
num_workers: 2
1717
max_jobs: 0
1818
allocate_timeout: 60s
1919
destroy_timeout: 1s
2020

21-
otel:
22-
resource:
23-
service_name: "rr_test"
24-
service_version: "1.0.0"
25-
service_namespace: "RR-Shop"
26-
service_instance_id: "UUID"
27-
insecure: false
28-
compress: true
29-
exporter: stderr
30-
3121
logs:
3222
mode: development
3323
level: info

tests/configs/.rr-http-otel2.yaml

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,22 +14,12 @@ server:
1414
http:
1515
address: 127.0.0.1:43239
1616
max_request_size: 1024
17-
middleware: [ gzip, otel ]
17+
middleware: [ gzip, tracetestOtel ]
1818
pool:
1919
num_workers: 2
2020
allocate_timeout: 60s
2121
destroy_timeout: 1s
2222

23-
otel:
24-
resource:
25-
service_name: "rr_test"
26-
service_version: "1.0.0"
27-
service_namespace: "RR-Shop2"
28-
service_instance_id: "UUID2"
29-
insecure: false
30-
compress: true
31-
exporter: stderr
32-
3323
logs:
3424
mode: development
3525
level: debug

0 commit comments

Comments
 (0)