Merge pull request #27 from naspirato/fix_redirects

naspirato · web-flow · commit 137d609ab03d · 2025-12-01T18:20:32.000+01:00
Fix redirects
diff --git a/backend/routes/auth.py b/backend/routes/auth.py
@@ -3,6 +3,7 @@
 """
 import secrets
 import logging
+from urllib.parse import urlparse
 from fastapi import APIRouter, Request, HTTPException
 from fastapi.responses import RedirectResponse
 
@@ -12,23 +13,80 @@
 router = APIRouter()
 
 
+def validate_redirect_url(url: str, request: Request) -> str:
+    """
+    Validate and normalize redirect URL for security.
+    Only allows internal paths (starting with /) to prevent open redirect attacks.
+    
+    Args:
+        url: URL to validate
+        request: Request object to get base URL
+        
+    Returns:
+        Normalized internal path (relative URL)
+        
+    Raises:
+        HTTPException: If URL is invalid or external
+    """
+    if not url:
+        return "/"
+    
+    # Parse URL
+    parsed = urlparse(url)
+    
+    # If URL has scheme (http/https), check if it's internal
+    if parsed.scheme:
+        # Get base URL from request
+        base_url = str(request.base_url).rstrip('/')
+        request_host = request.url.hostname
+        
+        # Allow only same host
+        if parsed.netloc and parsed.netloc != request_host:
+            logger.warning(f"External redirect URL blocked: {url}")
+            return "/"
+        
+        # Extract path and query
+        path = parsed.path or "/"
+        query = parsed.query
+        if query:
+            path = f"{path}?{query}"
+        return path
+    
+    # If no scheme, treat as relative path
+    # Ensure it starts with /
+    if not url.startswith('/'):
+        url = '/' + url
+    
+    return url
+
+
 @router.get("/github")
 async def github_login(request: Request, redirect_after: str = None):
-    """Initiate GitHub OAuth login"""
+    """
+    Initiate GitHub OAuth login
+    
+    Saves redirect URL for post-authentication redirect.
+    Only saves if explicitly provided via parameter or query string.
+    """
     # Generate state for CSRF protection
     state = secrets.token_urlsafe(32)
     request.session["oauth_state"] = state
     
-    # Save redirect URL if provided
-    if redirect_after:
-        request.session["oauth_redirect_after"] = redirect_after
-    elif "oauth_redirect_after" not in request.session:
-        # Get redirect_after from query parameter if not in session
+    # Get redirect_after from parameter or query string
+    if not redirect_after:
         redirect_after = request.query_params.get("redirect_after")
-        if redirect_after:
-            request.session["oauth_redirect_after"] = redirect_after
     
-    logger.info(f"OAuth login initiated, state generated: {state[:10]}..., redirect_after: {request.session.get('oauth_redirect_after', 'not set')}")
+    # Validate and save redirect URL if provided
+    if redirect_after:
+        try:
+            validated_url = validate_redirect_url(redirect_after, request)
+            request.session["oauth_redirect_after"] = validated_url
+            logger.info(f"OAuth login initiated, state: {state[:10]}..., redirect_after: {validated_url}")
+        except Exception as e:
+            logger.warning(f"Invalid redirect URL provided: {redirect_after}, error: {e}")
+            # Continue without redirect_after, will redirect to / after auth
+    else:
+        logger.info(f"OAuth login initiated, state: {state[:10]}..., no redirect_after (will redirect to /)")
     
     # Redirect to GitHub OAuth
     oauth_url = get_oauth_url(state=state)
@@ -85,8 +143,15 @@ async def github_callback(request: Request, code: str = None, state: str = None)
         request.session.pop("oauth_state", None)
         logger.info(f"Session updated for user: {user_info['login']}")
         
-        # Redirect to saved URL or main page
+        # Get and validate redirect URL
         redirect_url = request.session.pop("oauth_redirect_after", "/")
+        try:
+            # Validate redirect URL for security (prevent open redirect attacks)
+            redirect_url = validate_redirect_url(redirect_url, request)
+        except Exception as e:
+            logger.warning(f"Invalid redirect URL in session: {redirect_url}, error: {e}, redirecting to /")
+            redirect_url = "/"
+        
         logger.info(f"Redirecting after OAuth to: {redirect_url}")
         return RedirectResponse(url=redirect_url, status_code=303)
     except HTTPException:
diff --git a/backend/routes/workflow.py b/backend/routes/workflow.py
@@ -41,10 +41,13 @@ async def _trigger_and_show_result(
     access_token = request.session.get("access_token")
     
     if not user or not access_token:
-        # Save current URL for redirect after OAuth
-        current_url = str(request.url)
-        request.session["oauth_redirect_after"] = current_url
-        logger.info(f"No session found, saving redirect URL: {current_url}")
+        # Save current URL (relative path with query) for redirect after OAuth
+        # Use relative path for security (prevents open redirect attacks)
+        redirect_path = request.url.path
+        if request.url.query:
+            redirect_path = f"{redirect_path}?{request.url.query}"
+        request.session["oauth_redirect_after"] = redirect_path
+        logger.info(f"No session found, saving redirect path: {redirect_path}")
         
         # Redirect to login
         oauth_url = get_oauth_url()
diff --git a/frontend/static/style.css b/frontend/static/style.css
@@ -266,14 +266,26 @@ header h1 {
     width: 100%;
 }
 
-.btn-primary:hover:not(:disabled) {
+/* Green color when button is active (can run workflow) */
+.btn-primary.btn-active {
+    background: #10b981;
+    color: white;
+    border-color: #059669;
+}
+
+.btn-primary.btn-active:hover:not(:disabled) {
+    background: #059669;
+    border-color: #047857;
+}
+
+.btn-primary:not(.btn-active):hover:not(:disabled) {
     background: #f3f4f6;
 }
 
 .btn-primary:active:not(:disabled) {
-    background: #10b981;  /* Зеленый цвет при нажатии */
+    background: #047857;
     color: white;
-    border-color: #059669;
+    border-color: #065f46;
 }
 
 .btn-primary:disabled {
diff --git a/frontend/templates/index.html b/frontend/templates/index.html
@@ -14,7 +14,7 @@
             <div class="auth-modal">
                 <h2>Authentication Required</h2>
                 <p>Please sign in with GitHub to run workflows</p>
-                <a href="/auth/github" class="btn btn-primary">Sign in with GitHub</a>
+                <a href="/auth/github" id="auth-link" class="btn btn-primary">Sign in with GitHub</a>
             </div>
         </div>
         {% endif %}
@@ -135,7 +135,7 @@ <h1>Run GitHub Action</h1>
                         {% endif %}
                         <span class="user-login">{{ user.login }}</span>
                     </div>
-                    <a href="/auth/logout" class="btn btn-secondary btn-small">Выйти</a>
+                    <a href="/auth/logout" class="btn btn-secondary btn-small">Logout</a>
             </div>
             </footer>
         </main>
@@ -434,6 +434,7 @@ <h1>Run GitHub Action</h1>
                         // Disable button and show message if workflow doesn't support manual trigger
                         if (runButton) {
                             runButton.disabled = true;
+                            runButton.classList.remove('btn-active');
                             runButton.style.opacity = '0.5';
                             runButton.style.cursor = 'not-allowed';
                         }
@@ -934,6 +935,7 @@ <h1>Run GitHub Action</h1>
             // If owner or repo are empty, disable button and show message
             if (!owner || !repo) {
                 runButton.disabled = true;
+                runButton.classList.remove('btn-active');
                 runButton.style.opacity = '0.5';
                 runButton.style.cursor = 'not-allowed';
                 permissionMessage.style.display = 'block';
@@ -947,6 +949,7 @@ <h1>Run GitHub Action</h1>
                 if (response.status === 401) {
                     // Not authenticated
                     runButton.disabled = true;
+                    runButton.classList.remove('btn-active');
                     runButton.style.opacity = '0.5';
                     runButton.style.cursor = 'not-allowed';
                     permissionMessage.style.display = 'block';
@@ -965,6 +968,7 @@ <h1>Run GitHub Action</h1>
                 
                 if (data.can_trigger && hasWorkflowDispatch) {
                     runButton.disabled = false;
+                    runButton.classList.add('btn-active');
                     runButton.style.opacity = '1';
                     runButton.style.cursor = 'pointer';
                     permissionMessage.textContent = '';
@@ -974,6 +978,7 @@ <h1>Run GitHub Action</h1>
                     }
                 } else {
                     runButton.disabled = true;
+                    runButton.classList.remove('btn-active');
                     runButton.style.opacity = '0.5';
                     runButton.style.cursor = 'not-allowed';
                     permissionMessage.style.display = 'block';
@@ -991,24 +996,33 @@ <h1>Run GitHub Action</h1>
                 console.error('Error checking permissions:', error);
                 // In case of error, allow attempt (check will be on server)
                 runButton.disabled = false;
+                runButton.classList.add('btn-active');
                 runButton.style.opacity = '1';
                 runButton.style.cursor = 'pointer';
                 permissionMessage.textContent = '';
                 permissionMessage.style.display = 'none';
             }
         }
         
-        // Инициализация при загрузке
+        // Initialize on page load
         function initAll() {
-            // Инициализируем скрытое поле return_url из URL параметров, если его нет
+            // Update the authorization link to pass the current URL with parameters
+            const authLink = document.getElementById('auth-link');
+            if (authLink && window.location.search) {
+                const currentUrl = window.location.href;
+                const redirectAfter = encodeURIComponent(currentUrl);
+                authLink.href = `/auth/github?redirect_after=${redirectAfter}`;
+            }
+            
+            // Initialize the hidden return_url field from URL parameters if not present
             const form = document.getElementById('workflowForm');
             if (form) {
                 const urlParams = new URLSearchParams(window.location.search);
                 const returnUrlFromUrl = urlParams.get('return_url');
                 if (returnUrlFromUrl) {
                     let returnUrlInput = form.querySelector('input[name="return_url"]');
                     if (!returnUrlInput) {
-                        // Создаем скрытое поле, если его нет
+                        // Create hidden field if it doesn't exist
                         returnUrlInput = document.createElement('input');
                         returnUrlInput.type = 'hidden';
                         returnUrlInput.name = 'return_url';
diff --git a/frontend/templates/result.html b/frontend/templates/result.html
@@ -76,7 +76,7 @@ <h2>Success</h2>
 
                 <div id="run-loader" class="run-loader" style="display: {% if run_url %}none{% else %}flex{% endif %};">
                     <div class="loader"></div>
-                    <p class="loading-message">Ищем запуск workflow...</p>
+                    <p class="loading-message">Searching for workflow run...</p>
                 </div>
 
                 <div class="action-links" id="action-links" style="display: {% if run_url %}flex{% else %}none{% endif %};">
@@ -175,15 +175,15 @@ <h2>Error</h2>
                     })
                     .then(data => {
                         if (data.found && data.run_url) {
-                            // Нашли run!
+                            // Found run!
                             const loader = document.getElementById('run-loader');
                             const actionLinks = document.getElementById('action-links');
                             
                             if (loader) loader.style.display = 'none';
                             if (actionLinks) {
                                 actionLinks.style.display = 'flex';
                                 
-                                // Создаем или обновляем ссылку
+                                // Create or update the link
                                 let runLink = document.getElementById('run-link');
                                 if (!runLink) {
                                     runLink = document.createElement('a');
@@ -199,22 +199,22 @@ <h2>Error</h2>
                                 }
                             }
                             
-                            // Убеждаемся, что второй ряд кнопок виден
+                            // Ensure the second row of buttons is visible
                             const secondRow = actionLinks ? actionLinks.nextElementSibling : null;
                             if (secondRow && secondRow.classList.contains('action-links')) {
                                 secondRow.style.display = 'flex';
                             }
                         } else if (attempts < maxAttempts) {
-                            // Продолжаем опрос
+                            // Continue polling
                             setTimeout(findRun, pollInterval);
                         } else {
-                            // Превысили лимит попыток
+                            // Exceeded attempt limit
                             const loader = document.getElementById('run-loader');
                             if (loader) {
-                                loader.innerHTML = '<p class="loading-message">Не удалось найти запуск. <a href="' + workflowUrl + '" target="_blank" style="color: #000; text-decoration: underline;">Открыть список workflow</a></p>';
+                                loader.innerHTML = '<p class="loading-message">Failed to find run. <a href="' + workflowUrl + '" target="_blank" style="color: #000; text-decoration: underline;">Open workflow list</a></p>';
                             }
                             
-                            // Показываем кнопки даже если не нашли run
+                            // Show buttons even if run was not found
                             const actionLinks = document.getElementById('action-links');
                             if (actionLinks) {
                                 actionLinks.style.display = 'flex';
@@ -228,13 +228,13 @@ <h2>Error</h2>
                         } else {
                             const loader = document.getElementById('run-loader');
                             if (loader) {
-                                loader.innerHTML = '<p class="loading-message" style="color: #999;">Ошибка при поиске запуска. <a href="' + workflowUrl + '" target="_blank" style="color: #000; text-decoration: underline;">Открыть список workflow</a></p>';
+                                loader.innerHTML = '<p class="loading-message" style="color: #999;">Error searching for run. <a href="' + workflowUrl + '" target="_blank" style="color: #000; text-decoration: underline;">Open workflow list</a></p>';
                             }
                         }
                     });
             }
             
-            // Начинаем поиск через небольшую задержку
+            // Start search after a short delay
             setTimeout(findRun, 500);
         })();
     </script>
diff --git a/tests/test_app.py b/tests/test_app.py
diff --git a/tests/test_auth_redirect.py b/tests/test_auth_redirect.py
