Conversation
WalkthroughThe changes establish a complete database infrastructure layer for the application. They add PostgreSQL connection pooling configuration with schema support, database migration management using Goose, SQLC-based code generation for type-safe SQL operations, an initial user schema with role-based access control, password hashing utilities using bcrypt, and the necessary configuration for SQLC code generation. These changes enable structured database access patterns and proper initialization flows. Sequence Diagram(s)sequenceDiagram
actor App as Application
participant Init as InitDB Function
participant PG as PostgreSQL
participant Pool as pgxpool.Pool
App->>Init: InitDB(ctx, url, schema)
Init->>Init: Parse pgxpool.Config
Init->>Init: Set MaxConns=10, MinConns=2
Init->>Init: Register AfterConnect Hook
Init->>PG: CreatePool with config
PG->>Pool: Initialize pool
Init->>Pool: pool.Ping(ctx)
Pool->>PG: Test connectivity
PG-->>Pool: Response
Pool-->>Init: Success
Init-->>App: Return *pgxpool.Pool
sequenceDiagram
actor App as Application
participant Migrate as RunMigrations
participant DB as SQL Connection
participant PG as PostgreSQL
participant Goose as Goose
App->>Migrate: RunMigrations(dbURL, schema)
Migrate->>DB: sql.Open(pgx driver)
DB->>PG: Connect
PG-->>DB: Connected
Migrate->>DB: ensureSchemaExists(schema)
DB->>PG: CREATE SCHEMA IF NOT EXISTS
DB->>PG: SET search_path TO schema
PG-->>DB: Confirmed
Migrate->>Goose: SetDialect(PostgreSQL)
Migrate->>Goose: SetBaseFS(embedded migrations)
Goose->>PG: goose.Up(ctx, db)
PG->>PG: Execute 0001_create_users.sql
PG-->>Goose: Migrations applied
Migrate-->>App: Return success or error
sequenceDiagram
actor App as Application
participant Querier as SQLC Querier
participant Pool as pgxpool.Pool
participant PG as PostgreSQL
participant Models as User Models
App->>Querier: GetUserByUsername(ctx, username)
Querier->>Pool: db.QueryRow(ctx, sql, username)
Pool->>PG: Execute parameterized query
PG->>PG: SELECT * FROM users WHERE username = $1
PG-->>Pool: Return row
Pool->>Models: row.Scan into User struct
Models-->>Querier: Populate ID, Username, Role, etc.
Querier-->>App: Return User or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@go.mod`:
- Line 46: The go.mod currently pins github.com/quic-go/quic-go to v0.54.0 which
has known high-severity vulnerabilities; update the module version to v0.57.0 by
changing the dependency entry for github.com/quic-go/quic-go to v0.57.0, then
run `go get github.com/quic-go/quic-go@v0.57.0` (or `go mod tidy`) to update
go.sum and resolve transitive changes, rebuild and run tests to ensure no
breakages from the quic-go upgrade.
In `@internal/db/db.go`:
- Around line 28-29: The code assigns to
poolConfig.ConnConfig.RuntimeParams["search_path"] without ensuring
RuntimeParams is initialized, which can panic; update the schema handling in
internal/db/db.go to check if poolConfig.ConnConfig.RuntimeParams is nil and if
so allocate a new map before setting the "search_path" key (i.e., guard and
initialize RuntimeParams on poolConfig.ConnConfig then set
RuntimeParams["search_path"] = schema), referencing poolConfig and
ConnConfig.RuntimeParams to locate the change.
In `@internal/db/migration.go`:
- Around line 53-71: The migration code uses sql.DB with SET search_path but
because sql.DB is a pooled client the SET only affects a single connection;
ensureSchemaExists (and the migration runner) must force all migration
statements to use the same connection: before setting search_path and running
goose.Up, constrain the pool (e.g., call db.SetMaxOpenConns(1) and
db.SetMaxIdleConns(1)), perform the schema creation, SET search_path
(setPathQuery) and run migrations, then restore the original pool settings;
alternatively open a dedicated single connection (db.Conn(ctx)) and execute
ensureSchemaExists and goose.Up on that connection so the search_path persists
for the migration session.
In `@internal/db/migrations/0001_create_users.sql`:
- Around line 18-19: Remove the hardcoded default admin password in the
migration that inserts into the users table; instead modify the migration logic
so it does not bake a known password into the INSERT INTO users (username,
password_hash, role) VALUES (...) statement—either omit creating the root user
in this SQL migration and require creation via a startup/provisioning script or
have the deployment process set a hashed password from an environment-provided
value (read at startup and inserted by application code), referencing the users
table and the password_hash column so you update any code paths that relied on
this seed to expect creation via env-driven provisioning or an explicit
admin-creation routine.
In `@internal/users/password_test.go`:
- Around line 39-49: TestCheckPasswordWithMigrationHash uses a hardcoded initial
password ("changeme") and migrationHash; update the repository to avoid relying
on a production default by (1) documenting in README/DEPLOYMENT that the initial
admin password must be rotated immediately, and (2) changing the seed/config
that uses CheckPassword to take the initial admin password from an environment
variable (e.g., INITIAL_ADMIN_PASSWORD) instead of hardcoding "changeme"; keep
TestCheckPasswordWithMigrationHash by either reading that test value from a
test-only env var or clearly marking the test as validating the static
migrationHash only (no runtime auth use).
🧹 Nitpick comments (13)
Makefile (2)
14-15: Renaminginstall→deplooks good.Clean rename. The
.PHONYlist on line 10 still referencesgeneratebut doesn't listdep— consider adding it somake depalways runs even if a file nameddepexists.Suggested fix
-.PHONY: all build build-server build-agent clean test generate docker docker-server docker-agent docker-all protoc protoc-gen run run-agent generate-certs +.PHONY: all build build-server build-agent clean test generate dep docker docker-server docker-agent docker-all protoc protoc-gen run run-agent generate-certs
30-31:generateno longer depends ondep;sqlcmust be pre-installed.If a developer runs
make generateon a fresh checkout withoutsqlcon$PATH, it will fail with a cryptic "command not found". Consider either restoring the dependency (generate: dep) or adding a guard:generate: `@command` -v sqlc >/dev/null 2>&1 || { echo "sqlc not found. Run 'make dep' first."; exit 1; } sqlc generateinternal/users/password.go (1)
13-18: Be aware of bcrypt's 72-byte input limit.
bcrypt.GenerateFromPasswordsilently truncates passwords longer than 72 bytes. If long passwords or passphrases are expected, a common mitigation is to pre-hash the input (e.g., SHA-256) before passing it to bcrypt. For most use cases the 72-byte limit is fine — just flagging for awareness.internal/users/password_test.go (1)
15-20: Redundant length check.Line 19 (
len(hash) > 0) is already covered byassert.NotEmptyon line 15. You can drop one of them.internal/db/queries/users.sql (2)
9-11:ListUsershas no pagination — this won't scale.Consider adding
LIMITandOFFSET(or cursor-based pagination) parameters now, before callers depend on the unbounded signature. Even a reasonable default limit would help.-- name: ListUsers :many SELECT * FROM users ORDER BY created_at DESC LIMIT $1 OFFSET $2;
1-11: Consider using explicit column lists instead ofSELECT *.While sqlc regenerates code on schema changes (mitigating the usual fragility concern), explicit columns make the queries self-documenting and avoid accidentally exposing sensitive fields (e.g.,
password_hash) in list endpoints.internal/db/migrations/0001_create_users.sql (2)
7-16: Redundant index onusername.The
UNIQUEconstraint on line 9 already creates a unique index onusername. The explicitCREATE INDEX idx_users_usernameon line 16 adds a second, non-unique index on the same column, which wastes storage and slows writes with no query benefit.Proposed fix — remove the redundant index
-CREATE INDEX IF NOT EXISTS idx_users_username ON users(username);And in the down migration:
-DROP INDEX IF EXISTS idx_users_username;
12-13: PreferTIMESTAMPTZoverTIMESTAMP.
TIMESTAMP(without time zone) discards timezone information, which can lead to ambiguous times across different server/client timezone settings.TIMESTAMPTZis the recommended default for PostgreSQL.Proposed fix
- created_at TIMESTAMP NOT NULL DEFAULT NOW(), - updated_at TIMESTAMP NOT NULL DEFAULT NOW() + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW()internal/db/migration.go (1)
37-46:goose.SetBaseFSandgoose.SetDialectmutate global state.These are package-level setters in goose v3. If any other part of the application (or tests) calls
RunMigrationsconcurrently, there's a data race. This is unlikely to be a problem today since migrations run once at startup, but worth noting as a caveat if the function is ever reused in parallel test suites.internal/db/db.go (2)
12-15:Configstruct is defined but never used.
InitDBtakesurlandschemaas separate arguments. Either useConfigas the parameter type or remove the dead struct.
23-25: Pool sizing is hardcoded.
MaxConns=10andMinConns=2are reasonable defaults but should be configurable (e.g., viaConfigor environment) for production tuning.internal/db/sqlc/users.sql.go (1)
52-55:ListUsersreturnspassword_hashand has no pagination.Two observations for the source queries (changes should go in
internal/db/queries/users.sql, not this generated file):
- All three queries select
password_hash. Ensure callers never serialize theUserstruct directly to API responses, or add a separate query/view that omits sensitive fields.ListUsershas noLIMIT/OFFSETor cursor — this will degrade as the user count grows.internal/db/sqlc/models.go (1)
57-64:PasswordHashcarries ajson:"password_hash"tag — risk of accidental serialization.Since this is generated code, the fix belongs in the API/handler layer: use a separate response DTO that omits sensitive fields rather than marshaling
Userdirectly. This complements the observation onusers.sql.go.
| github.com/pelletier/go-toml/v2 v2.2.4 // indirect | ||
| github.com/pmezard/go-difflib v1.0.0 // indirect | ||
| github.com/quic-go/qpack v0.5.1 // indirect | ||
| github.com/quic-go/quic-go v0.54.0 // indirect |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
quic-go v0.54.0 vulnerabilities GO-2025-4017 GO-2025-4233
💡 Result:
quic-go v0.54.0 is vulnerable to both GO-2025-4017 and GO-2025-4233:
-
GO-2025-4017 (aliases: CVE-2025-59530 / GHSA-47m2-4cr7-mhcw)
- Affected:
github.com/quic-go/quic-gov0.50.0–v0.54.0 (inclusive), plus older branches as noted. v0.54.0 is affected. - Impact: remote peer can trigger a panic / process crash (DoS) during handshake by sending a premature
HANDSHAKE_DONE. - Fix: upgrade to v0.54.1 (or v0.55.0+).
Sources: [1], [2]
- Affected:
-
GO-2025-4233 (aliases: CVE-2025-64702 / GHSA-g754-hx8w-x2g6)
- Affected:
github.com/quic-go/quic-go/http3before v0.57.0. If you use the http3 package with quic-go v0.54.0, you’re affected. - Impact: memory exhaustion (DoS) via HTTP/3 QPACK header expansion (decoded headers can massively exceed encoded size).
- Fix: upgrade to v0.57.0 (or later).
Sources: [3], [4]
- Affected:
Practical remediation for a project on quic-go v0.54.0: upgrade at least to v0.57.0 to address both issues (since it is ≥ v0.54.1 and includes the http3 fix at v0.57.0). [1], [3]
Sources
- Go Vulnerability DB: GO-2025-4017 (affected versions / fixed in v0.54.1) — https://pkg.go.dev/vuln/GO-2025-4017
- GitHub Advisory: GHSA-47m2-4cr7-mhcw (affected <=0.54.0, patched v0.54.1/v0.55.0) — GHSA-47m2-4cr7-mhcw
- Go Vulnerability DB: GO-2025-4233 (affected before v0.57.0) — https://pkg.go.dev/vuln/GO-2025-4233
- GitHub Advisory: GHSA-g754-hx8w-x2g6 (patched v0.57.0; QPACK expansion DoS details) — GHSA-g754-hx8w-x2g6
Bump quic-go from v0.54.0 to v0.57.0 to address known HIGH-severity vulnerabilities.
v0.54.0 contains two unpatched DoS vulnerabilities:
- GO-2025-4017 (CVE-2025-59530): remote peer can crash the process during QUIC handshake (fixed in v0.54.1)
- GO-2025-4233 (CVE-2025-64702): HTTP/3 QPACK header expansion enables memory exhaustion (fixed in v0.57.0)
Upgrade to v0.57.0 to address both issues.
🧰 Tools
🪛 OSV Scanner (2.3.2)
[HIGH] 46-46: github.com/quic-go/quic-go 0.54.0: Panic occurs when queuing undecryptable packets after handshake completion in github.com/quic-go/quic-go
(GO-2025-4017)
[HIGH] 46-46: github.com/quic-go/quic-go 0.54.0: HTTP/3 QPACK Header Expansion DoS in github.com/quic-go/quic-go
(GO-2025-4233)
[HIGH] 46-46: github.com/quic-go/quic-go 0.54.0: quic-go: Panic occurs when queuing undecryptable packets after handshake completion
[HIGH] 46-46: github.com/quic-go/quic-go 0.54.0: quic-go HTTP/3 QPACK Header Expansion DoS
🤖 Prompt for AI Agents
In `@go.mod` at line 46, The go.mod currently pins github.com/quic-go/quic-go to
v0.54.0 which has known high-severity vulnerabilities; update the module version
to v0.57.0 by changing the dependency entry for github.com/quic-go/quic-go to
v0.57.0, then run `go get github.com/quic-go/quic-go@v0.57.0` (or `go mod tidy`)
to update go.sum and resolve transitive changes, rebuild and run tests to ensure
no breakages from the quic-go upgrade.
| if schema != "" { | ||
| poolConfig.ConnConfig.RuntimeParams["search_path"] = schema |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/db/db.goRepository: EternisAI/silo-proxy
Length of output: 2155
🏁 Script executed:
rg -n 'RuntimeParams' --type go -B 2 -A 2Repository: EternisAI/silo-proxy
Length of output: 455
🏁 Script executed:
rg -n 'ParseConfig' --type go -B 3 -A 3Repository: EternisAI/silo-proxy
Length of output: 432
🌐 Web query:
pgxpool.ParseConfig RuntimeParams initialization pgx golang
💡 Result:
In pgx v5, pgxpool.ParseConfig returns a *pgxpool.Config that contains ConnConfig *pgx.ConnConfig (which embeds pgconn.Config). You set session defaults via ConnConfig.RuntimeParams (e.g. search_path, application_name).[1][2]
cfg, err := pgxpool.ParseConfig(connString)
if err != nil { /* handle */ }
// RuntimeParams is a map; ensure it’s initialized before writing to it.
if cfg.ConnConfig.RuntimeParams == nil {
cfg.ConnConfig.RuntimeParams = map[string]string{}
}
cfg.ConnConfig.RuntimeParams["application_name"] = "my-service"
cfg.ConnConfig.RuntimeParams["search_path"] = "my_schema,public"
pool, err := pgxpool.NewWithConfig(ctx, cfg)Notes:
RuntimeParamsare “run-time parameters to set on connection as session default values”.[3]- Configs must be created by
ParseConfig(manual initialization can panic later when connecting).[2]
Sources: [1] https://pkg.go.dev/github.com/jackc/pgx/v5/pgxpool [2] https://pkg.go.dev/github.com/jackc/pgx/v5 [3] https://pkg.go.dev/github.com/jackc/pgx/v5/pgconn
Potential nil map panic on line 29 — RuntimeParams may not be initialized.
pgxpool.ParseConfig initializes RuntimeParams only if the connection string contains runtime parameters. If the map is nil, the direct assignment will panic.
Proposed fix — guard with nil check
if schema != "" {
+ if poolConfig.ConnConfig.RuntimeParams == nil {
+ poolConfig.ConnConfig.RuntimeParams = make(map[string]string)
+ }
poolConfig.ConnConfig.RuntimeParams["search_path"] = schema📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if schema != "" { | |
| poolConfig.ConnConfig.RuntimeParams["search_path"] = schema | |
| if schema != "" { | |
| if poolConfig.ConnConfig.RuntimeParams == nil { | |
| poolConfig.ConnConfig.RuntimeParams = make(map[string]string) | |
| } | |
| poolConfig.ConnConfig.RuntimeParams["search_path"] = schema |
🤖 Prompt for AI Agents
In `@internal/db/db.go` around lines 28 - 29, The code assigns to
poolConfig.ConnConfig.RuntimeParams["search_path"] without ensuring
RuntimeParams is initialized, which can panic; update the schema handling in
internal/db/db.go to check if poolConfig.ConnConfig.RuntimeParams is nil and if
so allocate a new map before setting the "search_path" key (i.e., guard and
initialize RuntimeParams on poolConfig.ConnConfig then set
RuntimeParams["search_path"] = schema), referencing poolConfig and
ConnConfig.RuntimeParams to locate the change.
| func ensureSchemaExists(db *sql.DB, schema string) error { | ||
| // Create schema if it doesn't exist | ||
| query := "CREATE SCHEMA IF NOT EXISTS " + pgx.Identifier{schema}.Sanitize() | ||
| _, err := db.Exec(query) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| slog.Info("Schema is ready", "schema", schema) | ||
|
|
||
| // Set search_path to the schema to ensure migrations run there | ||
| setPathQuery := "SET search_path TO " + pgx.Identifier{schema}.Sanitize() | ||
| _, err = db.Exec(setPathQuery) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| slog.Info("Set search_path", "schema", schema) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
SET search_path may not stick for the migration — sql.DB pools connections.
db.Exec("SET search_path TO ...") applies to whichever connection the pool hands out. Subsequent calls from goose.Up may acquire a different connection that still has the default search_path. This can cause migrations to run in the wrong schema.
A straightforward fix is to constrain the pool to a single connection so all statements share it:
Proposed fix
func RunMigrations(dbURL string, schema string) error {
slog.Info("Running database migrations...")
if schema == "" {
schema = "public"
}
db, err := sql.Open("pgx", dbURL)
if err != nil {
return err
}
defer db.Close()
+ // Force a single connection so SET search_path persists across
+ // all statements issued by goose.
+ db.SetMaxOpenConns(1)
+
if err := ensureSchemaExists(db, schema); err != nil {
return err
}🤖 Prompt for AI Agents
In `@internal/db/migration.go` around lines 53 - 71, The migration code uses
sql.DB with SET search_path but because sql.DB is a pooled client the SET only
affects a single connection; ensureSchemaExists (and the migration runner) must
force all migration statements to use the same connection: before setting
search_path and running goose.Up, constrain the pool (e.g., call
db.SetMaxOpenConns(1) and db.SetMaxIdleConns(1)), perform the schema creation,
SET search_path (setPathQuery) and run migrations, then restore the original
pool settings; alternatively open a dedicated single connection (db.Conn(ctx))
and execute ensureSchemaExists and goose.Up on that connection so the
search_path persists for the migration session.
| -- Default root user with bcrypt hashed password: 'changeme' | ||
| INSERT INTO users (username, password_hash, role) VALUES ('root', '$2a$10$uejoNCSLZ9YkKOZriLlSGeg0pm/nuGVS3nRuSPyYuk/Z7HJHKBhGO', 'Admin'); |
There was a problem hiding this comment.
Hardcoded default admin password is a security risk.
The root user seed with a well-known password (changeme) will ship into every deployment. If not rotated immediately, this is a trivially exploitable admin credential. Consider requiring the password to be set via an environment variable at startup or through a separate provisioning step rather than baking it into the migration.
🤖 Prompt for AI Agents
In `@internal/db/migrations/0001_create_users.sql` around lines 18 - 19, Remove
the hardcoded default admin password in the migration that inserts into the
users table; instead modify the migration logic so it does not bake a known
password into the INSERT INTO users (username, password_hash, role) VALUES (...)
statement—either omit creating the root user in this SQL migration and require
creation via a startup/provisioning script or have the deployment process set a
hashed password from an environment-provided value (read at startup and inserted
by application code), referencing the users table and the password_hash column
so you update any code paths that relied on this seed to expect creation via
env-driven provisioning or an explicit admin-creation routine.
| func TestCheckPasswordWithMigrationHash(t *testing.T) { | ||
| // This test verifies the hardcoded hash in the migration works | ||
| // The hash was generated with password "changeme" | ||
| migrationHash := "$2a$10$uejoNCSLZ9YkKOZriLlSGeg0pm/nuGVS3nRuSPyYuk/Z7HJHKBhGO" | ||
|
|
||
| // Should match the expected password | ||
| assert.True(t, CheckPassword("changeme", migrationHash)) | ||
|
|
||
| // Should not match other passwords | ||
| assert.False(t, CheckPassword("root", migrationHash)) | ||
| assert.False(t, CheckPassword("admin", migrationHash)) |
There was a problem hiding this comment.
Good practice testing the migration hash — ensure "changeme" is changed before production.
The test validates the hardcoded migration hash, which is great for catching accidental changes to the seed data. Make sure the default password "changeme" is documented as requiring immediate rotation after first deployment, or consider using an environment variable for the initial admin password instead.
🤖 Prompt for AI Agents
In `@internal/users/password_test.go` around lines 39 - 49,
TestCheckPasswordWithMigrationHash uses a hardcoded initial password
("changeme") and migrationHash; update the repository to avoid relying on a
production default by (1) documenting in README/DEPLOYMENT that the initial
admin password must be rotated immediately, and (2) changing the seed/config
that uses CheckPassword to take the initial admin password from an environment
variable (e.g., INITIAL_ADMIN_PASSWORD) instead of hardcoding "changeme"; keep
TestCheckPasswordWithMigrationHash by either reading that test value from a
test-only env var or clearly marking the test as validating the static
migrationHash only (no runtime auth use).
1 participant
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores