Browse volumes#678
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #678 +/- ##
==========================================
- Coverage 71.04% 70.79% -0.26%
==========================================
Files 470 471 +1
Lines 29992 30213 +221
Branches 816 822 +6
==========================================
+ Hits 21307 21388 +81
- Misses 8600 8740 +140
Partials 85 85
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
ryanmelt
left a comment
ryanmelt
left a comment
There was a problem hiding this comment.
Changes uses OPENC3_NAME_VOLUME environment as discuss and address path security issue found by CodeQL
| volume = ENV[params[:volume]] # Get the actual volume name | ||
| raise "Unknown volume #{params[:volume]}" unless volume | ||
| filename = "/#{volume}/#{params[:object_id]}" | ||
| file = File.read(filename, mode: 'rb') |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression
| metadata = params[:metadata].present? ? true : false | ||
| results = bucket.list_files(bucket: bucket_name, path: path, metadata: metadata) | ||
| root = ENV[params[:root]] # Get the actual bucket / volume name | ||
| raise "Unknown bucket / volume #{params[:root]}" unless root |
There was a problem hiding this comment.
Probably remove params[:root] from this. Could be used to expose any ENV variable.
There was a problem hiding this comment.
I'm simply passing back the parameters passed in the request. They get no additional information except that the environment variable doesn't exist.
| elsif params[:root].include?('_VOLUME') | ||
| dirs = [] | ||
| files = [] | ||
| list = Dir["/#{root}/#{params[:path].gsub('.', '')}/*"] |
There was a problem hiding this comment.
You could have . in a real directory name. Like .ssh. Need to do more of an absolute path kind of thing.
There was a problem hiding this comment.
Didn't think about that. I was trying to avoid going back with ../ or whatever. It's already an absolute path with the leading /. Trying to follow the recommendations here: https://codeql.github.com/codeql-query-help/java/java-path-injection/
There was a problem hiding this comment.
I guess its ok, just might burn us at some point in the future.
| return unless authorization('system') | ||
| volume = ENV[params[:volume]] # Get the actual volume name | ||
| raise "Unknown volume #{params[:volume]}" unless volume | ||
| filename = "/#{volume}/#{params[:object_id].gsub('.', '')}" |
|
Just need to fix playwright tests then. |
| dirs = [] | ||
| files = [] | ||
| path = sanitize_path(params[:path]) | ||
| list = Dir["/#{root}/#{path}/*"] # Ok for path to be blank |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression
3 participants
closes #500