Added flask-json-response-type codemod #162

andrecsilva · 2023-12-05T12:43:29Z

Overview

Added flask-json-response-type codemod.

Description

Added flask version of django-json-response-type codemod.
The reason why this one is just not a case of django-json-response-type codemod stems from the fact that we would need to identify which pattern rule was active when making the change. This would require the use of the ScopeProvider and would make it expensive.

codecov · 2023-12-05T12:49:50Z

Codecov Report

Merging #162 (95470e7) into main (6263f09) will increase coverage by 0.07%.
The diff coverage is 97.80%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #162      +/-   ##
==========================================
+ Coverage   96.31%   96.38%   +0.07%     
==========================================
  Files          77       78       +1     
  Lines        3500     3680     +180     
==========================================
+ Hits         3371     3547     +176     
- Misses        129      133       +4

Files	Coverage Δ
src/core_codemods/__init__.py	`100.00% <100.00%> (ø)`
src/core_codemods/django_json_response_type.py	`100.00% <100.00%> (ø)`
src/core_codemods/flask_json_response_type.py	`100.00% <100.00%> (ø)`
src/codemodder/codemods/utils_mixin.py	`91.69% <90.47%> (-0.25%)`	⬇️

drdavella

This is a nice idea for a codemod. However, my main substantive comment is that make_response does not appear to accept a kwarg called mimetype: https://flask.palletsprojects.com/en/2.3.x/api/#flask.Flask.make_response

 File "/Users/danieldavella/sandbox/flask_json.py", line 12, in return_json_str
    return make_response(json.dumps(response), mimetype="application/json")
TypeError: make_response() got an unexpected keyword argument 'mimetype'

There are a probably a few different ways to implement this codemod. One would involve explicitly setting the header on the generated response:

response = make_response(json.dumps(...))
response.headers['Content-Type'] = 'application/json'

Another (imo cleaner) way would involve using werkzeug.wrappers.Response to set the content type. werkzeug is an explicit dependency of flask so there is no issue with adding this import:

from werkzeug.wrappers import Response

def return_json_str():
    response_data = {"message": "This is a JSON response"}
    response_json = json.dumps(response_data)
    return Response(response_json, content_type='application/json')

It's also worth mentioning that in newer versions of flask it is possible to return a dict directly from a view and the response will be properly interpreted as JSON data.

I also think there are also a few other cases we can handle for flask. You can feel free to add them to this PR or create a ticket/add it in another PR.

It is possible to return a string directly from a view in Flask:

@app.route("/return-json-str")
def return_json_str():
    return json.dumps({ "user_input": request.GET.get("input") })

The make_request function also takes a tuple argument where the body is the first element:

def foo():
    data = json.dumps({ "user_input": request.GET.get("input") })
    headers = {}
    return make_response((data, 200, headers))

drdavella

Now that this codemod has been restricted only to decorated Flask view functions, we need to revisit the conditions where this fix is actually necessary. Flask will call jsonify by default on dict objects that are returned from view functions. Furthermore, it appears that make_response also calls jsonify on dict objects.

This would suggest that the set of problematic conditions is confined to cases where the wrong content type is being set explicitly, either with make_response, or in a tuple returned by a view function, or when used to construct a Response object.

drdavella · 2023-12-11T19:40:49Z

tests/codemods/test_flask_json_response_type.py

+        @app.route("/test")
+        def foo(request):
+            json_response = json.dumps({ "user_input": request.GET.get("input") })
+            return (make_response(json_response), {'Content-Type': 'application/json'})


Wouldn't the simplest solution in this case to be to just return json_response since Flask will automatically set the proper context type?

Actually, according to the flask documentation, make_response will also correctly handle the content type by calling jsonify on a dict: https://flask.palletsprojects.com/en/3.0.x/api/#flask.Flask.make_response

drdavella · 2023-12-11T19:42:49Z

tests/codemods/test_flask_json_response_type.py

+        @app.route("/test")
+        def foo(request):
+            json_response = json.dumps({ "user_input": request.GET.get("input") })
+            return json_response


