fix(imageproxy): restrict to relative paths to prevent baseURL override#2473
fix(imageproxy): restrict to relative paths to prevent baseURL override#2473RinZ27 wants to merge 1 commit intoseerr-team:developfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughInput validation was added to the TMDB/TVDB image proxy route to ensure the image path adheres to security requirements, rejecting paths with invalid formats such as double slashes, missing leading slash, or URL schemes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/routes/imageproxy.ts (1)
41-41: Consider sanitizing logged input.The
imagePathis user-controlled and logged directly. While this is useful for debugging, consider truncating or sanitizing it to prevent potential log injection or excessively large log entries from malicious inputs.🔧 Optional: Truncate logged path
- logger.error('Invalid image path detected', { imagePath }); + logger.error('Invalid image path detected', { imagePath: imagePath.slice(0, 200) });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/imageproxy.ts` at line 41, The logger.error call logs the user-controlled variable imagePath directly; sanitize and/or truncate it before logging to prevent log injection or oversized entries by e.g. stripping control/newline characters and limiting length (e.g. to 200 chars) and then pass the sanitized value to logger.error; update the usage in server/routes/imageproxy.ts where logger.error('Invalid image path detected', { imagePath }) is called to use the sanitized/truncated imagePath variable instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/routes/imageproxy.ts`:
- Line 41: The logger.error call logs the user-controlled variable imagePath
directly; sanitize and/or truncate it before logging to prevent log injection or
oversized entries by e.g. stripping control/newline characters and limiting
length (e.g. to 200 chars) and then pass the sanitized value to logger.error;
update the usage in server/routes/imageproxy.ts where logger.error('Invalid
image path detected', { imagePath }) is called to use the sanitized/truncated
imagePath variable instead.
4923e31 to
03d4bb6
Compare
|
I don't think I can reproduce this issue. When I try to inject an absolute URL to the image proxy, the URL is always prefixed with |
Description
/imageproxyroute could be tricked into fetching absolute URLs, bypassing the intendedbaseURL.imagePathis a relative path starting with/and does not contain protocol strings or double slashes.How Has This Been Tested?
/t/p/xxx.jpg) still work.Checklist:
Summary by CodeRabbit