Skip to content

Conversation

@HeshamHM28
Copy link
Contributor

@HeshamHM28 HeshamHM28 commented Nov 17, 2025

Fixes cf-841

Screen.Recording.2025-11-18.at.12.05.33.AM.mov

PR Type

Enhancement


Description

  • Add OAuth PKCE login flow

  • CLI prompt supports login or API key

  • Save OAuth token as API key

  • Improve auth success/error UX


Diagram Walkthrough

flowchart LR
  CLI["CLI init: prompt_api_key"] -- "Choose Login" --> OAuth["perform_oauth_signin()"]
  CLI -- "Choose API key" --> Manual["enter_api_key_and_save_to_rc()"]
  OAuth -- "Start local server" --> Server["HTTP /callback + /status"]
  OAuth -- "Open browser" --> AuthPage["CodeFlash OAuth page"]
  AuthPage -- "Redirect with code/state" --> Server
  Server -- "Provide status" --> Browser["Loading/Success/Error UI"]
  OAuth -- "Exchange code for token" --> Token["API key (access_token)"]
  Token -- "Save to RC + env" --> CLI
Loading

File Walkthrough

Relevant files
Enhancement
cmd_init.py
CLI prompt adds OAuth login path                                                 

codeflash/cli_cmds/cmd_init.py

  • Add OAuth signin option to API key prompt
  • Integrate perform_oauth_signin and save token
  • Preserve existing manual API key flow
  • Telemetry events for both auth paths
+54/-3   
oauth_handler.py
New OAuth handler with PKCE and local HTTP                             

codeflash/code_utils/oauth_handler.py

  • Implement OAuth PKCE flow with local server
  • Serve callback and status endpoints with HTML UX
  • Exchange auth code for API token via cfwebapp
  • Expose perform_oauth_signin helper for CLI
+744/-0 

@github-actions github-actions bot changed the title [Feat] CLI Login [Feat] CLI Login Nov 17, 2025
@github-actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 Security concerns

Open redirect/callback exposure:
The local server’s /status endpoint sets "Access-Control-Allow-Origin: *" which allows any web page to poll local auth status. While data is minimal, it could leak success state. Consider restricting CORS or removing the header.

⚡ Recommended focus areas for review

Typo/UX

The auth choice label says "Login in with Codeflash" which seems grammatically off; consider "Log in with CodeFlash" or "Sign in with CodeFlash" to avoid user confusion.

auth_choices = [
    "🔐 Login in with Codeflash",
    "🔑 Use Codeflash API key"
]

questions = [
Port Binding

Local HTTP server binds to "localhost" only; some systems/browsers may resolve to IPv6 (::1) causing callback issues. Consider binding to 127.0.0.1 explicitly and ensuring the redirect URI matches, or support dual-stack.

handler_class = self.create_callback_handler()
httpd = http.server.HTTPServer(("localhost", port), handler_class)

def serve_forever_wrapper():
    httpd.serve_forever()
Error Messaging

Token exchange sets a generic "Unauthorized" for most failures, discarding server-provided details. Preserve and surface specific error messages to improve debugging and UX, and reflect them via /status for the browser UI.

except requests.exceptions.HTTPError as e:
    error_msg = f"HTTP {e.response.status_code}"
    try:
        error_data = e.response.json()
        error_msg = error_data.get("error_description", error_data.get("error", error_msg))
    except:
        pass
    self.token_error = "Unauthorized"
    click.echo(f"❌ {self.token_error}")
    return None
except Exception as e:
    self.token_error = "Unauthorized"
    click.echo(f"❌ {self.token_error}")
    return None

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Security
Bind port to localhost only

Bind explicitly to localhost to avoid selecting an interface that's not loopback.
This prevents the auth server from being unreachable when the browser is redirected
to 127.0.0.1 while the server listens on 0.0.0.0 or another interface. It also
reduces surface area by not exposing the callback server on all interfaces.

codeflash/code_utils/oauth_handler.py [591-595]

 def get_free_port() -> int:
-    """Find an available port"""
+    """Find an available port bound to localhost"""
     with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
-        s.bind(("", 0))
+        s.bind(("127.0.0.1", 0))
         return s.getsockname()[1]
Suggestion importance[1-10]: 7

__

Why: Correctly narrows the bind to loopback which improves security and aligns with the redirect to localhost; a modest but meaningful improvement with low risk.

Medium
Possible issue
Use loopback and reuse address

Use the same explicit loopback address consistently and enable socket reuse to avoid
race conditions and "address already in use" errors on quick retries. This improves
reliability of starting the local callback server across platforms.

codeflash/code_utils/oauth_handler.py [608-620]

 def start_local_server(self, port: int) -> http.server.HTTPServer:
     """Start local HTTP server for OAuth callback"""
     handler_class = self.create_callback_handler()
-    httpd = http.server.HTTPServer(("localhost", port), handler_class)
+    http.server.HTTPServer.allow_reuse_address = True
+    httpd = http.server.HTTPServer(("127.0.0.1", port), handler_class)
 
     def serve_forever_wrapper():
         httpd.serve_forever()
 
-    server_thread = threading.Thread(target=serve_forever_wrapper)
+    server_thread = threading.Thread(target=serve_forever_wrapper, name="cf-oauth-http")
     server_thread.daemon = True
     server_thread.start()
 
     return httpd
Suggestion importance[1-10]: 6

__

Why: Consistently using 127.0.0.1 and enabling address reuse can reduce bind errors and exposure; helpful but not critical.

Low
General
Surface OAuth error details

Include the actual OAuth error details in the CLI output to aid debugging and user
remediation. Currently the message hides oauth.error, leaving users without
actionable information.

codeflash/code_utils/oauth_handler.py [721-724]

 if oauth.error:
     httpd.shutdown()
-    click.echo(f"❌ Authentication failed:")
+    click.echo(f"❌ Authentication failed: {oauth.error}")
     return None
Suggestion importance[1-10]: 6

__

Why: Including oauth.error in the message improves diagnosability with minimal change and no downside, but is not critical to functionality.

Low

@aseembits93
Copy link
Contributor

screenshots if possible @HeshamHM28

return self._get_error_html("unauthorized", theme)

@staticmethod
def _get_loading_html(theme: str = "light") -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we have html in the cli?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is for the local server used on local machines.

# Setup PKCE
port = oauth.get_free_port()
redirect_uri = f"http://localhost:{port}/callback"
code_verifier, code_challenge = oauth.generate_pkce_pair()
Copy link
Contributor

Choose a reason for hiding this comment

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

is generating this in the cli side where the user can modify this generation logic a security vulnerability? what is user has full control of the generation string, would that degrade the security somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is safe because we ensure the user is authenticated in cf-webapp before exchanging the code. Even if the user overrides this value, they will still receive their own API key.

def start_local_server(self, port: int) -> http.server.HTTPServer:
"""Start local HTTP server for OAuth callback."""
handler_class = self.create_callback_handler()
httpd = http.server.HTTPServer(("localhost", port), handler_class)
Copy link
Contributor

Choose a reason for hiding this comment

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

wait, we are doing local server way of authentication? this won't work in remote machines.
why are we not doing polling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented remote-machine logic in the last commit.

Co-authored-by: codeflash-ai[bot] <148906541+codeflash-ai[bot]@users.noreply.github.com>
@codeflash-ai
Copy link
Contributor

codeflash-ai bot commented Nov 18, 2025

This PR is now faster! 🚀 HeshamHM28 accepted my code suggestion above.

@codeflash-ai
Copy link
Contributor

codeflash-ai bot commented Nov 20, 2025

⚡️ Codeflash found optimizations for this PR

📄 11% (0.11x) speedup for should_attempt_browser_launch in codeflash/code_utils/oauth_handler.py

⏱️ Runtime : 248 microseconds 224 microseconds (best of 63 runs)

A dependent PR with the suggested changes has been created. Please review:

If you approve, it will be merged into this PR (branch feat/cli/login).

Static Badge

Copy link
Contributor

@aseembits93 aseembits93 left a comment

Choose a reason for hiding this comment

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

waiting for #931 to be merged @KRRT7 @HeshamHM28

@misrasaurabh1
Copy link
Contributor

why do we want TUI to be merged for this? we should merge this first

@HeshamHM28
Copy link
Contributor Author

why do we want TUI to be merged for this? we should merge this first

Because this change will require additional updates to TUI, so @KRRT7 suggested merging it after TUI.

@misrasaurabh1
Copy link
Contributor

merge this first for the CLI init. we still need to experiment more with TUI to make a decision to see if it is going to be better than the cli init for the users.

@KRRT7
Copy link
Contributor

KRRT7 commented Nov 21, 2025

let's merge it in now, the TUI flow currently requires a bit more testing before merging it in

@aseembits93 aseembits93 self-requested a review November 21, 2025 20:31
@Saga4 Saga4 disabled auto-merge November 21, 2025 21:18
@Saga4 Saga4 merged commit fd2d2b8 into main Nov 21, 2025
22 checks passed
@Saga4 Saga4 deleted the feat/cli/login branch November 21, 2025 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants