FEATURE: Can edit category/host relationships for embedding by zaibkhan · Pull Request #1 · codoki-ai/discourse-codoki

zaibkhan · 2025-09-10T15:59:02Z

No description provided.

codoki-pr-intelligence · 2025-09-10T15:59:08Z

Codoki PR Review

Summary: Persist embeddable host changes, fix test fabricators, align admin UI
What’s good: - Migrating host checks to EmbeddableHost and adding an EmbeddingSerializer is a solid step toward DB-backed configuration and admin UI support.

Tests updated to fabricate embeddable hosts and validate topic category association increase confidence in the new flow.
Review Status: ❌ Requires changes
Overall Priority: High
Review Update:
• Scope: Large PR detected (36 files)
• Coverage: Reviewed 30 highest-risk files across 3 batches
• Note: For full confidence, consider splitting this PR; current review covers top 30/36 files

This review covered the top 30 of 36 files (risk-ranked). For complete coverage and faster feedback, consider splitting into ~1 smaller PR(s).

Issues (Critical & High only)

Severity	Issue	Why it matters
High	Correctness — Nil-safe host normalization to prevent crashes	`…/models/embeddable_host.rb` Possible NoMethodError when host is nil: calling sub! on nil will raise, and sub! returns nil when no substitution occurs. Normalize only when host is present and assign the sanitized value back to self.host.
High	Security — Unsafe string interpolation in migration INSERT	`…/migrate/20150818190757_create_embeddable_hosts.rb` Directly interpolating host strings into SQL risks SQL injection and will break on single quotes in values. Quote the value via the DB connection and ensure category_id is an integer.
High	Compatibility — Breaking change: no fallback/backfill for embeddable hosts	`…/controllers/embed_controller.rb` This hard switch to EmbeddableHost.host_allowed? will break existing deployments that relied on SiteSetting.embeddable_hosts; until a data migration backfills those hosts, all embeds will be rejected. Either provide a migration that seeds embeddable_hosts from the legacy setting, or retain a temporary fallback to the old check to preserve behavior during rollout.
High	Correctness — Empty update payload won’t persist embeddable_hosts changes	`…/controllers/admin-embedding.js.es6` Calling update with an empty payload will not persist added/removed embeddable_hosts; send the association IDs so the backend can reconcile the relationship. Also, ensure newly created hosts are saved (so they have IDs) before updating the embedding, or handle creation server-side.
High	Testing — Category fabricators removed and embeddable_host fabricat...	`…/fabricators/category_fabricator.rb` This file now defines an embeddable_host fabricator and removes the category fabricators, which can break tests relying on Category and also duplicates the embeddable_host fabricator (a dedicated file exists). Restore the Category fabricator here and keep the embeddable_host fabricator in spec/fabricators/embeddable_host_fabricator.rb.

Showing top 5 issues. Critical: 0, High: 5. See inline suggestions for more.

Key Feedback (click to expand)

Needs improvement: - The admin UI currently sends an empty payload on save; send embeddable_host_ids to persist add/remove operations.
Restore category fabricators and avoid redefining embeddable_host fabricators in category_fabricator.rb to prevent collisions and broken tests.
Testing: The category fabricators were removed and replaced with an embeddable_host fabricator inside category_fabricator.rb, while a dedicated embeddable_host_fabricator also exists. This likely breaks tests depending on category fabricators and may cause duplicate fabricator definitions. Please restore at least the base Category fabricator in spec/fabricators/category_fabricator.rb and keep embeddable_host in spec/fabricators/embeddable_host_fabricator.rb.
Documentation: Consider documenting the new admin embedding workflow: how hosts are added/removed, what fields are required, and how these changes are persisted (IDs vs nested attributes). A note on migration from SiteSetting.embeddable_hosts would help operators.
Compatibility: Switching from SiteSetting.embeddable_hosts to a DB-backed EmbeddableHost requires a data migration to preserve existing allowed hosts; otherwise, embedding may break until admins reconfigure. Please confirm a migration strategy.
Open questions: - How are existing SiteSetting.embeddable_hosts values migrated to embeddable_hosts records to maintain backward compatibility?
Should deleting a host from the UI soft-delete the record or only disassociate from embedding? What is the intended lifecycle?

Confidence: 2/5 — Not ready to merge (5 high · status: Requires changes · scope: top 30/36 files reviewed)

React with 👍 or 👎 if you found this review useful.

codoki-pr-intelligence · 2025-09-10T16:02:01Z

