Skip to content

Conversation

@Wryhder
Copy link
Contributor

@Wryhder Wryhder commented Feb 17, 2025

@kingthorin I tried to add callouts/admonitions (called alerts in the Hugo docs) like this to match the look in the Notion doc, but it doesn't seem to work:

> [!NOTE]
> Useful information that users should know, even when skimming content.

Can I add a template for alerts as shown in the docs here?

@psiinon
Copy link
Member

psiinon commented Feb 17, 2025

Logo
Checkmarx One – Scan Summary & Details63d47ee5-1de3-4037-84ae-564fb9a3b40c

Great job, no security vulnerabilities found in this Pull Request

@kingthorin
Copy link
Member

I'm fine with that being added.

@psiinon, @thc202 do we want those in a separate PR then rebase the blog on top after accepted/merged??

@Wryhder Wryhder deleted the branch zaproxy:main February 18, 2025 19:33
@Wryhder Wryhder closed this Feb 18, 2025
@Wryhder Wryhder deleted the main branch February 18, 2025 19:33
@Wryhder Wryhder restored the main branch February 18, 2025 19:34
@Wryhder Wryhder reopened this Feb 18, 2025
@Wryhder Wryhder force-pushed the main branch 5 times, most recently from f7f341e to 1e54b34 Compare February 20, 2025 17:56
@Wryhder
Copy link
Contributor Author

Wryhder commented Feb 20, 2025

Leaving a comment here about force-pushed changes. I added the sign-off trailer.

EDIT: @kingthorin I don't remember being prompted to sign the CLA when I created this PR. Do I need to create a PR to the CLA repo?

@kingthorin
Copy link
Member

Nope you're good, CLA isn't enabled for this repo. Thanks for checking though.

@Wryhder Wryhder marked this pull request as draft February 21, 2025 16:53
@Wryhder
Copy link
Contributor Author

Wryhder commented Feb 21, 2025

@kingthorin I was working through a different lab and realized I got some details in this walkthrough mixed up with details from another lab. I'll correct the inaccuracies and let you know when this is ready for review again.

@kingthorin
Copy link
Member

Thanks for the heads up. I hadn't gone to look at the lab at all, so we easily could have missed that.....

@psiinon
Copy link
Member

psiinon commented Feb 26, 2025

Thanks for this! As you've probably noticed we've got another Portswigger lab PR outstanding #2973 so we'll aim to get that one published first and then focus on this one 😁

@kingthorin
Copy link
Member

This can be rebased now

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'll try to do a thorough review of the text and details over the next few days.

@Wryhder Wryhder marked this pull request as ready for review March 28, 2025 02:55
@Wryhder
Copy link
Contributor Author

Wryhder commented Mar 28, 2025

I'll try to do a thorough review of the text and details over the next few days.

Alright, thank you!

@kingthorin
Copy link
Member

kingthorin commented Mar 31, 2025

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

Handle this how you like, but I find it's easiest to collapse all the commits into one and just do the signoff on that one.

@Wryhder
Copy link
Contributor Author

Wryhder commented Mar 31, 2025

@kingthorin I've read through your comments (left replies on some). I'll address them within the next few days. Thanks for being so thorough with your review. Appreciate it!

@kingthorin
Copy link
Member

No problem at all, thanks for tackling this!

@Wryhder
Copy link
Contributor Author

Wryhder commented Apr 2, 2025

@kingthorin I've implemented all feedback from your review. Also fixed the DCO issue (I had the trailer in the previous commits but was modifying it manually and it ended up being invalid).

Didn't squash commits because I wanted to keep the history (initial article vs current one).

summary: >
Walkthrough for the PortSwigger lab, "Broken brute-force protection, IP block".
images:
- https://www.zaproxy.org/blog/2025-02-17-portswigger-lab-walkthrough-broken-brute-force-protection-ip-block/images/image header-key and house models.jpg
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kingthorin I followed the example of the other articles here. Is this correct?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'd suggest using underscores or hyphens in the filename, and reducing the length (it seems overly verbose).
(Here and the file itself obviously 😉)

Copy link
Contributor Author

@Wryhder Wryhder Apr 3, 2025

Choose a reason for hiding this comment

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

reducing the length (it seems overly verbose)

Just to be clear, you mean just the filename of the image, yes? Not including the article (that one's long, too)?

Copy link
Member

Choose a reason for hiding this comment

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

Ya just the image

Comment on lines 753 to 755
7. [Java Interoperability](https://www.graalvm.org/jdk21/reference-manual/js/JavaInteroperability/)

_Image credit: [Pexels](https://www.pexels.com/photo/real-estate-concept-with-key-and-house-models-31424880/)._
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I needed to include some attribution but wasn't sure where to add it. Is this okay?

Oh, and what do you think of the image itself?

Copy link
Member

Choose a reason for hiding this comment

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

I'm good with this, and I'm good with the image too.

Copy link
Member

@thc202 thc202 Apr 8, 2025

Choose a reason for hiding this comment

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

The license says that attribution is not required:
https://www.pexels.com/license/

Attribution is not required. Giving credit to the photographer or Pexels is not necessary but always appreciated.

I don't mind the attribution just saying.

@kingthorin
Copy link
Member

Assuming that one filename/reference thing is addressed I'm good with this article now.

@Wryhder
Copy link
Contributor Author

Wryhder commented Apr 3, 2025

@kingthorin I've made the filename change. Also squashed a few commits.

@Wryhder
Copy link
Contributor Author

Wryhder commented Apr 3, 2025

Force-pushed change is more squashing. Done now.

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.

Thanks!

@Wryhder
Copy link
Contributor Author

Wryhder commented Apr 4, 2025

@kingthorin My apologies. Missed a couple of spots.

@psiinon
Copy link
Member

psiinon commented Apr 9, 2025

I've updated the date to today and made some minor tweaks to address @thc202's feedback

@psiinon psiinon force-pushed the main branch 3 times, most recently from bb0f04d to 6582620 Compare April 9, 2025 10:26
block)

Signed-off-by: Simon Bennetts <psiinon@gmail.com>
@thc202 thc202 enabled auto-merge April 9, 2025 10:33
@thc202
Copy link
Member

thc202 commented Apr 9, 2025

Thank you both!

@thc202 thc202 merged commit a04c790 into zaproxy:main Apr 9, 2025
2 of 3 checks passed
@psiinon
Copy link
Member

psiinon commented Apr 9, 2025

Thanks @Wryhder and sorry this took so long to get published.
Its now live on https://www.zaproxy.org/blog/2025-04-09-portswigger-labs-broken-brute-force-protection-ip-block/ and shared via the usual social media sites 😁

@kingthorin
Copy link
Member

Thanks @Wryhder !!

@Wryhder
Copy link
Contributor Author

Wryhder commented Apr 9, 2025

@thc202 Thanks a bunch for your review.

Thanks @psiinon for making the changes. Article looks good, thank you! I'll repost.

And thanks again @kingthorin for your help with this.

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.

4 participants