Defect #19304

<a> tag without attributes in description results in undefined method + for nil:NilClass

Added by Pavel Rosický over 3 years ago. Updated over 2 years ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Jean-Philippe Lang% Done:

0%

Category:Code cleanup/refactoring
Target version:3.2.0
Resolution:Cant reproduce Affected version:3.0.0

Description

Observed Behavior
Create new issue with description "<a>link.com</a>" (without tag attributes)

ActionView::Template::Error: undefined method `+' for nil:NilClass
[RAILS_ROOT]/app/helpers/application_helper.rb:872:in `block in parse_redmine_links'
[RAILS_ROOT]/app/helpers/application_helper.rb:741:in `gsub!'
[RAILS_ROOT]/app/helpers/application_helper.rb:741:in `parse_redmine_links'
[RAILS_ROOT]/app/helpers/application_helper.rb:583:in `block (2 levels) in textilizable'

Workaround
http://www.redmine.org/projects/redmine/repository/revisions/14003/entry/trunk/app/helpers/application_helper.rb#L741
<a( [^>]+?)?>(.*?)</a> should be <a( [^>]+?)>(.*?)</a>

Environment:
Redmine version 3.0.0.stable
Ruby version 2.1.5-p273 (2014-11-13) [i386-mingw32]
Rails version 4.2.0
Environment production
Database adapter Mysql2

application_helper.rb.patch Magnifier (1.22 KB) Pavel Rosický, 2015-03-08 16:20

application_helper_test.rb.patch Magnifier - test case (1.11 KB) Pavel Rosický, 2015-03-11 00:26

Associated revisions

Revision 14774
Added by Jean-Philippe Lang over 2 years ago

Fixed that #parse_redmine_links errors when given a link tag without attributes (#19304).

History

#2 Updated by Jean-Philippe Lang over 3 years ago

  • Status changed from New to Needs feedback
  • Resolution set to Cant reproduce

I'm not able to reproduce, could you write a test case for this?

#3 Updated by Pavel Rosický over 3 years ago

I forgot, that standard textile formatters unescape html tags before, but this method should handle this anyway. Test case attached.

Original result: error
With patch: returns nil as it should

#4 Updated by Toshi MARUYAMA over 3 years ago

  • Status changed from Needs feedback to New
  • Target version set to 3.0.2
  • Resolution deleted (Cant reproduce)

#5 Updated by Toshi MARUYAMA over 3 years ago

  • Related to Defect #18301: Revision shortlink at end of URL breaks URL autolinking added

#6 Updated by Toshi MARUYAMA over 3 years ago

  • Related to deleted (Defect #18301: Revision shortlink at end of URL breaks URL autolinking)

#7 Updated by Toshi MARUYAMA over 3 years ago

  • Status changed from New to Needs feedback
  • Target version deleted (3.0.2)
  • Resolution set to Cant reproduce

I cannot reproduce too.

diff --git a/test/unit/helpers/application_helper_test.rb b/test/unit/helpers/application_helper_test.rb
--- a/test/unit/helpers/application_helper_test.rb
+++ b/test/unit/helpers/application_helper_test.rb
@@ -1261,6 +1261,19 @@ RAW
     end
   end

+  def test_should_handle_a_tag_without_attributes
+    text = '<a>http://example.com</a>'
+    with_settings :text_formatting => 'textile' do
+      assert_not_nil textilizable(text)
+    end
+    with_settings :text_formatting => 'markdown' do
+      assert_not_nil textilizable(text)
+    end
+    with_settings :text_formatting => 'unknown' do
+      assert_not_nil textilizable(text)
+    end
+  end
+
   def test_due_date_distance_in_words
     to_test = { Date.today => 'Due in 0 days',
                 Date.today + 1 => 'Due in 1 day',

#8 Updated by Pavel Rosický almost 3 years ago

Hi, this bug is still present, but you can reproduce it only if you use non-redmine custom text formatter which supports html (like ckeditor, markdown/textile formatter escape brackets, so <a> will be &\lt;a&\gt; and the affected regex will not fail).
To avoid that I need to patch this method and gsub! these occurances, which causes unnecessary overhead. Or I can patch the whole method (~140 lines!!! definetely worth to refactor) only because of one character.

Can someone take a look at it again? Patch, test-case and backtrace are already attached.

#9 Updated by Toshi MARUYAMA almost 3 years ago

Pavel Rosický wrote:

Hi, this bug is still present, but you can reproduce it only if you use non-redmine custom text formatter which supports html (like ckeditor, markdown/textile formatter escape brackets, so <a> will be <a> and the affected regex will not fail).

Could you add test cases for it?

#10 Updated by Pavel Rosický almost 3 years ago

Toshi MARUYAMA wrote:

Could you add test cases for it?

I already did, 7 months ago.

Test case - http://www.redmine.org/attachments/13295/application_helper_test.rb.patch :

def test_should_handle_a_tag_without_attributes
  text = '<a>http://example.com</a>'
  assert_nil parse_redmine_links(text, nil, nil, nil, true, {})
end

Without patch:
1) Error:
ApplicationHelperTest#test_should_handle_a_tag_without_attributes:
NoMethodError: undefined method `+' for nil:NilClass
app/helpers/application_helper.rb:873:in `block in parse_redmine_links'
app/helpers/application_helper.rb:742:in `gsub!'
app/helpers/application_helper.rb:742:in `parse_redmine_links'
test/unit/helpers/application_helper_test.rb:1262:in `test_should_handle_a_t
ag_without_attributes'
94 runs, 379 assertions, 0 failures, 1 errors, 0 skips

With patch - http://www.redmine.org/attachments/13280/application_helper.rb.patch :
94 runs, 380 assertions, 0 failures, 0 errors, 0 skips

What else can I do?
If you want to test textilizable(), I'll have to include ckeditor and a custom html formatter (plugin). Such tests can't be included in a standard Redmine's test suite.

#11 Updated by Toshi MARUYAMA over 2 years ago

Pavel Rosický wrote:

If you want to test textilizable(), I'll have to include ckeditor and a custom html formatter (plugin).

Could you add dummy formatter?

#12 Updated by Jean-Philippe Lang over 2 years ago

  • Category changed from Text formatting to Code cleanup/refactoring
  • Status changed from Needs feedback to Closed
  • Assignee set to Jean-Philippe Lang
  • Target version set to 3.2.0

Fixed in r14774.

Also available in: Atom PDF