Skip to content

feat: tts proxy to forward request to openai tts server#2030

Open
YiminW wants to merge 2 commits intomainfrom
dev/openai_proxy
Open

feat: tts proxy to forward request to openai tts server#2030
YiminW wants to merge 2 commits intomainfrom
dev/openai_proxy

Conversation

@YiminW
Copy link
Contributor

@YiminW YiminW commented Feb 2, 2026

No description provided.

@@ -0,0 +1,3 @@
"""

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

建议整体放到 openai_tts2_python/examples/openai_tts_proxy 目录下

@claude
Copy link

claude bot commented Feb 3, 2026

Code Review: TTS Proxy Server

Thank you for this contribution! Overall, this is a well-structured HTTP proxy implementation. Below are my findings across code quality, potential issues, performance, security, and test coverage.


✅ Strengths

  1. Good separation of concerns - The proxy logic is cleanly separated into reusable functions
  2. Comprehensive logging - Excellent request/response logging with sensitive data masking
  3. Proper resource management - Uses context managers for httpx client lifecycle
  4. Connection pooling - HTTP/2 support with proper connection limits
  5. Error handling - Handles various error scenarios gracefully
  6. Documentation - Clear README with usage examples

🔴 Critical Issues

1. Security: API Key Exposure Risk

Location: proxy_server.py:97-98

The proxy forwards the Authorization header from environment variables if not present in requests. This could expose API keys in logs or to unauthorized clients.

Recommendation:

  • Remove the fallback to OPENAI_API_KEY environment variable
  • Require clients to always provide Authorization headers
  • Add explicit validation that Authorization header exists before forwarding
# Current code (line 97-98)
if "authorization" not in headers and OPENAI_API_KEY:
    headers["authorization"] = f"Bearer {OPENAI_API_KEY}"

# Recommended approach
if "authorization" not in headers:
    logger.error("Missing Authorization header in request")
    return Response(
        content="Authorization header is required",
        status_code=401,
    )

2. Security: No Input Validation

Location: proxy_server.py:63-68

The proxy accepts and forwards any request body without validation. This could be exploited to:

  • Send malformed requests to OpenAI
  • Potentially bypass rate limits
  • Inject malicious payloads

Recommendation:
Add basic validation for expected fields (model, input, voice, etc.) and reject invalid requests early.

3. Resource Leak: Stream Context Not Always Cleaned Up

Location: proxy_server.py:118-123

If an exception occurs between __aenter__ and the try block, the stream context won't be properly closed.

Recommendation:

async def _proxy_to_openai(request: Request):
    stream_context = None
    try:
        # ... existing code ...
        stream_context = http_client.stream(...)
        response = await stream_context.__aenter__()
        # ... rest of logic ...
    except Exception as e:
        logger.error(f"Error: {e}")
        raise
    finally:
        if stream_context:
            try:
                await stream_context.__aexit__(None, None, None)
            except Exception:
                pass

⚠️ Important Issues

4. Performance: Excessive Logging in Production

Location: proxy_server.py:72-88, 177-185

Logging every chunk and full request/response bodies will significantly impact performance and fill disk space in production.

Recommendation:

  • Use debug level for verbose logging
  • Add environment variable to control log verbosity
  • Consider sampling (log 1 in N requests)
LOG_LEVEL = os.getenv("LOG_LEVEL", "INFO")
VERBOSE_LOGGING = os.getenv("VERBOSE_LOGGING", "false").lower() == "true"

if VERBOSE_LOGGING:
    logger.debug(f"Body (JSON): {json.dumps(body_json_log, indent=2)}")

5. Error Handling: Silent Exception Swallowing

Location: proxy_server.py:187-196

The code catches StreamClosed exceptions but only logs a debug message. This could hide real connection issues.

Recommendation:
Log at warning level and include metrics for monitoring.

6. Code Quality: Line Length Violations

Location: Multiple lines throughout

The project's .pylintrc specifies max-line-length=80, but many lines exceed this (e.g., lines 90-105, 155-165).

Recommendation:
Break long lines following the project's style guide.


💡 Suggestions

7. Missing Type Hints

Location: proxy_server.py:63, 245, 254

Functions lack return type annotations.

async def _proxy_to_openai(request: Request) -> Response:
async def proxy_audio_speech(request: Request) -> Response:
async def proxy_root(request: Request) -> Response:

8. Hardcoded Values

Location: proxy_server.py:32-34, 178-179

Magic numbers should be constants:

HTTP_TIMEOUT = 60.0
MAX_CONNECTIONS = 100
MAX_KEEPALIVE_CONNECTIONS = 20
KEEPALIVE_EXPIRY = 600.0
CHUNK_LOG_INTERVAL = 10

9. Inconsistent Error Responses

Location: proxy_server.py:221-237

Different exception types return different error response formats. Standardize error responses.

10. Missing Health Check Details

Location: proxy_server.py:265-267

The health check should verify the httpx client is operational and can reach OpenAI.

@app.get("/health")
async def health_check():
    try:
        # Verify client is initialized
        if not http_client:
            return {"status": "error", "message": "HTTP client not initialized"}
        return {
            "status": "ok", 
            "service": "openai-tts-proxy",
            "openai_base_url": OPENAI_BASE_URL
        }
    except Exception as e:
        return {"status": "error", "message": str(e)}

🧪 Test Coverage

CRITICAL: This PR adds no tests.

Required tests:

  1. Unit tests for _proxy_to_openai function
  2. Integration tests with mocked OpenAI responses
  3. Error scenario tests (401, 500, timeout, connection errors)
  4. Stream interruption handling tests
  5. Authorization header validation tests
  6. Health check endpoint tests

Recommendation:
Add a tests/ directory with pytest tests following the pattern used in other extensions (e.g., ai_agents/agents/ten_packages/extension/openai_tts2_python/tests/).


📦 Dependencies

11. Missing Version Pins

Location: requirements.txt:1-3

The requirements use >= which could introduce breaking changes.

Recommendation:

fastapi>=0.104.0,<0.115.0
uvicorn[standard]>=0.24.0,<0.30.0
httpx>=0.25.0,<0.28.0

🔐 Security Summary

  1. High: Remove API key fallback mechanism
  2. Medium: Add input validation
  3. Medium: Add rate limiting to prevent abuse
  4. Low: Consider adding request size limits

🎯 Recommendations Priority

Must Fix Before Merge:

  • Add tests
  • Fix API key exposure issue
  • Add input validation
  • Fix resource leak potential
  • Follow line length conventions

Should Fix:

  • Reduce logging verbosity
  • Add type hints
  • Standardize error responses
  • Pin dependency versions

Nice to Have:

  • Extract magic numbers to constants
  • Enhance health check
  • Add request metrics/monitoring

Overall, this is a solid foundation for a proxy server. The main blockers are the lack of tests and the security concerns around API key handling. Once those are addressed, this will be a valuable addition to the project.

Please let me know if you have any questions about these recommendations!

@claude
Copy link

claude bot commented Feb 3, 2026

PR Review: TTS Proxy to Forward Request to OpenAI TTS Server

Summary

This PR adds an OpenAI TTS proxy server that forwards requests from TTS clients to the OpenAI API. The proxy is implemented as a FastAPI application and is located in the examples folder of the openai_tts2_python extension.

Code Quality & Best Practices

✅ Strengths

  1. Well-structured logging: Comprehensive logging throughout the proxy with clear separators and detailed request/response information
  2. Proper async patterns: Correct use of async/await and context managers for HTTP client lifecycle
  3. Good documentation: README is clear and explains usage well
  4. Streaming architecture: Proper implementation of streaming response handling

⚠️ Issues & Recommendations

1. Security Concerns - API Key Logging (HIGH PRIORITY)

Location: proxy_server.py:82-108

The code logs request bodies which may contain API keys. While there is masking for api_key in the body, Authorization headers are logged with partial masking that may expose sensitive data (first/last 10 characters).

Recommendation:

  • Consider using shorter prefixes for masked data (e.g., 4 chars instead of 10)
  • For production use, reduce logging verbosity or add a configuration flag
  • Consider encryption pattern from codebase standards

2. Error Handling - Stream Context Management (MEDIUM PRIORITY)

Location: proxy_server.py:114-147

The stream context management manually calls __aenter__/__aexit__, which is fragile.

Recommendation: Use proper async context manager pattern instead of manual context management for better cleanup guarantees.

3. HTTP/2 Configuration (LOW PRIORITY)

Location: proxy_server.py:38

HTTP/2 is enabled but may not be necessary for this simple proxy use case.

Recommendation: Test with and without HTTP/2, or make it configurable.

4. Missing Input Validation (MEDIUM PRIORITY)

The proxy doesn't validate request body size limits, content-type headers, or required fields.

Recommendation: Add basic validation for content type and request size limits.

5. Resource Cleanup (MEDIUM PRIORITY)

Location: proxy_server.py:205-222

Errors during stream cleanup are silently caught without logging.

Recommendation: Log exceptions instead of silently catching them.

Performance Considerations

✅ Good Practices

  • Connection pooling configured correctly
  • Keepalive settings are reasonable
  • Streaming responses avoid buffering

⚠️ Potential Issues

  1. Max connections: 100 may be high for a single-instance proxy
  2. Timeout: 60s timeout might be too long for TTS requests
  3. No rate limiting: Could lead to API quota exhaustion

Testing

❌ Missing Test Coverage

This PR adds 416 lines of new code but no tests. The parent extension has comprehensive tests, but the proxy has none.

Recommendation: Add at least:

  1. Unit tests for header forwarding logic
  2. Integration tests for streaming behavior
  3. Tests for error handling

Documentation

✅ Strengths

  • Clear README with usage examples
  • Good inline comments

⚠️ Improvements Needed

  1. Document security implications of request logging
  2. Clarify production readiness
  3. Expand Docker usage documentation

Summary of Priority Issues

Priority Issue Location
🔴 HIGH API key logging security proxy_server.py:82-108
🟡 MEDIUM Stream context management proxy_server.py:114-147
🟡 MEDIUM Missing input validation Throughout
🟡 MEDIUM Silent error swallowing proxy_server.py:205-222
🟡 MEDIUM No test coverage Entire PR
🟢 LOW HTTP/2 configuration proxy_server.py:38

Recommendations

Must Fix Before Merge

  1. Improve sensitive data masking in logs
  2. Add proper async context manager usage
  3. Add at least basic test coverage

Should Fix

  1. Add input validation
  2. Log exceptions instead of silently catching them
  3. Clarify production readiness in documentation

Consider

  1. Make HTTP/2 configurable
  2. Add rate limiting
  3. Reduce logging verbosity or make it configurable

Conclusion

This is a useful addition to the TEN framework that provides a practical solution for forwarding TTS requests to OpenAI. The code is generally well-written and follows good async patterns. However, there are security concerns around logging, missing test coverage, and some error handling improvements needed before this should be merged.

Overall Assessment: Approve with changes requested ✋

Please address the high-priority security issues and add basic test coverage before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants