Add download to release files #4704

HazAT · 2016-12-21T15:19:54Z

getsentry-bot · 2016-12-21T15:20:32Z

	1 Warning
⚠️	Changes require @getsentry/security sign-off

Security concerns found

src/sentry/api/endpoints/release_file_details.py
src/sentry/static/sentry/app/views/releaseArtifacts.jsx

Generated by 🚫 danger

mitsuhiko · 2016-12-21T15:46:02Z

Looks good to me. Might need a CHANGES entry

mattrobenolt · 2016-12-21T15:46:51Z

No no, we explicitly cannot do this for security reasons. There are specific reasons why uploads are a one-way.

mattrobenolt

We need to surface this ability possibly only for superusers only or in our admin.

mitsuhiko · 2016-12-21T15:48:32Z

Can we put this behind a org admin permission / superuser flag or something like thatat least? It's super useful for support on react native

…

On Wed, Dec 21, 2016 at 16:46 Matt Robenolt ***@***.***> wrote: No no, we explicitly cannot do this for security reasons. There are specific reasons why uploads are a one-way. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#4704 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAc5EvrLgTy1cf0RICh9ethXInJot7Kks5rKUnrgaJpZM4LTDdy> .

mattrobenolt · 2016-12-21T15:51:53Z

Unsure what we wanted to do, but @dcramer and @benvinegar should weigh their thoughts.

I personally think behind superuser check is fine.

dcramer · 2016-12-21T17:30:54Z

What's the security concern?

mattrobenolt · 2016-12-21T17:34:28Z

What's the security concern?

We never wanted having access to downloading release artifacts through the UI since this would expose source code to people on the org who maybe didn't have access before.

dcramer · 2016-12-21T17:42:29Z

@mattrobenolt then we scope it to the same perms. They already have indirect access via source expansion.

mitsuhiko · 2016-12-21T17:44:00Z

@dcramer by same perms you mean project:releases?

dcramer · 2016-12-21T18:28:30Z

Yeah I think so -- will defer to others here, but its my feeling that these aren't highly sensitive, and should be accessible by the same users who have permission to create/delete them.

mitsuhiko · 2016-12-22T07:20:53Z

I would just say we add an explicit check for projects:releases or projects:write and if one of them passes we permit the download.

mattrobenolt · 2016-12-22T19:06:05Z

src/sentry/api/endpoints/release_file_details.py


+    def download(self, releasefile):
+        file = releasefile.file.getfile()
+        response = HttpResponse(FileWrapper(file), mimetype='application/octet-stream')


We should do the same thing that Django does here for static file delivery, since it's the same idea: https://github.com/django/django/blob/1.6.11/django/views/static.py#L67-L68

The reason for this is if it's this CompatibleStreamingHttpResponse, we can actually stream the bytes back to the browser instead of blocking process while it downloads in memory. This streaming behavior is then also handled directly by uWSGI and deferred away from Django.

This happens here: https://github.com/getsentry/sentry/blob/master/src/sentry/wsgi.py#L44

We should also extract the mimetype from file.headers.get('content-type', 'application/octet-stream') since we typically have this information.

To be more explicit, this last line could be:

file = releasefile.file fp = file.getfile() CompatibleStreamingHttpResponse( iter(lambda: fp.read(4096), b''), content_type=file.headers.get('content-type', 'application/octet-stream'), )

Oh I see. I thought HttpResponse already can handle this on its own but this just works by chance. Clever :P

mattrobenolt · 2016-12-22T19:09:02Z

src/sentry/api/endpoints/release_file_details.py

            raise ResourceDoesNotExist

