Add OpenAI (ChatGPT) OAuth sign-in, status endpoints and UI integration #18
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis pull request introduces OpenAI OAuth authentication functionality to the application. It implements a PKCE-based authorization flow with local token persistence and adds API handlers for authentication status checks and logout. The implementation includes a callback server to handle OAuth redirects, core authentication logic in the helpers module, API endpoints in the python/api directory, and corresponding UI components in the webui for authentication management and status display within the external settings section. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (6)
🧰 Additional context used🧬 Code graph analysis (2)python/api/openai_auth_status.py (3)
python/api/openai_auth_logout.py (4)
🪛 Ruff (0.14.11)python/api/openai_auth_status.py6-6: Unused method argument: (ARG002) 6-6: Unused method argument: (ARG002) python/api/openai_auth_logout.py6-6: Unused method argument: (ARG002) 6-6: Unused method argument: (ARG002) python/helpers/openai_auth.py21-21: Possible hardcoded password assigned to: "TOKEN_URL" (S105) 67-67: Do not catch blind exception: (BLE001) 99-99: Do not catch blind exception: (BLE001) 299-299: Unused method argument: (ARG002) 299-299: Unused method argument: (ARG002) 299-299: Dynamically typed expressions (typing.Any) are disallowed in (ANN401) 🔇 Additional comments (14)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@python/helpers/openai_auth.py`:
- Around line 41-45: Concurrent initializations of the callback HTTP server can
race and overwrite globals; add a dedicated lock (e.g., _callback_lock =
threading.Lock()) and wrap the initialization logic inside
ensure_callback_server() with this lock: re-check _callback_server after
acquiring the lock, create and bind the ThreadingHTTPServer instance locally
first, only assign to the global _callback_server and start/set _callback_thread
after successful bind/start, and use try/finally to release the lock; apply the
same guarded pattern to the other initialization block at the 343-358 region
that touches _callback_server and _callback_thread.
- Around line 103-111: The persisted OAuth tokens in AUTH_FILE are written with
default permissions; update the _save_auth_file function (and consider
clear_auth's behavior) to enforce restrictive file permissions (owner read/write
only) after creating the file: write the JSON as before using
files.write_file(AUTH_FILE, ...), then call os.chmod on
files.get_abs_path(AUTH_FILE) to set mode 0o600 (or create the file with os.open
and os.fdopen using os.O_CREAT|os.O_WRONLY with explicit mode 0o600), ensuring
the token file is owner-only readable/writable and not world-readable.
- Around line 58-68: The _decode_jwt_payload function currently catches all
Exceptions; narrow the except clause to only the decode/parse errors (e.g.,
binascii.Error from base64.urlsafe_b64decode, UnicodeDecodeError from .decode(),
and json.JSONDecodeError from json.loads) so unexpected bugs aren't
masked—import binascii if needed and replace "except Exception:" with "except
(binascii.Error, UnicodeDecodeError, json.JSONDecodeError): return None" while
preserving the same return behavior.
- Around line 173-199: In exchange_authorization_code, guard the httpx.post call
and the response.json() parse: wrap the httpx.post(...) in a try/except that
catches httpx.RequestError (log via PrintStyle.error with the exception and
return None) and then wrap payload = response.json() in a try/except catching
ValueError/JSONDecodeError (log the raw response.text and the parse error via
PrintStyle.error and return None); keep the existing status_code check and field
validations after successful parse.
In `@webui/components/settings/external/openai_auth.html`:
- Around line 90-100: The Token Expiration field is rendered when status is
'error' due to the condition using only !== 'disconnected' and !== 'loading';
update the template rendering logic (the <template> that checks the status
variable) to explicitly exclude the 'error' state or only render when status ===
'connected' (or the appropriate successful state), so expiresLabel is only shown
when the auth status is valid and not when an error occurred.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
python/api/openai_auth_logout.pypython/api/openai_auth_status.pypython/helpers/openai_auth.pyrun_ui.pywebui/components/settings/external/external-settings.htmlwebui/components/settings/external/openai_auth.html
🧰 Additional context used
🧬 Code graph analysis (4)
run_ui.py (2)
python/helpers/api.py (1)
requires_auth(31-32)python/helpers/openai_auth.py (3)
create_authorization_flow(134-151)set_pending_flow(154-157)ensure_callback_server(343-358)
python/api/openai_auth_status.py (1)
python/helpers/tool.py (1)
Response(11-14)
python/helpers/openai_auth.py (2)
python/helpers/print_style.py (1)
PrintStyle(6-157)python/helpers/files.py (3)
exists(486-488)read_file(206-212)write_file(357-362)
python/api/openai_auth_logout.py (4)
python/helpers/api.py (1)
ApiHandler(17-99)python/helpers/tool.py (1)
Response(11-14)python/api/openai_auth_status.py (1)
process(6-7)python/helpers/openai_auth.py (1)
clear_auth(107-110)
🪛 Ruff (0.14.11)
python/api/openai_auth_status.py
6-6: Unused method argument: input
(ARG002)
6-6: Unused method argument: request
(ARG002)
python/helpers/openai_auth.py
21-21: Possible hardcoded password assigned to: "TOKEN_URL"
(S105)
67-67: Do not catch blind exception: Exception
(BLE001)
99-99: Do not catch blind exception: Exception
(BLE001)
299-299: Unused method argument: format
(ARG002)
299-299: Unused method argument: args
(ARG002)
299-299: Dynamically typed expressions (typing.Any) are disallowed in *args
(ANN401)
python/api/openai_auth_logout.py
6-6: Unused method argument: input
(ARG002)
6-6: Unused method argument: request
(ARG002)
🔇 Additional comments (8)
webui/components/settings/external/external-settings.html (1)
18-23: LGTM! Navigation and section structure follow existing patterns.The new OpenAI Auth entry correctly integrates with the existing navigation framework. Minor observation: this reuses
/public/auth.svgwhich is also used by the "Authentication" section (line 38). Consider using a distinct icon if visual differentiation is desired.Also applies to: 66-68
python/api/openai_auth_logout.py (1)
5-8: LGTM! Simple delegation to helper.The unused
inputandrequestparameters flagged by static analysis are required by theApiHandler.processabstract method signature. This is a false positive.python/api/openai_auth_status.py (1)
5-7: LGTM! Clean delegation to helper.Same as the logout handler—the unused parameters are required by the abstract method signature. The implementation correctly delegates to
get_auth_status()which handles all edge cases (missing file, expired tokens).webui/components/settings/external/openai_auth.html (2)
15-32: LGTM! Status refresh logic is well-structured.The error handling with try/catch and status state management is clean. The status determination logic correctly prioritizes
connectedoverexpiredbased on the backend's response structure.
110-115: Good security practice withnoopener,noreferrer.Opening the OAuth flow in a new tab with these attributes prevents reverse tabnabbing attacks.
python/helpers/openai_auth.py (3)
47-55: PKCE helper implementation looks solid.Verifier/challenge generation and base64url encoding are clear and correct.
134-171: Flow state + PKCE wiring is clean and thread-safe.Nice use of
state, PKCE, and a lock around pending flow state.
298-329: Callback handler validates state and provides clear UX.State checks + explicit success/error pages are well done.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| _pending_lock = threading.Lock() | ||
| _pending_flow: AuthorizationFlow | None = None | ||
| _callback_server: ThreadingHTTPServer | None = None | ||
| _callback_thread: threading.Thread | None = None | ||
|
|
There was a problem hiding this comment.
Serialize callback server initialization to avoid races.
Concurrent calls to ensure_callback_server() can clobber _callback_server on a bind race. Guard the initialization with a lock and only set the global after successful bind.
🧵 Suggested fix
_pending_lock = threading.Lock()
_pending_flow: AuthorizationFlow | None = None
_callback_server: ThreadingHTTPServer | None = None
_callback_thread: threading.Thread | None = None
+_callback_lock = threading.Lock()
@@
def ensure_callback_server() -> None:
global _callback_server, _callback_thread
- if _callback_server:
- return
-
- try:
- _callback_server = ThreadingHTTPServer(("127.0.0.1", 1455), _CallbackHandler)
- except OSError as exc:
- PrintStyle.error(f"OpenAI OAuth callback server unavailable: {exc}")
- _callback_server = None
- return
-
- _callback_thread = threading.Thread(
- target=_callback_server.serve_forever, daemon=True
- )
- _callback_thread.start()
+ with _callback_lock:
+ if _callback_server:
+ return
+ try:
+ server = ThreadingHTTPServer(("127.0.0.1", 1455), _CallbackHandler)
+ except OSError as exc:
+ PrintStyle.error(f"OpenAI OAuth callback server unavailable: {exc}")
+ return
+ _callback_server = server
+ _callback_thread = threading.Thread(
+ target=_callback_server.serve_forever, daemon=True
+ )
+ _callback_thread.start()Also applies to: 343-358
🤖 Prompt for AI Agents
In `@python/helpers/openai_auth.py` around lines 41 - 45, Concurrent
initializations of the callback HTTP server can race and overwrite globals; add
a dedicated lock (e.g., _callback_lock = threading.Lock()) and wrap the
initialization logic inside ensure_callback_server() with this lock: re-check
_callback_server after acquiring the lock, create and bind the
ThreadingHTTPServer instance locally first, only assign to the global
_callback_server and start/set _callback_thread after successful bind/start, and
use try/finally to release the lock; apply the same guarded pattern to the other
initialization block at the 343-358 region that touches _callback_server and
_callback_thread.
| def _decode_jwt_payload(token: str) -> dict[str, Any] | None: | ||
| try: | ||
| parts = token.split(".") | ||
| if len(parts) != 3: | ||
| return None | ||
| payload = parts[1] | ||
| payload += "=" * (-len(payload) % 4) | ||
| decoded = base64.urlsafe_b64decode(payload.encode()).decode("utf-8") | ||
| return json.loads(decoded) | ||
| except Exception: | ||
| return None |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid blanket exception handling in JWT payload decoding.
Catching Exception can mask unexpected bugs. Narrow to decode/JSON errors to keep failures explicit.
♻️ Suggested refactor
+import binascii- except Exception:
+ except (binascii.Error, UnicodeDecodeError, ValueError):
return None🧰 Tools
🪛 Ruff (0.14.11)
67-67: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@python/helpers/openai_auth.py` around lines 58 - 68, The _decode_jwt_payload
function currently catches all Exceptions; narrow the except clause to only the
decode/parse errors (e.g., binascii.Error from base64.urlsafe_b64decode,
UnicodeDecodeError from .decode(), and json.JSONDecodeError from json.loads) so
unexpected bugs aren't masked—import binascii if needed and replace "except
Exception:" with "except (binascii.Error, UnicodeDecodeError,
json.JSONDecodeError): return None" while preserving the same return behavior.
| def _save_auth_file(auth: OpenAIAuth) -> None: | ||
| files.write_file(AUTH_FILE, json.dumps(auth, indent=2)) | ||
|
|
||
|
|
||
| def clear_auth() -> None: | ||
| abs_path = files.get_abs_path(AUTH_FILE) | ||
| if os.path.exists(abs_path): | ||
| os.remove(abs_path) | ||
|
|
There was a problem hiding this comment.
Restrict permissions on persisted OAuth tokens.
tmp/openai_auth.json contains access/refresh tokens but is written with default permissions. On shared machines this can expose tokens to other users.
🔐 Proposed hardening
def _save_auth_file(auth: OpenAIAuth) -> None:
files.write_file(AUTH_FILE, json.dumps(auth, indent=2))
+ try:
+ os.chmod(files.get_abs_path(AUTH_FILE), 0o600)
+ except OSError as exc:
+ PrintStyle.warning(f"Unable to restrict permissions on {AUTH_FILE}: {exc}")🤖 Prompt for AI Agents
In `@python/helpers/openai_auth.py` around lines 103 - 111, The persisted OAuth
tokens in AUTH_FILE are written with default permissions; update the
_save_auth_file function (and consider clear_auth's behavior) to enforce
restrictive file permissions (owner read/write only) after creating the file:
write the JSON as before using files.write_file(AUTH_FILE, ...), then call
os.chmod on files.get_abs_path(AUTH_FILE) to set mode 0o600 (or create the file
with os.open and os.fdopen using os.O_CREAT|os.O_WRONLY with explicit mode
0o600), ensuring the token file is owner-only readable/writable and not
world-readable.
| def exchange_authorization_code(code: str, verifier: str) -> OpenAIAuth | None: | ||
| response = httpx.post( | ||
| TOKEN_URL, | ||
| headers={"Content-Type": "application/x-www-form-urlencoded"}, | ||
| data={ | ||
| "grant_type": "authorization_code", | ||
| "client_id": CLIENT_ID, | ||
| "code": code, | ||
| "code_verifier": verifier, | ||
| "redirect_uri": REDIRECT_URI, | ||
| }, | ||
| timeout=15.0, | ||
| ) | ||
| if response.status_code != HTTPStatus.OK: | ||
| PrintStyle.error( | ||
| f"OpenAI OAuth token exchange failed: {response.status_code} {response.text}" | ||
| ) | ||
| return None | ||
|
|
||
| payload = response.json() | ||
| access_token = payload.get("access_token") | ||
| refresh_token = payload.get("refresh_token") | ||
| expires_in = payload.get("expires_in") | ||
| if not access_token or not refresh_token or not isinstance(expires_in, (int, float)): | ||
| PrintStyle.error("OpenAI OAuth token response missing fields.") | ||
| return None | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n python/helpers/openai_auth.py | head -210 | tail -50Repository: 3clyp50/agent-zero
Length of output: 1947
🏁 Script executed:
head -50 python/helpers/openai_auth.pyRepository: 3clyp50/agent-zero
Length of output: 1220
🏁 Script executed:
web_search
httpx.RequestError and httpx.post exception handling documentationRepository: 3clyp50/agent-zero
Length of output: 169
🌐 Web query:
httpx.post RequestError exception handling Python documentation
💡 Result:
Short answer — catch httpx.RequestError (or the broader httpx.HTTPError) around your httpx.post call. RequestError is the base class for network/transport errors and exposes the original .request; HTTPStatusError is raised by response.raise_for_status(). Example:
- Catch transport/network errors only:
try:
resp = httpx.post("https://example.com", json=data)
except httpx.RequestError as exc:
print(f"Request failed for {exc.request.url!r}: {exc}")
- Catch both request/transport errors and non-2xx responses:
try:
resp = httpx.post("https://example.com", json=data)
resp.raise_for_status()
except httpx.RequestError as exc:
print(f"Request error for {exc.request.url!r}: {exc}")
except httpx.HTTPStatusError as exc:
print(f"HTTP error {exc.response.status_code} for {exc.request.url!r}: {exc}")
Reference: httpx exceptions and quickstart (covers RequestError, HTTPError, and raise_for_status()). [1][2]
Sources
[1] HTTPX — Exceptions (api) documentation.
[2] HTTPX — Quickstart (Exceptions section).
🏁 Script executed:
rg -t py "httpx\." --context 3Repository: 3clyp50/agent-zero
Length of output: 3905
Guard token exchange against network and JSON parsing failures.
httpx.post() can raise httpx.RequestError for network timeouts, connection errors, and other transport issues. response.json() can raise ValueError if the response body is not valid JSON. Both exceptions are currently unhandled and will break the OAuth callback flow.
Wrap the HTTP request and JSON parsing in explicit error handlers:
🛠️ Proposed fix
def exchange_authorization_code(code: str, verifier: str) -> OpenAIAuth | None:
- response = httpx.post(
- TOKEN_URL,
- headers={"Content-Type": "application/x-www-form-urlencoded"},
- data={
- "grant_type": "authorization_code",
- "client_id": CLIENT_ID,
- "code": code,
- "code_verifier": verifier,
- "redirect_uri": REDIRECT_URI,
- },
- timeout=15.0,
- )
+ try:
+ response = httpx.post(
+ TOKEN_URL,
+ headers={"Content-Type": "application/x-www-form-urlencoded"},
+ data={
+ "grant_type": "authorization_code",
+ "client_id": CLIENT_ID,
+ "code": code,
+ "code_verifier": verifier,
+ "redirect_uri": REDIRECT_URI,
+ },
+ timeout=15.0,
+ )
+ except httpx.RequestError as exc:
+ PrintStyle.error(f"OpenAI OAuth token exchange failed: {exc}")
+ return None
if response.status_code != HTTPStatus.OK:
PrintStyle.error(
f"OpenAI OAuth token exchange failed: {response.status_code} {response.text}"
)
return None
- payload = response.json()
+ try:
+ payload = response.json()
+ except ValueError:
+ PrintStyle.error("OpenAI OAuth token response was not valid JSON.")
+ return None🤖 Prompt for AI Agents
In `@python/helpers/openai_auth.py` around lines 173 - 199, In
exchange_authorization_code, guard the httpx.post call and the response.json()
parse: wrap the httpx.post(...) in a try/except that catches httpx.RequestError
(log via PrintStyle.error with the exception and return None) and then wrap
payload = response.json() in a try/except catching ValueError/JSONDecodeError
(log the raw response.text and the parse error via PrintStyle.error and return
None); keep the existing status_code check and field validations after
successful parse.
| <template x-if="status !== 'disconnected' && status !== 'loading'"> | ||
| <div class="field"> | ||
| <div class="field-label"> | ||
| <div class="field-title">Token Expiration</div> | ||
| <div class="field-description">Refresh tokens are stored to keep the session active.</div> | ||
| </div> | ||
| <div class="field-control"> | ||
| <input type="text" readonly :value="expiresLabel" /> | ||
| </div> | ||
| </div> | ||
| </template> |
There was a problem hiding this comment.
Token expiration field visible during error state.
The condition status !== 'disconnected' && status !== 'loading' also includes status === 'error', which may show stale or placeholder expiration data when the status fetch fails.
Suggested fix
- <template x-if="status !== 'disconnected' && status !== 'loading'">
+ <template x-if="status === 'connected' || status === 'expired'">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <template x-if="status !== 'disconnected' && status !== 'loading'"> | |
| <div class="field"> | |
| <div class="field-label"> | |
| <div class="field-title">Token Expiration</div> | |
| <div class="field-description">Refresh tokens are stored to keep the session active.</div> | |
| </div> | |
| <div class="field-control"> | |
| <input type="text" readonly :value="expiresLabel" /> | |
| </div> | |
| </div> | |
| </template> | |
| <template x-if="status === 'connected' || status === 'expired'"> | |
| <div class="field"> | |
| <div class="field-label"> | |
| <div class="field-title">Token Expiration</div> | |
| <div class="field-description">Refresh tokens are stored to keep the session active.</div> | |
| </div> | |
| <div class="field-control"> | |
| <input type="text" readonly :value="expiresLabel" /> | |
| </div> | |
| </div> | |
| </template> |
🤖 Prompt for AI Agents
In `@webui/components/settings/external/openai_auth.html` around lines 90 - 100,
The Token Expiration field is rendered when status is 'error' due to the
condition using only !== 'disconnected' and !== 'loading'; update the template
rendering logic (the <template> that checks the status variable) to explicitly
exclude the 'error' state or only render when status === 'connected' (or the
appropriate successful state), so expiresLabel is only shown when the auth
status is valid and not when an error occurred.
1 participant
Motivation
Description
python/helpers/openai_auth.pyimplementing a PKCE OAuth flow, local callback server bound tohttp://localhost:1455/auth/callback, token exchange, basic JWT payload parsing to extractchatgpt_account_id, and token storage intmp/openai_auth.json.python/api/openai_auth_status.pyandpython/api/openai_auth_logout.pyto report stored auth status and clear stored tokens respectively./auth/openaiinrun_ui.pythat creates the authorization flow and launches the local callback server, and update the External Services UI by addingwebui/components/settings/external/openai_auth.htmlplus a navigation entry inwebui/components/settings/external/external-settings.htmlto show status, expiration, account id, a "Sign in with ChatGPT" button and a Disconnect control.Testing
pip install -r requirements.txtsuccessfully and thenpip install -r requirements2.txtwhich installed packages but emitted a dependency conflict warning wherebrowser-useexpectsopenai==1.99.2whilerequirements2.txtprovidedopenai==1.99.5.WEB_UI_HOST=0.0.0.0 python run_ui.py, which started the server and served the index page athttp://localhost:5000; an embedding preload reported a Hugging Face connectivity warning but the server remained responsive.artifacts/openai-auth-settings.png) verifying the new UI was rendered.Codex Task