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.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T5651: "400 Bad Request" errors from Aphlict server
- Commits
- Restricted Diffusion Commit
rP41a8837f78db: Make HTTP errors returned from the Aphlict server more specific
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
Diff Detail
- Repository
- rP Phabricator
- Branch
- aphlicterr
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 1750 Build 1751: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
| 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. | |
| 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(); } | |
Comment Actions
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?
Comment Actions
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.
Comment Actions
Yeah okay. It might be a little messy because notification.server-uri is used in a few places I think. Let me check.
Comment Actions
Yeah, it's used in PhabricatorAphlictManagementWorkflow and PhabricatorNotificationClient.
Comment Actions
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.
Comment Actions
| src/applications/aphlict/management/PhabricatorAphlictManagementWorkflow.php | ||
|---|---|---|
| 85–87 | This is fine, but has no effect since $server_uri isn't directly used (we pick the domain/port parts out of it). | |