Skip to content

Comments

Update class-srm-redirect.php to check $_SERVER['REQUEST_URI']#427

Open
turtlepod wants to merge 2 commits intodevelopfrom
fix/check-request-uri-isset
Open

Update class-srm-redirect.php to check $_SERVER['REQUEST_URI']#427
turtlepod wants to merge 2 commits intodevelopfrom
fix/check-request-uri-isset

Conversation

@turtlepod
Copy link
Member

@turtlepod turtlepod commented Oct 24, 2025

Description of the Change

This PR fixes a PHP error reported by the WordPress coding standards:

Error (severity 5): Detected usage of a possibly undefined superglobal array index: $_SERVER['REQUEST_URI'].
Use isset() or ! empty() to check if the index exists before using it.
(WordPress.Security.ValidatedSanitizedInput.InputNotValidated)

The issue was caused by incorrect logic when checking $_SERVER['REQUEST_URI'].
This update ensures the value is validated before use, preventing potential PHP notices and improving code safety.

Benefits

  • Prevents PHP “undefined index” warnings.
  • Improves code reliability and security by ensuring proper input validation.
  • Maintains compliance with WordPress coding standards.

How to Test the Change

  1. Test any functionality that relies on $_SERVER['REQUEST_URI'] (e.g., URL-based logic or redirects).
  2. Verify that no PHP notices or warnings appear in the error logs.
  3. Confirm that all affected features continue to function as expected.

Changelog Entry

Fixed – Added a proper check for $_SERVER['REQUEST_URI'] to prevent undefined index errors and improve input validation.
(WordPress.Security.ValidatedSanitizedInput.InputNotValidated)


Credits

Props @turtlepod


Checklist

  • I agree to follow this project's Code of Conduct.
  • I have tested the changes and confirmed no regressions.
  • I have ensured compliance with WordPress coding standards.
  • All new and existing tests pass.

❌ Error( severity 5 ): Detected usage of a possibly undefined superglobal array index: $_SERVER['REQUEST_URI']. Use isset() or empty() to check the index exists before using it (WordPress.Security.ValidatedSanitizedInput.InputNotValidated).
@turtlepod turtlepod requested a review from Copilot October 24, 2025 00:39
@github-actions github-actions bot added this to the 2.3.0 milestone Oct 24, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR addresses a WordPress coding standards violation by adding proper validation for the $_SERVER['REQUEST_URI'] superglobal before accessing it.

  • Added isset() check using null coalescing operator for $_SERVER['REQUEST_URI']

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@turtlepod turtlepod requested a review from Copilot October 24, 2025 00:45
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@turtlepod turtlepod marked this pull request as ready for review October 24, 2025 00:58
@github-actions github-actions bot added the needs:code-review This requires code review. label Oct 24, 2025
@peterwilsoncc
Copy link
Contributor

peterwilsoncc commented Oct 24, 2025

Coincidently I was looking at this a few minutes ago. WordPress ensures the value is populated in the wp_fix_server_vars(), so I think these are a false positive.

Let me know if I am missing something...

@turtlepod
Copy link
Member Author

@peterwilsoncc hmm.. interesting.
but the initial code

sanitize_text_field( ... ) ?? ''

is a valid bug/incorrect code.

@jeffpaul
Copy link
Member

@peterwilsoncc note that @turtlepod has some OSS time this/next week, so if there's an update needed here please share

* @returns {string} Request path.
*/
$requested_path = esc_url_raw( apply_filters( 'srm_requested_path', sanitize_text_field( wp_unslash( $_SERVER['REQUEST_URI'] ) ) ?? '' ) );
$requested_path = esc_url_raw( apply_filters( 'srm_requested_path', sanitize_text_field( wp_unslash( $_SERVER['REQUEST_URI'] ?? '' ) ) ) );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$requested_path = esc_url_raw( apply_filters( 'srm_requested_path', sanitize_text_field( wp_unslash( $_SERVER['REQUEST_URI'] ?? '' ) ) ) );
$requested_path = esc_url_raw( apply_filters( 'srm_requested_path', sanitize_text_field( wp_unslash( $_SERVER['REQUEST_URI'] ) ) ) );

I think the null coalescence can be removed all together as it's not possible for the value to be so in WP due to these lines.

https://github.com/WordPress/wordpress-develop/blob/f7bd33addcc31c568b51dbb4c04a37875f917d91/src/wp-includes/load.php#L37-L42

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think that's a good idea. even when it's not needed. it's still getting flagged by wp coding standard.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the sniff because the global may not be defined, if so I think we can use a phpcs ignore rather than add code that won't be hit.

How about a phpcs:ignore WordPress.Sniff.Name -- see class-simple-local-avatars.php()

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

Labels

needs:code-review This requires code review.

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

3 participants