Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions server/src/main/java/com/defold/extender/Extender.java
Original file line number Diff line number Diff line change
Expand Up @@ -2199,6 +2199,9 @@ private File[] buildClassesDex(List<String> jars, File mainDexList) throws Exten
context.put("jars", jars);
context.put("engineJars", empty_list);
context.put("mainDexList", mainDexList.getAbsolutePath());
// Always bound, also for engines that don't send it, so that a build.yml referencing
// '--min-api {{minAndroidSdkVersion}}' can never render the flag without its argument.
context.put("minAndroidSdkVersion", buildState.getMinAndroidSdkVersion());

// replace parameter name because '--main-dex-list' is deprecated and reported as error
// we can't change command format for older version of engine so replace parameter here.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ public class ExtenderBuildState {
static final String APPMANIFEST_BUILD_ARTIFACTS_KEYWORD = "buildArtifacts";
static final String APPMANIFEST_JETIFIER_KEYWORD = "jetifier";
static final String APPMANIFEST_DEBUG_SOURCE_PATH = "debugSourcePath";
static final String APPMANIFEST_MIN_ANDROID_SDK_VERSION_KEYWORD = "minAndroidSdkVersion";

static final int DEFAULT_MIN_ANDROID_SDK_VERSION = 21;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we pass it from somewhere in configs? I thought we have it somewhere specified based on toolchain

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We pass ndk min version and right now there are no unified approach to manage ndk and sdk minimum version. I think it should be fixed as part of #726


