feat(fonts): add dynamic font selection and font manager improvements#232
Open
ChuckBuilds wants to merge 2 commits intomainfrom
Open
feat(fonts): add dynamic font selection and font manager improvements#232ChuckBuilds wants to merge 2 commits intomainfrom
ChuckBuilds wants to merge 2 commits intomainfrom
Conversation
- Add font-selector widget for dynamic font selection in plugin configs - Enhance /api/v3/fonts/catalog with filename, display_name, and type - Add /api/v3/fonts/preview endpoint for server-side font rendering - Add /api/v3/fonts/<family> DELETE endpoint with system font protection - Fix /api/v3/fonts/upload to actually save uploaded font files - Update font manager tab with dynamic dropdowns, server-side preview, and font deletion - Add new BDF fonts: 6x10, 6x12, 6x13, 7x13, 7x14, 8x13, 9x15, 9x18, 10x20 (with bold/oblique variants) - Add tom-thumb, helvR12, clR6x12, texgyre-27 fonts Plugin authors can use x-widget: "font-selector" in schemas to enable dynamic font selection that automatically shows all available fonts. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds font assets and documentation; implements backend font endpoints (upload, preview, delete) and catalog metadata; introduces a Font Selector frontend widget and replaces static font lists with a dynamic, catalog-driven UI integrated into templates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant "Font Selector\nWidget"
participant "API Server\n(api_v3)"
participant "Font Store\nassets/fonts"
participant "Image Renderer\n(preview)"
User->>Browser: open Fonts UI / select font
Browser->>Font Selector\nWidget: render(), fetchCatalog()
Font Selector\nWidget->>API Server: GET /api/v3/fonts/catalog
API Server->>Font Store: read catalog metadata
Font Store-->>API Server: catalog entries
API Server-->>Font Selector\nWidget: font catalog (JSON)
Font Selector\nWidget-->>Browser: render options
User->>Browser: upload font file
Browser->>API Server: POST /api/v3/fonts/upload (file)
API Server->>Font Store: write safe filename
API Server-->>Browser: upload response (filename, path)
Browser->>Font Selector\nWidget: clear cache & fetchCatalog()
User->>Browser: request preview for text
Browser->>API Server: GET /api/v3/fonts/preview?font=...&text=...
API Server->>Image Renderer: generate PNG using font from Font Store
Image Renderer->>API Server: PNG (base64)
API Server-->>Browser: preview image
Browser-->>User: display preview
User->>Browser: delete custom font
Browser->>API Server: DELETE /api/v3/fonts/<family>
API Server->>Font Store: remove file (if not system font)
API Server-->>Browser: deletion result
Browser->>Font Selector\nWidget: clear cache & refresh UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web_interface/blueprints/api_v3.py (1)
5496-5500:⚠️ Potential issue | 🟠 MajorAllow
.otfuploads consistently with earlier validation.
validate_file_upload()accepts.otf, but the later extension check rejects it, so valid files fail.✅ Proposed fix
- allowed_extensions = ['.ttf', '.bdf'] - file_extension = font_file.filename.lower().split('.')[-1] - if f'.{file_extension}' not in allowed_extensions: - return jsonify({'status': 'error', 'message': 'Only .ttf and .bdf files are allowed'}), 400 + allowed_extensions = ['.ttf', '.otf', '.bdf'] + file_extension = os.path.splitext(font_file.filename)[1].lower() + if file_extension not in allowed_extensions: + return jsonify({'status': 'error', 'message': 'Only .ttf, .otf, and .bdf files are allowed'}), 400
🤖 Fix all issues with AI agents
In `@assets/fonts/README.md`:
- Around line 2-46: The README.md has typos, inconsistent filenames/casing,
missing hyphens, and several fenced code blocks lacking language identifiers
(MD040); fix wording typos ("human readable" -> "human-readable", "editbable" ->
"editable", "30pixel" -> "30-pixel", "fairly straight-foward" -> "fairly
straightforward"), correct filename casing/typos (ensure "texgyre-27.bdf" and
"texgyreadventor-regular.otf" are spelled consistently), and add language tags
("bash") to the code fences around otf2bdf and apt/build command blocks; update
the example otf2bdf and compilation instruction blocks to match the suggested
edits while keeping the referenced commands (otf2bdf, texgyre-27.bdf,
texgyreadventor-regular.otf) intact.
In `@web_interface/blueprints/api_v3.py`:
- Around line 5643-5645: There are two Flask view functions both named
delete_font causing an endpoint name collision; locate the legacy stub
delete_font (the one without implementation) and remove it or rename its
function to a unique name and update any references; ensure only the intended
DELETE handler (the fully implemented delete_font for route
'/fonts/<font_family>') remains registered so Flask will no longer raise an
AssertionError on startup.
- Around line 5550-5559: The code converts the query param directly with size =
int(request.args.get('size', 12)), which raises a ValueError for non-integer
input and causes a 500; wrap the conversion in a try/except ValueError (or use a
safe integer parsing helper) when reading request.args.get('size', 12) and
return a 400 JSON error (e.g., {'status':'error','message':'Invalid font size'})
on parse failure before the existing size range check so invalid inputs produce
a 400 instead of a 500.
- Around line 5664-5688: The code currently uses the user-controlled font_family
directly to build filesystem paths (variables font_family, potential_path,
filepath), allowing path traversal; fix by validating/sanitizing font_family
before any filesystem access: reject values containing path separators or '..'
and allow only a safe whitelist of characters (e.g., alphanumeric, hyphen,
underscore) or percent-encoded names; when constructing potential_path or
filepath, resolve the resulting Path and ensure it is inside fonts_dir (compare
potential_path.resolve().startswith(fonts_dir.resolve()) or use
Path.is_relative_to) before calling exists()/is_file()/unlink(); also avoid
using the empty-extension branch that could match directories and ensure
fonts_dir exists before listing it (os.listdir).
- Around line 5561-5574: The font_filename from the /fonts/preview endpoint is
user-controlled and can include path traversal; to fix, canonicalize and
validate it before building font_path: reject if it contains path separators or
.. (e.g., ensure font_filename == Path(font_filename).name), restrict/validate
extension against allowed list ('.ttf','.otf','.bdf'), then resolve the
constructed path and verify it is a descendant of fonts_dir (compare
resolved_path.resolve().is_relative_to(fonts_dir.resolve()) or equivalent)
before opening; if any check fails, return the existing 404/error response. Use
the existing variables font_filename, fonts_dir, font_path and the route handler
for patching.
In `@web_interface/templates/v3/partials/fonts.html`:
- Around line 490-518: font names/display names from fontCatalog are being
injected into container.innerHTML and inline onclick handlers
(deleteFont('${font.name}')), which allows stored XSS; instead build the list
using DOM APIs or safely-escaped data attributes and attach click handlers via
addEventListener. Locate the fontEntries mapping and the container.innerHTML
assignment, stop concatenating unescaped strings (displayName, name, fontType),
render each entry by creating elements (e.g., div, span, button) and set text
via textContent, or set a data-font-name attribute with a safely-encoded value
and bind the delete action with document.querySelectorAll(...).forEach(btn =>
btn.addEventListener('click', () => deleteFont(btn.dataset.fontName))). Ensure
system font badge also uses textContent and never interpolates raw user strings
into HTML.
- Around line 526-529: Move the declaration const baseUrl =
window.location.origin out of loadFontData() and into the IIFE/shared scope
where fontCatalog, fontTokens, fontOverrides, and selectedFontFiles are defined
so that deleteFont(), addFontOverride(), deleteFontOverride(),
updateFontPreview(), and uploadSelectedFonts() can reference it without a
ReferenceError; then remove the duplicate const baseUrl declaration inside
loadFontData() to avoid shadowing.
🧹 Nitpick comments (1)
web_interface/blueprints/api_v3.py (1)
5523-5527: Log cache invalidation failures instead of silently swallowing them.Silent failures make remote debugging harder on Raspberry Pi.
📝 Proposed logging
- except (ImportError, Exception): - pass # Cache clearing failed, but file was saved + except (ImportError, Exception) as exc: + logger.warning("[FONTS] Failed to clear fonts_catalog cache: %s", exc)- except (ImportError, Exception): - pass # Cache clearing failed, but file was deleted + except (ImportError, Exception) as exc: + logger.warning("[FONTS] Failed to clear fonts_catalog cache: %s", exc)As per coding guidelines, Use structured logging with context (e.g., “[NHL Recent]”) for logging messages.
Also applies to: 5694-5698
- Fix README.md typos and add language tags to code fences - Remove duplicate delete_font function causing Flask endpoint collision - Add safe integer parsing for size parameter in preview endpoint - Fix path traversal vulnerability in /fonts/preview endpoint - Fix path traversal vulnerability in /fonts/<family> DELETE endpoint - Fix XSS vulnerability in fonts.html by using DOM APIs instead of innerHTML - Move baseUrl to shared scope to fix ReferenceError in multiple functions Security improvements: - Validate font filenames reject path separators and '..' - Validate paths are within fonts_dir before file operations - Use textContent and data attributes instead of inline onclick handlers - Restrict file extensions to known font types Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@web_interface/blueprints/api_v3.py`:
- Around line 5548-5641: The handler allows unbounded user-controlled text which
can create arbitrarily large images; add fast input validation to cap rendered
size: enforce a max character count for text (e.g., MAX_TEXT_CHARS) and/or max
lines, then after measuring bbox (using temp_draw.textbbox) compute
img_width/img_height and reject if width, height, or total pixels exceed safe
limits (e.g., MAX_DIM and MAX_PIXELS) by returning
jsonify({'status':'error','message':'Requested image too large'}) with 400;
apply these checks near the top (before heavy rendering) and immediately after
bbox calculation (before allocating the final Image), referencing variables
text, size, temp_draw.textbbox, text_width/text_height, img_width/img_height,
padding and return early to avoid memory/CPU spikes on Raspberry Pi.
🧹 Nitpick comments (1)
web_interface/blueprints/api_v3.py (1)
5523-5527: Add logging and narrow the exception when cache invalidation fails.Swallowing all exceptions makes remote debugging harder; log unexpected failures with context and only ignore
ImportError.As per coding guidelines, Use structured logging with context (e.g., "[NHL Recent]") for logging messages.♻️ Proposed fix
- try: - from web_interface.cache import delete_cached - delete_cached('fonts_catalog') - except (ImportError, Exception): - pass # Cache clearing failed, but file was saved + try: + from web_interface.cache import delete_cached + delete_cached('fonts_catalog') + except ImportError: + pass + except Exception as exc: + logger.warning("[Fonts] Cache clear failed after upload: %s", exc, exc_info=True)- try: - from web_interface.cache import delete_cached - delete_cached('fonts_catalog') - except (ImportError, Exception): - pass # Cache clearing failed, but file was deleted + try: + from web_interface.cache import delete_cached + delete_cached('fonts_catalog') + except ImportError: + pass + except Exception as exc: + logger.warning("[Fonts] Cache clear failed after delete: %s", exc, exc_info=True)Also applies to: 5755-5759
Comment on lines
+5548
to
+5641
| font_filename = request.args.get('font', '') | ||
| text = request.args.get('text', 'Sample Text 123') | ||
| bg_color = request.args.get('bg', '000000') | ||
| fg_color = request.args.get('fg', 'ffffff') | ||
|
|
||
| # Safe integer parsing for size | ||
| try: | ||
| size = int(request.args.get('size', 12)) | ||
| except (ValueError, TypeError): | ||
| return jsonify({'status': 'error', 'message': 'Invalid font size'}), 400 | ||
|
|
||
| if not font_filename: | ||
| return jsonify({'status': 'error', 'message': 'Font filename required'}), 400 | ||
|
|
||
| # Validate size | ||
| if size < 4 or size > 72: | ||
| return jsonify({'status': 'error', 'message': 'Font size must be between 4 and 72'}), 400 | ||
|
|
||
| # Security: Validate font_filename to prevent path traversal | ||
| # Only allow alphanumeric, hyphen, underscore, and dot (for extension) | ||
| from pathlib import Path as PathLib | ||
| safe_name = PathLib(font_filename).name # Strip any directory components | ||
| if safe_name != font_filename or '..' in font_filename: | ||
| return jsonify({'status': 'error', 'message': 'Invalid font filename'}), 400 | ||
|
|
||
| # Validate extension | ||
| allowed_extensions = ['.ttf', '.otf', '.bdf'] | ||
| has_valid_ext = any(safe_name.lower().endswith(ext) for ext in allowed_extensions) | ||
| name_without_ext = safe_name.rsplit('.', 1)[0] if '.' in safe_name else safe_name | ||
|
|
||
| # Find the font file | ||
| fonts_dir = PROJECT_ROOT / "assets" / "fonts" | ||
| if not fonts_dir.exists(): | ||
| return jsonify({'status': 'error', 'message': 'Fonts directory not found'}), 404 | ||
|
|
||
| font_path = fonts_dir / safe_name | ||
|
|
||
| if not font_path.exists() and not has_valid_ext: | ||
| # Try finding by family name (without extension) | ||
| for ext in allowed_extensions: | ||
| potential_path = fonts_dir / f"{name_without_ext}{ext}" | ||
| if potential_path.exists(): | ||
| font_path = potential_path | ||
| break | ||
|
|
||
| # Final security check: ensure path is within fonts_dir | ||
| try: | ||
| font_path.resolve().relative_to(fonts_dir.resolve()) | ||
| except ValueError: | ||
| return jsonify({'status': 'error', 'message': 'Invalid font path'}), 400 | ||
|
|
||
| if not font_path.exists(): | ||
| return jsonify({'status': 'error', 'message': f'Font file not found: {font_filename}'}), 404 | ||
|
|
||
| # Parse colors | ||
| try: | ||
| bg_rgb = tuple(int(bg_color[i:i+2], 16) for i in (0, 2, 4)) | ||
| fg_rgb = tuple(int(fg_color[i:i+2], 16) for i in (0, 2, 4)) | ||
| except (ValueError, IndexError): | ||
| bg_rgb = (0, 0, 0) | ||
| fg_rgb = (255, 255, 255) | ||
|
|
||
| # Load font | ||
| font = None | ||
| if str(font_path).endswith('.bdf'): | ||
| # For BDF fonts, try using freetype via PIL BDF support or fallback | ||
| try: | ||
| import freetype | ||
| face = freetype.Face(str(font_path)) | ||
| face.set_pixel_sizes(0, size) | ||
| # For BDF, we'll render character by character | ||
| # This is a simplified approach - full BDF rendering is complex | ||
| font = ImageFont.load_default() | ||
| except Exception: | ||
| font = ImageFont.load_default() | ||
| else: | ||
| # TTF/OTF fonts | ||
| try: | ||
| font = ImageFont.truetype(str(font_path), size) | ||
| except Exception: | ||
| font = ImageFont.load_default() | ||
|
|
||
| # Calculate text size | ||
| temp_img = Image.new('RGB', (1, 1)) | ||
| temp_draw = ImageDraw.Draw(temp_img) | ||
| bbox = temp_draw.textbbox((0, 0), text, font=font) | ||
| text_width = bbox[2] - bbox[0] | ||
| text_height = bbox[3] - bbox[1] | ||
|
|
||
| # Create image with padding | ||
| padding = 10 | ||
| img_width = max(text_width + padding * 2, 100) | ||
| img_height = max(text_height + padding * 2, 30) | ||
|
|
There was a problem hiding this comment.
Bound preview text/image size to avoid Pi memory/CPU spikes.
text is user-controlled and can produce arbitrarily large images. Add hard caps and fail fast with a 400 to prevent resource exhaustion.
🛡️ Proposed fix
- text = request.args.get('text', 'Sample Text 123')
+ text = request.args.get('text', 'Sample Text 123')
+ max_chars = 200
+ if len(text) > max_chars:
+ return jsonify({'status': 'error', 'message': f'Text too long (max {max_chars} chars)'}), 400
@@
text_width = bbox[2] - bbox[0]
text_height = bbox[3] - bbox[1]
+ max_width = 1024
+ max_height = 256
+ if text_width > max_width or text_height > max_height:
+ return jsonify({'status': 'error', 'message': 'Preview text is too large'}), 400🧰 Tools
🪛 Ruff (0.14.14)
[warning] 5621-5621: Do not catch blind exception: Exception
(BLE001)
[warning] 5627-5627: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@web_interface/blueprints/api_v3.py` around lines 5548 - 5641, The handler
allows unbounded user-controlled text which can create arbitrarily large images;
add fast input validation to cap rendered size: enforce a max character count
for text (e.g., MAX_TEXT_CHARS) and/or max lines, then after measuring bbox
(using temp_draw.textbbox) compute img_width/img_height and reject if width,
height, or total pixels exceed safe limits (e.g., MAX_DIM and MAX_PIXELS) by
returning jsonify({'status':'error','message':'Requested image too large'}) with
400; apply these checks near the top (before heavy rendering) and immediately
after bbox calculation (before allocating the final Image), referencing
variables text, size, temp_draw.textbbox, text_width/text_height,
img_width/img_height, padding and return early to avoid memory/CPU spikes on
Raspberry Pi.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Plugin authors can use x-widget: "font-selector" in schemas to enable dynamic font selection that automatically shows all available fonts.
Summary by CodeRabbit
New Features
Documentation