Skip to content

Refuse a malformed actor in the two siblings of the credential guard - #696

Merged
ddon merged 1 commit into
BeamLabEU:mainfrom
timujinne:fix/actor-shape-in-status-and-confirmation
Aug 9, 2026
Merged

Refuse a malformed actor in the two siblings of the credential guard#696
ddon merged 1 commit into
BeamLabEU:mainfrom
timujinne:fix/actor-shape-in-status-and-confirmation

Conversation

@timujinne

Copy link
Copy Markdown
Contributor

Follow-up to the post-merge review of #691, which found the actor-shape hole
closed for admin_update_user_password/3 still open in its two siblings.

The hole

update_user_status/3 and toggle_user_confirmation/2 both ended in:

_system->do_...(user)# unchecked write

That branch exists for callers with no actor — seeds, mix tasks, and
Users.Referrals, which expires an account deliberately without one. It also
swallowed every malformed actor: %{"uuid" => …} decoded from JSON by a host
controller, a bare uuid string, false, 42. Each took the unchecked path.

toggle_user_confirmation/2 is the serious one — its unchecked path reaches
admin_unconfirm_user/1, clearing confirmed_at and locking the target out of
every confirmation-gated page. update_user_status/3 is the same shape with a
reversible effect.

Neither is reachable from core: every LiveView caller passes a real %User{}.
This is host-facing surface, and the same authority-bypass class the series set
out to close.

What changed

Both now use the three-branch shape from admin_update_user_password/3,
including the refusal log. Consistency between the three was the goal — three
functions with one stated rule and three different behaviours is the next bug.

An explicit actor: nil still takes the system path, identically in all three:
Keyword.get/Map.get cannot distinguish it from an absent key. Making it fail
closed is a decision for all three at once and is left out of this PR.

Tests, and an honest accounting of them

The two malformed-actor tests are the ones that carry the fix — verified by
reverting auth.ex and re-running: exactly those two go red.

The six new toggle_user_confirmation/2 tests survive that revert. They are
characterization coverage for a rule that had none — toggle_user_confirmation
appeared zero times in this file before — and they are described that way rather
than counted as regression detection.

A vacuous test, fixed

user_form_authority_test.exs's drift pin had been red on main, and underneath
that it was proving nothing.

It submitted "first_name" => "Harmless" as harness scaffolding while asserting
first_name was unchanged — an Admin editing an Owner may legitimately set it,
so the test failed on its own input. Removing that exposed the real problem: it
poisoned all five identity fields in one payload, Auth.update_user_fields/2
routes them into a singleprofile_changeset, and the generated
"attacker-user_timezone" fails that field's format validation. One invalid
member invalidated the whole changeset, so nothing was written whether the filter
ran or not.

Confirmed by deleting the drop_schema_identity_fields/2 call and watching the
test still pass. It now submits one poisoned-but-schema-valid field per
submission, so no single value can abort the others, and it goes red when the
filter is removed.

Worth noting for the next reader: the "sixth field added to
updatable_profile_fields/0 and forgotten in the filter" drift this test names
is structurally impossible — the filter is derived from that list, not a copy of
it. What it actually pins is that nobody replaces the derived list with a
hardcoded one.

Verification

test/integration/users383 tests, 0 failures, no excluded count.
mix format --check-formatted, mix compile --warnings-as-errors,
mix credo --strict (10298 mods/funs) clean.

@version and CHANGELOG.md untouched.

