fix: add INFO-level logging to critical code paths for RCA diagnosability - #42082
fix: add INFO-level logging to critical code paths for RCA diagnosability#42082subrata71 wants to merge 3 commits into
Conversation
…lity Authentication, datasource, Git, import/export, and startup code paths either had no logging at all or logged critical failures at DEBUG level (invisible in production where INFO is the floor). An RCA agent working from customer-shared logs had zero signal for the most common failure scenarios. Changes: - Auth failure/access denied: log source, path, and reason at WARN - Startup errors: elevate from DEBUG to WARN/ERROR (were invisible) - Redis config: log scheme/host/port at startup for connectivity RCA - Git fetch: preserve stack traces (2 sites still used getMessage()) - Git merge: log failure with repo/source/dest + success with duration - Git pull: add entry/exit INFO logs with timing in CentralGitService - Datasource connection: log on creation failure with datasourceId - Export: add entry/exit INFO logs (was 0 log statements in 346 lines) - Import: add entry INFO log with workspaceId and artifactType - Stopwatch: add stopAndLogTimeAtInfoLevel() for production visibility All additions are at INFO or WARN level — visible on Grafana in production without any configuration change.
WalkthroughThe changes add structured logging for Git operations, artifact processing, authentication failures, startup events, Redis initialization, and datasource connection errors. They also add operation timing support and CRLF sanitization for authentication metrics. ChangesOperational logging
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/InstanceConfig.java (1)
51-51: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPreserve the registration exception as a throwable.
This
onErrorResumeconsumes the error withMono.empty(). The warning is the only diagnostic for the registration failure.errorSignal.getMessage()may be null and does not include the cause.Proposed fix
- log.warn("Instance registration failed: error={}", errorSignal.getMessage()); + log.warn("Instance registration failed: error={}", errorSignal.getMessage(), errorSignal);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/InstanceConfig.java` at line 51, Update the registration failure handling in the onErrorResume flow of InstanceConfig to pass the original errorSignal throwable to log.warn alongside the message, preserving the exception cause in the warning before returning Mono.empty().app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)
1987-1988: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMeasure pull duration from subscription time.
pullStartTimeis created while theMonois assembled, not when it is subscribed. A delayed or repeated subscription can reuse a stale timestamp.System.currentTimeMillis()is also wall-clock time, so clock corrections can distort elapsed time. Capture the timestamp insideMono.deferand useSystem.nanoTime()or Reactorelapsed().Also applies to: 2023-2032
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java` around lines 1987 - 1988, Update the pull timing flow around pullStartTime so the start timestamp is captured per subscription inside Mono.defer rather than during Mono assembly. Use System.nanoTime() or Reactor elapsed timing to measure duration monotonically, and apply the same correction to the related logic covering the reported subsequent lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java`:
- Around line 1315-1321: Update the merge flow around the GitAPIException
handler and outer Mono.using boundary so every failure, including Git.open,
cleanup, timeout, reset, and recovery errors, is logged before recovery.
Preserve and propagate the original Throwable when onErrorResume handles
failures with keepWorkingDirChanges disabled instead of replacing it with a new
Throwable or converting the failure to a successful String.
- Line 1390: Move the fetch-failure logging from the inner reactive operation to
doOnError placed after the complete Mono.using(...).timeout(...) chain in both
overloads. Ensure the first overload logs repoSuffix, branchName, and
isFetchAll, while the second logs repoSuffix, refType, refNames, and isFetchAll,
so all boundary failures are captured.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AccessDeniedHandlerCE.java`:
- Line 16: Apply one redaction policy across AccessDeniedHandlerCE.java (line
16), AuthenticationFailureHandlerCE.java (line 32), and
DatasourceContextServiceCEImpl.java (lines 238-242): replace raw exception or
request message values used in logs, redirects, and the errorMessage Micrometer
tag with stable error codes plus bounded, sanitized context, ensuring secrets,
PII, plugin arguments, and upstream response bodies are excluded. Update
handleErrorRedirect message handling consistently and add tests covering those
sensitive values.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/exports/internal/ExportServiceCEImpl.java`:
- Around line 96-97: Update the export flow around ExportServiceCEImpl so the
branch recorded by the “Export started” log matches the branch used by
findExistingArtifactByIdAndBranchName: pass branchName to the lookup when branch
selection is required, or log the effective branch value returned/used when the
lookup intentionally remains branch-agnostic.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java`:
- Around line 2028-2033: Update the doOnError logging in the Git pull flow to
pass the caught error object as the final log.warn argument while retaining
error.getMessage() in the message parameters. Preserve the existing artifact,
branch, duration, and error-message fields.
---
Nitpick comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/InstanceConfig.java`:
- Line 51: Update the registration failure handling in the onErrorResume flow of
InstanceConfig to pass the original errorSignal throwable to log.warn alongside
the message, preserving the exception cause in the warning before returning
Mono.empty().
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java`:
- Around line 1987-1988: Update the pull timing flow around pullStartTime so the
start timestamp is captured per subscription inside Mono.defer rather than
during Mono assembly. Use System.nanoTime() or Reactor elapsed timing to measure
duration monotonically, and apply the same correction to the related logic
covering the reported subsequent lines.
🪄 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: e58d2a30-cec4-40b7-ac13-463188886a1f
📒 Files selected for processing (10)
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.javaapp/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/Stopwatch.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AccessDeniedHandlerCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/configurations/InstanceConfig.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/configurations/RedisConfig.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/exports/internal/ExportServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java
| } catch (GitAPIException e) { | ||
| log.warn( | ||
| "Git merge failed: repo={}, source={}, dest={}", | ||
| repoSuffix, | ||
| sourceBranch, | ||
| destinationBranch, | ||
| e); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file='app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java'
printf '%s\n' '--- target context ---'
sed -n '1240,1365p' "$file"
printf '%s\n' '--- relevant declarations and calls ---'
rg -n -C 4 'onErrorResume|Mono\.using|merge\(\)|GitAPIException|merge failed|timeout|Timeout' "$file"
printf '%s\n' '--- method outline ---'
ast-grep outline "$file" | sed -n '1,180p'Repository: appsmithorg/appsmith
Length of output: 47091
🏁 Script executed:
#!/bin/bash
set -eu
target='app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java'
printf '%s\n' '--- merge implementations and overrides ---'
rg -n -C 8 'mergeBranch\s*\(' app/server/appsmith-git
printf '%s\n' '--- merge callers and result handling ---'
rg -n -C 5 '\.mergeBranch\(' app/server --glob '*.java'
printf '%s\n' '--- tests for mergeBranch and failure handling ---'
rg -n -C 6 'mergeBranch|Git merge failed|hard resetting to latest commit|FS_MERGE' app/server --glob '*Test*.java' --glob '*.java'
printf '%s\n' '--- exact operator ordering ---'
sed -n '1284,1352p' "$target"Repository: appsmithorg/appsmith
Length of output: 50376
Attach merge failure logging to every failure path.
The GitAPIException catch does not cover failures from Git.open, Mono.using cleanup, or timeout. When keepWorkingDirChanges is false, onErrorResume converts merge failures into a successful String after reset and replaces reset failures with a new Throwable. Log before recovery and at the outer Mono.using boundary, and preserve the original Throwable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java`
around lines 1315 - 1321, Update the merge flow around the GitAPIException
handler and outer Mono.using boundary so every failure, including Git.open,
cleanup, timeout, reset, and recovery errors, is logged before recovery.
Preserve and propagate the original Throwable when onErrorResume handles
failures with keepWorkingDirChanges disabled instead of replacing it with a new
Throwable or converting the failure to a successful String.
| }) | ||
| .onErrorResume(error -> { | ||
| log.error(error.getMessage()); | ||
| log.error("Git fetch failed: repo={}", repoSuffix, error); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file='app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java'
printf '%s\n' '--- target methods and surrounding code ---'
sed -n '1320,1495p' "$file"
printf '%s\n' '--- declarations and call sites ---'
rg -n -C 3 'Git fetch failed|fetch\(|Mono\.using|\.timeout\(' "$file"
printf '%s\n' '--- related CE/EE implementations ---'
rg -n -C 3 'Git fetch failed|fetch\(' app/server/appsmith-git/src/main/java/com/appsmith/git/handlerRepository: appsmithorg/appsmith
Length of output: 45889
🏁 Script executed:
#!/bin/bash
set -eu
file='app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java'
sed -n '1320,1495p' "$file"
rg -n -C 3 'Git fetch failed|fetch\(|Mono\.using|\.timeout\(' "$file"
rg -n -C 3 'Git fetch failed|fetch\(' app/server/appsmith-git/src/main/java/com/appsmith/git/handlerRepository: appsmithorg/appsmith
Length of output: 45771
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Reactor dependency versions ---'
rg -n -C 2 'reactor-core|spring-boot-starter-webflux|spring-boot' app/server/pom.xml app/server/appsmith-git/pom.xml pom.xml 2>/dev/null || true
printf '%s\n' '--- fetch DTO and ref types ---'
rg -n -C 4 'class FetchRemoteDTO|record FetchRemoteDTO|enum RefType|getRefNames|getRefType|getIsFetchAll' app/server
printf '%s\n' '--- complete push logging pattern ---'
sed -n '250,390p' app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java
printf '%s\n' '--- all fetchRemote declarations and overrides ---'
rg -n -C 5 'fetchRemote\s*\(' app/server/appsmith-gitRepository: appsmithorg/appsmith
Length of output: 50377
🌐 Web query:
Project Reactor Mono.using resource supplier cleanup error onErrorResume timeout operator error propagation official documentation
💡 Result:
In Project Reactor, Mono.using is designed to manage resources with guaranteed cleanup, but its behavior during error scenarios is strictly defined to prioritize resource integrity [1][2]. Resource Cleanup and Error Handling When using Mono.using (or Mono.usingWhen for asynchronous cleanup), the cleanup handler is invoked when the sequence terminates—whether by completion, error, or cancellation [1][2]. Crucially, if the resource cleanup process itself fails (i.e., the cleanup Publisher or logic throws an exception), this failure is treated as a critical event [3][4]. In such cases, the cleanup failure will override any previous terminal signal, resulting in a lone onError being propagated downstream, while any data from the main Mono is discarded [3][2][4]. Interaction with timeout and onErrorResume 1. Timeout Propagation: The timeout operator triggers a TimeoutException when the specified duration is exceeded [5]. This is a terminal event for the sequence it wraps [6]. 2. Cleanup Execution: Because timeout causes the sequence to terminate (via cancellation of the upstream), the using operator's cleanup handler is guaranteed to execute, as it triggers upon cancellation [1][2]. 3. Error Handling with onErrorResume: If a timeout occurs, the onErrorResume operator can catch the resulting TimeoutException to switch to a fallback publisher [7][8]. Important Considerations - Cleanup Failures: If your cleanup logic itself is prone to error, ensure it is robust. An error in the cleanup block will mask the original cause of termination (such as a timeout) by propagating a new error downstream [3][4]. - Orphaned Exceptions: If a timeout occurs, the main sequence is cancelled. If the underlying task continues to run (e.g., in a separate thread) and subsequently fails, that secondary exception might be dropped or logged as an onErrorDropped because the main sequence has already terminated [5]. - Resource Management: Mono.usingWhen is generally preferred over Mono.using when the cleanup operation is itself asynchronous (e.g., closing a network connection) [3][9]. For complex scenarios, use doOnDiscard to handle potential resource leaks if elements are discarded due to upstream errors or cancellations [10][11]. Always ensure your cleanup logic is exception-safe to prevent it from interfering with your error recovery strategy [3].
Citations:
- 1: https://projectreactor.io/docs/core/release/api/reactor/core/publisher/Mono.html
- 2: https://docs.spring.io/projectreactor/reactor-core/docs/current/api/reactor/core/publisher/Mono.html
- 3: https://github.com/reactor/reactor-core/blob/main/reactor-core/src/main/java/reactor/core/publisher/Mono.java
- 4: https://projectreactor.io/docs/core/3.4.33/api/reactor/core/publisher/Mono.html
- 5: https://stackoverflow.com/questions/51433079/operator-called-default-onerrordropped-on-mono-timeout
- 6: https://docs.spring.io/projectreactor/reactor-core/docs/current-SNAPSHOT/reference/html/coreFeatures/error-handling.html
- 7: https://projectreactor.io/docs/core/release/reference/coreFeatures/error-handling.html
- 8: Memory leak with webclient timeout reactor/reactor-netty#2500
- 9: https://stackoverflow.com/questions/75190658/how-can-i-use-the-try-with-resources-patter-in-reactor-if-resource-is-a-mono
- 10: Cleanup handler is not subscribed/called if Mono/FluxUsingWhen is cancelled before resource is emitted reactor/reactor-core#2769
- 11: timeout does not support onErrorContinue reactor/reactor-core#1670
Log fetch failures after the full reactive boundary.
Move each log to doOnError after Mono.using(...) and .timeout(...). Include branchName and isFetchAll in the first overload. Include refType, refNames, and isFetchAll in the second overload. This also logs timeout, Git.open, configuration, and Git.close failures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java`
at line 1390, Move the fetch-failure logging from the inner reactive operation
to doOnError placed after the complete Mono.using(...).timeout(...) chain in
both overloads. Ensure the first overload logs repoSuffix, branchName, and
isFetchAll, while the second logs repoSuffix, refType, refNames, and isFetchAll,
so all boundary failures are captured.
| @Override | ||
| public Mono<Void> handle(ServerWebExchange exchange, AccessDeniedException denied) { | ||
| return Mono.fromRunnable(() -> { | ||
| log.warn("Access denied: path={}, reason={}", exchange.getRequest().getPath(), denied.getMessage()); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
for f in \
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AccessDeniedHandlerCE.java \
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.java \
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java
do
echo "===== $f ====="
sed -n '1,90p' "$f"
if [[ "$f" == *DatasourceContextServiceCEImpl.java ]]; then
sed -n '200,260p' "$f"
fi
done
echo "===== related logging/sanitization symbols ====="
rg -n --glob '*.java' \
'sanitize|redact|mask|AccessDeniedHandlerCE|AuthenticationFailureHandlerCE|DatasourceContextServiceCEImpl|log\.(warn|error)\(' \
app/server/appsmith-server/src/main/java/app/server 2>/dev/null || true
rg -n --glob '*.java' \
'sanitize|redact|mask|AccessDeniedHandlerCE|AuthenticationFailureHandlerCE|DatasourceContextServiceCEImpl|log\.(warn|error)\(' \
app/server 2>/dev/null | head -300Repository: appsmithorg/appsmith
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -eu
for f in \
app/server/appsmith-server/src/main/java/appsmith/server/authentication/handlers/ce/AccessDeniedHandlerCE.java \
app/server/appsmith-server/src/main/java/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.java \
app/server/appsmith-server/src/main/java/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java
do
echo "===== $f ====="
sed -n '1,90p' "$f"
if [[ "$f" == *DatasourceContextServiceCEImpl.java ]]; then
sed -n '200,260p' "$f"
fi
done
echo "===== related logging and sanitization symbols ====="
rg -n --glob '*.java' \
'sanitize|redact|mask|AccessDeniedHandlerCE|AuthenticationFailureHandlerCE|DatasourceContextServiceCEImpl|log\.(warn|error)\(' \
app/server | head -300Repository: appsmithorg/appsmith
Length of output: 435
🏁 Script executed:
#!/bin/bash
set -eu
echo "===== authentication exception construction ====="
rg -n --glob '*.java' \
'new (AccessDeniedException|OAuth2AuthenticationException|AuthenticationException)|throw .*AuthenticationException|AccessDeniedException\(' \
app/server/appsmith-server/src/main/java app/server/appsmith-server/src/test/java | head -240
echo "===== datasource error types and message construction ====="
rg -n --glob '*.java' \
'class (AppsmithPluginException|StaleConnectionException)|extends .*Exception|AppsmithPluginException\(|throw .*AppsmithPluginException|new AppsmithPluginException' \
app/server | head -240
echo "===== handler and datasource tests ====="
rg -n --glob '*Test.java' \
'AccessDeniedHandlerCE|AuthenticationFailureHandlerCE|DatasourceContextService|LOGIN_FAILURE|Datasource connection creation failed' \
app/server/appsmith-server/src/test | head -240
echo "===== logging configuration ====="
rg -n --glob '*.{xml,yml,yaml,properties}' \
'logback|logging\.|appender|console|pattern|message' \
app/server | head -180
echo "===== deterministic source scan for the three call sites ====="
python3 - <<'PY'
from pathlib import Path
files = [
Path("app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AccessDeniedHandlerCE.java"),
Path("app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.java"),
Path("app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java"),
]
for path in files:
text = path.read_text()
print(path)
for i, line in enumerate(text.splitlines(), 1):
if "getMessage()" in line or "counter(LOGIN_FAILURE" in line:
print(f" {i}: {line.strip()}")
PYRepository: appsmithorg/appsmith
Length of output: 45098
🏁 Script executed:
#!/bin/bash
set -eu
echo "===== authentication exception construction ====="
rg -n --glob '*.java' \
'new (AccessDeniedException|OAuth2AuthenticationException|AuthenticationException)|throw .*AuthenticationException|AccessDeniedException\(' \
app/server/appsmith-server/src/main/java app/server/appsmith-server/src/test/java | head -240
echo "===== datasource error types and message construction ====="
rg -n --glob '*.java' \
'class (AppsmithPluginException|StaleConnectionException)|extends .*Exception|AppsmithPluginException\(|throw .*AppsmithPluginException|new AppsmithPluginException' \
app/server | head -240
echo "===== handler and datasource tests ====="
rg -n --glob '*Test.java' \
'AccessDeniedHandlerCE|AuthenticationFailureHandlerCE|DatasourceContextService|LOGIN_FAILURE|Datasource connection creation failed' \
app/server/appsmith-server/src/test | head -240
echo "===== logging configuration ====="
rg -n --glob '*.{xml,yml,yaml,properties}' \
'logback|logging\.|appender|console|pattern|message' \
app/server | head -180
echo "===== deterministic source scan for the three call sites ====="
python3 - <<'PY'
from pathlib import Path
files = [
Path("app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AccessDeniedHandlerCE.java"),
Path("app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.java"),
Path("app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java"),
]
for path in files:
print(path)
for i, line in enumerate(path.read_text().splitlines(), 1):
if "getMessage()" in line or "counter(LOGIN_FAILURE" in line:
print(f" {i}: {line.strip()}")
PYRepository: appsmithorg/appsmith
Length of output: 45352
🏁 Script executed:
#!/bin/bash
set -eu
echo "===== exception message formatting ====="
for f in \
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/exceptions/BaseException.java \
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/exceptions/pluginExceptions/AppsmithPluginException.java \
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/exceptions/pluginExceptions/StaleConnectionException.java
do
echo "--- $f"
cat -n "$f"
done
echo "===== plugin error definitions ====="
rg -l --glob '*.java' \
'interface BasePluginError|enum AppsmithPluginError|enum .*PluginError' \
app/server/appsmith-interfaces app/server/appsmith-plugins | head -30
for f in $(rg -l --glob '*.java' 'enum AppsmithPluginError' app/server/appsmith-interfaces app/server/appsmith-plugins | head -5); do
echo "--- $f"
sed -n '1,220p' "$f"
done
echo "===== authentication wiring and message sources ====="
rg -n --glob '*.java' \
'AuthenticationFailureHandlerCE|ServerAccessDeniedHandler|OAuth2AuthenticationException|AuthenticationException' \
app/server/appsmith-server/src/main/java | head -220
echo "===== representative plugin response/message propagation ====="
rg -n --glob '*.java' \
'errorMessage|response body|responseBody|e\.getMessage\(\)|error\.getMessage\(\)' \
app/server/appsmith-plugins app/server/appsmith-interfaces/src/main/java \
| head -260
echo "===== focused verifier for dynamic log and metric values ====="
python3 - <<'PY'
from pathlib import Path
targets = {
"AccessDeniedHandlerCE.java": Path("app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AccessDeniedHandlerCE.java"),
"AuthenticationFailureHandlerCE.java": Path("app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.java"),
"DatasourceContextServiceCEImpl.java": Path("app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java"),
}
needles = ("log.warn", "log.error", ".counter(")
for name, path in targets.items():
print(name)
lines = path.read_text().splitlines()
for number, line in enumerate(lines, 1):
if any(needle in line for needle in needles):
print(f"{number}: {line.strip()}")
for following in lines[number:number+5]:
stripped = following.strip()
if stripped and not stripped.startswith(("}", "return")):
print(f" {stripped}")
PYRepository: appsmithorg/appsmith
Length of output: 50376
Apply one redaction policy to dynamic error data.
Exception messages can contain plugin arguments or upstream response bodies. errorMessage is also used as an unbounded Micrometer metric tag. Replace raw values with stable error codes and bounded, sanitized context. Apply the same rule to the request message used by handleErrorRedirect. Add tests for secrets, PII, and upstream response bodies.
📍 Affects 3 files
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AccessDeniedHandlerCE.java#L16-L16(this comment)app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.java#L32-L32app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java#L238-L242
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AccessDeniedHandlerCE.java`
at line 16, Apply one redaction policy across AccessDeniedHandlerCE.java (line
16), AuthenticationFailureHandlerCE.java (line 32), and
DatasourceContextServiceCEImpl.java (lines 238-242): replace raw exception or
request message values used in logs, redirects, and the errorMessage Micrometer
tag with stable error codes plus bounded, sanitized context, ensuring secrets,
PII, plugin arguments, and upstream response bodies are excluded. Update
handleErrorRedirect message handling consistently and add tests covering those
sensitive values.
| log.info("Export started: artifactId={}, branchName={}, artifactType={}", artifactId, branchName, artifactType); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the logged branch consistent with the export lookup.
The new log records branchName, but Line [134] passes null to findExistingArtifactByIdAndBranchName. A branch-specific request can therefore produce a log for a branch that the export did not query. Pass branchName to the lookup if branch selection is required. Otherwise, log the effective branch value instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/exports/internal/ExportServiceCEImpl.java`
around lines 96 - 97, Update the export flow around ExportServiceCEImpl so the
branch recorded by the “Export started” log matches the branch used by
findExistingArtifactByIdAndBranchName: pass branchName to the lookup when branch
selection is required, or log the effective branch value returned/used when the
lookup intentionally remains branch-agnostic.
| .doOnError(error -> log.warn( | ||
| "Git pull failed: artifactId={}, branch={}, durationMs={}, error={}", | ||
| baseArtifactId, | ||
| branchName, | ||
| System.currentTimeMillis() - pullStartTime, | ||
| error.getMessage())) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
file="app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java"
printf '%s\n' '--- target lines ---'
sed -n '2005,2040p' "$file"
printf '%s\n' '--- logger declaration/imports ---'
rg -n '^(import .*Logger|import .*Slf4j|.*LoggerFactory|.*log\.warn)' "$file" | head -80
printf '%s\n' '--- logging dependencies/configuration ---'
rg -n 'slf4j|logback|log4j|spring-boot-starter-logging' app/server -g 'pom.xml' -g '*.xml' -g '*.java' | head -120Repository: appsmithorg/appsmith
Length of output: 17797
🏁 Script executed:
#!/bin/bash
set -e
file="app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java"
sed -n '2005,2040p' "$file"
rg -n '^(import .*Logger|import .*Slf4j|.*LoggerFactory|.*log\.warn)' "$file" | head -80
rg -n 'slf4j|logback|log4j|spring-boot-starter-logging' app/server -g 'pom.xml' -g '*.xml' -g '*.java' | head -120Repository: appsmithorg/appsmith
Length of output: 17601
🌐 Web query:
SLF4J Logger API trailing Throwable final argument parameterized logging placeholders
💡 Result:
In SLF4J (since version 1.6.0), you can use parameterized logging in the presence of an exception or Throwable, provided the Throwable is passed as the final argument [1][2]. When the last argument in a logging call is an instance of Throwable, SLF4J automatically treats it as a throwable to be logged with its stack trace, rather than as a simple object to be formatted into the string placeholders [1][3]. Key points regarding this behavior: - Placement: The Throwable must be the final argument [1]. If it is placed anywhere else in the argument list, it will be treated as a standard object and its stack trace will not be printed [1][2]. - Placeholders: The presence of the Throwable does not affect how the other placeholders ({}) are filled. SLF4J will fill the placeholders with the preceding arguments and then handle the final Throwable separately [1][3]. - Fluent API (SLF4J 2.x): For more explicit control, especially when you need to avoid potential ambiguities or want to ensure a Throwable is treated as such without relying on argument position, SLF4J 2.0+ provides a fluent API where you can set the cause explicitly using.setCause(throwable) [4][5][2]. Example: logger.error("Failed to process item {} for user {}", itemId, userId, exception); In this example, {} placeholders are replaced by itemId and userId, and the exception is logged with its full stack trace [1][3].
Citations:
- 1: https://www.slf4j.org/faq.html
- 2: https://www.baeldung.com/slf4j-parameterized-logging
- 3: https://www.baeldung.com/slf4j-log-exceptions
- 4: Message formatting with last argument being Throwable qos-ch/slf4j#390
- 5: mix logger.error(String format, Object... arguments) and logger.error(String msg, Throwable t) qos-ch/slf4j#296
Preserve the pull exception in the WARN log. Pass error as the final argument to log.warn while retaining error.getMessage(). This preserves the stack trace and cause context for RCA.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java`
around lines 2028 - 2033, Update the doOnError logging in the Git pull flow to
pass the caught error object as the final log.warn argument while retaining
error.getMessage() in the message parameters. Preserve the existing artifact,
branch, duration, and error-message fields.
- Security: remove raw exception messages from auth logs to avoid PII/secrets leakage; log only error class name and request path - Git fetch: move logging from inner onErrorResume to doOnError after timeout chain so timeout errors are also captured; add branch/refType context to each overload - Git merge: add WARN log in the onErrorResume recovery path (where errors were silently converted to a success string after reset) - Git pull: pass error as trailing SLF4J argument to preserve stack trace - Export: remove branchName from log since lookup is branch-agnostic - Datasource: pass exception as trailing argument for full stack trace
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
…-117) The OAuth2 error code originates from the attacker-controlled 'error' query parameter in the OAuth2 callback. Without sanitization, an attacker can inject CRLF characters to forge log entries, misleading SIEM tools and administrators. Strip \r and \n from the error code before it reaches the log statement and the Micrometer metric tag. Adds a regression test that verifies CRLF characters are replaced with underscores.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCETest.java`:
- Around line 32-43: The test should construct OAuth2AuthenticationException
using the error-derived message with a CRLF-containing description, then verify
the recorded metric’s message tag contains neither carriage returns nor
newlines. Update the onAuthenticationFailure metric-tag handling to sanitize
exception.getMessage() before recording it, and extend
AuthenticationFailureHandlerCETest to assert the sanitized message value while
retaining the source assertion.
🪄 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: 7d9b44b8-f7f5-42f2-86d1-4be9050cbd8c
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCETest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.java
| String maliciousErrorCode = "invalid_grant\r\nINFO: Forged log entry"; | ||
| OAuth2Error error = new OAuth2Error(maliciousErrorCode, "some description", null); | ||
| OAuth2AuthenticationException exception = new OAuth2AuthenticationException(error, "auth failed"); | ||
|
|
||
| // The handler should not throw — it sanitizes and logs safely | ||
| handler.onAuthenticationFailure(null, exception).block(); | ||
|
|
||
| // Verify the metric tag was recorded with sanitized source (no CRLF) | ||
| String recordedSource = | ||
| meterRegistry.get("appsmith.login_failure").counter().getId().getTag("source"); | ||
| assertThat(recordedSource).doesNotContain("\r").doesNotContain("\n"); | ||
| assertThat(recordedSource).isEqualTo("invalid_grant__INFO: Forged log entry"); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'spring-security|spring-boot-starter-security' app/server --glob 'pom.xml' || true
rg -n -C 6 'exception\.getMessage\(\)|counter\(.*message|log\.warn|OAuth2AuthenticationException' \
app/server/appsmith-server/src/main app/server/appsmith-server/src/testRepository: appsmithorg/appsmith
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
test_file="$(fd -t f 'AuthenticationFailureHandlerCETest.java' . | head -n 1)"
handler_file="$(fd -t f 'AuthenticationFailureHandlerCE.java' . | head -n 1)"
printf '%s\n' "TEST=$test_file" "HANDLER=$handler_file"
ast-grep outline "$test_file" --lang java
ast-grep outline "$handler_file" --lang java
printf '\n--- test ---\n'
cat -n "$test_file"
printf '\n--- handler ---\n'
cat -n "$handler_file"
printf '\n--- focused dependency declarations ---\n'
rg -n -C 4 'spring-security|spring-boot-starter-security' app/server --glob 'pom.xml' | head -n 160Repository: appsmithorg/appsmith
Length of output: 9426
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- managed Spring versions ---'
rg -n -C 3 'spring-boot.version|spring-security.version|spring-security' \
app/server/pom.xml app/server/appsmith-server/pom.xml pom.xml 2>/dev/null | head -n 200
printf '%s\n' '--- authentication failure handler references ---'
rg -n -C 5 'handleErrorRedirect|onAuthenticationFailure|AuthenticationFailureHandlerCE' \
app/server/appsmith-server/src/main app/server/appsmith-server/src/test \
--glob '*.java' | head -n 320
printf '%s\n' '--- exact metric tag construction ---'
rg -n -C 3 'LOGIN_FAILURE|appsmith\.login_failure' \
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication \
app/server/appsmith-server/src/test/java/com/appsmith/server/authentication \
--glob '*.java' | head -n 240Repository: appsmithorg/appsmith
Length of output: 1874
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- POM files ---'
git ls-files '*pom.xml' | head -n 120
printf '%s\n' '--- Spring version declarations ---'
for f in $(git ls-files '*pom.xml'); do
rg -n -H -C 3 'spring-boot.version|spring-security.version|spring-boot-dependencies|spring-security' "$f" || true
done | head -n 260
printf '%s\n' '--- handler references ---'
rg -n -C 5 'handleErrorRedirect|onAuthenticationFailure|AuthenticationFailureHandlerCE' \
app/server/appsmith-server/src/main app/server/appsmith-server/src/test \
--glob '*.java' || true
printf '%s\n' '--- login failure metric construction ---'
rg -n -C 3 'LOGIN_FAILURE|appsmith\.login_failure' \
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication \
app/server/appsmith-server/src/test/java/com/appsmith/server/authentication \
--glob '*.java' || trueRepository: appsmithorg/appsmith
Length of output: 23127
🌐 Web query:
Spring Security 6.4 OAuth2AuthenticationException.java constructor OAuth2Error getDescription message
💡 Result:
In Spring Security 6.4, the OAuth2AuthenticationException class provides multiple constructors to handle OAuth 2.0-related authentication errors [1][2]. These constructors often accept an OAuth2Error object, which serves as a representation of the OAuth 2.0 error [3]. When an OAuth2AuthenticationException is constructed using only an OAuth2Error object, the exception's detail message is automatically populated using the description retrieved from that error [4]. Specifically, the implementation performs the following: public OAuth2AuthenticationException(OAuth2Error error) { this(error, error.getDescription); } In this context, the error.getDescription method returns the error description associated with the OAuth2Error instance [3][5]. If the description is not available in the OAuth2Error, it may return null [6][7], which is then passed as the message to the superclass constructor (AuthenticationException) [4]. Other available constructors in Spring Security 6.4 allow for explicit messaging, such as: - OAuth2AuthenticationException(OAuth2Error error, String message) [1][8] - OAuth2AuthenticationException(OAuth2Error error, String message, Throwable cause) [1][8] These constructors permit you to override or provide a custom detail message independently of the description contained within the OAuth2Error object [8].
Citations:
- 1: https://docs.spring.io/spring-security/site/docs/6.4.x/api/org/springframework/security/oauth2/core/OAuth2AuthenticationException.html
- 2: https://docs.spring.io/spring-security/site/docs/6.4.4/api/org/springframework/security/oauth2/core/OAuth2AuthenticationException.html
- 3: https://docs.spring.io/spring-security/site/docs/6.4.x/api/org/springframework/security/oauth2/core/OAuth2Error.html
- 4: https://github.com/spring-projects/spring-security/blob/master/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/OAuth2AuthenticationException.java
- 5: https://docs.spring.io/spring-security/site/docs/current/api/org/springframework/security/oauth2/core/OAuth2Error.html
- 6: https://docs.spring.io/spring-security/reference/api/java/org/springframework/security/oauth2/core/OAuth2Error.html
- 7: https://www.springframework.org/spring-security/reference/api/java/org/springframework/security/oauth2/core/OAuth2Error.html
- 8: https://docs.spring.io/spring-security/reference/api/java/org/springframework/security/oauth2/core/OAuth2AuthenticationException.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
handler = Path("app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.java").read_text()
test = Path("app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCETest.java").read_text()
assert re.search(
r'counter\(LOGIN_FAILURE,\s*"source",\s*source,\s*"message",\s*errorMessage\)',
handler,
)
assert re.search(r'String errorMessage = exception\.getMessage\(\);', handler)
assert re.search(
r'log\.warn\(\s*"Authentication failed: source=\{\}, errorCode=\{\}",\s*source,\s*exception\.getClass\(\)\.getSimpleName\(\)',
handler,
re.S,
)
assert 'new OAuth2AuthenticationException(error, "auth failed")' in test
assert 'getTag("message")' not in test
assert 'getTag("source")' in test
print("message metric tag: direct exception.getMessage()")
print("warning log inputs: sanitized source and exception class only")
print("test message: explicit safe literal 'auth failed'")
print("test assertions: source tag only")
PYRepository: appsmithorg/appsmith
Length of output: 353
Test and sanitize the exception message metric tag.
Construct OAuth2AuthenticationException with the error-derived message and a CRLF-containing description. Assert that the message tag contains no CRLF. onAuthenticationFailure currently passes exception.getMessage() directly to the metric, while this test supplies the safe literal "auth failed" and checks only source.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCETest.java`
around lines 32 - 43, The test should construct OAuth2AuthenticationException
using the error-derived message with a CRLF-containing description, then verify
the recorded metric’s message tag contains neither carriage returns nor
newlines. Update the onAuthenticationFailure metric-tag handling to sanitize
exception.getMessage() before recording it, and extend
AuthenticationFailureHandlerCETest to assert the sanitized message value while
retaining the source assertion.
|
@coderabbitai review again please |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java (1)
1315-1321:⚠️ Potential issue | 🟠 MajorThe merge failure boundary still loses failures and can report success.
The handler logs only the inner
GitAPIException.Git.open, timeout,Git::close, and asynchronous reset failures can occur outside this callback. The reset path still returnserror.getMessage()after recovery, soapp/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.javacan continue with a successfulMono<String>after a failed merge. Move failure logging after the completeMono.using(...).timeout(...)chain, log reset errors in the reactive chain, and propagate the original error.Also applies to: 1336-1342
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java` around lines 1315 - 1321, Update the merge flow surrounding the complete Mono.using(...).timeout(...) chain so failures from Git.open, timeout, Git.close, and asynchronous reset are logged after the chain completes rather than only inside the GitAPIException callback. Log reset failures within the reactive chain, and ensure the reset recovery path re-propagates the original error instead of returning error.getMessage() as a successful Mono<String>, preserving failure propagation to CentralGitServiceCEImpl.
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/InstanceConfig.java (1)
51-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the registration failure cause chain.
The warning records only
errorSignal.getMessage(). It loses the exception type and cause chain, which weakens RCA when the message is null or generic. Log an explicit error type and the throwable, subject to the same redaction policy used for operational logs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/InstanceConfig.java` at line 51, Update the registration-failure warning in InstanceConfig to retain the full throwable and explicitly record its error type, rather than logging only errorSignal.getMessage(). Pass the throwable through the existing operational-log redaction policy so the cause chain is preserved without exposing sensitive data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AccessDeniedHandlerCE.java`:
- Line 16: Apply one bounded redaction policy across the affected telemetry: in
AccessDeniedHandlerCE, log a route template or bounded sanitized path instead of
the raw path; in AuthenticationFailureHandlerCE, use fixed metric categories and
bounded sanitized error context; in DatasourceContextServiceCEImpl, redact
exception messages and stack traces before logging. Add
AuthenticationFailureHandlerCETest coverage for null, long, CRLF-containing, and
sensitive values in both telemetry fields.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.java`:
- Around line 33-36: Update the warning log in AuthenticationFailureHandlerCE so
exception.getClass().getSimpleName() is labeled with the exceptionType field
instead of errorCode; preserve source as the existing sanitized code field.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java`:
- Around line 1985-1988: Update the pull flow around lockHandledpullDTOMono so
the start log and timer initialization occur inside Mono.defer, executing only
when the Mono is subscribed. Use System.nanoTime() for pullStartTime and
calculate elapsed duration from that monotonic timestamp, while preserving the
existing pull behavior.
---
Duplicate comments:
In
`@app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java`:
- Around line 1315-1321: Update the merge flow surrounding the complete
Mono.using(...).timeout(...) chain so failures from Git.open, timeout,
Git.close, and asynchronous reset are logged after the chain completes rather
than only inside the GitAPIException callback. Log reset failures within the
reactive chain, and ensure the reset recovery path re-propagates the original
error instead of returning error.getMessage() as a successful Mono<String>,
preserving failure propagation to CentralGitServiceCEImpl.
---
Nitpick comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/InstanceConfig.java`:
- Line 51: Update the registration-failure warning in InstanceConfig to retain
the full throwable and explicitly record its error type, rather than logging
only errorSignal.getMessage(). Pass the throwable through the existing
operational-log redaction policy so the cause chain is preserved without
exposing sensitive data.
🪄 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: 530a05db-a8d7-40e6-ae7f-7b626509a64f
📒 Files selected for processing (11)
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.javaapp/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/Stopwatch.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AccessDeniedHandlerCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/configurations/InstanceConfig.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/configurations/RedisConfig.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/exports/internal/ExportServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCETest.java
| @Override | ||
| public Mono<Void> handle(ServerWebExchange exchange, AccessDeniedException denied) { | ||
| return Mono.fromRunnable(() -> { | ||
| log.warn("Access denied: path={}", exchange.getRequest().getPath()); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
Apply one bounded redaction policy to operational telemetry.
These changes emit request-, provider-, and upstream-controlled values without one common length, cardinality, and redaction policy. CRLF replacement alone does not prevent sensitive data or high-cardinality telemetry.
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AccessDeniedHandlerCE.java#L16-L16: Log a route template or bounded sanitized path instead of the raw request path.app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.java#L27-L40: Use fixed metric categories and keep only bounded sanitized error context in logs.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java#L238-L244: Redact exception messages and stack traces before logging them.app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCETest.java#L30-L44: Add coverage for null, long, CRLF-containing, and sensitive values in both telemetry fields.
Repository verification
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 \
'Access denied: path=|LOGIN_FAILURE|Datasource connection creation failed|errorMessage|doOnError\(e -> log\.error' \
app/server/appsmith-server/src/main/java \
app/server/appsmith-server/src/test/java \
--glob '*.java'📍 Affects 4 files
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AccessDeniedHandlerCE.java#L16-L16(this comment)app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.java#L27-L40app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java#L238-L244app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCETest.java#L30-L44
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AccessDeniedHandlerCE.java`
at line 16, Apply one bounded redaction policy across the affected telemetry: in
AccessDeniedHandlerCE, log a route template or bounded sanitized path instead of
the raw path; in AuthenticationFailureHandlerCE, use fixed metric categories and
bounded sanitized error context; in DatasourceContextServiceCEImpl, redact
exception messages and stack traces before logging. Add
AuthenticationFailureHandlerCETest coverage for null, long, CRLF-containing, and
sensitive values in both telemetry fields.
| log.warn( | ||
| "Authentication failed: source={}, errorCode={}", | ||
| source, | ||
| exception.getClass().getSimpleName()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the correct field name in the warning.
exception.getClass().getSimpleName() is an exception type, not an OAuth2 error code. The current event reports an exception type under errorCode; source already carries the sanitized code. Rename the field to exceptionType.
Proposed fix
- "Authentication failed: source={}, errorCode={}",
+ "Authentication failed: source={}, exceptionType={}",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log.warn( | |
| "Authentication failed: source={}, errorCode={}", | |
| source, | |
| exception.getClass().getSimpleName()); | |
| log.warn( | |
| "Authentication failed: source={}, exceptionType={}", | |
| source, | |
| exception.getClass().getSimpleName()); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationFailureHandlerCE.java`
around lines 33 - 36, Update the warning log in AuthenticationFailureHandlerCE
so exception.getClass().getSimpleName() is labeled with the exceptionType field
instead of errorCode; preserve source as the existing sanitized code field.
| String branchName = branchedGitMetadata.getRefName(); | ||
|
|
||
| log.info("Git pull started: artifactId={}, branch={}", baseArtifactId, branchName); | ||
| long pullStartTime = System.currentTimeMillis(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java"
printf '%s\n' '--- target method context ---'
sed -n '1935,2050p' "$file"
printf '%s\n' '--- reactive timing and pull call sites ---'
rg -n -C 3 'pullStartTime|Git pull started|durationMs|doOnSuccess|doOnError|Mono\.defer|doOnSubscribe' "$file"
printf '%s\n' '--- method outline ---'
ast-grep outline "$file" | sed -n '1,220p'Repository: appsmithorg/appsmith
Length of output: 17457
🏁 Script executed:
#!/bin/bash
set -eu
file="app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java"
sed -n '1935,2050p' "$file"
rg -n -C 3 'pullStartTime|Git pull started|durationMs|doOnSuccess|doOnError|Mono\.defer|doOnSubscribe' "$file"
ast-grep outline "$file" | sed -n '1,220p'Repository: appsmithorg/appsmith
Length of output: 17360
🏁 Script executed:
#!/bin/bash
set -eu
file="app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java"
printf '%s\n' '--- pullArtifact call sites ---'
rg -n -C 4 'pullArtifact\s*\(' app/server --glob '*.java' --glob '*Test.java' | sed -n '1,260p'
printf '%s\n' '--- related pull tests and assertions ---'
rg -n -C 4 'pullArtifact|Git pull started|Git pull completed|Git pull failed|durationMs' app/server --glob '*Test.java' | sed -n '1,320p'
printf '%s\n' '--- return shapes in the pull pipeline ---'
sed -n '2055,2145p' "$file"
printf '%s\n' '--- Reactor and test dependency versions ---'
rg -n -C 2 'reactor-core|reactor-test|spring-boot-starter-webflux|spring-boot' app/server --glob 'pom.xml' --glob '*.xml' | sed -n '1,220p'Repository: appsmithorg/appsmith
Length of output: 32647
🏁 Script executed:
#!/bin/bash
set -eu
file="app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java"
rg -n -C 4 'pullArtifact\s*\(' app/server --glob '*.java' --glob '*Test.java' | sed -n '1,260p'
rg -n -C 4 'pullArtifact|Git pull started|Git pull completed|Git pull failed|durationMs' app/server --glob '*Test.java' | sed -n '1,320p'
sed -n '2055,2145p' "$file"
rg -n -C 2 'reactor-core|reactor-test|spring-boot-starter-webflux|spring-boot' app/server --glob 'pom.xml' --glob '*.xml' | sed -n '1,220p'Repository: appsmithorg/appsmith
Length of output: 33178
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
path = Path("app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java")
text = path.read_text()
start = text.index(" protected Mono<GitPullDTO> pullArtifact(")
end = text.index("\n /**\n * Method to pull the files from remote repo", start)
method = text[start:end]
log_pos = method.index('log.info("Git pull started:')
timer_pos = method.index("long pullStartTime = System.currentTimeMillis();")
subscribe_pos = method.index(".subscribe(")
assert log_pos < subscribe_pos
assert timer_pos < subscribe_pos
assert "Mono.usingWhen(" in method
assert "Mono.defer(" in text[text.index("private Mono<GitPullDTO> pullAndRehydrateArtifact"):end]
print("source_check: start log and wall-clock timestamp precede inner subscription")
class ColdMono:
def __init__(self, events):
self.events = events
def subscribe(self):
self.events.append("work subscribed")
def assemble():
events = ["start logged", "timestamp captured"]
return events, ColdMono(events)
events, mono = assemble()
assert events == ["start logged", "timestamp captured"]
assert "work subscribed" not in events
events.append("time passes before subscription")
mono.subscribe()
assert events[-2:] == ["time passes before subscription", "work subscribed"]
print("behavior_check: assembly side effects occur before delayed subscription")
events, mono = assemble()
assert events == ["start logged", "timestamp captured"]
print("behavior_check: assembly side effects occur even when the returned publisher is never subscribed")
PYRepository: appsmithorg/appsmith
Length of output: 247
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
path = Path("app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java")
text = path.read_text()
protected_start = text.index(" protected Mono<GitPullDTO> pullArtifact(")
protected_end = text.index(
"\n /**\n * Method to pull the files from remote repo", protected_start
)
method = text[protected_start:protected_end]
log_pos = method.index('log.info("Git pull started:')
timer_pos = method.index("long pullStartTime = System.currentTimeMillis();")
subscribe_pos = method.index(".subscribe(")
assert log_pos < subscribe_pos
assert timer_pos < subscribe_pos
assert "Mono.usingWhen(" in method
private_start = text.index(
" private Mono<GitPullDTO> pullAndRehydrateArtifact", protected_end
)
private_end = text.index(
"\n `@Override`\n public Mono<BranchTrackingStatus> fetchRemoteChanges",
private_start
)
private_method = text[private_start:private_end]
assert "return Mono.defer(" in private_method
print("source_check: start logging and wall-clock timestamp precede the inner subscription")
print("source_check: the pull pipeline itself is deferred")
class ColdMono:
def __init__(self, events):
self.events = events
def subscribe(self):
self.events.append("work subscribed")
def assemble():
events = ["start logged", "timestamp captured"]
return events, ColdMono(events)
events, mono = assemble()
assert events == ["start logged", "timestamp captured"]
assert "work subscribed" not in events
events.append("time passes before subscription")
mono.subscribe()
assert events[-2:] == ["time passes before subscription", "work subscribed"]
print("behavior_check: assembly side effects precede delayed subscription")
events, _ = assemble()
assert events == ["start logged", "timestamp captured"]
print("behavior_check: assembly side effects occur without subscription")
PYRepository: appsmithorg/appsmith
Length of output: 425
Start the pull timer on subscription.
log.info and pullStartTime execute before lockHandledpullDTOMono is subscribed. A delayed subscription inflates durationMs, and an unsubscribed protected-overload call still emits the start log. Wrap both side effects in Mono.defer and use System.nanoTime() for elapsed time.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java`
around lines 1985 - 1988, Update the pull flow around lockHandledpullDTOMono so
the start log and timer initialization occur inside Mono.defer, executing only
when the Mono is subscribed. Use System.nanoTime() for pullStartTime and
calculate elapsed duration from that monotonic timestamp, while preserving the
existing pull behavior.
1 participant
Summary
Critical code paths (authentication, datasource connections, Git operations, import/export, startup) either had no logging at all or logged failures at DEBUG level — invisible in production where INFO is the floor. An RCA agent working from customer-shared Grafana logs had zero signal for the most common failure scenarios.
Slack thread that prompted this: https://theappsmith.slack.com/archives/C09NG5BJ18S/p1785766042770509
Builds on the foundation of #42053 (which fixed Git error messages and stack traces) by adding the timing and entry/exit breadcrumbs that are still missing.
Changes
AuthenticationFailureHandlerCEAccessDeniedHandlerCEInstanceConfigRedisConfigFSGitHandlerCEImpl.fetchRemotelog.error(e.getMessage())— now logs full stack trace with repo contextFSGitHandlerCEImpl.mergeBranchCentralGitServiceCEImpl.pullArtifactDatasourceContextServiceCEImplExportServiceCEImplImportServiceCEImplStopwatchstopAndLogTimeAtInfoLevel()for callers that need production-visible durationDesign decisions
Test plan
mvn compile -q -DskipTests— zero errors)Automation
/ok-to-test tags="@tag.Sanity"
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/30845659889
Commit: d42a276
Cypress dashboard.
Tags:
@tag.SanitySpec:
Mon, 03 Aug 2026 19:57:22 UTC
Summary by CodeRabbit
Improvements