Fix header normalization for xlsx #612
Conversation
WalkthroughThe changes introduce a new test suite for the Changes
Possibly related PRs
Recent review detailsConfiguration used: CodeRabbit UI Files ignored due to path filters (1)
Files selected for processing (3)
Additional comments not posted (7)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![flatfile-nullify[bot]](https://avatars.githubusercontent.com/u/31830338?s=60&v=4){kind=small}
| console.log(event.topic) | ||
| }) | ||
|
|
||
| await api.files.upload(fs.createReadStream(path.join(__dirname,'../ref/test-headers.xlsx')), { |
There was a problem hiding this comment.
Nullify Code Language: TypeScript 🔵 MEDIUM Severity CWE-22
Javascript pathtraversal rule non literal fs filename
The application dynamically constructs file or path information. If the path
information comes from user-supplied input, it could be abused to read sensitive files,
access other users' data, or aid in exploitation to gain further system access.
User input should never be used in constructing paths or files for interacting
with the filesystem. This includes filenames supplied by user uploads or downloads.
If possible, consider hashing user input or using unique values and
use path.normalize to resolve and validate the path information
prior to processing any file functionality.
Example using path.normalize and not allowing direct user input:
// User input, saved only as a reference
// id is a randomly generated UUID to be used as the filename
const userData = {userFilename: userSuppliedFilename, id: crypto.randomUUID()};
// Restrict all file processing to this directory only
const basePath = '/app/restricted/';
// Create the full path, but only use our random generated id as the filename
const joinedPath = path.join(basePath, userData.id);
// Normalize path, removing any '..'
const fullPath = path.normalize(joinedPath);
// Verify the fullPath is contained within our basePath
if (!fullPath.startsWith(basePath)) {
console.log("Invalid path specified!");
}
// Process / work with file
// ...
For more information on path traversal issues see OWASP:
https://owasp.org/www-community/attacks/Path_Traversal
⚡ Here's how you might fix this potential vulnerability
The modified code first constructs the full path of the file, then uses 'path.basename()' to get the filename. This ensures that only the filename is used in the filesystem operation, preventing directory traversal. Even though '__dirname' is not controllable by an attacker, it's a good practice to always validate or sanitize filenames in filesystem operations.
autoFixesExperimental
Use path.basename() to get the filename and prevent directory traversal
| await api.files.upload(fs.createReadStream(path.join(__dirname,'../ref/test-headers.xlsx')), { | |
| const filePath = path.join(__dirname, '../ref/test-headers.xlsx'); | |
| const fileName = path.basename(filePath); | |
| await api.files.upload(fs.createReadStream(filePath), { |
poweredByNullify
Reply with /nullify to interact with me like another developer
(you will need to refresh the page for updates)
Nullify Code Vulnerabilities1 findings found in this pull request
You can find a list of all findings here |
Please explain how to summarize this PR for the Changelog:
Tell code reviewer how and what to test: