Feat/oauth cli by nicornk · Pull Request #118 · emdgroup/foundry-dev-tools

nicornk · 2026-03-02T16:27:11Z

Summary

Checklist

You agree with our CLA
Included tests (or is not applicable).
Updated documentation (or is not applicable).
Used pre-commit hooks to format and lint the code.

Implements a cross-platform OAuth2 Authorization Code + PKCE CLI designed to work as Claude Code's apiKeyHelper. Supports browser-based and console login flows, concurrent token refresh with file locking, and platform-specific credential storage (macOS Keychain / Windows Credential Manager via keyring crate, JSON file on Linux). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- Rename directory libs/foundry-oauth-cli -> libs/oauth-cli - Rename Cargo package to foundry-dev-tools-oauth-cli - Rename binary to foundry-dev-tools-oauth - Remove all Claude Code and Anthropic references, make tool generic - Simplify bind_server to single port (no port scanning) - Fix clippy warnings (ServerError -> ServerCallback, needless borrows) - Apply cargo fmt formatting - Add Rust pre-commit hooks (fmt, cargo-check) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- Bump actions/checkout from v3 to v5 - Add Rust CI job with fmt, clippy, build, test, and cargo audit - Add CARGO_TERM_COLOR and --locked flags for reproducible builds - Move --no-browser from login-only to global flag so it works with the token command's auto-login flow Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Resolve the OAuth config directory using the same multi-location strategy as the Python foundry-dev-tools: check for config.toml in ~/.foundry-dev-tools/, ~/.config/foundry-dev-tools/, and the platform-native config path, then store OAuth data in an oauth/ subfolder. Defaults to ~/.foundry-dev-tools/oauth/ if no config.toml is found. Removes --cache-dir flag and FOUNDRY_CACHE_DIR env var; the config directory is now fully auto-resolved. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Update reqwest 0.12→0.13 (with native-tls and form features), rand 0.8→0.10, and dirs 5→6. Adapt code to rand 0.10 API changes (thread_rng→rng, gen_range→random_range, gen→random). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- server.rs: implement actual timeout via recv_timeout instead of ignoring _timeout_secs; restructure into CallbackServer with separate bind() and wait_for_callback() methods; extract respond_html helper to reduce duplication - cli.rs: fix race condition by binding server before opening browser; remove defunct start_server_and_open_browser function - oauth.rs: reuse HTTP client via OnceLock; use form_urlencoded Serializer instead of manual encoding; use Vec for form params instead of HashMap + clone-to-extend-lifetime hack; remove redundant Content-Type header; remove unnecessary Clone on Pkce; remove unused TokenResponse fields - error.rs: remove #[allow(dead_code)] and unused Error::Pkce variant - cache.rs: use explicit truncate(false) on lock file; remove unnecessary truncate(true) - config.rs: add #[serial] to all tests that read/mutate env vars - Cargo.toml: add serial_test dev-dependency - docs: add code signing research for macOS/Windows in GitHub Actions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…OU file permissions - Add hostname validation rejecting URL injection characters (slashes, colons, @, query strings) to prevent credential theft via crafted FOUNDRY_HOSTNAME values - Harden callback server to only accept GET requests to root path, ignoring favicon fetches, POST requests, and other spurious traffic by looping until a valid OAuth callback arrives or timeout expires - Fix TOCTOU race in file permissions: use OpenOptions::mode() and DirBuilder::mode() to set 0o600/0o700 atomically at creation time instead of creating with default umask then chmod after - Standardize env vars to FDT_CREDENTIALS__* namespace, removing inconsistent FOUNDRY_* variants Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Per RFC 6749 §3.2 and OAuth 2.0 Security BCP §4.11, token endpoint requests must not follow redirects to prevent leaking credentials (auth codes, client secrets, PKCE verifiers) to a redirect target. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…ad-data default scope Use std::env::current_exe() to display the full path to the binary in error messages so users (and tools like Claude Code) get a copy-pasteable login command. Also add "api:read-data" to default scopes alongside "offline_access" and extract both into a DEFAULT_SCOPES constant. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Remove dead code (no-op if block, unused scopes_from_flag binding), fix port default in help text (8888 → 9876), track --debug in explicit_cli_args, include explicit CLI args in LoginRequired error Display, print real client_secret value in CLI args, remove TOCTOU in ensure_config_dir, and document intentional stdout usage in try_auto_login. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

