Skip to content
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

[backend/frontend] enforce null consistency with executor and command (#1621) #1825

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

antoinemzs
Copy link

@antoinemzs antoinemzs commented Nov 6, 2024

Proposed changes

  • Enforce having a value for both the Cleanup Executor and the Cleanup Command in Payload definition

Related issues

Checklist

  • I consider the submitted work as finished
  • I tested the code for its functionality
  • I wrote test cases for the relevant uses case
  • I added/update the relevant documentation (either on github or on notion)
  • Where necessary I refactored code to improve the overall quality

Further comments

On the payload definition panel, it is now not possible to only define the cleanup executor or the cleanup command. Both must be defined at the same time or none of them (i.e. blank fields).

The backend part was done with a "pure" null philosophy, unfortunately the frontend is not yet able to handle null values in react field components: the frontend special cases empty string for fields to submit null instead.

@antoinemzs antoinemzs added the filigran team use to identify PR from the Filigran team label Nov 6, 2024
Signed-off-by: Antoine MAZEAS <[email protected]>
Signed-off-by: Antoine MAZEAS <[email protected]>
Copy link

codecov bot commented Nov 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 32.31%. Comparing base (77b583d) to head (b1d9b67).
Report is 15 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1825      +/-   ##
============================================
+ Coverage     32.08%   32.31%   +0.22%     
+ Complexity     1609     1605       -4     
============================================
  Files           561      562       +1     
  Lines         16850    17049     +199     
  Branches        974     1019      +45     
============================================
+ Hits           5406     5509     +103     
- Misses        11200    11291      +91     
- Partials        244      249       +5     

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

@antoinemzs antoinemzs marked this pull request as ready for review November 7, 2024 17:03
@savacano28 savacano28 self-requested a review November 8, 2024 07:54
@antoinemzs antoinemzs marked this pull request as draft November 8, 2024 08:26
@antoinemzs antoinemzs marked this pull request as ready for review November 8, 2024 10:17
@@ -2680,6 +2681,7 @@ const i18n = {
'Update the security platform': '更新安全平台',
'Security Platforms': '安全平台',
'Content should not be empty': '内容不可为空',
'Cleanup command and executor must be defined together or none at all': '清理命令和执行器必须一起定义,否则根本无法定义',
Copy link
Author

Choose a reason for hiding this comment

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

Deepl translation

@antoinemzs antoinemzs changed the title enforce null consistency with executor and command (#1621) [backend/frontend] enforce null consistency with executor and command (#1621) Nov 8, 2024
@antoinemzs antoinemzs marked this pull request as draft November 8, 2024 16:31
@antoinemzs
Copy link
Author

Issue found: the new constraint prevents importing the atomic red team database

@antoinemzs antoinemzs marked this pull request as ready for review November 12, 2024 09:27
@antoinemzs
Copy link
Author

Issue found: the new constraint prevents importing the atomic red team database

fixed

@savacano28
Copy link
Contributor

savacano28 commented Nov 15, 2024

nitpick: When I try to update a payload (image 1), I receive errors for all the mandatory fields, but not for the scenario where only one of two fields is filled, so maybe it is possible to add this verification?

image

Question: When I create a payload with a type other than 'executable type,' I receive all the expected errors, but for 'executable type,' I don't. Could you please verify why the behavior is different?

image

image

@@ -158,4 +279,20 @@ private PayloadCreateInput getExecutablePayloadCreateInput() {
input.setExecutableArch(Endpoint.PLATFORM_ARCH.x86_64);
return input;
}

private PayloadCreateInput getCommandLinePayloadCreateInput() {
PayloadCreateInput input = new PayloadCreateInput();
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: Maybe use a fixture?

SET command_executor = NULL, command_content = NULL
WHERE (command_executor = '') IS NOT FALSE OR (command_content = '') IS NOT FALSE
""");
migrator.execute(
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: Thanks for adding this constraint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filigran team use to identify PR from the Filigran team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Payload - Enforce required cleanup command AND executor when choosing one of the two
2 participants