From 851abd5a06198f1cf9dfa53f5ca8ddbae260d09f Mon Sep 17 00:00:00 2001 From: Shrikant Sharat Kandula Date: Wed, 15 Sep 2021 17:03:47 +0530 Subject: [PATCH] fix: Add missing variables in env file in config (#7100) The env config API currently only changes values that are already defined in the env file. It is not capable of adding anything to the file. This commit adds the capability to do so. However, since we don't want to let the client add just any variable, we've switched from a black list to a white list of env variables that can be managed by this API. If the requested variable is present in the whitelist, we add it, if its missing in the env file. --- .../appsmith/server/solutions/EnvManager.java | 58 ++++++-- .../server/solutions/EnvManagerTest.java | 137 +++++++++++------- 2 files changed, 136 insertions(+), 59 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/EnvManager.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/EnvManager.java index 993196ede0..8f91feb00d 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/EnvManager.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/EnvManager.java @@ -10,6 +10,7 @@ import com.appsmith.server.services.SessionUserService; import com.appsmith.server.services.UserService; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; +import org.apache.commons.lang3.StringUtils; import org.springframework.stereotype.Component; import reactor.core.publisher.Mono; @@ -17,6 +18,7 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -43,9 +45,29 @@ public class EnvManager { "^(?[A-Z0-9_]+)\\s*=\\s*\"?(?.*?)\"?$" ); - private static final Set VARIABLE_BLACKLIST = Set.of( - "APPSMITH_ENCRYPTION_PASSWORD", - "APPSMITH_ENCRYPTION_SALT" + private static final Set VARIABLE_WHITELIST = Set.of( + "APPSMITH_INSTANCE_NAME", + "APPSMITH_MONGODB_URI", + "APPSMITH_REDIS_URL", + "APPSMITH_MAIL_ENABLED", + "APPSMITH_MAIL_FROM", + "APPSMITH_REPLY_TO", + "APPSMITH_MAIL_HOST", + "APPSMITH_MAIL_PORT", + "APPSMITH_MAIL_USERNAME", + "APPSMITH_MAIL_PASSWORD", + "APPSMITH_MAIL_SMTP_TLS_ENABLED", + "APPSMITH_SIGNUP_DISABLED", + "APPSMITH_SIGNUP_ALLOWED_DOMAINS", + "APPSMITH_ADMIN_EMAILS", + "APPSMITH_RECAPTCHA_SITE_KEY", + "APPSMITH_RECAPTCHA_SECRET_KEY", + "APPSMITH_GOOGLE_MAPS_API_KEY", + "APPSMITH_DISABLE_TELEMETRY", + "APPSMITH_OAUTH2_GOOGLE_CLIENT_ID", + "APPSMITH_OAUTH2_GOOGLE_CLIENT_SECRET", + "APPSMITH_OAUTH2_GITHUB_CLIENT_ID", + "APPSMITH_OAUTH2_GITHUB_CLIENT_SECRET" ); /** @@ -57,13 +79,24 @@ public class EnvManager { * @return List of string lines for updated env file content. */ public static List transformEnvContent(String envContent, Map changes) { - for (final String variable : VARIABLE_BLACKLIST) { - if (changes.containsKey(variable)) { - throw new AppsmithException(AppsmithError.UNAUTHORIZED_ACCESS); - } + final Set variablesNotInWhitelist = new HashSet<>(changes.keySet()); + variablesNotInWhitelist.removeAll(VARIABLE_WHITELIST); + + if (!variablesNotInWhitelist.isEmpty()) { + throw new AppsmithException(AppsmithError.UNAUTHORIZED_ACCESS); } - return envContent.lines() + if (changes.containsKey("APPSMITH_MAIL_HOST")) { + changes.put("APPSMITH_MAIL_ENABLED", Boolean.toString(StringUtils.isEmpty(changes.get("APPSMITH_MAIL_HOST")))); + } + + if (changes.containsKey("APPSMITH_MAIL_USERNAME")) { + changes.put("APPSMITH_MAIL_SMTP_AUTH", Boolean.toString(StringUtils.isEmpty(changes.get("APPSMITH_MAIL_USERNAME")))); + } + + final Set remainingChangedNames = new HashSet<>(changes.keySet()); + + final List outLines = envContent.lines() .map(line -> { final Matcher matcher = ENV_VARIABLE_PATTERN.matcher(line); if (!matcher.matches()) { @@ -73,11 +106,18 @@ public class EnvManager { if (!changes.containsKey(name)) { return line; } + remainingChangedNames.remove(name); return line.substring(0, matcher.start("value")) + changes.get(name) + line.substring(matcher.end("value")); }) .collect(Collectors.toList()); + + for (final String name : remainingChangedNames) { + outLines.add(name + "=" + changes.get(name)); + } + + return outLines; } public Mono applyChanges(Map changes) { @@ -114,7 +154,7 @@ public class EnvManager { final Matcher matcher = ENV_VARIABLE_PATTERN.matcher(line); if (matcher.matches()) { final String name = matcher.group("name"); - if (!VARIABLE_BLACKLIST.contains(name)) { + if (VARIABLE_WHITELIST.contains(name)) { data.put(name, matcher.group("value")); } } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/EnvManagerTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/EnvManagerTest.java index cf8da6e777..e62cae3499 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/EnvManagerTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/EnvManagerTest.java @@ -1,5 +1,7 @@ package com.appsmith.server.solutions; +import com.appsmith.server.exceptions.AppsmithError; +import com.appsmith.server.exceptions.AppsmithException; import lombok.extern.slf4j.Slf4j; import org.junit.Test; import org.junit.runner.RunWith; @@ -8,6 +10,7 @@ import org.springframework.test.context.junit4.SpringRunner; import java.util.Map; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; @RunWith(SpringRunner.class) @Slf4j @@ -15,117 +18,116 @@ public class EnvManagerTest { @Test public void simpleSample() { - final String content = "VAR_1=first value\nVAR_2=second value\n\nVAR_3=third value"; + final String content = "APPSMITH_MONGODB_URI=first value\nAPPSMITH_REDIS_URL=second value\n\nAPPSMITH_INSTANCE_NAME=third value"; assertThat(EnvManager.transformEnvContent( content, - Map.of("VAR_1", "new first value") + Map.of("APPSMITH_MONGODB_URI", "new first value") )).containsExactly( - "VAR_1=new first value", - "VAR_2=second value", + "APPSMITH_MONGODB_URI=new first value", + "APPSMITH_REDIS_URL=second value", "", - "VAR_3=third value" + "APPSMITH_INSTANCE_NAME=third value" ); assertThat(EnvManager.transformEnvContent( content, - Map.of("VAR_2", "new second value") + Map.of("APPSMITH_REDIS_URL", "new second value") )).containsExactly( - "VAR_1=first value", - "VAR_2=new second value", + "APPSMITH_MONGODB_URI=first value", + "APPSMITH_REDIS_URL=new second value", "", - "VAR_3=third value" + "APPSMITH_INSTANCE_NAME=third value" ); assertThat(EnvManager.transformEnvContent( content, - Map.of("VAR_3", "new third value") + Map.of("APPSMITH_INSTANCE_NAME", "new third value") )).containsExactly( - "VAR_1=first value", - "VAR_2=second value", + "APPSMITH_MONGODB_URI=first value", + "APPSMITH_REDIS_URL=second value", "", - "VAR_3=new third value" + "APPSMITH_INSTANCE_NAME=new third value" ); assertThat(EnvManager.transformEnvContent( content, Map.of( - "VAR_1", "new first value", - "VAR_3", "new third value" + "APPSMITH_MONGODB_URI", "new first value", + "APPSMITH_INSTANCE_NAME", "new third value" ) )).containsExactly( - "VAR_1=new first value", - "VAR_2=second value", + "APPSMITH_MONGODB_URI=new first value", + "APPSMITH_REDIS_URL=second value", "", - "VAR_3=new third value" + "APPSMITH_INSTANCE_NAME=new third value" ); } @Test public void emptyValues() { - final String content = "VAR_1=first value\nVAR_2=\n\nVAR_3=third value"; + final String content = "APPSMITH_MONGODB_URI=first value\nAPPSMITH_REDIS_URL=\n\nAPPSMITH_INSTANCE_NAME=third value"; assertThat(EnvManager.transformEnvContent( content, - Map.of("VAR_2", "new second value") + Map.of("APPSMITH_REDIS_URL", "new second value") )).containsExactly( - "VAR_1=first value", - "VAR_2=new second value", + "APPSMITH_MONGODB_URI=first value", + "APPSMITH_REDIS_URL=new second value", "", - "VAR_3=third value" + "APPSMITH_INSTANCE_NAME=third value" ); assertThat(EnvManager.transformEnvContent( content, - Map.of("VAR_2", "") + Map.of("APPSMITH_REDIS_URL", "") )).containsExactly( - "VAR_1=first value", - "VAR_2=", + "APPSMITH_MONGODB_URI=first value", + "APPSMITH_REDIS_URL=", "", - "VAR_3=third value" + "APPSMITH_INSTANCE_NAME=third value" ); } @Test public void quotedValues() { - final String content = "VAR_1=first value\nVAR_2=\"quoted value\"\n\nVAR_3=third value"; + final String content = "APPSMITH_MONGODB_URI=first value\nAPPSMITH_REDIS_URL=\"quoted value\"\n\nAPPSMITH_INSTANCE_NAME=third value"; assertThat(EnvManager.transformEnvContent( content, Map.of( - "VAR_1", "new first value", - "VAR_2", "new second value" + "APPSMITH_MONGODB_URI", "new first value", + "APPSMITH_REDIS_URL", "new second value" ) )).containsExactly( - "VAR_1=new first value", - "VAR_2=\"new second value\"", + "APPSMITH_MONGODB_URI=new first value", + "APPSMITH_REDIS_URL=\"new second value\"", "", - "VAR_3=third value" + "APPSMITH_INSTANCE_NAME=third value" ); assertThat(EnvManager.transformEnvContent( content, - Map.of("VAR_2", "") + Map.of("APPSMITH_REDIS_URL", "") )).containsExactly( - "VAR_1=first value", - "VAR_2=\"\"", + "APPSMITH_MONGODB_URI=first value", + "APPSMITH_REDIS_URL=\"\"", "", - "VAR_3=third value" + "APPSMITH_INSTANCE_NAME=third value" ); } - @Test public void parseTest() { assertThat(EnvManager.parseToMap( - "VAR_1=first value\nVAR_2=second value\n\nVAR_3=third value" + "APPSMITH_MONGODB_URI=first value\nAPPSMITH_REDIS_URL=second value\n\nAPPSMITH_INSTANCE_NAME=third value" )).containsExactlyInAnyOrderEntriesOf(Map.of( - "VAR_1", "first value", - "VAR_2", "second value", - "VAR_3", "third value" + "APPSMITH_MONGODB_URI", "first value", + "APPSMITH_REDIS_URL", "second value", + "APPSMITH_INSTANCE_NAME", "third value" )); } @@ -134,11 +136,11 @@ public class EnvManagerTest { public void parseEmptyValues() { assertThat(EnvManager.parseToMap( - "VAR_1=first value\nVAR_2=\n\nVAR_3=third value" + "APPSMITH_MONGODB_URI=first value\nAPPSMITH_REDIS_URL=\n\nAPPSMITH_INSTANCE_NAME=third value" )).containsExactlyInAnyOrderEntriesOf(Map.of( - "VAR_1", "first value", - "VAR_2", "", - "VAR_3", "third value" + "APPSMITH_MONGODB_URI", "first value", + "APPSMITH_REDIS_URL", "", + "APPSMITH_INSTANCE_NAME", "third value" )); } @@ -147,12 +149,47 @@ public class EnvManagerTest { public void parseQuotedValues() { assertThat(EnvManager.parseToMap( - "VAR_1=first value\nVAR_2=\"quoted value\"\n\nVAR_3=third value" + "APPSMITH_MONGODB_URI=first value\nAPPSMITH_REDIS_URL=\"quoted value\"\n\nAPPSMITH_INSTANCE_NAME=third value" )).containsExactlyInAnyOrderEntriesOf(Map.of( - "VAR_1", "first value", - "VAR_2", "quoted value", - "VAR_3", "third value" + "APPSMITH_MONGODB_URI", "first value", + "APPSMITH_REDIS_URL", "quoted value", + "APPSMITH_INSTANCE_NAME", "third value" )); } + + @Test + public void disallowedVariable() { + final String content = "APPSMITH_MONGODB_URI=first value\nDISALLOWED_NASTY_STUFF=\"quoted value\"\n\nAPPSMITH_INSTANCE_NAME=third value"; + + assertThatThrownBy(() -> EnvManager.transformEnvContent( + content, + Map.of( + "APPSMITH_MONGODB_URI", "new first value", + "DISALLOWED_NASTY_STUFF", "new second value" + ) + )) + .matches(value -> value instanceof AppsmithException + && AppsmithError.UNAUTHORIZED_ACCESS.equals(((AppsmithException) value).getError())); + } + + @Test + public void addNewVariable() { + final String content = "APPSMITH_MONGODB_URI=first value\nAPPSMITH_REDIS_URL=\"quoted value\"\n\nAPPSMITH_INSTANCE_NAME=third value"; + + assertThat(EnvManager.transformEnvContent( + content, + Map.of( + "APPSMITH_MONGODB_URI", "new first value", + "APPSMITH_DISABLE_TELEMETRY", "false" + ) + )).containsExactly( + "APPSMITH_MONGODB_URI=new first value", + "APPSMITH_REDIS_URL=\"quoted value\"", + "", + "APPSMITH_INSTANCE_NAME=third value", + "APPSMITH_DISABLE_TELEMETRY=false" + ); + } + }