Conversation
This needs some love in validation and token processing
There was a problem hiding this comment.
Pull Request Overview
The PR implements foundational OAuth 2.0 and JWT support for the AYON Server.
- Adds
oauth_clientstable, indexes, and triggers to the database schema. - Introduces
OAuthStorage, OAuth server logic, and JWT utilities for token issuance and validation. - Exposes OAuth and OpenID Connect endpoints, including discovery, token, introspection, and userinfo.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| schemas/schema.public.sql | Add oauth_clients table, indexes, and updated_at trigger |
| schemas/migrations/00000007_oauth.sql | Migration script mirroring the new oauth_clients schema |
| ayon_server/oauth/storage.py | Implement client and token persistence in PostgreSQL and Redis |
| ayon_server/oauth/server.py | Core OAuth2 server implementation using oauthlib |
| ayon_server/oauth/jwt_manager.py | JWT access and ID token creation, decoding, and JWKS endpoint |
| ayon_server/api/server.py | Add OpenID Connect discovery and JWKS endpoints |
| api/oauth/oauth_provider.py | Define FastAPI OAuth endpoints (authorize, token, consent, etc.) |
|
So I've tested it and it sort of works but there are several weak points/questions to be answered access token vs. oauthonce you get oauth access token, you shouldn't need ayon access token but you still do. Maybe we should support JWT issuerI wasn't able to figure out how to get the server name without access to Request object. Now, issuer is stupidly hardcoded and not validated. KeysNow we use JWT endpointIs very useful but right now it is using some hardcoded values. Maybe this should be turned into helper method to be used in individual addons for better context? |
|
you can use this little script for testing btw: https://gist.github.com/antirotor/7f5720a5ba168edac2c6c31825f46a2b
{
"clientName": "demo-client",
"redirectUris": [
"http://localhost:5000/callback"
],
"grantTypes": [
"authorization_code"
],
"responseTypes": [
"code"
],
"scope": "openid",
"clientType": "confidential"
}(note that
|
| error_params["state"] = state | ||
| if redirect_uri: | ||
| return RedirectResponse( | ||
| f"{redirect_uri}?{urlencode(error_params)}" |
Check warning
Code scanning / CodeQL
URL redirection from remote source Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 months ago
To fix the issue, we must ensure that any redirect_uri used for a redirect is validated or replaced with a safe, server‑controlled URL, even in error paths. Specifically, in the branch where the client is not found (if not client:), we should not redirect to an arbitrary redirect_uri supplied by the user; instead, either (a) skip redirecting and just raise an error, or (b) only redirect if the redirect_uri passes the same validation we apply for valid clients. Since we do not have access to client.redirect_uris when the client is invalid, the safest and simplest fix that preserves behavior is to drop the redirect in this error path and respond with a BadRequestException.
Concretely, in api/oauth/oauth_provider.py within authorize_endpoint, remove the branch that returns RedirectResponse(f"{redirect_uri}?{urlencode(error_params)}") when client is None. Replace it with unconditionally raising BadRequestException("Invalid client_id"), optionally including state in the error message for debugging if desired, but not redirecting. No new imports or helper methods are required; we simply avoid using the tainted redirect_uri in that branch. All other uses of redirect_uri occur after it is either validated against client.redirect_uris or replaced with a safe default, so they can remain unchanged.
| @@ -98,16 +98,7 @@ | ||
| # Validate client | ||
| client = await OAuthStorage.get_client(client_id) | ||
| if not client: | ||
| error_params = { | ||
| "error": "invalid_client", | ||
| "error_description": "Invalid client_id" | ||
| } | ||
| if state: | ||
| error_params["state"] = state | ||
| if redirect_uri: | ||
| return RedirectResponse( | ||
| f"{redirect_uri}?{urlencode(error_params)}" | ||
| ) | ||
| # Do not redirect to an unvalidated redirect_uri; return an error instead | ||
| raise BadRequestException("Invalid client_id") | ||
|
|
||
| # Validate redirect_uri |
| } | ||
| if state: | ||
| error_params["state"] = state | ||
| return RedirectResponse(f"{redirect_uri}?{urlencode(error_params)}") |
Check warning
Code scanning / CodeQL
URL redirection from remote source Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 months ago
General approach: Ensure that untrusted user input cannot influence the redirect target in an unsafe way. In this endpoint, that mainly means making it explicit (and obvious to tools) that redirect_uri used in redirects is a server-trusted value, not raw request data, and avoiding building redirect URLs from unvalidated data.
Best concrete fix here without changing behavior: introduce a separate variable (e.g. safe_redirect_uri) that is only ever assigned from validated or server-derived values, and use that for all redirects. Also preserve the current validation logic that enforces membership in client.redirect_uris. This makes the data flow clearer: user input is validated, then the validated value is copied into a “safe” variable, which is what is used in RedirectResponse.
Specific changes in api/oauth/oauth_provider.py within authorize_endpoint:
- After client validation and before any use of
redirect_uri, create asafe_redirect_urivariable:- Initialize it from
redirect_urionly if it is present and inclient.redirect_uris. - Otherwise, set it to
client.redirect_uris[0]if available, or raise as currently done. - This replaces the existing
redirect_urivalidation and fallback logic.
- Initialize it from
- Replace later uses of
redirect_uriin redirects withsafe_redirect_uri, in particular:- At line 131, use
safe_redirect_uriinstead ofredirect_uri. - In the consent-page redirect (lines 151–158), use
safe_redirect_urifor theredirect_uriquery parameter in the URL.
- At line 131, use
- Do not introduce any new imports or change existing ones; only adjust variable handling and redirect construction in the shown function.
This keeps the functional behavior the same (still only allowing registered redirect URIs) but makes the trusted/untrusted boundary apparent to both readers and static analyzers.
| @@ -110,14 +110,15 @@ | ||
| ) | ||
| raise BadRequestException("Invalid client_id") | ||
|
|
||
| # Validate redirect_uri | ||
| if redirect_uri and redirect_uri not in client.redirect_uris: | ||
| raise BadRequestException(f"Invalid redirect_uri {redirect_uri}") | ||
| # Validate and normalize redirect_uri to a trusted value | ||
| if redirect_uri: | ||
| if redirect_uri not in client.redirect_uris: | ||
| raise BadRequestException(f"Invalid redirect_uri {redirect_uri}") | ||
| safe_redirect_uri = redirect_uri | ||
| else: | ||
| safe_redirect_uri = client.redirect_uris[0] if client.redirect_uris else None | ||
|
|
||
| if not redirect_uri: | ||
| redirect_uri = client.redirect_uris[0] if client.redirect_uris else None | ||
|
|
||
| if not redirect_uri: | ||
| if not safe_redirect_uri: | ||
| raise BadRequestException("No valid redirect_uri") | ||
|
|
||
| # Validate response_type | ||
| @@ -128,7 +128,7 @@ | ||
| } | ||
| if state: | ||
| error_params["state"] = state | ||
| return RedirectResponse(f"{redirect_uri}?{urlencode(error_params)}") | ||
| return RedirectResponse(f"{safe_redirect_uri}?{urlencode(error_params)}") | ||
|
|
||
| # If user is not authenticated, redirect to login | ||
| if not current_user: | ||
| @@ -151,7 +151,7 @@ | ||
| f"/consent?client_name={client.client_name}&" | ||
| f"client_id={client.client_id}&" | ||
| f"response_type={response_type}&" | ||
| f"redirect_uri={redirect_uri}&" | ||
| f"redirect_uri={safe_redirect_uri}&" | ||
| f"scope={scope}&" | ||
| f"state={state or ''}&" | ||
| f"code_challenge={code_challenge or ''}&" |
| f"/consent?client_name={client.client_name}&" | ||
| f"client_id={client.client_id}&" | ||
| f"response_type={response_type}&" | ||
| f"redirect_uri={redirect_uri}&" | ||
| f"scope={scope}&" | ||
| f"state={state or ''}&" | ||
| f"code_challenge={code_challenge or ''}&" | ||
| f"code_challenge_method={code_challenge_method or ''}" |
Check warning
Code scanning / CodeQL
URL redirection from remote source Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 months ago
General approach: ensure that any user-controlled values included in a redirect URL are safely encoded so they cannot alter the structure of the URL or introduce a new redirect target. In this case, that means treating all untrusted pieces as query parameter values and letting a URL encoder build the query string, instead of interpolating them directly into an f-string.
Best concrete fix here: before returning the redirect to /consent, build a dictionary of query parameters including both trusted and untrusted values, then use urllib.parse.urlencode (already imported at the top of the file) to encode them. This automatically escapes &, ?, #, whitespace, newlines, etc., preventing these characters from breaking the intended URL structure. The base path (/consent) remains hard-coded and unchanged, so functionality stays the same from the application’s perspective.
Changes needed in api/oauth/oauth_provider.py:
-
In
authorize_endpoint, in theelseblock where the comment says# redirect to user consent page(lines ~148–159), replace the f-string construction:return RedirectResponse( f"/consent?client_name={client.client_name}&" f"client_id={client.client_id}&" f"response_type={response_type}&" f"redirect_uri={redirect_uri}&" f"scope={scope}&" f"state={state or ''}&" f"code_challenge={code_challenge or ''}&" f"code_challenge_method={code_challenge_method or ''}" )
with code that:
-
Builds a
paramsdict:params = { "client_name": client.client_name, "client_id": client.client_id, "response_type": response_type, "redirect_uri": redirect_uri, "scope": scope, "state": state or "", "code_challenge": code_challenge or "", "code_challenge_method": code_challenge_method or "", }
-
Builds the redirect URL:
consent_url = "/consent?" + urlencode(params). -
Returns
RedirectResponse(consent_url).
-
No new imports are needed because urlencode is already imported and standard library–based. This fix addresses all variants of the alert because every tainted variable now flows through urlencode, which treats them as opaque values.
| @@ -147,16 +147,18 @@ | ||
| else: | ||
| # redirect to user consent page | ||
| # TODO: Implement user consent page in frontend | ||
| return RedirectResponse( | ||
| f"/consent?client_name={client.client_name}&" | ||
| f"client_id={client.client_id}&" | ||
| f"response_type={response_type}&" | ||
| f"redirect_uri={redirect_uri}&" | ||
| f"scope={scope}&" | ||
| f"state={state or ''}&" | ||
| f"code_challenge={code_challenge or ''}&" | ||
| f"code_challenge_method={code_challenge_method or ''}" | ||
| ) | ||
| params = { | ||
| "client_name": client.client_name, | ||
| "client_id": client.client_id, | ||
| "response_type": response_type, | ||
| "redirect_uri": redirect_uri, | ||
| "scope": scope, | ||
| "state": state or "", | ||
| "code_challenge": code_challenge or "", | ||
| "code_challenge_method": code_challenge_method or "", | ||
| } | ||
| consent_url = "/consent?" + urlencode(params) | ||
| return RedirectResponse(consent_url) | ||
|
|
||
|
|
||
| @router.post("/consent") |
| } | ||
| if state: | ||
| error_params["state"] = state | ||
| return RedirectResponse(f"{redirect_uri}?{urlencode(error_params)}") |
Check warning
Code scanning / CodeQL
URL redirection from remote source Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 months ago
In general, to fix untrusted URL redirection you must not redirect directly to user-supplied URLs. Instead, either (a) select among server-known safe URLs based on user input, or (b) validate the user-supplied URL to ensure it is a safe relative path (no scheme/host) or belongs to an allowed set of hosts/paths.
For this specific snippet, the minimal, behavior-preserving fix is to validate redirect_uri in consent_endpoint before using it to build the error redirect on line 185. A simple and robust approach that doesn’t depend on unseen project code is:
- Treat only relative URLs without scheme and host as safe; reject absolute URLs or those that look like protocol-relative or malformed external URLs.
- Normalize the input by removing backslashes (browsers may treat them as slashes) before parsing.
- Use
urllib.parse.urlparseto inspect the URL. Allow redirect only ifscheme,netlocare empty and the path starts with/(or at least does not start with//).
If the redirect_uri fails validation, fall back to redirecting to a safe local default (e.g., / or a fixed error page), instead of using the tainted value. This keeps existing functionality for legitimate relative redirect URIs and eliminates open redirects to attacker-controlled domains.
Concretely, in api/oauth/oauth_provider.py:
- Add an import for
urlparsefromurllib.parse(we already importurlencode). - Introduce a small helper function or inline logic in
consent_endpointthat:- Normalizes
redirect_uri(replace('\\', '/')). - Parses it with
urlparse. - Checks that
parsed.schemeandparsed.netlocare empty, and that it does not start with//.
- Normalizes
- Before constructing the error redirect URL, validate
redirect_uriand, if invalid, replace it with a safe default path before using it inRedirectResponse.
This requires only edits in the shown file: one import and some extra logic in consent_endpoint.
| @@ -1,7 +1,7 @@ | ||
| """OAuth provider endpoints for AYON Server.""" | ||
|
|
||
| from typing import Any | ||
| from urllib.parse import urlencode | ||
| from urllib.parse import urlencode, urlparse | ||
|
|
||
| from fastapi import Form, Query, Request | ||
| from fastapi.responses import RedirectResponse | ||
| @@ -182,8 +182,19 @@ | ||
| } | ||
| if state: | ||
| error_params["state"] = state | ||
| return RedirectResponse(f"{redirect_uri}?{urlencode(error_params)}") | ||
|
|
||
| # Validate redirect_uri to prevent open redirect vulnerabilities. | ||
| # Only allow relative URLs without scheme or netloc; otherwise, | ||
| # fall back to a safe local default. | ||
| safe_redirect_uri = redirect_uri or "/" | ||
| # Normalize backslashes which some browsers treat as path separators | ||
| normalized_target = safe_redirect_uri.replace("\\", "/") | ||
| parsed = urlparse(normalized_target) | ||
| if parsed.scheme or parsed.netloc or normalized_target.startswith("//"): | ||
| normalized_target = "/" | ||
|
|
||
| return RedirectResponse(f"{normalized_target}?{urlencode(error_params)}") | ||
|
|
||
| # User approved, generate authorization code | ||
| auth_url = ( | ||
| f"{request.url.scheme}://{request.url.netloc}/oauth/authorize?" |
| return { | ||
| "valid": False, | ||
| "error": str(e) | ||
| } |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 months ago
In general, to fix this kind of problem you should avoid returning raw exception text or stack traces to the client. Instead, log the full exception server-side (if needed for debugging) and send a generic, high-level error message to the caller that does not reveal internal implementation details.
For this specific endpoint in api/oauth/oauth_provider.py, the best low-impact fix is to:
- Keep the existing
try/exceptstructure. - In the
exceptblock, stop usingstr(e)in the response body. - Replace it with a generic message such as
"Invalid or expired token"or"Token validation failed", which preserves the meaning of the endpoint without leaking implementation details. - Optionally, if logging is available in this file, log the exception so developers can still see it; if no logger is shown in the snippet, we should not add logging that depends on unknown project conventions, so we will only adjust the response content.
Concretely:
- In
validate_jwt_endpoint(around lines 422–432), change theexceptblock so that"error"is set to a generic string and does not interpolatee. - No additional imports or helper methods are strictly required for this minimal fix.
| @@ -425,8 +425,8 @@ | ||
| "valid": True, | ||
| "claims": payload | ||
| } | ||
| except Exception as e: | ||
| except Exception: | ||
| return { | ||
| "valid": False, | ||
| "error": str(e) | ||
| "error": "Token validation failed" | ||
| } |
| user_name=current_user.name | ||
| ) | ||
|
|
||
| return RedirectResponse(redirect_url) |
Check warning
Code scanning / CodeQL
URL redirection from remote source Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 22 days ago
In general, the problem is that redirect_uri (and related query parameters) are taken from untrusted input and used to construct a redirect destination without checking that the URL is allowed or at least local/relative. To fix this, we should validate the redirect_uri (and any redirect target we use) before performing the redirect, either by: (a) checking against the client’s registered redirect URIs, or (b) enforcing that the URI is local/relative and has no scheme/host. Since we must not change external behavior more than necessary and we don’t see all client-registration logic, the safest minimal change is to ensure that the actual redirect URL returned to the browser is constrained to a relative or same-origin path unless the redirect_uri matches a pre-registered URI for the client retrieved from OAuthStorage.
The single best fix with minimal functional change is:
- In
OAuthServer.create_authorization_response, validate theredirect_urivalue extracted fromquery_params:- If it is not provided, fall back to the client’s default redirect URI (as stored in
client.redirect_uris), but only if that default is present. - If it is provided, check that it is either:
- exactly one of the client’s registered redirect URIs, or
- a relative URL (no scheme, no host) under the same application.
- If it fails validation, raise an OAuth error (
InvalidClientErroror other suitableOAuth2Error) instead of using it.
- If it is not provided, fall back to the client’s default redirect URI (as stored in
- If the
redirect_uriis accepted, still be defensive for local redirects: normalize it, strip backslashes, and useurlparseto ensure no unexpected scheme/host sneaks in (e.g., via malformed URLs browsers still interpret as absolute). - Continue using the (now validated)
redirect_urito buildredirect_url, so existing OAuth clients that are correctly configured continue to work.
This fix primarily touches ayon_server/oauth/server.py in the create_authorization_response method. We can reuse the already-imported urlparse from urllib.parse. We’ll add:
- A small helper inside
create_authorization_response(or inline logic) to:- Normalize
redirect_uri(replace backslashes, strip whitespace). - Parse it with
urlparseand checkschemeandnetloc. - Compare against
client.redirect_uriswhen a client is loaded.
- Normalize
- Additional logic around where
redirect_uriis extracted (line 210) to perform that validation and possibly fall back or error.
We do not need to change api/oauth/oauth_provider.py to validate there, because the main vulnerable redirect is built in OAuthServer.create_authorization_response; fixing it there addresses all the CodeQL variants in one place and ensures that any future caller of create_authorization_response is also safe.
| @@ -190,6 +190,7 @@ | ||
| query_params = parse_qs(parsed_uri.query) | ||
|
|
||
| client_id = query_params.get('client_id', [None])[0] | ||
| client = None | ||
| if client_id: | ||
| client = await OAuthStorage.get_client(client_id) | ||
| if client: | ||
| @@ -207,7 +208,37 @@ | ||
| # If user is authenticated, generate code | ||
| if user_name: | ||
| code = generate_token() | ||
| redirect_uri = query_params.get('redirect_uri', [None])[0] | ||
|
|
||
| # Determine and validate redirect_uri | ||
| raw_redirect_uri = query_params.get('redirect_uri', [None])[0] | ||
| redirect_uri = raw_redirect_uri | ||
|
|
||
| # If no redirect_uri was provided in the request, fall back to | ||
| # the client's default registered redirect URI (if available). | ||
| if not redirect_uri and client and getattr(client, "redirect_uris", None): | ||
| redirect_uri = client.redirect_uris[0] | ||
|
|
||
| if not redirect_uri: | ||
| # No redirect URI could be determined; treat as invalid client/request. | ||
| raise InvalidClientError("Missing redirect_uri") | ||
|
|
||
| # Normalise and validate redirect_uri to guard against open redirects. | ||
| # Replace backslashes which some browsers treat like forward slashes. | ||
| redirect_uri = redirect_uri.replace("\\", "").strip() | ||
| parsed_redirect = urlparse(redirect_uri) | ||
|
|
||
| # If a client is known, ensure the redirect_uri matches one of the | ||
| # client's registered redirect URIs. This is the primary protection | ||
| # against arbitrary redirects. | ||
| if client and getattr(client, "redirect_uris", None): | ||
| if redirect_uri not in client.redirect_uris: | ||
| raise InvalidClientError("Unregistered redirect_uri") | ||
| else: | ||
| # If no client or no registered URIs are available, only allow | ||
| # relative redirect URIs without scheme/host to avoid open redirects. | ||
| if parsed_redirect.scheme or parsed_redirect.netloc: | ||
| raise InvalidClientError("Invalid redirect_uri") | ||
|
|
||
| scope = query_params.get('scope', ['read'])[0] | ||
|
|
||
| # Save authorization code |
OAuth Provider Implementation for AYON Server
This implementation provides a basicOAuth 2.0 authorization server for AYON Server, allowing third-party applications to securely access user data through standardized OAuth processes. It is using oauthlib
Authorization Endpoints
GET /api/oauth/authorize- OAuth authorization endpointPOST /api/oauth/consent- User consent handlingPOST /api/oauth/token- Token exchange endpointPOST /api/oauth/introspect- Token introspection endpointGET /api/oauth/userinfo- User information endpointJWT Token Endpoints
POST /api/oauth/jwt- Generate JWT tokens (authenticated users)POST /api/oauth/jwt/exchange- Exchange OAuth tokens for JWT tokensGET /api/oauth/validate- Validate JWT tokensGET /.well-known/jwks.json- JSON Web Key SetClient Management (Admin Only)
GET /api/oauth/clients- List OAuth clientsPOST /api/oauth/clients- Create OAuth clientGET /api/oauth/clients/{client_id}- Get OAuth clientDELETE /api/oauth/clients/{client_id}- Delete OAuth clientDiscovery
GET /.well-known/openid_configuration- OpenID Connect DiscoveryNotes