Skip to content

Expand environment variables in override argument #5551

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

farheen-shaikh530
Copy link

@farheen-shaikh530 farheen-shaikh530 commented Jun 21, 2025


Summary of Changes

This pull request adds support to expand environment variables passed in the --override argument during execution. The update ensures that any environment variable (e.g., %ProgramFiles% or $HOME) used in the override value is properly expanded before proceeding with the install process.

Motivation

Previously, environment variables in the override argument were not resolved, potentially leading to incorrect paths or failed executions. This change improves user experience by handling environment expansion automatically, aligning with common CLI expectations and improving script compatibility.

I have read the CLA Document and I hereby sign the CLA.

@microsoft-github-policy-service agree

@farheen-shaikh530
Copy link
Author

Hi team, kindly requesting a review for this PR when convenient. This change expands environment variables in the --override argument. Thanks!

@farheen-shaikh530
Copy link
Author

Hi maintainers!
This PR adds support to expand environment variables in the --override argument (e.g., %ProgramFiles%, $HOME), improving script compatibility and CLI behavior. Kindly requesting a review when convenient. Thanks for your time and help!

@microsoft-github-policy-service agree

@yao-msft
Copy link
Contributor

I don't feel winget should be expanding user inputs, override is mainly for take whatever user inputs and pass to the installer. And I feel it may have potential security implications if we just expand whatever env var that's in the string and pass along to execution.

@farheen-shaikh530
Copy link
Author

I don't feel winget should be expanding user inputs, override is mainly for take whatever user inputs and pass to the installer. And I feel it may have potential security implications if we just expand whatever env var that's in the string and pass along to execution.

Thank you for the insightful feedback @yao-msft

I understand the concern around expanding user inputs by default and the possible security implications. Would it be acceptable to introduce an explicit opt-in flag (e.g., --expand-env) that enables variable expansion only when the user explicitly requests it? This would preserve default safety while still supporting power users who want to inject env vars intentionally. Happy to update the PR accordingly if this sounds like a reasonable compromise.

@yao-msft
Copy link
Contributor

yao-msft commented Jun 23, 2025

The opt-in would be an admin setting and a policy behind it if we are going to do it. @denelon to decide if we want to do this feature. By the way, if we are doing it, some other arguments should support it too (InstallLocation, Custom, loglocation, etc, maybe also --manifest?). The code should move to when we process the input arguments, instead of some workflow task in the middle.

@farheen-shaikh530
Copy link
Author

The opt-in would be an admin setting and a policy behind it if we are going to do it. @denelon to decide if we want to do this feature. By the way, if we are doing it, some other arguments should support it too (InstallLocation, Custom, loglocation, etc, maybe also --manifest?). The code should move to when we process the input arguments, instead of some workflow task in the middle.

Hi @yao-msft,

Thank you for your feedback! I understand your concern regarding the potential security implications of expanding environment variables directly. Based on your suggestion, I can rework the implementation to:
• Limit expansion to only specific, predefined arguments (e.g., InstallLocation, Custom).
• Move the expansion logic to where input arguments are processed, instead of later in the workflow.
• Wrap this behind an admin policy toggle to ensure explicit opt-in.

I’ll wait to hear from @denelon for a final go/no-go decision before proceeding. Let me know if there are any additional constraints or preferred safeguards you’d recommend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants