Sync ERB handling up with rails/rails from Rails 5 #878

Merged
merged 1 commit into from May 13, 2016

Projects

None yet

3 participants

@ptoomey3
Contributor
ptoomey3 commented May 11, 2016 edited

Per your comment in #873 (comment), this PR syncs up ERB handling with Rails 5 from upstream. The one caveat here is that upstream uses @output_buffer.safe_expr_append during code generation. Given this is all being analyzed statically, I'm not sure that matters or not if we are analyzing a rails 3 app. Since it is all just an intermediate format used for analysis anyway, I tend to think it doesn't matter. But, I'll leave it up to you if you would like to override that change from upstream or not.

@ptoomey3 ptoomey3 commented on the diff May 11, 2016
lib/brakeman/parsers/rails3_erubis.rb
if code =~ BLOCK_EXPR
- src << "@output_buffer.safe_append= " << code
+ src << "@output_buffer.safe_expr_append= " << code
@ptoomey3
ptoomey3 May 11, 2016 Contributor

Here is the change from safe_append= to safe_expr_append=. I tend to think it is easiest (and safe) to just match upstream.

@ptoomey3 ptoomey3 commented on the diff May 11, 2016
lib/brakeman/processors/erubis_template_processor.rb
@@ -2,7 +2,7 @@
#Processes ERB templates using Erubis instead of erb.
class Brakeman::ErubisTemplateProcessor < Brakeman::TemplateProcessor
-
+
@ptoomey3
ptoomey3 May 11, 2016 Contributor

Unless you are opposed, I'm going to stop undoing the whitespace fixes that happen when I click save in my editor. Maybe over time they will all be fixed 😛. But seriously, if you would prefer me to rip out whitespace changes to preserve the blame log just let me know.

@presidentbeef
presidentbeef May 11, 2016 Owner

No worries. I tend to fix them as I go, too.

@ptoomey3 ptoomey3 and 1 other commented on an outdated diff May 11, 2016
@@ -3,3 +3,4 @@ coverage/
test/coverage/
.bundle
bundle
+.tags
@ptoomey3
ptoomey3 May 11, 2016 Contributor

I kept running into this and finally forgot to remove my tags file before pushing. Let me know if you prefer this in a unique PR.

@presidentbeef
presidentbeef May 11, 2016 Owner

This seems pretty specific to your setup...

@ptoomey3
ptoomey3 May 12, 2016 Contributor

I can rip it out...It is generated by whatever some atom plugin is kicking of ctags with. I just added it to my global .gitignore.

@presidentbeef
presidentbeef May 12, 2016 Owner

Yes, please.

@ptoomey3
ptoomey3 May 12, 2016 Contributor

Ok...removed and squashed commit history.

@ptoomey3 ptoomey3 Sync ERB handling up with rails/rails from Rails 5 8fbe90f
@presidentbeef presidentbeef commented on the diff May 12, 2016
lib/brakeman/parsers/rails3_erubis.rb
class Brakeman::Rails3Erubis < ::Erubis::Eruby
def add_preamble(src)
- # src << "_buf = ActionView::SafeBuffer.new;\n"
+ @newline_pending = 0
+ src << "@output_buffer = output_buffer || ActionView::OutputBuffer.new;"
@presidentbeef
presidentbeef May 12, 2016 Owner

Just as a note, this line and the postamble will be ignored anyway. Otherwise this causes a lot of overhead processing templates.

@ptoomey3
ptoomey3 May 12, 2016 edited Contributor

Are you cool with leaving the code as is to match upstream rails?

@presidentbeef
presidentbeef May 12, 2016 Owner

Yeah, it's okay. More of a note for the future.

@presidentbeef presidentbeef merged commit bdb7687 into presidentbeef:master May 13, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gregose
Contributor
gregose commented May 31, 2016

FWIW, this also fixes an issue with Rails3Erubis parsing of case statements:

Before the fix:

=> [{:error=>"/test_app/app/views/log/_action.html.erb:2 :: parse error on value \"@output_buffer\" (tIVAR)",
  :location=>"Could not parse /test_app/app/views/log/_action.html.erb"}]

This is due to prepended @output_buffer in the generated ruby source:

case view.action
@output_buffer << (''.html_safe!); when "foo" ;@output_buffer.append= ( "bar" );
@output_buffer << (''.html_safe!); when "foo2" ;@output_buffer.append= ( "bar2" );
end
@output_buffer << (''.html_safe!);

After the fix to move to the upstream ERB parsing:

@output_buffer = output_buffer || ActionView::OutputBuffer.new; case view.action
 when "foo" ;@output_buffer.append=( "bar" );@output_buffer.safe_append='
'.freeze; when "foo2" ;@output_buffer.append=( "bar2" );@output_buffer.safe_append='
'.freeze; end
@output_buffer.to_s
@presidentbeef presidentbeef locked and limited conversation to collaborators Oct 9, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.