feat(login): authenticate with a control-plane PAT via --facets - #64
Open
SatyendraGollaFacets wants to merge 2 commits into
Open
feat(login): authenticate with a control-plane PAT via --facets#64SatyendraGollaFacets wants to merge 2 commits into
SatyendraGollaFacets wants to merge 2 commits into
Conversation
Contributor
Author
|
Server side: Facets-cloud/agent-factory#1590 (CLI gateway accepts the Basic control-plane PAT). Merge that first / together. |
WalkthroughThe CLI adds facets PAT login and profile-specific authentication headers. API clients and commands now pass complete authorization headers instead of raw tokens. Credentials persistence, login setup, MCP, duty, memory, catalog, status, and profile verification paths are updated with tests. ChangesAuthentication and API propagation
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginCommand
participant CredentialsStore
participant AuthAPI
participant PostAuthSetup
User->>LoginCommand: invoke facets login
LoginCommand->>CredentialsStore: read facets profile
CredentialsStore-->>LoginCommand: URL, username, PAT
LoginCommand->>AuthAPI: verify authentication headers
AuthAPI-->>LoginCommand: user and canonical URL
LoginCommand->>CredentialsStore: persist profile and auth mode
LoginCommand->>PostAuthSetup: initialize catalogs and MCP snapshot
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
cmd/login.go (2)
433-440: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse the
osExitseam (with an explicit early return) instead of rawos.Exit.This new fatal-exit path uses
os.Exitdirectly rather than the testableosExitvar already used for the equivalent case intryReuseStoredToken(Line 229). As written it also relies onos.Exitnever returning — swapping toosExitwithout adding areturnwould fall through touser.canonicalBaseURLwithuser == niland panic.♻️ Proposed fix
user, err := fetchAuthMe(baseURL, prof.AuthHeader()) if err != nil { render.PrintError(out, asJSON, fmt.Sprintf("control-plane PAT validation failed: %v", err), "the PAT may be invalid/expired, or the --url isn't a facets-mode agent server", exitcode.Auth) - os.Exit(exitcode.Auth) + osExit(exitcode.Auth) + return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/login.go` around lines 433 - 440, Update the fatal error branch in the authentication flow around fetchAuthMe to call the existing osExit seam instead of os.Exit, then add an explicit early return after it so execution cannot continue to user.canonicalBaseURL when validation fails.
62-76: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
--facetsshould conflict with--token.
--facetsreturns early, sopraxis login --facets --token Xignores the token. Add a mutual-exclusion rule for the two flags.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/login.go` around lines 62 - 76, Update the login command’s flag validation around loginFacets and loginToken so using --facets together with --token is rejected as a mutually exclusive combination before the --facets early-return path runs. Preserve the existing behavior of each flag when used independently.internal/credentials/facets_test.go (1)
22-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse table-driven profile cases.
These tests repeat the same fixture/call/assert structure and differ only in input and expected outcome. As per coding guidelines, “Use table-driven tests as the default pattern.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/credentials/facets_test.go` around lines 22 - 76, Refactor the ReadFacetsProfile tests into a single table-driven test covering default, named, missing, empty-credentials, and missing-file cases. Define per-case fixture/setup, profile name, expected values, and expected error state, then run each case with subtests while preserving the existing assertions and HOME isolation behavior.Source: Coding guidelines
internal/duties/duties_test.go (1)
69-69: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a Basic-auth regression case.
All updated tests still exercise only
"Bearer tok", so the new facets path can regress without detection. Make the helper compare the supplied header verbatim and add a table-driven"Basic ..."case for representative duty calls.As per coding guidelines, internal package tests must cover the package's exported API and main failure paths, and table-driven tests are the default pattern.
Also applies to: 90-90, 118-118, 132-132, 157-157, 174-174, 188-188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/duties/duties_test.go` at line 69, Update the request-header assertions in the duty test helper around ListSchedules and the other representative duty calls to compare the supplied authorization header verbatim instead of assuming Bearer authentication. Convert the relevant tests to a table-driven form and add a Basic authentication case alongside the existing Bearer case, covering the exported duty-call paths and preserving their current failure assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/agentcatalog/agentcatalog.go`:
- Around line 101-103: Update the empty-auth validation errors to consistently
report “auth is required” instead of “token is required” in the validation at
internal/agentcatalog/agentcatalog.go lines 101-103, duties.FetchArtifactContent
at internal/duties/duties.go lines 202-208, and duties.doJSON at
internal/duties/duties.go lines 252-259.
- Around line 123-129: Update fetchOne to accept a context.Context parameter and
construct the request with http.NewRequestWithContext instead of
http.NewRequest. Propagate the caller’s context through every fetchOne call site
so request cancellation is controlled by the caller while preserving the
existing request behavior.
In `@internal/credentials/facets_test.go`:
- Around line 57-75: Strengthen the failure-path assertions in
TestReadFacetsProfile_MissingProfile, TestReadFacetsProfile_EmptyCredentials,
and TestReadFacetsProfile_MissingFile by verifying each returned error’s
expected contents, not merely that it is non-nil. Use the existing error
messages or sentinel/type conventions from ReadFacetsProfile so unrelated errors
cannot satisfy these tests.
- Around line 11-12: Update both test fixtures that set HOME to also set
USERPROFILE to the same t.TempDir() value, ensuring os.UserHomeDir() resolves
the temporary fixture on Windows as well.
---
Nitpick comments:
In `@cmd/login.go`:
- Around line 433-440: Update the fatal error branch in the authentication flow
around fetchAuthMe to call the existing osExit seam instead of os.Exit, then add
an explicit early return after it so execution cannot continue to
user.canonicalBaseURL when validation fails.
- Around line 62-76: Update the login command’s flag validation around
loginFacets and loginToken so using --facets together with --token is rejected
as a mutually exclusive combination before the --facets early-return path runs.
Preserve the existing behavior of each flag when used independently.
In `@internal/credentials/facets_test.go`:
- Around line 22-76: Refactor the ReadFacetsProfile tests into a single
table-driven test covering default, named, missing, empty-credentials, and
missing-file cases. Define per-case fixture/setup, profile name, expected
values, and expected error state, then run each case with subtests while
preserving the existing assertions and HOME isolation behavior.
In `@internal/duties/duties_test.go`:
- Line 69: Update the request-header assertions in the duty test helper around
ListSchedules and the other representative duty calls to compare the supplied
authorization header verbatim instead of assuming Bearer authentication. Convert
the relevant tests to a table-driven form and add a Basic authentication case
alongside the existing Bearer case, covering the exported duty-call paths and
preserving their current failure assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b6f0042-c23c-4b5a-af1e-3bfbb5b69972
📒 Files selected for processing (25)
cmd/duty.gocmd/duty_test.gocmd/login.gocmd/login_reuse_test.gocmd/login_setup.gocmd/mcp.gocmd/mcp_test.gocmd/memory.gocmd/memory_test.gocmd/profiles.gocmd/skill.gocmd/status.gointernal/agentcatalog/agentcatalog.gointernal/credentials/credentials.gointernal/credentials/credentials_test.gointernal/credentials/facets.gointernal/credentials/facets_test.gointernal/duties/duties.gointernal/duties/duties_test.gointernal/mcpmanifest/mcpmanifest.gointernal/mcpmanifest/mcpmanifest_test.gointernal/memory/client.gointernal/memory/client_test.gointernal/skillcatalog/skillcatalog.gointernal/skillcatalog/skillcatalog_test.go
Comment on lines
101
to
103
| if auth == "" { | ||
| return nil, fmt.Errorf("token is required") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align empty-auth errors with the authorization-header contract.
All three functions validate auth but report token is required, which is misleading for Basic authentication.
internal/agentcatalog/agentcatalog.go#L101-L103: returnauth is required.internal/duties/duties.go#L202-L208: updateFetchArtifactContentto the same message.internal/duties/duties.go#L252-L259: updatedoJSONto the same message.
📍 Affects 2 files
internal/agentcatalog/agentcatalog.go#L101-L103(this comment)internal/duties/duties.go#L202-L208internal/duties/duties.go#L252-L259
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/agentcatalog/agentcatalog.go` around lines 101 - 103, Update the
empty-auth validation errors to consistently report “auth is required” instead
of “token is required” in the validation at
internal/agentcatalog/agentcatalog.go lines 101-103, duties.FetchArtifactContent
at internal/duties/duties.go lines 202-208, and duties.doJSON at
internal/duties/duties.go lines 252-259.
Comment on lines
+123
to
+129
| func fetchOne(baseURL, auth, path, kind string) ([]Agent, error) { | ||
| url := strings.TrimRight(baseURL, "/") + path | ||
| req, err := http.NewRequest("GET", url, nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| req.Header.Set("Authorization", "Bearer "+token) | ||
| req.Header.Set("Authorization", auth) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## File outline\n'
ast-grep outline internal/agentcatalog/agentcatalog.go --view expanded || true
printf '\n## Relevant slice\n'
sed -n '100,170p' internal/agentcatalog/agentcatalog.go
printf '\n## fetchOne usages\n'
rg -n "fetchOne\(" -S .
printf '\n## context usage in package\n'
rg -n "NewRequestWithContext|context\.WithTimeout|context\.Background|noctx" internal/agentcatalog -SRepository: Facets-cloud/praxis-cli
Length of output: 3511
Thread a context into fetchOne. http.NewRequest still triggers noctx; pass a context.Context through and use http.NewRequestWithContext so the request can be canceled by the caller. The client timeout alone does not provide caller cancellation.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 125-125: net/http.NewRequest must not be called. use net/http.NewRequestWithContext
(noctx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/agentcatalog/agentcatalog.go` around lines 123 - 129, Update
fetchOne to accept a context.Context parameter and construct the request with
http.NewRequestWithContext instead of http.NewRequest. Propagate the caller’s
context through every fetchOne call site so request cancellation is controlled
by the caller while preserving the existing request behavior.
Source: Linters/SAST tools
Comment on lines
+11
to
+12
| home := t.TempDir() | ||
| t.Setenv("HOME", home) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== facets_test.go ==\n'
sed -n '1,140p' internal/credentials/facets_test.go
printf '\n== search for UserHomeDir usage ==\n'
rg -n "UserHomeDir|HOME|USERPROFILE|HOME_DIR" internal cmd . -g'*.go'
printf '\n== os.UserHomeDir docs from local go env version ==\n'
go doc os.UserHomeDir 2>/dev/null || trueRepository: Facets-cloud/praxis-cli
Length of output: 19007
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os, sys, platform
print("platform:", platform.system())
print("HOME:", os.environ.get("HOME"))
print("USERPROFILE:", os.environ.get("USERPROFILE"))
PYRepository: Facets-cloud/praxis-cli
Length of output: 216
🏁 Script executed:
#!/bin/bash
set -euo pipefail
go env GOROOT
printf '\n== os.UserHomeDir source ==\n'
sed -n '1,220p' "$(go env GOROOT)/src/os/file.go" | rg -n -A20 -B5 "func UserHomeDir|USERPROFILE|HOME|home"
printf '\n== facets implementation ==\n'
sed -n '1,120p' internal/credentials/facets.go
printf '\n== credentials tests using home helpers ==\n'
sed -n '1,120p' internal/credentials/credentials_test.goRepository: Facets-cloud/praxis-cli
Length of output: 204
Set USERPROFILE alongside HOME in both fixtures.
os.UserHomeDir() reads USERPROFILE on Windows, so these tests can still resolve a real home directory unless it points at the temp fixture too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/credentials/facets_test.go` around lines 11 - 12, Update both test
fixtures that set HOME to also set USERPROFILE to the same t.TempDir() value,
ensuring os.UserHomeDir() resolves the temporary fixture on Windows as well.
Comment on lines
+57
to
+75
| func TestReadFacetsProfile_MissingProfile(t *testing.T) { | ||
| writeFacetsCreds(t, "[default]\nusername = u\ntoken = t\n") | ||
| if _, _, _, err := ReadFacetsProfile("nope"); err == nil { | ||
| t.Error("want error for missing profile") | ||
| } | ||
| } | ||
|
|
||
| func TestReadFacetsProfile_EmptyCredentials(t *testing.T) { | ||
| writeFacetsCreds(t, "[default]\ncontrol_plane_url = https://x\n") | ||
| if _, _, _, err := ReadFacetsProfile("default"); err == nil { | ||
| t.Error("want error when username/token empty") | ||
| } | ||
| } | ||
|
|
||
| func TestReadFacetsProfile_MissingFile(t *testing.T) { | ||
| t.Setenv("HOME", t.TempDir()) | ||
| if _, _, _, err := ReadFacetsProfile("default"); err == nil { | ||
| t.Error("want error when file absent") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert expected error contents.
The failure-path tests accept any error, so an unrelated regression passes. Assert the expected error text for missing profiles, empty credentials, and missing files. As per coding guidelines, “Error paths matter: assert on the type/contents of returned errors, not just err != nil.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/credentials/facets_test.go` around lines 57 - 75, Strengthen the
failure-path assertions in TestReadFacetsProfile_MissingProfile,
TestReadFacetsProfile_EmptyCredentials, and TestReadFacetsProfile_MissingFile by
verifying each returned error’s expected contents, not merely that it is
non-nil. Use the existing error messages or sentinel/type conventions from
ReadFacetsProfile so unrelated errors cannot satisfy these tests.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/login_setup_test.go (1)
26-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert Facets headers through the post-login seams.
Capture and compare an auth map containing both
AuthorizationandX-Facets-UsernameinfetchCatalog,fetchAgents, andmcpmanifest.Fetch. The current stubs only validate the new parameter type, so a dropped identity header would pass.Also applies to: 61-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/login_setup_test.go` around lines 26 - 32, Update the test stubs for fetchCatalog, fetchAgents, and mcpmanifest.Fetch to capture the provided auth map and assert it contains both Authorization and X-Facets-Username. Keep the existing successful stub responses, but fail the test when either identity header is missing or has an unexpected value, ensuring post-login calls propagate both headers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/agentcatalog/agentcatalog.go`:
- Around line 129-131: Apply a shared http.Client redirect policy that removes
X-Facets-Username whenever a redirect crosses hosts, while preserving it for
same-origin redirects and retaining normal redirect behavior. Update the request
paths at internal/agentcatalog/agentcatalog.go:129-131,
internal/duties/duties.go:222-224, and internal/duties/duties.go:272-274 to use
this policy, and add httptest cross-host redirect regression coverage for all
three sites.
In `@internal/duties/duties.go`:
- Around line 10-12: Update the transport-call comment to describe
caller-supplied authorization headers generically, supporting both Basic and
Bearer authentication rather than stating Authorization is always Bearer.
Clarify that X-Facets-Username is optional and applies when needed for
facets-mode PAT authentication.
In `@internal/mcpmanifest/mcpmanifest_test.go`:
- Line 30: Add independent Facets-header regression coverage at all listed
sites: in internal/mcpmanifest/mcpmanifest_test.go:30, test Fetch with both
headers and assert receipt; in internal/memory/client_test.go:59, add a Facets
auth-map case for doJSON; in internal/skillcatalog/skillcatalog_test.go:33,
assert Fetch forwards X-Facets-Username; in
internal/igcatalog/igcatalog_test.go:323 and :378, assert
PublishMember/sendBytes and DownloadBundle forward both headers; and in
cmd/mcp_test.go:202, use Facets auth for the same-origin redirect and verify the
identity header is preserved.
---
Nitpick comments:
In `@cmd/login_setup_test.go`:
- Around line 26-32: Update the test stubs for fetchCatalog, fetchAgents, and
mcpmanifest.Fetch to capture the provided auth map and assert it contains both
Authorization and X-Facets-Username. Keep the existing successful stub
responses, but fail the test when either identity header is missing or has an
unexpected value, ensuring post-login calls propagate both headers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f82c930-79ab-4f6b-a0f1-22ecc57bf860
📒 Files selected for processing (37)
cmd/duty.gocmd/duty_test.gocmd/git_credential.gocmd/git_credential_test.gocmd/ig.gocmd/ig_hook.gocmd/ig_test.gocmd/login.gocmd/login_canonical_test.gocmd/login_local_test.gocmd/login_reuse_test.gocmd/login_setup.gocmd/login_setup_test.gocmd/mcp.gocmd/mcp_test.gocmd/memory.gocmd/memory_test.gocmd/profiles.gocmd/profiles_test.gocmd/skill.gocmd/skill_test.gocmd/status.gocmd/status_test.gointernal/agentcatalog/agentcatalog.gointernal/agentcatalog/agentcatalog_test.gointernal/credentials/credentials.gointernal/credentials/credentials_test.gointernal/duties/duties.gointernal/duties/duties_test.gointernal/igcatalog/igcatalog.gointernal/igcatalog/igcatalog_test.gointernal/mcpmanifest/mcpmanifest.gointernal/mcpmanifest/mcpmanifest_test.gointernal/memory/client.gointernal/memory/client_test.gointernal/skillcatalog/skillcatalog.gointernal/skillcatalog/skillcatalog_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/status.go
- cmd/profiles.go
- cmd/memory.go
- internal/duties/duties_test.go
- cmd/login.go
- cmd/duty.go
Comment on lines
+129
to
+131
| for k, v := range auth { | ||
| req.Header.Set(k, v) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Files =="
git ls-files internal/agentcatalog/agentcatalog.go internal/duties/duties.go internal | sed -n '1,200p'
echo
echo "== Search for X-Facets-Username and redirect handling =="
rg -n "X-Facets-Username|CheckRedirect|redirect|Authorization|Header.Set\\(k, v\\)" internal/agentcatalog internal/duties -S
echo
echo "== Outline relevant files =="
ast-grep outline internal/agentcatalog/agentcatalog.go --view expanded
echo
ast-grep outline internal/duties/duties.go --view expandedRepository: Facets-cloud/praxis-cli
Length of output: 8357
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' internal/agentcatalog/agentcatalog.go | cat -n
echo
sed -n '1,360p' internal/duties/duties.go | cat -n
echo
rg -n "CheckRedirect|X-Facets-Username|httptest|redirect" -S .Repository: Facets-cloud/praxis-cli
Length of output: 39444
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant snippets =="
for f in internal/agentcatalog/agentcatalog.go internal/duties/duties.go; do
echo "--- $f ---"
nl -ba "$f" | sed -n '1,340p' | sed -n '1,340p' | rg -n "CheckRedirect|X-Facets-Username|Header.Set|http.Client|Do\\(|Get\\(|Post\\(" -n -C 3
done
echo
echo "== tests mentioning redirect or the header =="
rg -n "X-Facets-Username|CheckRedirect|redirect" -S internal test . --glob '*_test.go'Repository: Facets-cloud/praxis-cli
Length of output: 267
Strip X-Facets-Username on cross-origin redirects. http.Client will still follow 3xx responses here, and this custom header is not automatically removed the way Authorization is, so a redirect to another host can leak the user identity. Apply a redirect policy to the shared transport path and add an httptest cross-host redirect regression test for:
internal/agentcatalog/agentcatalog.go#L129-L131internal/duties/duties.go#L222-L224internal/duties/duties.go#L272-L274
📍 Affects 2 files
internal/agentcatalog/agentcatalog.go#L129-L131(this comment)internal/duties/duties.go#L222-L224internal/duties/duties.go#L272-L274
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/agentcatalog/agentcatalog.go` around lines 129 - 131, Apply a shared
http.Client redirect policy that removes X-Facets-Username whenever a redirect
crosses hosts, while preserving it for same-origin redirects and retaining
normal redirect behavior. Update the request paths at
internal/agentcatalog/agentcatalog.go:129-131,
internal/duties/duties.go:222-224, and internal/duties/duties.go:272-274 to use
this policy, and add httptest cross-host redirect regression coverage for all
three sites.
Source: Coding guidelines
`praxis login --facets` reads a control-plane PAT from ~/.facets/credentials and sends it as HTTP Basic, instead of minting a Praxis API key via the browser. Lets facets-mode users drive the CLI with the control-plane credentials they already have. Pairs with agent-factory #1590 (the server now accepts a Basic control-plane PAT on the CLI gateway). - credentials: Profile.AuthMode + Profile.AuthHeader() — the single place an Authorization header is built (Bearer for Praxis keys, Basic for facets PATs); auth_mode persisted in the INI. ReadFacetsProfile reads the raptor ~/.facets/credentials file. - login: --facets / --facets-profile flags; facetsLogin verifies + persists a basic-mode profile. persistAndSetup takes a full Profile so a facets profile keeps its control-plane username for header rebuild. - Thread the auth-header value (not the raw token) through every agent-server call site (mcp, manifest, memory, duties, catalogs, auth/me) so all honor the profile's auth mode. "Bearer " and "basic" now each live in exactly one place. Non-facets (Bearer) login is unchanged. Verified end-to-end against a live facets-mode server: login (no browser) -> mcp manifest (12 MCPs) -> tool call returns real data; server logs status=200 for the control-plane identity. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Matches the server-side review on agent-factory #1590, which moved the control-plane PAT off HTTP Basic (browser-ambient, needed a CSRF carve-out). - credentials: AuthHeader() (returned "Basic base64(user:token)") replaced by Auth() map[string]string — always Authorization: Bearer <token>, plus X-Facets-Username: <username> for facets (AuthModeBasic) profiles; nil when there is no token. base64 dependency dropped. - Thread the header-set (map) through every agent-server call site instead of a single auth string: mcp, manifest, memory, duties, skill/agent catalogs, ig-catalog, auth/me, post-auth setup. Each sets all headers and guards len(auth)==0. - callMCP's cross-domain redirect strips X-Facets-Username too (the identity header must not leak to a foreign host, same rule as Authorization). Verified: gofmt/vet/build/test-race green, no "Basic " left in non-test code, and live end-to-end against a facets-mode server -- login (no browser) -> mcp (12 MCPs) -> tool call 200 -> ig list 200. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SatyendraGollaFacets
force-pushed
the
feat/facets-login
branch
from
August 5, 2026 09:28
300b510 to
f6509b3
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/login_dryrun.go`:
- Around line 36-47: Harden the authentication probe around fetchAuthMe by
requiring an HTTPS base URL before sending loginToken or stored profile
credentials, including rejecting insecure URLs resolved through resolveLoginURL
or normalizeBaseURL. Add an explicit redirect policy to fetchAuthMe, following
callMCP’s CheckRedirect pattern, so credential headers are not forwarded to
unsafe redirect destinations while preserving the intended same-domain behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 994a933d-e1d8-4e56-83a6-e29610665856
📒 Files selected for processing (42)
cmd/duty.gocmd/duty_test.gocmd/git_credential.gocmd/git_credential_test.gocmd/ig.gocmd/ig_hook.gocmd/ig_test.gocmd/login.gocmd/login_canonical_test.gocmd/login_dryrun.gocmd/login_dryrun_test.gocmd/login_local_test.gocmd/login_raptor_test.gocmd/login_reuse_test.gocmd/login_setup.gocmd/login_setup_test.gocmd/mcp.gocmd/mcp_test.gocmd/memory.gocmd/memory_test.gocmd/profiles.gocmd/profiles_test.gocmd/skill.gocmd/skill_test.gocmd/status.gocmd/status_test.gointernal/agentcatalog/agentcatalog.gointernal/agentcatalog/agentcatalog_test.gointernal/credentials/credentials.gointernal/credentials/credentials_test.gointernal/credentials/facets.gointernal/credentials/facets_test.gointernal/duties/duties.gointernal/duties/duties_test.gointernal/igcatalog/igcatalog.gointernal/igcatalog/igcatalog_test.gointernal/mcpmanifest/mcpmanifest.gointernal/mcpmanifest/mcpmanifest_test.gointernal/memory/client.gointernal/memory/client_test.gointernal/skillcatalog/skillcatalog.gointernal/skillcatalog/skillcatalog_test.go
🚧 Files skipped from review as they are similar to previous changes (37)
- cmd/status_test.go
- cmd/memory.go
- internal/mcpmanifest/mcpmanifest_test.go
- cmd/skill.go
- cmd/login_canonical_test.go
- cmd/ig.go
- cmd/login_setup.go
- cmd/login_reuse_test.go
- cmd/profiles.go
- internal/skillcatalog/skillcatalog_test.go
- cmd/ig_hook.go
- internal/memory/client_test.go
- cmd/git_credential.go
- internal/mcpmanifest/mcpmanifest.go
- cmd/memory_test.go
- internal/skillcatalog/skillcatalog.go
- cmd/login_local_test.go
- cmd/duty_test.go
- cmd/status.go
- cmd/profiles_test.go
- internal/agentcatalog/agentcatalog_test.go
- internal/credentials/credentials_test.go
- internal/memory/client.go
- internal/agentcatalog/agentcatalog.go
- cmd/git_credential_test.go
- cmd/login_setup_test.go
- internal/credentials/credentials.go
- cmd/mcp.go
- cmd/mcp_test.go
- internal/credentials/facets.go
- cmd/login.go
- cmd/skill_test.go
- internal/duties/duties.go
- internal/duties/duties_test.go
- cmd/ig_test.go
- internal/igcatalog/igcatalog.go
- internal/igcatalog/igcatalog_test.go
Comment on lines
+36
to
+47
| var probeAuth map[string]string | ||
| tokenSource := "none" | ||
| switch { | ||
| case loginToken != "": | ||
| probeToken, tokenSource = loginToken, "supplied" | ||
| probeAuth, tokenSource = credentials.Profile{Token: loginToken}.Auth(), "supplied" | ||
| case exists && prof.Token != "" && prof.URL == baseURL: | ||
| probeToken, tokenSource = prof.Token, "stored" | ||
| probeAuth, tokenSource = prof.Auth(), "stored" | ||
| } | ||
|
|
||
| reachable := true | ||
| tokenStatus, action := tokenSource, "browser" | ||
| _, err := fetchAuthMe(baseURL, probeToken) | ||
| _, err := fetchAuthMe(baseURL, probeAuth) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect: HTTPS enforcement and safe redirect handling at the fetchAuthMe boundary.
rg -n -C 5 \
'fetchAuthMe|normalizeBaseURL|CheckRedirect|http\.Client|NewRequest|Authorization|X-Facets-Username' \
--glob '*.go' cmd internalRepository: Facets-cloud/praxis-cli
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Extract the fetchAuthMe implementation fully
echo "=== fetchAuthMe implementation ==="
sed -n '638,670p' cmd/login.go
# Check for any scheme validation in the codebase
echo -e "\n=== Scheme validation (https check) ==="
rg -n 'http://' cmd/login_dryrun.go cmd/login.go internal/credentials/ || echo "No explicit http:// rejection found"
# Look for normalizeBaseURL and resolveLoginURL to see if they enforce https
echo -e "\n=== normalizeBaseURL and related URL handling ==="
sed -n '190,195p' cmd/login.go
# Check if there's any middleware or wrapper enforcing HTTPS
echo -e "\n=== Looking for URL scheme validation ==="
rg -n 'scheme|https|http\.Client' cmd/login.go | head -20Repository: Facets-cloud/praxis-cli
Length of output: 2661
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how baseURL is used in login_dryrun and what validation exists
echo "=== login_dryrun.go relevant lines ==="
sed -n '1,50p' cmd/login_dryrun.go | grep -A5 -B5 "baseURL"
# Check resolveLoginURL which sets baseURL
echo -e "\n=== resolveLoginURL function ==="
sed -n '173,188p' cmd/login.go
# Check if credentials store or any validation rejects http://
echo -e "\n=== Checking if http:// URLs are rejected anywhere ==="
rg -n 'ParseURL|url.Parse|scheme|IsHTTPS|ValidateURL|http://' cmd/login_dryrun.go cmd/login.go internal/credentials/ || echo "No explicit validation found"
# Check Go version to understand redirect behavior
echo -e "\n=== Check for Go module or documentation hints ==="
head -20 go.mod 2>/dev/null || echo "go.mod not accessible"
</parameter>Repository: Facets-cloud/praxis-cli
Length of output: 2556
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there's any URL validation before fetchAuthMe is called
echo "=== All callers of fetchAuthMe in login_dryrun.go ==="
sed -n '25,100p' cmd/login_dryrun.go
# Check credentials.DefaultURL to see if it enforces https
echo -e "\n=== credentials.DefaultURL ==="
rg -n 'DefaultURL|DefaultProfileName' internal/credentials/credentials.go | head -5
grep -A2 'DefaultURL' internal/credentials/credentials.go
# Check what credential sources are allowed
echo -e "\n=== Credential storage - is http:// blocked? ==="
sed -n '50,150p' internal/credentials/credentials.go | head -40Repository: Facets-cloud/praxis-cli
Length of output: 5075
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there's any validation of baseURL scheme before fetchAuthMe is called
echo "=== Check runLoginDryRun signature and baseURL source ==="
sed -n '27,37p' cmd/login_dryrun.go
# Check what calls runLoginDryRun
echo -e "\n=== Find all calls to runLoginDryRun ==="
rg -n 'runLoginDryRun' cmd/
# Check how baseURL flows from user input through login command
echo -e "\n=== login_dryrun command definition ==="
sed -n '1,26p' cmd/login_dryrun.go
</parameter>Repository: Facets-cloud/praxis-cli
Length of output: 2073
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the call at line 147 of cmd/login.go
echo "=== Call to runLoginDryRun in login.go ==="
sed -n '140,150p' cmd/login.go
# Check what baseURL is at that point
echo -e "\n=== Check how baseURL is resolved in cmd/login.go ==="
sed -n '120,150p' cmd/login.goRepository: Facets-cloud/praxis-cli
Length of output: 1632
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Final check: does the code accept http:// URLs?
# Run a test to see what happens with http://
echo "=== Check for any URL scheme validation ==="
rg -n 'scheme|StartsWith.*https|strings.Contains.*http' cmd/login.go cmd/login_dryrun.go internal/credentials/ || echo "No scheme validation found"
# Check Go's http.Client behavior on redirects
# Go 1.8+ strips Authorization header on cross-origin redirects
# But same-domain and subdomain redirects preserve headers
echo -e "\n=== Go version ==="
grep 'go 1\.' go.mod
# Confirm: baseURL from --url can be http://
# Check login command flags
echo -e "\n=== loginURL flag definition ==="
rg -B2 -A2 'loginURL' cmd/login.go | head -15
</parameter>Repository: Facets-cloud/praxis-cli
Length of output: 1146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there's any wrapping or validation around baseURL before it reaches fetchAuthMe
# Does the calling code in login.go perform any URL validation?
echo "=== Full resolveLoginURL function ==="
sed -n '173,188p' cmd/login.go
# Check if there are any tests that use http:// URLs with fetchAuthMe
echo -e "\n=== Tests using fetchAuthMe with http:// ==="
rg -n 'http://' cmd/login_dryrun_test.go cmd/login_test.go cmd/login_reuse_test.go 2>/dev/null || echo "Checking test files"
# Check if there's any evidence of HTTPS-only enforcement upstream
echo -e "\n=== Check for any validation in Profile.URL storage ==="
sed -n '120,180p' internal/credentials/credentials.go | grep -A10 -B10 'URL'Repository: Facets-cloud/praxis-cli
Length of output: 1774
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Final verification: confirm that fetchAuthMe truly lacks redirect handling
# Compare with callMCP to see the difference
echo "=== fetchAuthMe client setup ==="
sed -n '638,645p' cmd/login.go
echo -e "\n=== callMCP client setup with CheckRedirect ==="
sed -n '314,351p' cmd/mcp.go | head -40
# Verify Go 1.24 behavior: does it strip auth headers on redirects?
echo -e "\n=== Test evidence of Go's redirect header behavior ==="
grep -A5 'DropsAuthOnCrossDomainRedirect\|PreservesPOSTAcrossRedirect' cmd/mcp_test.go | head -20
</parameter>Repository: Facets-cloud/praxis-cli
Length of output: 3389
Sensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: External · Exploitability: Moderate
fetchAuthMe lacks explicit protection against credential leakage on same-domain or subdomain redirects.
loginToken and stored profile credentials are converted to Authorization and, for Facets profiles, X-Facets-Username headers, then passed to fetchAuthMe. The proof shows:
- fetchAuthMe uses a bare
http.Client(no customCheckRedirect), relying on Go's default redirect policy. - Go 1.24's default behavior strips
Authorizationonly on cross-domain redirects, but preserves it on same-domain and subdomain redirects (which is correct for same-domain but risky if an intermediate in the redirect chain is compromised). - callMCP in the same codebase implements an explicit
CheckRedirectfunction that mirrors Go's sensitive-header rule and documents the intent to drop credentials on cross-domain redirects—a pattern absent fromfetchAuthMe. - No URL scheme validation exists in
resolveLoginURLornormalizeBaseURL. Users can passhttp://URLs via--url, which would send credentials over plaintext. - The
prof.URL == baseURLcheck only scopes stored-token reuse to matching deployments; it does not enforce transport confidentiality or validate the redirect chain.
Add explicit HTTPS enforcement and, if fetchAuthMe follows same-domain redirects, explicitly document or enforce the credential-preservation semantics. Alternatively, adopt the CheckRedirect pattern from callMCP if same-domain redirects must survive credential forwarding.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/login_dryrun.go` around lines 36 - 47, Harden the authentication probe
around fetchAuthMe by requiring an HTTPS base URL before sending loginToken or
stored profile credentials, including rejecting insecure URLs resolved through
resolveLoginURL or normalizeBaseURL. Add an explicit redirect policy to
fetchAuthMe, following callMCP’s CheckRedirect pattern, so credential headers
are not forwarded to unsafe redirect destinations while preserving the intended
same-domain behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
praxis login --facetsauthenticates with a control-plane PAT (read from~/.facets/credentials) sent as HTTP Basic, instead of minting a Praxis API key through the browser. Facets-mode users can drive the CLI with the control-plane credentials they already have — no separate Praxis key.Pairs with agent-factory PR #1590 (server side: the CLI gateway now accepts a
Basiccontrol-plane PAT in facets mode). This is the client half.Changes
internal/credentials—Profile.AuthMode+Profile.AuthHeader(): the single place anAuthorizationheader value is built (Bearer for Praxis keys, Basic for facets PATs).auth_modepersisted in the INI. NewReadFacetsProfilereads the raptor~/.facets/credentialsfile (reusing the existing INI parser).cmd/login.go—--facets/--facets-profileflags;facetsLoginverifies + persists abasic-mode profile.persistAndSetupnow takes a fullProfileso a facets profile keeps its control-plane username (needed to rebuild the Basic header on reuse).mcp, manifest, memory, duties, skill/agent catalogs,auth/me) now takes the auth-header value fromProfile.AuthHeader()instead of hand-setting"Bearer "+token. After this,"Bearer "and"basic"each live in exactly one place.Non-facets (Bearer) login is unchanged.
Testing
gofmt,go vet,go build ./...,go test -race ./...all green. New unit tests:AuthHeader(both modes + empty),auth_modeINI round-trip,ReadFacetsProfile(5 cases).praxis login --facets(no browser) →praxis mcplists 12 MCPs →praxis mcp cloud_cli list_cloud_integrationsreturns real data. Server logs[CLI_GATEWAY] … status=200for the control-plane identity.Usage
🤖 Generated with Claude Code
Summary by CodeRabbit
--facetsand--facets-profile, including credential verification and profile selection.