fix(auth): address PR review comments for Google Workspace OAuth
- Secure token file permissions (0o600) in dashboard callback handler - Validate refresh_token presence after code exchange - HTML-escape all dynamic values in callback pages (XSS prevention) - Raise error when only placeholder credentials are available - Fix docstring to match actual behavior (no standalone fallback) - Validate OAuth state parameter in headless mode - Reduce client_id log exposure to 8 chars - Use configurable port for dashboard redirect URI (app.state.bound_port) - Read HERMES_DASHBOARD_PORT env var instead of hardcoding 9119
This commit is contained in:
@@ -253,6 +253,16 @@ def get_client_credentials() -> Tuple[str, str]:
|
||||
if _NOUS_CLIENT_ID and _NOUS_CLIENT_ID != "PLACEHOLDER.apps.googleusercontent.com":
|
||||
return _NOUS_CLIENT_ID, _NOUS_CLIENT_SECRET
|
||||
|
||||
# Placeholder credentials are not usable — raise so callers get a clear error
|
||||
if _NOUS_CLIENT_ID.startswith("PLACEHOLDER"):
|
||||
raise GoogleWorkspaceOAuthError(
|
||||
"No usable Google OAuth client credentials found.\n"
|
||||
"The bundled Nous Research app is not yet verified. Please either:\n"
|
||||
" 1. Set HERMES_GOOGLE_WORKSPACE_CLIENT_ID and HERMES_GOOGLE_WORKSPACE_CLIENT_SECRET env vars\n"
|
||||
" 2. Place a google_client_secret.json in ~/.hermes/\n",
|
||||
code="google_workspace_oauth_no_client",
|
||||
)
|
||||
|
||||
# If we only have the placeholder, still return it — caller can decide
|
||||
# whether to proceed or error out.
|
||||
return _NOUS_CLIENT_ID, _NOUS_CLIENT_SECRET
|
||||
@@ -706,8 +716,7 @@ def run_oauth_login() -> dict:
|
||||
saves the token. This function polls the dashboard session until
|
||||
it's approved.
|
||||
|
||||
Falls back to a standalone local HTTP server on port 8098 if the
|
||||
dashboard is not reachable.
|
||||
If the dashboard is not running, exits with instructions to start it or use --no-browser mode.
|
||||
|
||||
Returns:
|
||||
The saved token data dict.
|
||||
@@ -727,7 +736,8 @@ def run_oauth_login() -> dict:
|
||||
)
|
||||
|
||||
# Try the dashboard flow first (preferred — single source of truth)
|
||||
dashboard_url = "http://127.0.0.1:9119"
|
||||
dashboard_port = os.environ.get("HERMES_DASHBOARD_PORT", "9119")
|
||||
dashboard_url = f"http://127.0.0.1:{dashboard_port}"
|
||||
use_dashboard = _is_dashboard_running(dashboard_url)
|
||||
|
||||
if use_dashboard:
|
||||
|
||||
+11
-1
@@ -1874,14 +1874,24 @@ def login_google_workspace_command(args) -> None:
|
||||
|
||||
# Extract code from URL if user pasted the full redirect URL
|
||||
code = callback_input
|
||||
returned_state = None
|
||||
if callback_input.startswith("http"):
|
||||
from urllib.parse import parse_qs, urlparse
|
||||
parsed = urlparse(callback_input)
|
||||
params = parse_qs(parsed.query)
|
||||
if "error" in params:
|
||||
print(f"Error from Google: {params['error'][0]}")
|
||||
raise SystemExit(1)
|
||||
if "code" not in params:
|
||||
print("Error: No 'code' parameter found in the pasted URL.")
|
||||
raise SystemExit(1)
|
||||
code = params["code"][0]
|
||||
returned_state = params.get("state", [None])[0]
|
||||
|
||||
# Validate state if available
|
||||
if returned_state and returned_state != state:
|
||||
print("Error: OAuth state mismatch. This may be a code from a different session.")
|
||||
raise SystemExit(1)
|
||||
|
||||
try:
|
||||
token_data = exchange_code(code, state, code_verifier, redirect_uri="http://localhost:1")
|
||||
@@ -1891,7 +1901,7 @@ def login_google_workspace_command(args) -> None:
|
||||
else:
|
||||
# Interactive flow — opens browser, waits for callback
|
||||
print("Starting Google Workspace PKCE login...")
|
||||
print(f" Client ID: {client_id[:20]}...")
|
||||
print(f" Client ID: {client_id[:8]}…")
|
||||
print(f" Token will be saved to: {credentials_path()}")
|
||||
print()
|
||||
try:
|
||||
|
||||
@@ -12,6 +12,7 @@ Usage:
|
||||
import asyncio
|
||||
import base64
|
||||
import hashlib
|
||||
import html
|
||||
import hmac
|
||||
import importlib.util
|
||||
import json
|
||||
@@ -2157,7 +2158,9 @@ def _start_google_workspace_pkce() -> Dict[str, Any]:
|
||||
sess["client_secret"] = client_secret
|
||||
|
||||
# Use localhost:9119 callback so the dashboard catches the redirect automatically
|
||||
redirect_uri = "http://localhost:9119/auth/google/callback"
|
||||
# Use the actual bound port if available, default to 9119
|
||||
bound_port = getattr(app.state, "bound_port", 9119)
|
||||
redirect_uri = f"http://localhost:{bound_port}/auth/google/callback"
|
||||
sess["redirect_uri"] = redirect_uri
|
||||
|
||||
params = {
|
||||
@@ -2242,6 +2245,12 @@ def _submit_google_workspace_pkce(session_id: str, code_input: str) -> Dict[str,
|
||||
sess["error_message"] = "No access token returned"
|
||||
return {"ok": False, "status": "error", "message": sess["error_message"]}
|
||||
|
||||
if not refresh_token:
|
||||
with _oauth_sessions_lock:
|
||||
sess["status"] = "error"
|
||||
sess["error_message"] = "No refresh token returned. Ensure 'access_type=offline' and 'prompt=consent' are set."
|
||||
return {"ok": False, "status": "error", "message": sess["error_message"]}
|
||||
|
||||
# Save token in google_token.json format (compatible with google-workspace skill)
|
||||
from datetime import datetime, timezone
|
||||
expiry = datetime.fromtimestamp(
|
||||
@@ -2262,7 +2271,12 @@ def _submit_google_workspace_pkce(session_id: str, code_input: str) -> Dict[str,
|
||||
|
||||
token_path = credentials_path()
|
||||
token_path.parent.mkdir(parents=True, exist_ok=True)
|
||||
token_path.write_text(json.dumps(token_payload, indent=2), encoding="utf-8")
|
||||
import os as _os
|
||||
fd = _os.open(str(token_path), _os.O_WRONLY | _os.O_CREAT | _os.O_TRUNC, 0o600)
|
||||
try:
|
||||
_os.write(fd, json.dumps(token_payload, indent=2).encode("utf-8"))
|
||||
finally:
|
||||
_os.close(fd)
|
||||
|
||||
with _oauth_sessions_lock:
|
||||
sess["status"] = "approved"
|
||||
@@ -2369,7 +2383,7 @@ body{{background:#041c1c;color:#ffe6cb;font-family:system-ui,-apple-system,"Sego
|
||||
h1{{font-size:1.5rem;font-weight:600;letter-spacing:0.05em;text-transform:uppercase;margin-bottom:0.75rem}}
|
||||
p{{color:rgba(255,230,203,0.7);font-size:0.95rem;line-height:1.6}}
|
||||
</style></head>
|
||||
<body><div class="card"><div class="icon">✗</div><h1>Authorization Failed</h1><p>{error}</p></div></body>
|
||||
<body><div class="card"><div class="icon">✗</div><h1>Authorization Failed</h1><p>{html.escape(error)}</p></div></body>
|
||||
</html>""",
|
||||
status_code=400,
|
||||
)
|
||||
@@ -2396,7 +2410,7 @@ p{color:rgba(255,230,203,0.7);font-size:0.95rem;line-height:1.6}
|
||||
result = _submit_google_workspace_pkce(state, code)
|
||||
except HTTPException as exc:
|
||||
return HTMLResponse(
|
||||
f"<html><body><h2>❌ {exc.detail}</h2></body></html>",
|
||||
f"<html><body><h2>❌ {html.escape(str(exc.detail))}</h2></body></html>",
|
||||
status_code=exc.status_code,
|
||||
)
|
||||
|
||||
@@ -2488,7 +2502,7 @@ p{color:rgba(255,230,203,0.7);font-size:0.95rem;line-height:1.6}
|
||||
<div class="card">
|
||||
<div class="icon">✗</div>
|
||||
<h1>Connection Failed</h1>
|
||||
<p>{msg}</p>
|
||||
<p>{html.escape(str(msg))}</p>
|
||||
</div>
|
||||
</body>
|
||||
</html>""",
|
||||
|
||||
Reference in New Issue
Block a user