Fixes #2659 - Make console logs in browser configuration details easier to read #3103

ksy36 · 2019-12-03T20:49:38Z

…issue body

tests/unit/test_console_logs.py

webcompat/api/console_logs.py

miketaylr · 2019-12-05T21:56:42Z

webcompat/api/console_logs.py

+
+from webcompat import app
+
+console_logs = Blueprint('console_logs', __name__, url_prefix='/console_logs')


I'm not sure I have a super strong opinion, but is there a reason you didn't just add support for uploading JSON content in the /uploads route? What are the advantages of having a new route to handle uploads of a different kind of file type?

I agree. We should probably make the code versatile enough so that it can handle images and json.So it would be probably better to have the uploads route (blueprint) to behave differently with the content-type. So depending on the Content-Type, there is a validation process depending probably on the content of the file, size, etc. Like we do already for images.

Thanks for the feedback. I've moved console log saving to uploads blueprint, could you please take another look @miketaylr @karlcow

webcompat/templates/console-logs.html

karlcow

@ksy36 Thanks a lot for working on this.
So sorry for the delay in reviewing.
A couple of things in there and Probably a big change on making the Upload more generic.

karlcow · 2019-12-06T06:21:02Z

webcompat/api/console_logs.py

+
+from webcompat import app
+
+console_logs = Blueprint('console_logs', __name__, url_prefix='/console_logs')


I agree. We should probably make the code versatile enough so that it can handle images and json.So it would be probably better to have the uploads route (blueprint) to behave differently with the content-type. So depending on the Content-Type, there is a validation process depending probably on the content of the file, size, etc. Like we do already for images.

webcompat/helpers.py

webcompat/api/console_logs.py

karlcow · 2019-12-09T07:33:58Z

tests/unit/test_urls.py

@@ -151,6 +151,11 @@ def test_contact(self):
        rv = self.app.get('/contact')
        self.assertEqual(rv.status_code, 200)

+    def test_console_logs(self):
+        """Test that /console_logs exists."""
+        rv = self.app.get('/console_logs/11/20/1c0a7174-2c15-480f-9cee-e6183e6a781b')  # noqa


note to self, we probably needs to create a console_logs directory

karlcow

This is a lot better. Wonderful @ksy36 for the nicely done job.
I will check tomorrow morning again to make extra-sure I didn'T miss anything and I will torture a bit the uploads.

Thanks 👩‍🚀 🚀 ⭐️

miketaylr · 2019-12-10T16:11:32Z

🎈 I'll deploy this to staging so we can play around with it.

(before merging)

miketaylr · 2019-12-10T18:59:20Z

@ksy36 I filed a test issue on staging:

https://staging.webcompat.com/issues/2274

miketaylr · 2019-12-10T19:01:08Z

Looks like we just need to sanitize the JSON, as it may contain HTML strings (and would be an XSS attack vector).

karlcow · 2019-12-11T01:24:39Z

@ksy36

Looks like we just need to sanitize the JSON, as it may contain HTML strings (and would be an XSS attack vector).

so that's what I wanted to check but @miketaylr beat me to it. Cool!
my hunch was motivated by

webcompat.com/webcompat/helpers.py

Lines 766 to 774 in e12e205

    
           def get_data_from_request(request): 
        
               if 'image' in request.files and request.files['image'].filename: 
        
                   return True, request.files['image'] 
        
               elif 'image' in request.form: 
        
                   return True, request.form['image'] 
        
               elif 'console_logs' in request.form: 
        
                   return False, request.form['console_logs'] 
        
               else: 
        
                   return False, None

Instead of True, False here the function should probably report a type, be Image or JSON (These could be strings). To return this specific type we need to validate it, the validation criteria needs to make it clear what we accept or not.

Then there is the question that @miketaylr was pointing about sanitization. server side or client side?

ksy36 · 2019-12-11T03:00:54Z

Instead of True, False here the function should probably report a type, be Image or JSON (These could be strings). To return this specific type we need to validate it, the validation criteria needs to make it clear what we accept or not.

For both of them validation happens later on, in their corresponding classes after an instance is created and before saving attempt. I wanted to separate these two paths (image/json) and have related methods contained within their classes.

So get_data_from_request method just meant to indicate which class should be used based on request contents. I'll change True/False to return type as string (image/json) as @karlcow suggests, since now it's a bit confusing.

And validation we have these:

For image (existing validation):

webcompat.com/webcompat/api/uploads.py

