-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ Amplify ] CWE-22 Fix routes/dataErasure.ts:69 #159
base: master
Are you sure you want to change the base?
Conversation
Automated code fix by Amplify Security accepted by [email protected]
🔍 Amplify code check status:
Vulnerabilities detectedClick on a CWE to view vulnerability in Amplify
Last updated by commit 0d0ccdc at 2024-12-12 15:02:26 UTC. |
@@ -66,7 +66,8 @@ router.post('/', async (req: Request<{}, {}, DataErasureRequestParams>, res: Res | |||
|
|||
res.clearCookie('token') | |||
if (req.body.layout !== undefined) { | |||
const filePath: string = path.resolve(req.body.layout).toLowerCase() | |||
const sanitizedLayout: string = path.basename(req.body.layout) | |||
const filePath: string = path.resolve('allowed_directory', sanitizedLayout).toLowerCase() | |||
const isForbiddenFile: boolean = (filePath.includes('ftp') || filePath.includes('ctf.key') || filePath.includes('encryptionkeys')) | |||
if (!isForbiddenFile) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 Impact: MEDIUM
📰 The Gist: Detected possible user input going into a path.join
or path.resolve
function. This could possibly lead to a path traversal vulnerability, where the attacker can access arbitrary files stored in the file system. Instead, be sure to sanitize or validate user input first.
Suggested code
--- routes/dataErasure.ts
+++ routes/dataErasure.ts
@@ -69,7 +69,8 @@
const sanitizedLayout: string = path.basename(req.body.layout)
const filePath: string = path.resolve('allowed_directory', sanitizedLayout).toLowerCase()
const isForbiddenFile: boolean = (filePath.includes('ftp') || filePath.includes('ctf.key') || filePath.includes('encryptionkeys'))
- if (!isForbiddenFile) {
+ const isValidPath: boolean = filePath.startsWith(path.resolve('allowed_directory') + path.sep)
+ if (!isForbiddenFile && isValidPath) {
res.render('dataErasureResult', {
...req.body
}, (error, html) => {
@@ -66,7 +66,8 @@ router.post('/', async (req: Request<{}, {}, DataErasureRequestParams>, res: Res | |||
|
|||
res.clearCookie('token') | |||
if (req.body.layout !== undefined) { | |||
const filePath: string = path.resolve(req.body.layout).toLowerCase() | |||
const sanitizedLayout: string = path.basename(req.body.layout) | |||
const filePath: string = path.resolve('allowed_directory', sanitizedLayout).toLowerCase() | |||
const isForbiddenFile: boolean = (filePath.includes('ftp') || filePath.includes('ctf.key') || filePath.includes('encryptionkeys')) | |||
if (!isForbiddenFile) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 Impact: MEDIUM
📰 The Gist: Possible writing outside of the destination, make sure that the target path is nested in the intended destination
Suggested code
--- routes/dataErasure.ts
+++ routes/dataErasure.ts
@@ -69,7 +69,8 @@
const sanitizedLayout: string = path.basename(req.body.layout)
const filePath: string = path.resolve('allowed_directory', sanitizedLayout).toLowerCase()
const isForbiddenFile: boolean = (filePath.includes('ftp') || filePath.includes('ctf.key') || filePath.includes('encryptionkeys'))
- if (!isForbiddenFile) {
+ const isOutsideAllowedDirectory: boolean = !filePath.startsWith(path.resolve('allowed_directory').toLowerCase())
+ if (!isForbiddenFile && !isOutsideAllowedDirectory) {
res.render('dataErasureResult', {
...req.body
}, (error, html) => {
This Pull Request fixes CWE-22, Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal'), in routes/dataErasure.ts:69.