Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Bypass basic auth or change error message #174

Open
fumikito opened this issue Aug 13, 2024 · 2 comments
Open

Bypass basic auth or change error message #174

fumikito opened this issue Aug 13, 2024 · 2 comments
Labels
type:enhancement New feature or request.

Comments

@fumikito
Copy link

At this part, the plugin is self-remote-requesting to check if ads.txt is accessible via the plugin filter by confirming X-Ads-Txt-Generator header information. In a success, file_exists property is false.

ads-txt/inc/admin.php

Lines 547 to 589 in 176d28d

/**
* Check if ads.txt file already exists in the server
*
* @return void
*/
function adstxts_check_for_existing_file() {
current_user_can( ADS_TXT_MANAGE_CAPABILITY ) || die;
check_admin_referer( 'adstxt_save' );
$home_url_parsed = wp_parse_url( home_url() );
$adstxt_type = sanitize_text_field( $_POST['adstxt_type'] );
if ( 'adstxt' !== $adstxt_type && 'app-adstxt' !== $adstxt_type ) {
wp_die();
}
$file_name = 'adstxt' === $adstxt_type ? '/ads.txt' : '/app-ads.txt';
if ( empty( $home_url_parsed['path'] ) ) {
$response = wp_remote_request( home_url( $file_name ) );
$file_exist = false;
if ( ! is_wp_error( $response ) ) {
// 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;
}
// Return the response
wp_send_json(
[
'success' => true,
'file_exist' => $file_exist,
]
);
// Make sure to exit
wp_die();
}
}
add_action( 'wp_ajax_adstxts_check_for_existing_file', __NAMESPACE__ . '\adstxts_check_for_existing_file' );

On front-end part in admin screen, js/admin.js checks if response.file_exist property and display message.

ads-txt/js/admin.js

Lines 31 to 36 in 176d28d

if ( ! response.file_exist ) {
// Ads.txt not found
$( '.existing-adstxt' ).hide();
} else {
$( '.existing-adstxt' ).show();
}

Toggle message(therefore static) is here:

ads-txt/inc/admin.php

Lines 303 to 308 in 176d28d

<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>

By the way, self-remote-request can fail in other case than existing ads.txt.

  • Server error (50x)
  • WordPress is fully under Basic Auth(e.g. a staging site and this is my case)
  • CDN dropping additional header.

To bypassing Basic Auth, you can pass auth credentials for wp_remote_request() from Ajax request header. This saves me, but somehow selfish request, I guess.

So, I request some changes on the plugins.

  • Return proper error message(server problem, auth required, and do on)
  • Display error messages on front end.
  • Check file is really existent(e.g. file_exists( 'path_to_file' ) )

I wish send PR if welcomed.
Thanks!

@vikrampm1 vikrampm1 added the type:enhancement New feature or request. label Aug 13, 2024
@peterwilsoncc
Copy link
Contributor

Due to the variety of systems the plugin can run on, I don't think it's possible to switch to a file_exists() checks. The use of a server side HTTP request was added in PR #131 to fix the problems described in issue #48.

If the issue of CDNs stripping the X-Ads-Txt-Generator becomes a common issue, then I think that can be handled in a separate ticket.

I think there are two options to consider in the event the response is not a 200 or 404:

  • fail silently: simply don't show the message in the event the response is not one that's expected
  • show a notice indicating that the presence or absence of the file is not able to be determined.

@fumikito As you've experienced this issue, which of the two would you find most helpful while using the plugin?

@fumikito
Copy link
Author

@peterwilsoncc Thanks for the reply!

Reading #131 and #48, they are curious and I've found that the current structure is reasonable. People use WordPress in various situations.

About your 2 solutions, I prefer latter:

show a notice indicating that the presence or absence of the file is not able to be determined.

The problem I've experienced is that the plugin told about the existence of a physical ads.txt, but actually no one.
So, preventing such kind of confusion, changing message satisfies me(and maybe other users)

The message could be...

The plugin could not retrieve the ads.txt file. There may be several reasons for this:

  • The ads.txt file is placed on the server. This plugin generates a virtual ads.txt file, but it requires that no physical ads.txt file exists.
  • There are special restrictions on server access, such as Basic Authentication or a CDN. In this case, the ads.txt file may be displayed correctly. Please try accessing the ads.txt file from your browser to check if it contains the expected content.

Such kind of an allusion can make up for the situation blow:

If the issue of CDNs stripping the X-Ads-Txt-Generator becomes a common issue, then I think that can be handled in a separate ticket.

Hope my comment helps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New feature or request.
Projects
Status: Backlog
Development

No branches or pull requests

3 participants