Quality sweep: fix stored XSS, close authorization gaps, and add the missing test harness - #31
Open
mdon wants to merge 17 commits into
Open
Quality sweep: fix stored XSS, close authorization gaps, and add the missing test harness#31mdon wants to merge 17 commits into
mdon wants to merge 17 commits into
Conversation
Comments are where people actually write prose, so this is the surface that matters. The composer and the edit box both offer the typeahead on the plain textarea — not the rich editor, which owns its own key handling and would fight a second listener for the caret. Rendering resolves per reader before markdown runs: a link if they may open it, the author's words if it's gone, "no access" if it isn't theirs. The viewer's scope is built ONCE per render rather than per comment — resolving needs permissions, not just a uuid, and Scope.for_user/1 reads the database. Notify asks about the comment's PARENT resource, not the comment row: the question that matters is whether this person can open the thing being discussed. Editing a comment to add a mention pings; reshuffling text that already mentioned someone pings nobody.
composer_form/1 and render_comment/1 are function components, so `@x` resolves against their OWN assigns — and neither declares mentions_on or id. Opening the "Write comment" box raised KeyError and took the whole LiveView down; the same applied to editing a comment in place. Both read through ctx, which is the parent's full assigns and carries them. Missed because the only suite that renders this component runs against the PUBLISHED comments pin, where none of the mentions wiring exists. Reproduced and now guarded from phoenix_kit_projects' portal test with PHOENIX_KIT_COMMENTS_PATH pointed at this checkout.
Comment bodies go through the same mention resolver as everything else, which with the site-wide redaction setting off looks up the CURRENT title of records the reader cannot open. On a public board a hand-typed `#[project_task:...]` therefore published an internal name — the typeahead never offers one, but nothing stops someone typing it and nothing validates tokens on write. Off by default; the portal opts in.
The header rendered `user.email`, so every public board printed its
commenters' addresses. It now shows the frozen `author_display_name`,
falling back to the canonical chain for rows written before this.
A person answering on their employer's public board may be speaking for
themselves or for the project, and those are different acts. The composer
offers `Post as {project}` — but only when the host supplied a verifier AND
that verifier says this person qualifies right now. No verifier, no
control, and nobody can claim to speak for anything: hosts that never heard
of this are unaffected.
The checkbox is intent, not permission. `resolve_attribution/2` asks the
host's verifier again at submit, because the composer may have rendered
long before the send and membership can be revoked in between — otherwise a
removed member keeps speaking for the project by leaving a tab open. A
refused claim quietly becomes a personal comment: the comment is still
valid, and an error would make the control a membership oracle.
The attribution fields are absent from `cast/3` and applied by
`put_attribution/2` from server-computed values. A client that could set
them could sign a comment as anyone.
`user_uuid` is never cleared. Posting as the project changes what the
PUBLIC sees and nothing else — internally the author stays on the row, so
moderation and audit keep working. A shared voice with nobody accountable
behind it is how this feature goes wrong.
Comment markdown renders with `unsafe: true`, which disables MDEx's own
escaping and makes whatever runs afterwards the security boundary. That was
six regexes over the rendered string — a blocklist over HTML, which loses.
Verified against the real renderer, two payloads went through untouched:
<script>alert(document.domain) # no closing tag: the pattern
# required a matching </script>
<a href=javascript:alert(1)>click</a> # unquoted: the pattern required
# the value to be quoted
Either one executes for every reader of a thread and again in the admin
moderation list, which renders the same component — so an unprivileged
commenter runs script in an owner's authenticated session.
MDEx ships an ammonia allow-list and it was simply not enabled: unknown tags
and every attribute outside the allowed set are dropped rather than matched
against, and links get `rel` for free. Its default permits `style` on `div`
(it strips it from `p`), which lets a comment lay an overlay over the
moderation UI's own buttons, so that comes off too.
The `sanitize` flag is gone rather than defaulted safely. Comment bodies are
written by whoever can post, so no caller wants unsanitised output, and one
`sanitize={false}` anywhere was stored XSS on every reader.
Ten payloads are now pinned by tests against the real renderer, plus the
markdown that has to keep working.
`resolve_mentions/2` was called from `render_comment/1` — a FUNCTION
component, which sees only its declared attrs — and handed that component's
own `assigns`. Neither `pk_scope` nor `withhold_mention_titles` is declared
there, so both read as nil on every render. Because they are bracket
lookups, nothing crashed:
* `withhold_mention_titles` was always false, so "Let a host withhold
mention titles in comment bodies" shipped as a no-op;
* a nil scope makes core's `Mentions.visible/3` fail closed, so every
mention rendered locked AND still printed the record's live title —
precisely the leak withholding exists to prevent.
Both values are already in `@ctx`, forwarded at both call sites. Same class
as the KeyError fixed in ff6c378, which missed this one because bracket
access fails silently where `@assign` raises.
Also: `User.display_name/1` was called unguarded although it landed in a
core release newer than this module's declared floor, so a host resolving
core from Hex got UndefinedFunctionError on the main render path. Guarded,
with the same chain reproduced locally for older cores — the point is that
it never falls back to printing the email address.
The attribution COLUMNS need core V166 and cannot be guarded that way. The
floor is left alone rather than pinned to an unreleased version; mix.exs now
says so, and it has to be bumped when that core release ships.
**Decoration edits had no authorization at all.** `save_decoration` and `begin_decoration_edit` checked nothing — not the user, not ownership, not admin. The only thing in front of them was a pencil icon rendered on hover, and a hidden control is not a control. A logged-out visitor could push `save_decoration` at the component and rename any host record backed by a visible comment; the `send_update` the host then receives is byte-identical to a legitimate one, so it has no way to tell. Both now resolve through `decoration_if_permitted/2`, gated on the same rule as editing the comment itself. The label is also capped server-side — `maxlength` on the input is a courtesy to the typist, not a limit — and a non-binary label no longer reaches `String.trim/1` and kills the LiveView. **`save_edit` forwarded the decoration before checking permission.** A caller whose edit rights had gone still landed the label on the host record while their body edit was refused: half an edit, from a refused request. **`@enabled` gated only the template.** The component still rendered its outer div, so its cid stayed live: a page open when an admin switched comments off — or a replayed push — could still create, delete and react. Writes are now refused at the handler. Reads are deliberately untouched. Caught while fixing this: the guard clause for a non-binary label had been placed BEFORE the real clause and shadowed it completely, so the fix would have shipped dead. Compiler flagged it; clause order corrected.
Three defects in one path. **Counters could be inflated without bound.** `insert_reaction/4` deduped with a SELECT followed by an INSERT, and the code comment explains why that was the only dedup available: the UNIQUE(comment_id, user_id) index was dropped when the integer `user_id` column went during the uuid-FK migration and nothing recreated it on `user_uuid`. The schemas still declare `unique_constraint(..., name: :uq_comments_likes_comment_user)`, which is dead code — Ecto turns a DATABASE violation into a changeset error, and there is no database constraint to violate. Confirmed: that name appears in no migration in core's chain. So two clicks in the same instant both saw "no reaction", both inserted, and one user's like counted twice. Reactions now serialise on the parent comment row (`SELECT ... FOR UPDATE`), which is exactly the scope that matters. Restoring the index in core is still worth doing, and would also cover writers that bypass this function. **The counter drifted permanently once duplicates existed.** `maybe_remove_reaction/4` deletes N rows and decremented by exactly 1, so the count stayed above the truth with no user-reachable way to bring it down. It now decrements by the number actually deleted, and the floor guard became `>= by` so a decrement that would go negative is skipped rather than clamping halfway. Four near-identical counter helpers collapsed to two. **A repeat click committed a write and told nobody.** Both paths remove the opposing reaction before deciding, so `:already_liked` still commits a delete and a counter change — and `after_reaction/3` skipped exactly those two atoms, leaving every subscriber stale. The broadcast now fires; the host callback stays scoped to real state changes, since a second like is not a new like. Also: `unlike`/`undislike` ran their delete and decrement outside a transaction while their counterparts wrapped them, so a crash in between orphaned the count.
Every item here is reachable by an ordinary user, most by accident. **Comment uuids were never validated.** Callers pass raw `phx-value-uuid` into `Repo.get/2`, where a malformed value raises Ecto.Query.CastError and kills the LiveView. `save_edit` reads `editing_uuid`, which is nil when no edit is open, so `Repo.get(Comment, nil)` raised ArgumentError. User uuids and file uuids on these same paths were already validated — comment uuids were the gap. "Not a uuid" and "no such comment" are the same answer. `bulk_update_status/2` filters the same way: one bad element in a client-supplied list raised for the whole batch. **A map where a string belonged.** Submitting `comment[x]=y` instead of `comment=y` put a map into `String.length/1` — FunctionClauseError, and the crash report carried the user's draft into the application log. **Replies accepted any parent.** Delete and edit both verify the comment belongs to the current resource; reply took any string. A uuid from another resource passed `create_comment/4` (the FK is to comments, not to the resource) and `get_comment_tree/2` only walks this resource's roots — so the comment was stored, published, counted, and rendered in neither thread, under a "Comment added" flash. **Submitting mid-upload killed the page.** `consume_uploaded_entries/3` raises while entries are in progress, and a raising LiveComponent takes the host LiveView with it — draft, staged files, sibling state. Nothing disabled the button while a file climbed. **The configured length cap could exceed the schema's.** Settings clamp `comments_max_length` to 100_000 while the changeset hardcoded 10_000, so an admin raising it produced comments refused with "should be at most 10000 character(s)" — a limit they had just changed. **`get_max_depth/0` and `get_max_length/0`** lacked the `n > 0` guard and rescue their sibling `get_max_attachments/0` has: a stored "0" rejected every comment, one for nesting and one for length. **Storage errors were `inspect`ed into a user-visible banner** — bucket names, paths, changesets. Logged now, generic message shown.
`mix.exs` declared `test/support` in its elixirc paths and the directory did
not exist. `config/test.exs` configured no repo, so roughly 40 of the ~57
public functions could not be exercised at all — and every defect this sweep
found lived in that gap.
Worse than absent: some tests were asserting the ABSENCE. `count_comments/3`
wraps its query in `rescue _ -> 0`, and with no database configured the test
for it exercised the rescue and passed. Deleting the query body would not
have failed it.
Adds a sandboxed `Test.Repo` + `DataCase`, and a helper that builds the
schema from core's versioned chain — the comments tables live there, so
there is no module-owned DDL to run. Integration tests are tagged and
excluded when PostgreSQL is unreachable, so `mix test` still runs the unit
half on a machine without one.
Two things the harness has to get right, both learned the hard way here:
* `start_link/0` connects lazily and succeeds against a database that does
not exist, so the guard now asks a question before believing it;
* the V166 column probe runs BEFORE sandbox mode goes manual, or it fails
for want of connection ownership rather than for want of the column.
That probe is load-bearing: attribution needs core V166, `mix.exs` still
declares an older floor, and a plain `mix test` therefore builds a schema
without those columns. Rather than fail with "column does not exist" on
every query, the suite says so and skips, pointing at
PHOENIX_KIT_PATH=../phoenix_kit.
First tests through it cover the reaction defects fixed in 71f9418 —
including the duplicate-row case, which is what proves the counter now
decrements by rows actually deleted rather than by one.
…success
**Reads were unguarded while every write was checked.** Core's admin routes
pipeline through `:phoenix_kit_ensure_admin` only — the `permission:
"comments"` on the tab controls sidebar VISIBILITY, not access. So every
mutation called `check_authorization/1` and correctly answered "Not
authorized", while `mount/3`, `handle_params/3` and `view_comment` handed an
admin *without* the comments permission the whole platform's comments:
bodies, commenter email addresses, and on the settings page the Giphy API
key as a form value. `type="password"` masks that visually; it is not a
control. That every write was gated is what shows the read side was meant to
be too.
Both LiveViews now check on mount. `view_comment` keeps its own check
because it reads an ARBITRARY comment by uuid — the one handler where
"already on the page" is not the same as "may see this row".
**Bulk actions reported success unconditionally.** `bulk_update_status/2`
returns `{ok_count, error_count}` and all three call sites discarded it, so
a bulk approve where every row failed still flashed "Comments approved". The
count is now used, the flash is an error when anything failed, and the three
copy-pasted branches collapse to one.
The module wrote nothing to the audit trail. Eleven mutating functions — including the moderation ones, `approve_comment`, `hide_comment` and `bulk_update_status` — left no record, so "who hid this comment, and when" had no answer anywhere in the system. Moderation is exactly the kind of action whose value is knowing who took it. `PhoenixKitComments.Activity` wraps core's logger with the shape the other modules use: guarded by `Code.ensure_loaded?`, a missing activity table and a dead pool both `:ok`, everything else a warning. An audit line is not worth losing the write it describes. Metadata carries the shape of the action and nothing a user typed: status, resource type, depth, whether it was a reply. Never the body, never an email. Mutating context functions take `opts` so a LiveView can thread the acting admin (`actor_opts/1`), and `log: false` lets a wrapper keep one line instead of two — `delete_comment/2` goes through `update_comment/3`, and "updated" followed by "deleted" reads like two separate acts.
**Giphy search ran inside `handle_event`.** A slow or unreachable Giphy blocked every other event on the page — typing, likes, replies, navigation — for the full request timeout, with the UI frozen and nothing to say why. It runs through `start_async` now, which also supplies the in-flight guard it never had: a keystroke supersedes the previous search instead of stacking requests against the host's quota. **The picker's gate and the search's gate disagreed.** `giphy_enabled?/0` requires the toggle AND a key; `search_giphy/2` required only the key. With the toggle off the picker was hidden and a crafted `giphy_search` event still spent the host's quota. Hiding the control was never the control. **The API key could reach the log.** It rides in the query string, so `inspect(e)` on a Req exception writes it in plaintext. Only the exception type is logged now. **No `phx-disable-with` existed anywhere in the module.** Every submit and destructive button was unprotected: a double-click on a slow link posted the same comment twice — two events queue with the same text, the first consumes the attachments and the second lands as a text-only twin. Composer, edit, settings saves, per-row moderation and the bulk actions all carry one now.
**Uploads took the browser's word for everything.** `filename`,
`content_type` and `size_bytes` all came from `entry.client_*`, and core's
storage does `Keyword.fetch!` on them — nothing in the chain looks at the
bytes. The size is now measured with `File.stat/1` rather than declared, and
the filename is stripped of path separators and control characters before it
reaches storage and, from there, download headers. Content sniffing belongs
next to the other `fetch!`s in core's storage and is recorded in the sweep
rather than reached around from here.
`.zip .rar .7z` also came off the accept list. It is a client-side hint on
any public comment thread, and archives are not what a comment box is for.
**The audio hook pushed English prose into a flash.** Four hardcoded
sentences landed untranslated in every locale — and because the handler put
the client's own string straight into `put_flash/3`, any client could paint
arbitrary text in the app's error chrome. The hook sends a reason CODE now
and the server maps it; a non-binary value no longer raises.
**Five settings validation messages** were bare English on a page that was
otherwise fully translated.
**One string could never resolve at all.** `"%{count} selected"` was called
through `gettext/2` while the catalog held it as a PLURAL entry left over
from an `ngettext` call, so the lookup missed and Russian fell back to
English with a perfectly good translation sitting unused. The count is
substituted client-side by the bulk hook, so no locale's plural rules can
apply — the wording now reads correctly for any number and has its own
singular entry.
ru and et are back to zero untranslated. The msgfmt charset warning on both
catalogs is pre-existing and untouched by this pass.
**The six-callback host contract was duck-typed.** It lived in a moduledoc
and was invoked through `function_exported?/3`, so a typo in a callback name
or drift in its arity produced silence — the callback simply never fired and
nothing said why. `PhoenixKitComments.ResourceHandler` declares it with
`@optional_callbacks`, which buys compile-time checking of the callbacks a
host does write. Adopting it is opt-in; handlers registered today keep
working untouched.
**Docs said things that were not true.** README named `:admin_comments` and
`:admin_settings_comments` as permissions — those are tab ids, and the only
permission this module declares is `"comments"`. AGENTS.md pointed three
times at `lib/phoenix_kit_comments/phoenix_kit_comments.ex`, a path that
does not exist, and its settings table omitted `comments_rich_text`
entirely while `comments_max_depth` did not mention that depths are 0-based
(10 yields 0–9).
**Two host-facing surfaces were documented nowhere.** `PhoenixKitComments.Embed`
is required for the rich-text composer to post at all — without it "Post
comment" silently no-ops — and the README's JavaScript section, the only
place a host would look, stopped at LiveSocket hooks. The `handle_info`
catch-all requirement is now spelled out too: the component sends
`{:comments_updated, _}` to the host, and LiveView only tolerates unmatched
messages while a view exports NO `handle_info/2`, so the documented wiring
was one clause away from killing the host LiveView.
Also: `render_comment/1` was public by accident while its siblings are
private; `humanize_resource_type/1` lowercased whole strings ("GitHubRepo" →
"Githubrepo"); the same field rendered raw in one place and humanized in
another; a redundant self-alias; and `1024 * 1024` got a name.
None of the 14 PR folders had a FOLLOW_UP.md, so Phase 1 had never been run
here. All 19 review files across those PRs are now triaged: each finding
verified against current code and recorded as fixed pre-existing, fixed in
this sweep, or N/A with the reason.
Three had been written up as "left as-is". The playbook is explicit that
deferrals are not mine to declare, so they are fixed instead:
**Restoring a comment always published it.** `restore` called
`approve_comment/2` unconditionally, so undoing a delete on a comment that
had never been approved published it as a side effect. `restore_comment/2`
returns it to `pending` when the site moderates.
**Admin previews rendered every comment body in full** — parsed and
sanitised, per row — only to clamp the result to one line with CSS. The
source is truncated first.
**A settings sentence was five gettext fragments** assembled around `<code>`
tokens ("Use" / "for resource ID," / "or"), which freezes word order to
English and hands a translator "for resource ID," with no context. One msgid
per sentence now, tokens as placeholders so a locale can position them; ru
and et translated.
Both catalogs are at zero untranslated.
`mix precommit` was already failing before this sweep began — recorded in the C0 baseline — because every call into optional core code (`PhoenixKit.Mentions`, and later `User.display_name/1`) was written as a direct call guarded by `Code.ensure_loaded?`. The guard is right; the direct call still resolves at compile time, so it warns against exactly the core version the guard exists for, and `--warnings-as-errors` turns that into a failure. Those calls go through `apply/3` now, which is the point rather than an oversight — annotated where credo's "known arity" rule fires, and pulled into `mentions_available?/0` so the annotation survives the formatter re-joining the lines. Two genuine style findings fixed rather than annotated: two negated if-else conditions, and `save_edit`'s three-deep nesting, which became a flat `cond` in `save_edit_for/4` — the permission branch reads as a rule now instead of an else inside an else. `mix precommit` exits 0. `mix test` is 65/0 against published core (8 integration tests excluded for want of V166) and 65/0 against local core with all 8 running.
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
A quality sweep over
phoenix_kit_commentsfollowing the workspaceplaybook (
dev_docs/quality_sweep.md), Phase 1 and Phase 2.Rebased onto
upstream/main; the only conflict was thephoenix_kitfloor,where upstream's 1.7.214 is kept.
The one that matters
Stored XSS in every comment body. Bodies render with
unsafe: true,which disables MDEx's escaping and makes whatever runs next the security
boundary. That was six regexes over the rendered string — a blocklist over
HTML. Verified against the real renderer, two payloads went through
untouched:
Either executes for every reader of a thread and again in the admin
moderation list, which renders the same component — so an unprivileged
commenter runs script in an owner's authenticated session.
MDEx ships an ammonia allow-list and it simply was not enabled. Its default
permits
styleondiv(it strips it fromp), which lets a comment layan overlay over the moderation UI's own buttons, so that comes off too. The
sanitizeflag is gone rather than defaulted safely — onesanitize={false}anywhere was stored XSS on every reader. Ten payloads are pinned by tests.
Authorization
save_decorationhad no check at all. A logged-out visitor could pushthe event and rename any host record backed by a visible comment; the
send_updatethe host receives is byte-identical to a legitimate one.routes pipeline through
ensure_adminonly, andpermission: "comments"on the tab controls sidebar visibility, not access. An admin without the
permission got every comment on the platform, commenter emails, and the
Giphy API key as a form value.
@enabledgated only the template, so writes went through on adisabled thread.
save_editforwarded the decoration before checking permission — halfan edit, from a refused request.
Data integrity
no unique index behind it — the index was dropped during the uuid-FK
migration and never recreated, which also makes the schemas' own
unique_constraintdead code (confirmed: that name is in no migration incore's chain). Reactions now serialise on the parent comment row.
delete_allremoves N rows and the decrement was hardcoded to 1.opposing reaction before deciding, and
after_reaction/3skipped exactlythose two atoms.
unlike/undislikeran outside a transaction while theircounterparts wrapped theirs.
Crashes reachable by ordinary use
Comment uuids went unvalidated into
Repo.get/2(CastError, andRepo.get(Comment, nil)when no edit was open); a map submitted where astring belonged reached
String.length/1and wrote the user's draft intothe crash report; replies accepted a parent from another resource, storing a
comment that rendered in neither thread under a "Comment added" flash;
submitting mid-upload raised inside
consume_uploaded_entries/3and tookthe host LiveView with it.
Everything else
Activity logging (there was none — eleven mutations including moderation);
Giphy moved off the LiveView process (it blocked every event on the page for
the request timeout) and its gate aligned with the picker's;
phx-disable-with(there was not one in the module, so a double-click posted twice); upload
metadata no longer taken from the client; the resource-handler contract
declared as a behaviour instead of duck-typed; i18n (five bare-English
flashes, four hardcoded JS strings, and one string that could never resolve
because the catalog held it as a plural entry while the code called
gettext/2).Test infrastructure
mix.exsdeclaredtest/supportand the directory did not exist;config/test.exsconfigured no repo. ~40 of ~57 public functions wereuntestable, and some tests were asserting the rescue path —
count_comments/3returned 0 because there was no database, and deletingthe query body would not have failed the test. Now a sandboxed repo,
DataCase, and integration coverage for the reaction defects.Phase 1
None of the 14 PR folders had a
FOLLOW_UP.md. All 19 review files aretriaged, each finding verified against current code and recorded as fixed
pre-existing, fixed in this sweep, or N/A with the reason.
Verification
mix test— 68 tests, 0 failures.mix precommit— exits 0, which it did not before this sweep. It wasfailing on optional-core calls written as direct calls behind
Code.ensure_loaded?: the guard is right, but the call still resolves atcompile time and warns against exactly the version the guard exists for.
Comment attribution needs core V166, which is unpublished (it is in
BeamLabEU/phoenix_kit#692). The declared floor predates it, so a host at the
floor gets a missing-column error from
get_comment_tree/2.mix.exssaysso at the pin,
display_name/1is guarded at the call site, and the testhelper detects the missing column and skips the integration half with a
pointer rather than failing. Bump the floor when that core release ships.