Skip to content

fix(cli): replace $SESSION_TOKEN placeholder for external apps #17048

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

Merged
merged 2 commits into from
Mar 24, 2025

Conversation

johnstcn
Copy link
Member

@johnstcn johnstcn commented Mar 21, 2025

Fixes an oversight in #17032

The FE has logic to replace the string $SESSION_TOKEN with a newly-minted session token.
This adds corresponding logic to the coder open app command.

@johnstcn johnstcn self-assigned this Mar 21, 2025
@johnstcn johnstcn requested review from mafredri and matifali March 21, 2025 18:46
// strings in the URL with the actual session token.
// This is consistent behaviour with the frontend. See: site/src/modules/resources/AppLink/AppLink.tsx
func replacePlaceholderExternalSessionTokenString(client *codersdk.Client, appURL string) string {
if !strings.Contains(appURL, "$SESSION_TOKEN") {
Copy link
Member

Choose a reason for hiding this comment

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

Is this the only supported format? Not e.g. ${SESSION_TOKEN}?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, it's just const magicTokenString = "$SESSION_TOKEN";

return appURL
}

// We will just re-use the existing session token we're already using.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine for apps. For VS Code I think we generate a new one but since it’s long-lived in the editor, it makes sense to decouple them. Same doesn’t apply here.

Copy link
Member

@matifali matifali left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks 👍

@johnstcn johnstcn merged commit 674f60f into main Mar 24, 2025
32 checks passed
@johnstcn johnstcn deleted the cj/cli-open-external-session-token branch March 24, 2025 09:03
@github-actions github-actions bot locked and limited conversation to collaborators Mar 24, 2025
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.

3 participants