Scope server credentials and validate legacy relocation coordinates - #12954
Scope server credentials and validate legacy relocation coordinates#12954slachiewicz wants to merge 3 commits into
Conversation
Credentials configured in settings for a server id are keyed only by that id. A repository definition can also arrive from a POM resolved out of a remote repository and reuse the id of a server the operator holds credentials for, at a different origin than the one the operator configured. Wrap the authentication selector so credentials for an id are offered only to a repository whose origin (scheme, host, port) matches a repository or mirror the operator declared, in settings or on the command line, with that same id; other origins get a once-per-id/origin warning naming the id and are refused. Ids with no operator-declared repository (for example a deploy-only server whose URL comes from the project's distributionManagement) keep serving credentials as before, with a warning, so mvn deploy using settings credentials plus a POM's distributionManagement keeps working. The new maven.repository.credentialScope user property selects the policy: "origin" (default), "strict" (also refuse undeclared ids), or legacy "id".
Aligns the legacy WagonManager credential lookup with the exact-match semantics used by every other id-keyed credential path; server ids have never been documented as case-insensitive.
gnodet
left a comment
There was a problem hiding this comment.
Three well-implemented security-hardening fixes. The credential-scoping wrapper is correctly designed with good defaults (origin-based by default, strict opt-in, id fallback), proper thread safety (ConcurrentHashMap.newKeySet() for reported, read-only declaredOrigins), and clean escape hatches. All CI checks pass.
Observations (informational):
-
Credential scoping design — The three-tier policy (origin/strict/id) with origin as default is well balanced. The undeclared-ID path emitting a warning rather than refusing is the right call for the
distributionManagementdeployment case. -
DAV URLs —
dav:http://host/pathURLs produce a null origin becausejava.net.URIparses them as opaque URIs with no host. In origin mode this works (credentials served with a warning), but in strict mode credentials would be refused. DAV URLs are extremely rare in practice, but worth documenting inoriginOf's Javadoc. -
Relocation validation — The denylist approach (
..,/,\,:, control chars) is adequate for blocking path traversal on the legacy metadata path. Note: the PR description states it "matches the check already applied in the resolver provider" but the resolver provider doesn't appear to have this check — the model validator uses a stricter whitelist ([a-zA-Z0-9._-]). The denylist is fine for this purpose. -
Style nit —
assertEquals(true, exception.getMessage().contains("a/b"))could beassertTrue(...)for readability.
🔀 Backport / Forward-port Status
maven-3.9.x— has theequalsIgnoreCaseserver-ID bug and the relocation validation gapmaster— has theequalsIgnoreCaseincompat/maven-compat/.../DefaultWagonManager.java
The three commits are cleanly separated, making selective cherry-picking straightforward.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
|
Related work in It approaches the same question as this PR from the resolver side. The default implementation there delegates to the existing method, so nothing changes for callers that do not opt in. This comment was created with AI assistance. |
Three fixes in
maven-coreandmaven-compat.maven.repository.credentialScope=strictrefuses them instead. Defaults toorigin.DefaultWagonManagermatched settings server ids case-insensitively while the rest of the code matches exactly; it now matches exactly.Each change is a separate commit.
Draft while related code paths are reviewed — the same guard may be needed in sibling implementations of these interfaces, and I would rather establish that before asking for review time.