Security audit: LOW robustness fixes — skip, fallback, policy, redaction, containment, parse by gnodet · Pull Request #699 · apache/maven-deploy-plugin

gnodet · 2026-08-31T12:01:17Z

Summary

Applies 6 LOW-severity robustness findings from the Glasswing static security audit (scan-maven-deploy-plugin-20260811). All confirmed TP with 5/5 adversarial-panel unanimity. Stacks on #698 (MEDIUM fixes).

Findings addressed

Finding	Severity	Title	Patch
f007	LOW	`skip` is a fail-open tri-state — unrecognized values silently publish	bug_07
f008	LOW	Snapshot deploy silently falls back to the release repository	bug_08
f009	LOW	No client-side release/snapshot policy guard; alt route gets API-default policies	bug_09
f010	LOW	Repository strings logged verbatim — URL-embedded userinfo leaks to CI logs	bug_10
f011	LOW	No containment on deploy-file file/files/sources/javadoc paths — composable exfil primitive	bug_11
f012	LOW	Alt-repo parse quirks: legacy pattern first, empty id accepted	bug_12

Changes

f007: Shared parseSkipMode in both mojos — true/false/releases/snapshots matched case-insensitively; unrecognized values (typos like ture) now fail the build instead of silently deploying. Deploy-file releases/snapshots classification reordered after coordinate resolution
f008: WARN with fallback id+URL when a declared-but-unusable snapshotRepository routes a snapshot to the release repository
f009: warnIfPolicyMismatch WARNs pre-upload when the selected repository's explicit releases/snapshots policy rejects the artifact kind; enforcement stays server-side
f010: redactUrlUserInfo masks scheme://userinfo@ as scheme://***@ at all log sites; deploy lines log id (redacted-url)
f011: New opt-in maven.deploy.file.containedIn parameter: restricts all deploy-file path inputs to a containment directory (off by default)
f012: Fail-closed alt-repo parse: empty-trimmed id/url refused; ambiguous legacy id::layout::url whose URL part contains :: refused

Breaking changes

skip vocabulary is now fail-closed: unrecognized values fail the build
Ambiguous legacy id::layout::url alt-repo values now refused (was silently misdeploying)

Test plan

All 45 tests pass (36 existing + 9 new)
Build verified locally with Maven 4 rc-5

Audit source: scan-maven-deploy-plugin-20260811.zip

🤖 Generated with Claude Code

…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>

…kipMode(value, parame Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…sitory declared' (doc Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…ic limit the audit re Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…heme://userinfo@ as s Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…ion (pipeline variabl Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

… swap: reordering the Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

The f007 patch intentionally moved the releases/snapshots skip check after version resolution so that a version supplied via the jar's embedded POM is classified correctly. This means the jar IS inspected before the skip decision, which is expected. Update the IT to assert what matters (the deployment was skipped) instead of an implementation detail (whether the jar was opened). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Same fix as deployfile-release-skip: the jar is inspected to resolve the version before the snapshots skip check, which is the intended behavior of the version-aware skip reordering (f007). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

On macOS, /var is a symlink to /private/var. When a non-existent path (e.g. an artifact that hasn't been created yet) is checked against an existing containment root, toRealPath() fails for the child but succeeds for the root — giving /var/... vs /private/var/..., which breaks the startsWith check. Fix realOrNormalized() to walk up to the closest existing ancestor, resolve its real path, then re-append the non-existent suffix. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…ustness, guard, log (#700) * fix: f013 (LOW) — Verified-true doc fixes (each claim checked against src/main before writ Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: f014 (LOW) — All four fixed in readingPomFromJarFile/initProperties/finally: (1) the Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: f015 (LOW) — The guard now compares locations: isSameLocation() resolves both sides t Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: f016 (INFO) — The immediate branch now logs 'Deploying <gav>' with a comment explainin Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: add per-project partial-deploy inventory to deployAllAtOnce Align deploy-at-end failure reporting with install plugin PR #445: when the batch deploy fails mid-loop, log an explicit per-project inventory showing which projects were already published and which were not, instead of only reporting at the request level. Also moves the DEPLOYED state marking into the deploy loop so each project transitions TO_BE_DEPLOYED → DEPLOYED as soon as its contributing request completes, making the exactly-once guard and the partial-deploy inventory consistent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

slawekjaranowski · 2026-09-06T16:05:57Z

@gnodet please check conflicts

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

gnodet and others added 7 commits August 31, 2026 13:46

gnodet mentioned this pull request Aug 31, 2026

Security audit: LOW/INFO docs and minor fixes — docs refresh, jar robustness, guard, log #700

Merged

3 tasks

gnodet and others added 7 commits August 31, 2026 15:44

fix: f007 (LOW) — Shared fail-closed parser AbstractDeployMojo.parseS…

fdeb595

…kipMode(value, parame Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

fix: f008 (LOW) — The dm selection now distinguishes 'no snapshotRepo…

baf3a02

…sitory declared' (doc Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

fix: f009 (LOW) — Warn-based client-side guard, honest about the stat…

a710c2f

…ic limit the audit re Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

fix: f010 (LOW) — New AbstractDeployMojo.redactUrlUserInfo() masks sc…

8c7c7e1

…heme://userinfo@ as s Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

fix: f011 (LOW) — Opt-in containment knob, per the triage recommendat…

be183fe

…ion (pipeline variabl Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

fix: f012 (LOW) — Two targeted rejections rather than a pattern-order…

b8e8bcb

… swap: reordering the Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gnodet force-pushed the security/audit-low-robustness branch from 01b6a86 to b8e8bcb Compare August 31, 2026 13:45

gnodet and others added 2 commits September 1, 2026 13:01

gnodet mentioned this pull request Sep 1, 2026

Backport security audit fixes to 3.x (f004-f008, f010, f012) #701

Merged

4 tasks

gnodet marked this pull request as ready for review September 1, 2026 21:16

slawekjaranowski approved these changes Sep 5, 2026

View reviewed changes

slawekjaranowski added the bug Something isn't working label Sep 5, 2026

Base automatically changed from security/audit-medium-fixes to master September 6, 2026 15:40

Merge branch 'master' into security/audit-low-robustness

6433f9a

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

slawekjaranowski merged commit c6baa47 into master Sep 8, 2026
11 checks passed

slawekjaranowski deleted the security/audit-low-robustness branch September 8, 2026 18:51

Navigation Menu

Uh oh!

Security audit: LOW robustness fixes — skip, fallback, policy, redaction, containment, parse - #699

Security audit: LOW robustness fixes — skip, fallback, policy, redaction, containment, parse#699
slawekjaranowski merged 19 commits into
masterfrom
security/audit-low-robustness

gnodet commented Aug 31, 2026

Uh oh!

slawekjaranowski commented Sep 6, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

gnodet commented Aug 31, 2026

Summary

Findings addressed

Changes

Breaking changes

Test plan

Uh oh!

slawekjaranowski commented Sep 6, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants