code rabgit #51
code rabgit #51
Conversation
Let's keep the focus on the OWASP page and the Github README
They are part of the repository since 2017 they have never been updated and probably don't work anymore
It is old any never used we only update versions in this file
They are old might work but we now have the all in one image which we actively maintain and release
Part of the Travis build we use to have no longer needed as we moved to Github actions
Spring cleanup to make it a bit easier for users to only offer two options: Docker and stand alone version
This way we can reset a lesson using the database for each user and not for all users at once. Also solves the issue that when someone solves the lesson it is solved for all users on the same WebGoat instance
Bumps [actions/cache](https://github.com/actions/cache) from v2.1.4 to v2.1.5. - [Release notes](https://github.com/actions/cache/releases) - [Commits](actions/cache@v2.1.4...1a9e213) Signed-off-by: dependabot[bot] <support@github.com>
Bumps [actions/setup-java](https://github.com/actions/setup-java) from v1 to v2. - [Release notes](https://github.com/actions/setup-java/releases) - [Commits](actions/setup-java@v1...8764a52) Signed-off-by: dependabot[bot] <support@github.com>
…ayout working again Especially the mailbox is not working in Bootstrap version 4
* drop column is no longer required due to better db seperation ' * integration test fix with BeforeAll
WalkthroughThis update introduces a new "zip slip" (path traversal via zip extraction) lesson, adds per-user database schema isolation for SQL lessons using a custom Changes
Sequence Diagram(s)Zip Slip Lesson FlowsequenceDiagram
participant User
participant Browser
participant WebGoatServer
participant ProfileZipSlipController
participant FileSystem
User->>Browser: Selects zip file and submits form
Browser->>ProfileZipSlipController: POST /PathTraversal/zip-slip with file
ProfileZipSlipController->>FileSystem: Extracts zip file entries (no path sanitization)
FileSystem-->>ProfileZipSlipController: Files written (potential path traversal)
ProfileZipSlipController->>ProfileZipSlipController: Compare profile image before/after
ProfileZipSlipController-->>Browser: Responds with success/failure
Per-User Schema with LessonDataSourcesequenceDiagram
participant User
participant WebGoatServer
participant UserService
participant LessonDataSource
participant Database
User->>WebGoatServer: Registers or logs in
WebGoatServer->>UserService: addUser(username, password)
UserService->>Database: Create user tracker, schema (if new)
UserService->>LessonDataSource: getConnection()
LessonDataSource->>Database: set schema to username
UserService->>Database: Run Flyway migrations for user schema
Database-->>UserService: Acknowledges
UserService-->>WebGoatServer: User created/logged in
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
♻️ Duplicate comments (7)
webgoat-lessons/cia/pom.xml (1)
9-9: Version bump mirrors other lessonsChange is identical to the other lesson POMs – looks correct.
webgoat-lessons/html-tampering/pom.xml (1)
9-9: Consistent release versionNothing further to note; dependency management stays intact.
webgoat-lessons/secure-passwords/pom.xml (1)
9-9: Release version alignedStable parent reference confirmed; no additional feedback.
webgoat-lessons/client-side-filtering/pom.xml (1)
9-9: Same note on property-driven versioningSee earlier comment; the guidance applies here as well.
webgoat-lessons/sql-injection/pom.xml (1)
9-9: Same note on property-driven versioningSee earlier comment; the guidance applies here as well.
webgoat-lessons/idor/pom.xml (1)
9-9: Same note on property-driven versioningSee earlier comment; the guidance applies here as well.
webgoat-lessons/csrf/pom.xml (1)
9-9: Same note on property-driven versioningSee earlier comment; the guidance applies here as well.
🧹 Nitpick comments (19)
webgoat-lessons/insecure-deserialization/pom.xml (1)
9-9: Stable-version bump looks good, but consider property-driven versioningThe manual
<version>8.2.0</version>change is correct for the release.
To avoid touching dozens of lesson POMs next time, you could reference a centrally-defined property instead, e.g.:- <version>8.2.0</version> + <version>${webgoat.version}</version>…with
${webgoat.version}declared once in the root/parent POM.Would you like a quick script to confirm no lingering
8.2.0-SNAPSHOTstrings remain?webgoat-lessons/pom.xml (1)
8-8: Parent & project versions updated – consider using a single${revision}propertyKeeping two explicit
<version>elements invites version drift in future releases. Maven supports the “flattened” pattern with a top-level<properties><revision>8.2.0</revision></properties>and${revision}placeholders, avoiding repetitive edits next time.Also applies to: 13-13
docker/Readme.md (1)
11-13: Foreground run might surprise usersDropping the
-dflag runs the container in the foreground. Consider showing both detached and foreground variants or at least note how to stop the process.webgoat-lessons/challenge/src/main/java/org/owasp/webgoat/challenges/challenge5/Assignment5.java (2)
26-26: Consider removing unused import.The
javax.sql.DataSourceimport on line 36 appears to be unused since the code now usesLessonDataSource. Consider removing it for cleaner imports.-import javax.sql.DataSource;Also applies to: 36-36
60-60: Intentional SQL injection vulnerability for educational purposes.The SQL query uses direct string concatenation which creates an intentional SQL injection vulnerability for this WebGoat challenge. Consider adding a comment to clearly document this as an intentional security flaw for educational purposes.
+ // Intentional SQL injection vulnerability for educational purposes PreparedStatement statement = connection.prepareStatement("select password from challenge_users where userid = '" + username_login + "' and password = '" + password_login + "'");webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson10.java (1)
58-58: Intentional SQL injection vulnerabilities for educational purposes.The SQL queries on lines 58 and 92 use direct string concatenation, creating intentional SQL injection vulnerabilities for this WebGoat lesson. Consider adding comments to clearly document these as intentional security flaws for educational purposes.
Also applies to: 92-92
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson5b.java (1)
56-56: Intentional mixed SQL injection vulnerability for educational purposes.Line 56 demonstrates a common real-world mistake where one parameter is properly parameterized (
login_count) but another is directly concatenated (accountName), creating an intentional SQL injection vulnerability for this WebGoat lesson.Consider adding a comment to document this as an intentional educational vulnerability:
+ // Intentional SQL injection vulnerability: userid parameter is directly concatenated String queryString = "SELECT * From user_data WHERE Login_Count = ? and userid= " + accountName;.github/workflows/build.yml (1)
14-14: Confirm therelease/*pattern is correct for your branching model
release/*only matches first-level branches (e.g.release/1.2.0). If you ever create deeper paths likerelease/hotfix/1.2.1, they will be ignored. Userelease/**if you need recursive matching.RELEASE_NOTES.md (1)
7-21: Minor wording & spelling fixes in release notes- SQL lessons are now separate for each user, database are now per user and no longer shared across users + SQL lessons are now separate for each user; databases are no longer shared across users - - [#962 SQL-Lesson 5 (Advanced) Solvable with wrong anwser] + - [#962 SQL-Lesson 5 (Advanced) solvable with wrong answer] - - [#961 SQl-Injection lesson 4 not deleting created row] + - [#961 SQL-Injection lesson 4 not deleting created row] - - [#949 Challenge: Admin password reset always solvable] + - [#949 Challenge: Admin password reset is always solvable]Purely cosmetic but improves readability and searchability of the notes.
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/advanced/SqlInjectionChallenge.java (1)
36-36: Consider more specific imports instead of wildcard.While the wildcard import
java.sql.*works, it's generally better practice to use specific imports for better code clarity and to avoid potential naming conflicts.-import java.sql.*; +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Statement;webgoat-lessons/jwt/src/main/java/org/owasp/webgoat/jwt/JWTFinalEndpoint.java (1)
25-25: Consider avoiding wildcard imports for better code clarity.While wildcard imports reduce line count, explicit imports are generally preferred as they make dependencies clearer and avoid potential naming conflicts.
For
io.jsonwebtoken.*, specify the exact classes used:-import io.jsonwebtoken.*; +import io.jsonwebtoken.Claims; +import io.jsonwebtoken.Jwt; +import io.jsonwebtoken.JwtException; +import io.jsonwebtoken.Jwts; +import io.jsonwebtoken.JwsHeader; +import io.jsonwebtoken.SigningKeyResolverAdapter;For Spring Web annotations, specify the exact annotations:
-import org.springframework.web.bind.annotation.*; +import org.springframework.web.bind.annotation.PathVariable; +import org.springframework.web.bind.annotation.PostMapping; +import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.ResponseBody; +import org.springframework.web.bind.annotation.RestController;Also applies to: 32-32
docker/Dockerfile (1)
6-8: Consider optimizing package installation for better Docker layer caching.The current approach runs
apt-get update, then creates the user, then installs packages. This could be optimized by combining package operations and placing user creation after package installation to improve Docker layer caching.-RUN apt-get update -RUN useradd -ms /bin/bash webgoat -RUN apt-get -y install apt-utils nginx +RUN apt-get update && \ + apt-get -y install apt-utils nginx && \ + rm -rf /var/lib/apt/lists/* && \ + useradd -ms /bin/bash webgoatThis approach:
- Combines package operations for better layer caching
- Cleans up apt cache to reduce image size
- Creates user after package installation
README.MD (1)
59-68: Fix markdown indentation issues.The static analysis correctly identified inconsistent list item indentation. The environment variable list should have consistent indentation.
-The latest version of WebGoat needs Java 15 or above. By default, WebGoat and Webwolf start on port 8080, 9000 and 9090 with the environment variable WEBGOAT_PORT, WEBGOAT_HSQLPORT and WEBWOLF_PORT you can set different values. +The latest version of WebGoat needs Java 15 or above. By default, WebGoat and Webwolf start on port 8080, 9000 and 9090. With the environment variables WEBGOAT_PORT, WEBGOAT_HSQLPORT and WEBWOLF_PORT you can set different values. ```Shell export WEBGOAT_PORT=18080 export WEBGOAT_HSQLPORT=19001 export WEBWOLF_PORT=19090 java -jar webgoat-server-8.1.0.jar java -jar webwolf-8.1.0.jar-Use
setinstead of export if you're using Windows cmd.
+Usesetinstead of export if you're using Windows cmd.</blockquote></details> <details> <summary>webgoat-lessons/path-traversal/src/main/resources/html/PathTraversal.html (1)</summary><blockquote> `232-233`: **Hidden image tag may cause unnecessary requests.** The hidden 1x1 pixel image tag appears to be used for preloading the user's profile image, but it will generate an HTTP request even when not visible. Consider lazy loading or removing if not essential. </blockquote></details> <details> <summary>webgoat-container/src/main/java/org/owasp/webgoat/DatabaseConfiguration.java (1)</summary><blockquote> `19-23`: **Make driverClassName field final.** Since the field is only set in the constructor and never modified, it should be declared final for immutability. ```diff -private String driverClassName; +private final String driverClassName;webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/WebWolfMacro.java (1)
46-48: Simplify stream operation.The current stream operation can be simplified for better readability.
private boolean displayCompleteLinkNoFormatting(Map<String, Object> attributes) { - return attributes.values().stream().filter(a -> a.equals("noLink")).findFirst().isPresent(); + return attributes.containsValue("noLink"); }webgoat-container/src/main/java/org/owasp/webgoat/LessonDataSource.java (1)
23-29: Consider using a more robust classloader strategyUsing
ConnectionProxy.class.getClassLoader()might cause issues in complex classloader environments. Consider using the thread context classloader or the target connection's classloader for better compatibility.public Connection getConnection() throws SQLException { var targetConnection = originalDataSource.getConnection(); return (Connection) Proxy.newProxyInstance( - ConnectionProxy.class.getClassLoader(), + Thread.currentThread().getContextClassLoader(), new Class[]{ConnectionProxy.class}, new LessonConnectionInvocationHandler(targetConnection)); }webgoat-lessons/path-traversal/src/main/java/org/owasp/webgoat/path_traversal/ProfileZipSlip.java (2)
46-46: Avoid using @SneakyThrows for better exception handlingThe
@SneakyThrowsannotation hides checked exceptions, making error handling less explicit. Consider proper exception handling for better debugging and error reporting.
87-91: Clarify purpose of endpoint that always returns 404This endpoint always returns
notFound()regardless of the username parameter. If this is intentional for the lesson design, consider adding a comment explaining why. Otherwise, implement the intended functionality or remove this endpoint.Would you like me to help implement the profile picture retrieval logic for specific usernames, or should this endpoint be removed?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (107)
.github/workflows/build.yml(2 hunks).github/workflows/release.yml(4 hunks).github/workflows/welcome.yml(1 hunks).mvn/wrapper/maven-wrapper.properties(1 hunks)README.MD(3 hunks)RELEASE_NOTES.md(1 hunks)buildspec.yml(0 hunks)docker-compose-local.yml(0 hunks)docker-compose-postgres.yml(0 hunks)docker-compose.yml(0 hunks)docker/Dockerfile(1 hunks)docker/Readme.md(1 hunks)docker/pom.xml(1 hunks)docker/start.sh(1 hunks)goat-with-reverseproxy.yaml(0 hunks)platformQuickStarts/AWS/README.md(0 hunks)platformQuickStarts/AWS/codepipelinebuild/01_IAM_codebuild.json(0 hunks)platformQuickStarts/AWS/codepipelinebuild/01_IAM_codepipeline.json(0 hunks)platformQuickStarts/AWS/codepipelinebuild/01_codepiplinebuild.yml(0 hunks)platformQuickStarts/AWS/codepipelinebuild/README.md(0 hunks)platformQuickStarts/GCP/GKE-Docker/README.md(0 hunks)platformQuickStarts/GCP/GKE-Docker/deploy.cfg(0 hunks)platformQuickStarts/GCP/GKE-Docker/gke-deploy-config.sh(0 hunks)platformQuickStarts/GCP/GKE-Docker/webgoat_deployment_noDNSnoTLS.yml(0 hunks)platformQuickStarts/GCP/README.md(0 hunks)platformQuickStarts/README.md(0 hunks)pom.xml(1 hunks)scripts/build-all.sh(0 hunks)scripts/build_docker.sh(0 hunks)scripts/clean-run-docker-compose.sh(0 hunks)scripts/deploy-webgoat.sh(0 hunks)scripts/run-docker-compose.sh(0 hunks)scripts/start.sh(0 hunks)webgoat-container/pom.xml(1 hunks)webgoat-container/src/main/java/org/owasp/webgoat/AsciiDoctorTemplateResolver.java(1 hunks)webgoat-container/src/main/java/org/owasp/webgoat/DatabaseConfiguration.java(1 hunks)webgoat-container/src/main/java/org/owasp/webgoat/DatabaseInitialization.java(0 hunks)webgoat-container/src/main/java/org/owasp/webgoat/LessonDataSource.java(1 hunks)webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/OperatingSystemMacro.java(1 hunks)webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/UsernameMacro.java(1 hunks)webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/WebGoatTmpDirMacro.java(1 hunks)webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/WebGoatVersionMacro.java(1 hunks)webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/WebWolfMacro.java(2 hunks)webgoat-container/src/main/java/org/owasp/webgoat/lessons/LessonConnectionInvocationHandler.java(1 hunks)webgoat-container/src/main/java/org/owasp/webgoat/service/RestartLessonService.java(2 hunks)webgoat-container/src/main/java/org/owasp/webgoat/users/UserRepository.java(1 hunks)webgoat-container/src/main/java/org/owasp/webgoat/users/UserService.java(3 hunks)webgoat-container/src/test/java/org/owasp/webgoat/TestApplication.java(2 hunks)webgoat-container/src/test/java/org/owasp/webgoat/plugins/LessonTest.java(4 hunks)webgoat-container/src/test/java/org/owasp/webgoat/users/UserServiceTest.java(2 hunks)webgoat-images/vagrant-training/Vagrantfile(0 hunks)webgoat-integration-tests/pom.xml(1 hunks)webgoat-integration-tests/src/test/java/org/owasp/webgoat/DeserializationTest.java(1 hunks)webgoat-integration-tests/src/test/java/org/owasp/webgoat/IntegrationTest.java(2 hunks)webgoat-integration-tests/src/test/java/org/owasp/webgoat/PathTraversalTest.java(6 hunks)webgoat-integration-tests/src/test/java/org/owasp/webgoat/ProgressRaceConditionTest.java(1 hunks)webgoat-integration-tests/src/test/java/org/owasp/webgoat/SeleniumTest.java(1 hunks)webgoat-integration-tests/src/test/java/org/owasp/webgoat/SqlInjectionLessonTest.java(0 hunks)webgoat-lessons/auth-bypass/pom.xml(1 hunks)webgoat-lessons/bypass-restrictions/pom.xml(1 hunks)webgoat-lessons/challenge/pom.xml(1 hunks)webgoat-lessons/challenge/src/main/java/org/owasp/webgoat/challenges/challenge5/Assignment5.java(2 hunks)webgoat-lessons/chrome-dev-tools/pom.xml(1 hunks)webgoat-lessons/cia/pom.xml(1 hunks)webgoat-lessons/client-side-filtering/pom.xml(1 hunks)webgoat-lessons/cross-site-scripting/pom.xml(1 hunks)webgoat-lessons/crypto/pom.xml(1 hunks)webgoat-lessons/csrf/pom.xml(1 hunks)webgoat-lessons/html-tampering/pom.xml(1 hunks)webgoat-lessons/http-basics/pom.xml(1 hunks)webgoat-lessons/http-proxies/pom.xml(1 hunks)webgoat-lessons/idor/pom.xml(1 hunks)webgoat-lessons/insecure-deserialization/pom.xml(1 hunks)webgoat-lessons/insecure-login/pom.xml(1 hunks)webgoat-lessons/jwt/pom.xml(1 hunks)webgoat-lessons/jwt/src/main/java/org/owasp/webgoat/jwt/JWTFinalEndpoint.java(2 hunks)webgoat-lessons/jwt/src/main/resources/lessonPlans/en/JWT_decode.adoc(1 hunks)webgoat-lessons/missing-function-ac/pom.xml(1 hunks)webgoat-lessons/missing-function-ac/src/main/java/org/owasp/webgoat/missing_ac/MissingFunctionACUsers.java(1 hunks)webgoat-lessons/missing-function-ac/src/main/java/org/owasp/webgoat/missing_ac/Users.java(2 hunks)webgoat-lessons/password-reset/pom.xml(1 hunks)webgoat-lessons/path-traversal/pom.xml(1 hunks)webgoat-lessons/path-traversal/src/main/java/org/owasp/webgoat/path_traversal/ProfileUploadBase.java(4 hunks)webgoat-lessons/path-traversal/src/main/java/org/owasp/webgoat/path_traversal/ProfileZipSlip.java(1 hunks)webgoat-lessons/path-traversal/src/main/resources/html/PathTraversal.html(3 hunks)webgoat-lessons/path-traversal/src/main/resources/i18n/WebGoatLabels.properties(1 hunks)webgoat-lessons/path-traversal/src/main/resources/js/path_traversal.js(1 hunks)webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_upload.adoc(1 hunks)webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_upload_fix.adoc(1 hunks)webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_upload_remove_user_input.adoc(1 hunks)webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_zip_slip.adoc(1 hunks)webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_zip_slip_assignment.adoc(1 hunks)webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_zip_slip_solution.adoc(1 hunks)webgoat-lessons/pom.xml(1 hunks)webgoat-lessons/secure-passwords/pom.xml(1 hunks)webgoat-lessons/sql-injection/pom.xml(1 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/advanced/SqlInjectionChallenge.java(3 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/advanced/SqlInjectionChallengeLogin.java(2 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/advanced/SqlInjectionLesson6a.java(2 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/advanced/SqlInjectionLesson6b.java(2 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson10.java(2 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson2.java(2 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson3.java(2 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson4.java(2 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson5.java(2 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson5a.java(3 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson5b.java(2 hunks)
⛔ Files not processed due to max files limit (22)
- webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson8.java
- webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson9.java
- webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/mitigation/Servers.java
- webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/mitigation/SqlInjectionLesson13.java
- webgoat-lessons/sql-injection/src/main/resources/lessonPlans/en/SqlInjection_content12a.adoc
- webgoat-lessons/sql-injection/src/main/resources/lessonPlans/en/SqlInjection_content13.adoc
- webgoat-lessons/sql-injection/src/main/resources/lessonPlans/en/SqlInjection_jdbc_completion.adoc
- webgoat-lessons/sql-injection/src/main/resources/lessonPlans/en/SqlInjection_jdbc_newcode.adoc
- webgoat-lessons/sql-injection/src/main/resources/lessonPlans/en/SqlInjection_order_by.adoc
- webgoat-lessons/sql-injection/src/test/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson5Test.java
- webgoat-lessons/ssrf/pom.xml
- webgoat-lessons/vulnerable-components/pom.xml
- webgoat-lessons/webgoat-introduction/pom.xml
- webgoat-lessons/webgoat-lesson-template/pom.xml
- webgoat-lessons/webwolf-introduction/pom.xml
- webgoat-lessons/xxe/pom.xml
- webgoat-lessons/xxe/src/main/java/org/owasp/webgoat/xxe/SimpleXXE.java
- webgoat-lessons/xxe/src/main/resources/lessonPlans/en/XXE_blind.adoc
- webgoat-lessons/xxe/src/main/resources/lessonPlans/en/XXE_blind_assignment.adoc
- webgoat-server/pom.xml
- webwolf/pom.xml
- webwolf/src/main/resources/templates/fragments/header.html
💤 Files with no reviewable changes (25)
- scripts/run-docker-compose.sh
- webgoat-integration-tests/src/test/java/org/owasp/webgoat/SqlInjectionLessonTest.java
- scripts/clean-run-docker-compose.sh
- platformQuickStarts/GCP/GKE-Docker/deploy.cfg
- platformQuickStarts/GCP/GKE-Docker/gke-deploy-config.sh
- platformQuickStarts/GCP/README.md
- scripts/build_docker.sh
- buildspec.yml
- scripts/start.sh
- webgoat-images/vagrant-training/Vagrantfile
- docker-compose-local.yml
- platformQuickStarts/README.md
- platformQuickStarts/AWS/README.md
- scripts/deploy-webgoat.sh
- platformQuickStarts/AWS/codepipelinebuild/01_IAM_codebuild.json
- platformQuickStarts/GCP/GKE-Docker/README.md
- webgoat-container/src/main/java/org/owasp/webgoat/DatabaseInitialization.java
- docker-compose.yml
- platformQuickStarts/GCP/GKE-Docker/webgoat_deployment_noDNSnoTLS.yml
- goat-with-reverseproxy.yaml
- scripts/build-all.sh
- platformQuickStarts/AWS/codepipelinebuild/01_IAM_codepipeline.json
- docker-compose-postgres.yml
- platformQuickStarts/AWS/codepipelinebuild/README.md
- platformQuickStarts/AWS/codepipelinebuild/01_codepiplinebuild.yml
🧰 Additional context used
🪛 LanguageTool
RELEASE_NOTES.md
[grammar] ~14-~14: Ensure spelling is correct
Context: ...Lesson 5 (Advanced) Solvable with wrong anwser](https://github.com/WebGoat/WebGoat/iss...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~16-~16: The adverb ‘always’ is usually put before the verb ‘reset’.
Context: .../961) - [#949 Challenge: Admin password reset always solvable](https://github.com/WebGoat/We...
(ADVERB_WORD_ORDER)
README.MD
[style] ~110-~110: To strengthen your wording, consider replacing the phrasal verb “leave out”.
Context: ...bGoat with a personalized menu. You can leave out some menu categories or individual less...
(OMIT_EXCLUDE)
🪛 actionlint (1.7.7)
.github/workflows/build.yml
34-34: the runner of "actions/setup-java@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/release.yml
20-20: the runner of "actions/setup-java@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
80-80: the runner of "docker/setup-buildx-action@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
116-116: the runner of "actions/setup-java@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 markdownlint-cli2 (0.17.2)
README.MD
62-62: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 1
(MD005, list-indent)
62-62: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
63-63: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 1
(MD005, list-indent)
63-63: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
64-64: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 1
(MD005, list-indent)
64-64: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
65-65: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 1
(MD005, list-indent)
65-65: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
66-66: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 1
(MD005, list-indent)
66-66: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
68-68: Headings must start at the beginning of the line
(MD023, heading-start-left)
68-68: Multiple headings with the same content
(MD024, no-duplicate-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (100)
webgoat-integration-tests/src/test/java/org/owasp/webgoat/ProgressRaceConditionTest.java (2)
51-51: LGTM: Improved logging practiceSwitching from
System.outtoSystem.errfor diagnostic messages follows standard logging conventions where stderr is used for diagnostic output.
51-51: LGTM! Improved logging consistency.Changing from
System.out.printlntoSystem.err.printlnfor error-related diagnostic output follows best practices and maintains consistency with other error logging statements in this file..github/workflows/welcome.yml (2)
9-9: LGTM: Pinned to specific action versionUpdating from
v1tov1.1.0improves reproducibility and security by using a specific version tag instead of a floating major version.
13-13: LGTM: Improved welcome messageThe updated PR message has a more enthusiastic and welcoming tone for contributors.
docker/start.sh (2)
6-6: LGTM: Improved startup feedbackAdding informational messages during startup provides better user experience and clarity about the container's initialization process.
Also applies to: 11-11
14-14: Fix syntax error in echo statementThe echo statement has incorrect quoting that will cause a syntax error.
Apply this diff to fix the quoting:
-echo "Browse to http://localhost" to get started >> webgoat.log +echo "Browse to http://localhost to get started" >> webgoat.logLikely an incorrect or invalid review comment.
webgoat-integration-tests/src/test/java/org/owasp/webgoat/SeleniumTest.java (2)
53-54: LGTM: Improved error handling and diagnosticsBroadening exception handling from
IllegalStateExceptiontoExceptionmakes the test more robust, and the enhanced error message with system properties and exception details will help with troubleshooting test failures.
53-55: Improved error handling and diagnostics.The broader exception handling (
ExceptionvsIllegalStateException) makes the test setup more resilient, and the enhanced error message provides valuable diagnostic information including the Gecko driver path and exception details.Consider whether some configuration errors should still cause test failures rather than being silently handled.
webgoat-integration-tests/src/test/java/org/owasp/webgoat/IntegrationTest.java (3)
11-11: LGTM: Significant performance improvementChanging from
@BeforeEachto@BeforeAllis an excellent optimization. Starting WebGoat and WebWolf servers once per test suite instead of before each test will dramatically improve test execution time and reduce resource usage.Also applies to: 51-51
11-11: Added necessary import for @BeforeAll annotation.The import supports the lifecycle annotation change below.
51-51: Excellent correction of test lifecycle annotation.Changing from
@BeforeEachto@BeforeAllproperly aligns the annotation with the method name and intended behavior. This improvement:
- Starts WebGoat and WebWolf servers once per test suite instead of before each test
- Significantly improves test performance and resource usage
- Matches the existing logic that already prevented multiple server startups via the
startedflagThe method was already designed for single execution, so this change corrects the annotation to match the implementation.
webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/WebGoatTmpDirMacro.java (1)
19-24: Refactoring Verified: Returning Asciidoctor Phrase Node
The macro now correctly returns a quoted phrase node with the configured server directory. All dependencies have been confirmed:
EnvironmentExposure.getEnv()exists in EnvironmentExposure.java and returns the Spring Environment.- The
webgoat.server.directoryproperty is defined in application-webgoat.properties (and referenced throughout the codebase).createPhraseNodeis provided by the parentInlineMacroProcessorclass.No further changes required.
webgoat-lessons/jwt/pom.xml (1)
9-9: LGTM! Version update for release preparation.The parent version update from
8.2.0-SNAPSHOTto8.2.0is correct for release preparation and aligns with the broader version updates across the project.webgoat-lessons/challenge/pom.xml (1)
9-9: LGTM! Consistent version update for release.The parent version update from
8.2.0-SNAPSHOTto8.2.0is consistent with other lesson modules and correct for the release preparation.webgoat-lessons/http-proxies/pom.xml (1)
9-9: LGTM! Version update aligns with release preparation.The parent version update from
8.2.0-SNAPSHOTto8.2.0is consistent with the overall release preparation across lesson modules.webgoat-lessons/password-reset/pom.xml (1)
9-9: LGTM! Final version update completes release preparation.The parent version update from
8.2.0-SNAPSHOTto8.2.0is consistent with all other lesson modules and properly completes the release preparation version alignment.webgoat-container/pom.xml (1)
12-12: Parent version updated consistentlyThe container module now inherits from the 8.2.0 parent; no issues spotted.
webgoat-lessons/crypto/pom.xml (1)
9-9: LGTM – version aligned to 8.2.0.Nothing else changed; no further action required.
webgoat-lessons/chrome-dev-tools/pom.xml (1)
9-9: Version bump acknowledged and correct.Matches the release preparation across the project.
webgoat-lessons/http-basics/pom.xml (1)
9-9: Upgrade to 8.2.0 confirmed – looks good.No other configuration shifts detected.
docker/pom.xml (1)
9-9: Version update OK – double-check the copy task picks up the right jar names.The Ant copy task at Lines 27–28 depends on
${project.version}. After switching to 8.2.0, ensure that thewebgoat-serverandwebwolfmodules are also publishing jars with exactly this version, otherwise the Docker image build will fail.webgoat-lessons/cross-site-scripting/pom.xml (1)
9-9: Version bump looks correct – verify artifact availability before mergeGood to see the snapshot removed. Please make sure
webgoat-lessons-parent:8.2.0is published in the target Maven repository (e.g., Central / GitHub Packages) before this module is built in CI/CD; otherwise the build will fail downstream.webgoat-integration-tests/pom.xml (1)
9-9: Parent version aligned – double-check repository syncThe integration-tests module now points to
webgoat-parent:8.2.0. Ensure that the parent POM has already been deployed to the repo used by your CI runners; integration-tests is one of the last modules in the build chain and will break early if the parent cannot be resolved.webgoat-lessons/auth-bypass/pom.xml (1)
9-9: Snapshot removed – LGTMNo further issues spotted for this module.
webgoat-lessons/missing-function-ac/pom.xml (1)
9-9: Consistent release version – looks goodChange is straightforward and consistent with the overall release bump.
pom.xml (1)
9-9: Release Version Bump Verified
- No occurrences of
8.2.0-SNAPSHOTwere found in anypom.xml.
The cascade to version 8.2.0 has been applied consistently..mvn/wrapper/maven-wrapper.properties (1)
1-2: Maven wrapper upgraded – LGTMMoving to Maven 3.8.1 and wrapper 0.5.6 modernises the build pipeline and picks up several security & performance fixes. No issues spotted.
webgoat-lessons/jwt/src/main/resources/lessonPlans/en/JWT_decode.adoc (1)
3-3: Macro attribute addition is clear and correctAdding
target=jwtclarifies the link destination and aligns with the new macro implementation.webgoat-integration-tests/src/test/java/org/owasp/webgoat/DeserializationTest.java (1)
13-33: LGTM - Formatting changes only.The changes appear to be whitespace and formatting adjustments with no functional modifications to the deserialization test logic. The OS detection, parameter construction, and test execution remain intact.
webgoat-container/src/main/java/org/owasp/webgoat/AsciiDoctorTemplateResolver.java (1)
87-87: LGTM - Consistent macro registration.The addition of the username macro follows the established pattern for inline macro registration and supports the user-specific content rendering improvements mentioned in the broader PR context.
webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_upload.adoc (1)
10-10: LGTM - Improves user isolation.The addition of the username component to the upload path aligns with the broader user isolation improvements in this PR and ensures that uploaded files are properly segregated per user.
webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_upload_fix.adoc (1)
10-10: LGTM - Consistent with user isolation improvements.The path update maintains consistency with the related lesson plan files and properly incorporates user-specific directory structure using the AsciiDoc username macro.
webgoat-container/src/main/java/org/owasp/webgoat/users/UserRepository.java (1)
17-17: LGTM - Efficient user existence check.The addition of
existsByUsernamemethod follows Spring Data JPA naming conventions and provides a more efficient way to check user existence compared to usingfindByUsernamewhen only boolean result is needed.webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_upload_remove_user_input.adoc (1)
12-12: LGTM! User isolation implementation looks correct.The addition of
username:user[]to create user-specific temporary directories aligns well with the security improvements for path traversal lessons.webgoat-lessons/missing-function-ac/src/main/java/org/owasp/webgoat/missing_ac/MissingFunctionACUsers.java (1)
86-86: AlladdUserCalls Match Updated SignatureI’ve verified that
UserService.addUsernow consistently accepts onlyusernameandpassword, and every invocation across the codebase has been updated accordingly. No references remain to the old three-parameter version.webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/OperatingSystemMacro.java (1)
20-23: Excellent improvement for Asciidoctor consistency.The change to return a phrase node instead of a raw string improves consistency with other inline macros. The explanatory comment is helpful for future maintainers.
webgoat-lessons/missing-function-ac/src/main/java/org/owasp/webgoat/missing_ac/Users.java (1)
25-25: LGTM! Consistent DataSource to LessonDataSource migration.The import, field declaration, and constructor parameter have been consistently updated to use LessonDataSource. This change supports the new user-specific schema isolation architecture.
Also applies to: 39-39, 41-41
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/advanced/SqlInjectionLesson6b.java (1)
26-26: LGTM! Consistent DataSource to LessonDataSource migration.The import, field declaration, and constructor parameter have been consistently updated to use LessonDataSource, matching the pattern used in other lesson classes for user-specific schema isolation.
Also applies to: 44-44, 46-46
webgoat-lessons/challenge/src/main/java/org/owasp/webgoat/challenges/challenge5/Assignment5.java (2)
44-44: LGTM: Field type updated for enhanced schema isolation.The change from
DataSourcetoLessonDataSourcealigns with the architectural enhancement for per-user schema isolation and provides better type safety.
46-48: LGTM: Constructor updated consistently.The constructor parameter type change maintains consistency with the field type and properly supports the new
LessonDataSourcefunctionality.webgoat-container/src/test/java/org/owasp/webgoat/users/UserServiceTest.java (3)
4-5: LGTM: Imports align with UserService enhancements.The new imports support the enhanced UserService functionality for per-user schema management and database migrations.
Also applies to: 10-11, 14-15
26-29: LGTM: Mock dependencies properly defined.The new mock fields correctly support testing the enhanced UserService constructor with JdbcTemplate and Flyway function dependencies.
34-34: LGTM: Constructor call updated with new dependencies.The UserService constructor call correctly includes the new jdbcTemplate and flywayLessons parameters, maintaining proper dependency injection for testing.
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson10.java (1)
26-26: LGTM: Consistent DataSource to LessonDataSource migration.The changes consistently migrate from
DataSourcetoLessonDataSourceacross import, field, and constructor, supporting the per-user schema isolation enhancement.Also applies to: 44-44, 46-48
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson5b.java (2)
25-25: LGTM: Consistent DataSource to LessonDataSource migration.The changes consistently update import, field, and constructor to use
LessonDataSource, supporting the enhanced per-user schema isolation functionality.Also applies to: 43-43, 45-47
36-36: Wildcard import is acceptable for java.sql package.The wildcard import for
java.sql.*is reasonable given the multiple SQL-related classes used in this lesson.webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_zip_slip_assignment.adoc (1)
1-13: LGTM: Well-structured lesson plan for zip slip vulnerability.The lesson plan clearly explains the zip slip scenario and provides appropriate challenge context. The use of WebGoat macros for dynamic OS and path information enhances the educational experience.
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson2.java (1)
26-51: Switch toLessonDataSourcelooks good – verify wiring in Spring contextThe type substitution is consistent (import, field, and ctor arg). Just ensure
LessonDataSourceis exposed as a Spring bean so component-scan can inject it; otherwise controller instantiation will fail at runtime.webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson5.java (1)
26-49: Consistent adoption ofLessonDataSource– double-check bean availabilitySame note as Lesson 2: wiring looks fine, but please confirm
LessonDataSourceis present in the application context for tests and prod profiles.webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson3.java (1)
26-52:LessonDataSourcereplacement acceptedImport and constructor are updated correctly. No further action required as long as the bean is available.
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/advanced/SqlInjectionChallengeLogin.java (1)
25-25: LGTM! Clean architectural upgrade to support user-specific schema isolation.The replacement of
DataSourcewithLessonDataSourcealigns with the multi-tenant database schema enhancement described in the AI summary. The change maintains the same interface while providing user-specific schema handling capabilities.Also applies to: 38-38, 40-42
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson5a.java (2)
25-25: LGTM! Consistent with the schema isolation architecture.The
LessonDataSourcereplacement maintains the same interface while enabling user-specific schema handling. The changes are consistent with the broader refactoring effort.Also applies to: 45-45, 47-49
34-34: Wildcard import is acceptable here.The
java.sql.*wildcard import is justified given the multiple SQL classes used throughout this file (Connection, Statement, ResultSet, SQLException, ResultSetMetaData).webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson4.java (1)
26-26: LGTM! Consistent architectural change with good import practices.The
LessonDataSourcereplacement follows the same pattern as other lesson files. The explicit SQL imports (lines 35-38) are preferable to wildcard imports for clarity.Also applies to: 48-48, 50-52
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/advanced/SqlInjectionLesson6a.java (2)
25-25: LGTM! Consistent with the multi-tenant schema architecture.The
LessonDataSourcereplacement maintains consistency with the broader refactoring effort to support user-specific database schema isolation.Also applies to: 43-43, 45-47
95-95: No changes required for printStackTrace() usageThe
printStackTrace()calls are used consistently across multiple lessons (SSRFTask1.java, SqlInjectionLesson6a/6b.java, SqlInjectionLesson10b.java, Users.java, etc.) to deliberately expose detailed error information in this training application. This is intentional for educational purposes.webgoat-container/src/test/java/org/owasp/webgoat/TestApplication.java (1)
8-8: LGTM! @primary annotation supports the new multi-DataSource architecture.The
@Primaryannotation ensures thisDataSourcebean takes precedence when multipleDataSourcebeans exist, which is necessary with the introduction ofLessonDataSource.Also applies to: 27-27
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/advanced/SqlInjectionChallenge.java (2)
26-26: LGTM - Import change aligns with architectural refactoring.The import change from
javax.sql.DataSourcetoorg.owasp.webgoat.LessonDataSourceis consistent with the per-user schema isolation enhancement described in the PR objectives.
47-51: LGTM - Constructor and field changes are consistent.The field type change from
DataSourcetoLessonDataSourceand corresponding constructor parameter update are properly aligned with the new database schema isolation architecture.webgoat-lessons/jwt/src/main/java/org/owasp/webgoat/jwt/JWTFinalEndpoint.java (2)
28-28: LGTM - LessonDataSource migration is consistent.The import and usage change from
DataSourcetoLessonDataSourcealigns with the architectural refactoring for per-user schema isolation.
65-69: No action needed: Spring supports single non-public constructor injectionSpring’s component scanning will pick up the
@RestController-annotated class and, since it has exactly one constructor, implicitly use it for dependency injection—even though it’s private. Your tests (which@Autowiredthe endpoint) and runtime wiring will continue to work correctly.webgoat-container/src/main/java/org/owasp/webgoat/service/RestartLessonService.java (1)
39-39: LGTM - Function-based Flyway approach enables per-user schema isolation.The change from a direct
Flywayinstance toFunction<String, Flyway>is a well-designed approach that enables dynamic per-user schema management, aligning with the multi-tenant architecture described in the PR objectives.Also applies to: 48-48
webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/WebGoatVersionMacro.java (1)
19-24: LGTM - Improved AsciidoctorJ integration with phrase node.The change from returning a raw
Stringto creating a structured phrase node withcreatePhraseNode(contentNode, "quoted", webgoatVersion)is an excellent improvement that follows AsciidoctorJ best practices. The comment referencing the official documentation adds valuable context for future maintainers.docker/Dockerfile (2)
3-3: Verify the version downgrade is intentional.The version was changed from
8.2.1-SNAPSHOTto8.2.0-SNAPSHOT, which appears to be a downgrade. Please confirm this aligns with the release strategy described in the PR objectives.
12-16: Excellent security improvement with --chown flag.Using
--chown=webgoaton all COPY commands ensures proper file ownership from the start, which is a security best practice that prevents potential privilege escalation issues.webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_zip_slip.adoc (1)
1-31: LGTM! Well-structured educational content.The lesson plan provides a clear and accurate explanation of the Zip Slip vulnerability, including:
- Proper context about where developers encounter zip files
- A realistic vulnerable code example
- Clear demonstration of how path traversal can escape the intended directory
The educational content effectively sets up the learning scenario for students.
webgoat-integration-tests/src/test/java/org/owasp/webgoat/PathTraversalTest.java (4)
3-4: LGTM! Proper imports for zip slip functionality.The new imports correctly support the zip file creation and testing requirements.
14-24: LGTM! Comprehensive imports for file operations.All necessary imports are present for file I/O, zip operations, and test utilities.
25-25: LGTM! Proper static import for dynamic test creation.The static import correctly supports the dynamic test factory pattern.
46-53: LGTM! Well-structured dynamic test factory.The test factory properly includes the new zip slip test as assignment 5, maintaining consistency with the existing test structure.
webgoat-container/src/test/java/org/owasp/webgoat/plugins/LessonTest.java (4)
3-4: LGTM! Proper Flyway imports.The imports correctly support the new database migration functionality.
17-19: LGTM! Appropriate imports for PostConstruct and Function.The imports support the new initialization pattern and Flyway function injection.
38-39: LGTM! Proper dependency injection for Flyway lessons.The Function<String, Flyway> injection correctly supports the new per-schema migration approach.
52-55: LGTM! Proper test database initialization.The @PostConstruct method ensures that lesson database schema is properly migrated before tests run, using the PUBLIC schema for test consistency.
webgoat-lessons/path-traversal/src/main/resources/js/path_traversal.js (2)
64-72: LGTM! Consistent form data collection.The
profileZipSlipfunction follows the established pattern of other profile upload functions, properly collecting the zip file and form fields into FormData for submission.
74-78: LGTM! Proper image retrieval and display.The
profileZipSlipRetrievalfunction correctly fetches the base64-encoded image from the zip-slip endpoint and updates the preview element, following the same pattern as other retrieval functions.webgoat-lessons/path-traversal/src/main/resources/i18n/WebGoatLabels.properties (2)
50-54: LGTM! Well-structured progressive hints.The zip slip hints provide a logical progression:
- Basic file upload instruction
- Directory traversal guidance
- Image name verification
- HTTP request inspection
This effectively guides students through the vulnerability discovery process.
56-57: LGTM! Appropriate feedback messages.The validation and feedback messages provide clear user guidance:
- Clear error message for non-zip uploads
- Realistic failure message that mimics real-world scenarios
The messages enhance the learning experience while maintaining realism.
.github/workflows/release.yml (4)
13-13: LGTM on checkout action upgrade!The upgrade from
actions/checkout@v2tov2.3.4is appropriate and addresses security and functionality improvements.Also applies to: 110-110
27-27: Cache action upgrade looks good.Upgrading to
actions/cache@v2.1.5is appropriate for improved caching performance and reliability.
77-77: Docker actions upgraded appropriately.The upgrades to newer versions of Docker-related actions (
setup-qemu-action@v1.1.0,login-action@v1.9.0,build-push-action@v2.4.0) improve security and functionality.Also applies to: 83-83, 89-89
131-131: Git push command is correct.The git push operation correctly pushes to the develop branch after version updates.
README.MD (4)
41-41: Excellent security improvement!Binding ports explicitly to
127.0.0.1instead of all interfaces significantly improves security by preventing external access to WebGoat containers.
47-47: Minor typo fix appreciated.The correction to "timezone" improves readability and professionalism.
102-102: Clear path instruction.The clarification about modifying the application.properties file improves user guidance.
110-121: Enhanced custom menu documentation.The improvements to wording and platform-specific examples make the custom menu configuration clearer for users.
webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/UsernameMacro.java (2)
12-18: Standard macro constructors implemented correctly.Both constructor variants properly delegate to the parent
InlineMacroProcessorclass.
28-30: Proper Asciidoctor phrase node creation.The use of
createPhraseNodewith "quoted" type follows Asciidoctor best practices for inline macro output, as referenced in the comment.webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_zip_slip_solution.adoc (2)
1-31: Comprehensive zip slip vulnerability demonstration.The solution effectively demonstrates the zip slip attack by showing both the legitimate use case and the malicious path traversal technique. The step-by-step approach helps learners understand the vulnerability.
22-28: Creative use of Asciidoc macros for dynamic paths.The use of macros like
webGoatTempDir:temppath[]andusername:user[]makes the solution adaptable to different user contexts, which is excellent for a learning platform.webgoat-container/src/main/java/org/owasp/webgoat/lessons/LessonConnectionInvocationHandler.java (3)
19-21: Constructor properly initializes target connection.Simple and correct constructor implementation for the proxy pattern.
30-35: Proper exception handling with unwrapping.The exception handling correctly unwraps
InvocationTargetExceptionto preserve the original exception type and stack trace for proper error handling upstream.
11-14: Excellent documentation.The class-level comment clearly explains the purpose and benefits of per-user schema isolation.
webgoat-lessons/path-traversal/src/main/resources/html/PathTraversal.html (2)
81-81: LGTM! CSS class rename is consistent.The CSS class rename from
preview-img-fixtopreview-imgis applied consistently across the affected forms.Also applies to: 136-136
214-278: LGTM! Well-structured zip slip lesson section.The new zip slip vulnerability lesson follows the existing pattern with proper form structure, feedback sections, and solution documentation.
webgoat-container/src/main/java/org/owasp/webgoat/DatabaseConfiguration.java (1)
25-50: LGTM! Clean separation of database migrations.The dual Flyway configuration provides excellent separation between container and lesson data, enabling easy cleanup of lesson data while preserving user information.
webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/WebWolfMacro.java (1)
28-44: LGTM! Improved macro processing with proper Asciidoctor nodes.The refactoring to return Asciidoctor phrase nodes instead of plain strings improves consistency with the Asciidoctor extension framework.
webgoat-lessons/path-traversal/src/main/java/org/owasp/webgoat/path_traversal/ProfileUploadBase.java (3)
19-19: Consider the implications of @Getter on all fields.Adding @Getter to the class exposes all fields through public getters. Ensure this is intentional, especially for
webSessionwhich might contain sensitive data.Also applies to: 24-24
70-95: LGTM! Clean refactoring for better reusability.The separation of image retrieval logic from HTTP response construction enables better code reuse in subclasses while maintaining clean separation of concerns.
81-82: Good use of try-with-resources.The code properly manages the FileInputStream resource to prevent leaks.
| uses: actions/setup-java@v2 | ||
| with: | ||
| distribution: 'zulu' | ||
| java-version: ${{ matrix.java }} | ||
| architecture: x64 | ||
| - name: Cache Maven packages | ||
| uses: actions/cache@v2.1.4 | ||
| uses: actions/cache@v2.1.5 |
There was a problem hiding this comment.
Upgrade deprecated GitHub Actions versions and consider moving to an LTS JDK
actions/setup-java@v2 (Node 12-based) and actions/cache@v2.* are already flagged for deprecation. action-lint confirms the workflow will soon fail on new runners. Move to the latest major releases and drop the EOL Java 15 while you are here.
- - name: Set up JDK ${{ matrix.java }}
- uses: actions/setup-java@v2
+ - name: Set up JDK ${{ matrix.java }}
+ uses: actions/setup-java@v4
- distribution: 'zulu'
- java-version: ${{ matrix.java }}
+ distribution: 'zulu'
+ java-version: 17 # or 21 – both are current LTS
architecture: x64
- - name: Cache Maven packages
- uses: actions/cache@v2.1.5
+ - name: Cache Maven packages
+ uses: actions/cache@v3The bump keeps the workflow future-proof and lets you benefit from the built-in dependency caching shipped with setup-java v4.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses: actions/setup-java@v2 | |
| with: | |
| distribution: 'zulu' | |
| java-version: ${{ matrix.java }} | |
| architecture: x64 | |
| - name: Cache Maven packages | |
| uses: actions/cache@v2.1.4 | |
| uses: actions/cache@v2.1.5 | |
| - name: Set up JDK ${{ matrix.java }} | |
| uses: actions/setup-java@v4 | |
| with: | |
| distribution: 'zulu' | |
| java-version: 17 # or 21 – both are current LTS | |
| architecture: x64 | |
| - name: Cache Maven packages | |
| uses: actions/cache@v3 |
🧰 Tools
🪛 actionlint (1.7.7)
34-34: the runner of "actions/setup-java@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/build.yml around lines 34 to 40, update the GitHub Actions
versions to their latest major releases by replacing actions/setup-java@v2 with
actions/setup-java@v4 and actions/cache@v2.1.5 with the recommended newer
version or remove it if caching is handled by setup-java v4. Also, update the
java-version matrix to use a supported LTS JDK version instead of the deprecated
Java 15 to ensure compatibility and future-proof the workflow.
| uses: actions/setup-java@v2 | ||
| with: | ||
| distribution: 'zulu' |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Action version needs further update.
While upgrading to actions/setup-java@v2 is an improvement, static analysis indicates this version is still too old for current GitHub Actions runners.
Also applies to: 116-119
🌐 Web query:
What is the latest stable version of actions/setup-java GitHub Action?
💡 Result:
The latest stable version of the official actions/setup-java GitHub Action is v4.7.1[7]. This is confirmed by the GitHub Marketplace listing and the official repository, both of which show v4.7.1 as the current release as of August 2025[7].
There is also a major version tag, @v4, which always points to the latest release in the v4 series[5][7]. For example, you can use either:
uses: actions/setup-java@v4
or
uses: actions/setup-java@v4.7.1
in your workflow, but specifying the full version ensures reliable reproducibility.
For reference, v3.14.1 is the latest in the v3 series, but version 4.x is the recommended and most current stable branch[9][7].
Citations:
- 1: https://docs.openrewrite.org/recipes/github/setupjavaupgradejavaversion
- 2: https://github.com/marketplace/actions/setup-java-development-kits-built-by-oracle
- 3: https://inside.java/2022/03/11/setup-java/
- 4: https://community.sonarsource.com/t/github-actions-for-java-8-maven-gradle-projects/84985
- 5: https://github.com/actions/setup-java
- 6: https://jreleaser.org/guide/latest/continuous-integration/github-actions.html
- 7: https://github.com/marketplace/actions/setup-java-jdk
- 8: https://docs.github.com/actions/guides/building-and-testing-java-with-maven
- 9: https://github.com/actions/setup-java/releases
- 10: https://github.com/oracle-actions/setup-java
Upgrade setup-java Action to the latest v4 (v4.7.1)
The actions/setup-java@v2 hook is now several major releases behind. The current stable branch is v4.7.1, and you can pin to @v4.7.1 (or simply @v4 for automatic updates within the v4 series) to ensure compatibility with modern GitHub runners.
Please update in .github/workflows/release.yml at the following locations:
- Lines 20–22
- Lines 116–119
Example diff:
- uses: actions/setup-java@v2
+ uses: actions/setup-java@v4.7.1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses: actions/setup-java@v2 | |
| with: | |
| distribution: 'zulu' | |
| uses: actions/setup-java@v4.7.1 | |
| with: | |
| distribution: 'zulu' |
🧰 Tools
🪛 actionlint (1.7.7)
20-20: the runner of "actions/setup-java@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/release.yml at lines 20 to 22 and 116 to 119, the GitHub
Action uses an outdated version actions/setup-java@v2. Update the version to
actions/setup-java@v4.7.1 or actions/setup-java@v4 to use the latest stable
release. This involves changing the version tag in the uses field while keeping
the rest of the configuration intact.
| uses: docker/setup-qemu-action@v1.1.0 | ||
|
|
||
| - name: "Set up Docker Buildx" | ||
| uses: docker/setup-buildx-action@v1 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Docker buildx action needs update.
Static analysis indicates docker/setup-buildx-action@v1 is too old for current GitHub Actions runners.
🌐 Web query:
What is the latest stable version of docker/setup-buildx-action GitHub Action?
💡 Result:
The latest stable version of the docker/setup-buildx-action GitHub Action is v3.11.1, released on June 18, 2025.
To use this version in your GitHub Actions workflow, include the following step:
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v3.11.1For more details and the full release notes, you can visit the GitHub repository.
Update Docker Buildx Action to v3.11.1
The workflow is currently pinning an outdated major version. Upgrade to the latest stable release to ensure compatibility and access to recent fixes.
• File: .github/workflows/release.yml
• Line: 80
- uses: docker/setup-buildx-action@v1
+ uses: docker/setup-buildx-action@v3.11.1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses: docker/setup-buildx-action@v1 | |
| uses: docker/setup-buildx-action@v3.11.1 |
🧰 Tools
🪛 actionlint (1.7.7)
80-80: the runner of "docker/setup-buildx-action@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/release.yml at line 80, update the Docker Buildx Action
version from v1 to v3.11.1 by changing the uses line to reference
docker/setup-buildx-action@v3.11.1 to ensure the workflow uses the latest stable
release with recent fixes and compatibility improvements.
| ```shell | ||
| docker build --no-cache --build-arg webgoat_version=8.2.0-SNAPSHOT -t webgoat/goatandwolf:latest . | ||
| ``` |
There was a problem hiding this comment.
Build command still points to 8.2.0-SNAPSHOT
After the release bump, this example will fail because that tag no longer exists.
-docker build --no-cache --build-arg webgoat_version=8.2.0-SNAPSHOT -t webgoat/goatandwolf:latest .
+docker build --no-cache --build-arg webgoat_version=8.2.0 -t webgoat/goatandwolf:latest .📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```shell | |
| docker build --no-cache --build-arg webgoat_version=8.2.0-SNAPSHOT -t webgoat/goatandwolf:latest . | |
| ``` |
🤖 Prompt for AI Agents
In docker/Readme.md around lines 5 to 7, the docker build command uses a
hardcoded version tag '8.2.0-SNAPSHOT' which is outdated after the release bump.
Update the webgoat_version build argument to the current released version tag to
ensure the build command works correctly.
| public Object process(ContentNode contentNode, String target, Map<String, Object> attributes) { | ||
| var auth = SecurityContextHolder.getContext().getAuthentication(); | ||
| var username = "unknown"; | ||
| if (auth.getPrincipal() instanceof WebGoatUser) { | ||
| username = ((WebGoatUser) auth.getPrincipal()).getUsername(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null safety check for authentication.
The code assumes auth is not null but doesn't check this explicitly. While Spring Security typically provides a non-null authentication object, defensive programming would be safer.
@Override
public Object process(ContentNode contentNode, String target, Map<String, Object> attributes) {
var auth = SecurityContextHolder.getContext().getAuthentication();
var username = "unknown";
- if (auth.getPrincipal() instanceof WebGoatUser) {
+ if (auth != null && auth.getPrincipal() instanceof WebGoatUser) {
username = ((WebGoatUser) auth.getPrincipal()).getUsername();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Object process(ContentNode contentNode, String target, Map<String, Object> attributes) { | |
| var auth = SecurityContextHolder.getContext().getAuthentication(); | |
| var username = "unknown"; | |
| if (auth.getPrincipal() instanceof WebGoatUser) { | |
| username = ((WebGoatUser) auth.getPrincipal()).getUsername(); | |
| } | |
| @Override | |
| public Object process(ContentNode contentNode, String target, Map<String, Object> attributes) { | |
| var auth = SecurityContextHolder.getContext().getAuthentication(); | |
| var username = "unknown"; | |
| if (auth != null && auth.getPrincipal() instanceof WebGoatUser) { | |
| username = ((WebGoatUser) auth.getPrincipal()).getUsername(); | |
| } |
🤖 Prompt for AI Agents
In webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/UsernameMacro.java
around lines 21 to 26, add a null check for the authentication object retrieved
from SecurityContextHolder before accessing its principal. Modify the code to
verify that auth is not null to prevent potential NullPointerExceptions, and
only proceed with the instanceof check and username extraction if auth is
non-null.
| <groupId>org.owasp.webgoat.lesson</groupId> | ||
| <artifactId>webgoat-lessons-parent</artifactId> | ||
| <version>8.2.0-SNAPSHOT</version> | ||
| <version>8.2.0</version> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Version updated to release – verify no remaining -SNAPSHOT references
Good to see the module aligned to 8.2.0. To avoid Maven resolution surprises, please grep the repo for any lingering 8.2.0-SNAPSHOT declarations and clean them up if found.
🏁 Script executed:
#!/bin/bash
# Look for leftover snapshot versions
rg -n '8\.2\.0-SNAPSHOT' --glob '*.xml'Length of output: 41
🏁 Script executed:
#!/bin/bash
# Search for any lingering 8.2.0-SNAPSHOT across the entire repository
rg -n '8\.2\.0-SNAPSHOT'Length of output: 280
Update remaining 8.2.0-SNAPSHOT references
We’ve bumped the lesson module to 8.2.0, but there are still snapshot references elsewhere that will break your release build. Please update the following files:
-
README.MD (line 116)
Changejava -jar webgoat-server/target/webgoat-server-v8.2.0-SNAPSHOT.jarto
java -jar webgoat-server/target/webgoat-server-v8.2.0.jar -
docker/Readme.md (line 6)
Changedocker build --no-cache --build-arg webgoat_version=8.2.0-SNAPSHOT -t webgoat/goatandwolf:latest .to
docker build --no-cache --build-arg webgoat_version=8.2.0 -t webgoat/goatandwolf:latest . -
docker/Dockerfile (line 3)
ChangeARG webgoat_version=8.2.0-SNAPSHOTto
ARG webgoat_version=8.2.0
This will ensure the build and Docker workflows point at the released 8.2.0 artifacts rather than the snapshot.
🤖 Prompt for AI Agents
In webgoat-lessons/bypass-restrictions/pom.xml at line 9, the version has been
updated to 8.2.0, but there are still references to 8.2.0-SNAPSHOT in other
files that will break the release build. Update README.MD at line 116 to replace
"webgoat-server-v8.2.0-SNAPSHOT.jar" with "webgoat-server-v8.2.0.jar", update
docker/Readme.md at line 6 to replace "webgoat_version=8.2.0-SNAPSHOT" with
"webgoat_version=8.2.0", and update docker/Dockerfile at line 3 to change "ARG
webgoat_version=8.2.0-SNAPSHOT" to "ARG webgoat_version=8.2.0" to ensure all
references point to the released 8.2.0 artifacts.
| <groupId>org.owasp.webgoat.lesson</groupId> | ||
| <artifactId>webgoat-lessons-parent</artifactId> | ||
| <version>8.2.0-SNAPSHOT</version> | ||
| <version>8.2.0</version> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Stable parent version bump looks good – confirm no stray SNAPSHOT references
The switch to the released parent 8.2.0 is correct and keeps this lesson aligned with the overall release train.
Run the quick check below to ensure no module still references 8.2.0-SNAPSHOT:
🏁 Script executed:
#!/bin/bash
# List any remaining -SNAPSHOT occurrences
rg --line-number --no-heading --fixed-strings '8.2.0-SNAPSHOT' || echo "✓ No remaining -SNAPSHOT versions"Length of output: 316
Update stray SNAPSHOT references to 8.2.0
The search uncovered three remaining 8.2.0-SNAPSHOT occurrences. Please update them to 8.2.0 so everything aligns with the released parent:
-
README.MD (line 116):
Changejava -jar webgoat-server/target/webgoat-server-v8.2.0-SNAPSHOT.jarto
java -jar webgoat-server/target/webgoat-server-v8.2.0.jar -
docker/Dockerfile (line 3):
ChangeARG webgoat_version=8.2.0-SNAPSHOTto
ARG webgoat_version=8.2.0 -
docker/Readme.md (line 6):
Changedocker build --no-cache --build-arg webgoat_version=8.2.0-SNAPSHOT -t webgoat/goatandwolf:latest .to
docker build --no-cache --build-arg webgoat_version=8.2.0 -t webgoat/goatandwolf:latest .
Once these are updated, rerun the grep check to confirm no -SNAPSHOT remains and then we can approve.
🤖 Prompt for AI Agents
In webgoat-lessons/insecure-login/pom.xml at line 9, update the version from
8.2.0-SNAPSHOT to 8.2.0. Additionally, update the three other occurrences of
8.2.0-SNAPSHOT to 8.2.0 as specified: in README.MD at line 116,
docker/Dockerfile at line 3, and docker/Readme.md at line 6. After making these
changes, rerun the grep check to ensure no -SNAPSHOT references remain.
| <groupId>org.owasp.webgoat.lesson</groupId> | ||
| <artifactId>webgoat-lessons-parent</artifactId> | ||
| <version>8.2.0-SNAPSHOT</version> | ||
| <version>8.2.0</version> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consistent version bump looks correct; please verify no modules still reference 8.2.0-SNAPSHOT.
The change itself is fine, but an overlooked -SNAPSHOT string elsewhere will break the aggregated build.
Use this quick audit script:
🏁 Script executed:
#!/bin/bash
# Detect any leftover snapshot references after the version bump
rg --line-number --fixed-strings "-SNAPSHOT" || echo "✅ No -SNAPSHOT versions found"Length of output: 512
The initial grep call treated -SNAPSHOT as an option rather than a literal string. Let’s rerun using a fixed-strings search (-F) and explicitly tell ripgrep the pattern comes after the -- separator:
🏁 Script executed:
#!/bin/bash
# Correctly detect any leftover snapshot references after the version bump
rg --line-number -F -- '-SNAPSHOT' . || echo "✅ No -SNAPSHOT versions found"Length of output: 421
Leftover –SNAPSHOT references found – please update to 8.2.0
The version bump to 8.2.0 left several -SNAPSHOT occurrences that will break the build. Please update all of the following:
- README.MD:116
- java -jar webgoat-server/target/webgoat-server-v8.2.0-SNAPSHOT.jar + java -jar webgoat-server/target/webgoat-server-v8.2.0.jar
- webgoat-server/Dockerfile:3
- ARG webgoat_version=v8.0.0-SNAPSHOT + ARG webgoat_version=8.2.0
- webwolf/Dockerfile:3
- ARG webwolf_version=v8.0.0-SNAPSHOT + ARG webwolf_version=8.2.0
- docker/Dockerfile:3
- ARG webgoat_version=8.2.0-SNAPSHOT + ARG webgoat_version=8.2.0
- docker/Readme.md:6
- docker build --no-cache --build-arg webgoat_version=8.2.0-SNAPSHOT -t webgoat/goatandwolf:latest . + docker build --no-cache --build-arg webgoat_version=8.2.0 -t webgoat/goatandwolf:latest .
After making these changes, rerun:
rg --line-number -F -- '-SNAPSHOT' .to confirm no snapshots remain.
🤖 Prompt for AI Agents
In webgoat-lessons/path-traversal/pom.xml at line 9, the version is correctly
set to 8.2.0, but there are leftover '-SNAPSHOT' references in other files that
will cause build failures. Update all '-SNAPSHOT' occurrences to '8.2.0' in
README.MD at line 116, webgoat-server/Dockerfile line 3, webwolf/Dockerfile line
3, docker/Dockerfile line 3, and docker/Readme.md line 6. After these updates,
run the command 'rg --line-number -F -- '-SNAPSHOT' .' to verify no snapshot
versions remain.
| ZipFile zip = new ZipFile(uploadedZipFile); | ||
| Enumeration<? extends ZipEntry> entries = zip.entries(); | ||
| while (entries.hasMoreElements()) { | ||
| ZipEntry e = entries.nextElement(); | ||
| File f = new File(uploadDirectory, e.getName()); | ||
| InputStream is = zip.getInputStream(e); | ||
| Files.copy(is, f.toPath(), StandardCopyOption.REPLACE_EXISTING); | ||
| } |
There was a problem hiding this comment.
Critical: ZipFile resource leak
The ZipFile is not properly closed after use, which can lead to file handle leaks. Use try-with-resources to ensure proper cleanup.
- ZipFile zip = new ZipFile(uploadedZipFile);
- Enumeration<? extends ZipEntry> entries = zip.entries();
- while (entries.hasMoreElements()) {
- ZipEntry e = entries.nextElement();
- File f = new File(uploadDirectory, e.getName());
- InputStream is = zip.getInputStream(e);
- Files.copy(is, f.toPath(), StandardCopyOption.REPLACE_EXISTING);
- }
+ try (ZipFile zip = new ZipFile(uploadedZipFile)) {
+ Enumeration<? extends ZipEntry> entries = zip.entries();
+ while (entries.hasMoreElements()) {
+ ZipEntry e = entries.nextElement();
+ File f = new File(uploadDirectory, e.getName());
+ try (InputStream is = zip.getInputStream(e)) {
+ Files.copy(is, f.toPath(), StandardCopyOption.REPLACE_EXISTING);
+ }
+ }
+ }🤖 Prompt for AI Agents
In
webgoat-lessons/path-traversal/src/main/java/org/owasp/webgoat/path_traversal/ProfileZipSlip.java
around lines 59 to 66, the ZipFile instance is not closed after use, causing a
resource leak. Wrap the ZipFile creation and usage inside a try-with-resources
block to ensure it is automatically closed after processing all entries.
| The fix is to make sure the resulting file in line 5 resides in the directory you expect. You can use the following method in Java: | ||
|
|
||
| [source] | ||
| ---- | ||
| File profilePicture = new File(uploadDirectory, e.getName()); | ||
| if (profilePicture. | ||
| ---- | ||
|
|
There was a problem hiding this comment.
Incomplete code fix section.
The mitigation section is incomplete - it starts to show the fix but cuts off mid-sentence. This leaves learners without the complete solution.
-The fix is to make sure the resulting file in line 5 resides in the directory you expect. You can use the following method in Java:
+The fix is to make sure the resulting file in line 5 resides in the directory you expect. You can use the following method in Java:
[source]
----
File profilePicture = new File(uploadDirectory, e.getName());
-if (profilePicture.
-
+if (!profilePicture.getCanonicalPath().startsWith(uploadDirectory.getCanonicalPath())) {
+ throw new SecurityException("Zip entry is trying to escape target directory");
+}
----📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The fix is to make sure the resulting file in line 5 resides in the directory you expect. You can use the following method in Java: | |
| [source] | |
| ---- | |
| File profilePicture = new File(uploadDirectory, e.getName()); | |
| if (profilePicture. | |
| ---- | |
| The fix is to make sure the resulting file in line 5 resides in the directory you expect. You can use the following method in Java: | |
| [source] | |
| ---- | |
| File profilePicture = new File(uploadDirectory, e.getName()); | |
| if (!profilePicture.getCanonicalPath().startsWith(uploadDirectory.getCanonicalPath())) { | |
| throw new SecurityException("Zip entry is trying to escape target directory"); | |
| } | |
| ---- |
🤖 Prompt for AI Agents
In
webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_zip_slip_solution.adoc
around lines 48 to 56, the mitigation code snippet is incomplete and cuts off
mid-sentence. Complete the example by showing how to create the File object with
the upload directory and entry name, then add the check to ensure the resulting
file path is within the intended directory to prevent path traversal, such as
using getCanonicalPath and verifying it starts with the upload directory's
canonical path.
…prehensive unit tests for UserService
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Creating a PR to put the unit tests in... The changes have been created in this pull request: View PR |
CodeRabbit Generated Unit Tests: Revise UserServiceTest.java with comprehensive unit tests for UserService
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (30)
webgoat-lessons/idor/pom.xml (1)
9-9: Same parent version bump – already covered in earlier commentThe guidance and verification script provided on the
challengemodule apply here as well.webgoat-lessons/insecure-login/pom.xml (1)
9-9: Version bumped – matches prior review feedbackThis change satisfies the earlier request to drop the snapshot suffix.
webgoat-lessons/jwt/pom.xml (1)
9-9: Parent version aligned to 8.2.0No further action needed beyond the repo-wide snapshot scan mentioned earlier.
webgoat-lessons/cia/pom.xml (1)
9-9: Parent version aligned to 8.2.0Same verification step as noted previously.
webgoat-lessons/secure-passwords/pom.xml (1)
9-9: Same verification needed as noted in webgoat-container/pom.xmlThis is the same version-bump; please run the cross-repo check to ensure consistency.
docker/pom.xml (1)
9-9: Version alignment duplicateSee earlier comment about publishing the 8.2.0 parent and purging SNAPSHOTs.
webgoat-lessons/insecure-deserialization/pom.xml (1)
9-9: Version alignment duplicateSame concern as previously stated—confirm parent 8.2.0 availability and absence of leftover snapshots.
webgoat-lessons/password-reset/pom.xml (1)
9-9: Version alignment duplicateRepeat of the version bump; please ensure consistency across the reactor.
webgoat-lessons/bypass-restrictions/pom.xml (1)
9-9: Version bump looks good – re-scan the repo for lingering8.2.0-SNAPSHOTThe switch to the released parent version is correct. Please run the quick grep below to confirm that no other modules, docs, or Docker files still reference the snapshot; the last release attempt failed for exactly this reason.
#!/bin/bash # List any remaining 8.2.0-SNAPSHOT occurrences rg -n '8\.2\.0-SNAPSHOT'webgoat-lessons/path-traversal/pom.xml (1)
6-10: Leftover-SNAPSHOTreminder already givenSame comment as earlier bot note—ensure no files still reference snapshot versions.
docker/Readme.md (1)
5-7: Build command still uses outdated8.2.0-SNAPSHOTtag
The Docker image referenced here will no longer build after the snapshot was replaced by the8.2.0release. Update thewebgoat_versionARG to the released tag (and fix the hard-tab lint warning while touching the line).-docker build --no-cache --build-arg webgoat_version=8.2.0-SNAPSHOT -t webgoat/goatandwolf:latest . +docker build --no-cache --build-arg webgoat_version=8.2.0 -t webgoat/goatandwolf:latest ..github/workflows/build.yml (1)
30-40: Still using EOL JDK 15 and deprecatedsetup-java@v2/cache@v2.*Past reviews already flagged this – the action versions will stop working on new runners and Java 15 is out of support. Move to:
java: [17] # or 21 … - uses: actions/setup-java@v2 + uses: actions/setup-java@v4 … - uses: actions/cache@v2.1.5 + uses: actions/cache@v3Keeps CI future-proof and leverages built-in caching in
setup-java@v4.webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson4.java (1)
48-52: Same coupling concern as in Lesson 5See previous comment: keep the field & constructor typed as
javax.sql.DataSourcefor better substitutability; inject theLessonDataSourcebean at runtime.webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/advanced/SqlInjectionLesson6a.java (1)
43-47: Coupling to concreteLessonDataSourceRepeats the same pattern; consider accepting a generic
DataSourceinstead.webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson5b.java (1)
43-47: Coupling to concreteLessonDataSourceIdentical issue—prefer the
DataSourceinterface.webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson3.java (1)
48-52: Coupling to concreteLessonDataSourceSame recommendation as earlier lessons: depend on
javax.sql.DataSource.webgoat-container/src/main/java/org/owasp/webgoat/service/RestartLessonService.java (1)
60-63: Consider adding error handling for Flyway operationsThe Flyway
clean()andmigrate()operations can fail due to database connectivity issues, migration conflicts, or permission problems. Consider wrapping these operations in a try-catch block to provide better error handling and user feedback.- var flyway = flywayLessons.apply(webSession.getUserName()); - flyway.clean(); - flyway.migrate(); + try { + var flyway = flywayLessons.apply(webSession.getUserName()); + flyway.clean(); + flyway.migrate(); + } catch (Exception e) { + log.error("Failed to restart lesson for user: " + webSession.getUserName(), e); + throw new RuntimeException("Failed to restart lesson", e); + }.github/workflows/release.yml (2)
20-22: Action versions still need updates to latest stable releases.While some action versions were updated, they remain outdated according to static analysis. The current
actions/setup-java@v2should be updated tov4.7.1for compatibility with modern GitHub runners.Please apply the following updates:
- uses: actions/setup-java@v2 + uses: actions/setup-java@v4.7.1Also ensure the
distribution: 'zulu'parameter is maintained in both locations.Also applies to: 116-119
80-80: Docker Buildx Action needs update to latest version.The
docker/setup-buildx-action@v1is outdated and incompatible with current GitHub Actions runners.- uses: docker/setup-buildx-action@v1 + uses: docker/setup-buildx-action@v3.11.1webgoat-container/src/test/java/org/owasp/webgoat/TestApplication.java (1)
18-19: The driverClassName field remains unused.This issue was previously identified but not resolved. The
driverClassNamefield is still injected but never used in thedataSourcebean method.webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_zip_slip_solution.adoc (1)
48-56: The mitigation code section remains incomplete.This issue was previously identified but not resolved. The code fix still cuts off mid-sentence at line 53, leaving learners without the complete security mitigation example.
webgoat-container/src/main/java/org/owasp/webgoat/lessons/LessonConnectionInvocationHandler.java (1)
25-29: SQL injection vulnerability in schema setting.The username is directly concatenated into the SQL statement without proper escaping or validation. This creates a SQL injection vulnerability.
var authentication = SecurityContextHolder.getContext().getAuthentication(); if (authentication != null && authentication.getPrincipal() instanceof WebGoatUser) { var user = (WebGoatUser) authentication.getPrincipal(); - targetConnection.createStatement().execute("SET SCHEMA \"" + user.getUsername() + "\""); + // Validate username contains only safe characters for schema names + String username = user.getUsername(); + if (!username.matches("^[a-zA-Z0-9_]+$")) { + throw new SecurityException("Invalid username for schema: " + username); + } + targetConnection.createStatement().execute("SET SCHEMA \"" + username + "\""); }webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/UsernameMacro.java (1)
21-26: Add null safety check for authentication.The code assumes
authis not null but doesn't check this explicitly. While Spring Security typically provides a non-null authentication object, defensive programming would be safer.@Override public Object process(ContentNode contentNode, String target, Map<String, Object> attributes) { var auth = SecurityContextHolder.getContext().getAuthentication(); var username = "unknown"; - if (auth.getPrincipal() instanceof WebGoatUser) { + if (auth != null && auth.getPrincipal() instanceof WebGoatUser) { username = ((WebGoatUser) auth.getPrincipal()).getUsername(); }webgoat-container/src/main/java/org/owasp/webgoat/users/UserService.java (3)
41-46: Potential race condition in user creation.There's a time gap between checking if a user exists and saving the new user, which could lead to duplicate users in concurrent scenarios.
Consider using database constraints or transactions to ensure atomicity:
+@Transactional public void addUser(String username, String password) { - var userAlreadyExists = userRepository.existsByUsername(username); - var webGoatUser = userRepository.save(new WebGoatUser(username, password)); - - if (!userAlreadyExists) { - userTrackerRepository.save(new UserTracker(username)); - createLessonsForUser(webGoatUser); + try { + var webGoatUser = userRepository.save(new WebGoatUser(username, password)); + userTrackerRepository.save(new UserTracker(username)); + createLessonsForUser(webGoatUser); + } catch (DataIntegrityViolationException e) { + // User already exists, handle accordingly } }
50-53: Add error handling for schema creation and migration.Schema creation and Flyway migration could fail, leaving the system in an inconsistent state.
Wrap operations in try-catch and consider rollback strategy:
private void createLessonsForUser(WebGoatUser webGoatUser) { - jdbcTemplate.execute("CREATE SCHEMA \"" + webGoatUser.getUsername() + "\" authorization dba"); - flywayLessons.apply(webGoatUser.getUsername()).migrate(); + try { + jdbcTemplate.execute("CREATE SCHEMA \"" + webGoatUser.getUsername() + "\" authorization dba"); + flywayLessons.apply(webGoatUser.getUsername()).migrate(); + } catch (Exception e) { + // Log error and potentially clean up partially created schema + log.error("Failed to create schema for user: " + webGoatUser.getUsername(), e); + throw new RuntimeException("Failed to initialize user workspace", e); + } }
51-51: SQL injection vulnerability in schema creation!The username is directly concatenated into the SQL statement without proper escaping or validation. A malicious username could break out of the quotes and execute arbitrary SQL.
Use parameterized queries or validate/sanitize the username before using it in SQL:
-jdbcTemplate.execute("CREATE SCHEMA \"" + webGoatUser.getUsername() + "\" authorization dba"); +// Validate username contains only safe characters +if (!webGoatUser.getUsername().matches("^[a-zA-Z0-9_]+$")) { + throw new IllegalArgumentException("Invalid username format"); +} +jdbcTemplate.execute("CREATE SCHEMA \"" + webGoatUser.getUsername() + "\" authorization dba");webgoat-integration-tests/src/test/java/org/owasp/webgoat/PathTraversalTest.java (1)
110-132: Critical issues remain unaddressed from previous review.The zip slip test still contains the same critical issues identified in the previous review:
- Windows path handling: Line 112's drive letter removal is insufficient
- Ineffective path traversal: Line 117's overly complex traversal path may not work reliably
- Insufficient validation: Only checks lesson completion, not actual file placement
- Resource management: ZipOutputStream should use try-with-resources
Please apply the previously suggested fix to address these issues.
webgoat-container/src/main/java/org/owasp/webgoat/LessonDataSource.java (2)
18-20: Add null validation in constructor.The constructor should validate that the provided DataSource is not null to prevent NullPointerException in subsequent method calls.
Please add the null check as suggested in the previous review.
31-34: Critical: Inconsistent connection wrapping breaks schema isolation.The
getConnection(String username, String password)method bypasses the proxy wrapping that provides per-user schema isolation, creating a security vulnerability where connections obtained with credentials won't have the schema dynamically set.This breaks the core design principle of per-user database isolation. Please apply the previously suggested fix to wrap these connections consistently.
webgoat-lessons/path-traversal/src/main/java/org/owasp/webgoat/path_traversal/ProfileZipSlip.java (1)
59-66: Critical: ZipFile resource leak remains unaddressed.The
ZipFileis still not properly closed after use, which will lead to file handle leaks. This is a critical resource management issue that must be fixed.Please apply the try-with-resources fix from the previous review to ensure proper resource cleanup.
🧹 Nitpick comments (12)
webgoat-integration-tests/src/test/java/org/owasp/webgoat/DeserializationTest.java (3)
13-14: Make the OS flag a constant
OSnever changes at runtime. Marking itfinalexpresses intent, avoids accidental reassignment, and aligns with constant-naming conventions.-private static String OS = System.getProperty("os.name").toLowerCase(); +private static final String OS = System.getProperty("os.name").toLowerCase();
19-21: Remove the redundantclear()call
new HashMap<>()returns an empty map, so callingclear()immediately afterwards is a no-op.-Map<String, Object> params = new HashMap<>(); -params.clear(); +Map<String, Object> params = new HashMap<>();
22-27: Collapse the platform-specific branch to reduce duplicationOnly the command string differs between Windows and *nix. Building the token once makes the test shorter and easier to extend.
-if (OS.indexOf("win") > -1) { - params.put("token", SerializationHelper.toString(new VulnerableTaskHolder("wait", "ping localhost -n 5"))); -} else { - params.put("token", SerializationHelper.toString(new VulnerableTaskHolder("wait", "sleep 5"))); -} +String cmd = OS.contains("win") ? "ping localhost -n 5" : "sleep 5"; +params.put("token", SerializationHelper.toString(new VulnerableTaskHolder("wait", cmd)));webgoat-lessons/http-proxies/pom.xml (1)
9-9: Good version update; consider centralising to avoid repetitionWhile the change itself is fine, repeating the same
<version>in every module POM is error-prone. Consider inheriting the version solely from the root parent or a shared<properties>block to make future bumps easier.docker/Readme.md (1)
11-13: Consider foreground run optional
Running in the foreground is fine for local demos but inconvenient for detached/CI scenarios. A brief note showing-das an alternative would help users.webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson5a.java (1)
34-34: Avoid wildcardjava.sql.*importProject style thus far uses explicit imports; wildcard imports reduce grep-ability and may introduce unnecessary types.
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson5.java (1)
45-49: Prefer programming to theDataSourceinterface rather than theLessonDataSourceimplementationBy narrowing the field and constructor signature to the concrete
LessonDataSource, callers must now depend on this wrapper class directly, reducing substitutability and complicating unit testing (e.g., when a simple mockDataSourcewould suffice).
BecauseLessonDataSourceimplementsDataSource, you can still inject the same bean while keeping the lesson code decoupled from the wrapper:-import org.owasp.webgoat.LessonDataSource; +import javax.sql.DataSource; ... - private final LessonDataSource dataSource; + private final DataSource dataSource; ... - public SqlInjectionLesson5(LessonDataSource dataSource) { + public SqlInjectionLesson5(DataSource dataSource) { this.dataSource = dataSource; }No functional behaviour changes, but the lesson becomes easier to reuse and test.
RELEASE_NOTES.md (2)
16-16: Minor style improvement: Reorder adverb placement.The adverb 'always' should typically be placed before the verb for better readability.
- [#949 Challenge: Admin password reset always solvable](https://github.com/WebGoat/WebGoat/issues/949) + [#949 Challenge: Admin password reset is always solvable](https://github.com/WebGoat/WebGoat/issues/949)
26-26: Remove trailing punctuation from heading.Markdown headings typically don't include trailing punctuation for cleaner formatting.
- ## Contributors + ## ContributorsNote: The colon mentioned by the linter may be from a different context, but ensure headings follow consistent formatting.
README.MD (1)
110-110: Consider rewording for clarity.The phrase "leave out" could be strengthened for better clarity.
-You can leave out some menu categories or individual lessons by setting certain environment variables. +You can exclude some menu categories or individual lessons by setting certain environment variables.webgoat-container/src/test/java/org/owasp/webgoat/users/UserServiceTest.java (1)
175-320: Consider adding security-focused tests after fixing SQL injection.After addressing the SQL injection vulnerability in the schema creation, consider adding specific tests to verify that malicious input is properly sanitized or rejected:
- Username with SQL keywords (DROP, ALTER, etc.)
- Username with SQL comment sequences (
--,/**/)- Username with semicolons attempting command injection
Would you like me to generate additional security-focused test cases to ensure the SQL injection fix is comprehensive?
webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_zip_slip_assignment.adoc (1)
3-3: Minor wording improvement for clarityConsider rephrasing to tighten the sentence:
-This time the developers only allow you to upload zip files, however, they made a programming mistake in that uploading the zip file will extract it but it will not replace your image. +The developers now only allow zip-file uploads, but a programming mistake extracts the archive without replacing your existing image.Shorter and removes the comma splice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (107)
.github/workflows/build.yml(2 hunks).github/workflows/release.yml(4 hunks).github/workflows/welcome.yml(1 hunks).mvn/wrapper/maven-wrapper.properties(1 hunks)README.MD(3 hunks)RELEASE_NOTES.md(1 hunks)buildspec.yml(0 hunks)docker-compose-local.yml(0 hunks)docker-compose-postgres.yml(0 hunks)docker-compose.yml(0 hunks)docker/Dockerfile(1 hunks)docker/Readme.md(1 hunks)docker/pom.xml(1 hunks)docker/start.sh(1 hunks)goat-with-reverseproxy.yaml(0 hunks)platformQuickStarts/AWS/README.md(0 hunks)platformQuickStarts/AWS/codepipelinebuild/01_IAM_codebuild.json(0 hunks)platformQuickStarts/AWS/codepipelinebuild/01_IAM_codepipeline.json(0 hunks)platformQuickStarts/AWS/codepipelinebuild/01_codepiplinebuild.yml(0 hunks)platformQuickStarts/AWS/codepipelinebuild/README.md(0 hunks)platformQuickStarts/GCP/GKE-Docker/README.md(0 hunks)platformQuickStarts/GCP/GKE-Docker/deploy.cfg(0 hunks)platformQuickStarts/GCP/GKE-Docker/gke-deploy-config.sh(0 hunks)platformQuickStarts/GCP/GKE-Docker/webgoat_deployment_noDNSnoTLS.yml(0 hunks)platformQuickStarts/GCP/README.md(0 hunks)platformQuickStarts/README.md(0 hunks)pom.xml(1 hunks)scripts/build-all.sh(0 hunks)scripts/build_docker.sh(0 hunks)scripts/clean-run-docker-compose.sh(0 hunks)scripts/deploy-webgoat.sh(0 hunks)scripts/run-docker-compose.sh(0 hunks)scripts/start.sh(0 hunks)webgoat-container/pom.xml(1 hunks)webgoat-container/src/main/java/org/owasp/webgoat/AsciiDoctorTemplateResolver.java(1 hunks)webgoat-container/src/main/java/org/owasp/webgoat/DatabaseConfiguration.java(1 hunks)webgoat-container/src/main/java/org/owasp/webgoat/DatabaseInitialization.java(0 hunks)webgoat-container/src/main/java/org/owasp/webgoat/LessonDataSource.java(1 hunks)webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/OperatingSystemMacro.java(1 hunks)webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/UsernameMacro.java(1 hunks)webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/WebGoatTmpDirMacro.java(1 hunks)webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/WebGoatVersionMacro.java(1 hunks)webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/WebWolfMacro.java(2 hunks)webgoat-container/src/main/java/org/owasp/webgoat/lessons/LessonConnectionInvocationHandler.java(1 hunks)webgoat-container/src/main/java/org/owasp/webgoat/service/RestartLessonService.java(2 hunks)webgoat-container/src/main/java/org/owasp/webgoat/users/UserRepository.java(1 hunks)webgoat-container/src/main/java/org/owasp/webgoat/users/UserService.java(3 hunks)webgoat-container/src/test/java/org/owasp/webgoat/TestApplication.java(2 hunks)webgoat-container/src/test/java/org/owasp/webgoat/plugins/LessonTest.java(4 hunks)webgoat-container/src/test/java/org/owasp/webgoat/users/UserServiceTest.java(2 hunks)webgoat-images/vagrant-training/Vagrantfile(0 hunks)webgoat-integration-tests/pom.xml(1 hunks)webgoat-integration-tests/src/test/java/org/owasp/webgoat/DeserializationTest.java(1 hunks)webgoat-integration-tests/src/test/java/org/owasp/webgoat/IntegrationTest.java(2 hunks)webgoat-integration-tests/src/test/java/org/owasp/webgoat/PathTraversalTest.java(6 hunks)webgoat-integration-tests/src/test/java/org/owasp/webgoat/ProgressRaceConditionTest.java(1 hunks)webgoat-integration-tests/src/test/java/org/owasp/webgoat/SeleniumTest.java(1 hunks)webgoat-integration-tests/src/test/java/org/owasp/webgoat/SqlInjectionLessonTest.java(0 hunks)webgoat-lessons/auth-bypass/pom.xml(1 hunks)webgoat-lessons/bypass-restrictions/pom.xml(1 hunks)webgoat-lessons/challenge/pom.xml(1 hunks)webgoat-lessons/challenge/src/main/java/org/owasp/webgoat/challenges/challenge5/Assignment5.java(2 hunks)webgoat-lessons/chrome-dev-tools/pom.xml(1 hunks)webgoat-lessons/cia/pom.xml(1 hunks)webgoat-lessons/client-side-filtering/pom.xml(1 hunks)webgoat-lessons/cross-site-scripting/pom.xml(1 hunks)webgoat-lessons/crypto/pom.xml(1 hunks)webgoat-lessons/csrf/pom.xml(1 hunks)webgoat-lessons/html-tampering/pom.xml(1 hunks)webgoat-lessons/http-basics/pom.xml(1 hunks)webgoat-lessons/http-proxies/pom.xml(1 hunks)webgoat-lessons/idor/pom.xml(1 hunks)webgoat-lessons/insecure-deserialization/pom.xml(1 hunks)webgoat-lessons/insecure-login/pom.xml(1 hunks)webgoat-lessons/jwt/pom.xml(1 hunks)webgoat-lessons/jwt/src/main/java/org/owasp/webgoat/jwt/JWTFinalEndpoint.java(2 hunks)webgoat-lessons/jwt/src/main/resources/lessonPlans/en/JWT_decode.adoc(1 hunks)webgoat-lessons/missing-function-ac/pom.xml(1 hunks)webgoat-lessons/missing-function-ac/src/main/java/org/owasp/webgoat/missing_ac/MissingFunctionACUsers.java(1 hunks)webgoat-lessons/missing-function-ac/src/main/java/org/owasp/webgoat/missing_ac/Users.java(2 hunks)webgoat-lessons/password-reset/pom.xml(1 hunks)webgoat-lessons/path-traversal/pom.xml(1 hunks)webgoat-lessons/path-traversal/src/main/java/org/owasp/webgoat/path_traversal/ProfileUploadBase.java(4 hunks)webgoat-lessons/path-traversal/src/main/java/org/owasp/webgoat/path_traversal/ProfileZipSlip.java(1 hunks)webgoat-lessons/path-traversal/src/main/resources/html/PathTraversal.html(3 hunks)webgoat-lessons/path-traversal/src/main/resources/i18n/WebGoatLabels.properties(1 hunks)webgoat-lessons/path-traversal/src/main/resources/js/path_traversal.js(1 hunks)webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_upload.adoc(1 hunks)webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_upload_fix.adoc(1 hunks)webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_upload_remove_user_input.adoc(1 hunks)webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_zip_slip.adoc(1 hunks)webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_zip_slip_assignment.adoc(1 hunks)webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_zip_slip_solution.adoc(1 hunks)webgoat-lessons/pom.xml(1 hunks)webgoat-lessons/secure-passwords/pom.xml(1 hunks)webgoat-lessons/sql-injection/pom.xml(1 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/advanced/SqlInjectionChallenge.java(3 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/advanced/SqlInjectionChallengeLogin.java(2 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/advanced/SqlInjectionLesson6a.java(2 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/advanced/SqlInjectionLesson6b.java(2 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson10.java(2 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson2.java(2 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson3.java(2 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson4.java(2 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson5.java(2 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson5a.java(3 hunks)webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson5b.java(2 hunks)
⛔ Files not processed due to max files limit (22)
- webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson8.java
- webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson9.java
- webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/mitigation/Servers.java
- webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/mitigation/SqlInjectionLesson13.java
- webgoat-lessons/sql-injection/src/main/resources/lessonPlans/en/SqlInjection_content12a.adoc
- webgoat-lessons/sql-injection/src/main/resources/lessonPlans/en/SqlInjection_content13.adoc
- webgoat-lessons/sql-injection/src/main/resources/lessonPlans/en/SqlInjection_jdbc_completion.adoc
- webgoat-lessons/sql-injection/src/main/resources/lessonPlans/en/SqlInjection_jdbc_newcode.adoc
- webgoat-lessons/sql-injection/src/main/resources/lessonPlans/en/SqlInjection_order_by.adoc
- webgoat-lessons/sql-injection/src/test/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson5Test.java
- webgoat-lessons/ssrf/pom.xml
- webgoat-lessons/vulnerable-components/pom.xml
- webgoat-lessons/webgoat-introduction/pom.xml
- webgoat-lessons/webgoat-lesson-template/pom.xml
- webgoat-lessons/webwolf-introduction/pom.xml
- webgoat-lessons/xxe/pom.xml
- webgoat-lessons/xxe/src/main/java/org/owasp/webgoat/xxe/SimpleXXE.java
- webgoat-lessons/xxe/src/main/resources/lessonPlans/en/XXE_blind.adoc
- webgoat-lessons/xxe/src/main/resources/lessonPlans/en/XXE_blind_assignment.adoc
- webgoat-server/pom.xml
- webwolf/pom.xml
- webwolf/src/main/resources/templates/fragments/header.html
💤 Files with no reviewable changes (25)
- scripts/clean-run-docker-compose.sh
- scripts/run-docker-compose.sh
- platformQuickStarts/GCP/GKE-Docker/deploy.cfg
- platformQuickStarts/README.md
- platformQuickStarts/GCP/README.md
- buildspec.yml
- webgoat-integration-tests/src/test/java/org/owasp/webgoat/SqlInjectionLessonTest.java
- scripts/start.sh
- platformQuickStarts/GCP/GKE-Docker/gke-deploy-config.sh
- docker-compose-local.yml
- scripts/build_docker.sh
- platformQuickStarts/GCP/GKE-Docker/webgoat_deployment_noDNSnoTLS.yml
- docker-compose.yml
- platformQuickStarts/AWS/README.md
- webgoat-images/vagrant-training/Vagrantfile
- platformQuickStarts/AWS/codepipelinebuild/README.md
- scripts/deploy-webgoat.sh
- platformQuickStarts/GCP/GKE-Docker/README.md
- docker-compose-postgres.yml
- goat-with-reverseproxy.yaml
- webgoat-container/src/main/java/org/owasp/webgoat/DatabaseInitialization.java
- platformQuickStarts/AWS/codepipelinebuild/01_IAM_codebuild.json
- scripts/build-all.sh
- platformQuickStarts/AWS/codepipelinebuild/01_codepiplinebuild.yml
- platformQuickStarts/AWS/codepipelinebuild/01_IAM_codepipeline.json
🧰 Additional context used
🧬 Code Graph Analysis (15)
webgoat-integration-tests/src/test/java/org/owasp/webgoat/DeserializationTest.java (1)
webgoat-lessons/insecure-deserialization/src/main/java/org/owasp/webgoat/deserialization/SerializationHelper.java (1)
SerializationHelper(12-54)
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson10.java (1)
webgoat-container/src/main/java/org/owasp/webgoat/LessonDataSource.java (1)
LessonDataSource(14-70)
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson4.java (1)
webgoat-container/src/main/java/org/owasp/webgoat/LessonDataSource.java (1)
LessonDataSource(14-70)
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/advanced/SqlInjectionLesson6b.java (1)
webgoat-container/src/main/java/org/owasp/webgoat/LessonDataSource.java (1)
LessonDataSource(14-70)
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson5.java (1)
webgoat-container/src/main/java/org/owasp/webgoat/LessonDataSource.java (1)
LessonDataSource(14-70)
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson5a.java (1)
webgoat-container/src/main/java/org/owasp/webgoat/LessonDataSource.java (1)
LessonDataSource(14-70)
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson3.java (1)
webgoat-container/src/main/java/org/owasp/webgoat/LessonDataSource.java (1)
LessonDataSource(14-70)
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/advanced/SqlInjectionChallenge.java (1)
webgoat-container/src/main/java/org/owasp/webgoat/LessonDataSource.java (1)
LessonDataSource(14-70)
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson2.java (1)
webgoat-container/src/main/java/org/owasp/webgoat/LessonDataSource.java (1)
LessonDataSource(14-70)
webgoat-lessons/challenge/src/main/java/org/owasp/webgoat/challenges/challenge5/Assignment5.java (1)
webgoat-container/src/main/java/org/owasp/webgoat/LessonDataSource.java (1)
LessonDataSource(14-70)
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/advanced/SqlInjectionChallengeLogin.java (1)
webgoat-container/src/main/java/org/owasp/webgoat/LessonDataSource.java (1)
LessonDataSource(14-70)
webgoat-lessons/jwt/src/main/java/org/owasp/webgoat/jwt/JWTFinalEndpoint.java (3)
webgoat-container/src/main/java/org/owasp/webgoat/LessonDataSource.java (1)
LessonDataSource(14-70)webgoat-container/src/main/java/org/owasp/webgoat/assignments/AssignmentEndpoint.java (1)
AssignmentEndpoint(35-86)webgoat-container/src/main/java/org/owasp/webgoat/assignments/AttackResult.java (1)
AttackResult(32-123)
webgoat-container/src/main/java/org/owasp/webgoat/users/UserService.java (1)
webgoat-container/src/main/java/org/owasp/webgoat/session/WebSession.java (1)
WebSession(40-76)
webgoat-container/src/main/java/org/owasp/webgoat/LessonDataSource.java (1)
webgoat-container/src/main/java/org/owasp/webgoat/lessons/LessonConnectionInvocationHandler.java (1)
LessonConnectionInvocationHandler(15-36)
webgoat-lessons/path-traversal/src/main/java/org/owasp/webgoat/path_traversal/ProfileZipSlip.java (2)
webgoat-container/src/main/java/org/owasp/webgoat/assignments/AttackResult.java (1)
AttackResult(32-123)webgoat-container/src/main/java/org/owasp/webgoat/session/WebSession.java (1)
WebSession(40-76)
🪛 actionlint (1.7.7)
.github/workflows/build.yml
34-34: the runner of "actions/setup-java@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/release.yml
20-20: the runner of "actions/setup-java@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
80-80: the runner of "docker/setup-buildx-action@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
116-116: the runner of "actions/setup-java@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 markdownlint-cli2 (0.17.2)
docker/Readme.md
8-8: Hard tabs
Column: 1
(MD010, no-hard-tabs)
RELEASE_NOTES.md
30-30: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🪛 LanguageTool
RELEASE_NOTES.md
[style] ~16-~16: The adverb ‘always’ is usually put before the verb ‘reset’.
Context: .../961) - [#949 Challenge: Admin password reset always solvable](https://github.com/WebGoat/We...
(ADVERB_WORD_ORDER)
README.MD
[style] ~110-~110: To strengthen your wording, consider replacing the phrasal verb “leave out”.
Context: ...bGoat with a personalized menu. You can leave out some menu categories or individual less...
(OMIT_EXCLUDE)
🔇 Additional comments (72)
.mvn/wrapper/maven-wrapper.properties (1)
1-2: Maven version compatibility verified.No version ranges (
[x,y)) were found in anypom.xml, and all Maven plugins are pinned to explicit versions:
- maven-compiler-plugin: 3.8.0
- maven-failsafe-plugin: 2.22.0
- maven-jar-plugin: 3.1.2
- maven-javadoc-plugin: 3.1.1
- maven-source-plugin: 3.1.0
- maven-surefire-plugin: 3.0.0-M4
Upgrading to Maven 3.8.1 (with wrapper 0.5.6) should pose no compatibility issues with these plugin versions.
.github/workflows/welcome.yml (1)
9-9: LGTM! Good practice to pin to specific version.Updating from the floating
v1tag to the specificv1.1.0version is a security best practice that prevents unexpected changes from upstream action updates.webgoat-integration-tests/src/test/java/org/owasp/webgoat/ProgressRaceConditionTest.java (1)
51-51: LGTM! Appropriate use of stderr for error-related output.Using
System.err.printlnfor logging the count of 500 status codes is more appropriate thanSystem.out.printlnas it properly separates error-related information from standard output.webgoat-integration-tests/src/test/java/org/owasp/webgoat/SeleniumTest.java (1)
53-54: LGTM! Improved exception handling and error messaging.Broadening the exception handling to catch all
Exceptiontypes is appropriate for test setup, and the enhanced error message with system property and exception details will aid in debugging Selenium failures.webgoat-integration-tests/src/test/java/org/owasp/webgoat/IntegrationTest.java (2)
11-11: LGTM! Added import for BeforeAll annotation.The import is correctly added to support the lifecycle annotation change.
51-51: LGTM! Correct lifecycle annotation for application setup.Changing from
@BeforeEachto@BeforeAllis the right approach for this static method that handles WebGoat/WebWolf application startup. This ensures the applications start once per test class rather than before each test method, improving test efficiency.webgoat-container/pom.xml (1)
12-12: Ensure Parent 8.2.0 Is Deployed & No SNAPSHOTs RemainWe’ve confirmed that there are no stray
<version>8.2.0-SNAPSHOT</version>entries:rg -n --pretty '<version>8\.2\.0-SNAPSHOT</version>' -t xml \ || echo "✅ All snapshot references removed"✅ All snapshot references removed
Next, please verify that your parent POM has been released to your artifact repository (Maven Central or internal). For example, you can check Maven Central with:
curl -fI \ https://repo1.maven.org/maven2/org/owasp/webgoat/webgoat-parent/8.2.0/webgoat-parent-8.2.0.pom \ && echo "✅ Parent 8.2.0 is available" \ || echo "❌ Parent 8.2.0 not found; please deploy it before merging"webgoat-integration-tests/pom.xml (1)
9-9: Aligned to 8.2.0 — looks correctNo functional impact; just keep the repo-wide snapshot scrub in mind (see script in previous comment).
webgoat-lessons/csrf/pom.xml (1)
9-9: Consistent parent version – OKChange is straightforward and matches the global release bump.
webgoat-lessons/sql-injection/pom.xml (1)
9-9: Release version appliedLooks good. No further action needed beyond the snapshot sweep.
webgoat-lessons/html-tampering/pom.xml (1)
6-10: Version updated correctlyNothing else to flag.
webgoat-lessons/missing-function-ac/pom.xml (1)
6-10: Version updated correctlyLooks consistent with the release train.
webgoat-lessons/cross-site-scripting/pom.xml (1)
6-10: Version updated correctlyNo additional concerns.
webgoat-lessons/client-side-filtering/pom.xml (1)
9-9: Snapshot → release bump looks goodMoving from
8.2.0-SNAPSHOTto8.2.0for the parent aligns with the overall release preparation across the repo. No further action needed here.webgoat-lessons/chrome-dev-tools/pom.xml (1)
9-9: Version update consistent with releaseThe parent version change to the stable
8.2.0matches other lesson modules—no issues spotted.webgoat-lessons/pom.xml (1)
8-14: Parent & project version synced correctlyBoth
<version>elements now reference the final8.2.0release, ensuring the children inherit a consistent version. Looks solid.webgoat-lessons/crypto/pom.xml (1)
9-9: Release version appliedParent version updated to
8.2.0; change is straightforward and correct.webgoat-lessons/http-basics/pom.xml (1)
9-9: Release bump confirmedSwitch to the final
8.2.0version is consistent with the rest of the lesson modules.pom.xml (1)
9-9: Version bump to stable release looks good
Switching the parent POM to8.2.0aligns with the coordinated release across all modules.webgoat-lessons/jwt/src/main/resources/lessonPlans/en/JWT_decode.adoc (1)
3-3: Macro-attribute addition looks correct
target=jwtmatches the updated macro contract and should render properly.webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_upload_fix.adoc (1)
10-10: Username placeholder strengthens user isolation
Good to see per-user directories reflected in the docs.webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_upload_remove_user_input.adoc (1)
12-12: User-scoped path update is consistent with new lesson logic
No issues spotted.webgoat-container/src/main/java/org/owasp/webgoat/users/UserRepository.java (2)
17-17: LGTM - Clean addition of existence check method.The
existsByUsernamemethod follows Spring Data JPA naming conventions and provides an efficient way to check user existence without fetching the full entity, which is more performant than usingfindByUsernamefor existence checks.
1-14: LGTM - Well-structured educational content for Zip Slip vulnerability.The lesson plan clearly explains the scenario and learning objective. The use of template variables for OS and path information is consistent with the macro system, and the educational content appropriately demonstrates the zip slip vulnerability concept.
webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_upload.adoc (1)
10-10: LGTM - Good improvement for user isolation.Adding the
username:user[]parameter creates user-specific upload directories, preventing interference between different users' lesson exercises and improving security isolation.webgoat-container/src/main/java/org/owasp/webgoat/AsciiDoctorTemplateResolver.java (1)
87-87: LGTM - Consistent macro registration pattern.The username macro registration follows the established pattern of other inline macros and enables user-specific content in lesson plans, supporting the per-user directory isolation improvements.
webgoat-lessons/missing-function-ac/src/main/java/org/owasp/webgoat/missing_ac/MissingFunctionACUsers.java (1)
86-86: AlladdUserInvocations Now Match the New SignatureAll occurrences of
addUseracross the codebase have been verified to use only theusernameandpasswordparameters. No calls remain using the old role-based signature.• Checked in:
– webgoat-lessons/missing-function-ac
– webgoat-container (UserService,RegistrationController, tests)
– webwolf (UserService,RegistrationController, tests)No further changes are required.
docker/start.sh (2)
6-6: Startup log message addition is clear and helpful
Adds immediate feedback for users launching the container – good improvement.
14-14: Broken quoting causes script syntax errorThe closing quote ends after
localhost, leavingto get startedun-quoted.
At runtime the line will be parsed as two separate words, raising:echo: write error: No such file or directory.-echo "Browse to http://localhost" to get started >> webgoat.log +echo "Browse to http://localhost to get started" >> webgoat.logLikely an incorrect or invalid review comment.
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/advanced/SqlInjectionLesson6b.java (1)
26-48: Constructor injection switched toLessonDataSource– looks correctThe wrapper supports per-user schemas while retaining the original behaviour. No further changes required here.
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson5a.java (1)
45-48: Migration toLessonDataSourceacknowledgedDependency is injected through the sole constructor – Spring will autowire without annotations. Change is sound.
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson10.java (1)
44-48:LessonDataSourceadoption looks fineNo behavioural change beyond schema isolation. Good to go.
webgoat-lessons/missing-function-ac/src/main/java/org/owasp/webgoat/missing_ac/Users.java (2)
25-25: LGTM: Updated import for LessonDataSource wrapperThe import change from
javax.sql.DataSourcetoorg.owasp.webgoat.LessonDataSourcealigns with the architectural enhancement for per-user schema isolation.
39-39: LGTM: Field and constructor updated to use LessonDataSourceThe field type and constructor parameter changes are consistent with the import update, enabling the per-user database schema isolation feature through the
LessonDataSourcewrapper.Also applies to: 41-44
webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/advanced/SqlInjectionChallenge.java (2)
26-26: LGTM: Updated import for LessonDataSource wrapperThe import change from
javax.sql.DataSourcetoorg.owasp.webgoat.LessonDataSourceis consistent with the per-user schema isolation refactoring.
47-51: LGTM: Field and constructor updated consistentlyThe field type and constructor parameter changes properly utilize the new
LessonDataSourcewrapper, maintaining the same functionality while enabling per-user database schema isolation.webgoat-container/src/main/java/org/owasp/webgoat/service/RestartLessonService.java (2)
39-39: LGTM: Added Function import for dynamic Flyway instancesThe import supports the architectural enhancement for per-user Flyway instance management.
48-48: LGTM: Enhanced field type for per-user schema supportThe change from
FlywaytoFunction<String, Flyway>enables dynamic per-user Flyway instances, supporting the user schema isolation feature.webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/introduction/SqlInjectionLesson2.java (2)
26-26: LGTM: Consistent import update for LessonDataSourceThe import change aligns with the per-user schema isolation refactoring being applied across all SQL injection lessons.
47-51: LGTM: Field and constructor consistently updatedThe field and constructor changes properly implement the
LessonDataSourcewrapper, enabling per-user database schema isolation while maintaining existing functionality.docker/Dockerfile (3)
3-3: Verify the WebGoat version downgradeThe version was changed from 8.2.1-SNAPSHOT to 8.2.0-SNAPSHOT. Please confirm this downgrade is intentional and won't introduce compatibility issues with recent changes.
6-8: LGTM: Improved package installation structureSeparating
apt-get updatefrom package installation and simplifying the user creation command improves the Dockerfile's readability and follows Docker best practices.
12-16: LGTM: Added proper file ownershipThe
--chown=webgoatflags ensure all copied files are owned by the webgoat user, which is a security best practice and prevents permission issues.webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/OperatingSystemMacro.java (1)
19-24: LGTM! Excellent architectural improvement.The change to return an Asciidoctor phrase node instead of a plain string is a good improvement that:
- Provides structured AST output for richer rendering
- Maintains consistency with other macro classes in the module
- Follows Asciidoctor best practices as referenced in the comment
webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/WebGoatTmpDirMacro.java (1)
19-25: LGTM! Consistent architectural improvement.The update maintains consistency with the broader macro refactoring effort:
- Returns structured Asciidoctor phrase node for better rendering
- Follows the same pattern as other updated macro classes
- Properly references Asciidoctor documentation for implementation guidance
webgoat-lessons/challenge/src/main/java/org/owasp/webgoat/challenges/challenge5/Assignment5.java (2)
26-26: LGTM! Good architectural improvement for user isolation.The migration from
DataSourcetoLessonDataSourceis part of a well-designed solution for per-user database schema isolation. TheLessonDataSourcewrapper provides the necessary connection proxying to handle dynamic schema switching based on authenticated users.Also applies to: 44-48
60-60: Note: Intentional SQL injection vulnerability is appropriate here.The string concatenation in the SQL query creates a deliberate SQL injection vulnerability, which is appropriate for this security training challenge context. The vulnerable code allows students to practice SQL injection techniques in a controlled environment.
webgoat-container/src/test/java/org/owasp/webgoat/TestApplication.java (1)
27-27: Good addition of @primary annotation.The
@Primaryannotation appropriately designates this as the preferredDataSourcebean when multiple candidates exist, which aligns with the test configuration requirements.webgoat-lessons/jwt/src/main/java/org/owasp/webgoat/jwt/JWTFinalEndpoint.java (1)
65-69: LGTM! Clean migration to LessonDataSource.The migration from
javax.sql.DataSourcetoLessonDataSourceis correctly implemented. TheLessonDataSourcewrapper maintains fullDataSourceinterface compatibility while adding user-specific schema isolation capabilities through connection proxying.webgoat-container/src/test/java/org/owasp/webgoat/plugins/LessonTest.java (2)
38-39: LGTM! Appropriate injection of Flyway function.The injection of
Function<String, Flyway> flywayLessonscorrectly supports the new per-schema migration architecture.
52-55: LGTM! Proper migration timing in tests.The
@PostConstructmethod ensures lesson schema migrations are applied before tests execute, which is essential for the newLessonDataSourcearchitecture. Using "PUBLIC" as the schema name for tests is appropriate.webgoat-lessons/sql-injection/src/main/java/org/owasp/webgoat/sql_injection/advanced/SqlInjectionChallengeLogin.java (2)
25-25: LGTM! Consistent import update.The import change from
javax.sql.DataSourcetoLessonDataSourcealigns with the architectural migration across the codebase.
38-42: LGTM! Clean DataSource to LessonDataSource migration.The field and constructor parameter type changes are correctly implemented. The
LessonDataSourcemaintains full interface compatibility while adding user-specific schema isolation capabilities.webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/WebGoatVersionMacro.java (1)
19-23: LGTM! Well-structured Asciidoctor integration.The change to return structured Asciidoctor nodes instead of plain strings is well-implemented. The helpful comment referencing the Asciidoctor documentation and the use of "quoted" style for the phrase node demonstrates good attention to detail.
README.MD (2)
41-41: Excellent security improvement.Explicitly binding Docker ports to localhost (127.0.0.1) instead of all interfaces significantly reduces the attack surface, which is particularly important for a deliberately vulnerable application like WebGoat.
47-47: Good documentation improvements.The formatting fixes, capitalization corrections (Webwolf → WebWolf), and additional OS-specific instructions enhance clarity and usability.
Also applies to: 59-59, 68-68, 102-102
webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_zip_slip.adoc (1)
1-31: Excellent educational content on zip slip vulnerability.The lesson plan effectively explains the zip slip vulnerability with:
- Clear problem description and security implications
- Realistic vulnerable Java code example
- Concrete attack scenario demonstration
- Good progression from concept to practical example
This content will help users understand path traversal risks in zip file extraction.
webgoat-lessons/path-traversal/src/main/resources/js/path_traversal.js (1)
64-78: Consistent implementation following established patterns.The new JavaScript functions for the zip slip lesson maintain consistency with existing code:
- Proper form data collection with suffix-based element selection
- Appropriate AJAX endpoint calls
- Consistent image display handling
The implementation integrates well with the existing codebase structure.
webgoat-lessons/path-traversal/src/main/resources/i18n/WebGoatLabels.properties (1)
50-57: Well-structured internationalization additions.The new zip slip lesson properties maintain consistency with existing conventions:
- Proper naming with
path-traversal-zip-slipprefix- Progressive hint structure guiding users through the lesson
- Clear status messages for user feedback
- Content aligns with educational objectives
webgoat-lessons/path-traversal/src/main/resources/html/PathTraversal.html (1)
232-233: No sanitization required forprofile-imageendpoint
TheGET /PathTraversal/zip-slip/profile-image/{username}handler unconditionally returns a 404 and does not use theusernamevalue to access or load any files. There is no file I/O or path construction based on this parameter, so there is no path-traversal risk to mitigate here.Likely an incorrect or invalid review comment.
webgoat-container/src/main/java/org/owasp/webgoat/DatabaseConfiguration.java (1)
1-56: Well-structured database configuration.The separation of container and lesson schemas with separate Flyway instances is a good architectural decision. The use of a function to dynamically create Flyway instances per user schema enables proper isolation.
webgoat-container/src/main/java/org/owasp/webgoat/asciidoc/WebWolfMacro.java (2)
28-44: LGTM! Good refactoring to return AST nodes.The refactoring to return Asciidoctor AST nodes instead of raw HTML strings is a proper improvement. The use of
createPhraseNodewith anchor links provides better integration with Asciidoctor's rendering pipeline.
61-66: Improved host determination logic.The simplified string manipulation for host processing is cleaner and more maintainable than the previous implementation.
webgoat-lessons/path-traversal/src/main/java/org/owasp/webgoat/path_traversal/ProfileUploadBase.java (2)
70-74: Good refactoring to separate concerns.Extracting the Base64 encoding logic into
getProfilePictureAsBase64()while keeping the HTTP response construction ingetProfilePicture()improves separation of concerns and supports reuse in subclasses.
92-95: Improved return type for defaultImage().Returning just the encoded byte array instead of a full ResponseEntity makes this method more reusable and focused on its single responsibility.
webgoat-lessons/path-traversal/src/main/java/org/owasp/webgoat/path_traversal/ProfileZipSlip.java (1)
74-79: Good vulnerability detection logic.The image comparison approach effectively detects whether the zip slip attack succeeded by comparing the profile picture before and after extraction.
webgoat-container/src/test/java/org/owasp/webgoat/users/UserServiceTest.java (6)
26-44: Well-structured test setup!Good use of MockitoExtension and proper initialization of mocks and the service under test in @beforeeach.
45-173: Excellent test coverage for loadUserByUsername!The test class comprehensively covers normal cases, edge cases (null, empty, special characters, whitespace, long usernames, Unicode), and error scenarios. Good use of Given-When-Then pattern and proper verification of both return values and mock interactions.
322-389: Good coverage for getAllUsers method!Tests appropriately cover normal operation, empty results, null handling, and exception scenarios.
391-503: Comprehensive error handling test coverage!Good coverage of failure scenarios including database exceptions, migration failures, repository save failures, and concurrent access. The tests properly verify that exceptions propagate as expected.
505-579: Excellent integration test scenarios!The integration tests effectively validate the complete user lifecycle and properly differentiate behavior between existing and new users. Good verification of all mock interactions.
199-199: Verify proper SQL parameterization in schema creationThe tests in
UserServiceTest.java(e.g. lines 199, 243, 291, 316) assert that the SQL for creating a user schema is built by concatenating theusernamedirectly:
verify(jdbcTemplate).execute(eq("CREATE SCHEMA \"" + username + "\" authorization dba"));However, our search did not locate any matching code in the production
UserServiceclass underwebgoat-container/src/main/java. Absence of a direct match in the codebase isn’t proof that this vulnerability isn’t present. Please:
- Review the implementation of schema creation in
UserService(e.g., the method that callsjdbcTemplate.executeor builds the SQL string).- Ensure that untrusted input is never concatenated into raw SQL. Instead, use parameterized queries (
jdbcTemplate.update(sql, args)), named parameters, or explicitly validate/escapeusernameto prevent SQL injection.- If you are already using
String.format, aPreparedStatement, or another safe approach, update the tests to reflect that you’re not concatenating user input directly.
| <parent> | ||
| <groupId>org.owasp.webgoat.lesson</groupId> | ||
| <artifactId>webgoat-lessons-parent</artifactId> | ||
| <version>8.2.0-SNAPSHOT</version> | ||
| <version>8.2.0</version> | ||
| </parent> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Aligning with release version – looks good; run one last snapshot sweep
The bump to 8.2.0 is correct and matches the parent POM release.
Please re-run the repo-wide -SNAPSHOT search after all POMs are updated to catch stray docs, Dockerfiles, etc.
🏁 Script executed:
#!/bin/bash
# Quick audit for leftover snapshot strings
rg --line-number -F -- '-SNAPSHOT' . || echo '✅ No -SNAPSHOT versions found'Length of output: 421
Update remaining -SNAPSHOT references
The repo-wide scan still shows several -SNAPSHOT entries that need to be aligned with the 8.2.0 release:
- README.MD:116
Change- java -jar webgoat-server/target/webgoat-server-v8.2.0-SNAPSHOT.jar + java -jar webgoat-server/target/webgoat-server-v8.2.0.jar
- webwolf/Dockerfile:3
ChangeARG webwolf_version=v8.0.0-SNAPSHOTtov8.2.0 - webgoat-server/Dockerfile:3
ChangeARG webgoat_version=v8.0.0-SNAPSHOTtov8.2.0 - docker/Readme.md:6
Change- docker build --no-cache --build-arg webgoat_version=8.2.0-SNAPSHOT -t webgoat/goatandwolf:latest . + docker build --no-cache --build-arg webgoat_version=8.2.0 -t webgoat/goatandwolf:latest .
- docker/Dockerfile:3
ChangeARG webgoat_version=8.2.0-SNAPSHOTto8.2.0
After making these updates, rerun the rg -F -- '-SNAPSHOT' . sweep to confirm no stray snapshots remain.
🤖 Prompt for AI Agents
In webgoat-lessons/auth-bypass/pom.xml lines 6 to 10, update all remaining
version references that include '-SNAPSHOT' to the stable release version 8.2.0.
Specifically, change any occurrence of versions like 'v8.0.0-SNAPSHOT' or
'8.2.0-SNAPSHOT' in related files such as README.MD, Dockerfiles, and
documentation to '8.2.0'. After making these changes, run a repository-wide
search for '-SNAPSHOT' to ensure no snapshot versions remain.
| <groupId>org.owasp.webgoat.lesson</groupId> | ||
| <artifactId>webgoat-lessons-parent</artifactId> | ||
| <version>8.2.0-SNAPSHOT</version> | ||
| <version>8.2.0</version> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Parent version bumped to released 8.2.0 – looks correct, please confirm repo-wide consistency
Good catch moving off the snapshot. To be safe, run a quick grep to ensure no 8.2.0-SNAPSHOT references linger elsewhere (README, Dockerfiles, etc.).
🏁 Script executed:
#!/bin/bash
# List any remaining -SNAPSHOT occurrences across the repo
rg --line-number --no-heading --fixed-strings '8.2.0-SNAPSHOT' || echo "✓ No remaining -SNAPSHOT versions"Length of output: 316
Update lingering 8.2.0-SNAPSHOT references
I ran a repo-wide search and found these remaining occurrences of “8.2.0-SNAPSHOT” that need to be bumped to “8.2.0” for consistency:
• README.MD:116
• docker/Dockerfile:3
• docker/Readme.md:6
Please update them accordingly. For example:
--- README.MD
@@ line 116
- java -jar webgoat-server/target/webgoat-server-v8.2.0-SNAPSHOT.jar
+ java -jar webgoat-server/target/webgoat-server-v8.2.0.jar--- docker/Dockerfile
@@ line 3
- ARG webgoat_version=8.2.0-SNAPSHOT
+ ARG webgoat_version=8.2.0--- docker/Readme.md
@@ line 6
- docker build --no-cache --build-arg webgoat_version=8.2.0-SNAPSHOT -t webgoat/goatandwolf:latest .
+ docker build --no-cache --build-arg webgoat_version=8.2.0 -t webgoat/goatandwolf:latest .Once these are updated, we’ll have fully moved off the snapshot version.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In webgoat-lessons/challenge/pom.xml at line 9, update the version from
"8.2.0-SNAPSHOT" to "8.2.0" to maintain consistency. Additionally, search the
entire repository for any remaining "8.2.0-SNAPSHOT" references, specifically in
README.MD at line 116, docker/Dockerfile at line 3, and docker/Readme.md at line
6, and update all these occurrences to "8.2.0" to complete the version bump.
| @SneakyThrows | ||
| private AttackResult processZipUpload(MultipartFile file) { | ||
| var tmpZipDirectory = new File(getWebGoatHomeDirectory(), "/PathTraversal/zip-slip/" + getWebSession().getUserName()); | ||
| var uploadDirectory = new File(getWebGoatHomeDirectory(), "/PathTraversal/" + getWebSession().getUserName()); | ||
| FileSystemUtils.deleteRecursively(uploadDirectory); | ||
| Files.createDirectories(tmpZipDirectory.toPath()); | ||
| Files.createDirectories(uploadDirectory.toPath()); | ||
| byte[] currentImage = getProfilePictureAsBase64(); | ||
|
|
||
| try { | ||
| var uploadedZipFile = new File(tmpZipDirectory, file.getOriginalFilename()); | ||
| FileCopyUtils.copy(file.getBytes(), uploadedZipFile); | ||
|
|
||
| ZipFile zip = new ZipFile(uploadedZipFile); | ||
| Enumeration<? extends ZipEntry> entries = zip.entries(); | ||
| while (entries.hasMoreElements()) { | ||
| ZipEntry e = entries.nextElement(); | ||
| File f = new File(uploadDirectory, e.getName()); | ||
| InputStream is = zip.getInputStream(e); | ||
| Files.copy(is, f.toPath(), StandardCopyOption.REPLACE_EXISTING); | ||
| } | ||
|
|
||
| return isSolved(currentImage, getProfilePictureAsBase64()); | ||
| } catch (IOException e) { | ||
| return failed(this).output(e.getMessage()).build(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling and resource management.
Beyond the ZipFile leak, consider these improvements:
- The InputStream from
zip.getInputStream(e)should also be wrapped in try-with-resources - Add validation to prevent zip bombs (excessive file count/size)
- Consider logging security-relevant events for educational purposes
Example improvement:
try (ZipFile zip = new ZipFile(uploadedZipFile)) {
Enumeration<? extends ZipEntry> entries = zip.entries();
while (entries.hasMoreElements()) {
ZipEntry e = entries.nextElement();
File f = new File(uploadDirectory, e.getName());
try (InputStream is = zip.getInputStream(e)) {
Files.copy(is, f.toPath(), StandardCopyOption.REPLACE_EXISTING);
}
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
webgoat-lessons/path-traversal/src/main/java/org/owasp/webgoat/path_traversal/ProfileZipSlip.java
between lines 46 and 72, improve resource management by wrapping the InputStream
from zip.getInputStream(e) in a try-with-resources block to ensure it is closed
properly. Add validation to check the number of entries and total extracted size
to prevent zip bomb attacks. Also, add logging statements for key
security-relevant events such as file extraction start, validation failures, and
exceptions to aid educational tracing and debugging.
| |=== | ||
| |OS |Location | ||
|
|
||
| |`operatingSystem:os[]` | ||
| |`webGoatTempDir:temppath[]PathTraversal/username:user[]` | ||
|
|
||
| |=== |
There was a problem hiding this comment.
Fix malformed AsciiDoc table attribute placeholders to avoid broken rendering
The table currently uses placeholders like operatingSystem:os[] and webGoatTempDir:temppath[]PathTraversal/username:user[].
These are not valid AsciiDoc attribute references and will be rendered literally, confusing students.
Suggested correction (matches the pattern used in the existing Path-Traversal lesson plans):
-|OS |Location
-
-|`operatingSystem:os[]`
-|`webGoatTempDir:temppath[]PathTraversal/username:user[]`
+|OS |Location
+
+|`{operatingSystem}`
+|`{webGoatTempDir}/PathTraversal/{username}`• {operatingSystem}, {webGoatTempDir}, and {username} are standard attribute syntax and will be substituted at runtime.
• Added a blank line after the header row to keep header/body separation explicit.
Please verify these attribute names match those defined elsewhere in the project.
🤖 Prompt for AI Agents
In
webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_zip_slip_assignment.adoc
around lines 5 to 11, the AsciiDoc table uses invalid attribute placeholders
like `operatingSystem:os[]` which render literally and confuse readers. Replace
these with proper AsciiDoc attribute references using curly braces, such as
`{operatingSystem}`, `{webGoatTempDir}`, and `{username}`. Also, add a blank
line after the header row to clearly separate the header from the table body for
correct rendering.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores
Tests