Post-merge review of BeamLabEU#691 found the actor-shape hole closed for
`admin_update_user_password/3` still open in its two siblings.
`update_user_status/3` and `toggle_user_confirmation/2` both ended in a
`_system ->` catch-all that performed the UNCHECKED write. That branch exists
for callers with no actor — seeds, mix tasks, and `Users.Referrals`, which
expires an account deliberately without one — but it also swallowed every
malformed actor: a map decoded from JSON by a host controller, a bare uuid
string, `false`, `42`. Each took the unchecked path and wrote.
`toggle_user_confirmation/2` is the serious one: its unchecked path reaches
`admin_unconfirm_user/1`, so `confirmed_at` is cleared and the target is locked
out of every confirmation-gated page. `update_user_status/3` is the same shape
with a reversible effect. Neither is reachable from core — every LiveView caller
passes a real `%User{}` — so this is host-facing surface, and it is the same
authority-bypass class the series set out to close.
Both now use the three-branch shape from `admin_update_user_password/3`,
including the refusal log. An explicit `actor: nil` still takes the system path,
identically in all three: `Map.get`/`Keyword.get` cannot tell it from an absent
key, and making it fail closed is a decision for all three at once, not one.
Tests. The malformed-actor cases are the two that carry the fix — verified by
reverting `auth.ex` and re-running: exactly those two go red. The six new
`toggle_user_confirmation/2` tests survive that revert and are stated here as
what they are — characterization coverage for a rule that had none, since
`toggle_user_confirmation` appeared zero times in this file before. They pin
behaviour; they do not defend this hole.
Also fixes a test in `user_form_authority_test.exs` that had been red on `main`
and, underneath that, vacuous. It submitted `"first_name" => "Harmless"` as
harness scaffolding while asserting `first_name` was unchanged — an Admin
editing an Owner may legitimately set it, so the test failed on its own input.
Removing that revealed the deeper problem: it poisoned all five identity fields
in ONE payload, `Auth.update_user_fields/2` routes them into a single
`profile_changeset`, and the generated `"attacker-user_timezone"` fails that
field's format validation — one invalid member made the whole changeset invalid,
so nothing was written whether the filter ran or not. Confirmed by deleting
`drop_schema_identity_fields/2`'s call and watching the test still pass. It now
submits one poisoned-but-schema-valid field per submission, so no single value
can abort the others, and goes red when the filter is removed.
test/integration/users — 383 tests, 0 failures, no excluded count. format
--check-formatted, compile --warnings-as-errors and credo --strict (10298
mods/funs) clean.
@ddon
ddon merged commit 855c17f into BeamLabEU:mainAug 9, 2026
ddon pushed a commit that referenced this pull request Aug 9, 2026
Both clean — no new defects. Verified the parts this series has repeatedly got
wrong rather than the parts the diffs touch:
#695 introduces two new return values and both propagate completely.
estimated_rows/3's :unknown is handled by an explicit pattern match in V163
(not by term ordering, which would have made it correct by accident) and named
out loud by repair_uuid. Probe's :unparseable reaches classify/3, Repair.run,
error_message/1, the error type, and the JSON error_tag — traced each. The '0'
behaviour change it implies is unreachable: nothing stamps IS '0'.
#696's "two siblings" claim is exhaustive: no `_system ->` left in auth.ex, and
delete_user was already fail-closed by a different mechanism
(validate_can_delete_user/2's fallback refuses even nil).
It also corrects a test I added in the #692 pass, and the correction is worse
than its commit message claims. My version poisoned all five identity fields in
one payload; "attacker-user_timezone" fails validate_user_timezone/1, one
invalid member invalidates the whole profile_changeset, so nothing was written
either way. It passed with the filter DELETED and failed with the filter
working — inverted, not merely vacuous, and red on main since I added it. The
exact defect I spent three reviews naming in other people's tests, in a test I
could not run. Written up in full in the #696 review.
CHANGELOG deliberately NOT committed: another agent is mid-edit in this tree
(implementing the mentions N+1 and access-request findings from my #692 review,
and hand-declaring V165/V166 in the manifest), and CHANGELOG.md now holds their
in-flight entries alongside mine. Committing it would separate their notes from
their code.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@timujinne
timujinne deleted the fix/actor-shape-in-status-and-confirmation branch August 10, 2026 08:35
ddon pushed a commit that referenced this pull request Aug 10, 2026
Tim's call: the release ships as 2.0.0, with the feature-module pins widened the
same day so the ecosystem lands with it. That removes the cost I had weighted
most heavily when I bumped to 1.7.237 — the unsatisfiable-dependency window is
coordinated away rather than endured — and it restores what the patch route
could not offer: `{:phoenix_kit, "~> 1.7"}` does not resolve to 2.0, so no
below-floor host is dragged across the floor by a routine `mix deps.update`.
That last point is why one paragraph had to be rewritten rather than kept. The
upgrade-requirement section warned that a routine deps.update WOULD carry a
below-floor host across; true of 1.7.237, false of 2.0.0. It now explains that
the major is precisely what prevents it, and carries the other half hosts need:
`~> 2.0` is unsatisfiable alongside any phoenix_kit_* package still pinning
`~> 1.7.x`, so modules must be upgraded together with core, not after it.
CHANGELOG now covers #689 through #697. The #695/#696 entries existed only in
the main working tree, which is dirty with another agent's in-flight work, so
they are re-created here rather than committed from there. #697's entries name
the two behaviour changes hosts will actually notice — the dev mailbox going
quiet by default, and /api/files/:uuid/info now requiring auth — and the
unauthenticated upload fix its own PR description never mentioned.
Written in an isolated worktree at origin/main so the other agent's uncommitted
CHANGELOG and lib/ changes are untouched; their entries stay in their tree and
will merge as ordinary changelog text.
Release gate at v2.0.0: CHANGELOG heading and body PASS, tag collision PASS.
Only the stale chain_hash remains, which that same agent is fixing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ddon pushed a commit that referenced this pull request Aug 10, 2026
…6 manifest
Not my work — another agent's changes, uncommitted in the working tree while I
was reviewing #695/#696/#697. Committing at the maintainer's request so nothing
is stranded. Verified it compiles with warnings-as-errors before committing.
Three pieces, all closing findings from my earlier review passes:
- Mentions.Users.search/2 no longer calls Scope.for_user/1 per candidate; the
admin-area rule became two SQL EXISTS subqueries with the limit applied in the
database. This is the ~128-queries-per-keystroke item I reported in the #692
review and deliberately did not fix, because rewriting an authorization filter
as hand-written SQL without a database to test against was not a change to
make blind. It has a database now.
- AccessRequests.request/4 validates the resource type against
ResourceLinks.handlers/0, requires a castable uuid, and rate-limits per
account — the unvalidated/unthrottled path I filed as a nitpick on the same PR.
- ExpectedSchema hand-declares V165/V166 objects and restamps chain_hash, which
is the release blocker. Their choice of the hand-declare route matches what the
#692 review concluded was necessary: no generator run can emit V164+ objects,
because regeneration replays a pre-squash checkout whose chain ends at V163.
CHANGELOG.md is deliberately NOT in this commit. The copy in this tree predates
the 2.0.0 retitle and the #695-#697 entries now on main, so committing it would
have reverted them. Their three entries were extracted verbatim first and are
re-applied on top of the current file in the next commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@timujinne@ddon