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

Process triggers and notifications also on exception during ActiveScans sendAndReceive. #7005

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Process triggers and notifications also on exception during ActiveScans sendAndReceive. #7005

wants to merge 1 commit into from

Conversation

denniskniep
Copy link
Member

@denniskniep denniskniep commented Jan 4, 2022 •
edited by thc202

The results in the Active Scan panel will look like this:

image

Signed-off-by: Dennis Kniep kniepdennis@gmail.com

Fixes #7004

@denniskniep denniskniep force-pushed the feature/active-scan-process-notification-on-error branch from 2adde5a to 8808e34 Compare January 4, 2022 08:36
@denniskniep
Copy link
Member Author

denniskniep commented Jan 4, 2022 •
edited

To be discussed:

  • process triggers and notifications only on specific ExceptionTypes: SocketTimeoutException, IOException ?
  • Set the exception/error message as response body?

@kingthorin
Copy link
Member

I'd be good with the exceptions being made into the response body (that's done elsewhere IIRC). I wonder if this should be tied to an option though and default off (to maintain closest to current behavior)?

@denniskniep
Copy link
Member Author

denniskniep commented Jan 5, 2022 •
edited

Discussion from IRC:

  • have a visual indication - just be an icon for the ones with errors (like the spider has with the red one)
    image
  • add exceptions to the response body
  • provide messages with errors separately, at least for the API/programmatically (way easier to inspect them that way rather than go through thousands of requests)

@denniskniep denniskniep changed the title Process triggers and notifications also on exception during ActiveScans sendAndReceive. [WIP] Process triggers and notifications also on exception during ActiveScans sendAndReceive. Jan 5, 2022
@denniskniep
Copy link
Member Author

@thc202 can you provide more Context regarding

provide messages with errors separately, at least for the API/programmaticall

Is it only separating for API (/ascan/view/messagesIds/)

@thc202
Copy link
Member

thc202 commented Jan 10, 2022 •
edited

For the code and the ZAP API, in the ActiveScan expose a method that returns a list with the IDs of the messages with errors (similar to SpiderScan which exposes successful and error messages separately). In the ZAP API another endpoint to get those requests.

thc202
thc202 reviewed Jan 10, 2022
View reviewed changes
zap/src/main/java/org/parosproxy/paros/core/scanner/AbstractPlugin.java Outdated
}
} finally {
// ZAP: Notify parent
parent.notifyNewMessage(this, message);
Copy link
Member

Choose a reason for hiding this comment

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

Might be better just notify on IOExceptions.

zap/src/main/java/org/parosproxy/paros/core/scanner/AbstractPlugin.java Outdated
parent.performScannerHookAfterScan(message, this);
// ZAP: Set the history reference back and run the "afterScan" methods of any
// ScannerHooks
parent.performScannerHookAfterScan(message, this);
Copy link
Member

Choose a reason for hiding this comment

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

Since we didn't have a request for this I'd keep the previous behaviour.

@denniskniep denniskniep force-pushed the feature/active-scan-process-notification-on-error branch from 8808e34 to 0d4ba97 Compare January 14, 2022 00:14
denniskniep
denniskniep commented Jan 14, 2022
View reviewed changes
zap/src/main/java/org/parosproxy/paros/core/scanner/AbstractPlugin.java Outdated
parent.getHttpSender().sendAndReceive(message, false);
}
} catch (IOException e) {
// TODO: How to update HistoryRef with error Response?
Copy link
Member Author

Choose a reason for hiding this comment

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

@thc202 can you give me any hint here?
How can I update the HistoryRef with error Response? In case HttpMessage has already a HistoryRef and is attached to the Alert

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed in IRC: No workarounds here. Problem is releated to oast
I guess the way how histRef ist set to HttpMessage is like the following:
.alertFound(alert, null) is triggered by oast here before exception is catched:

https://github.com/zaproxy/zap-extensions/blob/7016bdf3607f31f42ceb3206c86b437131b0452b/addOns/oast/src/main/java/org/zaproxy/addon/oast/ExtensionOast.java#L268

And then a HistRef is created

zaproxy/zap/src/main/java/org/zaproxy/zap/extension/alert/ExtensionAlert.java

Line 189 in f68b04e

new HistoryReference(

Which sets automatically the histref member on the Httpmessage

Resulting in that the Histref is set to the message not including the error message which is later set

we should change oast to allow to manipulate the alert/message before caching it

@denniskniep denniskniep force-pushed the feature/active-scan-process-notification-on-error branch 2 times, most recently from b2f6be5 to 7f82c53 Compare January 16, 2022 22:11
@denniskniep
Process triggers and notifications also on exception during ActiveSca…
e965de5 
…ns sendAndReceive. Fixes zaproxy#7004

Signed-off-by: Dennis Kniep <kniepdennis@gmail.com>
@denniskniep denniskniep force-pushed the feature/active-scan-process-notification-on-error branch from 7f82c53 to e965de5 Compare January 16, 2022 22:13
@denniskniep denniskniep changed the title [WIP] Process triggers and notifications also on exception during ActiveScans sendAndReceive. Process triggers and notifications also on exception during ActiveScans sendAndReceive. Jan 16, 2022
@denniskniep
Copy link
Member Author

Anything else to do here before merge ?

psiinon
psiinon reviewed Jan 27, 2022
View reviewed changes
zap/src/main/java/org/zaproxy/zap/extension/callback/ui/CustomColumn.java
@@ -19,7 +19,7 @@
*/
package org.zaproxy.zap.extension.callback.ui;

/** @deprecated (2.11.0) Superseded by the OAST add-on. */
/** @deprecated (2.11.0) {@link org.zaproxy.zap.view.table.CustomColumn}. */
Copy link
Member

Choose a reason for hiding this comment

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

Is this intensional? If so why?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was deprecated once due to moving Oast addon from ZAP to ZAP-Extension.

But I used the original class again. Therefore its there again but within an other package. I thought this info could be useful to know if someone stumbles upon that deprecation

Copy link
Member

Choose a reason for hiding this comment

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

ok, find by me :)

psiinon
psiinon reviewed Jan 27, 2022
View reviewed changes
...va/org/zaproxy/zap/extension/callback/ui/DefaultCustomColumnHistoryReferencesTableModel.java
@@ -25,7 +25,10 @@
import org.zaproxy.zap.view.table.DefaultHistoryReferencesTableEntry;
import org.zaproxy.zap.view.table.DefaultHistoryReferencesTableModel;

/** @deprecated (2.11.0) Superseded by the OAST add-on. */
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

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

@psiinon psiinon psiinon left review comments

@thc202 thc202 thc202 left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
Type-Task
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sent Http Requests with Exceptions are not displayed in the ActiveScan Panel
4 participants
@denniskniep @kingthorin @thc202 @psiinon