Skip to content

Commit be24fc8

Browse files
cataphractcataphract
committed
remove global block_params; centralize blocking logic
1 parent 193c190 commit be24fc8

File tree

12 files changed

+288
-235
lines changed

12 files changed

+288
-235
lines changed

‎appsec/src/extension/commands/request_exec.c‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,8 @@ static const dd_command_spec _spec = {
3030
.config_features_cb = dd_command_process_config_features_unexpected,
3131
};
3232

33-
dd_result dd_request_exec(
34-
dd_conn *nonnull conn, zval *nonnull data, zend_string *nullable rasp_rule)
33+
dd_result dd_request_exec(dd_conn *nonnull conn, zval *nonnull data,
34+
zend_string *nullable rasp_rule, struct block_params *nonnull block_params)
3535
{
3636
if (Z_TYPE_P(data) != IS_ARRAY) {
3737
mlog(dd_log_debug, "Invalid data provided to command request_exec, "
@@ -41,7 +41,11 @@ dd_result dd_request_exec(
4141

4242
struct ctx ctx = {.rasp_rule = rasp_rule, .data = data};
4343

44-
return dd_command_exec_req_info(conn, &_spec, &ctx.req_info);
44+
dd_result res = dd_command_exec_req_info(conn, &_spec, &ctx.req_info);
45+
46+
memcpy(block_params, &ctx.req_info.block_params, sizeof *block_params);
47+
48+
return res;
4549
}
4650

4751
static dd_result _pack_command(mpack_writer_t *nonnull w, void *nonnull _ctx)

‎appsec/src/extension/commands/request_exec.h‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,11 @@
55
// (https://www.datadoghq.com/). Copyright 2021 Datadog, Inc.
66
#pragma once
77

8-
#include "../network.h"
98
#include <SAPI.h>
109
#include <php.h>
1110

12-
dd_result dd_request_exec(
13-
dd_conn *nonnull conn, zval *nonnull data, zend_string *nullable rasp_rule);
11+
#include "../network.h"
12+
#include "../request_abort.h"
13+
14+
dd_result dd_request_exec(dd_conn *nonnull conn, zval *nonnull data,
15+
zend_string *nullable rasp_rule, struct block_params *nonnull block_params);

‎appsec/src/extension/commands_ctx.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,11 @@
22

33
#include <php.h>
44
#include "attributes.h"
5+
#include "request_abort.h"
56

67
struct req_info {
78
const char *nullable command_name; // for logging
89
zend_object *nullable root_span;
910
zend_string *nullable client_ip;
11+
struct block_params block_params; // out
1012
};

‎appsec/src/extension/commands_helpers.c‎

Lines changed: 65 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,17 @@ static inline ATTR_WARN_UNUSED mpack_error_t _imsg_destroy(
5656
dd_imsg *nonnull imsg);
5757
static void _imsg_cleanup(dd_imsg *nullable *imsg);
5858

59+
static void _set_redirect_code_and_location(
60+
struct block_params *nonnull block_params,
61+
// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
62+
int code, zend_string *nullable location,
63+
zend_string *nullable security_response_id);
64+
65+
static void _set_block_code_and_type(struct block_params *nonnull block_params,
66+
// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
67+
int code, dd_response_type type,
68+
zend_string *nullable security_response_id);
69+
5970
static dd_result _dd_command_exec(dd_conn *nonnull conn,
6071
const dd_command_spec *nonnull spec, void *unspecnull ctx)
6172
{
@@ -299,7 +310,8 @@ static void _imsg_cleanup(dd_imsg *nullable *imsg)
299310
static void _add_appsec_span_data_frag(mpack_node_t node);
300311
static void _set_appsec_span_data(mpack_node_t node);
301312

302-
static void _command_process_block_parameters(mpack_node_t root)
313+
static void _command_process_block_parameters(
314+
struct block_params *nonnull block_params, mpack_node_t root)
303315
{
304316
int status_code = DEFAULT_BLOCKING_RESPONSE_CODE;
305317
dd_response_type type = DEFAULT_RESPONSE_TYPE;
@@ -365,10 +377,12 @@ static void _command_process_block_parameters(mpack_node_t root)
365377
"Blocking parameters: status_code=%d, type=%d, security_response_id=%s",
366378
status_code, type,
367379
security_response_id ? ZSTR_VAL(security_response_id) : "NULL");
368-
dd_set_block_code_and_type(status_code, type, security_response_id);
380+
_set_block_code_and_type(
381+
block_params, status_code, type, security_response_id);
369382
}
370383

371-
static void _command_process_redirect_parameters(mpack_node_t root)
384+
static void _command_process_redirect_parameters(
385+
struct block_params *nonnull block_params, mpack_node_t root)
372386
{
373387
int status_code = 0;
374388
zend_string *location = NULL;
@@ -423,9 +437,52 @@ static void _command_process_redirect_parameters(mpack_node_t root)
423437
"security_response_id=%s",
424438
status_code, location ? ZSTR_VAL(location) : "NULL",
425439
security_response_id ? ZSTR_VAL(security_response_id) : "NULL");
426-
dd_set_redirect_code_and_location(
427-
status_code, location, security_response_id);
440+
441+
_set_redirect_code_and_location(
442+
block_params, status_code, location, security_response_id);
428443
}
444+
445+
static void _set_block_code_and_type(struct block_params *nonnull block_params,
446+
// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
447+
int code, dd_response_type type, zend_string *nullable security_response_id)
448+
{
449+
dd_response_type _type = type;
450+
// Account for lack of enum type safety
451+
switch (type) {
452+
case response_type_auto:
453+
case response_type_html:
454+
case response_type_json:
455+
_type = type;
456+
break;
457+
default:
458+
_type = response_type_auto;
459+
break;
460+
}
461+
462+
block_params->security_response_id = security_response_id;
463+
block_params->response_type = _type;
464+
block_params->response_code = code;
465+
block_params->redirection_location = NULL;
466+
}
467+
468+
static void _set_redirect_code_and_location(
469+
struct block_params *nonnull block_params,
470+
// NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
471+
int code, zend_string *nullable location,
472+
zend_string *nullable security_response_id)
473+
{
474+
int response_code = DEFAULT_REDIRECTION_RESPONSE_CODE;
475+
476+
// NOLINTNEXTLINE(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
477+
if (code >= 300 && code < 400) {
478+
response_code = code;
479+
}
480+
block_params->security_response_id = security_response_id;
481+
block_params->response_type = response_type_auto;
482+
block_params->response_code = response_code;
483+
block_params->redirection_location = location;
484+
}
485+
429486
static void _command_process_stack_trace_parameters(mpack_node_t root)
430487
{
431488
size_t count = mpack_node_map_count(root);
@@ -540,13 +597,14 @@ static dd_result _command_process_actions(
540597
if (dd_mpack_node_lstr_eq(verdict, "block") && res != dd_should_block &&
541598
res != dd_should_redirect) { // Redirect take over block
542599
res = dd_should_block;
543-
_command_process_block_parameters(mpack_node_array_at(action, 1));
600+
_command_process_block_parameters(
601+
&ctx->block_params, mpack_node_array_at(action, 1));
544602
dd_tags_add_blocked();
545603
} else if (dd_mpack_node_lstr_eq(verdict, "redirect") &&
546604
res != dd_should_redirect) {
547605
res = dd_should_redirect;
548606
_command_process_redirect_parameters(
549-
mpack_node_array_at(action, 1));
607+
&ctx->block_params, mpack_node_array_at(action, 1));
550608
dd_tags_add_blocked();
551609
} else if (dd_mpack_node_lstr_eq(verdict, "record") &&
552610
res == dd_success) {

‎appsec/src/extension/ddappsec.c‎

Lines changed: 11 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -279,7 +279,6 @@ static PHP_RINIT_FUNCTION(ddappsec)
279279
DDAPPSEC_G(skip_rshutdown) = false;
280280
dd_msgpack_helpers_rinit();
281281
dd_trace_rinit();
282-
dd_request_abort_rinit();
283282

284283
// Waf calls happen here. Not many rinits should go after this line.
285284
dd_req_lifecycle_rinit(false);
@@ -288,7 +287,7 @@ static PHP_RINIT_FUNCTION(ddappsec)
288287
if (get_global_DD_APPSEC_TESTING_ABORT_RINIT()) {
289288
const char *pt = SG(request_info).path_translated;
290289
if (pt && !strstr(pt, "skip.php")) {
291-
dd_request_abort_static_page();
290+
dd_request_abort_static_page(&(struct block_params){0});
292291
}
293292
}
294293
}
@@ -475,11 +474,13 @@ static PHP_FUNCTION(datadog_appsec_testing_request_exec)
475474
RETURN_FALSE;
476475
}
477476

478-
if (dd_request_exec(conn, data, false) != dd_success) {
479-
RETURN_FALSE;
477+
struct block_params block_params = {0};
478+
if (dd_request_exec(conn, data, false, &block_params) != dd_success) {
479+
RETVAL_FALSE;
480+
} else {
481+
RETVAL_TRUE;
480482
}
481-
482-
RETURN_TRUE;
483+
dd_request_abort_destroy_block_params(&block_params);
483484
}
484485

485486
static PHP_FUNCTION(datadog_appsec_push_addresses)
@@ -517,7 +518,8 @@ static PHP_FUNCTION(datadog_appsec_push_addresses)
517518
return;
518519
}
519520

520-
dd_result res = dd_request_exec(conn, addresses, rasp_rule);
521+
struct block_params block_params = {0};
522+
dd_result res = dd_request_exec(conn, addresses, rasp_rule, &block_params);
521523

522524
if (rasp_rule && ZSTR_LEN(rasp_rule) > 0) {
523525
clock_gettime(CLOCK_MONOTONIC_RAW, &end);
@@ -532,17 +534,8 @@ static PHP_FUNCTION(datadog_appsec_push_addresses)
532534
}
533535
}
534536

535-
if (dd_req_is_user_req()) {
536-
if (res == dd_should_block || res == dd_should_redirect) {
537-
dd_req_call_blocking_function(res);
538-
}
539-
} else {
540-
if (res == dd_should_block) {
541-
dd_request_abort_static_page();
542-
} else if (res == dd_should_redirect) {
543-
dd_request_abort_redirect();
544-
}
545-
}
537+
dd_req_lifecycle_abort(REQUEST_STAGE_REQUEST_END, res, &block_params);
538+
dd_request_abort_destroy_block_params(&block_params);
546539
}
547540

548541
ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(

0 commit comments

Comments
 (0)