-
-
Notifications
You must be signed in to change notification settings - Fork 637
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 3 commits
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.instance("package.json").raise_if_gem_and_node_package_versions_differ && | ||
| VersionChecker.instance("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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -12,6 +12,10 @@ def self.build | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| new(NodePackageVersion.build) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def self.instance(package_json_path) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
justin808 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| new(NodePackageVersion.new(package_json_path)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 |
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.
is anybody code still calling this one?
we'll need a reminder for the next big update? Maybe leave a PR pending?
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.
We can create a follow-up PR and leave it pending.
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
Consider renaming method to better reflect its behavior
The method name raise_if_gem_and_node_package_versions_differ suggests it will raise an exception, but it now logs warnings and returns a boolean. Consider renaming to something like check_gem_and_node_package_versions or verify_version_match to better reflect its current behavior.
Additionally, the false return value on line 35 seems arbitrary and its purpose is unclear.
-def raise_if_gem_and_node_package_versions_differ
+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
- false
+ versions_match
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.
| 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" | ||
| } | ||
| } |
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