Ad snippet analysis #69

senky · 2017-08-06T20:00:43Z

Analysis of entered snippet will be presented to the admin, such as “untrusted connection” when board runs on HTTPS but advertisement snippet will try to load HTTP JS file, “blocking javascript” when <script> tag without async attribute will be present, etc.

iMattPro · 2017-08-08T14:03:56Z

adm/style/manage_ads.html

-				<dd><textarea id="ad_code" name="ad_code" rows="20" cols="60" style="width: 95%;">{{ AD_CODE }}</textarea></dd>
+				<dd>
+					<textarea id="ad_code" name="ad_code" rows="20" cols="60" style="width: 95%;">{{ AD_CODE }}</textarea>
+					<p>{{ lang('AD_CODE_NOT_SURE') }} <input type="submit" class="button2" id="analyse_ad_code" name="analyse_ad_code" value="{{ lang('ANALYSE_AD_CODE') }}"></p>


I think all we need here is the button (dont need the NOT_SURE line)

iMattPro · 2017-08-08T14:07:50Z

adm/style/manage_ads.html

+								</ul>
+							{% endif %}
+						{% else %}
+							{{ lang('EVERYTHING_OK') }}


We probably shouldn't say anything here. If the ad code has not been analyzed, we shouldn't presume to say the code is ok.

iMattPro · 2017-08-08T14:09:16Z

adm/style/manage_ads.html

+								<h4>{{ lang('WARNINGS') }}</h4>
+								<ul>
+									{% for warning in loops.analyser_results_warning %}
+										<li>{{ warning.MESSAGE }}</li>


There's a little inconsistency in the indentations between these two blocks of code here and above.

We should only say something if the test has been run and passed (maybe define this template var that everything is OK only if there are no test fails, after it has been run).

iMattPro · 2017-08-08T14:11:44Z

analyser/manager.php

+		$this->lang = $lang;
+	}
+
+	public function test($ad_code)


iMattPro · 2017-08-08T14:13:11Z

analyser/manager.php

+		$this->lang = $lang;
+	}
+
+	public function test($ad_code)


I would prefer calling this run. Also in the following classes, calling it run. It looks better when we have:

$test->run(); $analyser->run();

iMattPro · 2017-08-08T14:34:02Z

language/en/acp.php

+	'ANALYSE_AD_CODE'			=> 'analyse advertisement code',
+	'RESULTS'					=> 'Results',
+	'NOTICES'					=> 'Notices',
+	'EVERYTHING_OK'				=> 'The code seems to be OK.',


The code appears OK.

iMattPro · 2017-08-08T14:40:32Z

language/en/acp.php

@@ -80,6 +85,12 @@
 	'ACP_AD_DISABLE_SUCCESS'	=> 'Advertisement disabled successfully.',
 	'ACP_AD_DISABLE_ERRORED'	=> 'There was an error disabling the advertisement.',

+	// Analyser tests
+	'UNSECURE_CONNECTION'	=> '<strong>Mixed Content</strong><br />Your code will probably generate “Mixed Content” warning in the browser, because your board runs on HTTPS, but the ad code attempts to load a file from unsecure HTTP connection.',


Mixed Content
Your board runs on a secure HTTPS connection, however the ad code is attempting to load content from an insecure HTTP connection. This can cause browsers to generate a "Mixed Content" warning to let users know that the page contains insecure resources.

iMattPro · 2017-08-08T14:45:08Z

language/en/acp.php

@@ -80,6 +85,12 @@
 	'ACP_AD_DISABLE_SUCCESS'	=> 'Advertisement disabled successfully.',
 	'ACP_AD_DISABLE_ERRORED'	=> 'There was an error disabling the advertisement.',

+	// Analyser tests
+	'UNSECURE_CONNECTION'	=> '<strong>Mixed Content</strong><br />Your code will probably generate “Mixed Content” warning in the browser, because your board runs on HTTPS, but the ad code attempts to load a file from unsecure HTTP connection.',
+	'SCRIPT_WITHOUT_ASYNC'	=> '<strong>script without async attribute</strong><br />Your code will load script synchronously, which affects page load performance. Use <samp>async</samp> attribute to speed up the page load.',


