Conversation
📝 WalkthroughWalkthroughAdds a RuntimeState container and service-oriented request bootstrap (auth, CSRF, policy, router, exception handler, response headers); replaces many global dependencies with runtime lookups and guards across models/views; centralizes sync/SSH runtime/diagnostics and failure classification; introduces CI quality gates, phpstan/php-cs-fixer configs, a Makefile, and a smoke-test + browser automation harness; migrates frontend templates/JS/CSS toward Bootstrap 5 and native DOM. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant RH as RequestHandler
participant RC as RequestContext
participant RS as RuntimeState
participant Auth as RequestAuthGuard
participant CSRF as RequestCsrfGuard
participant Policy as RequestPolicyGuard
participant Router as RequestRouterDispatcher
participant View as View
participant Sec as ResponseSecurityHeaders
participant Ex as RequestExceptionHandler
Client->>RH: HTTP request
RH->>RC: RequestContext::from_globals()
RH->>RS: RuntimeState::set_many(...)
RH->>Auth: resolve_active_user(RC)
Auth-->>RH: active_user|null
alt active_user present
RH->>CSRF: validate(active_user, RC, POST)
end
RH->>Policy: enforce_web_enabled(config)
RH->>Router: build_router(...) / resolve_view_path(...)
Router-->>RH: view path
RH->>View: include resolved view (view may call services)
View-->>RH: rendered output
RH->>Sec: apply(config)
RH-->>Client: HTTP response
alt exception thrown
RH->>Ex: handle(exception)
Ex-->>Client: error500 response
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
scripts/sync-common.php (1)
113-139:⚠️ Potential issue | 🟡 Minor
get_data()can returnnullon second call after spawn failure.After the first call with
spawn_failed = true, the flag is reset tofalse(line 115). On a subsequent call,spawn_failedisfalseand$this->handlewas never set, so neither branch executes and the method implicitly returnsnull. Callers (e.g.,finish()on line 149 and the polling loop insync.phpline 135) may not handlenullgracefully —finish()calls$this->get_data()unconditionally, and the loop checks!empty($data).In practice,
finish()is the main risk: it proceeds to check$this->exit_statusafter a null-returningget_data(), which works by coincidence sinceexit_statuswas set in the constructor. But it's fragile.🛡️ Suggested safeguard
public function get_data() { if($this->spawn_failed) { $this->spawn_failed = false; if($this->errors) { echo $this->errors."\n"; } return array('done' => true, 'output' => ''); } if(isset($this->handle) && is_resource($this->handle)) { // ... existing code ... } + return array('done' => true, 'output' => ''); }views/user_pubkeys.php (1)
35-51:⚠️ Potential issue | 🟠 MajorVerify duplicate key detection is implemented.
The delegation to
PublicKey::import()correctly validates key format (regex parsing) and enforces minimum key strength viaget_openssh_info()and config-drivenmin_*_bitschecks. However, the database schema for thepublic_keytable has no UNIQUE constraints onfingerprint_md5,fingerprint_sha256, orkeydata, andEntity::add_public_key()performs no duplicate detection before INSERT. Confirm whether duplicate keys should be allowed per user or whether duplicate prevention needs to be added via database constraint or application logic.README.md (1)
210-210:⚠️ Potential issue | 🟡 MinorTypo: "Maintanence" → "Maintenance".
Proposed fix
-Maintanence, Modifications and Improvements by Msprg +Maintenance, Modifications and Improvements by Msprgmodel/server.php (1)
654-661:⚠️ Potential issue | 🟠 Major
sync_request_dirmay be null — unguarded dereference on line 660.
self::runtime_value('sync_request_dir', ...)falls back to$GLOBALS['sync_request_dir'], but if neither source has it,$sync_request_dirisnulland->add_sync_request()will fatal. The same issue exists inreschedule_sync_request(line 698/704). In contrast,Group::list_group_membershipguards with a null check and throwsDomainException.🛡️ Proposed guard
public function sync_access() { $sync_request_dir = self::runtime_value('sync_request_dir', array_key_exists('sync_request_dir', $GLOBALS) ? $GLOBALS['sync_request_dir'] : null); + if($sync_request_dir === null) { + throw new DomainException('Sync request directory service is unavailable'); + } $sync_request = new SyncRequest;Apply the same pattern to
reschedule_sync_request()at line 698.model/serveraccount.php (2)
63-69:⚠️ Potential issue | 🟡 MinorUndefined array key warning on PHP 8.x when config lacks
defaults.If
runtime_config()returns the empty-array fallback, accessing$config['defaults']['account_groups']on line 65 emits anE_WARNINGfor undefined array keydefaultsbeforeis_array()evaluates. Useisset()or null coalescing instead.🔧 Proposed fix
- if(is_array($config['defaults']['account_groups'])) { + if(isset($config['defaults']['account_groups']) && is_array($config['defaults']['account_groups'])) {As per coding guidelines:
**/*.php: Ensure PHP 8.2+ compatibility for all PHP code.
429-434:⚠️ Potential issue | 🟠 MajorMissing null guard on
sync_request_dir— same issue asServer::sync_access.Line 434 will fatal if
sync_request_diris null. Add a null guard consistent withlist_group_membershipabove.🛡️ Proposed guard
public function sync_access() { $sync_request_dir = self::runtime_value('sync_request_dir', array_key_exists('sync_request_dir', $GLOBALS) ? $GLOBALS['sync_request_dir'] : null); + if($sync_request_dir === null) { + throw new DomainException('Sync request directory service is unavailable'); + } $sync_request = new SyncRequest;model/user.php (1)
490-530:⚠️ Potential issue | 🟡 MinorGood addition of
find_accessible_server_accounts()with explicit return type.Logic is clear and return type
arrayis explicit. Minor nit on line 520: typo "aGroup" → "a Group" in the exception message, though it's an edge case.Typo fix
- throw new Exception("One accessible entity is neither a ServerAccount nor aGroup."); + throw new Exception("One accessible entity is neither a ServerAccount nor a Group.");core.php (1)
109-125:⚠️ Potential issue | 🔴 Critical
mysqli::options()calls after the constructor have no effect — SQL strict mode is never applied.The
new mysqli(...)constructor connects immediately, rendering the subsequentoptions()calls on lines 123–125 ineffective.MYSQLI_OPT_CONNECT_TIMEOUT,MYSQLI_OPT_READ_TIMEOUT, andMYSQLI_INIT_COMMANDmust all be set before the connection is established viamysqli_real_connect(). This means the SQL strict mode (the most critical setting) will not be enforced, and the database will use its default (potentially permissive) mode.Use
mysqli_init()+options()+real_connect()instead:Proposed fix
- $database = new mysqli( - $config['database']['hostname'], - $config['database']['username'], - $config['database']['password'], - $config['database']['database'], - $config['database']['port'] - ); + $database = mysqli_init(); + $database->options(MYSQLI_OPT_CONNECT_TIMEOUT, 5); + $database->options(MYSQLI_OPT_READ_TIMEOUT, 30); + $database->options(MYSQLI_INIT_COMMAND, "SET SESSION sql_mode='STRICT_TRANS_TABLES,NO_ZERO_DATE,NO_ZERO_IN_DATE,ERROR_FOR_DIVISION_BY_ZERO'"); + $database->real_connect( + $config['database']['hostname'], + $config['database']['username'], + $config['database']['password'], + $config['database']['database'], + (int)($config['database']['port'] ?? 3306) + ); // Database optimizations $database->set_charset('utf8mb4'); - $database->options(MYSQLI_OPT_CONNECT_TIMEOUT, 5); - $database->options(MYSQLI_OPT_READ_TIMEOUT, 30); - $database->options(MYSQLI_INIT_COMMAND, "SET SESSION sql_mode='STRICT_TRANS_TABLES,NO_ZERO_DATE,NO_ZERO_IN_DATE,ERROR_FOR_DIVISION_BY_ZERO'");
🤖 Fix all issues with AI agents
In @.github/workflows/docker.yml:
- Around line 53-54: The CI step named "Docker compose config validation" runs
"docker compose config -q" but fails because docker-compose.yml is gitignored
and only docker-compose.yml.example exists; update the workflow so that before
running the validation it either copies or creates docker-compose.yml from
docker-compose.yml.example (or validates the example file instead), ensuring the
"Docker compose config validation" step points at a real compose file (reference
the step name "Docker compose config validation" and the files
docker-compose.yml.example / docker-compose.yml).
In `@model/entity.php`:
- Around line 484-499: The variables $group_dir, $server_dir, and
$sync_request_dir can be null and are used immediately; add null guards like in
model/serveraccount.php: after resolving each (using RuntimeState or $GLOBALS)
check if the variable is null and throw a RuntimeException with a clear message
(e.g., "Missing group_dir" / "Missing server_dir" / "Missing sync_request_dir")
before calling $group_dir->list_group_membership(...),
$server_dir->list_servers(...), or $sync_request_dir->add_sync_request(...), so
the code fails fast with a clear error instead of causing a fatal null-method
call.
In `@model/server.php`:
- Around line 304-307: The add_standard_accounts method calls
$group_dir->get_group_by_name but does not guard against
runtime_value('group_dir') being null; add a null-check like the one used in
Group::list_group_membership: after obtaining $group_dir in
add_standard_accounts, verify it's non-null (and return/skip group-related work
or handle the error) before calling $group_dir->get_group_by_name, ensuring you
follow the same control flow and logging/return behavior used in
Group::list_group_membership to avoid a fatal call on null.
In `@model/user.php`:
- Around line 42-46: Extract the repeated RuntimeState-or-$GLOBALS fallback into
a single private helper (e.g., a static resolve(string $key, mixed $default =
null): mixed) placed on the shared Entity base class or a trait so all models
can call it; implement resolve to check class_exists('RuntimeState', false) and
call RuntimeState::get($key, array_key_exists($key, $GLOBALS) ? $GLOBALS[$key] :
$default) otherwise return array_key_exists($key, $GLOBALS) ? $GLOBALS[$key] :
$default, then replace all inline patterns (the LDAP assignment and the other
occurrences that use class_exists('RuntimeState', false) / RuntimeState::get /
$GLOBALS) with calls to Entity::resolve('ldap', null) or the appropriate
key/default.
In `@services/auth.php`:
- Around line 231-246: The sanitize_redirect_path function currently misses CRLF
injection and authority component checks; update sanitize_redirect_path to
reject candidates containing CR or LF characters and to return '/' if parse_url
yields any of 'user', 'pass', or 'port' (align behavior with the stricter
implementation in request_auth_guard.php), keep the existing checks for
scheme/host and leading '//' and preserve query/fragment handling; additionally,
factor this logic out into a single shared utility (e.g., sanitize_redirect_path
in a common helper used by auth.php, login_flow.php, and request_auth_guard.php)
so all call sites reuse the same strict validation.
In `@services/login_flow.php`:
- Around line 92-107: The sanitize_redirect_path implementation in
services/login_flow.php must be replaced by a call to a new shared utility that
consolidates the stronger logic from RequestAuthGuard::sanitize_redirect_path:
create a single function (e.g., sanitize_redirect_path_shared) and move the full
checks into it (reject non-strings/empty, parse_url failure, presence of
scheme/host/user/pass/port, CR/LF characters \r/\n, path must start with single
'/', disallow leading '//' ), ensure it returns '/' on any failure (do not
throw), and update both RequestAuthGuard::sanitize_redirect_path and the
login_flow method to call this shared function; also remove the weaker duplicate
code from login_flow and ensure redirect() still receives a sanitized value.
- Around line 74-90: The session-backed rate limiter in is_rate_limited and
record_failed_attempt is bypassable; replace it with a server-side store (e.g.,
Redis or DB) keyed by username and/or IP: implement record_failed_attempt to
perform an atomic increment and set/refresh an expiry (e.g., INCR with TTL) for
the key, implement is_rate_limited to read the persisted count and compare
against the threshold and time window, ensure successful logins reset/clear the
server-side key, and sanitize/normalize the username/IP used as the key to avoid
injection or key-collision issues.
- Around line 16-17: The current CSRF check calls
hash_equals($_SESSION['csrf_token'], $post_data['csrf_token']) which will throw
a TypeError if $post_data['csrf_token'] is an array; update the conditional
around hash_equals to first ensure both tokens are strings (use
is_string($_SESSION['csrf_token']) and is_string($post_data['csrf_token']))
before calling hash_equals, e.g. add || !is_string($post_data['csrf_token'])
(and check the session token too) so the if in login_flow.php guards against
non-string input and avoids the TypeError when calling hash_equals.
In `@services/request_auth_guard.php`:
- Around line 12-31: resolve_active_user currently does exact comparisons
against $request_context->relative_request_url which can include query strings;
extract the path portion (e.g., via PHP's
parse_url($request_context->relative_request_url, PHP_URL_PATH)) into a local
$request_path_before_query and use that for is_public_route checks and the exact
matches in resolve_active_user (the comparisons against '/login' and '/logout');
keep using sanitize_redirect_path($request_context->request_url) when storing
$_SESSION['redirect_after_login'] so query params intended for redirect are
preserved but ensure all route-matching uses the parsed path variable instead of
the raw relative_request_url.
In `@services/request_context.php`:
- Line 20: The code assigns $context->absolute_request_url using
$_SERVER['HTTP_HOST'], which is client-controlled and risks host header
injection; update the code that constructs $context->absolute_request_url to use
a validated host instead: read an allowed hosts list (e.g., from config.ini),
check $_SERVER['HTTP_HOST'] against that whitelist and reject or replace it if
not allowed, or fall back to $_SERVER['SERVER_NAME'] or a canonical host config
value; ensure the redirect() helper in core.php uses this validated host value
rather than raw $_SERVER['HTTP_HOST'] (look for usages of
$context->absolute_request_url and the redirect() function) so only whitelisted
or canonical hosts are used for absolute URLs.
In `@templates/groups.php`:
- Line 29: The Bootstrap 5 compatibility shim (bootstrap5-compat.js) is missing
the class translation that maps "show" to Bootstrap 3's "in" for tab panes;
update the shim so that when it translates classes for elements with the
tab-pane class (e.g., the element rendered as <div class="tab-pane fade show
active"> in templates/groups.php) it converts "show" → "in" (preserving other
mapped classes like "fade" and "active") so tab panes render correctly under the
Bootstrap 3 tab plugin.
In `@views/servers_bulk_action.php`:
- Around line 66-69: RuntimeState::get('server_dir') may return null so add a
null-guard before mapping over $server_names: check the $server_dir variable
returned by RuntimeState::get('server_dir') and if it's null, set
$selected_servers to an empty array (or handle/log the missing server_dir and
short-circuit) instead of calling $server_dir->get_server_by_hostname(); update
the block that builds $selected_servers (references: $server_dir,
get_server_by_hostname, $selected_servers, $server_names) to safely handle the
missing runtime key.
🟡 Minor comments (15)
docs/phase-9-checkpoint.md-3-4 (1)
3-4:⚠️ Potential issue | 🟡 MinorBranch name mismatch.
The document references branch
bootstrap5-upgrade, but the PR source branch isbootstrap5-upgrade-part1. Update for accuracy.Proposed fix
-Branch: `bootstrap5-upgrade` +Branch: `bootstrap5-upgrade-part1`docs/phase-8-checkpoint.md-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorBranch name mismatch.
The documented branch is
bootstrap5-upgradebut the PR source branch isbootstrap5-upgrade-part1. Update the reference to avoid confusion during rollback.docs/phase-6-checkpoint.md-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorBranch name mismatch.
The checkpoint references branch
bootstrap5-upgrade, but the PR source branch isbootstrap5-upgrade-part1. Update for accuracy so future readers can locate the correct branch.Proposed fix
-Branch: `bootstrap5-upgrade` +Branch: `bootstrap5-upgrade-part1`templates/base.php-78-79 (1)
78-79:⚠️ Potential issue | 🟡 Minor
btn-closewith×text content may cause a double-X icon under Bootstrap 5.Bootstrap 5's
btn-closerenders its close icon via a CSSbackground-imageand expects an empty element. The×text was needed for Bootstrap 3's.closebut will overlay the BS5 icon, producing a visual artifact when the BS5 styles are active.Consider hiding the text content from BS5 rendering, e.g. wrapping it so the compat layer can toggle visibility, or relying on
aria-label="Close"instead ofaria-hidden="true"+ visible text.services/response_security_headers.php-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorDefault CSP
default-src 'self'may break pages with inline scripts or styles.The strict default is a good security posture, but if the app serves any inline
<script>or<style>blocks (e.g., via theheadpage section intemplates/base.phpline 31), they'll be blocked. Ensure the config example ships a CSP that matches the app's actual needs, or thatcsp_report_onlydefaults to1during the migration so breakage is surfaced without blocking users.Also applies to: 41-58
views/pubkeys.php-34-36 (1)
34-36:⚠️ Potential issue | 🟡 MinorGood guard against empty recipient.
Prevents calling
add_recipientwith an empty string, which would likely produce an invalid email. However, if$report_addressis empty,$email->send()on line 45 will still be called with no recipients. Depending on theEmail::send()implementation, this may silently do nothing or could error.#!/bin/bash # Check if Email::send() handles zero recipients gracefully ast-grep --pattern $'class Email { $$$ function send($$$) { $$$ } $$$ }'docs/phase-7-checkpoint.md-19-23 (1)
19-23:⚠️ Potential issue | 🟡 MinorInconsistency: validation command uses
COMPOSER_ALLOW_SUPERUSER=1contrary to runbook guidance.
docs/operations-runbook.md(line 33) explicitly advises against usingCOMPOSER_ALLOW_SUPERUSER=1for local/dev workflows, but the validation evidence here includes it. Consider using the--usermapping approach documented in the runbook, or at minimum note why it was necessary in this context.docs/phase-10-checkpoint.md-3-4 (1)
3-4:⚠️ Potential issue | 🟡 MinorBranch name mismatch.
The document references branch
bootstrap5-upgrade, but this PR's source branch isbootstrap5-upgrade-part1. Update to match the actual branch name to avoid confusion.📝 Proposed fix
-Branch: `bootstrap5-upgrade` +Branch: `bootstrap5-upgrade-part1`services/request_exception_handler.php-16-26 (1)
16-26:⚠️ Potential issue | 🟡 Minor
time()is not a reliable unique error identifier.
time()returns the current Unix timestamp in seconds. If two exceptions occur within the same second, they'll share the same$error_number, making it impossible to correlate a specific user-visible error ID with its log entry.💡 Proposed fix: use a more unique identifier
public function handle($e) { - $error_number = time(); + $error_number = bin2hex(random_bytes(4)) . '-' . time(); error_log("$error_number: ".str_replace("\n", "\n$error_number: ", $e));views/home.php-37-48 (1)
37-48:⚠️ Potential issue | 🟡 MinorVerify that the generic
Exceptioncatch is intentional and sufficiently informative.The
catch(Exception $e)on line 45 silently discards the exception details, showing only a generic message to the user. While this avoids leaking internals, it also means the error is never logged, making debugging harder. Consider logging$e->getMessage()before rendering the error.Also, note that
delete_entity_public_keyinternally calls$entity->delete_public_key($public_key)which triggerssync_remote_access(). If the sync fails, the key may be soft-deleted in the DB but the error surfaces here as "Could not delete the selected public key" — which is misleading since the key was deleted.💡 Proposed: log the exception before presenting the error
} catch(Exception $e) { + error_log('Public key deletion error: ' . $e->getMessage()); $content = new PageSection('key_upload_fail'); $content->set('message', 'Could not delete the selected public key.'); }views/servers_bulk_action.php-25-26 (1)
25-26:⚠️ Potential issue | 🟡 Minor
active_userfrom RuntimeState has no null guard;configdefault[]will break email access.
RuntimeState::get('active_user')returnsnullif not set, which would cause a fatal on$active_user->name. The empty-array default forconfigwould throw on$config['email']['report_address'](line 32). While the caller context should guarantee these exist, the model-layer methods that also use RuntimeState consistently guard or fall back to$GLOBALS. Consider adding a fallback or guard here as well.scripts/smoke/sync-preview.sh-40-43 (1)
40-43:⚠️ Potential issue | 🟡 MinorEmpty array expansion under
set -umay fail on Bash < 4.4.When
SYNC_USER_ARGis an empty array(),"${SYNC_USER_ARG[@]}"triggers an "unbound variable" error in Bash versions prior to 4.4. If this is expected to run on older systems (e.g., CentOS 7 ships Bash 4.2), consider guarding:🔧 Safer expansion
( cd "$REPO_ROOT" - php scripts/sync.php --id "$SKA_SMOKE_SYNC_SERVER_ID" --preview "${SYNC_USER_ARG[@]}" + php scripts/sync.php --id "$SKA_SMOKE_SYNC_SERVER_ID" --preview ${SYNC_USER_ARG[@]+"${SYNC_USER_ARG[@]}"} ) > "$RAW_OUTPUT"services/request_auth_guard.php-34-44 (1)
34-44:⚠️ Potential issue | 🟡 MinorRoute patterns are not escaped — regex metacharacters in route strings will misfire.
$routeis inserted into the regex withoutpreg_quote(). Characters like.,?,+,(,)in a route string will be interpreted as regex operators. For example, a route/api/v1.0/statuswould also match/api/v1X0/status.Escape the route before replacing placeholders:
Proposed fix
private function is_public_route($request_path) { foreach($this->public_routes as $route => $is_public) { if($is_public) { - $pattern = preg_replace('/\{[^}]+\}/', '[^/]+', $route); + $escaped = preg_quote($route, '|'); + $pattern = preg_replace('/\\\\\\{[^}]+\\\\\\}/', '[^/]+', $escaped); if(preg_match('|^'.$pattern.'$|', $request_path)) { return true; }services/login_flow.php-22-25 (1)
22-25:⚠️ Potential issue | 🟡 MinorEmpty username receives misleading error message.
When
$usernameis''(aftertrim), the regex'/^[a-zA-Z0-9._-]+$/'fails first (since+requires ≥1 char), so the user sees "Invalid username format" instead of the more appropriate "Please enter both username and password." from line 24. Swap the checks so the emptiness test runs first.Proposed fix
- if(!preg_match('/^[a-zA-Z0-9._-]+$/', $username)) { - $error_message = 'Invalid username format. Username can only contain letters, numbers, dots, hyphens, and underscores.'; - } elseif($username === '' || $password === '') { + if($username === '' || $password === '') { $error_message = 'Please enter both username and password.'; + } elseif(!preg_match('/^[a-zA-Z0-9._-]+$/', $username)) { + $error_message = 'Invalid username format. Username can only contain letters, numbers, dots, hyphens, and underscores.';services/relation_lifecycle_service.php-97-115 (1)
97-115:⚠️ Potential issue | 🟡 MinorPartial failures in
reassign_server_admins_by_hostnameare silently swallowed.If
add_server_adminsucceeds butdelete_server_adminthrows, the server ends up with both admins — the old one is never removed. Errors are only logged, not returned to the caller. Consider returning the$errorsarray so the caller can surface feedback (e.g., alert the admin), and wrapping the add+delete pair so that a failed delete can at least attempt to roll back the add.Suggestion: return errors to caller
- public function reassign_server_admins_by_hostname(array $servers, array $selected_server_hostnames, Entity $from_admin, Entity $to_admin): void { + public function reassign_server_admins_by_hostname(array $servers, array $selected_server_hostnames, Entity $from_admin, Entity $to_admin): array { $errors = array(); foreach($servers as $server) { ... } foreach($errors as $error) { error_log('reassign_server_admins_by_hostname: '.$error); } + return $errors; }
🧹 Nitpick comments (40)
templates/user_pubkeys.php (1)
36-36: Incomplete Bootstrap 5 migration — onlyform-group→mb-3updated.The rest of the file still uses Bootstrap 3 classes (
panel,panel-heading,panel-body,glyphicon,btn-default,btn-xs,help-block). If these are handled by the compatibility layer (bootstrap5-compat.css/js), this is fine. Otherwise, consider tracking remaining migrations for this template.templates/group.php (1)
373-417: Inconsistent migration: the "Add administrator" and "Settings" forms still use BS3-only classes.Lines 376 and 389–416 still use
form-group,form-horizontal,control-label, andcol-sm-offset-2, while the rest of the file has been migrated. Consider migrating these in the same pass for consistency, or note them for a follow-up.public_html/extra.js (1)
41-41: Tab selector only matchesdata-toggle, notdata-bs-toggle.Line 41 uses
$('a[data-toggle="tab"]:first')as a fallback when no hash fragment matches. Since the templates now emit bothdata-toggle="tab"anddata-bs-toggle="tab", this works today, but once the legacydata-toggleattribute is removed in a future phase, this fallback (and theshown.bs.tabbinding on line 46) will silently stop working.Consider updating the selectors to match either attribute now:
$('a[data-toggle="tab"], a[data-bs-toggle="tab"]').first()templates/pubkeys.php (2)
66-69: Dualdata-toggleanddata-bs-toggleattributes are redundant given the JS shim.The
bootstrap5-compat.jsshim already mapsdata-bs-toggle→data-toggleat runtime. Having both hardcoded means the shim's attribute check (!hasAttribute(dstAttr)) will finddata-togglealready present and skip the mapping — which works, but makes the shim unnecessary for these elements. Consider using onlydata-bs-togglehere (the forward-looking attribute) and letting the shim handle the backfill, so you don't have to revisit these lines when BS3 is eventually removed.
86-107:mb-3migration is correct; note thatinput-group-addonis BS3-only.The
form-group→mb-3change is appropriate for BS5. However,input-group-addon(lines 97, 104) is a Bootstrap 3 class — in BS5 it becomesinput-group-text. Since this file is being touched for BS5 compat, consider adding the BS5 class alongside or updating it to avoid a second pass later.public_html/bootstrap5-compat.js (1)
6-63: Shim only runs once — dynamically inserted elements won't be patched.The compatibility mapping executes once at
DOMContentLoaded. If any page uses AJAX or JS to insert elements withdata-bs-*attributes after initial load, those elements won't receive the BS3 equivalents. If that's not a concern for this codebase (i.e., all relevant markup is server-rendered), this is fine. Otherwise, consider exposingmapBootstrap5ToBootstrap3so it can be re-invoked after dynamic DOM mutations.templates/serveraccount.php (1)
51-59: Inconsistent tab markup — missingnav-item/nav-linkclasses unlikegroups.php.In
templates/groups.php(lines 22-23), tab links were updated withnav-itemon<li>andnav-linkon<a>. Here, onlydata-bs-toggle="tab"was added without the corresponding Bootstrap 5 nav classes. This inconsistency may cause styling differences when the full Bootstrap 5 migration completes.♻️ Suggested fix for consistency
<ul class="nav nav-tabs"> <?php if($this->get('server')->key_management == 'keys') { ?> - <li><a href="#access" data-toggle="tab" data-bs-toggle="tab">Access</a></li> + <li class="nav-item"><a href="#access" class="nav-link" data-toggle="tab" data-bs-toggle="tab">Access</a></li> <?php } ?> - <li><a href="#pubkeys" data-toggle="tab" data-bs-toggle="tab">Public keys</a></li> - <li><a href="#outbound" data-toggle="tab" data-bs-toggle="tab">Outbound access</a></li> - <li><a href="#admins" data-toggle="tab" data-bs-toggle="tab">Leaders</a></li> - <li><a href="#log" data-toggle="tab" data-bs-toggle="tab">Log</a></li> + <li class="nav-item"><a href="#pubkeys" class="nav-link" data-toggle="tab" data-bs-toggle="tab">Public keys</a></li> + <li class="nav-item"><a href="#outbound" class="nav-link" data-toggle="tab" data-bs-toggle="tab">Outbound access</a></li> + <li class="nav-item"><a href="#admins" class="nav-link" data-toggle="tab" data-bs-toggle="tab">Leaders</a></li> + <li class="nav-item"><a href="#log" class="nav-link" data-toggle="tab" data-bs-toggle="tab">Log</a></li> </ul>services/response_security_headers.php (1)
100-114:is_https_request()duplicated inservices/request_context.php.
request_context.php(lines 25-42) contains an identical fallback implementation and conditionally delegates to this class. This is fine for decoupling, but consider documenting which is the canonical source of truth to avoid future drift.scripts/smoke/lib.sh (1)
29-31:smoke_urlencodedepends onphpbeing available.This works for this project, but note that
smoke_require_cmd phpmust be called before any script uses this function. Theweb-workflows.shscript does callsmoke_require_cmd php(line 9), so this is fine there — just something to keep in mind for other consumers oflib.sh.scripts/smoke/web-workflows.sh (1)
33-40: Credentials visible in process listing.The
--data-urlencode "password=$SKA_SMOKE_PASSWORD"on line 37 exposes the password in the process's command line (visible viapsor/proc/*/cmdline). For a smoke test this is likely acceptable, but if the smoke environment shares infrastructure, consider piping credentials from a file or stdin instead.Example: pass password via stdin
-curl -fsS -L -b "$COOKIE_JAR" -c "$COOKIE_JAR" \ - --data-urlencode "csrf_token=$LOGIN_CSRF" \ - --data-urlencode "username=$SKA_SMOKE_USERNAME" \ - --data-urlencode "password=$SKA_SMOKE_PASSWORD" \ - "$BASE_URL/login" -o "$TMP_DIR/post-login.html" +curl -fsS -L -b "$COOKIE_JAR" -c "$COOKIE_JAR" \ + --data-urlencode "csrf_token=$LOGIN_CSRF" \ + --data-urlencode "username=$SKA_SMOKE_USERNAME" \ + --data-urlencode "password@$TMP_DIR/.pw" \ + "$BASE_URL/login" -o "$TMP_DIR/post-login.html"Where
$TMP_DIR/.pwis written securely beforehand withprintf '%s' "$SKA_SMOKE_PASSWORD" > "$TMP_DIR/.pw".scripts/sync-runtime.php (1)
45-51:diagnostics()hardcodestimeout_seconds => 60butbuild_timeout_wrapped_commandaccepts a configurable timeout.If a caller passes a different timeout to
build_timeout_wrapped_command, the diagnostics output will be misleading. Consider accepting$timeout_secondsas a parameter or documenting that this reflects the default only.♻️ Suggested improvement
- public static function diagnostics(array $config) { + public static function diagnostics(array $config, $timeout_seconds = 60) { return array( 'timeout_util' => self::resolve_timeout_util($config), 'timeout_binary' => self::resolve_timeout_binary($config), - 'timeout_seconds' => 60, + 'timeout_seconds' => $timeout_seconds, ); }services/access_rule_service.php (1)
52-70: Delete methods silently no-op when the access ID is not found.Both
delete_access_by_idanddelete_group_access_by_idsilently do nothing iffind_access_by_idreturnsnull. This is likely intentional for form resubmission / race conditions, but consider whether callers would benefit from knowing the delete was a no-op (e.g., returning a boolean).scripts/sync-failure.php (2)
138-149:normalize_reasonis called twice with the same input incompose_message.Lines 141 and 142 both call
self::normalize_reason($reason). Store the result in a local variable to avoid the redundant work.♻️ Proposed fix
private static function compose_message($summary, $reason, $code, $reschedule) { $parts = array(); $parts[] = trim((string)$summary); - if(self::normalize_reason($reason) !== '') { - $parts[] = 'reason='.self::normalize_reason($reason); + $normalized_reason = self::normalize_reason($reason); + if($normalized_reason !== '') { + $parts[] = 'reason='.$normalized_reason; } $parts[] = 'code='.$code; if($reschedule) {
48-88:classify_reasonfalls back tossh_connection_failedfor all unrecognized patterns.If a non-SSH error (e.g., a database exception or file system error) passes through here without matching any pattern, it will be misclassified as
ssh_connection_failed. Consider whether a more generic fallback likeunknown_failurewould be more accurate, or document that this method is intentionally SSH-scoped.scripts/sync.php (1)
163-175: Diagnostics output is useful for runtime troubleshooting.The function aggregates diagnostics from
SyncRuntime,SSH, andSyncFailureReporterinto a simple key=value format. One note: if any of these diagnostic calls throw (e.g., due to a missing config key), the--diagnosticsinvocation will fail with an unhandled exception rather than a user-friendly message.scripts/ssh.php (1)
208-217:summarize_stderruses 200-char truncation whilenormalize_reasoninsync-failure.phpuses 240.Both serve similar purposes (bounding noisy error output). The difference is minor but could be unified into a shared utility if desired.
composer.json (1)
19-29: Consider adding@format:checkto theqascript.The
qacomposite script runs@lintand@stanbut not@format:check. If formatting consistency is desired as a quality gate, adding it here would catch formatting drift early. If it's intentionally separate (e.g., to avoid blocking CI on style), this is fine as-is..php-cs-fixer.dist.php (2)
3-9: Consider including theviews/directory in the fixer scope.Views contain PHP code (e.g.,
views/user_pubkeys.php) and would benefit from consistent formatting. If excluded intentionally due to mixed HTML/PHP templates, a brief comment in the config explaining the omission would help future contributors.
13-16:line_endingis already part of@PSR12.The explicit
'line_ending' => trueis redundant since the@PSR12ruleset already includes it.views/user_pubkeys.php (1)
28-28: MoveKeyLifecycleServiceinstantiation inside the POST block.The service is only used within the
if(isset($_POST['add_public_key']))branch but is instantiated on every request, including GET.♻️ Suggested change
-$key_lifecycle_service = new KeyLifecycleService(); if(isset($_POST['add_public_key'])) { if(!$can_submit_key) { require('views/error403.php'); die; } try { + $key_lifecycle_service = new KeyLifecycleService(); $key_lifecycle_service->add_user_public_key($user, $_POST['add_public_key']);model/externalkey.php (1)
46-56: Extract the duplicated database-resolution logic into a shared helper.Lines 46–56 and 100–110 are identical. During the migration, if you adjust the fallback chain, you'd need to update both. A
private statichelper would eliminate this risk:♻️ Proposed refactor
Add a helper method to the class:
private static function resolve_database(): \mysqli { if (class_exists('RuntimeState', false)) { $database = RuntimeState::get('database', null); } else { $database = null; } if (is_null($database) && array_key_exists('database', $GLOBALS)) { $database = $GLOBALS['database']; } if (is_null($database)) { throw new BadMethodCallException('Database connection is not available.'); } return $database; }Then replace both blocks:
public static function list_external_keys($with_hostnames = false) { - if(class_exists('RuntimeState', false)) { - $database = RuntimeState::get('database', null); - } else { - $database = null; - } - if(is_null($database) && array_key_exists('database', $GLOBALS)) { - $database = $GLOBALS['database']; - } - if(is_null($database)) { - throw new BadMethodCallException('Database connection is not available.'); - } + $database = self::resolve_database();docs/operations-runbook.md (1)
70-78: Minor: repetitive sentence starts in the deployment checklist.Lines 74-77 have three consecutive steps starting with "Verify". Consider varying the phrasing for readability (e.g., "Confirm", "Check", "Ensure").
model/publickey.php (1)
28-33: Clean runtime config abstraction preserving key validation security defaults.The fallback chain (RuntimeState → $GLOBALS → empty array) is correct, and the
falseparameter toclass_existsavoids triggering autoloading side effects.The duplicated
array_key_exists('config', $GLOBALS) ? $GLOBALS['config'] : array()expression on lines 30 and 32 could be extracted into a local variable for DRY, but it's a minor nit.Optional: extract repeated fallback expression
private static function get_runtime_config() { + $globals_config = array_key_exists('config', $GLOBALS) ? $GLOBALS['config'] : array(); if(class_exists('RuntimeState', false)) { - return RuntimeState::get('config', array_key_exists('config', $GLOBALS) ? $GLOBALS['config'] : array()); + return RuntimeState::get('config', $globals_config); } - return array_key_exists('config', $GLOBALS) ? $GLOBALS['config'] : array(); + return $globals_config; }ldap.php (1)
185-197: Array-valued LDAP attributes are joined with commas for sorting — verify this is acceptable.When an LDAP entry has a multi-valued attribute (e.g.,
memberOfwith multiple DNs),normalize_sort_valuejoins them with commas. The sort result depends on the order LDAP returns these values, which is typically undefined. If any sort field can be multi-valued, this could produce non-deterministic ordering.For the common case (sorting by
cn,uid,sAMAccountName) these are single-valued, so this is fine. Just noting the edge case.model/entity.php (1)
37-47: Inconsistent fallback priority:RuntimeState→$GLOBALShere, but$GLOBALS→RuntimeStateinRecord::__construct.
Entity::load()checksRuntimeStatefirst, then falls back to$GLOBALS['database']. However,Record::__construct()(seemodel/record.phplines 51–60) does the opposite — it checks$GLOBALSfirst, thenRuntimeState. During the transition period where both mechanisms coexist, this inconsistency could cause different code paths to resolve different database instances if the two sources ever diverge.Consider standardizing the priority order across the codebase.
services/request_exception_handler.php (1)
7-9: Constructor$active_userparameter is untyped, butset_active_userrequiresUser.The constructor accepts any value for
$active_user(includingnull, which is the expected initial state before authentication). This asymmetry is correct, but adding a?Usertype hint to the constructor parameter would make the contract explicit and prevent accidentally passing an incompatible type.♻️ Proposed
- public function __construct($active_user, &$config) { + public function __construct(?User $active_user, array &$config) {scripts/smoke/run.sh (1)
20-25:--web-onlyand--sync-onlyare mutually exclusive but not enforced.If both are passed, the last one silently wins. This is unlikely in practice (Makefile targets pass one at a time), but a defensive check or a note in a
--helpmessage would prevent confusion.services/request_policy_guard.php (1)
18-20: Strict comparison forauth_realm.
==works here since both sides are strings, but===is the safer default to prevent any future type-coercion surprises.♻️ Proposed
- return $is_public || ($active_user && $active_user->auth_realm == 'LDAP'); + return $is_public || ($active_user && $active_user->auth_realm === 'LDAP');scripts/smoke/sync-preview.sh (1)
60-61: Consider showing diff output on failure to aid debugging.When the diff fails, only the fixture path is reported. Showing the actual diff (or a snippet) would help diagnose mismatches without requiring a manual re-run.
♻️ Show diff output on failure
-diff -u "$FIXTURE_PATH" "$NORMALIZED_OUTPUT" >/dev/null \ - || smoke_die "Sync preview differs from fixture: $FIXTURE_PATH" +if ! diff -u "$FIXTURE_PATH" "$NORMALIZED_OUTPUT"; then + smoke_die "Sync preview differs from fixture: $FIXTURE_PATH" +fiservices/runtime_state.php (1)
3-25: Add parameter and return type declarations for PHP 8.2+ compliance.Per coding guidelines requiring PHP 8.2+ compatibility, consider adding type declarations. This also improves IDE support and static analysis.
♻️ Type-hinted version
class RuntimeState { private static $state = array(); - public static function set($key, $value) { + public static function set(string $key, mixed $value): void { self::$state[$key] = $value; } - public static function set_many(array $values) { + public static function set_many(array $values): void { foreach($values as $key => $value) { self::$state[$key] = $value; } } - public static function has($key) { + public static function has(string $key): bool { return array_key_exists($key, self::$state); } - public static function get($key, $default = null) { + public static function get(string $key, mixed $default = null): mixed { if(self::has($key)) { return self::$state[$key]; } return $default; } }As per coding guidelines:
**/*.php: Ensure PHP 8.2+ compatibility for all PHP code.model/serveraccount.php (1)
419-422: Inconsistent exception type vs.Group::list_group_membership.
GroupthrowsDomainExceptionfor the same null-guard scenario (line 440 in model/group.php), whileServerAccountthrowsRuntimeException. Using the same exception type would make it easier for callers to handle consistently.model/group.php (1)
23-32: Extract duplicatedruntime_value/runtime_confighelpers into a shared trait.These two methods are byte-for-byte identical in
model/group.php,model/server.php, andmodel/serveraccount.php. Since the project requires PHP 8.2+, extract them into a trait (e.g.,RuntimeConfigAware) to eliminate duplication:♻️ Example trait extraction
trait RuntimeConfigAware { private static function runtime_value($key, $default = null) { if(class_exists('RuntimeState', false)) { return RuntimeState::get($key, $default); } return $default; } private static function runtime_config() { return self::runtime_value('config', array_key_exists('config', $GLOBALS) ? $GLOBALS['config'] : array()); } }Then in each model:
class Group extends Entity { - private static function runtime_value($key, $default = null) { - if(class_exists('RuntimeState', false)) { - return RuntimeState::get($key, $default); - } - return $default; - } - - private static function runtime_config() { - return self::runtime_value('config', array_key_exists('config', $GLOBALS) ? $GLOBALS['config'] : array()); - } + use RuntimeConfigAware;services/request_context.php (2)
36-41:X-Forwarded-Protois trusted without verifying the source is a trusted proxy.A client can send
X-Forwarded-Proto: httpsdirectly to trick the app into believing the connection is secure. This is typically mitigated by stripping or overwriting this header at the reverse proxy, but if the app is ever exposed directly, it becomes exploitable. Low risk if deployment is always behind a proxy, but worth noting.
3-9: Consider adding property type declarations for PHP 8.2+ alignment.All six public properties are untyped. Adding explicit types improves IDE support, static analysis, and aligns with the PHP 8.2+ coding guideline.
Suggested types
class RequestContext { - public $base_url; - public $request_url; - public $relative_request_url; - public $absolute_request_url; - public $request_method; - public $bypass_csrf_protection; + public string $base_url = ''; + public string $request_url = ''; + public string $relative_request_url = ''; + public string $absolute_request_url = ''; + public string $request_method = 'GET'; + public bool $bypass_csrf_protection = false;views/user.php (2)
34-48: The implicit-variable guard chain works but is fragile.The flow relies on
$new_adminbeing undefined (not just null) to skip line 45 in the empty-input path. This is correct in PHP's request lifecycle, but the coupling betweenisset($content)andisset($new_admin)across non-contiguous blocks makes the logic harder to follow. An earlyreturnorelse-chain would be clearer, but this matches the existing codebase style, so no action needed.
49-57: Key deletion return value is ignored — silent no-op on invalid key ID.
delete_entity_public_keyreturnsfalsewhen the key isn't found, but line 54 discards the result. If an admin submits a stale or tampered key ID, the deletion silently fails with no user feedback. This may be acceptable (matches prior behavior), but consider logging or alerting onfalse.Based on learnings: validate any change that touches SSH key generation or distribution, and keep audit/event logging intact when altering access flows.
services/key_lifecycle_service.php (1)
48-57:(int)cast on arbitrary input silently produces 0 for non-numeric strings.If
$public_key_idis"abc",$target_idbecomes0. While DB auto-increment IDs typically start at 1 (making a false match unlikely), a defensive early-return for non-positive IDs would be safer since this is a public method callable from various contexts.Proposed guard
public function find_public_key_by_id(array $existing_public_keys, $public_key_id): ?PublicKey { $target_id = (int)$public_key_id; + if($target_id <= 0) { + return null; + } foreach($existing_public_keys as $public_key) {views/servers.php (1)
129-129:RuntimeState::get('server_dir')assumes bootstrap has set the value.If called before
server_diris registered (e.g., in a test or CLI context), this returnsnulland the subsequent$server_dir->add_server()on line 140 will throw a fatalCall to a member function on null. Consider providing a guard or explicit assertion. Same pattern appears at line 27 infind_server_accountsand likely elsewhere, so this could be addressed holistically.views/group.php (1)
82-84: Raw$_POST['delete_access']silently converts non-numeric input to0via integer casting.The service method
delete_group_access_by_idcallsfind_access_by_id, which uses(int)$access_idto cast the ID. A non-numeric payload from$_POST['delete_access']will become0, which may silently fail to match any access rule in the list. This differs fromdelete_group_admin_by_idanddelete_group_member_by_entity_id, which safely cast to strings for comparison.While the service method validates IDs against a pre-fetched access list (mitigating actual harm), consistency with
views/user.php(which usesfilter_var(..., FILTER_VALIDATE_INT)before the service call) would improve defense-in-depth and code clarity across the application.Consider adding input validation:
Example validation
} elseif(isset($_POST['delete_access']) && ($group_admin || $active_user->admin)) { - $access_rule_service->delete_group_access_by_id($group, $group_access, $_POST['delete_access']); + $access_id = filter_var($_POST['delete_access'], FILTER_VALIDATE_INT); + if($access_id !== false) { + $access_rule_service->delete_group_access_by_id($group, $group_access, $access_id); + } redirect('#access');Makefile (1)
1-1: Consider adding conventionalallandtesttargets; add trailing newline.
checkmakeflags missingall,clean, andtestphony targets. At minimum, atestalias (perhaps pointing toqaorsmoke) would be useful for developer ergonomics. Also, the file is missing a trailing newline after line 45, which violates POSIX conventions and can cause warnings in some tools.Proposed additions
-.PHONY: lint stan qa format format-check composer-validate composer-audit platform-check docker-config-check ci-check smoke smoke-dry-run smoke-web smoke-sync smoke-sync-record +.PHONY: all test clean lint stan qa format format-check composer-validate composer-audit platform-check docker-config-check ci-check smoke smoke-dry-run smoke-web smoke-sync smoke-sync-record + +all: ci-check + +test: qa + +clean: + `@echo` "No build artifacts to clean." lint:At line 45, ensure the file ends with a newline character.
Also applies to: 44-45
| $group_dir = class_exists('RuntimeState', false) | ||
| ? RuntimeState::get('group_dir', array_key_exists('group_dir', $GLOBALS) ? $GLOBALS['group_dir'] : null) | ||
| : (array_key_exists('group_dir', $GLOBALS) ? $GLOBALS['group_dir'] : null); | ||
| $memberships = $group_dir->list_group_membership($this); | ||
| foreach($memberships as $group) { | ||
| if(!isset($seen[$group->entity_id])) { | ||
| $group->sync_remote_access($seen); | ||
| } | ||
| } | ||
| // If this is a user, also sync across LDAP-based servers | ||
| global $server_dir; | ||
| global $sync_request_dir; | ||
| $server_dir = class_exists('RuntimeState', false) | ||
| ? RuntimeState::get('server_dir', array_key_exists('server_dir', $GLOBALS) ? $GLOBALS['server_dir'] : null) | ||
| : (array_key_exists('server_dir', $GLOBALS) ? $GLOBALS['server_dir'] : null); | ||
| $sync_request_dir = class_exists('RuntimeState', false) | ||
| ? RuntimeState::get('sync_request_dir', array_key_exists('sync_request_dir', $GLOBALS) ? $GLOBALS['sync_request_dir'] : null) | ||
| : (array_key_exists('sync_request_dir', $GLOBALS) ? $GLOBALS['sync_request_dir'] : null); |
There was a problem hiding this comment.
Missing null checks on $group_dir, $server_dir, and $sync_request_dir — potential NPE.
If neither RuntimeState nor $GLOBALS provides these directory objects, the variables will be null, and the subsequent method calls (line 487: $group_dir->list_group_membership(...), line 501: $server_dir->list_servers(...), line 506: $sync_request_dir->add_sync_request(...)) will cause a fatal error.
The same pattern is already handled correctly in model/serveraccount.php (lines 417–423), which checks for null and throws a clear RuntimeException.
🐛 Proposed fix: add null guards consistent with serveraccount.php
$group_dir = class_exists('RuntimeState', false)
? RuntimeState::get('group_dir', array_key_exists('group_dir', $GLOBALS) ? $GLOBALS['group_dir'] : null)
: (array_key_exists('group_dir', $GLOBALS) ? $GLOBALS['group_dir'] : null);
+ if($group_dir === null) {
+ throw new RuntimeException('Group directory service is unavailable; cannot sync remote access.');
+ }
$memberships = $group_dir->list_group_membership($this); $server_dir = class_exists('RuntimeState', false)
? RuntimeState::get('server_dir', array_key_exists('server_dir', $GLOBALS) ? $GLOBALS['server_dir'] : null)
: (array_key_exists('server_dir', $GLOBALS) ? $GLOBALS['server_dir'] : null);
+ if($server_dir === null) {
+ throw new RuntimeException('Server directory service is unavailable; cannot sync LDAP servers.');
+ }
$sync_request_dir = class_exists('RuntimeState', false)
? RuntimeState::get('sync_request_dir', array_key_exists('sync_request_dir', $GLOBALS) ? $GLOBALS['sync_request_dir'] : null)
: (array_key_exists('sync_request_dir', $GLOBALS) ? $GLOBALS['sync_request_dir'] : null);
+ if($sync_request_dir === null) {
+ throw new RuntimeException('Sync request directory service is unavailable; cannot queue sync requests.');
+ }🤖 Prompt for AI Agents
In `@model/entity.php` around lines 484 - 499, The variables $group_dir,
$server_dir, and $sync_request_dir can be null and are used immediately; add
null guards like in model/serveraccount.php: after resolving each (using
RuntimeState or $GLOBALS) check if the variable is null and throw a
RuntimeException with a clear message (e.g., "Missing group_dir" / "Missing
server_dir" / "Missing sync_request_dir") before calling
$group_dir->list_group_membership(...), $server_dir->list_servers(...), or
$sync_request_dir->add_sync_request(...), so the code fails fast with a clear
error instead of causing a fatal null-method call.
| */ | ||
| public function add_standard_accounts() { | ||
| global $group_dir, $config; | ||
| $group_dir = self::runtime_value('group_dir', array_key_exists('group_dir', $GLOBALS) ? $GLOBALS['group_dir'] : null); | ||
| $config = self::runtime_config(); |
There was a problem hiding this comment.
Missing null guard for group_dir in add_standard_accounts.
If group_dir resolves to null, line 314 ($group_dir->get_group_by_name(...)) will fatal. Add a guard consistent with the pattern used in Group::list_group_membership.
🛡️ Proposed guard
public function add_standard_accounts() {
$group_dir = self::runtime_value('group_dir', array_key_exists('group_dir', $GLOBALS) ? $GLOBALS['group_dir'] : null);
+ if($group_dir === null) {
+ throw new DomainException('Group directory service is unavailable; cannot add standard accounts.');
+ }
$config = self::runtime_config();🤖 Prompt for AI Agents
In `@model/server.php` around lines 304 - 307, The add_standard_accounts method
calls $group_dir->get_group_by_name but does not guard against
runtime_value('group_dir') being null; add a null-check like the one used in
Group::list_group_membership: after obtaining $group_dir in
add_standard_accounts, verify it's non-null (and return/skip group-related work
or handle the error) before calling $group_dir->get_group_by_name, ensuring you
follow the same control flow and logging/return behavior used in
Group::list_group_membership to avoid a fatal call on null.
| private function sanitize_redirect_path($candidate) { | ||
| if(!is_string($candidate) || $candidate === '') { | ||
| return '/'; | ||
| } | ||
| $parts = parse_url($candidate); | ||
| if($parts === false || isset($parts['scheme']) || isset($parts['host'])) { | ||
| return '/'; | ||
| } | ||
| $path = $parts['path'] ?? '/'; | ||
| if($path === '' || substr($path, 0, 1) !== '/' || substr($path, 0, 2) === '//') { | ||
| return '/'; | ||
| } | ||
| $query = isset($parts['query']) ? '?'.$parts['query'] : ''; | ||
| $fragment = isset($parts['fragment']) ? '#'.$parts['fragment'] : ''; | ||
| return $path.$query.$fragment; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicate sanitize_redirect_path is weaker than the version in RequestAuthGuard.
This method is a near-copy of RequestAuthGuard::sanitize_redirect_path (in services/request_auth_guard.php, lines 45-66) but omits checks for \r/\n newlines, and user/pass/port URL components. While redirect() in core.php does a newline check downstream, it throws an exception rather than gracefully falling back to /. The missing user/pass/port checks create an inconsistency in security posture.
Extract this into a shared utility to avoid divergence.
🤖 Prompt for AI Agents
In `@services/login_flow.php` around lines 92 - 107, The sanitize_redirect_path
implementation in services/login_flow.php must be replaced by a call to a new
shared utility that consolidates the stronger logic from
RequestAuthGuard::sanitize_redirect_path: create a single function (e.g.,
sanitize_redirect_path_shared) and move the full checks into it (reject
non-strings/empty, parse_url failure, presence of scheme/host/user/pass/port,
CR/LF characters \r/\n, path must start with single '/', disallow leading '//'
), ensure it returns '/' on any failure (do not throw), and update both
RequestAuthGuard::sanitize_redirect_path and the login_flow method to call this
shared function; also remove the weaker duplicate code from login_flow and
ensure redirect() still receives a sanitized value.
| public function resolve_active_user($request_context) { | ||
| $request_path = $request_context->relative_request_url; | ||
| $active_user = $this->auth_service->getCurrentUser(); | ||
|
|
||
| if(!$active_user && !$this->is_public_route($request_path)) { | ||
| $_SESSION['redirect_after_login'] = $this->sanitize_redirect_path($request_context->request_url); | ||
| redirect('/login'); | ||
| } | ||
|
|
||
| if($active_user && $request_path === '/login') { | ||
| $redirect_url = $this->sanitize_redirect_path($_SESSION['redirect_after_login'] ?? '/'); | ||
| unset($_SESSION['redirect_after_login']); | ||
| redirect($redirect_url); | ||
| } | ||
|
|
||
| if(!$active_user && $request_path === '/logout') { | ||
| redirect('/login'); | ||
| } | ||
|
|
||
| return $active_user; |
There was a problem hiding this comment.
relative_request_url includes the query string — exact-match comparisons will break.
RequestContext::$relative_request_url is derived from REQUEST_URI, which includes the query string (e.g., /login?next=/dashboard). The exact comparisons on lines 21 and 27 (=== '/login', === '/logout') will fail when a query string is present. Similarly, is_public_route() patterns won't match paths that carry query parameters.
Extract the path component before matching:
Proposed fix
public function resolve_active_user($request_context) {
- $request_path = $request_context->relative_request_url;
+ $request_path = parse_url($request_context->relative_request_url, PHP_URL_PATH) ?? '/';
$active_user = $this->auth_service->getCurrentUser();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function resolve_active_user($request_context) { | |
| $request_path = $request_context->relative_request_url; | |
| $active_user = $this->auth_service->getCurrentUser(); | |
| if(!$active_user && !$this->is_public_route($request_path)) { | |
| $_SESSION['redirect_after_login'] = $this->sanitize_redirect_path($request_context->request_url); | |
| redirect('/login'); | |
| } | |
| if($active_user && $request_path === '/login') { | |
| $redirect_url = $this->sanitize_redirect_path($_SESSION['redirect_after_login'] ?? '/'); | |
| unset($_SESSION['redirect_after_login']); | |
| redirect($redirect_url); | |
| } | |
| if(!$active_user && $request_path === '/logout') { | |
| redirect('/login'); | |
| } | |
| return $active_user; | |
| public function resolve_active_user($request_context) { | |
| $request_path = parse_url($request_context->relative_request_url, PHP_URL_PATH) ?? '/'; | |
| $active_user = $this->auth_service->getCurrentUser(); | |
| if(!$active_user && !$this->is_public_route($request_path)) { | |
| $_SESSION['redirect_after_login'] = $this->sanitize_redirect_path($request_context->request_url); | |
| redirect('/login'); | |
| } | |
| if($active_user && $request_path === '/login') { | |
| $redirect_url = $this->sanitize_redirect_path($_SESSION['redirect_after_login'] ?? '/'); | |
| unset($_SESSION['redirect_after_login']); | |
| redirect($redirect_url); | |
| } | |
| if(!$active_user && $request_path === '/logout') { | |
| redirect('/login'); | |
| } | |
| return $active_user; |
🤖 Prompt for AI Agents
In `@services/request_auth_guard.php` around lines 12 - 31, resolve_active_user
currently does exact comparisons against $request_context->relative_request_url
which can include query strings; extract the path portion (e.g., via PHP's
parse_url($request_context->relative_request_url, PHP_URL_PATH)) into a local
$request_path_before_query and use that for is_public_route checks and the exact
matches in resolve_active_user (the comparisons against '/login' and '/logout');
keep using sanitize_redirect_path($request_context->request_url) when storing
$_SESSION['redirect_after_login'] so query params intended for redirect are
preserved but ensure all route-matching uses the parsed path variable instead of
the raw relative_request_url.
| $server_dir = RuntimeState::get('server_dir'); | ||
| $selected_servers = array_map(function($name) use ($server_dir) { | ||
| return $server_dir->get_server_by_hostname($name); | ||
| }, $server_names); |
There was a problem hiding this comment.
No null guard on server_dir — will fatal if RuntimeState key is missing.
If RuntimeState::get('server_dir') returns null, line 68 calls ->get_server_by_hostname() on null, causing a fatal error. Other model methods (e.g., Server::sync_access, Group::list_group_membership) guard against this.
🛡️ Proposed fix
$server_dir = RuntimeState::get('server_dir');
+if($server_dir === null) {
+ require('views/error500.php');
+ die;
+}
$selected_servers = array_map(function($name) use ($server_dir) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $server_dir = RuntimeState::get('server_dir'); | |
| $selected_servers = array_map(function($name) use ($server_dir) { | |
| return $server_dir->get_server_by_hostname($name); | |
| }, $server_names); | |
| $server_dir = RuntimeState::get('server_dir'); | |
| if($server_dir === null) { | |
| require('views/error500.php'); | |
| die; | |
| } | |
| $selected_servers = array_map(function($name) use ($server_dir) { | |
| return $server_dir->get_server_by_hostname($name); | |
| }, $server_names); |
🤖 Prompt for AI Agents
In `@views/servers_bulk_action.php` around lines 66 - 69,
RuntimeState::get('server_dir') may return null so add a null-guard before
mapping over $server_names: check the $server_dir variable returned by
RuntimeState::get('server_dir') and if it's null, set $selected_servers to an
empty array (or handle/log the missing server_dir and short-circuit) instead of
calling $server_dir->get_server_by_hostname(); update the block that builds
$selected_servers (references: $server_dir, get_server_by_hostname,
$selected_servers, $server_names) to safely handle the missing runtime key.
Added a new protected static method `resolve_runtime` to the Record class to streamline the retrieval of runtime dependencies, maintaining legacy behavior while improving code organization. Updated the User class to utilize this new method for fetching runtime values, enhancing readability and reducing redundancy.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
templates/groups.php (1)
29-29: Conditionally applytabpanelrole for non-admin rendering.At Line 29,
role="tabpanel"is always present, but for non-admin users there is no tablist/tab relationship. Consider making the role conditional to keep semantics precise.♻️ Suggested tweak
- <div class="tab-pane fade in active show" id="list" role="tabpanel"<?php if($this->get('admin')) out(' aria-labelledby="groups_list_tab"', ESC_NONE) ?> aria-hidden="false"> + <div class="tab-pane fade in active show" id="list"<?php if($this->get('admin')) out(' role="tabpanel" aria-labelledby="groups_list_tab"', ESC_NONE) ?> aria-hidden="false">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/groups.php` at line 29, The div with id="list" currently always includes role="tabpanel" even when $this->get('admin') is false; update the template (the div with id="list" in templates/groups.php) so the role="tabpanel" attribute is only rendered when $this->get('admin') is true (i.e., keep the existing conditional around aria-labelledby but extend it to include role="tabpanel"), leaving aria-hidden and id intact to preserve functionality.public_html/bootstrap5-compat.css (1)
59-92: Replace deprecatedclipproperty withclip-path.The
clipproperty is deprecated per CSS specifications. Bootstrap 5's actual.visually-hiddenimplementation usesclip-path: inset(50%)instead. Since modern browser support forclip-pathis excellent, consider updating to the current standard.♻️ Proposed fix to use modern clip-path
.visually-hidden { position: absolute !important; width: 1px !important; height: 1px !important; padding: 0 !important; margin: -1px !important; overflow: hidden !important; - clip: rect(0, 0, 0, 0) !important; + clip-path: inset(50%) !important; white-space: nowrap !important; border: 0 !important; } .visually-hidden-focusable { position: absolute !important; width: 1px !important; height: 1px !important; padding: 0 !important; margin: -1px !important; overflow: hidden !important; - clip: rect(0, 0, 0, 0) !important; + clip-path: inset(50%) !important; white-space: nowrap !important; border: 0 !important; } .visually-hidden-focusable:active, .visually-hidden-focusable:focus { position: static !important; width: auto !important; height: auto !important; margin: 0 !important; overflow: visible !important; - clip: auto !important; + clip-path: none !important; white-space: normal !important; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public_html/bootstrap5-compat.css` around lines 59 - 92, The CSS uses the deprecated clip: rect(...) in the .visually-hidden and .visually-hidden-focusable rules; update both selectors to use modern clip-path syntax (e.g., replace clip: rect(0, 0, 0, 0) !important; with clip-path: inset(50%) !important;) and in the .visually-hidden-focusable:active, .visually-hidden-focusable:focus rule restore visibility by setting clip-path: none !important; (and remove or stop using the old clip property) so the behavior matches Bootstrap 5's .visually-hidden and .visually-hidden-focusable semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/library-security-plan.md`:
- Line 8: Replace hardcoded absolute repository paths in
docs/library-security-plan.md with relative repository paths: change occurrences
like /var/www/ska/templates/base.php to a relative link such as
[templates/base.php](templates/base.php), and apply the same pattern for the
other referenced files mentioned in the comment — replace
`[composer.json](../composer.json)` and
`[public_html/extra.js](../public_html/extra.js)` (or their absolute
equivalents) with repository-root-relative links so the documentation is
portable across installations.
---
Nitpick comments:
In `@public_html/bootstrap5-compat.css`:
- Around line 59-92: The CSS uses the deprecated clip: rect(...) in the
.visually-hidden and .visually-hidden-focusable rules; update both selectors to
use modern clip-path syntax (e.g., replace clip: rect(0, 0, 0, 0) !important;
with clip-path: inset(50%) !important;) and in the
.visually-hidden-focusable:active, .visually-hidden-focusable:focus rule restore
visibility by setting clip-path: none !important; (and remove or stop using the
old clip property) so the behavior matches Bootstrap 5's .visually-hidden and
.visually-hidden-focusable semantics.
In `@templates/groups.php`:
- Line 29: The div with id="list" currently always includes role="tabpanel" even
when $this->get('admin') is false; update the template (the div with id="list"
in templates/groups.php) so the role="tabpanel" attribute is only rendered when
$this->get('admin') is true (i.e., keep the existing conditional around
aria-labelledby but extend it to include role="tabpanel"), leaving aria-hidden
and id intact to preserve functionality.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8621cda-1245-45dd-91ea-aaae6e899297
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
docs/bootstrap5-gap-analysis.mddocs/library-security-plan.mdmodel/record.phpmodel/user.phppublic_html/bootstrap5-compat.csspublic_html/bootstrap5-compat.jspublic_html/extra.jspublic_html/style.csstemplates/access_options.phptemplates/functions.phptemplates/group.phptemplates/groups.phptemplates/help.phptemplates/pubkey.phptemplates/pubkeys.phptemplates/server.phptemplates/serveraccount.phptemplates/servers.phptemplates/servers_bulk_action.phptemplates/user.php
✅ Files skipped from review due to trivial changes (4)
- public_html/style.css
- templates/servers_bulk_action.php
- templates/access_options.php
- docs/bootstrap5-gap-analysis.md
🚧 Files skipped from review as they are similar to previous changes (11)
- templates/functions.php
- templates/user.php
- templates/group.php
- templates/pubkeys.php
- templates/pubkey.php
- model/record.php
- templates/serveraccount.php
- public_html/bootstrap5-compat.js
- model/user.php
- templates/server.php
- public_html/extra.js
|
|
||
| ## Runtime Library Inventory | ||
|
|
||
| The authenticated UI currently loads these frontend assets from [templates/base.php](/var/www/ska/templates/base.php): |
There was a problem hiding this comment.
Use relative repository paths instead of hardcoded absolute paths.
The documentation references files using absolute paths like /var/www/ska/templates/base.php, which assumes a specific installation directory. For portable repository documentation, use relative paths from the repository root.
📝 Proposed fix
-The authenticated UI currently loads these frontend assets from [templates/base.php](/var/www/ska/templates/base.php):
+The authenticated UI currently loads these frontend assets from [templates/base.php](../templates/base.php):Apply the same pattern for:
- Line 29:
[composer.json](../composer.json) - Line 59:
[public_html/extra.js](../public_html/extra.js)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The authenticated UI currently loads these frontend assets from [templates/base.php](/var/www/ska/templates/base.php): | |
| The authenticated UI currently loads these frontend assets from [templates/base.php](../templates/base.php): |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/library-security-plan.md` at line 8, Replace hardcoded absolute
repository paths in docs/library-security-plan.md with relative repository
paths: change occurrences like /var/www/ska/templates/base.php to a relative
link such as [templates/base.php](templates/base.php), and apply the same
pattern for the other referenced files mentioned in the comment — replace
`[composer.json](../composer.json)` and
`[public_html/extra.js](../public_html/extra.js)` (or their absolute
equivalents) with repository-root-relative links so the documentation is
portable across installations.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@templates/base.php`:
- Line 65: The line generating dropdown anchor elements in templates/base.php
contains a stray closing </li> which produces invalid HTML; edit the anchor
output (the line using outurl($url), out($name) and the conditional class check
comparing $url to $this->get('relative_request_url')) and remove the trailing
</li> so the <a> elements are not incorrectly closed as list items.
In `@templates/functions.php`:
- Around line 110-116: The Windows tab pane with id "windows_instructions" is
missing the Bootstrap 5 default-visible classes and accessibility attributes;
update the <div> for the pane (id "windows_instructions") to include the "show
active" classes and add role="tabpanel" and
aria-labelledby="windows_instructions_tab" so it behaves and is announced the
same as the Mac and Linux panes.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aef50f0a-5fff-4ce0-93c8-7e8e9660417e
📒 Files selected for processing (3)
templates/base.phptemplates/functions.phptemplates/user.php
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 10 file(s) based on 13 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
templates/functions.php (1)
111-113:⚠️ Potential issue | 🟠 MajorGenerate per-render unique tab IDs in
keygen_help()
keygen_help()still emits fixed tab/pane IDs. If rendered more than once on the same page, tab controls can target the wrong pane and ARIA relationships become ambiguous. This appears to be the same unresolved issue raised earlier.♻️ Suggested fix
function keygen_help($box_position) { + static $keygen_help_tabset_counter = 0; + $keygen_help_tabset_counter++; + $tab_prefix = 'keygen_help_'.$keygen_help_tabset_counter.'_'; ?> <ul class="nav nav-tabs" role="tablist"> - <li class="nav-item" role="presentation"><a href="#windows_instructions" id="windows_instructions_tab" class="nav-link active" role="tab" data-bs-toggle="tab" aria-controls="windows_instructions" aria-selected="true">Windows</a></li> - <li class="nav-item" role="presentation"><a href="#mac_instructions" id="mac_instructions_tab" class="nav-link" role="tab" data-bs-toggle="tab" aria-controls="mac_instructions" aria-selected="false" tabindex="-1">Mac</a></li> - <li class="nav-item" role="presentation"><a href="#linux_instructions" id="linux_instructions_tab" class="nav-link" role="tab" data-bs-toggle="tab" aria-controls="linux_instructions" aria-selected="false" tabindex="-1">Linux</a></li> + <li class="nav-item" role="presentation"><a href="#<?php out($tab_prefix)?>windows_instructions" id="<?php out($tab_prefix)?>windows_instructions_tab" class="nav-link active" role="tab" data-bs-toggle="tab" aria-controls="<?php out($tab_prefix)?>windows_instructions" aria-selected="true">Windows</a></li> + <li class="nav-item" role="presentation"><a href="#<?php out($tab_prefix)?>mac_instructions" id="<?php out($tab_prefix)?>mac_instructions_tab" class="nav-link" role="tab" data-bs-toggle="tab" aria-controls="<?php out($tab_prefix)?>mac_instructions" aria-selected="false" tabindex="-1">Mac</a></li> + <li class="nav-item" role="presentation"><a href="#<?php out($tab_prefix)?>linux_instructions" id="<?php out($tab_prefix)?>linux_instructions_tab" class="nav-link" role="tab" data-bs-toggle="tab" aria-controls="<?php out($tab_prefix)?>linux_instructions" aria-selected="false" tabindex="-1">Linux</a></li> </ul> <div class="tab-content clearfix"> - <div class="tab-pane fade show active" id="windows_instructions" role="tabpanel" aria-labelledby="windows_instructions_tab"> + <div class="tab-pane fade show active" id="<?php out($tab_prefix)?>windows_instructions" role="tabpanel" aria-labelledby="<?php out($tab_prefix)?>windows_instructions_tab"> ... - <div class="tab-pane fade" id="mac_instructions" role="tabpanel" aria-labelledby="mac_instructions_tab" aria-hidden="true"> + <div class="tab-pane fade" id="<?php out($tab_prefix)?>mac_instructions" role="tabpanel" aria-labelledby="<?php out($tab_prefix)?>mac_instructions_tab" aria-hidden="true"> ... - <div class="tab-pane fade" id="linux_instructions" role="tabpanel" aria-labelledby="linux_instructions_tab" aria-hidden="true"> + <div class="tab-pane fade" id="<?php out($tab_prefix)?>linux_instructions" role="tabpanel" aria-labelledby="<?php out($tab_prefix)?>linux_instructions_tab" aria-hidden="true">Also applies to: 116-116, 141-141, 153-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/functions.php` around lines 111 - 113, keygen_help() currently emits fixed tab and pane IDs (e.g., windows_instructions_tab, windows_instructions, mac_instructions_tab, mac_instructions, linux_instructions_tab, linux_instructions) which break when the component is rendered multiple times; modify keygen_help() to generate a per-render unique suffix (e.g., using uniqid() or random string) and append it to all tab id attributes, their aria-controls/href targets, and the corresponding pane id attributes so each tab control/pane pair remains uniquely linked across renders—ensure every occurrence in keygen_help() that references those IDs/panes is updated consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@templates/functions.php`:
- Around line 111-113: keygen_help() currently emits fixed tab and pane IDs
(e.g., windows_instructions_tab, windows_instructions, mac_instructions_tab,
mac_instructions, linux_instructions_tab, linux_instructions) which break when
the component is rendered multiple times; modify keygen_help() to generate a
per-render unique suffix (e.g., using uniqid() or random string) and append it
to all tab id attributes, their aria-controls/href targets, and the
corresponding pane id attributes so each tab control/pane pair remains uniquely
linked across renders—ensure every occurrence in keygen_help() that references
those IDs/panes is updated consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e94c02d2-eaf4-41cf-8b0e-6802f713d3d6
📒 Files selected for processing (2)
templates/base.phptemplates/functions.php
🚧 Files skipped from review as they are similar to previous changes (1)
- templates/base.php
Fixed 10 file(s) based on 13 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
views/user_pubkeys.php (1)
77-82:⚠️ Potential issue | 🟠 MajorPotential null pointer on
$active_user->pop_alerts().Line 81 calls
$active_user->pop_alerts()unconditionally, but$active_usercan be falsy (line 25 checks$active_user &&). This will cause a fatal error for unauthenticated users viewing public key pages.🐛 Proposed fix
$page->set('title', 'Public keys for ' . $user->name); $page->set('head', $head); $page->set('content', $content); - $page->set('alerts', $active_user->pop_alerts()); + $page->set('alerts', $active_user ? $active_user->pop_alerts() : []); echo $page->generate();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@views/user_pubkeys.php` around lines 77 - 82, The view unconditionally calls $active_user->pop_alerts() which can be null for unauthenticated visitors; update the PageSection alert assignment so it safely handles a missing $active_user (e.g., when setting 'alerts' on the $page, use a conditional check and pass an empty array or null if $active_user is falsy). Locate the $page->set('alerts', ...) line and replace the direct call to $active_user->pop_alerts() with a safe conditional using $active_user and its pop_alerts() method.templates/user_pubkeys.php (1)
62-63:⚠️ Potential issue | 🟡 MinorMissing closing
</dd>tag.The
<dd>element for "Status" is not properly closed before the</dl>tag.🐛 Proposed fix
<dt>Status</dt> - <dd><?php out($pubkey->deletion_date !== null ? 'Deleted' : 'Active') ?> + <dd><?php out($pubkey->deletion_date !== null ? 'Deleted' : 'Active') ?></dd> </dl>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/user_pubkeys.php` around lines 62 - 63, The <dd> element rendering the status (<?php out($pubkey->deletion_date !== null ? 'Deleted' : 'Active') ?>) is not closed, so add a closing </dd> tag immediately after that PHP output before the existing </dl> to properly terminate the definition entry and fix the malformed HTML.model/serveraccount.php (1)
429-434:⚠️ Potential issue | 🟠 MajorAdd the same null guard for
sync_request_dirhere.
runtime_value()can still returnnull, and the next line becomes a fataladd_sync_request()call on null.list_group_membership()already uses the safer pattern.Possible fix
public function sync_access() { $sync_request_dir = self::runtime_value('sync_request_dir', array_key_exists('sync_request_dir', $GLOBALS) ? $GLOBALS['sync_request_dir'] : null); + if($sync_request_dir === null) { + throw new RuntimeException('Sync request directory service is unavailable; cannot queue account sync.'); + } $sync_request = new SyncRequest; $sync_request->server_id = $this->server_id; $sync_request->account_name = $this->name; $sync_request_dir->add_sync_request($sync_request); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/serveraccount.php` around lines 429 - 434, In sync_access(), runtime_value('sync_request_dir', ...) may return null so calling add_sync_request() on it can fatal; mirror the safe pattern used in list_group_membership() by checking the $sync_request_dir for null before invoking $sync_request_dir->add_sync_request($sync_request) (e.g., return or log/error when null), keeping references to the sync_access() method, runtime_value(), the $sync_request_dir variable and the add_sync_request() call so the null-guard is applied exactly around that invocation.model/user.php (1)
194-205:⚠️ Potential issue | 🟠 MajorFail fast when LDAP/config services are unavailable.
These paths now assume
resolve_runtime('config', array())and the runtime LDAP handle always succeed, but they immediately index$config['email']/$config['ldap']and call$this->ldap->search(). If the runtime state is not seeded, this turns user mail and LDAP refresh flows into null-method calls and undefined-key notices instead of a clear setup error.Based on learnings: "Maintain LDAP/AD integration compatibility when modifying authentication or user/group handling."
Also applies to: 307-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/user.php` around lines 194 - 205, The code assumes resolve_runtime('config', array()) and the LDAP handle always exist which leads to undefined-index/method notices; update the add_public_key flow (around parent::add_public_key and resolve_runtime('config', ...)) and the LDAP refresh paths to validate that the returned $config contains the expected keys (e.g., email.admin_address, email.report_address, web.baseurl) and that $this->ldap (or the LDAP runtime from resolve_runtime('ldap')) is non-null and implements search before using them; if any required config entries or the LDAP handle are missing, immediately raise/throw a clear setup exception or return a fatal error with a descriptive message instead of proceeding. Ensure checks reference resolve_runtime('config', array()), parent::add_public_key, $this->ldap->search and the LDAP-related refresh methods so callers fail fast when services are not seeded.templates/servers_bulk_action.php (1)
80-82:⚠️ Potential issue | 🟠 MajorKeep the body cells in sync with the conditional Actions header.
The header is gated by
$this->get('admin'), but the delete buttons always render. If that flag is false, the table shifts columns and still exposes the remove UI.Suggested fix
- <td> - <button type="submit" name="delete_admin" value="<?php out($admin->id) ?>" class="btn btn-secondary btn-sm"><span class="ska-icon ska-icon-trash"></span> Remove leader</button> - </td> + <?php if($this->get('admin')) { ?> + <td> + <button type="submit" name="delete_admin" value="<?php out($admin->id) ?>" class="btn btn-secondary btn-sm"><span class="ska-icon ska-icon-trash"></span> Remove leader</button> + </td> + <?php } ?> ... - <td> - <button type="submit" name="delete_admin" value="<?php out($admin->id) ?>" class="btn btn-secondary btn-sm"><span class="ska-icon ska-icon-trash"></span> Remove leader</button> - </td> + <?php if($this->get('admin')) { ?> + <td> + <button type="submit" name="delete_admin" value="<?php out($admin->id) ?>" class="btn btn-secondary btn-sm"><span class="ska-icon ska-icon-trash"></span> Remove leader</button> + </td> + <?php } ?>Also applies to: 95-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/servers_bulk_action.php` around lines 80 - 82, The table header prints the "Actions" <th> only when $this->get('admin') is truthy, but the body always renders the delete/remove buttons causing misaligned columns; update the body markup that outputs the delete button(s) (the cell rendering around the delete/remove UI) to be wrapped in the same conditional ($this->get('admin')) so the <td> with the button(s) is only rendered for admins (apply the same change to the second occurrence that mirrors lines 95-106), or alternatively render an empty <td> when !$this->get('admin') to keep columns aligned.views/servers.php (1)
128-140:⚠️ Potential issue | 🟡 MinorMissing null guard for
server_dirinrun_import.
RuntimeState::get('server_dir')may returnnullif the runtime state is not properly initialized. Line 140 calls$server_dir->add_server($server)without checking if$server_diris null, which would cause a fatal error.🛡️ Proposed guard
function run_import(array $entries): array { $server_dir = RuntimeState::get('server_dir'); + if($server_dir === null) { + throw new RuntimeException('Server directory service is unavailable; cannot import servers.'); + } $imported = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@views/servers.php` around lines 128 - 140, run_import calls RuntimeState::get('server_dir') which may return null and then dereferences $server_dir->add_server($server); add a null guard after retrieving $server_dir in run_import: check if $server_dir is null and handle it (throw an exception, log an error and return an empty result, or initialize the server_dir before proceeding) so the loop never calls add_server on null; update references to $server_dir->add_server(...) and any subsequent uses to be inside the null-checked/early-return block to avoid a fatal error.
♻️ Duplicate comments (7)
services/login_flow.php (2)
28-42:⚠️ Potential issue | 🟠 MajorSession-backed throttling is still easy to bypass.
Because the counter lives in
$_SESSION, starting a new session resets the limit. This should move to a server-side store keyed by normalized username and/or IP, and be cleared on successful auth.Also applies to: 74-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/login_flow.php` around lines 28 - 42, The throttling currently stored in $_SESSION can be bypassed by creating a new session; refactor login attempt storage from $_SESSION to a server-side store (e.g., Redis or DB) keyed by a normalized username and/or client IP, update is_rate_limited($user_attempts, $current_time) to read from that store, change record_failed_attempt($username, $current_time) to increment/write into the server-side store, and ensure the store entry is cleared on successful authenticate(...) (and when unset of redirect state occurs) so brute-force protection persists across sessions.
16-17:⚠️ Potential issue | 🟠 MajorGuard
hash_equals()against non-string CSRF input.
csrf_token[]=makes the submitted value an array, sohash_equals()throws before thecatch(Exception)block can run. Verify both the session token and submitted token are strings before comparing them.In PHP 8.2, does `hash_equals()` throw a `TypeError` when either argument is an array instead of a string?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/login_flow.php` around lines 16 - 17, The CSRF check calls hash_equals($_SESSION['csrf_token'], $post_data['csrf_token']) without ensuring both arguments are strings, which can throw a TypeError if the submitted token is an array; update the guard around hash_equals in services/login_flow.php to first verify is_string($_SESSION['csrf_token']) and is_string($post_data['csrf_token']) (or safely cast/normalize) before calling hash_equals, and treat non-string or missing tokens as invalid (set $error_message) so the exception is avoided and the existing error handling path is used.model/entity.php (1)
484-499:⚠️ Potential issue | 🟠 MajorGuard the runtime-resolved directories before dereferencing them.
These lookups still fall back to
null, solist_group_membership(),list_servers(), andadd_sync_request()can fatal here. Apply the same fail-fast checks already used inmodel/serveraccount.php.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/entity.php` around lines 484 - 499, The RuntimeState lookups for group_dir, server_dir, and sync_request_dir may return null and are dereferenced immediately; update the code using the same fail-fast guards as in model/serveraccount.php: after resolving RuntimeState::get(...) for group_dir, server_dir, and sync_request_dir, check for a non-null object (and optionally instanceof the expected class) before calling methods like list_group_membership(), list_servers(), or add_sync_request(); if a directory is null, return or skip that branch so you don't call methods on null (reference symbols: RuntimeState, group_dir, list_group_membership, server_dir, list_servers, sync_request_dir, add_sync_request).docs/library-security-plan.md (1)
8-8:⚠️ Potential issue | 🟡 MinorUse repo-relative links in this doc.
These
/var/www/ska/...references are install-specific, so they break in PR/repository rendering and in checkouts rooted somewhere else. Please switch them to paths relative todocs/library-security-plan.md.Suggested fix
-The UI now loads these frontend assets from [templates/base.php](/var/www/ska/templates/base.php): +The UI now loads these frontend assets from [templates/base.php](../templates/base.php): ... -- `phpseclib/phpseclib:^3.0` is the only third-party runtime PHP library in [composer.json](/var/www/ska/composer.json) +- `phpseclib/phpseclib:^3.0` is the only third-party runtime PHP library in [composer.json](../composer.json)Apply the same
../...pattern to the remainingpublic_html/...andstyle.csslinks in this file.Also applies to: 27-27, 61-68, 71-71, 78-78, 90-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/library-security-plan.md` at line 8, Replace install-specific absolute paths like /var/www/ska/templates/base.php with repository-relative links from docs/library-security-plan.md (e.g., use ../... paths) so the doc renders correctly in PRs; update all occurrences including the templates/base.php reference and the other absolute references to public_html/... and style.css mentioned in the comment (lines referenced: 27, 61-68, 71, 78, 90-91) to equivalent repo-relative paths.templates/group.php (1)
261-264:⚠️ Potential issue | 🟠 MajorGive the access-hostname field a real accessible name.
Line 263 still labels the required input only as
@. That leaves this second hostname field without a meaningful name in assistive tech, even though the first hostname field in this file has already been fixed.Suggested fix
- <span class="input-group-text"><label for="access-hostname">@</label></span> + <span class="input-group-text"> + <label for="access-hostname"><span aria-hidden="true">@</span><span class="visually-hidden">Hostname</span></label> + </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/group.php` around lines 261 - 264, The label for the input with id "access-hostname" currently uses only "@" which is not an accessible name; update the associated label or provide an explicit accessible name (e.g., change the label text to "Access hostname" or add an aria-label/aria-labelledby that reads "Access hostname") so assistive tech can announce the field; ensure the <label for="access-hostname"> or aria attribute targets the input id "access-hostname" and that the input retains name="hostname" and required behavior.services/request_context.php (1)
20-20:⚠️ Potential issue | 🟠 MajorStill use a canonical host here.
$_SERVER['HTTP_HOST']is still client-controlled, soabsolute_request_urlcan be poisoned and then reused by self-redirects or any absolute links built from the request context. Validate against a canonical/allowed host before concatenating it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/request_context.php` at line 20, $context->absolute_request_url is built using the client-controlled $_SERVER['HTTP_HOST'], allowing host header poisoning; validate the host before concatenation by checking $_SERVER['HTTP_HOST'] against a configured canonical/allowed host (e.g. a config value or getCanonicalHost()/allowedHosts list) and use the canonical value when it does not match, or fall back to a trusted server name, then build $context->absolute_request_url using that trusted host instead of the raw $_SERVER['HTTP_HOST'].public_html/extra.js (1)
681-690:⚠️ Potential issue | 🟡 MinorFallback to the mapped status text when
last_syncis absent.The null guard stops the exception, but
''still blanks the server status for responses that have no sync record. UsemappedServerStatus.messagewhendetailsis missing.🛠️ Suggested fix
var mappedServerStatus = map_sync_status(status.sync_status); - var details = (status.last_sync != null) ? status.last_sync.details : ''; - update_server_sync_status(mappedServerStatus.classname, details); + var serverMessage = (status.last_sync != null && status.last_sync.details) + ? status.last_sync.details + : mappedServerStatus.message; + update_server_sync_status(mappedServerStatus.classname, serverMessage); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public_html/extra.js` around lines 681 - 690, In poll_server_sync_status, the details variable falls back to an empty string when status.last_sync is null which blanks the displayed server status; change the fallback to use the mapped status text by setting details to status.last_sync ? status.last_sync.details : mappedServerStatus.message (or equivalent) so that update_server_sync_status(mappedServerStatus.classname, details) receives mappedServerStatus.message when last_sync is absent; ensure you compute mappedServerStatus before using it in the fallback and keep references to map_sync_status and update_server_sync_status as shown.
🧹 Nitpick comments (22)
templates/activity.php (1)
23-26: Consider removing or utilizing the empty<col>elements.The first three
<col>elements have no attributes and serve no purpose. Either remove them or addwidthorclassattributes if column styling is intended. The last<col class="date">has a class but ensure the corresponding CSS rule exists.♻️ Suggested simplification if col styling is not needed
<table class="table table-sm"> - <col></col> - <col></col> - <col></col> - <col class="date"></col> <thead>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/activity.php` around lines 23 - 26, The three empty <col> elements in templates/activity.php are redundant—either remove those first three <col> tags if no column-specific styling is required, or add explicit attributes (e.g., width or class names) to each to reflect intended layout; also ensure the remaining <col class="date"> has a matching CSS rule (e.g., .date) in your stylesheet if you keep it.services/runtime_state.php (1)
3-26: LGTM! Consider adding areset()method for testability.The static state container is a reasonable approach for migrating away from
$GLOBALS. For unit testing scenarios, consider adding areset()method to clear the state between tests.♻️ Optional: Add reset method for testing
public static function get($key, $default = null) { if(self::has($key)) { return self::$state[$key]; } return $default; } + + public static function reset() { + self::$state = array(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/runtime_state.php` around lines 3 - 26, Add a public static reset() method to RuntimeState that clears the internal static $state array so tests can start with a clean slate; locate the RuntimeState class (which currently defines private static $state and methods set, set_many, has, get) and implement reset() to reinitialize self::$state to an empty array (or use array = []), keeping it static and public so test suites can call RuntimeState::reset() between tests.model/dbdirectory.php (1)
27-35: Inconsistent fallback order compared toUserDirectory.
DBDirectoryprioritizes$GLOBALSoverRuntimeState, whileUserDirectory(lines 34-38) prioritizesRuntimeStatewith$GLOBALSas the default. This inconsistency could cause subtle bugs during the migration.Consider aligning the fallback order across all model classes:
♻️ Proposed fix to match UserDirectory pattern
public function __construct() { - if(array_key_exists('database', $GLOBALS)) { - $this->database = $GLOBALS['database']; - } elseif(class_exists('RuntimeState', false)) { - $this->database = RuntimeState::get('database'); + if(class_exists('RuntimeState', false)) { + $this->database = RuntimeState::get('database', array_key_exists('database', $GLOBALS) ? $GLOBALS['database'] : null); + } elseif(array_key_exists('database', $GLOBALS)) { + $this->database = $GLOBALS['database']; } else { $this->database = null; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/dbdirectory.php` around lines 27 - 35, The DBDirectory constructor currently prefers $GLOBALS over RuntimeState which is inconsistent with UserDirectory; change DBDirectory::__construct to first check if class_exists('RuntimeState', false) and use RuntimeState::get('database') if present, falling back to $GLOBALS['database'] only if RuntimeState is absent, and otherwise set $this->database = null so DBDirectory matches UserDirectory's fallback order.model/externalkey.php (2)
115-118: Add explicitreturn nullfor clarity.When no row is found, the method implicitly returns
null. While PHP handles this correctly, an explicitreturn null;improves readability and matches the@return ExternalKey|nulldocblock.♻️ Suggested fix
if ($row = $result->fetch_assoc()) { return new ExternalKey($row['id'], $row); } + return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/externalkey.php` around lines 115 - 118, The method that reads from $result and returns new ExternalKey($row['id'], $row) currently falls through implicitly when no row is found; add an explicit "return null;" at the end of that function (the method that uses $result->fetch_assoc()) so its behavior matches the `@return` ExternalKey|null docblock and improves readability for callers of ExternalKey.
46-56: Consider extracting duplicate database resolution logic.The database resolution block is duplicated verbatim in both
list_external_keys()andget_by_id(). Consider extracting to a private static helper method similar toPublicKey::get_runtime_config().♻️ Suggested refactor
+ private static function get_database() { + if(class_exists('RuntimeState', false)) { + $database = RuntimeState::get('database', null); + } else { + $database = null; + } + if(is_null($database) && array_key_exists('database', $GLOBALS)) { + $database = $GLOBALS['database']; + } + if(is_null($database)) { + throw new BadMethodCallException('Database connection is not available.'); + } + return $database; + } + public static function list_external_keys($with_hostnames = false) { - if(class_exists('RuntimeState', false)) { - $database = RuntimeState::get('database', null); - } else { - $database = null; - } - if(is_null($database) && array_key_exists('database', $GLOBALS)) { - $database = $GLOBALS['database']; - } - if(is_null($database)) { - throw new BadMethodCallException('Database connection is not available.'); - } + $database = self::get_database();Also applies to: 100-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/externalkey.php` around lines 46 - 56, The database resolution logic is duplicated in list_external_keys() and get_by_id(); extract that logic into a single private static helper (e.g., ExternalKey::get_runtime_config() or ExternalKey::resolveDatabase()) that encapsulates the RuntimeState/GLOBALS fallback and throws the BadMethodCallException when null, then replace the duplicated blocks in list_external_keys() and get_by_id() with calls to that helper (mirroring the pattern used by PublicKey::get_runtime_config()).services/request_csrf_guard.php (1)
4-4: Consider adding type hints for better maintainability.The method parameters lack type declarations. Adding them would improve IDE support and catch type mismatches earlier.
♻️ Suggested improvement
- public function validate($active_user, $request_context, $post_data) { + public function validate(?User $active_user, object $request_context, array $post_data): void {Adjust the type hints based on the actual expected types (e.g.,
$request_contextmight have a specific class).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/request_csrf_guard.php` at line 4, The validate method lacks parameter type hints; update the RequestCsrfGuard::validate signature to add the appropriate types for $active_user, $request_context and $post_data (e.g., a User class or int for $active_user, the concrete request context class for $request_context, and array|string for $post_data) and add a return type (bool) if appropriate; locate the validate method in services/request_csrf_guard.php and change the signature to use those concrete types to improve static checks and IDE support while ensuring callers are updated to match.services/auth.php (1)
73-77: Consider reducing PII in log messages.The authentication logs include partial usernames (first 3 characters). While this aids debugging, logging even partial user identifiers can be a compliance concern under GDPR/CCPA. Consider using a session correlation ID instead or ensuring log retention policies are in place.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/auth.php` around lines 73 - 77, Replace the partial-username PII in the authentication logs by logging a session correlation ID or a non-reversible identifier instead of substr($username, 0, 3) in the error_log calls inside the foreach ($auth_methods as $method) loop and the prior authentication attempt log; update the code that calls error_log("Authentication attempt for user: ..." ) and error_log("Trying {$method['method']} format for user: ..." ) to use a request/session ID (or a hashed value generated once per request) so no raw or partial username appears in logs while preserving traceability.views/pubkeys.php (1)
81-83: Inconsistent variable access pattern within the same file.The
send_mail_key_allowed()function (lines 25-31) retrieves$active_userand$configfromRuntimeState, but lines 81, 83, and 99 access$active_userand$configdirectly as globals. This inconsistency may cause issues during the migration away from globals.Consider either:
- Updating the file-level code to also use RuntimeState, or
- Documenting that globals are expected to be set by the request handler before this view executes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@views/pubkeys.php` around lines 81 - 83, The file mixes RuntimeState-based access and direct globals: update the places using globals ($active_user and $config) to read from RuntimeState like send_mail_key_allowed() does, so change the view-level references that call $content->set('admin', $active_user->admin) and $content->set('security_config', ...) to instead fetch RuntimeState::get('active_user') and RuntimeState::get('config') (or use the same accessor used in send_mail_key_allowed()) and then set those values; ensure you preserve the fallback to an empty array for security_config and reference the same property names (admin, security_config) so templates remain unchanged.views/server.php (1)
47-54: Control flow relies onisset()returning false for null — correct but subtle.The pattern
$entity = resolve...; if($entity === null) { $content = ...; } if(isset($entity)) { ... }works becauseisset()returnsfalsefornull. However, this two-separate-ifpattern is less clear than anif/elseconstruct.♻️ Suggested clarification
} elseif(isset($_POST['add_admin']) && ($active_user->admin)) { $entity = $relation_lifecycle_service->resolve_user_or_group_by_name($user_dir, $group_dir, $_POST['user_name']); - if($entity === null) { - $content = new PageSection('user_not_found'); - } - if(isset($entity)) { + if($entity !== null) { $relation_lifecycle_service->add_server_admin($server, $entity); redirect('#admins'); + } else { + $content = new PageSection('user_not_found'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@views/server.php` around lines 47 - 54, The control flow around $entity is unclear because it relies on isset() being false for null; replace the two separate ifs with a clear if/else: call $relation_lifecycle_service->resolve_user_or_group_by_name(...) and if the result is null set $content = new PageSection('user_not_found') else call $relation_lifecycle_service->add_server_admin($server, $entity) and redirect('#admins'); reference resolve_user_or_group_by_name, $entity, PageSection('user_not_found'), add_server_admin and redirect('#admins') when making the change.model/report.php (1)
39-44: Consider simplifying the nested ternary fallback logic.The current implementation uses nested ternaries that are hard to follow. A clearer approach would improve maintainability.
♻️ Proposed refactor for clarity
public static function create(): Report { - $server_dir = class_exists('RuntimeState', false) - ? RuntimeState::get('server_dir', array_key_exists('server_dir', $GLOBALS) ? $GLOBALS['server_dir'] : null) - : (array_key_exists('server_dir', $GLOBALS) ? $GLOBALS['server_dir'] : null); + $server_dir = null; + if (class_exists('RuntimeState', false)) { + $server_dir = RuntimeState::get('server_dir', $GLOBALS['server_dir'] ?? null); + } elseif (isset($GLOBALS['server_dir'])) { + $server_dir = $GLOBALS['server_dir']; + } if($server_dir === null) { throw new RuntimeException('Server directory service is unavailable; cannot build report.'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/report.php` around lines 39 - 44, Simplify the nested ternary by explicitly computing $server_dir: first check class_exists('RuntimeState', false) and if true call RuntimeState::get('server_dir', array_key_exists('server_dir', $GLOBALS) ? $GLOBALS['server_dir'] : null); otherwise set $server_dir to $GLOBALS['server_dir'] if array_key_exists('server_dir', $GLOBALS) or to null; then keep the existing null check that throws the RuntimeException. This replaces the nested ternary with a clear if/else assignment around RuntimeState::get and a simple fallback to $GLOBALS['server_dir'].phpstan-baseline.neon (1)
1-31: Consider addressing the missing return statements rather than suppressing them.Suppressing "return statement is missing" for methods declared to return
arrayorstringindicates a mismatch between the declared return type and actual behavior. These methods likely have code paths that fall through without returning, which could cause runtime issues in strict type checking contexts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpstan-baseline.neon` around lines 1 - 31, The baseline shows suppressed "return statement is missing" errors for simplify_search() and methods Group::update, Server::update, ServerAccount::update, User::update and SyncProcess::get_data; instead of ignoring them, ensure every code path returns the declared type (add explicit return [] for array methods or ''/string value for SyncProcess::get_data) or adjust the function/method signatures to nullable/void if appropriate; locate the functions simplify_search and class methods Group::update, Server::update, ServerAccount::update, User::update and SyncProcess::get_data in their respective files and either add explicit return statements on all branches (including default/fall-through) or change the declared return type to match actual behavior and update callers accordingly.model/record.php (2)
60-60: Add explicit return type for PHP 8.2+ consistency.The docblock declares
@return mixed, but the method signature lacks the corresponding: mixedreturn type.Proposed fix
- protected static function resolve_runtime($key, $default = null) { + protected static function resolve_runtime(string $key, mixed $default = null): mixed {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/record.php` at line 60, The method resolve_runtime lacks an explicit return type despite the docblock declaring `@return` mixed; update the method signature for PHP 8.2+ consistency by adding the mixed return type to resolve_runtime (i.e., change "protected static function resolve_runtime($key, $default = null)" to include ": mixed"), ensure imports/type hints remain valid, and run tests to confirm no type conflicts.
60-65:resolve_runtimemethod is defined but never used.The constructor (lines 68-84) duplicates this resolution logic inline instead of calling
resolve_runtime('database')andresolve_runtime('active_user'). Additionally, the resolution order differs:resolve_runtimechecksRuntimeStatefirst with$GLOBALSas fallback, while the constructor checks$GLOBALSfirst thenRuntimeState.Consider either removing this unused method or refactoring the constructor to use it consistently.
♻️ Suggested refactor to use resolve_runtime consistently
public function __construct($id = null, $preload_data = array()) { - if(array_key_exists('database', $GLOBALS)) { - $this->database = $GLOBALS['database']; - } elseif(class_exists('RuntimeState', false)) { - $this->database = RuntimeState::get('database'); - } else { - $this->database = null; - } + $this->database = self::resolve_runtime('database'); if($this->database === null) { throw new RuntimeException('Database service is unavailable for Record initialization.'); } - if(array_key_exists('active_user', $GLOBALS)) { - $this->active_user = $GLOBALS['active_user']; - } elseif(class_exists('RuntimeState', false)) { - $this->active_user = RuntimeState::get('active_user'); - } else { - $this->active_user = null; - } + $this->active_user = self::resolve_runtime('active_user'); $this->id = $id;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/record.php` around lines 60 - 65, The resolve_runtime static method is unused and its logic conflicts with the inline resolution in the Record class constructor; either remove resolve_runtime or refactor the constructor to call resolve_runtime('database') and resolve_runtime('active_user') so the same resolution order (RuntimeState first, fallback to $GLOBALS/default) is applied consistently; update the constructor code that currently checks $GLOBALS then RuntimeState to instead call self::resolve_runtime(...) (or delete resolve_runtime if you prefer removing dead code) and ensure unique symbols referenced are resolve_runtime and the Record constructor.views/login.php (1)
21-22: Consider defensive access for returned state array.If
LoginFlowService::handle_request()returns an incomplete array (due to an exception or early return), accessing undefined keys will produce notices/warnings in PHP 8.2+.Defensive key access
$login_state = $login_flow->handle_request($_SERVER['REQUEST_METHOD'], $_POST); -$error_message = $login_state['error_message']; -$success_message = $login_state['success_message']; +$error_message = $login_state['error_message'] ?? null; +$success_message = $login_state['success_message'] ?? null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@views/login.php` around lines 21 - 22, The view accesses $login_state['error_message'] and $login_state['success_message'] directly which can cause undefined index notices if LoginFlowService::handle_request() returns an incomplete array; update the access in views/login.php to defensively read these keys (e.g., use isset() or the null coalescing operator) so $error_message and $success_message default to empty strings (or null) when the keys are missing, and keep the rest of the view logic unchanged.services/request_exception_handler.php (2)
4-14: Inconsistent type handling between constructor and setter.The constructor accepts
$active_userof any type (includingnull), butset_active_user()strictly requires aUsertype. This creates a type inconsistency where the property could benullbut the setter rejects it.Add consistent type declarations
class RequestExceptionHandler { - private $active_user; - private $config; + private ?User $active_user; + private array $config; - public function __construct($active_user, &$config) { + public function __construct(?User $active_user, array &$config) { $this->active_user = $active_user; $this->config = &$config; } - public function set_active_user(User $active_user): void { + public function set_active_user(?User $active_user): void { $this->active_user = $active_user; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/request_exception_handler.php` around lines 4 - 14, The $active_user property is treated inconsistently: the constructor accepts any value (including null) while set_active_user(User $active_user) enforces a User; make types consistent by updating the property and constructor to accept the same nullable User type used by the setter (or change the setter to accept nullable) — specifically adjust the class property $active_user, the constructor __construct($active_user, &$config) to use a matching type declaration (e.g., ?User $active_user) and/or change set_active_user(User $active_user): void to set_active_user(?User $active_user): void so both the constructor and set_active_user use the same nullable User type.
16-26: Use explicit exception message extraction and consider absolute path.Line 18 implicitly converts
$eto string. Using$e->getMessage()with$e->getTraceAsString()is more explicit. The relative path on line 24 may resolve incorrectly depending on the working directory.Proposed improvements
public function handle($e) { $error_number = time(); - error_log("$error_number: ".str_replace("\n", "\n$error_number: ", $e)); + $message = $e instanceof Throwable ? $e->getMessage() . "\n" . $e->getTraceAsString() : (string)$e; + error_log("$error_number: ".str_replace("\n", "\n$error_number: ", $message)); while(ob_get_length()) { ob_end_clean(); } $active_user = $this->active_user; $config = $this->config; - require('views/error500.php'); - die; + require(__DIR__ . '/../views/error500.php'); + exit(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/request_exception_handler.php` around lines 16 - 26, In handle(), avoid implicit stringification of $e: extract $e->getMessage() and $e->getTraceAsString() (and optionally $e->getFile()/$e->getLine()) and include them in the error_log along with $error_number so the log contains both message and stack trace; also require the view using an absolute path (e.g. build path with __DIR__ or the class's base config) instead of the current relative 'views/error500.php' to avoid working-directory issues.services/request_policy_guard.php (1)
4-15: Set the HTTP status in the guard, not just the template.That keeps the policy decision self-contained and avoids relying on
views/error403.php/views/error503.phpto remember the correct status code.Suggested change
public function enforce_web_enabled($config) { if(empty($config['web']['enabled'])) { + http_response_code(503); require('views/error503.php'); - die; + exit; } } public function enforce_active_user_status($active_user, $relative_request_url, $absolute_request_url) { if($active_user && (!$active_user->active || $active_user->force_disable)) { + http_response_code(403); require('views/error403.php'); - die; + exit; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/request_policy_guard.php` around lines 4 - 15, In enforce_web_enabled and enforce_active_user_status ensure the guard sets the HTTP status code before rendering the error view and exiting: in enforce_web_enabled call http_response_code(503) (or send a 503 header) immediately before require('views/error503.php') and die; likewise in enforce_active_user_status call http_response_code(403) (or send a 403 header) immediately before require('views/error403.php') and die; update those functions (enforce_web_enabled, enforce_active_user_status) so the policy decision is self-contained and not reliant on the view to set status codes.scripts/smoke/run.sh (1)
48-51: Inconsistent script invocation pattern.Line 49 calls
"$SCRIPT_DIR/web-workflows.sh"directly (relying on execute permission), while line 50 explicitly usesbash "$SCRIPT_DIR/browser-interactions.sh". For consistency and robustness, consider using the same invocation style for all scripts.♻️ Suggested fix for consistent invocation
if [ "$MODE" = "all" ] || [ "$MODE" = "web" ]; then - "$SCRIPT_DIR/web-workflows.sh" - bash "$SCRIPT_DIR/browser-interactions.sh" + bash "$SCRIPT_DIR/web-workflows.sh" + bash "$SCRIPT_DIR/browser-interactions.sh" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/smoke/run.sh` around lines 48 - 51, The script invocation is inconsistent: web-workflows.sh is executed relying on its executable bit while browser-interactions.sh is invoked via bash; update the conditional block to invoke both scripts the same way (preferably via bash for robustness) by calling bash "$SCRIPT_DIR/web-workflows.sh" and bash "$SCRIPT_DIR/browser-interactions.sh" so both web-workflows.sh and browser-interactions.sh use the same invocation pattern.scripts/sync-common.php (1)
113-139: Missing explicit return when handle is unset/not a resource.The
get_data()method has an implicitnullreturn when$this->handleis not set or not a resource (after line 138). While this may be intentional (handled by callers), an explicit return would clarify the contract.♻️ Suggested fix for explicit return
if($this->errors) { echo $this->errors; $this->output = ''; } return array('done' => true, 'output' => $this->output); } + return array('done' => true, 'output' => ''); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sync-common.php` around lines 113 - 139, The get_data() method may implicitly return null when $this->handle is unset or not a resource; add an explicit return at the end of get_data() (after the existing if blocks) to make the contract clear — e.g. return an array like array('done' => false, 'output' => '') — so callers always receive a predictable array; reference get_data(), $this->handle, and the existing return shapes in the method when implementing.model/server.php (1)
656-661: Consider adding null guards forsync_request_dir.Similar to
group_dir, thesync_request_dirruntime value could be null. While the failure would be less catastrophic (method would fail immediately), adding defensive guards would improve error messaging and consistency with the pattern established inGroup::list_group_membership.Also applies to: 698-704
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/server.php` around lines 656 - 661, The runtime lookup for sync_request_dir can return null; before calling $sync_request_dir->add_sync_request(...) (in model/server.php where you construct SyncRequest and set server_id/account_name), add a null guard similar to Group::list_group_membership: check if $sync_request_dir is null and either throw a descriptive exception or log an explicit error and return, so callers get a clear message instead of a fatal method call; apply the same null-check pattern to the other occurrence around lines 698-704 that also uses the runtime_value('sync_request_dir') result.templates/groups.php (1)
56-63: Minor HTML indentation inconsistency in filter form.The closing tags for the checkbox div (line 57-58), row div (line 59), button (line 60), form (line 61), and card-body div (line 62-63) have inconsistent indentation. While functionally correct, this could make maintenance harder.
🧹 Suggested indentation fix
<div class="form-check"><label class="form-check-label"><input type="checkbox" class="form-check-input" name="active[]" value="<?php out($value)?>"<?php out($checked) ?>> <span><?php out($label) ?></span></label></div> - <?php } ?> - </div> + <?php } ?> </div> - <button type="submit" class="btn btn-primary">Display results</button> - </form> - </div> + </div> + <button type="submit" class="btn btn-primary">Display results</button> + </form> </div> + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/groups.php` around lines 56 - 63, Reformat the HTML indentation in templates/groups.php around the filter form so nested elements align consistently: align the closing </div> for the checkbox block that contains the <input class="form-check-input" ...>, the closing </div> for the row/container wrapping those checkboxes, the <button type="submit" class="btn btn-primary">Display results</button>, the closing </form>, and the closing </div> for the card-body so each closing tag matches the indentation level of its corresponding opening tag (look for the form element that contains name="active[]" and the surrounding divs with classes form-check, row, and card-body). Ensure the <label> and <span> lines remain properly indented under their parent <div class="form-check"> to improve readability.views/serveraccount.php (1)
56-58: Please smoke the service-backed mutations on this route.This controller now switches the public-key, access-rule, and admin write paths over to lifecycle services. I’d validate add/remove key, add/remove access, and a sync preview on a real server-account flow before merging.
Based on learnings: "For changes, prefer targeted manual verification: Login and LDAP auth flow, Add/Remove public keys, Create/Remove access rules, Trigger a sync (CLI) and verify output files".
Also applies to: 84-86, 96-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@views/serveraccount.php` around lines 56 - 58, Exercise the controller endpoints that now delegate to KeyLifecycleService, AccessRuleService, and RelationLifecycleService: manually verify add and remove public-key operations (the methods invoked on KeyLifecycleService), add and remove access-rule flows (AccessRuleService), and admin write/relation mutations (RelationLifecycleService), plus trigger the sync preview path and confirm output matches expected server-account sync. Test both login and LDAP auth during these flows, and exercise the same scenarios covering the other changed blocks (the later usages around lines noted in the comment) to ensure the service-backed mutations behave identically to the previous implementation. Ensure any failing mutation surfaces service errors in logs and the UI/CLI responses are correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4d3268d-ccd8-4279-a78b-e3dac4e9a8b1
⛔ Files ignored due to path filters (29)
composer.lockis excluded by!**/*.lockpublic_html/bootstrap/css/bootstrap-theme.css.mapis excluded by!**/*.mappublic_html/bootstrap/css/bootstrap-theme.min.css.mapis excluded by!**/*.mappublic_html/bootstrap/css/bootstrap.css.mapis excluded by!**/*.mappublic_html/bootstrap/css/bootstrap.min.css.mapis excluded by!**/*.mappublic_html/bootstrap/fonts/glyphicons-halflings-regular.eotis excluded by!**/*.eotpublic_html/bootstrap/fonts/glyphicons-halflings-regular.svgis excluded by!**/*.svgpublic_html/bootstrap/fonts/glyphicons-halflings-regular.ttfis excluded by!**/*.ttfpublic_html/bootstrap/fonts/glyphicons-halflings-regular.woffis excluded by!**/*.woffpublic_html/bootstrap/fonts/glyphicons-halflings-regular.woff2is excluded by!**/*.woff2public_html/bootstrap/js/bootstrap.min.jsis excluded by!**/*.min.jspublic_html/icons/ban-circle.svgis excluded by!**/*.svgpublic_html/icons/cog.svgis excluded by!**/*.svgpublic_html/icons/console.svgis excluded by!**/*.svgpublic_html/icons/folder-close.svgis excluded by!**/*.svgpublic_html/icons/folder-open.svgis excluded by!**/*.svgpublic_html/icons/group.svgis excluded by!**/*.svgpublic_html/icons/ok.svgis excluded by!**/*.svgpublic_html/icons/pencil.svgis excluded by!**/*.svgpublic_html/icons/plus.svgis excluded by!**/*.svgpublic_html/icons/pushpin.svgis excluded by!**/*.svgpublic_html/icons/remove.svgis excluded by!**/*.svgpublic_html/icons/server.svgis excluded by!**/*.svgpublic_html/icons/serveraccount.svgis excluded by!**/*.svgpublic_html/icons/trash.svgis excluded by!**/*.svgpublic_html/icons/user.svgis excluded by!**/*.svgpublic_html/jquery/jquery-3.7.1.min.jsis excluded by!**/*.min.jspublic_html/jquery/jquery-3.7.1.min.mapis excluded by!**/*.mappublic_html/vendor/bootstrap5/bootstrap-5.3.8.bundle.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (112)
.github/workflows/docker.yml.gitignore.php-cs-fixer.dist.phpAGENTS.mdMakefileREADME.mdcomposer.jsonconfig/config.ini.examplecore.phpdocs/bootstrap5-gap-analysis.mddocs/compatibility-contract.mddocs/library-security-plan.mddocs/modernization-plan.mddocs/modernization-risks.mddocs/modernization-roadmap.mddocs/operations-runbook.mddocs/phase-10-checkpoint.mddocs/phase-5-checkpoint.mddocs/phase-6-checkpoint.mddocs/phase-7-checkpoint.mddocs/phase-8-checkpoint.mddocs/phase-9-checkpoint.mddocs/smoke-tests.mdldap.phpmodel/dbdirectory.phpmodel/entity.phpmodel/entityevent.phpmodel/externalkey.phpmodel/format.phpmodel/group.phpmodel/publickey.phpmodel/record.phpmodel/report.phpmodel/server.phpmodel/serveraccount.phpmodel/serverevent.phpmodel/servernote.phpmodel/user.phpmodel/userdirectory.phpphpstan-baseline.neonphpstan.neon.distpublic_html/bootstrap/css/bootstrap-theme.csspublic_html/bootstrap/css/bootstrap-theme.min.csspublic_html/bootstrap/css/bootstrap.csspublic_html/bootstrap/css/bootstrap.min.csspublic_html/bootstrap/js/bootstrap.jspublic_html/bootstrap/js/npm.jspublic_html/extra.jspublic_html/style.csspublic_html/vendor/bootstrap5/bootstrap-5.3.8.min.cssrequesthandler.phpscripts/smoke/browser-capture.shscripts/smoke/browser-interactions.shscripts/smoke/browser-lib.shscripts/smoke/fixtures/sync/README.mdscripts/smoke/helpers/find_account_access_rule.phpscripts/smoke/helpers/find_user_public_key.phpscripts/smoke/lib.shscripts/smoke/run.shscripts/smoke/sync-preview.shscripts/smoke/web-workflows.shscripts/ssh.phpscripts/sync-common.phpscripts/sync-failure.phpscripts/sync-runtime.phpscripts/sync.phpservices/access_rule_service.phpservices/auth.phpservices/key_lifecycle_service.phpservices/login_flow.phpservices/relation_lifecycle_service.phpservices/request_auth_guard.phpservices/request_context.phpservices/request_csrf_guard.phpservices/request_exception_handler.phpservices/request_policy_guard.phpservices/request_router_dispatcher.phpservices/response_security_headers.phpservices/runtime_state.phptemplates/access_options.phptemplates/activity.phptemplates/base.phptemplates/bulk_mail.phptemplates/bulk_mail_choose.phptemplates/error500.phptemplates/functions.phptemplates/group.phptemplates/groups.phptemplates/help.phptemplates/home.phptemplates/login.phptemplates/pubkey.phptemplates/pubkeys.phptemplates/report.phptemplates/server.phptemplates/serveraccount.phptemplates/servers.phptemplates/servers_bulk_action.phptemplates/tools.phptemplates/user.phptemplates/user_pubkeys.phptemplates/users.phpviews/group.phpviews/home.phpviews/login.phpviews/pubkeys.phpviews/server.phpviews/serveraccount.phpviews/servers.phpviews/servers_bulk_action.phpviews/user.phpviews/user_pubkeys.php
💤 Files with no reviewable changes (6)
- model/serverevent.php
- model/entityevent.php
- public_html/bootstrap/js/npm.js
- public_html/bootstrap/css/bootstrap-theme.min.css
- public_html/bootstrap/css/bootstrap-theme.css
- public_html/bootstrap/js/bootstrap.js
| function autoload_model($classname) { | ||
| global $base_path; | ||
| $original_classname = $classname; | ||
| $classname = preg_replace('/[^a-z]/', '', strtolower($classname)); # Prevent directory traversal and sanitize name | ||
| $base_path = RuntimeState::get('base_path', dirname(__FILE__)); | ||
| $filename = path_join($base_path, 'model', $classname.'.php'); | ||
| if(file_exists($filename)) { | ||
| include($filename); | ||
| } else { | ||
| eval("class $classname {}"); | ||
| throw new InvalidArgumentException("Attempted to load a class $classname that did not exist."); | ||
| throw new InvalidArgumentException("Attempted to load class {$original_classname} but model file {$filename} does not exist."); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "core.php" | head -20Repository: Msprg/ssh-key-authority
Length of output: 1897
🌐 Web query:
PHP SPL autoload_register exception abort chain behavior
💡 Result:
In PHP, the spl_autoload_register() stack is not “exception-safe” in the sense of “one autoloader throws ⇒ stop the chain and let me handle it cleanly”.
- Autoloaders are tried in order until the class is defined. However, if one autoloader throws before a later autoloader successfully loads the class, PHP can still end up throwing those earlier exceptions (“stacked exceptions”) even though the class ultimately loaded, so the exception can “poison” the whole autoload attempt. This is why throwing from autoloaders is strongly discouraged. [1]
- In some call sites / older behaviors, exceptions from autoload may not behave like normal catchable exceptions and can degrade into a fatal “Class not found” situation, rather than being cleanly catchable where you expected. [2] (This “autoload exception weirdness” is widely observed in practice.) [3]
Practical takeaway: don’t throw from autoload callbacks. Instead, have the autoloader only attempt loading (and return), and if you want a controlled failure, do it after the autoload attempt (e.g., check class_exists($fqcn, false) / interface_exists / trait_exists and then throw from non-autoloading code). [1]
Sources: PHP manual spl_autoload_register() notes on stacked exceptions [1], PHP bug report illustrating catch/fatal edge cases [2], and a write-up of the behavior in practice [3].
Don't throw exceptions from autoload callbacks.
The autoload_model function is registered before Composer's autoload stack. When a model file doesn't exist, throwing an exception prevents later autoloaders from attempting to resolve the class, which breaks loading of vendor and namespaced classes. PHP's autoload chain treats exceptions from earlier autoloaders as fatal failures that poison subsequent attempts.
Instead, silently return when the model file is absent and let other registered autoloaders handle the resolution.
♻️ Proposed fix
function autoload_model($classname) {
- $original_classname = $classname;
$classname = preg_replace('/[^a-z]/', '', strtolower($classname)); # Prevent directory traversal and sanitize name
$base_path = RuntimeState::get('base_path', dirname(__FILE__));
$filename = path_join($base_path, 'model', $classname.'.php');
if(file_exists($filename)) {
include($filename);
- } else {
- throw new InvalidArgumentException("Attempted to load class {$original_classname} but model file {$filename} does not exist.");
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core.php` around lines 84 - 93, The autoload_model autoloader throws an
InvalidArgumentException when a model file is missing, which prevents later
autoloaders from running; modify autoload_model so that if the computed
$filename does not exist it simply returns (no exception) to allow other
registered autoloaders to try resolving the class (keep the existing
sanitization with preg_replace, RuntimeState::get, and path_join logic intact
and only change the else branch to a silent return).
| function setup_database($config = null) { | ||
| if(is_null($config)) { | ||
| $config = RuntimeState::get('config', null); | ||
| if(is_null($config) && isset($GLOBALS['config']) && is_array($GLOBALS['config'])) { | ||
| $config = $GLOBALS['config']; | ||
| } | ||
| } | ||
| if(is_null($config) || !is_array($config)) { | ||
| throw new InvalidArgumentException('Database setup requires configuration.'); | ||
| } | ||
| try { | ||
| $database = new mysqli($config['database']['hostname'], $config['database']['username'], $config['database']['password'], $config['database']['database'], $config['database']['port']); | ||
| $database = new mysqli( | ||
| $config['database']['hostname'], | ||
| $config['database']['username'], | ||
| $config['database']['password'], | ||
| $config['database']['database'], | ||
| $config['database']['port'] | ||
| ); | ||
| } catch(ErrorException $e) { | ||
| throw new DBConnectionFailedException($e->getMessage()); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PHP 8.2 mysqli_sql_exception vs ErrorException new mysqli constructor throws
💡 Result:
In PHP 8.1+ (so PHP 8.2 included), MySQLi’s default error mode changed from “silent/warnings” to exceptions, so a failed new mysqli(...) typically throws a mysqli_sql_exception. [1], [2]
Why you might see ErrorException instead:
mysqli_sql_exceptionis thrown when MySQLi is in an exceptions/reporting mode (e.g.MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT).MYSQLI_REPORT_ERRORenables throwingmysqli_sql_exception“instead of warnings”. [3]ErrorExceptionis not thrown by MySQLi itself; it usually appears when your app/framework’s error handler converts PHP warnings/notices intoErrorException. That happens if MySQLi is not throwing exceptions (e.g. report mode turned off), so it emits anE_WARNINGwhich your handler wraps.
How to make it deterministic:
mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
$mysqli = new mysqli($host, $user, $pass, $db, $port);(or set it via mysqli_driver::$report_mode). [4]
If you want the old “no exception” behavior:
mysqli_report(MYSQLI_REPORT_OFF);[2]
Sources:
[1] https://php.watch/versions/8.1/mysqli-error-mode
[2] https://www.php.net/manual/en/migration81.incompatible.php
[3] https://www.php.net/manual/en/mysqli.constants.php
[4] https://www.php.net/manual/en/class.mysqli-driver.php
🌐 Web query:
PHP 8.2 mysqli exception handling breaking changes from PHP 8.0 8.1
💡 Result:
Main breaking change (PHP 8.0 → 8.1/8.2): MySQLi now throws exceptions by default
Before PHP 8.1 (e.g., PHP 8.0): the default MySQLi reporting mode was effectively “silent” (MYSQLI_REPORT_OFF), so many failures typically showed up as false returns and/or warnings that code manually checked (e.g., if (!$result) { ... }). [1], [2]
From PHP 8.1 onward (including PHP 8.2): the default is now:
MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT
This converts many MySQLi errors into mysqli_sql_exception (fatal if uncaught), so legacy code that relied on checking false, mysqli_errno(), mysqli_error(), or ... or die(...) can “break” with uncaught exceptions. [1], [2], [3]
What to do when upgrading to PHP 8.2
Option A — keep the old (pre-8.1) behavior
mysqli_report(MYSQLI_REPORT_OFF);This restores the previous “don’t throw exceptions by default” behavior. [1]
Option B — adopt exception handling (recommended with 8.1+)
Wrap connection/queries in try/catch (mysqli_sql_exception $e) and handle/log accordingly (the exception class is mysqli_sql_exception). [1], [3]
PHP 8.2 note (not exception-handling, but a mysqli-related BC change)
In PHP 8.2, mysqli can no longer be compiled against libmysql (only mysqlnd), which typically affects custom/legacy PHP builds more than application code. [4]
Sources:
[1] PHP manual – Migration 8.1 incompatible changes (MySQLi default error mode; restore via mysqli_report(MYSQLI_REPORT_OFF)) (php.net)
[2] PHP.Watch – MySQLi default error mode set to exceptions in PHP 8.1 (MYSQLI_REPORT_ERROR|MYSQLI_REPORT_STRICT) (php.watch)
[3] PHP manual – mysqli_sql_exception class (php.net)
[4] PHP.Watch – PHP 8.2: mysqli cannot be compiled with libmysql (php.watch)
Citations:
- 1: https://www.php.net/manual/en/migration81.incompatible.php?utm_source=openai
- 2: https://php.watch/versions/8.1/mysqli-error-mode?utm_source=openai
- 3: https://www.php.net/mysqli-sql-exception?utm_source=openai
- 4: https://php.watch/versions/8.2/mysqli-libmysql-no-longer-supported?utm_source=openai
🏁 Script executed:
# Check if there are any other mysqli connection attempts in the codebase
rg "new mysqli" -A 5 -B 2Repository: Msprg/ssh-key-authority
Length of output: 1897
Catch mysqli_sql_exception for PHP 8.2+ compatibility.
On PHP 8.2, new mysqli(...) throws mysqli_sql_exception by default (MySQLi's error reporting mode defaults to MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT). Catching only ErrorException will not catch this exception, causing connection failures to bypass your DBConnectionFailedException mapping.
🛠️ Proposed fix
- } catch(ErrorException $e) {
+ } catch(mysqli_sql_exception | ErrorException $e) {
throw new DBConnectionFailedException($e->getMessage());
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core.php` around lines 99 - 118, The setup_database function currently only
catches ErrorException but on PHP 8.2 new mysqli(...) can throw
mysqli_sql_exception; update the try/catch so it also catches
mysqli_sql_exception (or Throwable) and rethrows DBConnectionFailedException,
preserving the original exception (pass it as the previous exception and include
its message) so connection failures are mapped consistently; locate the
try/catch around new mysqli(...) in setup_database and add a catch branch for
mysqli_sql_exception (or replace with a single catch (Throwable $e)) that throws
new DBConnectionFailedException($e->getMessage(), 0, $e).
| if($actor === null && class_exists('RuntimeState', false)) { | ||
| $runtime_actor = RuntimeState::get('active_user', null); | ||
| if($runtime_actor instanceof User) { | ||
| $actor = $runtime_actor; | ||
| } | ||
| } | ||
| if($actor === null && array_key_exists('active_user', $GLOBALS) && $GLOBALS['active_user'] instanceof User) { | ||
| $actor = $GLOBALS['active_user']; | ||
| } | ||
| if(!($actor instanceof User)) { | ||
| throw new RuntimeException('No active user available for entity event logging.'); | ||
| } |
There was a problem hiding this comment.
This actor check breaks non-request logging paths.
ServerAccount::sync_report() updates sync_status via update(), and update() logs that change without passing an actor. In sync/CLI contexts there is no request active_user, so this now throws instead of recording the status update.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/entity.php` around lines 81 - 92, The current check in the entity event
logging enforces that $actor must be a User and throws otherwise, which breaks
non-request/CLI sync paths like ServerAccount::sync_report() calling update()
without an actor; change the validation in the Entity logging path so null
actors are allowed (treated as system/automated actions) but invalid non-User
objects still throw: replace the unconditional if(!($actor instanceof User))
throw... with a guard that only throws when $actor is non-null and not an
instance of User (e.g. if($actor !== null && !($actor instanceof User)) { throw
new RuntimeException(...); }), leaving RuntimeState::get('active_user'),
$GLOBALS['active_user'], ServerAccount::sync_report(), and update() behavior
intact.
| private static function runtime_config() { | ||
| return self::runtime_value('config', array_key_exists('config', $GLOBALS) ? $GLOBALS['config'] : array()); | ||
| } |
There was a problem hiding this comment.
Don’t let runtime_config() silently return an empty array.
Its callers immediately index required keys like defaults, web, and email, so a missing config service now degrades into undefined-key warnings and malformed URLs instead of a clear bootstrap failure. Fail fast here.
Possible fix
private static function runtime_config() {
- return self::runtime_value('config', array_key_exists('config', $GLOBALS) ? $GLOBALS['config'] : array());
+ $config = self::runtime_value('config', array_key_exists('config', $GLOBALS) ? $GLOBALS['config'] : null);
+ if(!is_array($config) || $config === array()) {
+ throw new RuntimeException('Configuration service is unavailable.');
+ }
+ return $config;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/serveraccount.php` around lines 39 - 41, runtime_config() currently
falls back to an empty array when the global 'config' is missing, causing
downstream undefined-key warnings; change runtime_config() (and its use of
runtime_value('config', ...)) to fail fast: detect when 'config' isn't present
or runtime_value returns an empty/non-array, and throw a clear RuntimeException
(or trigger a fatal error) with a message like "Missing required config service"
so callers that expect keys like defaults, web, and email fail immediately.
| <?php if(count($key->list_signatures()) > 0) { ?><a href="<?php outurl('/users/'.urlencode($this->get('uid')).'/pubkeys/'.urlencode($key->id).'#sig')?>"><span class="ska-icon ska-icon-pencil" title="Signed key"></span></a><?php } ?> | ||
| <?php if(count($key->list_destination_rules()) > 0) { ?><a href="<?php outurl('/users/'.urlencode($this->get('uid')).'/pubkeys/'.urlencode($key->id).'#dest')?>"><span class="ska-icon ska-icon-pushpin" title="Destination-restricted"></span></a><?php } ?> |
There was a problem hiding this comment.
Add accessible names to these status links.
These anchors only contain icon spans with title, which does not give the link itself a reliable accessible name. Screen readers can end up with two unlabeled links here.
Suggested fix
- <?php if(count($key->list_signatures()) > 0) { ?><a href="<?php outurl('/users/'.urlencode($this->get('uid')).'/pubkeys/'.urlencode($key->id).'#sig')?>"><span class="ska-icon ska-icon-pencil" title="Signed key"></span></a><?php } ?>
- <?php if(count($key->list_destination_rules()) > 0) { ?><a href="<?php outurl('/users/'.urlencode($this->get('uid')).'/pubkeys/'.urlencode($key->id).'#dest')?>"><span class="ska-icon ska-icon-pushpin" title="Destination-restricted"></span></a><?php } ?>
+ <?php if(count($key->list_signatures()) > 0) { ?><a href="<?php outurl('/users/'.urlencode($this->get('uid')).'/pubkeys/'.urlencode($key->id).'#sig')?>" aria-label="Signed key"><span class="ska-icon ska-icon-pencil" aria-hidden="true"></span></a><?php } ?>
+ <?php if(count($key->list_destination_rules()) > 0) { ?><a href="<?php outurl('/users/'.urlencode($this->get('uid')).'/pubkeys/'.urlencode($key->id).'#dest')?>" aria-label="Destination restricted"><span class="ska-icon ska-icon-pushpin" aria-hidden="true"></span></a><?php } ?>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <?php if(count($key->list_signatures()) > 0) { ?><a href="<?php outurl('/users/'.urlencode($this->get('uid')).'/pubkeys/'.urlencode($key->id).'#sig')?>"><span class="ska-icon ska-icon-pencil" title="Signed key"></span></a><?php } ?> | |
| <?php if(count($key->list_destination_rules()) > 0) { ?><a href="<?php outurl('/users/'.urlencode($this->get('uid')).'/pubkeys/'.urlencode($key->id).'#dest')?>"><span class="ska-icon ska-icon-pushpin" title="Destination-restricted"></span></a><?php } ?> | |
| <?php if(count($key->list_signatures()) > 0) { ?><a href="<?php outurl('/users/'.urlencode($this->get('uid')).'/pubkeys/'.urlencode($key->id).'#sig')?>" aria-label="Signed key"><span class="ska-icon ska-icon-pencil" aria-hidden="true"></span></a><?php } ?> | |
| <?php if(count($key->list_destination_rules()) > 0) { ?><a href="<?php outurl('/users/'.urlencode($this->get('uid')).'/pubkeys/'.urlencode($key->id).'#dest')?>" aria-label="Destination restricted"><span class="ska-icon ska-icon-pushpin" aria-hidden="true"></span></a><?php } ?> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/home.php` around lines 62 - 63, The two icon-only anchor links
generated when count($key->list_signatures()) > 0 and
count($key->list_destination_rules()) > 0 lack accessible names; update the
anchors in templates/home.php that reference $key and $key->id (the links for
signatures and destination rules) to provide explicit accessible names—either
add an appropriate aria-label (e.g., "Signed key" and "Destination-restricted")
on each <a> or include a visually-hidden text span inside the anchor for screen
readers—ensure the label text is descriptive and corresponds to the icon so
assistive tech can identify each link.
| } elseif(isset($_POST['delete_public_key']) && $active_user && $active_user->admin) { | ||
| $delete_key_raw = trim($_POST['delete_public_key']); | ||
| $delete_key_id = filter_var($delete_key_raw, FILTER_VALIDATE_INT); | ||
|
|
||
| if($delete_key_id !== false) { | ||
| $delete_key_id = (int)$delete_key_id; | ||
| foreach($active_user_keys as $public_key) { | ||
| if((int)$public_key->id === $delete_key_id) { | ||
| $user->delete_public_key($public_key); | ||
| break; | ||
| } | ||
| } | ||
| $key_lifecycle_service->delete_entity_public_key($user, $active_user_keys, (int)$delete_key_id); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PHP 8.2, what happens if trim() is called with an array instead of a string?
💡 Result:
In PHP 8.2, calling trim() with an array (instead of a string) throws a TypeError and will typically halt execution unless caught:
TypeError: trim(): Argument #1 ($string) must be of type string, array given [1][2]
This is consistent with PHP 8+ behavior where many internal functions now throw exceptions for invalid argument types rather than only emitting warnings. [3]
Sources: [1] [2] [3]
Guard trim() against non-scalar POST input for PHP 8.2+ compatibility.
Line 50 calls trim() directly on $_POST['delete_public_key']. A request with delete_public_key[]=value passes the isset() check but triggers a TypeError in trim() before the integer validation runs, resulting in a 500 error.
Suggested fix
- $delete_key_raw = trim($_POST['delete_public_key']);
- $delete_key_id = filter_var($delete_key_raw, FILTER_VALIDATE_INT);
+ $delete_key_raw = $_POST['delete_public_key'] ?? null;
+ $delete_key_id = is_scalar($delete_key_raw)
+ ? filter_var(trim((string)$delete_key_raw), FILTER_VALIDATE_INT)
+ : false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@views/user.php` around lines 49 - 54, Guard the call to trim() so it only
runs on scalar POST values: check that $_POST['delete_public_key'] is set and
is_scalar() (or is_string/is_numeric) before assigning $delete_key_raw =
trim($_POST['delete_public_key']); then proceed with filter_var and the existing
call to $key_lifecycle_service->delete_entity_public_key($user,
$active_user_keys, (int)$delete_key_id); in views/user.php to avoid TypeError
when arrays are submitted.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
templates/user_pubkeys.php (1)
47-63:⚠️ Potential issue | 🟡 MinorClose the
Statusdescription tag to keep valid HTML.
<dd>is opened at Line 62 but not closed before</dl>, which can break semantics/accessibility in the key details list.🛠️ Proposed fix
<dt>Status</dt> - <dd><?php out($pubkey->deletion_date !== null ? 'Deleted' : 'Active') ?> + <dd><?php out($pubkey->deletion_date !== null ? 'Deleted' : 'Active') ?></dd> </dl>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/user_pubkeys.php` around lines 47 - 63, The Status <dd> in templates/user_pubkeys.php is missing a closing tag causing invalid HTML; update the Status line that outputs "<?php out($pubkey->deletion_date !== null ? 'Deleted' : 'Active') ?>" (referencing the $pubkey->deletion_date expression) to include a closing </dd> before the closing </dl> so the description list is properly balanced and accessible.model/externalkey.php (1)
115-118:⚠️ Potential issue | 🟡 MinorMissing explicit
return nullstatement.When no row is found, the method implicitly returns
null, but the return typeExternalKey|nullsuggests this should be explicit for clarity and to satisfy static analyzers.♻️ Suggested fix
if ($row = $result->fetch_assoc()) { return new ExternalKey($row['id'], $row); } + return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/externalkey.php` around lines 115 - 118, The method that checks if ($row = $result->fetch_assoc()) and returns new ExternalKey($row['id'], $row) should explicitly return null when no row is found; update the method (the function that constructs and returns ExternalKey) to add a clear "return null;" as the final statement so the ExternalKey|null return type is explicit for static analyzers and readers.model/publickey.php (1)
204-204:⚠️ Potential issue | 🟡 MinorPotential undefined index when config fallback returns empty array.
If
get_runtime_config()returns an empty array (final fallback), accessing$config['web']['baseurl']on lines 204, 228, and 241 will trigger undefined index warnings/errors. Theimport()method handles missing config keys withisset()checks, but the export and summarize methods do not.Consider adding defensive checks or ensuring the fallback never returns an empty array when these methods are called.
🛡️ Proposed defensive check example
public function export_userkey_with_fixed_comment(User $owner, int $comment) { $config = self::get_runtime_config(); if ($comment == 1) { // ... existing logic } else if ($comment == 2) { - $comment = " " . $config['web']['baseurl'] ."/pubkeys/".urlencode($this->id); + $baseurl = $config['web']['baseurl'] ?? ''; + $comment = " " . $baseurl ."/pubkeys/".urlencode($this->id); } else { $comment = ''; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/publickey.php` at line 204, The code assumes $config['web']['baseurl'] exists and will raise undefined index errors if get_runtime_config() returns an empty array; update the places that build the public key URL (the $comment creation in model/publickey.php and the export() and summarize() methods) to defensively read the base URL using isset or null-coalescing (e.g. use $config['web']['baseurl'] ?? ''), then only append "/pubkeys/".urlencode($this->id) when a non-empty base URL is present (or fall back to a safe default like an empty string) so no undefined index warnings occur.model/serveraccount.php (1)
429-435:⚠️ Potential issue | 🟡 MinorAdd null guard for
$sync_request_dirto matchlist_group_membershippattern.Line 430 can return
nullif neither RuntimeState nor$GLOBALShas the key, but line 434 unconditionally calls->add_sync_request(). This causes a fatal error instead of a clear exception.Suggested fix
public function sync_access() { $sync_request_dir = self::runtime_value('sync_request_dir', array_key_exists('sync_request_dir', $GLOBALS) ? $GLOBALS['sync_request_dir'] : null); + if($sync_request_dir === null) { + throw new RuntimeException('Sync request directory service is unavailable.'); + } $sync_request = new SyncRequest; $sync_request->server_id = $this->server_id; $sync_request->account_name = $this->name; $sync_request_dir->add_sync_request($sync_request); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/serveraccount.php` around lines 429 - 435, sync_access currently calls $sync_request_dir->add_sync_request() without checking that runtime_value returned a non-null $sync_request_dir, which can be null; update the sync_access method to mirror list_group_membership's pattern by guarding against a null $sync_request_dir (obtained via runtime_value) and either throw a clear exception or return an error before calling add_sync_request; reference the sync_access method, the runtime_value call that assigns $sync_request_dir, and the add_sync_request invocation when making the change.scripts/sync.php (1)
537-590:⚠️ Potential issue | 🟠 MajorOnly call
sync_report()once per sync run.The all-success path writes
sync successbeforeupdate_status_file()runs. If that later call fails, Lines 580-588 write a secondsync warning, so one sync can emit two status events and the first one is wrong.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sync.php` around lines 537 - 590, Currently sync_report('sync success' ...) is called before update_status_file(), and a second sync_report('sync warning' ...) may be emitted if the status file write fails; instead, defer any sync_report call until after update_status_file() completes and emit exactly one report. Concretely: replace the immediate calls to $server->sync_report(...) in the success/warning branches with setting local variables like $final_status and $final_message (leave SyncFailureReporter::report_server_failure and $failure_occurred logic unchanged), run $server->update_status_file($connection) inside the try/catch as now, then after the try/catch decide the single report to send: if $status_file_update_reason is not null and there was no prior fatal failure use SyncFailureReporter::build_message to build the warning message, call $server->sync_report($final_status, $final_message) once, and for non-fatal status-file failures keep the existing SyncFailureReporter::log_server_nonfatal_issue flow but do not call sync_report earlier. Ensure all references use the existing symbols: $server->sync_report, $server->update_status_file, SyncFailureReporter::log_server_nonfatal_issue, and SyncFailureReporter::build_message.
♻️ Duplicate comments (12)
templates/group.php (1)
263-264:⚠️ Potential issue | 🟡 MinorMissing accessible name for the second hostname input.
Line 131 correctly adds
<span class="visually-hidden">Hostname</span>inside the label, but line 263 only has@without the visually-hidden text. This leaves theaccess-hostnameinput without a proper accessible name.🔧 Proposed fix for consistency
- <span class="input-group-text"><label for="access-hostname">@</label></span> + <span class="input-group-text"><label for="access-hostname">@<span class="visually-hidden">Hostname</span></label></span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/group.php` around lines 263 - 264, The second hostname input with id "access-hostname" currently uses a label span showing only "@" and lacks an accessible name; update the label associated with the "access-hostname" input (the <label for="access-hostname"> wrapper currently inside the span) to include a visually-hidden text node like "Hostname" (matching the pattern used for the first hostname input) so the input has a proper accessible name while preserving the "@" visual prefix.services/request_context.php (2)
13-15:⚠️ Potential issue | 🟠 MajorNormalize
relative_request_urlfrom the parsed path.This replacement keeps query strings in the route key and turns subdirectory requests like
/ska/logininto//login. That breaks downstream route checks and active-nav matching. Build this fromparse_url($_SERVER['REQUEST_URI'], PHP_URL_PATH)and strip the base prefix explicitly.🛠️ Minimal fix
$context->base_url = dirname($_SERVER['SCRIPT_NAME']); $context->request_url = $_SERVER['REQUEST_URI']; - $context->relative_request_url = preg_replace('/^'.preg_quote($context->base_url, '/').'/', '/', $context->request_url); + $request_path = parse_url($context->request_url, PHP_URL_PATH) ?? '/'; + if( + $context->base_url !== '/' + && ($request_path === $context->base_url || str_starts_with($request_path, $context->base_url.'/')) + ) { + $request_path = substr($request_path, strlen($context->base_url)); + } + $context->relative_request_url = $request_path === '' ? '/' : $request_path;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/request_context.php` around lines 13 - 15, The current computation of $context->relative_request_url uses $_SERVER['REQUEST_URI'] with preg_replace and preserves the query string and can produce double slashes; instead, derive the path via parse_url($_SERVER['REQUEST_URI'], PHP_URL_PATH'), then remove the $context->base_url prefix explicitly (e.g., check that the path starts with $context->base_url and substr it) and ensure the result begins with a single '/' (normalize empty result to '/'); update the assignment to $context->relative_request_url to use this parsed-and-trimmed path so query strings are excluded and subdirectory routes like '/ska/login' produce '/login' not '//login'.
20-20:⚠️ Potential issue | 🟠 MajorDo not derive absolute URLs from raw
HTTP_HOST.
HTTP_HOSTis client-controlled. Any consumer ofabsolute_request_urlnow depends on a spoofable host value, which is enough to poison self-links and redirect targets. Resolve this against a configured canonical host or an allowlist instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/request_context.php` at line 20, The code currently builds $context->absolute_request_url using the client-controlled $_SERVER['HTTP_HOST'; replace this by resolving the host against a server-side canonical host or an allowlist: read the canonical host (e.g. from configuration or env) or validate $_SERVER['HTTP_HOST'] against an allowlist and only use it if it matches; otherwise fall back to the canonical host; then assemble $context->absolute_request_url using $scheme, the validated/canonical host, and $context->request_url, and ensure any host value is normalized (lowercased, trimmed) before concatenation.services/login_flow.php (4)
22-25:⚠️ Potential issue | 🟡 MinorBlank usernames hit the wrong validation branch.
The format check runs before the empty-field check, so an empty username (after trim) returns the format error instead of the intended "enter both username and password" message. The regex
/^[a-zA-Z0-9._-]+$/fails on empty string, producing a misleading error.Proposed fix
- if(!preg_match('/^[a-zA-Z0-9._-]+$/', $username)) { - $error_message = 'Invalid username format. Username can only contain letters, numbers, dots, hyphens, and underscores.'; - } elseif($username === '' || $password === '') { + if($username === '' || $password === '') { $error_message = 'Please enter both username and password.'; + } elseif(!preg_match('/^[a-zA-Z0-9._-]+$/', $username)) { + $error_message = 'Invalid username format. Username can only contain letters, numbers, dots, hyphens, and underscores.'; } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/login_flow.php` around lines 22 - 25, The validation currently runs the regex check for $username before verifying emptiness, causing empty usernames to trigger the format error; update the validation in services/login_flow.php so the empty-field check (use trimmed values like $username = trim($username); $password = trim($password); and test $username === '' || $password === '') runs before the preg_match('/^[a-zA-Z0-9._-]+$/', $username) branch, i.e., swap the order of the conditions (or explicitly check emptiness first) and then apply the regex to non-empty $username to ensure the "Please enter both username and password." message is returned for blank inputs.
92-107: 🛠️ Refactor suggestion | 🟠 MajorDuplicate
sanitize_redirect_pathdiverges fromRequestAuthGuardversion.This method omits checks for
\r/\nnewlines anduser/pass/portURL components that exist inRequestAuthGuard::sanitize_redirect_path. Consider extracting to a shared utility to avoid security posture divergence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/login_flow.php` around lines 92 - 107, The sanitize_redirect_path in services/login_flow.php differs from RequestAuthGuard::sanitize_redirect_path by missing checks for CR/LF characters and URL components like user, pass, and port, so update this function (sanitize_redirect_path) to reject candidates containing "\r" or "\n" and to return "/" if parse_url finds 'user', 'pass', or 'port' set; then refactor by extracting the common logic into a shared utility (e.g., a new sanitize_redirect_path utility used by both RequestAuthGuard::sanitize_redirect_path and services/login_flow.php) and replace both implementations to call that shared function to keep behavior identical and avoid security drift.
16-17:⚠️ Potential issue | 🟠 Major
hash_equalswill throwTypeErrorifcsrf_tokenis submitted as an array.In PHP 8+,
hash_equalsrequires both arguments to be strings. An attacker can submitcsrf_token[]which makes$post_data['csrf_token']an array, bypassing theissetcheck and throwing aTypeError. Add anis_string()guard for PHP 8.2+ compatibility.Proposed fix
- if(!isset($post_data['csrf_token']) || !hash_equals($_SESSION['csrf_token'], $post_data['csrf_token'])) { + if(!isset($post_data['csrf_token']) || !is_string($post_data['csrf_token']) || !hash_equals($_SESSION['csrf_token'], $post_data['csrf_token'])) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/login_flow.php` around lines 16 - 17, The current CSRF check uses hash_equals($_SESSION['csrf_token'], $post_data['csrf_token']) which will throw a TypeError if $post_data['csrf_token'] is an array; update the conditional in the login_flow.php CSRF validation to first ensure the submitted token is a string (and optionally ensure $_SESSION['csrf_token'] is a string) using is_string() before calling hash_equals so that non-string inputs are rejected safely and TypeError is avoided.
74-90:⚠️ Potential issue | 🟠 MajorSession-based rate limiting is trivially bypassable.
Login attempts are tracked in
$_SESSION, so an attacker can bypass the limit by clearing cookies or starting a new session. Consider backing the rate limit with a server-side store (database, cache) keyed by username and/or IP for effective brute-force protection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/login_flow.php` around lines 74 - 90, The current rate limiter uses $_SESSION (in is_rate_limited and record_failed_attempt) which can be bypassed by clearing cookies; change these functions to use a server-side store (e.g., Redis, DB table, or in-memory cache) keyed by username and/or IP instead of $_SESSION, implement atomic increment and TTL semantics (store count and timestamp or use Redis INCR with expiration), enforce the same threshold (>=5) and 15-minute window, and update login flow to read the shared store in is_rate_limited and to increment/set the counter in record_failed_attempt (keep session-based as a fallback only if the server store is unavailable).core.php (2)
84-93:⚠️ Potential issue | 🟠 MajorStill don’t throw from the autoloader.
Line 92 turns a normal autoload miss into an exception, which aborts the remaining autoload chain and can block Composer/vendor resolution. This should just return when the model file is absent.
♻️ Suggested fix
function autoload_model($classname) { - $original_classname = $classname; $classname = preg_replace('/[^a-z]/', '', strtolower($classname)); # Prevent directory traversal and sanitize name $base_path = RuntimeState::get('base_path', dirname(__FILE__)); $filename = path_join($base_path, 'model', $classname.'.php'); if(file_exists($filename)) { include($filename); - } else { - throw new InvalidArgumentException("Attempted to load class {$original_classname} but model file {$filename} does not exist."); } }In PHP, what happens if an earlier `spl_autoload_register()` callback throws instead of returning when it cannot resolve a class?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core.php` around lines 84 - 93, The autoloader function autoload_model converts a miss into an exception which aborts the SPL autoload chain; instead of throwing in the else branch, simply return (or return false) so the autoload chain can continue (leave the sanitization, path construction and include logic in autoload_model intact, but replace the throw in the else block with a non-throwing return).
99-118:⚠️ Potential issue | 🟠 MajorStill catch
mysqli_sql_exceptionhere too.On PHP 8.2+, a failed
new mysqli(...)can throwmysqli_sql_exception. Catching onlyErrorExceptionlets connection failures bypass theDBConnectionFailedExceptionmapping.🛠️ Suggested fix
- } catch(ErrorException $e) { - throw new DBConnectionFailedException($e->getMessage()); + } catch(mysqli_sql_exception | ErrorException $e) { + throw new DBConnectionFailedException($e->getMessage(), 0, $e); }In PHP 8.2, what exception does `new mysqli(...)` throw by default when the connection fails, and does catching only `ErrorException` miss it?As per coding guidelines,
**/*.php: Ensure PHP 8.2+ compatibility for all PHP code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core.php` around lines 99 - 118, The setup_database function currently only catches ErrorException when constructing new mysqli, which on PHP 8.2+ can throw mysqli_sql_exception and thus bypass DBConnectionFailedException; update the error handling in setup_database to also catch mysqli_sql_exception (or a broader Throwable/Exception) and rethrow as DBConnectionFailedException with the original message (and optionally chain the previous exception) so connection failures from mysqli_sql_exception are wrapped consistently by DBConnectionFailedException.public_html/extra.js (1)
565-577:⚠️ Potential issue | 🟡 MinorGive each injected reassign field a unique id.
Every expanded section still creates
id="reassign_to"and the label is not linked to it, so multiple widgets on the same page end up with duplicate ids and no click-to-focus behavior.🛠️ Suggested fix
+ var inputId = 'reassign_to_' + id; var label = document.createElement('label'); label.className = 'form-label'; label.textContent = 'Reassign to'; + label.htmlFor = inputId; var input = document.createElement('input'); input.type = 'text'; - input.id = 'reassign_to'; + input.id = inputId; input.name = 'reassign_to'; input.className = 'form-control';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public_html/extra.js` around lines 565 - 577, The injected reassign field currently always uses the static id "reassign_to" causing duplicate ids and unlinked labels; modify the code that creates reassignWrapper/label/input so the input.id is generated uniquely (e.g., append a monotonic counter or a timestamp) and set label.htmlFor to that same unique id (update symbols: reassignWrapper, label, input, id "reassign_to"); ensure the generation logic is deterministic within the page (use a module-scoped counter or Date.now()+Math.random()) so each expanded section gets a distinct input.id and the label click focuses the correct input.scripts/smoke/browser-lib.sh (1)
32-34:⚠️ Potential issue | 🟠 MajorRemove the probe-and-reuse port flow.
browser_smoke_pick_port()closes the probe socket before the child process binds it, so another process can steal the chosen port and make the smoke run flaky. Let the spawned process bind the port itself and then persist the actual value instead of probing first.Also applies to: 74-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/smoke/browser-lib.sh` around lines 32 - 34, The current browser_smoke_pick_port() function probes a free port by opening and closing a socket which introduces a race where another process can steal the port; remove this probe-and-reuse flow (including the duplicate logic around lines 74-86) and instead change the smoke startup so the spawned browser/process binds an ephemeral port itself and then writes the bound port back (e.g., to stdout, a temp file, or env file) which the parent reads to persist the actual port value; update any callers of browser_smoke_pick_port() to stop using the probe function and to read the port from the child’s reported output (and delete the probe function/browser_smoke_pick_port() from the script).services/relation_lifecycle_service.php (1)
97-115:⚠️ Potential issue | 🟠 MajorReturn partial reassignment failures to the caller.
This still collapses per-server exceptions into
error_log()and returnsvoid, so the caller cannot warn about skipped hosts. Ifadd_server_admin()succeeds anddelete_server_admin()fails, the server is also left in a partially reassigned state.🛠️ Suggested direction
-public function reassign_server_admins_by_hostname(array $servers, array $selected_server_hostnames, Entity $from_admin, Entity $to_admin): void { +public function reassign_server_admins_by_hostname(array $servers, array $selected_server_hostnames, Entity $from_admin, Entity $to_admin): array { $errors = array(); foreach($servers as $server) { ... } - foreach($errors as $error) { - error_log('reassign_server_admins_by_hostname: '.$error); - } + return $errors; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/relation_lifecycle_service.php` around lines 97 - 115, The reassign_server_admins_by_hostname function currently swallows per-server errors and returns void; change it to surface partial failures by returning an array of failure messages (or throwing an AggregateException) and make the reassignment atomic per-server: for each Server in reassign_server_admins_by_hostname call add_server_admin($server, $to_admin) then call delete_server_admin($server, $from_admin) and if delete_server_admin throws, attempt to roll back by removing the newly added admin (e.g., call delete_server_admin or a remove_server_admin helper) and include both the original delete error and any rollback error in the returned failures; include references to the server hostname in each message so the caller can act on skipped hosts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@phpstan-baseline.neon`:
- Around line 28-36: SyncProcess::get_data() has a docblock/@return declaring
string but actually returns arrays and can fall through without a return; update
the method to have a consistent return type (either change the docblock to
`@return` array|string and ensure all return paths return that type, or refactor
to always return a string) and add an explicit final return to eliminate the
implicit null path; also verify the true-positive/false-positive for
history_username_env_format_is_valid by ensuring the function definition in
history_username_env_common.php is autoloaded or included before
scripts/sync.php uses it, but do not suppress SyncProcess::get_data() via
baseline—fix the implementation or its signature instead.
In `@public_html/extra.js`:
- Around line 793-799: The keydown handler on serverAdmin currently treats Space
(' ') as a token separator which prevents entering admin names with spaces;
update the event listener (serverAdmin.addEventListener) so it does not treat '
' as a trigger — only handle Enter and comma (e.g., check event.key === 'Enter'
|| event.key === ',') before calling append_admin(), and keep the existing
preventDefault(), blur(), focus() behavior unchanged.
In `@public_html/style.css`:
- Around line 225-256: Add back minimal legacy visibility utilities so existing
callers that toggle d-none and invisible keep working: define CSS rules for
.d-none (map to .ska-hide behavior), .invisible (map to .ska-hide but preserving
layout like visibility:hidden if intended), and their shown counterparts
(.d-block/.d-inline/.visible) consistent with .ska-show/.show; ensure
transitions/classes that use .collapse, .collapsing, .fade continue to behave by
matching display/opacity/height settings already used by
.ska-hide/.ska-show/.collapse/.collapsing/.fade so extra.js toggles still hide
elements when the previous dependency is removed.
In `@scripts/sync-failure.php`:
- Around line 138-148: The composed message can be broken by raw semicolons in
summary or reason; update compose_message to escape semicolons in the fields
before joining (e.g., replace ";" with "\;") so the ';' delimiter cannot create
extra pseudo-fields—apply this to the summary and the result of
self::normalize_reason($reason) in compose_message and mirror the same escaping
change in the other similar compose_message usage (the second occurrence
mentioned) so both places sanitize semicolons consistently.
In `@services/request_context.php`:
- Around line 21-22: The bypass flag assignment for
$context->bypass_csrf_protection currently trusts
$_SERVER['HTTP_X_BYPASS_CSRF_PROTECTION'] unconditionally; change it to only set
the flag when the header is present AND a non-production condition holds (e.g.,
APP_ENV is defined and equals 'development' or 'test', or the requester IP is in
an allowlist). Update the logic around $context->bypass_csrf_protection so it
checks both the header and an environment check (or IP allowlist) before
enabling the bypass, and ensure request_csrf_guard.php will only see the bypass
when that gated condition is true.
In `@services/response_security_headers.php`:
- Around line 28-31: Before emitting the new CSP headers in
services/response_security_headers.php (the block using
is_csp_report_only($config) and header('Content-Security-Policy...') /
header('Content-Security-Policy-Report-Only...')), remove any legacy CSP sent by
the template first by calling header_remove('Content-Security-Policy') (and
optionally header_remove('Content-Security-Policy-Report-Only') for safety)
immediately before sending the new header so browsers do not combine the old
blocking policy with the new one; keep using $csp and the existing
is_csp_report_only($config) branch for choosing which header to emit.
In `@templates/groups.php`:
- Around line 127-133: Remove or fix the orphaned label with for="name" in the
LDAP group connection block in templates/groups.php: either delete the <label
for="name" class="visually-hidden">Group name</label> since there is no input
with id="name" in the LDAP form, or replace/rewire it to point to the correct
input/id used by the LDAP treeview control (or add a matching hidden/select
input with id="name") so the label references an actual form control for
accessibility.
In `@templates/home.php`:
- Around line 86-88: The template includes a duplicate id "add_public_key" on
the textarea which conflicts with an earlier element; change the textarea's id
to a unique identifier (e.g., "add_public_key_2") and update the corresponding
<label for="..."> to match that new id, and also search for any JS references to
getElementById('add_public_key') or form field names that rely on that id and
update them to the new id to keep label associations and scripts working (target
the textarea element with id "add_public_key" in this diff and the label tag
that references it).
In `@templates/server.php`:
- Line 90: Remove hard-coded aria-hidden attributes from the tab pane elements
(e.g., the div with class "tab-pane fade active show" and id "accounts") so
Bootstrap can manage visibility for assistive tech; locate all elements using
class "tab-pane" (including the panes at the IDs or locations referenced in the
comment) and delete their aria-hidden="..." attributes, ensuring no other
changes to classes or aria-selected on tab controls.
In `@templates/serveraccount.php`:
- Around line 217-218: The hostname input lacks an accessible name because the
label for="hostname" contains only "@"; update the label or input so the control
has a clear accessible name: either add a visible or visually-hidden descriptive
string inside the existing <label for="hostname"> (e.g., include a span with a
"visually-hidden" or "sr-only" class containing "Hostname") or add an
appropriate aria-label on the input with id="hostname"; ensure the label's for
attribute still matches id="hostname" and remove redundant/confusing characters
so screen readers announce the field as "Hostname".
In `@views/login.php`:
- Around line 19-28: Update LoginFlowService::handle_request to emit persistent
audit/event logs for every auth attempt: inject or obtain an audit logger (e.g.,
AuditLogger or EventDispatcher) in the LoginFlowService constructor and call it
inside handle_request to record (1) successful authentications with username and
timestamp, (2) failed authentications with username, failure reason and
timestamp, and (3) rate-limited requests with username and timestamp; use the
username from the incoming request or from the returned $login_state and ensure
each log call references the LoginFlowService class and handle_request so these
events are consistently recorded.
---
Outside diff comments:
In `@model/externalkey.php`:
- Around line 115-118: The method that checks if ($row = $result->fetch_assoc())
and returns new ExternalKey($row['id'], $row) should explicitly return null when
no row is found; update the method (the function that constructs and returns
ExternalKey) to add a clear "return null;" as the final statement so the
ExternalKey|null return type is explicit for static analyzers and readers.
In `@model/publickey.php`:
- Line 204: The code assumes $config['web']['baseurl'] exists and will raise
undefined index errors if get_runtime_config() returns an empty array; update
the places that build the public key URL (the $comment creation in
model/publickey.php and the export() and summarize() methods) to defensively
read the base URL using isset or null-coalescing (e.g. use
$config['web']['baseurl'] ?? ''), then only append
"/pubkeys/".urlencode($this->id) when a non-empty base URL is present (or fall
back to a safe default like an empty string) so no undefined index warnings
occur.
In `@model/serveraccount.php`:
- Around line 429-435: sync_access currently calls
$sync_request_dir->add_sync_request() without checking that runtime_value
returned a non-null $sync_request_dir, which can be null; update the sync_access
method to mirror list_group_membership's pattern by guarding against a null
$sync_request_dir (obtained via runtime_value) and either throw a clear
exception or return an error before calling add_sync_request; reference the
sync_access method, the runtime_value call that assigns $sync_request_dir, and
the add_sync_request invocation when making the change.
In `@scripts/sync.php`:
- Around line 537-590: Currently sync_report('sync success' ...) is called
before update_status_file(), and a second sync_report('sync warning' ...) may be
emitted if the status file write fails; instead, defer any sync_report call
until after update_status_file() completes and emit exactly one report.
Concretely: replace the immediate calls to $server->sync_report(...) in the
success/warning branches with setting local variables like $final_status and
$final_message (leave SyncFailureReporter::report_server_failure and
$failure_occurred logic unchanged), run $server->update_status_file($connection)
inside the try/catch as now, then after the try/catch decide the single report
to send: if $status_file_update_reason is not null and there was no prior fatal
failure use SyncFailureReporter::build_message to build the warning message,
call $server->sync_report($final_status, $final_message) once, and for non-fatal
status-file failures keep the existing
SyncFailureReporter::log_server_nonfatal_issue flow but do not call sync_report
earlier. Ensure all references use the existing symbols: $server->sync_report,
$server->update_status_file, SyncFailureReporter::log_server_nonfatal_issue, and
SyncFailureReporter::build_message.
In `@templates/user_pubkeys.php`:
- Around line 47-63: The Status <dd> in templates/user_pubkeys.php is missing a
closing tag causing invalid HTML; update the Status line that outputs "<?php
out($pubkey->deletion_date !== null ? 'Deleted' : 'Active') ?>" (referencing the
$pubkey->deletion_date expression) to include a closing </dd> before the closing
</dl> so the description list is properly balanced and accessible.
---
Duplicate comments:
In `@core.php`:
- Around line 84-93: The autoloader function autoload_model converts a miss into
an exception which aborts the SPL autoload chain; instead of throwing in the
else branch, simply return (or return false) so the autoload chain can continue
(leave the sanitization, path construction and include logic in autoload_model
intact, but replace the throw in the else block with a non-throwing return).
- Around line 99-118: The setup_database function currently only catches
ErrorException when constructing new mysqli, which on PHP 8.2+ can throw
mysqli_sql_exception and thus bypass DBConnectionFailedException; update the
error handling in setup_database to also catch mysqli_sql_exception (or a
broader Throwable/Exception) and rethrow as DBConnectionFailedException with the
original message (and optionally chain the previous exception) so connection
failures from mysqli_sql_exception are wrapped consistently by
DBConnectionFailedException.
In `@public_html/extra.js`:
- Around line 565-577: The injected reassign field currently always uses the
static id "reassign_to" causing duplicate ids and unlinked labels; modify the
code that creates reassignWrapper/label/input so the input.id is generated
uniquely (e.g., append a monotonic counter or a timestamp) and set label.htmlFor
to that same unique id (update symbols: reassignWrapper, label, input, id
"reassign_to"); ensure the generation logic is deterministic within the page
(use a module-scoped counter or Date.now()+Math.random()) so each expanded
section gets a distinct input.id and the label click focuses the correct input.
In `@scripts/smoke/browser-lib.sh`:
- Around line 32-34: The current browser_smoke_pick_port() function probes a
free port by opening and closing a socket which introduces a race where another
process can steal the port; remove this probe-and-reuse flow (including the
duplicate logic around lines 74-86) and instead change the smoke startup so the
spawned browser/process binds an ephemeral port itself and then writes the bound
port back (e.g., to stdout, a temp file, or env file) which the parent reads to
persist the actual port value; update any callers of browser_smoke_pick_port()
to stop using the probe function and to read the port from the child’s reported
output (and delete the probe function/browser_smoke_pick_port() from the
script).
In `@services/login_flow.php`:
- Around line 22-25: The validation currently runs the regex check for $username
before verifying emptiness, causing empty usernames to trigger the format error;
update the validation in services/login_flow.php so the empty-field check (use
trimmed values like $username = trim($username); $password = trim($password);
and test $username === '' || $password === '') runs before the
preg_match('/^[a-zA-Z0-9._-]+$/', $username) branch, i.e., swap the order of the
conditions (or explicitly check emptiness first) and then apply the regex to
non-empty $username to ensure the "Please enter both username and password."
message is returned for blank inputs.
- Around line 92-107: The sanitize_redirect_path in services/login_flow.php
differs from RequestAuthGuard::sanitize_redirect_path by missing checks for
CR/LF characters and URL components like user, pass, and port, so update this
function (sanitize_redirect_path) to reject candidates containing "\r" or "\n"
and to return "/" if parse_url finds 'user', 'pass', or 'port' set; then
refactor by extracting the common logic into a shared utility (e.g., a new
sanitize_redirect_path utility used by both
RequestAuthGuard::sanitize_redirect_path and services/login_flow.php) and
replace both implementations to call that shared function to keep behavior
identical and avoid security drift.
- Around line 16-17: The current CSRF check uses
hash_equals($_SESSION['csrf_token'], $post_data['csrf_token']) which will throw
a TypeError if $post_data['csrf_token'] is an array; update the conditional in
the login_flow.php CSRF validation to first ensure the submitted token is a
string (and optionally ensure $_SESSION['csrf_token'] is a string) using
is_string() before calling hash_equals so that non-string inputs are rejected
safely and TypeError is avoided.
- Around line 74-90: The current rate limiter uses $_SESSION (in is_rate_limited
and record_failed_attempt) which can be bypassed by clearing cookies; change
these functions to use a server-side store (e.g., Redis, DB table, or in-memory
cache) keyed by username and/or IP instead of $_SESSION, implement atomic
increment and TTL semantics (store count and timestamp or use Redis INCR with
expiration), enforce the same threshold (>=5) and 15-minute window, and update
login flow to read the shared store in is_rate_limited and to increment/set the
counter in record_failed_attempt (keep session-based as a fallback only if the
server store is unavailable).
In `@services/relation_lifecycle_service.php`:
- Around line 97-115: The reassign_server_admins_by_hostname function currently
swallows per-server errors and returns void; change it to surface partial
failures by returning an array of failure messages (or throwing an
AggregateException) and make the reassignment atomic per-server: for each Server
in reassign_server_admins_by_hostname call add_server_admin($server, $to_admin)
then call delete_server_admin($server, $from_admin) and if delete_server_admin
throws, attempt to roll back by removing the newly added admin (e.g., call
delete_server_admin or a remove_server_admin helper) and include both the
original delete error and any rollback error in the returned failures; include
references to the server hostname in each message so the caller can act on
skipped hosts.
In `@services/request_context.php`:
- Around line 13-15: The current computation of $context->relative_request_url
uses $_SERVER['REQUEST_URI'] with preg_replace and preserves the query string
and can produce double slashes; instead, derive the path via
parse_url($_SERVER['REQUEST_URI'], PHP_URL_PATH'), then remove the
$context->base_url prefix explicitly (e.g., check that the path starts with
$context->base_url and substr it) and ensure the result begins with a single '/'
(normalize empty result to '/'); update the assignment to
$context->relative_request_url to use this parsed-and-trimmed path so query
strings are excluded and subdirectory routes like '/ska/login' produce '/login'
not '//login'.
- Line 20: The code currently builds $context->absolute_request_url using the
client-controlled $_SERVER['HTTP_HOST'; replace this by resolving the host
against a server-side canonical host or an allowlist: read the canonical host
(e.g. from configuration or env) or validate $_SERVER['HTTP_HOST'] against an
allowlist and only use it if it matches; otherwise fall back to the canonical
host; then assemble $context->absolute_request_url using $scheme, the
validated/canonical host, and $context->request_url, and ensure any host value
is normalized (lowercased, trimmed) before concatenation.
In `@templates/group.php`:
- Around line 263-264: The second hostname input with id "access-hostname"
currently uses a label span showing only "@" and lacks an accessible name;
update the label associated with the "access-hostname" input (the <label
for="access-hostname"> wrapper currently inside the span) to include a
visually-hidden text node like "Hostname" (matching the pattern used for the
first hostname input) so the input has a proper accessible name while preserving
the "@" visual prefix.
---
Nitpick comments:
In @.php-cs-fixer.dist.php:
- Around line 3-9: The PhpCsFixer Finder ($finder created by
PhpCsFixer\Finder::create()) currently restricts ->in([...]) to 'model',
'scripts', and 'services', omitting root-level PHP entrypoints; update the
Finder configuration to include the project root or explicitly append the root
files (e.g., core.php, requesthandler.php, router.php, routes.php, ldap.php,
email.php) so that ->name('*.php') also matches those files — modify the
->in(...) array to add __DIR__ (project root) or use ->append([...]) with the
specific __DIR__ . '/<filename>.php' entries while keeping the existing filters
intact.
In `@AGENTS.md`:
- Around line 38-49: The compatibility note forgot to require agents to run the
CLI sync and verify the actual deployed write target; update the agent workflow
and the sync scripts (scripts/sync*.php) to ensure the sync step writes the
authorized keys files to the real target directory /var/local/keys-sync/ on
target servers, and add a post-sync verification step that checks the expected
files (e.g., keys-sync and keys-sync.pub or per-host authorized_keys) exist and
have correct contents/permissions; modify scripts/sync*.php to explicitly write
to /var/local/keys-sync/ (not just a temp/smoke path) and return non-zero on
mismatch so CLI sync and the agent workflow can surface failures.
In `@docs/modernization-plan.md`:
- Around line 20-26: The three consecutive bullet items in the plan all start
with "Preserve", which reduces readability; update the bullets under the goals
section so they vary sentence starters (e.g., change one to "Keep event/audit
logging semantics unchanged" and another to "Maintain critical workflows:"), and
ensure the sub-bullets (LDAP login/auth, add/remove public keys, create/remove
access rules, trigger sync and verify keyfile outputs) remain intact under the
"Maintain critical workflows" item so meaning and structure are preserved.
In `@model/dbdirectory.php`:
- Around line 28-34: The constructor in model/dbdirectory.php currently sets
$this->database = null when no global or RuntimeState database exists; change
this to throw BadMethodCallException('Database connection is not available.')
(the same behavior as externalkey.php) so callers like
UserDirectory/ServerDirectory don't hit null-pointer errors; locate the
assignment block that checks $GLOBALS['database'] and
RuntimeState::get('database') and replace the final else (or add a null-check
after the block) to throw BadMethodCallException with the same message.
In `@model/externalkey.php`:
- Around line 46-56: The database resolution logic duplicated in
list_external_keys and get_by_id should be extracted into a single reusable
helper (e.g., a private static resolveDatabase method or a trait used by models)
that implements the RuntimeState → $GLOBALS → throw behavior; replace the inline
blocks in list_external_keys and get_by_id with calls to this helper (or trait
method) so both methods obtain the mysqli instance via resolveDatabase(), and
ensure the helper throws BadMethodCallException when no connection is found.
In `@model/group.php`:
- Around line 23-32: The Group model defines private static methods
runtime_value() and runtime_config() that duplicate runtime resolution logic
used elsewhere; move this logic into the shared Entity base class and have Group
call the existing self::resolve_runtime() (or add resolve_runtime if missing) to
unify behavior. Specifically, extract the bodies of Group::runtime_value and
Group::runtime_config into a single protected/static method on Entity (e.g.,
resolve_runtime or runtime_value on Entity), update Group to remove its private
methods and call Entity::resolve_runtime/self::resolve_runtime, and ensure User
and other models use the same Entity method so all models share the same runtime
resolution implementation.
In `@model/record.php`:
- Around line 52-65: The resolve_runtime() helper currently implements
RuntimeState-first resolution but is never used while the constructor uses
$GLOBALS-first; to fix, update the constructor (the method that currently reads
$GLOBALS and RuntimeState directly) to call self::resolve_runtime($key,
$default) for each runtime dependency so resolution is centralized and
consistent, or alternatively remove the unused protected static function
resolve_runtime if you intentionally prefer $GLOBALS-first behavior—pick one
approach and apply it across the constructor and any other runtime lookups to
eliminate dead code and ensure consistent resolution order.
In `@model/server.php`:
- Around line 583-584: connect_ssh may call $server_dir->list_servers() when
$server_dir is null (obtained via self::runtime_value('server_dir')), causing a
fatal error; update connect_ssh to null-guard $server_dir (from the variable
assigned after self::runtime_config()/self::runtime_value) before calling
list_servers by either returning/throwing a clear error or setting a sensible
default, e.g., check if $server_dir is null and handle early (log/throw/return)
or skip list_servers usage; reference the connect_ssh function and the
$server_dir variable and its list_servers() call when making the change.
In `@model/userdirectory.php`:
- Around line 34-38: The resolution order for LDAP configuration is inverted
compared to the pattern used in dbdirectory.php; change the assignment in
model/userdirectory.php so it prioritizes $GLOBALS['ldap'] and only falls back
to RuntimeState::get when the global is not present: update the code that sets
$this->ldap (currently using RuntimeState::get('ldap', $GLOBALS['ldap'] ??
null)) to mirror the dbdirectory.php approach by checking
array_key_exists('ldap', $GLOBALS) first and using RuntimeState::get('ldap')
only as the fallback, ensuring RuntimeState::get and $GLOBALS['ldap'] are
referenced as the unique identifiers to locate the change.
In `@scripts/smoke/browser-interactions.sh`:
- Around line 48-55: The JS check inside browser_smoke_wait_for_js uses a weak
assertion for the closed state by testing menu.getAttribute("aria-hidden") !==
"false"; update this to assert the explicit closed value instead
(menu.getAttribute("aria-hidden") === "true") so the check fails if the
attribute is missing or incorrect; adjust the conditional that references
trigger and menu (the variables named trigger and menu inside the
browser_smoke_wait_for_js string) accordingly to require aria-hidden === "true".
In `@scripts/smoke/helpers/find_account_access_rule.php`:
- Around line 1-65: This file duplicates CLI option parsing, file loading and
DOM setup logic that also exists in find_user_public_key.php; extract shared
helpers (e.g., parseCliArgs or getCliOptions for --html/--source-user,
loadHtmlDocument that reads and returns a DOMDocument, and
findAncestorRow(DOMNode $node, string $tagName) to climb to the enclosing tr)
and use them from this script (replace inline parsing, file_get_contents +
DOMDocument/loadHTML boilerplate, and the parent-tr traversal loop) and update
find_user_public_key.php to call the same helpers so both scripts share the same
parsing/loading/traversal code.
In `@scripts/smoke/helpers/find_user_public_key.php`:
- Around line 41-48: The parent traversal loop using $row = $row->parentNode may
end on a DOMDocument (not a DOMElement) so add a type check when walking
ancestors and before reading $row->nodeName: ensure $row is an instance of
DOMElement (or DOMNode with nodeName available) inside the while condition and
before the if-check; update the loop that references $buttons and $row to
break/continue when $row is not a DOMElement, and use instanceof DOMElement when
comparing nodeName to 'tr' to make the traversal robust against DOMDocument
roots.
In `@scripts/smoke/run.sh`:
- Around line 48-51: The invocation pattern is inconsistent: web-workflows.sh is
executed directly while browser-interactions.sh is invoked via bash; update the
block that checks MODE (variable MODE) to call both scripts the same way by
either prefixing both with bash or invoking both directly using
"$SCRIPT_DIR/web-workflows.sh" and "$SCRIPT_DIR/browser-interactions.sh"
(reference: SCRIPT_DIR, web-workflows.sh, browser-interactions.sh) so the script
invocation style is consistent.
In `@scripts/smoke/web-workflows.sh`:
- Around line 23-29: Remove the dead regex helper variables LEGACY_DROPDOWN_RE
and LEGACY_LAYOUT_RE from the script: delete their variable definitions and any
unused references to them so the file no longer defines unused patterns; keep
the remaining helpers (e.g., LEGACY_CARET_RE, LEGACY_IN_CLASS_RE,
LEGACY_LAYOUT_TOKENS, LEGACY_PRESENTATION_TOKENS, LEGACY_PRESENTATION_RE) intact
and run the smoke harness to ensure no runtime references remain after removal.
In `@scripts/sync-runtime.php`:
- Around line 28-39: The method build_timeout_wrapped_command currently lacks a
return type declaration; update its signature to declare a string return type
and do the same for related methods (diagnostics(array $config): array,
resolve_timeout_util(array $config): string, resolve_timeout_binary(array
$config): string) so PHP 8.2+ static typing and IDEs can validate return values;
locate and modify the function signatures for build_timeout_wrapped_command,
diagnostics, resolve_timeout_util, and resolve_timeout_binary to add the
appropriate return types without changing their bodies.
In `@services/access_rule_service.php`:
- Around line 17-20: Replace the ternary isset check with the null coalescing
operator when assigning the AccessOption->value: instead of using
isset($value['value']) ? $value['value'] : null, use $value['value'] ?? null in
the block that constructs the AccessOption instance ($option = new
AccessOption(); $option->option = $key; $option->value = ...; $options[] =
$option;).
- Around line 1-3: Add a file-level strict types declaration to enable strict
mode by inserting declare(strict_types=1); immediately after the opening <?php
tag in the file containing class AccessRuleService, and then update the class's
method signatures (in AccessRuleService) to use scalar and object type hints and
explicit return types where appropriate so the code complies with strict typing;
ensure any internal calls and tests are adjusted for strict type behavior.
- Around line 52-70: Both delete_access_by_id and delete_group_access_by_id
silently no-op when find_access_by_id returns null; change them to return a
boolean and emit a warning when no access is found. Modify
delete_access_by_id(ServerAccount $account, array $account_access, $access_id)
and delete_group_access_by_id(Group $group, array $group_access, $access_id) so
they return true after calling $account->delete_access($access) /
$group->delete_access($access), and return false (and call a logger warning or
error_log with the access_id and context) when $access is not an instance of
Access; keep idempotency but surface missing-ID cases via the boolean return and
a logged warning.
In `@services/request_exception_handler.php`:
- Around line 16-26: Replace the time()-based $error_number generation in the
handle method with a collision-resistant identifier: generate and assign a
unique ID (e.g., using uniqid with more_entropy or a secure random component
like bin2hex(random_bytes(...))) to $error_number before logging and including
it in the error output, so error IDs in error_log and views/error500.php are
unique even under high concurrency.
In `@services/request_policy_guard.php`:
- Around line 11-16: The method enforce_active_user_status declares unused
parameters $relative_request_url and $absolute_request_url; either remove these
parameters from the function signature and all call sites to avoid dead args, or
if they are reserved for future use keep them and add a brief inline comment or
PHPDoc (e.g., `@param` string $relative_request_url reserved) or an annotation to
suppress unused-parameter warnings so static analyzers understand they are
intentional; update the function signature or add the comment/annotation next to
enforce_active_user_status accordingly and ensure callers remain compatible if
you remove parameters.
In `@services/request_router_dispatcher.php`:
- Around line 13-22: The exception in resolve_view_path currently exposes the
full filesystem path ($view) which is sensitive; change the throw in
resolve_view_path to omit the absolute path and instead include only a safe
identifier such as the requested view name ($router->view) or a generic message.
Locate resolve_view_path (uses $router, $base_path and $view from path_join) and
replace the throw Exception("View file $view missing.") with something like a
generic "View file missing" or "View file for '{$router->view}' missing" so no
system path is leaked.
In `@services/runtime_state.php`:
- Around line 3-26: Add a public static reset/clear method to RuntimeState to
allow tests to wipe global state between runs: implement, for example, public
static function clear() { self::$state = array(); } (or named reset()) in the
RuntimeState class so tests can call RuntimeState::clear() to restore a clean
state; keep $state as the same static property and ensure the method is public
and static to match existing usage patterns.
In `@templates/activity.php`:
- Around line 29-33: The table header row in templates/activity.php uses plain
<th> elements for "Entity", "User", "Activity", and "Date" but lacks explicit
column scope; update those <th> tags (the header row containing "Entity",
"User", "Activity", "Date (<abbr title=\"Coordinated Universal
Time\">UTC</abbr>)") to include scope="col" on each <th> to improve
accessibility and table semantics for assistive technologies.
In `@templates/login.php`:
- Around line 57-59: The help text with id "login-help" isn't referenced by any
form control; update the relevant login inputs (e.g., the username input element
and/or password input) to include aria-describedby="login-help" so screen
readers will announce the "Authentication is handled via LDAP" message; ensure
the referenced input(s) use the same id value "login-help" and that you only add
aria-describedby to the actual form control elements (e.g., the input with
id/name "username" and/or "password") to maintain valid accessibility semantics.
In `@templates/user.php`:
- Around line 233-235: The anchor that wraps the pending-requests badge (the
link generated with
outurl('/servers/'.urlencode($server->hostname).'#requests')) lacks an
accessible name; update that <a> to include an aria-label that describes the
link (e.g., "View pending requests for {hostname}: {pending_requests}") using
$server->hostname and $server->pending_requests so screen readers get a clear
label consistent with the fix applied around lines 76-77.
In `@views/home.php`:
- Around line 45-48: The catch block currently catches the generic Exception and
only sets a generic message via PageSection('key_upload_fail'), which hides
diagnostic details; modify the exception handling around the deletion operation
to catch a more specific exception type if possible (e.g., a custom
KeyDeletionException or relevant storage/DB exception) or at minimum log the
caught exception $e (use $e->getMessage() and optionally $e->getTraceAsString())
before setting the user-facing message in PageSection('key_upload_fail') so
developers can debug while users still see the generic "Could not delete the
selected public key." message.
In `@views/servers.php`:
- Around line 128-129: The call to RuntimeState::get('server_dir') in run_import
can return null; add a defensive null check like in default_key_scan_setting():
retrieve $server_dir, verify it's not null (and log/throw or return an
error/empty array accordingly) before calling $server_dir->add_server, and
handle the null case consistently with existing config handling to avoid calling
methods on null.
🪄 Autofix (Beta)
✅ Autofix completed
| $login_flow = new LoginFlowService($auth_service); | ||
| $login_state = $login_flow->handle_request($_SERVER['REQUEST_METHOD'], $_POST); | ||
| $error_message = $login_state['error_message']; | ||
| $success_message = $login_state['success_message']; | ||
|
|
||
| // Create the login content | ||
| $login_content = new PageSection('login'); | ||
| $login_content->set('error_message', $error_message); | ||
| $login_content->set('success_message', $success_message); | ||
| $login_content->set('csrf_token', $_SESSION['csrf_token']); | ||
| $login_content->set('csrf_token', $login_state['csrf_token']); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check LoginFlowService for audit/logging behavior
fd -t f 'login_flow' --exec cat {} \; 2>/dev/null | head -200
# Also check for any logging calls in the service
rg -n -C3 '(log|audit|event)' --iglob '*login_flow*'Repository: Msprg/ssh-key-authority
Length of output: 6175
Add audit/event logging to LoginFlowService for authentication attempts.
The service currently lacks audit/event logging for both successful and failed authentication attempts. While session-based rate limiting is present, persistent audit logging (which was previously handled inline) is missing. This is required for compliance and security event tracking.
Add logging calls to track:
- Successful authentication attempts (with username and timestamp)
- Failed authentication attempts (with username, reason, and timestamp)
- Rate-limited requests (with username and timestamp)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@views/login.php` around lines 19 - 28, Update
LoginFlowService::handle_request to emit persistent audit/event logs for every
auth attempt: inject or obtain an audit logger (e.g., AuditLogger or
EventDispatcher) in the LoginFlowService constructor and call it inside
handle_request to record (1) successful authentications with username and
timestamp, (2) failed authentications with username, failure reason and
timestamp, and (3) rate-limited requests with username and timestamp; use the
username from the incoming request or from the returned $login_state and ensure
each log call references the LoginFlowService class and handle_request so these
events are consistently recorded.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 12 file(s) based on 11 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 12 file(s) based on 11 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
services/login_flow.php (4)
22-25:⚠️ Potential issue | 🟡 MinorRun blank-field validation before username format validation.
On Line 22, empty usernames currently hit the format error branch instead of the intended “enter both” message.
Proposed fix
- if(!preg_match('/^[a-zA-Z0-9._-]+$/', $username)) { - $error_message = 'Invalid username format. Username can only contain letters, numbers, dots, hyphens, and underscores.'; - } elseif($username === '' || $password === '') { + if($username === '' || $password === '') { $error_message = 'Please enter both username and password.'; + } elseif(!preg_match('/^[a-zA-Z0-9._-]+$/', $username)) { + $error_message = 'Invalid username format. Username can only contain letters, numbers, dots, hyphens, and underscores.'; } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/login_flow.php` around lines 22 - 25, The validation currently checks username format before emptiness, causing empty usernames to trigger the format error; swap the checks so the blank-field condition runs first: in the login handling code that uses $username and $password (the if/elseif block where preg_match('/^[a-zA-Z0-9._-]+$/', $username) is used), change the order so the emptiness check (if $username === '' || $password === '') comes before the preg_match format check, leaving the error messages unchanged.
96-111:⚠️ Potential issue | 🟠 MajorRedirect sanitizer is weaker than
RequestAuthGuardand duplicates logic.On Lines 100-101, this version misses CR/LF and
user/pass/portchecks present inservices/request_auth_guard.php(Lines 46-67). This creates inconsistent redirect hardening.Use one shared sanitizer function and call it from both services.
Minimal hardening patch if keeping local implementation
private function sanitize_redirect_path($candidate) { if(!is_string($candidate) || $candidate === '') { return '/'; } + if(strpos($candidate, "\r") !== false || strpos($candidate, "\n") !== false) { + return '/'; + } $parts = parse_url($candidate); - if($parts === false || isset($parts['scheme']) || isset($parts['host'])) { + if( + $parts === false || + isset($parts['scheme']) || + isset($parts['host']) || + isset($parts['user']) || + isset($parts['pass']) || + isset($parts['port']) + ) { return '/'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/login_flow.php` around lines 96 - 111, The sanitize_redirect_path function duplicates and is weaker than the sanitizer used by RequestAuthGuard; replace the local logic by calling the shared sanitizer used in RequestAuthGuard (or extract the common logic into a shared function) and update all callers to use it, ensuring the sanitizer (1) rejects CR/LF characters, (2) rejects any URL that parse_url returns with scheme, host, user, pass, or port set, and (3) preserves safe path+query+fragment behavior; specifically modify sanitize_redirect_path (or remove it) so it delegates to the shared sanitize routine used by RequestAuthGuard rather than reimplementing the checks.
78-94:⚠️ Potential issue | 🟠 MajorSession-backed rate limiting is bypassable.
On Line 89, tracking attempts in
$_SESSIONallows brute-force reset by starting a new session/cookie context. Move counters to a server-side shared store keyed by username/IP with TTL and atomic increment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/login_flow.php` around lines 78 - 94, The rate-limiting using $_SESSION in is_rate_limited and record_failed_attempt is bypassable by resetting client cookies; replace the session-backed counters with a server-side shared store (e.g., Redis) keyed by username and/or IP and implement atomic increment + TTL semantics: in record_failed_attempt switch to an atomic INCR on key "login_attempt:{username}" (or include IP), set EXPIRE to 900s when creating the key, and in is_rate_limited read the counter and remaining TTL to determine if count >= 5 and TTL not expired; ensure all places referencing $_SESSION['login_attempts'] are updated to use the new store client and handle errors/fallbacks.
16-17:⚠️ Potential issue | 🟠 MajorGuard CSRF tokens as strings before calling
hash_equals.On Line 16,
csrf_token[]can make$post_data['csrf_token']an array and trigger aTypeErrorin PHP 8.x.Proposed fix
- if(!isset($post_data['csrf_token']) || !hash_equals($_SESSION['csrf_token'], $post_data['csrf_token'])) { + if( + !isset($_SESSION['csrf_token'], $post_data['csrf_token']) || + !is_string($_SESSION['csrf_token']) || + !is_string($post_data['csrf_token']) || + !hash_equals($_SESSION['csrf_token'], $post_data['csrf_token']) + ) {As per coding guidelines,
**/*.php: Ensure PHP 8.2+ compatibility for all PHP code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/login_flow.php` around lines 16 - 17, The CSRF comparison calls hash_equals with $post_data['csrf_token'] which may be an array and cause a TypeError on PHP 8+; update the condition around hash_equals to first ensure both $_SESSION['csrf_token'] and $post_data['csrf_token'] are strings (e.g. use is_string checks or cast with (string) / strval) and treat non-string inputs as invalid, then call hash_equals($_SESSION['csrf_token'], $post_data['csrf_token']) only when both are confirmed strings.services/request_context.php (1)
20-20:⚠️ Potential issue | 🟠 MajorStop using raw
HTTP_HOSTwhen constructing absolute URLs.Line 20 still trusts
$_SERVER['HTTP_HOST'], which is client-controlled and can poison generated absolute URLs (including redirect surfaces).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/request_context.php` at line 20, The code is using client-controlled $_SERVER['HTTP_HOST'] to build $context->absolute_request_url; replace this with a trusted host source (e.g. a configured canonical host or $_SERVER['SERVER_NAME']) and validate/sanitize it before concatenation. Update the assignment that builds $context->absolute_request_url (which currently uses $scheme.'://'.$_SERVER['HTTP_HOST'].$context->request_url) to instead pull a trusted host value (for example $trustedHost = $config->canonical_host ?? $_SERVER['SERVER_NAME']), validate it with filter_var or an allowlist, then use $scheme.'://'.$trustedHost.$context->request_url so generated absolute URLs cannot be poisoned by HTTP_HOST.
🧹 Nitpick comments (3)
public_html/style.css (1)
499-499: Minor: Stylelint prefers lowercasecurrentcolor.Stylelint's
value-keyword-caserule flagscurrentColorat line 499. While both forms are valid CSS, using lowercase would satisfy the linter.- background-color: currentColor; + background-color: currentcolor;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public_html/style.css` at line 499, Change the CSS value from the mixed-case keyword to the lowercase keyword to satisfy Stylelint: locate the background-color rule that uses "currentColor" (the background-color declaration at the reported location) and replace it with "currentcolor" so it conforms to the value-keyword-case rule.scripts/sync-failure.php (1)
19-20: Keep retry metadata and actual schedule delay in one source of truth.At Line 19 / Line 146, retry metadata uses
RESCHEDULE_DELAY_MINUTES, but scheduling inmodel/server.php(Line 701) is hardcoded to30 * 60. These can drift and misreport retry timing.Also applies to: 145-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sync-failure.php` around lines 19 - 20, The retry metadata and scheduling use two different sources for the delay; replace the hardcoded seconds value with the single constant so they cannot drift: use the existing RESCHEDULE_DELAY_MINUTES constant (convert to seconds where scheduling requires it) instead of the literal 30 * 60 in the scheduling code (refer to RESCHEDULE_DELAY_MINUTES in scripts/sync-failure.php and the scheduling call in model/server.php) or move RESCHEDULE_DELAY_MINUTES into a shared config/module that both files import and reference for computing the reschedule interval.services/response_security_headers.php (1)
93-95: Guardpreloadbehind valid HSTS prerequisites.Line 93 enables
preloadindependently. Consider enforcingincludeSubDomains(and keeping sufficientmax-age) before appending it to reduce invalid/ineffective preload configurations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/response_security_headers.php` around lines 93 - 95, The code appends '; preload' whenever $config['security']['hsts_preload'] is 1 but doesn't verify HSTS preload prerequisites; update the conditional so it only appends '; preload' when includeSubDomains is enabled and max-age is large enough (preload requires includeSubDomains and max-age >= 10886400 seconds). Concretely, change the check around $config['security']['hsts_preload'] to also verify (int)$config['security']['hsts_include_subdomains'] === 1 and (int)$config['security']['hsts_max_age'] >= 10886400 before adding '; preload' to $hsts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/sync-failure.php`:
- Around line 48-49: classify_reason() is using normalize_reason() which
currently truncates to 240 chars (so failure-identifying phrases after char 240
are lost); change the flow so classification runs on an untruncated normalized
string: either remove the 240-char truncation from normalize_reason() and apply
truncation only when persisting/displaying the reason, or introduce a new helper
(e.g., normalize_reason_for_storage()) that truncates while leaving
normalize_reason() full-length; update classify_reason() to call the
non-truncating normalizer and ensure all callers that need the 240-char limit
use the storage-specific helper instead.
In `@services/login_flow.php`:
- Around line 48-51: The catch block currently only catches Exception and can
miss PHP 8+ Errors; change the catch(Exception $e) in services/login_flow.php to
catch(Throwable $e) so audit_log('failure', $username, 'Authentication error:
'.$e->getMessage()) and $error_message assignment still run for all throwables;
update the catch clause around the code that calls audit_log and sets
$error_message (refer to the audit_log method and $username variable) to use
catch(Throwable $e) instead of catch(Exception $e).
In `@services/request_context.php`:
- Line 15: The current preg_replace strips $context->base_url even when it is
only a prefix of a larger path segment (e.g. "/app" in "/application"); update
the regex used to compute $context->relative_request_url to only match
$context->base_url at the start when it is followed by a path separator or
end-of-string. Replace the existing preg_replace call with one using a lookahead
like '/^'.preg_quote($context->base_url, '/').'(?=\/|$)/' (and keep the existing
replacement behavior), and optionally normalize the result to ensure a single
leading '/' so $context->relative_request_url is a valid relative path.
In `@services/response_security_headers.php`:
- Around line 12-14: The current apply() method silently returns when
headers_sent() is true; instead, detect headers_sent($file, $line) and emit a
visible diagnostic (use error_log or the project's logger) that includes that
headers were already sent along with $file, $line and headers_list() so failures
to apply CSP/HSTS are discoverable, then return; update the
ResponseSecurityHeaders::apply method to perform this logging before exiting
rather than silently dropping headers.
In `@templates/groups.php`:
- Around line 29-30: The div with id="list" is always rendering role="tabpanel"
which creates an orphaned tabpanel for non-admin users; change the template so
role="tabpanel" is emitted only when tabs exist (same condition used for
aria-labelledby): i.e. check $this->get('admin') and only output role="tabpanel"
(and aria-labelledby) in that branch, leaving the plain div without tab
semantics for non-admins (refer to the div with id="list" and the
$this->get('admin') conditional).
---
Duplicate comments:
In `@services/login_flow.php`:
- Around line 22-25: The validation currently checks username format before
emptiness, causing empty usernames to trigger the format error; swap the checks
so the blank-field condition runs first: in the login handling code that uses
$username and $password (the if/elseif block where
preg_match('/^[a-zA-Z0-9._-]+$/', $username) is used), change the order so the
emptiness check (if $username === '' || $password === '') comes before the
preg_match format check, leaving the error messages unchanged.
- Around line 96-111: The sanitize_redirect_path function duplicates and is
weaker than the sanitizer used by RequestAuthGuard; replace the local logic by
calling the shared sanitizer used in RequestAuthGuard (or extract the common
logic into a shared function) and update all callers to use it, ensuring the
sanitizer (1) rejects CR/LF characters, (2) rejects any URL that parse_url
returns with scheme, host, user, pass, or port set, and (3) preserves safe
path+query+fragment behavior; specifically modify sanitize_redirect_path (or
remove it) so it delegates to the shared sanitize routine used by
RequestAuthGuard rather than reimplementing the checks.
- Around line 78-94: The rate-limiting using $_SESSION in is_rate_limited and
record_failed_attempt is bypassable by resetting client cookies; replace the
session-backed counters with a server-side shared store (e.g., Redis) keyed by
username and/or IP and implement atomic increment + TTL semantics: in
record_failed_attempt switch to an atomic INCR on key "login_attempt:{username}"
(or include IP), set EXPIRE to 900s when creating the key, and in
is_rate_limited read the counter and remaining TTL to determine if count >= 5
and TTL not expired; ensure all places referencing $_SESSION['login_attempts']
are updated to use the new store client and handle errors/fallbacks.
- Around line 16-17: The CSRF comparison calls hash_equals with
$post_data['csrf_token'] which may be an array and cause a TypeError on PHP 8+;
update the condition around hash_equals to first ensure both
$_SESSION['csrf_token'] and $post_data['csrf_token'] are strings (e.g. use
is_string checks or cast with (string) / strval) and treat non-string inputs as
invalid, then call hash_equals($_SESSION['csrf_token'],
$post_data['csrf_token']) only when both are confirmed strings.
In `@services/request_context.php`:
- Line 20: The code is using client-controlled $_SERVER['HTTP_HOST'] to build
$context->absolute_request_url; replace this with a trusted host source (e.g. a
configured canonical host or $_SERVER['SERVER_NAME']) and validate/sanitize it
before concatenation. Update the assignment that builds
$context->absolute_request_url (which currently uses
$scheme.'://'.$_SERVER['HTTP_HOST'].$context->request_url) to instead pull a
trusted host value (for example $trustedHost = $config->canonical_host ??
$_SERVER['SERVER_NAME']), validate it with filter_var or an allowlist, then use
$scheme.'://'.$trustedHost.$context->request_url so generated absolute URLs
cannot be poisoned by HTTP_HOST.
---
Nitpick comments:
In `@public_html/style.css`:
- Line 499: Change the CSS value from the mixed-case keyword to the lowercase
keyword to satisfy Stylelint: locate the background-color rule that uses
"currentColor" (the background-color declaration at the reported location) and
replace it with "currentcolor" so it conforms to the value-keyword-case rule.
In `@scripts/sync-failure.php`:
- Around line 19-20: The retry metadata and scheduling use two different sources
for the delay; replace the hardcoded seconds value with the single constant so
they cannot drift: use the existing RESCHEDULE_DELAY_MINUTES constant (convert
to seconds where scheduling requires it) instead of the literal 30 * 60 in the
scheduling code (refer to RESCHEDULE_DELAY_MINUTES in scripts/sync-failure.php
and the scheduling call in model/server.php) or move RESCHEDULE_DELAY_MINUTES
into a shared config/module that both files import and reference for computing
the reschedule interval.
In `@services/response_security_headers.php`:
- Around line 93-95: The code appends '; preload' whenever
$config['security']['hsts_preload'] is 1 but doesn't verify HSTS preload
prerequisites; update the conditional so it only appends '; preload' when
includeSubDomains is enabled and max-age is large enough (preload requires
includeSubDomains and max-age >= 10886400 seconds). Concretely, change the check
around $config['security']['hsts_preload'] to also verify
(int)$config['security']['hsts_include_subdomains'] === 1 and
(int)$config['security']['hsts_max_age'] >= 10886400 before adding '; preload'
to $hsts.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d3d39cd-bf08-4066-a05e-219b850887b4
📒 Files selected for processing (13)
docs/phase-7-checkpoint.mdphpstan-baseline.neonpublic_html/extra.jspublic_html/style.cssscripts/sync-common.phpscripts/sync-failure.phpservices/login_flow.phpservices/request_context.phpservices/response_security_headers.phptemplates/groups.phptemplates/home.phptemplates/server.phptemplates/serveraccount.php
✅ Files skipped from review due to trivial changes (5)
- phpstan-baseline.neon
- templates/home.php
- docs/phase-7-checkpoint.md
- templates/serveraccount.php
- public_html/extra.js
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/sync-common.php
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 5 file(s) based on 5 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 5 file(s) based on 5 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
services/login_flow.php (3)
22-25:⚠️ Potential issue | 🟡 MinorCheck empty credentials before regex format validation.
Line 22 currently runs format validation before blank-field validation, so empty usernames get the wrong error message.
Proposed fix
- if(!preg_match('/^[a-zA-Z0-9._-]+$/', $username)) { - $error_message = 'Invalid username format. Username can only contain letters, numbers, dots, hyphens, and underscores.'; - } elseif($username === '' || $password === '') { + if($username === '' || $password === '') { $error_message = 'Please enter both username and password.'; + } elseif(!preg_match('/^[a-zA-Z0-9._-]+$/', $username)) { + $error_message = 'Invalid username format. Username can only contain letters, numbers, dots, hyphens, and underscores.'; } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/login_flow.php` around lines 22 - 25, The login flow validates username format before checking for empty fields, causing empty usernames to trigger the wrong error; update the validation order in services/login_flow.php so the blank-field check for $username and $password occurs before the preg_match() format check (and consider trimming $username with trim($username) first) — i.e., check if $username === '' || $password === '' and set $error_message accordingly, then run the preg_match('/^[a-zA-Z0-9._-]+$/', $username) validation afterward.
16-17:⚠️ Potential issue | 🟠 MajorGuard CSRF tokens as strings before calling
hash_equals.On Line 16,
csrf_token[]input can make$post_data['csrf_token']an array and trigger aTypeErrorin PHP 8.2 before auth handling.Proposed fix
- if(!isset($post_data['csrf_token']) || !hash_equals($_SESSION['csrf_token'], $post_data['csrf_token'])) { + if( + !isset($_SESSION['csrf_token'], $post_data['csrf_token']) || + !is_string($_SESSION['csrf_token']) || + !is_string($post_data['csrf_token']) || + !hash_equals($_SESSION['csrf_token'], $post_data['csrf_token']) + ) { $error_message = 'Invalid request token. Please refresh the page and try again.'; } else {As per coding guidelines:
**/*.php: Ensure PHP 8.2+ compatibility for all PHP code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/login_flow.php` around lines 16 - 17, The CSRF check calls hash_equals($_SESSION['csrf_token'], $post_data['csrf_token']) without ensuring $post_data['csrf_token'] (or the session token) is a string, which can throw a TypeError if an array is submitted; update the guard around hash_equals in the login flow to first verify both $_SESSION['csrf_token'] and $post_data['csrf_token'] are strings/scalars (e.g. is_string or is_scalar) and if not treat the request as invalid (set $error_message accordingly) instead of passing an array to hash_equals; keep the existing $error_message text and only call hash_equals when both operands are confirmed strings.
78-94:⚠️ Potential issue | 🟠 MajorRate limiting logic is bypassable and has stale-counter lockout behavior.
Two issues here:
- Session-only storage is bypassable by starting a new session/cookie jar.
- Counter/time handling can re-lock users after a single failed attempt once the window has passed (count never resets when the window expires).
Please move this to a server-side atomic store (e.g., Redis/DB keyed by username+IP with TTL) and reset counters per window semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/login_flow.php` around lines 78 - 94, The current session-based rate limiter in is_rate_limited and record_failed_attempt is bypassable and keeps a stale counter; replace these to use a server-side atomic store (e.g., Redis or DB) keyed by username+IP instead of $_SESSION['login_attempts'], implement windowed semantics so counters reset when the TTL/window expires, and use atomic operations (Redis INCR with EXPIRE or a single Lua script) to increment the failure count and set the TTL on first failure; ensure is_rate_limited queries the store for the current count+timestamp and returns true only if the count >= 5 within the active window, while record_failed_attempt performs the atomic increment and sets TTL on new keys rather than updating a session array.
🧹 Nitpick comments (1)
scripts/sync-failure.php (1)
188-196: Remove unused deprecated private method.This method is marked deprecated and is never called within the class. Private methods cannot be called externally, making this dead code. Static analysis correctly identified this as unused.
🧹 Proposed fix: remove dead code
- /** - * `@deprecated` Use normalize_reason_full() or normalize_reason_for_storage() instead - * `@param` string|null $reason - * `@return` string - */ - private static function normalize_reason($reason) { - return self::normalize_reason_for_storage($reason); - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sync-failure.php` around lines 188 - 196, Remove the dead private static method normalize_reason (including its docblock) from the class; this method simply forwards to normalize_reason_for_storage and is unused, so delete the entire normalize_reason function declaration and docblock, then run static analysis/tests to confirm no callers remain and that normalize_reason_for_storage (and normalize_reason_full if present) continue to be used where intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/login_flow.php`:
- Around line 19-20: The code sets $username and $password directly from
$post_data and calls trim() which throws a TypeError if inputs are arrays;
update the assignments for $username and $password (the variables in
services/login_flow.php) to first ensure each value is a string (e.g., use
is_string($post_data['username']) ? trim($post_data['username']) : '' and
similarly for $password, or cast safely) so trim() is only invoked on strings
and non-string/array submissions are guarded before authentication.
---
Duplicate comments:
In `@services/login_flow.php`:
- Around line 22-25: The login flow validates username format before checking
for empty fields, causing empty usernames to trigger the wrong error; update the
validation order in services/login_flow.php so the blank-field check for
$username and $password occurs before the preg_match() format check (and
consider trimming $username with trim($username) first) — i.e., check if
$username === '' || $password === '' and set $error_message accordingly, then
run the preg_match('/^[a-zA-Z0-9._-]+$/', $username) validation afterward.
- Around line 16-17: The CSRF check calls hash_equals($_SESSION['csrf_token'],
$post_data['csrf_token']) without ensuring $post_data['csrf_token'] (or the
session token) is a string, which can throw a TypeError if an array is
submitted; update the guard around hash_equals in the login flow to first verify
both $_SESSION['csrf_token'] and $post_data['csrf_token'] are strings/scalars
(e.g. is_string or is_scalar) and if not treat the request as invalid (set
$error_message accordingly) instead of passing an array to hash_equals; keep the
existing $error_message text and only call hash_equals when both operands are
confirmed strings.
- Around line 78-94: The current session-based rate limiter in is_rate_limited
and record_failed_attempt is bypassable and keeps a stale counter; replace these
to use a server-side atomic store (e.g., Redis or DB) keyed by username+IP
instead of $_SESSION['login_attempts'], implement windowed semantics so counters
reset when the TTL/window expires, and use atomic operations (Redis INCR with
EXPIRE or a single Lua script) to increment the failure count and set the TTL on
first failure; ensure is_rate_limited queries the store for the current
count+timestamp and returns true only if the count >= 5 within the active
window, while record_failed_attempt performs the atomic increment and sets TTL
on new keys rather than updating a session array.
---
Nitpick comments:
In `@scripts/sync-failure.php`:
- Around line 188-196: Remove the dead private static method normalize_reason
(including its docblock) from the class; this method simply forwards to
normalize_reason_for_storage and is unused, so delete the entire
normalize_reason function declaration and docblock, then run static
analysis/tests to confirm no callers remain and that
normalize_reason_for_storage (and normalize_reason_full if present) continue to
be used where intended.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aff52f51-86c6-4702-b83f-e0dbdae1ebcc
📒 Files selected for processing (5)
scripts/sync-failure.phpservices/login_flow.phpservices/request_context.phpservices/response_security_headers.phptemplates/groups.php
✅ Files skipped from review due to trivial changes (2)
- services/request_context.php
- services/response_security_headers.php
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/smoke/web-workflows.sh (1)
22-30: Remove unused regex variables.Static analysis correctly identifies that
LEGACY_DROPDOWN_RE,LEGACY_LAYOUT_TOKENS, andLEGACY_LAYOUT_REare defined but never used. These appear to be leftover definitions that should be removed to reduce confusion.♻️ Proposed fix to remove dead code
LEGACY_FORM_GROUP_RE='class="form-group([[:space:]]|")|class="[^"]+[[:space:]]form-group([[:space:]]|")' -LEGACY_DROPDOWN_RE='class="dropdown([[:space:]]|")|class="[^"]+[[:space:]]dropdown([[:space:]]|")|class="dropdown-toggle([[:space:]]|")|class="[^"]+[[:space:]]dropdown-toggle([[:space:]]|")' LEGACY_CARET_RE='class="caret([[:space:]]|")|class="[^"]+[[:space:]]caret([[:space:]]|")' LEGACY_IN_CLASS_RE='class="in([[:space:]]|")|class="[^"]+[[:space:]]in([[:space:]]|")' -LEGACY_LAYOUT_TOKENS='container|row|col-sm-[0-9]+|col-md-[0-9]+|text-muted|w-100|mb-0|mb-1|mb-3|mb-4|mt-3|d-none|float-end|invisible' -LEGACY_LAYOUT_RE="class=\"(${LEGACY_LAYOUT_TOKENS})([[:space:]]|\")|class=\"[^\"]+[[:space:]](${LEGACY_LAYOUT_TOKENS})([[:space:]]|\")" LEGACY_PRESENTATION_TOKENS='text-muted|text-success|text-warning|text-danger|text-info|d-xl-none|h-50px'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/smoke/web-workflows.sh` around lines 22 - 30, Remove the dead regex variables LEGACY_DROPDOWN_RE, LEGACY_LAYOUT_TOKENS, and LEGACY_LAYOUT_RE from the script: they are defined but never used (remove their declarations from scripts/smoke/web-workflows.sh) and run a quick grep for those symbols to ensure nothing else (functions or patterns) relies on them before committing; leave the remaining regexes (e.g., LEGACY_FORM_GROUP_RE, LEGACY_CARET_RE, LEGACY_IN_CLASS_RE, LEGACY_PRESENTATION_RE, RETIRED_SKA_GENERIC_RE) intact.services/login_flow.php (1)
119-130: Consider sanitizing$detailsparameter for log integrity.While
$safe_usernameis sanitized, the$detailsparameter (which can include exception messages at line 55) is logged directly. Exception messages could theoretically contain newline characters that might interfere with log parsing or enable log injection. A simple newline replacement would harden log integrity.private function audit_log($event_type, $username, $details) { $timestamp = date('Y-m-d H:i:s'); $safe_username = preg_replace('/[^a-zA-Z0-9._-]/', '', $username); + $safe_details = str_replace(["\r", "\n"], ' ', $details); $log_message = sprintf( '[LoginFlowService::handle_request] %s - Event: %s, Username: %s, Details: %s', $timestamp, $event_type, $safe_username, - $details + $safe_details ); error_log($log_message); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/login_flow.php` around lines 119 - 130, The audit_log function is currently logging $details raw which can contain newlines or control chars; update audit_log to sanitize $details (similar to $safe_username) by replacing CR/LF and other control characters with a single space or removing them (e.g., use preg_replace('/[\\r\\n\\t\\x00-\\x1F]+/', ' ', $details)) and optionally trim/limit its length before including it in $log_message so logs cannot be injected or broken by newline characters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/smoke/web-workflows.sh`:
- Around line 22-30: Remove the dead regex variables LEGACY_DROPDOWN_RE,
LEGACY_LAYOUT_TOKENS, and LEGACY_LAYOUT_RE from the script: they are defined but
never used (remove their declarations from scripts/smoke/web-workflows.sh) and
run a quick grep for those symbols to ensure nothing else (functions or
patterns) relies on them before committing; leave the remaining regexes (e.g.,
LEGACY_FORM_GROUP_RE, LEGACY_CARET_RE, LEGACY_IN_CLASS_RE,
LEGACY_PRESENTATION_RE, RETIRED_SKA_GENERIC_RE) intact.
In `@services/login_flow.php`:
- Around line 119-130: The audit_log function is currently logging $details raw
which can contain newlines or control chars; update audit_log to sanitize
$details (similar to $safe_username) by replacing CR/LF and other control
characters with a single space or removing them (e.g., use
preg_replace('/[\\r\\n\\t\\x00-\\x1F]+/', ' ', $details)) and optionally
trim/limit its length before including it in $log_message so logs cannot be
injected or broken by newline characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40e504ea-8935-4b17-98ec-62e4e4c99305
📒 Files selected for processing (4)
scripts/smoke/browser-interactions.shscripts/smoke/web-workflows.shscripts/sync-failure.phpservices/login_flow.php
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/sync-failure.php
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
views/servers.php (1)
107-118:⚠️ Potential issue | 🟠 MajorMissing runtime config now silently turns key scanning off.
Using
RuntimeState::get('config', array())makes a missing or malformed config look the same as an explicitdefault_key_supervision=off, so newly created/imported servers can lose supervision without an operator opting out. Treat missing config as an error or default it to"full"and reserve the empty-string branch for an explicitly configuredoff.Suggested fix
function default_key_scan_setting(): string { - $config = RuntimeState::get('config', array()); - $setting = ''; - if(is_array($config) && isset($config['general']) && is_array($config['general'])) { - $setting = (string)($config['general']['default_key_supervision'] ?? ''); - } + $config = RuntimeState::get('config'); + if( + !is_array($config) || + !isset($config['general']) || + !is_array($config['general']) || + !array_key_exists('default_key_supervision', $config['general']) + ) { + return "full"; + } + $setting = (string)$config['general']['default_key_supervision']; if ($setting == "") { // "off" is mapped to an empty string by the ini parser return "off";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@views/servers.php` around lines 107 - 118, The current logic using RuntimeState::get('config', array()) conflates a missing/malformed config with an explicit default_key_supervision=off; update the branching so you only treat an empty string as "off" when the config key actually exists. Specifically, using RuntimeState::get(...) and the $setting extraction around $config['general']['default_key_supervision'], change the check to detect presence of the key (e.g., isset/array_key_exists on $config['general']['default_key_supervision']) and if the key is missing or the config is not present, return "full" (or surface an error), otherwise map "" -> "off", "rootonly" -> "rootonly", and any other value -> "full".
♻️ Duplicate comments (8)
scripts/smoke/web-workflows.sh (1)
75-75:⚠️ Potential issue | 🟡 MinorPID-only key comments can still collide across containerized smoke runs.
$$only guarantees uniqueness inside one process namespace. In CI containers it is often1, so two jobs that start in the same second can still generate the same comment and then find or delete the wrong key. Add a random suffix instead of relying on PID alone.🔧 Suggested fix
-KEY_COMMENT="ska-smoke-$(date +%s)-$$" +KEY_COMMENT="ska-smoke-$(date +%s)-$$-$RANDOM"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/smoke/web-workflows.sh` at line 75, The KEY_COMMENT value uses the PID token ($$) which can collide in containerized CI; replace the PID suffix in the KEY_COMMENT assignment (the KEY_COMMENT variable) with a cryptographically or sufficiently unique random suffix (for example an openssl/uuid/mktemp-generated token or $RANDOM-based hex) so each run has a unique comment; update the KEY_COMMENT assignment to append that random token instead of $$ and ensure any downstream lookups/deletes use the updated KEY_COMMENT variable.scripts/smoke/browser-lib.sh (1)
32-34:⚠️ Potential issue | 🟠 MajorPort probing is still racing ChromeDriver startup.
browser_smoke_pick_port()closes the probe socket beforechromedriverbinds it, so another parallel job can steal that port and make the suite flaky. Start ChromeDriver with--port=0and parse the selected port from its startup log instead of pre-allocating one.Does ChromeDriver support `--port=0` to bind an ephemeral port, and where is the selected port exposed on startup?Also applies to: 74-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/smoke/browser-lib.sh` around lines 32 - 34, Replace the pre-allocated-probe pattern in browser_smoke_pick_port (and the duplicate logic at the other occurrence) by starting chromedriver with --port=0 and parsing its startup stdout/stderr to discover the ephemeral port it bound; stop opening and closing a probe socket. Update the launcher code that currently calls browser_smoke_pick_port to instead spawn chromedriver with --port=0, capture its output, wait for the startup log line that reports the chosen port, extract the numeric port via a regex, and only then proceed (keeping the chromedriver process alive). Ensure any retry/timeout logic waits for the port-discovery log entry so no other process can race to claim the port.services/request_auth_guard.php (1)
15-31:⚠️ Potential issue | 🟠 MajorStrip the query string before route matching.
relative_request_urlis matched raw here, so/login?next=/servers,/logout?reason=timeout, and public routes with query parameters stop matching the exact checks/patterns and get redirected incorrectly. Parse the path once for all route checks, and keep using the rawrequest_urlonly when storingredirect_after_login.Suggested fix
public function resolve_active_user($request_context) { - $request_path = $request_context->relative_request_url; + $request_path = parse_url($request_context->relative_request_url, PHP_URL_PATH) ?? '/'; $active_user = $this->auth_service->getCurrentUser(); @@ private function is_public_route($request_path) { + $request_path = parse_url($request_path, PHP_URL_PATH) ?? '/'; foreach($this->public_routes as $route => $is_public) { if($is_public) { $pattern = preg_quote($route, '|');Also applies to: 37-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/request_auth_guard.php` around lines 15 - 31, The route-matching logic in resolve_active_user uses request_context->relative_request_url (which includes query strings) causing mismatches; update resolve_active_user to parse and store a stripped path (e.g., $request_path_stripped) by removing the query string from request_context->relative_request_url once at the top, use that stripped path for all is_public_route($request_path), login/logout equality checks, and any other route comparisons, while continuing to use request_context->request_url (raw) when assigning $_SESSION['redirect_after_login'] and calling sanitize_redirect_path; also apply the same query-stripping approach to the duplicate checks around the block referenced at lines 37-42 (same resolve_active_user flow).services/relation_lifecycle_service.php (1)
105-123:⚠️ Potential issue | 🟠 MajorRollback is skipped when
delete_server_admin()throws.If adding the new leader succeeds and removing the old one then raises, the outer
catchreturns an error but leaves the replacement leader attached. Track$added_new_adminoutside the inner block and run the same rollback path from the exception handler.Suggested fix
foreach($servers as $server) { if(!($server instanceof Server)) { continue; } if(in_array($server->hostname, $selected_server_hostnames, true)) { + $added_new_admin = false; try { - $added_new_admin = $this->add_server_admin($server, $to_admin) === true; + $added_new_admin = $this->add_server_admin($server, $to_admin) === true; if(!$this->delete_server_admin($server, $from_admin)) { if($added_new_admin) { try { $this->delete_server_admin($server, $to_admin); } catch(Throwable $rollback_error) { error_log('reassign_server_admins_by_hostname rollback failed for '.$server->hostname.': '.$rollback_error->getMessage()); } } $message = "Server {$server->hostname}: failed to remove previous leader after reassignment"; error_log('reassign_server_admins_by_hostname: '.$message); $errors[$server->hostname] = $message; } } catch(Throwable $error) { + if($added_new_admin) { + try { + $this->delete_server_admin($server, $to_admin); + } catch(Throwable $rollback_error) { + error_log('reassign_server_admins_by_hostname rollback failed for '.$server->hostname.': '.$rollback_error->getMessage()); + } + } $message = "Server {$server->hostname}: {$error->getMessage()}"; error_log('reassign_server_admins_by_hostname: '.$message); $errors[$server->hostname] = $message; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/relation_lifecycle_service.php` around lines 105 - 123, The current try/catch only rolls back (delete_server_admin($server, $to_admin)) when delete_server_admin($server, $from_admin) returns false inside the inner block, but if delete_server_admin throws and execution jumps to the outer catch the rollback is skipped; move the $added_new_admin declaration outside the inner try, set it based on add_server_admin($server, $to_admin), and in the outer catch check if $added_new_admin is true and attempt the same rollback (call delete_server_admin($server, $to_admin) inside its own try/catch) so the replacement leader is removed on both failure paths; reference variables/functions: $added_new_admin, add_server_admin(), delete_server_admin(), $server, $to_admin in reassign_server_admins_by_hostname.views/servers_bulk_action.php (1)
77-80:⚠️ Potential issue | 🟠 MajorGuard
server_dirbefore mapping selected hostnames.If runtime state does not contain a valid
ServerDirectory, the closure dereferencesnulland this page dies with a fatal before it can render an error. Add the same runtime guard you already use inviews/servers.php.Suggested fix
$server_names = $_POST['selected_servers'] ?? []; $server_dir = RuntimeState::get('server_dir'); +if(!$server_dir instanceof ServerDirectory) { + throw new RuntimeException('Server directory service is unavailable; cannot perform bulk server actions.'); +} $selected_servers = array_map(function($name) use ($server_dir) { return $server_dir->get_server_by_hostname($name); }, $server_names);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@views/servers_bulk_action.php` around lines 77 - 80, Guard access to RuntimeState::get('server_dir') before mapping: ensure $server_dir is a valid ServerDirectory (same check used in views/servers.php) and if it is null or invalid, avoid calling $server_dir->get_server_by_hostname inside the array_map; instead set $selected_servers to an empty array or render the error path. Update the block that computes $selected_servers (which currently maps $server_names using get_server_by_hostname) to first validate $server_dir and only perform the mapping when $server_dir is present.model/entity.php (2)
81-92:⚠️ Potential issue | 🟠 MajorKeep background entity updates loggable.
This still hard-fails whenever no request-scoped
active_userexists. Sync/CLI paths callupdate()/log()without an interactive user, so status changes now throw instead of being recorded. Please treat that case as a system action and only reject non-Usernon-null values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/entity.php` around lines 81 - 92, The current check in model/entity.php hard-fails whenever $actor is null; change the final validation so null is permitted (treat as system action) and only throw when $actor is non-null and not an instance of User. Update the condition around the RuntimeException (referencing $actor and the User class) to: if($actor !== null && !($actor instanceof User)) { throw new RuntimeException(...); } so sync/CLI flows can log without an interactive user.
484-511:⚠️ Potential issue | 🟠 MajorDon't continue remote-access sync with missing runtime directories.
Lines 487-492 now silently skip group-based propagation when
group_diris unavailable, while Lines 499-511 still dereferenceserver_dirandsync_request_dirwithout guards. After a key/access mutation has already committed, either path leaves sync state inconsistent instead of failing deterministically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/entity.php` around lines 484 - 511, The code checks for missing group_dir but then proceeds to use server_dir and sync_request_dir without guards; update the User-path to similarly resolve server_dir and sync_request_dir using the same RuntimeState/$GLOBALS pattern, then if either is null log an error (including context like get_class($this) and $this->entity_id or uid) and abort the remote-access sync (return or throw) instead of dereferencing them; ensure you only call server_dir->list_servers and sync_request_dir->add_sync_request when both $server_dir and $sync_request_dir are non-null.model/user.php (1)
446-450:⚠️ Potential issue | 🟠 MajorDon't report LDAP membership sync success when
group_diris missing.Returning here makes the reconciliation path look successful while leaving LDAP-driven memberships untouched, so access can drift until someone notices the log. This should fail the sync loudly instead of silently skipping it.
Suggested fix
$group_dir = self::resolve_runtime('group_dir'); if($group_dir === null) { - error_log('Group directory service is unavailable; skipping LDAP group membership sync for user '.($this->uid ?? '(unknown)').'.'); - return; + throw new RuntimeException('Group directory service is unavailable; cannot sync LDAP group memberships.'); }As per coding guidelines: "Maintain LDAP/AD integration compatibility when modifying authentication or user/group handling".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/user.php` around lines 446 - 450, The code currently returns early when self::resolve_runtime('group_dir') is null, silently skipping LDAP group sync; change that so the failure is surfaced instead: replace the silent return in the block that checks $group_dir = self::resolve_runtime('group_dir') with a loud failure (e.g. throw a RuntimeException or return a failing status value) and include a clear error_log message that contains $this->uid; ensure the calling code that invokes the method (the reconciliation path) will observe the exception/false return so the sync is marked as failed rather than successful.
🧹 Nitpick comments (4)
docs/bootstrap5-gap-analysis.md (1)
34-35: Consider tightening repetitive phrasing for readability.This section reads a bit repetitive (“Bootstrap ...” starts) and uses vague quantifiers (“a small number of”). A light wording pass would improve scanability.
Suggested wording tweak
-- Bootstrap 3 CSS is no longer loaded. -- Bootstrap 3 JS is no longer loaded. +- Legacy Bootstrap 3 CSS is no longer loaded. +- Legacy Bootstrap 3 JS is no longer loaded. ... -- The remaining `ska-*` runtime surface is app-specific: shell layout, navigation treatment, icon system, badge/status treatment, settings-shell wrappers, card-stack wrappers, and a small number of special admin layouts. +- The remaining `ska-*` runtime surface is app-specific: shell layout, navigation treatment, icon system, badge/status treatment, settings-shell wrappers, card-stack wrappers, and a few special admin layouts. ... -- Bootstrap 3 `panel-*` markup has been migrated either to Bootstrap 5 `card`/`accordion` markup or to a small number of app-specific `ska-card*` wrappers where the layout is domain-specific. +- Bootstrap 3 `panel-*` markup has been migrated either to Bootstrap 5 `card`/`accordion` markup or to a few app-specific `ska-card*` wrappers where the layout is domain-specific.Also applies to: 40-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/bootstrap5-gap-analysis.md` around lines 34 - 35, The wording is repetitive and vague in lines that start with "Bootstrap 3..." (e.g., the two lines "Bootstrap 3 CSS is no longer loaded." and "Bootstrap 3 JS is no longer loaded.") and the nearby block at 40-42; please tighten phrasing by combining related items and replacing vague quantifiers like "a small number of" with specific counts or clearer terms, e.g., merge CSS/JS notes into one concise sentence and clarify how many affected components or pages are impacted.templates/server.php (2)
568-570: Icon accessibility could be more consistent.Line 568's icon relies solely on
titlefor semantics, while similar patterns at lines 591 and 629 include<span class="visually-hidden">text. Thetitleattribute isn't reliably announced by screen readers.For consistency with the accessible patterns used elsewhere in this file, consider adding a visually-hidden span or
aria-labelto icon-only input groups, or addaria-hidden="true"if the icon is purely decorative and the adjacent placeholder/label provides sufficient context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/server.php` around lines 568 - 570, The server account icon (span with class "ska-icon ska-icon-serveraccount") next to the input with id "account_name" relies only on title and should be updated for consistent accessibility; either mark the decorative icon aria-hidden="true" and ensure the input label/placeholder conveys the name, or add a visually hidden label span (e.g. <span class="visually-hidden">Server account</span>) immediately after the icon so screen readers announce it—locate the span with class "ska-icon ska-icon-serveraccount" adjacent to input id "account_name" and apply one of these changes to match the pattern used elsewhere in the file.
303-306: Consider standardizing form-check markup for consistency.The file uses multiple form-check patterns interchangeably. Lines 303-336 use
<span class="form-check-label">inside a wrapping<label>, while lines 512-529 use<label class="form-check-label">directly wrapping the input. Both work but standard Bootstrap 5 recommends sibling elements with explicitforattributes.The current approach is functional—just noting the inconsistency as a potential future cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/server.php` around lines 303 - 306, Standardize the radio markup for the key_management group: replace the current pattern where the input is wrapped by a <label> and uses an inner <span class="form-check-label"> (seen on the radios with name="key_management" and values "keys","none","other","decommissioned") with the Bootstrap-recommended structure: a container element with class "form-check" containing an <input class="form-check-input" id="..." name="key_management" value="..."> and a separate <label class="form-check-label" for="...">...</label>; ensure each radio gets a unique id, remove the inner span usage, and apply the existing contextual classes (e.g., "ska-choice", "ska-text-success", etc.) to the container or label as appropriate so all radio groups use the same sibling input+label pattern.views/serveraccount.php (1)
122-149: Please smoke the server-account public-key path after this extraction.These handlers now route add/delete through
KeyLifecycleService, so I’d want one end-to-end add/remove key check plus a sync preview before merge to catch wiring regressions in the SSH distribution path.Based on learnings, "For changes, prefer targeted manual verification: Login and LDAP auth flow, Add/Remove public keys, Create/Remove access rules, Trigger a sync (CLI) and verify output files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@views/serveraccount.php` around lines 122 - 149, The extracted handlers now call KeyLifecycleService (add_server_account_public_key and delete_entity_public_key) but may have wiring regressions; manually smoke-test the server-account public-key path: using the UI or HTTP POST flows that hit the handlers in views/serveraccount.php, perform an end-to-end add (POST add_public_key) and delete (POST delete_public_key) of a public key as a normal account_admin and as a server_admin, then run a sync/preview via the CLI sync command and verify the resulting SSH distribution files on the target server (authorized_keys, ACLs) reflect the changes and that login/LDAP auth still succeeds. Ensure errors surface correctly and that KeyLifecycleService methods are invoked for both add_server_account_public_key and delete_entity_public_key during these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 82: The link in AGENTS.md currently points to an absolute filesystem path
(/var/www/ska/docs/operations-runbook.md) which is broken in GitHub and editors;
update the Markdown link text to use a repo-relative path
(docs/operations-runbook.md) so the link resolves correctly in the repository
and editors, leaving the surrounding sentence unchanged.
In `@model/user.php`:
- Around line 244-249: The code currently forces presence of web.baseurl by
calling require_config_section('web', ['baseurl']) and computing $url even
though $url is no longer used after refactor; remove that hard prerequisite by
deleting the require_config_section call and the $url computation (or only fetch
web.baseurl lazily when actually needed, e.g. inside the notification/sending
logic). Update add_public_key flow (the parent::add_public_key($key) usage) to
only require the email config via require_config_section('email', ...) and avoid
calling resolve_runtime('web')/require_config_section('web') unless the code
later references $web_config['baseurl'].
In `@public_html/style.css`:
- Around line 547-551: The stylelint error is due to missing/incorrect
empty-line spacing before the declaration in the .ska-icon rule; open the
.ska-icon block and add a blank line before the --ska-icon-image declaration (or
rearrange so the first declaration follows your configured
declaration-empty-line-before rule), ensuring the custom property
--ska-icon-image is preceded by the required empty line to satisfy
declaration-empty-line-before.
- Line 512: The CSS value "currentColor" on the background-color declaration
violates the value-keyword-case rule; update the background-color property (the
statement using background-color: currentColor) to use the lowercase keyword
"currentcolor" so it matches the configured stylelint rule.
In `@scripts/smoke/browser-interactions.sh`:
- Around line 59-121: The tests fail because they assert aria-hidden (which
Bootstrap doesn't auto-set) and only use href-based targets; update the collapse
and tab checks: in the browser_smoke_exec blocks that click the collapse trigger
(look for the query selector using
"a[href=\"#getting_started\"][data-bs-toggle=\"collapse\"]" and the subsequent
trigger.click logic) broaden the selector to accept elements with data-bs-target
or href (e.g. selector matching
[data-bs-toggle="collapse"][data-bs-target="#getting_started"],
[data-bs-toggle="collapse"][href="#getting_started"]), and in the corresponding
browser_smoke_wait_for_js checks remove any panel.getAttribute("aria-hidden")
comparisons and instead only assert panel.classList.contains("show") and
trigger.getAttribute("aria-expanded") === "true" (and the inverse for closing).
For tabs, where TAB_RESULT is built by the browser_smoke_exec that returns
tab.id + "|" + tab.getAttribute("href"), change that logic to resolve the target
as (tab.getAttribute("data-bs-target") || tab.getAttribute("href")) and then in
the browser_smoke_wait_for_js remove the pane.getAttribute("aria-hidden")
assertion and only assert pane.classList.contains("active") &&
pane.classList.contains("show") along with tab.classList.contains("active") and
tab.getAttribute("aria-selected")==="true"; keep using TAB_ID/TAB_TARGET
variables as-is.
In `@scripts/smoke/browser-lib.sh`:
- Around line 3-9: The prereq function browser_smoke_require_prereqs currently
checks for curl, php, chromedriver, and chromium but omits base64; update
browser_smoke_require_prereqs to also call smoke_require_cmd for base64 so the
script verifies the base64 binary before browser_smoke_capture() invokes `base64
-d`, ensuring a clear prerequisite error instead of a late "command not found"
failure.
In `@scripts/smoke/web-workflows.sh`:
- Around line 29-32: The legacy matchers currently flag valid Bootstrap 5.3
utilities; update LEGACY_PRESENTATION_TOKENS and LEGACY_SEMANTIC_TOKENS (and
their derived LEGACY_PRESENTATION_RE and LEGACY_SEMANTIC_RE) to only include
classes actually removed/renamed in the migration (remove tokens like
text-success, text-warning, text-danger, text-info, text-center, rounded,
img-fluid from those variables), then rebuild the regex strings
(LEGACY_PRESENTATION_RE and LEGACY_SEMANTIC_RE) from the narrowed token lists so
the grep -Eq checks no longer reject correct Bootstrap 5.3 classes.
In `@scripts/sync-common.php`:
- Around line 94-105: SyncProcess::__construct currently assumes
SyncRuntime::build_timeout_wrapped_command cannot throw, so a thrown exception
escapes the constructor and bypasses the existing spawn-failed handling; wrap
the call to SyncRuntime::build_timeout_wrapped_command(...) in a try/catch
(catch Exception or Throwable) inside SyncProcess::__construct, and in the catch
set $this->spawn_failed = true, $this->finished = true, $this->errors to include
the exception message, $this->exit_status = 1, clear $this->handle and
$this->pipes the same way the proc_open failure branch does, then return so the
sync item is marked failed/rescheduled.
In `@services/login_flow.php`:
- Around line 54-56: The error_log call in the catch(Throwable $e) inside
LoginFlowService::handle_request currently concatenates $e (stringifying the
Throwable) which can expose sensitive argument values; update the error logging
to avoid converting the exception to string and instead log only the exception
class and a sanitized message (use get_class($e) and $e->getMessage()), keep the
username sanitization via preg_replace and do not append the full exception
object or stack trace to the log; adjust the error_log invocation accordingly
and leave audit_log('failure', ...) as-is.
In `@services/response_security_headers.php`:
- Around line 116-119: The X-Forwarded-Proto check treats the header as a single
token and can misclassify when multiple comma-separated values exist; update the
branch that reads $_SERVER['HTTP_X_FORWARDED_PROTO'] so it parses the header and
extracts the first token (trimmed and lowercased) before comparing to 'https',
then keep the existing trust check via
self::is_request_from_trusted_proxy(self::get_runtime_config()) and return true
only when that first token === 'https' and the proxy is trusted; locate this
logic around the code that sets $proto and the surrounding conditional to
implement the parsing fix.
In `@templates/access_options.php`:
- Around line 67-79: The <div class="card-body"> is rendered even when $mode !==
'create', leaving it outside any .card; update templates/access_options.php so
card-body is only output inside the same if($mode == 'create') block that
renders the <div class="card"> and its header (or alternatively, render a
matching <div class="card"> wrapper when $mode !== 'create'); modify the
conditional around the collapse block and <div class="card-body"> (symbols:
$mode, <div class="card">, <div id="advanced_options" class="collapse">, and
<div class="card-body">) so that card-body always has a parent .card in both
branches.
In `@views/pubkeys.php`:
- Around line 27-54: Trim and validate config-derived email and URL fields
before using them: normalize $report_address = trim($report_address) and
$report_name = trim($report_name) (or default if empty) and only call
$email->add_recipient(...) and $email->send() when the trimmed $report_address
!== ''; likewise normalize $base_url = trim($base_url) before building
$allowed_keys_url and only build allowed_keys_url when trimmed $base_url !== ''
(use rtrim on the trimmed base URL when constructing the link); update
references in this block (Email, add_recipient, send, allowed_keys_url, rtrim)
so whitespace-only values are treated as empty.
---
Outside diff comments:
In `@views/servers.php`:
- Around line 107-118: The current logic using RuntimeState::get('config',
array()) conflates a missing/malformed config with an explicit
default_key_supervision=off; update the branching so you only treat an empty
string as "off" when the config key actually exists. Specifically, using
RuntimeState::get(...) and the $setting extraction around
$config['general']['default_key_supervision'], change the check to detect
presence of the key (e.g., isset/array_key_exists on
$config['general']['default_key_supervision']) and if the key is missing or the
config is not present, return "full" (or surface an error), otherwise map "" ->
"off", "rootonly" -> "rootonly", and any other value -> "full".
---
Duplicate comments:
In `@model/entity.php`:
- Around line 81-92: The current check in model/entity.php hard-fails whenever
$actor is null; change the final validation so null is permitted (treat as
system action) and only throw when $actor is non-null and not an instance of
User. Update the condition around the RuntimeException (referencing $actor and
the User class) to: if($actor !== null && !($actor instanceof User)) { throw new
RuntimeException(...); } so sync/CLI flows can log without an interactive user.
- Around line 484-511: The code checks for missing group_dir but then proceeds
to use server_dir and sync_request_dir without guards; update the User-path to
similarly resolve server_dir and sync_request_dir using the same
RuntimeState/$GLOBALS pattern, then if either is null log an error (including
context like get_class($this) and $this->entity_id or uid) and abort the
remote-access sync (return or throw) instead of dereferencing them; ensure you
only call server_dir->list_servers and sync_request_dir->add_sync_request when
both $server_dir and $sync_request_dir are non-null.
In `@model/user.php`:
- Around line 446-450: The code currently returns early when
self::resolve_runtime('group_dir') is null, silently skipping LDAP group sync;
change that so the failure is surfaced instead: replace the silent return in the
block that checks $group_dir = self::resolve_runtime('group_dir') with a loud
failure (e.g. throw a RuntimeException or return a failing status value) and
include a clear error_log message that contains $this->uid; ensure the calling
code that invokes the method (the reconciliation path) will observe the
exception/false return so the sync is marked as failed rather than successful.
In `@scripts/smoke/browser-lib.sh`:
- Around line 32-34: Replace the pre-allocated-probe pattern in
browser_smoke_pick_port (and the duplicate logic at the other occurrence) by
starting chromedriver with --port=0 and parsing its startup stdout/stderr to
discover the ephemeral port it bound; stop opening and closing a probe socket.
Update the launcher code that currently calls browser_smoke_pick_port to instead
spawn chromedriver with --port=0, capture its output, wait for the startup log
line that reports the chosen port, extract the numeric port via a regex, and
only then proceed (keeping the chromedriver process alive). Ensure any
retry/timeout logic waits for the port-discovery log entry so no other process
can race to claim the port.
In `@scripts/smoke/web-workflows.sh`:
- Line 75: The KEY_COMMENT value uses the PID token ($$) which can collide in
containerized CI; replace the PID suffix in the KEY_COMMENT assignment (the
KEY_COMMENT variable) with a cryptographically or sufficiently unique random
suffix (for example an openssl/uuid/mktemp-generated token or $RANDOM-based hex)
so each run has a unique comment; update the KEY_COMMENT assignment to append
that random token instead of $$ and ensure any downstream lookups/deletes use
the updated KEY_COMMENT variable.
In `@services/relation_lifecycle_service.php`:
- Around line 105-123: The current try/catch only rolls back
(delete_server_admin($server, $to_admin)) when delete_server_admin($server,
$from_admin) returns false inside the inner block, but if delete_server_admin
throws and execution jumps to the outer catch the rollback is skipped; move the
$added_new_admin declaration outside the inner try, set it based on
add_server_admin($server, $to_admin), and in the outer catch check if
$added_new_admin is true and attempt the same rollback (call
delete_server_admin($server, $to_admin) inside its own try/catch) so the
replacement leader is removed on both failure paths; reference
variables/functions: $added_new_admin, add_server_admin(),
delete_server_admin(), $server, $to_admin in reassign_server_admins_by_hostname.
In `@services/request_auth_guard.php`:
- Around line 15-31: The route-matching logic in resolve_active_user uses
request_context->relative_request_url (which includes query strings) causing
mismatches; update resolve_active_user to parse and store a stripped path (e.g.,
$request_path_stripped) by removing the query string from
request_context->relative_request_url once at the top, use that stripped path
for all is_public_route($request_path), login/logout equality checks, and any
other route comparisons, while continuing to use request_context->request_url
(raw) when assigning $_SESSION['redirect_after_login'] and calling
sanitize_redirect_path; also apply the same query-stripping approach to the
duplicate checks around the block referenced at lines 37-42 (same
resolve_active_user flow).
In `@views/servers_bulk_action.php`:
- Around line 77-80: Guard access to RuntimeState::get('server_dir') before
mapping: ensure $server_dir is a valid ServerDirectory (same check used in
views/servers.php) and if it is null or invalid, avoid calling
$server_dir->get_server_by_hostname inside the array_map; instead set
$selected_servers to an empty array or render the error path. Update the block
that computes $selected_servers (which currently maps $server_names using
get_server_by_hostname) to first validate $server_dir and only perform the
mapping when $server_dir is present.
---
Nitpick comments:
In `@docs/bootstrap5-gap-analysis.md`:
- Around line 34-35: The wording is repetitive and vague in lines that start
with "Bootstrap 3..." (e.g., the two lines "Bootstrap 3 CSS is no longer
loaded." and "Bootstrap 3 JS is no longer loaded.") and the nearby block at
40-42; please tighten phrasing by combining related items and replacing vague
quantifiers like "a small number of" with specific counts or clearer terms,
e.g., merge CSS/JS notes into one concise sentence and clarify how many affected
components or pages are impacted.
In `@templates/server.php`:
- Around line 568-570: The server account icon (span with class "ska-icon
ska-icon-serveraccount") next to the input with id "account_name" relies only on
title and should be updated for consistent accessibility; either mark the
decorative icon aria-hidden="true" and ensure the input label/placeholder
conveys the name, or add a visually hidden label span (e.g. <span
class="visually-hidden">Server account</span>) immediately after the icon so
screen readers announce it—locate the span with class "ska-icon
ska-icon-serveraccount" adjacent to input id "account_name" and apply one of
these changes to match the pattern used elsewhere in the file.
- Around line 303-306: Standardize the radio markup for the key_management
group: replace the current pattern where the input is wrapped by a <label> and
uses an inner <span class="form-check-label"> (seen on the radios with
name="key_management" and values "keys","none","other","decommissioned") with
the Bootstrap-recommended structure: a container element with class "form-check"
containing an <input class="form-check-input" id="..." name="key_management"
value="..."> and a separate <label class="form-check-label"
for="...">...</label>; ensure each radio gets a unique id, remove the inner span
usage, and apply the existing contextual classes (e.g., "ska-choice",
"ska-text-success", etc.) to the container or label as appropriate so all radio
groups use the same sibling input+label pattern.
In `@views/serveraccount.php`:
- Around line 122-149: The extracted handlers now call KeyLifecycleService
(add_server_account_public_key and delete_entity_public_key) but may have wiring
regressions; manually smoke-test the server-account public-key path: using the
UI or HTTP POST flows that hit the handlers in views/serveraccount.php, perform
an end-to-end add (POST add_public_key) and delete (POST delete_public_key) of a
public key as a normal account_admin and as a server_admin, then run a
sync/preview via the CLI sync command and verify the resulting SSH distribution
files on the target server (authorized_keys, ACLs) reflect the changes and that
login/LDAP auth still succeeds. Ensure errors surface correctly and that
KeyLifecycleService methods are invoked for both add_server_account_public_key
and delete_entity_public_key during these tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 582a9e71-e363-4ac1-820c-a22f41d08fcc
📒 Files selected for processing (50)
AGENTS.mddocs/bootstrap5-gap-analysis.mddocs/modernization-plan.mddocs/modernization-risks.mddocs/phase-5-checkpoint.mddocs/phase-8-checkpoint.mdmodel/dbdirectory.phpmodel/entity.phpmodel/group.phpmodel/publickey.phpmodel/record.phpmodel/user.phppublic_html/extra.jspublic_html/style.cssrequesthandler.phpscripts/smoke/browser-interactions.shscripts/smoke/browser-lib.shscripts/smoke/web-workflows.shscripts/ssh.phpscripts/sync-common.phpscripts/sync-failure.phpscripts/sync-runtime.phpservices/access_rule_service.phpservices/key_lifecycle_service.phpservices/login_flow.phpservices/relation_lifecycle_service.phpservices/request_auth_guard.phpservices/request_context.phpservices/request_csrf_guard.phpservices/request_exception_handler.phpservices/request_policy_guard.phpservices/request_router_dispatcher.phpservices/response_security_headers.phptemplates/access_options.phptemplates/activity.phptemplates/bulk_mail.phptemplates/functions.phptemplates/group.phptemplates/help.phptemplates/home.phptemplates/server.phptemplates/servers.phptemplates/servers_bulk_action.phpviews/group.phpviews/home.phpviews/pubkeys.phpviews/serveraccount.phpviews/servers.phpviews/servers_bulk_action.phpviews/user.php
✅ Files skipped from review due to trivial changes (6)
- templates/servers_bulk_action.php
- docs/modernization-risks.md
- docs/phase-8-checkpoint.md
- docs/phase-5-checkpoint.md
- templates/bulk_mail.php
- services/access_rule_service.php
🚧 Files skipped from review as they are similar to previous changes (15)
- templates/activity.php
- services/request_csrf_guard.php
- templates/functions.php
- services/request_policy_guard.php
- templates/help.php
- services/request_router_dispatcher.php
- model/record.php
- templates/home.php
- views/group.php
- model/publickey.php
- services/request_context.php
- templates/servers.php
- templates/group.php
- scripts/ssh.php
- public_html/extra.js
|
|
||
| Security note: | ||
| - Do not use `COMPOSER_ALLOW_SUPERUSER=1` for local/dev workflows. | ||
| - If a CI runner must execute Composer as root, follow the guidance in [docs/operations-runbook.md](/var/www/ska/docs/operations-runbook.md) and keep that exception scoped to the CI environment. |
There was a problem hiding this comment.
Use a repo-relative link for the runbook.
/var/www/ska/docs/operations-runbook.md is a local filesystem path, so the link is broken in GitHub and most editors. Point this at docs/operations-runbook.md instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` at line 82, The link in AGENTS.md currently points to an absolute
filesystem path (/var/www/ska/docs/operations-runbook.md) which is broken in
GitHub and editors; update the Markdown link text to use a repo-relative path
(docs/operations-runbook.md) so the link resolves correctly in the repository
and editors, leaving the surrounding sentence unchanged.
| $active_user = self::resolve_runtime('active_user'); | ||
| $config = self::resolve_runtime('config'); | ||
| $web_config = self::require_config_section($config, 'web', array('baseurl'), 'send public-key notifications'); | ||
| $email_config = self::require_config_section($config, 'email', array('admin_address', 'admin_name', 'report_address', 'report_name'), 'send public-key notifications'); | ||
| parent::add_public_key($key); | ||
| $url = $config['web']['baseurl'].'/pubkeys/'.urlencode($key->id); | ||
| $url = $web_config['baseurl'].'/pubkeys/'.urlencode($key->id); |
There was a problem hiding this comment.
Don't make web.baseurl a hard prerequisite for key uploads unless the mail still uses it.
Lines 246-249 now fail add_public_key() when web.baseurl is absent, but the only consumer is $url, which is never used after the refactor. That turns a dead config value into a runtime blocker for key uploads.
Suggested fix
- $web_config = self::require_config_section($config, 'web', array('baseurl'), 'send public-key notifications');
$email_config = self::require_config_section($config, 'email', array('admin_address', 'admin_name', 'report_address', 'report_name'), 'send public-key notifications');
parent::add_public_key($key);
- $url = $web_config['baseurl'].'/pubkeys/'.urlencode($key->id);
$email = new Email;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $active_user = self::resolve_runtime('active_user'); | |
| $config = self::resolve_runtime('config'); | |
| $web_config = self::require_config_section($config, 'web', array('baseurl'), 'send public-key notifications'); | |
| $email_config = self::require_config_section($config, 'email', array('admin_address', 'admin_name', 'report_address', 'report_name'), 'send public-key notifications'); | |
| parent::add_public_key($key); | |
| $url = $config['web']['baseurl'].'/pubkeys/'.urlencode($key->id); | |
| $url = $web_config['baseurl'].'/pubkeys/'.urlencode($key->id); | |
| $active_user = self::resolve_runtime('active_user'); | |
| $config = self::resolve_runtime('config'); | |
| $email_config = self::require_config_section($config, 'email', array('admin_address', 'admin_name', 'report_address', 'report_name'), 'send public-key notifications'); | |
| parent::add_public_key($key); |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 249-249: Avoid unused local variables such as '$url'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/user.php` around lines 244 - 249, The code currently forces presence of
web.baseurl by calling require_config_section('web', ['baseurl']) and computing
$url even though $url is no longer used after refactor; remove that hard
prerequisite by deleting the require_config_section call and the $url
computation (or only fetch web.baseurl lazily when actually needed, e.g. inside
the notification/sending logic). Update add_public_key flow (the
parent::add_public_key($key) usage) to only require the email config via
require_config_section('email', ...) and avoid calling
resolve_runtime('web')/require_config_section('web') unless the code later
references $web_config['baseurl'].
| mask-repeat: no-repeat; | ||
| -webkit-mask-size: contain; | ||
| mask-size: contain; | ||
| background-color: currentColor; |
There was a problem hiding this comment.
Fix Stylelint keyword-case at Line 512.
currentColor violates the configured value-keyword-case rule and will keep lint red.
🛠️ Suggested fix
- background-color: currentColor;
+ background-color: currentcolor;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| background-color: currentColor; | |
| background-color: currentcolor; |
🧰 Tools
🪛 Stylelint (17.6.0)
[error] 512-512: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public_html/style.css` at line 512, The CSS value "currentColor" on the
background-color declaration violates the value-keyword-case rule; update the
background-color property (the statement using background-color: currentColor)
to use the lowercase keyword "currentcolor" so it matches the configured
stylelint rule.
| .ska-icon { | ||
| --ska-icon-image: none; | ||
| position: relative; | ||
| top: auto; | ||
| display: inline-block; |
There was a problem hiding this comment.
Fix Stylelint declaration spacing around Line 549.
The declaration-empty-line-before rule is still failing in .ska-icon.
🛠️ Suggested fix
.ska-icon {
--ska-icon-image: none;
+
position: relative;
top: auto;
display: inline-block;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .ska-icon { | |
| --ska-icon-image: none; | |
| position: relative; | |
| top: auto; | |
| display: inline-block; | |
| .ska-icon { | |
| --ska-icon-image: none; | |
| position: relative; | |
| top: auto; | |
| display: inline-block; |
🧰 Tools
🪛 Stylelint (17.6.0)
[error] 549-549: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public_html/style.css` around lines 547 - 551, The stylelint error is due to
missing/incorrect empty-line spacing before the declaration in the .ska-icon
rule; open the .ska-icon block and add a blank line before the --ska-icon-image
declaration (or rearrange so the first declaration follows your configured
declaration-empty-line-before rule), ensuring the custom property
--ska-icon-image is preceded by the required empty line to satisfy
declaration-empty-line-before.
| browser_smoke_exec ' | ||
| var trigger = document.querySelector("a[href=\"#getting_started\"][data-bs-toggle=\"collapse\"]"); | ||
| if (!trigger) { | ||
| return false; | ||
| } | ||
| trigger.click(); | ||
| return true; | ||
| ' >/dev/null | ||
| browser_smoke_wait_for_js ' | ||
| var trigger = document.querySelector("a[href=\"#getting_started\"][data-bs-toggle=\"collapse\"]"); | ||
| var panel = document.getElementById("getting_started"); | ||
| return !!trigger && !!panel && | ||
| panel.classList.contains("show") && | ||
| panel.getAttribute("aria-hidden") === "false" && | ||
| trigger.getAttribute("aria-expanded") === "true"; | ||
| ' 50 0.1 || smoke_die "Help collapse did not open" | ||
| browser_smoke_exec ' | ||
| var trigger = document.querySelector("a[href=\"#getting_started\"][data-bs-toggle=\"collapse\"]"); | ||
| if (!trigger) { | ||
| return false; | ||
| } | ||
| trigger.click(); | ||
| return true; | ||
| ' >/dev/null | ||
| browser_smoke_wait_for_js ' | ||
| var trigger = document.querySelector("a[href=\"#getting_started\"][data-bs-toggle=\"collapse\"]"); | ||
| var panel = document.getElementById("getting_started"); | ||
| return !!trigger && !!panel && | ||
| !panel.classList.contains("show") && | ||
| panel.getAttribute("aria-hidden") === "true" && | ||
| trigger.getAttribute("aria-expanded") === "false"; | ||
| ' 50 0.1 || smoke_die "Help collapse did not close" | ||
|
|
||
| smoke_log "Checking server list tab interaction" | ||
| browser_smoke_navigate "${BASE_URL}/servers" | ||
| TAB_RESULT=$(browser_smoke_exec ' | ||
| var tab = document.querySelector(".nav-link[data-bs-toggle=\"tab\"]:not(.active)"); | ||
| if (!tab) { | ||
| return ""; | ||
| } | ||
| if (!tab.id) { | ||
| return ""; | ||
| } | ||
| tab.click(); | ||
| return tab.id + "|" + tab.getAttribute("href"); | ||
| ') | ||
| [ -n "$TAB_RESULT" ] || smoke_die "No inactive server-list tab was available to click" | ||
| TAB_ID=${TAB_RESULT%%|*} | ||
| TAB_TARGET=${TAB_RESULT#*|} | ||
| [ -n "$TAB_ID" ] || smoke_die "No inactive server-list tab with an id was available to click" | ||
| [ -n "$TAB_TARGET" ] || smoke_die "No inactive server-list tab target was available to click" | ||
| browser_smoke_wait_for_js ' | ||
| var tabId = arguments[0]; | ||
| var target = arguments[1]; | ||
| var tab = document.getElementById(tabId); | ||
| var pane = document.querySelector(target); | ||
| return !!tab && !!pane && | ||
| tab.classList.contains("active") && | ||
| tab.getAttribute("aria-selected") === "true" && | ||
| pane.classList.contains("active") && | ||
| pane.classList.contains("show") && | ||
| pane.getAttribute("aria-hidden") === "false"; | ||
| ' 50 0.1 "$TAB_ID" "$TAB_TARGET" || smoke_die "Server-list tab did not activate" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Bootstrap 5, for collapse and tabs, what trigger markup is supported (hrefvsdata-bs-target) and which ARIA attributes are updated automatically on the trigger versus the target panel?
💡 Result:
Collapse (Bootstrap 5)
Supported trigger markup
<a ... href="#collapseId" data-bs-toggle="collapse">(useshref) (getbootstrap.com)<button ... data-bs-target="#collapseId" data-bs-toggle="collapse">(usesdata-bs-target) (getbootstrap.com)
ARIA that Bootstrap updates automatically
- On the trigger (control):
aria-expandedis automatically toggled by the collapse plugin. (getbootstrap.com)
- On the trigger (control), but not auto-managed beyond what you author:
aria-controlsis recommended/expected in your markup (when targeting a single element), but Bootstrap does not “toggle” it—it’s just an ID reference you provide. (getbootstrap.com)
- On the target panel (
.collapseelement):- Bootstrap primarily toggles classes (
collapse,collapsing,show); it does not automatically manage ARIA attributes on the panel in the docs’ described behavior. (getbootstrap.com)
- Bootstrap primarily toggles classes (
Tabs / Pills / List (Bootstrap 5 “Tab” plugin)
Supported trigger markup (data-bs-target vs href)
Bootstrap’s selector resolution supports either:
data-bs-targetorhref(typical for<a>-based tabs)
to find the associated panel. (app.unpkg.com)
ARIA that Bootstrap updates automatically
- On the trigger (the tab itself, i.e., element with
role="tab"):aria-selectedis set totrueon activation andfalseon deactivation. (github.com)tabindexis removed for the active tab and set to-1for inactive tabs. (github.com)
- On the target panel (tab pane):
- On initialization, Bootstrap will add (if missing):
role="tabpanel"aria-labelledby="triggerId"(when the trigger has anid) (github.com)
- Visibility state is controlled via class changes (e.g.,
active,show,fade); Bootstrap does not toggle an ARIA “expanded/hidden” state on the panel as part of the core tab show/hide logic shown in the source/docs. (getbootstrap.com)
- On initialization, Bootstrap will add (if missing):
- Dropdown wrapper case (nav item is a dropdown):
aria-expandedis toggled on the dropdown outer element when the active tab is inside a dropdown. (github.com)
Citations:
- 1: https://getbootstrap.com/docs/5.0/components/collapse/
- 2: https://getbootstrap.com/docs/5.0/components/collapse/
- 3: https://getbootstrap.com/docs/5.0/components/collapse/
- 4: https://getbootstrap.com/docs/5.0/components/collapse/
- 5: https://getbootstrap.com/docs/5.0/components/collapse/
- 6: https://app.unpkg.com/bootstrap%405.2.2/files/dist/js/bootstrap.min.js.map?utm_source=openai
- 7: https://github.com/twbs/bootstrap/blob/v5.3.3/js/src/tab.js
- 8: https://github.com/twbs/bootstrap/blob/v5.3.3/js/src/tab.js
- 9: https://github.com/twbs/bootstrap/blob/v5.3.3/js/src/tab.js
- 10: https://getbootstrap.com/docs/5.3/components/navs-tabs/
- 11: https://github.com/twbs/bootstrap/blob/v5.3.3/js/src/tab.js
The collapse/tab assertions require aria-hidden changes that Bootstrap does not auto-manage.
The code at lines 59-121 checks aria-hidden on collapse panels and tab panes:
- Collapse (lines 68–70, 89–91): Asserts
panel.getAttribute("aria-hidden") === "false"or"true", but Bootstrap toggles theshowclass and manages onlyaria-expandedon the trigger—notaria-hiddenon the panel. - Tabs (lines 110–112): Asserts
pane.getAttribute("aria-hidden") === "false", but Bootstrap controls visibility via class toggles (active,show) and does not auto-managearia-hiddenon the pane.
These assertions will fail on valid Bootstrap markup that omits the aria-hidden attribute.
Additionally, the selectors assume specific trigger shapes. The collapse check requires an anchor (a[href]), and the tab target is resolved via getAttribute("href") only. Bootstrap 5 supports both href and data-bs-target for both components.
Remove the aria-hidden assertions and resolve tab targets using the pattern data-bs-target || href to match Bootstrap's plugin behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/smoke/browser-interactions.sh` around lines 59 - 121, The tests fail
because they assert aria-hidden (which Bootstrap doesn't auto-set) and only use
href-based targets; update the collapse and tab checks: in the
browser_smoke_exec blocks that click the collapse trigger (look for the query
selector using "a[href=\"#getting_started\"][data-bs-toggle=\"collapse\"]" and
the subsequent trigger.click logic) broaden the selector to accept elements with
data-bs-target or href (e.g. selector matching
[data-bs-toggle="collapse"][data-bs-target="#getting_started"],
[data-bs-toggle="collapse"][href="#getting_started"]), and in the corresponding
browser_smoke_wait_for_js checks remove any panel.getAttribute("aria-hidden")
comparisons and instead only assert panel.classList.contains("show") and
trigger.getAttribute("aria-expanded") === "true" (and the inverse for closing).
For tabs, where TAB_RESULT is built by the browser_smoke_exec that returns
tab.id + "|" + tab.getAttribute("href"), change that logic to resolve the target
as (tab.getAttribute("data-bs-target") || tab.getAttribute("href")) and then in
the browser_smoke_wait_for_js remove the pane.getAttribute("aria-hidden")
assertion and only assert pane.classList.contains("active") &&
pane.classList.contains("show") along with tab.classList.contains("active") and
tab.getAttribute("aria-selected")==="true"; keep using TAB_ID/TAB_TARGET
variables as-is.
| $commandline = SyncRuntime::build_timeout_wrapped_command($command, $args, $config, 60); | ||
|
|
||
| $this->handle = proc_open($commandline, $descriptorspec, $this->pipes); | ||
| if(!is_resource($this->handle)) { | ||
| $this->spawn_failed = true; | ||
| $this->finished = true; | ||
| $this->errors = "Failed to start sync worker process"; | ||
| $this->exit_status = 1; | ||
| $this->handle = null; | ||
| $this->pipes = array(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Catch timeout-bootstrap failures inside SyncProcess::__construct().
SyncRuntime::build_timeout_wrapped_command() can throw, but this constructor only handles proc_open() returning a non-resource. A missing/invalid timeout binary now escapes the constructor before the request enters the existing spawn-failed path, so the sync item is never marked failed/rescheduled.
Suggested fix
- $commandline = SyncRuntime::build_timeout_wrapped_command($command, $args, $config, 60);
-
- $this->handle = proc_open($commandline, $descriptorspec, $this->pipes);
+ try {
+ $commandline = SyncRuntime::build_timeout_wrapped_command($command, $args, $config, 60);
+ $this->handle = proc_open($commandline, $descriptorspec, $this->pipes);
+ } catch(Exception $e) {
+ $this->spawn_failed = true;
+ $this->finished = true;
+ $this->errors = 'Failed to start sync worker process: '.describe_oneline($e);
+ $this->exit_status = 1;
+ $this->handle = null;
+ $this->pipes = array();
+ return;
+ }As per coding guidelines: "Ensure sync script output correctly writes authorized keys files to /var/local/keys-sync/ on target servers".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/sync-common.php` around lines 94 - 105, SyncProcess::__construct
currently assumes SyncRuntime::build_timeout_wrapped_command cannot throw, so a
thrown exception escapes the constructor and bypasses the existing spawn-failed
handling; wrap the call to SyncRuntime::build_timeout_wrapped_command(...) in a
try/catch (catch Exception or Throwable) inside SyncProcess::__construct, and in
the catch set $this->spawn_failed = true, $this->finished = true, $this->errors
to include the exception message, $this->exit_status = 1, clear $this->handle
and $this->pipes the same way the proc_open failure branch does, then return so
the sync item is marked failed/rescheduled.
| } catch(Throwable $e) { | ||
| $this->audit_log('failure', $username, 'Authentication failed'); | ||
| error_log('[LoginFlowService::handle_request] Authentication exception for '.preg_replace('/[^a-zA-Z0-9._-]/', '', $username).': '.$e->getMessage()."\n".$e); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PHP 8.2, when a Throwable is converted to a string (for example by concatenating "$e" into a log message), can the resulting stack trace include function argument values?
💡 Result:
Yes. In PHP 8.2, Throwable::__toString() includes a “Stack trace:” section (it’s effectively the same style of output as getTraceAsString()), and that trace can include function/method argument values (e.g., password_hash('secret', 'unknown-algo')). [1][2]
Whether argument values appear depends on configuration and redaction features:
- If
zend.exception_ignore_args=On, arguments are excluded from exception stack traces (includinggetTraceAsString()/ string-cast output). In PHP 8.2 the default is still effectively “arguments included” unless you enable this. [3] - In PHP 8.2+, parameters marked
#[\SensitiveParameter]are redacted in traces (replaced by aSensitiveParameterValuewrapper), reducing the chance of leaking secrets into logs. [2][3]
Sources: PHP manual on exception string output [1], example of argument values in stack traces and redaction [2], and PHP RFC documenting zend.exception_ignore_args affecting getTraceAsString() and related outputs [3].
Do not stringify the caught Throwable in the auth path.
"\n".$e logs the full exception trace for a login failure. In PHP 8.2, when a Throwable is converted to a string, the stack trace includes function argument values—which can expose plaintext passwords and internals if authentication functions are in the call stack. Log only the exception class and sanitized message instead.
Suggested fix
- error_log('[LoginFlowService::handle_request] Authentication exception for '.preg_replace('/[^a-zA-Z0-9._-]/', '', $username).': '.$e->getMessage()."\n".$e);
+ error_log(
+ '[LoginFlowService::handle_request] Authentication exception for '.
+ preg_replace('/[^a-zA-Z0-9._-]/', '', $username).
+ ': '.get_class($e).': '.$this->sanitize_log_details($e->getMessage())
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/login_flow.php` around lines 54 - 56, The error_log call in the
catch(Throwable $e) inside LoginFlowService::handle_request currently
concatenates $e (stringifying the Throwable) which can expose sensitive argument
values; update the error logging to avoid converting the exception to string and
instead log only the exception class and a sanitized message (use get_class($e)
and $e->getMessage()), keep the username sanitization via preg_replace and do
not append the full exception object or stack trace to the log; adjust the
error_log invocation accordingly and leave audit_log('failure', ...) as-is.
| if(isset($_SERVER['HTTP_X_FORWARDED_PROTO'])) { | ||
| $proto = strtolower(trim((string)$_SERVER['HTTP_X_FORWARDED_PROTO'])); | ||
| if($proto === 'https' && self::is_request_from_trusted_proxy(self::get_runtime_config())) { | ||
| return true; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Can the X-Forwarded-Proto header contain a comma-separated list when a request passes through multiple proxies, and which value should an application use to determine the original client scheme?
💡 Result:
Yes. Although X-Forwarded-Proto is non-standard, in real deployments some proxy stacks append values, producing a comma-separated list when a request traverses multiple proxies (e.g., https,http). [2] Also, per HTTP header rules, multiple instances of the same header field can be combined into a single comma-separated value in-order. [1]
Which value to use: if your proxy chain appends oldest → newest, the left-most value represents the original client-facing scheme at the entry proxy (where TLS is often terminated), so that’s typically the one your application should use—but only when the header is coming from a trusted proxy chain (otherwise clients can spoof it). [2]
If you can, prefer the standardized Forwarded header and use Forwarded: ...;proto=...—it is explicitly defined as a comma-separated hop list, where the first entry is added by the first proxy and represents the original incoming side. [3]
Sources: [1] [2] [3]
🏁 Script executed:
# Check the file and the specific lines mentioned in the review
head -n 130 services/response_security_headers.php | tail -n +100Repository: Msprg/ssh-key-authority
Length of output: 925
🏁 Script executed:
# Get more context around the is_https_request method
sed -n '100,130p' services/response_security_headers.phpRepository: Msprg/ssh-key-authority
Length of output: 925
🏁 Script executed:
# Check if there are any tests or other references to this method
rg 'is_https_request' -A 3 -B 3Repository: Msprg/ssh-key-authority
Length of output: 2120
🏁 Script executed:
# Verify the file path exists
ls -la services/response_security_headers.phpRepository: Msprg/ssh-key-authority
Length of output: 154
Parse the first X-Forwarded-Proto value before comparing.
Lines 117-118 treat the header as a single token. In multi-proxy chains it is commonly comma-delimited, so values like https, http will be misclassified as HTTP and HSTS/other HTTPS-dependent behavior will be skipped.
💡 Minimal fix
- if(isset($_SERVER['HTTP_X_FORWARDED_PROTO'])) {
- $proto = strtolower(trim((string)$_SERVER['HTTP_X_FORWARDED_PROTO']));
+ if(isset($_SERVER['HTTP_X_FORWARDED_PROTO'])) {
+ $forwarded_proto = explode(',', (string)$_SERVER['HTTP_X_FORWARDED_PROTO'], 2);
+ $proto = strtolower(trim($forwarded_proto[0]));
if($proto === 'https' && self::is_request_from_trusted_proxy(self::get_runtime_config())) {
return true;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/response_security_headers.php` around lines 116 - 119, The
X-Forwarded-Proto check treats the header as a single token and can misclassify
when multiple comma-separated values exist; update the branch that reads
$_SERVER['HTTP_X_FORWARDED_PROTO'] so it parses the header and extracts the
first token (trimmed and lowercased) before comparing to 'https', then keep the
existing trust check via
self::is_request_from_trusted_proxy(self::get_runtime_config()) and return true
only when that first token === 'https' and the proxy is trusted; locate this
logic around the code that sets $proto and the surrounding conditional to
implement the parsing fix.
| <?php if($mode == 'create') { ?> | ||
| <div class="form-group"> | ||
| <div class="panel-group"> | ||
| <div class="panel panel-default"> | ||
| <div class="panel-heading"> | ||
| <h3 class="panel-title"> | ||
| <a data-toggle="collapse" href="#advanced_options"> | ||
| Advanced options <span class="caret"></span> | ||
| </a> | ||
| </h3> | ||
| </div> | ||
| <div id="advanced_options" class="collapse"> | ||
| <div class="mb-3"> | ||
| <div class="card"> | ||
| <div class="card-header"> | ||
| <h3 class="h5 mb-0"> | ||
| <a data-bs-toggle="collapse" href="#advanced_options" aria-expanded="false" aria-controls="advanced_options"> | ||
| Advanced options <span class="ska-caret" aria-hidden="true"></span> | ||
| </a> | ||
| </h3> | ||
| </div> | ||
| <div id="advanced_options" class="collapse"> | ||
| <?php } ?> | ||
| <div class="panel-body"> | ||
| <div class="card-body"> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n templates/access_options.php | head -135Repository: Msprg/ssh-key-authority
Length of output: 7951
Keep card-body inside a .card in non-create mode.
When $mode !== 'create', the card wrapper (lines 68–77) and closing divs (lines 115–117) are skipped, but line 79 still renders <div class="card-body">. This results in an orphaned card-body without a card parent, breaking Bootstrap 5 card styling which depends on the card context for proper spacing and CSS variable inheritance.
Suggested fix
<?php if($mode == 'create') { ?>
<div class="mb-3">
<div class="card">
<div class="card-header">
<h3 class="h5 mb-0">
<a data-bs-toggle="collapse" href="#advanced_options" aria-expanded="false" aria-controls="advanced_options">
Advanced options <span class="ska-caret" aria-hidden="true"></span>
</a>
</h3>
</div>
<div id="advanced_options" class="collapse">
- <?php } ?>
+ <?php } else { ?>
+ <div class="card mb-3">
+ <?php } ?>
<div class="card-body">
@@
<?php if($mode == 'create') { ?>
</div>
</div>
</div>
+ <?php } else { ?>
+ </div>
<?php } ?>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/access_options.php` around lines 67 - 79, The <div
class="card-body"> is rendered even when $mode !== 'create', leaving it outside
any .card; update templates/access_options.php so card-body is only output
inside the same if($mode == 'create') block that renders the <div class="card">
and its header (or alternatively, render a matching <div class="card"> wrapper
when $mode !== 'create'); modify the conditional around the collapse block and
<div class="card-body"> (symbols: $mode, <div class="card">, <div
id="advanced_options" class="collapse">, and <div class="card-body">) so that
card-body always has a parent .card in both branches.
| $report_address = $config['email']['report_address'] ?? ''; | ||
| $report_name = $config['email']['report_name'] ?? 'SSH Key Authority reports'; | ||
| $base_url = $config['web']['baseurl'] ?? ''; | ||
| $actor_name = ($active_user !== null && isset($active_user->name)) ? $active_user->name : 'Unknown user'; | ||
| $actor_uid = ($active_user !== null && isset($active_user->uid)) ? $active_user->uid : 'unknown'; | ||
|
|
||
| $email = new Email(); | ||
| $email->add_recipient($config['email']['report_address'], $config['email']['report_name']); | ||
| if($report_address !== '') { | ||
| $email->add_recipient($report_address, $report_name); | ||
| } | ||
| $email->subject = "Key has been allowed"; | ||
|
|
||
| $allowed_keys_url = "{$config['web']['baseurl']}/pubkeys#allowed"; | ||
| $email->body = "The following key has been allowed by {$active_user->name} ({$active_user->uid}):\n"; | ||
| $email->body .= "{$key->type} {$key->keydata}\n\n"; | ||
| $email->body .= "This means, the key will stay untouched in ~/.ssh/authorized_keys files on target machines, if it appears.\n"; | ||
| $email->body .= "You can see the full list of allowed keys at $allowed_keys_url"; | ||
| $allowed_keys_url = null; | ||
| if(trim($base_url) === '') { | ||
| error_log('send_mail_key_allowed: web.baseurl is not configured; omitting allowed keys link from email.'); | ||
| } else { | ||
| $allowed_keys_url = rtrim($base_url, '/').'/pubkeys#allowed'; | ||
| } | ||
| $email->body = "The following key has been allowed by {$actor_name} ({$actor_uid}):\n"; | ||
| $email->body .= "{$key->type} {$key->keydata}\n\n"; | ||
| $email->body .= "This means, the key will stay untouched in ~/.ssh/authorized_keys files on target machines, if it appears.\n"; | ||
| if($allowed_keys_url !== null) { | ||
| $email->body .= "You can see the full list of allowed keys at $allowed_keys_url"; | ||
| } | ||
|
|
||
| $email->send(); | ||
| if($report_address !== '') { | ||
| $email->send(); | ||
| } |
There was a problem hiding this comment.
Normalize config-derived email/url fields before send/link construction
Line 34 and Line 52 currently treat whitespace-only report_address as valid (!== ''), and Line 43 builds the URL from untrimmed $base_url. This can cause invalid recipient/send behavior and malformed links in notifications.
Suggested patch
- $report_address = $config['email']['report_address'] ?? '';
+ $report_address = trim((string)($config['email']['report_address'] ?? ''));
$report_name = $config['email']['report_name'] ?? 'SSH Key Authority reports';
- $base_url = $config['web']['baseurl'] ?? '';
+ $base_url = trim((string)($config['web']['baseurl'] ?? ''));
@@
- $email = new Email();
- if($report_address !== '') {
- $email->add_recipient($report_address, $report_name);
- }
+ if($report_address === '') {
+ error_log('send_mail_key_allowed: email.report_address is not configured; skipping notification.');
+ return;
+ }
+ $email = new Email();
+ $email->add_recipient($report_address, $report_name);
@@
- if(trim($base_url) === '') {
+ if($base_url === '') {
error_log('send_mail_key_allowed: web.baseurl is not configured; omitting allowed keys link from email.');
} else {
$allowed_keys_url = rtrim($base_url, '/').'/pubkeys#allowed';
}
@@
- if($report_address !== '') {
- $email->send();
- }
+ $email->send();Based on learnings: Applies to **/{sync*.php,*publickey*.php,*key*.php,services/auth.php,scripts/sync*.php} : Validate any change that touches SSH key generation or distribution.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $report_address = $config['email']['report_address'] ?? ''; | |
| $report_name = $config['email']['report_name'] ?? 'SSH Key Authority reports'; | |
| $base_url = $config['web']['baseurl'] ?? ''; | |
| $actor_name = ($active_user !== null && isset($active_user->name)) ? $active_user->name : 'Unknown user'; | |
| $actor_uid = ($active_user !== null && isset($active_user->uid)) ? $active_user->uid : 'unknown'; | |
| $email = new Email(); | |
| $email->add_recipient($config['email']['report_address'], $config['email']['report_name']); | |
| if($report_address !== '') { | |
| $email->add_recipient($report_address, $report_name); | |
| } | |
| $email->subject = "Key has been allowed"; | |
| $allowed_keys_url = "{$config['web']['baseurl']}/pubkeys#allowed"; | |
| $email->body = "The following key has been allowed by {$active_user->name} ({$active_user->uid}):\n"; | |
| $email->body .= "{$key->type} {$key->keydata}\n\n"; | |
| $email->body .= "This means, the key will stay untouched in ~/.ssh/authorized_keys files on target machines, if it appears.\n"; | |
| $email->body .= "You can see the full list of allowed keys at $allowed_keys_url"; | |
| $allowed_keys_url = null; | |
| if(trim($base_url) === '') { | |
| error_log('send_mail_key_allowed: web.baseurl is not configured; omitting allowed keys link from email.'); | |
| } else { | |
| $allowed_keys_url = rtrim($base_url, '/').'/pubkeys#allowed'; | |
| } | |
| $email->body = "The following key has been allowed by {$actor_name} ({$actor_uid}):\n"; | |
| $email->body .= "{$key->type} {$key->keydata}\n\n"; | |
| $email->body .= "This means, the key will stay untouched in ~/.ssh/authorized_keys files on target machines, if it appears.\n"; | |
| if($allowed_keys_url !== null) { | |
| $email->body .= "You can see the full list of allowed keys at $allowed_keys_url"; | |
| } | |
| $email->send(); | |
| if($report_address !== '') { | |
| $email->send(); | |
| } | |
| $report_address = trim((string)($config['email']['report_address'] ?? '')); | |
| $report_name = $config['email']['report_name'] ?? 'SSH Key Authority reports'; | |
| $base_url = trim((string)($config['web']['baseurl'] ?? '')); | |
| $actor_name = ($active_user !== null && isset($active_user->name)) ? $active_user->name : 'Unknown user'; | |
| $actor_uid = ($active_user !== null && isset($active_user->uid)) ? $active_user->uid : 'unknown'; | |
| if($report_address === '') { | |
| error_log('send_mail_key_allowed: email.report_address is not configured; skipping notification.'); | |
| return; | |
| } | |
| $email = new Email(); | |
| $email->add_recipient($report_address, $report_name); | |
| $email->subject = "Key has been allowed"; | |
| $allowed_keys_url = null; | |
| if($base_url === '') { | |
| error_log('send_mail_key_allowed: web.baseurl is not configured; omitting allowed keys link from email.'); | |
| } else { | |
| $allowed_keys_url = rtrim($base_url, '/').'/pubkeys#allowed'; | |
| } | |
| $email->body = "The following key has been allowed by {$actor_name} ({$actor_uid}):\n"; | |
| $email->body .= "{$key->type} {$key->keydata}\n\n"; | |
| $email->body .= "This means, the key will stay untouched in ~/.ssh/authorized_keys files on target machines, if it appears.\n"; | |
| if($allowed_keys_url !== null) { | |
| $email->body .= "You can see the full list of allowed keys at $allowed_keys_url"; | |
| } | |
| $email->send(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@views/pubkeys.php` around lines 27 - 54, Trim and validate config-derived
email and URL fields before using them: normalize $report_address =
trim($report_address) and $report_name = trim($report_name) (or default if
empty) and only call $email->add_recipient(...) and $email->send() when the
trimmed $report_address !== ''; likewise normalize $base_url = trim($base_url)
before building $allowed_keys_url and only build allowed_keys_url when trimmed
$base_url !== '' (use rtrim on the trimmed base URL when constructing the link);
update references in this block (Email, add_recipient, send, allowed_keys_url,
rtrim) so whitespace-only values are treated as empty.
1 participant
Summary by CodeRabbit
New Features
Security Enhancements
UI / Frontend
Tooling / Tests
Stability
Documentation