Skip to content
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

Fix docker images and dual mode #693

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Fix docker images and dual mode #693

wants to merge 11 commits into from

Conversation

sstidl
Copy link
Collaborator

@sstidl sstidl commented Dec 29, 2024

User description

This pull request includes updates to the Docker configuration and the PHP UI script to better support "dual" mode. The most important changes are as follows:

Docker Configuration Updates:

  • Added a test docker-compose file to test multiple databases with speedtest docker image.

PHP UI Script Updates:

  • Modified the conditional logic in docker/ui.php to handle the "dual" mode, including loading server configurations from servers.json and dynamically adding the current server to the list of speedtest servers.

PR Type

Bug fix, Enhancement


Description

  • Fixed broken dual mode functionality in the PHP UI by properly handling server configurations and adding dynamic server detection
  • Enhanced docker testing environment with:
    • New shared service configurations
    • Additional test container for dual mode
    • Improved test script reliability
  • Added comprehensive documentation and configuration files for testing environment
  • Added extensive servers.json file with global speedtest server locations

Changes walkthrough 📝

Relevant files
Bug fix
ui.php
Fix dual mode server configuration and detection                 

docker/ui.php

  • Fixed dual mode functionality by correctly handling server
    configurations from servers.json
  • Added dynamic server detection to include current server in speedtest
    servers list
  • Modified mode condition checks to properly handle standalone, backend,
    and dual modes
  • +28/-1   
    Enhancement
    test-script.sh
    Enhance test script reliability and documentation               

    docker/test/test-script.sh

  • Added shebang line for shell script
  • Increased sleep time from 10 to 15 seconds
  • Added descriptive comment about script purpose
  • +5/-1     
    Documentation
    README.md
    Add documentation for docker test environment                       

    docker/test/README.md

  • Added new documentation file explaining test folder usage
  • Included note about MySQL container timing issues
  • +4/-0     
    Configuration changes
    docker-compose.yml
    Enhance docker compose with shared configs and dual mode 

    docker/test/docker-compose.yml

  • Added shared configuration block for speedtest services
  • Added new speedtest-alpine-sqlite-dual service configuration
  • Added container dependencies for test container
  • +27/-0   
    servers.json
    Add servers configuration file for testing                             

    docker/test/servers.json

  • Added new file containing list of speedtest servers
  • Includes comprehensive list of global server locations with
    configuration details
  • +409/-0 

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @sstidl sstidl self-assigned this Dec 29, 2024
    @sstidl sstidl marked this pull request as ready for review December 29, 2024 16:42

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 Security concerns

    URL Manipulation:
    The dynamic URL construction in ui.php (lines 20-29) uses $_SERVER variables without proper validation or sanitization. This could potentially be exploited if the server configuration allows header manipulation. The code should validate and sanitize these inputs before using them to construct the backend URL.

    ⚡ Recommended focus areas for review

    Security Validation

    The dynamic URL construction for server detection should be validated to prevent potential URL manipulation or injection attacks

    $protocol = (!empty($_SERVER['HTTPS']) && $_SERVER['HTTPS'] !== 'off' || $_SERVER['SERVER_PORT'] == 443) ? "https://" : "http://";
    
    // Retrieve the host (e.g., www.example.com)
    $host = $_SERVER['HTTP_HOST'];
    
    // Retrieve the URI (path and query string)
    $uri = $_SERVER['REQUEST_URI'];
    
    // Combine them to get the full URL
    $url = $protocol . $host . $uri;
    Timing Issue

    The sleep duration may still be insufficient for MySQL container initialization in some cases, as noted in the README

    echo sleeping a little to get things setteled...
    sleep 15

    Copy link

    qodo-merge-pro-for-open-source bot commented Dec 29, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    ✅ Sanitize user-influenced server variables to prevent potential security vulnerabilities

    Sanitize the HTTP_HOST and REQUEST_URI values before using them in URL construction
    to prevent potential XSS or injection attacks.

    docker/ui.php [23-29]

    -$host = $_SERVER['HTTP_HOST'];
    -$uri = $_SERVER['REQUEST_URI'];
    -$url = $protocol . $host . $uri;
    +$host = filter_var($_SERVER['HTTP_HOST'], FILTER_SANITIZE_STRING);
    +$uri = filter_var($_SERVER['REQUEST_URI'], FILTER_SANITIZE_URL);
    +$url = $protocol . htmlspecialchars($host) . htmlspecialchars($uri);

    [Suggestion has been applied]

    Suggestion importance[1-10]: 9

    Why: This is a crucial security improvement that prevents potential XSS and injection attacks by properly sanitizing user-influenced server variables used in URL construction.

    9
    Possible issue
    ✅ Add error handling for file operations and JSON parsing to prevent application crashes

    Add input validation for the JSON data from servers.json to handle potential file
    read or JSON decode failures gracefully. The current implementation could throw
    errors if the file is missing or contains invalid JSON.

    docker/ui.php [17]

    -$servers = json_decode(file_get_contents('/servers.json'), true);
    +$jsonContent = @file_get_contents('/servers.json');
    +if ($jsonContent === false) {
    +    $servers = [];
    +} else {
    +    $servers = json_decode($jsonContent, true) ?: [];
    +}

    [Suggestion has been applied]

    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a critical error handling issue that could cause application crashes in production. The improved code adds robust error checking for both file reading and JSON parsing operations.

    8
    General
    ✅ Add error handling for package installation to prevent script from continuing after failures

    Add error handling for the 'apk add w3m' command to ensure the script doesn't
    continue if the package installation fails.

    docker/test/test-script.sh [7]

    -apk add w3m
    +if ! apk add w3m; then
    +    echo "Failed to install w3m package"
    +    exit 1
    +fi

    [Suggestion has been applied]

    Suggestion importance[1-10]: 6

    Why: The suggestion adds important error handling for package installation, preventing silent failures that could cause issues later in the script execution.

    6

    Co-authored-by: qodo-merge-pro-for-open-source[bot] <189517486+qodo-merge-pro-for-open-source[bot]@users.noreply.github.com>
    sstidl and others added 4 commits December 29, 2024 17:50
    Co-authored-by: qodo-merge-pro-for-open-source[bot] <189517486+qodo-merge-pro-for-open-source[bot]@users.noreply.github.com>
    Co-authored-by: qodo-merge-pro-for-open-source[bot] <189517486+qodo-merge-pro-for-open-source[bot]@users.noreply.github.com>
    @sstidl sstidl requested a review from adolfintel December 29, 2024 19:13
    @sstidl
    Copy link
    Collaborator Author

    sstidl commented Dec 29, 2024

    @adolfintel if you want please review and merge. i will merge it in the next days if i dont hear from you. 👋

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

    Successfully merging this pull request may close these issues.

    2 participants