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

Uploads world readable #320

Closed
virtuaris opened this issue Nov 19, 2015 · 17 comments
Closed

Uploads world readable #320

virtuaris opened this issue Nov 19, 2015 · 17 comments

Comments

@virtuaris
Copy link

Hi there,
i just noticed that the contents stored in folder /home/zulip/uploads/files/ are readable to everyone. No session/authentication needed. Just head your browser to https://my-custom-zulip-server.mydomain.net/user_uploads/x/y/zzzzzzzzzzz/the_uploaded_file.something and off you go.
Is this the expected behaviour?
(zulip 1.3.7, ubuntu 14.04)
Bests
Harald

@timabbott
Copy link
Sponsor Member

This is expected behavior for the locally hosted file uploads, but we should definitely add documentation of this in the discussion of local file uploads vs. S3 hosting (which does have a model that checks auth and then redirects to a short-lived auth token).

I think the approach we're planning to take for fixing #291 would be the first step towards changing the way local file uploads work to match the S3 implementation.

@asmartin
Copy link

+1 keeping uploads private to just the user who posted it and the members of the stream or private message where the uploader posted it is important to my environment

@timabbott
Copy link
Sponsor Member

@asmartin is it possible for you to use the S3 integration in your environment?

@asmartin
Copy link

No, unfortunately all uploads must be hosted locally

@timabbott
Copy link
Sponsor Member

OK. A potential workaround is to use the S3 integration with something that implements the S3 API like Eucalyptis, but I agree the local file upload integration needs to support access controls.

@asmartin
Copy link

What about when an upload is done, an entry is created in the database with the upload filename and also a list of the users in the stream where the upload was made. Then rather than exposing the files directly, require that they be served by a script which checks this entry to make sure the requesting user is in the list. This would have the added benefit of ensuring that someone who got added to the stream after the fact could not get access to files from before they were a member. For private messages this is easy, since the only the other person in the private message would have access.

@timabbott
Copy link
Sponsor Member

Yeah, that's roughly the approach I had in mind. Implementation-wise, I'd do it in two phases:
(1) Make the local uploads integration controlled by Django and use the same codepath as the S3 integration.
(2) Change that codepath to track exactly which users have had the file shared with them.

@wama881
Copy link

wama881 commented Mar 16, 2016

Hi Tim, Is there a timeline when the two phases you mentioned above will be implemented? And is there a way to get all uploaded files for a specific stream? Thanks.

@timabbott
Copy link
Sponsor Member

