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

Bug: Improve s2Member .htaccess File Writing Behaviour #832

Closed
renzms opened this Issue Jan 5, 2016 · 12 comments

Comments

Projects
None yet
5 participants
@renzms
Contributor

renzms commented Jan 5, 2016

Overview

Currently s2Member will automatically attempt to add this code to the .htaccess file at the web root.

# BEGIN s2Member GZIP exclusions 
<IfModule rewrite_module> 
RewriteEngine On 
RewriteBase / 
RewriteCond %{QUERY_STRING} (^|\?|&)s2member_file_download\=.+ [OR] 
RewriteCond %{QUERY_STRING} (^|\?|&)no-gzip\=1 
RewriteRule .* - [E=no-gzip:1] 
</IfModule> 
# END s2Member GZIP exclusions

This disables GZIP compression when s2Member attempts to deliver files. GZIP compression must be disabled during s2Member's attempt to deliver a file via PHP, because files that s2Member delivers via PHP are already compressed. Thus, re-compressing files delivered by a script will corrupt them in your browser.

Possible Improvements

  • s2Member currently does not obey the DISALLOW_FILE_MODS constant
  • s2Member currently does not have any hooks to allow a site owner to specifically disable s2Member writing to .htaccess.

referenced in this internal ticket: https://websharks.zendesk.com/agent/tickets/10358

@JeffStarr

This comment has been minimized.

JeffStarr commented Jan 5, 2016

It would be great to get a hook or something to disable the auto .htaccess rules for configurations where they are not needed. Thank you renzms and s2Member team.

@raamdev raamdev added the bug label Jan 5, 2016

@raamdev

This comment has been minimized.

Contributor

raamdev commented Jan 5, 2016

Labeling this a bug since s2Member does not currently obey the DISALLOW_FILE_MODS constant as it should.

@raamdev raamdev added this to the Next Release milestone Jan 5, 2016

@jaswrks

This comment has been minimized.

Member

jaswrks commented Jan 6, 2016

@kristineds Can you please outline this issue for @renzms also?

See: this block of code. Both of those methods should return FALSE when if(defined('DISALLOW_FILE_MODS') && DISALLOW_FILE_MODS){ } is the case.

Referencing: http://codex.wordpress.org/Hardening_WordPress#Disable_File_Editing

@renzms renzms changed the title from Improve s2Member .htaccess File Writing Behaviour to Bug: Improve s2Member .htaccess File Writing Behaviour Jan 7, 2016

@kristineds

This comment has been minimized.

Contributor

kristineds commented Jan 7, 2016

@jaswsinc Taking a stab at this one cc @renzms


Next Actions

  • New feature branch: feature/832 in the websharks/s2member repo.
  • Replace this block of code with the following:
  if(defined('DISALLOW_FILE_MODS') && DISALLOW_FILE_MODS)
  {
    if(c_ws_plugin__s2member_files::remove_no_gzip_from_root_htaccess() /* Must first be able to remove any existing entry. */)
    {
      $start_line              = '# BEGIN s2Member GZIP exclusions'; // Beginning line for this entry.
      $end_line                = '# END s2Member GZIP exclusions'; // Identifying end line for this entry.
      $htaccess                = ABSPATH.'.htaccess'; // Location of this `.htaccess` file we need to write in.
      $ideally_position_before = '# BEGIN WordPress'; // Ideally, we can position before this entry.

      $no_gzip = $start_line."\n".trim(c_ws_plugin__s2member_utilities::evl(file_get_contents($GLOBALS['WS_PLUGIN__']['s2member']['c']['files_no_gzip_htaccess'])))."\n".$end_line;

      if(file_exists($htaccess) && is_readable($htaccess) && is_writable($htaccess) && ($htaccess_contents = file_get_contents($htaccess)) !== FALSE && is_string($htaccess_contents = trim($htaccess_contents)))
      {
        if(stripos($htaccess_contents, $ideally_position_before) !== FALSE /* If we can position in the ideal location, that's awesome. Let's do that now. */)
          $htaccess_contents = trim(str_ireplace($ideally_position_before, $no_gzip."\n\n".$ideally_position_before, $htaccess_contents));

        else $htaccess_contents = trim($no_gzip."\n\n".$htaccess_contents); // Else, let's put it at the very top of the file by default.

        return file_put_contents($htaccess, $htaccess_contents);
      }
      else if(!file_exists($htaccess) && is_writable(dirname($htaccess)))
      {
        return file_put_contents($htaccess, $no_gzip);
      }
    }
  }
  return FALSE; // Default return `FALSE`.
  if(defined('DISALLOW_FILE_MODS') && DISALLOW_FILE_MODS)
  {
    if(file_exists($htaccess) && is_readable($htaccess) && is_writable($htaccess) && ($htaccess_contents = file_get_contents($htaccess)) !== FALSE && is_string($htaccess_contents = trim($htaccess_contents)))
    {
      $htaccess_contents = trim(preg_replace('/'.preg_quote($start_line, '/').'['."\r\n".']+.*?['."\r\n".']+'.preg_quote($end_line, '/').'['."\r\n".']{0,2}/is', '', $htaccess_contents));

      return (file_put_contents($htaccess, $htaccess_contents) !== FALSE); // Check for `FALSE`, because this could return `0` if the file is now empty.
    }
    else if(!file_exists($htaccess) /* Return `TRUE` here, we're OK. */)
    {
      return TRUE;
    }
  }
  return FALSE; // Default return `FALSE`.
  • Submit PR.
