fix: path Traversal #10447
fix: path Traversal #10447
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors locale template path resolution and configuration management to improve type safety and security. The main changes include adding a new resolveLocaleTemplatePath utility that validates template paths and sanitizes locale configuration values to prevent invalid locales from being stored.
- Introduced
resolveLocaleTemplatePathfunction for secure, locale-aware template path resolution with fallback support - Added
sanitizeConfigValuefunction to validate and normalize locale configuration values at persistence boundaries - Updated all email template path constructions to use the new secure resolution method
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/app/src/server/util/locale-utils.ts | Added new resolveLocaleTemplatePath and coerceToSupportedLang functions with path traversal protection and locale validation |
| apps/app/src/server/service/global-notification/global-notification-mail.js | Updated to use async resolveLocaleTemplatePath for template resolution |
| apps/app/src/server/service/config-manager/config-manager.ts | Integrated sanitizeConfigValue to normalize locale values before database persistence |
| apps/app/src/server/service/config-manager/config-loader.ts | Added sanitizeConfigValue function and applied sanitization during config loading from environment and database |
| apps/app/src/server/service/config-manager/config-definition.ts | Changed app:globalLang type from string to Lang enum |
| apps/app/src/server/routes/login.js | Migrated from direct path construction to resolveLocaleTemplatePath |
| apps/app/src/server/routes/apiv3/users.js | Migrated from direct path construction to resolveLocaleTemplatePath |
| apps/app/src/server/routes/apiv3/user-activation.ts | Migrated from direct path construction to resolveLocaleTemplatePath |
| apps/app/src/server/routes/apiv3/forgot-password.js | Migrated from direct path construction to resolveLocaleTemplatePath, improved error message security |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return relative !== '' && !relative.startsWith('..') && !path.isAbsolute(relative); | ||
| }; | ||
|
|
||
| const doesTemplateExist = async(candidatePath: string, baseDir: string): Promise<boolean> => { |
There was a problem hiding this comment.
Missing space between 'async' and the opening parenthesis. Should be async (candidatePath instead of async(candidatePath.
| return Array.from(locales).map(locale => path.resolve(resolvedBase, locale, ...segments)); | ||
| }; | ||
|
|
||
| // Guard functions that collapse to undefined or default values for further processing. |
There was a problem hiding this comment.
Corrected 'functions' to 'function' since only one function follows this comment.
| // Guard functions that collapse to undefined or default values for further processing. | |
| // Guard function that collapses to undefined or default values for further processing. |
|
|
||
| const isPathInsideBase = (basePath: string, targetPath: string): boolean => { | ||
| const relative = path.relative(basePath, targetPath); | ||
| return relative !== '' && !relative.startsWith('..') && !path.isAbsolute(relative); |
There was a problem hiding this comment.
The logic for isPathInsideBase is incorrect. When basePath and targetPath are the same, path.relative() returns an empty string, and the function returns false. However, a path pointing to the base directory itself could be considered valid. More importantly, if targetPath equals basePath, the condition relative !== '' will fail. Consider whether this edge case is intentional or if the logic should be relative === '' || (!relative.startsWith('..') && !path.isAbsolute(relative)).
| return relative !== '' && !relative.startsWith('..') && !path.isAbsolute(relative); | |
| return relative === '' || (!relative.startsWith('..') && !path.isAbsolute(relative)); |
タスク : https://redmine.weseek.co.jp/issues/171275