-
-
Notifications
You must be signed in to change notification settings - Fork 638
Fix Version Checker's hard-coded path for package.json #1657
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
Changes from 1 commit
ceb80a0
5c2164c
d066e37
9a07b64
a64c1b5
e073587
45cb6df
ad2d79d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,8 +5,9 @@ | |
| 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 | ||
| if VersionChecker.new(NodePackageVersion.new("package.json")).raise_if_gem_and_node_package_versions_differ && | ||
| VersionChecker.new(NodePackageVersion.new("client/package.json")).raise_if_gem_and_node_package_versions_differ | ||
| Rails.logger.warn("No 'react-on-rails' entry found in 'dependencies' in package.json or client/package.json.") | ||
|
||
| end | ||
| ReactOnRails::ServerRenderingPool.reset_pool | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -20,17 +20,19 @@ def initialize(node_package_version) | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # 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 true unless node_package_version.raw | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return if node_package_version.relative_path? | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise_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? | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| false | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return true unless node_package_version.raw | |
| return if node_package_version.relative_path? | |
| raise_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? | |
| false | |
| def check_gem_and_node_package_versions | |
| return true unless node_package_version.raw | |
| return if node_package_version.relative_path? | |
| raise_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 | |
| versions_match | |
| end |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Review warning message implementation
The current implementation:
- Always displays verbose warning messages
- Includes a note about fatal errors in v15, which might cause unnecessary concern
- Logs warnings for both version mismatches and semver wildcards
Consider:
- Making warning messages configurable or less verbose
- Removing the v15 warning if it's not immediately relevant
- Adding a debug log level for detailed information
Also applies to: 53-54, 58-60
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
method names can't be raise, but mean log
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplify warning messages per feedback
The current warning messages are still verbose and the v15 warning may cause unnecessary concern. Per justin808's feedback, these should be more concise.
def common_error_msg
<<-MSG.strip_heredoc
Detected: #{node_package_version.raw}
gem: #{gem_version}
- Ensure the installed version of the gem is the same as the version of
- your installed node package. Do not use >= or ~> in your Gemfile for react_on_rails.
- Do not use ^ or ~ in your package.json for react-on-rails.
- Run `yarn add react-on-rails --exact` in the directory containing folder node_modules.
- ***This warning will become a fatal error in ReactOnRails v15***
+ To fix: Run `yarn add react-on-rails@#{gem_version} --exact`
MSG
end
def raise_differing_versions_warning
- msg = "**WARNING** ReactOnRails: ReactOnRails gem and node package versions do not match\n#{common_error_msg}"
+ msg = "Version mismatch between gem and node package\n#{common_error_msg}"
Rails.logger.warn(msg)
end
def raise_node_semver_version_warning
- msg = "**WARNING** ReactOnRails: Your node package version for react-on-rails contains a " \
- "^ or ~\n#{common_error_msg}"
+ msg = "Semver wildcards not supported in package.json\n#{common_error_msg}"
Rails.logger.warn(msg)
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.
| ***This warning will become a fatal error in ReactOnRails v15*** | |
| 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 | |
| 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 " \ | |
| 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) | |
| def common_error_msg | |
| <<-MSG.strip_heredoc | |
| Detected: #{node_package_version.raw} | |
| gem: #{gem_version} | |
| To fix: Run `yarn add react-on-rails@#{gem_version} --exact` | |
| MSG | |
| end | |
| def raise_differing_versions_warning | |
| msg = "Version mismatch between gem and node package\n#{common_error_msg}" | |
| Rails.logger.warn(msg) | |
| end | |
| def raise_node_semver_version_warning | |
| msg = "Semver wildcards not supported in package.json\n#{common_error_msg}" | |
| Rails.logger.warn(msg) | |
| end |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add path validation for package.json
The method accepts any relative path without validation. Consider adding checks to:
- Ensure the path ends with 'package.json'
- Validate that the path doesn't escape the Rails root directory
def self.package_json_path(relative_path = "package.json")
+ unless relative_path.end_with?("package.json")
+ raise ReactOnRails::Error, "Path must end with package.json"
+ end
+ path = Rails.root.join(relative_path)
+ unless path.to_s.start_with?(Rails.root.to_s)
+ raise ReactOnRails::Error, "Path must be within Rails root directory"
+ end
- Rails.root.join(relative_path)
+ 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.
| def self.package_json_path(relative_path = "package.json") | |
| Rails.root.join(relative_path) | |
| def self.package_json_path(relative_path = "package.json") | |
| unless relative_path.end_with?("package.json") | |
| raise ReactOnRails::Error, "Path must end with package.json" | |
| end | |
| path = Rails.root.join(relative_path) | |
| unless path.to_s.start_with?(Rails.root.to_s) | |
| raise ReactOnRails::Error, "Path must be within Rails root directory" | |
| end | |
| path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security: Strengthen relative path detection
The current regex allows potentially dangerous path patterns:
..could allow directory traversalfile: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.
| 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" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,9 +35,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 ~/ | ||
| # expect { check_version(node_package_version) }.to raise_error(message) | ||
| expect(Rails.logger).to have_received(:warn).with(message) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Clean up and improve test expectations.
Here's a suggested refactor to make the tests more maintainable: - # expect { check_version(node_package_version) }.to raise_error(message)
- expect(Rails.logger).to have_received(:warn).with(message)
+ shared_examples "version mismatch logging" do |version_type|
+ it "logs version mismatch warning" do
+ allow(Rails.logger).to receive(:warn)
+ check_version(node_package_version)
+ expect(Rails.logger).to have_received(:warn).with(
+ a_string_matching(/#{expected_message}/)
+ )
+ end
+ end
+ context "when major versions differ" do
+ let(:expected_message) { "ReactOnRails: ReactOnRails gem and node package versions do not match" }
+ include_examples "version mismatch logging", "major"
+ endAlso applies to: 53-57, 68-72, 83-87 |
||
| end | ||
| end | ||
|
|
||
|
|
@@ -48,9 +50,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/ | ||
| # expect { check_version(node_package_version) }.to raise_error(message) | ||
| expect(Rails.logger).to have_received(:warn).with(message) | ||
| end | ||
| end | ||
|
|
||
|
|
@@ -61,9 +65,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/ | ||
| # expect { check_version(node_package_version) }.to raise_error(message) | ||
| expect(Rails.logger).to have_received(:warn).with(message) | ||
| end | ||
| end | ||
|
|
||
|
|
@@ -74,9 +80,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/ | ||
| # expect { check_version(node_package_version) }.to raise_error(message) | ||
| expect(Rails.logger).to have_received(:warn).with(message) | ||
| end | ||
| end | ||
|
|
||
|
|
@@ -193,6 +201,22 @@ 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 | ||
| end | ||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be using
node_modules_location.https://github.com/search?q=repo%3Ashakacode%2Freact_on_rails%20node_modules_location&type=code