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

Fix Version Checker's hard-coded path for package.json #1657

Merged
merged 8 commits into from
Dec 8, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@ Please follow the recommendations outlined at [keepachangelog.com](http://keepac
### [Unreleased]
Changes since the last non-beta release.

#### Added(https://github.com/AbanoubGhadban).
#### Fixed

- Changed the ReactOnRails' version checker to check package.jsons in both root and client directories. [PR 1657](https://github.com/shakacode/react_on_rails/pull/1657) by [judahmeek](https://github.com/judahmeek).
Copy link
Member

Choose a reason for hiding this comment

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


#### Added
- Added streaming server rendering support:
- [PR #1633](https://github.com/shakacode/react_on_rails/pull/1633) by [AbanoubGhadban](https://github.com/AbanoubGhadban).
- New `stream_react_component` helper for adding streamed components to views
Expand Down
4 changes: 1 addition & 3 deletions lib/react_on_rails/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,7 @@
module ReactOnRails
class Engine < ::Rails::Engine
config.to_prepare do
if File.exist?(VersionChecker::NodePackageVersion.package_json_path)
VersionChecker.build.raise_if_gem_and_node_package_versions_differ
end
VersionChecker.build.log_if_gem_and_node_package_versions_differ
ReactOnRails::ServerRenderingPool.reset_pool
end
end
Expand Down
35 changes: 21 additions & 14 deletions lib/react_on_rails/version_checker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,25 +12,28 @@ def self.build
new(NodePackageVersion.build)
end

def self.instance(package_json_path)
justin808 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

do we need the arg?

can we just use node_modules_location ?

new(NodePackageVersion.new(package_json_path))
end
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding parameter validation for package_json_path

The new instance method accepts a path parameter without validation. This could lead to issues if invalid paths are provided.

 def self.instance(package_json_path)
+  raise ReactOnRails::Error, "package_json_path cannot be nil" if package_json_path.nil?
+  raise ReactOnRails::Error, "package_json_path must be a string" unless package_json_path.is_a?(String)
   new(NodePackageVersion.new(package_json_path))
 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.

Suggested change
def self.instance(package_json_path)
new(NodePackageVersion.new(package_json_path))
end
def self.instance(package_json_path)
raise ReactOnRails::Error, "package_json_path cannot be nil" if package_json_path.nil?
raise ReactOnRails::Error, "package_json_path must be a string" unless package_json_path.is_a?(String)
new(NodePackageVersion.new(package_json_path))
end


def initialize(node_package_version)
@node_package_version = node_package_version
end

# For compatibility, the gem and the node package versions should always match,
# unless the user really knows what they're doing. So we will give a
# warning if they do not.
def raise_if_gem_and_node_package_versions_differ
return if node_package_version.relative_path?
def log_if_gem_and_node_package_versions_differ
return if node_package_version.raw.nil? || node_package_version.relative_path?
Copy link
Member

Choose a reason for hiding this comment

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

return if node_package_version.raw.nil? || node_package_version.relative_path?

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because raw is nil if the package.json file doesn't exist.

Do you want us to create a warning for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just modified the code so that raw is nil if package.json or the react-on-rails dependency in the package.json is missing.

return log_node_semver_version_warning if node_package_version.semver_wildcard?

node_major_minor_patch = node_package_version.major_minor_patch
gem_major_minor_patch = gem_major_minor_patch_version
versions_match = node_major_minor_patch[0] == gem_major_minor_patch[0] &&
node_major_minor_patch[1] == gem_major_minor_patch[1] &&
node_major_minor_patch[2] == gem_major_minor_patch[2]

raise_differing_versions_warning unless versions_match

raise_node_semver_version_warning if node_package_version.semver_wildcard?
log_differing_versions_warning unless versions_match
end

private
Expand All @@ -46,15 +49,15 @@ def common_error_msg
MSG
end

def raise_differing_versions_warning
msg = "**ERROR** ReactOnRails: ReactOnRails gem and node package versions do not match\n#{common_error_msg}"
raise ReactOnRails::Error, msg
def log_differing_versions_warning
msg = "**WARNING** ReactOnRails: ReactOnRails gem and node package versions do not match\n#{common_error_msg}"
Rails.logger.warn(msg)
end

def raise_node_semver_version_warning
msg = "**ERROR** ReactOnRails: Your node package version for react-on-rails contains a " \
def log_node_semver_version_warning
msg = "**WARNING** ReactOnRails: Your node package version for react-on-rails contains a " \
"^ or ~\n#{common_error_msg}"
raise ReactOnRails::Error, msg
Rails.logger.warn(msg)
end

def gem_version
Expand All @@ -74,20 +77,24 @@ def self.build
end

def self.package_json_path
Rails.root.join("client", "package.json")
Rails.root.join(ReactOnRails.configuration.node_modules_location, "package.json")
Copy link
Member

Choose a reason for hiding this comment

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

could ReactOnRails.configuration.node_modules_location be nil and if so, what would happened?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default is an empty string, so it would have to be manually configured to nil.

If it is nil, that will cause a fatal error.

end

def initialize(package_json)
@package_json = package_json
end

def raw
return nil unless File.exist?(package_json)

parsed_package_contents = JSON.parse(package_json_contents)
if parsed_package_contents.key?("dependencies") &&
parsed_package_contents["dependencies"].key?("react-on-rails")
parsed_package_contents["dependencies"]["react-on-rails"]
else
raise ReactOnRails::Error, "No 'react-on-rails' entry in package.json dependencies"
msg = "No 'react-on-rails' entry in the dependencies of #{NodePackageVersion.package_json_path}, " \
"which is the expected location according to ReactOnRails.configuration.node_modules_location"
Rails.logger.warn(msg)
end
end

Expand All @@ -96,7 +103,7 @@ def semver_wildcard?
end

def relative_path?
raw.match(%r{(\.\.|\Afile:///)}).present?
raw.match(/(\.\.|\Afile:)/).present?
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Security: Strengthen relative path detection

The current regex allows potentially dangerous path patterns:

  1. .. could allow directory traversal
  2. file: protocol could allow access to arbitrary files

Consider using a more restrictive approach:

-  raw.match(/(\.\.|\Afile:)/).present?
+  raw.start_with?('file:./') || raw.start_with?('file:src/')

Committable suggestion skipped: line range outside the PR's diff.

end

def major_minor_patch
Expand Down
10 changes: 10 additions & 0 deletions spec/react_on_rails/fixtures/yalc_package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"dependencies": {
"babel": "^6.3.26",
"react-on-rails": "file:.yalc/react-on-rails",
"webpack": "^1.12.8"
},
"devDependencies": {
"babel-eslint": "^5.0.0-beta6"
}
}
100 changes: 82 additions & 18 deletions spec/react_on_rails/version_checker_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ def error(message)
end
end

module ReactOnRails
module ReactOnRails # rubocop:disable Metrics/ModuleLength
describe VersionChecker do
describe "#warn_if_gem_and_node_package_versions_differ" do
let(:logger) { FakeLogger.new }
Expand All @@ -23,8 +23,10 @@ module ReactOnRails

before { stub_gem_version("2.2.5.beta.2") }

it "does not raise" do
expect { check_version(node_package_version) }.not_to raise_error
it "does not log" do
allow(Rails.logger).to receive(:warn)
check_version_and_log(node_package_version)
expect(Rails.logger).not_to have_received(:warn)
end
end

Expand All @@ -35,9 +37,11 @@ module ReactOnRails

before { stub_gem_version("2.2.5") }

it "does raise" do
error = /ReactOnRails: Your node package version for react-on-rails contains a \^ or ~/
expect { check_version(node_package_version) }.to raise_error(error)
it "logs" do
allow(Rails.logger).to receive(:warn)
message = /ReactOnRails: Your node package version for react-on-rails contains a \^ or ~/
check_version_and_log(node_package_version)
expect(Rails.logger).to have_received(:warn).with(message)
end
end

Expand All @@ -48,9 +52,11 @@ module ReactOnRails

before { stub_gem_version("12.0.0.beta.1") }

it "raises" do
error = /ReactOnRails: ReactOnRails gem and node package versions do not match/
expect { check_version(node_package_version) }.to raise_error(error)
it "logs" do
allow(Rails.logger).to receive(:warn)
message = /ReactOnRails: ReactOnRails gem and node package versions do not match/
check_version_and_log(node_package_version)
expect(Rails.logger).to have_received(:warn).with(message)
end
end

Expand All @@ -61,9 +67,11 @@ module ReactOnRails

before { stub_gem_version("13.1.0") }

it "raises" do
error = /ReactOnRails: ReactOnRails gem and node package versions do not match/
expect { check_version(node_package_version) }.to raise_error(error)
it "logs" do
allow(Rails.logger).to receive(:warn)
message = /ReactOnRails: ReactOnRails gem and node package versions do not match/
check_version_and_log(node_package_version)
expect(Rails.logger).to have_received(:warn).with(message)
end
end

Expand All @@ -74,9 +82,11 @@ module ReactOnRails

before { stub_gem_version("13.0.0") }

it "raises" do
error = /ReactOnRails: ReactOnRails gem and node package versions do not match/
expect { check_version(node_package_version) }.to raise_error(error)
it "logs" do
allow(Rails.logger).to receive(:warn)
message = /ReactOnRails: ReactOnRails gem and node package versions do not match/
check_version_and_log(node_package_version)
expect(Rails.logger).to have_received(:warn).with(message)
end
end

Expand All @@ -87,8 +97,20 @@ module ReactOnRails

before { stub_gem_version("2.0.0.beta.1") }

it "does not raise" do
expect { check_version(node_package_version) }.not_to raise_error
it "does not log" do
allow(Rails.logger).to receive(:warn)
check_version_and_log(node_package_version)
expect(Rails.logger).not_to have_received(:warn)
end
end

context "when package json doesn't exist" do
let(:node_package_version) do
double_package_version(raw: nil)
end

it "log method returns nil" do
expect(check_version_and_log(node_package_version)).to be_nil
end
end
end
Expand All @@ -102,14 +124,32 @@ def double_package_version(raw: nil, semver_wildcard: false,
relative_path?: relative_path)
end

def check_version(node_package_version)
def check_version_and_raise(node_package_version)
version_checker = VersionChecker.new(node_package_version)
version_checker.raise_if_gem_and_node_package_versions_differ
end

def check_version_and_log(node_package_version)
version_checker = VersionChecker.new(node_package_version)
version_checker.log_if_gem_and_node_package_versions_differ
end

describe VersionChecker::NodePackageVersion do
subject(:node_package_version) { described_class.new(package_json) }

describe "#build" do
it "initializes NodePackageVersion with ReactOnRails.configuration.node_modules_location" do
allow(ReactOnRails).to receive_message_chain(:configuration, :node_modules_location).and_return("spec/dummy")
root_package_json_path = File.expand_path("../../package.json", __dir__)
allow(Rails).to receive_message_chain(:root, :join).and_return(root_package_json_path)
message = "No 'react-on-rails' entry in the dependencies of #{root_package_json_path}, which is " \
"the expected location according to ReactOnRails.configuration.node_modules_location"
allow(Rails.logger).to receive(:warn)
described_class.build.raw
expect(Rails.logger).to have_received(:warn).with(message)
end
end

describe "#semver_wildcard?" do
context "when package json lists an exact version of '0.0.2'" do
let(:package_json) { File.expand_path("fixtures/normal_package.json", __dir__) }
Expand Down Expand Up @@ -193,6 +233,30 @@ def check_version(node_package_version)
specify { expect(node_package_version.major_minor_patch).to be_nil }
end
end

context "with node version of 'file:.yalc/react-on-rails'" do
let(:package_json) { File.expand_path("fixtures/yalc_package.json", __dir__) }

describe "#raw" do
specify { expect(node_package_version.raw).to eq("file:.yalc/react-on-rails") }
end

describe "#relative_path?" do
specify { expect(node_package_version.relative_path?).to be true }
end

describe "#major" do
specify { expect(node_package_version.major_minor_patch).to be_nil }
end
end

context "with non-existing package.json" do
let(:package_json) { File.expand_path("fixtures/nonexistent_package.json", __dir__) }

describe "#raw" do
specify { expect(node_package_version.raw).to be_nil }
end
end
end
end
end
Loading