Fix security and stability issues from PR #179 review #184
Conversation
This enhancement brings cypress-rails functionality to cypress-playwright-on-rails: - Added rake tasks for cypress:open, cypress:run, playwright:open, playwright:run - Implemented automatic Rails server management with dynamic port selection - Added server lifecycle hooks (before_server_start, after_server_start, etc.) - Added transactional test mode for automatic database rollback - Added state reset middleware for /cypress_rails_reset_state endpoint - Support for CYPRESS_RAILS_HOST and CYPRESS_RAILS_PORT environment variables These changes make cypress-playwright-on-rails a more complete replacement for cypress-rails, providing the same developer-friendly test execution experience while maintaining all the existing cypress-on-rails functionality. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Added detailed migration instructions for: - Users currently using manual server management (old way) - Users migrating from cypress-rails gem The migration guide clearly shows the before/after comparison and provides step-by-step instructions for both scenarios. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add require 'fileutils' to server.rb - Refactor shell command execution to prevent injection attacks - Use argument arrays instead of string interpolation - Pass commands as arrays to spawn() and system() - Improve database state reset safety - Move after_state_reset hook to run after cleanup - Add support for disable_referential_integrity when available - Use proper table name quoting with quote_table_name - Use File.expand_path for folder detection These changes address security concerns about command injection and improve compatibility across different Rails environments. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughIntroduces server lifecycle management for test runs via a new Server class, Rake tasks for Cypress/Playwright, state reset middleware, and configuration hooks and options. Updates documentation and initializer template with configuration and migration guidance. Railtie now loads rake tasks and wires the reset middleware. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Rake as Rake Task (cypress/playwright)
participant Srv as CypressOnRails::Server
participant Rails as Rails Server
participant MW as StateResetMiddleware
participant DB as DB/DBCleaner
participant Runner as Test Runner (Cypress/Playwright)
Dev->>Rake: rake cypress:open | run
Rake->>Srv: new(...).open/run
Note over Srv: before_server_start hook
Srv->>Rails: spawn bin/rails server (-p, -b)
Srv->>Rails: wait until listening
Note over Srv: after_server_start hook
alt transactional_server=true
Srv->>DB: BEGIN transaction
Note over Srv: after_transaction_start hook
end
Srv->>Runner: launch/open or run tests
Runner->>Rails: test requests
opt reset state call
Runner->>Rails: GET /__cypress__/reset_state
Rails->>MW: call(env)
MW->>DB: reset via DatabaseCleaner or truncate
MW-->>Rails: 200 "State reset completed"
end
Runner-->>Srv: exit status
Note over Srv: before_server_stop hook
alt transactional_server=true
Srv->>DB: ROLLBACK
end
Srv->>Rails: terminate server
Srv-->>Rake: success/failure
Rake-->>Dev: exit code (0/1)
sequenceDiagram
autonumber
participant Dev as Developer
participant Rake as rake cypress:init
participant Srv as CypressOnRails::Server
Dev->>Rake: rake cypress:init
Rake->>Srv: new.init
Srv->>Srv: detect/create install folder
Srv-->>Dev: initialized
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 RuboCop (1.80.2)lib/cypress_on_rails/server.rbCould not find gem 'rspec' in locally installed gems. lib/cypress_on_rails/configuration.rbCould not find gem 'rspec' in locally installed gems. lib/tasks/cypress.rakeCould not find gem 'rspec' in locally installed gems.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review for PR #184: Fix security and stability issuesThank you for addressing the security concerns from PR #179! This is a significant addition to the gem that brings automated server management and improved security. Here's my detailed review: ✅ Strengths
🔍 Issues & Suggestions1. Missing Error Handling in server.rb:
2. Potential Race Condition in state_reset_middleware.rb:
3. Missing Validation in server.rb:
4. Performance Consideration:
🔒 Security Assessment✅ Command injection vulnerabilities properly addressed 📝 Test Coverage RecommendationsThe PR mentions tests are pending. Consider adding specs for:
💡 Minor Suggestions
Overall AssessmentThis is a solid improvement that significantly enhances the gem's security posture and usability. The automated server management will be a great quality-of-life improvement for users. With the suggested error handling improvements and test coverage, this will be production-ready. Great work addressing the security concerns! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)README.md(2 hunks)lib/cypress_on_rails/configuration.rb(2 hunks)lib/cypress_on_rails/railtie.rb(1 hunks)lib/cypress_on_rails/server.rb(1 hunks)lib/cypress_on_rails/state_reset_middleware.rb(1 hunks)lib/generators/cypress_on_rails/templates/config/initializers/cypress_on_rails.rb.erb(1 hunks)lib/tasks/cypress.rake(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/cypress_on_rails/state_reset_middleware.rb (2)
lib/cypress_on_rails/server.rb (2)
initialize(10-19)run_hook(191-195)lib/cypress_on_rails/configuration.rb (2)
initialize(36-38)configuration(76-78)
lib/cypress_on_rails/server.rb (2)
lib/cypress_on_rails/state_reset_middleware.rb (4)
initialize(2-57)initialize(3-5)run_hook(54-56)call(7-14)lib/cypress_on_rails/configuration.rb (2)
initialize(36-38)configuration(76-78)
🪛 Brakeman (7.0.2)
lib/cypress_on_rails/server.rb
[medium] 188-188: Possible command injection
Type: Command Injection
Confidence: Medium
More info: https://brakemanscanner.org/docs/warning_types/command_injection/
(Command Injection)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: rails_8
- GitHub Check: rails_7_2
- GitHub Check: rails_6_1
| if @transactional && defined?(ActiveRecord::Base) | ||
| ActiveRecord::Base.connection.begin_transaction(joinable: false) | ||
| run_hook(config.after_transaction_start) | ||
| end | ||
|
|
||
| yield | ||
|
|
||
| ensure | ||
| run_hook(config.before_server_stop) | ||
|
|
||
| if @transactional && defined?(ActiveRecord::Base) | ||
| ActiveRecord::Base.connection.rollback_transaction if ActiveRecord::Base.connection.transaction_open? | ||
| end | ||
|
|
There was a problem hiding this comment.
Transactional rollback never applies to the spawned server process.
Line 79 starts a transaction in the rake task, but spawn_server boots Rails in a separate process that opens its own DB connection. That child process never sees the parent’s transaction, so nothing is rolled back—data leaks between runs even though transactional_server defaults to true. Please move the transaction management into the server process itself (e.g., via Rack middleware or by running the server in-process with a shared connection), or disable this code path until it actually isolates state.
🤖 Prompt for AI Agents
In lib/cypress_on_rails/server.rb around lines 78 to 91, the code begins a DB
transaction in the parent rake process but the spawned server runs in a separate
process and therefore does not share that transaction; move transaction
management into the server process or disable it here. Remove or bypass the
ActiveRecord::Base.connection.begin_transaction/rollback_transaction calls in
the parent when spawn_server is used, and instead implement transactional
isolation inside the server process (for example add a Rack middleware that
begins a transaction on request start and rolls back on completion, or run the
server in-process with a shared DB connection so parent transactions apply), or
detect spawned-server mode and set @transactional = false to avoid the
misleading noop rollback. Ensure any change cleanly logs the chosen behavior and
keeps existing hooks (after_transaction_start/before_server_stop) consistent.
| def call(env) | ||
| if env['PATH_INFO'] == '/__cypress__/reset_state' || env['PATH_INFO'] == '/cypress_rails_reset_state' | ||
| reset_application_state | ||
| [200, { 'Content-Type' => 'text/plain' }, ['State reset completed']] | ||
| else | ||
| @app.call(env) | ||
| end |
There was a problem hiding this comment.
Require POST (or stronger) for state reset endpoint
As written, any GET/HEAD/etc. to /__cypress__/reset_state or /cypress_rails_reset_state will wipe the database. That means a stray <img> tag, CSRF, or an automated link scanner can nuke test data—exactly the class of security issue we’re trying to eliminate. We need to restrict resets to an explicit POST (and ideally return 405 for other verbs) before calling reset_application_state.
Apply this diff to enforce a POST guard:
- if env['PATH_INFO'] == '/__cypress__/reset_state' || env['PATH_INFO'] == '/cypress_rails_reset_state'
+ if reset_state_request?(env)
reset_application_state
[200, { 'Content-Type' => 'text/plain' }, ['State reset completed']]
else
@app.call(env)
end
end
+
+ def reset_state_request?(env)
+ return false unless env['REQUEST_METHOD'] == 'POST'
+ path = env['PATH_INFO']
+ path == '/__cypress__/reset_state' || path == '/cypress_rails_reset_state'
+ end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def call(env) | |
| if env['PATH_INFO'] == '/__cypress__/reset_state' || env['PATH_INFO'] == '/cypress_rails_reset_state' | |
| reset_application_state | |
| [200, { 'Content-Type' => 'text/plain' }, ['State reset completed']] | |
| else | |
| @app.call(env) | |
| end | |
| def call(env) | |
| if reset_state_request?(env) | |
| reset_application_state | |
| [200, { 'Content-Type' => 'text/plain' }, ['State reset completed']] | |
| else | |
| @app.call(env) | |
| end | |
| end | |
| def reset_state_request?(env) | |
| return false unless env['REQUEST_METHOD'] == 'POST' | |
| path = env['PATH_INFO'] | |
| path == '/__cypress__/reset_state' || path == '/cypress_rails_reset_state' | |
| end |
🤖 Prompt for AI Agents
In lib/cypress_on_rails/state_reset_middleware.rb around lines 7–13, the
middleware currently performs a reset for any HTTP verb; change it to only
perform reset when env['PATH_INFO'] matches the reset paths AND
env['REQUEST_METHOD'] == 'POST'. If the path matches but the method is not POST,
return a 405 response with an Allow: POST header and an appropriate plain-text
body; otherwise continue to @app.call(env). Ensure the code checks both paths as
before and only invokes reset_application_state for POST requests.
|
Closing this PR - these changes should be added to PR #179 instead |
Summary
Changes
Security Improvements
server.rbto prevent injection attacksspawn(*args)andsystem(*args)for safer executionDatabase Reset Improvements
state_reset_middleware.rbafter_state_resethook now runs after cleanup (as requested in review)disable_referential_integritywhen availablequote_table_nameOther Fixes
require 'fileutils'toserver.rbFile.expand_pathfor folder detectionTest Plan
bundle exec rspec)References
🤖 Generated with Claude Code
Summary by CodeRabbit