Fix für Maintenance-AddOn URL-Parameter Authentifizierung#143
Fix für Maintenance-AddOn URL-Parameter Authentifizierung#143AWqxKAWERbXo merged 3 commits intomainfrom
Conversation
WalkthroughThe update script has been modified to enforce stricter validation and defaulting for two configuration keys: Changes
Sequence Diagram(s)sequenceDiagram
participant UpdateScript
participant Config
UpdateScript->>Config: Read authentification_mode
alt Invalid or empty authentification_mode
UpdateScript->>Config: Set authentification_mode to 'URL'
end
UpdateScript->>Config: Read maintenance_secret
alt Missing or empty maintenance_secret
UpdateScript->>Config: Generate random 32-char hex secret
UpdateScript->>Config: Set maintenance_secret
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
update.php (2)
61-65: Secure implementation of maintenance secret validation and generationThe code properly checks for both missing and empty
maintenance_secretvalues, and generates a cryptographically secure random secret when needed. Usingrandom_bytes()withbin2hex()is an appropriate way to generate secure random tokens.Consider adding error handling around the
random_bytes()call, as it can throw an Exception if there's not enough entropy available (though this is rare in modern systems):- $addon->setConfig('maintenance_secret', bin2hex(random_bytes(16))); + try { + $addon->setConfig('maintenance_secret', bin2hex(random_bytes(16))); + } catch (Exception $e) { + // Fallback to less secure but available method + $addon->setConfig('maintenance_secret', bin2hex(openssl_random_pseudo_bytes(16))); + }
54-54: Consider translating the German comment for international contributorsThe German comment explains that both empty string and 'URL' are considered valid URL authentication options. For better maintainability in an international open-source project, consider adding an English translation.
- // Leerer String ('') und 'URL' werden beide als gültige URL-Authentifizierung betrachtet + // Leerer String ('') und 'URL' werden beide als gültige URL-Authentifizierung betrachtet + // Empty string ('') and 'URL' are both considered valid URL authentication methods
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
update.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
update.php (1)
lib/Maintenance.php (1)
getConfig(302-305)
🔇 Additional comments (1)
update.php (1)
54-59: Good implementation of authentication mode validationThe code correctly validates that the
authentification_modeis set to one of the allowed values ('URL' or 'password') and defaults to 'URL' if the value is missing or invalid. Using strict comparison in thein_arrayfunction is a security best practice to prevent type juggling issues.
skerbis
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/Maintenance.php (2)
138-142: Remove redundant variable declaration and consider standardizing comment languageThere's a redundant redeclaration of the
$authentification_modevariable on line 139, which was already defined on line 136. This doesn't change the behavior since both lines fetch the same configuration value, but it's unnecessary duplication.Also, the comment is in German while the rest of the codebase appears to be in English, which could affect code readability for non-German speakers.
- // Authentifizierung prüfen - für URL-Parameter und auch bei leerem Modus - $authentification_mode = (string) self::getConfig('authentification_mode', ''); + // Check authentication - for URL parameters and for empty mode as well if (('' === $authentification_mode || 'URL' === $authentification_mode || 'password' === $authentification_mode) && '' !== $config_secret && $maintenance_secret === $config_secret) {
140-142: Consider case-insensitive comparison for authentication modesThe comparison for authentication modes is currently case-sensitive. If configuration values might be stored inconsistently (e.g., "url" vs "URL"), consider making this comparison case-insensitive to improve robustness.
- if (('' === $authentification_mode || 'URL' === $authentification_mode || 'password' === $authentification_mode) + if (('' === $authentification_mode || 'URL' === strtoupper($authentification_mode) || 'password' === strtolower($authentification_mode)) && '' !== $config_secret && $maintenance_secret === $config_secret) {Or alternatively, normalize using strtolower() throughout:
- if (('' === $authentification_mode || 'URL' === $authentification_mode || 'password' === $authentification_mode) + if (('' === $authentification_mode || 'url' === strtolower($authentification_mode) || 'password' === strtolower($authentification_mode)) && '' !== $config_secret && $maintenance_secret === $config_secret) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/Maintenance.php(1 hunks)
🔇 Additional comments (1)
lib/Maintenance.php (1)
140-142: The authentication mode expansion looks goodAllowing an empty string (
'') as a valid authentication mode alongside'URL'and'password'aligns with the PR objectives of improving authentication handling when using themaintenance_secretparameter.
AWqxKAWERbXo
left a comment
AWqxKAWERbXo
left a comment
There was a problem hiding this comment.
Sehr viel Prosa-Text von coderabbit, aber ich weiß ehrlich gesagt nicht genau, was das Szenario ist, wo ein Fehler vorliegt und was damit gelöst wird.
Wird schon stimmen.
|
@alxndr-w es führt nur einen update Schritt hinzu . Früher war der Mode leer jetzt heißt es URL. Nebenbei wurde noch eine kleine Änderung bei der Prüfung durchgeführt- mehr nicht |
|
@skerbis Kann man den PR mergen? Weil laut GitHub fehlt wohl noch was.
|
|
@crydotsnake keine Ahnung. @alxndr-w hats aber schon gemacht |
3 participants
Fix für den Zugriff via maintenance_secret Parameter - für alle Versionen
Stellen wir sicher, dass der authentification_mode korrekt gesetzt ist
Fix für Maintenance-AddOn URL-Parameter Authentifizierung
Problem
Nach einem Update des Maintenance-AddOns konnte der Wartungsmodus nicht mehr mit dem vorhandenen "maintenance_secret"-Passwort über einen URL-Parameter aufgerufen werden. Gesperrte Landingpages waren daher nur noch über einen Backend-Login erreichbar.
Lösungsansatz
Der Fehler lag in der
isSecretAllowed()-Funktion, die den authentification_mode zu streng prüfte und den URL-Parameter nur akzeptierte, wenn der Mode explizit auf "URL" oder "password" gesetzt war. Bei nicht oder anders gesetzten Werten wurde der Parameter ignoriert.Änderungen
isSecretAllowed()-Funktion akzeptiert jetzt den maintenance_secret URL-Parameter unabhängig vom konfigurierten authentification_mode und behandelt leere Werte wie den URL-Modusfixed: #142
Summary by CodeRabbit