Skip to content

Security enhancements#21

Open
mathetos wants to merge 1 commit intoProgressPlanner:mainfrom
mathetos:security-issue-no-20
Open

Security enhancements#21
mathetos wants to merge 1 commit intoProgressPlanner:mainfrom
mathetos:security-issue-no-20

Conversation

@mathetos
Copy link

Closes #20

Replaces unsafe redirects and unsanitized $_SERVER usage with WordPress-safe alternatives: wp_safe_redirect, wp_parse_url, and proper sanitization of $_SERVER input.

Context

The plugin used wp_redirect() with user-influenced URLs, parse_url() instead of wp_parse_url(), and unsanitized $_SERVER['REQUEST_URI'] and $_SERVER['HTTP_ACCEPT']. This could expose open redirect risks, inconsistent behavior across PHP versions, and potential misuse of untrusted input. This PR updates redirect handling and input sanitization to follow WordPress security and coding standards.

Summary

This PR can be summarized in the following changelog entry:

  • Security: Replace wp_redirect() with wp_safe_redirect(), use wp_parse_url() instead of parse_url(), sanitize $_SERVER['REQUEST_URI'] and $_SERVER['HTTP_ACCEPT'] with wp_unslash() and sanitize_text_field(), and use esc_url_raw() for redirect URLs.

Relevant technical choices:

  • wp_safe_redirect() instead of wp_redirect(): Limits redirect targets to same-origin URLs to reduce open redirect risk.
  • wp_parse_url() instead of parse_url(): Matches WordPress recommendations and behaves consistently across PHP versions.
  • sanitize_text_field(wp_unslash()) for $_SERVER: Aligns with WordPress Coding Standards and Plugin Check requirements for sanitizing superglobals.
  • esc_url_raw() for redirect URLs: Ensures URLs are validated and escaped before use in redirects.
  • Early return on invalid wp_parse_url() output: Handles false/null safely before use in regex.

Test instructions

  1. Visit a post’s .md URL (e.g. https://yoursite.com/post-slug.md) — markdown should be returned.
  2. Visit a post’s .md URL with a trailing slash (e.g. https://yoursite.com/post-slug.md/) — expect a 301 redirect to the URL without the slash.
  3. Send a request with Accept: text/markdown on a singular post URL — expect a 303 redirect to the .md URL.
  4. Verify that redirects target the correct URLs and that no new errors appear.

Relevant test scenarios

  • Changes should be tested with the browser console open
  • Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies — Use at least one post and one page.
  • Changes should be tested on different editors (Block/Classic/Elementor/other)
  • Changes should be tested on different browsers
  • Changes should be tested on multisite

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Documentation

  • I have written documentation for this change.

Quality assurance

  • I have tested this code to the best of my abilities.
  • I have added unit tests to verify the code works as intended.
  • I have checked that the base branch is correctly set.

Closes #20

1. Sanitize $_SERVER input
$_SERVER['REQUEST_URI']: now wrapped in sanitize_text_field(wp_unslash()) (in parse_markdown_url and handle_markdown_request)
$_SERVER['HTTP_ACCEPT']: same sanitization in handle_accept_negotiation
2. Replace parse_url() with wp_parse_url()
parse_markdown_url: path extracted with wp_parse_url(), with an early return if it returns false or null
handle_markdown_request: trailing-slash redirect path/query built using wp_parse_url()
3. Replace wp_redirect() with wp_safe_redirect()
Trailing-slash redirect: URL built with home_url(), validated with esc_url_raw(), then passed to wp_safe_redirect()
Accept header redirect: URL built and validated with esc_url_raw(), then passed to wp_safe_redirect() (303)
4. Use esc_url_raw() for URLs
Redirect URLs are passed through esc_url_raw() before being used
Accept header redirect returns early if esc_url_raw() returns an empty string
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security enhancements

1 participant

Comments