Skip to content

Conversation

@IanMatthewHuff
Copy link
Member

@IanMatthewHuff IanMatthewHuff commented May 25, 2022

This PR fixes #149104

@IanMatthewHuff
Copy link
Member Author

Ok, putting this up quick as a draft PR. Will look to discuss quick with @TylerLeonhardt .

Here is the rundown.

For Jupyter we add and delete secrets when deleting a secret that should be present the secret looks to be deleted, but the delete operation throws and we surface an error in our log.

The tricky bit here is that I'm not able to repro this issue in Code-OSS, only in Code-Insiders were I don't have source maps (they time out trying to load) and I'm not currently able to debug well into the depths of the keytar service. However I was able to catch the exception at the point it was seen and I was able to locate the location in the obfuscated code and look at that.

What I see is that we are failing on the first JSON.parse seen in the delete password code when looking to see if we are saved on a chunked password.

1. ce: Error: Unexpected token r in JSON at position 0
	1. name: "SyntaxError"
	2. message: "Unexpected token r in JSON at position 0"
	3. stack: Array(3)
		1. 0: "SyntaxError: Unexpected token r in JSON at position 0"
		2. 1: "    at JSON.parse (<anonymous>)"
		3. 2: "    at m.deletePassword (/Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/code/electron-main/main.js:31:9250)"
length: 3

let { content, hasNextChunk }: ChunkedPassword = JSON.parse(password);

If I look up at getPassword, I see that the check for chunks is wrapped in a try catch fully, and if it failed, then it just returns back the password. Given this, I'm putting up this tentative PR that makes the code in deletePassword that also looks for chunked passwords safe with a try catch. Makes sense to me that if getPassword considers an exception from JSON.parse ok, then this code in delete password would also be ok to eat the error.

My slight hesitation is that due to not being able to repro in Code - OSS or debug deeper into the service in shipping insiders I don't feel like I have a full root cause here. Given how the saving works I would expect that we would get something back that JSON.parse could operate on so it's not clear to me why it's throwing, or why it doesn't throw in Code - OSS. I do see the encrypted content that we are looking to save if that helps at all (this is encrypted value at the point we are passing to the cred service):

'{"extensionId":"ms-toolsai.jupyter","content":"http://localhost:8888\\nhttp://localhost:8888/?token=null"}'

Options as I see it.

  1. Take this PR, known issue that json.parse might fail (as seen in getPassword code)
  2. Someone could help me debug down a bit further and see if we can root cause the issue better
  3. I could just put in the info that I've uncovered + the repro and move to vscode to investigate more

Note, this issue seems mostly benign since the key is deleted. But the throw does skip out on the rest of the function, so the event for password changed would not fire, could lead to hard to diagnose issues where it's deleted but the change is not see in an extension.

Note: Since the chunking is Windows only I would say that I'm seeing this issue on MacOS.

rchiodo
rchiodo previously approved these changes May 25, 2022
@IanMatthewHuff
Copy link
Member Author

Per convo with @TylerLeonhardt I'm going to update the comment a bit with more info then submit this as a full PR. Will grab insiders in the morning to check if it's verified since it can't be verified on Code - OSS.

@IanMatthewHuff IanMatthewHuff marked this pull request as ready for review May 25, 2022 18:21
@vscodenpa vscodenpa added this to the May 2022 milestone May 25, 2022
@TylerLeonhardt TylerLeonhardt merged commit 280f1be into microsoft:main May 25, 2022
@IanMatthewHuff IanMatthewHuff deleted the dev/ianhu/deletePasswordIssues branch May 25, 2022 20:00
@IanMatthewHuff IanMatthewHuff changed the title quick draft PR for not throwing when looking for a chunked password Don't throw when trying to see if a stored password is a chunked password May 25, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jul 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error deleting secret from ExtensionContext.secrets on Windows

4 participants