WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 15 months ago

#39669 closed defect (bug) (wontfix)

Appearance/Menu, Custom Link: bad URL value sanitation

Reported by: TRILOS Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Menus Keywords:
Focuses: Cc:

Description

When adding Custom Link with URL value
../
and saving the menu, WP changes the URL to
http://../

Referring to the parent directory is therefore impossible.

As this IMHO is a valid "URL" for referring the parent directory, there´s no need to (I guess) "sanitize" the field value into a "valid" URL (despite this, "http://../" is not even a valid URL).

Same with ../dirname/ => http://../dirname/

Change History (9)

#1 @bisyarin
3 years ago

Hello, @TRILOS! Could you write how to reproduce it step by step?

On my local server this link 'http://wp/../' transforms to 'http://wp/1011-2', where '1011' is the id of the page, but on update it is saved with a slug named after the title of a page. It's also not the behavior I expect, but simultaneously not the one you described.

#2 @TRILOS
3 years ago

  • Version changed from 4.7.1 to 4.7.2

Thank you @bisyarin for the addition.

Here comes a desription:

  1. Edit a menu in Appearance>Menus
  2. Add a "Custom Link", insert "../" into "URL" field
  3. Klick "Add to menu"
  4. Review "Menu Structure": Custom Link URL field contains "http://../".

Note: Same with ../dirname/ => http://../dirname/

This ticket was mentioned in Slack in #core by trilos. View the logs.


3 years ago

#4 @TRILOS
3 years ago

  • Keywords needs-testing added

3 months without review, is this usual?
Issue is still a bug in 4.7.4 - how to change the affected version of this ticket?

#5 @welcher
3 years ago

  • Keywords needs-testing removed
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed
  • Version changed from 4.7.2 to 4.7.4

@TRILOS thank you for submitting this and welcome!

Please keep in mind that many of the people who contribute to WordPress Core do so both for free and in their spare time. We can't always get to tickets as quickly as we would like :)

This feels like this is an edge case and I am unsure of the practical use here. If the intention is to link to an asset on the server, then that can be accomplished by uploading said asset to the media library and using the link provided in the Custom Menu item. Is there a use-case that cannot be addressed by using an absolute url?

My biggest concern here is security. We'd need to bypass the esc_url call in the Walker class to allow these types of URLs that is in place to make sure the URL is safe.

#6 @welcher
3 years ago

  • Version 4.7.4 deleted

#7 follow-up: @TRILOS
3 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

Thank you for these informations. In the case I noted I had no media file to refer to, but a relative path: I wanted to link to a similar index page in a different language, placed in a path of e.g. /en/ instead of /de/. So if a WP component isn´t able to check this on safety, then it should be able and it is no edge case, but a essential case of handling syntacical correct URLs, isn´t it?

Last edited 3 years ago by TRILOS (previous) (diff)

#8 @SergeyBiryukov
2 years ago

  • Milestone set to Awaiting Review

#9 in reply to: ↑ 7 @welcher
15 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

Replying to TRILOS:

Thank you for these informations. In the case I noted I had no media file to refer to, but a relative path: I wanted to link to a similar index page in a different language, placed in a path of e.g. /en/ instead of /de/. So if a WP component isn´t able to check this on safety, then it should be able and it is no edge case, but a essential case of handling syntacical correct URLs, isn´t it?

Allowing for relative paths will require that we bypass the esc_url call and by doing so, creates a security vulnerability by allowing a user to input anything into that field.

A relative path such as ../my-translated-page could be an actual page in WordPress or potentially a directory that contains a malicious script. The code cannot determine the intent of the URL being input.

Closing as #wont-fix

Note: See TracTickets for help on using tickets.