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

Support json/xml request bodies #2499

Merged
merged 1 commit into from
Feb 19, 2024
Merged

Support json/xml request bodies #2499

merged 1 commit into from
Feb 19, 2024

Conversation

cataphract
Copy link
Contributor

@cataphract cataphract commented Jan 31, 2024

Also fix the handling of content-type/length in requests for the purposes of sending these headers to the helper and for setting the respective tags. These are available in $_SERVER['CONTENT_...'], not $_SERVER['HTTP_CONTENT_...']

APPSEC-51412

Description

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@cataphract cataphract requested review from a team as code owners January 31, 2024 20:02
@codecov-commenter
Copy link

codecov-commenter commented Jan 31, 2024

Codecov Report

Merging #2499 (20c5f0a) into master (27f4013) will increase coverage by 0.01%.
The diff coverage is 64.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2499      +/-   ##
============================================
+ Coverage     76.53%   76.54%   +0.01%     
  Complexity      267      267              
============================================
  Files           136      136              
  Lines         17526    17573      +47     
  Branches       1020     1034      +14     
============================================
+ Hits          13413    13451      +38     
- Misses         3580     3585       +5     
- Partials        533      537       +4     
Flag Coverage Δ
appsec-extension 70.24% <65.84%> (+0.18%) ⬆️
tracer-extension 78.46% <0.00%> (-0.03%) ⬇️
tracer-integrations 79.49% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
appsec/src/extension/commands/request_shutdown.c 67.82% <100.00%> (+5.95%) ⬆️
appsec/src/extension/ddappsec.c 80.74% <ø> (-0.11%) ⬇️
ext/ddtrace_arginfo.h 100.00% <ø> (ø)
appsec/src/extension/commands/request_init.c 89.28% <92.59%> (+1.15%) ⬆️
appsec/src/extension/request_lifecycle.c 64.32% <88.23%> (+0.61%) ⬆️
appsec/src/extension/tags.c 82.34% <86.66%> (-0.01%) ⬇️
ext/serializer.c 78.80% <0.00%> (-0.08%) ⬇️
ext/user_request.c 14.75% <0.00%> (-0.38%) ⬇️
appsec/src/extension/entity_body.c 61.30% <57.92%> (-13.17%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27f4013...20c5f0a. Read the comment docs.

@pr-commenter
Copy link

pr-commenter bot commented Jan 31, 2024

Benchmarks

Benchmark execution time: 2024-02-07 10:14:24

Comparing candidate commit 498fae5 in PR branch glopes/server-req-body with baseline commit 27f4013 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 40 metrics, 50 unstable metrics.

@cataphract cataphract force-pushed the glopes/server-resp-body branch 4 times, most recently from 3a8a9de to b4283de Compare February 6, 2024 12:15
Base automatically changed from glopes/server-resp-body to master February 6, 2024 14:16
Also fix the handling of content-type/length in requests for the
purposes of sending these headers to the helper and for setting the
respective tags. These are available in $_SERVER['CONTENT_...'], not
$_SERVER['HTTP_CONTENT_...']
@@ -657,6 +665,19 @@ static void _dd_http_client_ip(zend_array *meta_ht)
}
}

static void _try_add_tag(zend_array *meta_ht, zend_string *tag_name, zval *val)
{

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

nit

@@ -34,6 +34,8 @@
#define DD_TAG_HTTP_URL "http.url"
#define DD_TAG_NETWORK_CLIENT_IP "network.client.ip"
#define DD_PREFIX_TAG_REQUEST_HEADER "http.request.headers."
#define DD_TAG_HTTP_REQH_CONTENT_TYPE "http.request.headers.content-type"
#define DD_TAG_HTTP_REQH_CONTENT_LENGTH "http.request.headers.content-length"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the naming of the request / response macros and interned strings could be a bit more consistent.

appsec/src/extension/tags.c Show resolved Hide resolved
@cataphract
Copy link
Contributor Author

Naming stuff will be improved later.

@cataphract cataphract merged commit 7da29d0 into master Feb 19, 2024
580 checks passed
@cataphract cataphract deleted the glopes/server-req-body branch February 19, 2024 10:26
@github-actions github-actions bot added this to the 0.98.0 milestone Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants