Skip to content

fix(security): fail closed when APPSMITH_BASE_URL unset for token-bearing emails (GHSA-j9gf-vw2f-9hrw) #41767

Merged
subrata71 merged 22 commits intoreleasefrom
feat/admin-base-url-warning-banner-ghsa-j9gf
May 6, 2026
Merged

fix(security): fail closed when APPSMITH_BASE_URL unset for token-bearing emails (GHSA-j9gf-vw2f-9hrw) #41767
subrata71 merged 22 commits intoreleasefrom
feat/admin-base-url-warning-banner-ghsa-j9gf

Conversation

@subrata71
Copy link
Copy Markdown
Collaborator

@subrata71 subrata71 commented Apr 29, 2026

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_POLLwindow.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

  • SecureBaseUrlResolverCEImplTest — 2 new cases for isBaseUrlConfigurationHealthy() (true when set, false when blank); 18/18 total pass
  • usersSelectors.test.ts — 6 new cases pinning all gating dimensions plus the rolling-deploy === false guard
  • BaseUrlMissingBanner.test.tsx — 5 new Jest cases covering all gating dimensions
  • ConsolidatedAPIServiceImplTest — mock wired for resolver
  • Cypress: skipped — CI sets APPSMITH_BASE_URL=http://localhost, banner never fires by construction

Companion / refs

Automation

/ok-to-test tags="@tag.All"

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/25359958615
Commit: e9e92ce
Cypress dashboard.
Tags: @tag.All
Spec:


Tue, 05 May 2026 07:40:44 UTC

…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
… + 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
…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
…(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
…_URL (GHSA-j9gf-vw2f-9hrw)

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
…GHSA-j9gf-vw2f-9hrw)

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
…esolverCE (GHSA-j9gf-vw2f-9hrw)

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
…ProfileCE_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
…ldUserProfileDTO (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
…GHSA-j9gf-vw2f-9hrw)

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
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
…ter (GHSA-j9gf-vw2f-9hrw)

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
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 29, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds a fail-closed secure base-URL resolver, wires a runtime health signal to the client, surfaces an admin banner when APPSMITH_BASE_URL is unhealthy/missing, updates email/invite flows to use the resolver, and sets APPSMITH_BASE_URL in CI/local/deploy scripts.

Changes

