Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improve error logging while in SSI #7789

Draft
wants to merge 2 commits into
base: release-2.1
Choose a base branch
from

Conversation

jdarwood007
Copy link
Member

When a error occurs on a remote script, the full url is not logged. For example. Given a forum at: https://example.com/forum/ and site content at https://example.com/about/

Some examples here:
A error at /about/ will log just the /about/ if no query string is provided. A error at /about/?foo=bar will log just ?foo=bar
A error at https://sub.example.com/ will log no url

SMF will then when displaying the error log, give false urls due to its handling of urls in the viewer.

This will for SSI, attempt to use the correct URLs, falling back to various methods and default responses.

This is draft for now, I am performing testing with this.

When a error occurs on a remote script, the full url is not logged.
For example.  Given a forum at:  https://example.com/forum/
and site content at https://example.com/about/

Some examples here:
A error at /about/ will log just the /about/ if no query string is provided.
A error at /about/?foo=bar will log just ?foo=bar
A error at https://sub.example.com/ will log no url

SMF will then when displaying the error log, give false urls due to its handling of urls in the viewer.

This will for SSI, attempt to use the correct URLs, falling back to various methods and default responses.

This is draft for now, I am performing testing with this.
@@ -180,7 +180,7 @@ function ViewErrorLog()
'time' => timeformat($row['log_time']),
'timestamp' => $row['log_time'],
'url' => array(
'html' => $smcFunc['htmlspecialchars'](strpos($row['url'], 'cron.php') === false ? (substr($row['url'], 0, 1) == '?' ? $scripturl : '') . $row['url'] : $row['url']),
'html' => $smcFunc['htmlspecialchars'](strpos($row['url'], 'cron.php') === false && !strpos($row['url'], 'https://') && !strpos($row['url'], 'http://') ? (substr($row['url'], 0, 1) == '?' ? $scripturl : '') . $row['url'] : $row['url']),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'html' => $smcFunc['htmlspecialchars'](strpos($row['url'], 'cron.php') === false && !strpos($row['url'], 'https://') && !strpos($row['url'], 'http://') ? (substr($row['url'], 0, 1) == '?' ? $scripturl : '') . $row['url'] : $row['url']),
'html' => $smcFunc['htmlspecialchars'](strpos($row['url'], 'cron.php') === false && !strpos($row['url'], 'https://') && !strpos($row['url'], 'http://') ? ($row['url'][0] === '?' ? $scripturl : '') . $row['url'] : $row['url']),

perf: Check string inline; do not check copy

Copy link
Contributor

Choose a reason for hiding this comment

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

How's that for premature optimization, lol

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks good. I didn't even think about it using more memory that way as its copying it. Really wish PHP had kept {} as the way to navigate strings by character offset. So confusing think its an array. Yes I know technically in C/C++ a string is an array of chars, but still...

Copy link
Member Author

Choose a reason for hiding this comment

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

I sent up a change for the strpos stuff I did. http:// could exist somewhere in the string and break this. So I check it starts with 0 and then flip the check.

<?php
$url = 'https://google.com';

var_dump(strpos($url, 'cron.php') === false);
var_dump(!strpos($url, 'https://'));
var_dump(!strpos($url, 'http://'));

$url = 'bad-actor/https://google.com';

var_dump(strpos($url, 'cron.php') === false);
var_dump(!(strpos($url, 'https://') === 0));
var_dump(!(strpos($url, 'http://') === 0));

@jdarwood007 jdarwood007 added this to the 3.0 Alpha 5 milestone Jan 22, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants