Skip to content

Conversation

@techjewel
Copy link
Contributor

This pull request introduces important security improvements and code enhancements, primarily focusing on strengthening file access protections and improving the flexibility of reporting queries. The most significant changes are grouped below by theme.

Security Enhancements:

  • Added a security check in BaseHandler.php to block access to sensitive system directories (such as /etc/, /proc/, /sys/, /dev/, /root/, as well as wp-config.php and .htaccess), preventing unauthorized file reads by plugins or themes. The list of blocked paths can now be customized via the fluentsmtp_attachment_blocked_paths filter.
  • Updated the docblock for secureFileRead to clarify that it now always returns a string on success and throws an exception on failure, reflecting the stricter error handling.

Reporting Improvements:

  • Refactored the getSendingStats method in Reporting.php to dynamically build the SQL SELECT clause based on the groupBy parameter (day, week, or month). This ensures the selected columns match the grouping, improving query accuracy and maintainability.

Configuration Updates:

  • Added a .claude/settings.local.json file to explicitly allow certain shell commands (gh pr view:*, gh api:*) for development or automation purposes.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Improves FluentSMTP’s security around attachment file reads and refines reporting aggregation queries by adjusting how grouped stats are selected, plus adds a Claude tooling config file.

Changes:

  • Added blocked-path enforcement (with a WP filter) to prevent attachment reads from sensitive locations.
  • Refactored Reporting::getSendingStats() to build the SQL SELECT clause dynamically for day/week/month groupings.
  • Added .claude/settings.local.json to allow specific GitHub CLI commands.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File Description
app/Services/Reporting.php Builds a dynamic SELECT clause for grouped sending stats queries.
app/Services/Mailer/BaseHandler.php Adds a blocked-path security gate to secureFileRead() and updates its docblock.
.claude/settings.local.json Adds Claude permission allowlist entries for gh commands.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 35 to 40
$selectClause = 'COUNT(id) AS count, DATE(created_at) AS date';

