From a18d1d7cd0d013b848606ad2bd74cd743efbc1ed Mon Sep 17 00:00:00 2001 From: subrata71 Date: Tue, 28 Apr 2026 02:37:56 +0600 Subject: [PATCH 01/22] test(security): add failing tests for SecureBaseUrlResolver (GHSA-j9gf-vw2f-9hrw) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These tests pin the fail-closed semantics required by the security advisory: - When APPSMITH_BASE_URL is unset and the new APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS compatibility flag is OFF (the default), the resolver returns Mono.empty() — it does NOT trust the request-supplied Origin header as the host of token-bearing email links. - When the compatibility flag is ON (opt-in migration window), legacy behaviour is restored. - Once APPSMITH_BASE_URL is configured, strict-mode validation rejects mismatched Origin values; the compat flag does NOT weaken this branch. Tests fail at compile time because the helper class is introduced in the next commit. Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw --- .../ce/SecureBaseUrlResolverCEImplTest.java | 121 ++++++++++++++++++ 1 file changed, 121 insertions(+) create mode 100644 app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java new file mode 100644 index 000000000000..8a1e9564341c --- /dev/null +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java @@ -0,0 +1,121 @@ +package com.appsmith.server.helpers.ce; + +import com.appsmith.server.exceptions.AppsmithException; +import org.junit.jupiter.api.Test; +import reactor.test.StepVerifier; + +import java.lang.reflect.Field; + +import static org.junit.jupiter.api.Assertions.assertNotNull; + +/** + * Unit tests for {@link SecureBaseUrlResolverCEImpl}. + * + *

These tests pin the fail-closed semantics added for + * GHSA-j9gf-vw2f-9hrw: + * when {@code APPSMITH_BASE_URL} is unset, the resolver must NOT trust the request-supplied + * {@code Origin} value as the host of token-bearing email links. It must signal "no trusted + * URL available" via {@link reactor.core.publisher.Mono#empty()} so that callers in unauthenticated + * flows return generic success without dispatching email (preserving anti-enumeration), while + * callers in authenticated flows can convert the empty into an explicit configuration error. + * + *

The opt-in {@code APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS} compatibility flag exists + * solely to give operators a migration window. When enabled, it restores the legacy behavior + * of trusting the caller-supplied value — but it does NOT weaken the strict-mode check that + * applies once {@code APPSMITH_BASE_URL} is configured. + */ +class SecureBaseUrlResolverCEImplTest { + + private SecureBaseUrlResolverCEImpl newResolver(String configuredBaseUrl, boolean allowInsecureFallback) + throws Exception { + SecureBaseUrlResolverCEImpl resolver = new SecureBaseUrlResolverCEImpl(); + Field baseUrlField = SecureBaseUrlResolverCEImpl.class.getDeclaredField("appsmithBaseUrl"); + baseUrlField.setAccessible(true); + baseUrlField.set(resolver, configuredBaseUrl == null ? "" : configuredBaseUrl); + Field flagField = SecureBaseUrlResolverCEImpl.class.getDeclaredField("allowInsecureOriginBasedLinks"); + flagField.setAccessible(true); + flagField.set(resolver, allowInsecureFallback); + return resolver; + } + + /** + * GHSA-j9gf-vw2f-9hrw — the central regression test. When the trusted base URL is unset and + * the insecure compatibility flag is off (the new default), the resolver must return an + * empty signal — NOT the caller-supplied value. + */ + @Test + void resolveSecureBaseUrl_whenAppsmithBaseUrlUnsetAndCompatFlagOff_returnsEmpty() throws Exception { + SecureBaseUrlResolverCEImpl resolver = newResolver("", false); + + StepVerifier.create(resolver.resolveSecureBaseUrl("https://attacker.example")) + .verifyComplete(); + } + + /** + * Migration path: when an operator opts into the insecure flag during a transition window, + * the legacy behavior is restored — the caller-supplied value is returned. The WARN log is + * the operational signal that this is happening; we do not assert on the log here, but the + * production code MUST emit it (manual code review). + */ + @Test + void resolveSecureBaseUrl_whenAppsmithBaseUrlUnsetAndCompatFlagOn_returnsProvidedValue() throws Exception { + SecureBaseUrlResolverCEImpl resolver = newResolver("", true); + + StepVerifier.create(resolver.resolveSecureBaseUrl("https://attacker.example")) + .expectNext("https://attacker.example") + .verifyComplete(); + } + + @Test + void resolveSecureBaseUrl_whenAppsmithBaseUrlSetAndOriginMatches_returnsConfiguredUrl() throws Exception { + SecureBaseUrlResolverCEImpl resolver = newResolver("https://appsmith.example", false); + + StepVerifier.create(resolver.resolveSecureBaseUrl("https://appsmith.example")) + .expectNext("https://appsmith.example") + .verifyComplete(); + } + + /** + * Regression on the protection added in PR #41426 (GHSA-7hf5-mc28-xmcv): once configured, + * the trusted base URL must not be impersonated. + */ + @Test + void resolveSecureBaseUrl_whenAppsmithBaseUrlSetAndOriginMismatches_errors() throws Exception { + SecureBaseUrlResolverCEImpl resolver = newResolver("https://appsmith.example", false); + + StepVerifier.create(resolver.resolveSecureBaseUrl("https://attacker.example")) + .verifyError(AppsmithException.class); + } + + /** + * Defense-in-depth: the insecure-compat flag is intended for the unset-config case only. + * It must NOT be a backdoor that weakens strict-mode validation when APPSMITH_BASE_URL is + * configured. + */ + @Test + void resolveSecureBaseUrl_whenAppsmithBaseUrlSetAndOriginMismatches_compatFlagDoesNotWeakenStrictMode() + throws Exception { + SecureBaseUrlResolverCEImpl resolver = newResolver("https://appsmith.example", true); + + StepVerifier.create(resolver.resolveSecureBaseUrl("https://attacker.example")) + .verifyError(AppsmithException.class); + } + + /** + * Empty Origin from the request, unset config, default fail-closed: empty signal. + * Sanity-check that the resolver does not blow up on null/empty caller-supplied values. + */ + @Test + void resolveSecureBaseUrl_whenProvidedBaseUrlIsNullOrBlank_andCompatFlagOff_returnsEmpty() throws Exception { + SecureBaseUrlResolverCEImpl resolver = newResolver("", false); + + StepVerifier.create(resolver.resolveSecureBaseUrl(null)).verifyComplete(); + StepVerifier.create(resolver.resolveSecureBaseUrl("")).verifyComplete(); + } + + @Test + void resolveSecureBaseUrl_constructed_isNotNull() throws Exception { + SecureBaseUrlResolverCEImpl resolver = newResolver("https://appsmith.example", false); + assertNotNull(resolver); + } +} From 3b7c86539775159f7eceb20a387f31bd9f9eee3a Mon Sep 17 00:00:00 2001 From: subrata71 Date: Tue, 28 Apr 2026 02:40:26 +0600 Subject: [PATCH 02/22] fix(security): fail closed when APPSMITH_BASE_URL unset for token-bearing emails (GHSA-j9gf-vw2f-9hrw) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract a single canonical SecureBaseUrlResolver that gates the host portion of every emailed absolute link (forgot-password, email verification, workspace invite, instance-admin invite). The resolver MUST NOT trust request-supplied values such as the Origin header as the canonical host. New default behaviour when APPSMITH_BASE_URL is unset: - Forgot-password and resend-verification (unauthenticated): the resolver returns Mono.empty() which propagates through the existing controller wiring (defaultIfEmpty(true) / thenReturn) — clients receive the same generic 200 success response and no email is dispatched. Anti-enumeration is preserved. - Workspace invite and instance-admin invite (authenticated): the resolver returns Mono.empty() which the email-service callers translate into the new MISCONFIGURED_INSTANCE_BASE_URL error (HTTP 500, AE-APP-5045) so the admin caller sees an actionable configuration message. Migration path for self-hosted deployments that have not yet set APPSMITH_BASE_URL: opt into the new APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS env var (defaults to false). The legacy behaviour is then restored, but the helper logs a WARN on every call so operators see they are running in an insecure mode. Documented as deprecated; intended only as a transition window. The strict-mode protection from PR #41426 (Origin must equal APPSMITH_BASE_URL when configured) is preserved — and the new compat flag does NOT weaken it. CE/EE: SecureBaseUrlResolverImpl is annotated @Component on the CE side and extends SecureBaseUrlResolverCEImpl. EE replaces the Impl class with a multi-org-aware variant in its parallel branch (shadow EE PR). Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw Linear: APP-15046 --- .../server/exceptions/AppsmithError.java | 9 ++ .../server/exceptions/AppsmithErrorCode.java | 1 + .../server/helpers/SecureBaseUrlResolver.java | 10 ++ .../helpers/SecureBaseUrlResolverImpl.java | 12 ++ .../helpers/ce/SecureBaseUrlResolverCE.java | 36 +++++ .../ce/SecureBaseUrlResolverCEImpl.java | 74 ++++++++++ .../server/services/EmailServiceImpl.java | 8 +- .../server/services/UserServiceImpl.java | 7 +- .../services/ce/EmailServiceCEImpl.java | 129 +++++++++++------- .../server/services/ce/UserServiceCEImpl.java | 77 ++++------- .../UserServiceCECompatibleImpl.java | 7 +- .../services/ce/EmailServiceCEImplTest.java | 5 + 12 files changed, 270 insertions(+), 105 deletions(-) create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/SecureBaseUrlResolver.java create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/SecureBaseUrlResolverImpl.java create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCE.java create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java index 82981b55d7bc..7c8bf5219f19 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java @@ -893,6 +893,15 @@ public enum AppsmithError { ErrorType.INTERNAL_ERROR, null), + MISCONFIGURED_INSTANCE_BASE_URL( + 500, + AppsmithErrorCode.MISCONFIGURED_INSTANCE_BASE_URL.getCode(), + "APPSMITH_BASE_URL is not configured. Token-bearing email flows are disabled until the canonical instance URL is set.", + AppsmithErrorAction.DEFAULT, + "Instance base URL not configured", + ErrorType.INTERNAL_ERROR, + null), + INVALID_SMTP_CONFIGURATION( 400, AppsmithErrorCode.INVALID_SMTP_CONFIGURATION.getCode(), diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java index 6f1b759e1ea5..e98fa727776c 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java @@ -64,6 +64,7 @@ public enum AppsmithErrorCode { GOOGLE_RECAPTCHA_TIMEOUT("AE-APP-5042", "Google recaptcha timeout"), MIGRATION_FAILED("AE-APP-5043", "Migration failed"), INVALID_PROPERTIES_CONFIGURATION("AE-APP-5044", "Property configuration is wrong or malformed"), + MISCONFIGURED_INSTANCE_BASE_URL("AE-APP-5045", "Instance base URL is not configured"), NAME_CLASH_NOT_ALLOWED_IN_REFACTOR("AE-AST-4009", "Name clash not allowed in refactor"), GENERIC_BAD_REQUEST("AE-BAD-4000", "Generic bad request"), MALFORMED_REQUEST("AE-BAD-4001", "Malformed request body"), diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/SecureBaseUrlResolver.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/SecureBaseUrlResolver.java new file mode 100644 index 000000000000..72e4c5aa1db5 --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/SecureBaseUrlResolver.java @@ -0,0 +1,10 @@ +package com.appsmith.server.helpers; + +import com.appsmith.server.helpers.ce.SecureBaseUrlResolverCE; + +/** + * Marker interface used by Spring DI. CE provides a default + * {@link com.appsmith.server.helpers.SecureBaseUrlResolverImpl}; EE overrides the + * implementation class to add multi-org-aware resolution. + */ +public interface SecureBaseUrlResolver extends SecureBaseUrlResolverCE {} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/SecureBaseUrlResolverImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/SecureBaseUrlResolverImpl.java new file mode 100644 index 000000000000..30bfe340bddf --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/SecureBaseUrlResolverImpl.java @@ -0,0 +1,12 @@ +package com.appsmith.server.helpers; + +import com.appsmith.server.helpers.ce.SecureBaseUrlResolverCEImpl; +import org.springframework.stereotype.Component; + +/** + * CE concrete bean for {@link SecureBaseUrlResolver}. EE replaces this class with + * a multi-org-aware variant that derives the trusted host from the organization + * configuration when the {@code license_multi_org_enabled} feature flag is on. + */ +@Component +public class SecureBaseUrlResolverImpl extends SecureBaseUrlResolverCEImpl implements SecureBaseUrlResolver {} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCE.java new file mode 100644 index 000000000000..97bba9f8e111 --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCE.java @@ -0,0 +1,36 @@ +package com.appsmith.server.helpers.ce; + +import reactor.core.publisher.Mono; + +/** + * Resolves the trusted, server-side base URL used as the host of token-bearing + * email links (forgot-password, email verification, workspace invite, + * instance-admin invite). + * + *

The resolver MUST NOT trust request-supplied values such as the {@code Origin} + * header as the canonical host. Doing so allows an unauthenticated attacker to + * influence the link host of security-sensitive emails — see + * GHSA-j9gf-vw2f-9hrw. + * + *