Lines 68 to 86 in e12e205

    
           def to_image_object(self, imagedata): 
        
               '''Method to return a Pillow Image object from the raw imagedata.''' 
        
               try: 
        
                   # Is this a file upload (i.e., issue form upload)? 
        
                   if isinstance(imagedata, FileStorage): 
        
                       return Image.open(imagedata) 
        
                   # Is this a base64 encoded image (i.e., bug report screenshot)? 
        
                   if (isinstance(imagedata, str) and 
        
                           imagedata.startswith('data:image/')): 
        
                       # Chop off 'data:image/.+;base64,' before decoding 
        
                       imagedata = re.sub('^data:image/.+;base64,', '', imagedata) 
        
                       # This will fix any incorrectly padded data. 
        
                       imagedata = imagedata + '===' 
        
                       return Image.open(io.BytesIO(base64.b64decode(imagedata))) 
        
                   raise TypeError('TypeError: Not a valid image format') 
        
               except TypeError: 
        
                   # Not a valid format 
        
                   abort(415)

For json, it just checks that passed string is a valid JSON

webcompat.com/webcompat/api/uploads.py

Lines 149 to 153 in e12e205

    
           def process_data(self, data): 
        
               try: 
        
                   return json.loads(data) 
        
               except ValueError: 
        
                   abort(400)

Maybe we could use something like https://python-jsonschema.readthedocs.io/en/latest/ to validate json schema.

What do you think @karlcow ?

ksy36 · 2019-12-11T03:03:26Z

For sanitization part, @miketaylr mentioned https://api.jquery.com/text/ will escape HTML tags. So I've tried it and it works ok, I'll test it a bit more :)

karlcow · 2019-12-11T03:17:13Z

So get_data_from_request method just meant to indicate which class should be used based on request contents. I'll change True/False to return type as string (image/json) as @karlcow suggests, since now it's a bit confusing.

Good idea.

For image (existing validation):

webcompat.com/webcompat/api/uploads.py

Lines 68 to 86 in e12e205

def to_image_object(self, imagedata):

'''Method to return a Pillow Image object from the raw imagedata.'''

try:

# Is this a file upload (i.e., issue form upload)?

if isinstance(imagedata, FileStorage):

return Image.open(imagedata)

# Is this a base64 encoded image (i.e., bug report screenshot)?

if (isinstance(imagedata, str) and

imagedata.startswith('data:image/')):

# Chop off 'data:image/.+;base64,' before decoding

imagedata = re.sub('^data:image/.+;base64,', '', imagedata)

# This will fix any incorrectly padded data.

imagedata = imagedata + '==='

return Image.open(io.BytesIO(base64.b64decode(imagedata)))

raise TypeError('TypeError: Not a valid image format')

except TypeError:

# Not a valid format

abort(415)

For json, it just checks that passed string is a valid JSON

webcompat.com/webcompat/api/uploads.py

Lines 149 to 153 in e12e205

def process_data(self, data):

try:

return json.loads(data)

except ValueError:

abort(400)

So for images we try to convert to a Pillow Object (and Pillow handles the validation by raising TypeError if it doesn't work)

Probably we should rename the process_data as process_json
The schema validation is an interesting idea, but I have the fear that we would put ourselves in an dependency maintenance nightmare. Aka each time devtools is changing the format we would have to update the schema. So probably not necessary right now, but open question.

Thanks a lot @ksy36

ksy36 · 2019-12-11T21:45:51Z

So I'm thinking maybe a safer way for sanitization is to escape html tags before saving to the server. That way we don't have malicious code uploaded (in theory :)).

I've tried this and it seems to be working good given that the structure of json we get from the reporter is consistent:

    import html
    ....
    def process_json(self, data):
        try:
            d = json.loads(data)
            new_set = []
            for item in d:
                new_dict = {}
                for key, value in item.items():
                    if isinstance(value, list):
                        new_dict[key] = ', '.join(list(map(lambda x: html.escape(x), value)))
                    else:
                        new_dict[key] = html.escape(value)
                new_set.append(new_dict)
            return new_set
        except ValueError:
            abort(400)

So it converts strings like this

Loading failed for the <script> with source https://...ads.js.

To be:
Loading failed for the <script> with source https://...ads.js.

This involves some extra processing on the python side though. Do you have any thoughts on this @karlcow @miketaylr ?

miketaylr · 2019-12-11T22:58:20Z

I agree escaping before we save to disk is a better idea - we don't want to accidentally regress the client side escape...

Looking at https://github.com/webcompat/webcompat.com/pull/3103/files#diff-fbcd0a4c752e94ffddefb108ac14160fR196

