-
-
Notifications
You must be signed in to change notification settings - Fork 690
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
spider: do not add seeds to the found list #4963
base: main
Are you sure you want to change the base?
Conversation
Please see https://github.com/zaproxy/zap-extensions/pull/4963/checks?check_run_id=17379176664 The changelog should be updated. Better to have some tests. |
Note that the |
938f7cf
to
49cba99
Compare
maybe I could add a test like
Does that makes sense? BTW, since I'm yet to learn more about the ZAP code, I'm not sure at the moment how I can access 'foundURIs' in SpiderControllerUnitTest.Java? @thc202 Could you give me an example? |
+1 from a user perspective :) |
Could you let me know where the changelog is? Maybe it'd be better if the information could be found in https://github.com/zaproxy/zaproxy/blob/main/CONTRIBUTING.md#guidelines-for-pull-request-pr-submission-and-processing |
For the record that's in https://github.com/zaproxy/zap-extensions/blob/main/CONTRIBUTING.md#changelog |
@thc202 or anyone, could I get any info which will be helpful? |
In @Test
void shouldNotNotifySeedAsFoundUri() {
// Given
URI seed = …
// When
spiderController.addSeed(seed, …);
// Then
verify(spider, times(0)).notifyListenersFoundURI(eq(seed), …);
} although this is not correct (IMO) we should still notify about the seeds used, the referenced issue was about the count of found URIs so the fix should be to ignore seeds when counting. |
@jeremychoi do you still plan to finish this? |
@kingthorin will try to find time to finish this week. |
5078042
to
a24aa33
Compare
@kingthorin @thc202 please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this mean that none of the seeds are marked found, though we only want to skip the "artificial" seeds (robots, sitemap, ds_store)?
@kingthorin you're right. would this work if we checked the found-URI belongs to the aritificial seeds? |
I'd be good with that. |
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
f2b6f81
to
6765ec2
Compare
I have read the CLA Document and I hereby sign the CLA |
recheck |
Could anyone help with solving this error?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For binary compatibility you'll need to maintain the original Seed constructor and addSeed method. They can call the new one and pass defaults for the boolean (likely false?)
@@ -5,9 +5,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) | |||
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). | |||
|
|||
## Unreleased | |||
### Changed | |||
- Changed Spider not to notify as a found URI when a seed is added from addRootFileSeed() or addFileSeed() (Issue 7737). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be a user friendly explanation not a dev one.
@jeremychoi are you able to keep going with this? |
@kingthorin Thanks for the heads up. Yes. Sorry for the delay. I'll find time this weekend. |
Can we have a clarification of what the final behaviour is going to be? |
the file seeds(e.g. robots, sitemap) will not be notified as 'foundURI'. |
We are back to the remark in #4963 (comment). |
@thc202 |
Please feel free to close this if my last contribution is not good enough. Unfortunately I'm afraid I have no bandwidth to further work on this recently. |
We are working on it. |
@jeremychoi thanks for getting it started and thanks for letting us know. |
Signed-off-by: Jeremy Choi <jechoi@redhat.com>
Signed-off-by: Jeremy Choi <jechoi@redhat.com>
Co-authored-by: Rick M <kingthorin@users.noreply.github.com> Signed-off-by: Jeremy Bonghwan Choi <jechoi@redhat.com>
Signed-off-by: Jeremy Choi <jechoi@redhat.com>
Signed-off-by: Jeremy Choi <jechoi@redhat.com>
…() nor addRootFileSeed() Signed-off-by: Jeremy Choi <jechoi@redhat.com>
Signed-off-by: Jeremy Choi <jechoi@redhat.com>
cf9dce7
to
789ddde
Compare
Overview
Fix zaproxy/zaproxy#7737
Checklist
./gradlew spotlessApply
for code formattingFor more details, please refer to the developer rules and guidelines.