Fix default mutable arguments
`attributes: list[HTMLAttribute] = []` and `children: list[HTMLTag] | HTMLTag = []` are a classic Python gotcha — the default is shared across all callers and could silently corrupt state if ever mutated in place. Changed 8 functions (`Component`, `Popover`, `A`, `Button`, `Div`, `Input`, `Form`, `Icon`) to use the `None` sentinel pattern, preventing future bugs and eliminating linter warnings.
This commit is contained in:
@@ -21,27 +21,21 @@ All component functions now return `SafeText` and are annotated accordingly. Red
|
|||||||
### Fragile A() URL resolution
|
### 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()`).
|
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()`).
|
||||||
|
|
||||||
## Incomplete
|
|
||||||
|
|
||||||
### Toast XSS vulnerability
|
### Toast XSS vulnerability
|
||||||
Custom string escaping for Alpine.js interpolation:
|
The vulnerable `Toast()` component (which used unsafe string escaping for
|
||||||
```python
|
Alpine.js interpolation) had no callers and was deleted entirely. Toast display
|
||||||
safe_message = message.replace("\\", "\\\\").replace("`", "\\`")
|
is handled by the existing event-driven pipeline: middleware → `HX-Trigger`
|
||||||
```
|
headers → `show-toast` CustomEvent → Alpine store.
|
||||||
Doesn't protect against all injection vectors (e.g., `})` could close the
|
|
||||||
Alpine expression early).
|
|
||||||
|
|
||||||
**Fix**: Use proper HTML escaping + JSON serialization for safe template interpolation.
|
### Default mutable arguments
|
||||||
|
All functions with mutable defaults (`attributes` and `children`) changed from `= []` to `| None = None` with `or []` conversion in the body.
|
||||||
|
|
||||||
|
What was fixed: `attributes: list[HTMLAttribute] = []` and `children: list[HTMLTag] | HTMLTag = []` are a classic Python gotcha — the default is shared across all callers and could silently corrupt state if ever mutated in place. Changed 8 functions (`Component`, `Popover`, `A`, `Button`, `Div`, `Input`, `Form`, `Icon`) to use the `None` sentinel pattern, preventing future bugs and eliminating linter warnings.
|
||||||
|
|
||||||
|
## Incomplete
|
||||||
|
|
||||||
### No tests
|
### No tests
|
||||||
Zero test coverage for the entire component system.
|
Zero test coverage for the entire component system.
|
||||||
|
|
||||||
**Fix**: Add unit tests for each component function — basic rendering, edge cases,
|
**Fix**: Add unit tests for each component function — basic rendering, edge cases,
|
||||||
and cache hit/miss verification.
|
and cache hit/miss verification.
|
||||||
|
|
||||||
### Default mutable arguments
|
|
||||||
`attributes: list[HTMLAttribute] = []` is a classic Python gotcha (though harmless
|
|
||||||
here since the list is only read, never mutated in place).
|
|
||||||
|
|
||||||
**Fix**: Use `attributes: list[HTMLAttribute] | None = None` and convert to `[]`
|
|
||||||
inside the function body.
|
|
||||||
|
|||||||
+30
-35
@@ -36,11 +36,13 @@ def enable_cache():
|
|||||||
|
|
||||||
|
|
||||||
def Component(
|
def Component(
|
||||||
attributes: list[HTMLAttribute] = [],
|
attributes: list[HTMLAttribute] | None = None,
|
||||||
children: list[HTMLTag] | HTMLTag = [],
|
children: list[HTMLTag] | HTMLTag | None = None,
|
||||||
template: str = "",
|
template: str = "",
|
||||||
tag_name: str = "",
|
tag_name: str = "",
|
||||||
) -> SafeText:
|
) -> SafeText:
|
||||||
|
attributes = attributes or []
|
||||||
|
children = children or []
|
||||||
if not tag_name and not template:
|
if not tag_name and not template:
|
||||||
raise ValueError("One of template or tag_name is required.")
|
raise ValueError("One of template or tag_name is required.")
|
||||||
if isinstance(children, str):
|
if isinstance(children, str):
|
||||||
@@ -75,9 +77,11 @@ def Popover(
|
|||||||
popover_content: str,
|
popover_content: str,
|
||||||
wrapped_content: str = "",
|
wrapped_content: str = "",
|
||||||
wrapped_classes: str = "",
|
wrapped_classes: str = "",
|
||||||
children: list[HTMLTag] = [],
|
children: list[HTMLTag] | None = None,
|
||||||
attributes: list[HTMLAttribute] = [],
|
attributes: list[HTMLAttribute] | None = None,
|
||||||
) -> str:
|
) -> str:
|
||||||
|
attributes = attributes or []
|
||||||
|
children = children or []
|
||||||
if not wrapped_content and not children:
|
if not wrapped_content and not children:
|
||||||
raise ValueError("One of wrapped_content or children is required.")
|
raise ValueError("One of wrapped_content or children is required.")
|
||||||
id = randomid(content=f"{wrapped_content}:{popover_content}:{wrapped_classes}")
|
id = randomid(content=f"{wrapped_content}:{popover_content}:{wrapped_classes}")
|
||||||
@@ -127,8 +131,8 @@ def PopoverTruncated(
|
|||||||
|
|
||||||
|
|
||||||
def A(
|
def A(
|
||||||
attributes: list[HTMLAttribute] = [],
|
attributes: list[HTMLAttribute] | None = None,
|
||||||
children: list[HTMLTag] | HTMLTag = [],
|
children: list[HTMLTag] | HTMLTag | None = None,
|
||||||
url_name: str | None = None,
|
url_name: str | None = None,
|
||||||
href: str | None = None,
|
href: str | None = None,
|
||||||
) -> SafeText:
|
) -> SafeText:
|
||||||
@@ -139,6 +143,8 @@ def A(
|
|||||||
- url_name: URL pattern name, resolved via reverse()
|
- url_name: URL pattern name, resolved via reverse()
|
||||||
- href: Literal path string passed through as-is
|
- href: Literal path string passed through as-is
|
||||||
"""
|
"""
|
||||||
|
attributes = attributes or []
|
||||||
|
children = children or []
|
||||||
if url_name is not None and href is not None:
|
if url_name is not None and href is not None:
|
||||||
raise ValueError("Provide exactly one of 'url_name' or 'href', not both.")
|
raise ValueError("Provide exactly one of 'url_name' or 'href', not both.")
|
||||||
|
|
||||||
@@ -153,12 +159,14 @@ def A(
|
|||||||
|
|
||||||
|
|
||||||
def Button(
|
def Button(
|
||||||
attributes: list[HTMLAttribute] = [],
|
attributes: list[HTMLAttribute] | None = None,
|
||||||
children: list[HTMLTag] | HTMLTag = [],
|
children: list[HTMLTag] | HTMLTag | None = None,
|
||||||
size: str = "base",
|
size: str = "base",
|
||||||
icon: bool = False,
|
icon: bool = False,
|
||||||
color: str = "blue",
|
color: str = "blue",
|
||||||
) -> SafeText:
|
) -> SafeText:
|
||||||
|
attributes = attributes or []
|
||||||
|
children = children or []
|
||||||
return Component(
|
return Component(
|
||||||
template="cotton/button.html",
|
template="cotton/button.html",
|
||||||
attributes=attributes
|
attributes=attributes
|
||||||
@@ -173,17 +181,21 @@ def Button(
|
|||||||
|
|
||||||
|
|
||||||
def Div(
|
def Div(
|
||||||
attributes: list[HTMLAttribute] = [],
|
attributes: list[HTMLAttribute] | None = None,
|
||||||
children: list[HTMLTag] | HTMLTag = [],
|
children: list[HTMLTag] | HTMLTag | None = None,
|
||||||
) -> SafeText:
|
) -> SafeText:
|
||||||
|
attributes = attributes or []
|
||||||
|
children = children or []
|
||||||
return Component(tag_name="div", attributes=attributes, children=children)
|
return Component(tag_name="div", attributes=attributes, children=children)
|
||||||
|
|
||||||
|
|
||||||
def Input(
|
def Input(
|
||||||
type: str = "text",
|
type: str = "text",
|
||||||
attributes: list[HTMLAttribute] = [],
|
attributes: list[HTMLAttribute] | None = None,
|
||||||
children: list[HTMLTag] | HTMLTag = [],
|
children: list[HTMLTag] | HTMLTag | None = None,
|
||||||
) -> SafeText:
|
) -> SafeText:
|
||||||
|
attributes = attributes or []
|
||||||
|
children = children or []
|
||||||
return Component(
|
return Component(
|
||||||
tag_name="input", attributes=attributes + [("type", type)], children=children
|
tag_name="input", attributes=attributes + [("type", type)], children=children
|
||||||
)
|
)
|
||||||
@@ -192,9 +204,11 @@ def Input(
|
|||||||
def Form(
|
def Form(
|
||||||
action="",
|
action="",
|
||||||
method="get",
|
method="get",
|
||||||
attributes: list[HTMLAttribute] = [],
|
attributes: list[HTMLAttribute] | None = None,
|
||||||
children: list[HTMLTag] | HTMLTag = [],
|
children: list[HTMLTag] | HTMLTag | None = None,
|
||||||
) -> SafeText:
|
) -> SafeText:
|
||||||
|
attributes = attributes or []
|
||||||
|
children = children or []
|
||||||
return Component(
|
return Component(
|
||||||
tag_name="form",
|
tag_name="form",
|
||||||
attributes=attributes + [("action", action), ("method", method)],
|
attributes=attributes + [("action", action), ("method", method)],
|
||||||
@@ -204,8 +218,9 @@ def Form(
|
|||||||
|
|
||||||
def Icon(
|
def Icon(
|
||||||
name: str,
|
name: str,
|
||||||
attributes: list[HTMLAttribute] = [],
|
attributes: list[HTMLAttribute] | None = None,
|
||||||
) -> SafeText:
|
) -> SafeText:
|
||||||
|
attributes = attributes or []
|
||||||
try:
|
try:
|
||||||
result = Component(template=f"cotton/icon/{name}.html", attributes=attributes)
|
result = Component(template=f"cotton/icon/{name}.html", attributes=attributes)
|
||||||
except TemplateDoesNotExist:
|
except TemplateDoesNotExist:
|
||||||
@@ -312,24 +327,4 @@ def PurchasePrice(purchase) -> SafeText:
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
def Toast(
|
|
||||||
message: str = "",
|
|
||||||
type: str = "info",
|
|
||||||
attributes: list = [],
|
|
||||||
):
|
|
||||||
valid_types = ["success", "error", "info"]
|
|
||||||
if type not in valid_types:
|
|
||||||
type = "info"
|
|
||||||
|
|
||||||
safe_message = message.replace("\\", "\\\\").replace("`", "\\`")
|
|
||||||
safe_type = type.replace("\\", "\\\\").replace("`", "\\`")
|
|
||||||
return Component(
|
|
||||||
tag_name="div",
|
|
||||||
attributes=[
|
|
||||||
("class", "hidden"),
|
|
||||||
("x-data", "toastStore"),
|
|
||||||
("x-init", f"addToast(`{safe_message}`, `{safe_type}`)"),
|
|
||||||
]
|
|
||||||
+ attributes,
|
|
||||||
children=[message],
|
|
||||||
)
|
|
||||||
|
|||||||
Reference in New Issue
Block a user