Skip to content

Conversation

@MythodeaLoL
Copy link

  • fix category path's
  • added override to api key
  • added secure select for real category

- fix category path's
- added override to api key
- added secure select for real category
Copilot AI review requested due to automatic review settings January 5, 2026 08:22
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a "Default" category system for SABnzbd configuration, adds an API key override feature, and fixes category path handling. The changes ensure a Default category always exists and cannot be renamed or deleted, while allowing customization of its properties (order, priority, dir).

Key Changes:

  • Introduced a mandatory "Default" category that is automatically ensured in configuration
  • Added API key override functionality to bypass database-based authentication
  • Updated category path building logic to handle the Default category consistently with a default directory of "complete"

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
internal/importer/service.go Updated buildCategoryPath to use case-insensitive comparisons and handle Default category with default directory
internal/config/manager.go Added EnsureDefaultCategory function and constants for Default category, updated validation to normalize complete_dir path
internal/api/sabnzbd_handlers.go Added getDefaultCategory helper, updated buildCategoryPath to match importer logic (with inconsistencies)
internal/api/parsers.go Added API key override validation in validateAPIKey function
internal/api/auth_handlers.go Added logic to sync key_override when regenerating API keys
frontend/src/components/config/SABnzbdConfigSection.tsx Added UI protections to prevent renaming/deleting Default category, added visual indicators
config.sample.yaml Updated documentation and examples to show Default category and key_override field

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 753 to 754
// buildCategoryPath resolves a category name to its configured directory path.
// Returns the category's Dir if configured, otherwise falls back to the category name.
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate comment lines. Line 753 contains the original comment "buildCategoryPath resolves a category name to its configured directory path." and lines 755-756 add a new comment "buildCategoryPath builds the directory path for a category." These are redundant. The older comment on line 753-754 should be removed to avoid duplication.

Suggested change
// buildCategoryPath resolves a category name to its configured directory path.
// Returns the category's Dir if configured, otherwise falls back to the category name.

Copilot uses AI. Check for mistakes.
Comment on lines 321 to 340
// If key_override is configured (has a value with 33 chars), update it with the new key
if s.configManager != nil {
cfg := s.configManager.GetConfig()
if cfg.API.KeyOverride != "" && len(cfg.API.KeyOverride) == 33 {
// Update the key_override in config to match the new key
newConfig := cfg.DeepCopy()
newConfig.API.KeyOverride = apiKey

if err := s.configManager.UpdateConfig(newConfig); err != nil {
slog.WarnContext(c.Context(), "Failed to update key_override in config", "error", err)
// Don't fail the request, just log the warning
} else {
if err := s.configManager.SaveConfig(); err != nil {
slog.WarnContext(c.Context(), "Failed to save config after updating key_override", "error", err)
} else {
slog.InfoContext(c.Context(), "Updated key_override in config with new API key")
}
}
}
}
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic for updating key_override when regenerating an API key is unclear and potentially problematic. If a user has explicitly set a key_override (which should be independent of the database API key), regenerating the database API key shouldn't automatically update the key_override. This coupling could lead to unexpected behavior where the user's intended override is replaced. Consider whether this automatic sync is necessary, or if it should only happen when the user explicitly requests to sync the override with the database key.

