Page MenuHomePhabricator

Editing page ending with :number or going to such page via a namespace alias results in redirect to address with :number used as port
Closed, ResolvedPublicBUG REPORT

Description

List of steps to reproduce (step by step, including full links if applicable):

What happens?:

location: https://ua.wikimedia.org:2022/wiki/%D0%9E%D0%B1%D0%B3%D0%BE%D0%B2%D0%BE%D1%80%D0%B5%D0%BD%D0%BD%D1%8F:2022

What should have happened instead?:

Software version (if not a Wikimedia wiki), browser information, screenshots, other information, etc:

  • Firefox for Ubuntu 92.0 (64-bit)
  • 1.38.0-wmf.6 (f58fd66)

Event Timeline

Attempting to go to https://test.wikipedia.org/wiki/Project:1111 to reproduce also resulted in me ending up on https://test.wikipedia.org:1111/wiki/Wikipedia:1111 , so it is not about editing specifically.

But interestingly going directly to https://test.wikipedia.org/wiki/Wikipedia:1111 works fine. So the problem in this case happen only when a redirect is to happen anyway

Base renamed this task from Editing page with :numbers results in redirect to address with :number used as port to Editing page ending with :numbers or going to such page via a namespace alias results in redirect to address with :number used as port.Oct 28 2021, 1:28 PM
Base added a project: Platform Engineering.

Preemptively adding as a block until someone from releng has decided about it.

RhinosF1 triaged this task as Unbreak Now! priority.Oct 28 2021, 1:39 PM
RhinosF1 added a subscriber: RhinosF1.

Blocker = UBN!

Base renamed this task from Editing page ending with :numbers or going to such page via a namespace alias results in redirect to address with :number used as port to Editing page ending with :number or going to such page via a namespace alias results in redirect to address with :number used as port.Oct 28 2021, 1:40 PM
Base updated the task description. (Show Details)

Among recently merged patches, https://gerrit.wikimedia.org/r/c/mediawiki/core/+/728593 looks like it might be related. I haven't been able to reproduce locally to confirm that yet.

Mentioned in SAL (#wikimedia-operations) [2021-10-28T14:04:58Z] <twentyafterfour> rolling back group1 wikis to 1.38.0-wmf.5 (T293947) due to UBN T294559

I still can't reproduce the actual bug locally, but (using PHP 7.2.23):

$ php -a
Interactive shell

php > var_dump( parse_url( '//localhost/wiki/Foo:123' ) );
array(3) {
  ["host"]=>
  string(9) "localhost"
  ["port"]=>
  int(123)
  ["path"]=>
  string(13) "/wiki/Foo:123"
}

So that clearly looks like parse_url() still doesn't work correctly for protocol-relative URLs, and https://gerrit.wikimedia.org/r/c/mediawiki/core/+/728593 is almost certainly the cause of this bug.

Change 735331 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/core@master] Revert \"wfParseUrl: rely on parse_url for proto-relative urls\"

https://gerrit.wikimedia.org/r/735331

Change 735332 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/core@wmf/1.38.0-wmf.6] Revert \"wfParseUrl: rely on parse_url for proto-relative urls\"

https://gerrit.wikimedia.org/r/735332

Change 735331 merged by jenkins-bot:

[mediawiki/core@master] Revert \"wfParseUrl: rely on parse_url for proto-relative urls\"

https://gerrit.wikimedia.org/r/735331

Change 735332 merged by jenkins-bot:

[mediawiki/core@wmf/1.38.0-wmf.6] Revert \"wfParseUrl: rely on parse_url for proto-relative urls\"

https://gerrit.wikimedia.org/r/735332

Mentioned in SAL (#wikimedia-operations) [2021-10-28T19:36:16Z] <urbanecm@deploy1002> Synchronized php-1.38.0-wmf.6/includes/GlobalFunctions.php: b517ebd29396c23f66806cb0dca1d1b330c6e5be: Revert "wfParseUrl: rely on parse_url for proto-relative urls" (T294559) (duration: 01m 03s)

The revert seems to have resolved this issue.

mmodell claimed this task.
mmodell reassigned this task from mmodell to Urbanecm.

Change 735571 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/core@master] wfParseUrl: add regression test for T294559

https://gerrit.wikimedia.org/r/735571

I added a regression test that should protect against this issue in the future.

Can we make it best practice that a revert should always come with a regression test?

Change 735571 merged by jenkins-bot:

[mediawiki/core@master] wfParseUrl: add regression test for T294559

https://gerrit.wikimedia.org/r/735571

I added a regression test that should protect against this issue in the future.

Can we make it best practice that a revert should always come with a regression test?

Thanks for coming with the test, I appreciate that. My intention was to mitigate the issue ASAP to unblock the train, rather than figuring out a great solution that has tests.

I agree that tests should be made whenever possible. However, I also think that for UBNs, it should be acceptable to do whatever deemed necessary to mitigate the UBN, while leaving a note for other developers to follow up. In another words, I don't think it's okay to say "if you wish to revert sth to fix this UBN, you must also write regression tests".

I'm fine with saying a follow-up task must be opened, or the existing task only declassified as a blocker.

Would that be okay for you?

I added a regression test that should protect against this issue in the future.

Can we make it best practice that a revert should always come with a regression test?

Thanks for coming with the test, I appreciate that. My intention was to mitigate the issue ASAP to unblock the train, rather than figuring out a great solution that has tests.

Yes, this wasn't meant to criticize you. Fixing UBNs is always the priority, and ideally, such tests should be written by the author of the offending change, not by the person who is doing the revert.

I'm fine with saying a follow-up task must be opened, or the existing task only declassified as a blocker.

Would that be okay for you?

Sure! My comment was basically general musing. Doing it in-context here was probably not very helpful. Let's just keep it in mind... I currently don't have a specific idea of how this should work in the future. I just thing that things that caused a UBN should be covered by tests rather sooner than later. I didn't mean to imply that you should have done that.