Page MenuHomePhorge
Create Task
Maniphest T15866

Aphront400Response when editing a task
Open, Needs TriagePublic
Actions

Description

Phorge at ed9d212013cd0d8747f92bca4e7cb94900bccfd5. PHP version irrelevant as we've also seen this in downstream for a while.

When editing a task via the "Add Action..." dropdown and e.g. changing its priority and status (adding a text comment is not needed),

  • In PhabricatorEditEngine::buildResponse(), $action = 'comment' so the code does return $this->buildCommentResponse($object).
  • PhabricatorEditEngine::buildCommentResponse($object) calls/checks if (!$request->isFormOrHisecPost()) { return new Aphront400Response(); }.
  • AphrontRequest defines const TYPE_FORM = '__form__'.
  • AphrontRequest::isFormOrHisecPost() checks that $post = $this->getExists(self::TYPE_FORM) && $this->isHTTPPost(). The first condition fails as $this->getExists(self::TYPE_FORM) returns false because the payload (which can be inspected via json_encode($request->getRequestData())) in this single call does NOT include "__form__":"1": '{"__preview__":"1","editengine.actions":"[{\"type\":\"priority\",\"value\":\"high\",\"initialValue\":null},{\"type\":\"status\",\"value\":\"invalid\",\"initialValue\":null}]","viewData":"[]","__ajax__":"true","__metablock__":"14","__path__":"\/maniphest\/task\/edit\/3801\/comment\/"}'
  • As AphrontRequest::isFormOrHisecPost() returns false to PhabricatorEditEngine::buildCommentResponse(), it triggers return new Aphront400Response().

Thus I believe that the logic in rP543f2b6bf156be8eed7764927663d4366a8e1561 is slightly wrong.

Screenshot from 2024-06-24 14-03-07.png (599×1 px, 155 KB)

Revisions and Commits

rP Phorge
Needs ReviewD25733 Do not throw Aphront400Response when editing a task

Related Objects

Event Timeline

aklapper created this task.Jun 24 2024, 15:28
Herald added subscribers: Cigaryno, Matthew, chris, tobiaswiese. · View Herald TranscriptJun 24 2024, 15:28
aklapper added a comment.EditedJun 24 2024, 15:31

The diff below suppresses the error (and checks like trying to "Sign With MFA" without another action should fail etc still work) but no idea about potential side effects as isPreviewRequest() is called in numerous places.

Does anyone know/understand this code a bit better?

The other option would be including "__form__":"1" in that call to avoid the 400.

P44 T15866 suppress HTTP 400 error
1diff --git a/src/aphront/AphrontRequest.php b/src/aphront/AphrontRequest.php
2index 2561e397b6..4337577c3e 100644
3--- a/src/aphront/AphrontRequest.php
4+++ b/src/aphront/AphrontRequest.php
5@@ -705,7 +705,7 @@ final class AphrontRequest extends Phobject {
6 }
7
8 public function isPreviewRequest() {
9- return $this->isFormPost() && $this->getStr('__preview__');
10+ return $this->getStr('__preview__');
11 }
12
13 /**
14diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php
15index a696fb6cf7..2021da3615 100644
16--- a/src/applications/transactions/editengine/PhabricatorEditEngine.php
17+++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php
18@@ -1892,10 +1892,11 @@ abstract class PhabricatorEditEngine
19
20 $controller = $this->getController();
21 $request = $controller->getRequest();
22+ $is_preview = $request->isPreviewRequest();
23
24 // NOTE: We handle hisec inside the transaction editor with "Sign With MFA"
25 // comment actions.
26- if (!$request->isFormOrHisecPost()) {
27+ if (!$request->isFormOrHisecPost() && !$is_preview) {
28 return new Aphront400Response();
29 }
30
31@@ -1911,7 +1912,6 @@ abstract class PhabricatorEditEngine
32
33 $fields = $this->buildEditFields($object);
34
35- $is_preview = $request->isPreviewRequest();
36 $view_uri = $this->getEffectiveObjectViewURI($object);
37
38 $template = $object->getApplicationTransactionTemplate();

aklapper added a comment.Jul 15 2024, 16:12

The other option would be including "__form__":"1" in that call to avoid the 400.

I appreciate when method names describe exactly what they do. isPreviewRequest() currently does not, it seems...

Some notes to myself:

[acko@fedora phorge]$ grep -r isPreviewRequest .
./phorge/src/applications/badges/controller/PhabricatorBadgesCommentController.php:    $is_preview = $request->isPreviewRequest();
./phorge/src/applications/ponder/controller/PonderQuestionCommentController.php:    $is_preview = $request->isPreviewRequest();
./phorge/src/applications/ponder/controller/PonderAnswerCommentController.php:    $is_preview = $request->isPreviewRequest();
./phorge/src/applications/draft/storage/PhabricatorDraft.php:    if ($request->isPreviewRequest()) {
./phorge/src/applications/pholio/controller/PholioMockCommentController.php:    $is_preview = $request->isPreviewRequest();
./phorge/src/applications/transactions/editengine/PhabricatorEditEngine.php:    $is_preview = $request->isPreviewRequest();
./phorge/src/applications/slowvote/controller/PhabricatorSlowvoteCommentController.php:    $is_preview = $request->isPreviewRequest();
./phorge/src/aphront/AphrontRequest.php:  public function isPreviewRequest() {
[acko@fedora phorge]$ grep -r TYPE_PREVIEW .
./phorge/src/aphront/AphrontRequest.php:  const TYPE_PREVIEW = '__preview__';
^ NEVER USED.
[acko@fedora phorge]$ grep -r "__preview__" .
./phorge/src/aphront/AphrontRequest.php:  const TYPE_PREVIEW = '__preview__';
./phorge/src/aphront/AphrontRequest.php:    return $this->isFormPost() && $this->getStr('__preview__');
https://we.phorge.it/source/phorge/browse/master/webroot/rsrc/js/application/transactions/behavior-transaction-comment-form.js$16 : obj.__preview__ = 1;
https://we.phorge.it/source/phorge/browse/master/webroot/rsrc/js/application/transactions/behavior-comment-actions.js$90 :   data.__preview__ = 1;
^ none of these last two files set `__form__` explicitly either; see also `__form__` in https://we.phorge.it/source/phorge/browse/master/src/infrastructure/javelin/markup.php$136
aklapper added a revision: D25733: Do not throw Aphront400Response when editing a task.Jul 15 2024, 16:13
valerio.bozzolan awarded a token.Jul 19 2024, 14:15
valerio.bozzolan subscribed.EditedJul 21 2024, 09:41

So this is "just" something visible from server logs or browser console, right? It is not an error ever visible to user interface.

Thanks for digging into this in any case. Wow.

aklapper added a comment.Jul 21 2024, 17:26

Correct, the end user will not realize without opening the network tab of their web browser's developer tools.
However, as a downstream admin, I look at statistics/dashboards of HTTP 4xx and HTTP 5xx errors. When the numbers are not very low I'd usually assume something goes wrong and should be investigated. But that is not the case when Phorge throws a "silent" HTTP 400 for no obvious reasons.

Content licensed under Creative Commons Attribution-ShareAlike 4.0 (CC-BY-SA) unless otherwise noted; code licensed under Apache 2.0 or other open source licenses. · CC BY-SA 4.0 · Apache 2.0