-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
New common getExampleAlerts() method #6119
Comments
Is this going to be added to core and subsequently implemented/overridden by scan rules? |
Yeah, latest plan is to change the core interfaces, but I'll aim to implement the method for the rules in the first comment and we can see how well it works. The script already uses introspection so it should be able to cope without the core changes. |
Sounds good. One thing to keep in mind some i18n messages require replacements/insertions (I dunno what the proper term is). So dummy values will have to be passed for those. (The whole |
Yeah - thats what I meant by the i18n part - in case we need to introduce new more generic strings. |
Example of what one of the rules which raises multiple alerts would look like. |
Part of zaproxy/zaproxy#6119 Associated script changes will follow v shortly... Signed-off-by: Simon Bennetts <[email protected]>
Part of zaproxy/zaproxy#6119 Associated script changes will follow v shortly... Signed-off-by: Simon Bennetts <[email protected]>
Part of zaproxy/zaproxy#6119 Associated script changes will follow v shortly... Signed-off-by: Simon Bennetts <[email protected]>
Note: #7100 may as well be tackled at the same time. |
Updated, seem PII was the only one missed. |
On all those that remain to be done at this point I've added initials to clarify things a bit.
|
Hello! I'm looking upon to work on this issue with a friend of mine, we are trying to get into some repository on OWASP and this one is one of the most engaged, but we don't understand a lot of the field, and we confess we are a little bit lost on this, how could we start to work on it? |
I'd suggest having a look at some of the PRs that have already contributed to this (see links/refs above). Then look at the rules that have work outstanding (there's a table above, anything with a ❌ still needs work). Let us know which few you think you can tackle and we'll block them off for you. (I'd definitely suggest tackling singles or a small batch for your first contribution.) |
Alright, I'm going to study and see what I can do. Thanks! |
Please be sure to let us know if you decide to tackle some, so that we can ensure to mark them and not end-up with people tackling overlapping work 😉 |
Hey @kingthorin, sorry for keep you waiting. I decided I'm going to try to tackle "PersistentXssScanRule" |
@iagoscm assigned 🙂 You'll need to adjust:
|
Hello guys, would love to tackle this issue. Would you guys say any of the SQL Injection alerts are good for a first contribution on this issue? |
Thought you were tackling "PersistentXssScanRule". I haven't reviewed the SQLi rules recently. |
@kingthorin I think you are confusing me with @iagoscm . Im new to this issue. |
Oops sorry, you’re right. |
@kingthorin I'll try to tackle SpringActuatorScanRule, could you assign it to me? |
Added you. You'll need to adjust:
|
Hello, I would like to try working with the pscanrule InsecureJsfViewStatePassiveScanRule, could you assign me this alert? |
I'll set it Monday or Tuesday. |
This is for all scan rules - active, passive, http, websocket, future ones :)
The method is proposed to be a 'defacto standard' for now:
List<Alert> getExampleAlerts()
It will be accessed by the generate_alert_pages.js script using introspection. This script generates the https://www.zaproxy.org/docs/alerts/ pages.
At the moment the script can only cope with one alert per add-on, while many addons generate several.
Ease of maintenance is key - this new method should call any suitable existing methods - if they don't exist the new ones could be created.
Release & beta rules with little to no info which should implement this new method asap:
The plan is to only expose the information currently on the alert pages, so URLs can be https://www.example.com
Any new generic text created for these alerts should be i18n'd
The text was updated successfully, but these errors were encountered: