Skip to content

Bump rails version to 7.2 [WIP] #20109

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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dwelch-r7
Copy link
Contributor

initial attempt for upgrading Rails to 7.2

@dwelch-r7 dwelch-r7 force-pushed the rails-7.2-upgrade branch 3 times, most recently from b44b947 to 690c6a7 Compare May 6, 2025 16:12
@dwelch-r7 dwelch-r7 force-pushed the rails-7.2-upgrade branch 3 times, most recently from f971b6b to 3883404 Compare May 20, 2025 11:31
@dwelch-r7 dwelch-r7 force-pushed the rails-7.2-upgrade branch 5 times, most recently from 85fcc2b to 924fbef Compare May 28, 2025 12:30
@dwelch-r7 dwelch-r7 force-pushed the rails-7.2-upgrade branch from 924fbef to c2fdef7 Compare June 2, 2025 10:28
if config.respond_to?(:active_record)
# Timecop in tests followed by db interaction causes a ActiveRecord::InvalidMigrationTimestampError
# due to the current time (set by timecop) being less than the migration timestamp.
config.active_record.validate_migration_timestamps = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Coud we see what list of tests fail here? We might be able to update the tests instead here, and keep the option enabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rspec './spec/lib/msf/db_manager_spec.rb[1:14:3:2]' # Msf::DBManager it should behave like Msf::DBManager::Migration #migrate should return an ActiveRecord::MigrationContext with known migrations
rspec ./spec/lib/msf/ui/console/command_dispatcher/core_spec.rb:657 # Msf::Ui::Console::CommandDispatcher::Core#cmd_sessions Searches with sessions that have different checkins and types When the user specifies both type and checkin
rspec ./spec/lib/msf/ui/console/command_dispatcher/core_spec.rb:669 # Msf::Ui::Console::CommandDispatcher::Core#cmd_sessions Searches with sessions that have different checkins and types When the user specifies both type and checkin but there are only partial matches
rspec ./spec/lib/msf/ui/console/command_dispatcher/core_spec.rb:573 # Msf::Ui::Console::CommandDispatcher::Core#cmd_sessions searches with sessions with different checkin values When the user searches using fractions of a minute
rspec ./spec/lib/msf/ui/console/command_dispatcher/core_spec.rb:586 # Msf::Ui::Console::CommandDispatcher::Core#cmd_sessions searches with sessions with different checkin values When the user searches using capital letters
rspec ./spec/lib/msf/ui/console/command_dispatcher/core_spec.rb:560 # Msf::Ui::Console::CommandDispatcher::Core#cmd_sessions searches with sessions with different checkin values When the user searches using multiple units with fractional seconds
rspec ./spec/lib/msf/ui/console/command_dispatcher/core_spec.rb:547 # Msf::Ui::Console::CommandDispatcher::Core#cmd_sessions searches with sessions with different checkin values When the user searches using fractions of a second
rspec ./spec/lib/msf/ui/console/command_dispatcher/core_spec.rb:632 # Msf::Ui::Console::CommandDispatcher::Core#cmd_sessions searches with sessions with different checkin values When the user uses two before arguments with last checkin
rspec ./spec/lib/msf/ui/console/command_dispatcher/core_spec.rb:613 # Msf::Ui::Console::CommandDispatcher::Core#cmd_sessions searches with sessions with different checkin values When the user properly specifies both less_than and greater_than checkin ranges
rspec ./spec/lib/msf/ui/console/command_dispatcher/core_spec.rb:599 # Msf::Ui::Console::CommandDispatcher::Core#cmd_sessions searches with sessions with different checkin values When the user searches using an invalid checkin parameter
rspec ./spec/lib/msf/ui/console/command_dispatcher/core_spec.rb:625 # Msf::Ui::Console::CommandDispatcher::Core#cmd_sessions searches with sessions with different checkin values When the user specifies a greater_than time that is larger than the less_than time
rspec ./spec/lib/msf/ui/console/command_dispatcher/core_spec.rb:606 # Msf::Ui::Console::CommandDispatcher::Core#cmd_sessions searches with sessions with different checkin values When the user searches using duplicated time units
rspec ./spec/modules/auxiliary/admin/kerberos/forge_ticket_spec.rb:28 # kerberos keytab #run when forging golden tickets generates a golden ticket

