Skip to content

Conversation

@HeMan
Copy link

@HeMan HeMan commented Jan 7, 2026

✍️ Description

🔗 Related Issue

Fixes #

✅ Prerequisites (X in brackets)

  • Self-review completed – Code follows project standards.
  • Tested thoroughly – Changes work as expected.
  • No security risks – No hardcoded secrets, unnecessary privilege escalations, or permission issues.

🛠️ Type of Change (X in brackets)

  • 🐞 Bug fix – Resolves an issue without breaking functionality.
  • New feature – Adds new, non-breaking functionality.
  • 💥 Breaking change – Alters existing functionality in a way that may require updates.
  • 🆕 New script – A fully functional and tested script or script set.
  • 🌍 Website update – Changes to website-related JSON files or metadata.
  • 🔧 Refactoring / Code Cleanup – Improves readability or maintainability without changing functionality.
  • 📝 Documentation update – Changes to README, AppName.md, CONTRIBUTING.md, or other docs.

@HeMan HeMan requested a review from a team as a code owner January 7, 2026 09:33
Copy link
Member

@MickLesk MickLesk left a comment

Choose a reason for hiding this comment

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

It incorrectly allows:

  • test..example (double dots)
  • test.-example (dot + hyphen)
  • test. (trailing dot)

Only 1 label, max 63 characters total (not RFC-compliant)

Hostname: max. 63 characters, no dots
FQDN: Multiple labels with dots, each label max. 63 characters, total max. 253 characters

@HeMan
Copy link
Author

HeMan commented Jan 14, 2026

Should the check be fully FQDN compliant and allow capital letters as well?

@MickLesk
Copy link
Member

should be like this:

# ...existing code...
    if result=$(whiptail --backtitle "Proxmox VE Helper Scripts [Step $STEP/$MAX_STEP]" \
      --title "HOSTNAME" \
      --ok-button "Next" --cancel-button "Back" \
      --inputbox "\nSet Hostname (or FQDN, e.g. host.example.com)" 10 58 "$_hostname" \
      3>&1 1>&2 2>&3); then
      local hn_test="${result:-$NSAPP}"
      hn_test=$(echo "${hn_test,,}" | tr -d ' ')
      
      # Validate hostname/FQDN according to RFC 1123/952
      local valid=true
      
      # Check total length (max 253 for FQDN)
      if [[ ${#hn_test} -gt 253 ]]; then
        valid=false
      fi
      
      # Split by dots and validate each label
      if $valid; then
        IFS='.' read -ra labels <<< "$hn_test"
        for label in "${labels[@]}"; do
          # Each label: 1-63 chars, alphanumeric, hyphens allowed (not at start/end)
          if [[ -z "$label" ]] || [[ ${#label} -gt 63 ]]; then
            valid=false
            break
          fi
          if [[ ! "$label" =~ ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$ ]] && [[ ! "$label" =~ ^[a-z0-9]$ ]]; then
            valid=false
            break
          fi
        done
      fi
      
      if $valid; then
        _hostname="$hn_test"
        ((STEP++))
      else
        whiptail --msgbox "Invalid hostname: '$hn_test'\n\nRules:\n- Only lowercase letters, digits, dots and hyphens\n- Labels separated by dots (max 63 chars each)\n- No leading/trailing hyphens or dots\n- No consecutive dots\n- Total max 253 characters" 14 60
      fi
# ...existing code...

its a little bit more then 2-4 changed lines

MickLesk added a commit that referenced this pull request Jan 19, 2026
- Add validate_hostname() function for centralized validation
- Validate var_hostname from environment/vars files with fallback
- Allow FQDN hostnames (e.g. host.example.com) in Advanced Settings
- Enforce max 253 chars total, max 63 chars per label
- Prevent double dots, leading/trailing hyphens and dots

Implements requested changes from PR #10621
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