if ($groupBy === 'week') {
$selectClause .= ', WEEK(created_at) AS week';
} elseif ($groupBy === 'month') {
$selectClause .= ', MONTH(created_at) AS month';
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

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

When grouping by week or month, the query still selects DATE(created_at) AS date. Since date isn’t part of the GROUP BY (and isn’t aggregated), this can produce nondeterministic date values (and can fail under ONLY_FULL_GROUP_BY). Consider making the selected date expression match the grouping (e.g., compute a week-start / month-start date or use an aggregate like MIN(DATE(created_at))).

Suggested change
$selectClause = 'COUNT(id) AS count, DATE(created_at) AS date';
if ($groupBy === 'week') {
$selectClause .= ', WEEK(created_at) AS week';
} elseif ($groupBy === 'month') {
$selectClause .= ', MONTH(created_at) AS month';
if ($groupBy === 'week') {
// When grouping by week, aggregate date to keep query valid under ONLY_FULL_GROUP_BY
$selectClause = 'COUNT(id) AS count, MIN(DATE(created_at)) AS date, WEEK(created_at) AS week';
} elseif ($groupBy === 'month') {
// When grouping by month, aggregate date to keep query valid under ONLY_FULL_GROUP_BY
$selectClause = 'COUNT(id) AS count, MIN(DATE(created_at)) AS date, MONTH(created_at) AS month';
} else {
// Default: group by date (daily stats)
$selectClause = 'COUNT(id) AS count, DATE(created_at) AS date';

Copilot uses AI. Check for mistakes.
Comment on lines 38 to 40
$selectClause .= ', WEEK(created_at) AS week';
} elseif ($groupBy === 'month') {
$selectClause .= ', MONTH(created_at) AS month';
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

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

WEEK(created_at) and MONTH(created_at) don’t include the year, so stats can be merged across different years (e.g., week 1 of 2025 + week 1 of 2026). Use a year-qualified grouping/selection (e.g., YEARWEEK(...) for weeks and DATE_FORMAT(created_at, '%Y-%m') or YEAR(created_at), MONTH(created_at) for months) so results remain correct across year boundaries.

Suggested change
$selectClause .= ', WEEK(created_at) AS week';
} elseif ($groupBy === 'month') {
$selectClause .= ', MONTH(created_at) AS month';
// Use YEARWEEK so weeks are not merged across different years
$selectClause .= ', YEARWEEK(created_at, 1) AS week';
} elseif ($groupBy === 'month') {
// Use year-qualified month (YYYY-MM) to avoid merging months across years
$selectClause .= ", DATE_FORMAT(created_at, '%Y-%m') AS month";

Copilot uses AI. Check for mistakes.
'/sys/',
'/dev/',
'/root/',
ABSPATH . 'wp-config.php',
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

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

Blocking ABSPATH . 'wp-config.php' and ABSPATH . '.htaccess' won’t catch common WordPress setups where wp-config.php lives one directory above ABSPATH. If the goal is to reliably prevent reading wp-config.php, add the parent-directory location(s) as well (or resolve and compare against both possible real paths).

Suggested change
ABSPATH . 'wp-config.php',
ABSPATH . 'wp-config.php',
dirname(ABSPATH) . '/wp-config.php',

Copilot uses AI. Check for mistakes.

// Allow developers to customize blocked paths via filter
$blockedPaths = apply_filters('fluentsmtp_attachment_blocked_paths', $blockedPaths);

Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

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

apply_filters('fluentsmtp_attachment_blocked_paths', ...) is assumed to return an array of strings, but filters can return unexpected types. To avoid warnings/fatals in foreach and realpath(), validate/cast the result (e.g., ensure it’s an array and filter out non-string entries) before iterating.

Suggested change
// Ensure the filtered result is an array of strings before iterating
if (!is_array($blockedPaths)) {
$blockedPaths = [];
} else {
$blockedPaths = array_values(array_filter($blockedPaths, 'is_string'));
}

Copilot uses AI. Check for mistakes.
Comment on lines +500 to +503
if ($blockedRealPath && strpos($realPath, $blockedRealPath) === 0) {
throw new Exception(
__('Access to this file location is restricted for security reasons', 'fluent-smtp')
);
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

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

The prefix check strpos($realPath, $blockedRealPath) === 0 can over-match (e.g., blocking /etc2/... when /etc is blocked) because it doesn’t enforce a path boundary. Normalize paths and require either an exact match or a match followed by a directory separator to avoid false positives.

Suggested change
if ($blockedRealPath && strpos($realPath, $blockedRealPath) === 0) {
throw new Exception(
__('Access to this file location is restricted for security reasons', 'fluent-smtp')
);
if ($blockedRealPath) {
// Normalize blocked path (realpath removes trailing separators, but be defensive)
$normalizedBlocked = rtrim($blockedRealPath, DIRECTORY_SEPARATOR);
if ($normalizedBlocked === '') {
continue;
}
// Always block an exact match (for both files and directories)
if ($realPath === $normalizedBlocked) {
throw new Exception(
__('Access to this file location is restricted for security reasons', 'fluent-smtp')
);
}
// If the blocked path is a directory, also block any file within it,
// but require a path boundary to avoid over-matching (e.g. /etc2).
if (is_dir($normalizedBlocked)) {
$blockedDirPrefix = $normalizedBlocked . DIRECTORY_SEPARATOR;
if (strpos($realPath, $blockedDirPrefix) === 0) {
throw new Exception(
__('Access to this file location is restricted for security reasons', 'fluent-smtp')
);
}
}

Copilot uses AI. Check for mistakes.
Comment on lines 1 to 6
{
"permissions": {
"allow": [
"Bash(gh pr view:*)",
"Bash(gh api:*)"
]
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

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

This file is named settings.local.json, which typically implies a machine-local config that shouldn’t be committed. If it’s intended to be local-only, add it to .gitignore; if it’s intended to be shared, consider renaming to a non-.local filename to avoid confusion.

Copilot uses AI. Check for mistakes.
@claude
Copy link

claude bot commented Feb 7, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

app/Services/Reporting.php:56

  • In the month branch, DATE_FORMAT(created_at, '%Y-%m') introduces % tokens into the SQL string that is later passed to $wpdb->prepare(). wpdb::prepare() treats % sequences as placeholders, so this will trigger “called incorrectly” warnings or malformed SQL. Escape the percent signs (e.g., use %%Y-%%m) or restructure the query so the DATE_FORMAT fragment is not processed by prepare().
        } elseif ($groupBy === 'month') {
            // Use YYYY-MM format to prevent merging months across different years
            $selectClause = "COUNT(id) AS count, MIN(DATE(created_at)) AS date, DATE_FORMAT(created_at, '%Y-%m') AS month";
        } else {
            // Default: group by date (daily stats)
            $selectClause = 'COUNT(id) AS count, DATE(created_at) AS date';
        }

        // Only parameterize data values (dates), NOT table/column names
        // Column names are validated above against whitelist
        $items = $wpdb->get_results(
            $wpdb->prepare(
                "SELECT {$selectClause}
                 FROM `{$tableName}`
                 WHERE `created_at` BETWEEN %s AND %s
                 GROUP BY `{$groupBy}`
                 ORDER BY `{$orderBy}` ASC",

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 39 to 42
$selectClause = 'COUNT(id) AS count, MIN(DATE(created_at)) AS date, YEARWEEK(created_at, 1) AS week';
} elseif ($groupBy === 'month') {
// Use YYYY-MM format to prevent merging months across different years
$selectClause = "COUNT(id) AS count, MIN(DATE(created_at)) AS date, DATE_FORMAT(created_at, '%Y-%m') AS month";
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

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

For week/month grouping, MIN(DATE(created_at)) AS date can vary within the bucket (e.g., if no email exists on the period boundary), which may not align with the keys generated by DatePeriod in getResult() and can lead to missing/extra buckets in the returned array. Consider selecting a deterministic bucket date (start-of-week/start-of-month) that matches the period stepping and grouping expression.

Suggested change
$selectClause = 'COUNT(id) AS count, MIN(DATE(created_at)) AS date, YEARWEEK(created_at, 1) AS week';
} elseif ($groupBy === 'month') {
// Use YYYY-MM format to prevent merging months across different years
$selectClause = "COUNT(id) AS count, MIN(DATE(created_at)) AS date, DATE_FORMAT(created_at, '%Y-%m') AS month";
// Use the Monday of each week as a deterministic bucket date
$selectClause = 'COUNT(id) AS count, DATE(DATE_SUB(created_at, INTERVAL WEEKDAY(created_at) DAY)) AS date, YEARWEEK(created_at, 1) AS week';
} elseif ($groupBy === 'month') {
// Use YYYY-MM format to prevent merging months across different years
// Use the first day of the month as a deterministic bucket date
$selectClause = "COUNT(id) AS count, DATE_FORMAT(created_at, '%Y-%m-01') AS date, DATE_FORMAT(created_at, '%Y-%m') AS month";

Copilot uses AI. Check for mistakes.
$table = $wpdb->prefix . FLUENT_MAIL_DB_PREFIX.'email_logs';

if ($wpdb->get_var("SHOW TABLES LIKE '$table'") != $table) {
if ($wpdb->get_var($wpdb->prepare("SHOW TABLES LIKE %s", $table)) != $table) {
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

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

SHOW TABLES LIKE %s treats _ and % as wildcards in the pattern. Since WP table names commonly contain underscores, this check can match unintended tables and cause a false negative/positive. Consider escaping the table name for LIKE (e.g., via $wpdb->esc_like($table)) before passing it to the query so the existence check is exact.

Suggested change
if ($wpdb->get_var($wpdb->prepare("SHOW TABLES LIKE %s", $table)) != $table) {
if ($wpdb->get_var($wpdb->prepare("SHOW TABLES LIKE %s", $wpdb->esc_like($table))) != $table) {

Copilot uses AI. Check for mistakes.
);
} else {
// Query for all time data when lastDay <= 6
// Table name is safe - constructed from WordPress prefix and plugin constant
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

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

The new comment says the table name is constructed from a “plugin constant”, but $tableName is built from $wpdb->prefix plus a hard-coded string ('fsmpt_email_logs'). Please correct the comment (or update the code to use the intended constant) so it reflects reality.

Suggested change
// Table name is safe - constructed from WordPress prefix and plugin constant
// Table name is safe - constructed from the WordPress prefix and a hard-coded table suffix

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@techjewel techjewel merged commit 357fc04 into master Feb 7, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant