MLE-30237 fix serverStartStop to prevent OS command injection - #1946
Merged
RitaChen609 merged 2 commits intoJun 23, 2026
Merged
Conversation
…t OS command injection
RitaChen609
requested review from
BillFarber,
rjrudin and
stevebio
as code owners
June 22, 2026 18:17
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the functional test failover helpers (WBFailover/QBFailover) against OS command injection when starting/stopping MarkLogic on cluster hosts, and adds unit tests to verify input validation occurs before any process launch.
Changes:
- Replace incorrect string comparisons with
equals-based command validation and restrict commands tostart|stop. - Mitigate command injection by validating the hostname format and using
ProcessBuilderinstead of shell concatenation. - Add
ServerStartStopSecurityTestto ensure invalid hostnames/commands are rejected before any process is started.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
marklogic-client-api-functionaltests/src/test/java/com/marklogic/client/datamovement/functionaltests/WBFailover.java |
Adds hostname/command validation and swaps Runtime.exec(sh -c ...) for a ProcessBuilder-based starter that can be overridden in tests. |
marklogic-client-api-functionaltests/src/test/java/com/marklogic/client/datamovement/functionaltests/QBFailover.java |
Mirrors the WBFailover hardening changes for the query batcher failover tests. |
marklogic-client-api-functionaltests/src/test/java/com/marklogic/client/datamovement/functionaltests/ServerStartStopSecurityTest.java |
Adds tests to verify validation happens before any process creation attempt. |
rjdew-progress
approved these changes
Jun 22, 2026
rjrudin
reviewed
Jun 23, 2026
rjrudin
left a comment
rjrudin
left a comment
Contributor
There was a problem hiding this comment.
These two tests haven't actually run in several years now since there's not a multi-host setup available. I think the better approach is to consider the value of these tests - they're pretty difficult to read and understand in their current form - and either delete them or rewrite them to depend on a multi-host setup in Docker.
jonmille
approved these changes
Jun 23, 2026
RitaChen609
deleted the
MLE-30237-fix-serverStartStop-to-prevent-injection
branch
June 23, 2026 13:39
Contributor
Author
I will add a ticket to review these two tests as you suggested. |
RitaChen609
added a commit
that referenced
this pull request
Sep 3, 2026
* MLE-27481 Bumped to 8.2-SNAPSHOT * MLE-27841 Generating javadocs on each publish This simplifies life for the docs team by allowing them to download the zip of docs. * MLE-27841 Bumped undertow and ml-gradle Now using ml-gradle 6.2.0 instead of SNAPSHOT * SECCMP-1797: Downgrade contents permission from write to read The copyright-check reusable workflow only needs contents: read to checkout code. contents: write was unnecessarily granting the workflow token write access to repository contents, which expands the blast radius of any PwnRequest-style attack. The called workflow (copyright-check.yml) already declares its own permissions block with contents: read. * SECCMP-1797: Harden permissions - add top-level read, fix contents: write Adds top-level permissions: contents: read to restrict the default GITHUB_TOKEN. Also fixes the previous contents: write on the copyright-validation job to contents: read. This follows the principle of least privilege recommended in GitHub's PwnRequest security guidance. * PDP-1182: Remove per-repo pr-workflow.yaml * MLE-28459 Added regression-12 arm stage (#1934) MLE-28459 Added regression-12 arm stage * MLE-28498 11.3.5 Test Fixes for Security Update (#1935) * MLE-28498 Permission fixes * MLE-28498 testResultIgnore() Test fix * MLE-27881 Add cts.param support to Java Client API * MLE-28334 Implement Fragment Option (#1937) * MLE-28334 Implement Fragment Option * MLE-28334 Copilot Suggested Fixes + AbstractFromSearchFragmentTest.java: Refactored FromSearchDocsWithFragmentTest.java and FromSearchWithFragmentTest.java + Changed JavaDoc for fromsearchDocs in PlanBuilderBase.java * MLE-28583 Test fixes for ML12.1 (#1940) Ignore tests that are no longer supported in ML12. Getting the user a different way since xdmp calls have been restricted. * MLE-30241: Apply XXE protections (#1941) * MLE-30241: Apply XXE protections Replace bare DocumentBuilderFactory.newInstance() in OpenCSVBatcher.write() with XmlFactories.getDocumentBuilderFactory() * MLE-30241 Copyright and Copilot suggestions * MLE-30240: Redact auth headers from OkHttp logs (#1942) * MLE-30240: Redact auth headers from OkHttp logs Redact Authorization header from OkHttp logs * MLE-30240 Copilot Suggestions * MLE-30240 Refactor Extract the OkHttp logging message handler and header-redaction logic from OkHttpServices into a dedicated RedactingHttpLogger class. Move the unit test to RedactingHttpLoggerTest in the same package. Revert CONTRIBUTING.md changes (HTTP logging docs belong in product documentation, not the contributor guide). * MLE-30237 fix serverStartStop to prevent OS command injection (#1946) * MLE-30237 fix string comparison operator in serverStartStop to prevent OS command injection * MLE-30237 add -- after ssh to terminate option parsing * MLE-30243 Ensure password is sanitized in any logging * MLE-30667 Added toString() override to ClientCookie (#1949) Added a toString() override that returns `name + "=" + value`. * MLE-29888 Add cts expressions as allowable cts params * MLE-30244 move Jenkins registry credentials to the publish stage and … (#1951) * MLE-30244 move Jenkins registry credentials to the publish stage and validate the MARKLOGIC_IMAGE_TAGS build parameter * MLE-30244 refine the changes as the copilot reviews * MLE-30244 update regex for image tags. * MLE-30244 tighten the regex for image tags * MLE-30239 Use secure trust store * MLE-31024 Refactor Replace/Ignore Transform Tests (#1955) * MLE-31024 Refactor Replace/Ignore Transform Tests * MLE-30953 MLE-30952 MLE-30968 Vulnerability fixes (#1952) * MLE-30953 * MLE-30952 * MLE-30953 Kotlin upgrade Upgraded Kotlin to resolve opentelemetry java vulnerability * MLE-30952 Update logback to 1.5.37 for newer version * MLE-30968 Bump JacksonVersion to 2.22.0 Bump jacksonVersion from 2.21.1 to 2.22.0 * MLE-30953 Bump okhttpVersion to 5.4.0 * MLE-30952 Deleting pom.xml (#1956) Deleting pom.xml so that Black Duck does not scan it * MLE-29643 Configure OkHttp Logging with dedicated logger (#1957) * MLE-29643 Configure OkHttp Logging with dedicated logger --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * MLE-30967 Update CODEOWNERS * MLE-31167 Bump jacksonVersion to 2.22.1 (#1958) * MLE-31109 Bump kotlin plugin and remove kotlin.system.exitProcess (#1961) * MLE-31109 Bump kotlin plugin and remove kotlin.system.exitProcess Bump kotlin plugin from 2.4.0 to 2.4.10 Removed imported instances of kotlin.system.exitProcess from com.squareup.okhttp3:okhttp:5.4.0 vulnerable dependency org.jetbrains.kotlin:kotlin-stdlib:2.2.21 * MLE-31642 infostudio-user removal test fixes (#1962) Removed the infostudio-user role since it has been removed from MLS. Added the update capability to the app-user role. These changes fixed these 3 failing tests: - testWriteMultipleXMLDocWithXMLMetadata - testMetadataXMLCRUD - testRollbackMetadata * MLE-31642 Update noSslContext Test (#1963) * MLE-31642 Update noSslContext Test MarkLogic 12.1 is now returning a 403 error code, like we expect, instead of closing the connection and throwing a MarkLogicIOException for ML 12.1. -Renamed noSslContext() -> noSslContextWithMarkLogic11OrLower() -Added RequiresML12Dot0.java to only run tests against 12.0 versions -Renamed noSslContextWithMarkLogic12() to noSslContextWithMarkLogic12Dot0() -noSslContextWithMarkLogic12Dot0() ExtendWith updated to new RequiresML12Dot0.class to only run against 12.0 versions -Added noSslContextWithMarkLogic12Dot1OrHigher() for new ML 12.1 behavior * MLE-31642 fromLexicons Test fix (#1964) * MLE-31642 fromLexicons Test fix propertiesFragmentShouldNotBeReturnedByFromLexicons() was failing against ML12.1 due to intended behavior change, validated in MLE-31757. The test has been updated to only run on ML version >12.0. A new test has been added to validate the new ML 12.1 fromLexicons behavior. * MLE-31877 revert xdmp-login changes (#1966) * MLE-31877 Removal of xdmp-login * MLE-31877 Privilege Update Adding back the unprotected-uri and unprotected- collections privilege removed as part of xdmp-login removal * MLE-31877 Privilege Update Adding "xdmp:xslt-invoke", "xdmp:xslt-eval", and "xdmp:value" to test-rest-writer role to fix tests. These were removed when java-test-xdmp-login.json was deleted. * MLE-31874 update LICENSE file (#1965) * MLE-31887 Refactor xdmp privileges in test-rest-writer (#1967) * MLE-31877 Removal of xdmp-login * MLE-31877 Privilege Update Adding back the unprotected-uri and unprotected- collections privilege removed as part of xdmp-login removal * MLE-31877 Privilege Update Adding "xdmp:xslt-invoke", "xdmp:xslt-eval", and "xdmp:value" to test-rest-writer role to fix tests. These were removed when java-test-xdmp-login.json was deleted. * MLE-31887 Refactor xdmp privileges in test-rest-writer Moved privileges `xdmp:invoke`, `xdmp:xslt-invoke`, `xdmp:xslt-eval`, and `xdmp:value` from test-rest-writer role to new role test-invoke. Created new user writer-no-default-invoke to use new role test-invoke. Updated AbstractOpicUpdateTest.java to use new user writer-no-default-invoke since it needs the `xdmp:invoke` privilege * MLE-29883 (GH #1938) Include document version in bulk reads * MLE-32054: [java-client-api][polaris]Improper Resource Shutdown or * MLE-31570: Generate SBOM and merge to develop * MLE-32022 Avoid null pointer exception when FailedRequest is null * MLE-32111 Add annotation support to StructuredQueryBuilder (#1648) Adds StructuredQueryBuilder.annotation(String...) so callers can attach one or more <annotation> elements to a structured query. The server turns these into cts:annotation elements; they are ignored during evaluation and are useful for documenting or marking parts of a query. Annotation content may be arbitrary XML (including namespaced elements) or plain text. Content is parsed and copied into the query via DOMWriter, with a plain-text fallback for non-XML content. Annotations are serialized after the query elements to satisfy the search schema's ordering requirement. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Update StructuredQueryBuilder related copyright * MLE-31569 update NOTICE.txt for 8.2.0 release * MLE-32227 Updating build.gradle to publish SBOM artifact (#1975) * MLE-32227 Updating build.gradle to publish SBOM artifact Updated the build.gradle to imbed the marklogic-client-api-<version>.spdx.json SBOM file under META-INF/sbom. Update the publications section to publish the marklogic-client-api-<version>.spdx.json SBOM file as a stand alone artifact. * MLE-32227 Copyright Updates * MLE-31993 update version to 8.2.0 (#1976) --------- Co-authored-by: Rob Rudin <rudin@progress.com> Co-authored-by: GAdityaVarma <agottumu@progress.com> Co-authored-by: SameeraPriyathamTadikonda <preethu4476@gmail.com> Co-authored-by: Jonathan Miller <jonmille@progress.com> Co-authored-by: Ryan Dew <ryan.dew@progress.com> Co-authored-by: Ryan Dew <146854757+rjdew-progress@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: godugu@progress.com <godugu@progress.com> Co-authored-by: ngodugu-marklogic <74939216+ngodugu-marklogic@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Key Security Fixes:
CWE-480 (Use of Incorrect Operator): Broken != "start" reference checks replaced with .equals("start") value checks
CWE-78 (OS Command Injection): Mitigated through:
Hostname validation [A-Za-z0-9._-]+ before any process launch
Command value validation (only "start" and "stop" allowed)
ProcessBuilder with fixed argument array instead of shell string concatenation
Test Results
New test class: ServerStartStopSecurityTest.java
All 4 security unit tests executed and passed.