Repository navigation
Fix workspace link trust and credential isolation - #385
Conversation
Two security weaknesses addressed in the VS Code extension also affect Toolbox, as identified during a review of those fixes. The Toolbox URI handler accepts deployment URLs from workspace links without a trust prompt, while the secret store retrieves API tokens by hostname alone. A crafted link can start a connection to an untrusted deployment, and a different scheme or port on the same host can reuse a saved token. With this change, the URI handler requests confirmation for a different deployment or a fresh connection. Links to the active origin retain the existing deployment URL and continue without a prompt. Cancellation preserves the current session, and URI logging omits tokens. The secret store now keeps API tokens under separate keys for each normalized scheme, host, and port. Logout clears only the matching origin. Legacy hostname-only entries remain untouched and are ignored, so existing API-token users sign in again. The remote provider also prevents delayed OAuth refreshes from updating a CLI session for another origin. Regression tests cover trust decisions, cancellation, URL normalization, separate token storage, and OAuth refreshes. The changelog records the security fixes and the sign-in requirement.
jeremyruppel
left a comment
jeremyruppel
left a comment
There was a problem hiding this comment.
The two security fixes look correct and I didn't find anything blocking. Links to a different origin now prompt before connecting, the prompt shows the normalized origin (no userinfo, punycode host), cancelling keeps the current session, API tokens are keyed by origin, and a late OAuth refresh can no longer log in the CLI for another origin.
Checked locally on JDK 21: ./gradlew test passed except testDownloadRealCLI, which needs network access to dev.coder.com and isn't related to this PR. detekt is clean.
One open question: I couldn't confirm the trust-prompt behavior matches the VS Code extension, which matters most for the cold-start comment below.
Generated by Coder Agents on behalf of @jeremyruppel.
| val activeSession = if (context.settingsStore.requiresTokenAuth) { | ||
| newToken?.let { | ||
| refreshSession(newUrl, it) | ||
| refreshSession(activeUrl, it) |
There was a problem hiding this comment.
Low (not new in this PR): a link to the connected origin that carries a token still replaces the session with that token, with no prompt. A crafted link could quietly sign the user in to another account on the same deployment. This may be out of scope here, but since this PR is about link trust, it's worth deciding on deliberately: either prompt when the link token differs from the current one, or document why it's acceptable.
There was a problem hiding this comment.
Addressed. We now check which user owns the link token and compares that user’s ID with the active user’s ID. A different ID triggers a prompt naming both accounts
| } | ||
|
|
||
| @Test | ||
| fun `HTTP uses the keyring across supported CLI versions`() = runBlocking { |
There was a problem hiding this comment.
Nit (scope): these keyring tests (HTTP/HTTPS, CLI versions, platform defaults) and the credentialCli() → credentialSettings() refactor don't seem related to origin checks or token keys. Also, HTTP uses the keyring… and HTTPS uses the keyring… are almost identical and could be one test that loops over the scheme. Consider moving them to a separate PR so this one stays a focused security fix.
There was a problem hiding this comment.
You’re definitely right, but over time I’ve found that, instead of asserts like assertEquals(expectedUrl, actualUrl), it’s better to be explicit, even at the cost of some repetition. I followed this principle in the new tests and also updated the existing tests in the file for consistency.
The changelog mixes the security fixes with unrelated changes to existing Unreleased entries and an added format introduction, which expands the scope of the security review. This commit restores the original headings, wording, and formatting and removes the introduction. The Security section remains intact so the broader changelog cleanup can be reviewed separately.
The CLI manager tests duplicate HTTP and HTTPS keyring checks and loop over CLI versions inside each test. A failure stops the remaining versions in that loop, and the results are reported as only two tests. This commit replaces the duplicated checks with six named JUnit cases. Each URL and version pair runs independently, while the test data keeps explicit URL literals. The build includes the parameterized test module aligned with the existing JUnit version.
The Toolbox link handler only recognizes the active deployment, so it asks users to trust a familiar deployment again after a restart. A link to the active deployment can also replace the session without checking which account owns the new token. The secrets store now remembers successful sign-ins. The link handler selects the deployment by scheme, host, and port and compares the account behind the link token with the active or saved account. Toolbox asks for confirmation when the deployment is unknown, the accounts differ, or it cannot verify the saved account.
Older Toolbox versions store API tokens under the deployment hostname. The secret store now uses the deployment origin, so the old token stays in the keychain after sign-in or logout. The secret store now clears the hostname entry after it saves a replacement token or when the user signs out. A failed write preserves the old credential, and cleanup preserves the last deployment URL when its key matches the hostname.
The Toolbox secret store keys OAuth credentials by deployment URL. A change in the path or trailing slash can leave the same deployment with separate sessions. The secret store now uses the scheme, host, and effective port as the key. On first use, it moves credentials found under a legacy URL to the origin key. If that write fails, the legacy session remains available. Logout clears the origin key and the legacy keys for the supplied URL and root spelling.
Two security weaknesses addressed in the VS Code extension also affect Toolbox, as identified during a review of those fixes. The Toolbox URI handler accepts deployment URLs from workspace links without a trust prompt, while the secret store retrieves API tokens by hostname alone. A crafted link can start a connection to an untrusted deployment, and a different scheme or port on the same host can reuse a saved token.
With this change, the URI handler requests confirmation for a different deployment or a fresh connection. Links to the active origin retain the existing deployment URL and continue without a prompt. Cancellation preserves the current session, and URI logging omits tokens.
The secret store now keeps API tokens under separate keys for each normalized scheme, host, and port. Logout clears only the matching origin. Legacy hostname-only entries remain untouched and are ignored, so existing API-token users sign in again. The remote provider also prevents delayed OAuth refreshes from updating a CLI session for another origin.
Regression tests cover trust decisions, cancellation, URL normalization, separate token storage, and OAuth refreshes. The changelog records the security fixes and the sign-in requirement.