diff --git a/common/COMPONENT_IMPROVEMENTS.md b/common/COMPONENT_IMPROVEMENTS.md index 844adc0..c41465b 100644 --- a/common/COMPONENT_IMPROVEMENTS.md +++ b/common/COMPONENT_IMPROVEMENTS.md @@ -9,30 +9,26 @@ - Only caches `template` path calls; `tag_name` calls are already nanosecond string ops - Verified working: identical calls return identical output, different inputs produce separate cache entries -## Pending +### Non-deterministic IDs +`randomid()` was replaced with `hashlib.sha1(content_hash.encode()).hexdigest()[:10]` for deterministic ID generation. +- `Popover()` passes content hash (`wrapped_content:popover_content:wrapped_classes`) so IDs are deterministic per unique content +- `games/templatetags/randomid.py` uses the same hash-based approach +- Fixes: caching (Popover output now cacheable), page consistency, thread safety -### 1. Non-deterministic IDs -`randomid()` uses `random.choices()` producing unique IDs every call. Breaks: -- Caching (can't cache Popover output because IDs change between requests) -- Page consistency (same content produces different HTML across requests) -- Thread safety of the `random` module for ID generation - -**Fix**: Replace with `hashlib.sha1(content_hash.encode()).hexdigest()[:10]` based on deterministic content hash. - -### 2. Inconsistent return types +### 1. Inconsistent return types `Div()`/`A()`/`Button()` return `str`, but `LinkedPurchase()`/`NameWithIcon()` return `SafeText`. Forces callers to remember `mark_safe()` wrapping. **Fix**: Standardize — all component functions should return the same type. -### 3. Fragile A() URL resolution +### 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. **Fix**: Add explicit parameter like `url_name="view_game"` vs `href="/literal/path"`. -### 4. Toast XSS vulnerability +### 3. Toast XSS vulnerability Custom string escaping for Alpine.js interpolation: ```python safe_message = message.replace("\\", "\\\\").replace("`", "\\`") @@ -42,13 +38,13 @@ Alpine expression early). **Fix**: Use proper HTML escaping + JSON serialization for safe template interpolation. -### 5. No tests +### 4. 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. -### 6. Default mutable arguments +### 5. 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 bf4244f..9f76b3b 100644 --- a/common/components.py +++ b/common/components.py @@ -1,7 +1,6 @@ +import hashlib import json from functools import lru_cache -from random import choices as random_choices -from string import ascii_lowercase from typing import Any, Callable from django.conf import settings @@ -63,8 +62,13 @@ def Component( return mark_safe(tag) -def randomid(seed: str = "", length: int = 10) -> str: - return seed + "".join(random_choices(ascii_lowercase, k=length)) +def randomid(seed: str = "", content: str = "", length: int = 10) -> str: + if not seed and not content: + return seed + hash_input = f"{seed}:{content}" if seed else content + content_hash = hashlib.sha1(hash_input.encode()).hexdigest() + base = content_hash[:length] if not seed else content_hash[:max(0, length - len(seed))] + return seed + base def Popover( @@ -76,7 +80,7 @@ def Popover( ) -> str: if not wrapped_content and not children: raise ValueError("One of wrapped_content or children is required.") - id = randomid() + id = randomid(content=f"{wrapped_content}:{popover_content}:{wrapped_classes}") return Component( attributes=attributes + [ diff --git a/games/templatetags/randomid.py b/games/templatetags/randomid.py index 86beaa3..8b571cb 100644 --- a/games/templatetags/randomid.py +++ b/games/templatetags/randomid.py @@ -1,5 +1,4 @@ -import random -import string +import hashlib from django import template @@ -8,4 +7,7 @@ register = template.Library() @register.simple_tag def randomid(seed: str = "") -> str: - return str(hash(seed + "".join(random.choices(string.ascii_lowercase, k=10)))) + content_hash = hashlib.sha1(seed.encode()).hexdigest() + if seed: + return content_hash[:max(0, 10 - len(seed))] + seed + return content_hash[:10]