Copilot uses AI. Check for mistakes.
// Look for the category in configuration
for _, configCategory := range config.SABnzbd.Categories {
for _, configCategory := range cfg.SABnzbd.Categories {
if configCategory.Name == category {
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent case-sensitive comparison for the Default category. In internal/importer/service.go lines 766, 773, and 778, the code uses strings.EqualFold for case-insensitive comparison, but here it uses exact equality (==). This should use strings.EqualFold to maintain consistency with the other buildCategoryPath implementation.

Copilot uses AI. Check for mistakes.
if s.configManager != nil {
cfg := s.configManager.GetConfig()
for _, cat := range cfg.SABnzbd.Categories {
if cat.Name == config.DefaultCategoryName {
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent case-sensitive comparison for the Default category. This should use strings.EqualFold for case-insensitive comparison to maintain consistency with EnsureDefaultCategory in internal/config/manager.go which uses strings.EqualFold on line 261.

Suggested change
if cat.Name == config.DefaultCategoryName {
if strings.EqualFold(cat.Name, config.DefaultCategoryName) {

Copilot uses AI. Check for mistakes.
Comment on lines +170 to +171
// Prevent creating a category with the reserved name
if (newCategory.name.trim() === DEFAULT_CATEGORY_NAME) {
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Case-insensitive validation is missing for preventing reserved category name. The check newCategory.name.trim() === DEFAULT_CATEGORY_NAME is case-sensitive, but the backend's EnsureDefaultCategory uses strings.EqualFold (case-insensitive) on line 261 of internal/config/manager.go. This means a user could create a category named "default" or "DEFAULT" in the UI, which would then be normalized to "Default" by the backend, potentially causing duplicate category issues. Consider using a case-insensitive comparison: newCategory.name.trim().toLowerCase() === DEFAULT_CATEGORY_NAME.toLowerCase()

Suggested change
// Prevent creating a category with the reserved name
if (newCategory.name.trim() === DEFAULT_CATEGORY_NAME) {
// Prevent creating a category with the reserved name (case-insensitive to match backend)
if (newCategory.name.trim().toLowerCase() === DEFAULT_CATEGORY_NAME.toLowerCase()) {

Copilot uses AI. Check for mistakes.
Comment on lines 110 to 111
// Check config key_override first (must be exactly 33 chars)
if cfg.API.KeyOverride != "" && len(cfg.API.KeyOverride) == 33 {
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API key length validation is incorrect. The generateAPIKey function in internal/database/user_repository.go generates 24 random bytes and encodes them to base64 without padding, which produces exactly 32 characters (not 33). The check should be len(cfg.API.KeyOverride) == 32 instead of len(cfg.API.KeyOverride) == 33.

Suggested change
// Check config key_override first (must be exactly 33 chars)
if cfg.API.KeyOverride != "" && len(cfg.API.KeyOverride) == 33 {
// Check config key_override first (must be exactly 32 chars)
if cfg.API.KeyOverride != "" && len(cfg.API.KeyOverride) == 32 {

Copilot uses AI. Check for mistakes.
Comment on lines 321 to 324
// If key_override is configured (has a value with 33 chars), update it with the new key
if s.configManager != nil {
cfg := s.configManager.GetConfig()
if cfg.API.KeyOverride != "" && len(cfg.API.KeyOverride) == 33 {
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API key length validation is incorrect. The generateAPIKey function in internal/database/user_repository.go generates 24 random bytes and encodes them to base64 without padding, which produces exactly 32 characters (not 33). The check should be len(cfg.API.KeyOverride) == 32 instead of len(cfg.API.KeyOverride) == 33.

Suggested change
// If key_override is configured (has a value with 33 chars), update it with the new key
if s.configManager != nil {
cfg := s.configManager.GetConfig()
if cfg.API.KeyOverride != "" && len(cfg.API.KeyOverride) == 33 {
// If key_override is configured (has a value with 32 chars), update it with the new key
if s.configManager != nil {
cfg := s.configManager.GetConfig()
if cfg.API.KeyOverride != "" && len(cfg.API.KeyOverride) == 32 {

Copilot uses AI. Check for mistakes.
# REST API configuration
api:
prefix: '/api' # API endpoint prefix
key_override: '' # 33-char API key override (takes precedence over database key, leave empty to use database)
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation comment is incorrect. The generateAPIKey function in internal/database/user_repository.go generates 24 random bytes and encodes them to base64 without padding, which produces exactly 32 characters (not 33). The comment should say "32-char API key override" instead of "33-char API key override".

Suggested change
key_override: '' # 33-char API key override (takes precedence over database key, leave empty to use database)
key_override: '' # 32-char API key override (takes precedence over database key, leave empty to use database)

Copilot uses AI. Check for mistakes.
Comment on lines +919 to +929
if category == config.DefaultCategoryName {
return config.DefaultCategoryDir
}
return category
}

config := s.configManager.GetConfig()
cfg := s.configManager.GetConfig()

// If no categories are configured, use category name as directory
if len(config.SABnzbd.Categories) == 0 {
if len(cfg.SABnzbd.Categories) == 0 {
if category == config.DefaultCategoryName {
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent case-sensitive comparison for the Default category. This should use strings.EqualFold for case-insensitive comparison to maintain consistency with internal/importer/service.go and allow users to specify "default", "Default", or "DEFAULT".

Copilot uses AI. Check for mistakes.
return configCategory.Dir
}
// For Default category with empty Dir, return default dir
if category == config.DefaultCategoryName {
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent case-sensitive comparison for the Default category. This should use strings.EqualFold for case-insensitive comparison to maintain consistency with internal/importer/service.go and allow users to specify "default", "Default", or "DEFAULT".

Copilot uses AI. Check for mistakes.
@MythodeaLoL
Copy link
Author

The main code was updated while I was creating this PR, the tests I ran need to be redone to ensure correct functionality with the updated main branch.

@drondeseries drondeseries merged commit 2e9d7dd into drondeseries:main Jan 5, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants