Add more logging around stack generation

[estringana]

Description

Reviewer checklist

• Test coverage seems ok.
• Appropriate labels assigned.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 53.33333% with 7 lines in your changes missing coverage. Please review.

Project coverage is 73.04%. Comparing base (4da93b3) to head (b6b804d).
Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
appsec/src/extension/backtrace.c	36.36%	7 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3096      +/-   ##
============================================
- Coverage     74.86%   73.04%   -1.83%     
  Complexity     2792     2792              
============================================
  Files           112      139      +27     
  Lines         11046    15294    +4248     
  Branches          0     1046    +1046     
============================================
+ Hits           8270    11171    +2901     
- Misses         2776     3570     +794     
- Partials          0      553     +553     

Flag	Coverage Δ
appsec-extension	68.29% <53.33%> (?)
tracer-php	74.86% <ø> (ø)

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

Files with missing lines	Coverage Δ
appsec/src/extension/dddefs.c	72.22% <100.00%> (ø)
appsec/src/extension/ddtrace.c	57.57% <100.00%> (ø)
appsec/src/extension/request_lifecycle.c	61.50% <100.00%> (ø)
appsec/src/extension/backtrace.c	70.30% <36.36%> (ø)

... and 23 files with indirect coverage changes

---

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 4da93b3...b6b804d. Read the comment docs.

[pr-commenter]

Benchmarks [ appsec ]

Benchmark execution time: 2025-02-26 09:47:49

Comparing candidate commit b6b804d in PR branch estringana/add-more-loggin-around-stacktrace-generation with baseline commit 4da93b3 in branch master.

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

[cataphract]

This is confusing. You're mixing up two different methods to implement priority between the verdicts. On the one hand, you have three booleans, you set to true if found and then return in your desired preference order (according to which, redirect has apparently higher priority).

On the other hand you also refuse to set these booleans if you already found some, e.g. you refuse to set block if you before found redirect or record. This might make sense, if your priority is based not only on which is the action with higher priority but depending on which one appears first. But I don't think that's the case here. (If it is, please describe the rules in a comment)

[estringana]

As far as I know(and that's what I implemented is):

• Redirection has priority over block
• First redirection is the one taken into account
• First block(if end result is blocking) is the one taken into account
• Stack trace should be considered as a record but it needs to generate the stack trace.

Do this makes sense now?

[Anilm3]

This doesn't make much sense, the WAF will never provide you with two conflicting verdicts, you can consider block, redirect and record to be mutually exclusive (albeit record is a made up action).

All of this could be solved by just sending record to the extension, in addition to stack_trace, when there's an event and no other block or `redirect action, everything else is unnecessary.

[estringana]

ok

[estringana]

I'm ok with adding the extra record on the helper. However, I still think the each subsystem has to be robust enough to handle unexpected things. You can't ensure the waf will never send conflicting verdicts. It's unlikey but who knows. That said, taking into cosideration that unlikely scenario is not like it's impacting the performance or doing anything that it's worth to have the risk

[Anilm3]

I'm ok with adding the extra record on the helper. However, I still think the each subsystem has to be robust enough to handle unexpected things. You can't ensure the waf will never send conflicting verdicts. It's unlikey but who knows. That said, taking into cosideration that unlikely scenario is not like it's impacting the performance or doing anything that it's worth to have the risk

Yes, I'm okay with having a mechanism to ensure that only the highest precedence action is used, but this isn't it.