Skip to content

Commit

Permalink
Fix XSS vulnerability in nested forms
Browse files Browse the repository at this point in the history
  • Loading branch information
mshibuya committed Mar 14, 2020
1 parent 4ef2ea4 commit d72090e
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 5 deletions.
6 changes: 4 additions & 2 deletions app/assets/javascripts/rails_admin/ra.nested-form-hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@
$(document).on('nested:fieldAdded', 'form', function(content) {
var controls, field, nav, new_tab, one_to_one, parent_group, toggler;
field = content.field.addClass('tab-pane').attr('id', 'unique-id-' + (new Date().getTime()));
new_tab = $('<li><a data-toggle="tab" href="#' + field.attr('id') + '">' + field.children('.object-infos').data('object-label') + '</a></li>');
new_tab = $('<li></li>').append(
$('<a></a>').attr('data-toggle', 'tab').attr('href', '#' + field.attr('id')).text(field.children('.object-infos').data('object-label'))
)
parent_group = field.closest('.control-group');
controls = parent_group.children('.controls');
one_to_one = controls.data('nestedone') !== void 0;
Expand All @@ -27,7 +29,7 @@
content.select(':hidden').show('slow');
toggler.addClass('active').removeClass('disabled').children('i').addClass('icon-chevron-down').removeClass('icon-chevron-right');
if (one_to_one) {
controls.find('.add_nested_fields').removeClass('add_nested_fields').html(field.children('.object-infos').data('object-label'));
controls.find('.add_nested_fields').removeClass('add_nested_fields').text(field.children('.object-infos').data('object-label'));
}
});

Expand Down
14 changes: 11 additions & 3 deletions app/assets/javascripts/rails_admin/ra.widgets.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,11 @@
toggler = field.find('> .controls > .btn-group > .toggler');
tab_content.children('.fields:not(.tab-pane)').addClass('tab-pane').each(function() {
$(this).attr('id', 'unique-id-' + (new Date().getTime()) + Math.floor(Math.random() * 100000));
nav.append('<li><a data-toggle="tab" href="#' + this.id + '">' + $(this).children('.object-infos').data('object-label') + '</a></li>');
nav.append(
$('<li></li>').append(
$('<a></a>').attr('data-toggle', 'tab').attr('href', '#' + this.id).text($(this).children('.object-infos').data('object-label'))
)
);
});
if (nav.find("> li.active").length === 0) {
nav.find("> li > a[data-toggle='tab']:first").tab('show');
Expand Down Expand Up @@ -165,8 +169,12 @@
tab_content = field.find("> .tab-content");
toggler = field.find('> .controls > .btn-group > .toggler');
tab_content.children(".fields:not(.tab-pane)").addClass('tab-pane active').each(function() {
field.find('> .controls .add_nested_fields').removeClass('add_nested_fields').html($(this).children('.object-infos').data('object-label'));
nav.append('<li><a data-toggle="tab" href="#' + this.id + '">' + $(this).children('.object-infos').data('object-label') + '</a></li>');
field.find('> .controls .add_nested_fields').removeClass('add_nested_fields').text($(this).children('.object-infos').data('object-label'));
nav.append(
$('<li></li>').append(
$('<a></a>').attr('data-toggle', 'tab').attr('href', '#' + this.id).text($(this).children('.object-infos').data('object-label'))
)
);
});
first_tab = nav.find("> li > a[data-toggle='tab']:first");
first_tab.tab('show');
Expand Down
16 changes: 16 additions & 0 deletions spec/integration/fields/has_many_association_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,22 @@
expect(page.body).to include('field_test_nested_field_tests_attributes_new_nested_field_tests_deeply_nested_field_tests_attributes_new_deeply_nested_field_tests_title')
end
end

context 'when XSS attack is attempted', js: true do
it 'does not break on adding a new item' do
allow(I18n).to receive(:t).and_call_original
expect(I18n).to receive(:t).with('admin.form.new_model', name: 'Nested field test').and_return('<script>throw "XSS";</script>')
@record = FactoryBot.create :field_test
visit edit_path(model_name: 'field_test', id: @record.id)
find('#field_test_nested_field_tests_attributes_field .add_nested_fields').click
end

it 'does not break on editing an existing item' do
@record = FactoryBot.create :field_test
NestedFieldTest.create! title: '<script>throw "XSS";</script>', field_test: @record
visit edit_path(model_name: 'field_test', id: @record.id)
end
end
end

context 'with not nullable foreign key', active_record: true do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,25 @@
@record.reload
expect(@record.comment).to be_nil
end

context 'when XSS attack is attempted', js: true do
it 'does not break on adding a new item' do
allow(I18n).to receive(:t).and_call_original
expect(I18n).to receive(:t).with('admin.form.new_model', name: 'Comment').and_return('<script>throw "XSS";</script>')
@record = FactoryBot.create :field_test
visit edit_path(model_name: 'field_test', id: @record.id)
find('#field_test_comment_attributes_field .add_nested_fields').click
end

it 'does not break on adding an existing item' do
RailsAdmin.config Comment do
object_label_method :content
end
@record = FactoryBot.create :field_test
FactoryBot.create :comment, content: '<script>throw "XSS";</script>', commentable: @record
visit edit_path(model_name: 'field_test', id: @record.id)
end
end
end

context 'with custom primary_key option' do
Expand Down

2 comments on commit d72090e

@coorasse
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @mshibuya ,
was this issue reported? I don't see any automatic security tool raising a flag (depfu or github)

@jlm
Copy link

@jlm jlm commented on d72090e Jan 18, 2021

Choose a reason for hiding this comment

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

Github's dependabot raised this issue on my code on Jan 14th, 2021.

Please sign in to comment.