File jobDir;
File uploadDir;
Expand All @@ -19,6 +22,7 @@ public class ExtenderBuildState {
String hostPlatform;
private final String buildArtifacts;
private final String debugSourcePath;
private final int minAndroidSdkVersion;

private final Boolean withSymbols;
private final Boolean useJetifier;
Expand All @@ -37,6 +41,7 @@ public class ExtenderBuildState {
this.withSymbols = ExtenderUtil.getAppManifestContextBoolean(appManifest, APPMANIFEST_WITH_SYMBOLS_KEYWORD, true);
this.buildArtifacts = ExtenderUtil.getAppManifestContextString(appManifest, APPMANIFEST_BUILD_ARTIFACTS_KEYWORD, "");
this.debugSourcePath = ExtenderUtil.getAppManifestContextString(appManifest, APPMANIFEST_DEBUG_SOURCE_PATH, null);
this.minAndroidSdkVersion = ExtenderUtil.getAppManifestContextInteger(appManifest, APPMANIFEST_MIN_ANDROID_SDK_VERSION_KEYWORD, DEFAULT_MIN_ANDROID_SDK_VERSION);
// assign configuration names started with upper letter because it used for cocoapods
if (baseVariant != null && (baseVariant.equals("release") || baseVariant.equals("headless"))) {
this.buildConfiguration = "Release";
Expand Down Expand Up @@ -101,6 +106,10 @@ public String getDebugSourcePath() {
return debugSourcePath;
}

public int getMinAndroidSdkVersion() {
return minAndroidSdkVersion;
}

public Boolean isNeedSymbols() {
return withSymbols;
}
Expand Down
19 changes: 19 additions & 0 deletions server/src/main/java/com/defold/extender/ExtenderUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,25 @@ static String getAppManifestContextString(AppManifestConfiguration manifest, Str
return default_value;
}

// The app manifest context is generated by bob without quoting, so an integer written there is
// parsed by snakeyaml as an Integer, while a quoted one arrives as a String. Accept both, and
// fall back to the default for anything else (including older clients that omit the key).
static Integer getAppManifestContextInteger(AppManifestConfiguration manifest, String name, Integer default_value) throws ExtenderException {
Object o = getAppManifestContextObject(manifest, name);
if (o instanceof Integer) {
return (Integer)o;
}
if (o instanceof String) {
try {
return Integer.valueOf(((String)o).trim());
} catch (NumberFormatException e) {
throw new ExtenderException(String.format(
"Error in app.manifest: '%s' must be an integer, got '%s'.", name, o));
}
}
return default_value;
}

static public boolean isListOfStrings(List<Object> list) {
return list != null && list.stream().allMatch(o -> o instanceof String);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class ExtensionManifestValidator {

private static final Pattern VALID_INCLUDE_PATH = Pattern.compile("^[A-Za-z0-9._+\\-/]+$");
private static final Pattern VALID_SYMBOL_IDENTIFIER = Pattern.compile("^[A-Za-z_][A-Za-z0-9_]*$");
private static final Pattern VALID_API_LEVEL = Pattern.compile("^[0-9]+$");

ExtensionManifestValidator(WhitelistConfig whitelistConfig, List<String> allowedFlags, List<String> allowedSymbols) {
this.allowedDefines.add(WhitelistConfig.compile(whitelistConfig.defineRe));
Expand Down Expand Up @@ -57,6 +58,16 @@ void validateAppManifestContext(Map<String, Object> appContext) throws ExtenderE
ExtenderBuildState.APPMANIFEST_DEBUG_SOURCE_PATH, s));
}
}

// Reaches the command line as the argument of d8 --min-api, so it must be a bare number.
Object minAndroidSdkVersion = appContext.get(ExtenderBuildState.APPMANIFEST_MIN_ANDROID_SDK_VERSION_KEYWORD);
if (minAndroidSdkVersion != null && !(minAndroidSdkVersion instanceof Integer)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject negative integer API levels

For an unquoted YAML value such as minAndroidSdkVersion: -1, SnakeYAML produces an Integer, so this condition skips validation entirely; getAppManifestContextInteger() then preserves -1 and buildClassesDex() renders it as d8 --min-api -1, causing the Android dex build to fail. The equivalent quoted value is rejected, so validate integer instances against a positive API-level range as well.

Useful? React with 👍 / 👎.

if (!(minAndroidSdkVersion instanceof String) || !VALID_API_LEVEL.matcher((String) minAndroidSdkVersion).matches()) {
throw new ExtenderException(String.format(
"Error in app.manifest: '%s' must be an integer, got '%s'.",
ExtenderBuildState.APPMANIFEST_MIN_ANDROID_SDK_VERSION_KEYWORD, minAndroidSdkVersion));
}
}
}

private void validateIncludePaths(String extensionName, File extensionFolder, List<String> includes) throws ExtenderException {
Expand Down
24 changes: 24 additions & 0 deletions server/src/test/java/com/defold/extender/ExtenderUtilTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -376,4 +376,28 @@ public void testSanitizeJavacCmdFlagElsewhere() {
String cmd = "javac -source 11 -proc:none Foo.java";
assertEquals(cmd, ExtenderUtil.sanitizeJavacCmd(cmd));
}

@Test
public void testGetAppManifestContextInteger() throws ExtenderException {
// Exercised with the minAndroidSdkVersion key, the d8 --min-api value
AppManifestConfiguration manifest = new AppManifestConfiguration();

// Engines older than the --min-api change send no context at all, or a context without the
// key. Both must fall back to the default instead of failing the build.
assertEquals(Integer.valueOf(21), ExtenderUtil.getAppManifestContextInteger(manifest, "minAndroidSdkVersion", 21));
manifest.context = new HashMap<>();
assertEquals(Integer.valueOf(21), ExtenderUtil.getAppManifestContextInteger(manifest, "minAndroidSdkVersion", 21));

// bob writes the value unquoted, so snakeyaml parses it as an Integer
manifest.context.put("minAndroidSdkVersion", 24);
assertEquals(Integer.valueOf(24), ExtenderUtil.getAppManifestContextInteger(manifest, "minAndroidSdkVersion", 21));

// A quoted value arrives as a String
manifest.context.put("minAndroidSdkVersion", "24");
assertEquals(Integer.valueOf(24), ExtenderUtil.getAppManifestContextInteger(manifest, "minAndroidSdkVersion", 21));

manifest.context.put("minAndroidSdkVersion", "not-a-number");
assertThrows(ExtenderException.class,
() -> ExtenderUtil.getAppManifestContextInteger(manifest, "minAndroidSdkVersion", 21));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,44 @@ public void testValidateAppManifestContextDebugSourcePath() throws ExtenderExcep
}
}

@Test
public void testValidateAppManifestContextMinAndroidSdkVersion() throws ExtenderException {
List<String> empty = new ArrayList<>();
ExtensionManifestValidator validator = new ExtensionManifestValidator(new WhitelistConfig(), empty, empty);

// Missing key is accepted: engines older than the --min-api change never send it
assertDoesNotThrow(() -> validator.validateAppManifestContext(new HashMap<>()));

// bob writes the value unquoted, so snakeyaml hands us an Integer; a quoted one is a String
for (Object v : new Object[] { 21, 24, "21", "24" }) {
Map<String, Object> ctx = new HashMap<>();
ctx.put("minAndroidSdkVersion", v);
assertDoesNotThrow(() -> validator.validateAppManifestContext(ctx),
"expected to accept minAndroidSdkVersion: " + v);
}

// The value ends up as the argument of d8 --min-api, and the rendered command is split on
// spaces, so anything that is not a bare number is argv injection
Object[] bad = new Object[] {
"24 --output /tmp/evil",
"24;rm -rf /",
"24$(whoami)",
"-1",
"",
"twentyfour",
true,
};
for (Object v : bad) {
Map<String, Object> ctx = new HashMap<>();
ctx.put("minAndroidSdkVersion", v);
ExtenderException exc = assertThrows(ExtenderException.class,
() -> validator.validateAppManifestContext(ctx),
"expected rejection of minAndroidSdkVersion: " + v);
assertTrue(exc.getMessage().contains("minAndroidSdkVersion"),
"message should mention minAndroidSdkVersion, got: " + exc.getMessage());
}
}

@Test
public void testValidateSymbols() throws ExtenderException {
List<String> empty = new ArrayList<>();
Expand Down
14 changes: 14 additions & 0 deletions server/src/test/java/com/defold/extender/TemplateExecutorTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,18 @@ public void templateVariablesShouldBeReplacedByContext() {
assertThat(result).isEqualTo("Hello James!");
}

@Test
public void minAndroidSdkVersionShouldRenderAsASingleArgumentToMinApi() {
// The rendered command is split on spaces, so an unbound or multi-token
// minAndroidSdkVersion would make d8 read the next flag as the value of --min-api.
TemplateExecutor templateExecutor = new TemplateExecutor();
String template = "d8 --min-api {{minAndroidSdkVersion}} --main-dex-rules {{mainDexList}}";
Map<String, Object> context = new HashMap<>();
context.put("minAndroidSdkVersion", 24);
context.put("mainDexList", "/tmp/main.rules");
String result = templateExecutor.execute(template, context);
assertThat(result).isEqualTo("d8 --min-api 24 --main-dex-rules /tmp/main.rules");
assertThat(result.split(" ")).hasSize(5);
}

}