Skip to content

Conversation

@ChALkeR
Copy link

@ChALkeR ChALkeR commented Nov 2, 2025

User description

writeFileSync encoding option can take 'base64'
And this might be optimized at some point


PR Type

Enhancement


Description

  • Simplify base64 file writing using native encoding option

  • Remove unnecessary Buffer.from() conversion overhead

  • Leverage fs.writeFileSync() built-in base64 support


Diagram Walkthrough

flowchart LR
  A["base64Content string"] -->|"Buffer.from conversion"| B["Buffer object"]
  B -->|"writeFileSync"| C["File written"]
  A -->|"direct encoding option"| D["File written optimized"]
Loading

File Walkthrough

Relevant files
Enhancement
webdriver.js
Simplify base64 encoding in file write operation                 

javascript/selenium-webdriver/lib/webdriver.js

  • Replace Buffer.from(base64Content, 'base64') with direct encoding
    parameter
  • Pass 'base64' as third argument to fs.writeFileSync()
  • Eliminates intermediate Buffer object creation
  • Improves performance by leveraging native Node.js file system API
+1/-1     

Encoding option can take `base64`
And this might be optimized
@CLAassistant
Copy link

CLAassistant commented Nov 2, 2025

CLA assistant check
All committers have signed the CLA.

@selenium-ci selenium-ci added the C-nodejs JavaScript Bindings label Nov 2, 2025
@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Nov 2, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Unvalidated file write

Description: Writing unvalidated base64 content directly to disk can allow path-based or storage abuse
if upstream 'response.contents' or 'fileName' are attacker-controlled; ensure 'fileName'
and content source are trusted and validated.
webdriver.js [1740-1744]

Referred Code
fs.mkdirSync(targetDirectory, { recursive: true })
const zipFilePath = path.join(targetDirectory, `${fileName}.zip`)
fs.writeFileSync(zipFilePath, base64Content, 'base64')

const zipData = fs.readFileSync(zipFilePath)
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing audit logging: The new file write of decoded base64 content is not logged, so it’s unclear which
user/action triggered the write and its outcome.

Referred Code
fs.mkdirSync(targetDirectory, { recursive: true })
const zipFilePath = path.join(targetDirectory, `${fileName}.zip`)
fs.writeFileSync(zipFilePath, base64Content, 'base64')

const zipData = fs.readFileSync(zipFilePath)
await JSZip.loadAsync(zipData)
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Unhandled write errors: The direct call to fs.writeFileSync with base64 encoding lacks try/catch or error handling
for invalid base64, I/O failures, or permission issues.

Referred Code
fs.mkdirSync(targetDirectory, { recursive: true })
const zipFilePath = path.join(targetDirectory, `${fileName}.zip`)
fs.writeFileSync(zipFilePath, base64Content, 'base64')

const zipData = fs.readFileSync(zipFilePath)
await JSZip.loadAsync(zipData)
Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
No redaction assurance: Writing decoded content to disk may involve sensitive data, and there is no accompanying
structured logging or confirmation that sensitive content is not logged elsewhere.

Referred Code
fs.mkdirSync(targetDirectory, { recursive: true })
const zipFilePath = path.join(targetDirectory, `${fileName}.zip`)
fs.writeFileSync(zipFilePath, base64Content, 'base64')

const zipData = fs.readFileSync(zipFilePath)
await JSZip.loadAsync(zipData)
Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Unvalidated input: The base64 input from response.contents is written directly without validation of encoding
correctness, size limits, or path safety for targetDirectory/fileName.

Referred Code
const base64Content = response.contents

if (!targetDirectory.endsWith('/')) {
  targetDirectory += '/'
}

fs.mkdirSync(targetDirectory, { recursive: true })
const zipFilePath = path.join(targetDirectory, `${fileName}.zip`)
fs.writeFileSync(zipFilePath, base64Content, 'base64')
  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Nov 2, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Await asynchronous operations in loop

Replace the forEach loop with Promise.all and map to correctly await the
asynchronous file writing operations and prevent a race condition. Also, use
path.join for constructing file paths.

javascript/selenium-webdriver/lib/webdriver.js [1748-1752]

-Object.keys(zip.files).forEach(async (fileName) => {
-  const fileData = await zip.files[fileName].async('nodebuffer')
-  fs.writeFileSync(`${targetDirectory}/${fileName}`, fileData)
-  console.log(`File extracted: ${fileName}`)
-})
+await Promise.all(
+  Object.keys(zip.files).map(async (fileName) => {
+    const fileData = await zip.files[fileName].async('nodebuffer')
+    fs.writeFileSync(path.join(targetDirectory, fileName), fileData)
+    console.log(`File extracted: ${fileName}`)
+  }),
+)

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a race condition bug where forEach is used with an async callback, which can lead to the function resolving before file operations are complete. The proposed fix using Promise.all is the correct pattern to solve this issue.

High
Propagate errors in promise chain

Re-throw the error inside the .catch block to ensure that failures during the
unzipping process are propagated to the caller of the downloadFile function.

javascript/selenium-webdriver/lib/webdriver.js [1754-1756]

 .catch((error) => {
   console.error('Error unzipping file:', error)
+  throw error
 })

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a significant error handling flaw where an error is caught but not propagated, causing the function to appear successful to the caller. Re-throwing the error is crucial for robust error handling.

Medium
General
Avoid unnecessary intermediate file I/O

Improve efficiency by passing the base64Content string directly to
JSZip.loadAsync with the { base64: true } option, thus avoiding the unnecessary
step of writing to and reading from a temporary zip file.

javascript/selenium-webdriver/lib/webdriver.js [1742-1745]

-fs.writeFileSync(zipFilePath, base64Content, 'base64')
+// The zip file does not need to be written to disk.
+// fs.writeFileSync(zipFilePath, base64Content, 'base64')
 
-const zipData = fs.readFileSync(zipFilePath)
-await JSZip.loadAsync(zipData)
+// const zipData = fs.readFileSync(zipFilePath)
+await JSZip.loadAsync(base64Content, { base64: true })
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion provides a significant performance and simplification improvement by avoiding unnecessary file I/O. Processing the base64 content directly in memory is more efficient and makes the code cleaner.

Medium
  • Update

@ChALkeR ChALkeR changed the title just writeFileSync with base64 directly just call writeFileSync with base64 directly Nov 2, 2025
@cgoldberg cgoldberg changed the title just call writeFileSync with base64 directly [nodejs] Call writeFileSync with base64 directly Nov 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants