FEATURE: Can edit category/host relationships for embedding #10
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements a comprehensive embedding system enhancement that allows administrators to edit category/host relationships for embedding functionality. The changes migrate from a simple site setting-based approach to a robust database-backed system using a new EmbeddableHost model.
Key architectural changes:
- Database Migration: Moves embeddable hosts from site settings (
embeddable_hostsandembed_category) to a dedicatedembeddable_hoststable with foreign key relationships to categories - Model Enhancement: Introduces
EmbeddableHostmodel with host validation, URL normalization, and category associations - Admin Interface: Adds comprehensive admin UI with routes, controllers, components, and templates for managing embeddable hosts
- REST API: Enhances the REST serializer and adapter system to handle multiple relationships and admin-specific routing
- Data Flow: Updates embedding logic in
TopicEmber.importto use per-host category configuration instead of global settings
Integration with existing codebase:
The changes integrate deeply with Discourse's existing admin infrastructure, following established patterns for admin controllers, Ember.js components, and REST serialization. The embedding functionality connects with the topic creation system, allowing external sites to embed Discourse content while maintaining proper categorization based on the source host. The implementation preserves backward compatibility while enabling more granular control over embedded content organization.
PR Description Notes:
- The PR body only contains "Test 10" which provides no meaningful description of the changes
Confidence score: 1/5
- This PR has critical security vulnerabilities and fabricator issues that will cause immediate failures
- The main concerns are a SQL injection vulnerability in the migration and completely broken test fabricators that will cause widespread test suite failures
- Files
spec/fabricators/category_fabricator.rbandspec/fabricators/embeddable_host_fabricator.rbhave swapped content, destroying category fabricators and creating non-functional embeddable host fabricators
35 files reviewed, 15 comments
| const hydrated = obj[k].map(function(id) { | ||
| return self._lookupSubType(subType, type, id, root); | ||
| }); | ||
| obj[self.pluralize(subType)] = hydrated || []; |
There was a problem hiding this comment.
style: the || [] fallback may be unnecessary since obj[k].map() will always return an array
| obj[self.pluralize(subType)] = hydrated || []; | |
| obj[self.pluralize(subType)] = hydrated; |
| SiteSetting.embed_truncate = false | ||
| expect(topic.expandable_first_post?).to eq(false) | ||
| end | ||
| describe 'with an emeddable host' do |
There was a problem hiding this comment.
syntax: Typo: 'emeddable' should be 'embeddable'
| describe 'with an emeddable host' do | |
| describe 'with an embeddable host' do |
| def id | ||
| object.id | ||
| end | ||
|
|
||
| def host | ||
| object.host | ||
| end | ||
|
|
||
| def category_id | ||
| object.category_id | ||
| end |
There was a problem hiding this comment.
style: These method definitions are redundant. ActiveModel::Serializer automatically creates attribute methods that return object.attribute_name. You can remove these explicit method definitions unless you need custom logic.
|
|
||
| actions: { | ||
| edit() { | ||
| this.set('categoryId', this.get('host.category.id')); |
There was a problem hiding this comment.
logic: Potential null reference error if host.category is null/undefined. Should add safety check: this.get('host.category')?.id
|
|
||
| const host = this.get('host'); | ||
| host.save(props).then(() => { | ||
| host.set('category', Discourse.Category.findById(this.get('categoryId'))); |
There was a problem hiding this comment.
style: The manual category assignment after save may cause synchronization issues. Consider letting the server response handle category updates.
| 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.
logic: Critical SQL injection vulnerability - host value #{h} is directly interpolated without escaping. Use parameterized queries instead.
| 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 (?, ?, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)", h, category_id |
| Fabricator(:embeddable_host) do | ||
| host "eviltrout.com" | ||
| category | ||
| end |
There was a problem hiding this comment.
logic: This completely removes all category fabricators and replaces them with an embeddable_host fabricator. This will break all tests that use :category, :diff_category, :happy_category, or :private_category fabricators. The embeddable_host fabricator should be in spec/fabricators/embeddable_host_fabricator.rb instead.
| Fabricator(:category) do | ||
| name { sequence(:name) { |n| "Amazing Category #{n}" } } | ||
| user | ||
| end | ||
|
|
||
| Fabricator(:diff_category, from: :category) do | ||
| name "Different Category" | ||
| user | ||
| end | ||
|
|
||
| Fabricator(:happy_category, from: :category) do | ||
| name 'Happy Category' | ||
| slug 'happy' | ||
| user | ||
| end | ||
|
|
||
| Fabricator(:private_category, from: :category) do | ||
| transient :group | ||
|
|
||
| name 'Private Category' | ||
| slug 'private' | ||
| user | ||
| after_build do |cat, transients| | ||
| cat.update!(read_restricted: true) | ||
| cat.category_groups.build(group_id: transients[:group].id, permission_type: CategoryGroup.permission_types[:full]) | ||
| end | ||
| end |
There was a problem hiding this comment.
logic: Critical error: This file should contain EmbeddableHost fabricators, not Category fabricators. The filename indicates this should define fabricators for EmbeddableHost models (which have host and category_id attributes), but instead contains Category model fabricators. This will break tests expecting embeddable_host fabricators.
| Fabricator(:category) do | |
| name { sequence(:name) { |n| "Amazing Category #{n}" } } | |
| user | |
| end | |
| Fabricator(:diff_category, from: :category) do | |
| name "Different Category" | |
| user | |
| end | |
| Fabricator(:happy_category, from: :category) do | |
| name 'Happy Category' | |
| slug 'happy' | |
| user | |
| end | |
| Fabricator(:private_category, from: :category) do | |
| transient :group | |
| name 'Private Category' | |
| slug 'private' | |
| user | |
| after_build do |cat, transients| | |
| cat.update!(read_restricted: true) | |
| cat.category_groups.build(group_id: transients[:group].id, permission_type: CategoryGroup.permission_types[:full]) | |
| end | |
| end | |
| Fabricator(:embeddable_host) do | |
| host "example.com" | |
| category | |
| end | |
| Fabricator(:embeddable_host_with_path, from: :embeddable_host) do | |
| host "example.com" | |
| path_whitelist "^/articles/" | |
| category | |
| end | |
| Fabricator(:embeddable_host_with_category, from: :embeddable_host) do | |
| host "blog.example.com" | |
| category { Fabricate(:category) } | |
| end |
| self.host.sub!(/^https?:\/\//, '') | ||
| self.host.sub!(/\/.*$/, '') |
There was a problem hiding this comment.
logic: Calling sub! on potentially nil host will raise NoMethodError. Add nil check: self.host&.sub!(/^https?:\/\//, '') or ensure host is present first.
| self.host.sub!(/^https?:\/\//, '') | |
| self.host.sub!(/\/.*$/, '') | |
| return unless self.host.present? | |
| self.host.sub!(/^https?:\/\//, '') | |
| self.host.sub!(/\/.*$/, '') |
| 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.
logic: The removal of the blank check for embeddable_hosts setting changes the behavior. Previously, if no hosts were configured, embedding would be blocked entirely. Now it only checks if the specific referer host exists in the database. Consider if this maintains the intended security posture.
2 participants
Test 10