fix: the post /reset endpoint in the real-time servi... in git_routes.ts#41898
Open
orbisai0security wants to merge 4 commits into
Open
fix: the post /reset endpoint in the real-time servi... in git_routes.ts#41898orbisai0security wants to merge 4 commits into
orbisai0security wants to merge 4 commits into
Conversation
Automated security fix generated by OrbisAI Security
The POST /reset endpoint in the Real-Time Service (RTS) Git routes uses only a generic request validator without explicit authentication or authorization middleware
Contributor
WalkthroughThe ChangesGit Reset Endpoint Security
Sequence DiagramsequenceDiagram
participant Client
participant GitRoutes as gitRoutes
participant AuthMiddleware as requireInternalAuth
participant ValidateRequest as validator.validateRequest
participant GitController as gitController.reset
Client->>GitRoutes: POST /reset {repoPath}
GitRoutes->>GitRoutes: body("repoPath") validation
GitRoutes->>AuthMiddleware: requireInternalAuth
alt x-rts-secret missing or mismatched
AuthMiddleware->>Client: 401 JSON
else secret matches
AuthMiddleware->>ValidateRequest: next()
ValidateRequest->>GitController: next()
GitController->>Client: reset response
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/client/packages/rts/src/routes/git_routes.ts`:
- Around line 10-15: The POST /reset route currently only validates the body
then calls gitController.reset; add the RTS authentication/authorization
middleware into the router.post call so it runs before validator.validateRequest
and before gitController.reset (i.e., change the middleware chain for
router.post("/reset", body(...), <auth middleware>, validator.validateRequest,
gitController.reset) so unauthorized callers cannot invoke gitController.reset).
In `@tests/invariant_git_routes.test.ts`:
- Around line 24-31: The test block calling request(app).post('/git/reset') is
sending an invalid body key (repositoryId) so validation can fail before auth;
change the payload to send the required repoPath (e.g., .send({ repoPath:
'test-repo' })) and replace the loose expect([401,
403]).toContain(response.status) with a strict assertion against the
table-driven value (e.g., expect(response.status).toBe(expectedStatus)) so each
case verifies the intended auth rejection; update the variables used in this
test (headers, expectedStatus) accordingly.
🪄 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: 3f415e70-a2fc-4ad5-ad93-f9740089daab
📒 Files selected for processing (2)
app/client/packages/rts/src/routes/git_routes.tstests/invariant_git_routes.test.ts
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
- Add requireInternalAuth middleware (APPSMITH_RTS_SECRET header check) so unauthorized callers cannot reach gitController.reset - Fix test to send correct body field (repoPath), set env var so auth is enforced during test, and assert strict status code per table row Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/client/packages/rts/src/middlewares/AuthMiddleware.ts`:
- Around line 5-7: The AuthMiddleware.ts file currently silently bypasses
authentication when the secret is undefined by calling next(), which leaves the
endpoint unprotected if APPSMITH_RTS_SECRET is misconfigured. Replace the silent
bypass in the check for !secret with a fail-closed approach that returns a 401
or 503 status code, or at minimum log a warning to alert operators that
authentication is disabled. This ensures the security fix is not inadvertently
circumvented due to missing environment configuration.
In `@tests/invariant_git_routes.test.ts`:
- Around line 8-17: The beforeAll and afterAll hooks mutate the global
process.env without preserving pre-existing values, causing test pollution. In
the beforeAll block, before setting process.env.APPSMITH_RTS_SECRET to
'test-secret', store the original value of process.env.APPSMITH_RTS_SECRET in a
variable. In the afterAll block, instead of deleting the environment variable,
restore it to the original value that was saved (this ensures that if the
variable existed before the test, it is properly restored to its original state,
or if it didn't exist, it remains undefined).
🪄 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: 35b25677-aa8e-47e5-9a30-883fdb1927b7
📒 Files selected for processing (3)
app/client/packages/rts/src/middlewares/AuthMiddleware.tsapp/client/packages/rts/src/routes/git_routes.tstests/invariant_git_routes.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/rts/src/routes/git_routes.ts
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
…1898 - Make requireInternalAuth fail-closed when APPSMITH_RTS_SECRET is unset (previously silently bypassed auth; now returns 401 + console.warn) - Preserve and restore original env var value in test beforeAll/afterAll to avoid test pollution when the variable is already set in the environment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/client/packages/rts/src/middlewares/AuthMiddleware.ts (1)
6-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFail-closed behavior looks correct; swap
console.warnfor the structured logger.The 401 + warning on missing
APPSMITH_RTS_SECRETcorrectly closes the silent-bypass gap from the prior review. One nit:console.warnfor a security-relevant misconfiguration won't reach Sentry/OpenTelemetry, so operators may never see it. Route it through the RTS logger so it surfaces in observability.🔭 Suggested change to use structured logging
+import log from "loglevel"; + export function requireInternalAuth(req: Request, res: Response, next: NextFunction) { const secret = process.env.APPSMITH_RTS_SECRET; if (!secret) { - console.warn("[AuthMiddleware] APPSMITH_RTS_SECRET is not set; rejecting request to protected endpoint"); + log.warn("[AuthMiddleware] APPSMITH_RTS_SECRET is not set; rejecting request to protected endpoint"); return res.status(401).json({ success: false, message: "Unauthorized" }); }As per coding guidelines: "RTS must implement OpenTelemetry and Sentry instrumentation for observability".
🤖 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/client/packages/rts/src/middlewares/AuthMiddleware.ts` around lines 6 - 7, Replace the direct console.warn in AuthMiddleware with the RTS structured logger so the missing APPSMITH_RTS_SECRET warning is captured by observability tooling. Locate the protected-endpoint check in AuthMiddleware and route that security misconfiguration message through the existing logger used elsewhere in the RTS package, preserving the same 401 fail-closed response while ensuring it can surface in Sentry/OpenTelemetry.Source: Coding guidelines
🤖 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 `@tests/invariant_git_routes.test.ts`:
- Around line 9-15: The current invariant_git_routes.test.ts setup in beforeAll
always sets APPSMITH_RTS_SECRET, so it never covers the new fail-closed
behavior. Add a regression test around gitRoutes that temporarily removes
APPSMITH_RTS_SECRET before issuing the request and asserts the route returns
401, then restore the original env value so other cases still run with the test
secret. Use the existing app setup and route names to locate the test logic.
---
Nitpick comments:
In `@app/client/packages/rts/src/middlewares/AuthMiddleware.ts`:
- Around line 6-7: Replace the direct console.warn in AuthMiddleware with the
RTS structured logger so the missing APPSMITH_RTS_SECRET warning is captured by
observability tooling. Locate the protected-endpoint check in AuthMiddleware and
route that security misconfiguration message through the existing logger used
elsewhere in the RTS package, preserving the same 401 fail-closed response while
ensuring it can surface in Sentry/OpenTelemetry.
🪄 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: 139864d6-fcbf-4d5d-8c3e-3ef452e46c7c
📒 Files selected for processing (2)
app/client/packages/rts/src/middlewares/AuthMiddleware.tstests/invariant_git_routes.test.ts
Comment on lines
+9
to
+15
| beforeAll(() => { | ||
| originalSecret = process.env.APPSMITH_RTS_SECRET; | ||
| process.env.APPSMITH_RTS_SECRET = 'test-secret'; | ||
| app = express(); | ||
| app.use(express.json()); | ||
| app.use('/git', gitRoutes); | ||
| }); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Add a regression case for an unset APPSMITH_RTS_SECRET.
Line 11 forces the secret for every case, so this suite never exercises the new fail-closed branch that should return 401 when the env var is missing. Please add one case that removes the env var before the request and asserts the rejection path too.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 11-11: Express application should use Helmet
Context: express()
Note: [CWE-693] Protection Mechanism Failure (Express app without Helmet security headers).
(missing-helmet-typescript)
🤖 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 `@tests/invariant_git_routes.test.ts` around lines 9 - 15, The current
invariant_git_routes.test.ts setup in beforeAll always sets APPSMITH_RTS_SECRET,
so it never covers the new fail-closed behavior. Add a regression test around
gitRoutes that temporarily removes APPSMITH_RTS_SECRET before issuing the
request and asserts the route returns 401, then restore the original env value
so other cases still run with the test secret. Use the existing app setup and
route names to locate the test logic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix high severity security issue in
app/client/packages/rts/src/routes/git_routes.ts.Vulnerability
V-002app/client/packages/rts/src/routes/git_routes.ts:9Description: The POST /reset endpoint in the Real-Time Service (RTS) Git routes uses only a generic request validator without explicit authentication or authorization middleware. This state-changing endpoint could allow unauthorized users to reset Git repositories associated with Appsmith applications, potentially destroying version history or reverting applications to previous states.
Evidence
Exploitation scenario: An attacker with network access to the RTS service sends a crafted POST request to /reset with a valid request structure.
Scanner confirmation: multi_agent_ai rule
V-002flagged this pattern.Production code: This file is in the production codebase, not test-only code.
Threat Model Context
This route handler appears to be publicly accessible. This is a containerized service - vulnerabilities may be exploitable depending on network exposure.
Changes
app/client/packages/rts/src/routes/git_routes.tsVerification
Security Invariant
Regression test
This test guards against regressions — it's useful independent of the code change above.
Automated security fix by OrbisAI Security
Summary by CodeRabbit
POST /git/resetso it now requires internal authentication via thex-rts-secretheader.repoPath, ensuring it is present and a non-empty string with improved error messaging.POST /git/resetreturns401 Unauthorizedfor unauthenticated and invalidx-rts-secretscenarios (missing, malformed, empty, or invalid).