Allowing nil content-type with UnsupportedContentHandler #464

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@nhogle
  • Some websites will not send a Content-Type header when the content is sent using 'Content-Disposition: "attachment"; filename=""'. Mint.com is one example of this, when downloading transactions to a .csv file. This patch fixes this problem, and logs a warning if this ever occurs.
Nick Hogle Allowing nil content-type with UnsupportedContentHandler
	* Some websites will not send a Content-Type header when the content
	is sent using 'Content-Disposition: "attachment";
	filename="<some_file.ext>"'.  Mint.com is one example of this, when
	downloading transactions to a .csv file.  This patch fixes this
	problem, and logs a warning if this ever occurs.
7d204c0
@jferris jferris commented on the diff Feb 11, 2013
src/WebPage.cpp
@@ -269,14 +269,15 @@ NetworkAccessManager *WebPage::networkAccessManager() {
void WebPage::handleUnsupportedContent(QNetworkReply *reply) {
QVariant contentMimeType = reply->header(QNetworkRequest::ContentTypeHeader);
- if(!contentMimeType.isNull()) {
- triggerAction(QWebPage::Stop);
- UnsupportedContentHandler *handler = new UnsupportedContentHandler(this, reply);
- if (reply->isFinished())
- handler->renderNonHtmlContent();
- else
- handler->waitForReplyToFinish();
- }
+ if(contentMimeType.isNull())
+ m_manager->logger() << "Warning: no content type from " << reply->url().toString() << "Forcing content type to text/plain.";
@jferris
jferris Feb 11, 2013

Can you wrap this line at 80 characters?

@jferris
thoughtbot, inc. member

Thanks for the pull request. This project is 100% test-driven, so we're not accepting patches without tests. Are you willing to submit tests for your change?

@carloslopes

@jferris I'm willing to test this and close the PR. But i don't know where i should put the test for this behavior. Should it be inside the browser_spec.rb or driver_spec.rb?

@jferris
thoughtbot, inc. member

I think driver_spec.rb is probably the best place. If you're having trouble there, you could try browser_spec.rb. Thanks.

@carloslopes

Alright, i did a test but it's passing, so i don't know if this problem still happens or it's already solved.

Here are the test and the debug info:

  context "without Content-Type header" do
    let(:driver) do
      driver_for_app do
        get ('/') { "<html><body>D'oh!</body></html>" }
      end
    end

    before do
      Sinatra::Response.any_instance.stub(:drop_content_info?).and_return(true)
    end

    it "accepts the response" do
      visit('/')
    end
  end
    1445: 
    1446:     it "accepts the response" do
    1447:       visit('/')
    1448: 
    1449:       require 'pry'
 => 1450:       binding.pry
    1451:     end
    1452:   end
    1453: 
    1454:   context "no response app" do
    1455:     let(:driver) do

[1] pry(#<RSpec::Core::ExampleGroup::Nested_1::Nested_21>)> driver.response_headers
=> {"X-Content-Type-Options"=>"nosniff",
 "Server"=>"WEBrick/1.3.1 (Ruby/1.9.3/2013-02-06)",
 "Date"=>"Sat, 20 Apr 2013 23:08:23 GMT",
 "Content-Length"=>"31",
 "Connection"=>"Keep-Alive"}

Any toughs about this @jferris?

@jferris
thoughtbot, inc. member

Can you verify something about the response it sends back? For example, try sending a CSV file back and see if you can parse it. One thing the UnsupportedContentHandler does it prevent non-HTML from getting parsed as HTML.

@carloslopes

@jferris i did what you said but the test keep passing.. here is what i got:

  context "without Content-Type header" do
    let(:driver) do
      driver_for_app do
        get ('/') do
          attachment('foo.csv')
          "foo,bar\nhello,world"
        end
      end
    end

    before do
      Sinatra::Response.any_instance.stub(:drop_content_info?).and_return(true)
    end

    it "accepts the response" do
      visit('/')
    end
  end
[1] pry(#<RSpec::Core::ExampleGroup::Nested_1::Nested_21>)> driver.response_headers
=> {"Content-Disposition"=>"attachment; filename=\"foo.csv\"",
 "X-Content-Type-Options"=>"nosniff",
 "Server"=>"WEBrick/1.3.1 (Ruby/1.9.3/2013-02-06)",
 "Date"=>"Thu, 25 Apr 2013 22:12:40 GMT",
 "Content-Length"=>"19",
 "Connection"=>"Keep-Alive"}
[2] pry(#<RSpec::Core::ExampleGroup::Nested_1::Nested_21>)> driver.html
=> "foo,bar\nhello,world"
@jferris
thoughtbot, inc. member

Alright. Have you tried this with Mint.com, where you originally detected the problem? Is there a bug in capybara-webkit that you've run into, or did you suspect there might be a bug?

@carloslopes

It was not me that detected the problem, i'm just trying to help to close this issue.

And i don't have an account on mint.com, maybe @hogiedoo that was the creator of the issue could check this for us

@jferris
thoughtbot, inc. member

Okay. I'm going to close this for now, but feel free to comment back if you find out this is an actual bug. Thanks for looking into this.

@jferris jferris closed this Apr 26, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment