Pages

[T0ha]

No description provided.

[T0ha]

@copilot pls, review the PR

[T0ha]

@claude review, pls

[claude]

Claude finished @T0ha's task —— View job

---

PR Review Complete

This PR introduces a comprehensive Pages feature for managing dynamic content, along with significant UI/UX improvements and architectural refactorings. Below is my detailed review.

---

Overall Assessment

Scope: Large PR with 73 files changed (+3793/-980 lines). The changes are well-structured and include comprehensive test coverage.

Quality: Generally good code quality with proper separation of concerns, though there are several issues that should be addressed.

---

Critical Issues

1. Security: EEx Template Injection Vulnerability 🔴

Location: lib/bodhi_web/controllers/page_html.ex:18

def render_format_(%Page{format: :eex} = page, assigns),
  do: {:safe, EEx.eval_string(page.content, assigns: assigns)}

Issue: Using EEx.eval_string/2 on user-provided content creates a critical code injection vulnerability. An admin user could execute arbitrary Elixir code on the server.

Example Attack:

content = "<%= File.rm_rf!(\"/\") %>"  # Dangerous!

Recommendation:

• Either remove the :eex format entirely if not absolutely needed
• Or implement strict sandboxing/validation before rendering
• Consider using a safer templating approach like pre-compiled templates

2. Bug: Variable Shadowing in PageLive.Show 🔴

Location: lib/bodhi_web/live/page_live/show.ex:36-40

def mount(%{"id" => id}, _session, socket) do
  {:ok,
   socket
   |> assign(:page, %{title: "Show Page"})  # First assignment
   |> assign(:page, Pages.get_page!(id))}    # Shadows the first
end

Issue: The first assign(:page, %{title: "Show Page"}) is immediately overwritten, making it useless. This appears to be trying to set a page title for the layout, but it's incorrect.

Recommendation:

def mount(%{"id" => id}, _session, socket) do
  page = Pages.get_page!(id)
  {:ok,
   socket
   |> assign(:page_title, "Show Page")  # Use different key for layout title
   |> assign(:page, page)}
end

3. Hardcoded Bot Username 🟡

Location: lib/bodhi_web/controllers/page_controller.ex:23

|> assign(:bot_username, "compassion_lama_bot")

Issue: The bot username is hardcoded instead of using the actual bot data fetched from Bodhi.Telegram.get_me().

Recommendation:

|> assign(:bot_username, bot.username)

4. Migration Default Value Mismatch 🟡

Locations:

• Migration: priv/repo/migrations/20250902103724_create_pages.exs:12
• Schema: lib/bodhi/pages/page.ex:11

Issue: Migration sets default template to "page", but schema sets it to "page.html".

# Migration
add :template, :string, default: "page", null: false

# Schema
field :template, :string, default: "page.html"

Recommendation: Make them consistent. Use "page.html" in the migration.

---

Code Quality Issues

5. Missing Module Documentation 🟡

Location: lib/bodhi/pages/page.ex:2

@moduledoc false

Issue: Schema modules should have documentation explaining their purpose.

Recommendation:

@moduledoc """
Schema for managing CMS pages with support for multiple content formats.
"""

6. Potential Performance Issue in Template Options 🟡

Location: lib/bodhi_web/live/page_live/form.ex:111-115

defp template_options do
  BodhiWeb.PageHTML.__info__(:functions)
  |> Enum.map(fn {func, _arity} -> to_string(func) end)
  |> Enum.filter(&(!String.starts_with?(&1, "__") and !String.ends_with?(&1, "_")))
end

Issue: This is called every time the form is rendered. The function list is static and should be computed at compile time or cached.

Recommendation:

@template_options BodhiWeb.PageHTML.__info__(:functions)
                  |> Enum.map(fn {func, _arity} -> to_string(func) end)
                  |> Enum.filter(&(!String.starts_with?(&1, "__") and !String.ends_with?(&1, "_")))

defp template_options, do: @template_options

7. Inconsistent with AGENTS.md Guidelines 🟡

Location: lib/bodhi_web/live/page_live/form.ex:19

<.input field={@form[:title]} type="text" label="Title" />`

Issue: There's a backtick (`) at the end of line 19 that shouldn't be there.

8. Missing Unique Constraint Check 🟡

Location: lib/bodhi/pages/page.ex:23

Issue: The changeset validates unique_constraint(:slug), but the error might not be user-friendly in the LiveView form.

Recommendation: Consider adding validation feedback or better error messages for slug conflicts.

---

Best Practices & Suggestions

9. Test Coverage ✅

Excellent test coverage including:

• Context tests (test/bodhi/pages_test.exs)
• LiveView tests (test/bodhi_web/live/page_live_test.exs)
• Controller tests with format rendering (test/bodhi_web/controllers/page_controller_test.exs)

10. Behavior Pattern Implementation ✅

Good use of behaviors for dependency injection:

• Bodhi.Behaviours.AIClient
• Bodhi.Behaviours.TelegramClient
• Proper adapter pattern with configuration

This makes testing easier and follows SOLID principles.

11. SEO Component ✅

Nice addition of SEO meta tags component (lib/bodhi_web/components/seo.ex). However, there's a TODO comment on line 30:

# TODO: currently for simplicity we assume @url is the domain

Consider addressing this or creating a ticket to track it.

12. UI/Styling Updates ✅

• Migration to Tailwind v4 with proper import syntax
• DaisyUI integration
• Custom themes (light/dark)
• Follows AGENTS.md guidelines for Tailwind v4

13. Content Display Issue 🟡

Location: lib/bodhi_web/live/page_live/index.ex:31

<:col :let={{_id, page}} label="Content">{page.content}</:col>

Issue: Displaying full content in a table listing could be problematic for long content (markdown, HTML, etc.).

Recommendation: Truncate or show a preview:

<:col :let={{_id, page}} label="Content">{String.slice(page.content, 0, 50)}</:col>

---

Architecture & Design

14. Pages Context ✅

Well-structured Phoenix context with standard CRUD operations. Clean separation of concerns.

15. Migration Timestamp ⚠️

Location: priv/repo/migrations/20250902103724_create_pages.exs

Note: The migration timestamp is 20250902103724 (September 2, 2025), which is in the future. This might cause issues with migration ordering if other migrations are added before this date.

---

Documentation & Maintenance

16. Credo Configuration ✅

Good addition of .credo.exs for code quality checks.

Recommendation: Ensure mix precommit alias mentioned in AGENTS.md is defined in mix.exs. I see it's mentioned in the guidelines but I don't see it in the aliases section of mix.exs:106.

17. Missing precommit Alias 🔴

Location: mix.exs:95-108

Issue: AGENTS.md instructs to "Use mix precommit alias when you are done with all changes" but this alias is not defined in mix.exs.

Recommendation: Add the alias:

defp aliases do
  [
    precommit: ["format", "credo --strict", "dialyzer", "test"],
    # ... other aliases
  ]
end

---

Summary

Must Fix Before Merge:

1. 🔴 EEx code injection vulnerability - Critical security issue
2. 🔴 Variable shadowing in PageLive.Show - Bug
3. 🔴 Missing precommit alias - Required by AGENTS.md
4. 🟡 Migration/Schema default mismatch - Data consistency

Should Fix:
5. 🟡 Hardcoded bot username
6. 🟡 Template options performance
7. 🟡 Backtick typo in form.ex:19
8. 🟡 Content display truncation in table
9. 🟡 Add module documentation

Nice to Have:

• Address SEO component TODO
• Review migration timestamp

Strengths:

• ✅ Excellent test coverage
• ✅ Good architectural patterns (behaviors, adapters)
• ✅ Clean separation of concerns
• ✅ Follows Phoenix conventions
• ✅ Comprehensive UI improvements

---

[claude]

Pull Request Review: Pages Feature

This PR adds a comprehensive CMS pages feature with multiple format support. While well-structured with excellent test coverage, it contains critical security vulnerabilities that must be addressed.

CRITICAL Security Issues

Code Injection Vulnerability (lib/bodhi_web/controllers/page_html.ex:18-19)

The EEX format uses EEx.eval_string which executes arbitrary Elixir code from user content. Combined with String.to_atom on user input, this allows any admin to execute malicious code on the server.

Recommended: Remove EEX format or use pre-compiled templates. Never use EEx.eval_string on user content.

HTML/XSS Risk (lib/bodhi_web/controllers/page_html.ex:15-16)

HTML content is marked as safe without sanitization. Consider adding HTML sanitization.

Bugs

1. Missing auth on PageLive.Show - add on_mount BodhiWeb.Plugs.Auth
2. PageController error handling - use non-bang variants to return 404 instead of 500
3. SEO component crashes if image is nil
4. Duplicate page assignment in Index LiveView

Code Quality

1. Remove full content column from index table
2. Add template validation in changeset
3. Make NOT NULL explicit in migration
4. Add pagination for pages list
5. Consider markdown caching

Strengths

• Excellent comprehensive test coverage
• Clean architecture with proper context separation
• Good use of LiveView streams and Phoenix conventions
• SEO/OG meta tags component

Summary

Required: Fix EEX injection, String.to_atom usage, add auth to Show, fix error handling
Recommended: HTML sanitization, remove content column, add validations

Overall: Solid engineering but critical security fixes needed before merge.