fix(secureread,server): real SQL param binding, deterministic ServeHTTP tests - #209
Merged
Conversation
…TP tests Three small, independently-verified follow-ups: 1. dal-go/dalgo2sql v0.11.7 tags the TextQuery arg-binding fix (named args via sql.Named, positional args via their bare Value — see dal-go/dalgo2sql#177). Bump go.mod, go mod tidy, and remove apps/datatugapp/commands/cmd_query_run_saved.go's own @ParamName string-substitution workaround (bindSQLNamedParams/sqlLiteral): secureread.Executor.RunNativeSQL now takes variadic dal.QueryArg args and passes them straight to dal.NewTextQuery, so a saved SQL query's declared parameters reach the driver as real bind values, never as SQL text. New test: TestRunNativeSQL_NamedArgBindsThroughDriver (pkg/secureread), plus the existing TestQueryRunSaved_SQL (apps/datatugapp/commands, real @InvoiceId parameter against the demo project) now exercises the real binding path instead of the old text-substitution one and still passes unchanged. 2. pkg/server/auth_hook_test.go's TestServeHTTP_ProjectSummary_ReturnsSummary failed once in CI with "Shutdown: context deadline exceeded" and passed immediately on rerun. Root cause: every ServeHTTP test in this package used http.Get/http.Post/http.DefaultClient, whose default Transport keeps completed connections open for keep-alive reuse — net/http.Server.Shutdown (called from every such test's t.Cleanup) can only finish once every connection it tracks is idle from both sides, and that transition is a race under load (Go's own httptest package works around the identical race by tracking and force-closing every client connection itself on Close, rather than trusting Shutdown's idle-connection handling alone). Fix: a shared testHTTPClient (Transport{DisableKeepAlives: true}) used for every HTTP call this package's tests make, so a connection never lingers for Shutdown to wait on. Proven deterministic with `go test -count=20 -run TestServeHTTP ./pkg/server` (100 sub-test runs, 0 failures) and the full package run 5x clean. 3. Investigated the CORS comment S205 added (http_server.go's AddKnownHosts call and cors_test.go's TestServeHTTP_CORS_DatatugApp doc comment) on the premise that it falsely claims a dev server on 127.0.0.1 is allowed. Re-read character for character against PR #205's original, unmodified commit (7859c5b): it already says the opposite — "is NOT fixed by this call" — and explains why (AddKnownHosts only adds an http:// variant for security.IsLocalhostHost, which never matches "127.0.0.1"). No incorrect claim found, so nothing to correct. Verified the underlying gap is real (curl with Origin: http://127.0.0.1:4200 against a live serve process gets 403; localhost/datatug.app origins pass the check) and confirmed there is no clean way to fix it from datatug-cli alone: the only swap point (apicore.VerifyRequest) sits above auth-token verification too, not just origin-checking, and there is no narrower hook — matching the existing comment's own conclusion. Added TestServeHTTP_CORS_127001_StillRefused so the documented gap is an executable regression proof, not only prose. Rebased onto origin/main (PR #207, cli-spine's source-resolver + secureread Provenance work) before pushing; the only conflict was in cmd_query_run_saved.go's runSQLSavedQuery (upstream added a projectDir parameter for api.ResolveCatalogPath, this change added the dal.QueryArg-based args) — resolved by keeping both. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dd3aoE41JSShyUuUmW8sSi
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three small, independently-verified follow-ups:
1. Real SQL param binding through dal-go/dalgo2sql v0.11.7
dal-go/dalgo2sqlv0.11.7 tags theTextQueryarg-binding fix (named args viasql.Named, positional args via their bareValue— dal-go/dalgo2sql#177). Bumpedgo.mod, rango mod tidy, and removedapps/datatugapp/commands/cmd_query_run_saved.go's own@ParamNamestring-substitution workaround (bindSQLNamedParams/sqlLiteral):secureread.Executor.RunNativeSQLnow takes variadicdal.QueryArgargs and passes them straight todal.NewTextQuery, so a saved SQL query's declared parameters reach the driver as real bind values, never as SQL text.New test:
TestRunNativeSQL_NamedArgBindsThroughDriver(pkg/secureread) — positional, named, and mixed cases against a real in-memory SQLite database. The existingTestQueryRunSaved_SQL(apps/datatugapp/commands, a real@InvoiceIdparameter against the demo project) now exercises the real binding path instead of the old text-substitution one and still passes unchanged.2. Deterministic
TestServeHTTP_ProjectSummary_ReturnsSummaryThis test failed once in CI with
Shutdown: context deadline exceededand passed immediately on rerun. Root cause: everyServeHTTPtest inpkg/serverusedhttp.Get/http.Post/http.DefaultClient, whose defaultTransportkeeps completed connections open for keep-alive reuse —net/http.Server.Shutdown(called from every such test'st.Cleanup) can only finish once every connection it tracks is idle from both sides, and that transition is a genuine race under load. (Go's ownhttptestpackage works around the identical race by tracking and force-closing every client connection itself onClose, rather than trustingShutdown's idle-connection handling alone — confirming this class of race is real, not hypothetical.)Fix: a shared
testHTTPClient(Transport{DisableKeepAlives: true}) used for every HTTP call this package's tests make (http_server_test.go,auth_hook_test.go,cors_test.go,security_matrix_test.go), so a connection never lingers forShutdownto wait on. No production code touched.Proven deterministic:
100 sub-test runs (5 tests × 20 repeats), 0 failures. Full
pkg/serverpackage also run 5x clean.3. CORS comment /
127.0.0.1origin — investigated, comment already correctBriefed on the premise that the CORS comment PR #205 added (
http_server.go'sAddKnownHostscall, andcors_test.go'sTestServeHTTP_CORS_DatatugAppdoc comment) falsely claims a dev server on127.0.0.1is allowed. Re-read both character for character against PR #205's original, unmodified commit (7859c5b): they already say the opposite — "is NOT fixed by this call" — and explain exactly why (AddKnownHostsonly ever adds anhttp://origin variant whensecurity.IsLocalhostHostmatches, and that function never recognizes"127.0.0.1"as a loopback synonym of"localhost"). No incorrect claim found, so there was nothing to correct.Verified the underlying gap is real, independently: built the CLI, ran
datatug serve, and curled it directly —Also confirmed there is no clean way to fix this from
datatug-clialone: the only swap point (apicore.VerifyRequest) sits above auth-token verification too, not just origin-checking, andhttpserver.AccessControlAllowOrigin/security.VerifyOriginbeneath it are plain, non-swappable functions with no narrower hook — matching the existing comment's own conclusion that a real fix needs either an upstreamsneat-go-corechange or a bigger local reimplementation than this stream's scope.Added
TestServeHTTP_CORS_127001_StillRefusedso the documented gap is an executable regression proof, not only prose — it complements the OPTIONS-preflight side (endpoints.IsSupportedOrigin, already tested and already127.0.0.1-aware) by covering the actual-request side, which is not.Rebase note
Rebased onto
origin/main(PR #207, lane S58's source-resolver +secureread.Result.Provenancework) before pushing. The only conflict was incmd_query_run_saved.go'srunSQLSavedQuery— upstream added aprojectDirparameter (forapi.ResolveCatalogPath), this change added thedal.QueryArg-basedargs— resolved by keeping both.Validation
wb run -- go build ./...— clean.wb run -- go vet ./...— clean.wb run -- go test ./...— clean except the known cgo-stub baseline (pkg/api,pkg/dbcopy,pkg/schemers/sqliteschema,pkg/sqlexecute, pre-existing and unrelated, reproduced identically in earlier streams).pkg/secureread,pkg/server,pkg/server/endpoints,apps/datatugapp/commandsall pass.go test -count=20 -run TestServeHTTP ./pkg/server— 100/100 sub-test runs pass (item 2's specific reproduction command).gofmt -l .— clean on every touched file. (Repo-widegofmt -l .still flags one pre-existing, untouched file —pkg/server/endpoints/semantic_demo_project_test.go— from a concurrent lane; not part of this diff.)go mod tidy -diff— clean, no changes needed.Do not merge — landing is owned by a separate session.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Dd3aoE41JSShyUuUmW8sSi