Cohort / File(s) Summary
CI & Local Deploy
.github/workflows/ci-test-custom-script.yml, .../ci-test-limited-with-count.yml, .../ci-test-limited.yml, .../ci-test-playwright.yml, scripts/deploy_preview.sh, scripts/local_testing.sh
Injects APPSMITH_BASE_URL=http://localhost into CI appsmith containers and passes applicationConfig.APPSMITH_BASE_URL=https://$DOMAINNAME for preview deployments; local local_testing.sh sets localhost env var.
Client: Banner UI & Integration
app/client/src/components/editorComponents/BaseUrlMissingBanner.tsx, .../BaseUrlMissingBanner.test.tsx, app/client/src/ce/AppRouter.tsx, app/client/src/ce/constants/messages.ts
Adds banner component, message constants, tests, and mounts banner in AppRouter to show admin-facing warning when health signal indicates misconfiguration.
Client: Selector & Tests
app/client/src/selectors/usersSelectors.tsx, .../usersSelectors.test.ts
Adds getShouldShowBaseUrlMissingBanner selector that requires super-user + admin settings visible + instanceBaseUrlConfigurationHealthy === false; includes unit tests for cases and edge conditions.
Client: Cypress intercepts
app/client/cypress/support/commands.js
Replaces synthetic "Cypress" Origin with Cypress.config("baseUrl") in multiple intercept handlers and documents Origin-matching requirement.
Server: Secure Base-URL Resolver (CE)
app/server/.../helpers/ce/SecureBaseUrlResolverCE.java, .../SecureBaseUrlResolverCEImpl.java
Introduces reactive CE resolver contract and CE implementation that enforces strict origin matching against APPSMITH_BASE_URL, supports opt-in insecure fallback, returns empty when disabled (fail-closed), and exposes isBaseUrlConfigurationHealthy().
Server: Resolver DI & CE Bean
app/server/.../helpers/SecureBaseUrlResolver.java, .../SecureBaseUrlResolverImpl.java
Adds public interface for DI and a CE component implementation to allow EE overrides (multi-org behavior).
Server: Email & User Services
app/server/.../services/ce/EmailServiceCEImpl.java, app/server/.../services/EmailServiceImpl.java, app/server/.../services/ce/UserServiceCEImpl.java, .../ce_compatible/UserServiceCECompatibleImpl.java
Injects SecureBaseUrlResolver; invite/forgot-password/verification flows call resolver and fail with MISCONFIGURED_INSTANCE_BASE_URL (or skip sending) when resolver emits empty; constructors updated to accept resolver.
Server: Consolidated API & Org Config Health
app/server/.../services/ce/ConsolidatedAPIServiceCEImpl.java, .../ConsolidatedAPIServiceImpl.java, .../ce_compatible/ConsolidatedAPIServiceCECompatibleImpl.java
Wires secureBaseUrlResolver.isBaseUrlConfigurationHealthy() into org configuration fetch, populating instanceBaseUrlConfigurationHealthy with onErrorReturn(true) fallback; constructor DI updated.
Server: Domain & Error Types
app/server/.../domains/ce/OrganizationConfigurationCE.java, app/server/.../exceptions/AppsmithError.java, .../AppsmithErrorCode.java
Adds transient instanceBaseUrlConfigurationHealthy flag to org config and new error MISCONFIGURED_INSTANCE_BASE_URL/code for reporting fail-closed state.
Server: Tests & Test Config
app/server/.../helpers/ce/SecureBaseUrlResolverCEImplTest.java, app/server/.../services/ce/ConsolidatedAPIServiceImplTest.java, app/server/.../test/resources/application-test.properties
Adds comprehensive resolver unit tests (origin normalization, mismatch, insecure flag behavior, health checks), updates tests to mock resolver, and enables APPSMITH_ALLOW_INSECURE_ORIGIN_BASED_LINKS=true for testing.
Docs / Plans / Specs
docs/superpowers/plans/2026-04-29-admin-base-url-warning-banner.md, docs/superpowers/specs/2026-04-28-ci-base-url-config-design.md, docs/superpowers/specs/2026-04-29-admin-base-url-warning-banner-design.md
New design and implementation plan docs describing fail-closed resolver, CI/deploy config, client banner behavior, EE override expectations, and testing/coordination checklist.

Sequence Diagram(s)

sequenceDiagram
    actor Browser as User Browser
    participant App as App Server
    participant Resolver as SecureBaseUrlResolver
    participant EmailSvc as EmailService
    participant SMTP as Email Provider

    User Browser->>App: Request password reset / invite (includes Origin header)
    App->>Resolver: resolveSecureBaseUrl(providedOrigin)
    alt Resolver returns resolvedBaseUrl
        Resolver-->>App: resolvedBaseUrl
        App->>EmailSvc: build invite/reset link using resolvedBaseUrl
        EmailSvc->>SMTP: send email with token link
        SMTP-->>EmailSvc: accept/send confirmation
        EmailSvc-->>App: send success
        App-->>User Browser: 200 OK (email queued)
    else Resolver returns empty (fail-closed)
        Resolver-->>App: empty
        App->>App: raise MISCONFIGURED_INSTANCE_BASE_URL / skip send
        App-->>User Browser: 5xx / error response
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🔐 A base URL resolves with care,
Fail-closed when config's not there,
Admins see banners bright and true,
Origin-matched, secure and new,
Email flows locked 'til paths renew. 🚀

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed PR description is comprehensive with clear motivation, architecture overview, test coverage summary, and relevant references (Linear, security advisory, shadow EE PR).
Title check ✅ Passed The title clearly describes the main security fix: implementing fail-closed behavior for token-bearing emails when APPSMITH_BASE_URL is unset, with the security advisory reference providing context.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/admin-base-url-warning-banner-ghsa-j9gf

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (4)
app/client/src/selectors/usersSelectors.test.ts (1)

4-5: Avoid as never in the test state helper.

Line 5 suppresses type checking and can hide selector-state contract drift. Prefer a narrow user shape and cast through the selector’s input type instead of never.

