Skip to content

Conversation

@avara1986
Copy link
Member

@avara1986 avara1986 commented Mar 12, 2025

_on_wrapped_view contains the logic to block requests for AppSec and to taint path parameters for IAST. However, since the _on_wrapped_view hook function is in load_appsec, the IAST logic doesn’t run if AppSec isn’t enabled.

This PR splits that logic and creates two separate hooks.

This PR is a cherry-pick of one of the commits of this PR #12639

Checklist

  • PR author has checked that all the criteria below are met
  • The PR description includes an overview of the change
  • The PR description articulates the motivation for the change
  • The change includes tests OR the PR description describes a testing strategy
  • The PR description notes risks associated with the change, if any
  • Newly-added code is easy to change
  • The change follows the library release note guidelines
  • The change includes or references documentation updates if necessary
  • Backport labels are set (if applicable)

Reviewer Checklist

  • Reviewer has checked that all the criteria below are met
  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Newly-added code is easy to change
  • Release note makes sense to a user of the library
  • If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
  • Backport labels are set in a manner that is consistent with the release branch maintenance policy

@avara1986 avara1986 added changelog/no-changelog A changelog entry is not required for this PR. ASM Application Security Monitoring labels Mar 12, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2025

CODEOWNERS have been resolved as:

ddtrace/appsec/_asm_request_context.py                                  @DataDog/asm-python
ddtrace/appsec/_iast/_handlers.py                                       @DataDog/asm-python
ddtrace/appsec/_iast/_listener.py                                       @DataDog/asm-python
ddtrace/contrib/internal/flask/wrappers.py                              @DataDog/apm-core-python @DataDog/apm-idm-python
tests/appsec/iast/conftest.py                                           @DataDog/asm-python
tests/appsec/integrations/flask_tests/test_iast_flask.py                @DataDog/asm-python

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 18 lines in your changes missing coverage. Please review.

Project coverage is 3.68%. Comparing base (96fd080) to head (23db957).

Files with missing lines Patch % Lines
ddtrace/appsec/_iast/_handlers.py 0.00% 11 Missing ⚠️
ddtrace/appsec/_iast/_listener.py 0.00% 4 Missing ⚠️
ddtrace/appsec/_asm_request_context.py 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12706      +/-   ##
==========================================
- Coverage    3.68%    3.68%   -0.01%     
==========================================
  Files        1362     1362              
  Lines      134231   134232       +1     
==========================================
- Hits         4943     4942       -1     
- Misses     129288   129290       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pr-commenter
Copy link

pr-commenter bot commented Mar 12, 2025

Benchmarks

Benchmark execution time: 2025-03-12 14:23:34

Comparing candidate commit 0dc0d53 in PR branch avara1986/iast_split_wrapped_view with baseline commit 96fd080 in branch main.

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

@avara1986 avara1986 marked this pull request as ready for review March 12, 2025 14:41
@avara1986 avara1986 requested review from a team as code owners March 12, 2025 14:41
@christophe-papazian
Copy link
Contributor

core.dispatch_with_results seem fine to me. You can have several hooks and as long as they use different names for the results, the result object will contains all of them.

@avara1986 avara1986 merged commit 8670015 into main Mar 14, 2025
397 of 400 checks passed
@avara1986 avara1986 deleted the avara1986/iast_split_wrapped_view branch March 14, 2025 07:48
@github-actions
Copy link
Contributor

The backport to 2.21 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.21 2.21
# Navigate to the new working tree
cd .worktrees/backport-2.21
# Create a new branch
git switch --create backport-12706-to-2.21
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 8670015c1584bb2e7e675fc7be6646458e418fd2
# Push it to GitHub
git push --set-upstream origin backport-12706-to-2.21
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.21

Then, create a pull request where the base branch is 2.21 and the compare/head branch is backport-12706-to-2.21.

github-actions bot pushed a commit that referenced this pull request Mar 14, 2025
`_on_wrapped_view` contains the logic to block requests for AppSec and
to taint path parameters for IAST. However, since the `_on_wrapped_view`
hook function is in `load_appsec`, the IAST logic doesn’t run if AppSec
isn’t enabled.

This PR splits that logic and creates two separate hooks.

This PR is a cherry-pick of one of the commits of this PR
#12639

## Checklist
- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [x] Reviewer has checked that all the criteria below are met
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, author has acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment
- Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

(cherry picked from commit 8670015)
avara1986 added a commit that referenced this pull request Mar 14, 2025
`_on_wrapped_view` contains the logic to block requests for AppSec and
to taint path parameters for IAST. However, since the `_on_wrapped_view`
hook function is in `load_appsec`, the IAST logic doesn’t run if AppSec
isn’t enabled.

This PR splits that logic and creates two separate hooks.

This PR is a cherry-pick of one of the commits of this PR
#12639

- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

- [x] Reviewer has checked that all the criteria below are met
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, author has acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment
- Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

(cherry picked from commit 8670015)
avara1986 added a commit that referenced this pull request Mar 15, 2025
Backport 8670015 from #12706 to 3.2.

`_on_wrapped_view` contains the logic to block requests for AppSec and
to taint path parameters for IAST. However, since the `_on_wrapped_view`
hook function is in `load_appsec`, the IAST logic doesn’t run if AppSec
isn’t enabled.

This PR splits that logic and creates two separate hooks.  

This PR is a cherry-pick of one of the commits of this PR
#12639


## Checklist
- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [x] Reviewer has checked that all the criteria below are met 
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, author has acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment
- Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

Co-authored-by: Alberto Vara <alberto.vara@datadoghq.com>
github-actions bot pushed a commit that referenced this pull request Mar 15, 2025
`_on_wrapped_view` contains the logic to block requests for AppSec and
to taint path parameters for IAST. However, since the `_on_wrapped_view`
hook function is in `load_appsec`, the IAST logic doesn’t run if AppSec
isn’t enabled.

This PR splits that logic and creates two separate hooks.

This PR is a cherry-pick of one of the commits of this PR
#12639

## Checklist
- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [x] Reviewer has checked that all the criteria below are met
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, author has acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment
- Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

(cherry picked from commit 8670015)
avara1986 added a commit that referenced this pull request Mar 16, 2025
Backport 8670015 from #12706 to 2.21.

`_on_wrapped_view` contains the logic to block requests for AppSec and
to taint path parameters for IAST. However, since the `_on_wrapped_view`
hook function is in `load_appsec`, the IAST logic doesn’t run if AppSec
isn’t enabled.

This PR splits that logic and creates two separate hooks.  

This PR is a cherry-pick of one of the commits of this PR
#12639

Backport #12726 to fix the CI

## Checklist
- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [x] Reviewer has checked that all the criteria below are met 
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, author has acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment
- Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
avara1986 added a commit that referenced this pull request Mar 18, 2025
Backport 8670015 from #12706 to 3.1.

`_on_wrapped_view` contains the logic to block requests for AppSec and
to taint path parameters for IAST. However, since the `_on_wrapped_view`
hook function is in `load_appsec`, the IAST logic doesn’t run if AppSec
isn’t enabled.

This PR splits that logic and creates two separate hooks.  

This PR is a cherry-pick of one of the commits of this PR
#12639


## Checklist
- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [x] Reviewer has checked that all the criteria below are met 
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, author has acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment
- Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

Co-authored-by: Alberto Vara <alberto.vara@datadoghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ASM Application Security Monitoring backport 2.21 changelog/no-changelog A changelog entry is not required for this PR. [DEPRECATED] backport 3.1 [DEPRECATED] backport 3.2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants