fix: Incomplete authentication on AI Agent endpoint (GHSA-qwc3-h9mg-4582)#3224
Merged
mtrezza merged 7 commits intoparse-community:alphafrom Feb 19, 2026
Merged
fix: Incomplete authentication on AI Agent endpoint (GHSA-qwc3-h9mg-4582)#3224mtrezza merged 7 commits intoparse-community:alphafrom
mtrezza merged 7 commits intoparse-community:alphafrom
Conversation
|
🚀 Thanks for opening this pull request! |
|
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:
📝 WalkthroughWalkthroughExtracts agent POST handling into a new Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Router as Router (auth, csrf)
participant Agent as agentHandler
participant Cache as ConfigKeyCache
participant Authz as Access Control
participant OpenAI as OpenAI Service
Client->>Router: POST /apps/:appId/agent (message, modelName, appId)
Router->>Agent: pass if auth & CSRF OK
Agent->>Authz: verify user access to appId & permissions
alt access denied
Authz-->>Client: 401/403
else access granted
Agent->>Cache: resolve masterKey / readOnlyMasterKey (cache key varies)
Agent->>Agent: build shallow appContext, set effectivePermissions (empty if read-only)
Agent->>OpenAI: call with appContext & effectivePermissions
OpenAI-->>Client: response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Parse-Dashboard/app.js`:
- Around line 250-258: The agent endpoint fails to resolve function-typed master
keys and mutates appConfig: ensure both read-only and non-read-only paths create
a shallow copy (use appContext = { ...appConfig }) and resolve function-typed
keys before passing to Parse by calling ConfigKeyCache.get() for masterKey and
readOnlyMasterKey (respecting masterKeyTtl semantics) so that
appContext.masterKey is a resolved string when used in
Parse.initialize(appContext.appId, undefined, appContext.masterKey).
- Around line 201-207: The POST handler for '/apps/:appId/agent' is missing CSRF
validation; update the route to apply the same CSRF middleware used in /login by
adding Authentication.csrfProtection to the route middleware chain for the
'/apps/:appId/agent' POST handler (so the check runs before the async handler
and after session/auth), ensuring existing authentication logic (req.user and
users check) remains intact.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Parse-Dashboard/app.js`:
- Around line 245-257: The cache key collision occurs because when isReadOnly is
true you assign appConfig.readOnlyMasterKey into appContext.masterKey but still
call ConfigKeyCache.get(..., 'masterKey', ...), causing read-only and regular
master keys to share the same cache slot; update the call to ConfigKeyCache.get
to use a distinct key name (e.g., 'readOnlyMasterKey') when isReadOnly is true
(while keeping 'masterKey' for normal requests), and apply the same change to
the analogous code in the config endpoint that also calls ConfigKeyCache.get for
master keys.
- Around line 328-340: The dashboard SPA cannot get a CSRF token because the
splat route that serves the dashboard bundle doesn’t run
Authentication.csrfProtection, so frontend POSTs to the agent endpoint
(app.post('/apps/:appId/agent') handled by agentHandler) always 403 under
csrfSynchronisedProtection; fix by either running Authentication.csrfProtection
on the dashboard splat route so the rendered bundle can embed req.csrfToken(),
or add a small GET endpoint (e.g. /csrf-token) that invokes
Authentication.csrfProtection and returns req.csrfToken() to the SPA before it
posts to /apps/:appId/agent, ensuring the SPA can send the token in
req.body._csrf or x-csrf-token as expected.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Parse-Dashboard/app.js (4)
273-275: Fragile placeholder API key detection.
apiKey.includes('xxx')will match legitimate API keys that happen to contain the substringxxx. A more targeted check would test for the exact placeholder value or check that the key matches a known format (e.g., OpenAI keys start withsk-).🔧 Suggested improvement
- if (apiKey === 'xxxxx' || apiKey.includes('xxx')) { + if (apiKey === 'xxxxx' || apiKey === 'your-api-key-here' || apiKey.startsWith('sk-xxx')) { return res.status(400).json({ error: 'Please replace the placeholder API key with your actual API key' }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Parse-Dashboard/app.js` around lines 273 - 275, The current placeholder detection uses apiKey.includes('xxx') which can falsely match valid keys; update the check in the if block that references apiKey so it only rejects exact placeholder values (e.g., 'xxxxx') or keys that don't match the expected provider format—for example require the key to start with the expected prefix like 'sk-' for OpenAI—so change the condition to validate either exact placeholder equality OR a format/prefix test on apiKey and return the 400 only when the key is exactly the placeholder or fails the format check.
312-312:substris deprecated; prefersubstringorslice.
String.prototype.substris a legacy method. The codebase also usescrypto.randomUUID()elsewhere — consider using that for consistency.♻️ Suggested fix
- const finalConversationId = conversationId || `conv_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`; + const finalConversationId = conversationId || `conv_${Date.now()}_${crypto.randomUUID().slice(0, 9)}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Parse-Dashboard/app.js` at line 312, Replace the legacy substr usage when building finalConversationId: instead of using Math.random().toString(36).substr(...), generate a stable unique ID with crypto.randomUUID() and concatenate it with Date.now(); update the finalConversationId expression (the variable named finalConversationId that falls back from conversationId) to use `crypto.randomUUID()` (or use String.prototype.slice/substring if you must) and ensure the environment provides crypto (import from 'crypto' if not global).
227-230: App lookup uses URL-friendly name, butappIdparam name implies a ParseappId.The route parameter is
:appId, but line 227 matches againstappNameForURL || appName, not the actualappIdfield. This naming mismatch could confuse maintainers and API consumers. If the parameter represents a URL-friendly app name, consider renaming it to:appSlugor:appNamein the route definition, or document the lookup behavior inline.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Parse-Dashboard/app.js` around lines 227 - 230, The lookup uses config.apps.find(a => (a.appNameForURL || a.appName) === appId) but the route parameter is named appId, which is misleading; either change the route parameter to a URL-friendly name (e.g. :appSlug or :appName) and update any references to request param accordingly, or change the lookup to compare against the actual Parse appId field (e.g. a.appId) so the variable name matches behavior; update the route definition or the find predicate and adjust any tests/docs to reflect the chosen name.
325-329: Error response leaks full internal error messages to clients.Line 327 returns
error.messageverbatim to the HTTP response. For unexpected errors (database connection strings, internal stack details, etc.), this can expose sensitive server internals. Consider logging the full error server-side and returning a generic message for 500-level errors, while keeping specific messages for known/expected error types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Parse-Dashboard/app.js` around lines 325 - 329, The catch block currently returns error.message directly to clients (res.status(500).json({ error: `Error: ${errorMessage}` })), which can leak sensitive internals; change it to log the full error server-side (e.g., logger.error or console.error(error)) and return a generic 500 response like "Internal server error" while preserving specific messages only for known/expected error types (inspect error.constructor/name or use instanceof checks and map those to safe client messages). Ensure you update the catch block where res.status and error.message are used so logging records the full error but the HTTP response contains a non-sensitive, generic message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Parse-Dashboard/app.js`:
- Around line 202-206: The code mutates the global Parse singleton (via
Parse.initialize, Parse.serverURL, Parse.masterKey) inside
executeDatabaseFunction which races across concurrent requests from
makeOpenAIRequest; change executeDatabaseFunction to avoid touching the global
Parse singleton by creating and using an isolated Parse client per request (or
switch to direct REST API calls) and thread that client through
makeOpenAIRequest and any tool functions instead of calling Parse.initialize
globally; update all callers to accept the per-request Parse client (or a
short-lived REST helper) so each agent request uses its own Parse configuration
and no global state is mutated.
---
Duplicate comments:
In `@Parse-Dashboard/app.js`:
- Around line 332-342: The SPA lacks a CSRF token source so POSTs to
app.post('/apps/:appId/agent') (which uses Authentication.csrfProtection) get
403; add a small authenticated GET endpoint (e.g., app.get('/csrf-token',
authMiddleware, Authentication.csrfProtection, tokenHandler)) that calls
req.csrfToken() and returns it as JSON, or alternatively apply
Authentication.csrfProtection to the splat/dashboard route and render
req.csrfToken() into the served page; reference Authentication.csrfProtection,
app.post('/apps/:appId/agent') and the splat/dashboard route when making the
change.
---
Nitpick comments:
In `@Parse-Dashboard/app.js`:
- Around line 273-275: The current placeholder detection uses
apiKey.includes('xxx') which can falsely match valid keys; update the check in
the if block that references apiKey so it only rejects exact placeholder values
(e.g., 'xxxxx') or keys that don't match the expected provider format—for
example require the key to start with the expected prefix like 'sk-' for
OpenAI—so change the condition to validate either exact placeholder equality OR
a format/prefix test on apiKey and return the 400 only when the key is exactly
the placeholder or fails the format check.
- Line 312: Replace the legacy substr usage when building finalConversationId:
instead of using Math.random().toString(36).substr(...), generate a stable
unique ID with crypto.randomUUID() and concatenate it with Date.now(); update
the finalConversationId expression (the variable named finalConversationId that
falls back from conversationId) to use `crypto.randomUUID()` (or use
String.prototype.slice/substring if you must) and ensure the environment
provides crypto (import from 'crypto' if not global).
- Around line 227-230: The lookup uses config.apps.find(a => (a.appNameForURL ||
a.appName) === appId) but the route parameter is named appId, which is
misleading; either change the route parameter to a URL-friendly name (e.g.
:appSlug or :appName) and update any references to request param accordingly, or
change the lookup to compare against the actual Parse appId field (e.g. a.appId)
so the variable name matches behavior; update the route definition or the find
predicate and adjust any tests/docs to reflect the chosen name.
- Around line 325-329: The catch block currently returns error.message directly
to clients (res.status(500).json({ error: `Error: ${errorMessage}` })), which
can leak sensitive internals; change it to log the full error server-side (e.g.,
logger.error or console.error(error)) and return a generic 500 response like
"Internal server error" while preserving specific messages only for
known/expected error types (inspect error.constructor/name or use instanceof
checks and map those to safe client messages). Ensure you update the catch block
where res.status and error.message are used so logging records the full error
but the HTTP response contains a non-sensitive, generic message.
mtrezza
changed the title
mtrezza
changed the title
parseplatformorg
pushed a commit
that referenced
this pull request
Feb 19, 2026
# [9.0.0-alpha.8](9.0.0-alpha.7...9.0.0-alpha.8) (2026-02-19) ### Bug Fixes * Incomplete authentication on AI Agent endpoint ([GHSA-qwc3-h9mg-4582](GHSA-qwc3-h9mg-4582)) ([#3224](#3224)) ([f92a9ef](f92a9ef))
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/lib/tests/AgentAuth.test.js (3)
364-374: Missing access-denied test forappreadonlyon an unassigned app.The suite tests that
appadminis deniedSecretApp(line 310), but there is no symmetric test confirming thatappreadonlyis also deniedSecretApp. Sinceappreadonlyonly hastestAppIdin itsappslist, a request to/apps/SecretApp/agentshould return403. Without this test, a regression in per-app access enforcement for read-only users could go undetected.💡 Suggested additional test
it('returns 403 when per-app read-only user tries to access an unassigned app', async () => { const res = await makeRequest(port, { method: 'POST', path: '/apps/SecretApp/agent', body: agentBody(), cookie: appreadonlyCookie, headers: { 'X-CSRF-Token': CSRF_TOKEN }, }); expect(res.status).toBe(403); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tests/AgentAuth.test.js` around lines 364 - 374, Add a test that verifies a per-app read-only user (appreadonlyCookie) is denied access to an unassigned app by calling makeRequest with method 'POST', path '/apps/SecretApp/agent', body from agentBody(), and header 'X-CSRF-Token' set to CSRF_TOKEN, then assert res.status === 403; place it alongside the existing per-app access tests so it mirrors the existing appadmin denial check but uses appreadonlyCookie to confirm read-only users can't access apps not in their apps list.
29-57:bodyStrcomputed but unused inreq.write.
JSON.stringify(body)is called twice — once for theContent-Lengthheader (line 30) and again insidereq.write(line 56). Use the already-computedbodyStrfor the write to avoid duplicate serialization.♻️ Proposed refactor
if (body) { const bodyStr = JSON.stringify(body); options.headers['Content-Type'] = 'application/json'; options.headers['Content-Length'] = Buffer.byteLength(bodyStr); } // ... if (body) { - req.write(JSON.stringify(body)); + req.write(bodyStr); }Since
bodyStris declared inside theif (body)block on line 29, hoist it to the outer scope (or restructure the two blocks) so the variable is accessible in the secondif:+let bodyStr; if (body) { - const bodyStr = JSON.stringify(body); + bodyStr = JSON.stringify(body); options.headers['Content-Type'] = 'application/json'; options.headers['Content-Length'] = Buffer.byteLength(bodyStr); } // ... if (body) { - req.write(JSON.stringify(body)); + req.write(bodyStr); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tests/AgentAuth.test.js` around lines 29 - 57, The test helper currently serializes body twice (JSON.stringify called for Content-Length and again in req.write); hoist or declare a variable (bodyStr) in the outer scope, set bodyStr = JSON.stringify(body) inside the existing if (body) block, use Buffer.byteLength(bodyStr) to set options.headers['Content-Length'], and replace the second JSON.stringify(body) used in req.write with req.write(bodyStr) so the same serialized string is reused; update references to options.headers and req.write accordingly.
270-281: Prefer a positive assertion over dual negative assertions.The comment on line 278 explicitly states
500 expectedsince authentication passes but the OpenAI call fails with the fake API key. AssertingtoBe(500)is more precise and would catch regressions where the handler returns, say,400or200due to a future logic change — both of which would silently pass the current dual-negative guard. The same pattern applies to lines 306–307, 334–335, 356–357, and 372–373.♻️ Proposed refactor (example for lines 279-280; apply similarly to other "allowed" tests)
- expect(res.status).not.toBe(401); - expect(res.status).not.toBe(403); + // Auth passes; OpenAI call fails with fake key → 500 + expect(res.status).toBe(500);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tests/AgentAuth.test.js` around lines 270 - 281, Replace the dual negative assertions that check authentication succeeded (expect(res.status).not.toBe(401); expect(res.status).not.toBe(403);) with a single positive assertion expecting the known failure status (expect(res.status).toBe(500)) in the test "allows authenticated full admin to reach the agent handler" (the test that calls makeRequest with agentBody(), adminCookie and CSRF_TOKEN); do the same replacement for the other "allowed" tests that currently assert .not.toBe(401)/.not.toBe(403) so each explicitly asserts the expected 500 status returned from the OpenAI call failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/tests/AgentAuth.test.js`:
- Around line 338-358: The test name is misleading: it claims server-side
behavior (masterKey -> readOnlyMasterKey swap and clearing write permissions)
that the HTTP assertions in the test do not verify; rename the test to something
accurate (e.g., "allows read-only user to submit write permissions without being
rejected at the auth layer") and/or add a unit test that exercises agentHandler
with a spy/mock of the downstream caller (e.g., the OpenAI/Parse API caller used
by agentHandler) to capture forwarded keys and permissions; specifically, in
tests that use makeRequest, agentBody, readonlyCookie and CSRF_TOKEN either
rename the existing it(...) string to match what is actually asserted or add a
new unit test that stubs the OpenAI caller or Parse client and asserts that
agentHandler swapped masterKey to readOnlyMasterKey and replaced permissions
with {} before forwarding.
- Around line 72-75: MockSessionStore.get currently does JSON.parse(sess)
directly which can throw synchronously; wrap the parse in a try/catch inside
get(sid, callback) so any SyntaxError from JSON.parse is caught and forwarded to
callback(err) (e.g., callback(err) or callback(err, null)) instead of letting it
escape, and return callback(null, parsed) on success; reference the
MockSessionStore.get implementation and this.sessions[sid] when applying the
change.
- Line 14: The test imports cookieSignature from an internal transitive path
which is fragile; add "cookie-signature" to devDependencies in package.json and
change the import in AgentAuth.test.js from
require('express-session/node_modules/cookie-signature') to
require('cookie-signature') (update any variable name cookieSignature
accordingly) so the module is resolved from the top-level node_modules.
---
Nitpick comments:
In `@src/lib/tests/AgentAuth.test.js`:
- Around line 364-374: Add a test that verifies a per-app read-only user
(appreadonlyCookie) is denied access to an unassigned app by calling makeRequest
with method 'POST', path '/apps/SecretApp/agent', body from agentBody(), and
header 'X-CSRF-Token' set to CSRF_TOKEN, then assert res.status === 403; place
it alongside the existing per-app access tests so it mirrors the existing
appadmin denial check but uses appreadonlyCookie to confirm read-only users
can't access apps not in their apps list.
- Around line 29-57: The test helper currently serializes body twice
(JSON.stringify called for Content-Length and again in req.write); hoist or
declare a variable (bodyStr) in the outer scope, set bodyStr =
JSON.stringify(body) inside the existing if (body) block, use
Buffer.byteLength(bodyStr) to set options.headers['Content-Length'], and replace
the second JSON.stringify(body) used in req.write with req.write(bodyStr) so the
same serialized string is reused; update references to options.headers and
req.write accordingly.
- Around line 270-281: Replace the dual negative assertions that check
authentication succeeded (expect(res.status).not.toBe(401);
expect(res.status).not.toBe(403);) with a single positive assertion expecting
the known failure status (expect(res.status).toBe(500)) in the test "allows
authenticated full admin to reach the agent handler" (the test that calls
makeRequest with agentBody(), adminCookie and CSRF_TOKEN); do the same
replacement for the other "allowed" tests that currently assert
.not.toBe(401)/.not.toBe(403) so each explicitly asserts the expected 500 status
returned from the OpenAI call failure.
| const express = require('express'); | ||
| const http = require('http'); | ||
| const session = require('express-session'); | ||
| const cookieSignature = require('express-session/node_modules/cookie-signature'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether cookie-signature is a declared (dev)dependency in package.json
# and whether it exists at the top-level node_modules.
echo "=== package.json dependency entries ==="
rg '"cookie-signature"' package.json
echo ""
echo "=== Top-level node_modules ==="
fd -t d "^cookie-signature$" node_modules --max-depth 1
echo ""
echo "=== Nested under express-session ==="
fd -t d "^cookie-signature$" node_modules/express-session/node_modules --max-depth 1
echo ""
echo "=== Full node_modules tree for cookie-signature ==="
fd -t d "^cookie-signature$" node_modulesRepository: parse-community/parse-dashboard
Length of output: 583
🏁 Script executed:
#!/bin/bash
# Read package.json to check for cookie-signature declaration
echo "=== package.json (full content for dependency check) ==="
cat package.json | jq '.dependencies, .devDependencies' 2>/dev/null || cat package.json | grep -A 50 '"dependencies"' | head -100
echo ""
echo "=== Line 14 of AgentAuth.test.js ==="
sed -n '1,20p' src/lib/tests/AgentAuth.test.jsRepository: parse-community/parse-dashboard
Length of output: 3531
🏁 Script executed:
#!/bin/bash
echo "=== Usages of cookieSignature in AgentAuth.test.js ==="
rg 'cookieSignature' src/lib/tests/AgentAuth.test.js -nRepository: parse-community/parse-dashboard
Length of output: 282
Fix fragile import from transitive dependency's internal node_modules.
require('express-session/node_modules/cookie-signature') imports from an undocumented, internal dependency path. With standard npm hoisting (npm's default behavior), cookie-signature resolves to node_modules/cookie-signature, not nested under express-session. This will cause a runtime MODULE_NOT_FOUND error when the test file loads.
Add cookie-signature as a devDependency in package.json and update the import:
Diff
-const cookieSignature = require('express-session/node_modules/cookie-signature');
+const cookieSignature = require('cookie-signature');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const cookieSignature = require('express-session/node_modules/cookie-signature'); | |
| const cookieSignature = require('cookie-signature'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/tests/AgentAuth.test.js` at line 14, The test imports cookieSignature
from an internal transitive path which is fragile; add "cookie-signature" to
devDependencies in package.json and change the import in AgentAuth.test.js from
require('express-session/node_modules/cookie-signature') to
require('cookie-signature') (update any variable name cookieSignature
accordingly) so the module is resolved from the top-level node_modules.
Comment on lines
+72
to
+75
| get(sid, callback) { | ||
| const sess = this.sessions[sid]; | ||
| callback(null, sess ? JSON.parse(sess) : null); | ||
| } |
There was a problem hiding this comment.
JSON.parse in MockSessionStore.get can throw synchronously, violating callback convention.
If sess is somehow a non-JSON value, JSON.parse(sess) throws a SyntaxError that escapes the function uncaught rather than being forwarded as callback(err). The Node.js store contract expects errors to be surfaced via the callback.
🛡️ Proposed fix
get(sid, callback) {
const sess = this.sessions[sid];
- callback(null, sess ? JSON.parse(sess) : null);
+ if (!sess) return callback(null, null);
+ try {
+ callback(null, JSON.parse(sess));
+ } catch (err) {
+ callback(err);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| get(sid, callback) { | |
| const sess = this.sessions[sid]; | |
| callback(null, sess ? JSON.parse(sess) : null); | |
| } | |
| get(sid, callback) { | |
| const sess = this.sessions[sid]; | |
| if (!sess) return callback(null, null); | |
| try { | |
| callback(null, JSON.parse(sess)); | |
| } catch (err) { | |
| callback(err); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/tests/AgentAuth.test.js` around lines 72 - 75, MockSessionStore.get
currently does JSON.parse(sess) directly which can throw synchronously; wrap the
parse in a try/catch inside get(sid, callback) so any SyntaxError from
JSON.parse is caught and forwarded to callback(err) (e.g., callback(err) or
callback(err, null)) instead of letting it escape, and return callback(null,
parsed) on success; reference the MockSessionStore.get implementation and
this.sessions[sid] when applying the change.
Comment on lines
+338
to
+358
| it('uses readOnlyMasterKey for global read-only user even when write permissions are sent', async () => { | ||
| const res = await makeRequest(port, { | ||
| method: 'POST', | ||
| path: '/apps/TestApp/agent', | ||
| body: agentBody({ | ||
| permissions: { | ||
| deleteClass: true, | ||
| deleteObject: true, | ||
| createObject: true, | ||
| updateObject: true, | ||
| createClass: true, | ||
| }, | ||
| }), | ||
| cookie: readonlyCookie, | ||
| headers: { 'X-CSRF-Token': CSRF_TOKEN }, | ||
| }); | ||
| // Passes auth, request is processed (500 from fake API key, not 401/403). | ||
| // Server-side: masterKey swapped to readOnlyMasterKey + permissions overridden to {}. | ||
| expect(res.status).not.toBe(401); | ||
| expect(res.status).not.toBe(403); | ||
| }); |
There was a problem hiding this comment.
Test name asserts server-side behavior (readOnlyMasterKey swap) that the assertions cannot verify.
The test asserts only that the response is not 401 or 403, which only proves that authentication/authorization passed. The claimed invariant — that the server swaps masterKey → readOnlyMasterKey and clears write permissions — is invisible at the HTTP boundary. If that server-side logic were silently removed, this test would still pass.
Options:
- Rename the test to accurately describe what is actually being asserted (
allows read-only user to submit write permissions without being rejected at the auth layer). - Add deeper coverage (e.g., a spy/mock on the OpenAI caller or the Parse API caller to capture which key is forwarded) in a unit test for
agentHandleralongside this integration test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/tests/AgentAuth.test.js` around lines 338 - 358, The test name is
misleading: it claims server-side behavior (masterKey -> readOnlyMasterKey swap
and clearing write permissions) that the HTTP assertions in the test do not
verify; rename the test to something accurate (e.g., "allows read-only user to
submit write permissions without being rejected at the auth layer") and/or add a
unit test that exercises agentHandler with a spy/mock of the downstream caller
(e.g., the OpenAI/Parse API caller used by agentHandler) to capture forwarded
keys and permissions; specifically, in tests that use makeRequest, agentBody,
readonlyCookie and CSRF_TOKEN either rename the existing it(...) string to match
what is actually asserted or add a new unit test that stubs the OpenAI caller or
Parse client and asserts that agentHandler swapped masterKey to
readOnlyMasterKey and replaced permissions with {} before forwarding.
Contributor
|
🎉 This change has been released in version 9.0.0-alpha.8 |
parseplatformorg
pushed a commit
that referenced
this pull request
Feb 19, 2026
# [9.0.0](8.5.0...9.0.0) (2026-02-19) ### Bug Fixes * Cloud Config parameter modal overlays non-printable character info box ([#3221](#3221)) ([983253e](983253e)) * Incomplete authentication on AI Agent endpoint ([GHSA-qwc3-h9mg-4582](GHSA-qwc3-h9mg-4582)) ([#3224](#3224)) ([f92a9ef](f92a9ef)) * Non-printable character box missing when editing Cloud Config parameter ([#3218](#3218)) ([719ac09](719ac09)) ### Features * Add option to block saving Cloud Config parameter if validation fails ([#3225](#3225)) ([41691aa](41691aa)) * Add Regex string validation when editing Cloud Config parameter ([#3222](#3222)) ([067b9d1](067b9d1)) * Add support for checkbox, toggle, text input elements to script form ([#3219](#3219)) ([b9366bc](b9366bc)) * Add support for reversing info panel auto-scroll direction while holding Command+Option keys ([#3220](#3220)) ([7ebd121](7ebd121)) * Remove Node 18 support ([#3212](#3212)) ([a5c1bb2](a5c1bb2)) ### BREAKING CHANGES * Removes support for Node 18. ([a5c1bb2](a5c1bb2))
Contributor
|
🎉 This change has been released in version 9.0.0 |
2 tasks
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.
Pull Request
Issue
Fixes security vulnerability GHSA-qwc3-h9mg-4582.
Tasks
Summary by CodeRabbit
New Features
Bug Fixes
Tests