diff --git a/common/COMPONENT_IMPROVEMENTS.md b/common/COMPONENT_IMPROVEMENTS.md index 0ee77e6..50ef63c 100644 --- a/common/COMPONENT_IMPROVEMENTS.md +++ b/common/COMPONENT_IMPROVEMENTS.md @@ -15,17 +15,15 @@ - `games/templatetags/randomid.py` uses the same hash-based approach - Fixes: caching (Popover output now cacheable), page consistency, thread safety -### 1. Inconsistent return types (completed) +### Inconsistent return types All component functions now return `SafeText` and are annotated accordingly. Redundant `mark_safe()` wrappers removed from `LinkedPurchase()` and `NameWithIcon()`. -### 2. Fragile A() URL resolution -Tries `reverse(url)` first, then falls back to literal string. Uses `type(url) is str` -instead of `isinstance()`. Intentional but error-prone — a string matching a URL name -will be reversed, while one that doesn't pass through as-is. +### Fragile A() URL resolution +Replaced single `url` parameter with explicit `url_name` (URL pattern name resolved via `reverse()`) and `href` (literal path). Removed dead `Callable` type hint. `reverse()` now raises `NoReverseMatch` instead of silently falling back to literal text. Added mutual exclusion check — providing both parameters raises `ValueError`. Updated all 10 call sites across 6 view files and internal callers (`LinkedPurchase()`, `NameWithIcon()`). -**Fix**: Add explicit parameter like `url_name="view_game"` vs `href="/literal/path"`. +## Incomplete -### 3. Toast XSS vulnerability +### Toast XSS vulnerability Custom string escaping for Alpine.js interpolation: ```python safe_message = message.replace("\\", "\\\\").replace("`", "\\`") @@ -35,13 +33,13 @@ Alpine expression early). **Fix**: Use proper HTML escaping + JSON serialization for safe template interpolation. -### 4. No tests +### No tests Zero test coverage for the entire component system. **Fix**: Add unit tests for each component function — basic rendering, edge cases, and cache hit/miss verification. -### 5. Default mutable arguments +### Default mutable arguments `attributes: list[HTMLAttribute] = []` is a classic Python gotcha (though harmless here since the list is only read, never mutated in place). diff --git a/common/components.py b/common/components.py index 066175a..754a7ac 100644 --- a/common/components.py +++ b/common/components.py @@ -1,13 +1,13 @@ import hashlib import json from functools import lru_cache -from typing import Any, Callable +from typing import Any from django.conf import settings from django.template import TemplateDoesNotExist from django.template.defaultfilters import floatformat from django.template.loader import render_to_string -from django.urls import NoReverseMatch, reverse +from django.urls import reverse from django.utils.safestring import SafeText, mark_safe from common.utils import truncate @@ -129,27 +129,24 @@ def PopoverTruncated( def A( attributes: list[HTMLAttribute] = [], children: list[HTMLTag] | HTMLTag = [], - url: str | Callable[..., Any] = "", + url_name: str | None = None, + href: str | None = None, ) -> SafeText: """ - Returns the HTML tag "a". - "url" can either be: - - URL (string) - - path name passed to reverse() (string) - - function + Returns an anchor tag. + + Accepts one of two mutually-exclusive URL specifications: + - url_name: URL pattern name, resolved via reverse() + - href: Literal path string passed through as-is """ + if url_name is not None and href is not None: + raise ValueError("Provide exactly one of 'url_name' or 'href', not both.") + additional_attributes = [] - if url: - if type(url) is str: - try: - url_result = reverse(url) - except NoReverseMatch: - url_result = url - elif callable(url): - url_result = url() - else: - raise TypeError("'url' is neither str nor function.") - additional_attributes = [("href", url_result)] + if url_name is not None: + additional_attributes = [("href", reverse(url_name))] + elif href is not None: + additional_attributes = [("href", href)] return Component( tag_name="a", attributes=attributes + additional_attributes, children=children ) @@ -254,7 +251,7 @@ def LinkedPurchase(purchase: Purchase) -> SafeText: ), ], ) - return A(url=link, children=[a_content]) + return A(href=link, children=[a_content]) def NameWithIcon( @@ -299,7 +296,7 @@ def NameWithIcon( return ( A( - url=link, + href=link, children=[content], ) if create_link diff --git a/games/views/device.py b/games/views/device.py index 7c10a2d..5186f0a 100644 --- a/games/views/device.py +++ b/games/views/device.py @@ -36,7 +36,7 @@ def list_devices(request: HttpRequest) -> HttpResponse: else None ), "data": { - "header_action": A([], Button([], "Add device"), url="add_device"), + "header_action": A([], Button([], "Add device"), url_name="add_device"), "columns": [ "Name", "Type", diff --git a/games/views/game.py b/games/views/game.py index 71603f3..3cb8fb5 100644 --- a/games/views/game.py +++ b/games/views/game.py @@ -89,7 +89,7 @@ def list_games(request: HttpRequest, search_string: str = "") -> HttpResponse: ) ] ), - A([], Button([], "Add game"), url="add_game"), + A([], Button([], "Add game"), url_name="add_game"), ], attributes=[("class", "flex justify-between")], ), @@ -274,7 +274,7 @@ def view_game(request: HttpRequest, game_id: int) -> HttpResponse: "header_action": Div( children=[ A( - url="add_session", + url_name="add_session", children=Button( icon=True, size="xs", @@ -282,7 +282,7 @@ def view_game(request: HttpRequest, game_id: int) -> HttpResponse: ), ), A( - url=reverse( + href=reverse( "list_sessions_start_session_from_session", args=[last_session.pk], ), diff --git a/games/views/platform.py b/games/views/platform.py index b308cbf..cc675b7 100644 --- a/games/views/platform.py +++ b/games/views/platform.py @@ -37,7 +37,7 @@ def list_platforms(request: HttpRequest) -> HttpResponse: else None ), "data": { - "header_action": A([], Button([], "Add platform"), url="add_platform"), + "header_action": A([], Button([], "Add platform"), url_name="add_platform"), "columns": [ "Name", "Icon", diff --git a/games/views/playevent.py b/games/views/playevent.py index b16f65b..2151983 100644 --- a/games/views/playevent.py +++ b/games/views/playevent.py @@ -78,7 +78,7 @@ def create_playevent_tabledata( for row in row_list ] return { - "header_action": A([], Button([], "Add play event"), url="add_playevent"), + "header_action": A([], Button([], "Add play event"), url_name="add_playevent"), "columns": list(filtered_column_list), "rows": filtered_row_list, } diff --git a/games/views/purchase.py b/games/views/purchase.py index 92f4dac..370f886 100644 --- a/games/views/purchase.py +++ b/games/views/purchase.py @@ -100,7 +100,7 @@ def list_purchases(request: HttpRequest) -> HttpResponse: else None ), "data": { - "header_action": A([], Button([], "Add purchase"), url="add_purchase"), + "header_action": A([], Button([], "Add purchase"), url_name="add_purchase"), "columns": [ "Name", "Type", diff --git a/games/views/session.py b/games/views/session.py index 9ffb2d4..1aba2b0 100644 --- a/games/views/session.py +++ b/games/views/session.py @@ -81,7 +81,7 @@ def list_sessions(request: HttpRequest, search_string: str = "") -> HttpResponse Div( children=[ A( - url="add_session", + url_name="add_session", children=Button( icon=True, size="xs", @@ -89,7 +89,7 @@ def list_sessions(request: HttpRequest, search_string: str = "") -> HttpResponse ), ), A( - url=reverse( + href=reverse( "list_sessions_start_session_from_session", args=[last_session.pk], ), diff --git a/tests/test_components.py b/tests/test_components.py index 38ce12c..92620b8 100644 --- a/tests/test_components.py +++ b/tests/test_components.py @@ -376,9 +376,24 @@ class ComponentReturnTypeTest(unittest.TestCase): self.assertIsInstance(result, SafeText) def test_a_literal_href(self): - result = components.A([], "x", url="/literal/path") + result = components.A([], "x", href="/literal/path") self.assertIn('href="/literal/path"', result) + def test_a_url_name_reversed(self): + from unittest.mock import patch + with patch("common.components.reverse", return_value="/resolved/url"): + result = components.A([], "link", url_name="some_name") + self.assertIn('href="/resolved/url"', result) + + def test_a_no_url_or_href(self): + result = components.A([], "link") + self.assertIn('link', result) + self.assertNotIn('href=', result) + + def test_a_both_url_name_and_href_raises(self): + with self.assertRaises(ValueError): + components.A(href="/path", url_name="some_name") + def test_button_returns_safe_text(self): result = components.Button([], "click") self.assertIsInstance(result, SafeText)