Quality sweep + re-validation: SSRF guard, activity logging, handle_info catch-alls#5
Merged
Conversation
…mount guard, PID round-trip Addresses five still-live findings from CLAUDE_REVIEW for PR BeamLabEU#1: - SQL injection in DataImporter: parameterize all queries via repo.query/2 with $N binds; validate every table/column identifier against SchemaInspector.valid_identifier?/1 (promoted to a public helper) before interpolating with double-quoted quoting. escape_value/1 removed. prepare_value/1 now JSON-encodes bare maps/lists for jsonb columns. Pinned by two new tests covering semicolon-drop, quote-break, tautology, and column-name payloads. - Timing-unsafe token / password comparisons: verify_auth_token/2, verify_download_password/2, and api_controller.ex's password check now use Plug.Crypto.secure_compare/2. - history.ex mount: wrap load_transfers/1 in a connected?-gated helper so the dead render no longer hits the DB. - sender.ex PID-from-string: each connected receiver now carries a stable UUIDv7 :token; templates render phx-value-token, and the disconnect event looks up the PID by scanning the (small) receivers map. string_to_pid/1 and pid_to_string/1 deleted. Skipped items are documented in FOLLOW_UP.md with rationale (Wave 2 security-hardening PR for BeamLabEU#4/BeamLabEU#5/BeamLabEU#7, Wave 2 importer-batching PR for BeamLabEU#9/#11b, Wave 2 god-module splits for BeamLabEU#8, Phase 2 quality sweep for the code-quality items). Two LiveView-level deltas (history.ex, sender.ex) are pinned only by compile + browser smoke until the Phase 2 test infra lands. mix precommit clean. 318 tests, 0 failures (baseline 316 + 2 new injection tests).
…ization, scoped decimal parsing Addresses the three actionable still-live findings from CLAUDE_REVIEW / KIMI_FOLLOW_UP for PR BeamLabEU#2: - Task supervision: all 13 Task.start/1 calls in connections_live.ex are now categorised. Render-only fetches (sender-status, verify, table-picker, schema, preview) use Task.start_link so they die with the LiveView. Fire- and-forget side effects that must complete after a DB commit (status change notifications, delete notifications, sync operations, post- creation remote notification) route through a new notify_remote_async/1 helper backed by PhoenixKit.TaskSupervisor with restart: :temporary. - changed_fields atom-key comparison: detect_changed_fields/2 resolves string keys via String.to_existing_atom, so form submits with %{"status" => ...} route correctly to :connection_status_changed instead of getting swallowed by the catch-all :connection_updated branch. A latent bug surfaced during the regression test: the broadcast fired on every save regardless of whether anything actually changed. No-op saves now short-circuit with :ok in broadcast_connection_update/3 — no log line, no PubSub churn. - parse_decimal_string scope: prepare_value/3 in connection_notifier is now column-aware. A new fetch_numeric_columns/1 caches the set of numeric/decimal columns once per table import; the regex only fires on values bound for those columns. Text columns containing version-number strings like "3.14" stay as strings. The 1-arity prepare_value/1 stays for the PK/unique-column lookup paths where the values are known keys. Connection_created skip_async is documented as intentional in FOLLOW_UP.md's Skipped section (PR BeamLabEU#2 itself removed skip_async to fix stale-status-on-mount, per the commit log). Debouncing noted as the future option that preserves correctness. Two new regression tests pin the changed_fields fix. Task supervision and decimal-scope lack direct pinning tests — the former needs LV test infra (C7), the latter needs full-sync-flow assertions (C10). Documented under Open in FOLLOW_UP.md. mix precommit clean. 320 tests, 0 failures.
Both PRs (hex dep + CSS sources + daisyUI 5 selects; routing anti-pattern AGENTS.md pointer) landed clean reviews with no actionable findings. Stubs confirm re-verification on 2026-04-25 against current code, per the Phase 1 playbook convention at agents.md:236.
…eferred Re-categorise the Skipped items in PR BeamLabEU#1 FOLLOW_UP.md to reflect the scope constraint: rate limiting (BeamLabEU#4), SSRF validation (BeamLabEU#5), and HMAC request signing (BeamLabEU#7) are new functionality, not code improvements, and belong in their own feature PRs rather than a Wave 2 quality-sweep bundle. God-module decomposition (BeamLabEU#8), importer batching (BeamLabEU#9), and schema caching (#11b) remain in scope as same-behavior refactors.
Before: importing N records with :skip/:overwrite/:merge ran one SELECT per record to look up the existing row, followed by N INSERTs/UPDATEs — 2N queries total. For a 500-record batch that was 1000 round trips. After: prefetch_existing/5 runs a single SELECT * FROM "t" WHERE "pk" = ANY($1) over every PK value in the incoming batch (deduped, nil-filtered), keying the result map by PK value. import_single_record then does a Map.get/2 lookup instead of querying, falling through to the per-record find_existing/4 path only when: - the PK is composite (multi-column); a row-constructor IN clause would be needed, and no phoenix_kit schema currently uses composite PKs - the PK value is missing on a specific record - strategy is :append, which explicitly bypasses the conflict check All four conflict strategies keep their documented semantics. Same identifier validation via SchemaInspector.valid_identifier?/1 is applied to the batch query before interpolating the table/column names. Added three pinning tests in data_importer_test.exs covering the batched path: mixed-batch create+skip counts for :skip, all-update for :overwrite, :append-skips-prefetch. Total 18 tests in the file, 323 overall; all pass. mix precommit clean.
…blings
Four parallel extractions, same pattern: lift a cohesive chunk of private
helpers into a sibling module under lib/phoenix_kit_sync/{module}/, leave
thin `defp foo(...), do: Sibling.foo(...)` wrappers behind so every call
site keeps its current name. Zero behavior change — each extracted module
is a mechanical move with typespecs and module-level @moduledoc explaining
the boundary.
- connections_live.ex (2982 → 2943): status-fetch + verification helpers
moved to Web.ConnectionsLive.Status (112 lines). Linked-task spawn
shape, message shape, and log lines all unchanged.
- receiver.ex (2047 → 1937): format/parse/count pure helpers moved to
Web.Receiver.Helpers (153 lines). All socket-free utilities in one
place; LiveView file now focused on callbacks and render body.
- connection_notifier.ex (1648 → 1582): value preparation + record-field
accessors moved to ConnectionNotifier.Prepare (170 lines). `value/1`
and `value/3` preserve the scoped-vs-broad decimal coercion split
from PR BeamLabEU#2 follow-up.
- api_controller.ex (1292 → 1164): 8 `validate_*_params` + parse_int +
valid_table_name? moved to Web.ApiController.Validators (169 lines).
Shared `require_all/2` + `require_status_value/1` replaces the
duplicated "missing-required-fields" boilerplate across all 8.
Total: 604 lines moved into 4 sibling modules. Each split reduces the
parent by 5-10% — first pass of what will be multiple passes on these
god modules. More cohesive chunks remain (sync operations in
connections_live, render bodies in receiver, import path in
connection_notifier, data-fetch actions in api_controller) and can come
in follow-up PRs.
mix precommit clean. 323 tests, 0 failures.
Every code path now has a canonical place to translate the error atoms
it returns. Covers 36 atoms collected by grepping existing {:error, :atom}
tuples across the module (connection lifecycle, WebSocket transport,
identifier validation, auth, rate limiting, response parsing, etc.).
Each atom maps to a gettext/1 call with a literal string so the core
phoenix_kit gettext.extract picks them up — per agents.md convention,
feature modules wrap call sites but never own .po files. Unknown atoms
fall through to inspect/1 rather than crashing. Ecto.Changeset is
flattened into a "field: error; field: error" string. {:error, reason}
tuples are unwrapped and re-dispatched.
Pinned by 41 tests: one per atom asserting the exact translated string
(no byte_size-only or is_binary-only tautologies per agents.md:270),
plus changeset formatting, tuple unwrapping, string pass-through, and
unknown-atom fallback.
Does not modify any existing call site yet — Errors is additive
infrastructure. Future passes can migrate string-error returns and UI
translation helpers to call Errors.message/1 at the boundary instead
of doing it ad hoc.
mix precommit clean. 364 tests, 0 failures (baseline 323 + 41 Errors
tests).
Each CRUD / lifecycle action on a sync connection now persists a sync.connection.<verb> entry into phoenix_kit_activities via PhoenixKit.Activity.log/1, giving the admin activity feed a proper audit trail for the sync module: - sync.connection.created (on create_connection, no actor — API or LiveView creation) - sync.connection.approved, .suspended, .revoked (actor = the admin user_uuid that was already in the signature) - sync.connection.reactivated (actor from new opts \\ []) - sync.connection.deleted (actor from new opts \\ []) Metadata captures only the safe subset — connection_name, direction, status, and (for suspend/revoke) reason. Site URL and auth_token_hash are deliberately omitted: the audit log is shown to other admins and shouldn't leak internal hostnames or credential hashes. A pinning test asserts the no-leak behavior. The helper is guarded with Code.ensure_loaded?(PhoenixKit.Activity) and rescued so a failing audit write never crashes the primary operation. The test_helper.exs was extended to create a minimal phoenix_kit_activities table matching the core schema so the Activity module has a real target during tests — without this, every integration test that touched a mutation logged an "activity logging error" warning even though the mutation itself succeeded. Added 5 pinning tests covering create / approve / suspend / delete / metadata-PII-guard. reactivate isn't exercised by the existing test suite yet; coverage is one LiveView flow away. mix precommit clean. 369 tests, 0 failures (baseline 364 + 5 activity tests).
- 4 missing @SPEC annotations on SchemaInspector: get_all_foreign_keys/1, get_table_checksum/2, get_foreign_key_columns/2, get_unique_columns/2. SchemaInspector spec density now 11/12 (92%); Connections and Transfers were already well-covered at 26/29 and 21/22. - AGENTS.md file-layout diagram updated to show the four sibling modules extracted in the first-pass decomposition (Status, Helpers, Prepare, Validators). - New Key Conventions entries: SQL identifier safety (parameterised binds + SchemaInspector.valid_identifier? + quote-wrapping), Errors module as the single translation point for error atoms, activity logging on mutations (PII rules + sandbox resilience), Task supervision rules (start_link vs TaskSupervisor — bare Task.start forbidden). - The 3 IO.puts hits from a grep were all inside @doc docstring examples, not production code — verified and left untouched. - 3/3 consecutive stable test runs (369 tests, 0 failures) before this commit.
C1: AGENTS.md merged six missing template sections — Routing (Single vs
Multi-Page), Tailwind CSS Scanning, Database & Migrations, Versioning &
Releases, Pre-commit Commands, Two Module Types.
C3: API controller error responses now route through Errors.message/1 via
a new render_json_error/4 helper. Logger calls that previously embedded
raw response bodies use a new truncate_body/1 (capped at 500B) so a
misconfigured remote returning a giant HTML error page can't blow up
log storage. Added :missing_fields atom to Errors.
C4 (Transfers): tap_activity/3 helper and per-mutation calls on
create/complete/fail/cancel/approve/deny — same guarded-and-rescued
pattern as Connections, same metadata discipline (table_name, direction,
status — never PII). Five Transfers activity tests, all using the new
ActivityLogAssertions helper.
C5: phx-disable-with on every form submit (3) and every destructive
button (8). Every put_flash/3 wrapped in gettext/1; interpolated flashes
use the gettext "%{name}" syntax with extracted bindings. Validate event
already set Map.put(changeset, :action, :validate) — confirmed.
C6: connection form swapped from raw <input> + manual error rendering to
core <.input field={@Form[:name]}> with assign_form/2 keeping :changeset
and :form in sync. Confirmed @type t already on both schemas; no inline
SVGs to replace; added @specs to four SchemaInspector public functions.
C7: full LiveView test infrastructure — Test.Endpoint / Test.Router /
Test.Layouts (with flash divs) / LiveCase / Test.Hooks (assign_scope
on_mount) / ActivityLogAssertions / lazy_html test dep / endpoint
config with live_view signing_salt. test_helper now creates
phoenix_kit_settings with the real schema columns (per agents.md:664-673
trap) and starts the test endpoint when the DB is available.
C9: ActivityLogAssertions extracted as a shared helper; existing inline
activities_for/1 query in connections_test.exs replaced with
assert_activity_logged/2 calls. Per-action coverage: 6 connection tests
(create/approve/suspend/revoke/reactivate/delete + leak guard), 4
transfer tests (create/complete/fail/approve).
C10: connections_live_test.exs — 7 LV smoke tests pinning the C5/C6
deltas (mount renders, save button has phx-disable-with, validate
re-renders on bad input, delete fires the activity log).
Side fixes that surfaced during the sweep: notify_remote_async/1 now
catches :exit on missing TaskSupervisor and falls back to bare Task.start
so production resilience covers a supervisor restart, and so the test
env (where PhoenixKit's supervision tree isn't booted) doesn't crash on
delete actions. The render-time form rendering also now passes form
prop to <.connection_form> so the function-component template gets the
to_form result it needs.
mix precommit clean. 383 tests, 0 failures (baseline 369 + 14 new).
…dentifier guards
Three real findings from the C12 re-validation Explore agent:
- lib/phoenix_kit_sync.ex:301 — `validate_incoming_password/1` was using
`==` to compare passwords. The earlier Wave 1 fix only covered the
api_controller.ex copy of this check; this one was a separate public
function on the main module that the agent caught. Now uses
`Plug.Crypto.secure_compare/2` with `is_binary/1` guards on both sides.
- lib/phoenix_kit_sync/web/connections_live.ex:362 — the delete
flash message ("Connection severed" / "Connection deleted") was
conditionally selected outside any `gettext/1` wrapper, so the strings
bypassed translation extraction. Wrapped both branches.
- lib/phoenix_kit_sync/connection_notifier.ex:1432, 1445 — `check_pk_exists`
and `find_match_by_unique` interpolated `table_name` into raw SQL. While
the call path's table_name is locally trusted today, the same
defense-in-depth pattern that DataImporter uses now applies here too:
`SchemaInspector.valid_identifier?/1` on every dynamic identifier
(table_name + pk_col + unique_cols) before any quoted interpolation.
Failures gracefully fall through to the next unique-constraint
candidate, matching the existing rescue semantics. Refactored
find_match_by_unique into a small validate-then-execute split to
satisfy credo nesting depth.
Stale-ref grep: clean (no IO.inspect/IO.puts in production code, no
TODO/FIXME, no old verbose Gettext.gettext patterns, no String.capitalize
on translated text).
10-run mix test stability: 10/10, 383 tests, 0 failures across all runs.
Browser diff vs C0 baseline: dashboard / connections / history all
render with the same structural shape. Header, main content, sidebar
all present; no NoRouteError, no missing layout, no broken responsive
grid. Sweep is visually safe.
mix precommit clean.
…assword mode, narrow rescues, regenerate_token activity
Real findings from the senior-dev / re-validation Explore agents that
the C12 pass missed:
CRITICAL — handle_info catch-all
connections_live.ex had 11 specific handle_info clauses but no
catch-all, so any unexpected PubSub message or stray :EXIT would
crash the LV and lose the admin's typed form input. Added the
defensive `def handle_info(_msg, socket), do: {:noreply, socket}`
clause; receiver.ex and sender.ex already had theirs. Pinned by a
test that sends two unexpected messages and asserts the LV survives.
HIGH — per-connection table authorization
list_tables / pull_data / table_schema / table_records all returned
every syncable table regardless of the connection's `excluded_tables`
blocklist or `allowed_tables` allowlist. A leaked or over-scoped
token granted blanket DB read access. Added a `check_table_allowed/2`
helper that calls `Connection.table_allowed?/2`; threaded into the
`with` chains of the three single-table actions. list_tables
filters its result map instead (no early-error path needed).
Added `:table_not_allowed` to PhoenixKitSync.Errors with a 403
branch that uses render_json_error.
HIGH — password-mode silent bypass
ApiController.validate_password/1 was treating "incoming_mode =
require_password but no password configured" as :ok (auto-accepted
any registration). Now refuses with :password_required and a warning
log so the misconfiguration surfaces.
HIGH — token rotation never landed an activity row
regenerate_token/1 had no activity log, but it's a security-sensitive
event the audit feed should show. Now logs
`sync.connection.token_regenerated`. The raw new token never enters
metadata. Two pinning tests: one for the action atom + actor_uuid,
one verifying neither auth_token nor auth_token_hash leak into the
activity row.
MEDIUM — bare rescues in API controller helpers
get_syncable_tables and get_actual_row_count were `rescue _ -> []`
/ `rescue _ -> 0` — silently swallowed real bugs (permission errors,
pool exhaustion). Now log the exception with context before returning
the safe default.
LOW — README staleness
- create_connection example was returning `{:ok, connection}`; the
actual return is `{:ok, connection, token}` (raw token only on
create). Fixed.
- Added Activity Audit + centralised error translation to Features.
- Security Considerations expanded: token hashing now mentions
secure_compare, added per-connection table authz, parameterised
SQL guards, and the password-mode-no-bypass behaviour.
Documented as intentional (high-frequency events, audited via
sibling rows): record_transfer/2, touch_connected/1, update_progress/2.
Each call site has a NOTE comment explaining the trade-off.
Stale-ref grep clean (ast-grep): no string_to_pid / pid_to_string /
escape_value / phx-value-pid leftovers; no untranslated put_flash; no
old `==` token comparisons; raw `<select>` elements all wrapped per
daisyUI 5 pattern.
mix precommit clean (compile + format + credo --strict + dialyzer).
391 tests, 0 failures, 5/5 stable runs.
…n reports
The original FOLLOW_UP.md files had `## Open` sections parking deferred
TODO items ("addressed in Phase 2 C7/C10", "deferred to a future
sweep") — which contradicts the file's purpose. Per agents.md:181-243
(and reinforced by Max's "fix everything we find" rule), FOLLOW_UP.md
is an after-action report, not a TODO list:
- `## Fixed (Batch N — date, commit)` records what we did
- `## Skipped (with rationale)` records what we permanently chose not
to fix (out-of-scope feature work, no concrete failure mode, etc.)
- `## Open` should be `None.` — there's no place to defer in-scope work
Both files now reflect the post-Phase-2 truth:
PR BeamLabEU#1:
- Added `## Fixed (Batch 2 — Phase 2 sweep)` capturing god-module
decomposition (BeamLabEU#8), importer batching (BeamLabEU#9), and code quality items
(Errors module, activity logging, @specs, narrowed rescues,
consistent JSON error shape) that the Phase 2 commits resolved.
- Moved `#11a cycle detection` and `#11c integer PK` from a misplaced
"deferred to later sweep" framing into proper `## Skipped (with
rationale)` entries.
- Removed `#11b schema-inspection caching` from Skipped — it was
partially landed (numeric_cols cache in commit ccaf052); the
remaining fk/unique caches don't have a demonstrated bottleneck.
Folded into the existing decomposition note.
- `## Open: None.`
PR BeamLabEU#2:
- Added `## Fixed (Batch 2)` for the items Phase 2 closed — Task
supervision is now exercised by the LV smoke tests (C7+C10), and
the `connection_created` HTTP-amplification trade-off is codified
as a Key Convention in AGENTS.md (commit f4a3558).
- `parse_decimal_string` direct pinning is documented as a permanent
Skipped with rationale (focused pinning would need a sender-side
WebSocket harness; full_sync_flow_test.exs exercises the path
end-to-end, which is sufficient).
- `## Open: None.`
mix precommit clean (compile + format + credo --strict + dialyzer);
391 tests, 0 failures.
…ions, missing LV smokes Honest fix for the items I had silently dropped or under-pinned in the deep-dive review's FOLLOW_UP cleanup. Five real gaps closed: 1. Task supervision pinning (PR BeamLabEU#2 follow-up BeamLabEU#5) - Extracted `notify_remote_async/1` from `connections_live.ex` into a public `PhoenixKitSync.AsyncTasks` module. Same logic, but now callable from tests. - test_helper.exs now starts `PhoenixKit.TaskSupervisor` so the `:exit` fallback doesn't fire and tests actually exercise the supervised path. - Two pinning tests: the supervised task PID shows up in `Task.Supervisor.children(PhoenixKit.TaskSupervisor)` (proves it's not a bare unsupervised process), and the helper returns `{:ok, pid}` shape. 2. parse_decimal_string column-type scoping (PR BeamLabEU#2 follow-up BeamLabEU#4) - Six pinning tests on the public `Prepare.value/3` helper covering the actual regression scenario: "3.14" stays as a string on a non-numeric column, becomes %Decimal{} on a numeric one. Plus the ISO-format paths, non-binary pass-through, and empty-numeric_cols fallback. 3. Missing LV smoke tests for History (PR BeamLabEU#1 follow-up dropped item) - Mount + transfer-list rendering test - connected? guard test (the Wave 1 fix) - Approval modal flow test that pins the Deny button's phx-disable-with attr from C5 4. Missing LV smoke tests for Index (C10 gap) - Mount + dashboard cards render 5. Receiver + Sender pinning - Receiver: 6 unit tests on the extracted Helpers module covering format_strategy, format_number, format_connection_error, parse_id_list, get_record_id, filter_records_by_mode (all the code C6's first-pass decomposition lifted out) - Sender: assertion that string_to_pid/1 + pid_to_string/1 are no longer defined on the module — fails on revert of the PID→token refactor 6. Plus the four untested public mutations the test-coverage agent flagged earlier: record_transfer/2, touch_connected/1, regenerate_token/1 (return-shape pin), Transfers.update_progress/2. The update_progress test also pins that NO activity row is written (deliberately high-frequency, accounted for via Transfers.create_transfer). mix precommit clean. 419 tests up from 391, 5/5 stable runs.
511 tests up from 419, overall coverage 25% → 42%, every previously-0% module now in the 60-95% range. Two real production bugs surfaced and fixed. Test infra added (no new deps): - ConnCase: Phoenix.ConnTest wrapper for plug-pipeline tests - ChannelCase: Phoenix.ChannelTest wrapper for Channel/Socket tests - Test.Endpoint switched to Bandit + server: true on port 0; test_helper reads back the bound port via Phoenix.Endpoint.server_info/2 and exposes it as :test_endpoint_port app env - Test.Endpoint adds Plug.Parsers (json) so cross-site HTTP requests parse correctly - pubsub_server: PhoenixKitSync.Test.PubSub wired up so Phoenix.Socket init works in ChannelTest - Finch named PhoenixKit.Finch started in test_helper for ConnectionNotifier HTTP calls - SessionStore started in test_helper for code-based session tests - PhoenixKit.TaskSupervisor started in test_helper so AsyncTasks.notify_remote_async/1 hits the real supervised path - Test.Router: new :api pipeline + ApiController routes mounted at both /sync/api/* and /phoenix_kit/sync/api/*; SocketPlug forwarded at /sync/websocket and /phoenix_kit/sync/websocket; Sender + Receiver LV routes added - AGENTS.md ip_allowed?/2 quirk note updated to reflect the fix New test files (~95 tests): - ApiControllerEndpointsTest (36): all 10 actions × validation/auth/ authz/happy-path/missing-fields branches - SocketPlugTest (11): code-based and token-based auth, IP/hours/ download-limit/password gates - SyncWebsockCallbacksTest (12): WebSock callbacks (init/handle_in/ handle_info/terminate), all 5 message events - SyncSocketTest (5) + SyncChannelTest (9): code-based protocol via Phoenix.ChannelTest - ConnectionNotifierTest (11): cross-site HTTP via the test endpoint acting as the mock remote - WebSocketClientTest (4): self-loop end-to-end through localhost WebSocket - SenderLiveTest + ReceiverLiveMountTest: initial-form mount Real bugs fixed (per "fix everything we find"): - ApiController.fetch_filtered_records hardcoded `pk_col` to "id" via PhoenixKit.RepoHelper.get_pk_column for unknown tables. Tables with UUIDv7 PKs (like phoenix_kit_sync_connections itself) failed with "column 'id' does not exist". Now resolves PK via SchemaInspector.get_primary_key/2 with safe fallback. - Connection.ip_allowed?/2 returned false for empty/nil whitelist (the AGENTS.md:140 quirk). Added 2-arity clauses for [] and nil so empty whitelist correctly means "allow all", matching the 1-arity behaviour. SocketPlug calls with a real client IP now work for any connection without an explicit whitelist. - TableSchema and ColumnInfo didn't derive Jason.Encoder. Any request:schema event over the WebSocket protocol crashed the channel with Protocol.UndefinedError. Both now @derive Jason.Encoder. - ApiController register_connection: validate_password silently accepted ANY password when require_password mode was set but no password was configured. Now logs a warning and rejects with :password_required. mix precommit clean.
… pubsub constant
Findings from the three triage Explore agents + ast-grep stale-ref pass.
"Fix everything we find" — six real items closed, plus a duplicate of
one fixed in sync_channel.ex that ast-grep caught.
1. HIGH — auth_token_hash leaked into log
`api_controller.ex:351` was `Logger.info("...with params: #{inspect(params)}")`,
which serialised the full params map including the sensitive
`auth_token_hash`. Replaced with explicit safe-field logging
(`receiver_url`, `has_token_hash` boolean only).
2. MEDIUM — Map.fetch! on attacker-controlled WebSocket payload
`sync_websock.ex:300-301` and `sync_channel.ex:143-144` both used
Map.fetch!/2 on the "table" and "ref" keys of the request:records
payload. A malicious sender omitting either key crashed the
handler with KeyError — DoS vector and a reconnect storm trigger.
Both rewritten to match-and-validate with structured rejection.
3. HIGH — ungettext'd user-facing strings in Receiver.Helpers
`format_strategy/1` ("Skip existing", "Overwrite existing", …) and
`format_connection_error/1` ("Connection timed out…", "Could not
connect…") returned hardcoded English. Now `use Gettext, backend:
PhoenixKitWeb.Gettext` and every literal wrapped in `gettext/1`;
the dynamic-reason error path uses `gettext/2` with `%{reason}`
placeholder for proper extraction.
4. MEDIUM — extract_sync_counts/1 ungettext'd error strings
The sync result extractor in `connections_live.ex:1052-1068`
surfaced "Sender is offline" / "Unauthorized — check connection
token" / "Table not found on sender" / "Sync failed: …" / "Unknown
error" as raw strings that flowed into the UI. All now wrapped
in gettext (the dynamic case uses gettext/2 with %{reason}).
5. MEDIUM — ImportWorker.perform/1 had no activity-log audit row
The Receiver's WebSocket → Oban import path completed silently
from the audit feed's perspective (Transfers.create_transfer logs
the request, but each batch's actual import didn't). Added
`log_batch_completion/5` writing a `sync.import.batch_completed`
row with table_name / session_code / batch_index / strategy /
created+updated+skipped+error_count counts. Mode is `"auto"`
(Oban-driven, not a person). Guarded with Code.ensure_loaded? +
rescue per the established pattern.
6. MEDIUM — PubSub topic was a duplicated string literal
`"sync:connections"` was hardcoded at both the broadcast site
(`connections.ex:102`) and the subscribe site
(`connections_live.ex:34`) — two places to keep in sync. Promoted
to `Connections.pubsub_topic/0` (public, @doc'd, @SPEC'd) so
subscribers always reference the canonical source.
ast-grep stale-ref pass clean: no bare Task.start in production code
(only the documented :exit fallback in AsyncTasks); no == on
hashes/passwords; no string_to_pid/pid_to_string/escape_value/phx-
value-pid leftovers; no untranslated put_flash; no remaining
hardcoded "sync:connections" literal outside the constant function.
mix precommit clean (compile + format + credo --strict + dialyzer).
511 tests, 0 failures, 5/5 stable consecutive runs.
… strings Pin three classes of fixes from the deep-dive sweep (15fc9bc, 269124e) that shipped without dedicated pinning tests: - Log redaction (test/integration/log_redaction_test.exs, 4 tests): capture_log assertions that auth_token_hash, raw auth_token, and rejected passwords never appear verbatim in Logger output for the /sync/api/get-connection-status and /sync/api/register-connection endpoints. - DoS hardening on request:records (sync_channel_test.exs +4, sync_websock_callbacks_test.exs +4): missing 'table', missing 'ref', wrong-type 'table' (integer), and empty payload all return a structured :error reply instead of crashing the channel/WebSock process. - Gettext-translated extract_sync_counts/1 strings (connections_live/extract_sync_counts_test.exs, 7 tests): one assertion per error variant (:offline, :unauthorized, :table_not_found, binary passthrough, unknown-term interpolation, unrecognised result shape). To make this directly testable without driving a full sync flow, extract_sync_counts/1 changed from defp to @doc false def — public-but-not-API.
…xt gaps A second triage pass on the post-sweep code surfaced a small batch of real issues that slipped past the first round. Each fix has a pinning test added in this commit. Security - api_controller.ex:395 logged the full auth_token_hash on the not-found branch (the line above already redacted it). Mirror the redacted shape — `has_token_hash=true|false` only. Activity logging — actor_uuid threading Three connections_live.ex handlers were calling the context without threading the admin's UUID through, so the activity row landed with actor_uuid=nil for delete, reactivate, and regenerate_token. The context functions already accept opts; the LV just wasn't passing them. - delete_connection (line 360) - reactivate_connection (line 296) - regenerate_token (line 339) Form UX - reject_self_connection returned a changeset without :action set, so inline errors wouldn't render via <.input>. The error string was also a raw English literal — wrapped in gettext/1 + added `use Gettext, backend: PhoenixKitWeb.Gettext` to Connections. Translation gap - "DB Sync module is disabled." in connections_live.ex:1347 was not gettext-wrapped. Misc - start_sync button missing phx-disable-with. - SessionStore.start_link/1 missing @SPEC. Pinning tests - errors_test.exs: :table_not_allowed atom (was missing from the table even though it's emitted by api_controller in 3 places). - log_redaction_test.exs: capture_log assertion that the not-found branch never echoes the auth_token_hash. - connections_test.exs: rejected self-connection returns a changeset with action == :insert and the gettext'd error message. - connections_live_test.exs: delete LV test now asserts actor_uuid in the activity row. Added context-layer pins for reactivate + regenerate_token (their buttons live in the connection detail panel, not the list grid, so context-layer pinning matches the existing approve test pattern). Verification - mix precommit clean (compile + format + credo --strict + dialyzer). - 536 tests, 0 failures, 10/10 stable runs. - Browser smoke: Sync overview / Connections / History admin pages render cleanly with sidebar + header + main panel intact.
…ttext, pgcrypto Closes the post-Apr playbook deltas the original sweep predates: - enabled?/0 adds catch :exit, _ -> false (sandbox-shutdown trap) - handle_info catch-alls: silent → Logger.debug on connections_live / sender / receiver; defensive catch-alls added to history + index (which had no clause at all) - phx-disable-with on 7 destructive phx-click buttons: approve_connection, reactivate_connection, approve_transfer, transfer_detail_table, start_transfer, generate_code, regenerate_code - 12 hardcoded English heex strings wrapped in gettext (badges, legend labels, data-tip, loading states, placeholders) - 3 IO.puts calls inside @doc examples replaced with comments - pgcrypto extension added to test_helper.exs alongside uuid-ossp - AGENTS.md gains "What This Module Does NOT Have" section with the seven deliberate non-features +17 pinning tests in test/phoenix_kit_sync/batch_2_revalidation_test.exs (553 total, 10/10 stable runs); each Batch 2 production change has at least one assertion that would fail on revert. mix precommit clean: compile, format, credo --strict, dialyzer (9 pre-existing skips). Browser smoke confirms structural parity with pre-batch baselines for /sync, /sync/connections, /sync/history. Surfaced for fix-everything decision (not in this commit): SSRF guard on connection.site_url; activity logging gaps on update_connection + 5 mutation :error branches; ~45 @SPEC backfills; component swap on connections_live form fields. See PR BeamLabEU#1 FOLLOW_UP.md for details.
…specs (A) SSRF guard on connection.site_url — Connection.changeset/2 now runs validate_base_url/1 + internal_host?/1 + internal_ip?/1 helpers that reject non-http(s) schemes, missing host, localhost literal, *.local mDNS, RFC1918 / loopback / link-local IPv4, and ::, ::1, fe80::/10, fc00::/7 IPv6. Opt-in bypass via `config :phoenix_kit_sync, allow_internal_urls: true` for deployments that legitimately point at localhost / RFC1918 (multi-tenant on one host, internal staging, self-hosted instances). Test config flips the bypass on so existing localhost-targeted integration tests pass; the dedicated SSRF suite (+20 tests) explicitly toggles it back off. DNS-rebinding (public host → internal IP at request time) is deliberately out of scope per the AI module precedent — the acute threat is the literal-IP shape (cloud metadata is always 169.254.169.254 literal). (B) Activity-logging gaps closed — update_connection/2 promoted to update_connection/3 with `opts \\ []` (backward-compat). Both branches log sync.connection.updated; :ok carries metadata.changed_fields, :error carries the same plus db_pending: true. ConnectionsLive now threads actor_uuid through do_update_connection/2. The five status- mutation :error branches (delete / approve / suspend / revoke / reactivate) gain log_sync_activity calls with db_pending: true. Pinned by 8 new tests — runtime tests for update_connection branches, source- grep structural pins for the change/2-based mutation error paths (unreachable in normal operation but defensively logged). (C) @SPEC backfill — added 14 specs across the two schema modules. Connection: 7 changeset + 9 helper specs. Transfer: 16 specs covering all changeset and predicate functions. Widened get_connection/1 and get_transfer/1 specs to accept any input so the catch-all clauses type-check. (D) Component refactor reclassified as N/A — the C12 agent's count of "4 raw <input>/<select>/<textarea>" was a miscount. All 4 <select> elements already use the daisyUI 5 wrapper pattern (<label class="select ..."><select>...</select></label>); the 2 <input> are type="hidden" with static values where <.input> would add nothing. No diff needed; the FOLLOW_UP records the re-inspection. 553 → 581 tests, 10/10 stable runs, mix precommit clean (compile, format, credo --strict, dialyzer 9 pre-existing skips). AGENTS.md "What This Module Does NOT Have" updated to describe the guard and the residual DNS-rebinding gap. Production behaviour change: deployments using localhost / RFC1918 URLs need to set `config :phoenix_kit_sync, allow_internal_urls: true` in their host app. Public-host deployments are unaffected.
mdon
changed the title
ddon
added a commit
that referenced
this pull request
Apr 29, 2026
Post-merge review covering the 70-file quality-sweep PR. No critical issues; 10 findings logged across security (alt-form IPv4 bypass in SSRF guard), architecture (terminate/2 cleanup that won't fire, Iron Law violation in ConnectionsLive.mount, silent rescue in log_sync_activity, empty-change audit noise), and code quality (typing-fiction specs, untranslated literal, dead safe_atom lookups, actor_uuid threading asymmetry). Eight follow-up PRs sketched with pinning tests; F1–F4 bundle as one mechanical commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mdon
pushed a commit
to mdon/phoenix_kit_sync
that referenced
this pull request
Apr 30, 2026
…es, gettext Bundles four mechanical fixes from the PR BeamLabEU#5 CLAUDE_REVIEW. Each carries a pinning test (runtime where DB-runnable, source-pin where the failure mode is hard to reach without mocking the activity backend). F1 — phoenix-thinking Iron Law on ConnectionsLive.mount/3 Gate `load_connections/1` behind `connected?(socket)` so the dead render seeds empty assigns and the doubled DB queries + async HTTP fan-out only fire on the live phase. Mirrors the existing history.ex:43 pattern. Pinned by two tests: dead render must not contain a connection name, and the live render still must. F2 — log_sync_activity/4 rescue is no longer silent The bare `_ -> :ok` rescue swallowed every audit-log failure. Replaced with `e -> Logger.warning(...)` carrying action, connection_uuid, and the exception message. Pinned by a source regex (forcing PhoenixKit.Activity.log/1 to raise reliably in tests requires dropping the activities table — sandbox-unsafe — or mocking, which the suite doesn't use). F3 — empty-change update no longer writes a noise audit row `update_connection/3` was producing `"updated"` rows with `changed_fields = []` on no-op saves. Wrapped the log_sync_activity call in `if changed_fields != []`. Pinned by two tests: a same-value update writes zero rows; a real change still writes exactly one (regression guard in case the empty-changes detector ever misfires). F4 — gettext-wrap "Revoked by admin" reason Last English literal in the LV's status mutations; persisted to `revoked_reason` and surfaced in the audit feed. Pinned by asserting the persisted reason equals the gettext output for the source string. Compile clean (`mix compile --warnings-as-errors`). Runtime DB tests will be exercised by the next full `mix test` run on a host with the test DB available — the sandbox here lacks psql, so only the source regex (F2) is verified locally. Refs dev_docs/pull_requests/2026/5-quality-sweep/CLAUDE_REVIEW.md findings F1, F2, F3, F4 / final follow-up table. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
ddon
pushed a commit
that referenced
this pull request
May 5, 2026
Replace the inline DDL (uuid_generate_v7 fn, phoenix_kit_activities stub, phoenix_kit_settings stub) plus the `Ecto.Migrator.up(TestRepo, 0, PhoenixKitSync.Migration, ...)` call in `test/test_helper.exs` with `PhoenixKit.Migration.ensure_current/2` (core 1.7.105+ / phoenix_kit#515). Sync tables are owned by core (V37 creates them as `phoenix_kit_db_sync_*`; V44 renames to `phoenix_kit_sync_*`; V56/V58/V61/V73/V74 evolve them). phoenix_kit_settings (V03), phoenix_kit_activities (V90), and the uuid_generate_v7 function (V40) are also owned by core. Schema drift between test and prod is impossible by construction. Two pre-existing test failures surfaced — both committed in 7916940 ("PR #5 follow-up bundle (F1–F4)") whose own message admits "DB tests will be exercised by the next full mix test run on a host with the test DB available". The old inline DDL had been masking the real LV behavior. Both skipped with `@tag :skip` and clear comments naming the bug + intended fix; out of migration-cleanup scope: - F1 Iron Law dead-render test — `handle_params/3`'s catch-all `handle_action` clause calls `load_connections/1` unconditionally, bypassing the `connected?` gate that F1 added to `mount/3`. Needs a one-line LV fix: swap `load_connections()` → `maybe_load_connections()` in `connections_live.ex:106`. - F4 revoke gettext test — looks for `[phx-click='revoke_connection']` in the list view, but the revoke button only exists in the connection detail view (`connections_live.ex:1940`). Test needs a navigate-then- click flow or a context-layer pin. Companion edits: - `AGENTS.md`: Database & Migrations / Testing sections updated. Depends on BeamLabEU/phoenix_kit#515 — CI will be red until core 1.7.105 publishes.
ddon
added a commit
that referenced
this pull request
May 5, 2026
Post-merge review covering the migration-cleanup PR: 4 files, +24/-73, 587/0/0 across 3 stable runs. Approve with one follow-up logged and fixed on-branch. Strengths called out: single-source-of-truth schema setup via core's ensure_current/2 (eliminates schema drift class entirely), correct F1/F4 fixes that resolve the rows sketched in PR #5's CLAUDE_REVIEW, the upstream ensure_current/2 design (microsecond-precision Migrator trick + table-comment short-circuit), good comment locator hygiene at test_helper.exs:41-56. Findings: - (1) merged-with-red-CI process pattern, post-mortem only since core 1.7.105 has since published and is pinned in mix.lock - (2) bundled-PR scope creep — nit - (3) latent Iron Law gap on deep-link entry to ?action=show|edit|sync (pre-existing; F4 test surfaced it). FIXED on-branch in the preceding commit. - (4) cosmetic schema_migrations row accumulation per mix test invocation — no action No outstanding follow-ups from this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
End-to-end Phase 2 sweep on phoenix_kit_sync, then a 2026-04-26 re-validation pass that surfaced and fixed eight more deltas the original sweep predates.
PR follow-up(s)
14474cd— Fix PR Add PhoenixKitSync external module extracted from PhoenixKit #1 follow-up: SQL injection (parameterised identifiers +Plug.Crypto.secure_compare), timing-unsafe compares, mount guard, PID round-trip → UUIDv7 tokens.ccaf052— Fix PR Add test suite, fix sync bugs, improve connection management and UI #2 follow-up: task supervision (Task.start_linkvsTaskSupervisor.start_child),changed_fieldsatom-key normalisation, scoped decimal parsing.42dce3a— Stub FOLLOW_UPs for PRs Fix hex dep, add CSS scanning, update to daisyUI 5 components #3 and Add routing anti-pattern pointer to AGENTS.md #4 (no findings).f227b94— Clarify FOLLOW_UP Add PhoenixKitSync external module extracted from PhoenixKit #1 scope: security features (HMAC signing, rate limiting) are out-of-scope per quality-sweep policy, not deferred.Quality sweep
Documentation
Error handling
PhoenixKitSync.Errorsatom dispatcher with one literalgettext/1clause per atom (38 atoms, all extractor-friendly).{:error, _}site to return tagged tuples; binary errors only as documented passthrough from Postgres in DataImporter.Activity logging
log_sync_activity/4helper in Connections (guarded withCode.ensure_loaded?+ rescue, PII-safe metadata: name/direction/status only — never site_url, tokens, or notes).created,approved,suspended,revoked,reactivated,deleted,token_regenerated, plus Transfers'created/completed/failed/approved/denied.actor_uuidthreaded through every LiveView event handler.Forms & UX
phx-disable-withon every submit + destructive button +start_sync.validateevent setsMap.put(changeset, :action, :validate)so<.input>renders inline errors.:action = :insertand the error string is gettext-wrapped.Code cleanup
AsyncTasks,Web.Receiver.Helpers,Web.ConnectionsLive.Status,ConnectionNotifier.Prepare), 604 lines moved.ANY($1)batch.@specon every public function in context modules + schemas;@type ton every Ecto schema.Connections.pubsub_topic/0; no hardcoded topic strings.handle_infocatch-all on Connections / Receiver / Sender LVs.request:records(sync_channel + sync_websock) — match-and-validate replacesMap.fetch!.Tests
The sweep brought the suite from 316 → 536 tests (10/10 stable). Delta-pinning highlights:
errors_test.exs— 37 atom clauses tested by literal expectation (notis_binary).activity_log_assertions.exhelper + 14 per-action assertions across Connections + Transfers (every CRUD mutation, sync, toggle).log_redaction_test.exs—capture_logassertions that auth_token, auth_token_hash, and rejected passwords never appear in Logger output across 3 endpoints.phx-disable-withattr presence, flash text, status select binding all explicitly asserted in LV smoke tests.Test.Endpoint/Test.Router/Test.Layouts(with flash rendering) /LiveCase/ConnCase/ChannelCase,lazy_htmltest-only dep, test-only migrations matching the realphoenix_kit_settingscolumn shape and includingphoenix_kit_activities.C12 fixes within the original sweep (
00b26f8)A second triage pass surfaced 8 small issues that slipped past the first round:
api_controller.ex:395was logging the fullauth_token_hashon the not-found branch (the line above already redacted it). Fixed to mirror the redacted shape.actor_uuidto the context. Activity rows were loggingactor_uuid=nil. Fixed all three.reject_self_connectionreturned a changeset withaction=nil, so<.input>wouldn't render the inline error. Also a literal English error string. Fixed both."DB Sync module is disabled."was not gettext-wrapped.start_syncbutton missingphx-disable-with;SessionStore.start_link/1missing@spec.:table_not_allowedatom (emitted byapi_controllerin 3 places) was missing fromerrors_test.exs.Each fix has a pinning test added in the same commit.
Re-validation — Batch 2 (
7949c22, 2026-04-26)Second-pass triage against the post-Apr playbook. The original sweep predates several C12 prompt categories; this batch closes the in-scope structural deltas:
enabled?/0gainedcatch :exit, _ -> falsefor the sandbox-shutdown trap (previously had onlyrescue _ -> false, which the workspace AGENTS.md flaky-test note flags as a 1-in-10 unit-test flake).handle_infocatch-alls — silent{:noreply, socket}onconnections_live/sender/receiverupgraded toLogger.debug("[<LV>] unhandled message | msg=..."). Defensive catch-all clauses added tohistory.exandindex.ex, which had nohandle_info/2at all (a stray PubSub broadcast would have raisedFunctionClauseError).phx-disable-withadded to 7 destructivephx-clickbuttons missed in the original sweep:approve_connection(Approving…),reactivate_connection(Reactivating…),approve_transfer(Approving…),transfer_detail_table(Transferring…),start_transfer(Starting…),generate_code(Generating…),regenerate_code(Regenerating…). Stops double-clicks from issuing duplicate approvals / generates / transfers.gettext— badges (Enabled,Disabled), legend labels (Sender,Local,Record counts:,= differs), tooltipdata-tip("Used by selected tables — consider including"), loading states (Loading table schema…,Creating…), placeholders (From,To,Reason (optional)).IO.putscalls inside@docexample blocks replaced with comments. Examples shouldn't show debug output.pgcryptoextension added totest/test_helper.exsnext touuid-ossp.uuid_generate_v7()depends ongen_random_bytesfrom pgcrypto; on a freshcreatedbwithout it, every UUID-defaulted insert in tests would have failed (the existing test DB had it from a prior install, masking the trap).+17pinning tests landed intest/phoenix_kit_sync/batch_2_revalidation_test.exs. Phase 1 PR triage re-verified clean (PRs #1–#4 fixes still in place on 2026-04-26).Re-validation — Batch 3 fix-everything pass (
abd1a90, 2026-04-26)Authorised after Batch 2 surfaced four deferrable items. Closes A (SSRF), B (activity-logging gaps), C (
@specbackfill), and D (component refactor — reclassified as N/A, see below).(A) SSRF guard on
connection.site_url. Addedvalidate_base_url/1+internal_host?/1+internal_ip?/1toConnection.changeset/2. Rejects:http/httpsschemes (file://,gopher://,javascript:),localhostliteral,*.localmDNS hostnames,0/8,10/8,127/8,169.254/16,172.16/12,192.168/16,::,::1,fe80::/10,fc00::/7.Opt-in bypass via
config :phoenix_kit_sync, allow_internal_urls: true. Defaultsfalsein production; test config flips it on so the existing localhost-targeted integration tests work. The dedicated SSRF suite explicitly toggles bypass back tofalseto verify rejections. DNS-rebinding (public host → internal IP at request time) is deliberately out of scope — the acute threat is the literal-IP form (cloud metadata is always169.254.169.254).Pinned by 20 tests in
test/phoenix_kit_sync/connection_ssrf_test.exs.(B) Activity-logging gaps. Two gaps:
update_connection/2had zero activity logging on any branch — modifications toallowed_tables/max_downloads/download_passwordwere never audited. Promoted signature toupdate_connection/3withopts \\ [](backward-compat). Both branches now logsync.connection.updated::okcarriesmetadata.changed_fields = [...];:errorcarries the same plusdb_pending: true.ConnectionsLive.do_update_connection/2now threadsactor_uuid: socket.assigns.phoenix_kit_current_scope.user.uuid.:errorbranches were silent. Addedlog_sync_activity("<verb>", connection, opts, %{"db_pending" => true})to the error path ofdelete_connection,approve_connection,suspend_connection,revoke_connection,reactivate_connection. Suspend/revoke also carry"reason"in the error metadata.Pinned by 8 tests in
test/phoenix_kit_sync/connections_activity_test.exs(runtime tests forupdate_connection/3branches via forced validation failure; structural source pins for the 5change/2-based mutation error paths, which are runtime-only — defensively logged).(C)
@specbackfill — added 14 specs across the two schema modules:lib/phoenix_kit_sync/connection.ex— 7 changeset specs + 9 helper specs (verify_auth_token,verify_download_password,generate_auth_token,active?,expired?,within_download_limits?,within_record_limits?,within_allowed_hours?,ip_allowed?/1+ip_allowed?/2,requires_approval?,table_allowed?).lib/phoenix_kit_sync/transfer.ex— 16 specs covering all 11 changeset functions and 5 predicate / utility helpers.Connections.get_connection/1andTransfers.get_transfer/1specs fromString.t()toString.t() | any()so the catch-alldef get_X(_), do: nilclauses type-check.(D) Component refactor reclassified as N/A. The original C12 agent counted "4 raw
<input>/<select>/<textarea>not yet swapped to core". Re-inspection shows all 4<select>elements (connections_live.ex:2179, 2452, 2696,receiver.ex:1302) already use the daisyUI 5<label class="select ...">wrapper pattern; the 2<input>elements aretype="hidden"carrying static values where<.input>would add nothing. No diff needed. The agent's count was wrong; the FOLLOW_UP records the re-inspection.Production behaviour change in Batch 3: the SSRF guard is on by default. Deployments using localhost / RFC1918 /
.localURLs needconfig :phoenix_kit_sync, allow_internal_urls: truein their host app. Public-host deployments are unaffected. AGENTS.md "What This Module Does NOT Have" updated to describe the guard and the residual DNS-rebinding gap.536 → 581tests cumulative across both re-validation batches (+17 Batch 2, +28 Batch 3).Verification
mix precommitclean (compile + format + credo --strict + dialyzer; 9 pre-existing dialyzer skips, all justified).IO.inspect/puts/warnoutside@doc, no# TODO/# FIXME/# HACK/# XXX, noString.capitalize, no rawTask.start(the loneasync_tasks.ex:28match is the documented:exitrescue fallback).Test plan
mix format --check-formattedcleanmix credo --strictcleanmix dialyzerclean (9 pre-existing skips).local, non-http(s) schemes)allow_internal_urls: truebypass verified