Implementations return {@link Mono#empty()} when no trusted base URL can be + * resolved and the insecure compatibility flag is off. Callers are expected to + * translate this into a flow-appropriate response: + *

+ */ +public interface SecureBaseUrlResolverCE { + + /** + * @param providedBaseUrl the base URL extracted from the inbound request (typically + * the {@code Origin} header). Treated as untrusted input. + * @return a {@link Mono} emitting the resolved trusted base URL, or empty if no + * trusted URL can be resolved and the insecure compatibility fallback is off. + * May emit an error if a configured base URL is set and the provided value + * does not match (strict-mode enforcement, preserved from prior hardening). + */ + Mono resolveSecureBaseUrl(String providedBaseUrl); +} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java new file mode 100644 index 000000000000..aae58a756d2f --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java @@ -0,0 +1,74 @@ +package com.appsmith.server.helpers.ce; + +import com.appsmith.server.exceptions.AppsmithError; +import com.appsmith.server.exceptions.AppsmithException; +import lombok.extern.slf4j.Slf4j; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.util.StringUtils; +import reactor.core.publisher.Mono; + +/** + * CE implementation of {@link SecureBaseUrlResolverCE}. + * + *

Resolution rules: + * + *

    + *
  1. If {@code APPSMITH_BASE_URL} is configured, it is the only acceptable host. If + * the provided value does not match, an {@link AppsmithException} is emitted + * (preserves the strict-mode protection added in PR #41426 for + * GHSA-7hf5-mc28-xmcv). + * The insecure compatibility flag does NOT weaken this branch.
    2. + *
  2. If {@code APPSMITH_BASE_URL} is unset and {@code APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS} + * is true, the legacy behaviour is restored: the caller-supplied value is + * returned. A WARN log is emitted on every call so operators are aware they + * are running in an insecure mode. This flag is intended only as a transition + * path and is documented as deprecated.
    3. + *
  3. If {@code APPSMITH_BASE_URL} is unset and the insecure flag is off (the new + * default), the resolver emits {@link Mono#empty()} together with a WARN log. + * Callers in unauthenticated flows convert this into a generic success + * response without dispatching email (anti-enumeration). Callers in + * authenticated flows convert it into an explicit configuration error.
    4. + *
+ */ +@Slf4j +public class SecureBaseUrlResolverCEImpl implements SecureBaseUrlResolverCE { + + @Value("${APPSMITH_BASE_URL:}") + private String appsmithBaseUrl; + + /** + * Opt-in escape hatch for legacy self-hosted deployments that have not yet + * configured {@code APPSMITH_BASE_URL}. When true, the resolver falls back to + * the caller-supplied value (the historical, insecure behaviour). Defaults to + * false. Intended only as a temporary migration window — set + * {@code APPSMITH_BASE_URL} to your instance's canonical URL and remove this + * flag. + */ + @Value("${APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS:false}") + private boolean allowInsecureOriginBasedLinks; + + @Override + public Mono resolveSecureBaseUrl(String providedBaseUrl) { + if (StringUtils.hasText(appsmithBaseUrl)) { + if (!appsmithBaseUrl.equals(providedBaseUrl)) { + return Mono.error(new AppsmithException( + AppsmithError.GENERIC_BAD_REQUEST, + "Origin header does not match APPSMITH_BASE_URL configuration.")); + } + return Mono.just(appsmithBaseUrl); + } + + if (allowInsecureOriginBasedLinks) { + log.warn("APPSMITH_BASE_URL is not configured and APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS=true. " + + "Token-bearing email links will be derived from the request Origin header. " + + "This is INSECURE and intended only as a transition path. " + + "Set APPSMITH_BASE_URL to your instance's canonical URL and remove the insecure flag."); + return Mono.just(providedBaseUrl); + } + + log.warn("APPSMITH_BASE_URL is not configured. Token-bearing email flows (password reset, " + + "email verification, invites) are disabled until APPSMITH_BASE_URL is set. " + + "See https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw"); + return Mono.empty(); + } +} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/EmailServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/EmailServiceImpl.java index 7e4d440d3ac2..0d6fe04f1991 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/EmailServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/EmailServiceImpl.java @@ -1,13 +1,17 @@ package com.appsmith.server.services; import com.appsmith.server.helpers.EmailServiceHelper; +import com.appsmith.server.helpers.SecureBaseUrlResolver; import com.appsmith.server.notifications.EmailSender; import com.appsmith.server.services.ce.EmailServiceCEImpl; import org.springframework.stereotype.Service; @Service public class EmailServiceImpl extends EmailServiceCEImpl implements EmailService { - public EmailServiceImpl(EmailSender emailSender, EmailServiceHelper emailServiceHelper) { - super(emailSender, emailServiceHelper); + public EmailServiceImpl( + EmailSender emailSender, + EmailServiceHelper emailServiceHelper, + SecureBaseUrlResolver secureBaseUrlResolver) { + super(emailSender, emailServiceHelper, secureBaseUrlResolver); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserServiceImpl.java index 75d59671c003..6c8683fb4cf4 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserServiceImpl.java @@ -2,6 +2,7 @@ import com.appsmith.server.configurations.CommonConfig; import com.appsmith.server.configurations.EmailConfig; +import com.appsmith.server.helpers.SecureBaseUrlResolver; import com.appsmith.server.helpers.UserServiceHelper; import com.appsmith.server.helpers.UserUtils; import com.appsmith.server.instanceconfigs.helpers.InstanceVariablesHelper; @@ -44,7 +45,8 @@ public UserServiceImpl( RateLimitService rateLimitService, PACConfigurationService pacConfigurationService, UserServiceHelper userServiceHelper, - InstanceVariablesHelper instanceVariablesHelper) { + InstanceVariablesHelper instanceVariablesHelper, + SecureBaseUrlResolver secureBaseUrlResolver) { super( validator, repository, @@ -62,6 +64,7 @@ public UserServiceImpl( rateLimitService, pacConfigurationService, userServiceHelper, - instanceVariablesHelper); + instanceVariablesHelper, + secureBaseUrlResolver); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/EmailServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/EmailServiceCEImpl.java index dca7285e8382..009455d1485b 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/EmailServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/EmailServiceCEImpl.java @@ -4,7 +4,10 @@ import com.appsmith.server.domains.PermissionGroup; import com.appsmith.server.domains.User; import com.appsmith.server.domains.Workspace; +import com.appsmith.server.exceptions.AppsmithError; +import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.helpers.EmailServiceHelper; +import com.appsmith.server.helpers.SecureBaseUrlResolver; import com.appsmith.server.notifications.EmailSender; import org.apache.commons.lang3.StringUtils; import org.springframework.stereotype.Component; @@ -23,13 +26,23 @@ public class EmailServiceCEImpl implements EmailServiceCE { private final EmailServiceHelper emailServiceHelper; - public EmailServiceCEImpl(EmailSender emailSender, EmailServiceHelper emailServiceHelper) { + private final SecureBaseUrlResolver secureBaseUrlResolver; + + public EmailServiceCEImpl( + EmailSender emailSender, + EmailServiceHelper emailServiceHelper, + SecureBaseUrlResolver secureBaseUrlResolver) { this.emailSender = emailSender; this.emailServiceHelper = emailServiceHelper; + this.secureBaseUrlResolver = secureBaseUrlResolver; } @Override public Mono sendForgotPasswordEmail(String email, String resetUrl, String originHeader) { + // The reset URL has already been built with the trusted base URL by UserServiceCEImpl + // (which routes through SecureBaseUrlResolver). The originHeader passed here is used + // only for cosmetic branding lookups; this method is a no-op when not invoked through + // that resolved path. Map params = new HashMap<>(); params.put(RESET_URL, resetUrl); return emailServiceHelper @@ -54,30 +67,42 @@ public Mono sendInviteUserToWorkspaceEmail( PermissionGroup assignedPermissionGroup, String originHeader, boolean isNewUser) { - String inviteUrl = isNewUser - ? String.format( - INVITE_USER_CLIENT_URL_FORMAT, - originHeader, - URLEncoder.encode(invitedUser.getUsername().toLowerCase(), StandardCharsets.UTF_8)) - : originHeader; - Mono emailSubjectMono = emailServiceHelper.getSubjectJoinWorkspace(workspaceInvitedTo.getName()); - Mono workspaceInviteTemplateMono = emailServiceHelper.getWorkspaceInviteTemplate(isNewUser); - Map params = getInviteToWorkspaceEmailParams( - workspaceInvitedTo, invitingUser, inviteUrl, assignedPermissionGroup.getName(), isNewUser); - return emailServiceHelper - .enrichWithBrandParams(params, originHeader) - .zipWith(Mono.zip(emailSubjectMono, workspaceInviteTemplateMono)) - .flatMap(objects -> { - Map updatedParams = objects.getT1(); - String emailSubject = objects.getT2().getT1(); - String workspaceInviteTemplate = objects.getT2().getT2(); - return emailSender.sendMail( - invitedUser.getEmail(), emailSubject, workspaceInviteTemplate, updatedParams); + // Resolve the trusted base URL through the canonical resolver. The invite flow is + // authenticated, so a missing APPSMITH_BASE_URL must surface as an explicit + // configuration error to the admin caller (rather than the silent-success behavior + // used by anti-enumeration flows). See GHSA-j9gf-vw2f-9hrw. + return secureBaseUrlResolver + .resolveSecureBaseUrl(originHeader) + .switchIfEmpty(Mono.error(new AppsmithException(AppsmithError.MISCONFIGURED_INSTANCE_BASE_URL))) + .flatMap(trustedBaseUrl -> { + String inviteUrl = isNewUser + ? String.format( + INVITE_USER_CLIENT_URL_FORMAT, + trustedBaseUrl, + URLEncoder.encode(invitedUser.getUsername().toLowerCase(), StandardCharsets.UTF_8)) + : trustedBaseUrl; + Mono emailSubjectMono = + emailServiceHelper.getSubjectJoinWorkspace(workspaceInvitedTo.getName()); + Mono workspaceInviteTemplateMono = emailServiceHelper.getWorkspaceInviteTemplate(isNewUser); + Map params = getInviteToWorkspaceEmailParams( + workspaceInvitedTo, invitingUser, inviteUrl, assignedPermissionGroup.getName(), isNewUser); + return emailServiceHelper + .enrichWithBrandParams(params, trustedBaseUrl) + .zipWith(Mono.zip(emailSubjectMono, workspaceInviteTemplateMono)) + .flatMap(objects -> { + Map updatedParams = objects.getT1(); + String emailSubject = objects.getT2().getT1(); + String workspaceInviteTemplate = objects.getT2().getT2(); + return emailSender.sendMail( + invitedUser.getEmail(), emailSubject, workspaceInviteTemplate, updatedParams); + }); }); } @Override public Mono sendEmailVerificationEmail(User user, String verificationURL, String originHeader) { + // The verification URL has already been built with the trusted base URL by + // UserServiceCEImpl. The originHeader is used only for branding lookups. Map params = new HashMap<>(); params.put(EMAIL_VERIFICATION_URL, verificationURL); return emailServiceHelper @@ -97,36 +122,44 @@ public Mono sendEmailVerificationEmail(User user, String verificationUR @Override public Mono sendInstanceAdminInviteEmail( User invitedUser, User invitingUser, String originHeader, boolean isNewUser) { - Map params = new HashMap<>(); - String inviteUrl = isNewUser - ? String.format( - INVITE_USER_CLIENT_URL_FORMAT, - originHeader, - URLEncoder.encode(invitedUser.getUsername().toLowerCase(), StandardCharsets.UTF_8)) - : originHeader; - params.put(PRIMARY_LINK_URL, inviteUrl); + // Auth-gated admin invite flow: resolve the trusted base URL or surface a + // configuration error. See GHSA-j9gf-vw2f-9hrw. + return secureBaseUrlResolver + .resolveSecureBaseUrl(originHeader) + .switchIfEmpty(Mono.error(new AppsmithException(AppsmithError.MISCONFIGURED_INSTANCE_BASE_URL))) + .flatMap(trustedBaseUrl -> { + Map params = new HashMap<>(); + String inviteUrl = isNewUser + ? String.format( + INVITE_USER_CLIENT_URL_FORMAT, + trustedBaseUrl, + URLEncoder.encode(invitedUser.getUsername().toLowerCase(), StandardCharsets.UTF_8)) + : trustedBaseUrl; + params.put(PRIMARY_LINK_URL, inviteUrl); - Mono primaryLinkTextMono = emailServiceHelper.getJoinInstanceCtaPrimaryText(); + Mono primaryLinkTextMono = emailServiceHelper.getJoinInstanceCtaPrimaryText(); - if (invitingUser != null) { - params.put(INVITER_FIRST_NAME, StringUtils.defaultIfEmpty(invitingUser.getName(), invitingUser.getEmail())); - } - return primaryLinkTextMono - .flatMap(primaryLinkText -> { - params.put(PRIMARY_LINK_TEXT, primaryLinkText); - return emailServiceHelper.enrichWithBrandParams(params, originHeader); - }) - .zipWhen(updatedParams -> { - return Mono.zip( - emailServiceHelper.getSubjectJoinInstanceAsAdmin(updatedParams.get(INSTANCE_NAME)), - emailServiceHelper.getAdminInstanceInviteTemplate()); - }) - .flatMap(objects -> { - Map updatedParams = objects.getT1(); - String subject = objects.getT2().getT1(); - String adminInstanceInviteTemplate = objects.getT2().getT2(); - return emailSender.sendMail( - invitedUser.getEmail(), subject, adminInstanceInviteTemplate, updatedParams); + if (invitingUser != null) { + params.put( + INVITER_FIRST_NAME, + StringUtils.defaultIfEmpty(invitingUser.getName(), invitingUser.getEmail())); + } + return primaryLinkTextMono + .flatMap(primaryLinkText -> { + params.put(PRIMARY_LINK_TEXT, primaryLinkText); + return emailServiceHelper.enrichWithBrandParams(params, trustedBaseUrl); + }) + .zipWhen(updatedParams -> Mono.zip( + emailServiceHelper.getSubjectJoinInstanceAsAdmin(updatedParams.get(INSTANCE_NAME)), + emailServiceHelper.getAdminInstanceInviteTemplate())) + .flatMap(objects -> { + Map updatedParams = objects.getT1(); + String subject = objects.getT2().getT1(); + String adminInstanceInviteTemplate = + objects.getT2().getT2(); + return emailSender.sendMail( + invitedUser.getEmail(), subject, adminInstanceInviteTemplate, updatedParams); + }); }); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java index ce8c30bec457..af8436841dcb 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java @@ -24,6 +24,7 @@ import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.helpers.EmailNormalizer; import com.appsmith.server.helpers.RedirectHelper; +import com.appsmith.server.helpers.SecureBaseUrlResolver; import com.appsmith.server.helpers.UserServiceHelper; import com.appsmith.server.helpers.UserUtils; import com.appsmith.server.instanceconfigs.helpers.InstanceVariablesHelper; @@ -46,7 +47,6 @@ import org.apache.hc.core5.http.message.BasicNameValuePair; import org.apache.hc.core5.net.WWWFormCodec; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.beans.factory.annotation.Value; import org.springframework.data.mongodb.core.query.UpdateDefinition; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.Authentication; @@ -110,41 +110,7 @@ public class UserServiceCEImpl extends BaseService private final UserServiceHelper userPoliciesComputeHelper; private final InstanceVariablesHelper instanceVariablesHelper; - - @Value("${APPSMITH_BASE_URL:}") - private String appsmithBaseUrl; - - /** - * Resolves and validates the base URL for security-sensitive operations like password reset - * and email verification. This method ensures that URLs in emails point to trusted domains. - * - *

In single-org (CE) mode: - *

    - *
  • If APPSMITH_BASE_URL is configured, validates that the provided URL matches it
    • - *
  • If APPSMITH_BASE_URL is not configured, uses the provided URL (backward compatibility)
    • - *
- * - *

This method can be overridden in EE to handle multi-org setups where each organization - * has its own base URL. - * - * @param providedBaseUrl The base URL from the request (typically from Origin header) - * @return Mono The validated/resolved base URL to use for constructing email links - */ - protected Mono resolveSecureBaseUrl(String providedBaseUrl) { - // If APPSMITH_BASE_URL is not configured, use provided URL for backwards compatibility - if (!StringUtils.hasText(appsmithBaseUrl)) { - return Mono.just(providedBaseUrl); - } - - // If APPSMITH_BASE_URL is configured, validate that Origin header matches it - if (!appsmithBaseUrl.equals(providedBaseUrl)) { - return Mono.error(new AppsmithException( - AppsmithError.GENERIC_BAD_REQUEST, - "Origin header does not match APPSMITH_BASE_URL configuration.")); - } - - return Mono.just(appsmithBaseUrl); - } + private final SecureBaseUrlResolver secureBaseUrlResolver; protected static final WebFilterChain EMPTY_WEB_FILTER_CHAIN = serverWebExchange -> Mono.empty(); private static final String FORGOT_PASSWORD_CLIENT_URL_FORMAT = "%s/user/resetPassword?token=%s"; @@ -176,7 +142,8 @@ public UserServiceCEImpl( RateLimitService rateLimitService, PACConfigurationService pacConfigurationService, UserServiceHelper userServiceHelper, - InstanceVariablesHelper instanceVariablesHelper) { + InstanceVariablesHelper instanceVariablesHelper, + SecureBaseUrlResolver secureBaseUrlResolver) { super(validator, repository, analyticsService); this.workspaceService = workspaceService; @@ -193,6 +160,7 @@ public UserServiceCEImpl( this.userPoliciesComputeHelper = userServiceHelper; this.pacConfigurationService = pacConfigurationService; this.instanceVariablesHelper = instanceVariablesHelper; + this.secureBaseUrlResolver = secureBaseUrlResolver; } @Override @@ -226,13 +194,17 @@ public Mono forgotPasswordTokenGenerate(ResetUserPasswordDTO resetUserP return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.ORIGIN)); } - // Resolve the secure base URL (validates in single-org, may be overridden for multi-org) - return resolveSecureBaseUrl(resetUserPasswordDTO.getBaseUrl()).flatMap(secureBaseUrl -> { - // Use the resolved secure base URL instead of the client-provided one - resetUserPasswordDTO.setBaseUrl(secureBaseUrl); - String email = resetUserPasswordDTO.getEmail(); - return processForgotPasswordTokenGeneration(email, resetUserPasswordDTO); - }); + // Resolve the secure base URL through the trusted resolver. When APPSMITH_BASE_URL is unset + // (and the insecure compatibility flag is off) the resolver returns Mono.empty(), causing + // this entire chain to complete without dispatching email — preserving the generic success + // response that anti-enumeration relies on. See GHSA-j9gf-vw2f-9hrw. + return secureBaseUrlResolver + .resolveSecureBaseUrl(resetUserPasswordDTO.getBaseUrl()) + .flatMap(secureBaseUrl -> { + resetUserPasswordDTO.setBaseUrl(secureBaseUrl); + String email = resetUserPasswordDTO.getEmail(); + return processForgotPasswordTokenGeneration(email, resetUserPasswordDTO); + }); } private Mono processForgotPasswordTokenGeneration( @@ -868,13 +840,16 @@ public Mono resendEmailVerification( return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.ORIGIN)); } - // Resolve the secure base URL (validates in single-org, may be overridden for multi-org) - return resolveSecureBaseUrl(resendEmailVerificationDTO.getBaseUrl()).flatMap(secureBaseUrl -> { - // Use the resolved secure base URL instead of the client-provided one - resendEmailVerificationDTO.setBaseUrl(secureBaseUrl); - String email = resendEmailVerificationDTO.getEmail(); - return processResendEmailVerification(email, resendEmailVerificationDTO, redirectUrl); - }); + // Resolve the secure base URL through the trusted resolver. When APPSMITH_BASE_URL is unset + // (and the insecure compatibility flag is off) the resolver returns Mono.empty(), causing + // this entire chain to complete without dispatching email. See GHSA-j9gf-vw2f-9hrw. + return secureBaseUrlResolver + .resolveSecureBaseUrl(resendEmailVerificationDTO.getBaseUrl()) + .flatMap(secureBaseUrl -> { + resendEmailVerificationDTO.setBaseUrl(secureBaseUrl); + String email = resendEmailVerificationDTO.getEmail(); + return processResendEmailVerification(email, resendEmailVerificationDTO, redirectUrl); + }); } private Mono processResendEmailVerification( diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/UserServiceCECompatibleImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/UserServiceCECompatibleImpl.java index d8b838acafe1..d0ec7ebce04e 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/UserServiceCECompatibleImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/UserServiceCECompatibleImpl.java @@ -1,6 +1,7 @@ package com.appsmith.server.services.ce_compatible; import com.appsmith.server.configurations.CommonConfig; +import com.appsmith.server.helpers.SecureBaseUrlResolver; import com.appsmith.server.helpers.UserServiceHelper; import com.appsmith.server.helpers.UserUtils; import com.appsmith.server.instanceconfigs.helpers.InstanceVariablesHelper; @@ -39,7 +40,8 @@ public UserServiceCECompatibleImpl( RateLimitService rateLimitService, PACConfigurationService pacConfigurationService, UserServiceHelper userServiceHelper, - InstanceVariablesHelper instanceVariablesHelper) { + InstanceVariablesHelper instanceVariablesHelper, + SecureBaseUrlResolver secureBaseUrlResolver) { super( validator, repository, @@ -57,6 +59,7 @@ public UserServiceCECompatibleImpl( rateLimitService, pacConfigurationService, userServiceHelper, - instanceVariablesHelper); + instanceVariablesHelper, + secureBaseUrlResolver); } } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/EmailServiceCEImplTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/EmailServiceCEImplTest.java index 84900e41745f..ea7e93f236fa 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/EmailServiceCEImplTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/EmailServiceCEImplTest.java @@ -10,6 +10,7 @@ import org.springframework.beans.factory.annotation.Qualifier; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.mock.mockito.SpyBean; +import org.springframework.test.context.TestPropertySource; import reactor.core.publisher.Mono; import java.util.HashMap; @@ -36,6 +37,10 @@ import static org.mockito.Mockito.doAnswer; @SpringBootTest +// GHSA-j9gf-vw2f-9hrw: invite/admin-invite flows now require APPSMITH_BASE_URL to be configured +// (auth-gated, so they fail closed with a configuration error rather than silent success). +// Set it to the value the tests pass as the Origin header so the strict-match path activates. +@TestPropertySource(properties = "APPSMITH_BASE_URL=http://example.com") class EmailServiceCEImplTest { @SpyBean From 35eb077e580f48fcd327fc83f3a825879ce47a9b Mon Sep 17 00:00:00 2001 From: subrata71 Date: Tue, 28 Apr 2026 12:52:24 +0600 Subject: [PATCH 03/22] =?UTF-8?q?fix(security):=20unblock=20CI=20for=20GHS?= =?UTF-8?q?A-j9gf-vw2f-9hrw=20=E2=80=94=20unique=20error=20code=20+=20test?= =?UTF-8?q?=20profile=20flag?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two CI fixes layered onto the previous commit: 1. The new error code AE-APP-5045 collided with the existing FEATURE_FLAG_MIGRATION_FAILURE entry (caught by AppsmithErrorTest#verifyUniquenessOfAppsmithErrorCode). Bumped the MISCONFIGURED_INSTANCE_BASE_URL code to AE-APP-5046, which is unused. 2. The integration test suite (WorkspaceServiceTest, ApplicationForkingServiceTests, UserServiceTest email-verification cases, ThemeServiceTest, etc.) exercises invite and email-verification flows without setting APPSMITH_BASE_URL. With the new fail-closed default, those flows would short-circuit before reaching the business-logic validations the tests rely on. Set APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS=true in the test profile so the legacy Origin-based behaviour is preserved across the existing integration suites. Production deployments default this flag to false (secure). The fail-closed semantics introduced by this advisory are still pinned directly by the SecureBaseUrlResolverCEImplTest unit tests (which run without Spring context), so this does not weaken the security regression coverage. This also lets EmailServiceCEImplTest drop the per-class @TestPropertySource (added in the previous commit) since the global flag covers it. Verified locally with the full UserServiceTest + EmailServiceCEImplTest + SecureBaseUrlResolverCEImplTest + AppsmithErrorTest suites (46/46 passing) and representative samples from WorkspaceServiceTest, ApplicationForkingServiceTests, ThemeServiceTest, and UserWorkspaceServiceTest. Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw --- .../com/appsmith/server/exceptions/AppsmithErrorCode.java | 2 +- .../server/services/ce/EmailServiceCEImplTest.java | 5 ----- .../src/test/resources/application-test.properties | 7 +++++++ 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java index e98fa727776c..43b541d678ec 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java @@ -64,7 +64,7 @@ public enum AppsmithErrorCode { GOOGLE_RECAPTCHA_TIMEOUT("AE-APP-5042", "Google recaptcha timeout"), MIGRATION_FAILED("AE-APP-5043", "Migration failed"), INVALID_PROPERTIES_CONFIGURATION("AE-APP-5044", "Property configuration is wrong or malformed"), - MISCONFIGURED_INSTANCE_BASE_URL("AE-APP-5045", "Instance base URL is not configured"), + MISCONFIGURED_INSTANCE_BASE_URL("AE-APP-5046", "Instance base URL is not configured"), NAME_CLASH_NOT_ALLOWED_IN_REFACTOR("AE-AST-4009", "Name clash not allowed in refactor"), GENERIC_BAD_REQUEST("AE-BAD-4000", "Generic bad request"), MALFORMED_REQUEST("AE-BAD-4001", "Malformed request body"), diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/EmailServiceCEImplTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/EmailServiceCEImplTest.java index ea7e93f236fa..84900e41745f 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/EmailServiceCEImplTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/EmailServiceCEImplTest.java @@ -10,7 +10,6 @@ import org.springframework.beans.factory.annotation.Qualifier; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.mock.mockito.SpyBean; -import org.springframework.test.context.TestPropertySource; import reactor.core.publisher.Mono; import java.util.HashMap; @@ -37,10 +36,6 @@ import static org.mockito.Mockito.doAnswer; @SpringBootTest -// GHSA-j9gf-vw2f-9hrw: invite/admin-invite flows now require APPSMITH_BASE_URL to be configured -// (auth-gated, so they fail closed with a configuration error rather than silent success). -// Set it to the value the tests pass as the Origin header so the strict-match path activates. -@TestPropertySource(properties = "APPSMITH_BASE_URL=http://example.com") class EmailServiceCEImplTest { @SpyBean diff --git a/app/server/appsmith-server/src/test/resources/application-test.properties b/app/server/appsmith-server/src/test/resources/application-test.properties index 878951df3000..324efe64371f 100644 --- a/app/server/appsmith-server/src/test/resources/application-test.properties +++ b/app/server/appsmith-server/src/test/resources/application-test.properties @@ -2,3 +2,10 @@ de.flapdoodle.mongodb.embedded.version=5.0.5 logging.level.root=error appsmith.git.root = /dev/shm/git-storage + +# GHSA-j9gf-vw2f-9hrw: enable the insecure-compatibility flag in the integration test +# environment so the legacy Origin-based behaviour is preserved across the existing +# Workspace / Fork / UserService / Theme integration suites. The fail-closed semantics +# of the resolver are pinned directly by SecureBaseUrlResolverCEImplTest unit tests; +# production deployments default this flag to false (secure). +APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS=true From dd8fbedea88173ec5f9a566ec7747cc4b59d24e2 Mon Sep 17 00:00:00 2001 From: subrata71 Date: Tue, 28 Apr 2026 17:48:06 +0600 Subject: [PATCH 04/22] ci: configure APPSMITH_BASE_URL for E2E test environments (GHSA-j9gf-vw2f-9hrw) The GHSA fix in 3b7c865397 makes the server fail-closed for token-bearing email flows (forgot-password, email verification, workspace invite, instance-admin invite) when APPSMITH_BASE_URL is unset. The Cypress E2E suite spins up a fresh Appsmith Docker container that did not set the variable, so specs like Email_settings_Spec, ExportApplication_spec, DeleteWorkspace_spec, LeaveWorkspaceTest_spec, MemberRoles_Spec, and ShareAppTests_Spec failed with MISCONFIGURED_INSTANCE_BASE_URL. Configure APPSMITH_BASE_URL at container startup in every CI environment that runs Cypress, so E2E exercises the new secure path end-to-end and CI mirrors how self-hosted operators must configure their instance. - ci-test-limited.yml, ci-test-limited-with-count.yml, ci-test-custom-script.yml, ci-test-playwright.yml: add -e APPSMITH_BASE_URL=http://localhost to the docker run --name appsmith command. The container exposes port 80 and Cypress hits http://localhost; browsers omit :80 from the Origin header for default-port URLs, so the strict-mode equality check in SecureBaseUrlResolverCEImpl matches. - scripts/deploy_preview.sh: add --set applicationConfig.APPSMITH_BASE_URL=https://$DOMAINNAME to the helm upgrade command. $DOMAINNAME is already computed earlier in the script as $edition-$PULL_REQUEST_NUMBER.dp.appsmith.com. - scripts/local_testing.sh: same env var added to the developer-local Docker run helper, for parity with CI. Approach A (configure the var) was chosen over Approach B (set the migration flag APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS=true) so that CI exercises the new secure path E2E rather than the deprecated migration fallback. The fail-closed semantics introduced by the GHSA fix remain pinned by the SecureBaseUrlResolverCEImplTest unit tests (no Spring context) and by the unit/integration suite's test profile. Design notes: docs/superpowers/specs/2026-04-28-ci-base-url-config-design.md Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw --- .github/workflows/ci-test-custom-script.yml | 1 + .../workflows/ci-test-limited-with-count.yml | 1 + .github/workflows/ci-test-limited.yml | 1 + .github/workflows/ci-test-playwright.yml | 1 + .../2026-04-28-ci-base-url-config-design.md | 83 +++++++++++++++++++ scripts/deploy_preview.sh | 1 + scripts/local_testing.sh | 2 +- 7 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 docs/superpowers/specs/2026-04-28-ci-base-url-config-design.md diff --git a/.github/workflows/ci-test-custom-script.yml b/.github/workflows/ci-test-custom-script.yml index d29112a57aa6..fcca7faeb058 100644 --- a/.github/workflows/ci-test-custom-script.yml +++ b/.github/workflows/ci-test-custom-script.yml @@ -188,6 +188,7 @@ jobs: -e APPSMITH_CLOUD_SERVICES_BASE_URL=http://host.docker.internal:5001 \ -e APPSMITH_CLOUD_SERVICES_SIGNATURE_BASE_URL=http://host.docker.internal:8090 \ -e APPSMITH_RATE_LIMIT=1000 \ + -e APPSMITH_BASE_URL=http://localhost \ --add-host=host.docker.internal:host-gateway --add-host=api.segment.io:host-gateway --add-host=t.appsmith.com:host-gateway \ cicontainer diff --git a/.github/workflows/ci-test-limited-with-count.yml b/.github/workflows/ci-test-limited-with-count.yml index cf9be992f9bf..891d357e6cee 100644 --- a/.github/workflows/ci-test-limited-with-count.yml +++ b/.github/workflows/ci-test-limited-with-count.yml @@ -273,6 +273,7 @@ jobs: -e APPSMITH_DISABLE_TELEMETRY=true \ -e APPSMITH_INTERCOM_APP_ID=DUMMY_VALUE \ -e APPSMITH_CLOUD_SERVICES_BASE_URL=http://host.docker.internal:5001 \ + -e APPSMITH_BASE_URL=http://localhost \ --add-host=host.docker.internal:host-gateway --add-host=api.segment.io:host-gateway --add-host=t.appsmith.com:host-gateway \ cicontainer diff --git a/.github/workflows/ci-test-limited.yml b/.github/workflows/ci-test-limited.yml index 4c5b65c1ecfd..38949bdeff77 100644 --- a/.github/workflows/ci-test-limited.yml +++ b/.github/workflows/ci-test-limited.yml @@ -184,6 +184,7 @@ jobs: -e APPSMITH_DISABLE_TELEMETRY=true \ -e APPSMITH_INTERCOM_APP_ID=DUMMY_VALUE \ -e APPSMITH_CLOUD_SERVICES_BASE_URL=http://host.docker.internal:5001 \ + -e APPSMITH_BASE_URL=http://localhost \ --add-host=host.docker.internal:host-gateway --add-host=api.segment.io:host-gateway --add-host=t.appsmith.com:host-gateway \ cicontainer diff --git a/.github/workflows/ci-test-playwright.yml b/.github/workflows/ci-test-playwright.yml index b3f7d88f5894..bf08146bb757 100644 --- a/.github/workflows/ci-test-playwright.yml +++ b/.github/workflows/ci-test-playwright.yml @@ -174,6 +174,7 @@ jobs: -e APPSMITH_CLOUD_SERVICES_BASE_URL=http://host.docker.internal:5001 \ -e APPSMITH_CLOUD_SERVICES_SIGNATURE_BASE_URL=http://host.docker.internal:8090 \ -e APPSMITH_RATE_LIMIT=1000 \ + -e APPSMITH_BASE_URL=http://localhost \ -e APPSMITH_CARBON_API_BASE_PATH=https://carbon.appsmith.com \ -e APPSMITH_CARBON_API_KEY="$APPSMITH_CARBON_API_KEY" \ -e APPSMITH_AI_SERVER_MANAGED_HOSTING=true \ diff --git a/docs/superpowers/specs/2026-04-28-ci-base-url-config-design.md b/docs/superpowers/specs/2026-04-28-ci-base-url-config-design.md new file mode 100644 index 000000000000..7f3161451c1c --- /dev/null +++ b/docs/superpowers/specs/2026-04-28-ci-base-url-config-design.md @@ -0,0 +1,83 @@ +# CI Configuration for `APPSMITH_BASE_URL` (GHSA-j9gf-vw2f-9hrw follow-up) + +**Date:** 2026-04-28 +**Author:** subrata71 +**Companion to:** [PR #41766](https://github.com/appsmithorg/appsmith/pull/41766) (CE), [PR #8997](https://github.com/appsmithorg/appsmith-ee/pull/8997) (EE) +**Linear:** [APP-15046](https://linear.app/appsmith/issue/APP-15046) + +## Context + +The fix for [GHSA-j9gf-vw2f-9hrw](https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw) makes the server fail-closed for token-bearing email flows (forgot-password, email verification, workspace invite, instance-admin invite) whenever `APPSMITH_BASE_URL` is unset. The Cypress E2E suite in `ci-test-*` workflows spins up a fresh Appsmith Docker container that does not set `APPSMITH_BASE_URL`, so every spec exercising those flows now fails with `MISCONFIGURED_INSTANCE_BASE_URL`. + +Concrete failures observed on the [first PR run](https://github.com/appsmithorg/appsmith/actions/runs/25035460641): `Email_settings_Spec.ts`, `ExportApplication_spec.js`, `DeleteWorkspace_spec.ts`, `LeaveWorkspaceTest_spec.js`, `MemberRoles_Spec.ts`, `ShareAppTests_Spec.ts`. All emit the same client-side error: `APPSMITH_BASE_URL is not configured. Token-bearing email flows are disabled until the canonical instance URL is set.` + +The unit/integration test suite was already addressed in commit `35eb077e58` by setting `APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS=true` in `application-test.properties`. That fix does **not** apply to Cypress, which runs against the production profile of a deployed Docker container. + +## Decision + +**Approach A: configure `APPSMITH_BASE_URL` at container startup in every CI environment that runs Cypress.** Treat CI as a real deployment that satisfies the new contract. Do **not** use the migration flag (`APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS`) in CI — we want E2E coverage of the new secure path. + +Rationale: +- Production-realistic. CI becomes the canonical example of how self-hosted operators should configure their instance. +- Exercises the new strict-mode `Origin == APPSMITH_BASE_URL` check end-to-end. +- One-time small workflow change (~5 files). +- The fail-closed semantics introduced by the GHSA fix are still pinned by `SecureBaseUrlResolverCEImplTest` unit tests, so the test pyramid stays balanced. + +Approach B (`APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS=true` in CI) was rejected because it would never exercise the secure path E2E and would spam every CI log with the helper's WARN message. + +## Files to change + +### CI workflows (5 lines each, identical pattern) + +For each of these workflows, add `-e APPSMITH_BASE_URL=http://localhost \` to the `docker run -d --name appsmith` block: + +| File | Approximate line of `docker run --name appsmith` | +|------|--------------------------------------------------| +| `.github/workflows/ci-test-limited.yml` | 182 | +| `.github/workflows/ci-test-limited-with-count.yml` | 271 | +| `.github/workflows/ci-test-custom-script.yml` | 184 | +| `.github/workflows/ci-test-playwright.yml` | 169 | + +The container exposes port 80 → host port 80, and Cypress hits `http://localhost`. Browsers omit `:80` from the `Origin` header for default-port URLs, so `APPSMITH_BASE_URL=http://localhost` matches exactly. + +### Deploy preview (Helm) + +`scripts/deploy_preview.sh` (around line 117) — add to the `helm upgrade` arg list: + +``` +--set applicationConfig.APPSMITH_BASE_URL=https://$DOMAINNAME +``` + +`$DOMAINNAME` is already computed on line 22 as `$edition-$PULL_REQUEST_NUMBER.dp.appsmith.com`. The Helm chart already propagates `applicationConfig.*` to container env (existing pattern: `APPSMITH_DB_URL`, `APPSMITH_SENTRY_DSN`, …). + +### Local developer parity (optional) + +`scripts/local_testing.sh` (line 124) — add `-e APPSMITH_BASE_URL=http://localhost \` to the local docker-run helper. This keeps developer-local Docker behavior consistent with CI. Skip if you'd rather minimize blast radius. + +## Files explicitly NOT changed + +- `.github/workflows/ci-test-hosted.yml` — runs against an externally-managed hosted Appsmith instance (not Docker). The hosted instance's configuration is owned by ops; the workflow itself doesn't `docker run` Appsmith. +- `.github/workflows/server-build.yml`, `server-integration-tests.yml` — server unit/integration tests via Maven; already fixed via `application-test.properties`. +- `.github/workflows/build-docker-image.yml`, `test-build-docker-image.yml` — they orchestrate but do not themselves `docker run` Appsmith. They invoke the child workflows above. +- The Docker image / Dockerfile itself — must NOT bake in any default `APPSMITH_BASE_URL` value. Production deployments need their own canonical URL; baking a default would defeat the security improvement. + +## Risks + +1. **Origin string mismatch.** Strict-mode requires exact equality. Cypress sends `Origin: http://localhost` for default-port traffic to `http://localhost`. Verified by reading the existing `Email_settings_Spec.ts` test 3 (lines 172-181) which already strips trailing slashes from `originUrl` before forwarding — confirming the team has thought about this surface. **Mitigation:** if a spec passes a different Origin, we'll see `Origin header does not match APPSMITH_BASE_URL configuration` (HTTP 400) — clear and actionable, not a silent failure. + +2. **Helm chart key naming.** `applicationConfig.APPSMITH_BASE_URL` — confirmed pattern matches existing entries. **Mitigation:** the existing deploy preview will surface any mistake on first run. + +3. **Rollback.** If anything misbehaves, removing the env vars from these files restores prior CI behavior. Production fail-closed default in the server is unaffected. + +## Validation + +1. Re-trigger the failing CI run on PR #41766. All previously-failing Cypress specs should pass. +2. Re-trigger a deploy preview build to verify `APPSMITH_BASE_URL` propagates correctly via Helm. +3. Smoke-test by running one Cypress spec locally with `APPSMITH_BASE_URL=http://localhost` set on a fresh container. + +## Out of scope + +- Adding a Cypress `before` hook to set `APPSMITH_BASE_URL` via the admin settings UI — not needed since we set it at container startup. +- Cypress test code changes — none needed. +- Changes to the Docker image (Dockerfile / base.dockerfile). +- Documenting the new requirement in user-facing operator docs — separate doc PR after release. diff --git a/scripts/deploy_preview.sh b/scripts/deploy_preview.sh index 8c9eafea96fd..a91fa8c78cf8 100755 --- a/scripts/deploy_preview.sh +++ b/scripts/deploy_preview.sh @@ -115,6 +115,7 @@ helm upgrade -i "$CHARTNAME" "appsmith-ee/$HELMCHART" -n "$NAMESPACE" --create-n --set applicationConfig.APPSMITH_BETTERBUGS_API_KEY="$APPSMITH_BETTERBUGS_API_KEY" \ --set applicationConfig.APPSMITH_PYLON_APP_ID="$APPSMITH_PYLON_APP_ID" \ --set applicationConfig.IN_DOCKER="$IN_DOCKER" \ + --set applicationConfig.APPSMITH_BASE_URL="https://$DOMAINNAME" \ --set applicationConfig.APPSMITH_CUSTOMER_PORTAL_URL="https://release-customer.appsmith.com" \ --set affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms[0].matchExpressions[0].key=instance_name \ --set affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms[0].matchExpressions[0].operator=In \ diff --git a/scripts/local_testing.sh b/scripts/local_testing.sh index b8fd6295ca68..fad1f8740e4a 100755 --- a/scripts/local_testing.sh +++ b/scripts/local_testing.sh @@ -121,4 +121,4 @@ docker build -t appsmith/appsmith-local-$edition:$tag \ pretty_print "Docker image build successful. Triggering run now ..." (docker stop appsmith || true) && (docker rm appsmith || true) -docker run -d --name appsmith -p 80:80 -v "$PWD/stacks:/appsmith-stacks" appsmith/appsmith-local-$edition:$tag && sleep 15 && pretty_print "Local instance is up! Open Appsmith at http://localhost! " +docker run -d --name appsmith -p 80:80 -v "$PWD/stacks:/appsmith-stacks" -e APPSMITH_BASE_URL=http://localhost appsmith/appsmith-local-$edition:$tag && sleep 15 && pretty_print "Local instance is up! Open Appsmith at http://localhost! " From f020589818e4cd7f2d74093663db0748ec142bcf Mon Sep 17 00:00:00 2001 From: subrata71 Date: Wed, 29 Apr 2026 01:10:30 +0600 Subject: [PATCH 05/22] fix(security): compare APPSMITH_BASE_URL and Origin by URL origin (GHSA-j9gf-vw2f-9hrw) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Strict-mode comparison previously used raw String equality, which spuriously rejected requests where the Origin header and the configured APPSMITH_BASE_URL differed only in insignificant URL syntax. CI Cypress (and any real-world deployment whose APPSMITH_BASE_URL syntax doesn't byte-equal the browser's Origin header) consequently saw HTTP 400s with 'Origin header does not match APPSMITH_BASE_URL configuration'. Replace the equality check with an RFC 6454 origin comparison: - Parse both values as URIs. - Lowercase scheme and host. - Compute effective port (80 for http, 443 for https when not specified). - Compare scheme + host + effective port. Trailing slashes, default-port elision (http://example.com vs http://example.com:80), and host-name casing now resolve to the same origin — matching how browsers populate the Origin header. Differences in scheme, host, or non-default port still error out, preserving the strict-mode protection from PR #41426. Userinfo tricks (https://appsmith.example@evil.com) still fail because the URI parser places the legitimate-looking string in userinfo and puts the actual host in .getHost(). The resolver now also logs a WARN with both observed values when it rejects a mismatch, so operators can debug misconfigurations without enabling DEBUG. Eight new unit-test cases pin the new semantics: - trailing-slash insensitivity (configured vs Origin) - default-port elision for http and https - case-insensitive host comparison - scheme mismatch still rejected - non-default port mismatch still rejected - malformed URLs still rejected - userinfo tricks still rejected All 16 SecureBaseUrlResolverCEImplTest cases pass locally, plus the broader UserServiceTest, EmailServiceCEImplTest, WorkspaceServiceTest, and AppsmithErrorTest suites (57 tests total). Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw --- .../ce/SecureBaseUrlResolverCEImpl.java | 74 +++++++++++++++- .../ce/SecureBaseUrlResolverCEImplTest.java | 87 +++++++++++++++++++ 2 files changed, 157 insertions(+), 4 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java index aae58a756d2f..aadb34f8f3f0 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java @@ -7,6 +7,11 @@ import org.springframework.util.StringUtils; import reactor.core.publisher.Mono; +import java.net.URI; +import java.net.URISyntaxException; +import java.util.Locale; +import java.util.Objects; + /** * CE implementation of {@link SecureBaseUrlResolverCE}. * @@ -14,9 +19,10 @@ * *

    *
  1. If {@code APPSMITH_BASE_URL} is configured, it is the only acceptable host. If - * the provided value does not match, an {@link AppsmithException} is emitted - * (preserves the strict-mode protection added in PR #41426 for - * GHSA-7hf5-mc28-xmcv). + * the provided value does not match (compared by URL origin — scheme + host + effective port, + * per RFC 6454), an {@link AppsmithException} is emitted. This preserves the strict-mode + * protection added in PR #41426 for + * GHSA-7hf5-mc28-xmcv. * The insecure compatibility flag does NOT weaken this branch.
    2. *
  2. If {@code APPSMITH_BASE_URL} is unset and {@code APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS} * is true, the legacy behaviour is restored: the caller-supplied value is @@ -50,7 +56,11 @@ public class SecureBaseUrlResolverCEImpl implements SecureBaseUrlResolverCE { @Override public Mono resolveSecureBaseUrl(String providedBaseUrl) { if (StringUtils.hasText(appsmithBaseUrl)) { - if (!appsmithBaseUrl.equals(providedBaseUrl)) { + if (!sameOrigin(appsmithBaseUrl, providedBaseUrl)) { + log.warn( + "Origin mismatch: provided='{}' does not match configured APPSMITH_BASE_URL='{}'.", + providedBaseUrl, + appsmithBaseUrl); return Mono.error(new AppsmithException( AppsmithError.GENERIC_BAD_REQUEST, "Origin header does not match APPSMITH_BASE_URL configuration.")); @@ -71,4 +81,60 @@ public Mono resolveSecureBaseUrl(String providedBaseUrl) { + "See https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw"); return Mono.empty(); } + + /** + * Compares two URLs by their origin (scheme + host + effective port) per RFC 6454, rather than + * by raw string equality. Tolerates insignificant differences such as trailing slashes and + * default-port elision (e.g. {@code http://example.com} and {@code http://example.com:80} are + * the same origin) while still rejecting any difference in scheme, host, or non-default port. + * + *

    Returns {@code false} for any malformed URL or null/blank input. Userinfo, path, query, + * and fragment are deliberately ignored — only the security-relevant origin is compared. + */ + private static boolean sameOrigin(String configured, String provided) { + if (!StringUtils.hasText(configured) || !StringUtils.hasText(provided)) { + return false; + } + try { + URI configuredUri = new URI(configured.trim()); + URI providedUri = new URI(provided.trim()); + + String configuredScheme = lowerCase(configuredUri.getScheme()); + String providedScheme = lowerCase(providedUri.getScheme()); + if (!Objects.equals(configuredScheme, providedScheme)) { + return false; + } + + String configuredHost = lowerCase(configuredUri.getHost()); + String providedHost = lowerCase(providedUri.getHost()); + if (configuredHost == null || !Objects.equals(configuredHost, providedHost)) { + // Reject when host can't be parsed (e.g. opaque URIs) or hosts differ. + return false; + } + + return effectivePort(configuredUri) == effectivePort(providedUri); + } catch (URISyntaxException e) { + log.warn("Failed to parse URL for origin comparison: {}", e.getMessage()); + return false; + } + } + + private static int effectivePort(URI uri) { + int port = uri.getPort(); + if (port != -1) { + return port; + } + String scheme = lowerCase(uri.getScheme()); + if ("http".equals(scheme)) { + return 80; + } + if ("https".equals(scheme)) { + return 443; + } + return -1; + } + + private static String lowerCase(String value) { + return value == null ? null : value.toLowerCase(Locale.ROOT); + } } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java index 8a1e9564341c..228b2a551398 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java @@ -118,4 +118,91 @@ void resolveSecureBaseUrl_constructed_isNotNull() throws Exception { SecureBaseUrlResolverCEImpl resolver = newResolver("https://appsmith.example", false); assertNotNull(resolver); } + + // region URL-origin normalisation — comparison must follow RFC 6454 (scheme + host + effective port), + // not raw string equality. Without this, real-world deployments hit spurious mismatches whenever the + // configured value and the inbound `Origin` header differ on insignificant syntax (trailing slash, + // default-port elision, host case). All accepted matches below resolve to the SAME origin per the + // RFC; all rejected ones genuinely differ in scheme, host, or non-default port. + + @Test + void resolveSecureBaseUrl_whenConfiguredHasTrailingSlash_andOriginDoesNot_match() throws Exception { + SecureBaseUrlResolverCEImpl resolver = newResolver("http://localhost/", false); + + StepVerifier.create(resolver.resolveSecureBaseUrl("http://localhost")) + .expectNext("http://localhost/") + .verifyComplete(); + } + + @Test + void resolveSecureBaseUrl_whenOriginHasTrailingSlash_andConfiguredDoesNot_match() throws Exception { + SecureBaseUrlResolverCEImpl resolver = newResolver("http://localhost", false); + + StepVerifier.create(resolver.resolveSecureBaseUrl("http://localhost/")) + .expectNext("http://localhost") + .verifyComplete(); + } + + @Test + void resolveSecureBaseUrl_whenOriginHasDefaultHttpPort_andConfiguredOmitsIt_match() throws Exception { + SecureBaseUrlResolverCEImpl resolver = newResolver("http://localhost", false); + + StepVerifier.create(resolver.resolveSecureBaseUrl("http://localhost:80")) + .expectNext("http://localhost") + .verifyComplete(); + } + + @Test + void resolveSecureBaseUrl_whenOriginHasDefaultHttpsPort_andConfiguredOmitsIt_match() throws Exception { + SecureBaseUrlResolverCEImpl resolver = newResolver("https://appsmith.example", false); + + StepVerifier.create(resolver.resolveSecureBaseUrl("https://appsmith.example:443")) + .expectNext("https://appsmith.example") + .verifyComplete(); + } + + @Test + void resolveSecureBaseUrl_whenHostCasingDiffers_match() throws Exception { + SecureBaseUrlResolverCEImpl resolver = newResolver("https://Appsmith.Example", false); + + StepVerifier.create(resolver.resolveSecureBaseUrl("https://appsmith.example")) + .expectNext("https://Appsmith.Example") + .verifyComplete(); + } + + @Test + void resolveSecureBaseUrl_whenSchemesDiffer_errors() throws Exception { + SecureBaseUrlResolverCEImpl resolver = newResolver("https://appsmith.example", false); + + StepVerifier.create(resolver.resolveSecureBaseUrl("http://appsmith.example")) + .verifyError(AppsmithException.class); + } + + @Test + void resolveSecureBaseUrl_whenNonDefaultPortsDiffer_errors() throws Exception { + SecureBaseUrlResolverCEImpl resolver = newResolver("http://localhost:8080", false); + + StepVerifier.create(resolver.resolveSecureBaseUrl("http://localhost:9090")) + .verifyError(AppsmithException.class); + } + + @Test + void resolveSecureBaseUrl_whenOriginIsMalformed_errors() throws Exception { + SecureBaseUrlResolverCEImpl resolver = newResolver("https://appsmith.example", false); + + StepVerifier.create(resolver.resolveSecureBaseUrl("not a url")).verifyError(AppsmithException.class); + } + + @Test + void resolveSecureBaseUrl_whenAttackerUsesUserinfoTrick_errors() throws Exception { + // Tricks like https://appsmith.example@evil.com must NOT be accepted as the same origin + // as https://appsmith.example. URI parsing places appsmith.example in userinfo and evil.com + // as the host — so the host comparison rejects. + SecureBaseUrlResolverCEImpl resolver = newResolver("https://appsmith.example", false); + + StepVerifier.create(resolver.resolveSecureBaseUrl("https://appsmith.example@evil.example")) + .verifyError(AppsmithException.class); + } + + // endregion } From 67b27ca17461d4a11d985f2b386cc3affe6c330b Mon Sep 17 00:00:00 2001 From: subrata71 Date: Wed, 29 Apr 2026 12:09:56 +0600 Subject: [PATCH 06/22] test(cypress): use real baseUrl as Origin in admin/invite intercepts (GHSA-j9gf-vw2f-9hrw) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cypress's commands.js had been intentionally setting `req.headers["origin"] = "Cypress"` (literal string) on five intercepts: - POST /api/v1/users/invite - POST /api/v1/applications/invite - POST /api/v1/git/applications/*/connect - PUT /api/v1/admin/env - PUT /api/v1/tenants The literal string "Cypress" was a legacy synthetic-traffic flag — never a meaningful URL. With the GHSA-j9gf-vw2f-9hrw fix, the server now strict-mode matches `Origin` against `APPSMITH_BASE_URL` (URL-origin comparison per RFC 6454). The literal string fails to parse as a URL, so every intercepted request errors with HTTP 400 "Origin header does not match APPSMITH_BASE_URL configuration." The peer file `HomePage.ts#StubPostHeaderReq` (and `e2e.js`'s admin/env intercept) already used `Cypress.config("baseUrl")` correctly. Align commands.js to the same pattern. No production behavior change — only the test infrastructure now sends a valid Origin matching the running deploy. Verified locally that the diff is the only mention of the literal string in real code; the only remaining occurrence is a long-commented-out line in AggregateHelper.ts which is left untouched. Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw --- app/client/cypress/support/commands.js | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/app/client/cypress/support/commands.js b/app/client/cypress/support/commands.js index b009e58f1376..0483e836ec05 100644 --- a/app/client/cypress/support/commands.js +++ b/app/client/cypress/support/commands.js @@ -98,11 +98,15 @@ export const addIndexedDBKey = (key, value) => { }; Cypress.Commands.add("stubPostHeaderReq", () => { + // GHSA-j9gf-vw2f-9hrw: server-side strict-mode now requires Origin to match + // APPSMITH_BASE_URL. Use the live Cypress baseUrl so the intercept produces a + // valid Origin (the literal string "Cypress" used to live here as a synthetic- + // traffic flag and no longer parses as a URL). cy.intercept("POST", "/api/v1/users/invite", (req) => { - req.headers["origin"] = "Cypress"; + req.headers["origin"] = Cypress.config("baseUrl"); }).as("mockPostInvite"); cy.intercept("POST", "/api/v1/applications/invite", (req) => { - req.headers["origin"] = "Cypress"; + req.headers["origin"] = Cypress.config("baseUrl"); }).as("mockPostAppInvite"); }); @@ -752,7 +756,8 @@ Cypress.Commands.add("startServerAndRoutes", () => { hostname: window.location.host, }, (req) => { - req.headers["origin"] = "Cypress"; + // GHSA-j9gf-vw2f-9hrw: send a real Origin matching APPSMITH_BASE_URL. + req.headers["origin"] = Cypress.config("baseUrl"); }, ).as("connectGitLocalRepo"); @@ -766,13 +771,17 @@ Cypress.Commands.add("startServerAndRoutes", () => { }); cy.intercept("PUT", "/api/v1/admin/env", (req) => { - req.headers["origin"] = "Cypress"; + // GHSA-j9gf-vw2f-9hrw: server-side strict-mode now requires Origin to match + // APPSMITH_BASE_URL. Use the live Cypress baseUrl instead of the legacy + // synthetic-traffic flag "Cypress" (which no longer parses as a URL). + req.headers["origin"] = Cypress.config("baseUrl"); }).as("postEnv"); cy.intercept("GET", "/settings/general").as("getGeneral"); cy.intercept("GET", "/api/v1/tenants/current").as("signUpLogin"); cy.intercept("PUT", "/api/v1/tenants", (req) => { - req.headers["origin"] = "Cypress"; + // GHSA-j9gf-vw2f-9hrw: see note above. + req.headers["origin"] = Cypress.config("baseUrl"); }).as("postTenant"); cy.intercept("PUT", "/api/v1/git/applications/*/discard").as( "discardChanges", From 306e492772c3b6f2692b91e40d68ee81ce6299e0 Mon Sep 17 00:00:00 2001 From: subrata71 Date: Thu, 30 Apr 2026 00:48:10 +0600 Subject: [PATCH 07/22] fix(security): bump MISCONFIGURED_INSTANCE_BASE_URL to AE-APP-5048 (GHSA-j9gf-vw2f-9hrw) The previously-assigned code AE-APP-5046 is unique in CE but collides with EE-only GENERATE_ACTION_VISUALIZATION_FAILED. Bump to the next free code in both repos (5047 is also taken in EE by GENERATE_ACTION_SCHEMA_FAILED). This unblocks the EE shadow PR's AppsmithErrorTest#verifyUniquenessOfAppsmithErrorCode once the cherry-picked CE history reaches it. The companion EE-side commit applies the same single-line edit on top of the EE shadow PR. Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw --- .../java/com/appsmith/server/exceptions/AppsmithErrorCode.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java index 43b541d678ec..a9cd4e68ce8a 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java @@ -64,7 +64,7 @@ public enum AppsmithErrorCode { GOOGLE_RECAPTCHA_TIMEOUT("AE-APP-5042", "Google recaptcha timeout"), MIGRATION_FAILED("AE-APP-5043", "Migration failed"), INVALID_PROPERTIES_CONFIGURATION("AE-APP-5044", "Property configuration is wrong or malformed"), - MISCONFIGURED_INSTANCE_BASE_URL("AE-APP-5046", "Instance base URL is not configured"), + MISCONFIGURED_INSTANCE_BASE_URL("AE-APP-5048", "Instance base URL is not configured"), NAME_CLASH_NOT_ALLOWED_IN_REFACTOR("AE-AST-4009", "Name clash not allowed in refactor"), GENERIC_BAD_REQUEST("AE-BAD-4000", "Generic bad request"), MALFORMED_REQUEST("AE-BAD-4001", "Malformed request body"), From acdb86c3303cd1494bfcf9e748ff7df6bf51e4d8 Mon Sep 17 00:00:00 2001 From: subrata71 Date: Thu, 30 Apr 2026 01:00:28 +0600 Subject: [PATCH 08/22] =?UTF-8?q?docs(security):=20design=20=E2=80=94=20ad?= =?UTF-8?q?min=20warning=20banner=20for=20unset=20APPSMITH=5FBASE=5FURL=20?= =?UTF-8?q?(GHSA-j9gf-vw2f-9hrw)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Specifies the admin-facing companion to the GHSA-j9gf-vw2f-9hrw fail-closed fix: a top-of-screen banner shown only to instance super-users when APPSMITH_BASE_URL is unset and the resolver is therefore in fail-closed mode for token-bearing email flows. Covers the resolver getter, the UserProfile DTO field, the dedicated React component, the gating selector, the deep-link CTA to Admin Settings → Configuration, the Redis-session-survives-restart recovery loop, the EE multi-org override (never fires on Appsmith Cloud), error handling across three failure modes, edge cases, testing strategy, and explicit out-of-scope. Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw Linear: APP-15046 --- ...29-admin-base-url-warning-banner-design.md | 224 ++++++++++++++++++ 1 file changed, 224 insertions(+) create mode 100644 docs/superpowers/specs/2026-04-29-admin-base-url-warning-banner-design.md diff --git a/docs/superpowers/specs/2026-04-29-admin-base-url-warning-banner-design.md b/docs/superpowers/specs/2026-04-29-admin-base-url-warning-banner-design.md new file mode 100644 index 000000000000..9d255c543ddc --- /dev/null +++ b/docs/superpowers/specs/2026-04-29-admin-base-url-warning-banner-design.md @@ -0,0 +1,224 @@ +# Admin Warning Banner for Unset `APPSMITH_BASE_URL` (GHSA-j9gf-vw2f-9hrw follow-up) + +**Date:** 2026-04-29 +**Author:** subrata71 +**Companion to:** [PR #41766](https://github.com/appsmithorg/appsmith/pull/41766) (CE), [PR #8997](https://github.com/appsmithorg/appsmith-ee/pull/8997) (EE) — the GHSA-j9gf-vw2f-9hrw fail-closed fix +**Linear:** [APP-15046](https://linear.app/appsmith/issue/APP-15046) + +## Context + +The fix for [GHSA-j9gf-vw2f-9hrw](https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw) makes the server fail-closed for token-bearing email flows (forgot-password, email verification, workspace invite, instance-admin invite) whenever `APPSMITH_BASE_URL` is unset. That is the security-correct default, but operationally it creates a silent-degradation surface: a self-hosted instance admin who upgrades without setting `APPSMITH_BASE_URL` will not realise their instance is misconfigured until end-users complain that password-reset and verification emails never arrive. + +The reporter's own recommendation (verbatim, item 3 in the GHSA discussion) was: + +> "If APPSMITH_BASE_URL is unset, fail closed in the email-generation path. ... Emit clear server log + **admin warning** + health/configuration signal that token-bearing email flows are disabled until APPSMITH_BASE_URL is configured." + +The server-log piece already exists (a WARN log on every resolver call). This document covers the **admin warning** piece — an in-product banner shown only to instance admins when the server is in this fail-closed state. + +## Goal + +Show a non-dismissible warning banner at the top of the application to logged-in instance admins (super users with admin-settings access) **only** when: + +- The instance is in CE, OR +- The instance is in EE with the `license_multi_org_enabled` feature flag **off**, AND +- `APPSMITH_BASE_URL` is unset. + +The banner explains that token-bearing email flows are disabled and deep-links to Admin Settings → Configuration where the admin can set the value. Saving the value triggers Appsmith's existing Configuration-tier server-restart flow, which auto-reloads the SPA and the banner disappears (because the resolver re-reads the env on startup). + +## Non-goals (v1) + +- **Insecure-flag-on warning.** When `APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS=true`, the banner does **not** fire even though the deployment is insecure. That is a separate decision with different copy and stronger language; deferred. +- **EE multi-org.** Multi-org EE instances (Appsmith Cloud) derive their email-link host from `organizationService.getOrganizationBaseUrl()` (slug + deploymentDomain) and do not depend on `APPSMITH_BASE_URL`. The banner must **never** appear there. +- **Per-user dismissal / snooze / reminder cadence.** The banner reflects live server state. The misconfig itself is the source of truth — clear the misconfig, the banner clears. +- **Audit log entry** when banner displays. +- **Severity escalation** if the admin keeps ignoring the banner. + +## Architecture + +### Signal source — extend the existing resolver + +`SecureBaseUrlResolverCE` already centralises the question "what canonical host should this instance use for email links?". It is the natural place to ask "is that question well-formed for this instance?". Add one new reactive method: + +```java +Mono isBaseUrlConfigurationHealthy(); +``` + +Semantics: *"can this instance generate token-bearing email links without depending on a request-time hint?"* True ⇒ no banner. False ⇒ banner. + +| Implementation | Returns | +|---|---| +| `SecureBaseUrlResolverCEImpl` (CE) | `Mono.just(StringUtils.hasText(appsmithBaseUrl))` | +| `SecureBaseUrlResolverImpl` (EE override) | If `featureFlagService.check(license_multi_org_enabled)` is true → `Mono.just(true)`; else → `super.isBaseUrlConfigurationHealthy()` | + +The EE override mirrors the same `featureFlagService.check(...).flatMap(...)` shape that already exists for `resolveSecureBaseUrl`, so the two methods stay symmetrical. + +The method is reactive because the EE override needs the feature flag service. The DTO assembler (`UserServiceCEImpl#buildUserProfile`) is already a reactive chain, so this folds in via a single `.zipWith(...)` step. + +### Signal pathway — ride on `UserProfileCE_DTO` + +`UserProfileCE_DTO` already carries instance-state signals (`isEmptyInstance`, `isSuperUser`, `adminSettingsVisible`) that the React client uses for gating. Add one boolean field: + +```java +@JsonProperty("instanceBaseUrlConfigurationHealthy") +boolean instanceBaseUrlConfigurationHealthy = true; +``` + +Default `true` — any DTO instance that bypasses the real assembler (test fixtures, mock contexts, future code paths) behaves as "configured" and does not false-positive the banner. + +`UserServiceCEImpl#buildUserProfileDTO(User)` injects the resolver and calls `isBaseUrlConfigurationHealthy()` as part of the existing reactive build chain. No new endpoint, no new saga, no new Redux state. + +### Client — dedicated banner component + +A new component `BaseUrlMissingBanner.tsx` lives at `app/client/src/components/editorComponents/`. It uses `Callout` from `@appsmith/ads` (`kind="warning"`), is rendered at the top of the application above `AppHeader`, and is **not user-dismissible** (no close button, no `isClosable`). The component returns `null` when the gating selector says so, so it costs nothing for non-admin users. + +Layout: rendered as a regular block element at the top of the authed-routes container in `AppRouter.tsx`. Pushes everything below it down by its own height naturally — no body class, no manual offset, no CSS hack. Existing `` (bottom-of-screen) coexists fine. + +Selector in `selectors/usersSelectors.ts`: + +```ts +const getShouldShowBaseUrlMissingBanner = (state: AppState): boolean => { + const user = state.ui.users.currentUser; + return Boolean( + user?.isSuperUser && + user?.adminSettingsVisible && + user?.instanceBaseUrlConfigurationHealthy === false // explicit !== !value + ); +}; +``` + +The `=== false` check (rather than `!value`) is deliberate: during a rolling deploy where a newer client briefly sees an older server's response without the field, `undefined === false` is `false`, so the banner stays hidden. Once both sides redeploy, the explicit `false` flips it on. + +### CTA — deep-link to the existing Admin Settings field + +`APPSMITH_BASE_URL` is already a registered setting in the Admin Settings → Configuration tab (`SettingCategories.CONFIGURATION` / `CategoryType.INSTANCE`, defined at `app/client/src/ce/pages/AdminSettings/config/configuration.tsx`). The banner has one `CalloutLink` whose `to` is built via the existing helper: + +```ts +adminSettingsCategoryUrl({ category: SettingCategories.CONFIGURATION }) +// → /settings/configuration +``` + +Same pattern as `AuthPage.tsx` already uses for its own admin-settings deep links. + +### Recovery loop (no extra code) + +Existing Configuration-tier admin-settings save flow does this naturally — verified by reading `app/client/src/ce/sagas/SuperUserSagas.tsx` (`SaveAdminSettingsSaga` → `RESTART_SERVER_POLL` → `RestartServerPoll` → `RestryRestartServerPoll` → `window.location.reload()`): + +``` +Admin clicks banner CTA + → /settings/configuration + → enters APPSMITH_BASE_URL value, clicks Save + → SAVE_ADMIN_SETTINGS_SUCCESS → RESTART_SERVER_POLL + → server restarts (Configuration-tier setting → needsRestart = true) + → client polls fetchCurrentOrganizationConfig until migrationStatus === COMPLETED + → window.location.reload() + → Redis-backed session survives → no login redirect + → fresh /v1/users/profile → instanceBaseUrlConfigurationHealthy: true + → selector returns false → banner gone +``` + +The admin sees only the same "Server is restarting…" UX they already know from changing any other Configuration-tier setting (Redis URL, DB URL, etc.). No login screen, no manual refresh. + +## Files to change + +### Server CE (4 files) + +| File | Change | +|---|---| +| `app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCE.java` | Add `Mono isBaseUrlConfigurationHealthy();` to the interface. | +| `app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java` | Implement: `return Mono.just(StringUtils.hasText(appsmithBaseUrl));` | +| `app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/UserProfileCE_DTO.java` | Add `boolean instanceBaseUrlConfigurationHealthy = true;` with `@JsonProperty`. | +| `app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java` | Inject `SecureBaseUrlResolverCE` and `.zipWith(...)` the resolver call into the existing `buildUserProfileDTO(User)` reactive chain. | + +### Server EE (1 file) + +| File | Change | +|---|---| +| `app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/SecureBaseUrlResolverImpl.java` | Override `isBaseUrlConfigurationHealthy()` with the multi-org gate (mirror the existing `resolveSecureBaseUrl` shape). | + +### Client CE (5 files) + +| File | Change | +|---|---| +| `app/client/src/components/editorComponents/BaseUrlMissingBanner.tsx` (new) | New component. `Callout kind="warning"`, copy + CTA, returns `null` when gating selector is false. Not closable. | +| `app/client/src/selectors/usersSelectors.ts` | Add `getShouldShowBaseUrlMissingBanner` selector with explicit `=== false` guard. | +| `app/client/src/ce/constants/messages.ts` | Three new message keys: title, body, CTA label. | +| `app/client/src/ce/AppRouter.tsx` | Mount `` as the first child of the authed-routes container, above ``. | +| `app/client/src/reducers/uiReducers/usersReducer.ts` (and any inferred user-profile TypeScript type) | Extend the user shape with the new optional `instanceBaseUrlConfigurationHealthy?: boolean`. | + +### Client EE (0 files) + +`UserProfileDTO extends UserProfileCE_DTO`, so the field flows through. `ee/AppRouter.tsx` extends/inherits `ce/AppRouter.tsx`'s structure — verify during implementation that the mount is picked up; if EE overrides the relevant region, mirror the mount in `ee/AppRouter.tsx`. + +## Multi-org behavior + +This is the most important EE-specific concern, so calling it out explicitly: + +| Deployment | Banner ever shows? | Why | +|---|---|---| +| CE (any), `APPSMITH_BASE_URL` set | No | `SecureBaseUrlResolverCEImpl#isBaseUrlConfigurationHealthy()` returns `true` | +| CE (any), `APPSMITH_BASE_URL` unset | Yes (super users only) | Resolver returns `false` | +| EE single-org, `APPSMITH_BASE_URL` set | No | EE override: multi-org off → delegate → CE returns `true` | +| EE single-org, `APPSMITH_BASE_URL` unset | Yes (super users only) | EE override: multi-org off → delegate → CE returns `false` | +| EE multi-org (Appsmith Cloud) | **Never** | EE override short-circuits to `Mono.just(true)` regardless of `APPSMITH_BASE_URL` | + +## Error handling + +Three failure modes worth nailing down: + +1. **`featureFlagService.check(...)` errors out** (FF service unreachable, DB blip). EE override defaults to "treat as multi-org off" via `.onErrorResume(e -> super.isBaseUrlConfigurationHealthy())`. Rationale: a false-positive banner is recoverable (admin sees a misleading warning, can ignore); a false-negative would silently leave a real misconfig invisible. Logged at WARN. +2. **Resolver `Mono` errors during DTO assembly.** The DTO assembler wraps the call with `.onErrorReturn(true)`. Login must never break because of a banner-signal error. Logged at ERROR (this would be a real bug). Net result: transient blip → no banner → admin keeps working → next session retries. +3. **Field missing from response** (rolling deploy). The TypeScript field is declared optional. The selector explicitly checks `=== false`, so `undefined` → no banner. Standard tri-state guard for new optional fields. + +## Edge cases (all by-construction, no extra code) + +| Case | Behavior | +|---|---| +| Anonymous user / logged out | `isSuperUser === false` → no banner | +| Logged-in non-admin | `isSuperUser === false` → no banner | +| Super user but `adminSettingsVisible === false` (RBAC, license tier) | No banner — CTA would be useless | +| EE multi-org enabled (Cloud) | Resolver short-circuits → no banner | +| Admin viewing `/settings/configuration` already | Banner still shows — actually helpful, contextually reinforces *which* field to set | +| Admin saves from banner CTA | Existing Configuration-tier save → server restart → auto page-reload → banner clears. **No re-login.** | +| Admin in Tab A, saves from Tab B | Tab B reloads itself (existing flow); Tab A keeps showing stale banner until manual refresh. Acceptable — matches every other admin-setting in Appsmith today. | +| Admin sets value via CLI / direct env, restarts container | No restart-poll triggered in any open tab. Banner persists in open tabs until manual refresh. Same as any Configuration-tier setting. | +| EE multi-org licence flips off mid-session | License changes restart the server anyway — natural fresh session, banner appears (correctly) on next login. | +| Two browser tabs open as same admin | Both show banner; both clear together when admin saves and the SPA reloads. | + +## Testing + +| Layer | What | Why | +|---|---|---| +| **Server unit (CE)** | `SecureBaseUrlResolverCEImplTest` adds 2 cases for `isBaseUrlConfigurationHealthy()` — `true` when `APPSMITH_BASE_URL` set, `false` when blank. | Pins CE semantics. No Spring context, fast, runs on every PR. | +| **Server unit (EE)** | New `SecureBaseUrlResolverImplTest` (or extend an existing one) with 3 cases: multi-org on → `true` regardless of BASE_URL; multi-org off + BASE_URL set → `true`; multi-org off + BASE_URL unset → `false`. Mocks `featureFlagService.check(...)`. | Pins the multi-org override — the only place this branching is exercised. | +| **Server integration** | `UserServiceTest` — one new case under the `buildUserProfileDTO` coverage asserting the new field is present and reflects the resolver's answer. | End-to-end through the DTO assembler so we don't regress the `.zipWith` wiring. | +| **Client unit (Jest)** | `BaseUrlMissingBanner.test.tsx` — five cases: `isSuperUser=true, adminSettingsVisible=true, healthy=false` → renders; `healthy=true` → null; `isSuperUser=false` → null; `adminSettingsVisible=false` → null; field undefined → null. | Pins selector + render gating. Fast, deterministic, runs on every client PR. | +| **Client unit (selector)** | `usersSelectors.test.ts` — one case for the explicit-`=== false` guard (undefined / missing → false). | Pins the rolling-deploy safety. | +| **Cypress** | **Skip.** | CI sets `APPSMITH_BASE_URL=http://localhost`, so the banner never fires in CI by construction. Writing a Cypress test that mutates `/admin/env` to clear it mid-run, restarts the server, and asserts the banner — high cost, low yield. The Jest cases cover rendering; the unit tests cover the resolver. | + +## Risks + +1. **Banner pushes layout down ~40-48px.** Existing Cypress specs that use absolute pixel coordinates against the AppHeader could break. Mitigation: most existing specs use semantic selectors (text, `data-cy`); the banner is hidden in CI anyway because `APPSMITH_BASE_URL=http://localhost` is set in every CI workflow as of [`dd8fbedea8`](https://github.com/appsmithorg/appsmith/commit/dd8fbedea8). Risk is essentially zero in CI. +2. **Field name on the DTO.** Long but semantically accurate. Trivial rename if the team prefers shorter (`baseUrlHealthy`, `baseUrlConfigured`) — single-find-replace plus the TypeScript shape. +3. **EE override path.** The override needs `featureFlagService.check(...)` injected. If we ever introduce other resolver methods that need similar gating, refactor the multi-org branch into a private helper. Not needed for v1. + +## Files explicitly NOT changed + +- `app/client/src/components/editorComponents/ProductAlertBanner.tsx` — unrelated; covers ephemeral product news with per-message dismiss/snooze tracking. Wrong abstraction for "fix-the-config-and-it-stops" misconfig signal. +- The reducer/saga/action layer — no new Redux state because the field is on the existing `currentUser` slice that's already loaded. +- The HTTP layer — no new endpoint; signal rides on `/v1/users/profile` which already runs at session start. +- The Docker image / Dockerfile — must NOT bake in any default `APPSMITH_BASE_URL` value. Production deployments need their own canonical URL. + +## Validation + +1. CE branch builds and unit tests pass locally. +2. Manual smoke (CE): start a fresh container without `APPSMITH_BASE_URL`, log in as super user → banner visible. Set the value via Admin Settings → save → server restart → SPA auto-reload → banner gone, no login redirect. +3. Manual smoke (CE): same flow logged in as non-admin → banner not visible. +4. Manual smoke (EE single-org): same as CE. +5. Manual smoke (EE multi-org): banner not visible regardless of `APPSMITH_BASE_URL` value. +6. Sync test (per AGENTS.md): `git fetch community release` → branch from `community/release` → cherry-pick the CE commits → confirm clean apply (the only EE-specific addition is the resolver override which has no overlap with the CE files). + +## Out of scope + +- Documenting the new `instanceBaseUrlConfigurationHealthy` field in any external API doc — internal DTO field, not part of the public REST contract. +- Translating the banner copy into other locales (Appsmith does not yet have a runtime i18n layer; copy uses the existing `messages.ts` mechanism). +- Server-side enforcement that admins cannot dismiss the banner (the banner is non-dismissible client-side; an attacker mutating their local Redux state to hide it just hides it for themselves, no security impact). From 34c695b72965fef5695aa47ae98320a140eb56d2 Mon Sep 17 00:00:00 2001 From: subrata71 Date: Thu, 30 Apr 2026 01:05:03 +0600 Subject: [PATCH 09/22] =?UTF-8?q?docs(security):=20implementation=20plan?= =?UTF-8?q?=20=E2=80=94=20admin=20base-url=20warning=20banner=20(GHSA-j9gf?= =?UTF-8?q?-vw2f-9hrw)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bite-sized TDD task plan for the admin warning banner companion to the GHSA-j9gf-vw2f-9hrw fail-closed fix. 9 tasks: resolver method (CE), DTO field, UserService wiring, message constants, selector, banner component, AppRouter mount, EE multi-org override, sync test + PR pair. Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw Linear: APP-15046 --- ...026-04-29-admin-base-url-warning-banner.md | 906 ++++++++++++++++++ 1 file changed, 906 insertions(+) create mode 100644 docs/superpowers/plans/2026-04-29-admin-base-url-warning-banner.md diff --git a/docs/superpowers/plans/2026-04-29-admin-base-url-warning-banner.md b/docs/superpowers/plans/2026-04-29-admin-base-url-warning-banner.md new file mode 100644 index 000000000000..3b2f4841e794 --- /dev/null +++ b/docs/superpowers/plans/2026-04-29-admin-base-url-warning-banner.md @@ -0,0 +1,906 @@ +# Admin Base-URL Warning Banner Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Show a non-dismissible top-of-screen banner to instance super-users when `APPSMITH_BASE_URL` is unset and the resolver is in fail-closed mode for token-bearing email flows. Multi-org EE never sees it. + +**Architecture:** New reactive method on `SecureBaseUrlResolverCE` exposes the health signal. New boolean field on `UserProfileCE_DTO` carries it to the client via the existing `/v1/users/profile` call. New React component (`BaseUrlMissingBanner`) renders via `Callout` from `@appsmith/ads`, gated on `isSuperUser && adminSettingsVisible && instanceBaseUrlConfigurationHealthy === false`. EE override of the resolver short-circuits to healthy when `license_multi_org_enabled` is on. + +**Tech Stack:** Java 17 / Spring Boot Reactive (server), React + TypeScript + Redux (client), JUnit 5 + Reactor Test (server tests), Jest + React Testing Library (client tests). + +**Spec:** `docs/superpowers/specs/2026-04-29-admin-base-url-warning-banner-design.md` + +**Branch:** `feat/admin-base-url-warning-banner-ghsa-j9gf` (CE main repo at `/Users/subratadeypappu/IdeaProjects/appsmith`); EE work on the same branch name in EE main repo at `/Users/subratadeypappu/IdeaProjects/ee-appsmith` (created in Task 8). + +--- + +## File Structure + +### Server CE +- **Modify** `app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCE.java` — add interface method +- **Modify** `app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java` — implement method +- **Modify** `app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java` — add 2 unit cases +- **Modify** `app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/UserProfileCE_DTO.java` — add boolean field +- **Modify** `app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java` — wire resolver into `buildUserProfileDTO` + +### Client CE +- **Modify** `app/client/src/ce/constants/messages.ts` — 3 message keys +- **Modify** `app/client/src/selectors/usersSelectors.ts` — new selector with `=== false` guard +- **Create** `app/client/src/selectors/usersSelectors.test.ts` (or extend existing) — selector test +- **Create** `app/client/src/components/editorComponents/BaseUrlMissingBanner.tsx` — banner component +- **Create** `app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx` — Jest tests +- **Modify** `app/client/src/ce/AppRouter.tsx` — mount banner +- **Modify** `app/client/src/constants/userConstants.ts` (if profile type lives there) — extend type + +### Server EE +- **Modify** `app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/SecureBaseUrlResolverImpl.java` — override method +- **Create / Modify** `app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/SecureBaseUrlResolverImplTest.java` — 3 unit cases + +--- + +## Task 1: Server CE — `SecureBaseUrlResolverCE.isBaseUrlConfigurationHealthy()` + +**Files:** +- Modify: `app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCE.java` +- Modify: `app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java` +- Test: `app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java` + +- [ ] **Step 1: Write the failing tests** + +Append two test methods to `SecureBaseUrlResolverCEImplTest`: + +```java +@Test +void isBaseUrlConfigurationHealthy_returnsTrueWhenBaseUrlSet() { + SecureBaseUrlResolverCEImpl resolver = new SecureBaseUrlResolverCEImpl(); + ReflectionTestUtils.setField(resolver, "appsmithBaseUrl", "https://appsmith.example"); + StepVerifier.create(resolver.isBaseUrlConfigurationHealthy()) + .expectNext(true) + .verifyComplete(); +} + +@Test +void isBaseUrlConfigurationHealthy_returnsFalseWhenBaseUrlBlank() { + SecureBaseUrlResolverCEImpl resolver = new SecureBaseUrlResolverCEImpl(); + ReflectionTestUtils.setField(resolver, "appsmithBaseUrl", ""); + StepVerifier.create(resolver.isBaseUrlConfigurationHealthy()) + .expectNext(false) + .verifyComplete(); +} +``` + +(Add `import org.springframework.test.util.ReflectionTestUtils;` and `import reactor.test.StepVerifier;` if not already present.) + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +cd /Users/subratadeypappu/IdeaProjects/appsmith/app/server +./mvnw -pl appsmith-server test -Dtest=SecureBaseUrlResolverCEImplTest -DfailIfNoTests=false +``` + +Expected: FAIL with "cannot find symbol method isBaseUrlConfigurationHealthy()" (compile error). + +- [ ] **Step 3: Add interface method** + +In `SecureBaseUrlResolverCE.java`, add: + +```java +/** + * Reports whether this instance is in a state where token-bearing email flows + * (forgot-password, email verification, invites) can generate links without + * depending on a request-time hint such as the Origin header. False = the + * resolver is in fail-closed mode and the admin should configure the canonical + * base URL. + */ +Mono isBaseUrlConfigurationHealthy(); +``` + +- [ ] **Step 4: Implement in `SecureBaseUrlResolverCEImpl`** + +Add method (paste after `resolveSecureBaseUrl`): + +```java +@Override +public Mono isBaseUrlConfigurationHealthy() { + return Mono.just(StringUtils.hasText(appsmithBaseUrl)); +} +``` + +- [ ] **Step 5: Run tests to verify they pass** + +Same command as Step 2. Expected: 2 new tests PASS, plus the existing 16 cases still PASS. + +- [ ] **Step 6: Commit** + +```bash +cd /Users/subratadeypappu/IdeaProjects/appsmith +git add app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCE.java \ + app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java \ + app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java +git commit -m "feat(security): add isBaseUrlConfigurationHealthy() to SecureBaseUrlResolverCE (GHSA-j9gf-vw2f-9hrw)" +``` + +--- + +## Task 2: Server CE — `UserProfileCE_DTO` field + +**Files:** +- Modify: `app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/UserProfileCE_DTO.java` + +- [ ] **Step 1: Add the field** + +Insert after `@JsonProperty("isIntercomConsentGiven")` block (line 40 area, alongside other instance/admin booleans): + +```java + @JsonProperty("instanceBaseUrlConfigurationHealthy") + boolean instanceBaseUrlConfigurationHealthy = true; +``` + +Default `true` so any DTO instance built outside the real assembler (test fixtures, mocks) doesn't false-positive the banner. + +- [ ] **Step 2: Verify the file compiles** + +```bash +cd /Users/subratadeypappu/IdeaProjects/appsmith/app/server +./mvnw -pl appsmith-server compile -q +``` + +Expected: BUILD SUCCESS. + +- [ ] **Step 3: Commit** + +```bash +git add app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/UserProfileCE_DTO.java +git commit -m "feat(security): add instanceBaseUrlConfigurationHealthy field to UserProfileCE_DTO (GHSA-j9gf-vw2f-9hrw)" +``` + +--- + +## Task 3: Server CE — wire resolver into `UserServiceCEImpl#buildUserProfileDTO` + +**Files:** +- Modify: `app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java` +- Test: `app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java` + +- [ ] **Step 1: Read the existing `buildUserProfileDTO` method** (~line 770) + +Open the file and study the current `Mono` chain. Identify the terminal `.map`/`.flatMap` step where the `UserProfileDTO` is finalised so the new field can be set there. + +- [ ] **Step 2: Write failing integration test** + +Add a new test in `UserServiceTest`: + +```java +@Test +@WithUserDetails("api_user") +void buildUserProfileDTO_includesBaseUrlConfigurationHealthy() { + User user = new User(); + user.setEmail("api_user"); + StepVerifier.create(userService.buildUserProfileDTO(user)) + .assertNext(dto -> { + // In test profile APPSMITH_BASE_URL is unset and the insecure flag is on, + // so the resolver returns false (the field reflects the resolver, not the flag). + // For test stability we just assert the field is present and is a boolean. + Object value = ReflectionTestUtils.getField(dto, "instanceBaseUrlConfigurationHealthy"); + assertThat(value).isInstanceOf(Boolean.class); + }) + .verifyComplete(); +} +``` + +(Imports: `org.springframework.test.util.ReflectionTestUtils`, `org.assertj.core.api.Assertions.assertThat`.) + +- [ ] **Step 3: Run test to verify it fails** + +```bash +./mvnw -pl appsmith-server test -Dtest=UserServiceTest#buildUserProfileDTO_includesBaseUrlConfigurationHealthy -DfailIfNoTests=false +``` + +Expected: FAIL — current DTO build doesn't set the field, default `true` is returned, but the test asserts type only so it'll pass. **Adjust:** change the assertion to `assertThat((Boolean) value).isFalse();` since the resolver in test profile (where `APPSMITH_BASE_URL` is unset) returns `false`. Re-run; should fail because the wiring isn't in place yet (field stays at the DTO default of `true`). + +- [ ] **Step 4: Inject the resolver field** + +In the field declarations of `UserServiceCEImpl` (where other dependencies are listed), add: + +```java +private final SecureBaseUrlResolverCE secureBaseUrlResolver; +``` + +In the constructor, append the parameter (preserving existing parameter order; add at the end if mutating order would risk EE-side conflicts): + +```java +SecureBaseUrlResolverCE secureBaseUrlResolver, +``` + +And in the constructor body: + +```java +this.secureBaseUrlResolver = secureBaseUrlResolver; +``` + +- [ ] **Step 5: Wire into the `buildUserProfileDTO` chain** + +In the terminal `.map` (or `.flatMap`) step that finalises the DTO, replace it with a `.zipWith` so the resolver call runs in parallel. The exact shape depends on the existing method body; pattern: + +```java +return existingMonoChain + .zipWith(secureBaseUrlResolver.isBaseUrlConfigurationHealthy().onErrorReturn(true)) + .map(tuple -> { + UserProfileDTO dto = tuple.getT1(); + dto.setInstanceBaseUrlConfigurationHealthy(tuple.getT2()); + return dto; + }); +``` + +The `.onErrorReturn(true)` defends against resolver Mono errors per the spec's "Error handling" section — a transient failure must not break login. + +- [ ] **Step 6: Run test to verify it passes** + +Same command as Step 3. Expected: PASS. + +- [ ] **Step 7: Run the broader UserServiceTest suite** + +```bash +./mvnw -pl appsmith-server test -Dtest=UserServiceTest -DfailIfNoTests=false +``` + +Expected: All cases PASS (no regression in the existing 30+ cases). + +- [ ] **Step 8: Commit** + +```bash +git add app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java \ + app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java +git commit -m "feat(security): wire SecureBaseUrlResolver into UserServiceCEImpl#buildUserProfileDTO (GHSA-j9gf-vw2f-9hrw)" +``` + +--- + +## Task 4: Client CE — message constants + +**Files:** +- Modify: `app/client/src/ce/constants/messages.ts` + +- [ ] **Step 1: Add three message constants** + +Find a logical section (near other admin/setup messages) and append: + +```ts +export const BASE_URL_MISSING_BANNER_TITLE = () => + "Email delivery is disabled on this instance"; + +export const BASE_URL_MISSING_BANNER_BODY = () => + "Forgot-password, email-verification, and invite emails will not be delivered until APPSMITH_BASE_URL is configured. Set it from Admin Settings to restore email-based flows."; + +export const BASE_URL_MISSING_BANNER_CTA = () => "Configure APPSMITH_BASE_URL"; +``` + +- [ ] **Step 2: Verify file parses (TypeScript)** + +```bash +cd /Users/subratadeypappu/IdeaProjects/appsmith/app/client +yarn tsc --noEmit -p tsconfig.json 2>&1 | grep -E "messages\.ts|error" | head -20 +``` + +Expected: no errors related to `messages.ts`. + +- [ ] **Step 3: Commit** + +```bash +git add app/client/src/ce/constants/messages.ts +git commit -m "feat(client): add admin base-url banner copy constants (GHSA-j9gf-vw2f-9hrw)" +``` + +--- + +## Task 5: Client CE — selector with `=== false` guard + +**Files:** +- Modify: `app/client/src/selectors/usersSelectors.ts` +- Test: `app/client/src/selectors/usersSelectors.test.ts` (create if missing) + +- [ ] **Step 1: Write the failing tests** + +Create or extend `app/client/src/selectors/usersSelectors.test.ts`: + +```ts +import { getShouldShowBaseUrlMissingBanner } from "./usersSelectors"; + +const baseState = (currentUser: object | null) => + ({ ui: { users: { currentUser } } } as never); + +describe("getShouldShowBaseUrlMissingBanner", () => { + it("returns true when super user, settings visible, and unhealthy", () => { + expect( + getShouldShowBaseUrlMissingBanner( + baseState({ + isSuperUser: true, + adminSettingsVisible: true, + instanceBaseUrlConfigurationHealthy: false, + }), + ), + ).toBe(true); + }); + + it("returns false when healthy", () => { + expect( + getShouldShowBaseUrlMissingBanner( + baseState({ + isSuperUser: true, + adminSettingsVisible: true, + instanceBaseUrlConfigurationHealthy: true, + }), + ), + ).toBe(false); + }); + + it("returns false when not super user", () => { + expect( + getShouldShowBaseUrlMissingBanner( + baseState({ + isSuperUser: false, + adminSettingsVisible: true, + instanceBaseUrlConfigurationHealthy: false, + }), + ), + ).toBe(false); + }); + + it("returns false when admin settings not visible", () => { + expect( + getShouldShowBaseUrlMissingBanner( + baseState({ + isSuperUser: true, + adminSettingsVisible: false, + instanceBaseUrlConfigurationHealthy: false, + }), + ), + ).toBe(false); + }); + + it("returns false when field is missing (rolling deploy safety)", () => { + expect( + getShouldShowBaseUrlMissingBanner( + baseState({ + isSuperUser: true, + adminSettingsVisible: true, + }), + ), + ).toBe(false); + }); + + it("returns false when currentUser is null", () => { + expect(getShouldShowBaseUrlMissingBanner(baseState(null))).toBe(false); + }); +}); +``` + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +cd /Users/subratadeypappu/IdeaProjects/appsmith/app/client +yarn jest src/selectors/usersSelectors.test.ts +``` + +Expected: FAIL with "getShouldShowBaseUrlMissingBanner is not a function". + +- [ ] **Step 3: Add the selector** + +Append to `app/client/src/selectors/usersSelectors.ts`: + +```ts +export const getShouldShowBaseUrlMissingBanner = ( + state: AppState, +): boolean => { + const user = state.ui.users.currentUser; + return Boolean( + user?.isSuperUser && + user?.adminSettingsVisible && + user?.instanceBaseUrlConfigurationHealthy === false, + ); +}; +``` + +(Confirm `AppState` is already imported at the top of the file; if not, add `import type { AppState } from "ee/reducers";` matching the existing convention there.) + +- [ ] **Step 4: Run tests to verify they pass** + +Same command as Step 2. Expected: 6/6 PASS. + +- [ ] **Step 5: Extend the user-profile TypeScript shape** + +If the `currentUser` shape is declared (search by ripgrep for `isSuperUser` in `.ts`/`.tsx`), add the optional field: + +```ts +instanceBaseUrlConfigurationHealthy?: boolean; +``` + +If no explicit interface exists (it's inferred), the selector compiles via the `user?.instanceBaseUrlConfigurationHealthy === false` access without further work. + +- [ ] **Step 6: Commit** + +```bash +git add app/client/src/selectors/usersSelectors.ts app/client/src/selectors/usersSelectors.test.ts +# include the type sync file if you found one +git commit -m "feat(client): add getShouldShowBaseUrlMissingBanner selector (GHSA-j9gf-vw2f-9hrw)" +``` + +--- + +## Task 6: Client CE — `BaseUrlMissingBanner` component + +**Files:** +- Create: `app/client/src/components/editorComponents/BaseUrlMissingBanner.tsx` +- Create: `app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx` + +- [ ] **Step 1: Write the failing component tests** + +```tsx +import React from "react"; +import { render, screen } from "@testing-library/react"; +import { Provider } from "react-redux"; +import { createStore } from "redux"; +import { MemoryRouter } from "react-router-dom"; +import { ThemeProvider, lightTheme } from "@appsmith/ads-old"; +import BaseUrlMissingBanner from "./BaseUrlMissingBanner"; + +const renderWithStore = (currentUser: object | null) => { + const store = createStore(() => ({ + ui: { users: { currentUser } }, + })); + return render( + + + + + + + , + ); +}; + +describe("BaseUrlMissingBanner", () => { + it("renders for super user with unhealthy configuration", () => { + renderWithStore({ + isSuperUser: true, + adminSettingsVisible: true, + instanceBaseUrlConfigurationHealthy: false, + }); + expect(screen.getByText(/Email delivery is disabled/i)).toBeInTheDocument(); + }); + + it("does not render when configuration is healthy", () => { + const { container } = renderWithStore({ + isSuperUser: true, + adminSettingsVisible: true, + instanceBaseUrlConfigurationHealthy: true, + }); + expect(container.firstChild).toBeNull(); + }); + + it("does not render for non-super-user", () => { + const { container } = renderWithStore({ + isSuperUser: false, + adminSettingsVisible: true, + instanceBaseUrlConfigurationHealthy: false, + }); + expect(container.firstChild).toBeNull(); + }); + + it("does not render when admin settings hidden", () => { + const { container } = renderWithStore({ + isSuperUser: true, + adminSettingsVisible: false, + instanceBaseUrlConfigurationHealthy: false, + }); + expect(container.firstChild).toBeNull(); + }); + + it("does not render when field missing (rolling deploy)", () => { + const { container } = renderWithStore({ + isSuperUser: true, + adminSettingsVisible: true, + }); + expect(container.firstChild).toBeNull(); + }); +}); +``` + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +yarn jest src/components/editorComponents/BaseUrlMissingBanner.test.tsx +``` + +Expected: FAIL with "Cannot find module './BaseUrlMissingBanner'". + +- [ ] **Step 3: Create the component** + +```tsx +import React from "react"; +import { useSelector } from "react-redux"; +import styled from "styled-components"; +import { Callout } from "@appsmith/ads"; +import { getShouldShowBaseUrlMissingBanner } from "selectors/usersSelectors"; +import { adminSettingsCategoryUrl } from "ee/RouteBuilder"; +import { SettingCategories } from "ee/pages/AdminSettings/config/types"; +import { + BASE_URL_MISSING_BANNER_BODY, + BASE_URL_MISSING_BANNER_CTA, + BASE_URL_MISSING_BANNER_TITLE, + createMessage, +} from "ee/constants/messages"; + +const BannerContainer = styled.div` + position: sticky; + top: 0; + left: 0; + right: 0; + width: 100%; + z-index: var(--ads-v2-z-index-9, 100); +`; + +const BaseUrlMissingBanner: React.FC = () => { + const shouldShow = useSelector(getShouldShowBaseUrlMissingBanner); + + if (!shouldShow) return null; + + return ( + + + {createMessage(BASE_URL_MISSING_BANNER_TITLE)} +
    + {createMessage(BASE_URL_MISSING_BANNER_BODY)} +     +     + ); +}; + +export default BaseUrlMissingBanner; +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Same command as Step 2. Expected: 5/5 PASS. + +If the test fails because of `Callout` requiring additional theme/wiring, inspect the failure and either (a) use `data-testid` selectors instead of text, or (b) wrap the component test in the same provider wrappers used by `ProductAlertBanner.test.tsx` if that file exists; otherwise stick with the wrappers in Step 1. + +- [ ] **Step 5: Commit** + +```bash +git add app/client/src/components/editorComponents/BaseUrlMissingBanner.tsx \ + app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx +git commit -m "feat(client): add BaseUrlMissingBanner component (GHSA-j9gf-vw2f-9hrw)" +``` + +--- + +## Task 7: Client CE — mount banner in `AppRouter` + +**Files:** +- Modify: `app/client/src/ce/AppRouter.tsx` + +- [ ] **Step 1: Find the existing mount of ``** + +Search in `AppRouter.tsx` for `ProductAlertBanner`. The mount sits inside the authed-routes container (around line 195). + +- [ ] **Step 2: Import and mount the new banner above `AppHeader`** + +Add to imports: + +```tsx +import BaseUrlMissingBanner from "components/editorComponents/BaseUrlMissingBanner"; +``` + +In the JSX where `` and `` are rendered (around line 192-195), wrap them so the new banner sits above ``: + +```tsx +<> + + + + + + + +``` + +(Preserve the existing structure exactly — only add the `` line above ``. The fragment may already exist.) + +- [ ] **Step 3: Verify TypeScript compiles** + +```bash +yarn tsc --noEmit -p tsconfig.json 2>&1 | grep "AppRouter.tsx" | head -5 +``` + +Expected: no errors. + +- [ ] **Step 4: Smoke-render the affected file's tests if any** + +```bash +yarn jest src/ce/AppRouter --passWithNoTests +``` + +Expected: PASS or "no tests found". + +- [ ] **Step 5: Commit** + +```bash +git add app/client/src/ce/AppRouter.tsx +git commit -m "feat(client): mount BaseUrlMissingBanner above AppHeader (GHSA-j9gf-vw2f-9hrw)" +``` + +--- + +## Task 8: Server EE — multi-org-aware override + +**Files:** +- Switch repo: `/Users/subratadeypappu/IdeaProjects/ee-appsmith` (main repo, NOT a worktree — husky hook is broken in worktrees per `.security/ghsa-j9gf-vw2f-9hrw/state.md`) +- Modify: `app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/SecureBaseUrlResolverImpl.java` +- Test: `app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/SecureBaseUrlResolverImplTest.java` (create if missing) + +- [ ] **Step 1: Switch EE main repo to a new branch on this work** + +```bash +cd /Users/subratadeypappu/IdeaProjects/ee-appsmith +git stash push -u -m "WIP: parking SAST branch for banner work" +git checkout fix/origin-validation-email-links-ghsa-j9gf # base on the GHSA fix branch which has resolver scaffolding +git pull origin fix/origin-validation-email-links-ghsa-j9gf +git checkout -b feat/admin-base-url-warning-banner-ghsa-j9gf +``` + +- [ ] **Step 2: Cherry-pick the CE-only commits onto the EE branch (shadow PR pattern)** + +For each CE commit on `feat/admin-base-url-warning-banner-ghsa-j9gf` (Tasks 1-7), cherry-pick into EE: + +```bash +git -C /Users/subratadeypappu/IdeaProjects/ee-appsmith cherry-pick \ + $(git -C /Users/subratadeypappu/IdeaProjects/appsmith log --reverse --format=%H release..feat/admin-base-url-warning-banner-ghsa-j9gf -- app/server app/client) +``` + +If conflicts arise, resolve manually (most likely on `UserProfileCE_DTO.java` if EE has any divergent fields). For the spec/plan docs, skip them on the EE side — they only need to live in CE. + +- [ ] **Step 3: Write failing EE override tests** + +Create `app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/SecureBaseUrlResolverImplTest.java`: + +```java +package com.appsmith.server.helpers; + +import com.appsmith.external.enums.FeatureFlagEnum; +import com.appsmith.server.services.FeatureFlagService; +import com.appsmith.server.services.OrganizationService; +import org.junit.jupiter.api.Test; +import org.springframework.test.util.ReflectionTestUtils; +import reactor.core.publisher.Mono; +import reactor.test.StepVerifier; + +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +class SecureBaseUrlResolverImplTest { + + @Test + void isBaseUrlConfigurationHealthy_returnsTrueWhenMultiOrgEnabled() { + FeatureFlagService featureFlagService = mock(FeatureFlagService.class); + OrganizationService organizationService = mock(OrganizationService.class); + when(featureFlagService.check(eq(FeatureFlagEnum.license_multi_org_enabled))) + .thenReturn(Mono.just(true)); + + SecureBaseUrlResolverImpl resolver = + new SecureBaseUrlResolverImpl(featureFlagService, organizationService); + ReflectionTestUtils.setField(resolver, "appsmithBaseUrl", ""); + + StepVerifier.create(resolver.isBaseUrlConfigurationHealthy()) + .expectNext(true) + .verifyComplete(); + } + + @Test + void isBaseUrlConfigurationHealthy_delegatesWhenMultiOrgDisabledAndUrlSet() { + FeatureFlagService featureFlagService = mock(FeatureFlagService.class); + OrganizationService organizationService = mock(OrganizationService.class); + when(featureFlagService.check(eq(FeatureFlagEnum.license_multi_org_enabled))) + .thenReturn(Mono.just(false)); + + SecureBaseUrlResolverImpl resolver = + new SecureBaseUrlResolverImpl(featureFlagService, organizationService); + ReflectionTestUtils.setField(resolver, "appsmithBaseUrl", "https://appsmith.example"); + + StepVerifier.create(resolver.isBaseUrlConfigurationHealthy()) + .expectNext(true) + .verifyComplete(); + } + + @Test + void isBaseUrlConfigurationHealthy_delegatesWhenMultiOrgDisabledAndUrlUnset() { + FeatureFlagService featureFlagService = mock(FeatureFlagService.class); + OrganizationService organizationService = mock(OrganizationService.class); + when(featureFlagService.check(eq(FeatureFlagEnum.license_multi_org_enabled))) + .thenReturn(Mono.just(false)); + + SecureBaseUrlResolverImpl resolver = + new SecureBaseUrlResolverImpl(featureFlagService, organizationService); + ReflectionTestUtils.setField(resolver, "appsmithBaseUrl", ""); + + StepVerifier.create(resolver.isBaseUrlConfigurationHealthy()) + .expectNext(false) + .verifyComplete(); + } +} +``` + +- [ ] **Step 4: Run tests to verify they fail** + +```bash +cd /Users/subratadeypappu/IdeaProjects/ee-appsmith/app/server +./mvnw -pl appsmith-server test -Dtest=SecureBaseUrlResolverImplTest -DfailIfNoTests=false +``` + +Expected: FAIL — the override hasn't been added yet, so the EE class inherits the CE method which ignores multi-org. Test 1 fails (returns false instead of true). + +- [ ] **Step 5: Add the override to `SecureBaseUrlResolverImpl`** + +Append the new method (after `resolveSecureBaseUrl`): + +```java +@Override +public Mono isBaseUrlConfigurationHealthy() { + return featureFlagService + .check(FeatureFlagEnum.license_multi_org_enabled) + .flatMap(isMultiOrgEnabled -> Boolean.TRUE.equals(isMultiOrgEnabled) + ? Mono.just(true) + : super.isBaseUrlConfigurationHealthy()) + .onErrorResume(e -> { + log.warn( + "FeatureFlagService.check failed for license_multi_org_enabled; defaulting to single-org behavior: {}", + e.getMessage()); + return super.isBaseUrlConfigurationHealthy(); + }); +} +``` + +- [ ] **Step 6: Run tests to verify they pass** + +Same command as Step 4. Expected: 3/3 PASS. + +- [ ] **Step 7: Commit on the EE branch** + +```bash +cd /Users/subratadeypappu/IdeaProjects/ee-appsmith +git add app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/SecureBaseUrlResolverImpl.java \ + app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/SecureBaseUrlResolverImplTest.java +git commit -m "feat(security): EE multi-org override for isBaseUrlConfigurationHealthy (GHSA-j9gf-vw2f-9hrw)" +``` + +- [ ] **Step 8: Push EE branch** + +```bash +git push -u origin feat/admin-base-url-warning-banner-ghsa-j9gf +``` + +--- + +## Task 9: CE — sync test, push, open PRs + +- [ ] **Step 1: Verify CE→EE sync would apply cleanly (per AGENTS.md)** + +```bash +cd /Users/subratadeypappu/IdeaProjects/appsmith +git fetch community release +git checkout -b _sync-test-banner community/release +for sha in $(git log --reverse --format=%H release..feat/admin-base-url-warning-banner-ghsa-j9gf); do + git cherry-pick $sha || { echo "CONFLICT on $sha"; break; } +done +git checkout feat/admin-base-url-warning-banner-ghsa-j9gf +git branch -D _sync-test-banner +``` + +Expected: All commits cherry-pick cleanly. Any conflict means a CE file diverges from `community/release` — investigate and resolve before opening the PR. + +- [ ] **Step 2: Push CE branch** + +```bash +git push -u origin feat/admin-base-url-warning-banner-ghsa-j9gf +``` + +- [ ] **Step 3: Open CE PR** + +```bash +gh pr create --repo appsmithorg/appsmith \ + --base release \ + --title "feat(security): admin warning banner for unset APPSMITH_BASE_URL (GHSA-j9gf-vw2f-9hrw)" \ + --body "$(cat <<'EOF' +## Summary + +Companion to PR #41766 (the GHSA-j9gf-vw2f-9hrw fail-closed fix). + +Adds a non-dismissible top-of-screen banner shown only to instance super-users when `APPSMITH_BASE_URL` is unset and the resolver is therefore in fail-closed mode for token-bearing email flows. Multi-org EE deployments (e.g. Appsmith Cloud) **never** see the banner — verified via the `license_multi_org_enabled` short-circuit in the EE resolver override (shadow EE PR linked below). + +The banner deep-links to Admin Settings → Configuration where `APPSMITH_BASE_URL` is the registered field. Saving via the existing Configuration-tier admin-settings flow restarts the server, the SPA auto-reloads, the resolver re-reads the env, and the banner clears — no re-login. + +## Design + +`docs/superpowers/specs/2026-04-29-admin-base-url-warning-banner-design.md` + +## Plan + +`docs/superpowers/plans/2026-04-29-admin-base-url-warning-banner.md` + +## Test plan + +- [x] `SecureBaseUrlResolverCEImplTest` — 2 new cases for `isBaseUrlConfigurationHealthy()` +- [x] `UserServiceTest` — 1 new case asserting field is wired into `buildUserProfileDTO` +- [x] `usersSelectors.test.ts` — 6 cases including rolling-deploy `=== false` guard +- [x] `BaseUrlMissingBanner.test.tsx` — 5 cases covering all gating dimensions +- Cypress: skipped — CI sets `APPSMITH_BASE_URL=http://localhost`, banner never fires by construction + +## Companion PRs / refs + +- Shadow EE PR: +- Security advisory: https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw +- Linear: APP-15046 +EOF +)" +``` + +- [ ] **Step 4: Open shadow EE PR** + +```bash +cd /Users/subratadeypappu/IdeaProjects/ee-appsmith +gh pr create --repo appsmithorg/appsmith-ee \ + --base release \ + --title "feat(security): admin warning banner for unset APPSMITH_BASE_URL (GHSA-j9gf-vw2f-9hrw)" \ + --body "$(cat <<'EOF' +## Summary + +Shadow EE PR for CE PR #. Mirrors the CE work and adds the EE-only `SecureBaseUrlResolverImpl` override for `isBaseUrlConfigurationHealthy()` that short-circuits to `Mono.just(true)` when `license_multi_org_enabled` is on (so multi-org EE deployments — Appsmith Cloud — never display the banner). + +## Merge order + +1. CE PR merges first +2. CE→EE hourly sync brings the CE files into EE (cleanly — sync test passed before opening the CE PR) +3. This shadow EE PR merges on top with a merge commit (NOT squash — squashing breaks the next sync) + +## Companion PRs / refs + +- CE PR: +- Security advisory: https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw +- Linear: APP-15046 +EOF +)" +``` + +- [ ] **Step 5: Add shared labels** + +```bash +gh pr edit --repo appsmithorg/appsmith --add-label security +gh pr edit --repo appsmithorg/appsmith-ee --add-label security +``` + +- [ ] **Step 6: Restore EE main repo to its prior state** + +```bash +cd /Users/subratadeypappu/IdeaProjects/ee-appsmith +git checkout feature/app-15191-sast-semgrep-integration +git stash pop +``` + +--- + +## Self-Review Notes + +- Spec coverage: every section in the spec ("Architecture", "Multi-org", "Error handling", "Edge cases", "Testing", "Out of scope") is anchored to a specific task above (1-9). +- No placeholders: every code block contains executable code or executable commands. PR-body URLs are deliberately marked `` because they're computed at PR-open time. +- Type consistency: `instanceBaseUrlConfigurationHealthy` (Java + TS), `isBaseUrlConfigurationHealthy()` (Java method), `getShouldShowBaseUrlMissingBanner` (TS selector), `BaseUrlMissingBanner` (component) — all consistent across tasks. From 57cf9aef784b622581a1d98c84c83a866c05bfb3 Mon Sep 17 00:00:00 2001 From: subrata71 Date: Thu, 30 Apr 2026 01:07:21 +0600 Subject: [PATCH 10/22] feat(security): add isBaseUrlConfigurationHealthy() to SecureBaseUrlResolverCE (GHSA-j9gf-vw2f-9hrw) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New reactive method that reports whether the instance can generate token-bearing email links without depending on a request-time hint. CE returns true iff APPSMITH_BASE_URL is set. EE override (separate commit) returns true unconditionally when license_multi_org_enabled is on. Drives the in-product admin warning banner shown to instance super-users when the resolver is in fail-closed mode. The insecure-flag fallback intentionally does NOT mark the instance as healthy — operators on the deprecated escape hatch should still see the warning. Tests: 2 new cases in SecureBaseUrlResolverCEImplTest (true when set, false when blank), all 16 existing cases still pass. Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw --- .../helpers/ce/SecureBaseUrlResolverCE.java | 19 +++++++++++++ .../ce/SecureBaseUrlResolverCEImpl.java | 5 ++++ .../ce/SecureBaseUrlResolverCEImplTest.java | 27 +++++++++++++++++++ 3 files changed, 51 insertions(+) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCE.java index 97bba9f8e111..0adc4bb452bc 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCE.java @@ -33,4 +33,23 @@ public interface SecureBaseUrlResolverCE { * does not match (strict-mode enforcement, preserved from prior hardening). */ Mono resolveSecureBaseUrl(String providedBaseUrl); + + /** + * Reports whether this instance is in a state where token-bearing email flows + * (forgot-password, email verification, invites) can generate links without + * depending on a request-time hint such as the {@code Origin} header. + * + *

    The CE implementation returns {@code true} iff {@code APPSMITH_BASE_URL} is set; + * the EE override returns {@code true} unconditionally when the multi-org feature flag + * is on (each organization derives its own canonical URL from slug + deploymentDomain). + * + *

    Drives the in-product admin warning banner shown to instance super-users. The + * insecure-flag fallback intentionally does NOT mark the instance as healthy — operators + * who opted into the deprecated {@code APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS} escape + * hatch should still see the warning so the deprecation pressure is preserved. + * + * @return {@code Mono} emitting {@code true} when no banner should be shown, + * {@code false} when the banner should warn that token-bearing emails are disabled. + */ + Mono isBaseUrlConfigurationHealthy(); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java index aadb34f8f3f0..dbfc7cc5061b 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java @@ -82,6 +82,11 @@ public Mono resolveSecureBaseUrl(String providedBaseUrl) { return Mono.empty(); } + @Override + public Mono isBaseUrlConfigurationHealthy() { + return Mono.just(StringUtils.hasText(appsmithBaseUrl)); + } + /** * Compares two URLs by their origin (scheme + host + effective port) per RFC 6454, rather than * by raw string equality. Tolerates insignificant differences such as trailing slashes and diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java index 228b2a551398..a0e89b27fd70 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java @@ -205,4 +205,31 @@ void resolveSecureBaseUrl_whenAttackerUsesUserinfoTrick_errors() throws Exceptio } // endregion + + // region isBaseUrlConfigurationHealthy — instance-config health signal driving the admin + // warning banner. The signal answers "can this instance generate token-bearing email links + // without depending on a request-time hint?". CE semantics: true iff APPSMITH_BASE_URL is + // set. The insecure-flag fallback intentionally does NOT mark the instance as healthy — + // operators who opted into the deprecated escape hatch should still see the warning so the + // deprecation pressure is preserved. + + @Test + void isBaseUrlConfigurationHealthy_returnsTrueWhenBaseUrlSet() throws Exception { + SecureBaseUrlResolverCEImpl resolver = newResolver("https://appsmith.example", false); + + StepVerifier.create(resolver.isBaseUrlConfigurationHealthy()) + .expectNext(true) + .verifyComplete(); + } + + @Test + void isBaseUrlConfigurationHealthy_returnsFalseWhenBaseUrlBlank() throws Exception { + SecureBaseUrlResolverCEImpl resolver = newResolver("", false); + + StepVerifier.create(resolver.isBaseUrlConfigurationHealthy()) + .expectNext(false) + .verifyComplete(); + } + + // endregion } From 9cd222691d14e05a7b272d256837100c7a3089ce Mon Sep 17 00:00:00 2001 From: subrata71 Date: Thu, 30 Apr 2026 01:07:46 +0600 Subject: [PATCH 11/22] feat(security): add instanceBaseUrlConfigurationHealthy field to UserProfileCE_DTO (GHSA-j9gf-vw2f-9hrw) New boolean field carried via the existing /v1/users/profile call. Default true so DTOs built outside the real assembler (test fixtures, mocks) do not false-positive the admin warning banner. The wiring to populate it from the resolver lands in the next commit. Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw --- .../com/appsmith/server/dtos/ce/UserProfileCE_DTO.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/UserProfileCE_DTO.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/UserProfileCE_DTO.java index cdfe7399cd6e..31cf45bfd6af 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/UserProfileCE_DTO.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/UserProfileCE_DTO.java @@ -39,6 +39,16 @@ public class UserProfileCE_DTO { @JsonProperty("isIntercomConsentGiven") boolean isIntercomConsentGiven = false; + /** + * Health signal driving the admin warning banner introduced for + * GHSA-j9gf-vw2f-9hrw. + * Reflects the answer of {@code SecureBaseUrlResolverCE#isBaseUrlConfigurationHealthy()}. + * Default {@code true} so any DTO instance built outside the real assembler (test fixtures, + * mocks) does not false-positive the banner. + */ + @JsonProperty("instanceBaseUrlConfigurationHealthy") + boolean instanceBaseUrlConfigurationHealthy = true; + String photoId; String useCase; From afce3575ddf3531e44f8f5536249906d9b7ea3b4 Mon Sep 17 00:00:00 2001 From: subrata71 Date: Thu, 30 Apr 2026 01:15:50 +0600 Subject: [PATCH 12/22] feat(security): wire SecureBaseUrlResolver into UserServiceCEImpl#buildUserProfileDTO (GHSA-j9gf-vw2f-9hrw) Folds isBaseUrlConfigurationHealthy() into the existing parallel Mono.zip in buildUserProfileDTO so the health signal is computed alongside the other profile-build calls (no sequential cost). Result populates the new instanceBaseUrlConfigurationHealthy field on the assembled UserProfileDTO, which the React client uses to gate the admin warning banner. The resolver call is wrapped with .onErrorReturn(true) so a transient resolver/feature-flag failure produces a false-negative banner (admin sees no warning) rather than breaking /v1/users/profile entirely. False-negative is recoverable on next session; broken login is not. Tests: 1 new case in UserServiceTest pinning the field is present and reflects the resolver's answer (false in test profile where APPSMITH_BASE_URL is unset). 3/3 buildUserProfileDTO-related cases pass locally. Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw --- .../server/services/ce/UserServiceCEImpl.java | 13 ++++++++++++- .../server/services/UserServiceTest.java | 19 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java index af8436841dcb..3fd02910c86b 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java @@ -775,16 +775,26 @@ public Mono buildUserProfileDTO(User user) { Mono isSuperUserMono = userFromDbMono.flatMap(userUtils::isSuperUser).defaultIfEmpty(false); + // GHSA-j9gf-vw2f-9hrw — surface the resolver health signal on the profile so the + // client can render the admin warning banner when the instance is in fail-closed mode + // for token-bearing email flows. .onErrorReturn(true) defends against a transient + // resolver/feature-flag failure breaking login (false-negative banner is preferable to + // a broken /v1/users/profile call). + Mono isBaseUrlConfigurationHealthyMono = + secureBaseUrlResolver.isBaseUrlConfigurationHealthy().onErrorReturn(true); + return Mono.zip( isUsersEmpty(), userFromDbMono, userDataService.getForCurrentUser().defaultIfEmpty(new UserData()), - isSuperUserMono) + isSuperUserMono, + isBaseUrlConfigurationHealthyMono) .flatMap(tuple -> { final boolean isUsersEmpty = Boolean.TRUE.equals(tuple.getT1()); final User userFromDb = tuple.getT2(); final UserData userData = tuple.getT3(); Boolean isSuperUser = tuple.getT4(); + Boolean isBaseUrlConfigurationHealthy = tuple.getT5(); final UserProfileDTO profile = new UserProfileDTO(); @@ -804,6 +814,7 @@ public Mono buildUserProfileDTO(User user) { commonConfig.getIsCloudHosting() ? true : userData.getIsIntercomConsentGiven()); profile.setIsSuperUser(isSuperUser); profile.setIsConfigurable(!StringUtils.isEmpty(commonConfig.getEnvFilePath())); + profile.setInstanceBaseUrlConfigurationHealthy(isBaseUrlConfigurationHealthy); return pacConfigurationService.setRolesAndGroups(profile, userFromDb, true); }); } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java index a2203ae5d4a6..7e39bcdfb427 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java @@ -721,6 +721,25 @@ public void buildUserProfileDTO_WhenAnonymousUser_ReturnsProfile() { .verifyComplete(); } + /** + * GHSA-j9gf-vw2f-9hrw — pins that the {@code SecureBaseUrlResolver#isBaseUrlConfigurationHealthy()} + * signal flows through the DTO assembler. In the test profile {@code APPSMITH_BASE_URL} is unset, + * so the resolver returns {@code false} regardless of the {@code APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS} + * flag (the health signal answers "is base URL configured?", not "are emails currently being sent?"). + */ + @Test + @WithUserDetails(value = "api_user") + public void buildUserProfileDTO_includesBaseUrlConfigurationHealthy() { + StepVerifier.create(sessionUserService.getCurrentUser().flatMap(userService::buildUserProfileDTO)) + .assertNext(userProfileDTO -> { + assertThat(userProfileDTO.getInstanceBaseUrlConfigurationHealthy()) + .as("In test profile APPSMITH_BASE_URL is unset, so the resolver-derived " + + "health signal must be false on the assembled DTO") + .isFalse(); + }) + .verifyComplete(); + } + /** * This test case asserts that on every user creation, User Management role is auto-created and associated with that user. */ From 2881a8e89887e90309adedd07b43fac005bb5b58 Mon Sep 17 00:00:00 2001 From: subrata71 Date: Thu, 30 Apr 2026 01:23:51 +0600 Subject: [PATCH 13/22] feat(client): add base-url banner copy + selector + User type field (GHSA-j9gf-vw2f-9hrw) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small client-side pieces: - messages.ts: title, body, CTA constants for the new admin warning banner - usersSelectors.tsx: getShouldShowBaseUrlMissingBanner — gates on isSuperUser && adminSettingsVisible && instanceBaseUrlConfigurationHealthy === false. The explicit === false (rather than !value) is the rolling-deploy guard: undefined field on an older server response keeps the banner hidden. - userConstants.ts: extend User interface with optional instanceBaseUrlConfigurationHealthy?: boolean. Tests: 6 new selector cases pinning all gating dimensions plus the rolling-deploy safety. All pass. lint-staged verified clean (eslint + gitleaks) when run from app/client; commit uses --no-verify because the project's hook invokes lint-staged from the repo root with --cwd which breaks eslint binary resolution in a way the state.md flagged as a known local-env quirk. Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw --- app/client/src/ce/constants/messages.ts | 8 +++ app/client/src/constants/userConstants.ts | 3 + .../src/selectors/usersSelectors.test.ts | 72 +++++++++++++++++++ app/client/src/selectors/usersSelectors.tsx | 23 ++++++ 4 files changed, 106 insertions(+) create mode 100644 app/client/src/selectors/usersSelectors.test.ts diff --git a/app/client/src/ce/constants/messages.ts b/app/client/src/ce/constants/messages.ts index 9a9640abeb18..beab3c4fc2f1 100644 --- a/app/client/src/ce/constants/messages.ts +++ b/app/client/src/ce/constants/messages.ts @@ -1015,6 +1015,14 @@ export const READ_DOCUMENTATION = () => "Read documentation"; export const LEARN_MORE = () => "Learn more"; export const I_UNDERSTAND = () => "I understand"; + +// Admin warning banner — shown when APPSMITH_BASE_URL is unset and the resolver is in +// fail-closed mode for token-bearing email flows. See GHSA-j9gf-vw2f-9hrw. +export const BASE_URL_MISSING_BANNER_TITLE = () => + "Email delivery is disabled on this instance"; +export const BASE_URL_MISSING_BANNER_BODY = () => + "Forgot-password, email-verification, and invite emails will not be delivered until APPSMITH_BASE_URL is configured. Set it from Admin Settings to restore email-based flows."; +export const BASE_URL_MISSING_BANNER_CTA = () => "Configure APPSMITH_BASE_URL"; export const GIT_NO_UPDATED_TOOLTIP = () => "No new updates to push"; export const FIND_OR_CREATE_A_BRANCH = () => "Find or create a branch"; diff --git a/app/client/src/constants/userConstants.ts b/app/client/src/constants/userConstants.ts index 871d44e097ee..ba72be3abc1a 100644 --- a/app/client/src/constants/userConstants.ts +++ b/app/client/src/constants/userConstants.ts @@ -20,6 +20,9 @@ export interface User { isAnonymous?: boolean; isIntercomConsentGiven?: boolean; emailVerified: boolean; + // GHSA-j9gf-vw2f-9hrw — admin warning banner signal. Optional: older servers + // pre-rolling-deploy may not include the field, in which case the banner stays hidden. + instanceBaseUrlConfigurationHealthy?: boolean; } export interface UserApplication { diff --git a/app/client/src/selectors/usersSelectors.test.ts b/app/client/src/selectors/usersSelectors.test.ts new file mode 100644 index 000000000000..15543df53cf7 --- /dev/null +++ b/app/client/src/selectors/usersSelectors.test.ts @@ -0,0 +1,72 @@ +import { getShouldShowBaseUrlMissingBanner } from "./usersSelectors"; + +// Minimal Redux state shape; the selector only reads ui.users.currentUser. +const stateWith = (currentUser: object | null) => + ({ ui: { users: { currentUser } } }) as never; + +describe("getShouldShowBaseUrlMissingBanner — GHSA-j9gf-vw2f-9hrw", () => { + it("returns true for super user with admin settings visible and unhealthy config", () => { + expect( + getShouldShowBaseUrlMissingBanner( + stateWith({ + isSuperUser: true, + adminSettingsVisible: true, + instanceBaseUrlConfigurationHealthy: false, + }), + ), + ).toBe(true); + }); + + it("returns false when configuration is healthy", () => { + expect( + getShouldShowBaseUrlMissingBanner( + stateWith({ + isSuperUser: true, + adminSettingsVisible: true, + instanceBaseUrlConfigurationHealthy: true, + }), + ), + ).toBe(false); + }); + + it("returns false for non-super-user", () => { + expect( + getShouldShowBaseUrlMissingBanner( + stateWith({ + isSuperUser: false, + adminSettingsVisible: true, + instanceBaseUrlConfigurationHealthy: false, + }), + ), + ).toBe(false); + }); + + it("returns false when admin settings hidden (RBAC / license guard)", () => { + expect( + getShouldShowBaseUrlMissingBanner( + stateWith({ + isSuperUser: true, + adminSettingsVisible: false, + instanceBaseUrlConfigurationHealthy: false, + }), + ), + ).toBe(false); + }); + + // Rolling-deploy safety: newer client paired briefly with older server. + // Field absent => banner must stay hidden, not flip on as a false positive. + it("returns false when health field is missing (rolling deploy)", () => { + expect( + getShouldShowBaseUrlMissingBanner( + stateWith({ + isSuperUser: true, + adminSettingsVisible: true, + }), + ), + ).toBe(false); + }); + + it("returns false when currentUser is null", () => { + expect(getShouldShowBaseUrlMissingBanner(stateWith(null))).toBe(false); + }); +}); diff --git a/app/client/src/selectors/usersSelectors.tsx b/app/client/src/selectors/usersSelectors.tsx index 00a36332e49b..53054f76251a 100644 --- a/app/client/src/selectors/usersSelectors.tsx +++ b/app/client/src/selectors/usersSelectors.tsx @@ -27,3 +27,26 @@ export const getFeatureFlagsFetching = (state: DefaultRootState) => export const getIsUserLoggedIn = (state: DefaultRootState): boolean => state.ui.users.currentUser?.email !== ANONYMOUS_USERNAME; + +/** + * GHSA-j9gf-vw2f-9hrw — admin warning banner gate. Returns true only when ALL of: + * - the current user is an instance super user, + * - admin settings are visible to them (RBAC / license tier guard), + * - and the server explicitly reports `instanceBaseUrlConfigurationHealthy === false`. + * + * The explicit `=== false` (rather than `!value`) is deliberate: during a rolling + * deploy where a newer client briefly sees an older server's response without the + * field, `undefined === false` is `false`, so the banner stays hidden until both + * sides are deployed. + */ +export const getShouldShowBaseUrlMissingBanner = ( + state: DefaultRootState, +): boolean => { + const user = state.ui?.users?.currentUser; + + return Boolean( + user?.isSuperUser && + user?.adminSettingsVisible && + user?.instanceBaseUrlConfigurationHealthy === false, + ); +}; From 58e87054bc7d4a01516fa2600df7ff1e50da2324 Mon Sep 17 00:00:00 2001 From: subrata71 Date: Thu, 30 Apr 2026 01:26:13 +0600 Subject: [PATCH 14/22] feat(client): add BaseUrlMissingBanner component (GHSA-j9gf-vw2f-9hrw) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New React component rendering the admin warning banner via the design-system Callout (kind="warning"). Subscribes to getShouldShowBaseUrlMissingBanner; returns null otherwise so non-admin users pay zero render cost. CTA deep-links to Admin Settings → Configuration via the existing adminSettingsCategoryUrl helper. Not user-dismissible — banner reflects live server state and clears on the next profile fetch after admin sets APPSMITH_BASE_URL (which triggers the existing Configuration-tier server-restart + SPA-reload flow). Tests: 5 Jest cases covering all gating dimensions plus the rolling-deploy guard (5/5 pass). Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw --- .../BaseUrlMissingBanner.test.tsx | 83 +++++++++++++++++++ .../editorComponents/BaseUrlMissingBanner.tsx | 57 +++++++++++++ 2 files changed, 140 insertions(+) create mode 100644 app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx create mode 100644 app/client/src/components/editorComponents/BaseUrlMissingBanner.tsx diff --git a/app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx b/app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx new file mode 100644 index 000000000000..61c511fa33a5 --- /dev/null +++ b/app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx @@ -0,0 +1,83 @@ +import React from "react"; +import { render, screen } from "@testing-library/react"; +import "@testing-library/jest-dom"; +import { Provider } from "react-redux"; +import configureStore from "redux-mock-store"; +import { ThemeProvider } from "styled-components"; +import { BrowserRouter as Router } from "react-router-dom"; +import { lightTheme } from "selectors/themeSelectors"; +import BaseUrlMissingBanner from "./BaseUrlMissingBanner"; + +const mockStore = configureStore([]); + +const storeWith = (currentUser: object | null) => + mockStore({ ui: { users: { currentUser } } }); + +const renderBanner = (currentUser: object | null) => + render( + + + + + + + , + ); + +describe("BaseUrlMissingBanner — GHSA-j9gf-vw2f-9hrw", () => { + it("renders for super-user with admin settings visible and unhealthy config", () => { + renderBanner({ + isSuperUser: true, + adminSettingsVisible: true, + instanceBaseUrlConfigurationHealthy: false, + }); + + expect( + screen.getByTestId("t--base-url-missing-banner"), + ).toBeInTheDocument(); + expect( + screen.getByText(/Email delivery is disabled on this instance/i), + ).toBeInTheDocument(); + }); + + it("does not render when configuration is healthy", () => { + const { container } = renderBanner({ + isSuperUser: true, + adminSettingsVisible: true, + instanceBaseUrlConfigurationHealthy: true, + }); + + expect(container.firstChild).toBeNull(); + }); + + it("does not render for non-super-user", () => { + const { container } = renderBanner({ + isSuperUser: false, + adminSettingsVisible: true, + instanceBaseUrlConfigurationHealthy: false, + }); + + expect(container.firstChild).toBeNull(); + }); + + it("does not render when admin settings hidden (RBAC / license guard)", () => { + const { container } = renderBanner({ + isSuperUser: true, + adminSettingsVisible: false, + instanceBaseUrlConfigurationHealthy: false, + }); + + expect(container.firstChild).toBeNull(); + }); + + // Rolling-deploy safety: newer client briefly paired with older server has the + // health field absent. Banner must stay hidden until both sides are deployed. + it("does not render when health field is missing (rolling deploy)", () => { + const { container } = renderBanner({ + isSuperUser: true, + adminSettingsVisible: true, + }); + + expect(container.firstChild).toBeNull(); + }); +}); diff --git a/app/client/src/components/editorComponents/BaseUrlMissingBanner.tsx b/app/client/src/components/editorComponents/BaseUrlMissingBanner.tsx new file mode 100644 index 000000000000..a5e366b204f3 --- /dev/null +++ b/app/client/src/components/editorComponents/BaseUrlMissingBanner.tsx @@ -0,0 +1,57 @@ +import React from "react"; +import { useSelector } from "react-redux"; +import styled from "styled-components"; +import { Callout } from "@appsmith/ads"; +import { getShouldShowBaseUrlMissingBanner } from "selectors/usersSelectors"; +import { adminSettingsCategoryUrl } from "ee/RouteBuilder"; +import { SettingCategories } from "ee/pages/AdminSettings/config/types"; +import { + BASE_URL_MISSING_BANNER_BODY, + BASE_URL_MISSING_BANNER_CTA, + BASE_URL_MISSING_BANNER_TITLE, + createMessage, +} from "ee/constants/messages"; + +/** + * GHSA-j9gf-vw2f-9hrw — admin warning banner shown to instance super-users when + * the server's SecureBaseUrlResolver reports that APPSMITH_BASE_URL is unset and + * token-bearing email flows are therefore disabled. + * + * Positioned at the very top of the application (rendered above AppHeader in + * AppRouter), in normal document flow so the rest of the layout pushes down by + * the banner's height. Not user-dismissible — the banner reflects live server + * state and clears when the admin sets APPSMITH_BASE_URL via Admin Settings, + * which triggers the existing Configuration-tier server-restart + SPA-reload + * flow. + */ +const BannerContainer = styled.div` + width: 100%; +`; + +const BaseUrlMissingBanner: React.FC = () => { + const shouldShow = useSelector(getShouldShowBaseUrlMissingBanner); + + if (!shouldShow) return null; + + return ( + + + {createMessage(BASE_URL_MISSING_BANNER_TITLE)} +
    + {createMessage(BASE_URL_MISSING_BANNER_BODY)} +     +     + ); +}; + +export default BaseUrlMissingBanner; From 46f8118d975cc5e53d444f195e719ff4f23035f0 Mon Sep 17 00:00:00 2001 From: subrata71 Date: Thu, 30 Apr 2026 01:27:16 +0600 Subject: [PATCH 15/22] feat(client): mount BaseUrlMissingBanner above AppHeader in CE AppRouter (GHSA-j9gf-vw2f-9hrw) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Renders the new admin warning banner at the very top of the authed-routes container so it sits above AppHeader in normal document flow — pushes everything below down by its own height when visible. Returns null for non-admin users and healthy instances, so non-target users pay zero render cost. The existing ProductAlertBanner (bottom-of-screen) stays exactly where it was; the two coexist with no overlap. EE: ee/AppRouter.tsx already inherits/extends ce/AppRouter.tsx, so the mount flows through automatically — no EE-side change needed. Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw --- app/client/src/ce/AppRouter.tsx | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/client/src/ce/AppRouter.tsx b/app/client/src/ce/AppRouter.tsx index 43e9799a44aa..dfb4a03c6945 100644 --- a/app/client/src/ce/AppRouter.tsx +++ b/app/client/src/ce/AppRouter.tsx @@ -57,6 +57,7 @@ import RouteChangeListener from "RouteChangeListener"; import { initCurrentPage } from "../actions/initActions"; import Walkthrough from "components/featureWalkthrough"; import ProductAlertBanner from "components/editorComponents/ProductAlertBanner"; +import BaseUrlMissingBanner from "components/editorComponents/BaseUrlMissingBanner"; import { getAdminSettingsPath } from "ee/utils/BusinessFeatures/adminSettingsHelpers"; import { useFeatureFlag } from "utils/hooks/useFeatureFlag"; import { FEATURE_FLAG } from "ee/entities/FeatureFlag"; @@ -188,6 +189,11 @@ export default function AppRouter() { ) : ( <> + {/* GHSA-j9gf-vw2f-9hrw — admin warning banner sits at the very top of the + render tree so it pushes AppHeader and the rest of the layout down by its + own height. Returns null for non-admins / healthy instances, so non-target + users pay zero render cost. */} + From 6bfcea7ac2d363edac7f160ea53c23dcd8403bdf Mon Sep 17 00:00:00 2001 From: subrata71 Date: Thu, 30 Apr 2026 13:59:46 +0600 Subject: [PATCH 16/22] refactor(security): move instanceBaseUrlConfigurationHealthy from UserProfile to OrganizationConfiguration (GHSA-j9gf-vw2f-9hrw) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The admin-banner health signal was originally placed on UserProfileCE_DTO but that's the wrong bucket: it's instance-state, not user-state, and /v1/users/me is rarely re-fetched on dashboard routes. The canonical bootstrap call is /v1/consolidated-api which always carries organizationConfig — that's the right home for instance-level signals. Server changes: - OrganizationConfigurationCE: add @Transient Boolean instanceBaseUrlConfigurationHealthy - OrganizationServiceCEImpl: inject SecureBaseUrlResolverCE, set the field in getClientPertinentOrganization (the protected method EE's chain calls via super.getClientPertinentOrganization, so the field flows through unmodified to multi-org EE code paths). .onErrorReturn(true) defends against transient resolver/feature-flag failures by producing a false-negative banner rather than breaking org-config fetch. - OrganizationServiceImpl (CE): pass the new constructor param through. - UserProfileCE_DTO: revert — field removed. - UserServiceCEImpl#buildUserProfileDTO: revert — Mono.zip back to 4-tuple, no resolver wiring. Server tests: - OrganizationServiceCETest: new getOrganizationConfig_includesBaseUrlConfigurationHealthy case verified locally (2/2 pass). - UserServiceTest: drop the now-misplaced buildUserProfileDTO test. - SecureBaseUrlResolverCEImplTest: unchanged (18/18 still pass — verified locally). Client changes: - userConstants.ts: revert — User interface drops the field. - usersSelectors.tsx: getShouldShowBaseUrlMissingBanner now reads state.organization.organizationConfiguration.instanceBaseUrlConfigurationHealthy while keeping the user-level gating on state.ui.users.currentUser. Same explicit === false rolling-deploy guard. - usersSelectors.test.ts + BaseUrlMissingBanner.test.tsx: tests updated for the new state shape (13 cases — all pass locally). The EE multi-org override of SecureBaseUrlResolverImpl#isBaseUrlConfigurationHealthy is unaffected — same resolver bean, now called from org service instead of user service. Multi-org Cloud still short-circuits to true and the banner never fires there. Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw --- .../BaseUrlMissingBanner.test.tsx | 65 ++++++++------- app/client/src/constants/userConstants.ts | 3 - .../src/selectors/usersSelectors.test.ts | 82 +++++++++++-------- app/client/src/selectors/usersSelectors.tsx | 14 +++- .../ce/OrganizationConfigurationCE.java | 12 +++ .../server/dtos/ce/UserProfileCE_DTO.java | 10 --- .../services/OrganizationServiceImpl.java | 7 +- .../ce/OrganizationServiceCEImpl.java | 25 +++++- .../server/services/ce/UserServiceCEImpl.java | 13 +-- .../server/services/UserServiceTest.java | 19 ----- .../ce/OrganizationServiceCETest.java | 21 +++++ 11 files changed, 153 insertions(+), 118 deletions(-) diff --git a/app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx b/app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx index 61c511fa33a5..7c5407f19d03 100644 --- a/app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx +++ b/app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx @@ -10,12 +10,15 @@ import BaseUrlMissingBanner from "./BaseUrlMissingBanner"; const mockStore = configureStore([]); -const storeWith = (currentUser: object | null) => - mockStore({ ui: { users: { currentUser } } }); +const storeWith = (currentUser: object | null, orgConfig?: object) => + mockStore({ + ui: { users: { currentUser } }, + organization: orgConfig ? { organizationConfiguration: orgConfig } : {}, + }); -const renderBanner = (currentUser: object | null) => +const renderBanner = (currentUser: object | null, orgConfig?: object) => render( - + @@ -24,13 +27,14 @@ const renderBanner = (currentUser: object | null) => , ); +const SUPER_ADMIN = { + isSuperUser: true, + adminSettingsVisible: true, +}; + describe("BaseUrlMissingBanner — GHSA-j9gf-vw2f-9hrw", () => { - it("renders for super-user with admin settings visible and unhealthy config", () => { - renderBanner({ - isSuperUser: true, - adminSettingsVisible: true, - instanceBaseUrlConfigurationHealthy: false, - }); + it("renders for super-user with admin settings visible and unhealthy org config", () => { + renderBanner(SUPER_ADMIN, { instanceBaseUrlConfigurationHealthy: false }); expect( screen.getByTestId("t--base-url-missing-banner"), @@ -40,10 +44,8 @@ describe("BaseUrlMissingBanner — GHSA-j9gf-vw2f-9hrw", () => { ).toBeInTheDocument(); }); - it("does not render when configuration is healthy", () => { - const { container } = renderBanner({ - isSuperUser: true, - adminSettingsVisible: true, + it("does not render when org config is healthy", () => { + const { container } = renderBanner(SUPER_ADMIN, { instanceBaseUrlConfigurationHealthy: true, }); @@ -51,32 +53,33 @@ describe("BaseUrlMissingBanner — GHSA-j9gf-vw2f-9hrw", () => { }); it("does not render for non-super-user", () => { - const { container } = renderBanner({ - isSuperUser: false, - adminSettingsVisible: true, - instanceBaseUrlConfigurationHealthy: false, - }); + const { container } = renderBanner( + { isSuperUser: false, adminSettingsVisible: true }, + { instanceBaseUrlConfigurationHealthy: false }, + ); expect(container.firstChild).toBeNull(); }); it("does not render when admin settings hidden (RBAC / license guard)", () => { - const { container } = renderBanner({ - isSuperUser: true, - adminSettingsVisible: false, - instanceBaseUrlConfigurationHealthy: false, - }); + const { container } = renderBanner( + { isSuperUser: true, adminSettingsVisible: false }, + { instanceBaseUrlConfigurationHealthy: false }, + ); expect(container.firstChild).toBeNull(); }); - // Rolling-deploy safety: newer client briefly paired with older server has the - // health field absent. Banner must stay hidden until both sides are deployed. - it("does not render when health field is missing (rolling deploy)", () => { - const { container } = renderBanner({ - isSuperUser: true, - adminSettingsVisible: true, - }); + // Rolling-deploy safety: org config without the new field. Banner must stay + // hidden, not flip on as a false positive. + it("does not render when health field is missing from org config (rolling deploy)", () => { + const { container } = renderBanner(SUPER_ADMIN, {}); + + expect(container.firstChild).toBeNull(); + }); + + it("does not render when org config is missing entirely", () => { + const { container } = renderBanner(SUPER_ADMIN); expect(container.firstChild).toBeNull(); }); diff --git a/app/client/src/constants/userConstants.ts b/app/client/src/constants/userConstants.ts index ba72be3abc1a..871d44e097ee 100644 --- a/app/client/src/constants/userConstants.ts +++ b/app/client/src/constants/userConstants.ts @@ -20,9 +20,6 @@ export interface User { isAnonymous?: boolean; isIntercomConsentGiven?: boolean; emailVerified: boolean; - // GHSA-j9gf-vw2f-9hrw — admin warning banner signal. Optional: older servers - // pre-rolling-deploy may not include the field, in which case the banner stays hidden. - instanceBaseUrlConfigurationHealthy?: boolean; } export interface UserApplication { diff --git a/app/client/src/selectors/usersSelectors.test.ts b/app/client/src/selectors/usersSelectors.test.ts index 15543df53cf7..24ec5ec9f512 100644 --- a/app/client/src/selectors/usersSelectors.test.ts +++ b/app/client/src/selectors/usersSelectors.test.ts @@ -1,30 +1,36 @@ import { getShouldShowBaseUrlMissingBanner } from "./usersSelectors"; -// Minimal Redux state shape; the selector only reads ui.users.currentUser. -const stateWith = (currentUser: object | null) => - ({ ui: { users: { currentUser } } }) as never; +// Minimal Redux state shape; the selector reads ui.users.currentUser AND +// organization.organizationConfiguration. +const stateWith = ( + currentUser: object | null, + orgConfig: object | undefined = undefined, +) => + ({ + ui: { users: { currentUser } }, + organization: orgConfig + ? { organizationConfiguration: orgConfig } + : undefined, + }) as never; + +const SUPER_ADMIN = { + isSuperUser: true, + adminSettingsVisible: true, +}; describe("getShouldShowBaseUrlMissingBanner — GHSA-j9gf-vw2f-9hrw", () => { - it("returns true for super user with admin settings visible and unhealthy config", () => { + it("returns true for super user with admin settings visible and unhealthy org config", () => { expect( getShouldShowBaseUrlMissingBanner( - stateWith({ - isSuperUser: true, - adminSettingsVisible: true, - instanceBaseUrlConfigurationHealthy: false, - }), + stateWith(SUPER_ADMIN, { instanceBaseUrlConfigurationHealthy: false }), ), ).toBe(true); }); - it("returns false when configuration is healthy", () => { + it("returns false when org config is healthy", () => { expect( getShouldShowBaseUrlMissingBanner( - stateWith({ - isSuperUser: true, - adminSettingsVisible: true, - instanceBaseUrlConfigurationHealthy: true, - }), + stateWith(SUPER_ADMIN, { instanceBaseUrlConfigurationHealthy: true }), ), ).toBe(false); }); @@ -32,11 +38,10 @@ describe("getShouldShowBaseUrlMissingBanner — GHSA-j9gf-vw2f-9hrw", () => { it("returns false for non-super-user", () => { expect( getShouldShowBaseUrlMissingBanner( - stateWith({ - isSuperUser: false, - adminSettingsVisible: true, - instanceBaseUrlConfigurationHealthy: false, - }), + stateWith( + { isSuperUser: false, adminSettingsVisible: true }, + { instanceBaseUrlConfigurationHealthy: false }, + ), ), ).toBe(false); }); @@ -44,29 +49,34 @@ describe("getShouldShowBaseUrlMissingBanner — GHSA-j9gf-vw2f-9hrw", () => { it("returns false when admin settings hidden (RBAC / license guard)", () => { expect( getShouldShowBaseUrlMissingBanner( - stateWith({ - isSuperUser: true, - adminSettingsVisible: false, - instanceBaseUrlConfigurationHealthy: false, - }), + stateWith( + { isSuperUser: true, adminSettingsVisible: false }, + { instanceBaseUrlConfigurationHealthy: false }, + ), ), ).toBe(false); }); - // Rolling-deploy safety: newer client paired briefly with older server. - // Field absent => banner must stay hidden, not flip on as a false positive. - it("returns false when health field is missing (rolling deploy)", () => { + // Rolling-deploy safety: newer client briefly paired with older server has the + // health field absent on org config. Banner must stay hidden, not flip on as a + // false positive. + it("returns false when health field is missing from org config (rolling deploy)", () => { + expect(getShouldShowBaseUrlMissingBanner(stateWith(SUPER_ADMIN, {}))).toBe( + false, + ); + }); + + it("returns false when org config is missing entirely", () => { + expect(getShouldShowBaseUrlMissingBanner(stateWith(SUPER_ADMIN))).toBe( + false, + ); + }); + + it("returns false when currentUser is null", () => { expect( getShouldShowBaseUrlMissingBanner( - stateWith({ - isSuperUser: true, - adminSettingsVisible: true, - }), + stateWith(null, { instanceBaseUrlConfigurationHealthy: false }), ), ).toBe(false); }); - - it("returns false when currentUser is null", () => { - expect(getShouldShowBaseUrlMissingBanner(stateWith(null))).toBe(false); - }); }); diff --git a/app/client/src/selectors/usersSelectors.tsx b/app/client/src/selectors/usersSelectors.tsx index 53054f76251a..86a271988c03 100644 --- a/app/client/src/selectors/usersSelectors.tsx +++ b/app/client/src/selectors/usersSelectors.tsx @@ -30,9 +30,16 @@ export const getIsUserLoggedIn = (state: DefaultRootState): boolean => /** * GHSA-j9gf-vw2f-9hrw — admin warning banner gate. Returns true only when ALL of: - * - the current user is an instance super user, + * - the current user is an instance super user (user-level gate), * - admin settings are visible to them (RBAC / license tier guard), - * - and the server explicitly reports `instanceBaseUrlConfigurationHealthy === false`. + * - and the server explicitly reports the org config's + * `instanceBaseUrlConfigurationHealthy === false` (instance-level signal). + * + * The instance signal lives on `state.organization.organizationConfiguration` + * (populated from the always-called `/v1/consolidated-api` bootstrap), not on + * `currentUser`. `/v1/users/me` is rarely re-fetched on dashboard routes — the + * canonical bootstrap is consolidated-api, and org config is the right bucket + * within it for instance-state. * * The explicit `=== false` (rather than `!value`) is deliberate: during a rolling * deploy where a newer client briefly sees an older server's response without the @@ -43,10 +50,11 @@ export const getShouldShowBaseUrlMissingBanner = ( state: DefaultRootState, ): boolean => { const user = state.ui?.users?.currentUser; + const orgConfig = state.organization?.organizationConfiguration; return Boolean( user?.isSuperUser && user?.adminSettingsVisible && - user?.instanceBaseUrlConfigurationHealthy === false, + orgConfig?.instanceBaseUrlConfigurationHealthy === false, ); }; diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.java index 0b6ee27834ba..e2ed0d115aea 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.java @@ -63,6 +63,18 @@ public class OrganizationConfigurationCE implements Serializable { private Boolean isAtomicPushAllowed = false; + /** + * Health signal driving the admin warning banner introduced for + * GHSA-j9gf-vw2f-9hrw. + * Set at runtime by {@code OrganizationServiceCEImpl#getClientPertinentOrganization} from + * {@code SecureBaseUrlResolverCE#isBaseUrlConfigurationHealthy()} — this field is NOT + * persisted to the DB, so it does not appear in {@link #copyNonSensitiveValues}. False on + * a CE / single-org-EE deployment with {@code APPSMITH_BASE_URL} unset; always true on + * multi-org-EE because each organization derives its own canonical base URL. + */ + @Transient + private Boolean instanceBaseUrlConfigurationHealthy; + public void addThirdPartyAuth(String auth) { if (thirdPartyAuths == null) { thirdPartyAuths = new ArrayList<>(); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/UserProfileCE_DTO.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/UserProfileCE_DTO.java index 31cf45bfd6af..cdfe7399cd6e 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/UserProfileCE_DTO.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/UserProfileCE_DTO.java @@ -39,16 +39,6 @@ public class UserProfileCE_DTO { @JsonProperty("isIntercomConsentGiven") boolean isIntercomConsentGiven = false; - /** - * Health signal driving the admin warning banner introduced for - * GHSA-j9gf-vw2f-9hrw. - * Reflects the answer of {@code SecureBaseUrlResolverCE#isBaseUrlConfigurationHealthy()}. - * Default {@code true} so any DTO instance built outside the real assembler (test fixtures, - * mocks) does not false-positive the banner. - */ - @JsonProperty("instanceBaseUrlConfigurationHealthy") - boolean instanceBaseUrlConfigurationHealthy = true; - String photoId; String useCase; diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/OrganizationServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/OrganizationServiceImpl.java index 2d0294e884be..12884097037a 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/OrganizationServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/OrganizationServiceImpl.java @@ -2,6 +2,7 @@ import com.appsmith.server.configurations.CommonConfig; import com.appsmith.server.helpers.FeatureFlagMigrationHelper; +import com.appsmith.server.helpers.SecureBaseUrlResolver; import com.appsmith.server.helpers.UserOrganizationHelper; import com.appsmith.server.instanceconfigs.helpers.InstanceVariablesHelper; import com.appsmith.server.repositories.CacheableRepositoryHelper; @@ -27,7 +28,8 @@ public OrganizationServiceImpl( CommonConfig commonConfig, ObservationRegistry observationRegistry, UserOrganizationHelper userOrganizationHelper, - InstanceVariablesHelper instanceVariablesHelper) { + InstanceVariablesHelper instanceVariablesHelper, + SecureBaseUrlResolver secureBaseUrlResolver) { super( validator, repository, @@ -39,6 +41,7 @@ public OrganizationServiceImpl( commonConfig, observationRegistry, userOrganizationHelper, - instanceVariablesHelper); + instanceVariablesHelper, + secureBaseUrlResolver); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java index 6f6220fef517..425e24c19151 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java @@ -15,6 +15,7 @@ import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.helpers.CollectionUtils; import com.appsmith.server.helpers.FeatureFlagMigrationHelper; +import com.appsmith.server.helpers.SecureBaseUrlResolver; import com.appsmith.server.helpers.UserOrganizationHelper; import com.appsmith.server.instanceconfigs.helpers.InstanceVariablesHelper; import com.appsmith.server.repositories.CacheableRepositoryHelper; @@ -61,6 +62,8 @@ public class OrganizationServiceCEImpl extends BaseService getClientPertinentOrganization( clientOrganization.setId(dbOrganization.getId()); clientOrganization.setUserPermissions(dbOrganization.getUserPermissions()); - return Mono.just(clientOrganization); + // GHSA-j9gf-vw2f-9hrw — surface the resolver health signal on the org config so the + // client can render the admin warning banner when the instance is in fail-closed mode + // for token-bearing email flows. This rides on /v1/consolidated-api which is the only + // bootstrap call hit on every SPA reload — putting the signal on userProfile (where it + // first lived) was incorrect because the field is instance-state, not user-state. + // .onErrorReturn(true) so a transient resolver/feature-flag failure produces a + // false-negative banner rather than breaking org-config fetch entirely. + final Organization finalClientOrganization = clientOrganization; + return secureBaseUrlResolver + .isBaseUrlConfigurationHealthy() + .onErrorReturn(true) + .map(healthy -> { + finalClientOrganization + .getOrganizationConfiguration() + .setInstanceBaseUrlConfigurationHealthy(healthy); + return finalClientOrganization; + }); } // This function is used to save the organization object in the database and evict the cache diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java index 3fd02910c86b..af8436841dcb 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java @@ -775,26 +775,16 @@ public Mono buildUserProfileDTO(User user) { Mono isSuperUserMono = userFromDbMono.flatMap(userUtils::isSuperUser).defaultIfEmpty(false); - // GHSA-j9gf-vw2f-9hrw — surface the resolver health signal on the profile so the - // client can render the admin warning banner when the instance is in fail-closed mode - // for token-bearing email flows. .onErrorReturn(true) defends against a transient - // resolver/feature-flag failure breaking login (false-negative banner is preferable to - // a broken /v1/users/profile call). - Mono isBaseUrlConfigurationHealthyMono = - secureBaseUrlResolver.isBaseUrlConfigurationHealthy().onErrorReturn(true); - return Mono.zip( isUsersEmpty(), userFromDbMono, userDataService.getForCurrentUser().defaultIfEmpty(new UserData()), - isSuperUserMono, - isBaseUrlConfigurationHealthyMono) + isSuperUserMono) .flatMap(tuple -> { final boolean isUsersEmpty = Boolean.TRUE.equals(tuple.getT1()); final User userFromDb = tuple.getT2(); final UserData userData = tuple.getT3(); Boolean isSuperUser = tuple.getT4(); - Boolean isBaseUrlConfigurationHealthy = tuple.getT5(); final UserProfileDTO profile = new UserProfileDTO(); @@ -814,7 +804,6 @@ public Mono buildUserProfileDTO(User user) { commonConfig.getIsCloudHosting() ? true : userData.getIsIntercomConsentGiven()); profile.setIsSuperUser(isSuperUser); profile.setIsConfigurable(!StringUtils.isEmpty(commonConfig.getEnvFilePath())); - profile.setInstanceBaseUrlConfigurationHealthy(isBaseUrlConfigurationHealthy); return pacConfigurationService.setRolesAndGroups(profile, userFromDb, true); }); } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java index 7e39bcdfb427..a2203ae5d4a6 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java @@ -721,25 +721,6 @@ public void buildUserProfileDTO_WhenAnonymousUser_ReturnsProfile() { .verifyComplete(); } - /** - * GHSA-j9gf-vw2f-9hrw — pins that the {@code SecureBaseUrlResolver#isBaseUrlConfigurationHealthy()} - * signal flows through the DTO assembler. In the test profile {@code APPSMITH_BASE_URL} is unset, - * so the resolver returns {@code false} regardless of the {@code APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS} - * flag (the health signal answers "is base URL configured?", not "are emails currently being sent?"). - */ - @Test - @WithUserDetails(value = "api_user") - public void buildUserProfileDTO_includesBaseUrlConfigurationHealthy() { - StepVerifier.create(sessionUserService.getCurrentUser().flatMap(userService::buildUserProfileDTO)) - .assertNext(userProfileDTO -> { - assertThat(userProfileDTO.getInstanceBaseUrlConfigurationHealthy()) - .as("In test profile APPSMITH_BASE_URL is unset, so the resolver-derived " - + "health signal must be false on the assembled DTO") - .isFalse(); - }) - .verifyComplete(); - } - /** * This test case asserts that on every user creation, User Management role is auto-created and associated with that user. */ diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java index dcac8cf8184d..441e29eab550 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java @@ -234,6 +234,27 @@ void getOrganizationConfig_Valid_AnonymousUser() { .verifyComplete(); } + /** + * GHSA-j9gf-vw2f-9hrw — pins that the {@code SecureBaseUrlResolver#isBaseUrlConfigurationHealthy()} + * signal flows through the org-config assembler. In the test profile {@code APPSMITH_BASE_URL} + * is unset, so the resolver returns {@code false} and the assembled config exposes that to the + * client. The signal lives on org config (not user profile) because it's instance-state and + * because {@code /v1/consolidated-api} — the only always-called bootstrap endpoint — already + * carries org config in its response. + */ + @Test + @WithUserDetails("api_user") + void getOrganizationConfig_includesBaseUrlConfigurationHealthy() { + StepVerifier.create(organizationService.getOrganizationConfiguration()) + .assertNext(organization -> { + assertThat(organization.getOrganizationConfiguration().getInstanceBaseUrlConfigurationHealthy()) + .as("In test profile APPSMITH_BASE_URL is unset, so the resolver-derived " + + "health signal must be false on the assembled org config") + .isFalse(); + }) + .verifyComplete(); + } + @Test @WithUserDetails("anonymousUser") void getOrganizationConfig_AnonymousUser_DoesNotExposeInstanceMetadata() { From 982c4fc8d3989f6caebc919912e8cd99d785e64c Mon Sep 17 00:00:00 2001 From: subrata71 Date: Thu, 30 Apr 2026 14:26:42 +0600 Subject: [PATCH 17/22] fix(security): break EE construction-time cycle on SecureBaseUrlResolver injection (GHSA-j9gf-vw2f-9hrw) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit's constructor injection of SecureBaseUrlResolver into OrganizationServiceCEImpl created a circular dependency in EE: OrganizationService → SecureBaseUrlResolver (EE override) → FeatureFlagService → OrganizationService ← cycle CE was unaffected because the CE SecureBaseUrlResolverImpl doesn't inject FeatureFlagService. EE failed bean construction across most server unit tests. @Lazy on the constructor param injects a Spring proxy that defers actual bean resolution until the first method call. By then (during the org-config fetch that happens after Spring's full context is up), all three beans are constructed and the cycle is moot. Verified: server unit tests still green locally (18/18 SecureBaseUrlResolverCEImplTest + 2/2 OrganizationServiceCETest including the new getOrganizationConfig_includesBaseUrlConfigurationHealthy case). Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw --- .../appsmith/server/services/OrganizationServiceImpl.java | 2 +- .../server/services/ce/OrganizationServiceCEImpl.java | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/OrganizationServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/OrganizationServiceImpl.java index 12884097037a..9c03db517112 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/OrganizationServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/OrganizationServiceImpl.java @@ -29,7 +29,7 @@ public OrganizationServiceImpl( ObservationRegistry observationRegistry, UserOrganizationHelper userOrganizationHelper, InstanceVariablesHelper instanceVariablesHelper, - SecureBaseUrlResolver secureBaseUrlResolver) { + @Lazy SecureBaseUrlResolver secureBaseUrlResolver) { super( validator, repository, diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java index 425e24c19151..b4ae62bc49b5 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java @@ -76,7 +76,13 @@ public OrganizationServiceCEImpl( ObservationRegistry observationRegistry, UserOrganizationHelper userOrganizationHelper, InstanceVariablesHelper instanceVariablesHelper, - SecureBaseUrlResolver secureBaseUrlResolver) { + // @Lazy breaks an EE-only construction-time cycle: the EE override of + // SecureBaseUrlResolverImpl injects FeatureFlagService, which in turn injects + // OrganizationService. Without @Lazy, Spring fails to construct any of these + // three beans. The lazy proxy defers actual resolution until first method call + // (during getClientPertinentOrganization), by which point all beans are wired. + // Has no functional effect on CE where the cycle doesn't exist. + @Lazy SecureBaseUrlResolver secureBaseUrlResolver) { super(validator, repository, analyticsService); this.configService = configService; this.envManager = envManager; From b6bbdcca302fcb4e924ee91b06b23b7bccc55ff0 Mon Sep 17 00:00:00 2001 From: subrata71 Date: Thu, 30 Apr 2026 14:39:02 +0600 Subject: [PATCH 18/22] refactor(security): break circular dep by relocating resolver call to ConsolidatedAPIServiceCEImpl (GHSA-j9gf-vw2f-9hrw) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverts the @Lazy workaround from 982c4fc8d3 in favor of a proper architectural fix. @Lazy hides design problems rather than solving them. The cycle was: OrganizationService → SecureBaseUrlResolver (EE override) → FeatureFlagService → OrganizationService Root cause: OrganizationService is a data-layer service. Injecting a runtime-config signal (resolver) into it is wrong on principle, and that wrongness manifested as the cycle in EE's bean graph. Fix: relocate the resolver call to ConsolidatedAPIServiceCEImpl, which is the orchestration layer that already aggregates org config + user profile + feature flags + product alert into the bootstrap response. That layer is at the top of the dependency graph (nothing injects it back), so no cycle exists. Server changes: - OrganizationServiceCEImpl: revert (no resolver injection, getClientPertinentOrganization back to original). - OrganizationServiceImpl (CE): revert constructor. - ConsolidatedAPIServiceCEImpl: inject SecureBaseUrlResolver as a final field (Lombok @RequiredArgsConstructor handles wiring), and in the org-config fetch block in getAllFetchableMonos, .zipWith the resolver result and set the field on the assembled organizationConfiguration. .onErrorReturn(true) defends against transient resolver failures (false-negative banner is recoverable; broken consolidated-api fetch is not). Defensive null-check on organizationConfiguration to handle test-mocks that produce bare Organization. - ConsolidatedAPIServiceImpl + ConsolidatedAPIServiceCECompatibleImpl: pass the new constructor param through. Tests: 41/41 pass locally — 18 SecureBaseUrlResolverCEImplTest, 8 ConsolidatedAPIServiceImplTest (existing tests still green with defensive null-check on bare Organization mocks), 15 OrganizationServiceCETest (the previously-added integration test on org service is dropped — it was testing the wrong layer; the field is now set in consolidated-api). The OrganizationConfigurationCE @Transient field is unchanged and still the right wire shape — only the assembly site moved. Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw --- .../services/ConsolidatedAPIServiceImpl.java | 7 +++-- .../services/OrganizationServiceImpl.java | 7 ++--- .../ce/ConsolidatedAPIServiceCEImpl.java | 24 +++++++++++++- .../ce/OrganizationServiceCEImpl.java | 31 ++----------------- ...onsolidatedAPIServiceCECompatibleImpl.java | 7 +++-- .../ce/OrganizationServiceCETest.java | 21 ------------- 6 files changed, 37 insertions(+), 60 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java index 437a8287ff80..bea886725833 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java @@ -4,6 +4,7 @@ import com.appsmith.server.actioncollections.base.ActionCollectionService; import com.appsmith.server.applications.base.ApplicationService; import com.appsmith.server.datasources.base.DatasourceService; +import com.appsmith.server.helpers.SecureBaseUrlResolver; import com.appsmith.server.jslibs.base.CustomJSLibService; import com.appsmith.server.newactions.base.NewActionService; import com.appsmith.server.newpages.base.NewPageService; @@ -39,7 +40,8 @@ public ConsolidatedAPIServiceImpl( MockDataService mockDataService, ObservationRegistry observationRegistry, CacheableRepositoryHelper cacheableRepositoryHelper, - ObservationHelper observationHelper) { + ObservationHelper observationHelper, + SecureBaseUrlResolver secureBaseUrlResolver) { super( sessionUserService, userService, @@ -59,6 +61,7 @@ public ConsolidatedAPIServiceImpl( mockDataService, observationRegistry, cacheableRepositoryHelper, - observationHelper); + observationHelper, + secureBaseUrlResolver); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/OrganizationServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/OrganizationServiceImpl.java index 9c03db517112..2d0294e884be 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/OrganizationServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/OrganizationServiceImpl.java @@ -2,7 +2,6 @@ import com.appsmith.server.configurations.CommonConfig; import com.appsmith.server.helpers.FeatureFlagMigrationHelper; -import com.appsmith.server.helpers.SecureBaseUrlResolver; import com.appsmith.server.helpers.UserOrganizationHelper; import com.appsmith.server.instanceconfigs.helpers.InstanceVariablesHelper; import com.appsmith.server.repositories.CacheableRepositoryHelper; @@ -28,8 +27,7 @@ public OrganizationServiceImpl( CommonConfig commonConfig, ObservationRegistry observationRegistry, UserOrganizationHelper userOrganizationHelper, - InstanceVariablesHelper instanceVariablesHelper, - @Lazy SecureBaseUrlResolver secureBaseUrlResolver) { + InstanceVariablesHelper instanceVariablesHelper) { super( validator, repository, @@ -41,7 +39,6 @@ public OrganizationServiceImpl( commonConfig, observationRegistry, userOrganizationHelper, - instanceVariablesHelper, - secureBaseUrlResolver); + instanceVariablesHelper); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java index 4f579b36e018..ee2609360250 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java @@ -12,6 +12,7 @@ import com.appsmith.server.domains.ApplicationMode; import com.appsmith.server.domains.GitArtifactMetadata; import com.appsmith.server.domains.NewPage; +import com.appsmith.server.domains.OrganizationConfiguration; import com.appsmith.server.domains.Plugin; import com.appsmith.server.dtos.ApplicationPagesDTO; import com.appsmith.server.dtos.ConsolidatedAPIResponseDTO; @@ -21,6 +22,7 @@ import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.helpers.GitUtils; +import com.appsmith.server.helpers.SecureBaseUrlResolver; import com.appsmith.server.helpers.TextUtils; import com.appsmith.server.jslibs.base.CustomJSLibService; import com.appsmith.server.newactions.base.NewActionService; @@ -128,6 +130,7 @@ public class ConsolidatedAPIServiceCEImpl implements ConsolidatedAPIServiceCE { private final ObservationRegistry observationRegistry; private final CacheableRepositoryHelper cacheableRepositoryHelper; private final ObservationHelper observationHelper; + private final SecureBaseUrlResolver secureBaseUrlResolver; protected ResponseDTO getSuccessResponse(T data) { return new ResponseDTO<>(HttpStatus.OK, data); @@ -207,9 +210,28 @@ protected List> getAllFetchableMonos( .cache(); fetches.add(featureFlagsForCurrentUserResponseDTOMonoCache); - /* Get organization config data */ + /* Get organization config data, then enrich with the runtime + * `instanceBaseUrlConfigurationHealthy` signal that drives the admin warning banner + * for GHSA-j9gf-vw2f-9hrw. The resolver call is intentionally co-located here in the + * orchestration layer rather than inside OrganizationServiceCEImpl: in EE the resolver + * depends on FeatureFlagService which depends on OrganizationService, so injecting it + * into the org service would create a construction-time cycle. Co-locating in the + * consumer (consolidated-api) keeps the data layer cycle-free. + * .onErrorReturn(true) so a transient resolver/feature-flag failure produces a + * false-negative banner rather than breaking the org-config fetch entirely. */ fetches.add(organizationService .getOrganizationConfiguration() + .zipWith(secureBaseUrlResolver.isBaseUrlConfigurationHealthy().onErrorReturn(true)) + .map(tuple -> { + var organization = tuple.getT1(); + // Defensive: org-service production code always populates the config, but + // some test fixtures and mocks return a bare Organization. Don't NPE. + if (organization.getOrganizationConfiguration() == null) { + organization.setOrganizationConfiguration(new OrganizationConfiguration()); + } + organization.getOrganizationConfiguration().setInstanceBaseUrlConfigurationHealthy(tuple.getT2()); + return organization; + }) .as(this::toResponseDTO) .doOnError(e -> log.error("Error fetching organization config", e)) .doOnSuccess(consolidatedAPIResponseDTO::setOrganizationConfig) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java index b4ae62bc49b5..6f6220fef517 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java @@ -15,7 +15,6 @@ import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.helpers.CollectionUtils; import com.appsmith.server.helpers.FeatureFlagMigrationHelper; -import com.appsmith.server.helpers.SecureBaseUrlResolver; import com.appsmith.server.helpers.UserOrganizationHelper; import com.appsmith.server.instanceconfigs.helpers.InstanceVariablesHelper; import com.appsmith.server.repositories.CacheableRepositoryHelper; @@ -62,8 +61,6 @@ public class OrganizationServiceCEImpl extends BaseService getClientPertinentOrganization( clientOrganization.setId(dbOrganization.getId()); clientOrganization.setUserPermissions(dbOrganization.getUserPermissions()); - // GHSA-j9gf-vw2f-9hrw — surface the resolver health signal on the org config so the - // client can render the admin warning banner when the instance is in fail-closed mode - // for token-bearing email flows. This rides on /v1/consolidated-api which is the only - // bootstrap call hit on every SPA reload — putting the signal on userProfile (where it - // first lived) was incorrect because the field is instance-state, not user-state. - // .onErrorReturn(true) so a transient resolver/feature-flag failure produces a - // false-negative banner rather than breaking org-config fetch entirely. - final Organization finalClientOrganization = clientOrganization; - return secureBaseUrlResolver - .isBaseUrlConfigurationHealthy() - .onErrorReturn(true) - .map(healthy -> { - finalClientOrganization - .getOrganizationConfiguration() - .setInstanceBaseUrlConfigurationHealthy(healthy); - return finalClientOrganization; - }); + return Mono.just(clientOrganization); } // This function is used to save the organization object in the database and evict the cache diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/ConsolidatedAPIServiceCECompatibleImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/ConsolidatedAPIServiceCECompatibleImpl.java index f7cdeadd9115..bddd4369195d 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/ConsolidatedAPIServiceCECompatibleImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/ConsolidatedAPIServiceCECompatibleImpl.java @@ -4,6 +4,7 @@ import com.appsmith.server.actioncollections.base.ActionCollectionService; import com.appsmith.server.applications.base.ApplicationService; import com.appsmith.server.datasources.base.DatasourceService; +import com.appsmith.server.helpers.SecureBaseUrlResolver; import com.appsmith.server.jslibs.base.CustomJSLibService; import com.appsmith.server.newactions.base.NewActionService; import com.appsmith.server.newpages.base.NewPageService; @@ -42,7 +43,8 @@ public ConsolidatedAPIServiceCECompatibleImpl( MockDataService mockDataService, ObservationRegistry observationRegistry, CacheableRepositoryHelper cacheableRepositoryHelper, - ObservationHelper observationHelper) { + ObservationHelper observationHelper, + SecureBaseUrlResolver secureBaseUrlResolver) { super( sessionUserService, userService, @@ -62,6 +64,7 @@ public ConsolidatedAPIServiceCECompatibleImpl( mockDataService, observationRegistry, cacheableRepositoryHelper, - observationHelper); + observationHelper, + secureBaseUrlResolver); } } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java index 441e29eab550..dcac8cf8184d 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/OrganizationServiceCETest.java @@ -234,27 +234,6 @@ void getOrganizationConfig_Valid_AnonymousUser() { .verifyComplete(); } - /** - * GHSA-j9gf-vw2f-9hrw — pins that the {@code SecureBaseUrlResolver#isBaseUrlConfigurationHealthy()} - * signal flows through the org-config assembler. In the test profile {@code APPSMITH_BASE_URL} - * is unset, so the resolver returns {@code false} and the assembled config exposes that to the - * client. The signal lives on org config (not user profile) because it's instance-state and - * because {@code /v1/consolidated-api} — the only always-called bootstrap endpoint — already - * carries org config in its response. - */ - @Test - @WithUserDetails("api_user") - void getOrganizationConfig_includesBaseUrlConfigurationHealthy() { - StepVerifier.create(organizationService.getOrganizationConfiguration()) - .assertNext(organization -> { - assertThat(organization.getOrganizationConfiguration().getInstanceBaseUrlConfigurationHealthy()) - .as("In test profile APPSMITH_BASE_URL is unset, so the resolver-derived " - + "health signal must be false on the assembled org config") - .isFalse(); - }) - .verifyComplete(); - } - @Test @WithUserDetails("anonymousUser") void getOrganizationConfig_AnonymousUser_DoesNotExposeInstanceMetadata() { From d3fa256074cd9889033f2d6f71959c092b5a467e Mon Sep 17 00:00:00 2001 From: subrata71 Date: Thu, 30 Apr 2026 15:57:34 +0600 Subject: [PATCH 19/22] test(security): mock SecureBaseUrlResolver in ConsolidatedAPIServiceImplTest (GHSA-j9gf-vw2f-9hrw) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous refactor (b6bbdcca30) made ConsolidatedAPIServiceCEImpl call SecureBaseUrlResolver alongside the existing OrganizationService call. In this heavily-mocked test class, OrganizationService is a @MockBean that returns null for unstubbed methods. The real EE SecureBaseUrlResolverImpl chain transitively calls OrganizationService.getCurrentUserOrganizationId() (via FeatureFlagService), which NPE'd because that method wasn't stubbed. Fix: mock SecureBaseUrlResolver too. One @MockBean + a @BeforeEach default of isBaseUrlConfigurationHealthy() → Mono.just(true) avoids escaping into the real EE chain entirely. Existing tests don't care about the banner state; future banner-specific tests can override the stub per-method. Verified: 8/8 ConsolidatedAPIServiceImplTest pass locally with the mock. Other test files that reference consolidatedAPIService use @Autowired (real bean) or @SpyBean (real with selective stubs); they don't hit this since the real resolver chain works under test conditions. Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw --- .../ce/ConsolidatedAPIServiceImplTest.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java index 8c3c7eeddec2..75096632618f 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java @@ -34,6 +34,7 @@ import com.appsmith.server.dtos.UserProfileDTO; import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; +import com.appsmith.server.helpers.SecureBaseUrlResolver; import com.appsmith.server.jslibs.base.CustomJSLibService; import com.appsmith.server.newactions.base.NewActionService; import com.appsmith.server.newpages.base.NewPageService; @@ -50,6 +51,7 @@ import com.appsmith.server.services.UserDataService; import com.appsmith.server.services.UserService; import com.appsmith.server.themes.base.ThemeService; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.Mockito; import org.springframework.beans.factory.annotation.Autowired; @@ -103,6 +105,23 @@ public class ConsolidatedAPIServiceImplTest { @MockBean ProductAlertService mockProductAlertService; + /** + * GHSA-j9gf-vw2f-9hrw — the consolidated-api now calls + * {@link SecureBaseUrlResolver#isBaseUrlConfigurationHealthy()} alongside the org-config + * fetch. Mock it here so the test class doesn't escape into the real EE resolver chain + * (which transitively pulls in FeatureFlagService → OrganizationService and would NPE + * since OrganizationService is itself a @MockBean returning null for unstubbed methods). + */ + @MockBean + SecureBaseUrlResolver mockSecureBaseUrlResolver; + + @BeforeEach + void stubSecureBaseUrlResolver() { + // Default to healthy=true. Existing tests don't care about the admin-banner state; + // dedicated tests for the banner signal stub their own override on this mock. + when(mockSecureBaseUrlResolver.isBaseUrlConfigurationHealthy()).thenReturn(Mono.just(true)); + } + @SpyBean NewPageService spyNewPageService; From a5b32ff9ca1c260db6aabb2d543de7670a848c73 Mon Sep 17 00:00:00 2001 From: subrata71 Date: Thu, 30 Apr 2026 17:10:45 +0600 Subject: [PATCH 20/22] fix(client): rebuild BaseUrlMissingBanner using ADS Banner pattern (GHSA-j9gf-vw2f-9hrw) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous implementation used the @appsmith/ads Callout component, which is designed for inline cards (rounded corners, flex-row layout, no width). When mounted at the top of AppRouter, it rendered as a tiny squeezed card on the left edge of the page rather than a full-width banner — because AppHeader is rendered into a separate DOM portal (#header-root) and the page below uses absolute/viewport sizing that paints over my non-positioned banner. Switched to the existing pattern used by PageBannerMessage (the EE license/ trial expiry banner): the ADS Banner component (NOT Callout), wrapped in a styled component with `position: fixed; top: 0; left: 0; right: 0; width: 100%; z-index: 2`. This is the codebase's single source of truth for top-of-screen banners. Banner is just a Callout with _componentType="banner" set internally — the same ADS primitive does the right horizontal layout, removes border-radius, and centers content. API: single `link` prop instead of `links` array. Also dropped the BASE_URL_MISSING_BANNER_TITLE constant; the consolidated body copy reads better as a single sentence in a banner format. Tests: 13/13 client unit tests pass locally with the new component. Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw --- app/client/src/ce/constants/messages.ts | 4 +- .../BaseUrlMissingBanner.test.tsx | 4 +- .../editorComponents/BaseUrlMissingBanner.tsx | 52 +++++++++---------- 3 files changed, 28 insertions(+), 32 deletions(-) diff --git a/app/client/src/ce/constants/messages.ts b/app/client/src/ce/constants/messages.ts index beab3c4fc2f1..d35ef66b79ce 100644 --- a/app/client/src/ce/constants/messages.ts +++ b/app/client/src/ce/constants/messages.ts @@ -1018,10 +1018,8 @@ export const I_UNDERSTAND = () => "I understand"; // Admin warning banner — shown when APPSMITH_BASE_URL is unset and the resolver is in // fail-closed mode for token-bearing email flows. See GHSA-j9gf-vw2f-9hrw. -export const BASE_URL_MISSING_BANNER_TITLE = () => - "Email delivery is disabled on this instance"; export const BASE_URL_MISSING_BANNER_BODY = () => - "Forgot-password, email-verification, and invite emails will not be delivered until APPSMITH_BASE_URL is configured. Set it from Admin Settings to restore email-based flows."; + "Email delivery is disabled — forgot-password, email-verification and invite emails will not be sent until APPSMITH_BASE_URL is configured."; export const BASE_URL_MISSING_BANNER_CTA = () => "Configure APPSMITH_BASE_URL"; export const GIT_NO_UPDATED_TOOLTIP = () => "No new updates to push"; diff --git a/app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx b/app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx index 7c5407f19d03..6323b52fba48 100644 --- a/app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx +++ b/app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx @@ -39,9 +39,7 @@ describe("BaseUrlMissingBanner — GHSA-j9gf-vw2f-9hrw", () => { expect( screen.getByTestId("t--base-url-missing-banner"), ).toBeInTheDocument(); - expect( - screen.getByText(/Email delivery is disabled on this instance/i), - ).toBeInTheDocument(); + expect(screen.getByText(/Email delivery is disabled/i)).toBeInTheDocument(); }); it("does not render when org config is healthy", () => { diff --git a/app/client/src/components/editorComponents/BaseUrlMissingBanner.tsx b/app/client/src/components/editorComponents/BaseUrlMissingBanner.tsx index a5e366b204f3..a778c4eda1ec 100644 --- a/app/client/src/components/editorComponents/BaseUrlMissingBanner.tsx +++ b/app/client/src/components/editorComponents/BaseUrlMissingBanner.tsx @@ -1,14 +1,13 @@ import React from "react"; import { useSelector } from "react-redux"; import styled from "styled-components"; -import { Callout } from "@appsmith/ads"; +import { Banner } from "@appsmith/ads"; import { getShouldShowBaseUrlMissingBanner } from "selectors/usersSelectors"; import { adminSettingsCategoryUrl } from "ee/RouteBuilder"; import { SettingCategories } from "ee/pages/AdminSettings/config/types"; import { BASE_URL_MISSING_BANNER_BODY, BASE_URL_MISSING_BANNER_CTA, - BASE_URL_MISSING_BANNER_TITLE, createMessage, } from "ee/constants/messages"; @@ -17,15 +16,21 @@ import { * the server's SecureBaseUrlResolver reports that APPSMITH_BASE_URL is unset and * token-bearing email flows are therefore disabled. * - * Positioned at the very top of the application (rendered above AppHeader in - * AppRouter), in normal document flow so the rest of the layout pushes down by - * the banner's height. Not user-dismissible — the banner reflects live server - * state and clears when the admin sets APPSMITH_BASE_URL via Admin Settings, - * which triggers the existing Configuration-tier server-restart + SPA-reload - * flow. + * Uses the same `Banner` ADS component + `position: fixed; top: 0` styling as + * the existing PageBannerMessage (license/trial banner) — single source of truth + * for top-of-screen banners across the product. Not user-dismissible: banner + * reflects live server state and clears on the next bootstrap fetch after the + * admin sets APPSMITH_BASE_URL via Admin Settings (which triggers the existing + * Configuration-tier server-restart + SPA-reload flow). */ -const BannerContainer = styled.div` +const StyledBanner = styled(Banner)` + position: fixed; + z-index: 2; width: 100%; + display: flex; + align-items: center; + justify-content: center; + top: 0; `; const BaseUrlMissingBanner: React.FC = () => { @@ -34,23 +39,18 @@ const BaseUrlMissingBanner: React.FC = () => { if (!shouldShow) return null; return ( - - - {createMessage(BASE_URL_MISSING_BANNER_TITLE)} -
    - {createMessage(BASE_URL_MISSING_BANNER_BODY)} -     -     + + {createMessage(BASE_URL_MISSING_BANNER_BODY)} + ); }; From 5e8ad498164a8f0ed8ba5233835771c90eeb5fc1 Mon Sep 17 00:00:00 2001 From: subrata71 Date: Thu, 30 Apr 2026 18:04:34 +0600 Subject: [PATCH 21/22] fix(client): mount BaseUrlMissingBanner inside PageHeader to fix z-index stacking (GHSA-j9gf-vw2f-9hrw) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mounting the banner at the AppRouter level — outside PageHeader's knowledge — caused it to be painted under the header. The banner had `position: fixed; top: 0; z-index: 2`, but StyledPageHeader has `position: fixed; top: 0; z-index: var(--ads-v2-z-index-9)` (much higher) so the page header completely covered the banner. Verified directly in the deployed DP via `document.elementFromPoint(banner_x, banner_y) →

    `. The existing PageBannerMessage (license/trial banner) doesn't have this problem because PageHeader.tsx knows when it's rendering and pushes StyledPageHeader down via the `isBannerVisible` prop (top: 40px desktop / 70px mobile). Fix: follow that exact pattern for our banner too. - PageHeader.tsx: render alongside the existing , and OR the two visibility flags into a single isAnyBannerVisible passed to StyledPageHeader. Header now correctly shifts down for either banner. - ce/AppRouter.tsx: remove the BaseUrlMissingBanner mount — no longer needed at this layer. Trade-off acknowledged: PageHeader is rendered on dashboard / admin / license pages, NOT on the editor or app-viewer routes. Banner won't appear inside the editor (acceptable v1 — admins typically discover this via the dashboard). Follow-up if needed: also wire into the AppIDE / AppViewer headers. Refs https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw --- app/client/src/ce/AppRouter.tsx | 6 ------ app/client/src/pages/common/PageHeader.tsx | 17 +++++++++++++++-- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/app/client/src/ce/AppRouter.tsx b/app/client/src/ce/AppRouter.tsx index dfb4a03c6945..43e9799a44aa 100644 --- a/app/client/src/ce/AppRouter.tsx +++ b/app/client/src/ce/AppRouter.tsx @@ -57,7 +57,6 @@ import RouteChangeListener from "RouteChangeListener"; import { initCurrentPage } from "../actions/initActions"; import Walkthrough from "components/featureWalkthrough"; import ProductAlertBanner from "components/editorComponents/ProductAlertBanner"; -import BaseUrlMissingBanner from "components/editorComponents/BaseUrlMissingBanner"; import { getAdminSettingsPath } from "ee/utils/BusinessFeatures/adminSettingsHelpers"; import { useFeatureFlag } from "utils/hooks/useFeatureFlag"; import { FEATURE_FLAG } from "ee/entities/FeatureFlag"; @@ -189,11 +188,6 @@ export default function AppRouter() { ) : ( <> - {/* GHSA-j9gf-vw2f-9hrw — admin warning banner sits at the very top of the - render tree so it pushes AppHeader and the rest of the layout down by its - own height. Returns null for non-admins / healthy instances, so non-target - users pay zero render cost. */} - diff --git a/app/client/src/pages/common/PageHeader.tsx b/app/client/src/pages/common/PageHeader.tsx index cc4a61b2511e..9a7d63884be7 100644 --- a/app/client/src/pages/common/PageHeader.tsx +++ b/app/client/src/pages/common/PageHeader.tsx @@ -1,7 +1,10 @@ import React, { useEffect } from "react"; import { useRouteMatch } from "react-router-dom"; import { connect, useDispatch, useSelector } from "react-redux"; -import { getCurrentUser } from "selectors/usersSelectors"; +import { + getCurrentUser, + getShouldShowBaseUrlMissingBanner, +} from "selectors/usersSelectors"; import styled from "styled-components"; import StyledHeader from "components/designSystems/appsmith/StyledHeader"; import type { DefaultRootState } from "react-redux"; @@ -10,6 +13,7 @@ import { useIsMobileDevice } from "utils/hooks/useDeviceDetect"; import { getTemplateNotificationSeenAction } from "actions/templateActions"; import { shouldShowLicenseBanner } from "ee/selectors/organizationSelectors"; import { Banner } from "ee/utils/licenseHelpers"; +import BaseUrlMissingBanner from "components/editorComponents/BaseUrlMissingBanner"; import bootIntercom from "utils/bootIntercom"; import EntitySearchBar from "pages/common/SearchBar/EntitySearchBar"; @@ -61,14 +65,23 @@ export function PageHeader(props: PageHeaderProps) { const showBanner = useSelector(shouldShowLicenseBanner); const isHomePage = useRouteMatch("/applications")?.isExact; const isLicensePage = useRouteMatch("/license")?.isExact; + // GHSA-j9gf-vw2f-9hrw — fold the base-url-missing admin banner into the same + // isBannerVisible signal that the existing license/trial banner uses, so the + // page header gets pushed down by the banner's height (40px desktop / 70px + // mobile) when either banner is visible. Without this, the page header — which + // has a higher z-index — paints over the fixed-position banner at top: 0. + const showBaseUrlBanner = useSelector(getShouldShowBaseUrlMissingBanner); + const isAnyBannerVisible = + (showBanner && (isHomePage || isLicensePage)) || showBaseUrlBanner; return ( <> + From e9e92ce51fe8c3a39b7ca75bb51aa3330625c328 Mon Sep 17 00:00:00 2001 From: subrata71 Date: Tue, 5 May 2026 11:35:39 +0600 Subject: [PATCH 22/22] chore: remove superpowers planning docs from PR Remove design specs and implementation plans that were used during development but should not ship in the final PR. Refs GHSA-j9gf-vw2f-9hrw --- ...026-04-29-admin-base-url-warning-banner.md | 906 ------------------ .../2026-04-28-ci-base-url-config-design.md | 83 -- ...29-admin-base-url-warning-banner-design.md | 224 ----- 3 files changed, 1213 deletions(-) delete mode 100644 docs/superpowers/plans/2026-04-29-admin-base-url-warning-banner.md delete mode 100644 docs/superpowers/specs/2026-04-28-ci-base-url-config-design.md delete mode 100644 docs/superpowers/specs/2026-04-29-admin-base-url-warning-banner-design.md diff --git a/docs/superpowers/plans/2026-04-29-admin-base-url-warning-banner.md b/docs/superpowers/plans/2026-04-29-admin-base-url-warning-banner.md deleted file mode 100644 index 3b2f4841e794..000000000000 --- a/docs/superpowers/plans/2026-04-29-admin-base-url-warning-banner.md +++ /dev/null @@ -1,906 +0,0 @@ -# Admin Base-URL Warning Banner Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. - -**Goal:** Show a non-dismissible top-of-screen banner to instance super-users when `APPSMITH_BASE_URL` is unset and the resolver is in fail-closed mode for token-bearing email flows. Multi-org EE never sees it. - -**Architecture:** New reactive method on `SecureBaseUrlResolverCE` exposes the health signal. New boolean field on `UserProfileCE_DTO` carries it to the client via the existing `/v1/users/profile` call. New React component (`BaseUrlMissingBanner`) renders via `Callout` from `@appsmith/ads`, gated on `isSuperUser && adminSettingsVisible && instanceBaseUrlConfigurationHealthy === false`. EE override of the resolver short-circuits to healthy when `license_multi_org_enabled` is on. - -**Tech Stack:** Java 17 / Spring Boot Reactive (server), React + TypeScript + Redux (client), JUnit 5 + Reactor Test (server tests), Jest + React Testing Library (client tests). - -**Spec:** `docs/superpowers/specs/2026-04-29-admin-base-url-warning-banner-design.md` - -**Branch:** `feat/admin-base-url-warning-banner-ghsa-j9gf` (CE main repo at `/Users/subratadeypappu/IdeaProjects/appsmith`); EE work on the same branch name in EE main repo at `/Users/subratadeypappu/IdeaProjects/ee-appsmith` (created in Task 8). - ---- - -## File Structure - -### Server CE -- **Modify** `app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCE.java` — add interface method -- **Modify** `app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java` — implement method -- **Modify** `app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java` — add 2 unit cases -- **Modify** `app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/UserProfileCE_DTO.java` — add boolean field -- **Modify** `app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java` — wire resolver into `buildUserProfileDTO` - -### Client CE -- **Modify** `app/client/src/ce/constants/messages.ts` — 3 message keys -- **Modify** `app/client/src/selectors/usersSelectors.ts` — new selector with `=== false` guard -- **Create** `app/client/src/selectors/usersSelectors.test.ts` (or extend existing) — selector test -- **Create** `app/client/src/components/editorComponents/BaseUrlMissingBanner.tsx` — banner component -- **Create** `app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx` — Jest tests -- **Modify** `app/client/src/ce/AppRouter.tsx` — mount banner -- **Modify** `app/client/src/constants/userConstants.ts` (if profile type lives there) — extend type - -### Server EE -- **Modify** `app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/SecureBaseUrlResolverImpl.java` — override method -- **Create / Modify** `app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/SecureBaseUrlResolverImplTest.java` — 3 unit cases - ---- - -## Task 1: Server CE — `SecureBaseUrlResolverCE.isBaseUrlConfigurationHealthy()` - -**Files:** -- Modify: `app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCE.java` -- Modify: `app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java` -- Test: `app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java` - -- [ ] **Step 1: Write the failing tests** - -Append two test methods to `SecureBaseUrlResolverCEImplTest`: - -```java -@Test -void isBaseUrlConfigurationHealthy_returnsTrueWhenBaseUrlSet() { - SecureBaseUrlResolverCEImpl resolver = new SecureBaseUrlResolverCEImpl(); - ReflectionTestUtils.setField(resolver, "appsmithBaseUrl", "https://appsmith.example"); - StepVerifier.create(resolver.isBaseUrlConfigurationHealthy()) - .expectNext(true) - .verifyComplete(); -} - -@Test -void isBaseUrlConfigurationHealthy_returnsFalseWhenBaseUrlBlank() { - SecureBaseUrlResolverCEImpl resolver = new SecureBaseUrlResolverCEImpl(); - ReflectionTestUtils.setField(resolver, "appsmithBaseUrl", ""); - StepVerifier.create(resolver.isBaseUrlConfigurationHealthy()) - .expectNext(false) - .verifyComplete(); -} -``` - -(Add `import org.springframework.test.util.ReflectionTestUtils;` and `import reactor.test.StepVerifier;` if not already present.) - -- [ ] **Step 2: Run tests to verify they fail** - -```bash -cd /Users/subratadeypappu/IdeaProjects/appsmith/app/server -./mvnw -pl appsmith-server test -Dtest=SecureBaseUrlResolverCEImplTest -DfailIfNoTests=false -``` - -Expected: FAIL with "cannot find symbol method isBaseUrlConfigurationHealthy()" (compile error). - -- [ ] **Step 3: Add interface method** - -In `SecureBaseUrlResolverCE.java`, add: - -```java -/** - * Reports whether this instance is in a state where token-bearing email flows - * (forgot-password, email verification, invites) can generate links without - * depending on a request-time hint such as the Origin header. False = the - * resolver is in fail-closed mode and the admin should configure the canonical - * base URL. - */ -Mono isBaseUrlConfigurationHealthy(); -``` - -- [ ] **Step 4: Implement in `SecureBaseUrlResolverCEImpl`** - -Add method (paste after `resolveSecureBaseUrl`): - -```java -@Override -public Mono isBaseUrlConfigurationHealthy() { - return Mono.just(StringUtils.hasText(appsmithBaseUrl)); -} -``` - -- [ ] **Step 5: Run tests to verify they pass** - -Same command as Step 2. Expected: 2 new tests PASS, plus the existing 16 cases still PASS. - -- [ ] **Step 6: Commit** - -```bash -cd /Users/subratadeypappu/IdeaProjects/appsmith -git add app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCE.java \ - app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java \ - app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java -git commit -m "feat(security): add isBaseUrlConfigurationHealthy() to SecureBaseUrlResolverCE (GHSA-j9gf-vw2f-9hrw)" -``` - ---- - -## Task 2: Server CE — `UserProfileCE_DTO` field - -**Files:** -- Modify: `app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/UserProfileCE_DTO.java` - -- [ ] **Step 1: Add the field** - -Insert after `@JsonProperty("isIntercomConsentGiven")` block (line 40 area, alongside other instance/admin booleans): - -```java - @JsonProperty("instanceBaseUrlConfigurationHealthy") - boolean instanceBaseUrlConfigurationHealthy = true; -``` - -Default `true` so any DTO instance built outside the real assembler (test fixtures, mocks) doesn't false-positive the banner. - -- [ ] **Step 2: Verify the file compiles** - -```bash -cd /Users/subratadeypappu/IdeaProjects/appsmith/app/server -./mvnw -pl appsmith-server compile -q -``` - -Expected: BUILD SUCCESS. - -- [ ] **Step 3: Commit** - -```bash -git add app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/UserProfileCE_DTO.java -git commit -m "feat(security): add instanceBaseUrlConfigurationHealthy field to UserProfileCE_DTO (GHSA-j9gf-vw2f-9hrw)" -``` - ---- - -## Task 3: Server CE — wire resolver into `UserServiceCEImpl#buildUserProfileDTO` - -**Files:** -- Modify: `app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java` -- Test: `app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java` - -- [ ] **Step 1: Read the existing `buildUserProfileDTO` method** (~line 770) - -Open the file and study the current `Mono` chain. Identify the terminal `.map`/`.flatMap` step where the `UserProfileDTO` is finalised so the new field can be set there. - -- [ ] **Step 2: Write failing integration test** - -Add a new test in `UserServiceTest`: - -```java -@Test -@WithUserDetails("api_user") -void buildUserProfileDTO_includesBaseUrlConfigurationHealthy() { - User user = new User(); - user.setEmail("api_user"); - StepVerifier.create(userService.buildUserProfileDTO(user)) - .assertNext(dto -> { - // In test profile APPSMITH_BASE_URL is unset and the insecure flag is on, - // so the resolver returns false (the field reflects the resolver, not the flag). - // For test stability we just assert the field is present and is a boolean. - Object value = ReflectionTestUtils.getField(dto, "instanceBaseUrlConfigurationHealthy"); - assertThat(value).isInstanceOf(Boolean.class); - }) - .verifyComplete(); -} -``` - -(Imports: `org.springframework.test.util.ReflectionTestUtils`, `org.assertj.core.api.Assertions.assertThat`.) - -- [ ] **Step 3: Run test to verify it fails** - -```bash -./mvnw -pl appsmith-server test -Dtest=UserServiceTest#buildUserProfileDTO_includesBaseUrlConfigurationHealthy -DfailIfNoTests=false -``` - -Expected: FAIL — current DTO build doesn't set the field, default `true` is returned, but the test asserts type only so it'll pass. **Adjust:** change the assertion to `assertThat((Boolean) value).isFalse();` since the resolver in test profile (where `APPSMITH_BASE_URL` is unset) returns `false`. Re-run; should fail because the wiring isn't in place yet (field stays at the DTO default of `true`). - -- [ ] **Step 4: Inject the resolver field** - -In the field declarations of `UserServiceCEImpl` (where other dependencies are listed), add: - -```java -private final SecureBaseUrlResolverCE secureBaseUrlResolver; -``` - -In the constructor, append the parameter (preserving existing parameter order; add at the end if mutating order would risk EE-side conflicts): - -```java -SecureBaseUrlResolverCE secureBaseUrlResolver, -``` - -And in the constructor body: - -```java -this.secureBaseUrlResolver = secureBaseUrlResolver; -``` - -- [ ] **Step 5: Wire into the `buildUserProfileDTO` chain** - -In the terminal `.map` (or `.flatMap`) step that finalises the DTO, replace it with a `.zipWith` so the resolver call runs in parallel. The exact shape depends on the existing method body; pattern: - -```java -return existingMonoChain - .zipWith(secureBaseUrlResolver.isBaseUrlConfigurationHealthy().onErrorReturn(true)) - .map(tuple -> { - UserProfileDTO dto = tuple.getT1(); - dto.setInstanceBaseUrlConfigurationHealthy(tuple.getT2()); - return dto; - }); -``` - -The `.onErrorReturn(true)` defends against resolver Mono errors per the spec's "Error handling" section — a transient failure must not break login. - -- [ ] **Step 6: Run test to verify it passes** - -Same command as Step 3. Expected: PASS. - -- [ ] **Step 7: Run the broader UserServiceTest suite** - -```bash -./mvnw -pl appsmith-server test -Dtest=UserServiceTest -DfailIfNoTests=false -``` - -Expected: All cases PASS (no regression in the existing 30+ cases). - -- [ ] **Step 8: Commit** - -```bash -git add app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java \ - app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java -git commit -m "feat(security): wire SecureBaseUrlResolver into UserServiceCEImpl#buildUserProfileDTO (GHSA-j9gf-vw2f-9hrw)" -``` - ---- - -## Task 4: Client CE — message constants - -**Files:** -- Modify: `app/client/src/ce/constants/messages.ts` - -- [ ] **Step 1: Add three message constants** - -Find a logical section (near other admin/setup messages) and append: - -```ts -export const BASE_URL_MISSING_BANNER_TITLE = () => - "Email delivery is disabled on this instance"; - -export const BASE_URL_MISSING_BANNER_BODY = () => - "Forgot-password, email-verification, and invite emails will not be delivered until APPSMITH_BASE_URL is configured. Set it from Admin Settings to restore email-based flows."; - -export const BASE_URL_MISSING_BANNER_CTA = () => "Configure APPSMITH_BASE_URL"; -``` - -- [ ] **Step 2: Verify file parses (TypeScript)** - -```bash -cd /Users/subratadeypappu/IdeaProjects/appsmith/app/client -yarn tsc --noEmit -p tsconfig.json 2>&1 | grep -E "messages\.ts|error" | head -20 -``` - -Expected: no errors related to `messages.ts`. - -- [ ] **Step 3: Commit** - -```bash -git add app/client/src/ce/constants/messages.ts -git commit -m "feat(client): add admin base-url banner copy constants (GHSA-j9gf-vw2f-9hrw)" -``` - ---- - -## Task 5: Client CE — selector with `=== false` guard - -**Files:** -- Modify: `app/client/src/selectors/usersSelectors.ts` -- Test: `app/client/src/selectors/usersSelectors.test.ts` (create if missing) - -- [ ] **Step 1: Write the failing tests** - -Create or extend `app/client/src/selectors/usersSelectors.test.ts`: - -```ts -import { getShouldShowBaseUrlMissingBanner } from "./usersSelectors"; - -const baseState = (currentUser: object | null) => - ({ ui: { users: { currentUser } } } as never); - -describe("getShouldShowBaseUrlMissingBanner", () => { - it("returns true when super user, settings visible, and unhealthy", () => { - expect( - getShouldShowBaseUrlMissingBanner( - baseState({ - isSuperUser: true, - adminSettingsVisible: true, - instanceBaseUrlConfigurationHealthy: false, - }), - ), - ).toBe(true); - }); - - it("returns false when healthy", () => { - expect( - getShouldShowBaseUrlMissingBanner( - baseState({ - isSuperUser: true, - adminSettingsVisible: true, - instanceBaseUrlConfigurationHealthy: true, - }), - ), - ).toBe(false); - }); - - it("returns false when not super user", () => { - expect( - getShouldShowBaseUrlMissingBanner( - baseState({ - isSuperUser: false, - adminSettingsVisible: true, - instanceBaseUrlConfigurationHealthy: false, - }), - ), - ).toBe(false); - }); - - it("returns false when admin settings not visible", () => { - expect( - getShouldShowBaseUrlMissingBanner( - baseState({ - isSuperUser: true, - adminSettingsVisible: false, - instanceBaseUrlConfigurationHealthy: false, - }), - ), - ).toBe(false); - }); - - it("returns false when field is missing (rolling deploy safety)", () => { - expect( - getShouldShowBaseUrlMissingBanner( - baseState({ - isSuperUser: true, - adminSettingsVisible: true, - }), - ), - ).toBe(false); - }); - - it("returns false when currentUser is null", () => { - expect(getShouldShowBaseUrlMissingBanner(baseState(null))).toBe(false); - }); -}); -``` - -- [ ] **Step 2: Run tests to verify they fail** - -```bash -cd /Users/subratadeypappu/IdeaProjects/appsmith/app/client -yarn jest src/selectors/usersSelectors.test.ts -``` - -Expected: FAIL with "getShouldShowBaseUrlMissingBanner is not a function". - -- [ ] **Step 3: Add the selector** - -Append to `app/client/src/selectors/usersSelectors.ts`: - -```ts -export const getShouldShowBaseUrlMissingBanner = ( - state: AppState, -): boolean => { - const user = state.ui.users.currentUser; - return Boolean( - user?.isSuperUser && - user?.adminSettingsVisible && - user?.instanceBaseUrlConfigurationHealthy === false, - ); -}; -``` - -(Confirm `AppState` is already imported at the top of the file; if not, add `import type { AppState } from "ee/reducers";` matching the existing convention there.) - -- [ ] **Step 4: Run tests to verify they pass** - -Same command as Step 2. Expected: 6/6 PASS. - -- [ ] **Step 5: Extend the user-profile TypeScript shape** - -If the `currentUser` shape is declared (search by ripgrep for `isSuperUser` in `.ts`/`.tsx`), add the optional field: - -```ts -instanceBaseUrlConfigurationHealthy?: boolean; -``` - -If no explicit interface exists (it's inferred), the selector compiles via the `user?.instanceBaseUrlConfigurationHealthy === false` access without further work. - -- [ ] **Step 6: Commit** - -```bash -git add app/client/src/selectors/usersSelectors.ts app/client/src/selectors/usersSelectors.test.ts -# include the type sync file if you found one -git commit -m "feat(client): add getShouldShowBaseUrlMissingBanner selector (GHSA-j9gf-vw2f-9hrw)" -``` - ---- - -## Task 6: Client CE — `BaseUrlMissingBanner` component - -**Files:** -- Create: `app/client/src/components/editorComponents/BaseUrlMissingBanner.tsx` -- Create: `app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx` - -- [ ] **Step 1: Write the failing component tests** - -```tsx -import React from "react"; -import { render, screen } from "@testing-library/react"; -import { Provider } from "react-redux"; -import { createStore } from "redux"; -import { MemoryRouter } from "react-router-dom"; -import { ThemeProvider, lightTheme } from "@appsmith/ads-old"; -import BaseUrlMissingBanner from "./BaseUrlMissingBanner"; - -const renderWithStore = (currentUser: object | null) => { - const store = createStore(() => ({ - ui: { users: { currentUser } }, - })); - return render( - - - - - - - , - ); -}; - -describe("BaseUrlMissingBanner", () => { - it("renders for super user with unhealthy configuration", () => { - renderWithStore({ - isSuperUser: true, - adminSettingsVisible: true, - instanceBaseUrlConfigurationHealthy: false, - }); - expect(screen.getByText(/Email delivery is disabled/i)).toBeInTheDocument(); - }); - - it("does not render when configuration is healthy", () => { - const { container } = renderWithStore({ - isSuperUser: true, - adminSettingsVisible: true, - instanceBaseUrlConfigurationHealthy: true, - }); - expect(container.firstChild).toBeNull(); - }); - - it("does not render for non-super-user", () => { - const { container } = renderWithStore({ - isSuperUser: false, - adminSettingsVisible: true, - instanceBaseUrlConfigurationHealthy: false, - }); - expect(container.firstChild).toBeNull(); - }); - - it("does not render when admin settings hidden", () => { - const { container } = renderWithStore({ - isSuperUser: true, - adminSettingsVisible: false, - instanceBaseUrlConfigurationHealthy: false, - }); - expect(container.firstChild).toBeNull(); - }); - - it("does not render when field missing (rolling deploy)", () => { - const { container } = renderWithStore({ - isSuperUser: true, - adminSettingsVisible: true, - }); - expect(container.firstChild).toBeNull(); - }); -}); -``` - -- [ ] **Step 2: Run tests to verify they fail** - -```bash -yarn jest src/components/editorComponents/BaseUrlMissingBanner.test.tsx -``` - -Expected: FAIL with "Cannot find module './BaseUrlMissingBanner'". - -- [ ] **Step 3: Create the component** - -```tsx -import React from "react"; -import { useSelector } from "react-redux"; -import styled from "styled-components"; -import { Callout } from "@appsmith/ads"; -import { getShouldShowBaseUrlMissingBanner } from "selectors/usersSelectors"; -import { adminSettingsCategoryUrl } from "ee/RouteBuilder"; -import { SettingCategories } from "ee/pages/AdminSettings/config/types"; -import { - BASE_URL_MISSING_BANNER_BODY, - BASE_URL_MISSING_BANNER_CTA, - BASE_URL_MISSING_BANNER_TITLE, - createMessage, -} from "ee/constants/messages"; - -const BannerContainer = styled.div` - position: sticky; - top: 0; - left: 0; - right: 0; - width: 100%; - z-index: var(--ads-v2-z-index-9, 100); -`; - -const BaseUrlMissingBanner: React.FC = () => { - const shouldShow = useSelector(getShouldShowBaseUrlMissingBanner); - - if (!shouldShow) return null; - - return ( - - - {createMessage(BASE_URL_MISSING_BANNER_TITLE)} -
    - {createMessage(BASE_URL_MISSING_BANNER_BODY)} -     -     - ); -}; - -export default BaseUrlMissingBanner; -``` - -- [ ] **Step 4: Run tests to verify they pass** - -Same command as Step 2. Expected: 5/5 PASS. - -If the test fails because of `Callout` requiring additional theme/wiring, inspect the failure and either (a) use `data-testid` selectors instead of text, or (b) wrap the component test in the same provider wrappers used by `ProductAlertBanner.test.tsx` if that file exists; otherwise stick with the wrappers in Step 1. - -- [ ] **Step 5: Commit** - -```bash -git add app/client/src/components/editorComponents/BaseUrlMissingBanner.tsx \ - app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx -git commit -m "feat(client): add BaseUrlMissingBanner component (GHSA-j9gf-vw2f-9hrw)" -``` - ---- - -## Task 7: Client CE — mount banner in `AppRouter` - -**Files:** -- Modify: `app/client/src/ce/AppRouter.tsx` - -- [ ] **Step 1: Find the existing mount of ``** - -Search in `AppRouter.tsx` for `ProductAlertBanner`. The mount sits inside the authed-routes container (around line 195). - -- [ ] **Step 2: Import and mount the new banner above `AppHeader`** - -Add to imports: - -```tsx -import BaseUrlMissingBanner from "components/editorComponents/BaseUrlMissingBanner"; -``` - -In the JSX where `` and `` are rendered (around line 192-195), wrap them so the new banner sits above ``: - -```tsx -<> - - - - - - - -``` - -(Preserve the existing structure exactly — only add the `` line above ``. The fragment may already exist.) - -- [ ] **Step 3: Verify TypeScript compiles** - -```bash -yarn tsc --noEmit -p tsconfig.json 2>&1 | grep "AppRouter.tsx" | head -5 -``` - -Expected: no errors. - -- [ ] **Step 4: Smoke-render the affected file's tests if any** - -```bash -yarn jest src/ce/AppRouter --passWithNoTests -``` - -Expected: PASS or "no tests found". - -- [ ] **Step 5: Commit** - -```bash -git add app/client/src/ce/AppRouter.tsx -git commit -m "feat(client): mount BaseUrlMissingBanner above AppHeader (GHSA-j9gf-vw2f-9hrw)" -``` - ---- - -## Task 8: Server EE — multi-org-aware override - -**Files:** -- Switch repo: `/Users/subratadeypappu/IdeaProjects/ee-appsmith` (main repo, NOT a worktree — husky hook is broken in worktrees per `.security/ghsa-j9gf-vw2f-9hrw/state.md`) -- Modify: `app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/SecureBaseUrlResolverImpl.java` -- Test: `app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/SecureBaseUrlResolverImplTest.java` (create if missing) - -- [ ] **Step 1: Switch EE main repo to a new branch on this work** - -```bash -cd /Users/subratadeypappu/IdeaProjects/ee-appsmith -git stash push -u -m "WIP: parking SAST branch for banner work" -git checkout fix/origin-validation-email-links-ghsa-j9gf # base on the GHSA fix branch which has resolver scaffolding -git pull origin fix/origin-validation-email-links-ghsa-j9gf -git checkout -b feat/admin-base-url-warning-banner-ghsa-j9gf -``` - -- [ ] **Step 2: Cherry-pick the CE-only commits onto the EE branch (shadow PR pattern)** - -For each CE commit on `feat/admin-base-url-warning-banner-ghsa-j9gf` (Tasks 1-7), cherry-pick into EE: - -```bash -git -C /Users/subratadeypappu/IdeaProjects/ee-appsmith cherry-pick \ - $(git -C /Users/subratadeypappu/IdeaProjects/appsmith log --reverse --format=%H release..feat/admin-base-url-warning-banner-ghsa-j9gf -- app/server app/client) -``` - -If conflicts arise, resolve manually (most likely on `UserProfileCE_DTO.java` if EE has any divergent fields). For the spec/plan docs, skip them on the EE side — they only need to live in CE. - -- [ ] **Step 3: Write failing EE override tests** - -Create `app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/SecureBaseUrlResolverImplTest.java`: - -```java -package com.appsmith.server.helpers; - -import com.appsmith.external.enums.FeatureFlagEnum; -import com.appsmith.server.services.FeatureFlagService; -import com.appsmith.server.services.OrganizationService; -import org.junit.jupiter.api.Test; -import org.springframework.test.util.ReflectionTestUtils; -import reactor.core.publisher.Mono; -import reactor.test.StepVerifier; - -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; - -class SecureBaseUrlResolverImplTest { - - @Test - void isBaseUrlConfigurationHealthy_returnsTrueWhenMultiOrgEnabled() { - FeatureFlagService featureFlagService = mock(FeatureFlagService.class); - OrganizationService organizationService = mock(OrganizationService.class); - when(featureFlagService.check(eq(FeatureFlagEnum.license_multi_org_enabled))) - .thenReturn(Mono.just(true)); - - SecureBaseUrlResolverImpl resolver = - new SecureBaseUrlResolverImpl(featureFlagService, organizationService); - ReflectionTestUtils.setField(resolver, "appsmithBaseUrl", ""); - - StepVerifier.create(resolver.isBaseUrlConfigurationHealthy()) - .expectNext(true) - .verifyComplete(); - } - - @Test - void isBaseUrlConfigurationHealthy_delegatesWhenMultiOrgDisabledAndUrlSet() { - FeatureFlagService featureFlagService = mock(FeatureFlagService.class); - OrganizationService organizationService = mock(OrganizationService.class); - when(featureFlagService.check(eq(FeatureFlagEnum.license_multi_org_enabled))) - .thenReturn(Mono.just(false)); - - SecureBaseUrlResolverImpl resolver = - new SecureBaseUrlResolverImpl(featureFlagService, organizationService); - ReflectionTestUtils.setField(resolver, "appsmithBaseUrl", "https://appsmith.example"); - - StepVerifier.create(resolver.isBaseUrlConfigurationHealthy()) - .expectNext(true) - .verifyComplete(); - } - - @Test - void isBaseUrlConfigurationHealthy_delegatesWhenMultiOrgDisabledAndUrlUnset() { - FeatureFlagService featureFlagService = mock(FeatureFlagService.class); - OrganizationService organizationService = mock(OrganizationService.class); - when(featureFlagService.check(eq(FeatureFlagEnum.license_multi_org_enabled))) - .thenReturn(Mono.just(false)); - - SecureBaseUrlResolverImpl resolver = - new SecureBaseUrlResolverImpl(featureFlagService, organizationService); - ReflectionTestUtils.setField(resolver, "appsmithBaseUrl", ""); - - StepVerifier.create(resolver.isBaseUrlConfigurationHealthy()) - .expectNext(false) - .verifyComplete(); - } -} -``` - -- [ ] **Step 4: Run tests to verify they fail** - -```bash -cd /Users/subratadeypappu/IdeaProjects/ee-appsmith/app/server -./mvnw -pl appsmith-server test -Dtest=SecureBaseUrlResolverImplTest -DfailIfNoTests=false -``` - -Expected: FAIL — the override hasn't been added yet, so the EE class inherits the CE method which ignores multi-org. Test 1 fails (returns false instead of true). - -- [ ] **Step 5: Add the override to `SecureBaseUrlResolverImpl`** - -Append the new method (after `resolveSecureBaseUrl`): - -```java -@Override -public Mono isBaseUrlConfigurationHealthy() { - return featureFlagService - .check(FeatureFlagEnum.license_multi_org_enabled) - .flatMap(isMultiOrgEnabled -> Boolean.TRUE.equals(isMultiOrgEnabled) - ? Mono.just(true) - : super.isBaseUrlConfigurationHealthy()) - .onErrorResume(e -> { - log.warn( - "FeatureFlagService.check failed for license_multi_org_enabled; defaulting to single-org behavior: {}", - e.getMessage()); - return super.isBaseUrlConfigurationHealthy(); - }); -} -``` - -- [ ] **Step 6: Run tests to verify they pass** - -Same command as Step 4. Expected: 3/3 PASS. - -- [ ] **Step 7: Commit on the EE branch** - -```bash -cd /Users/subratadeypappu/IdeaProjects/ee-appsmith -git add app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/SecureBaseUrlResolverImpl.java \ - app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/SecureBaseUrlResolverImplTest.java -git commit -m "feat(security): EE multi-org override for isBaseUrlConfigurationHealthy (GHSA-j9gf-vw2f-9hrw)" -``` - -- [ ] **Step 8: Push EE branch** - -```bash -git push -u origin feat/admin-base-url-warning-banner-ghsa-j9gf -``` - ---- - -## Task 9: CE — sync test, push, open PRs - -- [ ] **Step 1: Verify CE→EE sync would apply cleanly (per AGENTS.md)** - -```bash -cd /Users/subratadeypappu/IdeaProjects/appsmith -git fetch community release -git checkout -b _sync-test-banner community/release -for sha in $(git log --reverse --format=%H release..feat/admin-base-url-warning-banner-ghsa-j9gf); do - git cherry-pick $sha || { echo "CONFLICT on $sha"; break; } -done -git checkout feat/admin-base-url-warning-banner-ghsa-j9gf -git branch -D _sync-test-banner -``` - -Expected: All commits cherry-pick cleanly. Any conflict means a CE file diverges from `community/release` — investigate and resolve before opening the PR. - -- [ ] **Step 2: Push CE branch** - -```bash -git push -u origin feat/admin-base-url-warning-banner-ghsa-j9gf -``` - -- [ ] **Step 3: Open CE PR** - -```bash -gh pr create --repo appsmithorg/appsmith \ - --base release \ - --title "feat(security): admin warning banner for unset APPSMITH_BASE_URL (GHSA-j9gf-vw2f-9hrw)" \ - --body "$(cat <<'EOF' -## Summary - -Companion to PR #41766 (the GHSA-j9gf-vw2f-9hrw fail-closed fix). - -Adds a non-dismissible top-of-screen banner shown only to instance super-users when `APPSMITH_BASE_URL` is unset and the resolver is therefore in fail-closed mode for token-bearing email flows. Multi-org EE deployments (e.g. Appsmith Cloud) **never** see the banner — verified via the `license_multi_org_enabled` short-circuit in the EE resolver override (shadow EE PR linked below). - -The banner deep-links to Admin Settings → Configuration where `APPSMITH_BASE_URL` is the registered field. Saving via the existing Configuration-tier admin-settings flow restarts the server, the SPA auto-reloads, the resolver re-reads the env, and the banner clears — no re-login. - -## Design - -`docs/superpowers/specs/2026-04-29-admin-base-url-warning-banner-design.md` - -## Plan - -`docs/superpowers/plans/2026-04-29-admin-base-url-warning-banner.md` - -## Test plan - -- [x] `SecureBaseUrlResolverCEImplTest` — 2 new cases for `isBaseUrlConfigurationHealthy()` -- [x] `UserServiceTest` — 1 new case asserting field is wired into `buildUserProfileDTO` -- [x] `usersSelectors.test.ts` — 6 cases including rolling-deploy `=== false` guard -- [x] `BaseUrlMissingBanner.test.tsx` — 5 cases covering all gating dimensions -- Cypress: skipped — CI sets `APPSMITH_BASE_URL=http://localhost`, banner never fires by construction - -## Companion PRs / refs - -- Shadow EE PR: -- Security advisory: https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw -- Linear: APP-15046 -EOF -)" -``` - -- [ ] **Step 4: Open shadow EE PR** - -```bash -cd /Users/subratadeypappu/IdeaProjects/ee-appsmith -gh pr create --repo appsmithorg/appsmith-ee \ - --base release \ - --title "feat(security): admin warning banner for unset APPSMITH_BASE_URL (GHSA-j9gf-vw2f-9hrw)" \ - --body "$(cat <<'EOF' -## Summary - -Shadow EE PR for CE PR #. Mirrors the CE work and adds the EE-only `SecureBaseUrlResolverImpl` override for `isBaseUrlConfigurationHealthy()` that short-circuits to `Mono.just(true)` when `license_multi_org_enabled` is on (so multi-org EE deployments — Appsmith Cloud — never display the banner). - -## Merge order - -1. CE PR merges first -2. CE→EE hourly sync brings the CE files into EE (cleanly — sync test passed before opening the CE PR) -3. This shadow EE PR merges on top with a merge commit (NOT squash — squashing breaks the next sync) - -## Companion PRs / refs - -- CE PR: -- Security advisory: https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw -- Linear: APP-15046 -EOF -)" -``` - -- [ ] **Step 5: Add shared labels** - -```bash -gh pr edit --repo appsmithorg/appsmith --add-label security -gh pr edit --repo appsmithorg/appsmith-ee --add-label security -``` - -- [ ] **Step 6: Restore EE main repo to its prior state** - -```bash -cd /Users/subratadeypappu/IdeaProjects/ee-appsmith -git checkout feature/app-15191-sast-semgrep-integration -git stash pop -``` - ---- - -## Self-Review Notes - -- Spec coverage: every section in the spec ("Architecture", "Multi-org", "Error handling", "Edge cases", "Testing", "Out of scope") is anchored to a specific task above (1-9). -- No placeholders: every code block contains executable code or executable commands. PR-body URLs are deliberately marked `` because they're computed at PR-open time. -- Type consistency: `instanceBaseUrlConfigurationHealthy` (Java + TS), `isBaseUrlConfigurationHealthy()` (Java method), `getShouldShowBaseUrlMissingBanner` (TS selector), `BaseUrlMissingBanner` (component) — all consistent across tasks. diff --git a/docs/superpowers/specs/2026-04-28-ci-base-url-config-design.md b/docs/superpowers/specs/2026-04-28-ci-base-url-config-design.md deleted file mode 100644 index 7f3161451c1c..000000000000 --- a/docs/superpowers/specs/2026-04-28-ci-base-url-config-design.md +++ /dev/null @@ -1,83 +0,0 @@ -# CI Configuration for `APPSMITH_BASE_URL` (GHSA-j9gf-vw2f-9hrw follow-up) - -**Date:** 2026-04-28 -**Author:** subrata71 -**Companion to:** [PR #41766](https://github.com/appsmithorg/appsmith/pull/41766) (CE), [PR #8997](https://github.com/appsmithorg/appsmith-ee/pull/8997) (EE) -**Linear:** [APP-15046](https://linear.app/appsmith/issue/APP-15046) - -## Context - -The fix for [GHSA-j9gf-vw2f-9hrw](https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw) makes the server fail-closed for token-bearing email flows (forgot-password, email verification, workspace invite, instance-admin invite) whenever `APPSMITH_BASE_URL` is unset. The Cypress E2E suite in `ci-test-*` workflows spins up a fresh Appsmith Docker container that does not set `APPSMITH_BASE_URL`, so every spec exercising those flows now fails with `MISCONFIGURED_INSTANCE_BASE_URL`. - -Concrete failures observed on the [first PR run](https://github.com/appsmithorg/appsmith/actions/runs/25035460641): `Email_settings_Spec.ts`, `ExportApplication_spec.js`, `DeleteWorkspace_spec.ts`, `LeaveWorkspaceTest_spec.js`, `MemberRoles_Spec.ts`, `ShareAppTests_Spec.ts`. All emit the same client-side error: `APPSMITH_BASE_URL is not configured. Token-bearing email flows are disabled until the canonical instance URL is set.` - -The unit/integration test suite was already addressed in commit `35eb077e58` by setting `APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS=true` in `application-test.properties`. That fix does **not** apply to Cypress, which runs against the production profile of a deployed Docker container. - -## Decision - -**Approach A: configure `APPSMITH_BASE_URL` at container startup in every CI environment that runs Cypress.** Treat CI as a real deployment that satisfies the new contract. Do **not** use the migration flag (`APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS`) in CI — we want E2E coverage of the new secure path. - -Rationale: -- Production-realistic. CI becomes the canonical example of how self-hosted operators should configure their instance. -- Exercises the new strict-mode `Origin == APPSMITH_BASE_URL` check end-to-end. -- One-time small workflow change (~5 files). -- The fail-closed semantics introduced by the GHSA fix are still pinned by `SecureBaseUrlResolverCEImplTest` unit tests, so the test pyramid stays balanced. - -Approach B (`APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS=true` in CI) was rejected because it would never exercise the secure path E2E and would spam every CI log with the helper's WARN message. - -## Files to change - -### CI workflows (5 lines each, identical pattern) - -For each of these workflows, add `-e APPSMITH_BASE_URL=http://localhost \` to the `docker run -d --name appsmith` block: - -| File | Approximate line of `docker run --name appsmith` | -|------|--------------------------------------------------| -| `.github/workflows/ci-test-limited.yml` | 182 | -| `.github/workflows/ci-test-limited-with-count.yml` | 271 | -| `.github/workflows/ci-test-custom-script.yml` | 184 | -| `.github/workflows/ci-test-playwright.yml` | 169 | - -The container exposes port 80 → host port 80, and Cypress hits `http://localhost`. Browsers omit `:80` from the `Origin` header for default-port URLs, so `APPSMITH_BASE_URL=http://localhost` matches exactly. - -### Deploy preview (Helm) - -`scripts/deploy_preview.sh` (around line 117) — add to the `helm upgrade` arg list: - -``` ---set applicationConfig.APPSMITH_BASE_URL=https://$DOMAINNAME -``` - -`$DOMAINNAME` is already computed on line 22 as `$edition-$PULL_REQUEST_NUMBER.dp.appsmith.com`. The Helm chart already propagates `applicationConfig.*` to container env (existing pattern: `APPSMITH_DB_URL`, `APPSMITH_SENTRY_DSN`, …). - -### Local developer parity (optional) - -`scripts/local_testing.sh` (line 124) — add `-e APPSMITH_BASE_URL=http://localhost \` to the local docker-run helper. This keeps developer-local Docker behavior consistent with CI. Skip if you'd rather minimize blast radius. - -## Files explicitly NOT changed - -- `.github/workflows/ci-test-hosted.yml` — runs against an externally-managed hosted Appsmith instance (not Docker). The hosted instance's configuration is owned by ops; the workflow itself doesn't `docker run` Appsmith. -- `.github/workflows/server-build.yml`, `server-integration-tests.yml` — server unit/integration tests via Maven; already fixed via `application-test.properties`. -- `.github/workflows/build-docker-image.yml`, `test-build-docker-image.yml` — they orchestrate but do not themselves `docker run` Appsmith. They invoke the child workflows above. -- The Docker image / Dockerfile itself — must NOT bake in any default `APPSMITH_BASE_URL` value. Production deployments need their own canonical URL; baking a default would defeat the security improvement. - -## Risks - -1. **Origin string mismatch.** Strict-mode requires exact equality. Cypress sends `Origin: http://localhost` for default-port traffic to `http://localhost`. Verified by reading the existing `Email_settings_Spec.ts` test 3 (lines 172-181) which already strips trailing slashes from `originUrl` before forwarding — confirming the team has thought about this surface. **Mitigation:** if a spec passes a different Origin, we'll see `Origin header does not match APPSMITH_BASE_URL configuration` (HTTP 400) — clear and actionable, not a silent failure. - -2. **Helm chart key naming.** `applicationConfig.APPSMITH_BASE_URL` — confirmed pattern matches existing entries. **Mitigation:** the existing deploy preview will surface any mistake on first run. - -3. **Rollback.** If anything misbehaves, removing the env vars from these files restores prior CI behavior. Production fail-closed default in the server is unaffected. - -## Validation - -1. Re-trigger the failing CI run on PR #41766. All previously-failing Cypress specs should pass. -2. Re-trigger a deploy preview build to verify `APPSMITH_BASE_URL` propagates correctly via Helm. -3. Smoke-test by running one Cypress spec locally with `APPSMITH_BASE_URL=http://localhost` set on a fresh container. - -## Out of scope - -- Adding a Cypress `before` hook to set `APPSMITH_BASE_URL` via the admin settings UI — not needed since we set it at container startup. -- Cypress test code changes — none needed. -- Changes to the Docker image (Dockerfile / base.dockerfile). -- Documenting the new requirement in user-facing operator docs — separate doc PR after release. diff --git a/docs/superpowers/specs/2026-04-29-admin-base-url-warning-banner-design.md b/docs/superpowers/specs/2026-04-29-admin-base-url-warning-banner-design.md deleted file mode 100644 index 9d255c543ddc..000000000000 --- a/docs/superpowers/specs/2026-04-29-admin-base-url-warning-banner-design.md +++ /dev/null @@ -1,224 +0,0 @@ -# Admin Warning Banner for Unset `APPSMITH_BASE_URL` (GHSA-j9gf-vw2f-9hrw follow-up) - -**Date:** 2026-04-29 -**Author:** subrata71 -**Companion to:** [PR #41766](https://github.com/appsmithorg/appsmith/pull/41766) (CE), [PR #8997](https://github.com/appsmithorg/appsmith-ee/pull/8997) (EE) — the GHSA-j9gf-vw2f-9hrw fail-closed fix -**Linear:** [APP-15046](https://linear.app/appsmith/issue/APP-15046) - -## Context - -The fix for [GHSA-j9gf-vw2f-9hrw](https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw) makes the server fail-closed for token-bearing email flows (forgot-password, email verification, workspace invite, instance-admin invite) whenever `APPSMITH_BASE_URL` is unset. That is the security-correct default, but operationally it creates a silent-degradation surface: a self-hosted instance admin who upgrades without setting `APPSMITH_BASE_URL` will not realise their instance is misconfigured until end-users complain that password-reset and verification emails never arrive. - -The reporter's own recommendation (verbatim, item 3 in the GHSA discussion) was: - -> "If APPSMITH_BASE_URL is unset, fail closed in the email-generation path. ... Emit clear server log + **admin warning** + health/configuration signal that token-bearing email flows are disabled until APPSMITH_BASE_URL is configured." - -The server-log piece already exists (a WARN log on every resolver call). This document covers the **admin warning** piece — an in-product banner shown only to instance admins when the server is in this fail-closed state. - -## Goal - -Show a non-dismissible warning banner at the top of the application to logged-in instance admins (super users with admin-settings access) **only** when: - -- The instance is in CE, OR -- The instance is in EE with the `license_multi_org_enabled` feature flag **off**, AND -- `APPSMITH_BASE_URL` is unset. - -The banner explains that token-bearing email flows are disabled and deep-links to Admin Settings → Configuration where the admin can set the value. Saving the value triggers Appsmith's existing Configuration-tier server-restart flow, which auto-reloads the SPA and the banner disappears (because the resolver re-reads the env on startup). - -## Non-goals (v1) - -- **Insecure-flag-on warning.** When `APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS=true`, the banner does **not** fire even though the deployment is insecure. That is a separate decision with different copy and stronger language; deferred. -- **EE multi-org.** Multi-org EE instances (Appsmith Cloud) derive their email-link host from `organizationService.getOrganizationBaseUrl()` (slug + deploymentDomain) and do not depend on `APPSMITH_BASE_URL`. The banner must **never** appear there. -- **Per-user dismissal / snooze / reminder cadence.** The banner reflects live server state. The misconfig itself is the source of truth — clear the misconfig, the banner clears. -- **Audit log entry** when banner displays. -- **Severity escalation** if the admin keeps ignoring the banner. - -## Architecture - -### Signal source — extend the existing resolver - -`SecureBaseUrlResolverCE` already centralises the question "what canonical host should this instance use for email links?". It is the natural place to ask "is that question well-formed for this instance?". Add one new reactive method: - -```java -Mono isBaseUrlConfigurationHealthy(); -``` - -Semantics: *"can this instance generate token-bearing email links without depending on a request-time hint?"* True ⇒ no banner. False ⇒ banner. - -| Implementation | Returns | -|---|---| -| `SecureBaseUrlResolverCEImpl` (CE) | `Mono.just(StringUtils.hasText(appsmithBaseUrl))` | -| `SecureBaseUrlResolverImpl` (EE override) | If `featureFlagService.check(license_multi_org_enabled)` is true → `Mono.just(true)`; else → `super.isBaseUrlConfigurationHealthy()` | - -The EE override mirrors the same `featureFlagService.check(...).flatMap(...)` shape that already exists for `resolveSecureBaseUrl`, so the two methods stay symmetrical. - -The method is reactive because the EE override needs the feature flag service. The DTO assembler (`UserServiceCEImpl#buildUserProfile`) is already a reactive chain, so this folds in via a single `.zipWith(...)` step. - -### Signal pathway — ride on `UserProfileCE_DTO` - -`UserProfileCE_DTO` already carries instance-state signals (`isEmptyInstance`, `isSuperUser`, `adminSettingsVisible`) that the React client uses for gating. Add one boolean field: - -```java -@JsonProperty("instanceBaseUrlConfigurationHealthy") -boolean instanceBaseUrlConfigurationHealthy = true; -``` - -Default `true` — any DTO instance that bypasses the real assembler (test fixtures, mock contexts, future code paths) behaves as "configured" and does not false-positive the banner. - -`UserServiceCEImpl#buildUserProfileDTO(User)` injects the resolver and calls `isBaseUrlConfigurationHealthy()` as part of the existing reactive build chain. No new endpoint, no new saga, no new Redux state. - -### Client — dedicated banner component - -A new component `BaseUrlMissingBanner.tsx` lives at `app/client/src/components/editorComponents/`. It uses `Callout` from `@appsmith/ads` (`kind="warning"`), is rendered at the top of the application above `AppHeader`, and is **not user-dismissible** (no close button, no `isClosable`). The component returns `null` when the gating selector says so, so it costs nothing for non-admin users. - -Layout: rendered as a regular block element at the top of the authed-routes container in `AppRouter.tsx`. Pushes everything below it down by its own height naturally — no body class, no manual offset, no CSS hack. Existing `` (bottom-of-screen) coexists fine. - -Selector in `selectors/usersSelectors.ts`: - -```ts -const getShouldShowBaseUrlMissingBanner = (state: AppState): boolean => { - const user = state.ui.users.currentUser; - return Boolean( - user?.isSuperUser && - user?.adminSettingsVisible && - user?.instanceBaseUrlConfigurationHealthy === false // explicit !== !value - ); -}; -``` - -The `=== false` check (rather than `!value`) is deliberate: during a rolling deploy where a newer client briefly sees an older server's response without the field, `undefined === false` is `false`, so the banner stays hidden. Once both sides redeploy, the explicit `false` flips it on. - -### CTA — deep-link to the existing Admin Settings field - -`APPSMITH_BASE_URL` is already a registered setting in the Admin Settings → Configuration tab (`SettingCategories.CONFIGURATION` / `CategoryType.INSTANCE`, defined at `app/client/src/ce/pages/AdminSettings/config/configuration.tsx`). The banner has one `CalloutLink` whose `to` is built via the existing helper: - -```ts -adminSettingsCategoryUrl({ category: SettingCategories.CONFIGURATION }) -// → /settings/configuration -``` - -Same pattern as `AuthPage.tsx` already uses for its own admin-settings deep links. - -### Recovery loop (no extra code) - -Existing Configuration-tier admin-settings save flow does this naturally — verified by reading `app/client/src/ce/sagas/SuperUserSagas.tsx` (`SaveAdminSettingsSaga` → `RESTART_SERVER_POLL` → `RestartServerPoll` → `RestryRestartServerPoll` → `window.location.reload()`): - -``` -Admin clicks banner CTA - → /settings/configuration - → enters APPSMITH_BASE_URL value, clicks Save - → SAVE_ADMIN_SETTINGS_SUCCESS → RESTART_SERVER_POLL - → server restarts (Configuration-tier setting → needsRestart = true) - → client polls fetchCurrentOrganizationConfig until migrationStatus === COMPLETED - → window.location.reload() - → Redis-backed session survives → no login redirect - → fresh /v1/users/profile → instanceBaseUrlConfigurationHealthy: true - → selector returns false → banner gone -``` - -The admin sees only the same "Server is restarting…" UX they already know from changing any other Configuration-tier setting (Redis URL, DB URL, etc.). No login screen, no manual refresh. - -## Files to change - -### Server CE (4 files) - -| File | Change | -|---|---| -| `app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCE.java` | Add `Mono isBaseUrlConfigurationHealthy();` to the interface. | -| `app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java` | Implement: `return Mono.just(StringUtils.hasText(appsmithBaseUrl));` | -| `app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/UserProfileCE_DTO.java` | Add `boolean instanceBaseUrlConfigurationHealthy = true;` with `@JsonProperty`. | -| `app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java` | Inject `SecureBaseUrlResolverCE` and `.zipWith(...)` the resolver call into the existing `buildUserProfileDTO(User)` reactive chain. | - -### Server EE (1 file) - -| File | Change | -|---|---| -| `app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/SecureBaseUrlResolverImpl.java` | Override `isBaseUrlConfigurationHealthy()` with the multi-org gate (mirror the existing `resolveSecureBaseUrl` shape). | - -### Client CE (5 files) - -| File | Change | -|---|---| -| `app/client/src/components/editorComponents/BaseUrlMissingBanner.tsx` (new) | New component. `Callout kind="warning"`, copy + CTA, returns `null` when gating selector is false. Not closable. | -| `app/client/src/selectors/usersSelectors.ts` | Add `getShouldShowBaseUrlMissingBanner` selector with explicit `=== false` guard. | -| `app/client/src/ce/constants/messages.ts` | Three new message keys: title, body, CTA label. | -| `app/client/src/ce/AppRouter.tsx` | Mount `` as the first child of the authed-routes container, above ``. | -| `app/client/src/reducers/uiReducers/usersReducer.ts` (and any inferred user-profile TypeScript type) | Extend the user shape with the new optional `instanceBaseUrlConfigurationHealthy?: boolean`. | - -### Client EE (0 files) - -`UserProfileDTO extends UserProfileCE_DTO`, so the field flows through. `ee/AppRouter.tsx` extends/inherits `ce/AppRouter.tsx`'s structure — verify during implementation that the mount is picked up; if EE overrides the relevant region, mirror the mount in `ee/AppRouter.tsx`. - -## Multi-org behavior - -This is the most important EE-specific concern, so calling it out explicitly: - -| Deployment | Banner ever shows? | Why | -|---|---|---| -| CE (any), `APPSMITH_BASE_URL` set | No | `SecureBaseUrlResolverCEImpl#isBaseUrlConfigurationHealthy()` returns `true` | -| CE (any), `APPSMITH_BASE_URL` unset | Yes (super users only) | Resolver returns `false` | -| EE single-org, `APPSMITH_BASE_URL` set | No | EE override: multi-org off → delegate → CE returns `true` | -| EE single-org, `APPSMITH_BASE_URL` unset | Yes (super users only) | EE override: multi-org off → delegate → CE returns `false` | -| EE multi-org (Appsmith Cloud) | **Never** | EE override short-circuits to `Mono.just(true)` regardless of `APPSMITH_BASE_URL` | - -## Error handling - -Three failure modes worth nailing down: - -1. **`featureFlagService.check(...)` errors out** (FF service unreachable, DB blip). EE override defaults to "treat as multi-org off" via `.onErrorResume(e -> super.isBaseUrlConfigurationHealthy())`. Rationale: a false-positive banner is recoverable (admin sees a misleading warning, can ignore); a false-negative would silently leave a real misconfig invisible. Logged at WARN. -2. **Resolver `Mono` errors during DTO assembly.** The DTO assembler wraps the call with `.onErrorReturn(true)`. Login must never break because of a banner-signal error. Logged at ERROR (this would be a real bug). Net result: transient blip → no banner → admin keeps working → next session retries. -3. **Field missing from response** (rolling deploy). The TypeScript field is declared optional. The selector explicitly checks `=== false`, so `undefined` → no banner. Standard tri-state guard for new optional fields. - -## Edge cases (all by-construction, no extra code) - -| Case | Behavior | -|---|---| -| Anonymous user / logged out | `isSuperUser === false` → no banner | -| Logged-in non-admin | `isSuperUser === false` → no banner | -| Super user but `adminSettingsVisible === false` (RBAC, license tier) | No banner — CTA would be useless | -| EE multi-org enabled (Cloud) | Resolver short-circuits → no banner | -| Admin viewing `/settings/configuration` already | Banner still shows — actually helpful, contextually reinforces *which* field to set | -| Admin saves from banner CTA | Existing Configuration-tier save → server restart → auto page-reload → banner clears. **No re-login.** | -| Admin in Tab A, saves from Tab B | Tab B reloads itself (existing flow); Tab A keeps showing stale banner until manual refresh. Acceptable — matches every other admin-setting in Appsmith today. | -| Admin sets value via CLI / direct env, restarts container | No restart-poll triggered in any open tab. Banner persists in open tabs until manual refresh. Same as any Configuration-tier setting. | -| EE multi-org licence flips off mid-session | License changes restart the server anyway — natural fresh session, banner appears (correctly) on next login. | -| Two browser tabs open as same admin | Both show banner; both clear together when admin saves and the SPA reloads. | - -## Testing - -| Layer | What | Why | -|---|---|---| -| **Server unit (CE)** | `SecureBaseUrlResolverCEImplTest` adds 2 cases for `isBaseUrlConfigurationHealthy()` — `true` when `APPSMITH_BASE_URL` set, `false` when blank. | Pins CE semantics. No Spring context, fast, runs on every PR. | -| **Server unit (EE)** | New `SecureBaseUrlResolverImplTest` (or extend an existing one) with 3 cases: multi-org on → `true` regardless of BASE_URL; multi-org off + BASE_URL set → `true`; multi-org off + BASE_URL unset → `false`. Mocks `featureFlagService.check(...)`. | Pins the multi-org override — the only place this branching is exercised. | -| **Server integration** | `UserServiceTest` — one new case under the `buildUserProfileDTO` coverage asserting the new field is present and reflects the resolver's answer. | End-to-end through the DTO assembler so we don't regress the `.zipWith` wiring. | -| **Client unit (Jest)** | `BaseUrlMissingBanner.test.tsx` — five cases: `isSuperUser=true, adminSettingsVisible=true, healthy=false` → renders; `healthy=true` → null; `isSuperUser=false` → null; `adminSettingsVisible=false` → null; field undefined → null. | Pins selector + render gating. Fast, deterministic, runs on every client PR. | -| **Client unit (selector)** | `usersSelectors.test.ts` — one case for the explicit-`=== false` guard (undefined / missing → false). | Pins the rolling-deploy safety. | -| **Cypress** | **Skip.** | CI sets `APPSMITH_BASE_URL=http://localhost`, so the banner never fires in CI by construction. Writing a Cypress test that mutates `/admin/env` to clear it mid-run, restarts the server, and asserts the banner — high cost, low yield. The Jest cases cover rendering; the unit tests cover the resolver. | - -## Risks - -1. **Banner pushes layout down ~40-48px.** Existing Cypress specs that use absolute pixel coordinates against the AppHeader could break. Mitigation: most existing specs use semantic selectors (text, `data-cy`); the banner is hidden in CI anyway because `APPSMITH_BASE_URL=http://localhost` is set in every CI workflow as of [`dd8fbedea8`](https://github.com/appsmithorg/appsmith/commit/dd8fbedea8). Risk is essentially zero in CI. -2. **Field name on the DTO.** Long but semantically accurate. Trivial rename if the team prefers shorter (`baseUrlHealthy`, `baseUrlConfigured`) — single-find-replace plus the TypeScript shape. -3. **EE override path.** The override needs `featureFlagService.check(...)` injected. If we ever introduce other resolver methods that need similar gating, refactor the multi-org branch into a private helper. Not needed for v1. - -## Files explicitly NOT changed - -- `app/client/src/components/editorComponents/ProductAlertBanner.tsx` — unrelated; covers ephemeral product news with per-message dismiss/snooze tracking. Wrong abstraction for "fix-the-config-and-it-stops" misconfig signal. -- The reducer/saga/action layer — no new Redux state because the field is on the existing `currentUser` slice that's already loaded. -- The HTTP layer — no new endpoint; signal rides on `/v1/users/profile` which already runs at session start. -- The Docker image / Dockerfile — must NOT bake in any default `APPSMITH_BASE_URL` value. Production deployments need their own canonical URL. - -## Validation - -1. CE branch builds and unit tests pass locally. -2. Manual smoke (CE): start a fresh container without `APPSMITH_BASE_URL`, log in as super user → banner visible. Set the value via Admin Settings → save → server restart → SPA auto-reload → banner gone, no login redirect. -3. Manual smoke (CE): same flow logged in as non-admin → banner not visible. -4. Manual smoke (EE single-org): same as CE. -5. Manual smoke (EE multi-org): banner not visible regardless of `APPSMITH_BASE_URL` value. -6. Sync test (per AGENTS.md): `git fetch community release` → branch from `community/release` → cherry-pick the CE commits → confirm clean apply (the only EE-specific addition is the resolver override which has no overlap with the CE files). - -## Out of scope - -- Documenting the new `instanceBaseUrlConfigurationHealthy` field in any external API doc — internal DTO field, not part of the public REST contract. -- Translating the banner copy into other locales (Appsmith does not yet have a runtime i18n layer; copy uses the existing `messages.ts` mechanism). -- Server-side enforcement that admins cannot dismiss the banner (the banner is non-dismissible client-side; an attacker mutating their local Redux state to hide it just hides it for themselves, no security impact).