From a8dc9fd27b845193fd209a249e084f993a10c19d Mon Sep 17 00:00:00 2001 From: Jeffrey Hardy Date: Wed, 14 Oct 2009 00:26:53 -0400 Subject: CookieJar#delete should return the key's value, consistent with a Hash Signed-off-by: Jeremy Kemper --- actionpack/lib/action_controller/metal/cookies.rb | 3 ++- actionpack/test/controller/cookie_test.rb | 7 +++++++ actionpack/test/dispatch/session/test_session_test.rb | 4 ++-- 3 files changed, 11 insertions(+), 3 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_controller/metal/cookies.rb b/actionpack/lib/action_controller/metal/cookies.rb index d4806623c3..c328db8beb 100644 --- a/actionpack/lib/action_controller/metal/cookies.rb +++ b/actionpack/lib/action_controller/metal/cookies.rb @@ -87,8 +87,9 @@ module ActionController #:nodoc: def delete(key, options = {}) options.symbolize_keys! options[:path] = "/" unless options.has_key?(:path) - super(key.to_s) + value = super(key.to_s) @controller.response.delete_cookie(key, options) + value end end end diff --git a/actionpack/test/controller/cookie_test.rb b/actionpack/test/controller/cookie_test.rb index 7199da3441..b429cbf0e6 100644 --- a/actionpack/test/controller/cookie_test.rb +++ b/actionpack/test/controller/cookie_test.rb @@ -118,6 +118,13 @@ class CookieTest < ActionController::TestCase assert_equal %w{1 2 3}, jar["pages"] end + def test_cookiejar_delete_removes_item_and_returns_its_value + @request.cookies["user_name"] = "david" + @controller.response = @response + jar = ActionController::CookieJar.new(@controller) + assert_equal "david", jar.delete("user_name") + end + def test_delete_cookie_with_path get :delete_cookie_with_path assert_cookie_header "user_name=; path=/beaten; expires=Thu, 01-Jan-1970 00:00:00 GMT" diff --git a/actionpack/test/dispatch/session/test_session_test.rb b/actionpack/test/dispatch/session/test_session_test.rb index 0ff93f1c5d..c8dc4ab461 100644 --- a/actionpack/test/dispatch/session/test_session_test.rb +++ b/actionpack/test/dispatch/session/test_session_test.rb @@ -26,11 +26,11 @@ class ActionController::TestSessionTest < ActiveSupport::TestCase assert_equal('value', session[:key]) end - def test_calling_delete_removes_item + def test_calling_delete_removes_item_and_returns_its_value session = ActionController::TestSession.new session[:key] = 'value' assert_equal('value', session[:key]) - session.delete(:key) + assert_equal('value', session.delete(:key)) assert_nil(session[:key]) end -- cgit v1.2.3 From a41c6c35cadf75bfd4bf0a17113ae37d628896e8 Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Wed, 14 Oct 2009 11:59:00 -0700 Subject: Start adding configuration to ActionView instead of using constants. By using config rather than hardcoded constants, we can evolve the configuration system over time (we'd just need to update the config method with more robust capabilities and all consumers would get the capabilities with no code changes) --- actionpack/lib/action_view/base.rb | 2 +- .../lib/action_view/helpers/asset_tag_helper.rb | 28 ++++++++++++---------- actionpack/test/template/asset_tag_helper_test.rb | 10 ++++++++ actionpack/test/template/form_tag_helper_test.rb | 3 +++ actionpack/test/template/url_helper_test.rb | 3 +++ 5 files changed, 33 insertions(+), 13 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_view/base.rb b/actionpack/lib/action_view/base.rb index 31e9c5ef9d..5f28ba6ccb 100644 --- a/actionpack/lib/action_view/base.rb +++ b/actionpack/lib/action_view/base.rb @@ -167,7 +167,7 @@ module ActionView #:nodoc: module Subclasses end - include Helpers, Rendering, Partials, ::ERB::Util + include Helpers, Rendering, Partials, ::ERB::Util, ActiveSupport::Configurable extend ActiveSupport::Memoizable diff --git a/actionpack/lib/action_view/helpers/asset_tag_helper.rb b/actionpack/lib/action_view/helpers/asset_tag_helper.rb index faa7f2e2e9..15b70ecff5 100644 --- a/actionpack/lib/action_view/helpers/asset_tag_helper.rb +++ b/actionpack/lib/action_view/helpers/asset_tag_helper.rb @@ -133,9 +133,13 @@ module ActionView # change. You can use something like Live HTTP Headers for Firefox to verify # that the cache is indeed working. module AssetTagHelper - ASSETS_DIR = defined?(Rails.public_path) ? Rails.public_path : "public" - JAVASCRIPTS_DIR = "#{ASSETS_DIR}/javascripts" - STYLESHEETS_DIR = "#{ASSETS_DIR}/stylesheets" + assets_dir = defined?(Rails.public_path) ? Rails.public_path : "public" + ActionView::DEFAULT_CONFIG = { + :assets_dir => assets_dir, + :javascripts_dir => "#{assets_dir}/javascripts", + :stylesheets_dir => "#{assets_dir}/stylesheets", + } + JAVASCRIPT_DEFAULT_SOURCES = ['prototype', 'effects', 'dragdrop', 'controls'].freeze unless const_defined?(:JAVASCRIPT_DEFAULT_SOURCES) # Returns a link tag that browsers and news readers can use to auto-detect @@ -280,7 +284,7 @@ module ActionView if concat || (ActionController::Base.perform_caching && cache) joined_javascript_name = (cache == true ? "all" : cache) + ".js" - joined_javascript_path = File.join(joined_javascript_name[/^#{File::SEPARATOR}/] ? ASSETS_DIR : JAVASCRIPTS_DIR, joined_javascript_name) + joined_javascript_path = File.join(joined_javascript_name[/^#{File::SEPARATOR}/] ? config.assets_dir : config.javascripts_dir, joined_javascript_name) unless ActionController::Base.perform_caching && File.exists?(joined_javascript_path) write_asset_file_contents(joined_javascript_path, compute_javascript_paths(sources, recursive)) @@ -431,7 +435,7 @@ module ActionView if concat || (ActionController::Base.perform_caching && cache) joined_stylesheet_name = (cache == true ? "all" : cache) + ".css" - joined_stylesheet_path = File.join(joined_stylesheet_name[/^#{File::SEPARATOR}/] ? ASSETS_DIR : STYLESHEETS_DIR, joined_stylesheet_name) + joined_stylesheet_path = File.join(joined_stylesheet_name[/^#{File::SEPARATOR}/] ? config.assets_dir : config.stylesheets_dir, joined_stylesheet_name) unless ActionController::Base.perform_caching && File.exists?(joined_stylesheet_path) write_asset_file_contents(joined_stylesheet_path, compute_stylesheet_paths(sources, recursive)) @@ -630,11 +634,11 @@ module ActionView # Prefix with /dir/ if lacking a leading +/+. Account for relative URL # roots. Rewrite the asset path for cache-busting asset ids. Include # asset host, if configured, with the correct request protocol. - def compute_public_path(source, dir, ext = nil, include_host = true) + def compute_public_path(source, dir, ext = nil, include_host = true) has_request = @controller.respond_to?(:request) source_ext = File.extname(source)[1..-1] - if ext && !is_uri?(source) && (source_ext.blank? || (ext != source_ext && File.exist?(File.join(ASSETS_DIR, dir, "#{source}.#{ext}")))) + if ext && !is_uri?(source) && (source_ext.blank? || (ext != source_ext && File.exist?(File.join(config.assets_dir, dir, "#{source}.#{ext}")))) source += ".#{ext}" end @@ -700,7 +704,7 @@ module ActionView if @@cache_asset_timestamps && (asset_id = @@asset_timestamps_cache[source]) asset_id else - path = File.join(ASSETS_DIR, source) + path = File.join(config.assets_dir, source) asset_id = File.exist?(path) ? File.mtime(path).to_i.to_s : '' if @@cache_asset_timestamps @@ -743,20 +747,20 @@ module ActionView def expand_javascript_sources(sources, recursive = false) if sources.include?(:all) - all_javascript_files = collect_asset_files(JAVASCRIPTS_DIR, ('**' if recursive), '*.js') + all_javascript_files = collect_asset_files(config.javascripts_dir, ('**' if recursive), '*.js') ((determine_source(:defaults, @@javascript_expansions).dup & all_javascript_files) + all_javascript_files).uniq else expanded_sources = sources.collect do |source| determine_source(source, @@javascript_expansions) end.flatten - expanded_sources << "application" if sources.include?(:defaults) && File.exist?(File.join(JAVASCRIPTS_DIR, "application.js")) + expanded_sources << "application" if sources.include?(:defaults) && File.exist?(File.join(config.javascripts_dir, "application.js")) expanded_sources end end def expand_stylesheet_sources(sources, recursive) if sources.first == :all - collect_asset_files(STYLESHEETS_DIR, ('**' if recursive), '*.css') + collect_asset_files(config.stylesheets_dir, ('**' if recursive), '*.css') else sources.collect do |source| determine_source(source, @@stylesheet_expansions) @@ -803,7 +807,7 @@ module ActionView end def asset_file_path(path) - File.join(ASSETS_DIR, path.split('?').first) + File.join(config.assets_dir, path.split('?').first) end def asset_file_path!(path) diff --git a/actionpack/test/template/asset_tag_helper_test.rb b/actionpack/test/template/asset_tag_helper_test.rb index d94135b04b..57802ebf42 100644 --- a/actionpack/test/template/asset_tag_helper_test.rb +++ b/actionpack/test/template/asset_tag_helper_test.rb @@ -3,6 +3,13 @@ require 'abstract_unit' class AssetTagHelperTest < ActionView::TestCase tests ActionView::Helpers::AssetTagHelper + DEFAULT_CONFIG = ActionView::DEFAULT_CONFIG.merge( + :assets_dir => File.dirname(__FILE__) + "/../fixtures/public", + :javascripts_dir => File.dirname(__FILE__) + "/../fixtures/public/javascripts", + :stylesheets_dir => File.dirname(__FILE__) + "/../fixtures/public/stylesheets") + + include ActiveSupport::Configurable + def setup super silence_warnings do @@ -872,6 +879,9 @@ end class AssetTagHelperNonVhostTest < ActionView::TestCase tests ActionView::Helpers::AssetTagHelper + DEFAULT_CONFIG = ActionView::DEFAULT_CONFIG + include ActiveSupport::Configurable + def setup super ActionController::Base.relative_url_root = "/collaboration/hieraki" diff --git a/actionpack/test/template/form_tag_helper_test.rb b/actionpack/test/template/form_tag_helper_test.rb index d64b9492e2..47462b1237 100644 --- a/actionpack/test/template/form_tag_helper_test.rb +++ b/actionpack/test/template/form_tag_helper_test.rb @@ -3,6 +3,9 @@ require 'abstract_unit' class FormTagHelperTest < ActionView::TestCase tests ActionView::Helpers::FormTagHelper + include ActiveSupport::Configurable + DEFAULT_CONFIG = ActionView::DEFAULT_CONFIG + def setup super @controller = Class.new do diff --git a/actionpack/test/template/url_helper_test.rb b/actionpack/test/template/url_helper_test.rb index 7f6ebc56b7..111a7619b5 100644 --- a/actionpack/test/template/url_helper_test.rb +++ b/actionpack/test/template/url_helper_test.rb @@ -5,6 +5,9 @@ require 'controller/fake_controllers' RequestMock = Struct.new("Request", :request_uri, :protocol, :host_with_port, :env) class UrlHelperTest < ActionView::TestCase + include ActiveSupport::Configurable + DEFAULT_CONFIG = ActionView::DEFAULT_CONFIG + def setup super @controller = Class.new do -- cgit v1.2.3 From 1b3195b63ca44f0a70b61b75fcf4991cb2fbb944 Mon Sep 17 00:00:00 2001 From: Phil Darnowsky Date: Wed, 7 Oct 2009 14:49:38 -0400 Subject: ActionView.url_for doesn't escape by default ActionView::Helpers::UrlHelper#url_for used to escape the URLs it generated by default. This was most commonly seen when generating a path with multiple query parameters, e.g. url_for(:controller => :foo, :action => :bar, :this => 123, :that => 456) would return http://example.com/foo/bar?that=456&this=123 escaping an ampersand that shouldn't be escaped. This is both wrong and inconsistent with the behavior of ActionController#url_for, and is changed. Signed-off-by: Michael Koziarski --- actionpack/lib/action_view/helpers/url_helper.rb | 2 +- actionpack/test/template/url_helper_test.rb | 16 +++++++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_view/helpers/url_helper.rb b/actionpack/lib/action_view/helpers/url_helper.rb index e651bc17a9..44e7073227 100644 --- a/actionpack/lib/action_view/helpers/url_helper.rb +++ b/actionpack/lib/action_view/helpers/url_helper.rb @@ -83,7 +83,7 @@ module ActionView options when Hash options = { :only_path => options[:host].nil? }.update(options.symbolize_keys) - escape = options.key?(:escape) ? options.delete(:escape) : true + escape = options.key?(:escape) ? options.delete(:escape) : false @controller.send(:url_for, options) when :back escape = false diff --git a/actionpack/test/template/url_helper_test.rb b/actionpack/test/template/url_helper_test.rb index 111a7619b5..cc3b2455d7 100644 --- a/actionpack/test/template/url_helper_test.rb +++ b/actionpack/test/template/url_helper_test.rb @@ -22,7 +22,7 @@ class UrlHelperTest < ActionView::TestCase def test_url_for_escapes_urls @controller.url = "http://www.example.com?a=b&c=d" - assert_equal "http://www.example.com?a=b&c=d", url_for(:a => 'b', :c => 'd') + assert_equal "http://www.example.com?a=b&c=d", url_for(:a => 'b', :c => 'd') assert_equal "http://www.example.com?a=b&c=d", url_for(:a => 'b', :c => 'd', :escape => true) assert_equal "http://www.example.com?a=b&c=d", url_for(:a => 'b', :c => 'd', :escape => false) end @@ -42,6 +42,16 @@ class UrlHelperTest < ActionView::TestCase assert_equal 'javascript:history.back()', url_for(:back) end + def test_url_for_from_hash_doesnt_escape_ampersand + @controller = TestController.new + @view = ActionView::Base.new + @view.controller = @controller + + path = @view.url_for(:controller => :cheeses, :foo => :bar, :baz => :quux) + + assert_equal '/cheeses?baz=quux&foo=bar', path + end + # todo: missing test cases def test_button_to_with_straight_url assert_dom_equal "
", button_to("Hello", "http://www.example.com") @@ -298,7 +308,7 @@ class UrlHelperTest < ActionView::TestCase @controller.request = RequestMock.new("http://www.example.com/weblog/show?order=desc&page=1") @controller.url = "http://www.example.com/weblog/show?order=desc&page=1" assert_equal "Showing", link_to_unless_current("Showing", { :action => "show", :controller => "weblog", :order=>'desc', :page=>'1' }) - assert_equal "Showing", link_to_unless_current("Showing", "http://www.example.com/weblog/show?order=desc&page=1") + assert_equal "Showing", link_to_unless_current("Showing", "http://www.example.com/weblog/show?order=desc&page=1") assert_equal "Showing", link_to_unless_current("Showing", "http://www.example.com/weblog/show?order=desc&page=1") @controller.request = RequestMock.new("http://www.example.com/weblog/show?order=desc") @@ -308,7 +318,7 @@ class UrlHelperTest < ActionView::TestCase @controller.request = RequestMock.new("http://www.example.com/weblog/show?order=desc&page=1") @controller.url = "http://www.example.com/weblog/show?order=desc&page=2" - assert_equal "Showing", link_to_unless_current("Showing", { :action => "show", :controller => "weblog" }) + assert_equal "Showing", link_to_unless_current("Showing", { :action => "show", :controller => "weblog" }) assert_equal "Showing", link_to_unless_current("Showing", "http://www.example.com/weblog/show?order=desc&page=2") -- cgit v1.2.3 From 5d5e34fa52183566968cb22f7c49544a7361a130 Mon Sep 17 00:00:00 2001 From: Michael Koziarski Date: Thu, 15 Oct 2009 09:58:17 +1300 Subject: Use ERB::Util.h over CGI.escapeHTML as the former is safety aware and the latter isn't --- actionpack/lib/action_controller/metal/redirector.rb | 2 +- actionpack/lib/action_view/safe_buffer.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_controller/metal/redirector.rb b/actionpack/lib/action_controller/metal/redirector.rb index f79fd54acd..b55f5e7bfc 100644 --- a/actionpack/lib/action_controller/metal/redirector.rb +++ b/actionpack/lib/action_controller/metal/redirector.rb @@ -16,7 +16,7 @@ module ActionController logger.info("Redirected to #{url}") if logger && logger.info? self.status = status self.location = url.gsub(/[\r\n]/, '') - self.response_body = "You are being redirected." + self.response_body = "You are being redirected." end end end diff --git a/actionpack/lib/action_view/safe_buffer.rb b/actionpack/lib/action_view/safe_buffer.rb index 8ba9cd80d6..09f44ab26f 100644 --- a/actionpack/lib/action_view/safe_buffer.rb +++ b/actionpack/lib/action_view/safe_buffer.rb @@ -5,7 +5,7 @@ module ActionView #:nodoc: if value.html_safe? super(value) else - super(CGI.escapeHTML(value)) + super(ERB::Util.h(value)) end end -- cgit v1.2.3 From 1d01bad3cedfd690c6d125cac6d4504baa9409e5 Mon Sep 17 00:00:00 2001 From: Michael Koziarski Date: Thu, 15 Oct 2009 09:58:35 +1300 Subject: Make sure non-escaped urls aren't considered safe --- actionpack/lib/action_view/helpers/url_helper.rb | 2 +- actionpack/test/template/url_helper_test.rb | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_view/helpers/url_helper.rb b/actionpack/lib/action_view/helpers/url_helper.rb index 44e7073227..5b136d4f54 100644 --- a/actionpack/lib/action_view/helpers/url_helper.rb +++ b/actionpack/lib/action_view/helpers/url_helper.rb @@ -93,7 +93,7 @@ module ActionView polymorphic_path(options) end - (escape ? escape_once(url) : url).html_safe! + escape ? escape_once(url).html_safe! : url end # Creates a link tag of the given +name+ using a URL created by the set diff --git a/actionpack/test/template/url_helper_test.rb b/actionpack/test/template/url_helper_test.rb index cc3b2455d7..cec53e479c 100644 --- a/actionpack/test/template/url_helper_test.rb +++ b/actionpack/test/template/url_helper_test.rb @@ -26,6 +26,11 @@ class UrlHelperTest < ActionView::TestCase assert_equal "http://www.example.com?a=b&c=d", url_for(:a => 'b', :c => 'd', :escape => true) assert_equal "http://www.example.com?a=b&c=d", url_for(:a => 'b', :c => 'd', :escape => false) end + + def test_url_for_escaping_is_safety_aware + assert url_for(:a => 'b', :c => 'd', :escape => true).html_safe?, "escaped urls should be html_safe?" + assert !url_for(:a => 'b', :c => 'd', :escape => false).html_safe?, "non-escaped urls shouldn't be safe" + end def test_url_for_escapes_url_once @controller.url = "http://www.example.com?a=b&c=d" -- cgit v1.2.3 From 316f4704eaa8aaba11e7ecebc1da9aa926fdd2d0 Mon Sep 17 00:00:00 2001 From: Craig Smith Date: Fri, 5 Jun 2009 14:58:38 +0100 Subject: Test cases should see all the cookies, not just cookies that have been set in the controller. Previously this example would always pass, even when cookies.delete was not called. @request.cookies['foo'] = 'bar' get :delete_cookie assert_nil cookies['foo'] Signed-off-by: Michael Koziarski [#2768 state:committed] --- actionpack/lib/action_controller/testing/process.rb | 2 +- actionpack/test/controller/test_test.rb | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_controller/testing/process.rb b/actionpack/lib/action_controller/testing/process.rb index bbc7f3c8f9..323cce6a2f 100644 --- a/actionpack/lib/action_controller/testing/process.rb +++ b/actionpack/lib/action_controller/testing/process.rb @@ -35,7 +35,7 @@ module ActionController #:nodoc: end def cookies - @response.cookies + @request.cookies.merge(@response.cookies) end def redirect_to_url diff --git a/actionpack/test/controller/test_test.rb b/actionpack/test/controller/test_test.rb index 73870a56bb..375878b755 100644 --- a/actionpack/test/controller/test_test.rb +++ b/actionpack/test/controller/test_test.rb @@ -108,6 +108,11 @@ XML head :created, :location => 'created resource' end + def delete_cookie + cookies.delete("foo") + render :nothing => true + end + private def rescue_action(e) raise e @@ -512,6 +517,18 @@ XML assert @request.params[:foo].blank? end + def test_should_have_knowledge_of_client_side_cookie_state_even_if_they_are_not_set + @request.cookies['foo'] = 'bar' + get :no_op + assert_equal 'bar', cookies['foo'] + end + + def test_should_detect_if_cookie_is_deleted + @request.cookies['foo'] = 'bar' + get :delete_cookie + assert_nil cookies['foo'] + end + %w(controller response request).each do |variable| %w(get post put delete head process).each do |method| define_method("test_#{variable}_missing_for_#{method}_raises_error") do -- cgit v1.2.3 From 3de8b44b26ecb64dc73661deb8dde1c5de92b496 Mon Sep 17 00:00:00 2001 From: George Ogata Date: Sun, 30 Aug 2009 23:46:48 -0400 Subject: Make IntegrationTest::Runner propagate method_missing to ancestors. Fixes RSpec integration example groups, which mixes its Matchers module into ActiveSupport::TestCase. Signed-off-by: Michael Koziarski --- actionpack/lib/action_dispatch/testing/integration.rb | 8 ++++++-- actionpack/test/controller/integration_test.rb | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/testing/integration.rb b/actionpack/lib/action_dispatch/testing/integration.rb index 2c4a3a356d..58ebe94a5b 100644 --- a/actionpack/lib/action_dispatch/testing/integration.rb +++ b/actionpack/lib/action_dispatch/testing/integration.rb @@ -396,8 +396,12 @@ module ActionDispatch # Delegate unhandled messages to the current session instance. def method_missing(sym, *args, &block) reset! unless @integration_session - returning @integration_session.__send__(sym, *args, &block) do - copy_session_variables! + if @integration_session.respond_to?(sym) + returning @integration_session.__send__(sym, *args, &block) do + copy_session_variables! + end + else + super end end end diff --git a/actionpack/test/controller/integration_test.rb b/actionpack/test/controller/integration_test.rb index 508364d0b5..fe95fb5750 100644 --- a/actionpack/test/controller/integration_test.rb +++ b/actionpack/test/controller/integration_test.rb @@ -199,6 +199,24 @@ class IntegrationTestTest < Test::Unit::TestCase assert_equal ::ActionController::Integration::Session, session2.class assert_not_equal session1, session2 end + + # RSpec mixes Matchers (which has a #method_missing) into + # IntegrationTest's superclass. Make sure IntegrationTest does not + # try to delegate these methods to the session object. + def test_does_not_prevent_method_missing_passing_up_to_ancestors + mixin = Module.new do + def method_missing(name, *args) + name.to_s == 'foo' ? 'pass' : super + end + end + @test.class.superclass.__send__(:include, mixin) + begin + assert_equal 'pass', @test.foo + ensure + # leave other tests as unaffected as possible + mixin.__send__(:remove_method, :method_missing) + end + end end # Tests that integration tests don't call Controller test methods for processing. -- cgit v1.2.3