Non-asynchronous javascript
This ad code loads JavaScript code in a non-asynchronous way. This means it will block any other Javascript from loading until it has completed loading, which can affect page load performance. Use of the async attribute can speed up the page load.

iMattPro · 2017-08-08T14:47:32Z

language/en/acp.php

+	// Analyser tests
+	'UNSECURE_CONNECTION'	=> '<strong>Mixed Content</strong><br />Your code will probably generate “Mixed Content” warning in the browser, because your board runs on HTTPS, but the ad code attempts to load a file from unsecure HTTP connection.',
+	'SCRIPT_WITHOUT_ASYNC'	=> '<strong>script without async attribute</strong><br />Your code will load script synchronously, which affects page load performance. Use <samp>async</samp> attribute to speed up the page load.',
+	'ALERT_USAGE'			=> '<strong>Usage of <samp>alert()</samp></strong><br />Your code uses <samp>alert()</samp> function which annoys users and even blocks them using your board before they close it. Some browsers will even block page load and display warning to the user.',


Usage of alert()
Your code uses the alert() function which is not a good practice and can distract users. Some browsers may also block page load and display additional warnings to the user.

iMattPro · 2017-08-08T14:50:20Z

language/en/acp.php

+	'UNSECURE_CONNECTION'	=> '<strong>Mixed Content</strong><br />Your code will probably generate “Mixed Content” warning in the browser, because your board runs on HTTPS, but the ad code attempts to load a file from unsecure HTTP connection.',
+	'SCRIPT_WITHOUT_ASYNC'	=> '<strong>script without async attribute</strong><br />Your code will load script synchronously, which affects page load performance. Use <samp>async</samp> attribute to speed up the page load.',
+	'ALERT_USAGE'			=> '<strong>Usage of <samp>alert()</samp></strong><br />Your code uses <samp>alert()</samp> function which annoys users and even blocks them using your board before they close it. Some browsers will even block page load and display warning to the user.',
+	'LOCATION_CHANGE'		=> '<strong>Redirection</strong><br />Your code will redirect user to another (probably malicious) page.',


Redirection
Your code appears it can redirect user to another page or site. Redirects can sometimes send users to unintended, often malicious, destinations. Please verify the integrity of your ad code's redirection destination.

iMattPro · 2017-08-08T15:47:16Z

analyser/test/script_without_async.php

