Skip to content
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

fix: invalid $HOME is not detected #4901

Merged
merged 3 commits into from
Jul 7, 2024
Merged

fix: invalid $HOME is not detected #4901

merged 3 commits into from
Jul 7, 2024

Conversation

ivikash
Copy link
Member

@ivikash ivikash commented May 1, 2024 •
edited by justinmk3
Loading

Problem

  • Users with misconfigured env variables -HOME or USERPROFILE or HOMEPATH AWS_SHARED_CREDENTIALS_FILE or AWS_CONFIG_FILE are unable to login into Amazon Q and AWS Toolkits extensions.

Solution

  • Validate $HOME and related env vars.
  • if $HOME is invalid, try other env vars or os.homedir().
  • Show an error on startup if a valid home dir couldn't be resolved.

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ivikash ivikash requested a review from a team as a code owner May 1, 2024 06:22
@ivikash ivikash marked this pull request as draft May 1, 2024 06:22
@ivikash ivikash force-pushed the fix/vikash/aws-credentials-location branch 4 times, most recently from aa1d7ec to c318a6e Compare May 5, 2024 01:38
@ivikash ivikash closed this May 5, 2024
@ivikash ivikash reopened this May 5, 2024
@ivikash ivikash marked this pull request as ready for review May 5, 2024 01:47
@ivikash ivikash force-pushed the fix/vikash/aws-credentials-location branch 4 times, most recently from faaf52a to 7a6b048 Compare May 6, 2024 23:44
@ivikash
Copy link
Member Author

ivikash commented May 7, 2024 •
edited
Loading

/retryBuilds

@justinmk3
Copy link
Contributor

Users on windows are unable to login to AWS Toolkits and Amazon Q extension on Windows because env variable: HOME is set incorrectly

It's not really a Windows specific issue. Any system with a misconfigured $HOME env var could have this issue.

@ivikash ivikash force-pushed the fix/vikash/aws-credentials-location branch from 7a6b048 to 7e7b1a1 Compare May 8, 2024 17:09
@ivikash
Copy link
Member Author

ivikash commented May 8, 2024 •
edited by justinmk3
Loading

/runIntegrationTests

@ivikash ivikash force-pushed the fix/vikash/aws-credentials-location branch from 7e7b1a1 to 7103ade Compare May 8, 2024 17:29
justinmk3
justinmk3 requested changes May 8, 2024
View reviewed changes
packages/core/src/shared/utilities/pathUtils.ts Outdated
* @param {string} [path] - The path to be checked. If not provided, defaults to an empty string.
* @returns {boolean} Returns true if the path is valid and exists, otherwise returns false.
*/
export function isValidPath(path?: string): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

this would never be hit in web-mode because getHomeDirectory returns early for web-mode

Copy link
Member Author

@ivikash ivikash May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

vscode only provide Thenable aka Promise based api. To allow this in web, it will require updating all touch points with a Promise based API.

https://github.com/microsoft/vscode/blob/main/src/vscode-dts/vscode.d.ts#L9045

Copy link
Contributor

Choose a reason for hiding this comment

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

I pushed a change to introduce initUserHomeDir(), which is a one-time routine done at startup that validates $HOME and other env vars. That allows initUserHomeDir() to be async while getUserHomeDir() remains "sync". As a minor perf bonus, getUserHomeDir() now uses a cached value.

packages/core/src/shared/utilities/pathUtils.ts Outdated Show resolved Hide resolved
packages/core/src/test/credentials/sharedCredentials.test.ts Outdated
@@ -17,6 +19,7 @@ describe('sharedCredentials', function () {
// Make a temp folder for all these tests
// Stick some temp credentials files in there to load from
tempFolder = await makeTemporaryToolkitFolder()
sinon.stub(pathUtils, 'isValidPath').returns(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

stubbing isValidPath in various tests that happen to current hit the getHomeDirectory() codepath, means that future tests might run into the same issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Given that in unit tests we are validating an actual path, indeed it will require stubbing in future as well. What would be your recommendation?

Copy link
Contributor

Choose a reason for hiding this comment

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

The change that I pushed avoids this problem by only validating at startup (when initUserHomeDir() is called).

@ivikash ivikash force-pushed the fix/vikash/aws-credentials-location branch 3 times, most recently from 92b8259 to 6820f30 Compare May 8, 2024 19:04
@justinmk3 justinmk3 force-pushed the fix/vikash/aws-credentials-location branch from 6820f30 to c120555 Compare July 6, 2024 15:19
@justinmk3 justinmk3 force-pushed the fix/vikash/aws-credentials-location branch from c120555 to 25199a5 Compare July 7, 2024 17:07
@justinmk3 justinmk3 self-requested a review July 7, 2024 17:10
@justinmk3 justinmk3 force-pushed the fix/vikash/aws-credentials-location branch from 25199a5 to c60910f Compare July 7, 2024 18:07
@justinmk3 justinmk3 changed the title fix: validate path for getHomeDirectory fix: invalid $HOME is not detected Jul 7, 2024
@justinmk3 justinmk3 force-pushed the fix/vikash/aws-credentials-location branch 3 times, most recently from 3387d09 to c0e6864 Compare July 7, 2024 19:13
justinmk3
justinmk3 reviewed Jul 7, 2024
View reviewed changes
packages/core/src/auth/credentials/sharedCredentialsFile.ts Outdated
const f = resolve(envVal)
fs.existsFile(f).then(r => {
if (!r) {
getLogger().error('$%s filepath is invalid (or is a directory): %O', envVar, f)
Copy link
Contributor

Choose a reason for hiding this comment

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

Error is logged asynchronously, and the value is still returned. This isn't ideal, but is less critical than the $HOME validation.

I'll followup later so that invalid AWS_CONFIG_FILE and AWS_SHARED_CREDENTIALS_FILE surface an error on startup.

@justinmk3 justinmk3 force-pushed the fix/vikash/aws-credentials-location branch from c0e6864 to 8fb47eb Compare July 7, 2024 19:24
@aws aws deleted a comment from ivikash Jul 7, 2024
@aws aws deleted a comment from ivikash Jul 7, 2024
@aws aws deleted a comment from ivikash Jul 7, 2024
@justinmk3 justinmk3 force-pushed the fix/vikash/aws-credentials-location branch 3 times, most recently from 4fa6a29 to 39b9fc8 Compare July 7, 2024 20:27
@justinmk3
fix(env): fs.getUserHomeDir()
9aef201 
Problem:
- `SystemUtilities.getHomeDirectory`:
  - does not use the cross-platform `fs.ts` module.
  - checks env vars every time it is called, which is a performance cost.

Solution:
- Move validation into a one-time `initUserHomeDir()` function which is
  called on startup.
- Introduce `fs.getUserHomeDir()`.
@justinmk3 justinmk3 force-pushed the fix/vikash/aws-credentials-location branch from 39b9fc8 to 9aef201 Compare July 7, 2024 20:39
@justinmk3 justinmk3 merged commit bbec356 into aws:master Jul 7, 2024
19 of 20 checks passed
@ivikash ivikash deleted the fix/vikash/aws-credentials-location branch July 8, 2024 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinmk3 justinmk3 justinmk3 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ivikash @justinmk3