@jaswrks

This comment has been minimized.

Member

jaswrks commented Jan 8, 2016

@kristineds Looks good!

Another way to do this would be:

if(defined('DISALLOW_FILE_MODS') && DISALLOW_FILE_MODS) {
      return FALSE; # Stop here. No write access on this site.
}
@raamdev

This comment has been minimized.

Contributor

raamdev commented Jan 9, 2016

@kristineds writes...

if(defined('DISALLOW_FILE_MODS') && DISALLOW_FILE_MODS)

That says "if DISALLOW_FILE_MODS is defined and DISALLOW_FILE_MODS is true, then do this", which I don't think is what you intended (if file mods are disabled, we don't want to do anything to the .htaccess file--your code is only modifying the htaccess when we've disabled file mods, which is not right).

@jaswrks

This comment has been minimized.

Member

jaswrks commented Jan 9, 2016

Aha. Thanks @raamdev :-) Totally missed that.

@kristineds

This comment has been minimized.

Contributor

kristineds commented Jan 15, 2016

@jaswsinc @raamdev Like this? cc @renzms


Next Actions

  • New feature branch: feature/832 in the websharks/s2member repo.
  • After this line and this line add the following:
if(defined('DISALLOW_FILE_MODS') && DISALLOW_FILE_MODS) {
      return FALSE; # Stop here. No write access on this site.
}
  • Submit PR.
@jaswrks

This comment has been minimized.

Member

jaswrks commented Jan 18, 2016

@kristineds See: #832 (comment)

Your code sample is:

  • Missing a round bracket, which will result in a parse error.
  • It returns TRUE instead of false; i.e., if we are not allowed, the function should stop and return FALSE to indicate that it was unable to perform the task requested.
  • You should check not only if the DISALLOW_FILE_MODS constant is defined, but also check if it's set to a true or false value.
@kristineds

This comment has been minimized.

Contributor

kristineds commented Jan 18, 2016

@jaswsinc I've updated the outline ⬆️ to use the one you suggested here: #832 (comment)

Updated Next Action here cc @renzms

@renzms

This comment has been minimized.

Contributor

renzms commented Jan 21, 2016

@jaswsinc @raamdev @kristineds

PR submitted thanks!

jaswrks pushed a commit that referenced this issue Jan 24, 2016

@jaswrks

This comment has been minimized.

Member

jaswrks commented Jan 24, 2016

Next Release Changelog:

  • (s2Member/s2Member Pro) Bug Fix: s2Member was not properly respecting DISALLOW_FILE_MODS in a specific scenario related to GZIP. Props @renzms @kristineds. See also: this GitHub issue for further details.

@jaswrks jaswrks closed this Jan 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment