-
Notifications
You must be signed in to change notification settings - Fork 0
FEATURE: Can edit category/host relationships for embedding #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: rest-serializer-enhancement-pre
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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"; | ||
| } | ||
| }); |
| 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')); | ||
| }); | ||
| } | ||
| }); | ||
| }, | ||
|
|
||
| cancel() { | ||
| const host = this.get('host'); | ||
| if (host.get('isNew')) { | ||
| this.sendAction('deleteHost', host); | ||
| } else { | ||
| this.rollbackBuffer(); | ||
| this.set('editToggled', false); | ||
| } | ||
| } | ||
| } | ||
| }); |
| 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({}); | ||
| }, | ||
|
|
||
| addHost() { | ||
| const host = this.store.createRecord('embeddable-host'); | ||
| this.get('embedding.embeddable_hosts').pushObject(host); | ||
| }, | ||
|
|
||
| deleteHost(host) { | ||
| this.get('embedding.embeddable_hosts').removeObject(host); | ||
| } | ||
| } | ||
| }); |
| 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); | ||
| } | ||
| }); |
| 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}} |
| 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%'> </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"}} | ||
|
|
| 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 | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: When destroying a host, if the given ID does not exist, Severity Level: Major
|
||||||
| host.destroy | |
| raise Discourse::NotFound unless host |
Steps of Reproduction ✅
1. Log in as a staff user and open the admin embedding UI at `GET
/admin/customize/embedding` (routed via `config/routes.rb:138-139` to
`Admin::EmbeddingController#show`, which lists `EmbeddableHost` records).
2. In one session or Rails console, delete a specific `EmbeddableHost` record directly so
that its ID becomes stale for any open admin page.
3. In another browser tab where the old list is still loaded, attempt to delete that stale
host by issuing `DELETE /admin/embeddable_hosts/:id.json` with the stale ID (resource
route defined at `config/routes.rb:42-43,153` to
`Admin::EmbeddableHostsController#destroy`).
4. The destroy action at `app/controllers/admin/embeddable_hosts_controller.rb:14-18` runs
`host = EmbeddableHost.where(id: params[:id]).first`, which returns `nil` for the
non-existent ID, then immediately calls `host.destroy`, raising `NoMethodError` on `nil`
and returning a 500 error instead of the `Discourse::NotFound`-driven 404 handled in
`app/controllers/application_controller.rb:108-110`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** app/controllers/admin/embeddable_hosts_controller.rb
**Line:** 16:16
**Comment:**
*Null Pointer: When destroying a host, if the given ID does not exist, `EmbeddableHost.where(...).first` returns `nil` and `host.destroy` will raise an exception, producing a 500 error instead of a clean 404-style response.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| 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 | ||
|
|
||
| protected | ||
|
|
||
| def fetch_embedding | ||
| @embedding = OpenStruct.new({ | ||
| id: 'default', | ||
| embeddable_hosts: EmbeddableHost.all.order(:host) | ||
| }) | ||
| end | ||
| end |
| 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 | ||||||||||||||||||||||||||||||
| belongs_to :category | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| before_validation do | ||||||||||||||||||||||||||||||
| self.host.sub!(/^https?:\/\//, '') | ||||||||||||||||||||||||||||||
| self.host.sub!(/\/.*$/, '') | ||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def self.record_for_host(host) | ||||||||||||||||||||||||||||||
| uri = URI(host) rescue nil | ||||||||||||||||||||||||||||||
| return false unless uri.present? | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| host = uri.host | ||||||||||||||||||||||||||||||
| return false unless host.present? | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
Comment on lines
+12
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Severity Level: Critical 🚨- ❌ Embeds from hosts on nonstandard ports always rejected.
- ⚠️ TopicRetriever invalid_host? misclassifies allowed port-specific hosts.
- ⚠️ New embedded topics miscategorized when using port-specific hosts.
Suggested change
Steps of Reproduction ✅1. Using `Admin::EmbeddableHostsController#create` in
`app/controllers/admin/embeddable_hosts_controller.rb:5-7`, create an `EmbeddableHost`
with `host` set to `"example.com:3000"`; `save_host` at lines 22-27 assigns this string
directly and the `before_validation` callback in `app/models/embeddable_host.rb:5-8`
leaves the `:3000` port intact.
2. Host a page at `http://example.com:3000/article` that uses Discourse embeds so that the
browser loads `EmbedController#comments` in `app/controllers/embed_controller.rb:7-35`
with `request.referer` equal to `"http://example.com:3000/article"`.
3. Before the action runs, the `ensure_embeddable` filter in
`app/controllers/embed_controller.rb:56-62` executes and calls
`EmbeddableHost.host_allowed?(request.referer)` (line 61), which delegates to
`EmbeddableHost.record_for_host` in `app/models/embeddable_host.rb:10-21`.
4. `record_for_host` parses the referer, sets `host = uri.host` (line 14, e.g.
`"example.com"` without `:3000`), and queries `where("lower(host) = ?", host).first` (line
17), which does not match the stored `"example.com:3000"` record; `host_allowed?` returns
`false` and `ensure_embeddable` raises `Discourse::InvalidAccess.new('invalid referer
host')`, so all embeds from `example.com:3000` are rejected.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** app/models/embeddable_host.rb
**Line:** 12:16
**Comment:**
*Logic Error: `record_for_host` currently uses only `uri.host` when looking up the embeddable host, ignoring any non-standard port; this means a host saved as `example.com:3000` will never match a referer like `http://example.com:3000/...`, causing `host_allowed?` to return false and valid embeds on custom ports to be rejected. Include the port in the lookup key when present so records with ports can be found correctly.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||||||||||||||||||||||||||
| where("lower(host) = ?", host).first | ||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def self.host_allowed?(host) | ||||||||||||||||||||||||||||||
| record_for_host(host).present? | ||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||
| 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 | ||
|
|
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: When updating a host, if the requested record ID does not exist,
EmbeddableHost.where(...).firstwill returnnilandsave_host(host)will attempt to call methods onnil, causing a 500 error instead of a proper not-found response. [null pointer]Severity Level: Major⚠️
Steps of Reproduction ✅
Prompt for AI Agent 🤖