Skip to content

Conversation

@dguido
Copy link
Member

@dguido dguido commented Dec 1, 2025

Summary

Implements #196 - a local web application that allows users to deploy Algo VPN servers through a browser-based GUI.

  • Single-step launch: ./algo-web opens browser automatically
  • Cloud provider support: DigitalOcean, Hetzner, Linode, Vultr (MVP scope)
  • Real-time credential validation via provider APIs
  • Live deployment progress streaming via Server-Sent Events
  • Config file downloads (individual files + ZIP archive)

Screenshots

The UI uses a "Luxury Terminal" dark theme with the official Algo VPN and Trail of Bits branding:

Web UI Screenshot

Technical Details

Stack:

  • Starlette + uvicorn (ASGI web framework)
  • htmx for frontend interactivity (no build step, 14KB)
  • Server-Sent Events for streaming Ansible output
  • In-memory session management with auto-expiry

Security:

  • Credentials passed to Ansible via --extra-vars JSON (never persisted to disk)
  • Credentials validated against provider APIs before deployment
  • Session tokens are cryptographically random UUIDs

Usage

./algo-web
# Opens http://127.0.0.1:8080 in browser

Test Plan

  • Run ./algo-web and verify browser opens
  • Select each provider and verify form fields appear
  • Enter invalid credentials and verify error message
  • Enter valid credentials and verify regions load
  • Start deployment and verify progress streaming
  • Verify config downloads work after successful deployment

🤖 Generated with Claude Code

Implements issue #196 - a local web application that allows users to
deploy Algo VPN servers through a browser-based GUI.

Features:
- Single-step launch: `./algo-web` opens browser automatically
- Cloud provider support: DigitalOcean, Hetzner, Linode, Vultr (MVP)
- Real-time credential validation via provider APIs
- Dynamic region loading after validation
- Live deployment progress streaming via SSE
- Terminal-style output with color-coded log lines
- Config file downloads (individual + ZIP archive)

Technical stack:
- Starlette + uvicorn (ASGI web framework)
- htmx for frontend interactivity (no build step)
- Server-Sent Events for streaming Ansible output
- Credentials passed via --extra-vars (never persisted)

Design:
- "Luxury Terminal" dark theme with electric teal accents
- Algo VPN and Trail of Bits branding
- Responsive layout (desktop, tablet, mobile)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@dguido dguido requested a review from jackivanov as a code owner December 1, 2025 05:42
@claude
Copy link
Contributor

claude bot commented Dec 1, 2025

Code Review - Web UI for VPN Deployment

This is a well-architected feature with good security practices. Here are the key findings:

Critical Issues

1. Credentials exposed in HTML (app/templates/partials/regions.html:18-20)
API tokens are embedded as hidden form fields visible in browser DevTools. Store credentials server-side in the session instead of passing them through HTML.

2. No CSRF protection
POST endpoints lack CSRF tokens. Add CSRF middleware or same-origin checks.

3. Rate limiting missing
No rate limiting on credential validation endpoints allows brute-force attacks.

High Priority

4. Session cleanup (app/sessions.py:101-105)
Passive cleanup only during create() could lead to memory accumulation.

5. No authentication
If ALGO_WEB_HOST=0.0.0.0, anyone on network can access. Document security implications.

6. Unbounded output storage (app/sessions.py:42-44)
output_lines grows indefinitely. Add max size limit.

Medium Priority

7. Missing input validation (app/main.py:174-188)
Users and server_name aren't validated before passing to Ansible.

8. ZIP bomb vulnerability (app/main.py:308-312)
No size limit on ZIP creation. Add 100MB limit.

9. Process orphaning (app/runner.py:171-193)
Server crash during deployment leaves Ansible running. Add shutdown cleanup.

10. Incomplete Vultr implementation (app/runner.py:42-43)
Comment mentions config file but not implemented.

