Provide path params to appsec

[estringana]

Description

Add path params to Laravel and Symfony

Readiness checklist

• (only for Members) Changelog has been added to the release document.
• Tests added for this feature/bug.

Reviewer checklist

• Appropriate labels assigned.
• Milestone is set.
• Changelog has been added to the release document. For community contributors the reviewer is in charge of this task.

APPSEC-14738

[PROFeNoM]

LGTM 😃 I would rebase onto master/merge master, though, before merging into master, considering the Debug Logs PR was merged and could spot errors, if any :)

[pr-commenter]

Benchmarks

Benchmark execution time: 2024-03-28 10:19:27

Comparing candidate commit 6c0628b in PR branch estringana/request-params with baseline commit 76e98df in branch master.

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

scenario:ContextPropagationBench/benchExtractHeaders64Bit-opcache

• 🟩 execution_time [-644.843ns; -531.157ns] or [-19.186%; -15.804%]

scenario:ContextPropagationBench/benchExtractTraceContext128Bit-opcache

• 🟥 execution_time [+491.765ns; +662.235ns] or [+11.176%; +15.051%]

scenario:ContextPropagationBench/benchExtractTraceContext64Bit-opcache

• 🟩 execution_time [-680.891ns; -595.109ns] or [-26.902%; -23.513%]

scenario:LogsInjectionBench/benchLogsInfoBaseline-opcache

• 🟥 execution_time [+482.777ns; +524.023ns] or [+15.285%; +16.590%]

scenario:PDOBench/benchPDOOverheadWithDBM-opcache

• 🟥 mem_peak [+99.696KB; +99.696KB] or [+4.423%; +4.423%]

scenario:TraceSerializationBench/benchSerializeTrace

• 🟩 execution_time [-13.719µs; -9.281µs] or [-3.978%; -2.691%]

[Anilm3]

Just did a quick pass over the extension code and left a few comments.

On the other hand, note that request_init already sends an inferred path_params. Do you have any thoughts on how we could avoid sending those unnecessary path_params ?

[Anilm3]

There should also be appsec integration tests .

[estringana]

Just did a quick pass over the extension code and left a few comments.

On the other hand, note that request_init already sends an inferred path_params. Do you have any thoughts on how we could avoid sending those unnecessary path_params ?

Right now there is nothing implemented that makes this easy. The options are:

• Implement a new feature on the tracer that once a integration is used, maps the request targeted script to a framework. From that moment any request to that script we know it's a specific framework and then avoid sending generic parameters. This approach makes that the first request a target script will either send only the generic params or boths.
• Send generic params at rshutdown if no framework has sent them

[Anilm3]

Just did a quick pass over the extension code and left a few comments.
On the other hand, note that request_init already sends an inferred path_params. Do you have any thoughts on how we could avoid sending those unnecessary path_params ?

Right now there is nothing implemented that makes this easy. The options are:

• Implement a new feature on the tracer that once a integration is used, maps the request targeted script to a framework. From that moment any request to that script we know it's a specific framework and then avoid sending generic parameters. This approach makes that the first request a target script will either send only the generic params or boths.
• Send generic params at rshutdown if no framework has sent them

RSHUTDOWN is already too late for blocking on path_params, perhaps the first approach could work but it's a relatively large effort so perhaps it would make sense to do that in the future.

[codecov-commenter]

Codecov Report

Merging #2395 (6c0628b) into master (76e98df) will decrease coverage by 1.12%.
Report is 1 commits behind head on master.
The diff coverage is 71.11%.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2395      +/-   ##
============================================
- Coverage     76.90%   75.78%   -1.12%     
- Complexity     2564     2577      +13     
============================================
  Files           215      241      +26     
  Lines         23095    27112    +4017     
  Branches          0      985     +985     
============================================
+ Hits          17761    20548    +2787     
- Misses         5334     6039     +705     
- Partials          0      525     +525     

Flag	Coverage Δ
appsec-extension	69.01% <51.85%> (?)
tracer-extension	78.65% <ø> (ø)
tracer-php	74.86% <100.00%> (+0.13%)	⬆️

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

Files	Coverage Δ
...ations/Integrations/Laravel/LaravelIntegration.php	82.12% <100.00%> (+0.21%)	⬆️
...ations/Integrations/Symfony/SymfonyIntegration.php	83.51% <100.00%> (+0.22%)	⬆️
...ns/Integrations/WordPress/WordPressIntegration.php	93.18% <100.00%> (+0.77%)	⬆️
appsec/src/extension/ddappsec.c	77.10% <51.85%> (ø)

... and 26 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 76e98df...6c0628b. Read the comment docs.