-
Notifications
You must be signed in to change notification settings - Fork 50
CLI: Add Auth login command #1772
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
base: trunk
Are you sure you want to change the base?
Conversation
cli/commands/auth/login.ts
Outdated
openInBrowser( authUrl ); | ||
|
||
console.log( __( 'Note: This will open the Studio app to complete authentication.' ) ); | ||
console.log( __( "If the Studio app doesn't open automatically, please install it first." ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather not require the Studio App. Did you check the authentication command in my prototype, I have a solution for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out. I'll take a look at your branch to bring the logic over here.
I initially took this approach to begin drafting the PR.
…cli-v2-auth-login
…cli-v2-auth-login
📊 Performance Test ResultsComparing b681381 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
// Get the CLI command using the copied callback handler | ||
const cliCommand = getCLICommand( destCallbackHandler ); | ||
|
||
const escapedCliCommand = cliCommand.replace( /"/g, '\\"' ); |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix this problem, we should robustly escape both double quotes ("
) and backslashes (\
) in cliCommand
before embedding it in the AppleScript script. This is needed because a backslash followed by a quote or other special sequence would defeat the quote escaping logic if not itself escaped. The best way to achieve this is to first escape all backslashes (replace \
with \\
), and then escape all double quotes (replace "
with \"
), both globally. This can be accomplished with two chained .replace()
calls using global regular expressions. It would be clearer and more error-proof to factor this into a helper function, e.g. escapeForAppleScript
, but since only a single change can be made in the code block shown, we will directly perform the two-step replacement on line 307. No new imports are needed, and all changes will be confined to the assignment to escapedCliCommand
.
-
Copy modified line R307
@@ -304,7 +304,7 @@ | ||
// Get the CLI command using the copied callback handler | ||
const cliCommand = getCLICommand( destCallbackHandler ); | ||
|
||
const escapedCliCommand = cliCommand.replace( /"/g, '\\"' ); | ||
const escapedCliCommand = cliCommand.replace(/\\/g, '\\\\').replace(/"/g, '\\"'); | ||
|
||
const appleScript = `on open location this_URL | ||
set command to "${ escapedCliCommand } '" & this_URL & "'" |
Related issues
Proposed Changes
studio auth login
studio auth logout
studio auth status
Testing Instructions
npm run cli:build
~/Library/Application Support/Studio/appdata-v1.json
:Run
ENABLE_CLI_V2=true node dist/cli/main.js auth login
Click on approve in your browser
Observe you are logged in
Run
node dist/cli/main.js auth login
againObserve the browser doesn't open again because you are already logged in.
Run
ENABLE_CLI_V2=true node dist/cli/main.js auth status
Observe that your data is displayed in the CLI
Run
ENABLE_CLI_V2=true node dist/cli/main.js auth logout
Observe you are logged out
Pre-merge Checklist