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
… + 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
|
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 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
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
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
app/client/src/selectors/usersSelectors.test.ts (1)
4-5: Avoidas neverin 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: PreferMemoryRouteroverBrowserRouterin this unit test.Line 7 and Line 20 can use
MemoryRouterto 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
bashorshellfor 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.mdaround 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., prependbash 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 toplain 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
📒 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.ymlapp/client/cypress/support/commands.jsapp/client/src/ce/AppRouter.tsxapp/client/src/ce/constants/messages.tsapp/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsxapp/client/src/components/editorComponents/BaseUrlMissingBanner.tsxapp/client/src/constants/userConstants.tsapp/client/src/selectors/usersSelectors.test.tsapp/client/src/selectors/usersSelectors.tsxapp/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/UserProfileCE_DTO.javaapp/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/UserServiceTest.javaapp/server/appsmith-server/src/test/resources/application-test.propertiesdocs/superpowers/plans/2026-04-29-admin-base-url-warning-banner.mddocs/superpowers/specs/2026-04-28-ci-base-url-config-design.mddocs/superpowers/specs/2026-04-29-admin-base-url-warning-banner-design.mdscripts/deploy_preview.shscripts/local_testing.sh
…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
There was a problem hiding this comment.
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 winAdd 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
📒 Files selected for processing (8)
app/client/src/components/editorComponents/BaseUrlMissingBanner.test.tsxapp/client/src/selectors/usersSelectors.test.tsapp/client/src/selectors/usersSelectors.tsxapp/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/OrganizationConfigurationCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/OrganizationServiceImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.javaapp/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
| // 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); | ||
| }); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (1)
224-224: ⚡ Quick winLog fallback path before
onErrorReturn(true).Right now resolver failures are silently converted to
true. Add adoOnErrorhere 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
📒 Files selected for processing (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.javaapp/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
Failed server tests
|
Remove design specs and implementation plans that were used during development but should not ship in the final PR. Refs GHSA-j9gf-vw2f-9hrw
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_URLis 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 thelicense_multi_org_enabledshort-circuit in the EE resolver override (shadow EE PR linked below).The banner deep-links to Admin Settings → Configuration where
APPSMITH_BASE_URLis 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
SecureBaseUrlResolverCE#isBaseUrlConfigurationHealthy()exposes the health signal. CE returnstrueiffAPPSMITH_BASE_URLis set; EE override (in shadow EE PR) returnstrueunconditionally whenlicense_multi_org_enabledis on. Wired intoConsolidatedAPIServiceCEImplalongside the existing org-config fetch so it runs in parallel (no sequential cost). Result populates a new transient boolean fieldinstanceBaseUrlConfigurationHealthyonOrganizationConfigurationCE.BaseUrlMissingBanner.tsxcomponent renders via the ADSBannercomponent (same pattern asPageBannerMessage). SelectorgetShouldShowBaseUrlMissingBannergates onisSuperUser && adminSettingsVisible && instanceBaseUrlConfigurationHealthy === false. The explicit=== false(rather than!value) is the rolling-deploy guard. Mounted insidePageHeader.tsxalongside the existing license/trial banner.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
SecureBaseUrlResolverCEImplTest— 2 new cases forisBaseUrlConfigurationHealthy()(true when set, false when blank); 18/18 total passusersSelectors.test.ts— 6 new cases pinning all gating dimensions plus the rolling-deploy=== falseguardBaseUrlMissingBanner.test.tsx— 5 new Jest cases covering all gating dimensionsConsolidatedAPIServiceImplTest— mock wired for resolverAPPSMITH_BASE_URL=http://localhost, banner never fires by constructionCompanion / 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.AllSpec:
Tue, 05 May 2026 07:40:44 UTC