From e6bd4c122e08d4da6e063313999e8d3137b07792 Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Sat, 3 Jan 2026 17:26:25 -0600 Subject: [PATCH] fix sql migration bug. add better error logs for auth failures. fix showing incorrect pull commands with helm charts --- pkg/appview/db/models.go | 3 +- pkg/appview/db/queries.go | 10 +-- pkg/appview/db/schema.go | 68 +++++++++++++++++++-- pkg/appview/handlers/repository.go | 9 ++- pkg/appview/storage/manifest_store.go | 6 ++ pkg/appview/templates/pages/repository.html | 2 +- pkg/auth/session.go | 28 +++++++-- pkg/auth/token/handler.go | 17 +++++- 8 files changed, 121 insertions(+), 22 deletions(-) diff --git a/pkg/appview/db/models.go b/pkg/appview/db/models.go index 7038561..6e656bf 100644 --- a/pkg/appview/db/models.go +++ b/pkg/appview/db/models.go @@ -154,7 +154,8 @@ type TagWithPlatforms struct { Tag Platforms []PlatformInfo IsMultiArch bool - HasAttestations bool // true if manifest list contains attestation references + HasAttestations bool // true if manifest list contains attestation references + ArtifactType string // container-image, helm-chart, unknown } // ManifestWithMetadata extends Manifest with tags and platform information diff --git a/pkg/appview/db/queries.go b/pkg/appview/db/queries.go index 8d34430..0aba278 100644 --- a/pkg/appview/db/queries.go +++ b/pkg/appview/db/queries.go @@ -653,6 +653,7 @@ func GetTagsWithPlatforms(db *sql.DB, did, repository string) ([]TagWithPlatform t.digest, t.created_at, m.media_type, + m.artifact_type, COALESCE(mr.platform_os, '') as platform_os, COALESCE(mr.platform_architecture, '') as platform_architecture, COALESCE(mr.platform_variant, '') as platform_variant, @@ -676,11 +677,11 @@ func GetTagsWithPlatforms(db *sql.DB, did, repository string) ([]TagWithPlatform for rows.Next() { var t Tag - var mediaType, platformOS, platformArch, platformVariant, platformOSVersion string + var mediaType, artifactType, platformOS, platformArch, platformVariant, platformOSVersion string var isAttestation bool if err := rows.Scan(&t.ID, &t.DID, &t.Repository, &t.Tag, &t.Digest, &t.CreatedAt, - &mediaType, &platformOS, &platformArch, &platformVariant, &platformOSVersion, &isAttestation); err != nil { + &mediaType, &artifactType, &platformOS, &platformArch, &platformVariant, &platformOSVersion, &isAttestation); err != nil { return nil, err } @@ -688,8 +689,9 @@ func GetTagsWithPlatforms(db *sql.DB, did, repository string) ([]TagWithPlatform tagKey := t.Tag if _, exists := tagMap[tagKey]; !exists { tagMap[tagKey] = &TagWithPlatforms{ - Tag: t, - Platforms: []PlatformInfo{}, + Tag: t, + Platforms: []PlatformInfo{}, + ArtifactType: artifactType, } tagOrder = append(tagOrder, tagKey) } diff --git a/pkg/appview/db/schema.go b/pkg/appview/db/schema.go index 5de2d79..b08677b 100644 --- a/pkg/appview/db/schema.go +++ b/pkg/appview/db/schema.go @@ -37,14 +37,27 @@ func InitDB(path string, skipMigrations bool) (*sql.DB, error) { return nil, err } - // Create schema from embedded SQL file - if _, err := db.Exec(schemaSQL); err != nil { - return nil, err + // Check if this is an existing database with migrations applied + isExisting, err := hasAppliedMigrations(db) + if err != nil { + return nil, fmt.Errorf("failed to check database state: %w", err) + } + + if isExisting { + // Existing database: skip schema.sql, only run pending migrations + slog.Debug("Existing database detected, skipping schema.sql") + } else { + // Fresh database: apply schema.sql + slog.Info("Fresh database detected, applying schema") + if err := applySchema(db); err != nil { + return nil, err + } } // Run migrations unless skipped + // For fresh databases, migrations are recorded but not executed (schema.sql is already complete) if !skipMigrations { - if err := runMigrations(db); err != nil { + if err := runMigrations(db, !isExisting); err != nil { return nil, err } } @@ -52,6 +65,39 @@ func InitDB(path string, skipMigrations bool) (*sql.DB, error) { return db, nil } +// hasAppliedMigrations checks if this is an existing database with migrations applied +func hasAppliedMigrations(db *sql.DB) (bool, error) { + // Check if schema_migrations table exists + var count int + err := db.QueryRow(` + SELECT COUNT(*) FROM sqlite_master + WHERE type='table' AND name='schema_migrations' + `).Scan(&count) + if err != nil { + return false, err + } + if count == 0 { + return false, nil // No migrations table = fresh DB + } + + // Table exists, check if it has entries + err = db.QueryRow("SELECT COUNT(*) FROM schema_migrations").Scan(&count) + if err != nil { + return false, err + } + return count > 0, nil +} + +// applySchema executes schema.sql for fresh databases +func applySchema(db *sql.DB) error { + for _, stmt := range splitSQLStatements(schemaSQL) { + if _, err := db.Exec(stmt); err != nil { + return fmt.Errorf("failed to apply schema: %w", err) + } + } + return nil +} + // Migration represents a database migration type Migration struct { Version int @@ -61,7 +107,8 @@ type Migration struct { } // runMigrations applies any pending database migrations -func runMigrations(db *sql.DB) error { +// If freshDB is true, migrations are recorded but not executed (schema.sql already includes their changes) +func runMigrations(db *sql.DB, freshDB bool) error { // Load migrations from files migrations, err := loadMigrations() if err != nil { @@ -86,7 +133,16 @@ func runMigrations(db *sql.DB) error { continue } - // Apply migration in a transaction + if freshDB { + // Fresh database: schema.sql already has everything, just record the migration + slog.Debug("Recording migration as applied (fresh DB)", "version", m.Version, "name", m.Name) + if _, err := db.Exec("INSERT INTO schema_migrations (version) VALUES (?)", m.Version); err != nil { + return fmt.Errorf("failed to record migration %d: %w", m.Version, err) + } + continue + } + + // Existing database: apply migration in a transaction slog.Info("Applying migration", "version", m.Version, "name", m.Name, "description", m.Description) tx, err := db.Begin() diff --git a/pkg/appview/handlers/repository.go b/pkg/appview/handlers/repository.go index 384db32..bfb417c 100644 --- a/pkg/appview/handlers/repository.go +++ b/pkg/appview/handlers/repository.go @@ -231,10 +231,13 @@ func (h *RepositoryPageHandler) ServeHTTP(w http.ResponseWriter, r *http.Request } } - // Determine dominant artifact type from manifests + // Determine artifact type for header section from first tag + // This is used for the "Pull this image/chart" header command artifactType := "container-image" - if len(manifests) > 0 { - // Use the most recent manifest's artifact type + if len(tagsWithPlatforms) > 0 { + artifactType = tagsWithPlatforms[0].ArtifactType + } else if len(manifests) > 0 { + // Fallback to manifests if no tags artifactType = manifests[0].ArtifactType } diff --git a/pkg/appview/storage/manifest_store.go b/pkg/appview/storage/manifest_store.go index f832cae..a162821 100644 --- a/pkg/appview/storage/manifest_store.go +++ b/pkg/appview/storage/manifest_store.go @@ -124,6 +124,12 @@ func (s *ManifestStore) Put(ctx context.Context, manifest distribution.Manifest, return "", fmt.Errorf("failed to create manifest record: %w", err) } + // OCI spec allows omitting mediaType from the manifest body (inferred from Content-Type header) + // Helm charts typically omit it, so use the media type from the request if body is empty + if manifestRecord.MediaType == "" && mediaType != "" { + manifestRecord.MediaType = mediaType + } + // Set the blob reference, hold DID, and hold endpoint manifestRecord.ManifestBlob = blobRef manifestRecord.HoldDID = s.ctx.HoldDID // Primary reference (DID) diff --git a/pkg/appview/templates/pages/repository.html b/pkg/appview/templates/pages/repository.html index 0130769..69a4307 100644 --- a/pkg/appview/templates/pages/repository.html +++ b/pkg/appview/templates/pages/repository.html @@ -183,7 +183,7 @@ {{ end }} - {{ if eq $.ArtifactType "helm-chart" }} + {{ if eq .ArtifactType "helm-chart" }} {{ template "docker-command" (print "helm pull oci://" $.RegistryURL "/" $.Owner.Handle "/" $.Repository.Name " --version " .Tag.Tag) }} {{ else }} {{ template "docker-command" (print "docker pull " $.RegistryURL "/" $.Owner.Handle "/" $.Repository.Name ":" .Tag.Tag) }} diff --git a/pkg/auth/session.go b/pkg/auth/session.go index 70283ea..3a9a8bb 100644 --- a/pkg/auth/session.go +++ b/pkg/auth/session.go @@ -9,6 +9,7 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "errors" "fmt" "io" "log/slog" @@ -19,6 +20,16 @@ import ( "atcr.io/pkg/atproto" ) +// Sentinel errors for authentication failures +var ( + // ErrIdentityResolution indicates handle/DID resolution failed + ErrIdentityResolution = errors.New("identity resolution failed") + // ErrInvalidCredentials indicates PDS returned 401 (bad password/app-password) + ErrInvalidCredentials = errors.New("invalid credentials") + // ErrPDSUnavailable indicates PDS is unreachable or returned a server error + ErrPDSUnavailable = errors.New("PDS unavailable") +) + // CachedSession represents a cached session type CachedSession struct { DID string @@ -99,13 +110,14 @@ func (v *SessionValidator) CreateSessionAndGetToken(ctx context.Context, identif // Resolve identifier to PDS endpoint _, _, pds, err := atproto.ResolveIdentity(ctx, identifier) if err != nil { - return "", "", "", err + return "", "", "", fmt.Errorf("%w: %v", ErrIdentityResolution, err) } // Create session sessionResp, err := v.createSession(ctx, pds, identifier, password) if err != nil { - return "", "", "", fmt.Errorf("authentication failed: %w", err) + // Pass through typed errors from createSession + return "", "", "", err } // Cache the session (ATProto sessions typically last 2 hours) @@ -146,7 +158,7 @@ func (v *SessionValidator) createSession(ctx context.Context, pdsEndpoint, ident resp, err := v.httpClient.Do(req) if err != nil { slog.Debug("Session creation HTTP request failed", "error", err) - return nil, fmt.Errorf("failed to create session: %w", err) + return nil, fmt.Errorf("%w: %v", ErrPDSUnavailable, err) } defer resp.Body.Close() @@ -155,13 +167,19 @@ func (v *SessionValidator) createSession(ctx context.Context, pdsEndpoint, ident if resp.StatusCode == http.StatusUnauthorized { bodyBytes, _ := io.ReadAll(resp.Body) slog.Debug("Session creation unauthorized", "response", string(bodyBytes)) - return nil, fmt.Errorf("invalid credentials") + return nil, ErrInvalidCredentials + } + + if resp.StatusCode >= 500 { + bodyBytes, _ := io.ReadAll(resp.Body) + slog.Debug("PDS server error", "status", resp.StatusCode, "response", string(bodyBytes)) + return nil, fmt.Errorf("%w: server returned %d", ErrPDSUnavailable, resp.StatusCode) } if resp.StatusCode != http.StatusOK { bodyBytes, _ := io.ReadAll(resp.Body) slog.Debug("Session creation failed", "status", resp.StatusCode, "response", string(bodyBytes)) - return nil, fmt.Errorf("create session failed with status %d: %s", resp.StatusCode, string(bodyBytes)) + return nil, fmt.Errorf("%w: unexpected status %d: %s", ErrPDSUnavailable, resp.StatusCode, string(bodyBytes)) } var sessionResp SessionResponse diff --git a/pkg/auth/token/handler.go b/pkg/auth/token/handler.go index 33dbfaf..8500f65 100644 --- a/pkg/auth/token/handler.go +++ b/pkg/auth/token/handler.go @@ -3,6 +3,7 @@ package token import ( "context" "encoding/json" + "errors" "fmt" "log/slog" "net/http" @@ -194,8 +195,20 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { slog.Debug("Trying app password authentication", "username", username) did, handle, accessToken, err = h.validator.CreateSessionAndGetToken(r.Context(), username, password) if err != nil { - slog.Debug("App password validation failed", "error", err, "username", username) - sendAuthError(w, r, "authentication failed") + // Log at WARN level with specific error type + if errors.Is(err, auth.ErrIdentityResolution) { + slog.Warn("Identity resolution failed", "error", err, "username", username) + sendAuthError(w, r, "authentication failed: could not resolve handle") + } else if errors.Is(err, auth.ErrInvalidCredentials) { + slog.Warn("Invalid credentials", "username", username) + sendAuthError(w, r, "authentication failed: invalid credentials") + } else if errors.Is(err, auth.ErrPDSUnavailable) { + slog.Warn("PDS unavailable", "error", err, "username", username) + sendAuthError(w, r, "authentication failed: PDS unavailable") + } else { + slog.Warn("Authentication failed", "error", err, "username", username) + sendAuthError(w, r, "authentication failed") + } return }