-
-
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 6 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 | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -12,25 +12,28 @@ 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 |
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.
return if node_package_version.raw.nil? || node_package_version.relative_path?
why?
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.
Because raw is nil if the package.json file doesn't exist.
Do you want us to create a warning for that?
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.
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.
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.
could ReactOnRails.configuration.node_modules_location be nil and if so, what would happened?
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.
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.
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