FEATURE: Can edit category/host relationships for embedding#1
FEATURE: Can edit category/host relationships for embedding#1ShashankFC wants to merge 1 commit into
Conversation
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 5 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
|
|
||
| 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.
Migration crashes when embed_category setting is unset
High Severity
The migration unconditionally accesses [0]['id'] on the query result for embed_category, which raises a NoMethodError on nil when no rows are returned. This happens for any instance that never configured embed_category (including fresh installs). The fallback at line 15 has the same issue — accessing [0]['value'] on an empty result for uncategorized_category_id. Both queries need nil-checking before element access.
Additional Locations (1)
| 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)" | ||
| end |
There was a problem hiding this comment.
SQL injection via string interpolation in migration
Medium Severity
Host values from site_settings are interpolated directly into a SQL string with '#{h}', creating a SQL injection risk. If any stored host contains a single quote (e.g., from prior data corruption or manual database editing), the migration would either crash or execute unintended SQL. Parameterized queries or proper escaping with quote would be safer.
| host = uri.host | ||
| return false unless host.present? | ||
|
|
||
| where("lower(host) = ?", host).first |
There was a problem hiding this comment.
Case-insensitive host lookup only lowercases one side
Medium Severity
record_for_host applies lower() to the database column but not to the input parameter from uri.host. Since Ruby's URI preserves the original casing of the hostname, a referer like http://EvilTrout.com would produce a query comparing 'eviltrout.com' = 'EvilTrout.com', which fails in PostgreSQL's case-sensitive string comparison. The input parameter also needs to be lowercased.
| Fabricator(:category) do | ||
| name { sequence(:name) { |n| "Amazing Category #{n}" } } | ||
| user | ||
| end |
There was a problem hiding this comment.
Fabricator definitions placed in wrong files
Low Severity
The fabricator file contents are swapped — category_fabricator.rb now contains the Fabricator(:embeddable_host) definition, while embeddable_host_fabricator.rb contains all the Fabricator(:category) definitions. While Fabrication loads by name so tests still pass, this is very confusing for anyone looking for these definitions by filename convention.
Additional Locations (1)
|
|
||
| basePath(store, type) { | ||
| if (ADMIN_MODELS.indexOf(type) !== -1) { return "/admin/"; } | ||
| if (ADMIN_MODELS.indexOf(type.replace('_', '-')) !== -1) { return "/admin/"; } |
There was a problem hiding this comment.
String replace only substitutes first underscore occurrence
Low Severity
type.replace('_', '-') only replaces the first underscore in the type string, since JavaScript's String.replace with a string argument is not global. Any future admin model type with multiple underscores would fail to match in ADMIN_MODELS. The codebase uses replace(/_/g, '-') elsewhere (e.g., in resolver.js.es6) for this reason.
2 participants
Test 10
Summary by CodeRabbit
Release Notes
New Features
Refactoring
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/discourse-coderabbit#10
Note
Medium Risk
Changes embedding access control and embedded-topic category assignment, and migrates configuration from site settings to a new database table, which could affect who can embed and where topics are created if misconfigured or migration data is unexpected.
Overview
Adds an Admin → Customize → Embedding screen to create/edit/delete allowed embeddable hosts and assign each host a target category.
Migrates embedding configuration from
SiteSetting.embeddable_hosts/embed_categoryto a newEmbeddableHostmodel + CRUD API/serializers, updates embed permission checks (EmbedController/TopicRetriever) to consult the DB, and changesTopicEmbed.importto pick the category from the matching host record.Extends the JS REST store hydration to support embedded
*_idsarrays (with updated tests/fixtures) and adjusts related specs to fabricateEmbeddableHostrecords instead of using removed site settings.Written by Cursor Bugbot for commit d1c6918. Configure here.