Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NoMethodError when request Content-Type is blank. #48887

Conversation

bryantraywick
Copy link
Contributor

Motivation / Background

This Pull Request has been created because since this change we started receiving a NoMethodError "undefined method `rstrip' for nil:NilClass" from line 166 of actionpack/lib/action_dispatch/http/mime_type.rb when a client makes a request with a blank Content-Type header. I believe this client is not adhering to the spec, but prior to the commit they would raise an ActionDispatch::Http::MimeNegotiation::InvalidType error and receive a "406 Not Acceptable" response instead of a "500 Server Error" response.

The NoMethodErrors also clog our error tracking system, whereas we are able to ignore the more specific invalid mime type errors.

Detail

This Pull Request changes the mime type lookup to use the safe nagivation operator on the call to rstrip to handle the case when a client makes a request with a blank Content-Type header.

Additional information

The following code snippet contains a working recreation of the issue with the current main branch.

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
  gem "rack", "~> 2.0"
end

require "action_controller/railtie"

class TestApp < Rails::Application
  config.root = __dir__
  config.hosts << "example.org"
  config.secret_key_base = "secret_key_base"

  config.logger = Logger.new($stdout)
  Rails.logger  = config.logger

  routes.draw do
    get "/" => "test#index"
  end
end

class TestController < ActionController::Base
  include Rails.application.routes.url_helpers

  def index
    render plain: "Home"
  end
end

require "minitest/autorun"
require "rack/test"

class BugTest < Minitest::Test
  include Rack::Test::Methods

  def test_returns_success
    get "/", {}, 'CONTENT_TYPE' => ''

    assert last_response.server_error?
    assert (last_response.status == 500)
  end

  private
    def app
      Rails.application
    end
end

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the actionpack label Aug 4, 2023
@guilleiguaran guilleiguaran merged commit 3b3e820 into rails:main Aug 4, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants