diff --git a/apps/datatugapp/commands/cmd_query_run_saved.go b/apps/datatugapp/commands/cmd_query_run_saved.go index a3a22ec..5121c2c 100644 --- a/apps/datatugapp/commands/cmd_query_run_saved.go +++ b/apps/datatugapp/commands/cmd_query_run_saved.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "os" - "regexp" "strings" "github.com/dal-go/dalgo/dal" @@ -230,18 +229,20 @@ func querySourceURLFromCatalog(catalog datatug.DbCatalog, projectDir string) (st } // runSQLSavedQuery resolves the SQL query's environment+database, binds its -// named "@param" placeholders (see bindSQLNamedParams) and runs it through -// Executor.RunNativeSQL. +// declared parameters (see sqlQueryArgs) as real dal.QueryArg values, and +// runs it through Executor.RunNativeSQL, which passes them straight to the +// database/sql driver — an "@ParamName" placeholder in sqlText binds by +// name (dal-go/dalgo2sql v0.11.7+, see RunNativeSQL's doc comment). func runSQLSavedQuery(ctx context.Context, executor *secureread.Executor, projStore datatug.ProjectStore, projectDir, envFlag string, queryDef *datatug.QueryDef, variables map[string]any) (secureread.Result, error) { sourceURL, err := resolveSQLOrDTQLSourceURL(ctx, projStore, projectDir, envFlag, queryDef) if err != nil { return secureread.Result{}, err } - text, err := bindSQLNamedParams(queryDef.Text, variables) + args, err := sqlQueryArgs(queryDef, variables) if err != nil { return secureread.Result{}, fmt.Errorf("query %q: %w", queryDef.ID, err) } - return executor.RunNativeSQL(ctx, sourceURL, text) + return executor.RunNativeSQL(ctx, sourceURL, queryDef.Text, args...) } // runDTQLSavedQuery resolves the DTQL query's environment+database and runs @@ -296,55 +297,30 @@ func runHTTPSavedQuery(ctx context.Context, executor *secureread.Executor, proje return executor.RunStructured(ctx, sourceURL, builder.SelectColumns(), nil) } -// sqlNamedParamPattern matches a "@ParamName" placeholder in saved SQL query -// text, e.g. demo-project-1's queries/invoices/invoice-lines.query.sql: -// "WHERE il.InvoiceId = @InvoiceId". -var sqlNamedParamPattern = regexp.MustCompile(`@([A-Za-z_][A-Za-z0-9_]*)`) - -// bindSQLNamedParams substitutes every "@ParamName" placeholder in sqlText -// with variables[ParamName], SQL-quoted as a literal. -// -// secureread.Executor.RunNativeSQL takes no variables/args parameter - -// dal.NewTextQuery does accept them (dal.QueryArg{Name, Value}), but -// dal-go/dalgo2sql's reader_base.go passes each raw QueryArg struct straight -// through to database/sql as a driver arg (`a[i] = arg`, not `arg.Value` or -// sql.Named(arg.Name, arg.Value)) - database/sql's default parameter -// converter rejects any type it doesn't recognize (struct included), so -// that path errors before a query ever runs. That is a dal-go/dalgo2sql bug, -// several repos upstream of this one; flagged in the PR body, not fixed -// here. Substituting into the literal SQL text ourselves, entirely within -// this file, sidesteps it without depending on unverified third-party -// binding behavior. sqlText only ever comes from a saved query file this -// CLI's own user already controls (git-tracked project content), and each -// substituted value is quoted as a SQL literal (strings single-quoted with -// ” escaping; everything else via fmt.Sprint) rather than concatenated -// unescaped. -func bindSQLNamedParams(sqlText string, variables map[string]any) (string, error) { +// sqlQueryArgs builds one dal.QueryArg{Name: p.ID, Value: variables[p.ID]} +// per declared parameter, in declaration order, for a real driver-level bind +// (see RunNativeSQL's doc comment) - the QueryDef's own declared parameters +// are the single source of truth for which "@name" placeholders sqlText is +// allowed to reference, exactly as before this switched from string +// substitution to real binding. A required parameter with no matching --var +// fails the same way bindSQLNamedParams's predecessor did. +func sqlQueryArgs(queryDef *datatug.QueryDef, variables map[string]any) ([]dal.QueryArg, error) { var missing []string - bound := sqlNamedParamPattern.ReplaceAllStringFunc(sqlText, func(match string) string { - name := match[1:] - value, ok := variables[name] + args := make([]dal.QueryArg, 0, len(queryDef.Parameters)) + for _, p := range queryDef.Parameters { + value, ok := variables[p.ID] if !ok { - missing = append(missing, name) - return match + if p.IsRequired { + missing = append(missing, p.ID) + } + continue } - return sqlLiteral(value) - }) - if len(missing) > 0 { - return "", fmt.Errorf("missing --var for SQL parameter(s): %s", strings.Join(missing, ", ")) + args = append(args, dal.QueryArg{Name: p.ID, Value: value}) } - return bound, nil -} - -func sqlLiteral(value any) string { - switch v := value.(type) { - case nil: - return "NULL" - case string: - return "'" + strings.ReplaceAll(v, "'", "''") + "'" - default: - return fmt.Sprint(v) + if len(missing) > 0 { + return nil, fmt.Errorf("missing --var for SQL parameter(s): %s", strings.Join(missing, ", ")) } + return args, nil } // secureRowsToQueryRows adapts secureread.Result's rows to query_output.go's diff --git a/go.mod b/go.mod index 738d8be..51eeb16 100644 --- a/go.mod +++ b/go.mod @@ -22,7 +22,7 @@ require ( github.com/atotto/clipboard v0.1.4 github.com/dal-go/dalgo v0.79.5 github.com/dal-go/dalgo2http v0.1.0 - github.com/dal-go/dalgo2sql v0.11.4 + github.com/dal-go/dalgo2sql v0.11.7 github.com/dal-go/dalgo2sqlite v0.1.8 github.com/dal-go/record v0.1.3 github.com/datatug/cliformat v0.0.3 @@ -57,7 +57,7 @@ require ( golang.org/x/oauth2 v0.36.0 google.golang.org/api v0.296.0 gopkg.in/yaml.v3 v3.0.1 - modernc.org/sqlite v1.57.0 + modernc.org/sqlite v1.58.0 ) require ( @@ -180,7 +180,7 @@ require ( google.golang.org/grpc v1.83.2 // indirect google.golang.org/protobuf v1.36.12 // indirect gopkg.in/warnings.v0 v0.1.2 // indirect - modernc.org/libc v1.74.4 // indirect + modernc.org/libc v1.75.6 // indirect modernc.org/mathutil v1.7.1 // indirect - modernc.org/memory v1.11.0 // indirect + modernc.org/memory v1.12.1 // indirect ) diff --git a/go.sum b/go.sum index 0667fb1..5373602 100644 --- a/go.sum +++ b/go.sum @@ -97,8 +97,8 @@ github.com/dal-go/dalgo v0.79.5 h1:mvWbKgtj3HyILrQgK8qwDvuHNPPDKQFulKU5IMNpHkM= github.com/dal-go/dalgo v0.79.5/go.mod h1:u7sD8jlXshi2qHrlPUm9GzR2uyPRaBI4Ro9ef5+WFzY= github.com/dal-go/dalgo2http v0.1.0 h1:RQKdWpWGW7KVLxdS7DAyPWXDa2lCR4ZQSNj6D5g6imU= github.com/dal-go/dalgo2http v0.1.0/go.mod h1:9D70F2oimIPSz9r5iKki5xuepReCAMSSla9TmgmuVcs= -github.com/dal-go/dalgo2sql v0.11.4 h1:7UrhL8Blr5qRJ4Hcdzyf0VxI3uEhQAywdADJrZrStX0= -github.com/dal-go/dalgo2sql v0.11.4/go.mod h1:OZKb7blQM1OXpEsshU5XWN/bD2jSmwVyvGEiCSJBulU= +github.com/dal-go/dalgo2sql v0.11.7 h1:nDPOlNPX+KgEUiB5iArw/wh9jLchUgNlcTn+5X72TFo= +github.com/dal-go/dalgo2sql v0.11.7/go.mod h1:eY0fdc5c9ZsaD9j2X2ZkYljn9XCooqR+1AYZYJIaFVo= github.com/dal-go/dalgo2sqlite v0.1.8 h1:i7lb9MEQ5/ZVbF7nGUPQUp6C4m58HxGLVlGi6MGamjE= github.com/dal-go/dalgo2sqlite v0.1.8/go.mod h1:+n9hXVRd5GEaaqiYpmeTrKlRuzut2gdjpjgD435F5xI= github.com/dal-go/record v0.1.3 h1:K85k/kSX08lTefMiMmPpC12nmCY2TVJk7+ZyC5PD9XU= @@ -478,30 +478,30 @@ gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= -modernc.org/cc/v4 v4.29.1 h1:MKgdCV3WykTSPqpVrnxdEDS0HEd2FHpKZDzxzU5LyeI= -modernc.org/cc/v4 v4.29.1/go.mod h1:OnovgIhbbMXMu1aISnJ0wvVD1KnW+cAUJkIrAWh+kVI= -modernc.org/ccgo/v4 v4.34.6 h1:sBgfIwyN0TQ9C5hwIeuqyeAKyMWnbvj2fvpF4L11uzU= -modernc.org/ccgo/v4 v4.34.6/go.mod h1:SZ8YcN9NG7XVsQYdm6jYBvi8PQP1qi+kqB6OhjqI3Fk= +modernc.org/cc/v4 v4.29.2 h1:h6+9ciCnPKutf4I03CvheAvDLX7+IHlqR6Iy6J+cgd8= +modernc.org/cc/v4 v4.29.2/go.mod h1:OnovgIhbbMXMu1aISnJ0wvVD1KnW+cAUJkIrAWh+kVI= +modernc.org/ccgo/v4 v4.35.0 h1:F+TUsmw09QxLzmi3aeYYGxjAXarmZaKgj3mKQHNaA8w= +modernc.org/ccgo/v4 v4.35.0/go.mod h1:qrVGs9S3Sr2Ztcg9ve+kTAYMp5a3YvWjo+SoN06kJ5I= modernc.org/fileutil v1.4.0 h1:j6ZzNTftVS054gi281TyLjHPp6CPHr2KCxEXjEbD6SM= modernc.org/fileutil v1.4.0/go.mod h1:EqdKFDxiByqxLk8ozOxObDSfcVOv/54xDs/DUHdvCUU= modernc.org/gc/v2 v2.6.5 h1:nyqdV8q46KvTpZlsw66kWqwXRHdjIlJOhG6kxiV/9xI= modernc.org/gc/v2 v2.6.5/go.mod h1:YgIahr1ypgfe7chRuJi2gD7DBQiKSLMPgBQe9oIiito= -modernc.org/gc/v3 v3.1.4 h1:2g65LGVSmFQrXeITAw97x7hCRvZFcyE1uDP+7Vng7JI= -modernc.org/gc/v3 v3.1.4/go.mod h1:HFK/6AGESC7Ex+EZJhJ2Gni6cTaYpSMmU/cT9RmlfYY= +modernc.org/gc/v3 v3.1.5 h1:21ldfPfRYE31Tb7B3mwAK8gy1AxP4+dKjrOQPfqakoc= +modernc.org/gc/v3 v3.1.5/go.mod h1:HFK/6AGESC7Ex+EZJhJ2Gni6cTaYpSMmU/cT9RmlfYY= modernc.org/goabi0 v0.2.0 h1:HvEowk7LxcPd0eq6mVOAEMai46V+i7Jrj13t4AzuNks= modernc.org/goabi0 v0.2.0/go.mod h1:CEFRnnJhKvWT1c1JTI3Avm+tgOWbkOu5oPA8eH8LnMI= -modernc.org/libc v1.74.4 h1:fX1Omw4o2/1C2iRkkIsrQTasJQldLhRmuPreXLoWs9k= -modernc.org/libc v1.74.4/go.mod h1:eeQAS9W3sZeKYMFubydxJpII9ybHWshk+7or7bLG9co= +modernc.org/libc v1.75.6 h1:yKk8qo+Di4gkmvRboK8ocCqH22FiUCR6jRy2OwtCRus= +modernc.org/libc v1.75.6/go.mod h1:bO5o2ztHxBb2rjz0PgdHN0sSMw57CgxGFLZ3Qd/QpVQ= modernc.org/mathutil v1.7.1 h1:GCZVGXdaN8gTqB1Mf/usp1Y/hSqgI2vAGGP4jZMCxOU= modernc.org/mathutil v1.7.1/go.mod h1:4p5IwJITfppl0G4sUEDtCr4DthTaT47/N3aT6MhfgJg= -modernc.org/memory v1.11.0 h1:o4QC8aMQzmcwCK3t3Ux/ZHmwFPzE6hf2Y5LbkRs+hbI= -modernc.org/memory v1.11.0/go.mod h1:/JP4VbVC+K5sU2wZi9bHoq2MAkCnrt2r98UGeSK7Mjw= +modernc.org/memory v1.12.1 h1:nFMiWrpStgZczNl6XI9GnIk/rWhYIyHGUaR04pGbp9g= +modernc.org/memory v1.12.1/go.mod h1:/JP4VbVC+K5sU2wZi9bHoq2MAkCnrt2r98UGeSK7Mjw= modernc.org/opt v0.2.0 h1:tGyef5ApycA7FSEOMraay9SaTk5zmbx7Tu+cJs4QKZg= modernc.org/opt v0.2.0/go.mod h1:03fq9lsNfvkYSfxrfUhZCWPk1lm4cq4N+Bh//bEtgns= modernc.org/sortutil v1.2.1 h1:+xyoGf15mM3NMlPDnFqrteY07klSFxLElE2PVuWIJ7w= modernc.org/sortutil v1.2.1/go.mod h1:7ZI3a3REbai7gzCLcotuw9AC4VZVpYMjDzETGsSMqJE= -modernc.org/sqlite v1.57.0 h1:qNQP6xnx5M0ISNtlnxoOX0+cD5bJ0/gr9aMmndFczzg= -modernc.org/sqlite v1.57.0/go.mod h1:yCJ2cmAaIkHQ25oXWrF8H4O1lIfPYPR26yCEDj2P3pQ= +modernc.org/sqlite v1.58.0 h1:38u40/bwkfM7f0Myhosl+SEMltSDxnGdQf8o6Kjmys0= +modernc.org/sqlite v1.58.0/go.mod h1:rsD2CckafgObKC4DhBlGBf+RiHxkc3hINGt1Xw32tVY= modernc.org/strutil v1.2.1 h1:UneZBkQA+DX2Rp35KcM69cSsNES9ly8mQWD71HKlOA0= modernc.org/strutil v1.2.1/go.mod h1:EHkiggD70koQxjVdSBM3JKM7k6L0FbGE5eymy9i3B9A= modernc.org/token v1.1.0 h1:Xl7Ap9dKaEs5kLoOQeQmPWevfnk/DM5qcLcYlA8ys6Y= diff --git a/pkg/secureread/executor_native_sql_test.go b/pkg/secureread/executor_native_sql_test.go index d7b6f35..6747a3c 100644 --- a/pkg/secureread/executor_native_sql_test.go +++ b/pkg/secureread/executor_native_sql_test.go @@ -5,6 +5,8 @@ import ( "errors" "strings" "testing" + + "github.com/dal-go/dalgo/dal" ) // TestRunNativeSQL_Labelled covers AC native-sql-labelled: a SQL-text query @@ -89,6 +91,32 @@ func TestRunNativeSQL_UnsupportedScheme_TypedError(t *testing.T) { } } +// TestRunNativeSQL_NamedArgBindsThroughDriver proves a named +// dal.QueryArg{Name, Value} reaches the SQLite driver as a real bind value +// (dal-go/dalgo2sql v0.11.7+, sql.Named) rather than failing or being +// substituted into the SQL text by hand: an "@name" placeholder in sqlText +// binds to exactly the row that value names, no more, no less. +func TestRunNativeSQL_NamedArgBindsThroughDriver(t *testing.T) { + sourceURL := newSQLiteFixture(t) + session := aliceSession(t, opaqueSQLAllowedPolicy) + executor := NewExecutor(session) + result, err := executor.RunNativeSQL(context.Background(), sourceURL, + "SELECT name, price FROM products WHERE name = @name", + dal.QueryArg{Name: "name", Value: "book"}) + if err != nil { + t.Fatalf("RunNativeSQL: %v", err) + } + if len(result.Rows) != 1 { + t.Fatalf("rows = %d, want 1: %+v", len(result.Rows), result.Rows) + } + if got := result.Rows[0].Data["name"]; got != "book" { + t.Errorf("name = %v, want book", got) + } + if got := result.Rows[0].Data["price"]; got != float64(10) { + t.Errorf("price = %v (%T), want 10", got, got) + } +} + // TestRunNativeSQL_Unrestricted_NoLabelNeeded covers the Unrestricted // bypass for native SQL too: nothing is enforced, so the "policies not // applied" note (which exists to warn about *skipped* policy) is pointless diff --git a/pkg/secureread/native_sql.go b/pkg/secureread/native_sql.go index dad01f8..c868557 100644 --- a/pkg/secureread/native_sql.go +++ b/pkg/secureread/native_sql.go @@ -32,6 +32,15 @@ var nativeSQLLimitation = Limitation{ // condition or field allow-list is applied to the returned rows regardless // of what accesspolicies.Explain reports for the collection-scoped rules. // +// args are the query's own bind values (dal.QueryArg{Name, Value}) — a named +// arg (Name != "") binds an "@name"/":name"/"$name" placeholder in sqlText; +// a positional arg (Name == "") binds an ordinary "?" placeholder, in order. +// They reach the database/sql driver exactly as given: dal-go/dalgo2sql +// v0.11.7+ converts a named dal.QueryArg to sql.Named(Name, Value) and a +// positional one to its bare Value (see dal-go/dalgo2sql#177 — a real bind, +// not string substitution into sqlText, so a value can never be mistaken +// for SQL syntax). +// // The SQLite session is pinned read-only at the engine level with // PRAGMA query_only on a dedicated, single-connection database handle, so // even a multi-statement injection inside sqlText cannot write — this is @@ -45,7 +54,7 @@ var nativeSQLLimitation = Limitation{ // A future SQL-capable adapter for another scheme should add a read-only // dal.DB.RunReadonlyTransaction branch alongside this PRAGMA one, per the // brief's "PRAGMA query_only for SQLite; read-only tx elsewhere" design. -func (e *Executor) RunNativeSQL(ctx context.Context, sourceURL, sqlText string) (Result, error) { +func (e *Executor) RunNativeSQL(ctx context.Context, sourceURL, sqlText string, args ...dal.QueryArg) (Result, error) { ref, err := dbcopy.Parse(sourceURL) if err != nil { return Result{}, err @@ -59,7 +68,7 @@ func (e *Executor) RunNativeSQL(ctx context.Context, sourceURL, sqlText string) } defer closeDB() - query := dal.NewTextQuery(sqlText, nil) + query := dal.NewTextQuery(sqlText, nil, args...) result, err := e.runThroughPolicies(ctx, db, query, nil) if err != nil { return Result{}, err diff --git a/pkg/server/auth_hook_test.go b/pkg/server/auth_hook_test.go index 1f8aeaa..58b9a78 100644 --- a/pkg/server/auth_hook_test.go +++ b/pkg/server/auth_hook_test.go @@ -46,7 +46,7 @@ func TestServeHTTP_ProjectSummary_ReturnsSummary(t *testing.T) { } baseURL := startServeHTTPWithSession(t, pathsByID, session) - resp, err := http.Get(baseURL + "/datatug/projects/project_summary?id=" + projectID) + resp, err := testHTTPClient.Get(baseURL + "/datatug/projects/project_summary?id=" + projectID) if err != nil { t.Fatalf("GET project_summary: %v", err) } @@ -105,7 +105,7 @@ func postCreateProject(t *testing.T, baseURL string) (*http.Response, error) { if err != nil { t.Fatalf("marshal request: %v", err) } - return http.Post(baseURL+"/datatug/projects/create_project?store=files", "application/json", bytes.NewReader(body)) + return testHTTPClient.Post(baseURL+"/datatug/projects/create_project?store=files", "application/json", bytes.NewReader(body)) } // TestServeHTTP_CreateProject_AuthGate is brief S36's item 1 regression test diff --git a/pkg/server/cors_test.go b/pkg/server/cors_test.go index ecc25ee..d691ac7 100644 --- a/pkg/server/cors_test.go +++ b/pkg/server/cors_test.go @@ -17,7 +17,7 @@ func getWithOrigin(t *testing.T, requestURL, origin string) *http.Response { t.Fatalf("NewRequest: %v", err) } req.Header.Set("Origin", origin) - resp, err := http.DefaultClient.Do(req) + resp, err := testHTTPClient.Do(req) if err != nil { t.Fatalf("GET %s (Origin: %s): %v", requestURL, origin, err) } @@ -89,3 +89,41 @@ func TestServeHTTP_CORS_DatatugApp(t *testing.T) { // POST/PUT/DELETE with a JSON body is preflighted, and that OPTIONS request // must pass this check too before the browser ever sends the real request // this file's tests cover. + +// TestServeHTTP_CORS_127001_StillRefused pins down, with a real request +// rather than only a doc comment, the exact gap TestServeHTTP_CORS_DatatugApp +// and http_server.go's AddKnownHosts comment both already describe: a dev +// server or UI addressing this agent via plain http://127.0.0.1: +// still gets refused, even though the OPTIONS-preflight side of the same +// origin (endpoints.IsSupportedOrigin, see the comment above) already +// accepts it — so a browser's preflight can succeed while its real request +// still 403s. S61 item 3 was briefed on the premise that the AddKnownHosts +// comment falsely claims 127.0.0.1 is allowed; it does not — re-read here +// character for character, it already says the opposite ("is NOT fixed by +// this call"), confirmed against PR #205's original, unmodified commit +// (7859c5b). Nothing needed correcting; this test instead turns the +// comment's claim into an executable, regression-proof one. Fixing the gap +// for real needs either an upstream sneat-go-core change +// (security.IsLocalhostHost treating 127.0.0.1/::1 as loopback synonyms of +// "localhost") or a local override of the swappable apicore.VerifyRequest +// var (not just origin-checking: VerifyRequest also does auth-token +// verification and is not itself swappable at a narrower grain — there is +// no exported hook between it and security.VerifyOrigin) — out of +// proportion for this stream, exactly as the existing comments already +// concluded. +func TestServeHTTP_CORS_127001_StillRefused(t *testing.T) { + const projectID = "cors-127001-project" + pathsByID := authHookProjectFixture(t, projectID) + session, err := secureread.NewSession(secureread.SessionOptions{As: "agent1", NoPolicies: true}) + if err != nil { + t.Fatalf("NewSession: %v", err) + } + baseURL := startServeHTTPWithSession(t, pathsByID, session) + summaryURL := baseURL + "/datatug/projects/project_summary?id=" + projectID + + resp := getWithOrigin(t, summaryURL, "http://127.0.0.1:4200") + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusForbidden { + t.Errorf("status = %d, want 403 (http://127.0.0.1: is still refused today — see this test's doc comment)", resp.StatusCode) + } +} diff --git a/pkg/server/http_server_test.go b/pkg/server/http_server_test.go index f5a7fdb..1762feb 100644 --- a/pkg/server/http_server_test.go +++ b/pkg/server/http_server_test.go @@ -94,12 +94,32 @@ func freeTCPPort(t *testing.T) int { return port } +// testHTTPClient is used for every HTTP request this package's ServeHTTP +// tests make against a server startServeHTTPWithSession started, instead of +// http.DefaultClient/http.Get/http.Post. http.DefaultClient's Transport +// keeps a completed connection open for keep-alive reuse; the server's +// graceful net/http.Server.Shutdown (called from every such test's +// t.Cleanup) can only finish once every connection it tracks has become +// idle from BOTH sides. TestServeHTTP_ProjectSummary_ReturnsSummary failed +// once in CI with "Shutdown: context deadline exceeded" and passed +// immediately on rerun - the signature of a race in that idle-connection +// bookkeeping, not a real, reproducible hang (Go's own httptest package +// works around the identical class of race by tracking and force-closing +// every client connection itself on Close, rather than trusting Shutdown's +// idle-connection handling alone - see httptest.Server.Close). Disabling +// keep-alives here means every response closes its connection as soon as +// it is read, so Shutdown never has anything to wait on from the client +// side at all. +var testHTTPClient = &http.Client{ + Transport: &http.Transport{DisableKeepAlives: true}, +} + func waitForServer(t *testing.T, url string) { t.Helper() deadline := time.Now().Add(5 * time.Second) var lastErr error for time.Now().Before(deadline) { - resp, err := http.Get(url) + resp, err := testHTTPClient.Get(url) if err == nil { _ = resp.Body.Close() return @@ -153,7 +173,7 @@ func assertPingAndAgentInfo(t *testing.T, baseURL string) { t.Helper() t.Run("ping", func(t *testing.T) { - resp, err := http.Get(baseURL + "/datatug/ping") + resp, err := testHTTPClient.Get(baseURL + "/datatug/ping") if err != nil { t.Fatalf("GET /datatug/ping: %v", err) } @@ -171,7 +191,7 @@ func assertPingAndAgentInfo(t *testing.T, baseURL string) { }) t.Run("agent-info", func(t *testing.T) { - resp, err := http.Get(baseURL + "/datatug/agent-info") + resp, err := testHTTPClient.Get(baseURL + "/datatug/agent-info") if err != nil { t.Fatalf("GET /datatug/agent-info: %v", err) } @@ -205,7 +225,7 @@ func TestServeHTTP_PingAndAgentInfo(t *testing.T) { assertPingAndAgentInfo(t, baseURL) t.Run("projects_summary uses the real store wiring", func(t *testing.T) { - resp, err := http.Get(baseURL + "/datatug/projects/projects_summary?storage=files") + resp, err := testHTTPClient.Get(baseURL + "/datatug/projects/projects_summary?storage=files") if err != nil { t.Fatalf("GET /datatug/projects/projects_summary: %v", err) } diff --git a/pkg/server/security_matrix_test.go b/pkg/server/security_matrix_test.go index 2c854bf..d137317 100644 --- a/pkg/server/security_matrix_test.go +++ b/pkg/server/security_matrix_test.go @@ -224,7 +224,7 @@ func postJSON(t *testing.T, baseURL, path string, body any) (status int, raw []b if err != nil { t.Fatalf("marshal request: %v", err) } - resp, err := http.Post(baseURL+path, "application/json", bytes.NewReader(encoded)) + resp, err := testHTTPClient.Post(baseURL+path, "application/json", bytes.NewReader(encoded)) if err != nil { t.Fatalf("POST %s: %v", path, err) } @@ -238,7 +238,7 @@ func postJSON(t *testing.T, baseURL, path string, body any) (status int, raw []b func getURL(t *testing.T, requestURL string) (status int, raw []byte) { t.Helper() - resp, err := http.Get(requestURL) + resp, err := testHTTPClient.Get(requestURL) if err != nil { t.Fatalf("GET %s: %v", requestURL, err) }