FEATURE: Can edit category/host relationships for embedding - #10
FEATURE: Can edit category/host relationships for embedding#10everettbu wants to merge 1 commit into
Conversation
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| records.each do |h| | ||
| execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES ('#{h}', #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)" |
There was a problem hiding this comment.
This SQL query is vulnerable to injection attacks because it directly interpolates user-provided values into the query string. Consider using parameterized queries instead:
execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES (?, ?, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)", h, category_idThis approach properly sanitizes the inputs and prevents potential SQL injection vulnerabilities.
| records.each do |h| | |
| execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES ('#{h}', #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)" | |
| records.each do |h| | |
| execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES (?, ?, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)", h, category_id |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| Fabricator(:embeddable_host) do | ||
| host "eviltrout.com" | ||
| category | ||
| end |
There was a problem hiding this comment.
It appears that the contents of spec/fabricators/category_fabricator.rb and spec/fabricators/embeddable_host_fabricator.rb have been swapped. This file should contain the Category fabricators, while the EmbeddableHost fabricator should be in the other file. This swap needs to be corrected to maintain proper organization of the test fixtures.
| Fabricator(:embeddable_host) do | |
| host "eviltrout.com" | |
| category | |
| end | |
| Fabricator(:category) do | |
| name { sequence(:name) { |i| "Amazing Category #{i}" } } | |
| user | |
| end | |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
|
This pull request has been automatically marked as stale because it has been open for 60 days with no activity. To keep it open, remove the stale tag, push code, or add a comment. Otherwise, it will be closed in 14 days. |
mfeuerstein
left a comment
mfeuerstein
left a comment
There was a problem hiding this comment.
PR Review — approved
Reviewed 30 files. 0 high-severity issues found. Verdict: approved.
app/assets/javascripts/admin/templates/customize.hbs (low)
- Reviewed app/assets/javascripts/admin/templates/customize.hbs — looks good
app/assets/javascripts/admin/templates/embedding.hbs (low)
- Reviewed app/assets/javascripts/admin/templates/embedding.hbs — looks good
app/assets/javascripts/admin/adapters/embedding.js.es6 (low)
- Reviewed app/assets/javascripts/admin/adapters/embedding.js.es6 — looks good
app/assets/javascripts/admin/routes/admin-route-map.js.es6 (low)
- Reviewed app/assets/javascripts/admin/routes/admin-route-map.js.es6 — looks good
app/assets/javascripts/admin/controllers/admin-embedding.js.es6 (low)
- Reviewed app/assets/javascripts/admin/controllers/admin-embedding.js.es6 — looks good
app/assets/javascripts/admin/templates/components/embeddable-host.hbs (low)
- Reviewed app/assets/javascripts/admin/templates/components/embeddable-host.hbs — looks good
app/assets/javascripts/discourse/models/store.js.es6 (low)
- Reviewed app/assets/javascripts/discourse/models/store.js.es6 — looks good
app/controllers/admin/embeddable_hosts_controller.rb (low)
- Reviewed app/controllers/admin/embeddable_hosts_controller.rb — looks good
app/controllers/admin/embedding_controller.rb (low)
- Reviewed app/controllers/admin/embedding_controller.rb — looks good
app/assets/javascripts/admin/routes/admin-embedding.js.es6 (low)
- Reviewed app/assets/javascripts/admin/routes/admin-embedding.js.es6 — looks good
app/assets/javascripts/admin/components/embeddable-host.js.es6 (low)
- Reviewed app/assets/javascripts/admin/components/embeddable-host.js.es6 — looks good
app/assets/javascripts/discourse/adapters/rest.js.es6 (low)
- Reviewed app/assets/javascripts/discourse/adapters/rest.js.es6 — looks good
app/models/site_setting.rb (low)
- Reviewed app/models/site_setting.rb — looks good
app/models/topic_embed.rb (low)
- Reviewed app/models/topic_embed.rb — looks good
app/models/topic.rb (low)
- Reviewed app/models/topic.rb — looks good
config/locales/client.en.yml (low)
- Reviewed config/locales/client.en.yml — looks good
app/serializers/embedding_serializer.rb (low)
- Reviewed app/serializers/embedding_serializer.rb — looks good
app/controllers/embed_controller.rb (low)
- Reviewed app/controllers/embed_controller.rb — looks good
app/models/embeddable_host.rb (low)
- Reviewed app/models/embeddable_host.rb — looks good
app/serializers/embeddable_host_serializer.rb (low)
- Reviewed app/serializers/embeddable_host_serializer.rb — looks good
config/routes.rb (low)
- Reviewed config/routes.rb — looks good
spec/controllers/admin/embedding_controller_spec.rb (low)
- Reviewed spec/controllers/admin/embedding_controller_spec.rb — looks good
config/site_settings.yml (low)
- Reviewed config/site_settings.yml — looks good
config/locales/server.en.yml (low)
- Reviewed config/locales/server.en.yml — looks good
lib/topic_retriever.rb (low)
- Reviewed lib/topic_retriever.rb — looks good
spec/controllers/admin/embeddable_hosts_controller_spec.rb (low)
- Reviewed spec/controllers/admin/embeddable_hosts_controller_spec.rb — looks good
db/migrate/20150818190757_create_embeddable_hosts.rb (low)
- Reviewed db/migrate/20150818190757_create_embeddable_hosts.rb — looks good
spec/fabricators/embeddable_host_fabricator.rb (low)
- Reviewed spec/fabricators/embeddable_host_fabricator.rb — looks good
spec/fabricators/category_fabricator.rb (low)
- Reviewed spec/fabricators/category_fabricator.rb — looks good
spec/controllers/embed_controller_spec.rb (low)
- Reviewed spec/controllers/embed_controller_spec.rb — looks good
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Code Review: FEATURE: Can edit category/host relationships for embedding
Problem
Discourse's embedding feature previously stored allowed embeddable hosts as a newline-delimited SiteSetting string (embeddable_hosts) with a single global embed_category setting. This PR replaces that with a first-class EmbeddableHost ActiveRecord model supporting per-host category associations, plus an admin CRUD UI, serializers, routes, a data migration, and updated callers.
Solution Reviewed
New EmbeddableHost model with host validation/normalization, Admin::EmbeddableHostsController (CRUD) and Admin::EmbeddingController (aggregate show/update) on the backend. Ember admin UI: route, controller, embeddable-host component with buffered editing, adapter, and templates. The Ember store's _hydrateEmbedded is generalized to handle plural _ids keys. A migration backfills the new table from the old site settings and deletes them. All callers (embed_controller, topic_retriever, topic_embed, topic) switch from SiteSetting.allows_embeddable_host?/embeddable_hosts/embed_category to the new model.
Summary
The architecture is sound — serializer/adapter contract is coherent, all callers are migrated, and the Ember UI wiring is correct. However, the data migration has two blocking defects that will prevent deployment on most installs and silently break embedding for migrated hosts. Several controller and model error-handling gaps should also be addressed.
Verification
- Full test suite — skipped: vintage 2015 Discourse codebase requires Ruby/Postgres/Redis setup not available in this environment.
- Fabricator swap, migration nil-crash, normalization bypass, and case-asymmetric matching confirmed by direct file inspection.
Issues Found
🔴 Blocking
- db/migrate/20150818190757_create_embeddable_hosts.rb:11 — The migration dereferences
execute("SELECT ... WHERE s.name = 'embed_category'")[0]['id'].to_iwith no nil guard. Discourse only persists overridden settings to thesite_settingstable; defaults live in memory. On any install where an admin never explicitly setembed_category(the default'', i.e. most installs), the query returns zero rows,[0]is nil, andnil['id']raisesNoMethodError— aborting the migration and blocking deploy. Theif category_id == 0fallback is unreachable because the exception fires first. - db/migrate/20150818190757_create_embeddable_hosts.rb:25 — Migrated host values are copied via raw SQL
INSERT ... VALUES ('#{h}', ...), bypassingEmbeddableHost'sbefore_validationscheme/path stripping. The oldembeddable_hostssetting accepted entries likehttp://discourse.orgorexample.com/1234. After migration these are stored verbatim, butEmbeddableHost.record_for_hostmatchesuri.host(e.g.discourse.org) againstlower(host)— a stored rowhttp://discourse.orgnever matches, silently breaking embedding for every host saved with a scheme or path.
🟡 Non-blocking
- db/migrate/20150818190757_create_embeddable_hosts.rb:25 —
#{h}is string-interpolated directly into SQL. The oldhost_listsetting had no validator, so a value containing a single quote breaks the statement mid-migration. Input is admin-controlled and one-time, but should use parameterized SQL orEmbeddableHost.create!. - db/migrate/20150818190757_create_embeddable_hosts.rb:3 —
create_table :embeddable_hosts, force: truecombined with irreversibleexecutestatements (INSERT/DELETE) makes this migration non-idempotent and unsafe to roll back. Usedef up/def downinstead ofdef change. - app/controllers/admin/embeddable_hosts_controller.rb:11 —
update,destroy, andsave_hosthave no nil guards. Missing/stale id → nil host →NoMethodError(500) instead of 404.save_hostalso dereferencesparams[:embeddable_host][:host]without presence check, producing 500 on malformed body instead of 422. - app/controllers/admin/embedding_controller.rb:10 —
updatere-renders the serialized embedding without persisting anything. Misleading no-op that could trap future maintainers. - app/assets/javascripts/admin/components/embeddable-host.js.es6:46 — The
deleteaction has no.catch(unlikesave). Server-side destroy failure is swallowed silently — row stays, no user feedback. - app/models/embeddable_host.rb:17 —
where("lower(host) = ?", host)lowercases the stored column but noturi.host. A referer with different casing won't match. Downcase the input:host = uri.host.downcase. - app/models/embeddable_host.rb:2 — Validation regex caps TLD at
[a-z]{2,5}, rejecting valid long-TLD hosts (.museum,.online,.photography). The oldhost_listsetting had no format validator, so this is a regression. Also no uniqueness validation or DB index onhost— duplicates possible from concurrent admin actions. - app/assets/javascripts/admin/templates/components/embeddable-host.hbs:17 — Delete button has no
disabledbinding (unlike save/cancel). Double-delete possible whiledestroyRecordis in flight. - app/controllers/embed_controller.rb:61 — Removed
'embeddable hosts not set'guard means all failures (unconfigured, nil referer, invalid host) surface as the single message'invalid referer host', conflating distinct conditions. Therescue URI::InvalidURIErroron line 65 is also dead code. - spec/fabricators/category_fabricator.rb:1 — Contents of
category_fabricator.rbandembeddable_host_fabricator.rbare swapped. Works by accident (Fabrication registers by symbol) but file naming is clearly accidental.
💡 Suggestions
- 20 non-EN locale files retain orphaned
embeddable_hosts/embed_categorykeys (onlyserver.en.ymlwas cleaned). In Discourse these are Transifex-managed and auto-cleaned on the next translation pull — no action required. - Controller specs only assert the subclass relationship — consider adding tests for CRUD behavior, authorization, and nil-id/missing-params error paths.
Verdict
Recommend changes before merge — the migration has two blocking defects (nil-crash on default config, raw host values that silently break embedding) that will prevent deployment or break existing installs.
|
|
||
| category_id = execute("SELECT c.id FROM categories AS c | ||
| INNER JOIN site_settings AS s ON s.value = c.name | ||
| WHERE s.name = 'embed_category'")[0]['id'].to_i |
There was a problem hiding this comment.
🔴 Blocking: Migration crashes on most installs.
execute("SELECT ... WHERE s.name = 'embed_category'")[0]['id'].to_i dereferences the first row of an empty result set. Discourse only persists overridden settings to the site_settings table (defaults live in memory), so on any install where an admin never explicitly set embed_category (the default '' — i.e. most installs), the query returns zero rows, [0] is nil, and nil['id'] raises NoMethodError.
The if category_id == 0 fallback on line 14 is unreachable because the exception fires on line 11 before category_id is assigned. Even if reached, line 15 has the same unguarded [0]['value'] pattern.
Other migrations in this repo guard exactly this pattern (e.g. if uncat && uncat[0] && uncat[0]['value']). This migration should do the same, or use SiteSetting.uncategorized_category_id (which reads the YAML default) instead of a raw DB query.
| 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)" |
There was a problem hiding this comment.
🔴 Blocking: Migrated hosts bypass normalization, embedding breaks silently.
The INSERT copies raw h values from the old embeddable_hosts setting verbatim, bypassing EmbeddableHost's before_validation scheme/path stripping. The old setting accepted entries like http://discourse.org or example.com/1234 (the old allows_embeddable_host? handled schemes via URI(h).host).
After migration, EmbeddableHost.record_for_host does where("lower(host) = ?", uri.host) — so a referer host discourse.org never matches a stored row http://discourse.org. Embedding silently breaks for every host saved with a scheme or path.
Fix: normalize each host before inserting (or use EmbeddableHost.create! to trigger before_validation):
records.each do |h|
host = h.sub(/^https?:\/\//, '').sub(/\/.*$/, '')
execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES (#{ActiveRecord::Base.connection.quote(host)}, #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)"
end| 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)" |
There was a problem hiding this comment.
🟡 SQL interpolation. #{h} is interpolated directly into the INSERT statement. The old host_list setting had no validator, so a value containing a single quote breaks the statement mid-migration — after some rows are inserted but before the DELETE FROM site_settings on line 31. On a retried run the source data is gone. Use parameterized SQL or ActiveRecord::Base.connection.quote(h).
| @@ -0,0 +1,33 @@ | |||
| class CreateEmbeddableHosts < ActiveRecord::Migration | |||
| def change | |||
| create_table :embeddable_hosts, force: true do |t| | |||
There was a problem hiding this comment.
🟡 create_table :embeddable_hosts, force: true combined with irreversible raw execute (INSERT/DELETE) makes this migration non-idempotent and unsafe to roll back. db:rollback drops the table but the DELETE FROM site_settings is not reversed, permanently destroying the source data. Use def up/def down (re-creating the site_settings rows) instead of def change.
|
|
||
| def update | ||
| host = EmbeddableHost.where(id: params[:id]).first | ||
| save_host(host) |
There was a problem hiding this comment.
🟡 No nil guards. EmbeddableHost.where(id: params[:id]).first returns nil for a missing/stale id, but save_host(nil) (line 11) and nil.destroy (line 16) raise NoMethodError → unhandled 500 instead of 404. save_host (line 23) also dereferences params[:embeddable_host][:host] without checking params[:embeddable_host].present?, producing 500 on a malformed body instead of 422.
Add a not-found guard before operating on the record:
def update
host = EmbeddableHost.where(id: params[:id]).first
raise Discourse::NotFound unless host
save_host(host)
end| host = uri.host | ||
| return false unless host.present? | ||
|
|
||
| where("lower(host) = ?", host).first |
There was a problem hiding this comment.
🟡 Case-asymmetric matching. where("lower(host) = ?", host) lowercases the stored column but not uri.host (line 14). A referer with different casing (e.g. EvilTrout.com) won't match a stored eviltrout.com, wrongly rejecting a legitimate embed.
host = uri.host.downcase
where("lower(host) = ?", host).first| @@ -0,0 +1,24 @@ | |||
| class EmbeddableHost < ActiveRecord::Base | |||
| validates_format_of :host, :with => /\A[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(:[0-9]{1,5})?(\/.*)?\Z/i | |||
There was a problem hiding this comment.
🟡 Two model-level issues:
- The validation regex caps the TLD at
[a-z]{2,5}, rejecting valid long-TLD hosts (.museum,.online,.photography). The oldhost_listsetting had no format validator, so this is a regression for existing installs. Consider loosening to{2,24}. - There is no uniqueness validation or DB index on
host, allowing duplicate hosts from concurrent admin actions.
| <td>{{category-badge host.category}}</td> | ||
| <td> | ||
| {{d-button icon="pencil" action="edit"}} | ||
| {{d-button icon="trash-o" action="delete" class='btn-danger'}} |
There was a problem hiding this comment.
🟡 The delete button has no disabled binding (unlike save/cancel which bind disabled=cantSave/disabled=host.isSaving). A user can click delete twice while destroyRecord is in flight.
| 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) |
There was a problem hiding this comment.
🟡 The removed 'embeddable hosts not set' guard means all failures — unconfigured, nil referer (e.g. Referrer-Policy: no-referrer), or invalid host — now surface as the single message 'invalid referer host', conflating distinct conditions. Operators can't distinguish "add hosts" from "fix the referer". Also, the rescue URI::InvalidURIError on line 65 is dead code: record_for_host already swallows errors via rescue nil, and URI(nil) raises TypeError not URI::InvalidURIError.
| 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 |
There was a problem hiding this comment.
🟡 The contents of category_fabricator.rb and embeddable_host_fabricator.rb are swapped — this file defines Fabricator(:embeddable_host) while embeddable_host_fabricator.rb defines all the category fabricators (:category, :diff_category, :happy_category, :private_category).
Fabricate(:category) still works because Fabrication registers by symbol and spec_helper eagerly loads all fabricator files, but the file naming is clearly accidental. Swap the contents back so each fabricator lives in its namesake file.
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Code Review: FEATURE: Can edit category/host relationships for embedding
Problem
Discourse's embedding feature previously stored allowed embeddable hosts as a newline-delimited SiteSetting string (embeddable_hosts) with a single global embed_category setting. This PR replaces that with a first-class EmbeddableHost ActiveRecord model supporting per-host category associations, plus an admin Ember UI and CRUD controllers.
Solution Reviewed
A new EmbeddableHost model (host + category_id) replaces the SiteSetting strings. A migration backfills the new table from old settings and deletes them. Admin CRUD routes through Admin::EmbeddableHostsController; an aggregate Admin::EmbeddingController serves the Ember admin page. The Ember store's _hydrateEmbedded was extended to support plural *_ids association hydration. The embed allowlist check moved from SiteSetting.allows_embeddable_host? to EmbeddableHost.host_allowed?.
Summary
The public-API migration is coherent and complete — all callers are migrated and the serializer/hydration contract matches. However, the data migration has two blocking defects: a SQL-injection vector via string-interpolated host values, and a crash on the common case where embed_category was never configured. Several controllers and the Ember component also lack error handling for missing records and failed requests.
Files Reviewed
db/migrate/20150818190757_create_embeddable_hosts.rb— deeply reviewed (high risk: migration + SQL)app/models/embeddable_host.rb— deeply reviewed (high risk: security-critical allowlist)app/controllers/admin/embeddable_hosts_controller.rb— deeply reviewed (medium risk: CRUD)app/controllers/admin/embedding_controller.rb— deeply reviewed (medium risk: API)app/controllers/embed_controller.rb— deeply reviewed (high risk: auth/allowlist)app/models/topic_embed.rb— deeply reviewed (medium risk: data import)app/models/topic.rb— lightly reviewed (one-line guard removal)app/models/site_setting.rb— lightly reviewed (method removal)app/assets/javascripts/discourse/models/store.js.es6— deeply reviewed (medium risk: hydration logic)app/assets/javascripts/discourse/adapters/rest.js.es6— lightly reviewed (adapter wiring)app/assets/javascripts/admin/components/embeddable-host.js.es6— deeply reviewed (medium risk: UI logic)app/assets/javascripts/admin/controllers/admin-embedding.js.es6— lightly reviewedapp/assets/javascripts/admin/templates/embedding.hbs— lightly reviewedapp/serializers/embedding_serializer.rb— lightly reviewed (contract verified)app/serializers/embeddable_host_serializer.rb— lightly reviewedconfig/routes.rb— lightly reviewed (route wiring verified)config/site_settings.yml,config/locales/*.en.yml— lightly reviewed (consistency)spec/**— deeply reviewed (test coverage)lib/topic_retriever.rb— lightly reviewed (caller migration verified)
Verification
- Static verification: confirmed
Ember.String.underscoreconvertsembeddable-hosts→embeddable_hostsmatching Railsresources :embeddable_hosts(no URL mismatch). - Grep verification: zero remaining production callers of
allows_embeddable_host?,SiteSetting.embeddable_hosts, orSiteSetting.embed_category. host_listSiteSetting type confirmed to have no validator inlib/site_setting_extension.rb, grounding the migration SQL-injection finding.- Authorization verified: routes are admin-gated via
AdminConstraint+StaffConstraint;ensure_embeddableis not weakened (returns false when no hosts configured). - Ruby/Rails test suite: not run (legacy Rails 4 codebase, no bundler environment configured in this review).
Issues Found
Blocking (2)
- db/migrate/20150818190757_create_embeddable_hosts.rb:25 — SQL injection via string-interpolated host values in migration INSERT; also bypasses model normalization so migrated hosts retain scheme/path and never match at runtime.
- db/migrate/20150818190757_create_embeddable_hosts.rb:11 — Migration crashes with NoMethodError when
embed_categoryis absent (the common case); theif category_id == 0fallback is dead code.
Non-blocking (7)
- app/controllers/admin/embeddable_hosts_controller.rb:15 —
update/destroycrash on nil when record not found (500 instead of 404). - app/controllers/admin/embedding_controller.rb:10 —
updateis a no-op returning 200 OK; the EmbersaveChangesaction is dead code. - app/models/embeddable_host.rb:17 — Case-sensitivity mismatch: lookup lowercases stored host but not input, so mixed-case entries never match.
- app/models/embeddable_host.rb:9 —
before_validationcrashes on nil host instead of producing a validation error. - app/assets/javascripts/admin/components/embeddable-host.js.es6:46 —
deleteaction doesn't.catchrejection; failures silently swallowed. - app/assets/javascripts/discourse/models/store.js.es6:197 —
.map()called without array guard; crashes if*_idskey is null/absent. - spec/controllers/admin/embeddable_hosts_controller_spec.rb — Both new admin controller specs only assert subclass relationship; no action/authorization/error-path tests.
Suggestions (2)
- spec/fabricators/category_fabricator.rb:1 — Fabricator definitions are in swapped files (embeddable_host fabricator in category_fabricator.rb and vice versa).
- spec/models/topic_spec.rb:1400 —
expandable_first_post?test is now vacuous; the behavioral change (true with no embeddable hosts) is untested.
Verdict
Recommend changes before merge — the migration has a SQL-injection vector and will crash on default-config installs. Both blocking issues must be fixed before deployment.
| 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)" |
There was a problem hiding this comment.
🔴 Blocking: SQL injection via string-interpolated host values
The host_list SiteSetting type has no validator (confirmed in lib/site_setting_extension.rb), so legacy embeddable_hosts values were stored unvalidated. Interpolating h directly into raw SQL (VALUES ('#{h}', ...)) means a stored value containing a single quote either breaks the migration with a syntax error or executes arbitrary SQL during upgrade.
Additionally, the INSERT bypasses the model's before_validation normalization, so hosts stored with a scheme or path (e.g. http://discourse.org, example.com/1234) are migrated as-is. At runtime, record_for_host compares against URI(referer).host (no scheme/path), so these migrated hosts will never match — embedding silently stops working.
Fix: Use EmbeddableHost.create!(host: h.strip, category_id: category_id) instead of raw SQL — this parameterizes the query and runs the model's normalization/validation:
records.each do |h|
EmbeddableHost.create!(host: h.strip, category_id: category_id)
end|
|
||
| category_id = execute("SELECT c.id FROM categories AS c | ||
| INNER JOIN site_settings AS s ON s.value = c.name | ||
| WHERE s.name = 'embed_category'")[0]['id'].to_i |
There was a problem hiding this comment.
🔴 Blocking: Migration crashes when embed_category is absent
execute("SELECT c.id ... WHERE s.name = 'embed_category'")[0]['id'].to_i dereferences nil when the query returns zero rows — the common case on fresh installs and sites that never customized embed_category (the setting default was '', and settings rows are often only persisted when changed from default). nil['id'] raises NoMethodError, halting db:migrate.
The if category_id == 0 fallback on line 14 is dead code — the crash on line 11 prevents execution from ever reaching it.
Fix: Guard the result so the fallback works:
row = execute("SELECT c.id FROM categories AS c
INNER JOIN site_settings AS s ON s.value = c.name
WHERE s.name = 'embed_category'").to_a.first
category_id = row ? row['id'].to_i : 0Apply the same guard to the uncategorized_category_id lookup on line 15.
| end | ||
|
|
||
| def destroy | ||
| host = EmbeddableHost.where(id: params[:id]).first |
There was a problem hiding this comment.
🟡 update and destroy crash on missing record
Both actions do EmbeddableHost.where(id: params[:id]).first with no nil check. A nonexistent or stale id yields nil, so save_host(nil) (→ nil.host=, NoMethodError) and nil.destroy raise 500 instead of returning a 404.
Fix:
def update
host = EmbeddableHost.where(id: params[:id]).first
return render_json_error('host not found', status: 404) if host.blank?
save_host(host)
end
def destroy
host = EmbeddableHost.where(id: params[:id]).first
return render_json_error('host not found', status: 404) if host.blank?
host.destroy
render json: success_json
end| end | ||
|
|
||
| def update | ||
| render_serialized(@embedding, EmbeddingSerializer, root: 'embedding', rest_serializer: true) |
There was a problem hiding this comment.
🟡 update is a no-op returning 200 OK
update ignores all request params and re-renders the same in-memory OpenStruct (rebuilt fresh from the DB on every request via fetch_embedding). It persists nothing. The Ember saveChanges action that calls embedding.update({}) is dead code — embedding.hbs has no save button.
This is misleading: a successful PUT /admin/customize/embedding that mutated nothing.
Fix: Since hosts now have their own CRUD endpoints, remove the put customize/embedding route, the update action, and the Ember saveChanges action. If global embed settings need saving, implement real persistence instead.
| host = uri.host | ||
| return false unless host.present? | ||
|
|
||
| where("lower(host) = ?", host).first |
There was a problem hiding this comment.
🟡 Case-sensitivity mismatch breaks mixed-case host entries
before_validation strips scheme/path but does not downcase the host. The validation regex uses /i, so mixed-case hosts like EvilTrout.com are accepted and stored as-is. But record_for_host queries where("lower(host) = ?", host) where host comes from uri.host (Ruby's URI preserves case). So lower('EvilTrout.com') = 'eviltrout.com' ≠ 'EvilTrout.com' — the lookup returns nil and a valid host is silently rejected.
Fix: Downcase the input in the lookup:
where("lower(host) = ?", host.downcase).firstOr normalize in before_validation: self.host = self.host.downcase if self.host.present?
| delete() { | ||
| bootbox.confirm(I18n.t('admin.embedding.confirm_delete'), (result) => { | ||
| if (result) { | ||
| this.get('host').destroyRecord().then(() => { |
There was a problem hiding this comment.
🟡 delete action doesn't catch rejection
The save action chains .catch(popupAjaxError) (line 40), but delete only chains .then(...) on destroyRecord(). If the server returns an error, the promise rejects silently — the host stays in the list (correct) but the admin gets zero feedback that the delete failed.
Fix:
this.get('host').destroyRecord().then(() => {
this.sendAction('deleteHost', this.get('host'));
}).catch(popupAjaxError);| obj[subType] = hydrated; | ||
|
|
||
| if (m[2]) { | ||
| const hydrated = obj[k].map(function(id) { |
There was a problem hiding this comment.
🟡 .map() called without array guard
When the plural *_ids branch is taken, obj[k].map(...) assumes obj[k] is an array. If the serializer omits the key or sends null for an empty has_many collection (possible with AMS embed: :ids), this throws TypeError: Cannot read property 'map' of null, crashing the admin embedding page hydration.
Fix:
const hydrated = (obj[k] || []).map(function(id) {
return self._lookupSubType(subType, type, id, root);
});| @@ -0,0 +1,9 @@ | |||
| require 'spec_helper' | |||
|
|
|||
| describe Admin::EmbeddableHostsController do | |||
There was a problem hiding this comment.
🟡 Missing test coverage for new CRUD controllers
Both embeddable_hosts_controller_spec.rb and embedding_controller_spec.rb only assert the AdminController subclass relationship. There are no tests for:
create/update/destroyactions (happy path and error path)- Authorization (non-staff/admin rejection)
- Missing record (404) or invalid params handling
show/updateserialization shape forEmbeddingController
Given these are new admin endpoints managing a security-critical allowlist, they need proper request specs.
| 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 |
There was a problem hiding this comment.
💡 Fabricator definitions are in swapped files
category_fabricator.rb now defines :embeddable_host, while embeddable_host_fabricator.rb holds all the category fabricators (:category, :diff_category, :happy_category, :private_category). This is the opposite of what the filenames imply. Functionally harmless (Fabricator loads the whole directory) but confusing for future maintainers.
Fix: Move Fabricator(:embeddable_host) into embeddable_host_fabricator.rb and keep the category fabricators in category_fabricator.rb.
| end | ||
| let(:topic) { Fabricate.build(:topic) } | ||
|
|
||
| it "is false if embeddable_host is blank" do |
There was a problem hiding this comment.
💡 expandable_first_post? test is now vacuous
The code dropped SiteSetting.embeddable_hosts.present? from expandable_first_post? (now SiteSetting.embed_truncate? && has_topic_embed?). The test "is false if embeddable_host is blank" has no before block, so embed_truncate defaults to false and the assertion holds regardless of any embeddable-host state. The actual behavior change — expandable_first_post? now returns true when embed_truncate=true + has_topic_embed=true but no embeddable hosts are configured — is untested.
Fix: Rename the test to reflect what it now checks, and add a test for the no-hosts + embed_truncate + has_topic_embed case to document the intentional behavior change.
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Code Review: FEATURE: Can edit category/host relationships for embedding
Problem
Discourse's embedding feature previously stored allowed hosts as a newline-delimited SiteSetting string (embeddable_hosts) with a single global embed_category. This PR replaces that with a first-class EmbeddableHost ActiveRecord model supporting per-host category associations, plus an admin CRUD UI, serializers, routes, a data migration, and updated callers.
Solution Reviewed
New EmbeddableHost model (host + category_id) with validation/normalization; Admin::EmbeddableHostsController (CRUD) and Admin::EmbeddingController (aggregate show/update) on the backend; Ember admin UI (route, controller, embeddable-host component, templates); serializers with embed: :ids; a backfill migration that reads the old settings and deletes them; and updated callers (embed_controller, topic_retriever, topic_embed, topic). The store gains plural *_ids embedded hydration.
Summary
Solid feature shape, but the data migration has two blocking defects — a nil-deref crash on installs that never set embed_category (most installs), and raw SQL interpolation that also bypasses host normalization so migrated hosts silently stop matching at runtime. Several controller/model edge cases (nil guards, case-asymmetric matching, no-op update, missing error handling) and thin test coverage on the new admin CRUD endpoints should be addressed before merge.
Files Reviewed
db/migrate/20150818190757_create_embeddable_hosts.rb— deeply reviewed (high risk: migration)app/models/embeddable_host.rb— deeply reviewed (high risk: security allowlist)app/controllers/admin/embeddable_hosts_controller.rb— deeply reviewed (high risk: admin CRUD)app/controllers/admin/embedding_controller.rb— deeply reviewedapp/controllers/embed_controller.rb— deeply reviewed (high risk: embed access control)app/models/topic_embed.rb,lib/topic_retriever.rb,app/models/topic.rb,app/models/site_setting.rb— reviewed (caller updates)app/assets/javascripts/discourse/models/store.js.es6— deeply reviewed (plural hydration change)app/assets/javascripts/admin/**(adapter, component, controller, route, templates) — reviewedconfig/routes.rb,config/site_settings.yml,config/locales/*.yml— reviewedspec/**— reviewed (coverage gaps noted)
Verification
gh pr diff/gh apireview history read — done- Code re-read in worktree for each cited line (store.js, embed_controller.rb, embeddable_host.rb, topic_spec.rb) — confirmed line numbers and behavior
- Test suite — not run (no Ruby/JS toolchain invoked; findings are from static analysis of the diff)
Issues Found
See inline comments. 2 blocking (migration), 11 non-blocking, 2 suggestions.
Verdict
Recommend changes before merge — the migration will crash on fresh installs and silently break embedding for pre-existing hosts with scheme/path; fix those plus the controller nil guards before shipping.
|
|
||
| category_id = execute("SELECT c.id FROM categories AS c | ||
| INNER JOIN site_settings AS s ON s.value = c.name | ||
| WHERE s.name = 'embed_category'")[0]['id'].to_i |
There was a problem hiding this comment.
🔴 Blocking: migration crashes on most installs.
execute("SELECT c.id ... WHERE s.name = 'embed_category'")[0]['id'].to_i dereferences the first row of an empty result set. Discourse only persists overridden settings to site_settings (defaults live in memory); embed_category defaulted to '', so on any install where an admin never explicitly set it the query returns zero rows, [0] is nil, and nil['id'] raises NoMethodError — halting db:migrate.
The if category_id == 0 fallback on line 14 is unreachable: the exception fires on line 11 before assignment. Line 15 has the same unguarded [0]['value'] pattern.
Fix: guard the result so the fallback actually runs:
row = execute("SELECT c.id FROM categories AS c
INNER JOIN site_settings AS s ON s.value = c.name
WHERE s.name = 'embed_category'").first
category_id = row ? row['id'].to_i : 0
if category_id == 0
row = execute("SELECT value FROM site_settings WHERE name = 'uncategorized_category_id'").first
category_id = row ? row['value'].to_i : SiteSetting.uncategorized_category_id
end| 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)" |
There was a problem hiding this comment.
🔴 Blocking: raw SQL interpolation + migrated hosts bypass normalization.
Two problems on this line:
-
SQL injection / breakage.
#{h}is interpolated directly into the INSERT. The oldembeddable_hostssetting had no format validator, so a value containing a single quote either breaks the statement mid-migration (after some rows inserted, before theDELETE FROM site_settingson line 31) or executes arbitrary SQL during upgrade. On a retried run the source data is already gone. Use parameterized SQL orActiveRecord::Base.connection.quote(h). -
Normalization bypassed. The INSERT copies raw
hvalues verbatim, skippingEmbeddableHost'sbefore_validationscheme/path stripping. The old setting accepted entries likehttp://discourse.orgorexample.com/1234. At runtimerecord_for_hostcompares againstURI(referer).host(no scheme/path), so a stored rowhttp://discourse.orgnever matches referer hostdiscourse.org— embedding silently breaks for every host saved with a scheme or path.
Fix: create through the model so normalization runs, and use the new table to backfill:
records.each do |h|
EmbeddableHost.create!(host: h, category_id: category_id)
end| @@ -0,0 +1,33 @@ | |||
| class CreateEmbeddableHosts < ActiveRecord::Migration | |||
| def change | |||
| create_table :embeddable_hosts, force: true do |t| | |||
There was a problem hiding this comment.
🟡 create_table :embeddable_hosts, force: true combined with irreversible raw execute (INSERT/DELETE) makes this migration non-idempotent and unsafe to roll back. db:rollback reverses create_table (drops the table) but does not reverse the DELETE FROM site_settings, permanently destroying the source data on rollback. Use def up/def down (re-creating the site_settings rows in down) instead of def change, and drop force: true unless you intentionally want to clobber an existing table.
|
|
||
| def update | ||
| host = EmbeddableHost.where(id: params[:id]).first | ||
| save_host(host) |
There was a problem hiding this comment.
🟡 No nil guards on update/destroy.
EmbeddableHost.where(id: params[:id]).first returns nil for a missing/stale id, so save_host(nil) (→ nil.host=, NoMethodError) and nil.destroy (line 15) raise unhandled 500s instead of 404. save_host (line 23) also dereferences params[:embeddable_host][:host] without checking params[:embeddable_host].present?, producing 500 on a malformed body instead of 422.
def update
host = EmbeddableHost.where(id: params[:id]).first
return render_json_error('host not found', status: 404) if host.blank?
save_host(host)
end
def destroy
host = EmbeddableHost.where(id: params[:id]).first
return render_json_error('host not found', status: 404) if host.blank?
host.destroy
render json: success_json
end| end | ||
|
|
||
| def update | ||
| render_serialized(@embedding, EmbeddingSerializer, root: 'embedding', rest_serializer: true) |
There was a problem hiding this comment.
🟡 update is a no-op returning 200 OK.
update ignores all request params and re-renders the same in-memory OpenStruct (rebuilt fresh from the DB on every request via fetch_embedding). It persists nothing. The Ember saveChanges action that PUTs here is dead code — embedding.hbs has no save button. A successful PUT /admin/customize/embedding that mutated nothing is misleading. Either implement real persistence for global embed settings, or remove the put customize/embedding route, the update action, and the Ember saveChanges action.
| <td>{{category-badge host.category}}</td> | ||
| <td> | ||
| {{d-button icon="pencil" action="edit"}} | ||
| {{d-button icon="trash-o" action="delete" class='btn-danger'}} |
There was a problem hiding this comment.
🟡 The delete button has no disabled binding, unlike save/cancel which bind disabled=cantSave/disabled=host.isSaving. A user can click delete twice while destroyRecord is in flight.
| 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) |
There was a problem hiding this comment.
🟡 Conflated error states + dead rescue.
Removing the 'embeddable hosts not set' guard means all failures — unconfigured, nil referer (e.g. Referrer-Policy: no-referrer), or invalid host — now surface as the single message 'invalid referer host', so operators can't distinguish "add hosts" from "fix the referer". Consider keeping a distinct message when no EmbeddableHost exists.
Also, the rescue URI::InvalidURIError on line 65 is dead code: record_for_host already swallows URI errors via rescue nil, and URI(nil) (a nil referer) raises TypeError, not URI::InvalidURIError — so that path is never reached here.
| @@ -0,0 +1,9 @@ | |||
| require 'spec_helper' | |||
|
|
|||
| describe Admin::EmbeddableHostsController do | |||
There was a problem hiding this comment.
🟡 Missing test coverage for new CRUD controllers.
Both embeddable_hosts_controller_spec.rb and embedding_controller_spec.rb only assert the AdminController subclass relationship. There are no tests for: create/update/destroy happy + error paths; authorization (non-staff/admin rejection); missing-record (404) or invalid-params handling; or the show/update serialization shape. These are new admin endpoints managing a security-critical embed allowlist — they need real request specs.
| 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 |
There was a problem hiding this comment.
💡 Fabricator definitions are in swapped files.
category_fabricator.rb now defines :embeddable_host, while embeddable_host_fabricator.rb holds all the category fabricators (:category, :diff_category, :happy_category, :private_category) — the opposite of what the filenames imply. Functionally harmless (Fabrication loads the whole directory by symbol) but confusing for future maintainers. Move Fabricator(:embeddable_host) into embeddable_host_fabricator.rb and restore the category fabricators here.
| end | ||
| let(:topic) { Fabricate.build(:topic) } | ||
|
|
||
| it "is false if embeddable_host is blank" do |
There was a problem hiding this comment.
💡 expandable_first_post? test is now vacuous.
The code dropped SiteSetting.embeddable_hosts.present? from expandable_first_post? (now embed_truncate? && has_topic_embed?). This test has no before block, so embed_truncate defaults to false and the assertion holds regardless of any embeddable-host state — it doesn't actually exercise the condition its name implies. The real behavior change (returns true with embed_truncate=true + has_topic_embed=true but no embeddable hosts configured) is untested. Rename the test to reflect what it now checks and add a no-hosts case to document the intentional change.
4 participants
Test 10