Skip to content

New functionality for managing tokens securely #584

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

Jump to bottom
Merged
AmelBawa-msft merged 11 commits into from
Feb 25, 2025
Merged

New functionality for managing tokens securely #584

AmelBawa-msft merged 11 commits into from
Feb 25, 2025

Conversation

@AmelBawa-msft
Copy link
Contributor

@AmelBawa-msft AmelBawa-msft commented Feb 20, 2025 •
edited
Loading

Summary of Changes

🟢 New URL: https://aka.ms/winget-create-token

This pull request includes multiple changes focused on improving the handling and documentation of GitHub tokens in the Winget-Create CLI. The changes include updates to documentation files and the addition of new functionality for managing tokens securely.

Screenshots

...
-t, --token                GitHub personal access token used for direct submission to the Windows
                           Package Manager repo. If no token is provided, tool will prompt for
                           GitHub login credentials.

                           Warning: Using this argument may result in the token being logged.
                           Consider an alternative approach https://aka.ms/winget-create-token.
...

PS c:\> wingetcreate submit -p "Example" 'C:\' --token "Example"
Warning: Using the --token argument may result in the token being logged. Consider an alternative approach https://aka.ms/winget-create-token.
...

Documentation Updates:

  • Added warnings to various documentation files (doc/new-locale.md, doc/new.md, doc/show.md, doc/submit.md, doc/token.md, doc/update-locale.md, doc/update.md) about the potential logging of GitHub tokens when using the --token argument, and recommended alternative approaches. [1] [2] [3] [4] [5] [6] [7] [8]

New Functionality:

  • Introduced the TokenHelper class to handle token operations using the Windows credentials manager and environment variables, enhancing security and flexibility.

Warning

For local development, it is recommended to go through the OAuth flow by omitting the --token argument.

For CI/CD scenarios, it is recommended to use the 'WINGET_CREATE_GITHUB_TOKEN' environment variable to store the token.

Logging Improvements:

  • Added a warning message (in the Program.cs file) to notify users when a token is provided via the command line, highlighting the risk of token logging.

Resource Updates:

  • Updated resource files (Resources.Designer.cs, Resources.resx) to include new warning messages related to token usage. [1] [2] [3]

Related links:

These changes collectively enhance the security and user awareness regarding the handling of GitHub tokens within the Winget-Create CLI.

Microsoft Reviewers: Open in CodeFlow

@AmelBawa-msft AmelBawa-msft requested review from a team, JohnMcPMS, florelis, ryfu-msft and yao-msft and removed request for a team February 20, 2025 19:12
AmelBawa-msft
AmelBawa-msft commented Feb 20, 2025
View reviewed changes
mdanish-kh
mdanish-kh reviewed Feb 20, 2025
View reviewed changes
@mdanish-kh
Copy link
Contributor

mdanish-kh commented Feb 20, 2025

Since we're concerned about the possibilities of token being logged, should we re-visit the setup for running E2E tests for the project? It requires a user writing their token in src/WingetCreateTests/WingetCreateTests/Test.runsettings, a file that isn't ignored by git by default. The responsibility is on the user to not commit this token to their tree, or worse to open a PR here. We do warn the users in the doc about ways to ignore it from git, but maybe we need to think of a better flow?

JohnMcPMS
JohnMcPMS reviewed Feb 21, 2025
View reviewed changes
@JohnMcPMS
Copy link
Member

JohnMcPMS commented Feb 21, 2025

Since we're concerned about the possibilities of token being logged, should we re-visit the setup for running E2E tests for the project? It requires a user writing their token in src/WingetCreateTests/WingetCreateTests/Test.runsettings, a file that isn't ignored by git by default. The responsibility is on the user to not commit this token to their tree, or worse to open a PR here. We do warn the users in the doc about ways to ignore it from git, but maybe we need to think of a better flow?

We discussed this and I agree that we should:

  1. No longer consume the token from a file that might easily end up in a PR
  2. Actively promote removal of the token from the runsettings file by flat out failing the tests if it is present

JohnMcPMS
JohnMcPMS reviewed Feb 24, 2025
View reviewed changes
@mdanish-kh
Copy link
Contributor

mdanish-kh commented Feb 25, 2025

With the new changes in d62fbcb, we should also update the Running Unit & E2E tests section in the README

@AmelBawa-msft
Copy link
Contributor Author

AmelBawa-msft commented Feb 25, 2025

With the new changes in d62fbcb, we should also update the Running Unit & E2E tests section in the README

Good catch! Updated 😊

@AmelBawa-msft AmelBawa-msft merged commit c79d2ee into main Feb 25, 2025
1 check passed
@AmelBawa-msft AmelBawa-msft deleted the user/amelbawa/token branch February 25, 2025 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JohnMcPMS JohnMcPMS JohnMcPMS approved these changes

@yao-msft yao-msft Awaiting requested review from yao-msft

@ryfu-msft ryfu-msft Awaiting requested review from ryfu-msft

@florelis florelis Awaiting requested review from florelis

@denelon denelon Awaiting requested review from denelon

+1 more reviewer

@mdanish-kh mdanish-kh mdanish-kh left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AmelBawa-msft @mdanish-kh @JohnMcPMS