Ads.txt file exists check from backend side #131

sksaju · 2023-03-06T14:37:45Z

Description of the Change

This PR fixes the #48 issue, by adding ads.txt file check from the backend side

Closes #

How to test the Change

Put an ads.txt in the root of a MS instance
Set up domain mapping for a subsite
Go to the ads.txt edit screen on the subsite
See notice that an existing ads.txt file was found

Changelog Entry

Added - ads.txt file exists check from backend side

Credits

Props @sksaju, @mmcachran, @peterwilsoncc, @dinhtungdu, @helen, @jeffpaul.

Checklist:

I agree to follow this project's Code of Conduct.
I have updated the documentation accordingly.
I have added tests to cover my change.
All new and existing tests pass.

peterwilsoncc · 2023-03-07T02:37:32Z

inc/admin.php

@@ -269,6 +269,14 @@ function settings_screen( $post_id, $strings, $args ) {
 	}
 	?>
 <div class="wrap">
+	<?php if ( file_exists( ABSPATH . '/ads.txt' ) ) { ?>


This check won't work I'm afraid.

ABSPATH points to the directory of the WordPress install, which isn't necessarily the same as the root directory of the website. See this guide for running wordpress in a subdirectory.

To use my site as an example, this would check for /path/to/files/wp/ads.txt rather than /path/to/files/ads.txt.

I think there would be some weirdness around multisite installs too.

Yeah, that makes sense, I'll try to fix the issue

I added a quick comment with some pseudo code on the original issue #48 (comment)

@peterwilsoncc I'm trying to check the file following your suggestions but somehow when I try to get the file via wp_remote_request request I'm getting empty headers data, I've pushed the latest changes on this branch, could you take a look, please? Thanks

I'm unable to reproduce an error with empty headers, this is what I am seeing in xdebug using the file_exists line as a break point.

Thanks @peterwilsoncc I was able to test header data today

This is the header response from ads.txt file

and this from adstxt post type

Can we check the file with the Content-Length and/or ETag properties as these are not present in the post-type response? although I don't have much experience with online server file systems, I think these might help to differentiate between file response and post-type response! please let me know your feedback. Thanks

jeffpaul · 2023-04-10T21:17:24Z

@sksaju there appears to be a merge conflict, mind resolving that and letting us know if this is ready for re-review? Thanks!

jeffpaul · 2023-04-26T18:09:59Z

@peterwilsoncc back to you for review here

peterwilsoncc

I've added some notes inline. I'm not sure why the E2E tests are failing.

peterwilsoncc · 2023-05-07T23:53:44Z

inc/admin.php

+			// Get the headers of the response
+			$headers      = wp_remote_retrieve_headers( $response );
+			$content_type = isset( $headers['content-type'] ) ? $headers['content-type'] : '';
+			$file_exist   = strpos( $content_type, 'application/octet-stream' ) !== false;


While testing, I was getting a false report here (I had an ads.txt file but it wasn't detected).

How about the adding header( 'X-Ads-Txt-Generator: https://wordpress.org/plugins/ads-txt/' ); to the output in the main file (both for ads.txt and for app-ads.txt) and then checking for the header:

Suggested change

// Get the headers of the response

$headers = wp_remote_retrieve_headers( $response );

$content_type = isset( $headers['content-type'] ) ? $headers['content-type'] : '';

$file_exist = strpos( $content_type, 'application/octet-stream' ) !== false;

// Check the ads.txt generator header.

$headers = wp_remote_retrieve_headers( $response );

$generator = isset( $headers['X-Ads-Txt-Generator'] ) ? $headers['X-Ads-Txt-Generator'] : '';

$file_exist = 'https://wordpress.org/plugins/ads-txt/' !== $generator;

The generator header would need to be included regardless of whether the post exists or not, so probably right after the get_option() check in each block.

peterwilsoncc · 2023-05-08T00:08:07Z

inc/admin.php

+function adstxts_check_for_existing_file() {
+	$home_url_parsed = wp_parse_url( home_url() );
+	if ( empty( $home_url_parsed['path'] ) ) {
+		$response   = wp_remote_request( home_url( '/ads.txt' ) );


We'll need something here (and in the JS) to determine whether the check should be for ads.txt or app-ads.txt .

You can probably include something in the admin_enqueue_scripts() function within this file based on the hook -- whether it contains app-adstxt or not.

To be secure you should send it in the form is_appstxt: true|false and modify the URL check based on the true false statement. Don't use the variable home_url( $passed_value ) as someone could modify that to a random file name, such as .env or similar.

peterwilsoncc · 2023-05-08T00:19:21Z

inc/admin.php

+ * @return void
+ */
+function adstxts_check_for_existing_file() {
+	$home_url_parsed = wp_parse_url( home_url() );


Security: This will need both a nonce and a capability check, similar to that used in the save function:

ads-txt/inc/save.php

Lines 19 to 20 in dc18245

current_user_can( ADS_TXT_MANAGE_CAPABILITY ) || die;

check_admin_referer( 'adstxt_save' );

peterwilsoncc · 2023-05-08T00:20:58Z

inc/admin.php

+
+		// Make sure to exit
+		wp_die();


wp_send_json() exits.

Suggested change

// Make sure to exit

wp_die();

peterwilsoncc · 2023-05-08T01:04:43Z

inc/admin.php

+	<div class="notice notice-error adstxt-notice existing-adstxt" style="display: none;">
+		<p><strong><?php echo esc_html( $strings['existing'] ); ?></strong></p>
+		<p><?php echo esc_html( $strings['precedence'] ); ?></p>
+
+		<p><?php echo esc_html_e( 'Removed the existing file but are still seeing this warning?', 'ads-txt' ); ?> <a class="ads-txt-rerun-check" href="#"><?php echo esc_html_e( 'Re-run the check now', 'ads-txt' ); ?></a> <span class="spinner" style="float:none;margin:-2px 5px 0"></span></p>
+	</div>


Follow up: it's the presence of this that is causing the tests to fail. I pushed a fix in 5d9f27d

…apped-domains-into-account

peterwilsoncc

This looks good to me, thank you for your patience during the review process.

Testing notes:

identical content in a real file and the generated version show the warning
different content in a real file and the generated version show the warning
the generated file includes the custom header
file not found errors include the custom header
accounts for built in Multisite domain mapping

added ads.txt file exists check from backend

d6a192e

sksaju requested a review from peterwilsoncc as a code owner March 6, 2023 14:37

fixe phplint issue

ece21bb

jeffpaul added this to the 1.5.0 milestone Mar 6, 2023

jeffpaul assigned sksaju Mar 6, 2023

jeffpaul linked an issue Mar 6, 2023 that may be closed by this pull request

Ads.txt file detection does not take mapped domains into account #48

Closed

peterwilsoncc reviewed Mar 7, 2023

View reviewed changes

sksaju added 4 commits April 3, 2023 00:38

revert changes

f8b7a9f

added backend function to check ads.txt file exist or not

9cae7fb

remove https_ssl_verify filter

ce9ac90

fixed php lint issue

55ddc7a

fixed merge conflicts

6eb4229

sksaju requested a review from peterwilsoncc April 26, 2023 15:11

peterwilsoncc reviewed May 8, 2023

View reviewed changes

Add specific class for the save errors.

5d9f27d

peterwilsoncc reviewed May 8, 2023

View reviewed changes

sksaju and others added 3 commits June 3, 2023 20:48

implement CR feedback from peterwilsoncc

9e461bc

fixed phpcs issue

0d178ec

Merge branch 'develop' into fix/adstxt-file-detection-does-not-take-m…

ac066da

…apped-domains-into-account

peterwilsoncc approved these changes Jun 5, 2023

View reviewed changes

peterwilsoncc merged commit da31148 into develop Jun 5, 2023

peterwilsoncc deleted the fix/adstxt-file-detection-does-not-take-mapped-domains-into-account branch June 5, 2023 01:50

This was referenced Jun 5, 2023

Ads.txt file detection does not take mapped domains into account #48

Closed

Fix issue with mapped domains. #53

Closed

dkotter modified the milestones: 1.5.0, 1.4.3 Jun 16, 2023

peterwilsoncc mentioned this pull request Aug 14, 2024

Bypass basic auth or change error message #174

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ads.txt file exists check from backend side #131

Ads.txt file exists check from backend side #131

sksaju commented Mar 6, 2023 •

edited by peterwilsoncc

Loading

peterwilsoncc Mar 7, 2023

sksaju Mar 7, 2023

peterwilsoncc Mar 8, 2023

sksaju Apr 2, 2023

peterwilsoncc Apr 3, 2023

sksaju Apr 26, 2023 •

edited

Loading

jeffpaul commented Apr 10, 2023

jeffpaul commented Apr 26, 2023

peterwilsoncc left a comment

peterwilsoncc May 7, 2023

peterwilsoncc May 8, 2023

peterwilsoncc May 8, 2023

peterwilsoncc May 8, 2023

peterwilsoncc May 8, 2023

peterwilsoncc left a comment

	current_user_can( ADS_TXT_MANAGE_CAPABILITY ) \|\| die;
	check_admin_referer( 'adstxt_save' );

Ads.txt file exists check from backend side #131

Ads.txt file exists check from backend side #131

Conversation

sksaju commented Mar 6, 2023 • edited by peterwilsoncc Loading

Description of the Change

How to test the Change

Changelog Entry

Credits

Checklist:

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

sksaju Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

jeffpaul commented Apr 10, 2023

jeffpaul commented Apr 26, 2023

peterwilsoncc left a 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

peterwilsoncc left a comment

Choose a reason for hiding this comment

sksaju commented Mar 6, 2023 •

edited by peterwilsoncc

Loading

sksaju Apr 26, 2023 •

edited

Loading