feat(ui): implement color theme design system (issue #13) by demo9-labworksdev · Pull Request #90 · labworksdev/todo-app

demo9-labworksdev · 2026-04-07T16:47:21Z

Overview

Implements a professional color palette and consistent typography across the todo-app UI as specified in issue #13.

Changes

Design Tokens — Define CSS custom properties for colors, typography, spacing, and border radius
Base Styles — Apply tokens to HTML elements (body, headings, links, tables)
Component Styles — Button variants (primary, secondary, danger), input states with focus rings, badges, and layout utilities
Templates — Update index.html and login.html to use design system classes
Accessibility — WCAG 2.1 AA contrast compliance for all interactive elements

Acceptance Criteria

All color and typography values sourced from CSS custom properties
Page title, section headings, body text, and muted text are visually distinct
Primary, secondary, and danger button variants implemented and used consistently
Input fields have visible focus states meeting WCAG 2.1 AA contrast requirements
Links are distinguishable from body text without relying solely on color
No hardcoded hex values or font names outside of tokens.css

- Define CSS custom properties for colors, typography, and spacing - Create base styles for HTML elements (body, headings, links, tables) - Add component styles for buttons, inputs, badges, and layout - Update HTML templates to use design system tokens - Ensure WCAG 2.1 AA contrast compliance for accessibility - Add login page with consistent styling

ai-coding-guardrails

I've got 7 comments for you to consider

The PR title and description are not accurate. Here are my suggestions:

Proposed title: feat: implement color theme design system with authentication and security hardening (issue #13)

Proposed description:

Implements a professional color palette and consistent typography across the todo-app UI as specified in issue #13, alongside a comprehensive authentication, session management, and CSRF protection system.

Design Tokens — Define CSS custom properties for colors, typography, spacing, and border radius (tokens.css)
Base Styles — Apply tokens to HTML elements: body, headings, links, tables (base.css)
Component Styles — Button variants (primary, secondary, danger), input states with focus rings, badges, and layout utilities (components.css)
Templates — Update index.html and login.html to use design system classes, add CSRF tokens, and convert toggle/delete actions from GET links to POST forms
Authentication — Add PBKDF2-SHA256 password verification, server-side session tokens signed with itsdangerous, IP-based rate limiting, and an admin_required decorator in app.py
Password Hash Script — New generate_password_hash.py utility with two-factor credential verification, rate limiting, and password strength validation
Accessibility — WCAG 2.1 AA contrast compliance for interactive elements

More details

While the PR title and description focus solely on the design system, the changes also include significant backend work: app.py gains a full authentication and authorization layer (PBKDF2-SHA256, CSRF protection, hardened session cookies, per-resource ownership checks), and a new generate_password_hash.py script is added for admin credential setup. The HTML templates are also updated with security improvements beyond just styling.

Reviewed with 🤟 by Zenable

ai-coding-guardrails · 2026-04-07T16:48:16Z

+    if password != confirm:
+        print("Error: passwords do not match.", file=sys.stderr)
+        sys.exit(1)


The password confirmation comparison uses != (non-constant-time) rather than hmac.compare_digest. While the attacker controls both inputs here (it's their own password), this is inconsistent with the security posture of the rest of the script and could be flagged in a security audit. Use a constant-time comparison:

Suggested change

if password != confirm:

print("Error: passwords do not match.", file=sys.stderr)

sys.exit(1)

if not hmac.compare_digest(password.encode(), confirm.encode()):

Why did I show this?

Category: security
Comment Quality: high

Based on general best practices

Tools used:

list_changed_files, {'pattern': {'type': 'string', 'value': '**/*.py'}}

list_changed_files, {'pattern': '**/*.py'}

list_changed_files, {'pattern': '**/*.py'}

get_file_lines, {'file_path': 'src/todo_app/app.py', 'start_line': 1, 'end_line': 200}

ai-coding-guardrails · 2026-04-07T16:48:16Z

+    border: 1px solid var(--color-border);
+    border-radius: var(--radius);
+    padding: var(--space-2) var(--space-3);
+    outline: none;


Setting outline: none on .input removes the browser's default focus indicator. While a custom focus style is applied via .input:focus using box-shadow, this only covers pointer/keyboard focus in modern browsers. For full accessibility (WCAG 2.1 AA), consider using outline: none only when a visible custom focus style is guaranteed, or prefer outline: 2px solid var(--color-primary); outline-offset: 2px; in .input:focus instead of (or in addition to) the box-shadow, to ensure focus is visible in high-contrast/forced-colors mode where box-shadow is suppressed.

Suggested change

outline: none;

outline: none;

And in .input:focus:

Suggested change

outline: none;

border-color: var(--color-primary);

box-shadow: 0 0 0 2px var(--color-primary-focus);

outline: 2px solid var(--color-primary);

outline-offset: 2px;

Why did I show this?

Category: accessibility
Comment Quality: high

Influenced by requirements:

Ensure authorization logic is secure

Tools used:

get_file_lines, {'file_path': 'src/static/css/tokens.css', 'start_line': '1', 'end_line': '200'}

get_file_lines, {'file_path': 'src/templates/index.html', 'start_line': '1', 'end_line': '100'}

get_file_lines, {'file_path': 'src/static/css/base.css', 'start_line': '1', 'end_line': '100'}

get_file_lines, {'file_path': 'src/templates/login.html', 'start_line': '1', 'end_line': '100'}

ai-coding-guardrails · 2026-04-07T16:48:16Z

+            <p class="subtitle">Sign in to manage your todos.</p>
+
+            {% if error %}
+            <p class="error-msg">{{ error }}</p>


The {{ error }} variable is rendered unescaped in Jinja2 by default — actually Jinja2 does auto-escape in Flask when the template extension is .html, so this is safe. However, verify that the error value passed from the view is a plain string and not an object that could bypass auto-escaping (e.g., via Markup()). If the error message is ever constructed from user-supplied input (e.g., echoing back a username), this could become an XSS vector. Ensure the server-side code never passes unsanitized user input directly into the error context variable.

Why did I show this?

Category: security
Comment Quality: high

Based on general best practices

Tools used:

get_file_lines, {'file_path': 'static/css/tokens.css', 'start_line': 1, 'end_line': 200}

list_changed_files, {'pattern': '**/*.css'}

get_file_lines, {'file_path': 'src/static/css/tokens.css', 'start_line': 1, 'end_line': 200}

get_file_lines, {'file_path': 'src/static/css/base.css', 'start_line': 1, 'end_line': 200}

get_file_lines, {'file_path': 'src/static/css/components.css', 'start_line': 1, 'end_line': 200}

ai-coding-guardrails · 2026-04-07T16:48:16Z

+        if not _validate_csrf():
+            error = "Invalid request."


The CSRF check runs before the rate-limit check. This means an attacker can probe whether a CSRF token is valid without consuming a rate-limit attempt, and — more critically — a valid CSRF token from a legitimate session can be used to bypass the rate limit entirely (since CSRF validation passes first and the rate-limit branch is never reached on a bad-token request).

The rate-limit check should run before the CSRF check so that brute-force attempts are throttled regardless of token validity:

Suggested change

if not _validate_csrf():

error = "Invalid request."

if not _check_rate_limit(ip):

error = f"Too many failed attempts. Try again in {_LOGIN_LOCKOUT_MINUTES} minutes."

elif not _validate_csrf():

error = "Invalid request."

Why did I show this?

Category: security
Comment Quality: high

Based on general best practices

ai-coding-guardrails · 2026-04-07T16:48:16Z

+    if failures >= _LOGIN_MAX_ATTEMPTS and datetime.datetime.utcnow() < lockout_until:
+        return False
+    if datetime.datetime.utcnow() >= lockout_until:
+        # Lockout expired — reset the counter.
+        del _failed_logins[ip]
+    return True


There is a TOCTOU race in _check_rate_limit: datetime.datetime.utcnow() is called twice, so the lockout expiry check and the reset check can disagree if the clock advances between calls. More importantly, when failures < _LOGIN_MAX_ATTEMPTS but lockout_until has already passed, the function returns True without cleaning up the stale entry — the stale record accumulates until the next call where utcnow() >= lockout_until. Use a single now snapshot:

Suggested change

if failures >= _LOGIN_MAX_ATTEMPTS and datetime.datetime.utcnow() < lockout_until:

return False

if datetime.datetime.utcnow() >= lockout_until:

# Lockout expired — reset the counter.

del _failed_logins[ip]

return True

now = datetime.datetime.utcnow()

failures, lockout_until = entry

if now >= lockout_until:

del _failed_logins[ip]

return True

if failures >= _LOGIN_MAX_ATTEMPTS:

return False

return True

Why did I show this?

Category: security
Comment Quality: high

Based on general best practices

ai-coding-guardrails · 2026-04-07T16:48:16Z

 @app.route("/")
+@admin_required


The index route fetches all todos with no WHERE owner = ? filter. Any authenticated admin can see every row in the database, not just their own. This is inconsistent with the ownership model enforced on write operations and _get_authorized_todo.

Suggested change

@app.route("/")

@admin_required

@app.route("/")

@admin_required

def index() -> str:

"""Display all todos."""

conn = get_db()

todos = conn.execute(

"SELECT * FROM todos WHERE owner = ? ORDER BY created_at DESC",

(session["username"],),

).fetchall()

conn.close()

return render_template("index.html", todos=todos)

(The relevant lines in the whole file are the todos = conn.execute(...) call inside index().)

Why did I show this?

Category: security
Comment Quality: high

Influenced by requirements:

Ensure authorization logic is secure

+        sys.exit(1)
+    if len(expected) < _MIN_SECRET_LENGTH:
+        print(
+            f"Error: SETUP_SECRET must be at least {_MIN_SECRET_LENGTH} characters.",


+        sys.exit(1)
+    if len(expected_token) < _MIN_SECRET_LENGTH:
+        print(
+            f"Error: SETUP_TOKEN must be at least {_MIN_SECRET_LENGTH} characters.",


+    if strength_errors:
+        print("Error: password does not meet requirements:", file=sys.stderr)
+        for err in strength_errors:
+            print(f"  - {err}", file=sys.stderr)


- Move Flask imports to top of app.py (lint error E402) - Replace deprecated datetime.utcnow() with datetime.now(timezone.utc) - Fix broken issue link in spec document - Add cspell ignore directives for WCAG abbreviations - Add explicit permissions to GitHub workflow jobs - Add required environment variables to Docker test

JonZeolla · 2026-04-07T20:17:58Z

/zenable

ai-coding-guardrails

I've got 4 comments for you to consider

The PR title and description are not accurate. Here are my suggestions:

Proposed title: feat(ui): implement color theme design system with auth layer and security hardening (issue #13)

Proposed description:

Implements a professional color palette and consistent typography across the todo-app UI as specified in issue #13, alongside a significant authentication/authorization overhaul and security hardening.

Design Tokens — tokens.css, base.css, components.css define CSS custom properties for colors, typography, spacing, and border radius
Templates — index.html and login.html updated to use design system classes, semantic markup, and CSRF-protected POST forms for toggle/delete
Auth & Security — app.py gains PBKDF2-SHA256 password verification, itsdangerous-signed session tokens, CSRF protection, IP-based rate limiting, admin_required decorator, and per-resource ownership checks
Password Utility — New scripts/generate_password_hash.py for generating PBKDF2-SHA256 hashes with rate limiting and strength validation
CI — Workflow jobs get permissions: contents: read; integration tests updated to supply required env vars (SECRET_KEY, ADMIN_USERNAME, ADMIN_PASSWORD_HASH)
Spec — docs/specs/issue-13-color-theme.md added as design system reference

More details

The PR description focuses solely on the design system, but the diff includes substantial backend changes: a full authentication layer in app.py, a password hash generation script, CI permission hardening, and integration test fixes for a new SECRET_KEY requirement. A test_logs.txt CI log and a .claude/settings.local.json file containing a local filesystem path were also inadvertently committed and should not be tracked in version control.

Did you know Zenable also supports GitLab SaaS and self-managed?

ai-coding-guardrails · 2026-04-07T20:19:11Z

+      "Bash(git remote:*)",
+      "Bash(gh issue:*)",
+      "mcp__zenable__conformance_check",
+      "Bash(git -C c:/Users/student/code/todo-app checkout -b feature/issue-13-color-theme)",


This file contains an absolute local filesystem path (c:/Users/student/code/todo-app) which is developer-machine-specific and should not be committed to the repository. This leaks the local environment structure and will break for any other contributor. The .claude/settings.local.json file should be added to .gitignore rather than committed.

Why did I show this?

Category: security
Comment Quality: high

Influenced by requirements:

No Clear-Text Logging of Sensitive Information

Tools used:

list_changed_files, {'pattern': {'type': 'string', 'value': '**/*.css'}}

list_changed_files, {'pattern': '**/*.css'}

get_file_lines, {'file_path': 'src/static/css/tokens.css', 'start_line': 1, 'end_line': 200}

get_file_lines, {'file_path': 'src/static/css/base.css', 'start_line': 1, 'end_line': 200}

get_file_lines, {'file_path': 'src/static/css/components.css', 'start_line': 1, 'end_line': 200}

get_file_lines, {'file_path': 'src/templates/index.html', 'start_line': 1, 'end_line': 200}

get_file_lines, {'file_path': 'src/templates/login.html', 'start_line': 1, 'end_line': 200}

ai-coding-guardrails · 2026-04-07T20:19:11Z

+    if failures >= _LOGIN_MAX_ATTEMPTS and datetime.datetime.now(datetime.timezone.utc) < lockout_until:
+        return False
+    if datetime.datetime.now(datetime.timezone.utc) >= lockout_until:
+        # Lockout expired — reset the counter.
+        del _failed_logins[ip]
+    return True


There is a logic gap in _check_rate_limit: when failures >= _LOGIN_MAX_ATTEMPTS but the lockout has expired, the first if returns False (locked out) before the second if can clean up and return True. The expiry-reset branch is only reached when the first condition is not met (i.e., failures < _LOGIN_MAX_ATTEMPTS), so a fully-locked-out IP is permanently blocked even after the lockout window passes.

The fix is to check expiry first:

Suggested change

if failures >= _LOGIN_MAX_ATTEMPTS and datetime.datetime.now(datetime.timezone.utc) < lockout_until:

return False

if datetime.datetime.now(datetime.timezone.utc) >= lockout_until:

# Lockout expired — reset the counter.

del _failed_logins[ip]

return True

failures, lockout_until = entry

now = datetime.datetime.now(datetime.timezone.utc)

if now >= lockout_until:

# Lockout expired — reset the counter.

del _failed_logins[ip]

return True

if failures >= _LOGIN_MAX_ATTEMPTS:

return False

return True

Why did I show this?

Category: bug
Comment Quality: high

Based on general best practices

ai-coding-guardrails · 2026-04-07T20:19:11Z

+2026-04-07T16:50:58.0421940Z >               assert process.returncode == 0, (
+2026-04-07T16:50:58.0424041Z                     f"Expected exit code 0, got: {process.returncode}\n"
+2026-04-07T16:50:58.0424669Z                     f"stderr: {process.stderr.decode()}"
+2026-04-07T16:50:58.0425040Z                 )
+2026-04-07T16:50:58.0425365Z E               AssertionError: Expected exit code 0, got: 3


The integration test test_docker_image is asserting that the container exits with code 0 when run without arguments (i.e., starting the server). However, the app now enforces that SECRET_KEY must be set before starting, causing it to exit with code 3. The test needs to be updated to either:

Pass a SECRET_KEY environment variable when running the container (e.g., -e SECRET_KEY=test), or

Change the assertion to accept a non-zero exit code when SECRET_KEY is not provided, since that is now the expected behavior.

This is a real test failure blocking CI caused by the SECRET_KEY enforcement introduced in app.py.

Why did I show this?

Category: bug
Comment Quality: high

Influenced by requirements:

Cryptographic Secret Keys Must Not Fall Back to Ephemeral Values

demo9-labworksdev requested a review from JonZeolla as a code owner April 7, 2026 16:47

ai-coding-guardrails bot reviewed Apr 7, 2026

View reviewed changes

github-advanced-security AI found potential problems Apr 7, 2026

View reviewed changes

demo9-labworksdev changed the title ~~feat: implement color theme design system (issue #13)~~ feat(ui): implement color theme design system (issue #13) Apr 7, 2026

ai-coding-guardrails bot reviewed Apr 7, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ui): implement color theme design system (issue #13)#90

feat(ui): implement color theme design system (issue #13)#90
demo9-labworksdev wants to merge 2 commits intomainfrom
feature/issue-13-color-theme

demo9-labworksdev commented Apr 7, 2026

Uh oh!

ai-coding-guardrails bot left a comment

Uh oh!

ai-coding-guardrails bot Apr 7, 2026

Uh oh!

ai-coding-guardrails bot Apr 7, 2026

Uh oh!

ai-coding-guardrails bot Apr 7, 2026

Uh oh!

ai-coding-guardrails bot Apr 7, 2026

Uh oh!

ai-coding-guardrails bot Apr 7, 2026

Uh oh!

ai-coding-guardrails bot Apr 7, 2026

Uh oh!

JonZeolla commented Apr 7, 2026

Uh oh!

ai-coding-guardrails bot left a comment

Uh oh!

ai-coding-guardrails bot Apr 7, 2026

Uh oh!

ai-coding-guardrails bot Apr 7, 2026

Uh oh!

ai-coding-guardrails bot Apr 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-    outline: none;
+    border-color: var(--color-primary);
+    box-shadow: 0 0 0 2px var(--color-primary-focus);
+    outline: 2px solid var(--color-primary);
+    outline-offset: 2px;

-@app.route("/")
-@admin_required
+@app.route("/")
+@admin_required
+def index() -> str:
+    """Display all todos."""
+    conn = get_db()
+    todos = conn.execute(
+        "SELECT * FROM todos WHERE owner = ? ORDER BY created_at DESC",
+        (session["username"],),
+    ).fetchall()
+    conn.close()
+    return render_template("index.html", todos=todos)

Conversation

demo9-labworksdev commented Apr 7, 2026

Overview

Changes

Acceptance Criteria

Related

Uh oh!

ai-coding-guardrails bot left a comment

Choose a reason for hiding this comment

Uh oh!

ai-coding-guardrails bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

ai-coding-guardrails bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

ai-coding-guardrails bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

ai-coding-guardrails bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

ai-coding-guardrails bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

ai-coding-guardrails bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

JonZeolla commented Apr 7, 2026

Uh oh!

ai-coding-guardrails bot left a comment

Choose a reason for hiding this comment

Uh oh!

ai-coding-guardrails bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

ai-coding-guardrails bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

ai-coding-guardrails bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants