Skip to content

Commit f8d3a0d

Browse files
committed
fix(api/v2): don't leak internal error detail in 5xx responses
Huma's handler-error path wraps raw errors as NewErrorWithContext(ctx, 500, "unexpected error occurred", err), and since the humaecho5 adapter writes Huma's response directly it bypasses Vikunja's CreateHTTPErrorHandler — which returns a generic 500 with no detail for non-domain errors. The huma.NewError override then copied err.Error() (raw DB/driver messages, SQL, table/column names) into the problem+json errors[], a regression vs v1. Override huma.NewErrorWithContext to drop errs for status >= 500, log the real cause server-side, and return a generic body. 4xx detail (validation errors, domain messages) is unaffected.
1 parent 36dae51 commit f8d3a0d

2 files changed

Lines changed: 80 additions & 0 deletions

File tree

pkg/routes/api/v2/errors.go

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,4 +103,23 @@ func init() {
103103
Errors: details,
104104
}}
105105
}
106+
107+
// Strip internal detail from server errors. Huma's handler-error path
108+
// wraps a raw error as NewErrorWithContext(ctx, 500, "unexpected error
109+
// occurred", err) and — because the humaecho5 adapter writes the
110+
// response itself — bypasses Vikunja's CreateHTTPErrorHandler, which for
111+
// v1 returns a generic 500 with no detail. Without this override a raw
112+
// DB/driver error (SQL, table, column names) would leak into the
113+
// problem+json `errors[]`. Log the real cause, return a generic body.
114+
huma.NewErrorWithContext = func(_ huma.Context, status int, msg string, errs ...error) huma.StatusError {
115+
if status >= 500 {
116+
for _, e := range errs {
117+
if e != nil {
118+
log.Errorf("v2: internal server error: %s", e)
119+
}
120+
}
121+
errs = nil
122+
}
123+
return huma.NewError(status, msg, errs...)
124+
}
106125
}

pkg/routes/api/v2/errors_test.go

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
// Vikunja is a to-do list application to facilitate your life.
2+
// Copyright 2018-present Vikunja and contributors. All rights reserved.
3+
//
4+
// This program is free software: you can redistribute it and/or modify
5+
// it under the terms of the GNU Affero General Public License as published by
6+
// the Free Software Foundation, either version 3 of the License, or
7+
// (at your option) any later version.
8+
//
9+
// This program is distributed in the hope that it will be useful,
10+
// but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
12+
// GNU Affero General Public License for more details.
13+
//
14+
// You should have received a copy of the GNU Affero General Public License
15+
// along with this program. If not, see <https://www.gnu.org/licenses/>.
16+
17+
package apiv2
18+
19+
import (
20+
"errors"
21+
"os"
22+
"testing"
23+
24+
"code.vikunja.io/api/pkg/log"
25+
26+
"github.com/danielgtaylor/huma/v2"
27+
"github.com/stretchr/testify/assert"
28+
"github.com/stretchr/testify/require"
29+
)
30+
31+
func TestMain(m *testing.M) {
32+
// The NewErrorWithContext override logs server errors; initialise a
33+
// logger so that path doesn't nil-panic in the bare test binary.
34+
log.InitLogger()
35+
os.Exit(m.Run())
36+
}
37+
38+
// TestNewErrorWithContext_StripsServerErrorDetail guards against leaking
39+
// internal error detail (raw DB/driver messages, etc.) in v2 5xx responses.
40+
// Huma's handler-error path funnels raw errors through NewErrorWithContext
41+
// at status 500; the override must drop the detail there while keeping it
42+
// for client (4xx) errors. Mirrors v1's generic-500 behaviour.
43+
func TestNewErrorWithContext_StripsServerErrorDetail(t *testing.T) {
44+
secret := errors.New(`pq: relation "labels" does not exist`)
45+
46+
t.Run("500 drops the wrapped detail", func(t *testing.T) {
47+
se := huma.NewErrorWithContext(nil, 500, "unexpected error occurred", secret)
48+
vm, ok := se.(*vikunjaErrorModel)
49+
require.True(t, ok)
50+
assert.Empty(t, vm.Errors, "server errors must not expose internal detail")
51+
assert.Equal(t, "unexpected error occurred", vm.Detail)
52+
})
53+
54+
t.Run("4xx keeps the detail", func(t *testing.T) {
55+
se := huma.NewErrorWithContext(nil, 422, "validation failed", secret)
56+
vm, ok := se.(*vikunjaErrorModel)
57+
require.True(t, ok)
58+
require.Len(t, vm.Errors, 1, "client errors keep their detail")
59+
assert.Equal(t, secret.Error(), vm.Errors[0].Message)
60+
})
61+
}

0 commit comments

Comments
 (0)