started using Claude Code
This commit is contained in:
Gavin McDonald
284
BUGS.md
Normal file
284
BUGS.md
Normal file
@@ -0,0 +1,284 @@
|
||||
# Known Bugs and Issues
|
||||
|
||||
This document lists identified bugs and potential issues in the Labelmaker codebase.
|
||||
|
||||
## Critical Issues
|
||||
|
||||
### 1. Race Condition in Image Generation
|
||||
|
||||
**Location**: `lib/labelmaker_web/controllers/label_controller.ex:21-31`
|
||||
|
||||
**Status**: ⚠️ **UNRESOLVED**
|
||||
|
||||
**Issue**: When multiple requests arrive simultaneously for the same non-existent image, both will start generating it concurrently. Additionally, `send_file/3` is called immediately after starting generation without waiting for it to complete, which will cause a file-not-found error.
|
||||
|
||||
```elixir
|
||||
unless File.exists?(filepath) do
|
||||
basic_settings(options)
|
||||
|> outline_settings(options)
|
||||
|> size_settings(options)
|
||||
|> final_settings(options)
|
||||
|> generate_image()
|
||||
end
|
||||
|
||||
conn
|
||||
|> put_resp_content_type("image/png")
|
||||
|> send_file(200, filepath) # Called before generation completes!
|
||||
```
|
||||
|
||||
**Impact**: High - Will cause 500 errors when requests race or when generation is slow
|
||||
|
||||
**Suggested Fix**:
|
||||
- Use a mutex/lock mechanism (e.g., via `:global` or a GenServer) to ensure only one process generates a given image
|
||||
- Ensure `send_file` is only called after generation completes
|
||||
- Consider using a temporary file with atomic rename to prevent serving partially-written images
|
||||
|
||||
```elixir
|
||||
# Example approach with synchronization
|
||||
case ensure_image_exists(filepath, options) do
|
||||
:ok ->
|
||||
conn
|
||||
|> put_resp_content_type("image/png")
|
||||
|> send_file(200, filepath)
|
||||
{:error, reason} ->
|
||||
conn
|
||||
|> put_status(500)
|
||||
|> json(%{error: "Failed to generate image"})
|
||||
end
|
||||
```
|
||||
|
||||
### 2. Crash on Invalid Numeric Inputs
|
||||
|
||||
**Location**: `lib/labelmaker_web/tools.ex:87-88, 136-137, 140-141`
|
||||
|
||||
**Status**: ⚠️ **UNRESOLVED**
|
||||
|
||||
**Issue**: When `height`, `width`, or `size` parameters contain non-numeric strings (e.g., "abc", "12.5px"), `String.to_integer/1` raises an `ArgumentError` that isn't caught, causing the entire request to crash.
|
||||
|
||||
```elixir
|
||||
defp process_height(height, _parameters) do
|
||||
height |> String.to_integer() |> max(0) |> min(Constants.max_height())
|
||||
end
|
||||
|
||||
defp process_width(width, _parameters) do
|
||||
width |> String.to_integer() |> max(0) |> min(Constants.max_width())
|
||||
end
|
||||
|
||||
defp calculate_preview_height(parameters) do
|
||||
size = parameters["size"] |> String.to_integer()
|
||||
# ...
|
||||
end
|
||||
```
|
||||
|
||||
**Impact**: High - User-supplied invalid input causes crashes
|
||||
|
||||
**Suggested Fix**:
|
||||
```elixir
|
||||
defp process_height(height, _parameters) do
|
||||
case Integer.parse(height) do
|
||||
{int, _} -> int |> max(0) |> min(Constants.max_height())
|
||||
:error -> String.to_integer(Constants.defaults().height)
|
||||
end
|
||||
end
|
||||
|
||||
defp process_width(width, _parameters) do
|
||||
case Integer.parse(width) do
|
||||
{int, _} -> int |> max(0) |> min(Constants.max_width())
|
||||
:error -> String.to_integer(Constants.defaults().width)
|
||||
end
|
||||
end
|
||||
|
||||
defp calculate_preview_height(parameters) do
|
||||
size = case Integer.parse(parameters["size"]) do
|
||||
{int, _} -> int
|
||||
:error -> 72 # default
|
||||
end
|
||||
rows = calculate_rows(parameters["label"])
|
||||
size + size * rows
|
||||
end
|
||||
```
|
||||
|
||||
## Moderate Issues
|
||||
|
||||
### 3. Missing Error Handling on ImageMagick Command
|
||||
|
||||
**Location**: `lib/labelmaker_web/controllers/label_controller.ex:112`
|
||||
|
||||
**Status**: ⚠️ **UNRESOLVED**
|
||||
|
||||
**Issue**: Pattern matching on `{_, 0}` will crash if ImageMagick returns a non-zero exit code (e.g., invalid arguments, ImageMagick not installed, permission errors, disk full).
|
||||
|
||||
```elixir
|
||||
{_, 0} = System.cmd("magick", args)
|
||||
```
|
||||
|
||||
**Impact**: Moderate - Crashes on ImageMagick errors instead of gracefully handling
|
||||
|
||||
**Suggested Fix**:
|
||||
```elixir
|
||||
case System.cmd("magick", args, stderr_to_stdout: true) do
|
||||
{_output, 0} ->
|
||||
:ok
|
||||
{error, code} ->
|
||||
require Logger
|
||||
Logger.error("ImageMagick failed with code #{code}: #{error}")
|
||||
{:error, :image_generation_failed}
|
||||
end
|
||||
```
|
||||
|
||||
Then handle the error tuple in the controller's `show/2` function and return appropriate HTTP error response.
|
||||
|
||||
### 4. Potential nil Crash on Font Lookup
|
||||
|
||||
**Location**: `lib/labelmaker_web/tools.ex:34-35`
|
||||
|
||||
**Status**: ⚠️ **UNRESOLVED**
|
||||
|
||||
**Issue**: If an invalid font is passed that isn't in the font_map, the map lookup returns `nil`. While later validation (line 64) filters it out, if defaults somehow don't include `font`, `nil` could be passed to ImageMagick causing cryptic errors.
|
||||
|
||||
```elixir
|
||||
{:font, font} ->
|
||||
{:font, Constants.font_map()[String.downcase(font)]}
|
||||
```
|
||||
|
||||
**Impact**: Low-Moderate - Edge case but could cause ImageMagick errors
|
||||
|
||||
**Suggested Fix**:
|
||||
```elixir
|
||||
{:font, font} ->
|
||||
{:font, Map.get(Constants.font_map(), String.downcase(font), Constants.defaults().font)}
|
||||
```
|
||||
|
||||
### 5. label_too_long Flag Logic Bug
|
||||
|
||||
**Location**: `lib/labelmaker_web/tools.ex:40-41`
|
||||
|
||||
**Status**: 🐛 **CONFIRMED BUG**
|
||||
|
||||
**Issue**: The `label_too_long` flag is calculated AFTER the label has already been truncated in `process_label/1` (line 16-17). This means the flag will always be `false` because it's checking the length of the already-truncated label, not the original input.
|
||||
|
||||
```elixir
|
||||
# Line 16-17: Label gets truncated here
|
||||
{"label", label} ->
|
||||
{"label", process_label(label)}
|
||||
|
||||
# Line 40-41: This checks the ALREADY TRUNCATED label
|
||||
{:label_too_long, _} ->
|
||||
{:label_too_long, String.length(parameters["label"]) > Constants.max_label_length()}
|
||||
```
|
||||
|
||||
**Impact**: Low - UI warning for long labels never appears
|
||||
|
||||
**Suggested Fix**:
|
||||
Check the length before truncation, or pass the original length through:
|
||||
```elixir
|
||||
def process_parameters(parameters) do
|
||||
# Calculate label_too_long before any processing
|
||||
original_label_length = String.length(Map.get(parameters, "label", ""))
|
||||
|
||||
parameters =
|
||||
Constants.defaults()
|
||||
|> Map.new(fn {k, v} -> {Atom.to_string(k), v} end)
|
||||
|> Map.merge(parameters)
|
||||
|> Map.put("_original_label_length", original_label_length)
|
||||
# ... rest of processing
|
||||
|
||||
# Later when setting the flag:
|
||||
{:label_too_long, _} ->
|
||||
{:label_too_long, parameters["_original_label_length"] > Constants.max_label_length()}
|
||||
```
|
||||
|
||||
## Low Priority Issues
|
||||
|
||||
### 6. Hash Collision Risk from inspect()
|
||||
|
||||
**Location**: `lib/labelmaker_web/controllers/label_controller.ex:11-15`
|
||||
|
||||
**Status**: ⚠️ **UNRESOLVED**
|
||||
|
||||
**Issue**: Using `inspect/1` on a map for hashing is fragile. Map ordering isn't guaranteed to be consistent across Erlang/OTP versions or runtime conditions. Two identical maps could theoretically produce different inspect strings, leading to cache misses.
|
||||
|
||||
```elixir
|
||||
filename =
|
||||
options
|
||||
|> inspect()
|
||||
|> (fn str -> :crypto.hash(:sha256, str) end).()
|
||||
|> Base.encode16(case: :lower)
|
||||
```
|
||||
|
||||
**Impact**: Low - Mostly theoretical, but could cause unnecessary cache misses
|
||||
|
||||
**Suggested Fix**: Hash a consistently-ordered representation:
|
||||
```elixir
|
||||
filename =
|
||||
options
|
||||
|> Map.take([:label, :color, :font, :outline, :size, :width, :height, :align])
|
||||
|> Enum.sort()
|
||||
|> :erlang.term_to_binary()
|
||||
|> :crypto.hash(:sha256)
|
||||
|> Base.encode16(case: :lower)
|
||||
```
|
||||
|
||||
### 7. Missing Type Guards on String Operations
|
||||
|
||||
**Location**: `lib/labelmaker_web/tools.ex:32, 35`
|
||||
|
||||
**Status**: ⚠️ **UNRESOLVED**
|
||||
|
||||
**Issue**: `String.downcase/1` is called on `align` and `font` parameters without verifying they're strings. Given the data flow this is unlikely to be a problem, but defensive programming would add guards.
|
||||
|
||||
```elixir
|
||||
{:align, align} ->
|
||||
{:align, align |> String.downcase()}
|
||||
|
||||
{:font, font} ->
|
||||
{:font, Constants.font_map()[String.downcase(font)]}
|
||||
```
|
||||
|
||||
**Impact**: Very Low - Input validation earlier in the pipeline makes this unlikely
|
||||
|
||||
**Suggested Fix**: Add type guards or convert to string first:
|
||||
```elixir
|
||||
{:align, align} when is_binary(align) ->
|
||||
{:align, String.downcase(align)}
|
||||
|
||||
# Or alternatively:
|
||||
{:align, align} ->
|
||||
{:align, align |> to_string() |> String.downcase()}
|
||||
```
|
||||
|
||||
## Fixed Issues
|
||||
|
||||
### ✅ 8. ImageMagick Property Interpolation Warning
|
||||
|
||||
**Location**: `lib/labelmaker_web/controllers/label_controller.ex:50, 65, 96`
|
||||
|
||||
**Status**: ✅ **FIXED**
|
||||
|
||||
**Issue**: Labels containing `%` characters (like "Test!@#$%" or URL-encoded text) caused ImageMagick to emit warnings about unknown properties like `%2`, `%20`, etc., because ImageMagick treats `%` as a special character for property interpolation.
|
||||
|
||||
**Fix Applied**: All `%` characters are now escaped to `%%` in:
|
||||
1. Label text passed to `label:` and `caption:` commands
|
||||
2. Metadata stored in image comments
|
||||
|
||||
```elixir
|
||||
# Lines 47-50, 62-65
|
||||
escaped_label =
|
||||
options.label
|
||||
|> String.slice(0, Constants.max_label_length())
|
||||
|> String.replace("%", "%%")
|
||||
|
||||
# Lines 91-96
|
||||
comment =
|
||||
options
|
||||
|> Map.drop([:filepath, :link])
|
||||
|> Jason.encode!()
|
||||
|> inspect()
|
||||
|> String.replace("%", "%%")
|
||||
```
|
||||
|
||||
## Notes
|
||||
|
||||
- All critical and moderate issues should be addressed before deploying to production
|
||||
- The test suite (105 tests) currently passes but doesn't cover error conditions for many of these bugs
|
||||
- Consider adding integration tests that specifically test error handling paths
|
||||
Reference in New Issue
Block a user