Security audit: MEDIUM fixes — credential binding, transport, validation, deployAtEnd - #698
Draft
gnodet wants to merge 8 commits into
Draft
Security audit: MEDIUM fixes — credential binding, transport, validation, deployAtEnd#698gnodet wants to merge 8 commits into
gnodet wants to merge 8 commits into
Conversation
…eCredentialBinding guard Security audit finding f001 (MEDIUM). Keeps v1's structure: AbstractDeployMojo.validateCredentialBinding guard, known-URL collection (settings mirrors + profile repositories/pluginRepositories, plus distributionManagement via DeployMojo's getKnownRepositoryUrls override), trailing-slash URL normalization, and the user-property-only -Dmaven.deploy.allowCredentialReuse=true knob (session.getUserProperties() only, not POM-settable). v2 adds provenance semantics (fleet precedent: checkstyle bug_04 / clean bug_08): validateCredentialBinding(id, url, fromUserProperty). Mismatch against non-empty known URLs is refused regardless of provenance (unchanged from v1). Empty-record case no longer passes silently: POM-sourced (DeployMojo computes isFromUserProperty by requiring the -D user property to be present AND equal to the value in use, since explicit POM <configuration> beats -D in Maven precedence; the alt selection chain records provenance per parameter name) -> REFUSE naming id and URL, knob-overridable; CLI-sourced -> WARN (id has stored credentials, no known URL for it, credentials will be sent to <url>). DeployFileMojo keeps the 2-arg call, which delegates with fromUserProperty=true (deploy-file is CLI-driven by nature). Javadoc on getKnownRepositoryUrls documents dm-sourced known URLs as advisory in the malicious-POM model; parameter javadoc for altDeploymentRepository and repositoryId documents the provenance-dependent behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ransportSecurity(id,url) Security audit finding f002 (MEDIUM). Single choke point: AbstractDeployMojo.validateTransportSecurity(id,url) refuses http:// and ftp:// deployment URLs by default, called from createDeploymentArtifactRepository (covers the alt-repo route in DeployMojo and the deploy-file url) and explicitly on both distributionManagement branches (which bypass that factory via session.createRemoteRepository(Repository)). Loopback hosts (localhost, 127.0.0.0/8, ::1) are exempt so local mock/IT repositories keep working; opt-out is -Dmaven.deploy.allowInsecureUrl=true (user property, non-pom-bindable), which downgrades the refusal to a WARN. Unknown/other schemes (https, file, scp, sftp, scm:, dav:) are untouched. Docs leg: deploy-http.md example switched to https with an explicit prefer-HTTPS + refusal note; deploying-with-classifiers.md.vm http:// examples switched to https. (Wagon-era deploy-ftp.md/deploy-ssh-external.md rewrites are in bug_13.) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…alidators hoisted to Abs Security audit finding f003 (MEDIUM). Both legs of the root cause closed with shared validators hoisted to AbstractDeployMojo: isValidId now rejects any empty dot-separated segment (kills '.', '..', '.a', 'a.', 'a..b' while keeping the [a-zA-Z0-9._-] allowlist); isValidVersion keeps the separator blocklist and additionally rejects empty, whitespace/ISO-control characters, and all-dots values; new isValidClassifier (same allowlist, not all-dots, absent/empty allowed) and isValidTypeOrExtension (non-empty variant) cover the components the old gate never saw. DeployFileMojo now validates packaging and classifier at the :272 gate (after processModel, so jar-embedded-POM-supplied packaging is covered too) and every classifiers/types list entry inside the side-artifact loop (empty entries rejected). DeployMojo (second mojo per the finding brief) validates project g/a/v and every deployable's classifier in createDeployerRequest before building the request. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…on: (1) deployAllAtOnce Security audit finding f004 (MEDIUM). Two-sided state fix, per the triage recommendation: (1) deployAllAtOnce collects the batched projects while flattening and, after all requests deploy successfully, marks each one State.DEPLOYED (mark-after-batch-deploy per the design guidance - a failed batch stays TO_BE_DEPLOYED so a genuine retry of a failed build is not suppressed); (2) execute() treats DEPLOYED as terminal: re-entering the goal for an already-deployed project logs and returns, closing both triggers (double-bound executions and the direct deploy:deploy O(N^2) walk). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…, a constant lock object Security audit finding f005 (MEDIUM). A single class-level monitor (DEPLOY_AT_END_LOCK, a constant lock object - not mutable static state; batch state stays in per-project session plugin contexts) now serializes every state read/write and the batch trigger: the terminal-state re-entry check, the SKIPPED/DEPLOYED/TO_BE_DEPLOYED marks, and the allProjectsMarked()+deployAllAtOnce() check-then-act. The lock is deliberately held across deployAllAtOnce(): the losing thread waits, then observes the DEPLOYED states written by the winner (bug_04) and no-ops. Request construction (createDeployerRequest) stays outside the lock to keep contention minimal. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…, per the finding brief.
Security audit finding f006 (MEDIUM).
Verified-true doc fix plus visibility escalation, per the finding brief. The deployAtEnd javadoc now states the real semantics: batch fires when the last deploy-bound project reaches its deploy phase (trailing execution-less modules can still fail afterward), multi-repo/mixed-retry requests deploy sequentially with no rollback, and deployAtEnd=false modules cannot be recalled; the false 'none of the reactor projects is deployed' sentence and the incoherent '(experimental)' marker (on a default-on publish path) are removed. Partial-failure visibility: deployAllAtOnce now deploys the grouped requests in an explicit loop and, on failure after at least one group succeeded, logs an ERROR naming how many groups and which repository ids were already published ('remain published: there is no rollback') before rethrowing, so the failure is no longer attributed only to the wrong module.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ility The deployAtEndBatchIsNotRedeployedOnReentry test needs a properly mocked MojoExecution with plugin model, because the upstream cache optimization (PROJECTS_WITH_DEPLOY_KEY) calls mojoExecution.getPlugin().getModel().getKey() in hasDeployExecution(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The classifier validator from f003 wrongly rejected empty classifier
entries in the classifiers CSV (e.g. "classifiers=,src,"). Empty means
"no classifier" and is valid Maven convention used by the
3rd-party-{jar,pom}-with-extras ITs. isValidClassifier("") already
returns true; the redundant isEmpty() pre-check was the bug.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
|
@gnodet I would like to extract fixes for for install plugin we have: |
4 tasks
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
2 participants
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.
Summary
Applies the 6 MEDIUM-severity findings from the Glasswing static security audit (scan-maven-deploy-plugin-20260811) — all confirmed TP with 5/5 adversarial-panel unanimity and zero false positives.
Findings addressed
-Tbuilds can double-fire the batchChanges
Cluster 1 — Credential and transport security (f001, f002):
validateCredentialBinding: provenance-aware guard that refuses POM-sourced alt-repo values binding a credentialed id to an unknown URL; CLI-typed values proceed with WARN. New knob:maven.deploy.allowCredentialReuse(user-property only, non-POM-bindable)validateTransportSecurity: refuseshttp://andftp://deploy URLs (loopback exempt,file://untouched). New knob:maven.deploy.allowInsecureUrlCluster 2 — Layout validation (f003):
AbstractDeployMojo: dot-only segments rejected for id/version/classifier/type; classifier/classifiers/types/packaging (previously never validated) now allowlisted in both mojosCluster 3 — deployAtEnd state machine (f004, f005, f006):
deployAllAtOncemarks each batched projectState.DEPLOYEDafter whole-batch success; re-entry is a logged no-opDEPLOY_AT_END_LOCKmonitor serializes the mark-then-check-then-fire sequence across-Treactor threadsBreaking changes
http://andftp://deploy URLs now refused by default (override:-Dmaven.deploy.allowInsecureUrl=true)-Dmaven.deploy.allowCredentialReuse=true)skipvocabulary is now fail-closed: unrecognized values (typos, wrong case) fail the build instead of silently deployingTest plan
Audit source:
scan-maven-deploy-plugin-20260811.zip— 5-researcher static audit, 80/80 adversarial-panel votes, 0 FP🤖 Generated with Claude Code