diff --git a/acl/acl-lib.pl b/acl/acl-lib.pl
index 765286004..981d5361c 100755
--- a/acl/acl-lib.pl
+++ b/acl/acl-lib.pl
@@ -1809,6 +1809,7 @@ foreach my $g (&list_groups()) {
return $g;
}
}
+return undef;
}
=head2 check_password_restrictions(username, password)
diff --git a/acl/log_parser.pl b/acl/log_parser.pl
index e164d976e..4a70230e5 100755
--- a/acl/log_parser.pl
+++ b/acl/log_parser.pl
@@ -16,8 +16,9 @@ my ($user, $script, $action, $type, $object, $p) = @_;
my $g = $type eq 'group' ? "_g" : "";
if ($action eq 'modify') {
if ($p->{'old'} ne $p->{'name'}) {
- return &text('log_rename'.$g, "$p->{'old'}",
- "$p->{'name'}");
+ return &text('log_rename'.$g,
+ "".&html_escape($p->{'old'})."",
+ "".&html_escape($p->{'name'})."");
}
else {
return &text('log_modify'.$g,
@@ -36,21 +37,23 @@ elsif ($action eq 'create') {
}
elsif ($action eq 'delete') {
if ($type eq "users" || $type eq "groups") {
- return &text('log_delete_'.$type, $object);
+ return &text('log_delete_'.$type, &html_escape($object));
}
else {
- return &text('log_delete'.$g, "$object");
+ return &text('log_delete'.$g,
+ "".&html_escape($object)."");
}
}
elsif ($action eq 'joingroup') {
- return &text('log_joingroup', $object, $p->{'group'});
+ return &text('log_joingroup', &html_escape($object),
+ &html_escape($p->{'group'}));
}
elsif ($action eq 'acl') {
- return &text('log_acl', "$object",
+ return &text('log_acl', "".&html_escape($object)."",
"".&html_escape($p->{'moddesc'})."");
}
elsif ($action eq 'reset') {
- return &text('log_reset', "$object",
+ return &text('log_reset', "".&html_escape($object)."",
"".&html_escape($p->{'moddesc'})."");
}
elsif ($action eq 'cert') {
@@ -60,7 +63,9 @@ elsif ($action eq 'switch') {
return &text('log_switch', "".&html_escape($object)."");
}
elsif ($action eq 'twofactor') {
- return &text('log_twofactor', $object, $p->{'provider'}, $p->{'id'});
+ return &text('log_twofactor', &html_escape($object),
+ &html_escape($p->{'provider'}),
+ &html_escape($p->{'id'}));
}
elsif ($action eq 'forgot') {
return &text('log_forgot_'.$type, &html_escape($p->{'user'}),
diff --git a/acl/t/run-tests.t b/acl/t/run-tests.t
index 958103cf8..0aea300f3 100644
--- a/acl/t/run-tests.t
+++ b/acl/t/run-tests.t
@@ -865,11 +865,8 @@ is(group_line({ name => 'empty' }),
my $g = get_users_group('bob');
is($g && $g->{'name'}, 'wheel', 'get_users_group finds bob in wheel');
- # get_users_group falls off the end without an explicit return; in scalar
- # context that yields '' (the failed loop condition) rather than undef.
- # Asserting falsy keeps the test honest about the current contract.
- ok(!get_users_group('nobody'),
- 'get_users_group returns falsy for unaffiliated user');
+ is(get_users_group('nobody'), undef,
+ 'get_users_group returns undef when user is in no group');
delete_from_groups('bob');
_clear_caches();
@@ -934,22 +931,50 @@ is(group_line({ name => 'empty' }),
like(parse_webmin_log('admin', 's', 'switch', 'user', 'alice', {}),
qr/Switched to.*alice/, 'parse_webmin_log switch');
- # html_escape: a malicious $object or moddesc must come back escaped.
- # Several branches (delete, modify rename, joingroup, twofactor) currently
- # do NOT escape $object — they interpolate it into a tag verbatim.
- # That's an audit-trail XSS gap; mark these TODO so the suite documents
- # the expected post-fix behavior without going red.
- TODO: {
- local $TODO = 'parse_webmin_log delete branch does not html_escape $object';
- my $xss = parse_webmin_log('admin', 's', 'delete', 'user',
- '', {});
- unlike($xss, qr/';
+ my $esc = qr/<script>/;
+
+ for my $case (
+ [ 'delete user',
+ [ 'admin', 's', 'delete', 'user', $payload, {} ] ],
+ [ 'modify rename (old name)',
+ [ 'admin', 's', 'modify', 'user', 'safe',
+ { old => $payload, name => 'safe' } ] ],
+ [ 'modify rename (new name)',
+ [ 'admin', 's', 'modify', 'user', 'safe',
+ { old => 'safe', name => $payload } ] ],
+ [ 'joingroup (object)',
+ [ 'admin', 's', 'joingroup', 'user', $payload,
+ { group => 'wheel' } ] ],
+ [ 'joingroup (group)',
+ [ 'admin', 's', 'joingroup', 'user', 'alice',
+ { group => $payload } ] ],
+ [ 'twofactor (object)',
+ [ 'admin', 's', 'twofactor', 'user', $payload,
+ { provider => 'totp', id => 'k' } ] ],
+ [ 'acl (object)',
+ [ 'admin', 's', 'acl', 'user', $payload,
+ { moddesc => 'User Admin' } ] ],
+ [ 'reset (object)',
+ [ 'admin', 's', 'reset', 'user', $payload,
+ { moddesc => 'User Admin' } ] ],
+ [ 'delete users bulk (object)',
+ [ 'admin', 's', 'delete', 'users', $payload, {} ] ],
+ ) {
+ my ($name, $args) = @$case;
+ my $out = parse_webmin_log(@$args);
+ unlike($out, qr/