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

Pscan rule tweaks #473

Merged
merged 1 commit into from Aug 9, 2016
Merged

Pscan rule tweaks #473

merged 1 commit into from Aug 9, 2016

Conversation

@psiinon
Copy link
Member

@psiinon psiinon commented Jul 28, 2016

As per https://groups.google.com/d/msg/zaproxy-develop/6IAnEHANWpI/zva5350aBgAJ

* 1. pscan rules cant make new requests and
* 2. the browser will check it anyway
*/
continue;
Copy link
Member

@kingthorin kingthorin Jul 28, 2016

Choose a reason for hiding this comment

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

Might be more clear if the logic is reversed here and the alert raising moved in. Or if you really like this the comment should make it clear that the continue moves to the next sourceElement because integrity was found on the current one.

@psiinon
Copy link
Member Author

@psiinon psiinon commented Aug 2, 2016

Have addressed most of these comments, with the exception of a header helper method - about to post to the dev group thread about this.
Have also included fixes for zaproxy/zaproxy#2732

return;
}
if (msg.getRequestHeader().getURI().toString().toLowerCase().endsWith(".css")) {

Copy link
Member

@kingthorin kingthorin Aug 2, 2016

Choose a reason for hiding this comment

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

Should there be an action inside this if?

@psiinon
Copy link
Member Author

@psiinon psiinon commented Aug 2, 2016

}

/**
* Returns the relevant SetCookie(2) line as well as just the cookie name, or null if not found.
Copy link
Member

@kingthorin kingthorin Aug 2, 2016

Choose a reason for hiding this comment

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

What's the significance of (2)? Is that meant to indicate that this handles both Set-Cookie and Set-Cookie2? If so I'd suggest being specific here:

Returns the relevant Set-Cookie or Set-Cookie2 line as well as...

The return statement should mention something like the SetCookie header plus name. Which should be clear enough once the description of the function is updated....my 2cents.

Also you might want to say header vs line, as line would seem to imply the whole header up to some sort of line ending or wordwrap.

HttpMessage msg = new HttpMessage();
msg.setRequestHeader("GET http://www.example.com/test/ HTTP/1.1");

msg.setResponseBody("<html></html>");
Copy link
Member

@kingthorin kingthorin Aug 2, 2016

Choose a reason for hiding this comment

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

Function name and content-type suggest text...while I suppose this isn't wrong (the text could be html tags), it's kind of misleading.

@psiinon
Copy link
Member Author

@psiinon psiinon commented Aug 3, 2016

Hopefully fixed all the comments.
Also changed to only report X-Frame-Options issues at LOW threshold if CSP 'frame-ancestors' element present.


msg.setResponseBody("<html></html>");
msg.setResponseHeader(
"HTTP/1.1 301 OK\r\n" +
Copy link
Member

@kingthorin kingthorin Aug 3, 2016

Choose a reason for hiding this comment

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

One seems to have squeaked by Moved Permanently

@kingthorin
Copy link
Member

@kingthorin kingthorin commented Aug 3, 2016

The CSP thing looks good, made a few other small notes, there are still a few places that there's a space between function name and brackets. Looking good.

Thanks for tackling this @psiinon.

@psiinon
Copy link
Member Author

@psiinon psiinon commented Aug 4, 2016

Hopefully fixed those comments

@psiinon
Copy link
Member Author

@psiinon psiinon commented Aug 4, 2016

Updated to reduce the boilerplate code in the unit tests c/o @thc202

@psiinon
Copy link
Member Author

@psiinon psiinon commented Aug 9, 2016

More tweaks based on feedback

@thc202
Copy link
Member

@thc202 thc202 commented Aug 9, 2016

Looks good to me.

@kingthorin kingthorin merged commit f81b3df into zaproxy:master Aug 9, 2016
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants