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

Sanitize custom permalink settings #5893

Closed
4 tasks done
webtrainingwheels opened this issue Apr 28, 2023 · 3 comments · Fixed by #5934
Closed
4 tasks done

Sanitize custom permalink settings #5893

webtrainingwheels opened this issue Apr 28, 2023 · 3 comments · Fixed by #5934
Assignees
Labels
effort: [XS] < 1 day of estimated development time module: filesystem priority: medium Issues which are important, but no one will go out of business. severity: critical Defect that prevents the testing/use of the software type: bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@webtrainingwheels
Copy link

webtrainingwheels commented Apr 28, 2023

Before submitting an issue please check that you’ve completed the following steps:

  • Made sure you’re on the latest version
  • Used the search feature to ensure that the bug hasn’t been reported before

Describe the bug
Since version 3.13 we are using the permalink structure in $rocket_permalink_structure
If the customer is using a custom structure with a weird input, it can cause a fatal error, because we don't sanitize this.
Example of an input that causes problems: /'/%year%/%monthnum%/%day%/%postname%/'
https://i.imgur.com/2rZIyea.png

To Reproduce
Steps to reproduce the behavior:

  1. Go to Settings → Permalinks. Choose custom structure and enter: /'/%year%/%monthnum%/%day%/%postname%/'
  2. Activate WP Rocket
  3. Check the wp-rocket-config file and you will see:
    $rocket_permalink_structure = '/'/%year%/%monthnum%/%day%/%postname%/'';
  4. Go to the frontend of the site and you will have a critical error.
  5. In the error log you will see: Parse error: syntax error, unexpected token "%" in /wp-content/wp-rocket-config/yourdomain.php on line 82

Expected behavior
If WordPress accepts a permalink structure, we should be able to as well. We should sanitize this so it doesn't cause an error, even if it's an edge case.

Screenshots
If applicable, add screenshots to help explain your problem.

Additional context
Ticket: https://secure.helpscout.net/conversation/2207611833/412789?folderId=377611
Slack:https://wp-media.slack.com/archives/C43T1AYMQ/p1682465607573189

Backlog Grooming (for WP Media dev team use only)

  • Reproduce the problem
  • Identify the root cause
  • Scope a solution
  • Estimate the effort
@piotrbak piotrbak added type: bug Indicates an unexpected problem or unintended behavior priority: medium Issues which are important, but no one will go out of business. module: filesystem severity: critical Defect that prevents the testing/use of the software labels May 6, 2023
@piotrbak
Copy link
Contributor

piotrbak commented May 6, 2023

@wp-media/php What do you think about that sanitization? Do you see this one risky?

@Tabrisrp
Copy link
Contributor

We can escape the ' character before adding the structure to the file, this should avoid the error. We can test with that solution if the behaviour is correctly preserved.

@jeawhanlee jeawhanlee added GROOMING IN PROGRESS Use this label when the issue is currently being groomed. and removed needs: grooming labels May 19, 2023
@jeawhanlee
Copy link
Contributor

Reproduce the problem ✅

I was able to reproduce the problem.

Identify the root cause ✅

Unescaped quotes in the config file.

Scope a solution ✅

Like @Tabrisrp proposed, we can escape these kind of quotes before adding the permalink structure to the config file
on this line

$buffer .= '$rocket_permalink_structure = \'' . get_option( 'permalink_structure' ) . "';\n";
:
we can escape like this wp_slash( get_option( 'permalink_structure' ) )

Though for a structure like this we will not be redirecting with a trailing slash.

Estimate the effort ✅

[XS]

@jeawhanlee jeawhanlee added effort: [XS] < 1 day of estimated development time and removed GROOMING IN PROGRESS Use this label when the issue is currently being groomed. waiting for feedback labels May 19, 2023
@jeawhanlee jeawhanlee self-assigned this May 19, 2023
@piotrbak piotrbak added this to the 3.13.4 milestone May 24, 2023
@engahmeds3ed engahmeds3ed mentioned this issue Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [XS] < 1 day of estimated development time module: filesystem priority: medium Issues which are important, but no one will go out of business. severity: critical Defect that prevents the testing/use of the software type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants