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.

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

paramdigger: Handle session change & previous PR follow-up #5306

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

kingthorin
Copy link
Member

@kingthorin kingthorin commented Feb 18, 2024

Overview

Related Issues

Checklist

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

@kingthorin kingthorin marked this pull request as ready for review February 18, 2024 16:07
httpSender.sendAndReceive(msg);
addCacheMessage(msg);
indicValue = msg.getResponseHeader().getHeader(cache.getIndicator());
return this.checkCacheHit(indicValue, cache);
Copy link
Member

Choose a reason for hiding this comment

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

It was returning false when true, should have test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will review, and see what I can do/add. I did double check that existing tests passed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed the logic was the other way around. However, I can't come up with a test case where it matters 😞

Copy link
Member

Choose a reason for hiding this comment

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

You mean based on the existing tests?

Copy link
Member Author

@kingthorin kingthorin Feb 19, 2024

Choose a reason for hiding this comment

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

Yes, shouldNotFindCacheWithAnyCacheBuster seems to hit one case (always miss true). But, really passes whether or not that block is true or false 🤷‍♂️

You can validate simply with the following in CacheController around L870:

                System.out.println("Here");
                return false;//!this.checkCacheHit(indicValue, cache);

and

                System.out.println("Here");
                return true;//!this.checkCacheHit(indicValue, cache);

Copy link
Member Author

Choose a reason for hiding this comment

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

Any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I will have to check the code/tests.

@kingthorin kingthorin force-pushed the pd-tweaks branch 2 times, most recently from cb1b866 to 230711e Compare February 18, 2024 21:04
@kingthorin kingthorin force-pushed the pd-tweaks branch 3 times, most recently from 2f0010d to f448f25 Compare July 12, 2024 11:13
- CHANGELOG > Add change note.
- CacheController > Remove unnecessary else block and conditional
handling.
- ExtensionParamDigger > Add and use SessionChangedListener.
- HeaderGuesser > Make constant final.

Signed-off-by: kingthorin <kingthorin@users.noreply.github.com>
@thc202
Copy link
Member

thc202 commented Jul 12, 2024

Thank you!

@psiinon psiinon merged commit 0f7ecf8 into zaproxy:main Jul 15, 2024
10 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 15, 2024
@kingthorin kingthorin deleted the pd-tweaks branch July 15, 2024 10:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants