-
Notifications
You must be signed in to change notification settings - Fork 0
fix(security): remediate 2026-07-11 codebase security audit findings #623
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -34,7 +34,9 @@ | |||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Single-word and short-phrase affirmatives. Whole-word matching against the | ||||||||||||||||||||||||||||||||||
| # single-word tokens; substring presence is sufficient for the short phrases. | ||||||||||||||||||||||||||||||||||
| # single-word tokens; the short phrases match only when they LEAD the message | ||||||||||||||||||||||||||||||||||
| # (see ``_LEADING_AFFIRMATIVE_PHRASE``) so a mid-sentence occurrence in a | ||||||||||||||||||||||||||||||||||
| # non-authorizing reply does not count. | ||||||||||||||||||||||||||||||||||
| _AFFIRMATIVE_TOKENS: frozenset[str] = frozenset( | ||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||
| "yes", | ||||||||||||||||||||||||||||||||||
|
|
@@ -56,6 +58,17 @@ | |||||||||||||||||||||||||||||||||
| "ship it", | ||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| #: Regex matching an affirmative phrase only when it *leads* the message (after | ||||||||||||||||||||||||||||||||||
| #: optional leading punctuation/whitespace). Anchoring kills the false positive | ||||||||||||||||||||||||||||||||||
| #: where a phrase appears mid-sentence in a non-authorizing reply — e.g. | ||||||||||||||||||||||||||||||||||
| #: "maybe do it tomorrow" or "I'll do it later" contain "do it" as a substring | ||||||||||||||||||||||||||||||||||
| #: but are not authorizations. A leading "go ahead" / "do it" / "ship it" is a | ||||||||||||||||||||||||||||||||||
| #: genuine go-ahead. Built from :data:`_AFFIRMATIVE_PHRASES` so the two never drift. | ||||||||||||||||||||||||||||||||||
| _LEADING_AFFIRMATIVE_PHRASE = re.compile( | ||||||||||||||||||||||||||||||||||
| r"^\W*(?:" + "|".join(re.escape(p) for p in _AFFIRMATIVE_PHRASES) + r")\b", | ||||||||||||||||||||||||||||||||||
| re.IGNORECASE, | ||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Negation tokens that, if present, disqualify the message from being | ||||||||||||||||||||||||||||||||||
| # treated as affirmative — even if it also contains an affirmative token | ||||||||||||||||||||||||||||||||||
|
|
@@ -77,9 +90,29 @@ | |||||||||||||||||||||||||||||||||
| "wait", | ||||||||||||||||||||||||||||||||||
| "nope", | ||||||||||||||||||||||||||||||||||
| "never", | ||||||||||||||||||||||||||||||||||
| # Deferral / inability tokens — a reply that contains these is not an | ||||||||||||||||||||||||||||||||||
| # authorization even if it also carries an affirmative token ("ok" in | ||||||||||||||||||||||||||||||||||
| # "ok thanks", "do it" in "do it later"). These are whole-word matched | ||||||||||||||||||||||||||||||||||
| # against the [a-z]+ token set, so they never fire inside a larger word | ||||||||||||||||||||||||||||||||||
| # (e.g. "cant" does NOT match "significant"). The apostrophe contraction | ||||||||||||||||||||||||||||||||||
| # forms "can't" / "couldn't" are handled separately in is_affirmative | ||||||||||||||||||||||||||||||||||
| # (the [a-z]+ split turns them into "can"+"t" / "couldn"+"t", so bare | ||||||||||||||||||||||||||||||||||
| # "can"/"couldn" are intentionally NOT here — they would false-reject | ||||||||||||||||||||||||||||||||||
| # valid affirmatives like "yes you can" / "I can confirm"). | ||||||||||||||||||||||||||||||||||
| "cant", | ||||||||||||||||||||||||||||||||||
| "cannot", | ||||||||||||||||||||||||||||||||||
| "couldnt", | ||||||||||||||||||||||||||||||||||
| "maybe", | ||||||||||||||||||||||||||||||||||
| "later", | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| #: Apostrophe-form inability contractions. Checked as substrings of the | ||||||||||||||||||||||||||||||||||
| #: lowercased (apostrophe-preserving) text because the ``[a-z]+`` tokenizer | ||||||||||||||||||||||||||||||||||
| #: splits them; the apostrophe makes the substring match safe (no common word | ||||||||||||||||||||||||||||||||||
| #: contains "can't" / "couldn't"). | ||||||||||||||||||||||||||||||||||
| _NEGATION_CONTRACTIONS: tuple[str, ...] = ("can't", "couldn't") | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| def is_affirmative(text: str) -> bool: | ||||||||||||||||||||||||||||||||||
| """Return ``True`` if ``text`` reads as user affirmation of a mutating action. | ||||||||||||||||||||||||||||||||||
|
|
@@ -94,10 +127,15 @@ def is_affirmative(text: str) -> bool: | |||||||||||||||||||||||||||||||||
| if not text: | ||||||||||||||||||||||||||||||||||
| return False | ||||||||||||||||||||||||||||||||||
| lowered = text.lower() | ||||||||||||||||||||||||||||||||||
| if any(neg in lowered for neg in _NEGATION_CONTRACTIONS): | ||||||||||||||||||||||||||||||||||
| return False | ||||||||||||||||||||||||||||||||||
| words = set(re.findall(r"[a-z]+", lowered)) | ||||||||||||||||||||||||||||||||||
| if _NEGATION_TOKENS & words: | ||||||||||||||||||||||||||||||||||
| return False | ||||||||||||||||||||||||||||||||||
| for phrase in _AFFIRMATIVE_PHRASES: | ||||||||||||||||||||||||||||||||||
| if phrase in lowered: | ||||||||||||||||||||||||||||||||||
| return True | ||||||||||||||||||||||||||||||||||
| # Affirmative *phrases* must LEAD the message (anchored) so a mid-sentence | ||||||||||||||||||||||||||||||||||
| # "do it" in "I'll do it later" doesn't unlock dispatch — the leading | ||||||||||||||||||||||||||||||||||
| # negation/deferral tokens above already reject most of those, this closes | ||||||||||||||||||||||||||||||||||
| # the residual where no negation token is present. | ||||||||||||||||||||||||||||||||||
| if _LEADING_AFFIRMATIVE_PHRASE.match(lowered): | ||||||||||||||||||||||||||||||||||
| return True | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+135
to
+140
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To support removing "can" and "couldn" from _NEGATION_TOKENS (preventing false negatives on valid confirmations), we can explicitly check for "can't" and "couldn't" contractions in is_affirmative before evaluating other rules.
Suggested change
|
||||||||||||||||||||||||||||||||||
| return bool(_AFFIRMATIVE_TOKENS & words) | ||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,6 +89,34 @@ async def lifespan(_app: FastAPI) -> AsyncIterator[None]: | |
| event_type="github_token_file_deprecated", | ||
| ) | ||
|
|
||
| # Hardened-posture guards (security audit 2026-07-11 findings #4/#5). | ||
| # ENVIRONMENT defaults to "development", which UN-gates the destructive, | ||
| # unauthenticated /api/v1/_test/* endpoints (hard-DELETE + demo reseed). | ||
| # That is correct for a laptop/CI host but a footgun if such an instance is | ||
| # ever exposed to an untrusted network before the auth surface lands, so we | ||
| # emit a one-line boot reminder. | ||
| if settings.environment == "development": | ||
| logger.warning( | ||
| "ENVIRONMENT=development: destructive, unauthenticated /api/v1/_test/* " | ||
| "endpoints (hard-delete + demo reseed) are ENABLED. Only run this " | ||
| "instance on a trusted local/CI host — never expose it to an untrusted " | ||
| "network. Set ENVIRONMENT=staging|production to disable them.", | ||
| event_type="dev_test_endpoints_enabled", | ||
| ) | ||
|
|
||
| # The cluster base_url SSRF guard is a no-op while RELYLOOP_ALLOW_PRIVATE_ | ||
| # CLUSTERS is True (the laptop-friendly default). On a non-development | ||
| # deployment that default means ZERO SSRF protection (internal hosts + cloud | ||
| # metadata IPs are registerable), so warn loudly to flip it. | ||
| if settings.relyloop_allow_private_clusters and settings.environment != "development": | ||
| logger.warning( | ||
| "RELYLOOP_ALLOW_PRIVATE_CLUSTERS=True on a non-development deployment " | ||
| f"(ENVIRONMENT={settings.environment}): the cluster base_url SSRF guard " | ||
| "is disabled, so internal/metadata endpoints can be registered and " | ||
| "probed. Set RELYLOOP_ALLOW_PRIVATE_CLUSTERS=False to enable the guard.", | ||
| event_type="ssrf_guard_disabled_non_dev", | ||
| ) | ||
|
Comment on lines
+112
to
+118
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using %s formatting with structlog loggers is discouraged and may not format correctly depending on the processor configuration. It is cleaner and safer to use an f-string or pass the environment as a structured keyword argument. logger.warning(
f"RELYLOOP_ALLOW_PRIVATE_CLUSTERS=True on a non-development deployment "
f"(ENVIRONMENT={settings.environment}): the cluster base_url SSRF guard is disabled, so "
f"internal/metadata endpoints can be registered and probed. Set "
f"RELYLOOP_ALLOW_PRIVATE_CLUSTERS=False to enable the guard.",
event_type="ssrf_guard_disabled_non_dev",
) |
||
|
|
||
| redis_client: Redis = Redis.from_url(settings.redis_url, decode_responses=False) | ||
| cap_task = asyncio.create_task( | ||
| run_capability_check_background( | ||
|
|
@@ -193,11 +221,25 @@ async def lifespan(_app: FastAPI) -> AsyncIterator[None]: | |
| # (comma-separated). Empty string disables. MVP1 default covers the local Next | ||
| # dev server; operators add production origins at MVP3. | ||
| _cors_origins = [o.strip() for o in get_settings().cors_allow_origins.split(",") if o.strip()] | ||
| # Security audit 2026-07-11 finding #10: a wildcard origin combined with | ||
| # allow_credentials=True is unsafe — Starlette reflects the request Origin | ||
| # (rather than sending a literal "*"), which lets ANY site make credentialed | ||
| # cross-origin requests. Disable credentials whenever a wildcard is configured; | ||
| # MVP1 has no cookies/auth so nothing depends on credentialed CORS today. | ||
| _cors_has_wildcard = "*" in _cors_origins | ||
| _cors_allow_credentials = not _cors_has_wildcard | ||
| if _cors_has_wildcard: | ||
| logger.warning( | ||
| "CORS_ALLOW_ORIGINS contains '*'; disabling allow_credentials to avoid " | ||
| "reflecting arbitrary origins with credentials. Configure explicit " | ||
| "origins if you need credentialed CORS.", | ||
| event_type="cors_wildcard_credentials_disabled", | ||
| ) | ||
| if _cors_origins: | ||
| app.add_middleware( | ||
| CORSMiddleware, | ||
| allow_origins=_cors_origins, | ||
| allow_credentials=True, | ||
| allow_credentials=_cors_allow_credentials, | ||
| allow_methods=["GET", "POST", "PATCH", "DELETE", "OPTIONS"], | ||
| # Browsers ALWAYS strip custom headers from the request preflight unless | ||
| # they're explicitly allowed. The UI's api-client.ts injects X-Request-ID | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Including "can" and "couldn" in _NEGATION_TOKENS causes false negatives for common, valid affirmative responses such as "yes you can" or "I can confirm". Since the [a-z]+ regex splits contractions like "can't" and "couldn't" into separate words, we should remove "can" and "couldn" from the token set and instead handle these contractions explicitly in is_affirmative.