Conversation
📝 WalkthroughWalkthroughAdds PostgreSQL support, DB migrations and install/upgrade CLI flows, a full authentication subsystem (OIDC, session, API-token), repository layer and models, S3 helpers, Dex OIDC test config, expanded docs and config, and related dependency and server/API wiring changes. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Main as main()
participant DB as PostgreSQL
participant Migrations as migrations
participant Admin as Admin Creator
rect rgb(220,230,240)
Note over CLI,Admin: Install flow (--install)
CLI->>Main: start with --install
Main->>DB: open connection
DB-->>Main: connected
Main->>Migrations: check last migration
Migrations->>DB: query schema_migrations
DB-->>Migrations: last version (or none)
Migrations-->>Main: pending migrations
Main->>Migrations: execute v0.1.0
Migrations->>DB: create tables/indexes
DB-->>Migrations: success
Migrations->>DB: record v0.1.0
DB-->>Migrations: recorded
Main->>DB: check users
DB-->>Main: none
Main->>Admin: generate password & hash
Admin-->>Main: hashed creds
Main->>DB: create default admin user (transaction)
DB-->>Main: committed
end
sequenceDiagram
participant Client
participant API as Server
participant Auth as auth.Middleware
participant Cache as TokenCache
participant DB as PostgreSQL
participant OIDC as Dex/OIDC
rect rgb(240,230,220)
Note over Client,API: API Token auth path
Client->>API: request with Bearer token
API->>Auth: middleware checks token
Auth->>Cache: GetAPIToken(value)
alt cached
Cache-->>Auth: token + user
else not cached
Auth->>DB: GetTokenByValue
DB-->>Auth: token + user
Auth->>Cache: cache token
end
Auth->>Auth: validate expiry & active user
Auth->>DB: async UpdateTokenLastUsed
Auth-->>API: set user context
API-->>Client: response
end
rect rgb(220,240,230)
Note over Client,OIDC: OIDC login flow
Client->>API: POST /auth/oidc/login
API->>Auth: OIDCLogin
Auth->>Client: set state/nonce cookie, redirect to OIDC
Client->>OIDC: auth request
OIDC-->>Client: auth response (redirect with code)
Client->>API: GET /auth/oidc/callback?code=...
API->>Auth: OIDCCallback
Auth->>OIDC: Exchange code -> tokens/claims
OIDC-->>Auth: id_token/claims
Auth->>DB: GetUserByEmail(claims.email)
DB-->>Auth: user
Auth->>Auth: SaveSession(user)
Auth-->>Client: redirect to frontend
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 15
🤖 Fix all issues with AI agents
In @cmd/install.go:
- Around line 134-143: Replace the hardcoded Base.ID "admin-user-id" in the
adminUser construction with a generated UUID: import your UUID generator (e.g.,
github.com/google/uuid) or use an existing helper, call uuid.NewString() (or
equivalent) to produce a unique ID before or inline in the adminUser :=
&models.User{ ... Base: models.Base{ID: uuid.NewString()}, ... } so each install
creates a unique primary key and avoids collisions on re-run.
In @cmd/main.go:
- Around line 120-124: The code logs "Starting server... at " +
cfg.Server.Address before checking the result of api.NewServer(core), which can
emit a misleading message if server creation fails; update the flow so you first
call server, err := api.NewServer(core), then immediately check if err != nil
and call log.Fatal().Err(err).Msg("Failed to create server"), and only after
that successful check call log.Info().Msg("Starting server... at " +
cfg.Server.Address) (i.e., move the log.Info() to after the nil error check or
log success inside the success branch).
In @cmd/upgrade.go:
- Around line 105-119: getLastMigrationVersion currently relies on SQL ORDER BY
version DESC which sorts lexicographically and can return an incorrect latest
semantic version; change getLastMigrationVersion to SELECT all version strings
from schema_migrations, scan them into a slice, and then determine the newest
version in Go using a semantic version comparator (e.g., semver.Compare or
equivalent) before returning it; ensure you still return "v0.0.0" when no rows
or when isTableNotExistErr(err) is encountered and propagate other errors
unchanged.
In @internal/auth/auth.go:
- Around line 40-53: The apiTokens map on the Auth struct is an unbounded
in-memory cache that can grow indefinitely; modify token caching to be bounded
and to remove expired entries: either replace apiTokens with a size-limited
LRU/time-based cache implementation (or wrap it with an LRU library) or, at
minimum, update the PruneExpiredTokens routine to also iterate a.apiTokens under
a.Lock() and delete entries whose Token.ExpiresAt is present and before
time.Now(); ensure all accesses use the existing a.RWMutex (use RLock/RUnlock
for reads and Lock/Unlock for deletions) and keep the token validation flow in
methods that reference Auth.apiTokens unchanged aside from using the new cache
API or deletion logic.
- Around line 188-240: The GetAPIToken function is logging the raw API token
value on cache miss and when not found in DB; remove the secret from logs and
instead log a non-secret identifier (e.g., token ID when available) or a
non-reversible fingerprint (e.g., sha256 hex or masked string). Concretely,
update the two log calls inside GetAPIToken that reference tokenValue (the Debug
"API token cache miss..." and the Warn "API token not found in DB: %s") so they
do not include tokenValue; either log a generic message ("API token cache miss,
fetching from DB") and ("API token not found in DB") or compute and log a safe
fingerprint (e.g., sha256(tokenValue) or maskToken(tokenValue)) before logging.
Ensure any new helper (masking or hashing) is non-reversible and used only for
logging, and keep existing logs that reference token.Name/ID unchanged.
In @internal/auth/handlers.go:
- Around line 31-54: The login handler returns early when a user is not found,
leaking timing differences; modify the logic in the handler that calls
repository.GetUserByUsername to perform a dummy password hash comparison when
err != nil or user == nil (before returning the same Unauthorized error) so
timing matches the real password path, then keep the existing flow for
PasswordLogin and user.CheckPassword; use a constant dummy bcrypt hash in the
dummy comparison (e.g., via bcrypt.CompareHashAndPassword) and retain the same
log/message ("Invalid credentials") and warning about non-existent users.
- Around line 170-181: Replace the detailed error strings returned to clients in
the ExchangeOIDCToken and GetUserByEmail failure paths with generic HTTP error
messages, while logging the original errors server-side; specifically, in the
block that calls a.ExchangeOIDCToken(c.QueryParam("code"), nonceCookie.Value)
and the block that calls a.repository.GetUserByEmail(ctx, claims.Email), call
the logger (e.g., a.logger or request context logger) to record the full err
details, then return echo.NewHTTPError with a non-revealing message like "OIDC
authentication failed" or "internal server error" respectively, preserving
existing status codes.
In @internal/core/core.go:
- Around line 39-40: The repository is being created with a potentially nil DB
which leads to panics when repository methods call r.db (e.g., GetUser calls
r.db.GetContext()). Fix by validating the DB before creating core.Repository
(check db != nil before calling storage.NewRepository in core.go) and also
harden repository methods (in the repository implementation referenced by
storage.NewRepository, add a nil check like if r.db == nil return an explicit
error from methods such as GetUser) so callers get a proper error instead of a
panic.
In @internal/core/s3_service.go:
- Around line 207-249: The PutObject implementation currently reads the entire
body into memory; update S3Service.PutObject to stream the provided io.Reader
directly into the s3.PutObjectInput (set Body to the original body instead of
bytes.NewReader(bodyBytes)) and remove the io.ReadAll logic and the misleading
"without extra memory allocation" comment; if seekability is required for
retries, replace that approach with either an io.ReadSeeker backed by a temp
file or use multipart upload for large files (handle/limit sizes accordingly)
and keep existing error logging around s.core.S3Client.PutObject and the
surrounding Logger calls.
In @internal/models/auth_models.go:
- Around line 72-81: The Session struct defined in
internal/models/auth_models.go (type Session) mismatches the v0.1.0 sessions
table and is unused; remove this struct to avoid confusion and runtime failures,
and ensure any session handling uses the external simplesessions.Session type
instead (search for Session, SessionID, UserID references and replace or update
usages to use simplesessions.Session or adjust the DB schema if you
intentionally need extra fields).
In @README.md:
- Around line 45-47: The README currently says authentication is optional but
the PR decision mandates no unauthenticated access; update the Authentication
section text to state that authentication is required for all API requests and
list the three supported authentication methods (refer to the documented methods
elsewhere in the repo) — remove the word "optional", replace phrasing like "If
authentication is enabled" with explicit language such as "Authentication is
required" and instruct callers to include one of the three supported
authentication headers or use the auth endpoints before making any API requests.
- Around line 62-75: In the "Login Response Example" JSON and all curl examples
that show token/API key strings, replace any token-like literals with clearly
marked placeholders (e.g., change the "token" value to "<YOUR-BEARER-TOKEN>" or
"YOUR-TOKEN-HERE" and any API key strings to "<YOUR-API-KEY>"); update every
occurrence referenced (the "Login Response Example" block, any "Authorization:
Bearer ..." header examples, and shown API key fields) so examples no longer
contain token-like values but use consistent placeholder text instead.
🧹 Nitpick comments (18)
README.md (1)
38-44: Fix markdown linting and style issues.
Line 41: The fenced code block for the base URL is missing a language specifier (MD040). Add a language identifier:
-``` +``` http://localhost:8080/api -``` +```Lines 10-15: Repetitive sentence structure with "Install" at the beginning of consecutive bullet points. Consider varying the phrasing for better readability:
- Install `curl` for making HTTP requests. - Install `awslocal` via `brew install awscli-local` for local AWS S3 interactions. - Install `awscli` for AWS CLI commands. - (Optional) Install `jq` for parsing JSON responses in shell scripts. - (Optional) Install `make` for easier command execution.Could be reworded as:
+ - `curl` for making HTTP requests + - `awslocal` (install via `brew install awscli-local`) for local AWS S3 interactions + - `awscli` for AWS CLI commands + - (Optional) `jq` for parsing JSON responses in shell scripts + - (Optional) `make` for easier command executionBare URLs (MD034): Several URLs appear without markdown link formatting. Wrap them in brackets and parentheses, e.g.,
[text](url), for better accessibility and consistency.Also applies to: 10-16
.docker/dex/README.md (1)
8-24: Wrap bare URLs in markdown link syntax.Lines 10, 11, 15, 22, and 29 contain bare URLs that should be wrapped in markdown link syntax for better formatting and accessibility:
- Client ID: dex - Client Secret: dex-secret - Issuer URL: http://localhost:5556/dex - The OAuth2 redirect URL should be set to: http://localhost:8080/callback - -The OIDC discovery endpoint is: - -http://localhost:5556/dex/.well-known/openid-configuration + Client ID: dex + Client Secret: dex-secret + Issuer URL: [http://localhost:5556/dex](http://localhost:5556/dex) + The OAuth2 redirect URL should be set to: [http://localhost:8080/callback](http://localhost:8080/callback) + + The OIDC discovery endpoint is: + + [http://localhost:5556/dex/.well-known/openid-configuration](http://localhost:5556/dex/.well-known/openid-configuration)Also wrap the bcrypt generator link on line 29:
- You can generate your via cli or using online tools like https://bcrypt-generator.com/ + You can generate your via cli or using online tools like [https://bcrypt-generator.com/](https://bcrypt-generator.com/)Also applies to: 29-32
docker-compose.yml (1)
68-77: Consider pinning the Dex image version and adding a healthcheck.A few observations on the new dex service:
- Using
:latesttag reduces reproducibility; consider pinning to a specific version (e.g.,v2.39.1).- The volume path
.docker/dex/differs from other services that use./docker/— verify this is intentional.- Other services define healthchecks; adding one for Dex would improve orchestration reliability.
♻️ Suggested improvements
dex: - image: ghcr.io/dexidp/dex:latest + image: ghcr.io/dexidp/dex:v2.39.1 volumes: - - .docker/dex/dex-config.yaml:/etc/dex/config.yaml + - ./docker/dex/dex-config.yaml:/etc/dex/config.yaml command: ["dex", "serve", "/etc/dex/config.yaml"] networks: - explorer451 ports: - "5556:5556" - "5557:5557" + healthcheck: + test: ["CMD", "wget", "-q", "--spider", "http://localhost:5556/dex/.well-known/openid-configuration"] + interval: 10s + timeout: 5s + retries: 5cmd/install.go (1)
119-127: Password generation is acceptable but could be cleaner.Truncating a base64 token to 16 characters still provides ~96 bits of entropy, which is sufficient for a temporary password. However, consider generating exactly the bytes needed rather than truncating.
♻️ Alternative approach
// Generate 12 random bytes -> 16 base64 characters bytes := make([]byte, 12) if _, err := rand.Read(bytes); err != nil { return fmt.Errorf("failed to generate random password: %w", err) } defaultPassword := base64.URLEncoding.EncodeToString(bytes)internal/migrations/v0.1.0.go (2)
34-43: Consider adding an index ontokens.expires_atfor cleanup queries.The
PruneExpiredTokensoperation (from Repository interface) will query expired tokens byexpires_at. An index would improve performance for this cleanup operation.♻️ Add index for token expiration queries
`CREATE INDEX IF NOT EXISTS idx_tokens_user_id ON tokens(user_id)`, `CREATE INDEX IF NOT EXISTS idx_tokens_token ON tokens(token)`, +`CREATE INDEX IF NOT EXISTS idx_tokens_expires_at ON tokens(expires_at)`, `CREATE INDEX IF NOT EXISTS idx_sessions ON sessions (id, created_at)`,
55-55: Review the sessions index design.The composite index
(id, created_at)hasidas the leading column, butidis already the primary key. If the intent is to support session cleanup by age, consider indexingcreated_atalone instead.♻️ Suggested change
-`CREATE INDEX IF NOT EXISTS idx_sessions ON sessions (id, created_at)`, +`CREATE INDEX IF NOT EXISTS idx_sessions_created_at ON sessions (created_at)`,internal/config/config.go (1)
27-34: Consider adding validation for required OIDC fields when enabled.When
OIDC.Enabledis true,ProviderURL,ClientID, andClientSecretare required. Consider adding validation inLoad()or a separateValidate()method to fail fast with clear error messages.cmd/upgrade.go (1)
121-130: Consider usingerrors.Asfor idiomatic error type assertion.Using
errors.Asis more robust as it handles wrapped errors.♻️ Suggested improvement
func isTableNotExistErr(err error) bool { - if p, ok := err.(*pq.Error); ok { - if p.Code == "42P01" { - return true - } + var pqErr *pq.Error + if errors.As(err, &pqErr) { + return pqErr.Code == "42P01" } return false }Note: This would require adding
"errors"to imports.cmd/main.go (1)
60-90: Database connection not closed on application shutdown.The database connection
dbis opened but never explicitly closed during graceful shutdown. While the OS will reclaim resources on exit, it's good practice to defer closing the connection, especially for connection pool cleanup and to ensure in-flight queries complete gracefully.♻️ Suggested fix
Add a deferred close after successful connection establishment:
} else { // Configure database connection pool db.SetMaxOpenConns(cfg.Database.MaxOpenConns) db.SetMaxIdleConns(cfg.Database.MaxIdleConns) db.SetConnMaxLifetime(cfg.Database.ConnMaxLifetime) + defer db.Close() // Test the connection if err := db.Ping(); err != nil {Note: Be careful with placement since
dbcan be set tonilon ping failure. An alternative is to close it in the shutdown sequence after server shutdown.internal/api/server.go (2)
46-60: Auth callback creates newcontext.Background()for each user lookup.The
GetUsercallback creates a freshcontext.Background()on each call, discarding request-scoped context (timeouts, cancellation, tracing). Consider passing the request context through or storing it appropriately.♻️ Consider refactoring the callback signature
The callback signature in
auth.Callbackscould be extended to accept a context parameter:GetUser: func(ctx context.Context, id string) (*models.User, error) { return core.Repository.GetUser(ctx, id) }This would require updating the
Callbacksstruct definition ininternal/auth/auth.go.
101-130: Route duplication between protected and unprotected blocks.The route definitions are duplicated between the
if s.auth != nilandelsebranches. Consider extracting a helper function to reduce duplication and ensure consistency.♻️ Suggested refactor to reduce duplication
func (s *Server) setupRoutes() { // Health check endpoint s.echo.GET("/health", func(c echo.Context) error { return c.JSON(http.StatusOK, map[string]string{"status": "ok"}) }) api := s.echo.Group("/api") // Authentication routes (if auth is available) if s.auth != nil { auth := api.Group("/auth") auth.POST("/login", s.auth.Login) auth.POST("/logout", s.auth.LogoutHandler) auth.GET("/profile", s.auth.Profile, s.auth.Middleware) auth.GET("/oidc/login", s.auth.OIDCLogin) auth.GET("/oidc/callback", s.auth.OIDCCallback) } // API endpoints - protected if auth available var apiGroup *echo.Group if s.auth != nil { apiGroup = api.Group("", s.auth.Middleware) } else { apiGroup = api } s.registerBucketRoutes(apiGroup) } func (s *Server) registerBucketRoutes(g *echo.Group) { g.GET("/buckets", s.listBuckets) g.GET("/buckets/:bucket/details", s.getBucketDetails) // ... etc }internal/storage/repository.go (5)
44-63: Non-atomic count and select may return inconsistent data.The
ListUsersfunction executes two separate queries (count then select) without a transaction. Between these queries, users could be added or removed, causing the returned count to not match the actual slice length or pagination to be off.💡 Consider using a window function or transaction
Option 1: Use a window function (PostgreSQL):
SELECT id, ..., COUNT(*) OVER() as total_count FROM users ORDER BY created_at DESC LIMIT $1 OFFSET $2Option 2: Wrap in a read transaction for consistency.
65-96: Get methods don't distinguish "not found" from other errors.The
GetUser,GetUserByUsername, andGetUserByEmailmethods wrap all errors generically. Callers cannot easily distinguish between "user not found" (sql.ErrNoRows) and database connectivity issues without unwrapping errors.💡 Consider exposing sql.ErrNoRows or defining a custom error
Option 1: Let
sql.ErrNoRowspass through unwrapped for "not found" cases.Option 2: Define a custom
ErrNotFounderror:var ErrNotFound = errors.New("record not found") func (r *repository) GetUser(ctx context.Context, id string) (*models.User, error) { var user models.User err := r.db.GetContext(ctx, &user, query, id) if err != nil { if errors.Is(err, sql.ErrNoRows) { return nil, ErrNotFound } return nil, fmt.Errorf("failed to get user: %w", err) } return &user, nil }
108-116: UpdateUser doesn't verify row was actually updated.The
UpdateUsermethod doesn't checkRowsAffected(). If the user ID doesn't exist, the update silently succeeds without modifying any rows, which may mask bugs where updates are attempted on non-existent users.♻️ Consider checking rows affected
func (r *repository) UpdateUser(ctx context.Context, user *models.User) error { query := `UPDATE users SET name = $2, email = $3, username = $4, password = $5, status = $6, role = $7, password_login = $8, loggedin_at = $9, updated_at = NOW() WHERE id = $1` - _, err := r.db.ExecContext(ctx, query, user.ID, user.Name, user.Email, user.Username, user.Password, user.Status, user.Role, user.PasswordLogin, user.LoggedinAt) + result, err := r.db.ExecContext(ctx, query, user.ID, user.Name, user.Email, user.Username, user.Password, user.Status, user.Role, user.PasswordLogin, user.LoggedinAt) if err != nil { return fmt.Errorf("failed to update user: %w", err) } + rows, err := result.RowsAffected() + if err != nil { + return fmt.Errorf("failed to get rows affected: %w", err) + } + if rows == 0 { + return ErrNotFound // or sql.ErrNoRows + } return nil }
118-125: DeleteUser doesn't verify row was actually deleted.Same issue as
UpdateUser- the delete silently succeeds even if no user with that ID exists.
170-177: DeleteToken doesn't verify row was deleted, unlike PruneExpiredTokens.For consistency with
PruneExpiredTokenswhich returns rows affected, consider checking if the token was actually deleted.internal/auth/auth.go (2)
86-130: Pruning goroutines use different contexts than the parent.The token pruner at line 118 creates a new
context.Background()instead of using the passedctx. While theselectonctx.Done()handles shutdown, the repository call itself won't be cancelled if the parent context is done during the operation.♻️ Consider using ctx for repository calls
case <-ticker.C: a.log.Debug().Msg("Pruning expired API tokens") - count, err := a.repository.PruneExpiredTokens(context.Background()) + pruneCtx, cancel := context.WithTimeout(ctx, 30*time.Second) + count, err := a.repository.PruneExpiredTokens(pruneCtx) + cancel()
60-84: OIDC initialization failure doesn't prevent service startup.When OIDC is enabled in config but initialization fails, the service logs an error and disables OIDC gracefully rather than failing startup. While the error is logged and OIDC routes properly guard against the disabled state (returning 501), consider whether startup should fail fatally when OIDC is explicitly required, or whether the current graceful degradation approach better suits your deployment expectations.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
.docker/dex/README.md.docker/dex/dex-config.yamlMakefileREADME.mdcmd/install.gocmd/main.gocmd/upgrade.goconfig.yml.sampledocker-compose.ymlgo.modinternal/api/server.gointernal/auth/auth.gointernal/auth/handlers.gointernal/config/config.gointernal/core/core.gointernal/core/s3_service.gointernal/core/utils.gointernal/migrations/v0.1.0.gointernal/models/auth_models.gointernal/storage/repository.gomain.go
🧰 Additional context used
🧬 Code graph analysis (10)
internal/migrations/v0.1.0.go (2)
internal/config/config.go (1)
Config(37-43)internal/logger/logger.go (1)
Logger(11-13)
cmd/install.go (2)
internal/core/utils.go (1)
GenerateSecureTokenBase64(9-19)internal/models/auth_models.go (2)
User(18-30)Base(12-16)
cmd/upgrade.go (1)
internal/migrations/v0.1.0.go (1)
V0_1_0(13-85)
internal/core/core.go (1)
internal/storage/repository.go (2)
Repository(13-31)NewRepository(37-41)
cmd/main.go (2)
internal/core/core.go (1)
NewCore(24-46)internal/api/server.go (2)
NewServer(36-78)Server(29-33)
internal/auth/handlers.go (3)
internal/auth/auth.go (2)
Auth(40-53)UserKey(27-27)internal/models/auth_models.go (1)
User(18-30)internal/core/utils.go (1)
GenerateSecureTokenBase64(9-19)
internal/auth/auth.go (5)
internal/models/auth_models.go (3)
Token(63-70)User(18-30)Session(72-81)internal/storage/repository.go (1)
Repository(13-31)internal/config/config.go (2)
OIDCConfig(28-34)Config(37-43)internal/logger/logger.go (1)
Logger(11-13)internal/api/server.go (1)
Server(29-33)
internal/api/server.go (4)
internal/core/core.go (1)
Core(13-21)internal/auth/auth.go (3)
Auth(40-53)New(132-186)Callbacks(56-58)internal/models/auth_models.go (1)
User(18-30)internal/config/config.go (1)
Config(37-43)
internal/storage/repository.go (1)
internal/models/auth_models.go (2)
User(18-30)Token(63-70)
internal/models/auth_models.go (1)
internal/auth/auth.go (1)
New(132-186)
🪛 Gitleaks (8.30.0)
README.md
[high] 143-144: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 154-155: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 72-72: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
README.md
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...localfor local AWS S3 interactions. - Installawscli` for AWS CLI commands. - (Optio...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~140-~140: Ensure spelling is correct
Context: ...uth/profile \ -b cookies.txt ### Logoutbash # Using Bearer Token curl -X POST http://localhost:8080/api/auth/logout \ -H "Authorization: Bearer your-token" # Using Session Cookie curl -X POST http://localhost:8080/api/auth/logout \ -b cookies.txt ### Refresh Tokenbash # Refresh your bea...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
.docker/dex/README.md
10-10: Bare URL used
(MD034, no-bare-urls)
11-11: Bare URL used
(MD034, no-bare-urls)
15-15: Bare URL used
(MD034, no-bare-urls)
22-22: Bare URL used
(MD034, no-bare-urls)
29-29: Bare URL used
(MD034, no-bare-urls)
README.md
41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (27)
main.go (1)
44-44: LGTM! Typo fix improves user-facing message.The correction from "ro" to "to" makes the default region message grammatically correct.
Makefile (1)
67-77: LGTM! Install and upgrade targets support the database integration workflow.These targets provide convenient commands for database schema installation and upgrades, aligning with the PR's authentication and database integration objectives.
internal/core/utils.go (1)
9-19: LGTM! Secure token generation follows cryptographic best practices.The implementation correctly uses:
crypto/randfor cryptographically secure randomness- 32 bytes (256 bits) providing strong entropy
base64.URLEncodingfor URL-safe encodinginternal/core/s3_service.go (1)
177-205: LGTM! GetObject implementation is correct.The method properly retrieves S3 objects with appropriate logging and error handling.
.docker/dex/dex-config.yaml (1)
14-26: Configuration is properly isolated for local development.This configuration is appropriately used only in the local
docker-compose.ymlfile with no references in production deployment configurations. The hardcoded credentials (dex-secret, password hash) do not appear elsewhere in the codebase, the Dockerfile does not reference them, and no Kubernetes, Helm, or Terraform deployment manifests were found that could accidentally deploy these credentials to production.config.yml.sample (1)
12-27: LGTM!The sample configuration clearly documents the new database and OIDC sections with appropriate placeholder values and helpful comments indicating their optional nature.
cmd/install.go (1)
180-188: Sensitive credential logging is intentional but warrants caution.Logging the plaintext password during install is a common bootstrap pattern. Ensure operators understand this appears in logs and should be rotated after first login. The prominent warning messages are helpful.
internal/core/core.go (1)
19-20: LGTM!The new
DBandRepositoryfields integrate cleanly with the existing Core structure, enabling database-backed authentication.internal/migrations/v0.1.0.go (1)
58-82: LGTM!Transaction handling is correct with proper deferred rollback that safely handles the already-committed case via
sql.ErrTxDonecheck.internal/config/config.go (1)
112-123: LGTM!Database connection pool defaults are sensible: 25 max open connections, 5 idle connections, and 5-minute lifetime for connection recycling.
internal/auth/handlers.go (2)
88-105: LGTM!The Profile handler correctly retrieves the authenticated user from context and returns a safe subset of user information.
80-85: LGTM!Clean logout handler that delegates session cleanup to the
Logoutmethod.cmd/upgrade.go (2)
16-23: LGTM on type definitions and migration list.The
migFuncstruct andmigListslice provide a clean, extensible migration framework. The design allows for easy addition of future migrations.
25-67: Upgrade function looks solid with proper flow.The upgrade flow correctly handles user prompts, migration execution, and version recording. Good that
createDefaultAdminUserIfNeededis called after migrations complete.cmd/main.go (1)
22-27: LGTM on flag definitions.The command-line flags are well-defined with clear descriptions. Good use of
pflagfor POSIX-compliant flags.internal/api/server.go (2)
18-26: LGTM on CustomValidator implementation.Clean implementation wrapping
go-playground/validatorfor Echo's validator interface.
63-69: Configure CORS middleware with explicit allowed origins and methods.The
middleware.CORS()on line 65 uses Echo's permissive default configuration, which allows all origins, methods, and headers. Since the API supports authentication mechanisms (JWT, session cookies, OIDC) and is designed for client consumption, explicitly configure allowed origins, methods, and headers rather than relying on defaults.internal/models/auth_models.go (4)
12-16: LGTM on Base struct.Clean base struct with common fields for ID and timestamps using nullable time types.
18-30: LGTM on User struct design.Good security practice: password hash is omitted from JSON responses via
json:"-"tag. The nullable password field correctly supports OIDC-only users.
32-43: LGTM on HashPassword implementation.Uses
bcrypt.DefaultCost(currently 10), which is reasonable. Correctly handles empty passwords for OIDC-only users by setting NULL.
45-61: LGTM on CheckPassword implementation.Correctly distinguishes between password mismatch (returns
false, nil) and other errors. The bcrypt comparison provides timing-attack resistance.internal/storage/repository.go (1)
13-31: LGTM on Repository interface.Clean interface definition with well-organized method signatures for User and Token operations. Good separation of concerns.
internal/auth/auth.go (5)
259-307: LGTM on OIDC token exchange implementation.Good security practices: nonce verification, fallback to UserInfo endpoint for missing email, and proper error handling. The use of
echo.NewHTTPErrorfor auth failures is appropriate.
346-373: LGTM on Middleware implementation.Clean authentication flow: API token first, then session, with appropriate error handling. Good use of context values for passing user/session to handlers.
422-437: LGTM on SaveSession implementation.Clean session creation with proper error handling and logging.
439-453: No action needed. Thesess.Destroy()call properly clears the session cookie. The zerodha simplesessions v3 library automatically handles cookie expiration (setting the cookie's MaxAge to the past) whenDestroy()is called, and the cookie clearing is successfully written to the response because the SetCookieHooks are properly registered (lines 165-179).Likely an incorrect or invalid review comment.
394-406: Reconsider the error handling - it may be necessary despite postgres store design.The
sessStore.String()method fromgithub.com/zerodha/simplesessions/stores/postgres/v3can involve database operations internally, not just type conversion as suggested. According to the library's design, the postgres store should mapsql.ErrNoRowsto simplesessions'ErrNil, but if the store implementation leakssql.ErrNoRowsdirectly, the error check at lines 398-402 becomes valid and necessary to handle this case defensively. Rather than unreachable code, this may be protecting against a store implementation issue or an intentional fallback. Verify whether the current postgres store version (v3.0.0) properly mapssql.ErrNoRowstoErrNilbefore removing this check.
| server, err := api.NewServer(core) | ||
| log.Info().Msg("Starting server... at " + cfg.Server.Address) | ||
| if err != nil { | ||
| log.Fatal().Err(err).Msg("Failed to create server") | ||
| } |
There was a problem hiding this comment.
Log message emitted before error check.
The log message "Starting server..." is printed before checking if NewServer returned an error. If server creation fails, the log shows a misleading "Starting server" message before the fatal error.
🐛 Proposed fix
// Setup and start HTTP server
server, err := api.NewServer(core)
- log.Info().Msg("Starting server... at " + cfg.Server.Address)
if err != nil {
log.Fatal().Err(err).Msg("Failed to create server")
}
+ log.Info().Msg("Starting server... at " + cfg.Server.Address)
go func() {📝 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.
| server, err := api.NewServer(core) | |
| log.Info().Msg("Starting server... at " + cfg.Server.Address) | |
| if err != nil { | |
| log.Fatal().Err(err).Msg("Failed to create server") | |
| } | |
| server, err := api.NewServer(core) | |
| if err != nil { | |
| log.Fatal().Err(err).Msg("Failed to create server") | |
| } | |
| log.Info().Msg("Starting server... at " + cfg.Server.Address) |
🤖 Prompt for AI Agents
In @cmd/main.go around lines 120 - 124, The code logs "Starting server... at " +
cfg.Server.Address before checking the result of api.NewServer(core), which can
emit a misleading message if server creation fails; update the flow so you first
call server, err := api.NewServer(core), then immediately check if err != nil
and call log.Fatal().Err(err).Msg("Failed to create server"), and only after
that successful check call log.Info().Msg("Starting server... at " +
cfg.Server.Address) (i.e., move the log.Info() to after the nil error check or
log success inside the success branch).
| func getPendingMigrations(db *sqlx.DB) (string, []migFunc, error) { | ||
| lastVer, err := getLastMigrationVersion(db) | ||
| if err != nil { | ||
| return "", nil, err | ||
| } | ||
|
|
||
| var toRun []migFunc | ||
| for i, m := range migList { | ||
| if semver.Compare(m.version, lastVer) > 0 { | ||
| toRun = migList[i:] | ||
| break | ||
| } | ||
| } | ||
|
|
||
| return lastVer, toRun, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the semver usage in the codebase
rg -n "semver\." --type goRepository: inbox451/explorer451
Length of output: 127
🏁 Script executed:
# Find the migList definition and understand its structure
rg -n "migList" --type go -B 2 -A 2Repository: inbox451/explorer451
Length of output: 477
🏁 Script executed:
# Check the context around the function and line 111 mentioned in the comment
cd cmd && sed -n '1,150p' upgrade.go | cat -nRepository: inbox451/explorer451
Length of output: 4124
🏁 Script executed:
# Look for any sorting of migList or version comparison logic
rg -n "sort\|Sort" cmd/upgrade.go -B 2 -A 2Repository: inbox451/explorer451
Length of output: 46
🏁 Script executed:
# Search for test files related to migrations
fd -e "test.go" -e "_test.go" | head -20Repository: inbox451/explorer451
Length of output: 46
🏁 Script executed:
# Look for any comments or docs about migration ordering
rg -n "migList|migration.*order|migration.*sort" --type go -B 2 -A 2Repository: inbox451/explorer451
Length of output: 477
🏁 Script executed:
# Check if there's a go.mod to understand semver package version
cat go.mod | grep semverRepository: inbox451/explorer451
Length of output: 46
🏁 Script executed:
# Verify semver.Compare behavior with specific test cases
python3 << 'EOF'
# Simulate lexicographic vs semantic comparison for version strings
versions = ["v0.1.0", "v0.9.0", "v0.10.0", "v1.0.0", "v0.99.0"]
print("Lexicographic (string) ordering:")
sorted_lex = sorted(versions)
for v in sorted_lex:
print(f" {v}")
print("\nLexicographic DESC ordering (SQL ORDER BY DESC):")
sorted_lex_desc = sorted(versions, reverse=True)
for v in sorted_lex_desc:
print(f" {v}")
# Semantic version comparison simulation
def semver_cmp(v1, v2):
# Remove 'v' prefix and split
parts1 = [int(x) for x in v1[1:].split('.')]
parts2 = [int(x) for x in v2[1:].split('.')]
if parts1 < parts2: return -1
elif parts1 > parts2: return 1
else: return 0
print("\nSemantic ordering:")
sorted_sem = sorted(versions, key=lambda x: [int(p) for p in x[1:].split('.')])
for v in sorted_sem:
print(f" {v}")
# Check mismatch between v0.9.0 and v0.10.0
print("\nMismatch check for v0.9.0 vs v0.10.0:")
print(f" Lexicographic: 'v0.9.0' > 'v0.10.0' = {'v0.9.0' > 'v0.10.0'}")
print(f" Semantic: [0,9,0] < [0,10,0] = {[0,9,0] < [0,10,0]}")
EOFRepository: inbox451/explorer451
Length of output: 429
Potential bug: getPendingMigrations assumes migList is sorted and doesn't account for lexicographic vs semantic version ordering inconsistency.
The function breaks on the first migration with version greater than lastVer and takes all remaining migrations from that index. This assumes migList is strictly sorted in ascending order by version. If migrations are added out of order, some could be skipped or re-run.
Additionally, there is an ordering mismatch: getLastMigrationVersion retrieves the last version using SQL's lexicographic string comparison (ORDER BY version DESC), while getPendingMigrations uses semver.Compare for semantic comparison. These orderings diverge for versions like v0.9.0 (lexicographically > v0.10.0) vs v0.10.0 (semantically < v0.9.0).
Both issues are currently latent since migList contains only one migration, but they will cause failures as more migrations are added. Consider enforcing sort order or storing versions as structured data rather than strings.
| // getLastMigrationVersion returns the last migration semver recorded in the DB. | ||
| // If there isn't any, `v0.0.0` is returned. | ||
| func getLastMigrationVersion(db *sqlx.DB) (string, error) { | ||
| var v string | ||
| if err := db.Get(&v, ` | ||
| SELECT COALESCE( | ||
| (SELECT version FROM schema_migrations ORDER BY version DESC LIMIT 1), | ||
| 'v0.0.0')`); err != nil { | ||
| if isTableNotExistErr(err) { | ||
| return "v0.0.0", nil | ||
| } | ||
| return v, err | ||
| } | ||
| return v, nil | ||
| } |
There was a problem hiding this comment.
SQL ORDER BY version DESC uses lexicographic ordering, not semantic versioning.
The query ORDER BY version DESC sorts versions lexicographically, not semantically. This means v0.9.0 would sort after v0.10.0 (because '9' > '1' character-wise), potentially returning an incorrect "last" version.
💡 Suggested approaches
Option 1: Store a sortable numeric representation alongside the version string.
Option 2: Fetch all versions and sort in Go using semver.Compare:
func getLastMigrationVersion(db *sqlx.DB) (string, error) {
- var v string
- if err := db.Get(&v, `
- SELECT COALESCE(
- (SELECT version FROM schema_migrations ORDER BY version DESC LIMIT 1),
- 'v0.0.0')`); err != nil {
+ var versions []string
+ if err := db.Select(&versions, `SELECT version FROM schema_migrations`); err != nil {
if isTableNotExistErr(err) {
return "v0.0.0", nil
}
- return v, err
+ return "", err
}
- return v, nil
+ if len(versions) == 0 {
+ return "v0.0.0", nil
+ }
+ semver.Sort(versions)
+ return versions[len(versions)-1], nil
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @cmd/upgrade.go around lines 105 - 119, getLastMigrationVersion currently
relies on SQL ORDER BY version DESC which sorts lexicographically and can return
an incorrect latest semantic version; change getLastMigrationVersion to SELECT
all version strings from schema_migrations, scan them into a slice, and then
determine the newest version in Go using a semantic version comparator (e.g.,
semver.Compare or equivalent) before returning it; ensure you still return
"v0.0.0" when no rows or when isTableNotExistErr(err) is encountered and
propagate other errors unchanged.
| go 1.24.4 | ||
|
|
||
| require ( | ||
| github.com/aarondl/null/v9 v9.0.1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat go.modRepository: inbox451/explorer451
Length of output: 3044
🌐 Web query:
latest version golang.org/x/crypto security advisories 2025 2026
💡 Result:
Summary of golang.org/x/crypto security advisories (2025–2026)
- CVE-2025-22869 — DoS in golang.org/x/crypto/ssh; fixed in v0.35.0 (announcement Feb 24, 2025; GH advisory published Apr 12, 2025). [1][2]
- GO-2025-4116 / CVE-2025-47913 — SSH agent panic/DoS; fixed in v0.43.0 (published Nov 13, 2025). [3]
- CVE-2025-47914 — SSH agent message-size handling (DoS/panic); fixes referenced in late 2025 (patches noted at v0.45.0 in advisory trackers). [4]
No additional x/crypto security advisories dated in 2026 were found in public trackers as of Jan 7, 2026.
Sources:
[1] golang-announce (security notice for v0.35.0).
[2] GitHub Advisory GHSA-hcg3-q754-cr77 (CVE-2025-22869).
[3] OSV / Go Packages report GO-2025-4116 (CVE-2025-47913).
[4] Vulnerability trackers (CVE-2025-47914 / v0.45.0 tracking).
🌐 Web query:
github.com/coreos/go-oidc v3 latest version security
💡 Result:
Latest (v3) release: v3.17.0 (released Nov 21, 2025). [1]
Public security advisories/CVEs specifically for github.com/coreos/go-oidc (v3) — none found in public sources during this check. I recommend upgrading to the latest v3 release and monitoring the repo Security/Advisories for any new issues. [1][2]
Sources:
- coreos/go-oidc (Releases page showing v3.17.0). [1]
- Package listing / registry view of github.com/coreos/go-oidc/v3 (version history). [2]
🌐 Web query:
github.com/lib/pq latest version security vulnerabilities
💡 Result:
Short answer: The github.com/lib/pq repository itself has no published security advisories on its GitHub Security page. [1][2]
Context and caveat:
- Several CVEs exist for PostgreSQL's native libpq (the C client library) — e.g. CVE-2021-23222 and the more recent CVE‑2025‑1094 — but those refer to PostgreSQL's libpq implementation, not the Go driver github.com/lib/pq. You should review whether your code uses the C libpq or the Go driver and test/patch accordingly. [3][4]
Sources:
- lib/pq GitHub security page (no advisories). [1]
- lib/pq repository. [2]
- PostgreSQL security advisory CVE-2021-23222. [3]
- PostgreSQL advisory (CVE‑2025‑1094 / libpq quoting APIs). [4]
🌐 Web query:
golang.org/x/oauth2 latest version 2025 2026
💡 Result:
Latest published tag: v0.32.0. [1][2]
Sources:
- go.googlesource.com/oauth2 (tags list showing v0.32.0). [1]
- pkg.go.dev golang.org/x/oauth2 (module documentation). [2]
🌐 Web query:
zerodha/simplesessions latest version
💡 Result:
The latest released module version is v3.0.0 (published Jun 1, 2024). [1][2]
Install: go get -u github.com/zerodha/simplesessions/v3. [1]
Sources: [pkg.go.dev listing for github.com/zerodha/simplesessions/v3][1], package registry index. [2]
Update golang.org/x/crypto to address critical security vulnerabilities.
The pinned version golang.org/x/crypto v0.39.0 contains multiple known CVEs affecting SSH functionality:
- CVE-2025-22869 (DoS in SSH) — fixed in v0.35.0
- CVE-2025-47913 (SSH agent panic/DoS) — fixed in v0.43.0
- CVE-2025-47914 (SSH agent message-size handling) — fixed in v0.45.0
Upgrade to at least v0.43.0, preferably v0.45.0 or later.
Additionally, consider updating these dependencies to their latest stable versions:
github.com/coreos/go-oidc/v3fromv3.15.0tov3.17.0(released Nov 2025)golang.org/x/oauth2fromv0.31.0tov0.32.0
| // Initialize repository | ||
| core.Repository = storage.NewRepository(db) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check NewRepository function and nil handling in storage package
rg -n -A 15 'func NewRepository' internal/storage/
rg -n 'if.*db.*nil|db == nil|nil.*db' internal/storage/ -B 2 -A 2Repository: inbox451/explorer451
Length of output: 1021
🏁 Script executed:
# Check how NewRepository is called in core.go and if db is validated
rg -n -B 10 -A 5 'NewRepository' internal/core/core.go
rg -n -B 15 'NewRepository' internal/core/core.go | head -40Repository: inbox451/explorer451
Length of output: 780
🏁 Script executed:
# Search for calls to NewCore to see if db is validated before passing
rg -n 'NewCore' internal/core/ cmd/ main.go 2>/dev/null | head -20
rg -n -B 5 'NewCore(' -A 2Repository: inbox451/explorer451
Length of output: 347
🏁 Script executed:
# Check how db is initialized in main.go before NewCore call
sed -n '100,130p' cmd/main.goRepository: inbox451/explorer451
Length of output: 793
🏁 Script executed:
# Check if repository methods are guarded at API level
rg -n 'core\.Repository\.' cmd/ internal/api/ | head -10
rg -n 'if.*Repository.*nil' internal/Repository: inbox451/explorer451
Length of output: 133
Database nil-safety: add validation to prevent runtime panics.
The database parameter can be nil (as shown in main.go where if db != nil guards schema checks), but storage.NewRepository(db) accepts nil without validation, and repository methods like GetUser() directly call r.db.GetContext() without nil guards. If a nil database is passed, any API handler calling repository methods will panic.
Either:
- Validate that
dbis non-nil before callingstorage.NewRepository(), or - Add nil checks in repository methods to return an appropriate error, or
- Add API-level guards that prevent repository method calls when database is unavailable.
🤖 Prompt for AI Agents
In @internal/core/core.go around lines 39 - 40, The repository is being created
with a potentially nil DB which leads to panics when repository methods call
r.db (e.g., GetUser calls r.db.GetContext()). Fix by validating the DB before
creating core.Repository (check db != nil before calling storage.NewRepository
in core.go) and also harden repository methods (in the repository implementation
referenced by storage.NewRepository, add a nil check like if r.db == nil return
an explicit error from methods such as GetUser) so callers get a proper error
instead of a panic.
| // PutObject uploads an object to S3 | ||
| func (s *S3Service) PutObject(ctx context.Context, bucket, key string, body io.Reader, contentType string) error { | ||
| s.core.Logger.Debug(). | ||
| Str("bucket", bucket). | ||
| Str("key", key). | ||
| Str("contentType", contentType). | ||
| Msg("Putting object") | ||
|
|
||
| // Read the entire body into memory to make it seekable | ||
| bodyBytes, err := io.ReadAll(body) | ||
| if err != nil { | ||
| s.core.Logger.Error(). | ||
| Err(err). | ||
| Str("bucket", bucket). | ||
| Str("key", key). | ||
| Msg("Failed to read request body") | ||
| return err | ||
| } | ||
|
|
||
| input := &s3.PutObjectInput{ | ||
| Bucket: aws.String(bucket), | ||
| Key: aws.String(key), | ||
| Body: bytes.NewReader(bodyBytes), // Use a seekable reader without extra memory allocation | ||
| ContentType: aws.String(contentType), | ||
| } | ||
|
|
||
| _, err = s.core.S3Client.PutObject(ctx, input) | ||
| if err != nil { | ||
| s.core.Logger.Error(). | ||
| Err(err). | ||
| Str("bucket", bucket). | ||
| Str("key", key). | ||
| Msg("Failed to put object") | ||
| return err | ||
| } | ||
|
|
||
| s.core.Logger.Info(). | ||
| Str("bucket", bucket). | ||
| Str("key", key). | ||
| Msg("Successfully uploaded object") | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Loading entire file into memory can cause issues with large uploads.
The implementation reads the entire request body into memory using io.ReadAll (lines 215-224) to create a seekable reader. However, this approach has significant drawbacks:
- Memory exhaustion: Large file uploads (GB+) will consume equivalent memory, potentially causing OOM errors
- Scalability concern: Concurrent uploads exacerbate memory pressure
- Misleading comment: Line 229's comment states "without extra memory allocation," but
io.ReadAllalready allocated memory for the entire file
The AWS SDK v2 supports streaming uploads directly from io.Reader without requiring seekability for most cases. Consider whether seekability is truly required for your use case.
♻️ Suggested streaming approach (if seekability is not required)
func (s *S3Service) PutObject(ctx context.Context, bucket, key string, body io.Reader, contentType string) error {
s.core.Logger.Debug().
Str("bucket", bucket).
Str("key", key).
Str("contentType", contentType).
Msg("Putting object")
- // Read the entire body into memory to make it seekable
- bodyBytes, err := io.ReadAll(body)
- if err != nil {
- s.core.Logger.Error().
- Err(err).
- Str("bucket", bucket).
- Str("key", key).
- Msg("Failed to read request body")
- return err
- }
-
input := &s3.PutObjectInput{
Bucket: aws.String(bucket),
Key: aws.String(key),
- Body: bytes.NewReader(bodyBytes), // Use a seekable reader without extra memory allocation
+ Body: body,
ContentType: aws.String(contentType),
}
_, err = s.core.S3Client.PutObject(ctx, input)If seekability is required (e.g., for retries with certain AWS SDK configurations), consider implementing a size limit or using multipart uploads for large files.
📝 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.
| // PutObject uploads an object to S3 | |
| func (s *S3Service) PutObject(ctx context.Context, bucket, key string, body io.Reader, contentType string) error { | |
| s.core.Logger.Debug(). | |
| Str("bucket", bucket). | |
| Str("key", key). | |
| Str("contentType", contentType). | |
| Msg("Putting object") | |
| // Read the entire body into memory to make it seekable | |
| bodyBytes, err := io.ReadAll(body) | |
| if err != nil { | |
| s.core.Logger.Error(). | |
| Err(err). | |
| Str("bucket", bucket). | |
| Str("key", key). | |
| Msg("Failed to read request body") | |
| return err | |
| } | |
| input := &s3.PutObjectInput{ | |
| Bucket: aws.String(bucket), | |
| Key: aws.String(key), | |
| Body: bytes.NewReader(bodyBytes), // Use a seekable reader without extra memory allocation | |
| ContentType: aws.String(contentType), | |
| } | |
| _, err = s.core.S3Client.PutObject(ctx, input) | |
| if err != nil { | |
| s.core.Logger.Error(). | |
| Err(err). | |
| Str("bucket", bucket). | |
| Str("key", key). | |
| Msg("Failed to put object") | |
| return err | |
| } | |
| s.core.Logger.Info(). | |
| Str("bucket", bucket). | |
| Str("key", key). | |
| Msg("Successfully uploaded object") | |
| return nil | |
| } | |
| // PutObject uploads an object to S3 | |
| func (s *S3Service) PutObject(ctx context.Context, bucket, key string, body io.Reader, contentType string) error { | |
| s.core.Logger.Debug(). | |
| Str("bucket", bucket). | |
| Str("key", key). | |
| Str("contentType", contentType). | |
| Msg("Putting object") | |
| input := &s3.PutObjectInput{ | |
| Bucket: aws.String(bucket), | |
| Key: aws.String(key), | |
| Body: body, | |
| ContentType: aws.String(contentType), | |
| } | |
| _, err = s.core.S3Client.PutObject(ctx, input) | |
| if err != nil { | |
| s.core.Logger.Error(). | |
| Err(err). | |
| Str("bucket", bucket). | |
| Str("key", key). | |
| Msg("Failed to put object") | |
| return err | |
| } | |
| s.core.Logger.Info(). | |
| Str("bucket", bucket). | |
| Str("key", key). | |
| Msg("Successfully uploaded object") | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In @internal/core/s3_service.go around lines 207 - 249, The PutObject
implementation currently reads the entire body into memory; update
S3Service.PutObject to stream the provided io.Reader directly into the
s3.PutObjectInput (set Body to the original body instead of
bytes.NewReader(bodyBytes)) and remove the io.ReadAll logic and the misleading
"without extra memory allocation" comment; if seekability is required for
retries, replace that approach with either an io.ReadSeeker backed by a temp
file or use multipart upload for large files (handle/limit sizes accordingly)
and keep existing error logging around s.core.S3Client.PutObject and the
surrounding Logger calls.
| type Session struct { | ||
| Base | ||
| SessionID string `db:"session_id" json:"session_id"` | ||
| UserID string `db:"user_id" json:"user_id"` | ||
| Data json.RawMessage `db:"data" json:"data"` | ||
| LastAccessedAt null.Time `db:"last_accessed_at" json:"last_accessed_at"` | ||
| IPAddress string `db:"ip_address" json:"ip_address"` | ||
| UserAgent string `db:"user_agent" json:"user_agent"` | ||
| IsActive bool `db:"is_active" json:"is_active"` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Session struct is actually used anywhere in the codebase
rg -n "models\.Session" --type goRepository: inbox451/explorer451
Length of output: 46
🏁 Script executed:
# Find migration files
find . -type f -name "*migration*" -o -name "*v0.1.0*" | head -20Repository: inbox451/explorer451
Length of output: 95
🏁 Script executed:
# Look for migration files in common locations
find . -type f -name "*.sql" -o -name "*.go" | grep -i migr | head -20Repository: inbox451/explorer451
Length of output: 95
🏁 Script executed:
# Read the migration file
cat -n ./internal/migrations/v0.1.0.goRepository: inbox451/explorer451
Length of output: 3171
🏁 Script executed:
# Search for Session struct usage without the "models." prefix
rg -n "Session" --type go | grep -v "^Binary"Repository: inbox451/explorer451
Length of output: 2458
🏁 Script executed:
# Look at the auth_models.go file in full context
cat -n internal/models/auth_models.goRepository: inbox451/explorer451
Length of output: 3389
Session struct is unused and would fail at runtime due to schema mismatch.
The Session struct defines fields (SessionID, UserID, LastAccessedAt, IPAddress, UserAgent, IsActive) that do not exist in the actual sessions table. The migration v0.1.0 creates a simpler schema with only id, data, and created_at columns. The struct is not used anywhere in the codebase—only simplesessions.Session from an external library is used. If this struct is ever used with the current schema, it will fail at runtime due to missing columns.
🤖 Prompt for AI Agents
In @internal/models/auth_models.go around lines 72 - 81, The Session struct
defined in internal/models/auth_models.go (type Session) mismatches the v0.1.0
sessions table and is unused; remove this struct to avoid confusion and runtime
failures, and ensure any session handling uses the external
simplesessions.Session type instead (search for Session, SessionID, UserID
references and replace or update usages to use simplesessions.Session or adjust
the DB schema if you intentionally need extra fields).
| ## Authentication | ||
|
|
||
| The API supports optional authentication. If authentication is enabled, you'll need to include authentication headers or use the auth endpoints first. |
There was a problem hiding this comment.
Authentication should be mandatory, not optional.
Line 47 states "optional authentication," but the PR objectives explicitly document that there is a "decision not to support unauthenticated access." This is contradictory. If authentication is required, the documentation must reflect that all API requests require one of the three authentication methods. Update the text to remove "optional" and clarify that authentication is mandatory.
🤖 Prompt for AI Agents
In @README.md around lines 45 - 47, The README currently says authentication is
optional but the PR decision mandates no unauthenticated access; update the
Authentication section text to state that authentication is required for all API
requests and list the three supported authentication methods (refer to the
documented methods elsewhere in the repo) — remove the word "optional", replace
phrasing like "If authentication is enabled" with explicit language such as
"Authentication is required" and instruct callers to include one of the three
supported authentication headers or use the auth endpoints before making any API
requests.
| **Login Response Example:** | ||
| ```json | ||
| { | ||
| "buckets":[ | ||
| "nb-bucket-eu-central-1" | ||
| ] | ||
| "user": { | ||
| "id": "123...", | ||
| "username": "john", | ||
| "name": "John Doe", | ||
| "email": "user@example.com", | ||
| "role": "user" | ||
| }, | ||
| "token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9...", | ||
| "expires": "2023-12-31T23:59:59Z" | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Replace example tokens with clearly marked placeholders.
The documentation includes example tokens and API keys (lines 72, 143-144, 154-155) that, while not real secrets, violate security best practices. Instead of including token-like strings, use clearly marked placeholders such as YOUR-TOKEN-HERE, <your-bearer-token>, or similar conventions. This prevents confusion and reduces the risk of accidental exposure if the documentation is later modified or referenced elsewhere.
🔐 Example placeholder replacements
For lines 62-75 (login response), replace the example JWT with:
- "token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9...",
+ "token": "<bearer-token-placeholder>",For lines 143-144, 154-155, and other curl examples, use:
- -H "Authorization: Bearer your-token"
+ -H "Authorization: Bearer <your-bearer-token>"Also applies to: 140-156
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 72-72: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In @README.md around lines 62 - 75, In the "Login Response Example" JSON and all
curl examples that show token/API key strings, replace any token-like literals
with clearly marked placeholders (e.g., change the "token" value to
"<YOUR-BEARER-TOKEN>" or "YOUR-TOKEN-HERE" and any API key strings to
"<YOUR-API-KEY>"); update every occurrence referenced (the "Login Response
Example" block, any "Authorization: Bearer ..." header examples, and shown API
key fields) so examples no longer contain token-like values but use consistent
placeholder text instead.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/main.go (2)
60-90: Consider refactoring the database initialization logic for clarity.The nested conditionals and error handling make this section challenging to follow. Additionally, there are minor inconsistencies:
- Lines 69 and 81 end error messages with periods, while other fatal messages (lines 50, 90) do not.
- Lines 82-83 correctly close the connection after a ping failure, but the placement inside the else-branch obscures the intent.
♻️ Optional refactoring for improved readability
Consider extracting database initialization into a helper function:
func initDatabase(cfg *config.Config, log *logger.Logger, requireDB bool) *sqlx.DB { if cfg.Database.URL == "" { if requireDB { log.Fatal().Msg("Database URL is required for install/upgrade commands") } return nil } log.Info().Msg("Initializing database connection") db, err := sqlx.Connect("postgres", cfg.Database.URL) if err != nil { log.Fatal().Err(err).Msg("Failed to connect to database") } // Configure pool db.SetMaxOpenConns(cfg.Database.MaxOpenConns) db.SetMaxIdleConns(cfg.Database.MaxIdleConns) db.SetConnMaxLifetime(cfg.Database.ConnMaxLifetime) // Verify connection if err := db.Ping(); err != nil { db.Close() log.Fatal().Err(err).Msg("Database ping failed") } log.Info().Msg("Database connection established") return db }Then in main:
requireDB := *installFlag || *upgradeFlag db := initDatabase(cfg, log, requireDB)This eliminates nested conditionals and standardizes error messages.
92-108: Redundant nil checks for database connection.The nil checks at lines 94-96 and 103-105 are redundant because lines 88-90 already ensure that
dbis non-nil wheninstallFlagorupgradeFlagis set. While defensive programming can be valuable, these checks represent unreachable code paths.♻️ Simplify by removing redundant checks
// Handle install command if *installFlag { - if db == nil { - log.Fatal().Msg("Database connection is required for install") - } install(db, cfg, !*yesFlag, *idempotentFlag) os.Exit(0) } // Handle upgrade command if *upgradeFlag { - if db == nil { - log.Fatal().Msg("Database connection is required for upgrade") - } upgrade(db, cfg, !*yesFlag) os.Exit(0) }go.mod (1)
6-26: Consider updatinggo-playground/validator/v10to v10.28.0+ for transitive dependency improvements.Most dependencies are at acceptable versions. However,
go-playground/validator/v10v10.27.0 has newer releases (v10.28.0+) that address transitive dependency concerns withgolang.org/x/crypto. The other dependencies—lib/pqv1.10.9 (latest),sqlxv1.4.0,pflagv1.0.10,uuidv1.6.0, andsimplesessionsv3.0.0—show no known security advisories. Rungo mod tidyand considergovulncheckto catch any transitive vulnerabilities in your final build.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
cmd/install.gocmd/main.gogo.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/install.go
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/main.go (2)
internal/core/core.go (1)
NewCore(24-46)internal/api/server.go (2)
NewServer(36-78)Server(29-33)
🔇 Additional comments (6)
cmd/main.go (5)
16-20: LGTM: Imports align with new database functionality.The imports correctly include sqlx for database operations, lib/pq as a side-effect import for the PostgreSQL driver, and pflag for command-line argument handling.
22-31: LGTM: Command-line flags are well-defined.The flag declarations are clear and follow Go conventions. The install/upgrade workflow with confirmation and idempotency options is a good practice for database operations.
117-117: LGTM: Core initialization updated to accept database handle.The signature change correctly passes the database connection to
core.NewCore, enabling the core service to initialize the repository layer when a database is available.
125-144: LGTM: Server lifecycle and graceful shutdown.The server startup in a goroutine and graceful shutdown with a 10-second timeout follow best practices for HTTP server lifecycle management.
110-114: Implementation ofcheckInstallandcheckUpgradeis sound.Both functions properly validate the database schema without performing destructive operations:
checkInstallverifies schema exists with clear error messaging and gracefully directs users to run--installif neededcheckUpgradechecks for pending migrations, returns early if none exist, and prevents auto-migrations by fataling with instructions to manually run--upgradego.mod (1)
27-27: Update golang.org/x/crypto to address critical security vulnerabilities.The pinned version
golang.org/x/crypto v0.39.0contains multiple known CVEs affecting SSH functionality. As noted in the previous review, this should be upgraded to at least v0.43.0, preferably v0.45.0 or later.Likely an incorrect or invalid review comment.
1 participant
Status
Work in Progress — opening this draft early to gather feedback.
Summary
This pull request introduces integration with three authentication mechanisms:
The goal is to ensure that all access to S3 resources is always authenticated — we explicitly do not plan to support unauthenticated access.
Next Steps / Open Questions
/cc @Jalmeida1994
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.