Minor Issues

  • Hardcoded timeouts (15s validation, 10s cancellation)
  • No structured logging
  • No health check endpoint
  • No tests for this major feature

Recommendations

Before merge (required):

  1. Fix credential exposure in HTML - CRITICAL
  2. Add input validation for users and server_name
  3. Add basic tests

Follow-up PRs:

  • CSRF protection and rate limiting
  • Logging and monitoring
  • Vultr implementation

Summary

Excellent architecture with proper subprocess handling and path traversal protection. The credential exposure issue is critical and must be fixed before merge. Adding tests is essential for a feature handling sensitive data.

Assessment: Approve with required changes (fix issue 1, add tests)

Remove Google Fonts dependency to ensure the web UI works
completely offline. Uses high-quality system font stacks:
- Mono: ui-monospace, SF Mono, Menlo, Consolas
- Sans: system-ui, Segoe UI, Roboto, Helvetica Neue

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@dguido dguido mentioned this pull request Dec 1, 2025
19 tasks
@claude
Copy link
Contributor

claude bot commented Dec 1, 2025

Code Review: Web UI for VPN Deployment

This is a well-structured feature implementation with good separation of concerns. However, there are critical security issues that must be addressed before merge.

Critical Security Issues

1. Credentials Exposed in HTML (app/templates/partials/regions.html:18-20) - MUST FIX BEFORE MERGE

API tokens are embedded as hidden form fields, making them visible in Browser DevTools, page source, and network inspector. Store credentials server-side in the session after validation instead.

2. No CSRF Protection - POST endpoints lack CSRF tokens. Add CSRF middleware.

3. No Rate Limiting - The validation endpoint has no rate limiting, allowing credential brute-forcing and DoS attacks.

High Priority Issues

4. Unbounded Output Storage (app/sessions.py:42-44) - output_lines grows indefinitely. Add max size limit.

5. No Input Validation (app/main.py:174-188) - users and server_name not validated before passing to Ansible. Add regex validation.

6. ZIP Bomb Vulnerability (app/main.py:308-312) - No size limit on ZIP creation. Add 100MB limit.

7. No Authentication - If ALGO_WEB_HOST=0.0.0.0, anyone can deploy servers and access credentials. Document security implications.

8. Process Orphaning (app/runner.py:109-117) - Server crash leaves Ansible processes running. Add shutdown handler.

Medium Priority Issues

9. Passive Session Cleanup - Only cleaned during create(). Add background cleanup task.

10. Incomplete Vultr Implementation - Comment at runner.py:42-43 mentions unimplemented feature.

11. Hardcoded Timeouts - 15s validation, 10s cancellation. Make configurable.

12. No Health Endpoint - Add /health endpoint for monitoring.

Good Practices Observed

  • Excellent subprocess handling with create_subprocess_exec (no shell injection)
  • Path traversal protection in download_file
  • Credentials cleared from memory after deployment starts
  • Proper error handling with good HTTP status codes
  • Clean separation of concerns

Missing Tests

CRITICAL: No tests for this major feature handling sensitive data. Need:

  1. Unit tests for credential validation
  2. Unit tests for input validation
  3. Unit tests for session management
  4. Unit tests for path traversal protection
  5. Integration test for deployment flow

Per CLAUDE.md: "Test everything"

Before Merge Checklist

Required:

  • Fix credential exposure (issue 1) - CRITICAL
  • Add input validation (issue 5)
  • Add basic unit tests
  • Run full lint suite

Strongly Recommended:

  • Add CSRF protection
  • Add rate limiting
  • Add output buffer limit
  • Add ZIP size limit
  • Document security implications

Summary

Architecture: Excellent
Security: Multiple critical issues - credential exposure is a showstopper
Code Quality: Good Python style
Testing: Missing entirely

Assessment: Approve with required changes

Fix issues 1, 5, and add tests before merging. Great work on the implementation - just need to shore up security aspects!

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