Skip to content

Don't rebuild the middleware stack when using with_routing #54705

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

Merged
merged 1 commit into from
Mar 7, 2025

Conversation

Edouard-chin
Copy link
Member

Motivation / Background

Fix #54595

Detail

Problem

When using the with_routing helper method in an integration test, we rebuild the middleware stack so that the entrypoint of our new stack is the new RouteSet object.

By rebuilding the middleware stack from scratch, we may end un-configuring a middleware from an application. We also break the contract of the Rails initialization process.

For instance, if you have a middleware that needs to be configured, after the Rails routes are loaded, this is what it would look like:

# config/application.rb

warden_config = nil

config.middleware.use Warden::Manager do |config|
  # The Warden middleware yields its config when the middleware is
  # instantiated. We can't configure warden yet as the routes
  # are not yet loaded.

  warden_config = config
end

initializer :configure_warden do
  ActiveSupport.on_load(:after_routes_loaded) do
    # The routes are now loaded, we can configure warden
  end
end

This snippet follows the order of the documented Rails initialization process where first the middleware stack is built and then the routes gets loaded.


By rebuilding the middleware stack, we unconfigure what was done in the initializer.

Solution

Since rebuilding the middleware stack has some caveats, my idea is to modify the middleware stack original entry point and redefine call so that it calls our new RouteSet middleware.
We have a reference to the original RouteSet middleware thanks to the application.routes method.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes 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.

Sorry, something went wrong.

- Fix rails#54595
- ### Problem

  When using the `with_routing` helper method in an integration test,
  we rebuild the middleware stack so that the entrypoint
  of our new stack is the new RouteSet object.

  By rebuilding the middleware stack from scratch, we may end
  un-configuring a middleware from an application. We also break
  the contract of the Rails initialization process.

  For instance, if you have a middleware that needs to be configured,
  after the Rails routes are loaded, this is what it would look like:

  ```ruby
  # config/application.rb

  warden_config = nil

  config.middleware.use Warden::Manager do |config|
    # The Warden middleware yields its config when the middleware is
    # instantiated. We can't configure warden yet as the routes
    # are not yet loaded.

    warden_config = config
  end

  initializer :configure_warden do
    ActiveSupport.on_load(:after_routes_loaded) do
      # The routes are now loaded, we can configure warden
    end
  end
  ```

  This snippet follows the order of the documented Rails
  initialization process where first the middleware stack is built
  and then the routes gets loaded.

  --------------------

  By rebuilding the middleware stack, we unconfigure what was done
  in the initializer.

  ### Solution

  Since rebuilding the middleware stack has some caveats, my idea is
  to modify the middleware stack original entry point and redefine
  `call` so that it calls our new RouteSet middleware.
  We have a reference to the original RouteSet middleware thanks to
  the `application.routes` method.
@byroot
Copy link
Member

byroot commented Mar 6, 2025

I'll try to review this tomorrow morning, it's a bit too hairy for tonight.

@quixoten
Copy link
Contributor

quixoten commented Mar 6, 2025

Verified my test suite passes against this branch (and devise 4.9.4)

@byroot byroot merged commit 3720ca0 into rails:main Mar 7, 2025
3 checks passed
byroot added a commit that referenced this pull request Mar 7, 2025
Don't rebuild the middleware stack when using `with_routing`
@Edouard-chin
Copy link
Member Author

Thanks for reviewing! Also thank you @quixoten for helping debug the issue !

@Edouard-chin Edouard-chin deleted the ec-with-routing branch March 7, 2025 13:56
stevenharman pushed a commit to docsend/activerecord-session_store that referenced this pull request Apr 3, 2025

Verified

This commit was signed with the committer’s verified signature.
stevenharman Steven Harman
This change allows the disabling of fallback used to access old,
insecure sessions, and rewrite them as secure sessions. The fallback was
originally added as part of the mitigation of CVE-2019-25025 several
years back.

However, this fallback mechanism was added over 5 years ago. In many
cases, or at least in our case, the expiry on old, insecure, sessions
has long since passed. We'd like the ability to disable the fallback
entirely as it will never be a valid path for us.

See: rails#151

Also, we had to improve our patch for
`ActionDispatch::Assertions::RoutingAssertions::WithIntegrationRouting`
to handle middleware correctly. This is the same implementation as was
added in Rails 8.0.

See: rails/rails#54705
stevenharman pushed a commit to docsend/activerecord-session_store that referenced this pull request Apr 4, 2025

Verified

This commit was signed with the committer’s verified signature.
stevenharman Steven Harman
This change allows the disabling of fallback used to access old,
insecure sessions, and rewrite them as secure sessions. The fallback was
originally added as part of the mitigation of CVE-2019-25025 several
years back.

However, this fallback mechanism was added over 5 years ago. In many
cases, or at least in our case, the expiry on old, insecure, sessions
has long since passed. We'd like the ability to disable the fallback
entirely as it will never be a valid path for us.

See: rails#151

Also, we had to improve our patch for
`ActionDispatch::Assertions::RoutingAssertions::WithIntegrationRouting`
to handle middleware correctly. This is the same implementation as was
added in Rails 8.0.

See: rails/rails#54705
stevenharman pushed a commit to docsend/activerecord-session_store that referenced this pull request Apr 4, 2025

Verified

This commit was signed with the committer’s verified signature.
stevenharman Steven Harman
This change allows the disabling of fallback used to access old,
insecure sessions, and rewrite them as secure sessions. The fallback was
originally added as part of the mitigation of CVE-2019-25025 several
years back.

However, this fallback mechanism was added over 5 years ago. In many
cases, or at least in our case, the expiry on old, insecure, sessions
has long since passed. We'd like the ability to disable the fallback
entirely as it will never be a valid path for us.

See: rails#151

Also, we had to improve our patch for
`ActionDispatch::Assertions::RoutingAssertions::WithIntegrationRouting`
to handle middleware correctly. This is the same implementation as was
added in Rails 8.0.

See: rails/rails#54705
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.

Race condition in with_routing integration test helper
3 participants