+        if request.GET.get('download') is not None and (
+           request.access.has_scope('project:releases') or


We should stick with just project:write.

Checking for project:releases is kinda moot at this point anyways since literally any member has project:releases permission. But as of now, project:releases doesn't have any concept of read vs write, so this is effectively opening it up to everybody.

We should limit to project:write and document this behavior. We can always loosen constraint here if we have the need in the future. I'm hesitant on exposing this feature to people who potentially didn't have access to source code before, so I feel project:write is sufficient to start.

mattrobenolt · 2016-12-22T19:10:07Z

src/sentry/static/sentry/app/views/releaseArtifacts.jsx

              <div className="col-sm-2 col-xs-2 align-right"><FileSize bytes={file.size} /></div>
-              <div className="col-sm-1 col-xs-2 align-right">
+              <div className="col-sm-2 col-xs-3 align-right actions">
+                {(access.has('project:releases') || access.has('project:write')) && 


Same here for project:write.

According to @dcramer, which I agree with, we should leave the button here even if they don't have permission, but disable it with a tooltip saying that they don't have permission. That way we're not hiding things in the UI and they can see easily that they don't have permission to perform an action.

mitsuhiko · 2016-12-26T20:24:41Z

Just needs changes and a sign-off from @mattrobenolt i think.

mattrobenolt

You need to revert changes to the locale files.

mattrobenolt · 2016-12-26T20:42:16Z

src/sentry/api/endpoints/release_file_details.py


+        if request.GET.get('download') is not None and (
+           request.access.has_scope('project:write')):
+            return self.download(releasefile)


We should also probably return a 403 here when requesting a download but don't have have permission.

This reverts commit 8417c16.

HazAT · 2016-12-31T07:40:09Z

@mattrobenolt is there anything left to do?

mattrobenolt · 2016-12-31T08:35:13Z

We need some tests for this. They can be pretty trivial.

HazAT · 2017-01-01T19:23:55Z

Also wanted to write a api integration test but this
https://github.com/getsentry/sentry/pull/4704/files#diff-17bacf0f00a6813b38ad91e1b0efa99eR76
raises a raise ValueError('Cannot seek to pos')
Don't know how to have a seekable file for testing...

mitsuhiko · 2017-01-01T19:45:06Z

cStringIO.StringIO can seek over binary data. Not sure if that helps.

mattrobenolt · 2017-01-01T19:46:44Z

Can you commit the test code that fails? Hard to debug without seeing what you're doing. :)

mitsuhiko · 2017-01-01T19:59:06Z

tests/sentry/api/endpoints/test_release_file_details.py

+            organization_id=project.organization_id,
+            project=project,
+            release=release,
+            file=File.objects.create(


I think you need to create the file before the release file and then use putfile on it:

from io import BytesIO f = File.objects.create(...) f.putfile(BytesIO(b'File contents here')) ReleaseFIle.objects.create(..., file=f)

mattrobenolt · 2017-01-01T20:01:22Z

Yeah, that's it. :)

HazAT · 2017-01-01T20:38:09Z

Thx guys :)

mitsuhiko · 2017-01-01T20:42:41Z

Now only needs a CHANGES entry and this is ready to go.

mattrobenolt · 2017-01-02T04:51:32Z

tests/sentry/api/endpoints/test_release_file_details.py

+        })
+
+        response = self.client.get(url + '?download=1')
+        assert response.status_code == 200, response.content


It'd be worth adding an assertion for the contents of the payload here to match the BytesIO contents to confirm that we've actually downloaded the correct file.

As well as the correct headers. Content-Length, Content-Type, and Content-Disposition. Mostly because in this case, these things are actually significant and we wouldn't want to break them.

Other than this, lgtm.

HazAT · 2017-01-02T09:13:54Z

Squash Merge?

mitsuhiko · 2017-01-02T10:05:52Z

+1

Add download to release files

1375057

HazAT self-assigned this Dec 21, 2016

getsentry-bot added the Security label Dec 21, 2016

HazAT requested a review from mitsuhiko December 21, 2016 15:25

mitsuhiko approved these changes Dec 21, 2016

View reviewed changes

mattrobenolt suggested changes Dec 21, 2016

View reviewed changes

HazAT added 3 commits December 22, 2016 09:18

Add permission check for releasefile download

3c0f306

Remove console.log statement

