Uh oh!
There was an error while loading. Please reload this page.
Fix sanitizer XSS + sidebar flip; harden phoenix_kit.doctor (prefix, child-order, schema-drift) - #641
Merged
Conversation
The drawer-side is its own scroll container (the admin menu outgrows the viewport) inside the drawer grid's auto-sized column. Chromium resolves that column 15px narrower on pages where the root scrolls, and a daisyUI modal's page scroll lock re-resolves it wider mid-session — shifting the whole content pane sideways around every modal open/cancel on long admin pages, then latching until the next load. Reserve the sidebar's scrollbar gutter (lg-scoped; the mobile overlay drawer needs none) so the column width is state-independent, and switch the sidebar-closed transform to -100% so the now-wider element still hides fully when collapsed.
sanitize_urls/1 blacklisted only literal javascript:/vbscript:/data: schemes, but MDEx renders raw HTML (unsafe: true) and the browser decodes entities and ignores whitespace/control chars in a scheme — so javascript:, java	script:, and java<tab>script: all slipped through as stored XSS in any markdown render (the dashboards note widget among them). Replace the blacklist with an ALLOWLIST over a normalized value: decode HTML entities, strip control/whitespace chars, then keep only http/https/mailto/tel (and relative/fragment) URLs. The transform only ever removes an attribute, never rewrites the visible URL, so it stays fail-safe. Covers the reported vectors with tests.
PhoenixKit.Supervisor reads Settings/OAuth config from the database as it
boots, and Oban opens a pool against the same Repo, so both must start AFTER
the Ecto Repo in the host's application.ex children list. When they land
before the Repo the app crash-loops at boot (Oban can't reach the DB). The
installer positions its child after the detected Repo, but a hand edit — or an
Igniter anchor miss that prepends instead of inserting — can regress the order.
The existing runtime supervisor check can't catch this: by the time doctor
runs, everything has already started. So read the source order instead.
- Add PhoenixKit.Install.ChildOrder: a pure analyzer over application.ex
source that extracts the ordered children head-modules (handling bare
aliases, {Mod, opts} tuples, and inline Supervisor.start_link([...])) and
reports whether the Repo precedes PhoenixKit.Supervisor and Oban.
- Wire a "Child Start Order" check into mix phoenix_kit.doctor that locates
the host application.ex (compiled module source, then lib/<app>/), runs the
analyzer, and FAILs with a fix-it message when misordered — catching both
fresh and existing bad installs.
- Cover the analyzer with unit tests (correct order, both crash cases,
indeterminate inputs, tuple/inline shapes).The Migration State check resolved the schema prefix as `opts[:prefix] || "public"`, ignoring `config :phoenix_kit, :prefix`. On a prefixed install (e.g. prefix "companyplexus") the version marker lives at `<prefix>.phoenix_kit`, so doctor read `public`, found nothing, and reported "PhoenixKit not installed / V0" — while `mix phoenix_kit.update --status`, run seconds apart, resolved the prefix and reported the real V145/V150. The B2-era prefix-resolution fix reached the updater/status but not doctor. - Resolve the prefix through PhoenixKit.Install.PrefixConfig.resolve_prefix/1 (--prefix flag → config :phoenix_kit, :prefix → "public"), the same path the updater and status use. Done after app.config so the config is loaded. This one change flows to every prefix-consuming check (migration state, uuid column types, null uuids, orphaned fk refs). - Fix the Oban Configuration check reporting "0 queues, 0 plugins": doctor's own cap_repo_pool_size/1 zeroes Oban queues/plugins in the app env (to conserve connections in update_mode) before any check runs. Snapshot the Oban config before capping and report from the snapshot.
A prefixed install reported `column phoenix_kit_users_tokens.browser does not
exist` at runtime while its version marker read V150 — the migration that adds
`browser`/`os` (V150, formerly V149 before the upstream-collision renumber).
The migration itself is correct and idempotent (add_if_not_exists, prefix-safe
native ADD COLUMN IF NOT EXISTS); the install had drifted so the marker sat
AHEAD of the actual schema. Re-running the migrator is then a no-op because the
marker already covers V150, so the crash has no self-service recovery — and the
existing heal_version_comment only heals the marker UP (schema ahead of marker),
never this direction.
Add a "Schema Drift" doctor check: for a small table of {version, table,
column} expectations, when the marker claims that version (or higher) but the
column is missing at the resolved prefix, FAIL with the surgical repair — roll
the marker back one version and re-run the migrator (idempotent). Prefix-aware
(uses the same resolution as the rest of doctor), schema-anchored existence
query, and a no-false-positive guard: marker < the expectation's version reads
as pending-migration, not drift.`Task.Supervisor.async_nolink/2` + `Task.yield(task, t) || Task.shutdown(task, :brutal_kill)` is the textbook bounded-async idiom, but dialyzer widens the opaque Task.t()'s :pid/:ref fields and flags Task.yield's first argument (call_without_opaque). The runtime code is correct and covered; add a scoped .dialyzer_ignore.exs regex mirroring the existing auth.ex entry for the same class rather than distorting correct code to satisfy the analyzer.
ddon pushed a commit
that referenced
this pull request
Jul 16, 2026
Both reviewed clean: V151 migration (PR #640) follows all established prefix-safety conventions; PR #641's sanitizer/doctor fixes are correct and well-tested. One policy-conflict finding flagged (not fixed, per maintainer instruction): PR #641 re-adds a scrollbar-gutter override to layout_wrapper.ex, which AGENTS.md's 2026-07-12 removal explicitly says not to do. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Six fixes across two themes: a security/CSS pair, and a
mix phoenix_kit.doctor+ prefix-safety hardening batch surfaced by a real--prefix(named-schema) install.1. HTML sanitizer URL bypass (security)
HtmlSanitizer.sanitize_urls/1blacklisted only literaljavascript:/vbscript:/data:schemes, but<.markdown>renders raw HTML (unsafe: true) and the browser decodes entities + ignores whitespace/control chars in a scheme — sojavascript:,java	script:, andjava<tab>script:all slipped through as stored XSS in any markdown sink (the dashboards note widget among them).Replaced the blacklist with an allowlist over a normalized value: decode HTML entities, strip control/whitespace chars, then keep only
http/https/mailto/tel(and relative/fragment) URLs. The transform only ever removes an attribute — never rewrites the visible URL — so it stays fail-safe. No new dependency. Covered bytest/phoenix_kit/utils/html_sanitizer_test.exs(every reported bypass vector).2. Admin sidebar width flip around modal scroll locks (CSS)
The admin
.drawer-sideis its own scroll container inside the drawer grid's auto-sized column. Chromium resolves that column ~15px narrower on pages where the root scrolls, and a daisyUI modal's page scroll-lock re-resolves it wider mid-session — shifting the whole content pane sideways on every modal open/cancel on long admin pages, then latching until the next load.Reserve the sidebar's scrollbar gutter (
lg:[scrollbar-gutter:stable]— the mobile overlay drawer needs none) so the column width is state-independent, and switch the collapsed transform to-100%so the now-wider element still hides fully. This targets the sidebar column's own width — distinct from the modal-body gutter compensations removed in39e93eb6(which daisyUI ≥5.1 handles).3. Installer child start order — new
phoenix_kit.doctorcheckField report: on a prefixed install the supervision children were ordered with
PhoenixKit.Supervisor/Obanbefore the app's Repo inapplication.ex, so PhoenixKit (which reads Settings from the DB at boot) and Oban (which needs the pool) started before the database — an Oban crash-loop at boot.The installer already positions its child
after: [repo], before: [endpoint]; the prepend happens inside Igniter's insertion fallback when its anchor doesn't match, which isn't reliably fixable from phoenix_kit. So the deterministic fix is detection: a new Child Start Order doctor check reads the hostapplication.ex, extracts the ordered supervision children via a purePhoenixKit.Install.ChildOrderanalyzer (handles bare aliases,{Mod, opts}tuples, and the inlineSupervisor.start_link([...])shape), and FAILs whenPhoenixKit.SupervisororObanprecede the Repo — catching both fresh and existing bad installs. Unit-tested intest/phoenix_kit/install/child_order_test.exs.4.
phoenix_kit.doctorprefix-blindness + Oban0/0reportOn a prefixed install (
config :phoenix_kit, prefix: "…"), doctor resolved the schema asopts[:prefix] || "public", so the Migration State check read the version marker atpublic, found nothing, and reported "not installed / V0" — whilephoenix_kit.update --status, run seconds apart, resolved the prefix and reported the truth.Routed doctor's prefix resolution through the same
PrefixConfig.resolve_prefix/1the updater/status use (--prefix→config :phoenix_kit, :prefix→"public", afterapp.configloads config), so the fix flows to every prefix-consuming check (migration state, uuid types, null uuids, orphaned FKs). Also fixed the Oban Configuration check reporting0 queues, 0 plugins: doctor's own pool-capping zeroes Oban queues/plugins in the app env before any check runs — now snapshots the config before capping and reports from the snapshot.5.
phoenix_kit.doctorschema-drift checkThe same prefixed install crashed at runtime with
column phoenix_kit_users_tokens.browser does not existwhile its version marker read V150 — the migration that addsbrowser/os. The migration is correct and idempotent (add_if_not_exists→ native, prefix-safeADD COLUMN IF NOT EXISTS); the install had drifted so the marker sat ahead of the actual schema (a residue of the V149→V150 renumber crossing earlier prefix-confused upgrades), and re-running the migrator is a no-op once the marker covers V150 — so the crash has no self-service recovery, and the existingheal_version_commentonly heals the marker up, never this direction.New Schema Drift doctor check: for a small
{version, table, column}table, when the marker claims that version (or higher) but the column is missing at the resolved prefix, FAIL with the surgical repair — roll the marker back one version and re-run the migrator (idempotent). A marker below the expected version reads as pending-migration, not drift (no false positive).6. Dialyzer: qr_login Task-opaque false positive
QrLogin.location_for/1uses the textbookTask.Supervisor.async_nolink+Task.yield(task, t) || Task.shutdown(task, :brutal_kill)bounded-async idiom, but dialyzer widens the opaqueTask.t()'s:pid/:reffields and flagsTask.yield's first argument (call_without_opaque). Added a scoped.dialyzer_ignore.exsregex mirroring the existingauth.exentry for the same class rather than distorting correct runtime code.mix dialyzernow passes.Testing
mix format,mix compile --warnings-as-errors,mix credo --strictclean;mix dialyzerpasses (the qr_login false positive is the only change to the ignore set).html_sanitizer_test.exs(bypass vectors),child_order_test.exs(11 cases); doctor's prefix resolution is covered by the existingprefix_config_test.exs(flag → config → public, including a non-public fixture).--prefixfield install and reproduced/verified against the local parent database.