There isn't a timeline -- most new feature development right now is driven by individual contributors deciding its worth working on an issue; we don't have a centralized planning process (though I'm hoping to set one up in the next month or two).

Right now, my time is currently absorbed with managing the 40 open pull requests we have due to Zulip being very popular with Google Summer of Code applicants. I've added a priority tag to this issue to make it clear I consider this one of the more important issues for someone to work on and as a reminder to myself; this will hopefully encourage GSOC students and other contributors looking for a project to work on this.

@timabbott timabbott added this to the 2016 roadmap milestone Apr 29, 2016
@timabbott
Copy link
Sponsor Member

Just as an FYI to the folks on this thread, there's ongoing work on this project here: #769

@timabbott
Copy link
Sponsor Member

Just a quick update, we just merged most of #769 into master. While there's more work planned as part of #769 to improve the performance of the new implementation (in particular, having nginx serve locally-uploaded files again, rather than Python), this does work now.

I just realized a bug in the new functionality, though; files uploaded before the Attachment model was introduced in 1.3.13 will be inaccessible with this version of the software. We'll discuss that on the #769 thread.

@timabbott
Copy link
Sponsor Member

Due to that bug, I reverted the enforcement commit from #769, so we can't close this quite yet. But I think it's likely we'll have this resolved for the next release!

@brainwane
Copy link
Contributor

I ran into this issue today.

@timabbott timabbott modified the milestones: Zulip roadmap, Old roadmap Nov 18, 2016
timabbott pushed a commit to timabbott/zulip that referenced this issue Feb 8, 2017
This is a remerge of e985b57 (after
resolving merge conflicts, updating the tests, adding mypy annotations
etc.), which should now be correct, because we've done the necessary
database migration.

This fixes an important part of zulip#320.
timabbott pushed a commit to timabbott/zulip that referenced this issue Feb 9, 2017
This is a remerge of e985b57 (after
resolving merge conflicts, updating the tests, adding mypy annotations
etc.), which should now be correct, because we've done the necessary
database migration.

This fixes an important part of zulip#320.
timabbott pushed a commit to timabbott/zulip that referenced this issue Feb 9, 2017
This is a remerge of e985b57 (after
resolving merge conflicts, updating the tests, adding mypy annotations
etc.), which should now be correct, because we've done the necessary
database migration.

This is an important part of zulip#320.
timabbott pushed a commit to timabbott/zulip that referenced this issue Feb 9, 2017
This is a remerge of e985b57 (after
resolving merge conflicts, updating the tests, adding mypy annotations
etc.), which should now be correct, because we've done the necessary
database migration.

This is an important part of zulip#320.
@adnrs96
Copy link
Member

adnrs96 commented Apr 2, 2017

@zulipbot claim.

adnrs96 pushed a commit to adnrs96/zulip that referenced this issue Apr 2, 2017
This is a remerge of e985b57 (after
resolving merge conflicts, updating the tests, adding mypy annotations
etc.), which should now be correct, because we've done the necessary
database migration.

This is an important part of zulip#320.
adnrs96 pushed a commit to adnrs96/zulip that referenced this issue Apr 2, 2017
This is a remerge of e985b57 (after
resolving merge conflicts, updating the tests, adding mypy annotations
etc.), which should now be correct, because we've done the necessary
database migration.

This is an important part of zulip#320.
adnrs96 added a commit to adnrs96/zulip that referenced this issue Apr 4, 2017
This commit will be introducing support for using django-sendfile for
serving uploads (attachments) when using LocalStorage. For the dev
environment the django backend will be used but for the Production nginx
will be used to serve files but after django performs authentication
checks.

Fixes: zulip#320, zulip#291.
timabbott pushed a commit that referenced this issue Apr 7, 2017
This is a remerge of e985b57 (after
resolving merge conflicts, updating the tests, adding mypy annotations
etc.), which should now be correct, because we've done the necessary
database migration.

The rebase/remerge work was done by Tim Abbott and Aditya Bansal.

This is an important part of #320.
sarahcstringer pushed a commit to sarahcstringer/zulip that referenced this issue Apr 8, 2017
This is a remerge of e985b57 (after
resolving merge conflicts, updating the tests, adding mypy annotations
etc.), which should now be correct, because we've done the necessary
database migration.

The rebase/remerge work was done by Tim Abbott and Aditya Bansal.

This is an important part of zulip#320.
adnrs96 added a commit to adnrs96/zulip that referenced this issue Apr 16, 2017
This commit will be introducing support for using django-sendfile for
serving uploads (attachments) when using LocalStorage. For the dev
environment the django backend will be used but for the Production nginx
will be used to serve files but after django performs authentication
checks.

Fixes: zulip#320, zulip#291.
adnrs96 added a commit to adnrs96/zulip that referenced this issue Apr 16, 2017
This commit will be introducing support for using django-sendfile for
serving uploads (attachments) when using LocalStorage. For the dev
environment the django backend will be used but for the Production nginx
will be used to serve files but after django performs authentication
checks.

Fixes: zulip#320, zulip#291.
adnrs96 added a commit to adnrs96/zulip that referenced this issue Apr 24, 2017
This commit will be introducing support for using django-sendfile for
serving uploads (attachments) when using LocalStorage. For the dev
environment the django backend will be used but for the Production nginx
will be used to serve files but after django performs authentication
checks.

Fixes: zulip#320, zulip#291.
adnrs96 added a commit to adnrs96/zulip that referenced this issue Apr 26, 2017
This commit will be introducing support for using django-sendfile for
serving uploads (attachments) when using LocalStorage. For the dev
environment the django backend will be used but for the Production nginx
will be used to serve files but after django performs authentication
checks.

Fixes: zulip#320, zulip#291.
adnrs96 added a commit to adnrs96/zulip that referenced this issue May 13, 2017
This commit will be introducing support for using django-sendfile for
serving uploads (attachments) when using LocalStorage. For the dev
environment the django backend will be used but for the Production nginx
will be used to serve files but after django performs authentication
checks.

Fixes: zulip#320, zulip#291.
adnrs96 added a commit to adnrs96/zulip that referenced this issue May 13, 2017
This commit will be introducing support for using django-sendfile for
serving uploads (attachments) when using LocalStorage. For the dev
environment the django backend will be used but for the Production nginx
will be used to serve files but after django performs authentication
checks.

Fixes: zulip#320, zulip#291.
adnrs96 added a commit to adnrs96/zulip that referenced this issue May 14, 2017
This commit will be introducing support for using django-sendfile for
serving uploads (attachments) when using LocalStorage. For the dev
environment the django backend will be used but for the Production nginx
will be used to serve files but after django performs authentication
checks.

Fixes: zulip#320, zulip#291.
adnrs96 added a commit to adnrs96/zulip that referenced this issue May 14, 2017
This commit will be introducing support for using django-sendfile for
serving uploads (attachments) when using LocalStorage. For the dev
environment the django backend will be used but for the Production nginx
will be used to serve files but after django performs authentication
checks.

Fixes: zulip#320, zulip#291.
@zulipbot
Copy link
Member

Hello @adnrs96, you claimed this issue to work on it, but this issue and any referenced pull requests haven't been updated for a week. Are you still working on this issue?

If so, please update this issue by leaving a comment on this issue to let me know that you're still working on it. Otherwise, I'll automatically remove you from this issue in 3 days.

If you've decided to work on something else, simply comment @zulipbot abandon so that someone else can claim it and continue from where you left off.

Thank you for your valuable contributions to Zulip!

@zulipbot
Copy link
Member

zulipbot commented Jun 8, 2017

Hello @adnrs96, you have been unassigned from this issue because you have not updated this issue or any referenced pull requests for over ten days.

You can reclaim this issue or claim any other issue by commenting @zulipbot claim on that issue.

Thanks for your contributions, and hope to see you again soon!

adnrs96 added a commit to adnrs96/zulip that referenced this issue Feb 12, 2018
From here on we start to authenticate uploaded file request before
serving this files in production. This involves allowing NGINX to
pass on these file requests to Django for authentication and then
serve these files by making use on internal redirect requests having
x-accel-redirect field. The redirection on requests and loading
of x-accel-redirect param is handled by django-sendfile.

NOTE: This commit starts to authenticate these requests for Zulip
servers running platforms either Ubuntu Xenial (16.04) or above.

Fixes: zulip#320 and zulip#291 partially.
adnrs96 added a commit to adnrs96/zulip that referenced this issue Feb 13, 2018
From here on we start to authenticate uploaded file request before
serving this files in production. This involves allowing NGINX to
pass on these file requests to Django for authentication and then
serve these files by making use on internal redirect requests having
x-accel-redirect field. The redirection on requests and loading
of x-accel-redirect param is handled by django-sendfile.

NOTE: This commit starts to authenticate these requests for Zulip
servers running platforms either Ubuntu Xenial (16.04) or above.

Fixes: zulip#320 and zulip#291 partially.
aero31aero pushed a commit to aero31aero/zulip that referenced this issue Feb 13, 2018
From here on we start to authenticate uploaded file request before
serving this files in production. This involves allowing NGINX to
pass on these file requests to Django for authentication and then
serve these files by making use on internal redirect requests having
x-accel-redirect field. The redirection on requests and loading
of x-accel-redirect param is handled by django-sendfile.

NOTE: This commit starts to authenticate these requests for Zulip
servers running platforms either Ubuntu Xenial (16.04) or above.

Fixes: zulip#320 and zulip#291 partially.
adnrs96 added a commit to adnrs96/zulip that referenced this issue Feb 14, 2018
From here on we start to authenticate uploaded file request before
serving this files in production. This involves allowing NGINX to
pass on these file requests to Django for authentication and then
serve these files by making use on internal redirect requests having
x-accel-redirect field. The redirection on requests and loading
of x-accel-redirect param is handled by django-sendfile.

NOTE: This commit starts to authenticate these requests for Zulip
servers running platforms either Ubuntu Xenial (16.04) or above.

Fixes: zulip#320 and zulip#291 partially.
timabbott pushed a commit to timabbott/zulip that referenced this issue Feb 15, 2018
From here on we start to authenticate uploaded file request before
serving this files in production. This involves allowing NGINX to
pass on these file requests to Django for authentication and then
serve these files by making use on internal redirect requests having
x-accel-redirect field. The redirection on requests and loading
of x-accel-redirect param is handled by django-sendfile.

NOTE: This commit starts to authenticate these requests for Zulip
servers running platforms either Ubuntu Xenial (16.04) or above.

Fixes: zulip#320 and zulip#291 partially.
adnrs96 added a commit to adnrs96/zulip that referenced this issue Feb 15, 2018
From here on we start to authenticate uploaded file request before
serving this files in production. This involves allowing NGINX to
pass on these file requests to Django for authentication and then
serve these files by making use on internal redirect requests having
x-accel-redirect field. The redirection on requests and loading
of x-accel-redirect param is handled by django-sendfile.

NOTE: This commit starts to authenticate these requests for Zulip
servers running platforms either Ubuntu Xenial (16.04) or above.

Fixes: zulip#320 and zulip#291 partially.
@timabbott
Copy link
Sponsor Member

This was fixed via #8357 for Ubuntu Xenial. A bit more work is required to support it on Trusty; whether we do that will depend on how soon we deprecate Trusty support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants