github-starred/vikunja
2
0
Fork 0
mirror of https://github.com/go-vikunja/vikunja.git synced 2026-03-22 21:15:18 -05:00
Code Issues 447 Packages Projects Releases 9 Wiki Activity

[PR #1379] [MERGED] Fix type assertion panics in avatar providers with graceful recovery and recursion guards #3385

New Issue
Closed
opened 2026-03-22 14:42:44 -05:00 by GiteaMirror · 0 comments
GiteaMirror commented 2026-03-22 14:42:44 -05:00
Owner
Copy Link

📋 Pull Request Information

Original PR: https://github.com/go-vikunja/vikunja/pull/1379
Author: @Copilot
Created: 9/2/2025
Status: ✅ Merged
Merged: 9/2/2025
Merged by: @kolaente

Base: main ← Head: copilot/fix-1378

📝 Commits (6)

  • fb6f0b7 Initial plan
  • 71900a4 Fix type assertion error in avatar upload provider with logging and graceful recovery
  • daeacdb Fix type assertion panics in initials avatar provider with safe type assertions
  • 6b706b3 Add recursion guards and refactor tests to use t.Run subtests
  • f5392f1 fix: use require.NoError and assert.Positive in initials tests
  • 4b83b6b fix: resolve testifylint issues in avatar tests

📊 Changes

4 files changed (+349 additions, -3 deletions)

View changed files

📝 pkg/modules/avatar/initials/initials.go (+48 -2)
➕ pkg/modules/avatar/initials/initials_test.go (+179 -0)
📝 pkg/modules/avatar/upload/upload.go (+29 -1)
➕ pkg/modules/avatar/upload/upload_test.go (+93 -0)

📄 Description

This PR fixes critical runtime panics that occur when the keyvalue cache contains corrupted or legacy data for avatar providers. The issues manifested as runtime.TypeAssertionError crashes in both upload and initials avatar providers:

runtime.TypeAssertionError: interface conversion: interface {} is string, not upload.CachedAvatar

runtime.TypeAssertionError: interface conversion: interface {} is string, not initials.CachedAvatar

runtime.TypeAssertionError: interface conversion: interface {} is string, not image.RGBA64

Problem

Multiple avatar providers were performing unsafe type assertions on cached data:

Upload Provider (pkg/modules/avatar/upload/upload.go):

  • Line 101: cachedAvatar := result.(CachedAvatar)

Initials Provider (pkg/modules/avatar/initials/initials.go):

  • Line 200: cachedAvatar := result.(CachedAvatar)
  • Line 158: aa := result.(image.RGBA64)

When the cache contained legacy string data or became corrupted, these would cause the application to crash instead of gracefully handling the situation.

Solution

  1. Safe Type Assertions: Replaced all unsafe type assertions with safe ones using the comma ok idiom
  2. Detailed Logging: Added comprehensive logging that shows the actual type and value stored in cache when type mismatches occur
  3. Graceful Recovery: When invalid cache data is detected, the system now:
    • Logs the issue with full details for debugging
    • Clears the corrupted cache entry
    • Recursively regenerates the avatar
  4. Recursion Guards: Added depth limits (max 3 attempts) to prevent infinite recursion loops when cache corruption persists or regeneration fails
  5. Enhanced Validation: Added proper input validation to prevent edge cases during regeneration
  6. Test Improvements: Refactored tests to use t.Run subtests for better organization and maintainability

Changes

Upload Provider:

  • Modified GetAvatar() to use safe type assertion with proper error handling
  • Added recursion guard with getAvatarWithDepth() helper method to prevent infinite loops
  • Added input validation for AvatarFileID to prevent nil pointer dereference
  • Refactored tests to use t.Run subtests for better test organization

Initials Provider:

  • Modified GetAvatar() to use safe type assertion for CachedAvatar
  • Modified getAvatarForUser() to use safe type assertion for image.RGBA64
  • Added recursion guards with depth-tracking helper methods (getAvatarWithDepth, getAvatarForUserWithDepth)
  • Refactored tests to use t.Run subtests with separate test groups for better organization

Both providers now automatically detect, log, clear, and regenerate corrupted cache entries without crashing the application. The recursion guards prevent infinite loops if cache corruption persists or regeneration continuously fails.

Fixed Sentry Issues:

  • API-OSS-25: interface conversion: interface {} is string, not initials.CachedAvatar
  • API-OSS-27: interface conversion: interface {} is string, not image.RGBA64

Fixes #1378.

💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/go-vikunja/vikunja/pull/1379 **Author:** [@Copilot](https://github.com/apps/copilot-swe-agent) **Created:** 9/2/2025 **Status:** ✅ Merged **Merged:** 9/2/2025 **Merged by:** [@kolaente](https://github.com/kolaente) **Base:** `main` ← **Head:** `copilot/fix-1378` --- ### 📝 Commits (6) - [`fb6f0b7`](https://github.com/go-vikunja/vikunja/commit/fb6f0b7648cbd75e8a867ab9baff5a2a3a19d330) Initial plan - [`71900a4`](https://github.com/go-vikunja/vikunja/commit/71900a4a6bb1c8f9b02ea46c738e1a481529e300) Fix type assertion error in avatar upload provider with logging and graceful recovery - [`daeacdb`](https://github.com/go-vikunja/vikunja/commit/daeacdbe4918be40e1a754c551014916c394bb43) Fix type assertion panics in initials avatar provider with safe type assertions - [`6b706b3`](https://github.com/go-vikunja/vikunja/commit/6b706b314d1f144ce881f7ca53d5b19d8ae43564) Add recursion guards and refactor tests to use t.Run subtests - [`f5392f1`](https://github.com/go-vikunja/vikunja/commit/f5392f19b8b6d21c8338c7baa59ec0b32aada4fc) fix: use require.NoError and assert.Positive in initials tests - [`4b83b6b`](https://github.com/go-vikunja/vikunja/commit/4b83b6bd19c718982113fc7c95a9824b9b224375) fix: resolve testifylint issues in avatar tests ### 📊 Changes **4 files changed** (+349 additions, -3 deletions) <details> <summary>View changed files</summary> 📝 `pkg/modules/avatar/initials/initials.go` (+48 -2) ➕ `pkg/modules/avatar/initials/initials_test.go` (+179 -0) 📝 `pkg/modules/avatar/upload/upload.go` (+29 -1) ➕ `pkg/modules/avatar/upload/upload_test.go` (+93 -0) </details> ### 📄 Description This PR fixes critical runtime panics that occur when the keyvalue cache contains corrupted or legacy data for avatar providers. The issues manifested as `runtime.TypeAssertionError` crashes in both upload and initials avatar providers: ``` runtime.TypeAssertionError: interface conversion: interface {} is string, not upload.CachedAvatar ``` ``` runtime.TypeAssertionError: interface conversion: interface {} is string, not initials.CachedAvatar ``` ``` runtime.TypeAssertionError: interface conversion: interface {} is string, not image.RGBA64 ``` ## Problem Multiple avatar providers were performing unsafe type assertions on cached data: **Upload Provider (`pkg/modules/avatar/upload/upload.go`):** - Line 101: `cachedAvatar := result.(CachedAvatar)` **Initials Provider (`pkg/modules/avatar/initials/initials.go`):** - Line 200: `cachedAvatar := result.(CachedAvatar)` - Line 158: `aa := result.(image.RGBA64)` When the cache contained legacy string data or became corrupted, these would cause the application to crash instead of gracefully handling the situation. ## Solution 1. **Safe Type Assertions**: Replaced all unsafe type assertions with safe ones using the comma ok idiom 2. **Detailed Logging**: Added comprehensive logging that shows the actual type and value stored in cache when type mismatches occur 3. **Graceful Recovery**: When invalid cache data is detected, the system now: - Logs the issue with full details for debugging - Clears the corrupted cache entry - Recursively regenerates the avatar 4. **Recursion Guards**: Added depth limits (max 3 attempts) to prevent infinite recursion loops when cache corruption persists or regeneration fails 5. **Enhanced Validation**: Added proper input validation to prevent edge cases during regeneration 6. **Test Improvements**: Refactored tests to use `t.Run` subtests for better organization and maintainability ## Changes **Upload Provider:** - Modified `GetAvatar()` to use safe type assertion with proper error handling - Added recursion guard with `getAvatarWithDepth()` helper method to prevent infinite loops - Added input validation for `AvatarFileID` to prevent nil pointer dereference - Refactored tests to use `t.Run` subtests for better test organization **Initials Provider:** - Modified `GetAvatar()` to use safe type assertion for `CachedAvatar` - Modified `getAvatarForUser()` to use safe type assertion for `image.RGBA64` - Added recursion guards with depth-tracking helper methods (`getAvatarWithDepth`, `getAvatarForUserWithDepth`) - Refactored tests to use `t.Run` subtests with separate test groups for better organization Both providers now automatically detect, log, clear, and regenerate corrupted cache entries without crashing the application. The recursion guards prevent infinite loops if cache corruption persists or regeneration continuously fails. **Fixed Sentry Issues:** - API-OSS-25: `interface conversion: interface {} is string, not initials.CachedAvatar` - API-OSS-27: `interface conversion: interface {} is string, not image.RGBA64` Fixes #1378. <!-- START COPILOT CODING AGENT TIPS --> --- 💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click [here](https://survey3.medallia.com/?EAHeSx-AP01bZqG0Ld9QLQ) to start the survey. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
GiteaMirror added the pull-request label 2026-03-22 14:42:44 -05:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
area/api
area/caldav
area/filters
area/frontend
area/gantt
area/internal-code
area/typesense
bug
changes requested
confirmed
dependencies
enhancement
good first issue
help wanted
kind/bug
kind/feature
needs reproduction
pull-request
Mirrored from GitHub Pull Request
 question
security
support
upstream issue
waiting for reply
wontfix
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
GiteaMirror ninjasurge
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: github-starred/vikunja#3385
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?