I think this does not need to be modified as flask will automatically set the correct content type in this case.

It won't. The return of json.dumps() is a string and flask will only apply jsonify for dict returns.

drdavella · 2023-12-11T19:43:54Z

tests/codemods/test_flask_json_response_type.py

+from textwrap import dedent
+
+
+class TestFlaskJsonResponseType(BaseCodemodTest):


What about a test case where a variety of different headers are already set? We need to make sure the only modified header is Content-Type.

drdavella · 2023-12-11T19:52:16Z

src/core_codemods/docs/pixee_python_flask-json-response-type.md

+def foo(request):
+    json_response = json.dumps({ "user_input": request.GET.get("input") })
+-   return make_response(json_response)
+   return (make_response(json_response), {'Content-Type':'application/json'}


See other comments regarding the overall conditions for this change, but I think it would be more idiomatic and more aligned with the purpose of make_response to call it like this:

return make_response(json_response, {'Content-Type':'application/json'})

See the documentation and specifically the tuple parameter case: https://flask.palletsprojects.com/en/3.0.x/api/#flask.Flask.make_response

The reason for the tuple response is because it is more general. It will also work for the direct json.dumps response. I'll adjust the transformation for the make_response case.

andrecsilva · 2023-12-12T11:02:18Z

Now that this codemod has been restricted only to decorated Flask view functions, we need to revisit the conditions where this fix is actually necessary. Flask will call jsonify by default on dict objects that are returned from view functions. Furthermore, it appears that make_response also calls jsonify on dict objects.

This would suggest that the set of problematic conditions is confined to cases where the wrong content type is being set explicitly, either with make_response, or in a tuple returned by a view function, or when used to construct a Response object.

First, note that the return of json.dumps() is a string, not a dict. This codemod will only act when the actual response is a result of json.dumps(...) either through make_response, a tuple or directly. In those cases, jsonify won't actually be called since the response is a string.

If you need to convince yourself, simply run the following small flask app:

from flask import Flask, jsonify, make_response
import json

app = Flask(__name__)

@app.route("/")
def hello_world():
    json_response = json.dumps({'data':'Hello World dict'})
    return make_response(json_response))

with

FLASK_APP=flask_test flask run

and check the Content-Type header from the response with your browser or:

curl -iX GET http://localhost:5000

The most idiomatic response is to actually remove the json.dumps call and either return its argument directly or explicitly call jsonify. The reason I've opted to use a tuple response is twofold: it's simpler and will only require local modifications on the return statement while also being more explicit about the change, and jsonify will call app.json.dumps which may actually not be the same as json.dumps (unlikely, but still possible).
See:
https://flask.palletsprojects.com/en/3.0.x/api/#flask.json.dumps
https://github.com/pallets/flask/blob/089f6a1c50ea2b18287106ecd357522bab200510/src/flask/json/provider.py#L74

drdavella · 2023-12-12T15:33:04Z

Yes you're right, sorry I forgot that the output of json.dumps was a string. I agree that the most idiomatic behavior in this case would be to remove the call to json.dumps entirely, although that is probably a difficult change to make. Also I understand the rationale for returning the tuple but it just doesn't look like a clean change when make_response is involved, so hopefully we can make it such that those cases don't overlap.

I realize this codemod has become substantially more complicated than you or I originally anticipated, so I'd be okay with descoping it if that makes it easier for you and we can revisit/refine it later.

andrecsilva · 2023-12-12T16:25:51Z

Yes you're right, sorry I forgot that the output of json.dumps was a string. I agree that the most idiomatic behavior in this case would be to remove the call to json.dumps entirely, although that is probably a difficult change to make. Also I understand the rationale for returning the tuple but it just doesn't look like a clean change when make_response is involved, so hopefully we can make it such that those cases don't overlap.

I realize this codemod has become substantially more complicated than you or I originally anticipated, so I'd be okay with descoping it if that makes it easier for you and we can revisit/refine it later.

I'm already working on making the changes to make_response as you suggested. This codemod certainly was way more touble than expected, but some of the new functions added will help with other codemods in the future.

