Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@greptileai comprehensively update your review based on the changes above : |
| # 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 |
There was a problem hiding this comment.
STOP opt-out silently lost on DB failure
Returning a 503 when the database is unavailable causes the STOP request to be silently dropped. Twilio does not automatically retry SMS webhooks by default on 5xx responses — it logs an error and moves on (a fallback URL must be explicitly configured in the Twilio console for retries). This means:
- User sends STOP
- DB is temporarily down → Twilio receives 503
- Twilio does not retry; the opt-out is never recorded
- User receives no TwiML acknowledgment
- User continues receiving messages, unaware the opt-out failed
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 200 to Twilio once the opt-out has been enqueued for reliable delivery.
| async def publish_inbound( | ||
| request: Request, | ||
| body: dict[str, Any] = Body(..., embed=False), |
There was a problem hiding this comment.
Breaking change: publish_inbound now requires JWT auth
Adding _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 calls POST /bus/inbound (or wherever this router is mounted) without a JWT token. Those callers will now receive 401 Unauthorized silently.
It would be worth documenting (in the PR description or changelog):
- Which callers are affected (e.g. AWS Lambda integrations)
- The migration path (how to obtain and pass a valid JWT)
- Whether a service-account token should be pre-generated and distributed to existing callers
|
|
||
| @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 |
There was a problem hiding this comment.
INSECURE_JWT_SECRETS check in create_directories conflates two concerns
create_directories validates directory creation and enforces a security policy on the JWT secret. If the secret check raises ValueError, the directories are never created, leaving workspace_dir and data_dir potentially absent. Consider reordering so directory creation happens first (or in a separate validator), and then raising on insecure secrets — or breaking these into two @model_validator methods with clearer names.
Additionally, the validate_secret field validator already strips and validates the key, so (self.jwt.secret_key or "").strip() here is redundant — self.jwt.secret_key is already stripped at that point.
1 participant
Greptile Summary
This PR (titled "Tx") covers a broad set of hardening and refactoring changes across the backend, configuration, and frontend layers. Key areas of change include: parameterizing Docker Compose Postgres credentials/ports, tightening JWT secret validation with a production guard, improving emergency approval error handling (proper 503s instead of misleading
ok:trueresponses), adding authentication to the previously-openpublish_inboundbus endpoint, removing theVITE_RADIOSHAQ_TOKENbuild-time env var in favour of runtime login, fixing a ReactuseEffectdependency onclearMarkers, and dropping the loguru stub in favour of the real package.Notable changes:
docker-compose.yml: Postgres credentials and ports are now fully overridable viaPOSTGRES_*env vars with defaults preserved — good DX improvement, but thehindsightservice's fallbackHINDSIGHT_API_DATABASE_URLis not updated to reference these vars, creating a silent mismatch when credentials are overriddentwilio.py: STOP/opt-out handling now returns503/500when the DB is unavailable instead of logging a warning and confirming. Because Twilio does not auto-retry SMS webhooks on 5xx, opt-out requests will be silently dropped (no DB record, no TwiML acknowledgment to the user) under any transient DB outage — a TCPA/CASL compliance riskbus.py: Addingget_current_userauth topublish_inboundis a security improvement but is a breaking change for any unauthenticated Lambda or service callerconfig/schema.py: Production insecure JWT secret guard and improved validator are good additions; thecreate_directoriesvalidator conflates directory creation with security enforcement, which can leave directories uncreated on validation failurepostgres_gis.py: Newcount_pending_coordination_eventsis clean and efficientOperatorMapdependency fix are correctConfidence Score: 3/5
radioshaq/radioshaq/api/routes/twilio.py(opt-out compliance) andradioshaq/infrastructure/local/docker-compose.yml(Hindsight credential mismatch)Important Files Changed
Sequence Diagram
sequenceDiagram participant U as User (SMS) participant T as Twilio participant API as RadioShaq API participant DB as PostgreSQL Note over API: Inbound STOP message flow (new behavior) U->>T: Sends "STOP" T->>API: POST /twilio/sms (webhook) API->>DB: record_opt_out_by_phone() alt DB available DB-->>API: success API-->>T: 200 TwiML "You have been opted out" T-->>U: Confirmation SMS else DB unavailable DB-->>API: exception / None API-->>T: 500 / 503 HTTP error Note over T: Twilio logs error,<br/>no auto-retry for SMS T--xU: No confirmation sent Note over DB: Opt-out record LOST end Note over API: Emergency approval flow (new behavior) participant Op as Operator participant MB as Message Bus Op->>API: POST /emergency/events/{id}/approve API->>DB: claim_emergency_event_pending() DB-->>API: claimed API->>MB: publish_outbound() alt Bus available & queue not full MB-->>API: ok=True API->>DB: update status=approved API-->>Op: 200 {ok: true, queued: true} else Bus unavailable or queue full MB-->>API: error / ok=False API->>DB: rollback status=pending API-->>Op: 503 HTTPException endComments Outside Diff (4)
radioshaq/web-interface/src/locales/es.json, line 25-46 (link)Missing i18n keys in Spanish locale
The
en.jsonlocale addsmap.setLocation,map.latInvalid,map.lngInvalid,emergency.mapTitle, andemergency.viewOnMapkeys, but these are missing fromes.json. TheCallsignsPage"Set location" dialog and theEmergencyPagemap section use these keys with?? 'fallback'fallbacks, so they'll show English text to Spanish users instead of translated text.Add the missing keys to the
mapsection:And to the
emergencysection:radioshaq/web-interface/src/locales/fr.json, line 25-46 (link)Missing i18n keys in French locale
Same issue as
es.json— thefr.jsonlocale is missingmap.setLocation,map.latInvalid,map.lngInvalid,emergency.mapTitle, andemergency.viewOnMapkeys that were added toen.json. French users will see English fallback text for the "Set location" dialog and the emergency map section.radioshaq/web-interface/src/components/maps/OperatorMap.tsx, line 133 (link)Missing
clearMarkersinuseEffectdependency arrayThe
useEffectat line 92 usesclearMarkers(defined viauseCallback) andcenterRef/zoomRefrefs, but its dependency array is[]. React's exhaustive-deps rule would flag this. While refs won't cause stale-closure issues,clearMarkersis a dependency and should be listed. In practice this likely won't cause bugs here (sinceclearMarkersis stable), but it's best to include it for correctness:radioshaq/infrastructure/local/docker-compose.yml, line 166 (link)Hindsight default DB URL not parameterized with
POSTGRES_*overridesThis PR parameterizes the main Postgres service credentials via
POSTGRES_USER,POSTGRES_PASSWORD, andPOSTGRES_DB, but the Hindsight service's fallbackHINDSIGHT_API_DATABASE_URLstill hard-codes the original default credentials. If a user overridesPOSTGRES_USERorPOSTGRES_PASSWORD, the Hindsight service will fail to authenticate against Postgres at startup, because its default connection URL still points to the original values.Docker Compose does not support nested variable interpolation within a single value, so this can't easily be a single-line fix. The recommended mitigation is to explicitly document in
configuration.md(and the docker-compose comments) that users who overridePOSTGRES_*credentials must also setHINDSIGHT_API_DATABASE_URLto match — otherwise the Hindsight service will silently fail to connect.Last reviewed commit: c1d8ceb