FEATURE: Can edit category/host relationships for embedding

Golden recall: 75% Extra findings: 23

Golden Comments (3/4 found)

#	Comment	Severity	Status	FriendlyReviewer Detail
#1	NoMethodError before_validation in EmbeddableHost	HIGH	✓ Found	"`before_validation` calls `sub!` directly on `host`, raising `NoMethodError` and producing a 500 instead of a validation failure when a record is created/updated without a host" (embeddable_host.rb L6)
#2	The update and destroy methods in Admin::EmbeddableHostsController do not validate the existence of the EmbeddableHost record retrieved by ID. If EmbeddableHost.where(id: params[:id]).first returns nil (i.e., the host does not exist), attempting to call methods on the nil object (e.g., save_host or destroy) will result in a NoMethodError.	MEDIUM	✓ Found	"`update` and `destroy` fetch the record with `EmbeddableHost.where(id: ...).first`, returning `nil` when no record matches; `save_host(nil)` and `host.destroy` then raise `NoMethodError` (500) instead of a 404" (embeddable_hosts_controller.rb L9)
#3	record_for_host compares lower(host) = ? but does not normalize the parameter's case, so mixed-case referer hosts may fail to match even though comparison intends to be case-insensitive.	MEDIUM	✗ Not found	The review identifies case-sensitivity elsewhere — the `sub!` normalization is case-sensitive while the validation regex is `/i`, and the scheme/port mismatch of migrated values is flagged as a red finding — but no candidate takes the final step: `record_for_host` compares `lower(host) = ?` without lowercasing the parameter itself, so mixed-case referers still fail to match. A genuinely adjacent bug, one step further in the query. See `app/models/embeddable_host.rb`
#4	Because this migration inserts embeddable_hosts rows with raw SQL, any existing embeddable_hosts values that include http:// or /https:// or path segments won't go through the EmbeddableHost model's normalization, so the new host lookup (which compares only the bare host) may fail for migrated data. Consider ensuring that migrated hosts are normalized to the same format as newly created EmbeddableHost records so existing embedding configurations keep working.	HIGH	✓ Found	"The migration inserts the legacy host value verbatim, without applying the model's `before_validation` normalization (stripping scheme/path). Legacy values often included `http(s)://` prefixes, so after migration such rows will never match `record_for_host`'s `uri.host` comparison and embedding will silently fail" (db/migrate/20150818190757_create_embeddable_hosts.rb L25)

Supplementary Findings (23 findings)

