-
-
Notifications
You must be signed in to change notification settings - Fork 0
Tx #37
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
base: dev
Are you sure you want to change the base?
Tx #37
Changes from all commits
dd6f802
4f56017
1f95adc
b3292fb
4936a0b
cdddb2b
ec8d48e
3b4044f
f2e13e0
9692b27
7ecb37e
c1d8ceb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -145,14 +145,24 @@ async def _handle_inbound(request: Request, explicit_channel: str) -> Response: | |
| "signature_validated": signature_validated, | ||
| } | ||
|
|
||
| # Opt-out handling (STOP): record directly if DB available, then acknowledge. | ||
| # Opt-out handling (STOP): record in DB first; only acknowledge if persisted (TCPA/CASL). | ||
| if body.upper() in _OPTOUT_KEYWORDS: | ||
| db = getattr(request.app.state, "db", None) | ||
| if db is not None and hasattr(db, "record_opt_out_by_phone"): | ||
| try: | ||
| await db.record_opt_out_by_phone(from_phone, channel) | ||
| except Exception as e: | ||
| logger.warning("Opt-out record failed (channel={} phone={}): {}", channel, from_phone, e) | ||
| if db is None or not hasattr(db, "record_opt_out_by_phone"): | ||
| raise HTTPException( | ||
| status_code=503, | ||
| detail="Opt-out not available; database not configured", | ||
| ) | ||
| try: | ||
| await db.record_opt_out_by_phone(from_phone, channel) | ||
| except Exception as e: | ||
| logger.error( | ||
| "Opt-out record failed (channel={} phone={}): {}", channel, from_phone, e | ||
| ) | ||
| raise HTTPException( | ||
| status_code=500, | ||
| detail="Opt-out could not be recorded; please try again", | ||
| ) from e | ||
|
Comment on lines
+148
to
+165
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. STOP opt-out silently lost on DB failure Returning a
This is arguably a TCPA/CASL regression from the previous code, which at least sent the user a confirmation message (even though the opt-out wasn't persisted). With the current approach, when the DB is degraded, there is no user-facing acknowledgment whatsoever and the opt-out record is lost. Consider implementing a durable retry mechanism (e.g., writing to a dead-letter queue or a fallback store) so opt-out requests survive transient DB outages, and always return a TwiML |
||
| return _twiml_response("You have been opted out. Reply START to re-subscribe.") | ||
|
|
||
| inbound = InboundMessage( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,7 @@ | |
| from radioshaq.constants import ASR_LANGUAGE_AUTO, ASR_LANGUAGE_VALUES | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| INSECURE_JWT_SECRETS = frozenset({"dev-secret", "dev-secret-change-in-production", "test"}) | ||
|
|
||
|
|
||
| class Mode(StrEnum): | ||
|
|
@@ -124,7 +125,7 @@ class DatabaseConfig(BaseModel): | |
|
|
||
| # PostgreSQL with PostGIS (default port 5434 matches docker-compose to avoid host 5432 conflict) | ||
| postgres_url: str = Field( | ||
| default="postgresql+asyncpg://radioshaq:radioshaq@127.0.0.1:5434/radioshaq", | ||
| default="postgresql+asyncpg://radioshaq:radioshaq@localhost:5434/radioshaq", | ||
| description="PostgreSQL connection URL with asyncpg driver", | ||
| ) | ||
|
Josephrp marked this conversation as resolved.
|
||
| postgres_pool_size: int = Field(default=10, ge=1, le=100) | ||
|
|
@@ -183,11 +184,11 @@ class JWTConfig(BaseModel): | |
| @field_validator("secret_key") | ||
| @classmethod | ||
| def validate_secret(cls, v: str) -> str: | ||
| """Warn about insecure secrets.""" | ||
| if v in ("dev-secret", "dev-secret-change-in-production", "test"): | ||
| # In production, this should raise an error | ||
| pass | ||
| return v | ||
| """Normalize and require a non-empty secret string.""" | ||
| value = (v or "").strip() | ||
| if not value: | ||
| raise ValueError("jwt.secret_key must not be empty") | ||
| return value | ||
|
|
||
|
|
||
| class LLMConfig(BaseModel): | ||
|
|
@@ -860,7 +861,17 @@ def expand_path(cls, v: str | Path) -> Path: | |
|
|
||
| @model_validator(mode="after") | ||
| def create_directories(self) -> Config: | ||
| """Ensure workspace and data directories exist.""" | ||
| """Ensure directories exist and block insecure runtime defaults.""" | ||
| secret = (self.jwt.secret_key or "").strip() | ||
| if secret in INSECURE_JWT_SECRETS: | ||
| if self.debug: | ||
| logger.warning( | ||
| "Using insecure jwt.secret_key in debug mode; set RADIOSHAQ_JWT__SECRET_KEY before production" | ||
| ) | ||
| else: | ||
| raise ValueError( | ||
| "Insecure jwt.secret_key configured. Set RADIOSHAQ_JWT__SECRET_KEY to a strong secret or enable debug for local development." | ||
| ) | ||
| self.workspace_dir.mkdir(parents=True, exist_ok=True) | ||
| self.data_dir.mkdir(parents=True, exist_ok=True) | ||
| return self | ||
|
Comment on lines
861
to
877
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.
Additionally, the |
||
|
|
||
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.
Breaking change:
publish_inboundnow requires JWT authAdding
_user: TokenPayload = Depends(get_current_user)to this endpoint is a good security improvement, but it is a breaking change for any Lambda function or external service that currently callsPOST /bus/inbound(or wherever this router is mounted) without a JWT token. Those callers will now receive401 Unauthorizedsilently.It would be worth documenting (in the PR description or changelog):