Repository: yippee-fun/phlex (ruby)

Issue:
Security risk with `process_attributes` override
@joelmoss I’ve just realised a possible problem with encouraging people to override `process_attributes`. If you don’t call `super` but return a String instead, you lose a bunch of security features.

I wonder if we should go back to the idea of having `process_attributes` called from `_attributes` so the attributes are always filtered. What do you think?


Discussion:
Oh the joys of forgetting 'super' 😩

I would always prefer not having to worry about super, but you you've been quite persistent on maintaining performance.

I suppose we have to pick our battles, and consider if it's a minor perf hit. 
Yeah, I think we should take the performance hit unfortunately. We can optimise it away in the compiler eventually.
It should be a very very minor hit. 

I'll get you a new PR
You wanna keep the kwargs change in `_attributes`?
Yeah, I think that's a good change.

Patch:
diff --git a/docs/pages/templates.rb b/docs/pages/templates.rb
--- a/docs/pages/templates.rb
+++ b/docs/pages/templates.rb
@@ -76,6 +76,32 @@ def template
 					e.execute "ChannelControls.new.call"
 				end
 
+				render Markdown.new(<<~MD)
+					## Processing Attributes
+
+					Sometimes you may want to process, modify, or simply be able to read the attributes that tags are given. For example, you may want to assign an `id` attribute to every tag, or even use the `tokens` helper to automatically tokenize the `class` attribute.
+
+					Simply define a `process_attributes` method in your view. This method will be called on each tag, with the attributes as keyword arguments, and should return a Hash of the attributes.
+				MD
+
+				render Example.new do |e|
+					e.tab "example.rb", <<~RUBY
+						class Example < Phlex::View
+							def template
+								h1(class: 'title') { "Hello" }
+							end
+
+							def process_attributes(**attributes)
+								attributes.tap do |attrs|
+									attrs[:id] = SecureRandom.uuid
+								end
+							end
+						end
+					RUBY
+
+					e.execute "Example.new.call"
+				end
+
 				render Markdown.new(<<~MD)
 					## The template tag
 
diff --git a/docs/pages/views.rb b/docs/pages/views.rb
--- a/docs/pages/views.rb
+++ b/docs/pages/views.rb
@@ -154,7 +154,7 @@ def template
 				render Markdown.new(<<~MD)
 					## Callbacks
 
-					Prepend the `Phlex::View::Callbacks` module, and if you define `#before_rendering_template` and/or `#after_rendering_template` method in your view, they will be called immediately before and after your template is compiled.
+					Prepend the `Phlex::View::Callbacks` module, and if you define `#before_rendering_template` and/or `#after_rendering_template` method in your view, they will be called immediately before and after your template is rendered.
 				MD
 
 				render Example.new do |e|
diff --git a/lib/phlex/compiler/generators/element.rb b/lib/phlex/compiler/generators/element.rb
--- a/lib/phlex/compiler/generators/element.rb
+++ b/lib/phlex/compiler/generators/element.rb
@@ -18,7 +18,7 @@ def call
 
 					if @node.arguments&.parts&.any?
 						@formatter.chain_append do |f|
-							f.text "process_attributes("
+							f.text "_attributes("
 							@node.arguments.format(@formatter)
 							f.text ")"
 						end
diff --git a/lib/phlex/html.rb b/lib/phlex/html.rb
--- a/lib/phlex/html.rb
+++ b/lib/phlex/html.rb
@@ -137,11 +137,11 @@ def #{element}(content = nil, **attributes, &block)
 
           if attributes.length > 0
             if block_given?
-              @_target << "<#{tag}" << (Phlex::ATTRIBUTE_CACHE[attributes.hash] || process_attributes(**attributes)) << ">"
+              @_target << "<#{tag}" << (Phlex::ATTRIBUTE_CACHE[attributes.hash] || _attributes(**attributes)) << ">"
               yield_content(&block)
               @_target << "</#{tag}>"
             else
-              @_target << "<#{tag}" << (Phlex::ATTRIBUTE_CACHE[attributes.hash] || process_attributes(**attributes)) << "></#{tag}>"
+              @_target << "<#{tag}" << (Phlex::ATTRIBUTE_CACHE[attributes.hash] || _attributes(**attributes)) << "></#{tag}>"
             end
           else
             if block_given?
