Permit attachments in inbound email conductor mail params

[unixmonkey]

Summary

When uploading email attachments using the inbound emails conductor in development mode, it throws a warning about the unpermitted parameter :attachments.

With the app configured with config.action_controller.action_on_unpermitted_parameters = :raise, this also raises an error.

Additionally, the "unpermitted" attachments parameter is then used to create attachments, bypassing the strong parameter safeguard.

This PR permits the :attachments param, but strips it from being passed to Mail.new, then uses the permitted param to build the mail attachments inside the mail block.

Steps to reproduce

• git clone rails
• cd rails
• bundle exec rails new ~/my-test-app --dev
• cd ~/my-test-app
• bin/rails action_mailbox:install
• bin/rails db:migrate
• Setup mailbox route:
  • echo "class ApplicationMailbox < ActionMailbox::Base\n routing all: :catchall\nend" > 'app/mailboxes/application_mailbox.rb'
  • echo "class CatchallMailbox < ApplicationMailbox\n def process; end\nend" > 'app/mailboxes/catchall_mailbox.rb
• bin/rails server
• Add file attachment to a test inbound email:
  • Visit http://localhost:3000/rails/conductor/action_mailbox/inbound_emails/new
  • Click "Choose Files"
  • Choose any test file to include as an attachment
  • Click the "Deliver inbound email" button

Expected result:

Email is processed with ActionMailbox without warning.

Actual result:

The console shows this in red text:

Unpermitted parameter: :attachments. Context: { }

With the app configured to raise on unpermitted parameters:

• Add config.action_controller.action_on_unpermitted_parameters = :raise to config/development.rb
• bin/rails server
• Visit http://localhost:3000/rails/conductor/action_mailbox/inbound_emails/new
• Click "Choose Files"
• Choose any test file to include as an attachment
• Click the "Deliver inbound email" button

Expected result:

Attachment is processed by ActionMailer without raising an error.

Actual result:

Rails throws error, displays the exception page, and message is not processed:

ActionController::UnpermittedParameters in Rails::Conductor::ActionMailbox::InboundEmailsController#create
found unpermitted parameter: :attachments

[ghiculescu]

Hmmmm, any ideas why this test wouldn't have failed? The patch looks good but it would be awesome to include a regression test.

[unixmonkey]

Hmmmm, any ideas why this test wouldn't have failed? The patch looks good but it would be awesome to include a regression test.

If you run

bundle exec rake test TEST="test/controllers/rails/action_mailbox/inbound_emails_controller_test.rb"

from the actionmailbox directory on the main branch today, you can see the Unpermitted parameter: :attachments. Context: { } line in actionmailbox/test/dummy/log/test.log.

And if you change actionmailbox/test/dummy/config/environments/test.rb to include the config.action_controller.action_on_unpermitted_parameters = :raise line, you will see that test fail.

I tried adding Rails.application.config.action_controller.action_on_unpermitted_parameters = :raise to that specific test, but it doesn't seem to actually change the configuration to cause it to fail as expected.

How would you recommend I change the test to account for the config change, or assert if the warning is being logged or not?

[ghiculescu]

And if you change actionmailbox/test/dummy/config/environments/test.rb to include the config.action_controller.action_on_unpermitted_parameters = :raise line, you will see that test fail.

Just thinking out loud, but maybe it's a good idea to actually do this. It feels like it would be good for the tests to be run in as strict an environment as possible.

[unixmonkey]

And if you change actionmailbox/test/dummy/config/environments/test.rb to include the config.action_controller.action_on_unpermitted_parameters = :raise line, you will see that test fail.

Just thinking out loud, but maybe it's a good idea to actually do this. It feels like it would be good for the tests to be run in as strict an environment as possible.

Good idea. I've added a commit that adds that, and the one spec you pointed out is the only one in the actionmailbox suite that fails on the main branch.

[eugeneius]

I think we should add a new test to cover the bug being fixed here, rather than modify the configuration for the whole test suite.

You can change the behaviour for a single test by setting it on ActionController::Parameters:

test "create inbound email with action_on_unpermitted_parameters = :raise" do
  original_action_on_unpermitted_parameters = ActionController::Parameters.action_on_unpermitted_parameters
  ActionController::Parameters.action_on_unpermitted_parameters = :raise
  # ...
ensure
  ActionController::Parameters.action_on_unpermitted_parameters = original_action_on_unpermitted_parameters
end

[ghiculescu]

My thinking when I suggested it was that it's better to run the whole test suite in as strict a configuration as possible. Then individual tests can opt out of the strictness, if they need it.

If the configuration had been set like this, the bug this PR fixes would never have slipped through, since there's already a test that covers it.

[unixmonkey]

I'm open to do this if desired, but this seems like it's indirectly testing that action_on_unpermitted_parameters works as intended, which should already be covered by that part of the suite. I could see submitting this as a test case that should not be committed to demonstrate the issue though.

I'm in agreement with @ghiculescu that the suite should be run with the stricter setting, which would have alerted the original author of the create inbound email with attachments test that it had this issue.

I am happy to revert the config change in this PR, but as you can see, there are no other tests in the suite that fail with it set to :raise, and this offers protection for future contributors.

For some personal background, several applications I have worked with also have action_on_unpermitted_parameters = :raise set in development, because the default behavior of :log has caused developers on my teams to miss issues that were only later caught in production.

[unixmonkey]

To add to this, when we added and enabled ActionMailbox in our application, we did so specifically to process inbound email attachments, so we were quite confused when we were unable to send a test email that included one from the conductor, and had to dig into Rails internals to discover the issue.

We temporarily worked around it with a monkey-patch in our app, before realizing that it is related to this config setting, which I admit is nonstandard, but I believe should only blow up when making an error in our application, which would be easier to track down, and not for Rails internals, which should work in every configuration, if possible.

[eugeneius]

For posterity: this came up previously in #37932.

[unixmonkey]

Anything I can do to help this along? Should I remove the config change and add an additional test that duplicates the first with the stricter setting, or squash commits?

[ghiculescu]

Could you please squash your commits? Other than that I think it's just waiting for @eugeneius or another member of the core team to take a look when they can. Sit tight - they are busy but they do get around to it :)