Skip to content

Automated Test: rest-serializer-enhancement-post #316

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions app/assets/javascripts/admin/adapters/embedding.js.es6
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import RestAdapter from 'discourse/adapters/rest';

export default RestAdapter.extend({
pathFor() {
return "/admin/customize/embedding";
}
});
63 changes: 63 additions & 0 deletions app/assets/javascripts/admin/components/embeddable-host.js.es6
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import { bufferedProperty } from 'discourse/mixins/buffered-content';
import computed from 'ember-addons/ember-computed-decorators';
import { on, observes } from 'ember-addons/ember-computed-decorators';
import { popupAjaxError } from 'discourse/lib/ajax-error';

export default Ember.Component.extend(bufferedProperty('host'), {
editToggled: false,
tagName: 'tr',
categoryId: null,

editing: Ember.computed.or('host.isNew', 'editToggled'),

@on('didInsertElement')
@observes('editing')
_focusOnInput() {
Ember.run.schedule('afterRender', () => { this.$('.host-name').focus(); });
},

@computed('buffered.host', 'host.isSaving')
cantSave(host, isSaving) {
return isSaving || Ember.isEmpty(host);
},

actions: {
edit() {
this.set('categoryId', this.get('host.category.id'));
this.set('editToggled', true);
},

save() {
if (this.get('cantSave')) { return; }

const props = this.get('buffered').getProperties('host');
props.category_id = this.get('categoryId');

const host = this.get('host');
host.save(props).then(() => {
host.set('category', Discourse.Category.findById(this.get('categoryId')));
this.set('editToggled', false);
}).catch(popupAjaxError);
},

delete() {
bootbox.confirm(I18n.t('admin.embedding.confirm_delete'), (result) => {
if (result) {
this.get('host').destroyRecord().then(() => {
this.sendAction('deleteHost', this.get('host'));
});
}
});
},
Comment on lines +43 to +51

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Missing error handling on destroyRecord().

Unlike the save action which uses .catch(popupAjaxError), the delete action silently swallows server errors. If deletion fails (e.g., 403/500), the user receives no feedback and the host remains in the list without any indication of failure.

🐛 Proposed fix to add error handling
     delete() {
       bootbox.confirm(I18n.t('admin.embedding.confirm_delete'), (result) => {
         if (result) {
           this.get('host').destroyRecord().then(() => {
             this.sendAction('deleteHost', this.get('host'));
-          });
+          }).catch(popupAjaxError);
         }
       });
     },
🤖 Prompt for AI Agents
In `@app/assets/javascripts/admin/components/embeddable-host.js.es6` around lines
43 - 51, The delete() action currently calls this.get('host').destroyRecord()
without error handling; update delete() to attach a .catch handler that calls
popupAjaxError (same error handler used by save) and ensure the promise chain
still triggers sendAction('deleteHost', this.get('host')) only on success;
specifically modify the destroyRecord().then(...) to destroyRecord().then(() =>
{ this.sendAction('deleteHost', this.get('host')); }).catch(popupAjaxError) so
server errors are surfaced to the user.


cancel() {
const host = this.get('host');
if (host.get('isNew')) {
this.sendAction('deleteHost', host);
} else {
this.rollbackBuffer();
this.set('editToggled', false);
}
}
}
});
18 changes: 18 additions & 0 deletions app/assets/javascripts/admin/controllers/admin-embedding.js.es6
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
export default Ember.Controller.extend({
embedding: null,

actions: {
saveChanges() {
this.get('embedding').update({});
Comment on lines +5 to +6

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

No error handling on saveChanges.

this.get('embedding').update({}) returns a promise, but failures are silently swallowed. Consider adding a .catch() to surface errors to the user (e.g., via a flash message or notification).

Proposed fix
     saveChanges() {
-      this.get('embedding').update({});
+      this.get('embedding').update({}).catch(popupAjaxError);
     },

You'll need to import popupAjaxError (or your app's equivalent error handler) at the top of the file.

🤖 Prompt for AI Agents
In `@app/assets/javascripts/admin/controllers/admin-embedding.js.es6` around lines
5 - 6, The saveChanges method calls this.get('embedding').update({}) but doesn't
handle the returned promise; update saveChanges to attach a .catch(...) that
surfaces errors (e.g., show a flash/notification) and import the app's error
handler (popupAjaxError or equivalent) at the top of the file; specifically,
locate the saveChanges method and change the call to
this.get('embedding').update({}).catch(err => popupAjaxError(err)) (or use your
app's notification API) and add the popupAjaxError import to ensure failures are
not silently swallowed.

},

addHost() {
const host = this.store.createRecord('embeddable-host');
this.get('embedding.embeddable_hosts').pushObject(host);
},

deleteHost(host) {
this.get('embedding.embeddable_hosts').removeObject(host);
}
}
});
9 changes: 9 additions & 0 deletions app/assets/javascripts/admin/routes/admin-embedding.js.es6
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export default Ember.Route.extend({
model() {
return this.store.find('embedding');
},

setupController(controller, model) {
controller.set('embedding', model);
}
});
1 change: 1 addition & 0 deletions app/assets/javascripts/admin/routes/admin-route-map.js.es6
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export default {
this.resource('adminUserFields', { path: '/user_fields' });
this.resource('adminEmojis', { path: '/emojis' });
this.resource('adminPermalinks', { path: '/permalinks' });
this.resource('adminEmbedding', { path: '/embedding' });
});
this.route('api');

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{{#if editing}}
<td>
{{input value=buffered.host placeholder="example.com" enter="save" class="host-name"}}
</td>
<td>
{{category-chooser value=categoryId}}
</td>
<td>
{{d-button icon="check" action="save" class="btn-primary" disabled=cantSave}}
{{d-button icon="times" action="cancel" class="btn-danger" disabled=host.isSaving}}
</td>
{{else}}
<td>{{host.host}}</td>
<td>{{category-badge host.category}}</td>
<td>
{{d-button icon="pencil" action="edit"}}
{{d-button icon="trash-o" action="delete" class='btn-danger'}}
</td>
{{/if}}
1 change: 1 addition & 0 deletions app/assets/javascripts/admin/templates/customize.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
{{nav-item route='adminUserFields' label='admin.user_fields.title'}}
{{nav-item route='adminEmojis' label='admin.emoji.title'}}
{{nav-item route='adminPermalinks' label='admin.permalink.title'}}
{{nav-item route='adminEmbedding' label='admin.embedding.title'}}
{{/admin-nav}}

<div class="admin-container">
Expand Down
15 changes: 15 additions & 0 deletions app/assets/javascripts/admin/templates/embedding.hbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{{#if embedding.embeddable_hosts}}
<table>
<tr>
<th style='width: 50%'>{{i18n "admin.embedding.host"}}</th>
<th style='width: 30%'>{{i18n "admin.embedding.category"}}</th>
<th style='width: 20%'>&nbsp;</th>
</tr>
{{#each embedding.embeddable_hosts as |host|}}
{{embeddable-host host=host deleteHost="deleteHost"}}
{{/each}}
</table>
{{/if}}

{{d-button label="admin.embedding.add_host" action="addHost" icon="plus" class="btn-primary"}}

4 changes: 2 additions & 2 deletions app/assets/javascripts/discourse/adapters/rest.js.es6
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const ADMIN_MODELS = ['plugin', 'site-customization'];
const ADMIN_MODELS = ['plugin', 'site-customization', 'embeddable-host'];

export function Result(payload, responseJson) {
this.payload = payload;
Expand All @@ -19,7 +19,7 @@ function rethrow(error) {
export default Ember.Object.extend({

basePath(store, type) {
if (ADMIN_MODELS.indexOf(type) !== -1) { return "/admin/"; }
if (ADMIN_MODELS.indexOf(type.replace('_', '-')) !== -1) { return "/admin/"; }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

String.replace with a string pattern only replaces the first underscore.

type.replace('_', '-') replaces only the first occurrence of _. If a model type with multiple underscores is ever added to ADMIN_MODELS, the lookup will silently fail. Use a regex with the global flag instead.

Suggested fix
-    if (ADMIN_MODELS.indexOf(type.replace('_', '-')) !== -1) { return "/admin/"; }
+    if (ADMIN_MODELS.indexOf(type.replace(/_/g, '-')) !== -1) { return "/admin/"; }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (ADMIN_MODELS.indexOf(type.replace('_', '-')) !== -1) { return "/admin/"; }
if (ADMIN_MODELS.indexOf(type.replace(/_/g, '-')) !== -1) { return "/admin/"; }
🤖 Prompt for AI Agents
In `@app/assets/javascripts/discourse/adapters/rest.js.es6` at line 22, The lookup
uses type.replace('_', '-') which only replaces the first underscore so
ADMIN_MODELS membership checks can fail for names with multiple underscores;
update the expression used in the membership test (the call to type.replace('_',
'-')) to perform a global replace (e.g., use a regex with the global flag) so
all underscores are replaced before checking ADMIN_MODELS, leaving the
surrounding if and return logic unchanged.

return "/";
},

Expand Down
18 changes: 14 additions & 4 deletions app/assets/javascripts/discourse/models/store.js.es6
Original file line number Diff line number Diff line change
Expand Up @@ -189,14 +189,24 @@ export default Ember.Object.extend({
_hydrateEmbedded(type, obj, root) {
const self = this;
Object.keys(obj).forEach(function(k) {
const m = /(.+)\_id$/.exec(k);
const m = /(.+)\_id(s?)$/.exec(k);
if (m) {
const subType = m[1];
const hydrated = self._lookupSubType(subType, type, obj[k], root);
if (hydrated) {
obj[subType] = hydrated;

if (m[2]) {
const hydrated = obj[k].map(function(id) {
return self._lookupSubType(subType, type, id, root);
});
obj[self.pluralize(subType)] = hydrated || [];
delete obj[k];
} else {
const hydrated = self._lookupSubType(subType, type, obj[k], root);
if (hydrated) {
obj[subType] = hydrated;
delete obj[k];
}
}

Comment on lines +192 to +209

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Multi-id hydration can silently introduce undefined entries and unconditionally deletes the source key.

Two concerns with the multi-id branch:

  1. undefined in hydrated array: If _lookupSubType returns undefined for any ID (e.g., a missing entry in root), the resulting array will contain undefined elements. Downstream code (including the tests) calls .get('id') on these items, which would throw.

  2. Asymmetric delete: The single-id branch (line 204-206) only deletes the _id key when hydration succeeds, preserving the raw ID as a fallback. The multi-id branch always deletes _ids (line 201) even if hydration produced undefined entries, losing the original IDs with no fallback.

Additionally, || [] on line 200 is dead code since .map() always returns an array.

Proposed fix: filter out failed lookups and guard the delete
         if (m[2]) {
-          const hydrated = obj[k].map(function(id) {
-            return self._lookupSubType(subType, type, id, root);
-          });
-          obj[self.pluralize(subType)] = hydrated || [];
+          const hydrated = obj[k].map(function(id) {
+            return self._lookupSubType(subType, type, id, root);
+          }).filter(Boolean);
+          obj[self.pluralize(subType)] = hydrated;
           delete obj[k];
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const m = /(.+)\_id(s?)$/.exec(k);
if (m) {
const subType = m[1];
const hydrated = self._lookupSubType(subType, type, obj[k], root);
if (hydrated) {
obj[subType] = hydrated;
if (m[2]) {
const hydrated = obj[k].map(function(id) {
return self._lookupSubType(subType, type, id, root);
});
obj[self.pluralize(subType)] = hydrated || [];
delete obj[k];
} else {
const hydrated = self._lookupSubType(subType, type, obj[k], root);
if (hydrated) {
obj[subType] = hydrated;
delete obj[k];
}
}
const m = /(.+)\_id(s?)$/.exec(k);
if (m) {
const subType = m[1];
if (m[2]) {
const hydrated = obj[k].map(function(id) {
return self._lookupSubType(subType, type, id, root);
}).filter(Boolean);
obj[self.pluralize(subType)] = hydrated;
delete obj[k];
} else {
const hydrated = self._lookupSubType(subType, type, obj[k], root);
if (hydrated) {
obj[subType] = hydrated;
delete obj[k];
}
}
🤖 Prompt for AI Agents
In `@app/assets/javascripts/discourse/models/store.js.es6` around lines 192 - 209,
For the multi-id branch, avoid producing undefined entries and avoid
unconditionally deleting the source _ids key: first collect the ids (const ids =
obj[k]) and map to hydrated = ids.map(id => self._lookupSubType(subType, type,
id, root)); remove the dead "|| []" and do not assign undefined entries — set
obj[self.pluralize(subType)] to hydratedFiltered = hydrated.filter(h => h !=
null); only delete the original obj[k] if every lookup succeeded (if
hydrated.every(h => h != null)), otherwise keep obj[k] as a fallback while
assigning the filtered list.

}
});
},
Expand Down
34 changes: 34 additions & 0 deletions app/controllers/admin/embeddable_hosts_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
class Admin::EmbeddableHostsController < Admin::AdminController

before_filter :ensure_logged_in, :ensure_staff

def create
save_host(EmbeddableHost.new)
end

def update
host = EmbeddableHost.where(id: params[:id]).first
save_host(host)
end

def destroy
host = EmbeddableHost.where(id: params[:id]).first
host.destroy
render json: success_json
end
Comment on lines +9 to +18

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

update and destroy crash with NoMethodError when the record is not found.

EmbeddableHost.where(id: params[:id]).first returns nil if no matching record exists. Lines 11 and 16 then call methods on nil, crashing the request. Use find (which raises ActiveRecord::RecordNotFound, returning a 404) or add a nil guard.

Proposed fix
   def update
-    host = EmbeddableHost.where(id: params[:id]).first
+    host = EmbeddableHost.find(params[:id])
     save_host(host)
   end
 
   def destroy
-    host = EmbeddableHost.where(id: params[:id]).first
+    host = EmbeddableHost.find(params[:id])
     host.destroy
     render json: success_json
   end
🤖 Prompt for AI Agents
In `@app/controllers/admin/embeddable_hosts_controller.rb` around lines 9 - 18,
The update and destroy actions call methods on a potentially nil record because
they use EmbeddableHost.where(id: params[:id]).first; change these to use
EmbeddableHost.find(params[:id]) so Rails will raise
ActiveRecord::RecordNotFound (rendering a 404) or alternatively add a nil guard
that renders a 404 when the lookup returns nil before calling save_host or
host.destroy; update the update method (which calls save_host) and the destroy
method to use one of these two approaches.


protected

def save_host(host)
host.host = params[:embeddable_host][:host]
host.category_id = params[:embeddable_host][:category_id]
host.category_id = SiteSetting.uncategorized_category_id if host.category_id.blank?

if host.save
render_serialized(host, EmbeddableHostSerializer, root: 'embeddable_host', rest_serializer: true)
else
render_json_error(host)
end
end

end
21 changes: 21 additions & 0 deletions app/controllers/admin/embedding_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
class Admin::EmbeddingController < Admin::AdminController

before_filter :ensure_logged_in, :ensure_staff, :fetch_embedding

def show
render_serialized(@embedding, EmbeddingSerializer, root: 'embedding', rest_serializer: true)
end

def update
render_serialized(@embedding, EmbeddingSerializer, root: 'embedding', rest_serializer: true)
end
Comment on lines +9 to +11

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

update action is a no-op — request params are never applied.

The update action re-renders the same serialized @embedding as show without reading or persisting any parameters from the request. This means PUT requests to customize/embedding silently discard all submitted changes. If this is intentional as a stub, it should be documented; otherwise, the action needs to actually process params and persist changes.

🤖 Prompt for AI Agents
In `@app/controllers/admin/embedding_controller.rb` around lines 9 - 11, The
update action currently just re-renders `@embedding` and ignores request params;
change it to apply strong params to `@embedding` (e.g. call an
embedding_params/permit method), attempt to update and persist (use
`@embedding.update`(embedding_params) or equivalent), and then
render_serialized(`@embedding`, EmbeddingSerializer, root: 'embedding',
rest_serializer: true) on success or return appropriate error responses
(validation errors / 422) on failure; also add or reuse a private
embedding_params method to whitelist allowed attributes.


protected

def fetch_embedding
@embedding = OpenStruct.new({
id: 'default',
embeddable_hosts: EmbeddableHost.all.order(:host)
})
end
end
3 changes: 1 addition & 2 deletions app/controllers/embed_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,7 @@ def count
def ensure_embeddable

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)
end

response.headers['X-Frame-Options'] = "ALLOWALL"
Expand Down
24 changes: 24 additions & 0 deletions app/models/embeddable_host.rb
Original file line number Diff line number Diff line change
@@ -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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

TLD length cap of 5 characters rejects valid domains.

The regex [a-z]{2,5} for the TLD portion rejects valid TLDs longer than 5 characters (e.g., .museum, .technology, .community, and many newer gTLDs). Consider relaxing to [a-z]{2,} or [a-z]{2,24}.

Proposed fix
-  validates_format_of :host, :with => /\A[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(:[0-9]{1,5})?(\/.*)?\Z/i
+  validates_format_of :host, :with => /\A[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,24}(:[0-9]{1,5})?(\/.*)?\Z/i
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
validates_format_of :host, :with => /\A[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(:[0-9]{1,5})?(\/.*)?\Z/i
validates_format_of :host, :with => /\A[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,24}(:[0-9]{1,5})?(\/.*)?\Z/i
🤖 Prompt for AI Agents
In `@app/models/embeddable_host.rb` at line 2, The host validation regex in
validates_format_of :host currently caps TLDs with `[a-z]{2,5}` which rejects
longer valid gTLDs; update the regex in embeddable_host.rb (the
validates_format_of :host line) to allow longer TLDs such as `[a-z]{2,}` (or a
bounded range like `{2,24}`) so modern TLDs pass validation, keeping the rest of
the pattern intact and preserving the case-insensitive flag.

belongs_to :category

before_validation do
self.host.sub!(/^https?:\/\//, '')
self.host.sub!(/\/.*$/, '')
end
Comment on lines +5 to +8

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

NoMethodError if host is nil.

self.host.sub! on Line 6 will raise NoMethodError: undefined method 'sub!' for nil:NilClass when host is nil, since before_validation runs before validates_format_of. Guard against nil:

Proposed fix
   before_validation do
+    return if self.host.blank?
     self.host.sub!(/^https?:\/\//, '')
     self.host.sub!(/\/.*$/, '')
   end
🤖 Prompt for AI Agents
In `@app/models/embeddable_host.rb` around lines 5 - 8, The before_validation
block calls self.host.sub! which raises NoMethodError when host is nil; update
the before_validation callback to guard against nil by returning early or only
calling sub! when host is present (e.g., check host.present? or host && before
mutating), so the two substitutions in the block (self.host.sub!(/^https?:\/\//,
'') and self.host.sub!(/\/.*$/, '')) are only invoked when host is not nil.


def self.record_for_host(host)
uri = URI(host) rescue nil
return false unless uri.present?

host = uri.host
return false unless host.present?

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

def self.host_allowed?(host)
record_for_host(host).present?
end

end
14 changes: 0 additions & 14 deletions app/models/site_setting.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,20 +68,6 @@ def self.anonymous_menu_items
@anonymous_menu_items ||= Set.new Discourse.anonymous_filters.map(&:to_s)
end

def self.allows_embeddable_host?(host)
return false if embeddable_hosts.blank?
uri = URI(host) rescue nil
return false unless uri.present?

host = uri.host
return false unless host.present?

!!embeddable_hosts.split("\n").detect {|h| h.sub(/^https?\:\/\//, '') == host }

hosts = embeddable_hosts.split("\n").map {|h| (URI(h).host rescue nil) || h }
!!hosts.detect {|h| h == host}
end

def self.anonymous_homepage
top_menu_items.map { |item| item.name }
.select { |item| anonymous_menu_items.include?(item) }
Expand Down
2 changes: 1 addition & 1 deletion app/models/topic.rb
Original file line number Diff line number Diff line change
Expand Up @@ -866,7 +866,7 @@ def has_topic_embed?
end

def expandable_first_post?
SiteSetting.embeddable_hosts.present? && SiteSetting.embed_truncate? && has_topic_embed?
SiteSetting.embed_truncate? && has_topic_embed?
end

TIME_TO_FIRST_RESPONSE_SQL ||= <<-SQL
Expand Down
4 changes: 3 additions & 1 deletion app/models/topic_embed.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,14 @@ def self.import(user, url, title, contents)
# If there is no embed, create a topic, post and the embed.
if embed.blank?
Topic.transaction do
eh = EmbeddableHost.record_for_host(url)

creator = PostCreator.new(user,
title: title,
raw: absolutize_urls(url, contents),
skip_validations: true,
cook_method: Post.cook_methods[:raw_html],
category: SiteSetting.embed_category)
category: eh.try(:category_id))
Comment on lines +36 to +43

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Find TopicEmbed.import callers
rg -n 'TopicEmbed\.import' --type=ruby -A 5 -B 5

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 5898


🏁 Script executed:

# Examine the TopicEmbed file structure
fd -e rb -p | grep -i topic_embed | head -20

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 258


🏁 Script executed:

# Read the TopicEmbed.import method to understand the full flow
cat -n app/models/topic_embed.rb | head -100

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 4010


🏁 Script executed:

# Find EmbeddableHost class and record_for_host method
fd -e rb | xargs rg -l "EmbeddableHost" | head -20

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 588


🏁 Script executed:

# Look at EmbeddableHost implementation
fd -e rb -p | xargs grep -l "class EmbeddableHost"

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 163


🏁 Script executed:

# Check poll_feed.rb to understand where the topic comes from
cat -n app/jobs/scheduled/poll_feed.rb | head -70

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 2162


🏁 Script executed:

# Read EmbeddableHost class implementation
cat -n app/models/embeddable_host.rb

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 817


🏁 Script executed:

# Check if there's any host validation/authorization logic elsewhere
rg -n "host_allowed|authorized|record_for_host" --type=ruby

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 5269


🏁 Script executed:

# Check topic_retriever.rb to see how it validates hosts
cat -n lib/topic_retriever.rb

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 1956


🏁 Script executed:

# Check embed_controller.rb to see how it uses host_allowed
cat -n app/controllers/embed_controller.rb | head -80

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 2698


🏁 Script executed:

# Verify all direct callers of TopicEmbed.import (not import_remote)
rg -n '\.import\(' --type=ruby | grep -i "TopicEmbed" | grep -v "import_remote"

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 876


Add host-authorization guard in TopicEmbed.import before creating posts.

The import flow is inconsistent: TopicRetriever validates hosts via EmbeddableHost.host_allowed? before calling import_remote, but PollFeed.import_topic calls TopicEmbed.import directly without any host validation. This means RSS feed imports bypass the host whitelist check that web-requested embeds enforce.

Currently, when EmbeddableHost.record_for_host(url) returns nil (unknown host), the post still gets created with no category. Consider adding an early return to reject unknown hosts:

eh = EmbeddableHost.record_for_host(url)
return unless eh.present?

This ensures consistent security enforcement across all import paths and prevents arbitrary domains from being embedded via RSS feeds.

🤖 Prompt for AI Agents
In `@app/models/topic_embed.rb` around lines 36 - 43, TopicEmbed.import currently
creates posts even when EmbeddableHost.record_for_host(url) returns nil,
allowing unknown hosts through RSS imports; add an early guard in
TopicEmbed.import right after eh = EmbeddableHost.record_for_host(url) to reject
unknown/unallowed hosts (e.g., return unless eh.present? or check
EmbeddableHost.host_allowed?(url)) so that PollFeed.import_topic and other
callers cannot bypass the host whitelist before calling PostCreator.

post = creator.create
if post.present?
TopicEmbed.create!(topic_id: post.topic_id,
Expand Down
16 changes: 16 additions & 0 deletions app/serializers/embeddable_host_serializer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
class EmbeddableHostSerializer < ApplicationSerializer
attributes :id, :host, :category_id

def id
object.id
end

def host
object.host
end

def category_id
object.category_id
end
end

8 changes: 8 additions & 0 deletions app/serializers/embedding_serializer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
class EmbeddingSerializer < ApplicationSerializer
attributes :id
has_many :embeddable_hosts, serializer: EmbeddableHostSerializer, embed: :ids

def id
object.id
end
end
8 changes: 8 additions & 0 deletions config/locales/client.en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2491,6 +2491,14 @@ en:
image: "Image"
delete_confirm: "Are you sure you want to delete the :%{name}: emoji?"

embedding:
confirm_delete: "Are you sure you want to delete that host?"
title: "Embedding"
host: "Allowed Hosts"
edit: "edit"
category: "Post to Category"
add_host: "Add Host"

permalink:
title: "Permalinks"
url: "URL"
Expand Down
2 changes: 0 additions & 2 deletions config/locales/server.en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1164,13 +1164,11 @@ en:
autohighlight_all_code: "Force apply code highlighting to all preformatted code blocks even when they didn't explicitly specify the language."
highlighted_languages: "Included syntax highlighting rules. (Warning: including too many langauges may impact performance) see: https://highlightjs.org/static/demo/ for a demo"

embeddable_hosts: "Host(s) that can embed the comments from this Discourse forum. Hostname only, do not begin with http://"
feed_polling_enabled: "EMBEDDING ONLY: Whether to embed a RSS/ATOM feed as posts."
feed_polling_url: "EMBEDDING ONLY: URL of RSS/ATOM feed to embed."
embed_by_username: "Discourse username of the user who creates the embedded topics."
embed_username_key_from_feed: "Key to pull discourse username from feed."
embed_truncate: "Truncate the embedded posts."
embed_category: "Category of embedded topics."
embed_post_limit: "Maximum number of posts to embed."
embed_whitelist_selector: "CSS selector for elements that are allowed in embeds."
embed_blacklist_selector: "CSS selector for elements that are removed from embeds."
Expand Down
Loading