fix(security): fail closed when APPSMITH_BASE_URL unset for token-bearing emails (GHSA-j9gf-vw2f-9hrw)#41766
Closed
fix(security): fail closed when APPSMITH_BASE_URL unset for token-bearing emails (GHSA-j9gf-vw2f-9hrw)#41766
Conversation
…f-vw2f-9hrw) 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
…ring emails (GHSA-j9gf-vw2f-9hrw) 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
subrata71
added
Security
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a secure base-URL resolution subsystem and CE implementation, wires it into email and user services, updates constructors and flows to enforce fail‑closed origin checks, adds tests, and updates CI/deploy/local scripts and docs to provide APPSMITH_BASE_URL for test/runtime parity. Changes
Sequence DiagramsequenceDiagram
participant Client
participant EmailService as EmailServiceCEImpl
participant Resolver as SecureBaseUrlResolver
participant Config as APPSMITH_BASE_URL
participant Logger as Logger
Client->>EmailService: start invite flow (originHeader)
EmailService->>Resolver: resolveSecureBaseUrl(originHeader)
alt APPSMITH_BASE_URL is set
Resolver->>Config: read APPSMITH_BASE_URL
alt originHeader == APPSMITH_BASE_URL
Resolver-->>EmailService: trustedBaseUrl
EmailService->>EmailService: build invite URL with trustedBaseUrl
EmailService-->>Client: send email
else originHeader != APPSMITH_BASE_URL
Resolver-->>Logger: emit AppsmithException (GENERIC_BAD_REQUEST)
Logger-->>Client: reject / fail
end
else APPSMITH_BASE_URL unset
alt APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS = true
Resolver-->>EmailService: emit originHeader (warn)
EmailService->>EmailService: build invite URL with originHeader
EmailService-->>Client: send email
else
Resolver-->>Logger: warn & return empty
Logger-->>Client: no email sent (silent success)
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
|
Shadow EE PR: https://github.com/appsmithorg/appsmith-ee/pull/8997 (pre-staged for sync resolution) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java (1)
197-207: Add a boundary test for the empty-completion path.These branches now rely on
Mono.empty()as the security-critical “generic success, no email sent” outcome. I’d add a service/controller-level assertion for that path so future refactors don’t accidentally turn it into a different HTTP response or dispatch emails again.Also applies to: 843-852
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java` around lines 197 - 207, Add a boundary test that asserts the empty-completion path from secureBaseUrlResolver.resolveSecureBaseUrl(...) produces the generic success response and does not send an email: mock secureBaseUrlResolver to return Mono.empty() for resetUserPasswordDTO.getBaseUrl(), call the public entry point that uses processForgotPasswordTokenGeneration (the service or controller method that consumes resetUserPasswordDTO), and assert the HTTP/Service response equals the generic success value and that the email-sending component was never invoked; repeat the same kind of test for the other occurrence around processForgotPasswordTokenGeneration referenced in the second region so future refactors cannot convert Mono.empty() into a different response or trigger email dispatch.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java (1)
69-72: Consider rate-limiting this warning.This branch is reachable from unauthenticated flows, so a misconfigured instance can be driven into unbounded WARN spam. A rate-limited log or startup health signal would be safer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java` around lines 69 - 72, In SecureBaseUrlResolverCEImpl, avoid unbounded warn spam from the log.warn(...) branch that returns Mono.empty(); implement rate-limiting so the warning is emitted at most once per configured interval (e.g., once per hour). For example, add a static/instance time-based guard (AtomicLong lastWarnAt or a Guava RateLimiter) checked before calling log.warn(...) and update the timestamp when the warn is emitted; keep the method behavior returning Mono.empty() unchanged. Ensure the guard is thread-safe and configurable, and document the interval constant near the class-level fields so future maintainers can adjust it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java`:
- Line 67: In AppsmithErrorCode, MISCONFIGURED_INSTANCE_BASE_URL currently
reuses the code string "AE-APP-5045" which collides with
FEATURE_FLAG_MIGRATION_FAILURE; update the MISCONFIGURED_INSTANCE_BASE_URL enum
entry to use a unique, unused error code string (e.g., increment the numeric
suffix) so each enum constant has a distinct code and leave
FEATURE_FLAG_MIGRATION_FAILURE unchanged; ensure the change is made only in the
MISCONFIGURED_INSTANCE_BASE_URL declaration within the AppsmithErrorCode enum.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java`:
- Around line 61-66: In SecureBaseUrlResolverCEImpl, the compat fallback branch
returns Mono.just(providedBaseUrl) which will NPE when providedBaseUrl is null;
update the branch that checks allowInsecureOriginBasedLinks to first null-check
providedBaseUrl and if it's null log a warning and return Mono.empty() (or
otherwise an appropriate empty/error Mono) instead of Mono.just(null), otherwise
continue to return Mono.just(providedBaseUrl).
---
Nitpick comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java`:
- Around line 69-72: In SecureBaseUrlResolverCEImpl, avoid unbounded warn spam
from the log.warn(...) branch that returns Mono.empty(); implement rate-limiting
so the warning is emitted at most once per configured interval (e.g., once per
hour). For example, add a static/instance time-based guard (AtomicLong
lastWarnAt or a Guava RateLimiter) checked before calling log.warn(...) and
update the timestamp when the warn is emitted; keep the method behavior
returning Mono.empty() unchanged. Ensure the guard is thread-safe and
configurable, and document the interval constant near the class-level fields so
future maintainers can adjust it.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java`:
- Around line 197-207: Add a boundary test that asserts the empty-completion
path from secureBaseUrlResolver.resolveSecureBaseUrl(...) produces the generic
success response and does not send an email: mock secureBaseUrlResolver to
return Mono.empty() for resetUserPasswordDTO.getBaseUrl(), call the public entry
point that uses processForgotPasswordTokenGeneration (the service or controller
method that consumes resetUserPasswordDTO), and assert the HTTP/Service response
equals the generic success value and that the email-sending component was never
invoked; repeat the same kind of test for the other occurrence around
processForgotPasswordTokenGeneration referenced in the second region so future
refactors cannot convert Mono.empty() into a different response or trigger email
dispatch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a4aac4e-d7ce-4853-bced-9028cca33403
📒 Files selected for processing (13)
app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/helpers/SecureBaseUrlResolver.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/helpers/SecureBaseUrlResolverImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/EmailServiceImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/UserServiceImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/EmailServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/UserServiceCECompatibleImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/EmailServiceCEImplTest.java
Comment on lines
+61
to
+66
| 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); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Project Reactor, does Mono.just(null) throw an exception, and what API is recommended when the value may be null?
💡 Result:
No, Mono.just(null) does not throw an exception at runtime; it fails to compile due to null-safety annotations and the Reactive Streams specification prohibiting null values (rule 2.13). When the value may be null, use Mono.justOrEmpty(T data), which emits the item if non-null or completes empty if null.
Citations:
- 1: https://stackoverflow.com/questions/69461051/why-cant-a-mono-hold-a-null
- 2: https://projectreactor.io/docs/core/release/reference/advancedFeatures/null-safety.html
- 3: https://projectreactor.io/docs/core/release/api/reactor/core/publisher/Mono.html
- 4: https://projectreactor.io/docs/core/3.4.29/api/reactor/core/publisher/Mono.html
- 5: https://projectreactor.io/docs/core/milestone/reference/apdx-operatorChoice.html
🏁 Script executed:
# Check the actual file content around lines 61-66
cat -n app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java | sed -n '50,75p'Repository: appsmithorg/appsmith
Length of output: 1561
🏁 Script executed:
# Check the method signature and parameter types
ast-grep --pattern $'public Mono<String> resolveSecureBaseUrl($_) {
$$$
}'Repository: appsmithorg/appsmith
Length of output: 46
🏁 Script executed:
# Check for null-safety annotations in the file and project configuration
grep -r "@NonNull\|@Nullable\|nonnull\|Nonnull" app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java || echo "No explicit null-safety annotations found in file"Repository: appsmithorg/appsmith
Length of output: 114
🏁 Script executed:
# Check if the project uses lombok or spring-boot null-safety
head -50 app/server/appsmith-server/pom.xml | grep -A 5 -B 5 "lombok\|spring"Repository: appsmithorg/appsmith
Length of output: 46
🏁 Script executed:
# Find all callers of resolveSecureBaseUrl
rg "resolveSecureBaseUrl" app/server/appsmith-server/src/ -A 2 -B 2 --type javaRepository: appsmithorg/appsmith
Length of output: 12891
🏁 Script executed:
# Check the request Origin header handling in the codebase
rg "Origin" app/server/appsmith-server/src/ --type java -l | head -10Repository: appsmithorg/appsmith
Length of output: 1061
🏁 Script executed:
# Look for the interface definition to check nullability contracts
find app/server -name "*SecureBaseUrlResolver*.java" -type fRepository: appsmithorg/appsmith
Length of output: 575
🏁 Script executed:
# Check for null-safety configuration in pom.xml
cat app/server/appsmith-server/pom.xml | grep -A 10 -B 5 "maven-compiler\|nullness\|<nullableAnnotations\|<nonnullAnnotations" || echo "No explicit config found"Repository: appsmithorg/appsmith
Length of output: 712
🏁 Script executed:
# Check for Spring Boot or Lombok null-safety processor configuration
grep -r "processor\|@NonNull\|@Nullable" app/server/appsmith-server/pom.xmlRepository: appsmithorg/appsmith
Length of output: 46
🏁 Script executed:
# Look at the test file more carefully to see what happens with null in compat mode
cat -n app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java | grep -A 10 -B 5 "CompatFlag"Repository: appsmithorg/appsmith
Length of output: 2686
Add null check in the compat fallback path.
Line 66 will throw an exception if providedBaseUrl is null. The resolver accepts nullable input (HTTP Origin headers, DTO fields), and callers can reach this branch with null when the compat flag is enabled. The test suite covers null only with the flag disabled, not in this path.
Suggested fix
if (allowInsecureOriginBasedLinks) {
+ if (!StringUtils.hasText(providedBaseUrl)) {
+ return Mono.empty();
+ }
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);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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); | |
| if (allowInsecureOriginBasedLinks) { | |
| if (!StringUtils.hasText(providedBaseUrl)) { | |
| return Mono.empty(); | |
| } | |
| 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); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java`
around lines 61 - 66, In SecureBaseUrlResolverCEImpl, the compat fallback branch
returns Mono.just(providedBaseUrl) which will NPE when providedBaseUrl is null;
update the branch that checks allowInsecureOriginBasedLinks to first null-check
providedBaseUrl and if it's null log a warning and return Mono.empty() (or
otherwise an appropriate empty/error Mono) instead of Mono.just(null), otherwise
continue to return Mono.just(providedBaseUrl).
Failed server tests
|
… + test profile flag 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
…vw2f-9hrw) The GHSA fix in 3b7c865 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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/specs/2026-04-28-ci-base-url-config-design.md`:
- Around line 47-49: The fenced code block containing the Helm/set snippet
"--set applicationConfig.APPSMITH_BASE_URL=https://$DOMAINNAME" is missing a
fence language; update the fenced block to declare a shell language (e.g., add
"bash" after the opening ``` fence) so the example is rendered and markdownlint
passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2a8976e2-2a75-457b-9ee6-957502188ce4
📒 Files selected for processing (7)
.github/workflows/ci-test-custom-script.yml.github/workflows/ci-test-limited-with-count.yml.github/workflows/ci-test-limited.yml.github/workflows/ci-test-playwright.ymldocs/superpowers/specs/2026-04-28-ci-base-url-config-design.mdscripts/deploy_preview.shscripts/local_testing.sh
Comment on lines
+47
to
+49
| ``` | ||
| --set applicationConfig.APPSMITH_BASE_URL=https://$DOMAINNAME | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language tag to the fenced example.
The shell snippet is missing a fence language, which will keep this doc tripping markdownlint.
Suggested fix
-```
+```bash
--set applicationConfig.APPSMITH_BASE_URL=https://$DOMAINNAME
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/specs/2026-04-28-ci-base-url-config-design.md` around lines
47 - 49, The fenced code block containing the Helm/set snippet "--set
applicationConfig.APPSMITH_BASE_URL=https://$DOMAINNAME" is missing a fence
language; update the fenced block to declare a shell language (e.g., add "bash"
after the opening ``` fence) so the example is rendered and markdownlint passes.
…SA-j9gf-vw2f-9hrw) 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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java (1)
71-76:⚠️ Potential issue | 🟠 MajorGuard the compat fallback against missing
Origin.If
APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS=trueandprovidedBaseUrlisnullor blank, this branch throws or propagates an unusable base URL instead of failing closed. That makes the migration path brittle for requests whereOriginis absent/stripped.Suggested fix
if (allowInsecureOriginBasedLinks) { + if (!StringUtils.hasText(providedBaseUrl)) { + log.warn("APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS=true but request Origin is empty."); + return Mono.empty(); + } 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); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java` around lines 71 - 76, In SecureBaseUrlResolverCEImpl where allowInsecureOriginBasedLinks is handled, guard the insecure fallback by validating providedBaseUrl (the value derived from the request Origin) before returning it; if providedBaseUrl is null or blank, do not return Mono.just(providedBaseUrl) — instead return a failed Mono (e.g., Mono.error with a clear message) or otherwise fail closed so callers do not receive an unusable URL; update the branch that currently returns Mono.just(providedBaseUrl) to perform this null/blank check and produce an error when Origin is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java`:
- Around line 60-67: Add a companion regression test in
SecureBaseUrlResolverCEImplTest that covers the compat-mode fallback when the
Origin is null or blank: create a new test (e.g.,
resolveSecureBaseUrl_whenOriginNullOrBlank_andCompatFlagOn_returnsProvidedValue)
that builds a SecureBaseUrlResolverCEImpl via newResolver("", true) and asserts
resolver.resolveSecureBaseUrl(null) and resolver.resolveSecureBaseUrl("") both
emit the provided URL (e.g., "https://attacker.example") and complete; this
mirrors
resolveSecureBaseUrl_whenAppsmithBaseUrlUnsetAndCompatFlagOn_returnsProvidedValue
but exercises the null/blank Origin regression path.
---
Duplicate comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.java`:
- Around line 71-76: In SecureBaseUrlResolverCEImpl where
allowInsecureOriginBasedLinks is handled, guard the insecure fallback by
validating providedBaseUrl (the value derived from the request Origin) before
returning it; if providedBaseUrl is null or blank, do not return
Mono.just(providedBaseUrl) — instead return a failed Mono (e.g., Mono.error with
a clear message) or otherwise fail closed so callers do not receive an unusable
URL; update the branch that currently returns Mono.just(providedBaseUrl) to
perform this null/blank check and produce an error when Origin is missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9dabe7f7-028f-4a2e-be78-0892ba042d97
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java
Comment on lines
+60
to
+67
| @Test | ||
| void resolveSecureBaseUrl_whenAppsmithBaseUrlUnsetAndCompatFlagOn_returnsProvidedValue() throws Exception { | ||
| SecureBaseUrlResolverCEImpl resolver = newResolver("", true); | ||
|
|
||
| StepVerifier.create(resolver.resolveSecureBaseUrl("https://attacker.example")) | ||
| .expectNext("https://attacker.example") | ||
| .verifyComplete(); | ||
| } |
Contributor
There was a problem hiding this comment.
Add the missing compat-mode null/blank regression case.
This suite only pins missing Origin when compat mode is off. Adding the same case with allowInsecureFallback=true would catch the current breakage in the migration path.
Suggested test
`@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_whenProvidedBaseUrlIsNullOrBlank_andCompatFlagOn_returnsEmpty() throws Exception {
+ SecureBaseUrlResolverCEImpl resolver = newResolver("", true);
+
+ StepVerifier.create(resolver.resolveSecureBaseUrl(null)).verifyComplete();
+ StepVerifier.create(resolver.resolveSecureBaseUrl("")).verifyComplete();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @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_whenAppsmithBaseUrlUnsetAndCompatFlagOn_returnsProvidedValue() throws Exception { | |
| SecureBaseUrlResolverCEImpl resolver = newResolver("", true); | |
| StepVerifier.create(resolver.resolveSecureBaseUrl("https://attacker.example")) | |
| .expectNext("https://attacker.example") | |
| .verifyComplete(); | |
| } | |
| `@Test` | |
| void resolveSecureBaseUrl_whenProvidedBaseUrlIsNullOrBlank_andCompatFlagOn_returnsEmpty() throws Exception { | |
| SecureBaseUrlResolverCEImpl resolver = newResolver("", true); | |
| StepVerifier.create(resolver.resolveSecureBaseUrl(null)).verifyComplete(); | |
| StepVerifier.create(resolver.resolveSecureBaseUrl("")).verifyComplete(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java`
around lines 60 - 67, Add a companion regression test in
SecureBaseUrlResolverCEImplTest that covers the compat-mode fallback when the
Origin is null or blank: create a new test (e.g.,
resolveSecureBaseUrl_whenOriginNullOrBlank_andCompatFlagOn_returnsProvidedValue)
that builds a SecureBaseUrlResolverCEImpl via newResolver("", true) and asserts
resolver.resolveSecureBaseUrl(null) and resolver.resolveSecureBaseUrl("") both
emit the provided URL (e.g., "https://attacker.example") and complete; this
mirrors
resolveSecureBaseUrl_whenAppsmithBaseUrlUnsetAndCompatFlagOn_returnsProvidedValue
but exercises the null/blank Origin regression path.
…(GHSA-j9gf-vw2f-9hrw)
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
…HSA-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
4 tasks
subrata71
added a commit
that referenced
this pull request
May 6, 2026
…ring emails (GHSA-j9gf-vw2f-9hrw) (#41767) ## 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. ## Architecture - **Server:** new reactive `SecureBaseUrlResolverCE#isBaseUrlConfigurationHealthy()` exposes the health signal. CE returns `true` iff `APPSMITH_BASE_URL` is set; EE override (in shadow EE PR) returns `true` unconditionally when `license_multi_org_enabled` is on. Wired into `ConsolidatedAPIServiceCEImpl` alongside the existing org-config fetch so it runs in parallel (no sequential cost). Result populates a new transient boolean field `instanceBaseUrlConfigurationHealthy` on `OrganizationConfigurationCE`. - **Client:** new `BaseUrlMissingBanner.tsx` component renders via the ADS `Banner` component (same pattern as `PageBannerMessage`). Selector `getShouldShowBaseUrlMissingBanner` gates on `isSuperUser && adminSettingsVisible && instanceBaseUrlConfigurationHealthy === false`. The explicit `=== false` (rather than `!value`) is the rolling-deploy guard. Mounted inside `PageHeader.tsx` alongside the existing license/trial banner. - **Recovery loop:** zero new code — leverages the existing `RESTART_SERVER_POLL` → `window.location.reload()` flow that every Configuration-tier admin setting already uses today. Fixes https://linear.app/appsmith/issue/APP-15046/security-high-configuration-dependent-origin-validation-bypass-in ## Test plan - [x] `SecureBaseUrlResolverCEImplTest` — 2 new cases for `isBaseUrlConfigurationHealthy()` (true when set, false when blank); 18/18 total pass - [x] `usersSelectors.test.ts` — 6 new cases pinning all gating dimensions plus the rolling-deploy `=== false` guard - [x] `BaseUrlMissingBanner.test.tsx` — 5 new Jest cases covering all gating dimensions - [x] `ConsolidatedAPIServiceImplTest` — mock wired for resolver - Cypress: skipped — CI sets `APPSMITH_BASE_URL=http://localhost`, banner never fires by construction ## Companion / refs - Shadow EE PR: appsmithorg/appsmith-ee#9003 - Security advisory: https://github.com/appsmithorg/appsmith/security/advisories/GHSA-j9gf-vw2f-9hrw - Linear: APP-15046 ## Automation /ok-to-test tags="@tag.All" <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/25359958615> > Commit: e9e92ce > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=25359958615&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Tue, 05 May 2026 07:40:44 UTC <!-- end of auto-generated comment: Cypress test results -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
fix(security): fail closed when APPSMITH_BASE_URL unset for token-bearing emails (GHSA-j9gf-vw2f-9hrw)
SecureBaseUrlResolverhelper that gates the host portion of every emailed absolute link (forgot-password, email verification, workspace invite, instance-admin invite). The resolver no longer trusts the requestOriginheader as the canonical host whenAPPSMITH_BASE_URLis unset — it returnsMono.empty()instead, which propagates through the existing controller wiring (defaultIfEmpty(true)/thenReturn) so unauthenticated flows return the same generic 200 success without dispatching email (anti-enumeration preserved). Auth-gated invite flows surface the newMISCONFIGURED_INSTANCE_BASE_URLerror.APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS(defaults tofalse). When set totrue, the legacy behaviour is restored. Documented as deprecated; intended only as a transition window for self-hosted deployments that have not yet configuredAPPSMITH_BASE_URL. The helper logs a clear WARN on every call so operators are aware they are running in an insecure mode.EmailServiceCEImplthrough the same helper. Previously these flows derived the invite URL host directly fromoriginHeader, which was an auth-gated but identical attack surface — and was a latent bug in EE multi-org mode (where forgot-password used a server-controlled URL but invite emails still usedOrigin).SecureBaseUrlResolverCEImplTest(7 tests, pure unit tests, no Spring context) pinning all four state permutations of the resolver. ExistingEmailServiceCEImplTestupdated with@TestPropertySource(properties = "APPSMITH_BASE_URL=http://example.com")so its invite/admin-invite tests exercise the strict-match path.APPSMITH_BASE_URLwhen configured) is preserved exactly. The new compat flag does NOT weaken it — it only governs the unconfigured case.Vulnerability
CVSS:3.1/AV:N/AC:L/PR:N/UI:R/S:U/C:H/I:H/A:NExposure Analysis
Who can exploit this? Any unauthenticated network attacker who can reach
POST /api/v1/users/forgotPasswordorPOST /api/v1/users/resendEmailVerification. No prior authentication, no recon.What can an attacker achieve? When
APPSMITH_BASE_URLis unset (the default state for self-hosted installs that haven't been hardened), an attacker can forge theOriginheader to make Appsmith send password-reset and email-verification emails whose clickable host is attacker-controlled. The emails are otherwise legitimate (sent from the real Appsmith server, signed with SPF/DKIM, rendered with the real Appsmith branding). A victim who clicks the link delivers a valid token to the attacker's host — the attacker can then replay it against the legitimate Appsmith instance viaPUT /api/v1/users/resetPasswordto take over the account.Blast radius: Per-user. Each successful exploit takes over one targeted account. There is no scope change to the wider workspace from this primitive alone, but a successful takeover of a workspace admin or instance admin escalates from there through normal application flows.
Affected configurations:
APPSMITH_BASE_URLunset and outbound email enabled — vulnerable.OrganizationService.getOrganizationBaseUrl()override that derived the host fromslug + deploymentDomain. This PR relocates that override into the new helper so it now also covers invite flows (which were a latent bug under multi-org).APPSMITH_BASE_URLconfigured, so the strict-mode check from PR fix(auth): validate Origin header against APPSMITH_BASE_URL #41426 applies. Not affected.Evidence of exploitation in the wild: None known. Reported responsibly via GHSA by an external researcher.
Fix
Root cause.
UserServiceCEImpl#resolveSecureBaseUrlfailed open whenAPPSMITH_BASE_URLwas empty: it returned the caller-supplied value verbatim. That caller-supplied value originated from@RequestHeader("Origin")and was used to interpolate the host of token-bearing email links. The underlying coding pattern is using request metadata (a value the client claims) to populate an outbound, server-issued security artifact. Token-bearing artifacts must be bound to the server's configured identity, not the requester's claimed identity.Fix strategy. Extract
SecureBaseUrlResolveras a Spring@Componentwith a single resolver method. Make this the only path through which any emailed absolute link gets its host. The helper has three resolution branches:Mono.empty(). Unauthenticated callers (forgot-password, resend-verification) translate this into a generic 200 success without dispatching email — anti-enumeration preserved by the existingdefaultIfEmpty(true).thenReturn(...)controller wiring. Authenticated callers (invite, admin-invite) translate it intoMISCONFIGURED_INSTANCE_BASE_URLso the admin caller sees an actionable configuration error.APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS=true): log WARN, return the caller-supplied value. Migration-only, deprecated.What was intentionally NOT changed:
SecurityConfigpermit-all matchers stay as-is — the endpoints must remain reachable without auth; the fix is in URL generation, not auth gating.ResetUserPasswordDTO,ResendEmailVerificationDTO) unchanged.Defense-in-depth.
resolveSecureBaseUrlentirely.Reporter coordination. This fix matches the design the reporter recommended in the GHSA discussion thread, point-for-point: single server-side helper, fail-closed by default, anti-enumeration preserved for unauth flows, opt-in insecure compatibility flag for migration. Reporter is
@0xmrma.CE/EE sync
Mostly CE-only safe with a small shadow EE PR.
UserServiceCEImpl.javaEmailServiceCEImpl.javaEmailServiceCEImplTest.javaUserServiceCECompatibleImpl.javaSecureBaseUrlResolver.java,SecureBaseUrlResolverCE.java,SecureBaseUrlResolverCEImpl.java(new)SecureBaseUrlResolverImpl.java(new)UserServiceImpl.javaresolveSecureBaseUrloverride and updates the constructor signatureEmailServiceImpl.javaSecureBaseUrlResolverAppsmithError.java/AppsmithErrorCode.javaMISCONFIGURED_INSTANCE_BASE_URLentryShadow EE branch (
fix/origin-validation-email-links-ghsa-j9gfonappsmithorg/appsmith-ee) is prepared and pushed; will be linked in a follow-up comment.Disclosure
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/25127579141
Commit: 306e492
Cypress dashboard.
Tags:
@tag.AllSpec:
Wed, 29 Apr 2026 20:02:20 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
APPSMITH_BASE_URLwill see password-reset / email-verification / invite emails stop being delivered after upgrading. Migration path: setAPPSMITH_BASE_URL, or as a temporary opt-in, setAPPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS=true(insecure, deprecated).Follow-ups
app/client/src/ce/pages/AdminSettings/config/configuration.tsxstrengthening the wording aroundAPPSMITH_BASE_URLbeing required (deferred — locallint-stagedrequires a freshyarn installinapp/client; will land as a separate small PR).APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKSenv var in a future major release once telemetry confirms self-hosted operators have migrated.UserServiceTestforforgotPasswordTokenGenerate/resendEmailVerificationfail-closed paths (deferred — needs Mongo+Redis; helper unit tests already pin the security-critical semantics).Summary by CodeRabbit
Bug Fixes
Tests
Documentation / CI