PCS is raising errors related to the request method when querying rest.php
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Jgiannelos
	Mar 6 2024, 10:46 AM

Description

After sending a patch for PCS jenkins failed with:

eg
GET /test.wikipedia.org/v1/page-mobile-html/Dog

The request method does not accept a request body

I managed to reproduce this locally and it looks like the problem is the case of request method when sending requests to rest.php
Error dissapears when I replace method with GET instead of get.

Details

Subject	Repo	Branch	Lines +/-
REST: allow lower-case method names	mediawiki/core	wmf/1.42.0-wmf.21	+16 -5
REST: allow lower-case method names	mediawiki/core	master	+16 -5
mobileapps: Use upper case method names for rest.php requests	operations/deployment-charts	master	+2 -2
CI: Use uppercase for /rest.php request method	mediawiki/services/mobileapps	master	+2 -2

Customize query in gerrit

Related Objects

Mentioned In: T359516: cxserver not able to load any page
T359509: REST API calls suddenly all returning 400
Mentioned Here: T359216: [testwiki - wmf.21] Homepage: Bad request for page/summary and user-impact

Event Timeline

Jgiannelos created this task.Mar 6 2024, 10:46 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 6 2024, 10:46 AM

Change 1009214 had a related patch set uploaded (by Jgiannelos; author: Jgiannelos):

[mediawiki/services/mobileapps@master] CI: Use uppercase for /rest.php request method

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

gerritbot added a project: Patch-For-Review.Mar 6 2024, 10:47 AM

Change 1009224 had a related patch set uploaded (by Jgiannelos; author: Jgiannelos):

[operations/deployment-charts@master] mobileapps: Use upper case method name for requests to rest.php

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

Jgiannelos changed the task status from Open to In Progress.Mar 6 2024, 10:50 AM

Jgiannelos triaged this task as High priority.

Jgiannelos added a subscriber: daniel.

Jgiannelos updated the task description. (Show Details)Mar 6 2024, 10:59 AM

Change 1009214 merged by jenkins-bot:

[mediawiki/services/mobileapps@master] CI: Use uppercase for /rest.php request method

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

Jgiannelos moved this task from Backlog to Code Review on the Content-Transform-Team-WIP board.Mar 6 2024, 11:24 AM

Oh, that shouldn't be needed. The framework should normalized that. I'll make a fix.

I didn't realize that lower case method names are actually a violation of the HTTP spec: https://stackoverflow.com/questions/10766221/is-serverrequest-method-guaranteed-to-be-uppercase

Anyway, I think we should be try and be robust against this issue.

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

[mediawiki/core@master] REST: allow lower-case method names

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

FWIW it used to work before in our test suite so there must be a change meanwhile that caused this issue.
From Jenkins, the last successful run was on Feb 26.

In T359306#9605833, @Jgiannelos wrote:

FWIW it used to work before in our test suite so there must be a change meanwhile that caused this issue.
From Jenkins, the last successful run was on Feb 26.

Yes, we are now checking that methods that need a body actually have one, and that methods that shouldn't have a body don't. That check goes wrong of the method is "get" rather than "GET". Though the error message is a bit misleading.

I'm still confused about the actual error though. The error message is triggered in a conditional block starting with if ( in_array( $requestMethod, RequestInterface::NO_BODY_METHODS ) ). NO_BODY_METHODS is defined as public const NO_BODY_METHODS = [ 'GET', 'HEAD' ]. So if the client sends "get", that shouldn't match...