@@ -164,7 +164,7 @@ def register_void_element(element, tag: element.name.tr("_", "-"))
 
         def #{element}(**attributes)
           if attributes.length > 0
-            @_target << "<#{tag}" << (Phlex::ATTRIBUTE_CACHE[attributes.hash] || process_attributes(**attributes)) << ">"
+            @_target << "<#{tag}" << (Phlex::ATTRIBUTE_CACHE[attributes.hash] || _attributes(**attributes)) << ">"
           else
             @_target << "<#{tag}>"
           end
diff --git a/lib/phlex/view.rb b/lib/phlex/view.rb
--- a/lib/phlex/view.rb
+++ b/lib/phlex/view.rb
@@ -182,11 +182,19 @@ def helpers
 			@_view_context
 		end
 
-		def process_attributes(buffer = +"", **attributes)
+		def _attributes(buffer = +"", **attributes)
 			if attributes[:href]&.start_with?(/\s*javascript/)
 				attributes[:href] = attributes[:href].sub(/^\s*(javascript:)+/, "")
 			end
 
+			if respond_to?(:process_attributes)
+				attributes = process_attributes(**attributes)
+
+				unless attributes.is_a?(Hash)
+					raise "`#{self.class.name}#process_attributes` must return a hash of attributes"
+				end
+			end
+
 			_build_attributes(attributes, buffer: buffer)
 
 			unless self.class.rendered_at_least_once


Test patch:
diff --git a/test/phlex/compilation/standard_element.rb b/test/phlex/compilation/standard_element.rb
--- a/test/phlex/compilation/standard_element.rb
+++ b/test/phlex/compilation/standard_element.rb
@@ -26,7 +26,7 @@ def template
 
 		expected = <<~RUBY
 			def template
-				@_target << "<h1" << process_attributes(class: "font-bold") << "></h1>"
+				@_target << "<h1" << _attributes(class: "font-bold") << "></h1>"
 			end
 		RUBY
 
@@ -53,7 +53,7 @@ def template
 
 		expect(output.first).to be == <<~RUBY
 			def template
-				@_target << "<h1" << process_attributes(class: "font-bold") << ">Hi</h1>"
+				@_target << "<h1" << _attributes(class: "font-bold") << ">Hi</h1>"
 			end
 		RUBY
 	end
@@ -78,7 +78,7 @@ def template
 
 		expected = <<~RUBY
 			def template
-				@_target << "<h1" << process_attributes(class: "font-bold") << ">Hi</h1>"
+				@_target << "<h1" << _attributes(class: "font-bold") << ">Hi</h1>"
 			end
 		RUBY
 
diff --git a/test/phlex/compilation/void_element.rb b/test/phlex/compilation/void_element.rb
--- a/test/phlex/compilation/void_element.rb
+++ b/test/phlex/compilation/void_element.rb
@@ -26,7 +26,7 @@ def template
 
 		expected = <<~RUBY
 			def template
-				@_target << "<img" << process_attributes(class: "a b c") << ">"
+				@_target << "<img" << _attributes(class: "a b c") << ">"
 			end
 		RUBY
 
diff --git a/test/phlex/view/attributes.rb b/test/phlex/view/attributes.rb
--- a/test/phlex/view/attributes.rb
+++ b/test/phlex/view/attributes.rb
@@ -17,6 +17,45 @@ def template
 		end
 	end
 
+	with "process_attributes" do
+		view do
+			def template
+				div class: "header"
+			end
+
+			def process_attributes(**attributes)
+				attributes.tap do |attrs|
+					attrs[:class] = "#{attrs[:class]}123abc" if attributes.key?(:class)
+				end
+			end
+		end
+
+		it "calls the attribute method" do
+			expect(output).to be == %(<div class="header123abc"></div>)
+		end
+	end
+
+	with "process_attributes returning no hash" do
+		view do
+			def template
+				div class: "header"
+			end
+
+			def process_attributes(**attributes)
+				nil
+			end
+		end
+
+		it "raises when return value is not a Hash" do
+			expect do
+				output
+			end.to raise_exception(
+				RuntimeError,
+				message: be =~ /process_attributes` must return a hash of attributes/
+			)
+		end
+	end
+
 	if RUBY_ENGINE == "ruby"
 		with "unique tag attributes" do
 			view do