Skip to content

IEP-1628: Remove conflicting System env variables for export command #1305

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

IEP-1628: Remove conflicting System env variables for export command #1305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ea86896
IEP-1628: Remove conflicting System env variables for export command
alirana01 Sep 8, 2025
060265c
updated keys to remove that can cause possible issues
alirana01 Sep 9, 2025
938727b
moved function to utility method
alirana01 Sep 15, 2025
bdd4169
fixing typo
alirana01 Sep 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.regex.Matcher;
Expand Down Expand Up @@ -993,4 +994,29 @@ public static String runEspDetectConfigScript()
return null;
}
}

/**
* Remove the variables which can affect the idf_tools.py export command
* These variables can come from system environment
* @param environment
*/
public static void cleanUpSystemEnvironment(Map<String, String> environment)
{
List<String> keysToRenmove = new LinkedList<>();
keysToRenmove.add(IDFEnvironmentVariables.IDF_PYTHON_ENV_PATH);
keysToRenmove.add(IDFEnvironmentVariables.IDF_PATH);
keysToRenmove.add(IDFEnvironmentVariables.ESP_IDF_VERSION);
keysToRenmove.add(IDFEnvironmentVariables.IDF_CCACHE_ENABLE);
keysToRenmove.add(IDFEnvironmentVariables.IDF_COMPONENT_MANAGER);
keysToRenmove.add(IDFEnvironmentVariables.IDF_MAINTAINER);
keysToRenmove.add(IDFEnvironmentVariables.OPENOCD_SCRIPTS);

for (String key : keysToRenmove)
{
if (environment.containsKey(key))
{
environment.remove(key);
}
}
}
}
Comment on lines +1003 to 1022
Copy link
Collaborator

@kolipakakondal kolipakakondal Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of cleaning up system environment variables, why can't we rely only on the environment variables configured in the IDE and that includes PATH as well?

IDFEnvironmentVariables().getEnvMap()

Otherwise, if we go with the cleanup approach, we'll have to maintain this code all the time whenever there are changes in the IDF environment variables.

Please check?

6 changes: 6 additions & 0 deletions bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,7 @@ protected IStatus runCommand(List<String> arguments, MessageConsoleStream consol
Logger.log(cmdMsg);

Map<String, String> environment = new HashMap<>(IDFUtil.getSystemEnv());
Logger.log("Environment Variables from System:");
Logger.log(environment.toString());
environment.put("PYTHONUNBUFFERED", "1"); //$NON-NLS-1$ //$NON-NLS-2$
Comment on lines +474 to 476
Copy link

@coderabbitai coderabbitai bot Sep 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Stop logging full environment (secrets/PII risk).
Dumping env maps can leak tokens, proxies, and credentials to logs.

-			Logger.log("Environment Variables from System:");
-			Logger.log(environment.toString());
+			// Avoid logging full environment; contains secrets.
@@
-			
-			Logger.log("Environment Variables after filtering from system:");
-			IDFUtil.cleanUpSystemEnvironment(environment);
-			Logger.log(environment.toString());
-			
+			IDFUtil.cleanUpSystemEnvironment(environment);

Also applies to: 492-495

🏁 Script executed:

#!/bin/bash
# Find other places that log entire environment maps/values
rg -nP -C1 '(environment\\.toString\\(\\)|Logger\\.log\\(.+Environment Variables)' -g '!**/target/**' .

Length of output: 72

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Searching for environment.toString occurrences..."
rg -n -C1 --hidden --no-ignore 'environment\.toString\(' -g '!**/target/**' . || true

echo "Searching for Logger.log(...) containing 'Environment Variables'..."
rg -n -C1 --hidden --no-ignore 'Logger\.log\([^)]*Environment Variables' -g '!**/target/**' . || true

echo "Searching for Logger.log(...) that logs environment.toString..."
rg -n -C1 --hidden --no-ignore 'Logger\.log\([^)]*environment\.toString\(' -g '!**/target/**' . || true

echo "Searching for System.getenv / System.getProperty usages..."
rg -n -C1 --hidden --no-ignore 'System\.getenv\(|System\.getProperty\(' -g '!**/target/**' . || true

echo "Done"

Length of output: 245

Stop logging full environment (secrets risk).

File: bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java — lines 474–476 and 492–495

-			Logger.log("Environment Variables from System:");
-			Logger.log(environment.toString());
+			// Avoid logging full environment; contains secrets.
@@
-			
-			Logger.log("Environment Variables after filtering from system:");
-			IDFUtil.cleanUpSystemEnvironment(environment);
-			Logger.log(environment.toString());
-			
+			IDFUtil.cleanUpSystemEnvironment(environment);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Logger.log("Environment Variables from System:");
Logger.log(environment.toString());
environment.put("PYTHONUNBUFFERED", "1"); //$NON-NLS-1$ //$NON-NLS-2$
// Avoid logging full environment; contains secrets.
environment.put("PYTHONUNBUFFERED", "1"); //$NON-NLS-1$ //$NON-NLS-2$
...
IDFUtil.cleanUpSystemEnvironment(environment);
🤖 Prompt for AI Agents 
In bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsJob.java
around lines 474–476 and 492–495, the code currently logs the full environment
map which may expose secrets; remove the Logger.log(environment.toString()) call
and any other direct prints of the environment, and instead log non-sensitive
metadata (e.g., number of variables or a filtered list of safe, whitelisted
variable names) or redact values before logging; ensure PYTHONUNBUFFERED is
still set but never include full environment contents or unredacted values in
logs.

environment.put("IDF_GITHUB_ASSETS", //$NON-NLS-1$
Expand All @@ -487,6 +488,11 @@ protected IStatus runCommand(List<String> arguments, MessageConsoleStream consol
{
addPathToEnvironmentPath(environment, gitExecutablePath);
}

Logger.log("Environment Variables after filtering from system:");
IDFUtil.cleanUpSystemEnvironment(environment);
Logger.log(environment.toString());

Process process = processRunner.run(arguments, org.eclipse.core.runtime.Path.ROOT, environment);
IStatus status = processData(process);
console.println();
Expand Down
2 changes: 2 additions & 0 deletions bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/ExportIDFTools.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ public IStatus getToolsExportOutputFromGivenIdfPath(final String pythonExePath,
addGitToEnvironment(environment, gitExePath);
}

IDFUtil.cleanUpSystemEnvironment(environment);

final ProcessBuilderFactory processRunner = new ProcessBuilderFactory();
try
{
Expand Down
Loading