+  validates_format_of :host, :with => /\A[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(:[0-9]{1,5})?(\/.*)?\Z/i
+  belongs_to :category
+
+  before_validation do


⚠️ High: Possible NoMethodError when host is nil: calling sub! on nil will raise, and sub! returns nil when no substitution occurs. Normalize only when host is present and assign the sanitized value back to self.host.

Suggested change

before_validation do

before_validation do

if self.host.present?

self.host = self.host.to_s.sub(/^https?:\/\//, '').sub(/\/.*$/, '')

end

end

codoki-pr-intelligence · 2025-09-10T16:02:03Z

+    host = uri.host
+    return false unless host.present?
+
+    where("lower(host) = ?", host).first


🔷 Medium: The query compares lower(host) to the unmodified input. If the input host contains uppercase letters (e.g., 'EVILTROUT.COM'), this will not match. Downcase the input to ensure consistent case-insensitive comparison.

Suggested change

where("lower(host) = ?", host).first

where("lower(host) = ?", host.downcase).first

codoki-pr-intelligence · 2025-09-10T16:02:04Z

+        records = val.split("\n")
+        if records.present?
+          records.each do |h|
+            execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES ('#{h}', #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)"


⚠️ High: Directly interpolating host strings into SQL risks SQL injection and will break on single quotes in values. Quote the value via the DB connection and ensure category_id is an integer.

Suggested change

execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES ('#{h}', #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)"

execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES (#{ActiveRecord::Base.connection.quote(h)}, #{category_id.to_i}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)"

codoki-pr-intelligence · 2025-09-10T16:05:18Z

      if !(Rails.env.development? && current_user.try(:admin?))
-        raise Discourse::InvalidAccess.new('embeddable hosts not set') if SiteSetting.embeddable_hosts.blank?
-        raise Discourse::InvalidAccess.new('invalid referer host') unless SiteSetting.allows_embeddable_host?(request.referer)
+        raise Discourse::InvalidAccess.new('invalid referer host') unless EmbeddableHost.host_allowed?(request.referer)


⚠️ High: This hard switch to EmbeddableHost.host_allowed? will break existing deployments that relied on SiteSetting.embeddable_hosts; until a data migration backfills those hosts, all embeds will be rejected. Either provide a migration that seeds embeddable_hosts from the legacy setting, or retain a temporary fallback to the old check to preserve behavior during rollout.

Suggested change

raise Discourse::InvalidAccess.new('invalid referer host') unless EmbeddableHost.host_allowed?(request.referer)

raise Discourse::InvalidAccess.new('invalid referer host') unless EmbeddableHost.host_allowed?(request.referer) || SiteSetting.allows_embeddable_host?(request.referer)

codoki-pr-intelligence · 2025-09-10T16:05:19Z

@@ -0,0 +1,16 @@
+class EmbeddableHostSerializer < ApplicationSerializer


🔷 Medium: The explicit id/host/category_id methods duplicate the default behavior of attributes; simplifying reduces maintenance and keeps the serializer idiomatic.

Suggested change

class EmbeddableHostSerializer < ApplicationSerializer

class EmbeddableHostSerializer < ApplicationSerializer

attributes :id, :host, :category_id

end

codoki-pr-intelligence · 2025-09-10T16:07:38Z

+
+  actions: {
+    saveChanges() {
+      this.get('embedding').update({});


⚠️ High: Calling update with an empty payload will not persist added/removed embeddable_hosts; send the association IDs so the backend can reconcile the relationship. Also, ensure newly created hosts are saved (so they have IDs) before updating the embedding, or handle creation server-side.

Suggested change

this.get('embedding').update({});

this.get('embedding').update({

embeddable_host_ids: this.get('embedding.embeddable_hosts').mapBy('id').filter(Boolean)

});

codoki-pr-intelligence · 2025-09-10T16:07:39Z

-    cat.update!(read_restricted: true)
-    cat.category_groups.build(group_id: transients[:group].id, permission_type: CategoryGroup.permission_types[:full])
-  end
+Fabricator(:embeddable_host) do


⚠️ High: This file now defines an embeddable_host fabricator and removes the category fabricators, which can break tests relying on Category and also duplicates the embeddable_host fabricator (a dedicated file exists). Restore the Category fabricator here and keep the embeddable_host fabricator in spec/fabricators/embeddable_host_fabricator.rb.

Suggested change

Fabricator(:embeddable_host) do

Fabricator(:category) do

name { sequence(:name) { |n| "Amazing Category #{n}" } }

user

end

FEATURE: Can edit category/host relationships for embedding

d1c6918

codoki-pr-intelligence Bot reviewed Sep 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEATURE: Can edit category/host relationships for embedding#1

FEATURE: Can edit category/host relationships for embedding#1
zaibkhan wants to merge 1 commit into
rest-serializer-enhancement-prefrom
rest-serializer-enhancement-post

zaibkhan commented Sep 10, 2025

Uh oh!

codoki-pr-intelligence Bot commented Sep 10, 2025 •

edited

Loading

Uh oh!

codoki-pr-intelligence Bot Sep 10, 2025

Uh oh!

codoki-pr-intelligence Bot Sep 10, 2025

Uh oh!

codoki-pr-intelligence Bot Sep 10, 2025

Uh oh!

codoki-pr-intelligence Bot Sep 10, 2025

Uh oh!

codoki-pr-intelligence Bot Sep 10, 2025

Uh oh!

codoki-pr-intelligence Bot Sep 10, 2025

Uh oh!

codoki-pr-intelligence Bot Sep 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	where("lower(host) = ?", host).first
	where("lower(host) = ?", host.downcase).first

	execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES ('#{h}', #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)"
	execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES (#{ActiveRecord::Base.connection.quote(h)}, #{category_id.to_i}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)"

	raise Discourse::InvalidAccess.new('invalid referer host') unless EmbeddableHost.host_allowed?(request.referer)
	raise Discourse::InvalidAccess.new('invalid referer host') unless EmbeddableHost.host_allowed?(request.referer) \|\| SiteSetting.allows_embeddable_host?(request.referer)

		@@ -0,0 +1,16 @@
		class EmbeddableHostSerializer < ApplicationSerializer

-Fabricator(:embeddable_host) do
+Fabricator(:category) do
+  name { sequence(:name) { |n| "Amazing Category #{n}" } }
+  user
+end

Conversation

zaibkhan commented Sep 10, 2025

Uh oh!

codoki-pr-intelligence Bot commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codoki PR Review

Uh oh!

codoki-pr-intelligence Bot Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

codoki-pr-intelligence Bot Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

codoki-pr-intelligence Bot Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

codoki-pr-intelligence Bot Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

codoki-pr-intelligence Bot Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

codoki-pr-intelligence Bot Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

codoki-pr-intelligence Bot Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codoki-pr-intelligence Bot commented Sep 10, 2025 •

edited

Loading