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

Make Bandit the default web server for new Phoenix apps #5706

Merged
merged 6 commits into from
Feb 2, 2024

Conversation

mtrudel
Copy link
Member

@mtrudel mtrudel commented Jan 30, 2024

This PR:

  • Makes Bandit the default web server for newly generated Phoenix apps
  • Updates the Phoenix docs to reflect both Bandit and Cowboy as first-class supported adapters
  • Keeps the default adapter as Cowboy if not explicitly specified for backwards compatibility
  • Since Phoenix still supports (and runs CI) back to 1.11, Phoenix's internal tests are NOT updated to use Bandit here. There's a separate PR coming to add this, which will gate on Phoenix bumping to 1.12+

@mtrudel
Copy link
Member Author

mtrudel commented Jan 30, 2024

This is working and ready


# Even though Bandit is the default in apps generated via the installer,
# we continue to use Cowboy as the default if not explicitly specified for
# backwards compatibility. TODO: Change this to default to Bandit in 2.0
Copy link
Member Author

@mtrudel mtrudel Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noto bene

(Also bump bandit dep to ~> 1.0)
Copy link
Member

@josevalim josevalim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If integration tests pass, this is good to go IMO!

@mtrudel
Copy link
Member Author

mtrudel commented Jan 30, 2024

Integration test failing on Plug not being loaded now that we've dropped plug_cowboy. There's likely some loading issue somewhere; I'll figure it out

@mtrudel
Copy link
Member Author

mtrudel commented Jan 30, 2024

Integration issue is upstream at swoosh/swoosh#857. Once this is fixed / released I'll update the dep here

@mtrudel
Copy link
Member Author

mtrudel commented Jan 30, 2024

Will ya look at that, all green and ready to go.

@krns
Copy link

krns commented Jan 31, 2024

We are using https in our local development setup and configured it like described in https://hexdocs.pm/phoenix/using_ssl.html#ssl-in-development. Today I tried Bandit but it crashes:

my-app-1       | [notice] Application my_app exited: MyApp.Application.start(:normal, []) returned an error: shutdown: failed to start child: MyAppWeb.Endpoint
my-app-1       |     ** (EXIT) shutdown: failed to start child: {MyAppWeb.Endpoint, :https}
my-app-1       |         ** (EXIT) an exception was raised:
my-app-1       |             ** (RuntimeError) Plug.SSL.configure/1 encountered error: the :otp_app option is required when setting relative SSL certfiles
my-app-1       |                 (bandit 1.1.3) lib/bandit.ex:262: Bandit.start_link/1
my-app-1       |                 (stdlib 5.2) supervisor.erl:420: :supervisor.do_start_child_i/3
my-app-1       |                 (stdlib 5.2) supervisor.erl:406: :supervisor.do_start_child/2
my-app-1       |                 (stdlib 5.2) supervisor.erl:390: anonymous fn/3 in :supervisor.start_children/2
my-app-1       |                 (stdlib 5.2) supervisor.erl:1258: :supervisor.children_map/4
my-app-1       |                 (stdlib 5.2) supervisor.erl:350: :supervisor.init_children/2
my-app-1       |                 (stdlib 5.2) gen_server.erl:980: :gen_server.init_it/2
my-app-1       |                 (stdlib 5.2) gen_server.erl:935: :gen_server.init_it/6
my-app-1       |                 (stdlib 5.2) proc_lib.erl:241: :proc_lib.init_p_do_apply/3

Maybe there are some things that are missing on our side, but I think these should be documented.

@mtrudel
Copy link
Member Author

mtrudel commented Jan 31, 2024

@krns yep - that's a wrinkle that you have to fix manually when using raw Bandit. In the case of Phoenix, we know for certain that we have an otp_app value so it's safe / preferable to pull it in automatically. I'm adding a change to Bandit.PhoenixAdapter to do just that. should be up in an hour or so

@mtrudel
Copy link
Member Author

mtrudel commented Jan 31, 2024

@krns Bandit 1.2.0 just released that should fix the issue!

@krns
Copy link

krns commented Jan 31, 2024

Bandit 1.2.0 works, thank you so much @mtrudel!

@chrismccord chrismccord merged commit a4a761c into phoenixframework:main Feb 2, 2024
5 of 6 checks passed
@chrismccord
Copy link
Member

❤️❤️❤️🐥🔥

@mtrudel mtrudel deleted the bandit_default branch February 2, 2024 03:29
@mtrudel
Copy link
Member Author

mtrudel commented Feb 2, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants