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.

Sign up for GitHub

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

Jump to bottom

authhelper: SAST (SonarLint) Fixes #6236

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

authhelper: SAST (SonarLint) Fixes #6236

wants to merge 1 commit into from

Conversation

kingthorin
Copy link
Member

@kingthorin kingthorin commented Mar 2, 2025 •
edited
Loading

Overview

Address minor code issues identified by SAST (sonarlint):

  • Use variable assignment from isntanceof checks avoiding subsequent casts.
  • Return conditions more directly.
  • Remove unnecessary curly braces.
  • Remove unnecessary semi-colon.
  • Use method references vs lambda where possible.
  • Combine sequential ifs where possible.
  • Remove unnecessary toString() calls.
  • Remove unnecessary parameterization where diamond is fine.
  • Use super references where the distinction helps.
  • Use curly braces consistently.
  • Remove unnecessary found brackets in lambda use.
  • Remove unnecessary throw Exception declarations in Unit Tests.
  • Name some local variables no not "hide" other class variables.
  • Use String text blocks vs concatenation where possible.
  • Remove public declarations from Unit Test class or methods where unnecessary.
  • Remove unnecessary string concatenation.
  • Declare methods static where possible/practical.
  • Switch single parameter for loops to while where practical.

Related Issues

Specify any related issues or pull requests by linking to them.

Checklist

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

@psiinon
Copy link
Member

psiinon commented Mar 3, 2025 •
edited
Loading

Logo
Checkmarx One – Scan Summary & Details – 16f55ead-5f98-49dd-8e43-d99ed444c412

New Issues (1)

Checkmarx found the following issues in this Pull Request

Severity Issue Source File / Package Checkmarx Insight
LOW Log_Forging /addOns/authhelper/src/main/java/org/zaproxy/addon/authhelper/BrowserBasedAuthenticationMethodType.java: 807
detailsMethod at line 807 of /addOns/authhelper/src/main/java/org/zaproxy/addon/authhelper/BrowserBasedAuthenticationMethodType.java gets user input from...
Attack Vector
Fixed Issues (1)

Great job! The following issues were fixed in this Pull Request

Severity Issue Source File / Package
LOW Log_Forging /addOns/authhelper/src/main/java/org/zaproxy/addon/authhelper/BrowserBasedAuthenticationMethodType.java: 811

@kingthorin kingthorin force-pushed the ah-sast branch 2 times, most recently from 4804ad2 to 0fe1c7f Compare March 3, 2025 16:02
@kingthorin
Copy link
Member Author

The failure is CX, it's non-blocking.

@kingthorin
Copy link
Member Author

If we are moving ahead with further AuthHelper work could we get this one knocked off?

I don't think any of it is terribly controversial.

If I'm wrong then we can hold off but at least I'd know why 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@psiinon psiinon psiinon approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kingthorin @psiinon