here ya go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  13) kerberos keytab #run when forging golden tickets generates a golden ticket
      Failure/Error: return context.needs_migration?

      ActiveRecord::InvalidMigrationTimestampError:


        Invalid timestamp 20221209005658 for migration file: create_index_on_private_data_and_type_for_pkcs12.
        Timestamp must be in form YYYYMMDDHHMMSS, and less than 20220716123340.
      # /Users/dwelch/.rvm/gems/ruby-3.2.8@metasploit-framework/gems/activerecord-7.2.2.1/lib/active_record/migration.rb:1311:in `block in migrations'
      # /Users/dwelch/.rvm/gems/ruby-3.2.8@metasploit-framework/gems/activerecord-7.2.2.1/lib/active_record/migration.rb:1307:in `map'
      # /Users/dwelch/.rvm/gems/ruby-3.2.8@metasploit-framework/gems/activerecord-7.2.2.1/lib/active_record/migration.rb:1307:in `migrations'
      # /Users/dwelch/.rvm/gems/ruby-3.2.8@metasploit-framework/gems/activerecord-7.2.2.1/lib/active_record/migration.rb:1303:in `pending_migration_versions'
      # /Users/dwelch/.rvm/gems/ruby-3.2.8@metasploit-framework/gems/activerecord-7.2.2.1/lib/active_record/migration.rb:1299:in `needs_migration?'
      # ./lib/msf/core/db_manager/migration.rb:57:in `block in needs_migration?'
      # ./lib/msf/core/db_manager/migration.rb:69:in `with_migration_context'
      # ./lib/msf/core/db_manager/migration.rb:56:in `needs_migration?'
      # ./lib/msf/core/db_manager/connection.rb:7:in `active'
      # ./lib/metasploit/framework/data_service/proxy/core.rb:45:in `active'
      # ./lib/msf/core/auxiliary/report.rb:84:in `db'
      # ./lib/msf/core/auxiliary/report.rb:419:in `store_loot'
      # ./lib/msf/core/exploit/remote/kerberos/ticket/storage/write_mixin.rb:26:in `store_ccache'
      # ./lib/msf/core/exploit/remote/kerberos/ticket/storage.rb:11:in `store_ccache'
      # ./modules/auxiliary/admin/kerberos/forge_ticket.rb:121:in `forge_ccache'
      # ./modules/auxiliary/admin/kerberos/forge_ticket.rb:142:in `forge_golden'
      # ./modules/auxiliary/admin/kerberos/forge_ticket.rb:86:in `run'
      # ./spec/modules/auxiliary/admin/kerberos/forge_ticket_spec.rb:39:in `block (4 levels) in <top (required)>'
      ```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

managed to sort these out

@@ -16,7 +16,7 @@ def creds(opts)
query = query.includes(logins: [ :service, { service: :host } ])

if opts[:type].present?
query = query.where('"metasploit_credential_privates"."type" = ?', opts[:type])
query = query.where('"metasploit_credential_privates"."type" = ?', opts[:type].to_s)
Copy link
Contributor

Choose a reason for hiding this comment

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

Fixing a type coercion, where opts[:type] was a real Ruby object when it was expecting a string or primitive value - which broke Rails. Forcing .to_s ensures the string value is used

@dwelch-r7 dwelch-r7 force-pushed the rails-7.2-upgrade branch 2 times, most recently from 40a3583 to aa76c01 Compare June 5, 2025 11:08
@dwelch-r7 dwelch-r7 force-pushed the rails-7.2-upgrade branch from aa76c01 to f85f661 Compare June 10, 2025 11:15
@@ -36,6 +35,9 @@
subject.datastore['EXTRA_SIDS'] = ' S-1-18-1, S-1-5-21-1266190811-2419310613-1856291569-519, '
subject.datastore['SessionKey'] = 'A' * 16

expect(framework.db.active).to be(true)
Timecop.freeze(Time.parse('Jul 15, 2022 12:33:40.000000000 GMT'))

subject.run
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
subject.run
Timecop.freeze(Time.parse('Jul 15, 2022 12:33:40.000000000 GMT')) do
subject.run
end

@@ -36,6 +35,9 @@
subject.datastore['EXTRA_SIDS'] = ' S-1-18-1, S-1-5-21-1266190811-2419310613-1856291569-519, '
subject.datastore['SessionKey'] = 'A' * 16

expect(framework.db.active).to be(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed? 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this one was a bit hacky, but if we trigger the check for the db being migrated before we set timecop then we don't need to re-check if the db has been migrated and so there's no issue anymore

allow(db_manager).to receive(:active).and_return(active)
db_manager.workspace = db_manager.default_workspace
Copy link
Contributor

Choose a reason for hiding this comment

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

Any context on why these lines needed to be switched around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

calling off to default_workspace was triggering the migration check before the db_manager was mocked out which led to the migration date being in the future for timecopped tests, so I just swapped them round

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming renaming this file will break assumptions in the test? 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope it gets the migration form the folder path and before we were getting this error

Invalid timestamp 99999999999999 for migration file: test_db_migration.
Timestamp must be in form YYYYMMDDHHMMSS, and less than 20250612131915.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to use timecop here to get it to work as it did previously? Or I might be missing a nuance still 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uhhhh I actually don't know if that would work, the current date/time in the file format is invalid even if we set timecop to the year 10,000 I think it'd still complain since there is no 99th day of the 99th month 😅

@dwelch-r7 dwelch-r7 force-pushed the rails-7.2-upgrade branch from 969d08c to be51583 Compare June 12, 2025 09:48
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.

2 participants