The debug log file was created with default permissions, which on systems with a permissive umask (e.g. 0o022) results in a world-readable file. Set mode 0o600 at creation time on Unix, consistent with the cache file and lock file handling in cache.rs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Adds a new Rust-based OAuth2 CLI (foundry-dev-tools-oauth) to support browser/console login flows, token refresh, and secure local caching, and wires it into repo tooling/CI.

Changes:

Introduces Rust OAuth CLI modules (config resolution, PKCE/state generation, local callback server, token exchange/refresh, caching, debug logging).
Adds a dedicated Rust CI job (fmt/clippy/build/test/audit) and Rust pre-commit hooks; updates .gitignore for Rust build outputs.
Adds a research doc covering macOS/Windows code-signing considerations for keychain/credential access.

Reviewed changes

Copilot reviewed 12 out of 14 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
libs/oauth-cli/src/main.rs	CLI entrypoint + subcommands wiring
libs/oauth-cli/src/cli.rs	Implements login/token/status/logout flows
libs/oauth-cli/src/config.rs	Merges CLI/env/defaults; builds Foundry OAuth URLs
libs/oauth-cli/src/oauth.rs	PKCE/state + auth URL + token exchange/refresh
libs/oauth-cli/src/server.rs	Local callback HTTP server + tests
libs/oauth-cli/src/cache.rs	Keyring/file token cache + locking + permission hardening
libs/oauth-cli/src/error.rs	Error types for config/oauth/server/cache
libs/oauth-cli/src/log.rs	Optional debug log file writer
libs/oauth-cli/Cargo.toml	New Rust crate manifest + dependencies
libs/oauth-cli/Cargo.lock	Locked dependency graph for the new crate
docs/dev/plans/oauth-cli-code-signing.md	Plan/research doc for signing/notarization
.pre-commit-config.yaml	Adds Rust fmt + cargo check hooks
.gitignore	Ignores Rust `target/` directory
.github/workflows/ci.yml	Adds Rust CI job; updates checkout action version

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-02T16:32:41Z

libs/oauth-cli/src/server.rs

+            // Only accept GET requests
+            if request.method() != &tiny_http::Method::Get {
+                respond_html(request, ERROR_HTML);
+                continue;
+            }
+
+            // Only accept requests to the root path (with query string)
+            let request_url = request.url();
+            if !request_url.starts_with("/?") && request_url != "/" {
+                respond_html(request, ERROR_HTML);
+                continue;


For unrelated requests (non-GET or wrong path), the server responds with an HTML page that says "Missing authorization code". This is misleading for cases like POST or /favicon.ico that are intentionally ignored. Consider responding with a minimal 404/405 (or a distinct message) for non-callback requests while continuing to wait for the real callback.

Copilot · 2026-03-02T16:32:42Z

libs/oauth-cli/src/cli.rs

+        Err(e) => return Err(e),
+    };
+
+    // Print access token to stdout (the ONLY thing that goes to stdout)


The comment says stdout will contain ONLY the access token, but try_auto_login intentionally prints login instructions to stdout in non-interactive scenarios. Please clarify the comment to reflect that stdout is token-only on success, but may contain guidance on failure.

Suggested change

// Print access token to stdout (the ONLY thing that goes to stdout)

// On success, print only the raw access token to stdout.

// Note: in some failure/non-interactive flows, try_auto_login may have already

// written login instructions or guidance to stdout before this point.

Copilot · 2026-03-02T16:32:42Z

.github/workflows/ci.yml

+      - uses: actions/checkout@v5
        with: {fetch-depth: 0}  # deep clone for git tag


This workflow updates actions/checkout to v5 here, but other workflows in the repo still use actions/checkout@v3. Aligning on a single major version across workflows reduces maintenance overhead and avoids subtle differences between CI pipelines.

Copilot · 2026-03-02T16:32:43Z

.github/workflows/ci.yml

+      - name: Run tests
+        run: cargo test --locked
+      - name: Audit dependencies
+        run: cargo install cargo-audit && cargo audit


cargo install cargo-audit in CI installs the latest version on every run, which is slow and can make CI behavior non-reproducible if a new cargo-audit release introduces changes. Pin the version and use --locked (or switch to a dedicated cargo-audit GitHub Action / cached install) to keep CI deterministic and faster.

Suggested change

run: cargo install cargo-audit && cargo audit

run: cargo install cargo-audit --version 0.20.0 --locked && cargo audit

Copilot · 2026-03-02T16:32:43Z

libs/oauth-cli/src/config.rs

+
+        let client_secret = flags
+            .client_secret
+            .inspect(|s| cli_args.push(format!("--client-secret {s}")))


explicit_cli_args includes --client-secret {s} when the secret is provided via CLI. This will leak the secret into user-facing login instructions (LoginRequired) and into debug logs/error strings. Omit --client-secret from explicit_cli_args (or replace with a constant like --client-secret <redacted>), and ensure no error path ever echoes the raw secret.

Suggested change

.inspect(|s| cli_args.push(format!("--client-secret {s}")))

.inspect(|_| cli_args.push("--client-secret <redacted>".to_string()))

Copilot · 2026-03-02T16:32:43Z

libs/oauth-cli/src/server.rs

+            let request = self
+                .server
+                .recv_timeout(remaining)
+                .map_err(|_| Error::ServerTimeout)?


recv_timeout errors are currently all converted into Error::ServerTimeout, which will misreport non-timeout I/O/server errors as timeouts and make debugging failures hard. Preserve the underlying error (e.g., distinguish real timeouts from other errors) and return a dedicated error variant containing the source error.

Suggested change

.map_err(|_| Error::ServerTimeout)?

.map_err(|e| match e {

std::sync::mpsc::RecvTimeoutError::Timeout => Error::ServerTimeout,

other => Error::ServerCallback(other.to_string()),

})?

Copilot · 2026-03-02T16:32:43Z

libs/oauth-cli/src/server.rs