+	 */
+	public function test($ad_code)
+	{
+		if (preg_match_all('/&lt;script(.*)&gt;/U', $ad_code, $matches))


This test is failing for me because this first regex is matching the closing script tag as well, and since the closing tag has no async...false positive.

Are you sure? It can't match closing tag, there is a slash after < in that case...

iMattPro · 2017-08-08T18:17:09Z

yep, positive. I had breakpoints and it ran through the condition twice…once for the opening tag and again for the closing tag.

…

On Aug 8, 2017, at 9:41 AM, Jakub Senko ***@***.***> wrote: @senky commented on this pull request. In analyser/test/script_without_async.php <#69 (comment)>: > + * + * @copyright (c) 2017 phpBB Limited <https://www.phpbb.com> + * @license GNU General Public License, version 2 (GPL-2.0) + * + */ + +namespace phpbb\ads\analyser\test; + +class script_without_async implements test_interface +{ + /** + * ***@***.***} + */ + public function test($ad_code) + { + if (preg_match_all('/<script(.*)>/U', $ad_code, $matches)) Are you sure? It can't match closing tag, there is a slash after < in that case... — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#69 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASiX6SoWlr0UUJXhYULHnm7F3SDUKKqks5sWI-tgaJpZM4Ou04U>.

senky · 2017-08-09T13:41:27Z

@VSEphpbb can you post your ad code here?

iMattPro · 2017-08-09T13:54:58Z

@senky I see what it is.. there's a script within a script in Google Adsense

<div class="stat-block" style="width:100%">
	<h3 style="margin-top:10px;">Advertisements</h3>
	<div  style="text-align: center; padding: 12px 0">
		<script async src="//pagead2.googlesyndication.com/pagead/js/adsbygoogle.js"></script>
		<!-- Blog Leaderboard -->
		<ins class="adsbygoogle"
		     style="display:inline-block;width:728px;height:90px"
		     data-ad-client="x"
		     data-ad-slot="x"></ins>
		<script>
		    (adsbygoogle = window.adsbygoogle || []).push({});
		</script>
	</div>
</div>

iMattPro · 2017-08-09T13:56:43Z

@senky you should update your regex to only check for async in script tags that contain a src attribute I think

iMattPro · 2017-08-09T14:09:19Z

analyser/manager.php

+		);
+		foreach ($this->tests as $test)
+		{
+			$result = array_merge_recursive($result, $test->run($ad_code));


Note that this triggers an inspection warning that merging arrays in a loop causes high CPU usage and is a resources greedy construction. Perhaps there is an alternative approach?

https://github.com/kalessil/phpinspectionsea/blob/master/docs/performance.md#slow-array-function-used-in-loop

iMattPro · 2017-08-09T14:14:52Z

adm/style/manage_ads.html

+									{% endfor %}
+							{% endif %}
+						{% elseif CODE_ANALYSED %}
+							{{ lang('EVERYTHING_OK') }}


We might as well wrap this in a <p class="successbox""></p> so it looks like the other messages and makes users feel extra good about themselves.

iMattPro · 2017-08-09T14:19:41Z

adm/style/manage_ads.html

+						{% if loops.analyser_results_notice or loops.analyser_results_warning %}
+							<h2 class="centered-text">{{ lang('RESULTS') }}</h2>
+							{% if loops.analyser_results_notice %}
+								<h4>{{ lang('NOTICES') }}</h4>


I doubt we need the Results, Warnings and Notices headings anymore. The newly stylized message boxes should be enough, and IMO look a little cleaner without all the headings now.

phpbb-user added WIP and removed WIP labels Aug 6, 2017

iMattPro requested changes Aug 8, 2017

View reviewed changes

iMattPro reviewed Aug 8, 2017

View reviewed changes

iMattPro requested changes Aug 9, 2017

View reviewed changes

Introduce ad code analyser

611de30

iMattPro approved these changes Aug 9, 2017

View reviewed changes

iMattPro merged commit 451a88b into phpbb-extensions:master Aug 9, 2017

senky deleted the ad-code-analysis branch August 9, 2017 15:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ad snippet analysis #69

Ad snippet analysis #69

senky commented Aug 6, 2017

iMattPro Aug 8, 2017

iMattPro Aug 8, 2017

iMattPro Aug 8, 2017

iMattPro Aug 8, 2017

iMattPro Aug 8, 2017

iMattPro Aug 8, 2017

iMattPro Aug 8, 2017

iMattPro Aug 8, 2017

iMattPro Aug 8, 2017

iMattPro Aug 8, 2017

iMattPro Aug 8, 2017 •

edited

senky Aug 8, 2017

iMattPro commented Aug 8, 2017 via email

senky commented Aug 9, 2017

iMattPro commented Aug 9, 2017 •

edited

iMattPro commented Aug 9, 2017

iMattPro Aug 9, 2017

iMattPro Aug 9, 2017

iMattPro Aug 9, 2017

Ad snippet analysis #69

Ad snippet analysis #69

Conversation

senky commented Aug 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iMattPro Aug 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iMattPro commented Aug 8, 2017 via email

senky commented Aug 9, 2017

iMattPro commented Aug 9, 2017 • edited

iMattPro commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iMattPro Aug 8, 2017 •

edited

iMattPro commented Aug 9, 2017 •

edited