Repository navigation
fix: security hardening — remove debug collector, add path traversal protection, add security headers - #6
Conversation
…protection, add security headers - Remove client/public/__manus__/debug-collector.js from production build (captures console logs, network requests, and user interactions) - Add .gitignore entry to prevent re-addition - Add path traversal validation in storage proxy (vite.config.ts) - Add request body size limit (2MB) to debug log endpoint - Add security headers middleware to Express production server: X-Content-Type-Options, X-Frame-Options, X-XSS-Protection, Referrer-Policy, Permissions-Policy Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
📝 WalkthroughWalkthroughThe PR removes an 821-line browser-side Manus debug collector script, updates ChangesSecurity Hardening
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](/web-api/iframe-proxy?url=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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@vite.config.ts`:
- Around line 176-181: The decodeURIComponent(key) call in the middleware can
throw a URIError when the key contains malformed percent-encoding, bypassing the
path traversal validation and causing middleware failure. Wrap the
decodeURIComponent call in a try-catch block to catch URIError exceptions, and
respond with a 400 status code and appropriate error message when such errors
occur, similar to how the existing traversal checks handle invalid keys.
- Around line 139-144: The code has two issues to fix. First, in the body size
validation logic where MAX_BODY_BYTES is checked, replace the immediate
req.destroy() call with a flag-based approach: introduce a boolean flag (e.g.,
payloadTooLarge) that is set to true when the size limit is exceeded, then check
this flag in the data event handler to prevent further processing while allowing
the 413 response to be sent cleanly through writeHead and end without destroying
the socket. Second, wrap the decodeURIComponent(key) call in a try-catch block
to handle URIError exceptions that can be thrown when users provide malformed
percent-encoded strings like incomplete percent sequences, and handle the caught
error appropriately by either skipping that key-value pair or logging a warning
depending on your error handling strategy.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c2a42b7c-47ad-46d2-a08f-f99cb929e504
📒 Files selected for processing (4)
.gitignoreclient/public/__manus__/debug-collector.jsserver/index.tsvite.config.ts
💤 Files with no reviewable changes (1)
- client/public/manus/debug-collector.js
| bodyBytes += chunk.length; | ||
| if (bodyBytes > MAX_BODY_BYTES) { | ||
| req.destroy(); | ||
| res.writeHead(413, { "Content-Type": "application/json" }); | ||
| res.end(JSON.stringify({ success: false, error: "Payload too large" })); | ||
| return; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n vite.config.ts | sed -n '130,165p'Repository: Player10thGames/Fireboy-the-Brothers-The-Legend-of-Terra-Nemesis
Length of output: 1416
🏁 Script executed:
grep -n "decodeURIComponent" vite.config.tsRepository: Player10thGames/Fireboy-the-Brothers-The-Legend-of-Terra-Nemesis
Length of output: 164
🏁 Script executed:
grep -n "manus-storage" vite.config.ts -A 20 | head -50Repository: Player10thGames/Fireboy-the-Brothers-The-Legend-of-Terra-Nemesis
Length of output: 1141
🌐 Web query:
Node.js req.destroy() during data event handler socket cleanup behavior
💡 Result:
When req.destroy() is called on a Node.js HTTP request during a data event handler, it initiates the destruction process for the request stream and the underlying socket [1][2]. The behavior is consistent with Node.js Readable stream destruction, where calling destroy() marks the stream as destroyed (req.destroyed becomes true) and triggers cleanup of the associated resources [1][3][2]. Key behavioral points include: 1. Immediate Cleanup and Socket Destruction: If the request has progressed to the point of being actively used (post onSocketNT), calling req.destroy() will drop remaining response data and destroy the underlying socket [4][5][6]. If the socket is not yet actively used (e.g., still in the connection phase or being managed by an Agent), it may be returned to the Agent pool instead of being destroyed [7][2][5]. 2. Data Event Handling: Because Node.js streams are event-driven, events that have already been queued in the event loop—including data events that were emitted just before or at the moment of the destroy() call—may still be processed by existing handlers [8][9]. A data handler might trigger one last time even if the socket is technically destroyed, as the data was already buffered or read from the socket before the destruction signal was fully propagated [8][9][10]. 3. Event Ordering: Calling req.destroy() typically causes the request to emit a close event [1][2]. If an error is passed to destroy(err), an error event is emitted before the close event [1][2]. 4. Preventing Further Logic: If your logic in the data handler depends on the request still being active, you should check the req.destroyed property explicitly, as the stream will not immediately stop the execution of callbacks already placed in the event loop [10][3]. In summary, req.destroy() is the authoritative way to terminate an HTTP request in Node.js [2]. While it immediately flags the stream as destroyed and initiates socket cleanup, it does not guarantee that in-flight callbacks or already-queued data events will be cancelled, so you should ensure your application code accounts for this asynchronous reality [8][10].
Citations:
- 1: nodejs/node@31c797cb11
- 2: https://nodejs.org/api/http.html
- 3: It's possible to push data to a Readable in mid-destroy state nodejs/node#35303
- 4: http: fix leaked error listener on sync HTTP req create + destroy nodejs/node#62872
- 5: nodejs/node@e0b79633f6
- 6: nodejs/node@64f15c274a
- 7: https://beta.docs.nodejs.org/http.html
- 8: Socket still fire data after destroyed nodejs/node#38034
- 9: Readable stream destroyed while reading nodejs/node#30794
- 10: stream: How to destroy and stop Readable? nodejs/node#29856
🌐 Web query:
JavaScript decodeURIComponent URIError malformed encoding exception handling
💡 Result:
The JavaScript decodeURIComponent() function throws a URIError (specifically "malformed URI sequence") when it encounters an invalid percent-encoded character sequence [1][2][3]. This typically occurs due to: 1. A lone percent sign (%) not followed by two hexadecimal digits [1][4]. 2. A percent sign followed by characters that are not valid hexadecimal digits (e.g., %G1) [4]. 3. An encoded sequence that does not represent a valid UTF-8 character [1][3]. Exception Handling Strategies: The most reliable way to handle these errors is by using a try...catch block, which prevents the application from crashing when processing external or user-provided input [5][4]. try { const decoded = decodeURIComponent(input); } catch (e) { if (e instanceof URIError) { // Handle the specific URI error console.warn("Malformed URI component detected:", input); } // Fallback: return original input or a default value } Fixing Malformed Sequences: If your data frequently contains raw percent signs that are intended to be literal characters rather than encoding markers, you can sanitize the input before decoding by replacing lone percent signs with their percent-encoded equivalent (%25) [5][4]: function safeDecode(str) { // Replace any % not followed by two hex digits with %25 const sanitized = str.replace(/%(?![0-9A-Fa-f]{2})/g, '%25'); try { return decodeURIComponent(sanitized); } catch (e) { return str; // Return original if still invalid } } When working with modern web applications, consider using the URL and URLSearchParams APIs, which handle encoding and decoding more robustly than the legacy global functions [4].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global%5FObjects/decodeURIComponent
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/URIError
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Malformed_URI
- 4: https://urlencoder.tools/en/blog/fix-uri-malformed-error
- 5: https://urlencoder.tools/en/blog/javascript-decode-url
Avoid destroying the request before sending 413 response.
Line 141 calls req.destroy() before writing the response. In Node.js, this immediately destroys the underlying socket and can prevent a reliable 413 response from being transmitted. Use a flag-based approach instead to prevent further data processing while allowing the response to be sent cleanly.
Suggested fix
let body = "";
let bodyBytes = 0;
+ let payloadTooLarge = false;
req.on("data", (chunk) => {
+ if (payloadTooLarge) return;
bodyBytes += chunk.length;
if (bodyBytes > MAX_BODY_BYTES) {
- req.destroy();
+ payloadTooLarge = true;
res.writeHead(413, { "Content-Type": "application/json" });
res.end(JSON.stringify({ success: false, error: "Payload too large" }));
+ req.removeAllListeners("data");
+ req.resume();
return;
}
body += chunk.toString();
});
req.on("end", () => {
+ if (payloadTooLarge) return;
try {
const payload = JSON.parse(body);
handlePayload(payload);Wrap decodeURIComponent() in try-catch to handle malformed URI encoding.
Line 176 calls decodeURIComponent(key) without exception handling. If a user provides a malformed percent-encoded string (e.g., % not followed by two hex digits), it throws URIError and crashes the handler. Add a try-catch block around the decode call.
🤖 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 `@vite.config.ts` around lines 139 - 144, The code has two issues to fix.
First, in the body size validation logic where MAX_BODY_BYTES is checked,
replace the immediate req.destroy() call with a flag-based approach: introduce a
boolean flag (e.g., payloadTooLarge) that is set to true when the size limit is
exceeded, then check this flag in the data event handler to prevent further
processing while allowing the 413 response to be sent cleanly through writeHead
and end without destroying the socket. Second, wrap the decodeURIComponent(key)
call in a try-catch block to handle URIError exceptions that can be thrown when
users provide malformed percent-encoded strings like incomplete percent
sequences, and handle the caught error appropriately by either skipping that
key-value pair or logging a warning depending on your error handling strategy.
| const decodedKey = decodeURIComponent(key); | ||
| if (decodedKey.includes("..") || decodedKey.startsWith("/") || decodedKey.includes("\\")) { | ||
| res.writeHead(400, { "Content-Type": "text/plain" }); | ||
| res.end("Invalid storage key"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let me find the vite.config.ts file and examine the code context
git ls-files | grep -i vite.configRepository: Player10thGames/Fireboy-the-Brothers-The-Legend-of-Terra-Nemesis
Length of output: 123
🏁 Script executed:
cat -n vite.config.ts | sed -n '160,200p'Repository: Player10thGames/Fireboy-the-Brothers-The-Legend-of-Terra-Nemesis
Length of output: 1786
🌐 Web query:
decodeURIComponent throws URIError malformed percent encoding JavaScript
💡 Result:
The JavaScript function decodeURIComponent throws a URIError (specifically "malformed URI sequence" or similar, depending on the environment) when it encounters invalid percent-encoded data [1][2]. This error occurs in two primary scenarios: 1. Invalid Percent-Encoding: The string contains a percent sign (%) that is not followed by exactly two hexadecimal digits [1][3]. For example, decodeURIComponent('%') or decodeURIComponent('%G1') will trigger the error [4][3]. 2. Invalid UTF-8 Sequence: The percent-encoded bytes do not represent a valid UTF-8 character sequence [1][3]. To handle this error safely, you should wrap the call in a try...catch block [3]. Example of safe decoding: function safeDecodeURIComponent(uri) { try { return decodeURIComponent(uri); } catch (e) { console.error("Malformed URI:", e); return uri; // Or handle the error as appropriate for your application } } If you are encountering this error, ensure that the data being passed is correctly encoded using encodeURIComponent before decoding, or that it is already valid percent-encoded UTF-8 [5][3]. If you need to handle strings that might contain literal percent signs not intended for encoding, you can replace them with '%25' (the percent-encoded representation of %) before decoding [4][3].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/decodeURIComponent
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Malformed_URI
- 3: https://urlencoder.tools/en/blog/fix-uri-malformed-error
- 4: https://stackoverflow.com/questions/7449588/why-does-decodeuricomponent-lock-up-my-browser
- 5: https://stackoverflow.com/questions/65095317/decodeuricomponent-throws-malformed-uri-sequence
Handle malformed URL encoding before traversal checks.
Line 176 uses decodeURIComponent(key) without guarding for URIError. Malformed percent-encoding (e.g., %, %G1) or invalid UTF-8 sequences throw an error and bypass your intended 400 response, causing middleware failure.
Suggested fix
- const decodedKey = decodeURIComponent(key);
+ let decodedKey: string;
+ try {
+ decodedKey = decodeURIComponent(key);
+ } catch {
+ res.writeHead(400, { "Content-Type": "text/plain" });
+ res.end("Invalid storage key");
+ return;
+ }
if (decodedKey.includes("..") || decodedKey.startsWith("/") || decodedKey.includes("\\")) {
res.writeHead(400, { "Content-Type": "text/plain" });
res.end("Invalid storage key");
return;
}📝 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.
| const decodedKey = decodeURIComponent(key); | |
| if (decodedKey.includes("..") || decodedKey.startsWith("/") || decodedKey.includes("\\")) { | |
| res.writeHead(400, { "Content-Type": "text/plain" }); | |
| res.end("Invalid storage key"); | |
| return; | |
| } | |
| let decodedKey: string; | |
| try { | |
| decodedKey = decodeURIComponent(key); | |
| } catch { | |
| res.writeHead(400, { "Content-Type": "text/plain" }); | |
| res.end("Invalid storage key"); | |
| return; | |
| } | |
| if (decodedKey.includes("..") || decodedKey.startsWith("/") || decodedKey.includes("\\")) { | |
| res.writeHead(400, { "Content-Type": "text/plain" }); | |
| res.end("Invalid storage key"); | |
| return; | |
| } |
🤖 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 `@vite.config.ts` around lines 176 - 181, The decodeURIComponent(key) call in
the middleware can throw a URIError when the key contains malformed
percent-encoding, bypassing the path traversal validation and causing middleware
failure. Wrap the decodeURIComponent call in a try-catch block to catch URIError
exceptions, and respond with a 400 status code and appropriate error message
when such errors occur, similar to how the existing traversal checks handle
invalid keys.
1 participant
Summary
Addresses 3 security vulnerabilities found during a full codebase audit:
1. CRITICAL — Debug collector removed from production (
client/public/__manus__/)debug-collector.js(821 lines) captured all console output, network requests (including bodies), and user interactions. While the Vite plugin only injects the<script>tag in dev mode, the file itself was served as a static asset in production at a guessable path. Deleted and added to.gitignore.2. HIGH — Path traversal protection in storage proxy
Previously
req.urlwas passed unsanitized as apathquery param to the Forge storage API. A request like/manus-storage/../../../etc/passwdcould traverse into arbitrary backend paths.Also added a 2MB body size limit to the
/__manus__/logsendpoint to prevent DoS.3. MEDIUM — Security headers on Express production server
Added middleware setting
X-Content-Type-Options,X-Frame-Options,X-XSS-Protection,Referrer-Policy, andPermissions-Policyheaders on all responses.Note: Pre-existing type errors in
GameCanvas.tsxandApp.tsxare unrelated to these changes (confirmed by runningpnpm checkon the base branch).Link to Devin session: https://app.devin.ai/sessions/c241842118954ad8a3c4ece80340a771
Requested by: @Player10thGames
Summary by CodeRabbit
Bug Fixes
Chores