Conversation
…lly by vs code by default, btw
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| # Validate the redirect URL | ||
| redirect_url = redirect if is_safe_redirect(redirect) else "/" | ||
|
|
||
| response = RedirectResponse(url=redirect_url) |
Check warning
Code scanning / CodeQL
URL redirection from remote source Medium
This autofix suggestion was applied.
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
In general, to fix untrusted URL redirection you must strictly validate any user-provided URL before using it in a redirect. The safest pattern is to (a) only allow relative paths (no scheme, no host), or (b) compare against a whitelist of allowed full URLs/hosts, using a robust parser and normalization to avoid browser parsing quirks.
For this specific code, the best targeted fix is to harden the existing is_safe_redirect helper so that any redirect string coming from the user is normalized and then must either be a pure relative path (no scheme, no netloc) or an absolute URL whose host is explicitly allowed (taking into account subdomains and ports). We should also handle backslashes and malformed schemes as recommended in the background section. The logout handler already uses is_safe_redirect, so upgrading that function will fix the tainted flow without changing the external behavior for legitimate redirects.
Concretely, modify is_safe_redirect in api.civicpatch.org/src/routers/auth.py:
- Normalize the input:
- Strip leading/trailing whitespace.
- Replace backslashes (
\) with forward slashes (/) before parsing.
- Parse once with
urlparse. - Treat as safe “relative” only if both
parsed.schemeandparsed.netlocare empty. This mirrors the recommendedurlparseusage and prevents malformed absolute URLs likehttps:/evil.comorhttps:///evil.comfrom being misclassified as relative. - For absolute URLs, require:
parsed.schemeishttporhttps.- Host check is done against the hostname only (ignoring port), using
parsed.hostname; allow onlyALLOWED_HOSTSand specific subdomains of known base domains (e.g.,.civicpatch.org,.civicpatch.local).
- Optionally, reject URLs that do not start with
/when treated as relative (to avoid confusing “relative” values like//evil.comif parsing quirks occur).
No new imports are required; urlparse is already imported. All changes stay inside the shown is_safe_redirect function so that the rest of the code continues to call it as before.
| @@ -20,16 +20,49 @@ | ||
| ALLOWED_HOSTS = ["civicpatch.org", "civicpatch.local"] | ||
|
|
||
| def is_safe_redirect(url: str) -> bool: | ||
| """Check if the redirect URL is safe (same origin or allowed host).""" | ||
| """Check if the redirect URL is safe (relative URL or allowed host).""" | ||
| if not url: | ||
| return False | ||
| parsed = urlparse(url) | ||
| # Allow relative URLs | ||
| if not parsed.netloc: | ||
|
|
||
| # Normalize URL to handle browser quirks: | ||
| # - Replace backslashes, which some browsers treat like slashes. | ||
| # - Strip surrounding whitespace. | ||
| normalized_url = url.replace("\\", "/").strip() | ||
| if not normalized_url: | ||
| return False | ||
|
|
||
| parsed = urlparse(normalized_url) | ||
|
|
||
| # Allow only *relative* URLs with no scheme and no network location. | ||
| # This prevents malformed absolute URLs like "https:/example.com" | ||
| # from being treated as relative. | ||
| if not parsed.scheme and not parsed.netloc: | ||
| # Optionally, ensure the path starts with "/" to avoid confusing | ||
| # relative values like "///example.com". | ||
| if not parsed.path: | ||
| return False | ||
| if parsed.path.startswith("/"): | ||
| return True | ||
| # Paths that do not start with "/" can be considered unsafe here. | ||
| return False | ||
|
|
||
| # For absolute URLs, only allow specific hosts over HTTP(S). | ||
| if parsed.scheme not in ("http", "https"): | ||
| return False | ||
|
|
||
| hostname = parsed.hostname or "" | ||
| if not hostname: | ||
| return False | ||
|
|
||
| if hostname in ALLOWED_HOSTS: | ||
| return True | ||
| # Allow only specific hosts | ||
| return parsed.netloc in ALLOWED_HOSTS or parsed.netloc.endswith(".civicpatch.org") or parsed.netloc.endswith(".civicpatch.local") | ||
|
|
||
| # Allow subdomains of civicpatch.org and civicpatch.local | ||
| if hostname.endswith(".civicpatch.org") or hostname.endswith(".civicpatch.local"): | ||
| return True | ||
|
|
||
| return False | ||
|
|
||
| from jose import jwt | ||
|
|
||
| # https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/scopes-for-oauth-apps |
shelltr
committed this autofix suggestion 4 days ago.
| algorithm="HS256", | ||
| ) | ||
| response = RedirectResponse(url="/", status_code=302) | ||
| response = RedirectResponse(url=redirect_url, status_code=302) |
Check warning
Code scanning / CodeQL
URL redirection from remote source Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
In general, to fix untrusted URL redirection you must not use user input directly in redirect targets. Instead, either (a) restrict redirections to a server-maintained whitelist, or (b) validate that the URL is “local-only” (no scheme, no external host, no tricky backslashes) and fall back to a safe default (such as /) otherwise.
For this file, the most compatible fix is to define a local helper is_safe_redirect(target: str) -> bool that uses urllib.parse.urlparse (already imported) to validate redirect targets. A safe pattern is:
- Normalize backslashes (
\) to remove any browser quirks. - Parse via
urlparse. - Allow only URLs with:
- no scheme, and
- no netloc (host/port).
- Optionally, require them to start with
/or be relative paths, depending on existing behavior. Since the default values in this file are/, and we want minimal behavioral change, we’ll allow any relative URL or path that does not specify scheme or host.
Then:
- In
logout, keep the existing pattern but ensure it calls this new helper. - In
login_callback, keep usingstate if is_safe_redirect(state) else "/"so that unsafe values are discarded and replaced with/.
We will add the is_safe_redirect function near the top of auth.py, after the constants or imports. No external libraries are needed beyond the already imported urlparse. This makes the dataflow clear: untrusted state is passed through a recognized sanitizer before reaching RedirectResponse.
| @@ -13,6 +13,26 @@ | ||
|
|
||
| INSTANCE_URL = os.getenv("INSTANCE_URL", "https://api.civicpatch.local") | ||
| COOKIE_INSTANCE_URL = os.getenv("COOKIE_INSTANCE_URL", ".civicpatch.local") | ||
|
|
||
|
|
||
| def is_safe_redirect(target: str) -> bool: | ||
| """ | ||
| Return True if the given redirect target is a local/relative URL without | ||
| an explicit scheme or host, making it safe to use for redirects. | ||
| """ | ||
| if not target: | ||
| return False | ||
|
|
||
| # Remove backslashes to avoid browser quirks where '\' is treated as '/' | ||
| cleaned = target.replace("\\", "") | ||
| parsed = urlparse(cleaned) | ||
|
|
||
| # Disallow absolute URLs or URLs with an explicit host/scheme | ||
| if parsed.scheme or parsed.netloc: | ||
| return False | ||
|
|
||
| # At this point the URL is relative to our application; accept it | ||
| return True | ||
| GITHUB_CLIENT_ID = os.getenv("GITHUB_CLIENT_ID") | ||
| GITHUB_CLIENT_SECRET = os.getenv("GITHUB_CLIENT_SECRET") | ||
| GITHUB_CALLBACK_URL = f"{INSTANCE_URL}/api/v1/auth/github/callback" |
…emote source Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
1 participant
No description provided.