I found the following answer on stackoverflow: https://stackoverflow.com/a/27382959

Which points to htmlsafe_dumps which comes with the flask json module: https://github.com/pallets/flask/blob/1351d0a56580df36872b466eb245e7634c20dab5/src/flask/json/__init__.py#L264.

Can we just use that instead of dumps?

miketaylr · 2019-12-11T23:00:46Z

Oops, wrong link!

https://github.com/webcompat/webcompat.com/pull/3103/files#diff-fbcd0a4c752e94ffddefb108ac14160fR166

and https://github.com/pallets/flask/blob/1351d0a56580df36872b466eb245e7634c20dab5/src/flask/json/__init__.py#L301 makes more sense.

ksy36 · 2019-12-15T21:03:46Z

Moving json rendering to the server (with jinja) helped with sanitization. So a <script> element, for example, is returned as <script> with no extra processing on our side. Also the js script is no longer needed :)

miketaylr

awesome!

ksy36 added 4 commits December 2, 2019 20:41

Issue #2659 - Save console logs to the server and append the link to …

3f78ca3

…issue body

Issue #2659 - Added console logs view page and ui

2b77c9f

Issue #2659 - Functional tests for console log ui

abc5f26

Issue #2659 - Add console logs saving to the new form

c939c7b

miketaylr requested review from karlcow and miketaylr December 4, 2019 19:06

miketaylr requested changes Dec 5, 2019

View reviewed changes

karlcow requested changes Dec 6, 2019

View reviewed changes

Issue #2659 - Move console logs saving to upload endpoint

e12e205

karlcow reviewed Dec 9, 2019

View reviewed changes

karlcow approved these changes Dec 9, 2019

View reviewed changes

ksy36 added 2 commits December 12, 2019 13:43

Issue #2659 - Trying htmlsafe_dumps for console logs upload

24a7f75

Issue #2659 - Move parsing console logs to the backend

6055e31

miketaylr approved these changes Dec 16, 2019

View reviewed changes

miketaylr merged commit 6dda1e2 into master Dec 16, 2019

miketaylr deleted the issue/2659/1 branch December 16, 2019 15:29

miketaylr added the status: add to changelog label Dec 16, 2019

karlcow removed the status: add to changelog label Jan 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #2659 - Make console logs in browser configuration details easier to read #3103

Fixes #2659 - Make console logs in browser configuration details easier to read #3103

ksy36 commented Dec 3, 2019

miketaylr Dec 5, 2019

karlcow Dec 6, 2019

ksy36 Dec 9, 2019

karlcow left a comment

karlcow Dec 6, 2019

karlcow Dec 9, 2019

karlcow left a comment

miketaylr commented Dec 10, 2019 •

edited

Loading

miketaylr commented Dec 10, 2019

miketaylr commented Dec 10, 2019

karlcow commented Dec 11, 2019

ksy36 commented Dec 11, 2019

ksy36 commented Dec 11, 2019

karlcow commented Dec 11, 2019

ksy36 commented Dec 11, 2019

miketaylr commented Dec 11, 2019

miketaylr commented Dec 11, 2019

ksy36 commented Dec 15, 2019

miketaylr left a comment


		from webcompat import app

		console_logs = Blueprint('console_logs', __name__, url_prefix='/console_logs')

Fixes #2659 - Make console logs in browser configuration details easier to read #3103

Fixes #2659 - Make console logs in browser configuration details easier to read #3103

Conversation

ksy36 commented Dec 3, 2019

miketaylr Dec 5, 2019

Choose a reason for hiding this comment

karlcow Dec 6, 2019

Choose a reason for hiding this comment

ksy36 Dec 9, 2019

Choose a reason for hiding this comment

karlcow left a comment

Choose a reason for hiding this comment

karlcow Dec 6, 2019

Choose a reason for hiding this comment

karlcow Dec 9, 2019

Choose a reason for hiding this comment

karlcow left a comment

Choose a reason for hiding this comment

miketaylr commented Dec 10, 2019 • edited Loading

miketaylr commented Dec 10, 2019

miketaylr commented Dec 10, 2019

karlcow commented Dec 11, 2019

ksy36 commented Dec 11, 2019

ksy36 commented Dec 11, 2019

karlcow commented Dec 11, 2019

ksy36 commented Dec 11, 2019

miketaylr commented Dec 11, 2019

miketaylr commented Dec 11, 2019

ksy36 commented Dec 15, 2019

miketaylr left a comment

Choose a reason for hiding this comment

miketaylr commented Dec 10, 2019 •

edited

Loading