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.

Sign up for GitHub

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

Jump to bottom

Include pluginId in alert events #8454

Merged
merged 1 commit into from
Apr 19, 2024
Merged

Include pluginId in alert events #8454

merged 1 commit into from
Apr 19, 2024

Conversation

psiinon
Copy link
Member

@psiinon psiinon commented Apr 18, 2024

This means event consumers will be able to reliably identify alerts.
Right now they only have access to the alert names, with are i18ned.

@kingthorin
Copy link
Member

Would we want this to be alert ref if it exists?

@thc202 thc202 added this to the 2.15.0 milestone Apr 18, 2024
@thc202 thc202 changed the title Inc plugin id in alert events Include scan rule ID in alert events Apr 18, 2024
@psiinon
Copy link
Member Author

psiinon commented Apr 18, 2024

Oh, good suggestion..

@psiinon psiinon changed the title Include scan rule ID in alert events Include alertRef in alert events Apr 19, 2024
@psiinon
Copy link
Member Author

psiinon commented Apr 19, 2024

Updated to use alertRef instead.

psiinon added a commit to psiinon/zap-extensions that referenced this pull request Apr 19, 2024
@psiinon
QuickStart: support AJAX "if modern" option
1d840b0 
Related PR: zaproxy/zaproxy#8454 (but not a
dependency)

Signed-off-by: Simon Bennetts <psiinon@gmail.com>
psiinon added a commit to psiinon/zap-extensions that referenced this pull request Apr 19, 2024
@psiinon
QuickStart: support AJAX "if modern" option
c52326e 
Related PR: zaproxy/zaproxy#8454 (but not a
dependency)

Signed-off-by: Simon Bennetts <psiinon@gmail.com>
@psiinon psiinon mentioned this pull request Apr 19, 2024
Merged
8 tasks
@kingthorin
Copy link
Member

I haven't dug through the code but does this always have a value? Does it default to pluginId if a specific ref hasn't been set?

@psiinon
Copy link
Member Author

psiinon commented Apr 19, 2024

Yes, its defaulted to the pluginId for standard rules.
It can be an empty string for user defined alerts.
I've tested it and both tests (alertRef and the name) seem to work fine..

@psiinon
Inc pluginId in alert events
8192477 
This means event consumers will be able to reliably identify alerts.
Right now they only have access to the alert names, with are i18ned.

Signed-off-by: Simon Bennetts <psiinon@gmail.com>
@psiinon psiinon changed the title Include alertRef in alert events Include pluginId in alert events Apr 19, 2024
@psiinon
Copy link
Member Author

psiinon commented Apr 19, 2024

Switched back to pluginId based on IRC discussions :)
The pluginId is easier to handle and we currently have no usecase where we need the alertRef...

@thc202
Copy link
Member

thc202 commented Apr 19, 2024

Thank you!

@thc202 thc202 requested a review from kingthorin April 19, 2024 15:46
@kingthorin
Copy link
Member

To me if it's about Alerts then it should be the thing that most specifically identifies the alert, but whatever we can always do more later.

@kingthorin kingthorin merged commit 381c3e4 into zaproxy:main Apr 19, 2024
9 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 19, 2024
@thc202
Copy link
Member

thc202 commented Apr 19, 2024

It definitely depends on the use case, and here the scan rule ID is enough.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@thc202 thc202 thc202 approved these changes

@kingthorin kingthorin kingthorin approved these changes

Assignees
No one assigned
Labels
enhancement
Milestone
2.15.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@psiinon @kingthorin @thc202