Skip to content

Conversation

@D-360
Copy link
Contributor

@D-360 D-360 commented Mar 2, 2021

HttpSender script for full session and csrf token management (nashorn based). Tested over DVWA and over a real life project with active defense enabled (ESAPI)

@kingthorin
Copy link
Member

To address the DCO requirement you'll need to sign-off the commit(s):

@lgtm-com

This comment has been minimized.

Copy link
Member

@kingthorin kingthorin left a comment

Choose a reason for hiding this comment

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

I have no way to re-test this. I don't see anything in the code that seems horrible, aside from the fact that it takes a rather huge amount of code. However that's neither here nor there. Most of my feedback is that some of the white space and comments can likely be removed as they're not likely relevant to end users.

@thc202
Copy link
Member

thc202 commented Mar 26, 2021

Could you remove the commits already merged?

@thc202
Copy link
Member

thc202 commented Mar 26, 2021

This seems to duplicate existing core functionality though.

@D-360
Copy link
Contributor Author

D-360 commented Apr 7, 2021

I guess you talk about the c79a9a5 commit, isn't it?

@thc202
Copy link
Member

thc202 commented Apr 7, 2021

I was referring to 088b663, though everything can be squashed and then rebased. Note that the changelog should be updated as well.

@D-360 D-360 force-pushed the master branch 2 times, most recently from 47901e8 to 1ddab90 Compare April 7, 2021 14:30
@D-360
Copy link
Contributor Author

D-360 commented Apr 7, 2021

Done.

@D-360
Copy link
Contributor Author

D-360 commented Aug 4, 2021

Hello, is this PR going to be merge someday?

@kingthorin
Copy link
Member

If/when #209 (comment) is addressed

@D-360
Copy link
Contributor Author

D-360 commented Aug 4, 2021

But, that was already addressed at following commits, isn't it?

@D-360
Copy link
Contributor Author

D-360 commented Aug 4, 2021

There is no more 088b663 at the commits' tab.

@kingthorin
Copy link
Member

kingthorin commented Aug 4, 2021

though everything can be squashed and then rebased.

@D-360
Copy link
Contributor Author

D-360 commented Aug 4, 2021

Ops, ok, then I'll do it later. My fault.

@kingthorin
Copy link
Member

No worries 😉

@D-360 D-360 force-pushed the master branch 3 times, most recently from 67502dc to 6c844ba Compare August 5, 2021 15:30
@D-360
Copy link
Contributor Author

D-360 commented Aug 6, 2021

Done. Isn't it?

@kingthorin
Copy link
Member

Well you’ve still ended up with two commits because of merge vs rebase. I’ll try to get someone else to review then I can address whatever is left (if anything).

@kingthorin kingthorin self-assigned this Aug 6, 2021
@D-360
Copy link
Contributor Author

D-360 commented Aug 6, 2021

Yeap, sorry, and thanks.

@thc202
Copy link
Member

thc202 commented Aug 7, 2021

The changelog needs to be updated.

@kingthorin
Copy link
Member

Okay, I'll take care of that. It seems there's an entry, just in the wrong place.

@kingthorin
Copy link
Member

Flattened and CHANGELOG.md tweaked.

HttpSender script for full session and csrf token management (nashorn
based). Tested over DVWA and over a real life project with active
defense enabled (ESAPI)

Signed-off-by: Diego Díaz Morales <D-36O@outlook.com>
@thc202
Copy link
Member

thc202 commented Aug 7, 2021

Thank you both!

@kingthorin kingthorin merged commit 227558f into zaproxy:main Aug 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants