Claude/plan refactoring 01 ngqvyu hf ax xg qbl6ab nn4r #7
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit addresses critical security vulnerabilities and improves
the overall stability and error handling of the better_image_tag gem.
## Security Fixes
### 1. Replace URI.open with secure Net::HTTP (CRITICAL)
- **Issue**: URI.open (deprecated Kernel#open) enables arbitrary command
execution and is a major security vulnerability
- **Fix**: Replaced with Net::HTTP implementation with proper timeout
configuration
- **Impact**: Prevents remote code execution attacks
- **Files**: lib/better_image_tag/inline_data.rb
### 2. Fix shell injection vulnerabilities (HIGH)
- **Issue**: String interpolation in system() calls allows shell injection
attacks via malicious filenames
- **Fix**: Use array form of system() which properly escapes arguments
- **Impact**: Prevents command injection via crafted image filenames
- **Files**:
- lib/better_image_tag/commands/convert_jpg_to_webp.rb
- lib/better_image_tag/commands/convert_jpg_to_avif.rb
### 3. Replace unmaintained mimemagic dependency (MEDIUM)
- **Issue**: mimemagic gem is unmaintained and has known security issues
- **Fix**: Migrate to marcel gem, actively maintained by Rails team
- **Impact**: Uses secure, maintained dependency for MIME type detection
- **Files**:
- better_image_tag.gemspec
- lib/better_image_tag/inline_data.rb
- lib/better_image_tag/image_tag.rb
### 4. Implement SVG sanitization to prevent XSS (HIGH)
- **Issue**: String mutation approach with regex is fragile and doesn't
sanitize malicious SVG content (script tags, event handlers)
- **Fix**: Use Nokogiri for proper XML parsing and sanitization
- Remove dangerous elements (script, foreignObject)
- Strip event handler attributes (onclick, onload, etc.)
- Validate SVG structure
- **Impact**: Prevents XSS attacks via malicious SVG files
- **Files**: lib/better_image_tag/svg_tag.rb
## Error Handling Improvements
### 5. Add comprehensive error handling strategy
- **New Features**:
- Added `on_error` callback configuration for custom error handling
- Enhanced HTTP error types (timeout, connection refused, socket errors)
- Graceful degradation on network failures
- Better error messages with context
- **Files**:
- lib/better_image_tag.rb (configuration)
- lib/better_image_tag/errors.rb (new error classes)
- lib/better_image_tag/inline_data.rb (error callbacks)
- lib/better_image_tag/svg_tag.rb (SVG error handling)
### 6. Add network timeout configuration
- **New Feature**: Configurable network timeouts (default: 10 seconds)
- **Configuration**: `BetterImageTag.configuration.network_timeout = 30`
- **Impact**: Prevents hanging requests, better production behavior
- **Files**: lib/better_image_tag.rb, lib/better_image_tag/inline_data.rb
## Dependency Updates
- **Removed**: mimemagic (unmaintained, security issues)
- **Added**: marcel ~> 1.0 (Rails-maintained MIME type detection)
- **Added**: nokogiri >= 1.12.0 (secure XML parsing and sanitization)
## New Error Classes
- `RemoteFetchError`: Network failures when fetching remote images
- `InvalidSvgError`: SVG parsing or validation failures
## Configuration Changes
New configuration options available:
```ruby
BetterImageTag.configure do |config|
# Network timeout for remote image fetching (default: 10)
config.network_timeout = 30
# Error callback for monitoring/logging
config.on_error = ->(error, context) do
Rails.logger.error("BetterImageTag error: #{error.message}")
Sentry.capture_exception(error, extra: context)
end
end
```
## Breaking Changes
None. All changes are backward compatible.
## Migration Notes
1. Run `bundle update` to install new dependencies (marcel, nokogiri)
2. Remove any mimemagic version constraints from your Gemfile
3. Optionally configure error callbacks for monitoring
## Testing
- All existing tests should continue to pass
- Manual testing recommended for:
- Remote image fetching with various URLs
- SVG inlining with malicious content
- Image conversion commands with special characters in filenames
---
This is Phase 1 of a larger refactoring effort focused on security and
stability. Future phases will address architecture improvements and
feature enhancements.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.