mattcasters commented on code in PR #8398:
URL: https://github.com/apache/hop/pull/8398#discussion_r4024059654
##########
plugins/misc/marketplace/src/main/java/org/apache/hop/marketplace/env/EnvironmentApplier.java:
##########
@@ -261,6 +249,60 @@ private MarketplaceConfig configFromEnv(HopInstallSpec
env) {
return config;
}
+ /**
+ * Turn a repository the install spec declares into a marketplace
repository. The URL is the
+ * project's, so the configured credentials are only reused when the project
points at the same
+ * repository they belong to — same scheme, host and port. A project naming
a host the operator
+ * never configured gets what the project itself declared, or nothing.
+ */
+ private MarketplaceRepository repositoryFromRef(HopInstallSpec.RepositoryRef
ref, boolean first) {
+ MarketplaceRepository source = configuredCredentialSource(ref.getUrl());
+ MarketplaceRepository repo =
+ new MarketplaceRepository(
+ StringUtils.defaultIfBlank(ref.getId(), "spec"),
+ ref.getUrl(),
+ StringUtils.isNotBlank(ref.getUsername())
+ ? ref.getUsername()
+ : (source == null ? null : source.getUsername()),
+ StringUtils.isNotBlank(ref.getPassword())
+ ? ref.getPassword()
+ : (source == null ? null : source.getPassword()));
+ if (source == null) {
Review Comment:
**[bug]** Opting out of the global `HOP_MARKETPLACE_USERNAME` / `_PASSWORD`
pair for an unmatched spec URL still leaves repository-scoped
`HOP_MARKETPLACE_<ID>_*` variables in effect, and the spec chooses `id`. A
`hop-env.yaml` with `id: nexus` (or any other configured id) and `url:
https://evil.example/` therefore still sends `HOP_MARKETPLACE_NEXUS_PASSWORD`
to a host the operator never configured — the same class of leak as #8393, for
the credential source the docs recommend when more than one private repository
is configured.
`RepositoryOriginTest.globalEnvironmentCredentialsCanBeSuppressedWhileScopedOnesStillApply`
locks this in. Host matching cannot save it: `configuredCredentialSource` only
copies stored fields; scoped lookup happens later in `effectiveUsername` /
`effectivePassword` using the spec id. Tracked as #8410.
**Suggestion:** For a spec repository with no origin match, do not apply
scoped env vars whose prefix is already a hop-config repository id (or,
stricter, do not apply scoped env vars at all unless origin matches that id’s
configured URL). Keep `HOP_MARKETPLACE_<ID>_*` as the way to credential a *new*
spec id. Add an apply/HTTP test: configured id `nexus` with scoped env secrets,
spec `id: nexus` on a different host, assert no `Authorization` header.
##########
plugins/misc/marketplace/src/main/java/org/apache/hop/marketplace/env/EnvironmentApplier.java:
##########
@@ -261,6 +249,60 @@ private MarketplaceConfig configFromEnv(HopInstallSpec
env) {
return config;
}
+ /**
+ * Turn a repository the install spec declares into a marketplace
repository. The URL is the
+ * project's, so the configured credentials are only reused when the project
points at the same
+ * repository they belong to — same scheme, host and port. A project naming
a host the operator
+ * never configured gets what the project itself declared, or nothing.
+ */
+ private MarketplaceRepository repositoryFromRef(HopInstallSpec.RepositoryRef
ref, boolean first) {
+ MarketplaceRepository source = configuredCredentialSource(ref.getUrl());
+ MarketplaceRepository repo =
+ new MarketplaceRepository(
+ StringUtils.defaultIfBlank(ref.getId(), "spec"),
+ ref.getUrl(),
+ StringUtils.isNotBlank(ref.getUsername())
+ ? ref.getUsername()
+ : (source == null ? null : source.getUsername()),
+ StringUtils.isNotBlank(ref.getPassword())
+ ? ref.getPassword()
+ : (source == null ? null : source.getPassword()));
+ if (source == null) {
+ // The global HOP_MARKETPLACE_USERNAME / _PASSWORD pair belongs to the
operator's own
+ // repositories for the same reason; the repository-scoped variables
stay available so a
+ // project repository can still be given credentials without putting
them in the spec file.
+ repo.setGlobalEnvironmentCredentials(false);
+ }
+ repo.setPrimary(first);
+ repo.setEnabled(true);
+ return repo;
+ }
+
+ /**
+ * The configured repository whose credentials may be reused for {@code
url}, or null when none of
+ * them belongs to that origin. Install order, so the primary wins a tie.
+ */
+ private MarketplaceRepository configuredCredentialSource(String url) {
+ boolean anyStoredCredentials = false;
+ for (MarketplaceRepository repo : baseConfig.orderedRepositories()) {
+ if (StringUtils.isAllBlank(repo.getUsername(), repo.getPassword())) {
Review Comment:
**[suggestion]** `configuredCredentialSource` skips every hop-config
repository whose stored `username` and `password` are blank, even when
`sameOriginAs(url)` would be true. A spec that names a second path on the
operator’s host is then treated as unmatched (`source == null`), so
`globalEnvironmentCredentials` is set false. Operators who put secrets only in
`HOP_MARKETPLACE_USERNAME` / `_PASSWORD` therefore do not reuse those
credentials on the same scheme/host/port — contradicting the PR’s compatibility
claim that a second repository on the same manager keeps working. This is
fail-closed (not a new leak). Also in #8410.
**Suggestion:** Decide origin match independently of whether stored fields
are populated. On a match, keep `globalEnvironmentCredentials` true and inherit
that configured repo’s *effective* credentials (stored fields and its own
scoped env), not only `getUsername()` / `getPassword()`. Cover env-only
same-origin and env-only foreign-host in `EnvironmentApplierTest` / the HTTP
test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
