A code review pass across the skilll codebase surfaced 10 issues — most of them security-relevant, a few purely about crash safety. None required new features. All were fixed in place and verified against the existing test suite (149 tests, 0 failures).

This post walks through each fix, explains the problem, and shows the pattern that replaced it.

The fixes

1. Cross-course lesson access (CatalogLive.Lesson)

Problem. After verifying a Student is enrolled in Course A, the code fetched a lesson by bare ID — with no check that the lesson actually belongs to Course A. A Student enrolled in Course A could read lessons from any Course by manipulating the URL.

Fix. After fetching the lesson, the code now verifies the lesson’s chapter is in the course’s chapter list. If it isn’t, the Student is redirected back to the course page.

lesson = Courses.get_lesson!(lesson_id)
chapters = Courses.list_chapters(course_id)
chapter = Enum.find(chapters, fn ch -> ch.id == lesson.chapter_id end)

if is_nil(chapter) do
  {:ok, push_navigate(socket, to: ~p"/catalog/#{course.id}")}
else
  # ... proceed with assigns
end

2. Admin authorization on WebSocket mounts (Router + UserAuth)

Problem. Admin routes were protected only by the :require_admin plug. Plugs run on the initial HTTP request, but LiveView WebSocket connections skip the plug pipeline after mount. If an admin’s status changed while they were connected, they’d retain access to the Instructor Studio until a full page reload.

Fix. Admin routes are wrapped in a live_session :admin block with on_mount: [{SkilllWeb.UserAuth, :require_admin}]. The on_mount callback re-verifies admin status on every mount — both HTTP and WebSocket.

scope "/", SkilllWeb do
  pipe_through [:browser, :require_admin]

  live_session :admin, on_mount: [{SkilllWeb.UserAuth, :require_admin}] do
    live "/courses", CourseLive.Index
    live "/courses/new", CourseLive.New
    # ...
  end
end

This is documented in ADR-0005. The trade-off is that navigation between :admin and default live sessions forces a full page reload — which is the correct security behavior.

3. Safe Integer.parse in EnrollmentController

Problem. String.to_integer(course_id) crashes with ArgumentError on malformed input. A typo in the URL would bring down the request.

Fix. Replaced with Integer.parse/1, which returns {id, ""} on success and :error on failure. Invalid IDs now get a flash message and a redirect to the catalog.

4. (Resolved by Fix 3)

The Integer.parse change also eliminates the crash path that Fix 4 was targeting.

5. Safe Earmark match in CourseLive.Curriculum

Problem. {:ok, html, _} = Earmark.as_html(body) crashes with MatchError when Earmark returns {:error, html, errors} for malformed markdown. This fires on every keystroke via the validate event.

Fix. Replaced the bare match with a case that handles both {:ok, _} and {:error, _}:

defp render_markdown(body) do
  case Earmark.as_html(body) do
    {:ok, html, _} -> html
    {:error, html, _} -> html
  end
end

The same pattern was already in use in CatalogLive.Lesson, so this fix brings consistency across the two LiveViews.

6. (Already safe)

CatalogLive.Lesson already used the case pattern for Earmark. No change needed.

7. TOCTOU race in Enrollments.enroll/2

Problem. The enrollment function did get_by then insert — two separate database calls. Under concurrent requests (two browser tabs, a retry), both could see no existing enrollment and both attempt to insert, causing one to fail with a unique constraint violation.

Fix. Replaced with a single atomic upsert:

%Enrollment{}
|> Enrollment.changeset(%{user_id: user_id, course_id: course_id})
|> Repo.insert(
  on_conflict: :nothing,
  conflict_target: [:user_id, :course_id]
)
|> case do
  {:ok, enrollment} -> {:ok, enrollment}
  {:error, _} ->
    {:ok, Repo.get_by!(Enrollment, user_id: user_id, course_id: course_id)}
end

The on_conflict: :nothing tells Postgres to silently skip the insert if the unique constraint is violated. The fallback get_by handles the case where the insert was skipped — returning the existing enrollment. No race condition, no constraint error surfacing to the user.

8. Unsafe Repo.update! in Courses.swap_adjacent

Problem. Repo.update! inside Repo.transaction re-raises on failure, crashing the calling LiveView. The transaction rolls back, but the error propagates as an unhandled exception.

Fix. Replaced Repo.update! with Repo.update and wrapped the transaction in a with + Repo.rollback pattern:

Repo.transaction(fn ->
  with {:ok, _} <-
         item |> Ecto.Changeset.change(position: other.position) |> Repo.update(),
       {:ok, _} <-
         other |> Ecto.Changeset.change(position: item.position) |> Repo.update() do
    :ok
  else
    {:error, cs} -> Repo.rollback(cs)
  end
end)

The caller now gets {:ok, :ok} on success or {:error, :swap_failed} on failure — no crash.

9. Stale sessions after password change

The update_user_and_delete_all_tokens/1 helper already deletes all session tokens for the user inside the same transaction that updates the password. No code change was needed — this was verified during the review.

10. Hardcoded admin email domain

Problem. User.admin?/1 checked String.ends_with?(email, "@example.com") — hardcoded in the module. Any email on that domain (or a takeable subdomain) would gain admin access with no way to change it without a code deploy.

Fix. The domain is now read from application config:

def admin?(%__MODULE__{email: email}) do
  domain = Application.get_env(:skilll, :admin_email_domain, "@example.com")
  String.ends_with?(email, domain)
end

The default is @example.com, so nothing changes in production without a config change. But the domain can now be swapped at deploy time — or set to a more restrictive value — without touching Elixir code.

What this changes

These fixes don’t add features. They close gaps that could have been exploited (cross-course access, stale admin sessions), prevent crashes on bad input (Integer.parse, Earmark), and eliminate race conditions (enrollment upsert). The test suite passes unchanged — the existing tests already covered the happy paths, and the fixes don’t alter behavior for valid inputs.

The most architecturally significant change is the live_session + on_mount pattern for admin authorization. It establishes the boundary that LiveView WebSocket connections need their own authorization check — plugs alone aren’t enough. This pattern will likely be extended to other authenticated LiveViews as the platform grows.

Next up: PBI 0030 — Track Lesson Progress (recording Completions per Student per Lesson).