fix(asm): fix reading wsgi input #4114
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @app.route("/body") | ||
| def body(): | ||
| data = request.get_json() | ||
| return data, 200 |
Check warning
Code scanning / CodeQL
Reflected server-side cross-site scripting
| content_type = request.META["CONTENT_TYPE"] | ||
| if content_type in ("application/json", "application/xml", "text/xml"): | ||
| data = request.body | ||
| return HttpResponse(data, status=200) |
Check warning
Code scanning / CodeQL
Reflected server-side cross-site scripting
| return HttpResponse(data, status=200) | ||
| else: | ||
| data = request.POST | ||
| return HttpResponse(str(dict(data)), status=200) |
Check warning
Code scanning / CodeQL
Reflected server-side cross-site scripting
Member
|
Benchmarks of appsec enabled: |
| @app.route("/post-view", methods=["POST"]) | ||
| def post_view(): | ||
| data = request.data | ||
| return data, 200 |
Check warning
Code scanning / CodeQL
Reflected server-side cross-site scripting
Codecov Report
@@ Coverage Diff @@
## 1.x #4114 +/- ##
==========================================
- Coverage 78.86% 78.80% -0.06%
==========================================
Files 720 720
Lines 57386 57504 +118
==========================================
+ Hits 45255 45316 +61
- Misses 12131 12188 +57
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Member
brettlangdon
left a comment
brettlangdon
left a comment
There was a problem hiding this comment.
flask is the only server which is having this problem?
P403n1x87
reviewed
Aug 18, 2022
Member
Yes, Django and Pylons have other approach when you try to retrieve the body |
juanjux
previously approved these changes
Aug 22, 2022
avara1986
previously approved these changes
Aug 22, 2022
releasenotes/notes/asm-fix-reset-wsgi-input-035e0a7d917af2b2.yaml
Outdated
Show resolved
Hide resolved
brettlangdon
approved these changes
Aug 22, 2022
Member
|
@Mergifyio backport 1.4 |
juanjux
approved these changes
Aug 22, 2022
Contributor
✅ Backports have been created
|
mabdinur
approved these changes
Aug 22, 2022
Contributor
mabdinur
left a comment
mabdinur
left a comment
There was a problem hiding this comment.
Can we run the new benchmarks and include the results in the PR description? It seems like an interesting to document
Member
why are these results not valid? #4114 (comment) |
mergify bot
pushed a commit
that referenced
this pull request
Aug 23, 2022
* feat(asm): added tests to check error reading body * feat(asm): added tests to check error reading body. Django tests * feat(asm): added tests to check error reading body. Pylons tests * feat(asm): added tests to check error reading body. Pylons tests * fix(asm): Reset wsgi input after reading it * chore(asm): add release notes * feat: update django tests * chore(asm): move seek to a better place * feat: update django tests * feat: update tests * feat: update tests * feat: update tests * feat: update tests * feat: update tests * feat: update tests * fix(asm): workaround for non seekable wsgi.input * fix(asm): ensure attr seekable exists for wsgi.input * feat(asm): add benchmark * fix(asm): ensure attr seekable exists for wsgi.input * fix(asm): ensure attr seekable exists for wsgi.input * Update releasenotes/notes/asm-fix-reset-wsgi-input-035e0a7d917af2b2.yaml Co-authored-by: Alberto Vara <alberto.vara@datadoghq.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Brett Langdon <brett.langdon@datadoghq.com> (cherry picked from commit 90e11db) # Conflicts: # benchmarks/set_http_meta/scenario.py # ddtrace/contrib/flask/patch.py # tests/contrib/django/test_django_appsec.py # tests/contrib/flask/test_flask_appsec.py # tests/contrib/pylons/test_pylons.py
brettlangdon
pushed a commit
that referenced
this pull request
Aug 23, 2022
This is an automatic backport of pull request #4114 done by [Mergify](https://mergify.com). Cherry-pick of 90e11db has failed: ``` On branch mergify/bp/1.4/pr-4114 Your branch is up to date with 'origin/1.4'. You are currently cherry-picking commit 90e11db. (fix conflicts and run "git cherry-pick --continue") (use "git cherry-pick --skip" to skip this patch) (use "git cherry-pick --abort" to cancel the cherry-pick operation) Changes to be committed: modified: benchmarks/flask_simple/app.py modified: benchmarks/flask_simple/config.yaml modified: benchmarks/flask_simple/gunicorn.conf.py modified: benchmarks/flask_simple/scenario.py modified: benchmarks/flask_simple/utils.py modified: benchmarks/span/utils.py new file: releasenotes/notes/asm-fix-reset-wsgi-input-035e0a7d917af2b2.yaml modified: tests/contrib/django/django1_app/urls.py modified: tests/contrib/django/django_app/urls.py modified: tests/contrib/django/views.py modified: tests/contrib/flask/app.py modified: tests/contrib/pylons/app/controllers/root.py modified: tests/contrib/pylons/app/router.py Unmerged paths: (use "git add/rm <file>..." as appropriate to mark resolution) deleted by us: benchmarks/set_http_meta/scenario.py both modified: ddtrace/contrib/flask/patch.py both modified: tests/contrib/django/test_django_appsec.py both modified: tests/contrib/flask/test_flask_appsec.py both modified: tests/contrib/pylons/test_pylons.py ``` To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally --- <details> <summary>Mergify commands and options</summary> <br /> More conditions and actions can be found in the [documentation](https://docs.mergify.com/). You can also trigger Mergify actions by commenting on this pull request: - `@Mergifyio refresh` will re-evaluate the rules - `@Mergifyio rebase` will rebase this PR on its base branch - `@Mergifyio update` will merge the base branch into this PR - `@Mergifyio backport <destination>` will backport this PR on `<destination>` branch Additionally, on Mergify [dashboard](https://dashboard.mergify.com/) you can: - look at your merge queues - generate the Mergify configuration with the config editor. Finally, you can contact us on https://mergify.com </details>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes an issue with Flask as we read the body request and it is consumed (so Flask cannot parse it afterwards).
Checklist
featandfixpull requests.Relevant issue(s)
Not reported
Testing strategy
Unit tests for Django, Flask and Pylons.
Reviewer Checklist
changelog/no-changeloglabel added.