github-advanced-security · 2023-12-12T18:22:04Z

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

tests/samples/flask_json_response_type.py

sonarqubecloud · 2023-12-13T13:41:27Z

Quality Gate failed

Failed conditions

1 Security Hotspot

See analysis details on SonarCloud

drdavella

Very nice, thanks for powering through on this one 😅

andrecsilva marked this pull request as ready for review December 5, 2023 12:59

andrecsilva requested review from drdavella and clavedeluna as code owners December 5, 2023 12:59

drdavella requested changes Dec 5, 2023

View reviewed changes

andrecsilva force-pushed the flask-json-response-type branch 2 times, most recently from 960cca0 to e800225 Compare December 11, 2023 12:53

andrecsilva requested a review from drdavella December 11, 2023 13:07

drdavella requested changes Dec 11, 2023

View reviewed changes

github-advanced-security bot found potential problems Dec 12, 2023

View reviewed changes

tests/samples/flask_json_response_type.py Show resolved Hide resolved

andrecsilva added 3 commits December 13, 2023 08:40

flask-json-response-type codemod with tests and docs

7e374ea

Added extra test for flask-json-response-type

59f3303

make_response fixes will now include the dict as argument

9657c31

andrecsilva force-pushed the flask-json-response-type branch from e800225 to 9657c31 Compare December 13, 2023 13:37

Removed unused import

95470e7

andrecsilva requested a review from drdavella December 13, 2023 13:45

drdavella approved these changes Dec 13, 2023

View reviewed changes

andrecsilva added this pull request to the merge queue Dec 13, 2023

Merged via the queue into main with commit c26f393 Dec 13, 2023
13 of 14 checks passed

andrecsilva deleted the flask-json-response-type branch December 13, 2023 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added flask-json-response-type codemod #162

Added flask-json-response-type codemod #162

andrecsilva commented Dec 5, 2023 •

edited

Loading

codecov bot commented Dec 5, 2023 •

edited

Loading

drdavella left a comment •

edited

Loading

drdavella left a comment

drdavella Dec 11, 2023

drdavella Dec 11, 2023

drdavella Dec 11, 2023

andrecsilva Dec 12, 2023 •

edited

Loading

drdavella Dec 11, 2023

drdavella Dec 11, 2023

andrecsilva Dec 12, 2023

andrecsilva commented Dec 12, 2023

drdavella commented Dec 12, 2023

andrecsilva commented Dec 12, 2023

github-advanced-security bot commented Dec 12, 2023

sonarqubecloud bot commented Dec 13, 2023

drdavella left a comment

		from textwrap import dedent


		class TestFlaskJsonResponseType(BaseCodemodTest):

Added flask-json-response-type codemod #162

Added flask-json-response-type codemod #162

Conversation

andrecsilva commented Dec 5, 2023 • edited Loading

Overview

Description

codecov bot commented Dec 5, 2023 • edited Loading

Codecov Report

drdavella left a comment • edited Loading

Choose a reason for hiding this comment

drdavella left a comment

Choose a reason for hiding this comment

drdavella Dec 11, 2023

Choose a reason for hiding this comment

drdavella Dec 11, 2023

Choose a reason for hiding this comment

drdavella Dec 11, 2023

Choose a reason for hiding this comment

andrecsilva Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

drdavella Dec 11, 2023

Choose a reason for hiding this comment

drdavella Dec 11, 2023

Choose a reason for hiding this comment

andrecsilva Dec 12, 2023

Choose a reason for hiding this comment

andrecsilva commented Dec 12, 2023

drdavella commented Dec 12, 2023

andrecsilva commented Dec 12, 2023

github-advanced-security bot commented Dec 12, 2023

sonarqubecloud bot commented Dec 13, 2023

Quality Gate failed

drdavella left a comment

Choose a reason for hiding this comment

andrecsilva commented Dec 5, 2023 •

edited

Loading

codecov bot commented Dec 5, 2023 •

edited

Loading

drdavella left a comment •

edited

Loading

andrecsilva Dec 12, 2023 •

edited

Loading