♻️ Proposed refactor
-const stateWith = (currentUser: object | null) =>
-  ({ ui: { users: { currentUser } } }) as never;
+type BannerUser = {
+  isSuperUser?: boolean;
+  adminSettingsVisible?: boolean;
+  instanceBaseUrlConfigurationHealthy?: boolean;
+};
+
+const stateWith = (currentUser: BannerUser | null) =>
+  ({ ui: { users: { currentUser } } } as unknown as Parameters<
+    typeof getShouldShowBaseUrlMissingBanner
+  >[0]);

As per coding guidelines, "Ensure TypeScript types are correct by running yarn run check-types."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/client/src/selectors/usersSelectors.test.ts` around lines 4 - 5, The
helper stateWith currently uses a blanket cast "as never" which suppresses
TypeScript checks; replace it by defining a narrow user shape (e.g., a minimal
object type matching the selector's expectation) and cast the returned object to
the selector input type instead of never — update the stateWith function (the
helper used in usersSelectors.test.ts) to return ({ ui: { users: { currentUser }
} }) typed as the selector's input type (or an explicit interface for
currentUser) so tests keep type-safety, then run yarn run check-types to verify.
app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx (1)

7-7: Prefer MemoryRouter over BrowserRouter in this unit test.

Line 7 and Line 20 can use MemoryRouter to avoid global history/location coupling and keep tests more deterministic.

♻️ Optional cleanup
-import { BrowserRouter as Router } from "react-router-dom";
+import { MemoryRouter as Router } from "react-router-dom";

Also applies to: 20-20

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx` at
line 7, Replace the use of BrowserRouter with MemoryRouter in this test file:
change the import (currently "import { BrowserRouter as Router } from
'react-router-dom'") to import MemoryRouter (e.g., "import { MemoryRouter as
Router } from 'react-router-dom'") and update the test render/wrapper usage
where Router is instantiated (occurrences around BaseUrlMissingBanner.test.tsx
lines referencing Router at import and the render at line ~20) so the test uses
MemoryRouter to avoid coupling to global history/location.
docs/superpowers/specs/2026-04-28-ci-base-url-config-design.md (1)

47-49: Add language specifier to fenced code block.

The code block is missing a language identifier. Use bash or shell for Helm/CLI snippets.

-```
+```bash
 --set applicationConfig.APPSMITH_BASE_URL=https://$DOMAINNAME

<details>
<summary>🤖 Prompt for AI Agents</summary>

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/CLI snippet (--set
applicationConfig.APPSMITH_BASE_URL=https://$DOMAINNAME) is missing a language
identifier; update the block to use a shell language tag (e.g., prepend bash and close with ) so the snippet is rendered/highlighted correctly in the
docs.


</details>

</blockquote></details>
<details>
<summary>docs/superpowers/specs/2026-04-29-admin-base-url-warning-banner-design.md (1)</summary><blockquote>

`106-117`: **Add language specifier to fenced code block.**

The recovery loop diagram code block lacks a language identifier.

```diff
-```
+```text
 Admin clicks banner CTA
   → /settings/configuration
```

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @docs/superpowers/specs/2026-04-29-admin-base-url-warning-banner-design.md
around lines 106 - 117, The fenced code block containing the recovery loop
diagram that starts with "Admin clicks banner CTA" should include a language
specifier (e.g., ```text) to ensure correct rendering; update the block fence to

plain text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx`:
- Line 7: Replace the use of BrowserRouter with MemoryRouter in this test file:
change the import (currently "import { BrowserRouter as Router } from
'react-router-dom'") to import MemoryRouter (e.g., "import { MemoryRouter as
Router } from 'react-router-dom'") and update the test render/wrapper usage
where Router is instantiated (occurrences around BaseUrlMissingBanner.test.tsx
lines referencing Router at import and the render at line ~20) so the test uses
MemoryRouter to avoid coupling to global history/location.

In `@app/client/src/selectors/usersSelectors.test.ts`:
- Around line 4-5: The helper stateWith currently uses a blanket cast "as never"
which suppresses TypeScript checks; replace it by defining a narrow user shape
(e.g., a minimal object type matching the selector's expectation) and cast the
returned object to the selector input type instead of never — update the
stateWith function (the helper used in usersSelectors.test.ts) to return ({ ui:
{ users: { currentUser } } }) typed as the selector's input type (or an explicit
interface for currentUser) so tests keep type-safety, then run yarn run
check-types to verify.

In `@docs/superpowers/specs/2026-04-28-ci-base-url-config-design.md`:
- Around line 47-49: The fenced code block containing the Helm/CLI snippet
(--set applicationConfig.APPSMITH_BASE_URL=https://$DOMAINNAME) is missing a
language identifier; update the block to use a shell language tag (e.g., prepend
```bash and close with ``` ) so the snippet is rendered/highlighted correctly in
the docs.

In `@docs/superpowers/specs/2026-04-29-admin-base-url-warning-banner-design.md`:
- Around line 106-117: The fenced code block containing the recovery loop
diagram that starts with "Admin clicks banner CTA" should include a language
specifier (e.g., ```text) to ensure correct rendering; update the block fence to
```text (or another appropriate language) so the diagram is explicitly marked as
plain text.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 540e6f23-0dce-409e-912e-da1643676911

📥 Commits

Reviewing files that changed from the base of the PR and between a92dee4 and 46f8118.

📒 Files selected for processing (32)
  • .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.yml
  • app/client/cypress/support/commands.js
  • app/client/src/ce/AppRouter.tsx
  • app/client/src/ce/constants/messages.ts
  • app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx
  • app/client/src/components/editorComponents/BaseUrlMissingBanner.tsx
  • app/client/src/constants/userConstants.ts
  • app/client/src/selectors/usersSelectors.test.ts
  • app/client/src/selectors/usersSelectors.tsx
  • app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/UserProfileCE_DTO.java
  • app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java
  • app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java
  • app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/SecureBaseUrlResolver.java
  • app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/SecureBaseUrlResolverImpl.java
  • 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/main/java/com/appsmith/server/services/EmailServiceImpl.java
  • app/server/appsmith-server/src/main/java/com/appsmith/server/services/UserServiceImpl.java
  • app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/EmailServiceCEImpl.java
  • app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java
  • app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/UserServiceCECompatibleImpl.java
  • app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/SecureBaseUrlResolverCEImplTest.java
  • app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java
  • app/server/appsmith-server/src/test/resources/application-test.properties
  • docs/superpowers/plans/2026-04-29-admin-base-url-warning-banner.md
  • docs/superpowers/specs/2026-04-28-ci-base-url-config-design.md
  • docs/superpowers/specs/2026-04-29-admin-base-url-warning-banner-design.md
  • scripts/deploy_preview.sh
  • scripts/local_testing.sh

@subrata71 subrata71 self-assigned this Apr 29, 2026
@subrata71 subrata71 added the ok-to-test Required label for CI label Apr 29, 2026
…rProfile to OrganizationConfiguration (GHSA-j9gf-vw2f-9hrw)

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
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java (1)

329-332: ⚡ Quick win

Add error logging before the onErrorReturn(true) fallback.

Right now resolver failures are silent, which makes persistent misconfiguration/resolver issues hard to detect operationally.

Proposed change
         return secureBaseUrlResolver
                 .isBaseUrlConfigurationHealthy()
+                .doOnError(error -> log.warn(
+                        "Failed to resolve base URL configuration health; defaulting to healthy=true", error))
                 .onErrorReturn(true)
                 .map(healthy -> {
                     finalClientOrganization
                             .getOrganizationConfiguration()
                             .setInstanceBaseUrlConfigurationHealthy(healthy);
                     return finalClientOrganization;
                 });
🤖 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/OrganizationServiceCEImpl.java`
around lines 329 - 332, The call chain on
secureBaseUrlResolver.isBaseUrlConfigurationHealthy() swallows errors via
onErrorReturn(true); add an error logging step before the fallback so failures
are visible: use a reactive operator such as doOnError or onErrorResume on the
Mono returned by isBaseUrlConfigurationHealthy() to log the Throwable via the
existing logger (or process logger) with context (e.g., "Base URL resolver
health check failed") and then return the original fallback (true) as currently
done; reference secureBaseUrlResolver, isBaseUrlConfigurationHealthy(), and
onErrorReturn(true) when making this change.
🤖 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/services/ce/UserServiceCEImpl.java`:
- Around line 197-207: The resolver chain currently completes empty when
secureBaseUrlResolver.resolveSecureBaseUrl(...) returns Mono.empty(), causing no
Boolean to be emitted; update the chain in UserServiceCEImpl so that after
calling
secureBaseUrlResolver.resolveSecureBaseUrl(resetUserPasswordDTO.getBaseUrl())
you switchIfEmpty to emit a successful Boolean (e.g., Mono.just(true)) while
still preserving the anti-enumeration behavior, and when a secureBaseUrl is
present continue to set resetUserPasswordDTO.setBaseUrl(...) and invoke
processForgotPasswordTokenGeneration(email, resetUserPasswordDTO); apply the
same change to the other identical block (the second resolveSecureBaseUrl usage)
so both paths always emit an explicit Boolean response.

---

Nitpick comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java`:
- Around line 329-332: The call chain on
secureBaseUrlResolver.isBaseUrlConfigurationHealthy() swallows errors via
onErrorReturn(true); add an error logging step before the fallback so failures
are visible: use a reactive operator such as doOnError or onErrorResume on the
Mono returned by isBaseUrlConfigurationHealthy() to log the Throwable via the
existing logger (or process logger) with context (e.g., "Base URL resolver
health check failed") and then return the original fallback (true) as currently
done; reference secureBaseUrlResolver, isBaseUrlConfigurationHealthy(), and
onErrorReturn(true) when making this change.
🪄 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: e881bc5d-381a-4df1-b630-09d76d706e57

📥 Commits

Reviewing files that changed from the base of the PR and between 46f8118 and 6bfcea7.

📒 Files selected for processing (8)
  • app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx
  • app/client/src/selectors/usersSelectors.test.ts
  • app/client/src/selectors/usersSelectors.tsx
  • app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.java
  • app/server/appsmith-server/src/main/java/com/appsmith/server/services/OrganizationServiceImpl.java
  • app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java
  • 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/ce/OrganizationServiceCETest.java
🚧 Files skipped from review as they are similar to previous changes (2)
  • app/client/src/selectors/usersSelectors.tsx
  • app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsx

Comment on lines +197 to +207
// 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);
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Return explicit success when resolver emits empty to keep response contract stable.

Both methods currently complete without emitting a Boolean when the resolver returns empty. That conflicts with the intended “generic success” behavior and can produce an empty response body instead of true.

Suggested patch
@@
         return secureBaseUrlResolver
                 .resolveSecureBaseUrl(resetUserPasswordDTO.getBaseUrl())
                 .flatMap(secureBaseUrl -> {
                     resetUserPasswordDTO.setBaseUrl(secureBaseUrl);
                     String email = resetUserPasswordDTO.getEmail();
                     return processForgotPasswordTokenGeneration(email, resetUserPasswordDTO);
-                });
+                })
+                .switchIfEmpty(Mono.just(true));
@@
         return secureBaseUrlResolver
                 .resolveSecureBaseUrl(resendEmailVerificationDTO.getBaseUrl())
                 .flatMap(secureBaseUrl -> {
                     resendEmailVerificationDTO.setBaseUrl(secureBaseUrl);
                     String email = resendEmailVerificationDTO.getEmail();
                     return processResendEmailVerification(email, resendEmailVerificationDTO, redirectUrl);
-                });
+                })
+                .switchIfEmpty(Mono.just(true));

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, The resolver chain currently completes empty when
secureBaseUrlResolver.resolveSecureBaseUrl(...) returns Mono.empty(), causing no
Boolean to be emitted; update the chain in UserServiceCEImpl so that after
calling
secureBaseUrlResolver.resolveSecureBaseUrl(resetUserPasswordDTO.getBaseUrl())
you switchIfEmpty to emit a successful Boolean (e.g., Mono.just(true)) while
still preserving the anti-enumeration behavior, and when a secureBaseUrl is
present continue to set resetUserPasswordDTO.setBaseUrl(...) and invoke
processForgotPasswordTokenGeneration(email, resetUserPasswordDTO); apply the
same change to the other identical block (the second resolveSecureBaseUrl usage)
so both paths always emit an explicit Boolean response.

…ver injection (GHSA-j9gf-vw2f-9hrw)

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
… ConsolidatedAPIServiceCEImpl (GHSA-j9gf-vw2f-9hrw)

Reverts the @lazy workaround from 982c4fc 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
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (1)

224-224: ⚡ Quick win

Log fallback path before onErrorReturn(true).

Right now resolver failures are silently converted to true. Add a doOnError here so fail-open fallback remains resilient but observable in prod.

Proposed patch
-                .zipWith(secureBaseUrlResolver.isBaseUrlConfigurationHealthy().onErrorReturn(true))
+                .zipWith(secureBaseUrlResolver
+                        .isBaseUrlConfigurationHealthy()
+                        .doOnError(e -> log.warn(
+                                "Falling back instanceBaseUrlConfigurationHealthy=true due to resolver error", e))
+                        .onErrorReturn(true))
🤖 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/ConsolidatedAPIServiceCEImpl.java`
at line 224, The call to secureBaseUrlResolver.isBaseUrlConfigurationHealthy()
is currently swallowing errors via onErrorReturn(true); change the reactive
chain to insert a doOnError that logs the failure before onErrorReturn(true) so
the fail-open behavior remains but failures are observable. Locate the usage of
secureBaseUrlResolver.isBaseUrlConfigurationHealthy() in
ConsolidatedAPIServiceCEImpl (the reactive chain that ends with .zipWith(...
.onErrorReturn(true))) and add .doOnError(e -> log.warn("secureBaseUrl
resolution failed, falling back to healthy=true", e)) immediately before the
.onErrorReturn(true) call, preserving the existing onErrorReturn(true).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java`:
- Line 224: The call to secureBaseUrlResolver.isBaseUrlConfigurationHealthy() is
currently swallowing errors via onErrorReturn(true); change the reactive chain
to insert a doOnError that logs the failure before onErrorReturn(true) so the
fail-open behavior remains but failures are observable. Locate the usage of
secureBaseUrlResolver.isBaseUrlConfigurationHealthy() in
ConsolidatedAPIServiceCEImpl (the reactive chain that ends with .zipWith(...
.onErrorReturn(true))) and add .doOnError(e -> log.warn("secureBaseUrl
resolution failed, falling back to healthy=true", e)) immediately before the
.onErrorReturn(true) call, preserving the existing onErrorReturn(true).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8974a373-e850-4203-a287-ebd6851c2269

📥 Commits

Reviewing files that changed from the base of the PR and between 982c4fc and b6bbdcc.

📒 Files selected for processing (3)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java
  • app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java
  • app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/ConsolidatedAPIServiceCECompatibleImpl.java

…mplTest (GHSA-j9gf-vw2f-9hrw)

The previous refactor (b6bbdcc) 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
…HSA-j9gf-vw2f-9hrw)

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
…dex stacking (GHSA-j9gf-vw2f-9hrw)

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) → <header data-testid="t--appsmith-page-header">`.

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 <BaseUrlMissingBanner /> alongside the existing <Banner />,
  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
@github-actions
Copy link
Copy Markdown

Failed server tests

  • com.appsmith.server.imports.internal.ImportServiceTests#importArtifactFromValidJsonFileTest

@subrata71 subrata71 requested a review from sondermanish May 4, 2026 09:59
Remove design specs and implementation plans that were used during
development but should not ship in the final PR.

Refs GHSA-j9gf-vw2f-9hrw
@subrata71 subrata71 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels May 5, 2026
@subrata71 subrata71 changed the title feat(security): admin warning banner for unset APPSMITH_BASE_URL (GHSA-j9gf-vw2f-9hrw) fix(security): fail closed when APPSMITH_BASE_URL unset for token-bearing emails (GHSA-j9gf-vw2f-9hrw) May 6, 2026
@subrata71 subrata71 merged commit e498a7b into release May 6, 2026
158 of 161 checks passed
@subrata71 subrata71 deleted the feat/admin-base-url-warning-banner-ghsa-j9gf branch May 6, 2026 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Required label for CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants