spider: do not add seeds to the found list

[jeremychoi]

Overview

Fix zaproxy/zaproxy#7737

Checklist

• [N/A] Update help
• Update changelog
• Run ./gradlew spotlessApply for code formatting
• Write tests
• Check code coverage
• Sign-off commits
• Squash commits
• Use a descriptive title

For more details, please refer to the developer rules and guidelines.

[thc202]

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.

[psiinon]

Note that the robots.txt and sitemap.xml URLs still appear in the SiteMap.
This makes sense if they really exist, but maybe we should not include them if they are 404s and requested by the spider? I've wondered about this for a while.
That doesnt have to be part of this PR, just a comment 😁

[jeremychoi]

Better to have some tests.

maybe I could add a test like

1. spiderController.addSeed(uri, ...)
2. assert if the uri does not exist in foundURIs.

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?

[jeremychoi]

maybe we should not include them if they are 404s and requested by the spider?

+1 from a user perspective :)

[jeremychoi]

The changelog should be updated.

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

[kingthorin]

https://github.com/zaproxy/zap-extensions/blob/main/addOns/spider/CHANGELOG.md

[thc202]

For the record that's in https://github.com/zaproxy/zap-extensions/blob/main/CONTRIBUTING.md#changelog

[jeremychoi]

maybe I could add a test like

1. spiderController.addSeed(uri, ...)
2. assert if the uri does not exist in foundURIs.

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?

@thc202 or anyone, could I get any info which will be helpful?

[thc202]

In SpiderControllerUnitTest something like:

@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.

[kingthorin]

@jeremychoi do you still plan to finish this?

[jeremychoi]

@kingthorin will try to find time to finish this week.

[jeremychoi]

@kingthorin @thc202 please review.
@thc202 we should still notify about the seeds used => In that case, IMO it should be implemented in a separate way, instead of calling notifyListenersFoundURI() whose name doesn't fit here, because it's not that seeds are found.

[kingthorin]

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)?

[jeremychoi]

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?

[kingthorin]

I'd be good with that.

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[jeremychoi]

I have read the CLA Document and I hereby sign the CLA

[jeremychoi]

recheck

[jeremychoi]

Could anyone help with solving this error?

A failure occurred while executing me.champeau.gradle.japicmp.JApiCmpWorkAction
937 actionable tasks: 99 executed, 311 from cache, 527 up-to-date
Detected binary changes.
- current: spider-0.11.0.jar
- baseline: spider-0.6.0.jar.

[kingthorin]

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?)

[kingthorin]

Should be a user friendly explanation not a dev one.

[kingthorin]

@jeremychoi are you able to keep going with this?

[jeremychoi]

@kingthorin Thanks for the heads up. Yes. Sorry for the delay. I'll find time this weekend.

[thc202]

Can we have a clarification of what the final behaviour is going to be?

[jeremychoi]

@thc202

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'.

[thc202]

We are back to the remark in #4963 (comment).

[jeremychoi]

@thc202 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. => do you mean file seeds should be notified as found URIs?