gnodet commented on issue #13084:
URL: https://github.com/apache/maven/issues/13084#issuecomment-5617669707
Follow-up after re-reading the code more carefully.
### Two distinct guards in `03c947d8`
The commit applies two separate restrictions to external model builds:
1. **Profile filtering** (`withoutFileAndPropertyActivation`): profiles with
`<property>` or `<file>` activation are removed from the candidate list before
the selector runs — they can never activate.
2. **Repository stripping** (`withoutRepositories`): profiles that *do*
activate (JDK, OS, `activeByDefault`) have their `<repositories>` /
`<pluginRepositories>` cleared before injection.
So my earlier "option 3" (strip repos, not activation) is already what the
code does for the profiles that pass filter #1. The bug is entirely in filter
#1: it discards `resteasy-default` before the selector ever sees it.
### Why `!property` breaks things but isn't a security threat
The security concern behind filter #1 is real: a property-activated profile
in a transitive POM could be toggled on/off by user-supplied `-D` properties,
letting the build environment influence what transitive code does (e.g. inject
additional repositories, change dependency versions). That's a valid threat.
**But negated-property activation (`<name>!foo</name>`) is the opposite
case.** It fires when `foo` is *absent* — i.e. it's the default, "always on
unless you explicitly disable me" idiom. Suppressing it doesn't prevent
environment leakage into external models; it just silently breaks the model.
The profile would also fire with no session properties at all.
Concretely: `resteasy-default` activates when `resteasy.dependencies.eap` is
not set. No attacker can force it to activate (it's already on by default); an
attacker can only *suppress* it by setting that property, which makes things
safer, not worse.
### Proposed fix
In `withoutFileAndPropertyActivation`, treat a negated-property activation
(name starts with `!`, no value required) as environment-independent — pass it
through, just like `activeByDefault`:
```java
private static List<Profile> withoutFileAndPropertyActivation(List<Profile>
profiles) {
List<Profile> eligible = new ArrayList<>(profiles.size());
for (Profile profile : profiles) {
Activation activation = profile.getActivation();
if (activation == null || (activation.getFile() == null &&
isSafePropertyActivation(activation.getProperty()))) {
eligible.add(profile);
}
}
return eligible;
}
/**
* A property activation is "safe" for external models if it cannot be
toggled on by
* supplying a user/system property — i.e. it fires on property *absence*
(negated name,
* no value). Such profiles are on by default and can only be turned *off*
by the
* operator, which is the safer direction.
*/
private static boolean
isSafePropertyActivation(org.apache.maven.model.ActivationProperty prop) {
if (prop == null) {
return true; // no property condition at all
}
String name = prop.getName();
// "!foo" with no value = active when 'foo' is absent = default-on,
cannot be injected
return name != null && name.startsWith("!") && (prop.getValue() == null
|| prop.getValue().isEmpty());
}
```
This keeps the security benefit (positive property conditions are still
blocked) while restoring correct behaviour for the common "opt-out flag"
pattern that resteasy (and presumably many other JBoss projects) relies on.
The existing `<repositories>` stripping (`withoutRepositories`) still
applies to these profiles, so the security property "external models cannot
inject new repositories" is preserved.
--
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]