Finding	File	Severity	Legitimate?
The validation regex allows an optional port and `before_validation` does not strip it — a host like `example.com:8080` is persisted while `record_for_host` compares `uri.host` (no port), so it can never match	embeddable_host.rb:6	HIGH	✓ Real: hosts with a port are permanently unmatchable — silently breaks embeds
Migration does `execute(...)[0]['id'].to_i` unguarded — `[0]` is nil on any install where `embed_category` is unset or matches nothing, aborting the migration	migration:11	HIGH	✓ Real: this is the default case, so the migration fails on most installs
Same nil-indexing on `uncategorized_category_id` — `execute(...)[0]['value'].to_i` raises if the site_settings row is absent	migration:15	HIGH	✓ Real: same crash, one query away from the previous one
`execute "INSERT INTO embeddable_hosts ... VALUES ('#{h}', ...)"` interpolates the raw admin-editable host string into SQL — a quote or metacharacter can break the migration or inject statements	migration:25	HIGH	✓ Real injection surface, admin-gated but still raw SQL interpolation
Legacy hosts are stored verbatim (with scheme/path) while `record_for_host` compares `lower(host) = uri.host` — migrated rows never match, silently disabling previously working embeds	topic_retriever.rb:16	HIGH	✓ Real: the migration's data loss consequence; adjacent to golden #4
Scheme-stripping `sub!` is case-sensitive (`/^https?:\/\//`) while the validation regex is `/i` — `HTTP://example.com` is not normalized and fails the format regex	embeddable_host.rb:6	MEDIUM	✓ Real inconsistency between normalization and validation
The migration runs destructive logic (INSERT plus `DELETE FROM site_settings` for the legacy keys) inside `def change` — automatic reversal only drops the table, permanently losing the legacy settings	migration:2	MEDIUM	✓ Real: `down` cannot restore the destroyed settings
`params[:embeddable_host][:host]` and `[:category_id]` are accessed without a require/presence guard — `NoMethodError` (500) on empty or malformed requests	embeddable_hosts_controller.rb:22	MEDIUM	✓ Real: crashes on malformed input instead of a clean 4xx
`host.category_id = SiteSetting.uncategorized_category_id if host.category_id.blank?` runs unconditionally inside `save_host` — also fires on `update`, silently resetting a host's category to uncategorized	embeddable_hosts_controller.rb:25	MEDIUM	✓ Real: an update without category_id silently changes data
`EmbeddingController#update` simply re-renders the current `@embedding` and never persists anything — a PUT that changes nothing	embedding_controller.rb:9	MEDIUM	✓ Real no-op action with update semantics
Both controller specs only assert that the controller subclasses `Admin::AdminController` — zero coverage of save_host, category defaulting, the JSON contract, authorization, or routes	embeddable_hosts_controller_spec.rb:5	MEDIUM	✓ Real: the specs guard none of the new behavior
Removing the public `self.allows_embeddable_host?` method is a breaking API change for out-of-tree plugins; in-repo consumers are all migrated	site_setting.rb:68	MEDIUM	✓ Borderline: safe in-repo, breaking for external consumers; a shim would be cheap
`resources :embeddable_hosts` generates `index`/`new`/`show`/`edit` routes that map to actions not implemented — runtime `ActionNotFound` on dead routes	config/routes.rb:153	MEDIUM	✓ Real: dead routes that 404 at the action layer
The 60-second Redis throttle is keyed on the raw, unnormalized `@embed_url` and only checks the host — many distinct URLs on one allowlisted host bypass the throttle window, an amplification/DoS vector	topic_retriever.rb:24	MEDIUM	✓ Real amplification vector: distinct keys per URL on a single host
`eh.try(:category_id)` silently falls back to nil (uncategorized) with no warning when no allowlisted host matches the import URL — including all migrated scheme/port hosts	topic_embed.rb:36	MEDIUM	✓ Real silent behavior regression for unmatchable hosts
The rewritten `expandable_first_post?` spec is vacuous — it would pass even if the `embed_truncate?` check were removed, and fabricates an `EmbeddableHost` the method never queries	topic_spec.rb:1403	MEDIUM	✓ Real: a test that asserts nothing about the behavior under review
The two fabricator files have their contents swapped relative to their filenames — `category_fabricator.rb` defines the `:embeddable_host` fabricator	spec/fabricators	LOW	✓ Real confusion for maintainers; functionally harmless
`before_filter :ensure_logged_in, :ensure_staff` is redundant in both admin controllers — `Admin::AdminController` already declares these filters	embeddable_hosts_controller.rb:3	LOW	✓ Borderline: noise only, diverges from project convention
The explicit `embed: :ids` on `has_many :embeddable_hosts` is redundant with `ApplicationSerializer`'s default `embed :ids, include: true`	embedding_serializer.rb:3	LOW	✓ Borderline: harmless redundancy, output is correct
The new `admin.embedding.edit` locale key (`edit: "edit"`) is never referenced by client code	client.en.yml:2497	LOW	✓ Real dead locale key
`response.headers['X-Frame-Options'] = "ALLOWALL"` is not a valid X-Frame-Options value (`DENY`/`SAMEORIGIN`/`ALLOW-FROM`) — the header is ignored by browsers; the actual gate is the client-controlled referer allowlist	embed_controller.rb:64	LOW	✓ Real, but pre-existing and outside this PR's scope
Collapsing the previous two-step check into a single `EmbeddableHost.host_allowed?` loses the distinct "no hosts configured" diagnostic — now an indistinguishable 403	embed_controller.rb:61	LOW	✓ Borderline: diagnostic regression, same 403 either way
Removing `SiteSetting.embeddable_hosts.present?` means a topic with a `topic_embed` reports `expandable_first_post? == true` even with no hosts configured — unprotected, no spec	topic.rb:868	LOW	✓ Borderline: likely acceptable behavior, unprotected against regression

Honest Analysis

FriendlyReviewer found three of the four golden comments of this PR — recall 75%. Golden #1 (the sub! on a nil host raising NoMethodError instead of a validation failure) is found at embeddable_host.rb L6; golden #2 (update/destroy on a nil record producing a 500 instead of a 404) at embeddable_hosts_controller.rb L9; golden #4 (the raw-SQL migration storing legacy hosts unnormalized, so they never match record_for_host) at the migration L25.

The miss, golden #3, is the subtle one: record_for_host compares lower(host) = ? without lowercasing the parameter, so mixed-case referers fail to match. The review did identify case-sensitivity elsewhere — the sub! normalization is case-sensitive while the validation regex is /i, and the scheme/port mismatch of migrated values is a red finding — but no candidate took the final step of noting that the lookup input itself is not normalized. It is a genuinely adjacent bug: same mechanism, one step further in the query. The dataset's case is a fair miss.

The supplementary set is the largest of the run and mostly high-value: the migration crashing on typical installs (nil-indexing on both lookups), the SQL interpolation of admin-editable host text, the port-allowing regex making hosts permanently unmatchable, dead routes to non-existent actions, the no-op EmbeddingController#update, and controller specs that assert nothing. The honest caveats: several reds are near-duplicates (two findings at L6, two at migration L25), the "8 high" count overstates independence, and the blue items (swapped fabricators, redundant before_filter, dead locale key) are noise. On balance the review is solid on this PR — the one miss is narrow and well explained by the adjacent angle it did cover.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech	🌤️ Feat
8 high, 11 medium, 7 low	1 mitigated

This MR introduces a new EmbeddableHost model/table and admin API to replace the legacy SiteSetting.embeddable_hosts/embed_category allowlist, with the goal of letting admins edit host/category relationships for embedding. The architectural direction is sound and the new model/serializer/fabricators are mostly coherent, but the implementation is not ready to merge: multiple correctness defects would cause runtime crashes (nil host handling in the model and controller, and migration crashes on typical installs), the data migration is not reversible and interpolates admin-provided text into SQL, hosts containing a scheme or port are stored in a form that the allowlist can never match, the EmbeddingController#update action is a no-op, unnecessary routes are generated for actions that do not exist, and the new controller specs exercise almost none of the added behavior. The legacy SiteSetting.allows_embeddable_host? method removal is advisory (no in-repo callers remain), and there is a minor dead locale key. Several of the issues are blocking and require rework before merge.

These might need a close look

