-
-
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 4 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").log_if_gem_and_node_package_versions_differ && | ||
| VersionChecker.instance("client/package.json").log_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 refactoring to reduce duplication and improve naming.
The raise_if_gem_and_node_package_versions_differ and log_if_gem_and_node_package_versions_differ methods have identical logic but different actions. Consider:
- Extracting the common logic into a private method
- Renaming methods to better reflect their behavior (they return boolean values)
-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?
-
- versions_match = compare_versions(node_package_version.major_minor_patch, gem_major_minor_patch_version)
-
- raise_differing_versions_warning unless versions_match
- false
-end
-
-def log_if_gem_and_node_package_versions_differ
- return true unless node_package_version.raw
- return if node_package_version.relative_path?
-
- log_node_semver_version_warning if node_package_version.semver_wildcard?
-
- versions_match = compare_versions(node_package_version.major_minor_patch, gem_major_minor_patch_version)
-
- log_differing_versions_warning unless versions_match
- false
-end
+def check_versions(action = :log)
+ return true unless node_package_version.raw
+ return if node_package_version.relative_path?
+
+ if node_package_version.semver_wildcard?
+ action == :log ? log_node_semver_version_warning : raise_node_semver_version_warning
+ end
+
+ versions_match = compare_versions(node_package_version.major_minor_patch, gem_major_minor_patch_version)
+
+ unless versions_match
+ action == :log ? log_differing_versions_warning : raise_differing_versions_warning
+ end
+ false
+end
+
+def check_versions_with_raise
+ check_versions(:raise)
+end
+
+def check_versions_with_log
+ check_versions(:log)
+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? | |
| 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_node_semver_version_warning if node_package_version.semver_wildcard? | |
| versions_match = compare_versions(node_package_version.major_minor_patch, gem_major_minor_patch_version) | |
| raise_differing_versions_warning unless versions_match | |
| false | |
| end | |
| raise_node_semver_version_warning if node_package_version.semver_wildcard? | |
| def log_if_gem_and_node_package_versions_differ | |
| return true unless node_package_version.raw | |
| return if node_package_version.relative_path? | |
| log_node_semver_version_warning if node_package_version.semver_wildcard? | |
| versions_match = compare_versions(node_package_version.major_minor_patch, gem_major_minor_patch_version) | |
| log_differing_versions_warning unless versions_match | |
| false | |
| def check_versions(action = :log) | |
| return true unless node_package_version.raw | |
| return if node_package_version.relative_path? | |
| if node_package_version.semver_wildcard? | |
| action == :log ? log_node_semver_version_warning : raise_node_semver_version_warning | |
| end | |
| versions_match = compare_versions(node_package_version.major_minor_patch, gem_major_minor_patch_version) | |
| unless versions_match | |
| action == :log ? log_differing_versions_warning : raise_differing_versions_warning | |
| end | |
| false | |
| end | |
| def check_versions_with_raise | |
| check_versions(:raise) | |
| end | |
| def check_versions_with_log | |
| check_versions(:log) | |
| 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.
@Judahmeek please commit this this suggestion.
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.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
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.
🛠️ 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