Skip to content
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

fix: DEV-3212: Add validation to avoid users import local files using URL #2840

Merged
merged 3 commits into from
Aug 23, 2022

Conversation

guilhermemachado26
Copy link
Contributor

Description of the proposed changes

LS users are able to use data import functionality to access local files on the running system, including environment variables, etc. This is achieved by using file:// url schema (ie. file:///etc/passwd).

Submitting a URL like this will cause Label Studio to fetch the local file and add it to the project, which can then be accessed in the /data/uploads folder.

Jira Ticket

https://heartex.atlassian.net/browse/DEV-3212

@guilhermemachado26 guilhermemachado26 self-assigned this Aug 17, 2022
@swarmia
Copy link

swarmia bot commented Aug 17, 2022

@github-actions github-actions bot added the fix label Aug 17, 2022
@farioas
Copy link
Member

farioas commented Aug 17, 2022

This is not the case. Now users lost ability to import files from local filesystem. I'd like to suggest restrict usage of "file://" to label-studio DATA_DIR or HOME_DIR

@makseq what do you think?

@codecov
Copy link

codecov bot commented Aug 17, 2022

Codecov Report

Merging #2840 (14c58a1) into develop (17b40da) will decrease coverage by 0.75%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2840      +/-   ##
===========================================
- Coverage    77.80%   77.04%   -0.76%     
===========================================
  Files          137      144       +7     
  Lines         9951    10601     +650     
===========================================
+ Hits          7742     8168     +426     
- Misses        2209     2433     +224     
Impacted Files Coverage Δ
label_studio/label_studio/users/mixins.py 75.00% <0.00%> (-12.50%) ⬇️
...bel_studio/label_studio/organizations/functions.py 66.66% <0.00%> (-10.26%) ⬇️
...l_studio/label_studio/core/templatetags/filters.py 48.75% <0.00%> (-6.20%) ⬇️
..._studio/label_studio/core/settings/label_studio.py 94.28% <0.00%> (-5.72%) ⬇️
.../label_studio/data_manager/actions/experimental.py 14.79% <0.00%> (-4.72%) ⬇️
label_studio/label_studio/tasks/api.py 88.26% <0.00%> (-4.43%) ⬇️
label_studio/label_studio/ml/api.py 86.17% <0.00%> (-4.38%) ⬇️
label_studio/label_studio/ml/serializers.py 92.30% <0.00%> (-2.43%) ⬇️
label_studio/label_studio/core/utils/contextlog.py 44.55% <0.00%> (-2.33%) ⬇️
label_studio/label_studio/tasks/serializers.py 84.27% <0.00%> (-2.27%) ⬇️
... and 60 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@makseq
Copy link
Member

makseq commented Aug 18, 2022

This is not the case. Now users lost ability to import files from local filesystem. I'd like to suggest restrict usage of "file://" to label-studio DATA_DIR or HOME_DIR

@makseq what do you think?

We have Local Storage functionality for file access on hard drives. It works the same way as you described, so I think it's ok to restrict file:// access here. Local Storage is more controllable and obvious way to do it.

@guilhermemachado26 guilhermemachado26 marked this pull request as ready for review August 23, 2022 20:53
@guilhermemachado26 guilhermemachado26 merged commit 501142c into develop Aug 23, 2022
@guilhermemachado26 guilhermemachado26 deleted the fb-dev-3212 branch August 23, 2022 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants