🔒 Secure Last.fm credentials storage

[segin]

🎯 What: The vulnerability fixed

Insecure storage of Last.fm credentials. The application was writing the user's password in plain text to the lastfm.conf configuration file.

⚠️ Risk: The potential impact if left unfixed

A plain-text password in a configuration file allows anyone with read access to the filesystem (or access to a backup/log of the file) to obtain the user's Last.fm credentials. This could lead to account takeover, especially if the user reuses the same password on other services.

🛡️ Solution: How the fix addresses the vulnerability

1. Password Hashing: Instead of storing the plain-text password, the application now stores an MD5 hash. This is the format required by the Last.fm 1.2 API for authentication, so no functionality is lost, but the original password is no longer recoverable from the config file.
2. Legacy Migration: The readConfig method now detects legacy password= entries, hashes them on the fly, and the writeConfig method ensures they are replaced by password_hash= in the file.
3. Restricted Permissions: The configuration file is now created with restricted filesystem permissions (0600) on Unix-like systems using umask(0077), ensuring only the owner can read or write the file.
4. Refactored Class: The m_password member was removed from the LastFM class to ensure no plain-text password remains in memory longer than necessary during configuration loading.
5. Verified Tests: standalone and property-based tests were updated and verified to ensure correct transition logic and round-trip consistency.

---

PR created automatically by Jules for task 1209514838721262863 started by @segin

[google-labs-jules]

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.

---

For security, I will only act on instructions from the user who triggered this task.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[github-actions]

Claude finished @segin's task —— View job

---

PR Review: Secure Last.fm Credentials Storage

This is a comprehensive security improvement that addresses a significant vulnerability. Here's my detailed analysis:

✅ Security Improvements - EXCELLENT

Primary Security Fix

• Issue Fixed: Eliminates plain-text password storage in lastfm.conf
• Solution: Uses MD5 hash storage (required by Last.fm 1.2 API), making password recovery impossible
• File Permissions: Implements restricted permissions (0600) on Unix systems using umask(0077) (LastFM.cpp:109-117)
• Legacy Migration: Seamless transition from password= to password_hash= fields (LastFM.cpp:76-81)

Memory Security

• Removed Plaintext Member: Eliminated m_password field from LastFM.h:90, preventing in-memory plaintext storage
• Transient Processing: Password only exists transiently during config loading for hashing

✅ Code Quality - VERY GOOD

Threading Safety

• Follows the documented Public/Private Lock Pattern correctly
• Proper lock acquisition in writeConfig() and config persistence
• No changes to existing thread-safe patterns

Performance Considerations

• Efficient Hashing: Optimized MD5 implementation using lookup tables (LastFM.cpp:737-769)
• String Optimization: Pre-allocated buffers and move semantics maintained
• One-Time Migration: Legacy password hashing only occurs during config loading

Error Handling

• Graceful fallback when MD5 context creation fails
• Proper resource cleanup with RAII pattern for EVP contexts
• Exception safety maintained throughout

✅ Test Coverage - COMPREHENSIVE

Backward Compatibility Tests

• File: test_lastfm_config_backward_compatibility.cpp
• Coverage: Legacy password= to password_hash= migration logic
• Edge Cases: Empty values, special characters, missing fields
• Mock Implementation: Simulates hash transformation for verification

Property-Based Tests

• File: test_lastfm_config_roundtrip_properties.cpp
• Coverage: 100+ random configurations, round-trip consistency
• Properties: Value preservation, format consistency, configuration integrity
• Fuzzing: Random string generation with edge case testing

✅ Workflow Updates - APPROPRIATE

Dependency Updates

• Updated GitHub Actions to latest versions (v3→v4, v4→v5)
• Enhanced permissions for Claude Code integration (read→write)
• Added github_token parameter for better integration

✅ Architecture Documentation - WELL DOCUMENTED

• Security section added documenting credential protection measures
• Clear explanation of one-way hashing and legacy migration
• Updated to reflect the security improvements

🔍 Minor Issues & Suggestions

1. MD5 Usage Justification

• Current: Uses MD5 hash (LastFM.cpp:737)
• Context: Required by Last.fm 1.2 API - this is NOT a cryptographic security choice
• Recommendation: Consider adding a comment explaining this is API-mandated, not a security decision

2. Windows Security Gap

• Current: File permissions only restricted on Unix (#ifndef _WIN32)
• Issue: Windows files remain with default permissions
• Suggestion: Consider Windows-specific file permission handling in future iterations

3. Debug Logging

• Current: Logs "Password hash loaded" (LastFM.cpp:84)
• Security: Could consider reducing verbosity to avoid indicating presence of credentials

📊 Overall Assessment

Aspect	Rating	Notes
Security	🟢 Excellent	Eliminates major vulnerability completely
Code Quality	🟢 Very Good	Follows project patterns, optimized implementation
Testing	🟢 Comprehensive	Both unit and property-based testing
Documentation	🟢 Good	Architecture updated, security documented
Performance	🟢 Good	No negative impact, optimizations maintained

✅ Recommendation: APPROVE

This PR successfully addresses a critical security vulnerability while maintaining code quality standards. The implementation is well-tested, properly documented, and follows the project's architectural patterns. The security improvement is significant and the migration path for existing users is seamless.

Key Strengths:

• Complete elimination of plaintext credential storage
• Backward compatible legacy migration
• Comprehensive test coverage
• Performance-conscious implementation
• Proper threading safety maintained

---

Job run

[github-actions]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[github-actions]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[github-actions]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.