Page MenuHomePhabricator

Make HTTP errors returned from the Aphlict server more specific
ClosedPublic

Authored by joshuaspence on Jul 17 2014, 9:46 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 10, 1:23 AM
Unknown Object (File)
Feb 14 2024, 9:46 PM
Unknown Object (File)
Feb 14 2024, 9:46 PM
Unknown Object (File)
Feb 14 2024, 9:46 PM
Unknown Object (File)
Feb 14 2024, 9:46 PM
Unknown Object (File)
Feb 14 2024, 9:46 PM
Unknown Object (File)
Feb 14 2024, 9:46 PM
Unknown Object (File)
Feb 14 2024, 9:46 PM
Subscribers

Details

Summary

Ref T5651. Currently, the Aphlict server returns either 200 OKAY or 400 Bad Request. We could return more specific errors in some cases and this may assist with debugging.

Test Plan

Sent myself a test notification at /notification/status/ and saw the Aphlict server process the request (running in debug mode). Also poked around with curl:

> curl http://localhost:22281/
405 Method Not Allowed

> curl http://localhost:22281/ -d ""
400 Bad Request

> curl http://localhost:22281/foobar/
404 Not Found

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

joshuaspence retitled this revision from to Make HTTP errors returned from the Aphlict server more specific.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
epriestley edited edge metadata.
epriestley added inline comments.
support/aphlict/server/aphlict_server.js
182–198

This possibly being a bad assumption is the only thing I can imagine being an issue here, should be clear from testing and I can't imagine we're requesting any other path.

This revision is now accepted and ready to land.Jul 17 2014, 9:48 PM
support/aphlict/server/aphlict_server.js
182–198

Hmm yeah I know. You're right actually, see code below. Currently, someone could set notification.server-uri to 'http://localhost:22281/foobar/' and everything should work fine. This is sort of gross and we should maybe add a setup check to ensure that the notifications server URI has no path component.

private static function postMessage(array $data) {
  $server_uri = PhabricatorEnv::getEnvConfig('notification.server-uri');

  id(new HTTPSFuture($server_uri, json_encode($data)))
    ->setMethod('POST')
    ->setTimeout(1)
    ->resolvex();
}

You could just add setPath('/').

Well, rather:

$uri = new PhutilURI($server_uri);
$uri->setPath('/');

...

You could... I think a setup check is possibly better? I'm not sure... would someone actually set a path on notification.server-uri and expect it to work?

joshuaspence edited edge metadata.
  • Add newline character to HTTP responses

In this case, I slightly favor just making it work, since I can't imagine ever doing anything with the path and it's easier for goofy cases where users specify thing.com// and such. Setup check is fine too, though, if you want to go that route.

Yeah okay. It might be a little messy because notification.server-uri is used in a few places I think. Let me check.

Yeah, it's used in PhabricatorAphlictManagementWorkflow and PhabricatorNotificationClient.

We should possibly setPath('/') for notification.client-uri as well?

  • Ensure that notification.server-uri has no path component

In PhabricatorAphlictManagementWorkflow, we already ignore the path (both client + server).

In getServerStatus(), we effectively ignore the path (overwrite it).

In aphlict_test_client.php it's essentially ignored too (no HTTP protocol).

In PhabricatorStandardPageView, it's deconstructed/ignored.

So I think this is the only case where the path could do anything.

shipitquick

src/applications/aphlict/management/PhabricatorAphlictManagementWorkflow.php
85–86 ↗(On Diff #23920)

This is fine, but has no effect since $server_uri isn't directly used (we pick the domain/port parts out of it).