a8065e0

Reformat if statement

b026567

mattrobenolt suggested changes Dec 22, 2016

View reviewed changes

HazAT added 3 commits December 23, 2016 13:50

Remove project:releases permission, Add streamed download, Add tooltip

b3da19b

Add translations

8417c16

Add context to tests

9d76c21

mattrobenolt suggested changes Dec 26, 2016

View reviewed changes

mattrobenolt reviewed Dec 26, 2016

View reviewed changes

HazAT added 2 commits December 26, 2016 21:45

Revert "Add translations"

629e3a1

This reverts commit 8417c16.

Return 403 response if download is not allowed

b1643a2

Add simple frontend tests for release artifact download

7c1857b

Add integration test for release artifact download

615ec7a

mitsuhiko suggested changes Jan 1, 2017

View reviewed changes

Fix integration test for artifact download

30c34e2

mitsuhiko approved these changes Jan 1, 2017

View reviewed changes

Added entry to CHANGES

b91b282

mattrobenolt approved these changes Jan 2, 2017

View reviewed changes

HazAT added 3 commits January 2, 2017 08:56

Add additional asserts for artifact download

394091e

Add content-type assert

f226f83

Fix content-type assert

ff4251a

HazAT merged commit c516cc8 into master Jan 2, 2017

HazAT deleted the feature/download-release-files branch January 2, 2017 10:36

github-actions bot locked and limited conversation to collaborators Dec 23, 2020

Uh oh!

Add download to release files #4704

Add download to release files #4704

Uh oh!

Conversation

HazAT commented Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

getsentry-bot commented Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security concerns found

Uh oh!

mitsuhiko commented Dec 21, 2016

Uh oh!

mattrobenolt commented Dec 21, 2016

Uh oh!

mattrobenolt left a comment

Choose a reason for hiding this comment

Uh oh!

mitsuhiko commented Dec 21, 2016 via email

Uh oh!

mattrobenolt commented Dec 21, 2016

Uh oh!

dcramer commented Dec 21, 2016 via email • edited by mitsuhiko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattrobenolt commented Dec 21, 2016

Uh oh!

dcramer commented Dec 21, 2016

Uh oh!

mitsuhiko commented Dec 21, 2016

Uh oh!

dcramer commented Dec 21, 2016

Uh oh!

mitsuhiko commented Dec 22, 2016

Uh oh!

mattrobenolt Dec 22, 2016

Choose a reason for hiding this comment

Uh oh!

mattrobenolt Dec 22, 2016

Choose a reason for hiding this comment

Uh oh!

mattrobenolt Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitsuhiko Dec 22, 2016

Choose a reason for hiding this comment

Uh oh!

mattrobenolt Dec 22, 2016

Choose a reason for hiding this comment

Uh oh!

mattrobenolt Dec 22, 2016

Choose a reason for hiding this comment

Uh oh!

mitsuhiko commented Dec 26, 2016

Uh oh!

mattrobenolt left a comment

Choose a reason for hiding this comment

Uh oh!

mattrobenolt Dec 26, 2016

Choose a reason for hiding this comment

Uh oh!

HazAT commented Dec 31, 2016

Uh oh!

mattrobenolt commented Dec 31, 2016

Uh oh!

HazAT commented Jan 1, 2017

Uh oh!

mitsuhiko commented Jan 1, 2017

Uh oh!

mattrobenolt commented Jan 1, 2017

Uh oh!

mitsuhiko Jan 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattrobenolt commented Jan 1, 2017 via email • edited by mitsuhiko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HazAT commented Dec 21, 2016 •

edited

Loading

getsentry-bot commented Dec 21, 2016 •

edited

Loading

dcramer commented Dec 21, 2016 via email •

edited by mitsuhiko

Loading

mattrobenolt Dec 22, 2016 •

edited

Loading

mitsuhiko Jan 1, 2017 •

edited

Loading

mattrobenolt commented Jan 1, 2017 via email •

edited by mitsuhiko

Loading