🔴 app/models/embeddable_host.rb (L6)
before_validation calls sub! directly on host, raising NoMethodError and producing a 500 instead of a validation failure when a record is created/updated without a host. Guard the value (e.g. self.host = self.host.to_s.sub(/^https?:\/\//, '')).
🔴 app/models/embeddable_host.rb (L6)
The validation regex allows an optional port, and before_validation does not strip it, so a host like example.com:8080 is persisted. record_for_host compares against uri.host (which excludes the port), so such a host can never match host_allowed?. Either strip the port in normalization or disallow it in the regex.
🔴 db/migrate/20150818190757_create_embeddable_hosts.rb (L11)
The migration does execute(...)[0]['id'].to_i without guarding against an empty result set. On any install where embed_category is unset or matches no category, [0] is nil and the migration aborts with NoMethodError. This is the default/common case, so the migration fails on most installs.
🔴 db/migrate/20150818190757_create_embeddable_hosts.rb (L15)
Same nil-indexing issue as above for uncategorized_category_id: execute(...)[0]['value'].to_i raises NoMethodError if the site_settings row is absent. Guard with rows.any? before indexing.
🔴 db/migrate/20150818190757_create_embeddable_hosts.rb (L25)
execute "INSERT INTO embeddable_hosts ... VALUES ('#{h}', ...)" interpolates the raw admin-editable host string into SQL. A value containing a quote or SQL metacharacter can break the migration or inject statements. Use parameterized statements or sanitize the value.
🔴 db/migrate/20150818190757_create_embeddable_hosts.rb (L25)
The migration inserts the legacy host value verbatim, without applying the model's before_validation normalization (stripping scheme/path). Legacy values often included http(s):// prefixes, so after migration such rows will never match record_for_host's uri.host comparison and embedding will silently fail.
🔴 app/controllers/admin/embeddable_hosts_controller.rb (L9)
update and destroy fetch the record with EmbeddableHost.where(id: ...).first, returning nil when no record matches; save_host(nil) and host.destroy then raise NoMethodError (500) instead of a 404. Use .find(params[:id]) (or explicitly guard nil) to follow project convention and return a 404.
🔴 lib/topic_retriever.rb (L16)
invalid_host? (and EmbedController#ensure_embeddable) rely on EmbeddableHost.record_for_host, which compares lower(host) = uri.host. Because the migration stores legacy hosts with scheme/path and the model allows ports, migrated hosts like http://example.com or example.com:8080 will never match — silently disabling previously-working embeds. Coordinate normalization and port handling across the model, migration, and this code path.

Worth checking

🟡 app/models/embeddable_host.rb (L6)
The scheme-stripping sub! is case-sensitive (/^https?:\/\//), while the validation regex is /i. A host entered as HTTP://example.com will not be normalized and will fail the format regex. Add the /i flag for consistency.
🟡 db/migrate/20150818190757_create_embeddable_hosts.rb (L2)
The migration runs destructive logic (INSERT plus DELETE FROM site_settings WHERE name IN ('embeddable_hosts','embed_category')) inside def change. The automatic reversal only drops the table, permanently destroying the legacy settings. Use explicit up/down and restore the settings in down (or deliberately accept the loss).
🟡 app/controllers/admin/embeddable_hosts_controller.rb (L22)
params[:embeddable_host][:host] and params[:embeddable_host][:category_id] are accessed without a require/presence guard, raising NoMethodError (500) on empty/malformed requests. Follow the convention by using params.require(:embeddable_host) in create/update.
🟡 app/controllers/admin/embeddable_hosts_controller.rb (L25)
host.category_id = SiteSetting.uncategorized_category_id if host.category_id.blank? runs unconditionally inside save_host, so it also fires on update. A client updating without a category_id silently resets the host's category to 'uncategorized'. Only default when host.new_record?, or move the defaulting into create.
🟡 app/controllers/admin/embedding_controller.rb (L9)
EmbeddingController#update simply re-renders the current @embedding and never persists anything from the request. A PUT /admin/customize/embedding that changes nothing is surprising and inconsistent with 'update' semantics. Either persist the submitted config or document/remove the action.
🟡 spec/controllers/admin/embeddable_hosts_controller_spec.rb (L5)
Both controller specs only assert that the controller subclasses Admin::AdminController. There is zero coverage of the save_host mechanism, category defaulting, the success/error JSON contract, authorization (ensure_staff), the serializer output, or route/action matching. Add request specs covering staff/non-staff, success, 422, nil-record 404, and category-defaulting behavior.
🟡 app/models/site_setting.rb (L68)
Removing the public self.allows_embeddable_host? method is a breaking API change for any out-of-tree plugin or external code that referenced it; in-repo consumers are all migrated. Consider a compatibility shim delegating to EmbeddableHost.host_allowed?, or confirm the removal is intentionally breaking and documented.
🟡 config/routes.rb (L153)
resources :embeddable_hosts generates index, new, show, and edit routes that map to actions not implemented in Admin::EmbeddableHostsController (only create/update/destroy exist), causing runtime ActionNotFound errors and exposing dead routes. Restrict with only: [:create, :update, :destroy].
🟡 lib/topic_retriever.rb (L24)
The 60-second Redis throttle is keyed on the raw, unnormalized @embed_url and only checks the host (path-agnostic). A client can supply many distinct URLs on one allowlisted host, producing distinct Redis keys and triggering repeated server-side fetches within the throttle window — an amplification/DoS vector. Normalize the URL before building the key and/or throttle per host.
🟡 app/models/topic_embed.rb (L36)
eh.try(:category_id) falls back to nil (uncategorized) with no warning when no allowlisted host matches the import URL. This is a behavior regression for any embed whose host is not in the table (including all migrated scheme/port hosts). Default explicitly to SiteSetting.uncategorized_category_id and add a spec for the no-match case.
🟡 spec/models/topic_spec.rb (L1403)
The rewritten expandable_first_post? spec is vacuous: expandable_first_post? no longer consults embeddable-host state, so the first test (is false if embeddable_host is blank) would pass even if the embed_truncate? check were removed, and the 'with an emeddable host' block fabricates an EmbeddableHost that the method never queries. Remove/rename the misleading context and add an explicit assertion that embeddable-host state is irrelevant (and fix the typo 'emeddable').

Small things (take or leave)

🔵 spec/fabricators/category_fabricator.rb (L1)
The two fabricator files have their contents swapped relative to their filenames: category_fabricator.rb defines the :embeddable_host fabricator while embeddable_host_fabricator.rb contains the category fabricators. Functionally harmless because spec_helper.rb auto-requires all fabricators, but confusing for maintainers. Swap the contents.
🔵 app/controllers/admin/embeddable_hosts_controller.rb (L3)
before_filter :ensure_logged_in, :ensure_staff is redundant in both admin controllers because Admin::AdminController already declares these filters. Functionally harmless, but diverges from project convention and adds noise. Drop the redundant declarations.
🔵 app/serializers/embedding_serializer.rb (L3)
The explicit embed: :ids on has_many :embeddable_hosts is redundant with ApplicationSerializer's default embed :ids, include: true. Not a bug — the output is correct — but dropping it would be cleaner and more consistent with the codebase.
🔵 config/locales/client.en.yml (L2497)
The new admin.embedding.edit key (edit: "edit") is never referenced by client code. Remove this dead locale key to avoid unused translations and localization noise.
🔵 app/controllers/embed_controller.rb (L64)
response.headers['X-Frame-Options'] = "ALLOWALL" is not a valid X-Frame-Options value (valid values are DENY, SAMEORIGIN, ALLOW-FROM), so the header is ignored by browsers. The actual gate is the referer allowlist, which is client-controlled and bypassable. Pre-existing, but documented as a known limitation within the feature's focus.
🔵 app/controllers/embed_controller.rb (L61)
Collapsing the previous two-step check into a single EmbeddableHost.host_allowed? loses the distinct 'no hosts configured' diagnostic signal (now indistinguishable 403). Also, the rescue URI::InvalidURIError is effectively dead because record_for_host already swallows URI parsing errors; consider guarding the referer explicitly before calling host_allowed?.
🔵 app/models/topic.rb (L868)
Removing SiteSetting.embeddable_hosts.present? means a topic with a topic_embed will report expandable_first_post? == true even when no embeddable_hosts rows are configured (e.g. after hosts are deleted). This is likely acceptable, but there is no spec covering this case, so the behavior is unprotected against regression. Add an assertion and confirm the intended behavior with the feature owner.

✅ Feature-level checklist

Partially covered

⚠️ Administrators can edit category/host relationships for embedding (FEATURE: Can edit category/host relationships for embedding).
The feature adds an EmbeddableHost model/table and admin endpoints for creating/updating/deleting host-category relationships, but it is not fully reliable: update/destroy return 500 on missing records, params are unguarded, category defaulting is incorrectly applied on update, EmbeddingController#update is a no-op, the data migration crashes on typical installs and stores hosts in an unmatchable form, and the controller specs provide essentially no coverage. These defects block the feature from meeting the edit-related acceptance criteria in practice.