FEATURE: Can edit category/host relationships for embedding

[joelachance]

Benchmark PR recreated from ai-code-review-evaluation/discourse-graphite for Code Review Bench. Upstream: ai-code-review-evaluation#10

[joelachance]

SQL injection in migration via string interpolation

The migration inserts host values from the embeddable_hosts site setting directly into SQL using Ruby string interpolation ('#{h}'). If any existing host value contains a single quote or other SQL metacharacters, the INSERT will either fail or allow SQL injection during the migration run.

Prevents migration failure or SQL injection when migrating data that contains unexpected characters (e.g., a host with an apostrophe or attacker-controlled content in site_settings).

In db/migrate/20150818190757_create_embeddable_hosts.rb line 25, replace raw interpolation with a properly quoted value. Use quote(h) (available via connection.quote) or use ActiveRecord::Base.connection.quote(h). For example: execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES (#{connection.quote(h)}, #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)". Verify by running the migration against a database where embeddable_hosts contains a value with a single quote.

[joelachance]

NullPointerError in update/destroy when host not found

In Admin::EmbeddableHostsController, both update and destroy call EmbeddableHost.where(id: params[:id]).first without checking for nil. If an invalid or deleted ID is provided, host is nil and the subsequent host.host = ... or host.destroy raises NoMethodError. This is a reachable path for any admin making concurrent edits or crafting requests.

Prevents 500 errors on the admin panel when an embeddable host record no longer exists, and avoids potential information disclosure through stack traces.

In app/controllers/admin/embeddable_hosts_controller.rb, add a nil guard after the lookup in both update (line 10) and destroy (line 14). For example: host = EmbeddableHost.where(id: params[:id]).first; raise Discourse::NotFound unless host. Verify by sending a PUT/DELETE request with a non-existent ID and confirming a 404 response.

[joelachance]

Migration crashes with NoMethodError when embed_category setting is absent

The migration unconditionally accesses [0]['id'] on the result of the embed_category query. If the embed_category site setting does not exist or the category name doesn't match, the query returns zero rows. Accessing [0] on an empty PG::Result returns nil, and calling ['id'] on nil raises NoMethodError, aborting the migration.

Prevents a hard crash during migration for any Discourse instance that never configured embed_category, which is the common case for instances not using embedding.

In db/migrate/20150818190757_create_embeddable_hosts.rb around line 9, guard against an empty result set. For example:

result = execute("SELECT c.id FROM categories AS c INNER JOIN site_settings AS s ON s.value = c.name WHERE s.name = 'embed_category'")
category_id = (result.cmd_tuples > 0 ? result[0]['id'].to_i : 0)

Then the existing if category_id == 0 fallback handles it. Verify by running the migration on a database without an embed_category setting.

[joelachance]

_hydrateEmbedded unconditionally deletes _ids key even when hydration returns undefined

In store.js.es6, when processing _ids arrays, the code maps each ID through _lookupSubType. If _lookupSubType returns undefined for any ID (because the sub-type collection is missing from the root payload), the hydrated array contains undefined entries. The code then assigns obj[self.pluralize(subType)] = hydrated || [] — but since hydrated is always an array (possibly containing undefineds), the || [] fallback never triggers. Meanwhile it unconditionally delete obj[k], destroying the original ID array.

Prevents silent data corruption where embedded relationship arrays contain undefined entries instead of the actual IDs, causing downstream template errors.

In app/assets/javascripts/discourse/models/store.js.es6 around line 197, add a check: only delete obj[k] and assign the hydrated array if all entries resolved (i.e., none are undefined). For example:

const hydrated = obj[k].map(function(id) {
  return self._lookupSubType(subType, type, id, root);
});
if (hydrated.every(function(h) { return h !== undefined; })) {
  obj[self.pluralize(subType)] = hydrated;
  delete obj[k];
}

Verify with a test where the root payload is missing the sub-type collection.

[joelachance]

basePath replace('_', '-') only replaces first occurrence

In rest.js.es6, the code uses type.replace('_', '-') to normalize type names before checking the ADMIN_MODELS array. JavaScript's String.replace with a string pattern (not a regex) only replaces the first occurrence. Any model type with multiple underscores (e.g., a future some_admin_model) would not be fully normalized, causing it to miss the ADMIN_MODELS check and route to the wrong base path.

Ensures all admin model types with underscores are correctly recognized regardless of how many underscores they contain.

In app/assets/javascripts/discourse/adapters/rest.js.es6 line 22, change type.replace('_', '-') to type.replace(/_/g, '-') to replace all underscores. Verify by adding a test with a type containing multiple underscores.

[joelachance]

EmbeddableHost.record_for_host returns false instead of nil, breaking .present? contract

The record_for_host method returns false in error cases and an ActiveRecord object (or nil from .first) in the success case. The caller host_allowed? calls .present? on the result, which works, but TopicEmbed (line 35 in topic_embed.rb) calls eh.try(:category_id) on the return value. When record_for_host returns false (not nil), false.try(:category_id) returns nil correctly in Rails, but this is fragile and semantically incorrect — a boolean posing as an optional record.

Prevents subtle bugs if any caller uses truthiness checks (e.g., if eh) rather than .present?, since false is falsy but violates the expected nil-or-record contract.

In app/models/embeddable_host.rb lines 11-12, change return false to return nil in both early-return paths of record_for_host. This aligns the method with standard ActiveRecord finder semantics. Verify the existing specs still pass.

[joelachance]

Fix SQL injection / broken-data risk in embeddable_hosts migration

The migration builds SQL with string interpolation from the old site_settings.embeddable_hosts value. If that value contains a quote, it will break the migration; if an attacker ever managed to write to that setting, it becomes SQL injection during migration. This is avoidable by using bound parameters / proper quoting.

Prevents migration failures on real-world data (hosts containing quotes/whitespace) and removes a direct SQL-injection primitive during deployment.

In db/migrate/20150818190757_create_embeddable_hosts.rb, replace the interpolated execute "INSERT ... VALUES ('#{h}', ...)" with either:

• EmbeddableHost.create!(host: h, category_id: category_id) (preferred for safety/readability), or
• execute with sanitized quoting, e.g. quoted = ActiveRecord::Base.connection.quote(h) and use VALUES (#{quoted}, ...).
  Then verify by running bundle exec rake db:migrate against a DB where site_settings.embeddable_hosts contains values with ' and leading/trailing spaces.

[joelachance]

Prevent nil crashes and bad writes in Admin::EmbeddableHostsController update/destroy paths

update/destroy use EmbeddableHost.where(id: params[:id]).first and then immediately call save_host(host) / host.destroy. If the id is missing/invalid, host is nil and this will 500. This is reachable via /admin/embeddable_hosts/:id with a non-existent id.

Avoids 500s in admin UI and makes failure mode explicit (404), improving reliability and debuggability.

In app/controllers/admin/embeddable_hosts_controller.rb, replace the lookups with EmbeddableHost.find_by(id: params[:id]) and handle nil:

• return render_json_error("not_found", status: 404) or raise ActiveRecord::RecordNotFound (depending on existing controller conventions).
  Also in destroy, only call destroy when found.
  Verify with a controller spec hitting PUT /admin/embeddable_hosts/999999 and DELETE /admin/embeddable_hosts/999999 expecting 404.

[joelachance]

Make store.js plural embedded hydration omit nulls (currently returns arrays containing undefined)

The new _hydrateEmbedded logic for *_ids maps each id through _lookupSubType. When an id can’t be resolved (missing from root payload), _lookupSubType returns undefined and the array will contain undefined entries. Callers expecting an array of models may break (e.g., .get('id') on undefined).

Prevents hard-to-debug client-side exceptions when server payloads are partially embedded or when ids reference missing objects.

In app/assets/javascripts/discourse/models/store.js.es6, when m[2] is present, filter falsy results:

const hydrated = obj[k]
  .map(id => self._lookupSubType(subType, type, id, root))
  .filter(Boolean);
obj[self.pluralize(subType)] = hydrated;

Add/adjust test/javascripts/models/store-test.js.es6 to include an id in color_ids that is not present in the colors collection and assert the resulting colors array excludes it (and does not throw).