Skip to content

Commit 59cd828

Browse files
justin808claude
andcommitted
Fix Playwright E2E test security issues and code quality
Address critical security vulnerabilities and code quality issues in Playwright E2E test infrastructure: Security Improvements: - Add test environment guard to eval.rb to prevent RCE in non-test envs - Bind Rails test server to 127.0.0.1 explicitly to prevent network exposure Code Quality Fixes: - Fix undefined logger references in clean.rb and activerecord_fixtures.rb - Improve error handling in on-rails.js with descriptive error messages - Remove parameter mutation in appVcrInsertCassette to avoid eslint disables - Remove dead code from activerecord_fixtures.rb fallback branch - Fix response.body to response.json() for proper API response parsing - Remove unused expect import after error handling refactor - Remove unused eslint-disable directives in Pro package CI/CD: - Replace npm with yarn for yalc installation per project guidelines All changes pass rubocop and eslint with zero violations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent 34461c8 commit 59cd828

File tree

8 files changed

+42
-35
lines changed

8 files changed

+42
-35
lines changed

.github/workflows/playwright.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ jobs:
2323
cache: 'yarn'
2424

2525
- name: Install yalc globally
26-
run: npm install -g yalc
26+
run: yarn global add yalc
2727

2828
- name: Install root dependencies
2929
run: yarn install

