-
Notifications
You must be signed in to change notification settings - Fork 51
Browse volumes #678
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
Browse volumes #678
Conversation
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. |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably remove params[:root] from this. Could be used to expose any ENV variable.
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'm simply passing back the parameters passed in the request. They get no additional information except that the environment variable doesn't exist.
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.
Ah ok.
elsif params[:root].include?('_VOLUME') | ||
dirs = [] | ||
files = [] | ||
list = Dir["/#{root}/#{params[:path].gsub('.', '')}/*"] |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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('.', '')}" |
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.
Same comment.
Just need to fix playwright tests then. |
closes #500