+        let addr = format!("127.0.0.1:{}", port);
+        let server = tiny_http::Server::http(&addr).map_err(|e| Error::ServerBind {
+            addr,
+            source: std::io::Error::new(std::io::ErrorKind::AddrInUse, e.to_string()),


Server::http bind failures are always wrapped as AddrInUse, regardless of the actual cause (e.g., permission denied, address not available). This can produce misleading diagnostics. Preserve the original error kind (or store the original error directly) instead of forcing AddrInUse.

Suggested change

source: std::io::Error::new(std::io::ErrorKind::AddrInUse, e.to_string()),

source: e,

dstoeckel

These is my first set of comments. Will do another pass later / when updated.

dstoeckel · 2026-03-02T21:42:25Z

libs/oauth-cli/Cargo.toml

+path = "src/main.rs"
+
+[dependencies]
+clap = { version = "4", features = ["derive"] }


Clap is notorious for being large and slow to build. If you are not using advanced features try something simpler such as https://docs.rs/argh/0.1.14/argh/

Just as slow as this Github page :D replaced it with argh

dstoeckel · 2026-03-02T21:48:51Z

libs/oauth-cli/src/server.rs

+
+/// A bound callback server ready to accept the OAuth redirect.
+pub struct CallbackServer {
+    server: tiny_http::Server,


You are using tiny_http here, which is likely fine functionality wise. However, you already are depending on reqwest, which is bringing in hyper that also allows you to write a server.

We evaluated replacing tiny_http with hyper (which reqwest pulls in transitively). However, reqwest only depends on hyper as a client — hyper's server functionality requires adding explicit dependencies (hyper with the server and http1 features, plus hyper-util with tokio-server). This would also pull in a tokio runtime for what is a single synchronous blocking-wait-for-one-request. tiny_http gives us a simple synchronous recv_timeout API that fits this use case well without introducing async complexity. We're keeping it for now.

dstoeckel · 2026-03-02T22:18:13Z

libs/oauth-cli/src/cache.rs

The approach in this file works, but for idiomatic rust code, you would:

Make the cache / keyring access a trait

Implement that trait for the linux platform and for the mac/windows platform

Switch to the appropriate implementation using a cfg! switch

dstoeckel · 2026-03-02T22:24:05Z

libs/oauth-cli/src/log.rs

Consider using the log crate for this. It is a generic logger facade, the de-facto standard, and can have multiple backends (including very simple ones).

libs/oauth-cli/src/cache.rs

dstoeckel · 2026-03-02T23:42:46Z

libs/oauth-cli/src/oauth.rs

+    let verifier: String = (0..128)
+        .map(|_| {
+            let idx = rng.random_range(0..PKCE_CHARS.len());
+            PKCE_CHARS[idx] as char


Why cast to char here, when the next thing that is being done is to cast to bytes again?

dstoeckel · 2026-03-03T21:06:02Z

libs/oauth-cli/Cargo.toml

+tiny_http = "0.12"
+open = "5"
+dirs = "6"
+url = "2"


You are including url here, but you implement host parsing and validation on your own. Why not use https://docs.rs/url/2.5.8/url/enum.Host.html#impl-Host to perform the parsing and then checking the enum whether it is a valid domain name (and not an IP address)

dstoeckel · 2026-03-03T21:07:07Z

libs/oauth-cli/Cargo.toml

+dirs = "6"
+url = "2"
+chrono = { version = "0.4", features = ["serde"] }
+fd-lock = "4"


This dependency seems unused. Either use it and discard the homebrew locking code or remove it.

dstoeckel · 2026-03-03T21:07:58Z

libs/oauth-cli/Cargo.toml

+url = "2"
+chrono = { version = "0.4", features = ["serde"] }
+fd-lock = "4"
+keyring = { version = "3", features = ["apple-native", "windows-native"] }


This should be a platform dependent feature and pulled in during compile on the required platforms only (cfg check). See comment in the cache.rs implementation.

dstoeckel · 2026-03-03T21:08:56Z

libs/oauth-cli/Cargo.toml

+[package]
+name = "foundry-dev-tools-oauth-cli"
+version = "0.1.0"
+edition = "2021"


Why not the 2024 edition? This is out for more than a year now.

nicornk and others added 11 commits February 25, 2026 14:37

nicornk requested review from Copilot and dstoeckel March 2, 2026 16:27

Copilot started reviewing on behalf of nicornk March 2, 2026 16:29 View session

Copilot AI reviewed Mar 2, 2026

View reviewed changes

dstoeckel reviewed Mar 5, 2026

View reviewed changes

-    // Print access token to stdout (the ONLY thing that goes to stdout)
+    // On success, print only the raw access token to stdout.
+    // Note: in some failure/non-interactive flows, try_auto_login may have already
+    // written login instructions or guidance to stdout before this point.

		- uses: actions/checkout@v5
		with: {fetch-depth: 0} # deep clone for git tag

	run: cargo install cargo-audit && cargo audit
	run: cargo install cargo-audit --version 0.20.0 --locked && cargo audit

	.inspect(\|s\| cli_args.push(format!("--client-secret {s}")))
	.inspect(\|_\| cli_args.push("--client-secret <redacted>".to_string()))

-                .map_err(|_| Error::ServerTimeout)?
+                .map_err(|e| match e {
+                    std::sync::mpsc::RecvTimeoutError::Timeout => Error::ServerTimeout,
+                    other => Error::ServerCallback(other.to_string()),
+                })?

	source: std::io::Error::new(std::io::ErrorKind::AddrInUse, e.to_string()),
	source: e,

Conversation

nicornk commented Mar 2, 2026

Summary

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

dstoeckel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants