Skip to content

Commit 03690c4

Browse files
coder-backporter-app[bot]f0sselclaudeLisa-Fiander
authored
fix(coderd): tighten up database connection errors (#31850) (#31863)
Backport of #31850 Original PR: #31850 — fix(coderd): tighten up database connection errors Merge commit: b0fe0ba Requested by: @f0ssel --------- Co-authored-by: Garrett Delfosse <garrett@coder.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Garrett Delfosse <delfossegarrett@gmail.com> Co-authored-by: Lisa Fiander <lisa@coder.com>
1 parent d2c3ca1 commit 03690c4

5 files changed

Lines changed: 108 additions & 9 deletions

File tree

‎coderd/database/awsiamrds/awsiamrds.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"database/sql"
66
"database/sql/driver"
7+
"errors"
78
"fmt"
89
"net/url"
910

@@ -65,7 +66,7 @@ func (d *awsIamRdsDriver) Open(name string) (driver.Conn, error) {
6566
// make connection
6667
conn, err := d.parent.Open(nURL)
6768
if err != nil {
68-
return nil, xerrors.Errorf("opening connection with %s: %w", nURL, err)
69+
return nil, xerrors.Errorf("opening connection: %w", err)
6970
}
7071

7172
return conn, nil
@@ -84,6 +85,12 @@ func (d *awsIamRdsDriver) Connector(name string) (driver.Connector, error) {
8485
func getAuthenticatedURL(cfg aws.Config, dbURL string) (string, error) {
8586
nURL, err := url.Parse(dbURL)
8687
if err != nil {
88+
// url.Error includes the full input URL. Return only the
89+
// underlying cause so errors contain no connection details.
90+
var urlErr *url.Error
91+
if errors.As(err, &urlErr) {
92+
err = urlErr.Err
93+
}
8794
return "", xerrors.Errorf("parsing dbURL: %w", err)
8895
}
8996

‎coderd/database/awsiamrds/awsiamrds_internal_test.go‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
package awsiamrds
2+
3+
import (
4+
"context"
5+
"database/sql/driver"
6+
"testing"
7+
8+
"github.com/aws/aws-sdk-go-v2/aws"
9+
"github.com/aws/aws-sdk-go-v2/credentials"
10+
"github.com/stretchr/testify/require"
11+
"golang.org/x/xerrors"
12+
)
13+
14+
// failingDriver records the DSN it was asked to open and always fails.
15+
type failingDriver struct {
16+
gotName string
17+
err error
18+
}
19+
20+
func (d *failingDriver) Open(name string) (driver.Conn, error) {
21+
d.gotName = name
22+
return nil, d.err
23+
}
24+
25+
// TestOpenErrorOmitsConnectionDetails ensures connection errors do not
26+
// include the connection URL.
27+
func TestOpenErrorOmitsConnectionDetails(t *testing.T) {
28+
t.Parallel()
29+
30+
const (
31+
accessKeyID = "AKIAEXAMPLEACCESSKEY"
32+
secretAccessKey = "exampleSecretAccessKey"
33+
)
34+
cfg := aws.Config{
35+
Region: "us-east-2",
36+
Credentials: credentials.NewStaticCredentialsProvider(accessKeyID, secretAccessKey, ""),
37+
}
38+
parentErr := xerrors.New("connection refused")
39+
parent := &failingDriver{err: parentErr}
40+
d := newDriver(parent, cfg)
41+
42+
_, err := d.Open("postgres://coder@db.example.com:5432/coder")
43+
require.Error(t, err)
44+
require.ErrorIs(t, err, parentErr)
45+
46+
// Sanity check that the parent received a signed URL, so the assertions
47+
// below are meaningful.
48+
require.Contains(t, parent.gotName, accessKeyID)
49+
require.Contains(t, parent.gotName, "X-Amz-Signature")
50+
51+
msg := err.Error()
52+
require.NotContains(t, msg, parent.gotName)
53+
require.NotContains(t, msg, accessKeyID)
54+
require.NotContains(t, msg, secretAccessKey)
55+
require.NotContains(t, msg, "X-Amz-")
56+
}
57+
58+
// TestParseErrorOmitsConnectionDetails ensures URL parse errors do not
59+
// include the connection URL.
60+
func TestParseErrorOmitsConnectionDetails(t *testing.T) {
61+
t.Parallel()
62+
63+
const password = "examplePassword"
64+
cfg := aws.Config{
65+
Region: "us-east-2",
66+
Credentials: credentials.NewStaticCredentialsProvider("AKIAEXAMPLEACCESSKEY", "exampleSecretAccessKey", ""),
67+
}
68+
// The non-numeric port makes url.Parse fail.
69+
dbURL := "postgres://coder:" + password + "@db.example.com:badport/coder"
70+
71+
d := newDriver(&failingDriver{}, cfg)
72+
_, err := d.Open(dbURL)
73+
require.Error(t, err)
74+
require.NotContains(t, err.Error(), password)
75+
76+
c, err := d.Connector(dbURL)
77+
require.NoError(t, err)
78+
_, err = c.Connect(context.Background())
79+
require.Error(t, err)
80+
require.NotContains(t, err.Error(), password)
81+
}

‎coderd/httpmw/apikey.go‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ func PrecheckAPIKey(cfg ValidateAPIKeyConfig) func(http.Handler) http.Handler {
248248
//
249249
// Returns (result, nil) on success or (nil, error) on failure.
250250
func ValidateAPIKey(ctx context.Context, cfg ValidateAPIKeyConfig, r *http.Request) (*ValidateAPIKeyResult, *ValidateAPIKeyError) {
251-
key, valErr := apiKeyFromRequestValidate(ctx, cfg.DB, cfg.SessionTokenFunc, r)
251+
key, valErr := apiKeyFromRequestValidate(ctx, cfg.DB, cfg.Logger, cfg.SessionTokenFunc, r)
252252
if valErr != nil {
253253
return nil, valErr
254254
}
@@ -492,16 +492,16 @@ func ValidateAPIKey(ctx context.Context, cfg ValidateAPIKeyConfig, r *http.Reque
492492
}, nil
493493
}
494494

495-
func APIKeyFromRequest(ctx context.Context, db database.Store, sessionTokenFunc func(r *http.Request) string, r *http.Request) (*database.APIKey, codersdk.Response, bool) {
496-
key, valErr := apiKeyFromRequestValidate(ctx, db, sessionTokenFunc, r)
495+
func APIKeyFromRequest(ctx context.Context, db database.Store, logger slog.Logger, sessionTokenFunc func(r *http.Request) string, r *http.Request) (*database.APIKey, codersdk.Response, bool) {
496+
key, valErr := apiKeyFromRequestValidate(ctx, db, logger, sessionTokenFunc, r)
497497
if valErr != nil {
498498
return nil, valErr.Response, false
499499
}
500500

501501
return key, codersdk.Response{}, true
502502
}
503503

504-
func apiKeyFromRequestValidate(ctx context.Context, db database.Store, sessionTokenFunc func(r *http.Request) string, r *http.Request) (*database.APIKey, *ValidateAPIKeyError) {
504+
func apiKeyFromRequestValidate(ctx context.Context, db database.Store, logger slog.Logger, sessionTokenFunc func(r *http.Request) string, r *http.Request) (*database.APIKey, *ValidateAPIKeyError) {
505505
tokenFunc := APITokenFromRequest
506506
if sessionTokenFunc != nil {
507507
tokenFunc = sessionTokenFunc
@@ -542,11 +542,12 @@ func apiKeyFromRequestValidate(ctx context.Context, db database.Store, sessionTo
542542
}
543543
}
544544

545+
logger.Error(ctx, "fetch api key by id", slog.Error(err))
545546
return nil, &ValidateAPIKeyError{
546547
Code: http.StatusInternalServerError,
547548
Response: codersdk.Response{
548549
Message: internalErrorMessage,
549-
Detail: fmt.Sprintf("Internal error fetching API key by id. %s", err.Error()),
550+
Detail: "Internal error fetching API key by id.",
550551
},
551552
Hard: true,
552553
}

‎coderd/httpmw/apikey_test.go‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"golang.org/x/xerrors"
2323

2424
"cdr.dev/slog/v3"
25+
"cdr.dev/slog/v3/sloggers/slogtest"
2526
"github.com/coder/coder/v2/coderd/apikey"
2627
"github.com/coder/coder/v2/coderd/database"
2728
"github.com/coder/coder/v2/coderd/database/dbauthz"
@@ -203,18 +204,27 @@ func TestAPIKey(t *testing.T) {
203204
rw := httptest.NewRecorder()
204205
r.Header.Set(codersdk.SessionTokenHeader, fmt.Sprintf("%s-%s", id, secret))
205206

206-
db.EXPECT().GetAPIKeyByID(gomock.Any(), id).Return(database.APIKey{}, xerrors.New("db unavailable"))
207+
// Database error details are logged, not returned to the client.
208+
const connURL = "postgres://coder:password@db.internal:5432/coder"
209+
db.EXPECT().GetAPIKeyByID(gomock.Any(), id).Return(database.APIKey{}, xerrors.Errorf("dial %s: connection refused", connURL))
207210

208211
httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{
209212
DB: db,
213+
Logger: slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}),
210214
RedirectToLogin: false,
211215
})(successHandler).ServeHTTP(rw, r)
212216
res := rw.Result()
213217
defer res.Body.Close()
214218
require.Equal(t, http.StatusInternalServerError, res.StatusCode)
215219

220+
body, err := io.ReadAll(res.Body)
221+
require.NoError(t, err)
222+
require.NotContains(t, string(body), connURL)
223+
require.NotContains(t, string(body), "db.internal")
224+
require.NotContains(t, string(body), "connection refused")
225+
216226
var resp codersdk.Response
217-
require.NoError(t, json.NewDecoder(res.Body).Decode(&resp))
227+
require.NoError(t, json.Unmarshal(body, &resp))
218228
require.NotEqual(t, httpmw.SignedOutErrorMessage, resp.Message)
219229
require.Contains(t, resp.Detail, "Internal error fetching API key by id")
220230
})

‎coderd/userauth.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2050,7 +2050,7 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
20502050
}
20512051

20522052
var key database.APIKey
2053-
oldKey, _, ok := httpmw.APIKeyFromRequest(ctx, api.Database, nil, r)
2053+
oldKey, _, ok := httpmw.APIKeyFromRequest(ctx, api.Database, api.Logger, nil, r)
20542054
if ok && oldKey != nil && isConvertLoginType {
20552055
// If this is a convert login type, and it succeeds, then delete the old
20562056
// session. Force the user to log back in.

0 commit comments

Comments
 (0)