packages/react-on-rails-pro/src/ClientSideRenderer.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,6 @@ You should return a React.Component always for the client side entry point.`);
188188
}
189189

190190
try {
191-
// eslint-disable-next-line @typescript-eslint/no-deprecated
192191
unmountComponentAtNode(domNode);
193192
} catch (e: unknown) {
194193
const error = e instanceof Error ? e : new Error('Unknown error');

packages/react-on-rails-pro/src/createReactOnRailsPro.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -145,13 +145,11 @@ export default function createReactOnRailsPro(
145145

146146
if (reactOnRailsPro.streamServerRenderedReactComponent) {
147147
reactOnRailsProSpecificFunctions.streamServerRenderedReactComponent =
148-
// eslint-disable-next-line @typescript-eslint/unbound-method
149148
reactOnRailsPro.streamServerRenderedReactComponent;
150149
}
151150

152151
if (reactOnRailsPro.serverRenderRSCReactComponent) {
153152
reactOnRailsProSpecificFunctions.serverRenderRSCReactComponent =
154-
// eslint-disable-next-line @typescript-eslint/unbound-method
155153
reactOnRailsPro.serverRenderRSCReactComponent;
156154
}
157155

spec/dummy/e2e/playwright.config.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ module.exports = defineConfig({
7171

7272
/* Run your local dev server before starting the tests */
7373
webServer: {
74-
command: 'RAILS_ENV=test bundle exec rails server -p 5017',
74+
command: 'RAILS_ENV=test bundle exec rails server -b 127.0.0.1 -p 5017',
7575
url: 'http://127.0.0.1:5017',
7676
reuseExistingServer: !process.env.CI,
7777
timeout: 120 * 1000,

spec/dummy/e2e/playwright/app_commands/activerecord_fixtures.rb

Lines changed: 10 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,23 +2,17 @@
22

33
# you can delete this file if you don't use Rails Test Fixtures
44

5+
raise "ActiveRecord is not defined. Unable to load fixtures." unless defined?(ActiveRecord)
6+
7+
require "active_record/fixtures"
8+
59
fixtures_dir = command_options.try(:[], "fixtures_dir")
610
fixture_files = command_options.try(:[], "fixtures")
711

8-
if defined?(ActiveRecord)
9-
require "active_record/fixtures"
10-
11-
fixtures_dir ||= ActiveRecord::Tasks::DatabaseTasks.fixtures_path
12-
fixture_files ||= Dir["#{fixtures_dir}/**/*.yml"].map { |f| f[(fixtures_dir.size + 1)..-5] }
12+
fixtures_dir ||= ActiveRecord::Tasks::DatabaseTasks.fixtures_path
13+
fixture_files ||= Dir["#{fixtures_dir}/**/*.yml"].map { |f| f[(fixtures_dir.size + 1)..-5] }
1314

14-
logger.debug "loading fixtures: { dir: #{fixtures_dir}, files: #{fixture_files} }"
15-
ActiveRecord::FixtureSet.reset_cache
16-
ActiveRecord::FixtureSet.create_fixtures(fixtures_dir, fixture_files)
17-
"Fixtures Done" # this gets returned
18-
else # this else part can be removed
19-
logger.error "Looks like activerecord_fixtures has to be modified to suite your need"
20-
Post.create(title: "MyCypressFixtures")
21-
Post.create(title: "MyCypressFixtures2")
22-
Post.create(title: "MyRailsFixtures")
23-
Post.create(title: "MyRailsFixtures2")
24-
end
15+
Rails.logger.debug "loading fixtures: { dir: #{fixtures_dir}, files: #{fixture_files} }"
16+
ActiveRecord::FixtureSet.reset_cache
17+
ActiveRecord::FixtureSet.create_fixtures(fixtures_dir, fixture_files)
18+
"Fixtures Done" # this gets returned

spec/dummy/e2e/playwright/app_commands/clean.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
DatabaseCleaner.strategy = :truncation
66
DatabaseCleaner.clean
77
else
8-
logger.warn "add database_cleaner or update cypress/app_commands/clean.rb"
8+
Rails.logger.warn "add database_cleaner or update cypress/app_commands/clean.rb"
99
Post.delete_all if defined?(Post)
1010
end
1111

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
11
# frozen_string_literal: true
22

3+
raise "eval command is only available in test environment" unless Rails.env.test?
4+
35
Kernel.eval(command_options) unless command_options.nil?
Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { request, expect } from '@playwright/test';
1+
import { request } from '@playwright/test';
22
import config from '../../playwright.config';
33

44
const contextPromise = request.newContext({
@@ -9,8 +9,12 @@ const appCommands = async (data) => {
99
const context = await contextPromise;
1010
const response = await context.post('/__e2e__/command', { data });
1111

12-
expect(response.ok()).toBeTruthy();
13-
return response.body;
12+
if (!response.ok()) {
13+
const text = await response.text();
14+
throw new Error(`Rails command '${data.name}' failed: ${response.status()} - ${text}`);
15+
}
16+
17+
return response.json();
1418
};
1519

1620
const app = (name, options = {}) => appCommands({ name, options }).then((body) => body[0]);
@@ -20,22 +24,32 @@ const appFactories = (options) => app('factory_bot', options);
2024

2125
const appVcrInsertCassette = async (cassetteName, options) => {
2226
const context = await contextPromise;
23-
// eslint-disable-next-line no-param-reassign
24-
if (!options) options = {};
25-
26-
// eslint-disable-next-line no-param-reassign
27-
Object.keys(options).forEach((key) => (options[key] === undefined ? delete options[key] : {}));
28-
const response = await context.post('/__e2e__/vcr/insert', { data: [cassetteName, options] });
29-
expect(response.ok()).toBeTruthy();
30-
return response.body;
27+
const normalizedOptions = options || {};
28+
const cleanedOptions = Object.fromEntries(
29+
Object.entries(normalizedOptions).filter(([, value]) => value !== undefined),
30+
);
31+
32+
const response = await context.post('/__e2e__/vcr/insert', { data: [cassetteName, cleanedOptions] });
33+
34+
if (!response.ok()) {
35+
const text = await response.text();
36+
throw new Error(`VCR insert cassette '${cassetteName}' failed: ${response.status()} - ${text}`);
37+
}
38+
39+
return response.json();
3140
};
3241

3342
const appVcrEjectCassette = async () => {
3443
const context = await contextPromise;
3544

3645
const response = await context.post('/__e2e__/vcr/eject');
37-
expect(response.ok()).toBeTruthy();
38-
return response.body;
46+
47+
if (!response.ok()) {
48+
const text = await response.text();
49+
throw new Error(`VCR eject cassette failed: ${response.status()} - ${text}`);
50+
}
51+
52+
return response.json();
3953
};
4054

4155
export { appCommands, app, appScenario, appEval, appFactories, appVcrInsertCassette, appVcrEjectCassette };

0 commit comments

Comments
 (0)