Skip to content

Change request blocking scenario for ATO #4308

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

Merged
merged 1 commit into from
Mar 20, 2025

Conversation

Strech
Copy link
Member

@Strech Strech commented Mar 14, 2025

Motivation

In the current setup in the request blocking for automated user events we use runtime activation scenario which requires 1-click activation. First of all, it's a mix of two tests - 1-click activation and request blocking. Second - in Ruby we don't support yet 1-click activation.

Changes

Change runtime activation scenario to request blocking

Reviewer checklist

  • If PR title starts with [<language>], double-check that only <language> is impacted by the change
  • No system-tests internal is modified. Otherwise, I have the approval from R&P team
  • CI is green, or failing jobs are not related to this change (and you are 100% sure about this statement)
  • A docker base image is modified?
    • the relevant build-XXX-image label is present
  • A scenario is added (or removed)?

@Strech Strech requested review from a team as code owners March 14, 2025 10:25
@Strech Strech marked this pull request as draft March 14, 2025 10:38
@Strech Strech force-pushed the appsec-56998-change-runtime-activation-to-blocking branch from ed0a332 to 1d0615e Compare March 14, 2025 14:46
@Strech Strech marked this pull request as ready for review March 14, 2025 14:46
@simon-id
Copy link
Member

simon-id commented Mar 14, 2025

are we sure this scenario will run in the libraries CI ? I don't want to accidently reduce our coverage.

Otherwise LGTM

@uurien
Copy link
Contributor

uurien commented Mar 14, 2025

@simon-id

are we sure this scenario will run in the libraries CI ? I don't want to accidently reduce our coverage.

No, it is not, because it is not part of scenario group that is enabled in node.js tracer, dd-trace-js is only running these three scenarios: essentials,appsec_rasp,debugger, and APPSEC is not part of none of then, each library should modify the loaded scenarios.

@@ -249,6 +249,19 @@ class _Scenarios:
scenario_groups=[ScenarioGroup.APPSEC],
)

appsec_auto_events_blocking = EndToEndScenario(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why using a new scenario? this one seems exactly the same as appsec_runtime_activation

Copy link
Collaborator

Choose a reason for hiding this comment

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

edit: I'm reading more carefully the PR description, to see if it answer my question

Copy link
Member

Choose a reason for hiding this comment

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

appsec activation has appsec disabled at the start to be able to activate it at runtime. For this test we need a scenario that has RC but appsec already activated. Maybe there is already one that fits and we don't need to add a new one

Copy link
Member Author

Choose a reason for hiding this comment

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

If we want - I can rename the scenario to be explicit and not tied to any feature. For some reason APPSEC_REQUEST_BLOCKING didn't work for all languages (maybe something with its setup)

I'm open to any name here

@simon-id
Copy link
Member

why not put this scenario in the essentials then ?

@cbeauchesne
Copy link
Collaborator

cbeauchesne commented Mar 14, 2025

First of all, it's a mix of two tests - 1-click activation and request blocking.

Scenario are not coupled with feature, it's just execution context where we can run test on any feature we can. The fact that their names often contains features reference is legacy and misleading. We can - and actually should, for several reasons - use a scenario for totally different features.

Second - in Ruby we don't support yet 1-click activation

The call to rc.rc_state.reset().apply() should reset RC state, and so this test should not be impacted by the fact another test is failing. But I'm not very familiar with how RC works, I may miss something.

@simon-id
Copy link
Member

simon-id commented Mar 17, 2025

@cbeauchesne you're misunderstanding his words. He means that this ATO test is currently also indirectly testing for ASM activation, and it should not. He is thus changing it in this PR, because the two features are unrelated, and also because ruby doesn't support it so it cannot enable that ATO tests.
The additional scenario is just here to provide RC enabled + ASM enabled by default. If there is already another scenario with those two, we can reuse it instead of creating a new one yes.

@cbeauchesne
Copy link
Collaborator

@cbeauchesne you're misunderstanding his words. He means that this ATO test is currently also indirectly testing for ASM activation, and it should not. He is thus changing it in this PR, because the two features are unrelated, and also because ruby doesn't support it so it cannot enable that ATO tests.

Ok, thanks for the explanation !

If there is already another scenario with those two, we can reuse it instead of creating a new one yes.

You can try APPSEC_REQUEST_BLOCKING

@Strech
Copy link
Member Author

Strech commented Mar 18, 2025

Thanks everyone for the review. Few points:

@cbeauchesne

  1. APPSEC_REQUEST_BLOCKING - tried, faling on all languages (I guess it's related to 1-click)
  2. In ruby if APPSEC_ENABLED or configuration option is not done we don't instrument and hence during 1-click activation or RC update nothing happen because we don't instrument at first place.
  3. We don't have scenario with explicit APPSEC_ENABLED + WAF configuration.

Maybe I'm missing something. Huge thanks to @simon-id 🤗

@Strech
Copy link
Member Author

Strech commented Mar 18, 2025

@uurien I can adjust it, only if I understand what should be done?

@cbeauchesne
Copy link
Collaborator

Ok, let's move forward with the new scenario. It'll be a entire project to merge scenarios that can be merged, but I don't won't to bother you with that now.

@cbeauchesne cbeauchesne dismissed their stale review March 19, 2025 08:51

Let's keep that for a dedicated project

@Strech Strech force-pushed the appsec-56998-change-runtime-activation-to-blocking branch from 1d0615e to c602526 Compare March 19, 2025 14:54
@Strech Strech merged commit 8c84e57 into main Mar 20, 2025
1408 checks passed
@Strech Strech deleted the appsec-56998-change-runtime-activation-to-blocking branch March 20, 2025 09:01
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.

5 participants