From 90d41cc88d13a1e011eb0aa1df97f5edf4f7356c Mon Sep 17 00:00:00 2001 From: Jordan Dominion Date: Mon, 6 Nov 2023 17:56:03 -0500 Subject: [PATCH] Replace HTTP 426 with 400 Notable exception is the swarm API, which still uses 426 for version mismatches and would be a pain in the ass to change correctly. Closes #1693 --- docs/API.dox | 1 - src/Tgstation.Server.Client/ApiClient.cs | 5 +++-- src/Tgstation.Server.Host/Controllers/README.md | 1 - .../Extensions/ApplicationBuilderExtensions.cs | 5 +---- src/Tgstation.Server.Host/Security/README.md | 4 ++-- tests/Tgstation.Server.Tests/Live/RawRequestTests.cs | 4 ++-- 6 files changed, 8 insertions(+), 12 deletions(-) diff --git a/docs/API.dox b/docs/API.dox index a859e36c47..be71673604 100644 --- a/docs/API.dox +++ b/docs/API.dox @@ -72,7 +72,6 @@ TGS will only every return the response codes listed here - 410: Gone. Attempted to access/modify a resource that ideally should have been ready, but isn't or no longer is - 422: Unprocessable Entity: Used specifically when an operation that requires a server restart is unable to be performed due to the @ref Tgstation.Server.Host.Watchdog not being present in the deployment. Should not happen with a proper server configuration. Response body contains an @ref Tgstation.Server.Api.Models.ErrorMessage - 424: Failed Dependency: When a request that depends on an external API fails for a reason other than rate limiting. The response body will contain an @ref Tgstation.Server.Api.Models.ErrorMessage model detailing the error. -- 426: Upgrade required: Used when the client's API version is not compatible with the server's. Response body contains an @ref Tgstation.Server.Api.Models.ErrorMessage - 429: Rate limited. Used with operations that rely on GitHub.com. If a rate limit is hit for an operation this will be returned. Response will contain a Retry-After header with the amount of seconds to wait. - 500: Server error. Please report the request and response body to the code repository - 501: Not implemented. Functionality not available in the current server version diff --git a/src/Tgstation.Server.Client/ApiClient.cs b/src/Tgstation.Server.Client/ApiClient.cs index 28c5611bdd..3270be8ea2 100644 --- a/src/Tgstation.Server.Client/ApiClient.cs +++ b/src/Tgstation.Server.Client/ApiClient.cs @@ -131,8 +131,6 @@ namespace Tgstation.Server.Client #pragma warning restore IDE0066 // Convert switch statement to expression #pragma warning restore IDE0010 // Add missing cases { - case HttpStatusCode.UpgradeRequired: - throw new VersionMismatchException(errorMessage, response); case HttpStatusCode.Unauthorized: throw new UnauthorizedException(errorMessage, response); case HttpStatusCode.InternalServerError: @@ -154,6 +152,9 @@ namespace Tgstation.Server.Client case (HttpStatusCode)429: throw new RateLimitException(errorMessage, response); default: + if (errorMessage?.ErrorCode == ErrorCode.ApiMismatch) + throw new VersionMismatchException(errorMessage, response); + throw new ApiConflictException(errorMessage, response); } } diff --git a/src/Tgstation.Server.Host/Controllers/README.md b/src/Tgstation.Server.Host/Controllers/README.md index e1214f91f2..66c5df1244 100644 --- a/src/Tgstation.Server.Host/Controllers/README.md +++ b/src/Tgstation.Server.Host/Controllers/README.md @@ -7,7 +7,6 @@ Some notable exceptions: - [ApiController](./ApiController.cs) is the base class of nearly all API related controllers. It does the following: - Contains code to deny the request if the instance is not present when it should be. - Contains the `IDatabaseContext` and `ILogger` properties for child controllers. - - Returns 426 Upgrade Required if the API version in the headers are incompatible with the request. - Returns 400 Bad Request if the headers or the PUT/POST'd model is invalid. - Returns 401 If an `IAuthenticationContext` could not be created for a request. - [BridgeController](./BridgeController.cs) is a special controller accessible only from localhost and is used to receive bridge request from DreamDaemon diff --git a/src/Tgstation.Server.Host/Extensions/ApplicationBuilderExtensions.cs b/src/Tgstation.Server.Host/Extensions/ApplicationBuilderExtensions.cs index 55086228dc..40661c9a5e 100644 --- a/src/Tgstation.Server.Host/Extensions/ApplicationBuilderExtensions.cs +++ b/src/Tgstation.Server.Host/Extensions/ApplicationBuilderExtensions.cs @@ -132,11 +132,8 @@ namespace Tgstation.Server.Host.Extensions var apiHeadersProvider = context.RequestServices.GetRequiredService(); if (apiHeadersProvider.ApiHeaders?.Compatible() == false) { - await new JsonResult( + await new BadRequestObjectResult( new ErrorMessageResponse(ErrorCode.ApiMismatch)) - { - StatusCode = (int)HttpStatusCode.UpgradeRequired, - } .ExecuteResultAsync(new ActionContext { HttpContext = context, diff --git a/src/Tgstation.Server.Host/Security/README.md b/src/Tgstation.Server.Host/Security/README.md index 404b27e162..ac6cbbc08f 100644 --- a/src/Tgstation.Server.Host/Security/README.md +++ b/src/Tgstation.Server.Host/Security/README.md @@ -16,7 +16,7 @@ ## For the login request (`POST /`) -1. An attempt to parse the `ApiHeaders` is made. If they were valid, the API version check is performed. If it fails, HTTP 426 with an `ErrorMessageResponse` will be returned. +1. An attempt to parse the `ApiHeaders` is made. If they were valid, the API version check is performed. If it fails, HTTP 400 with an `ErrorMessageResponse` will be returned. 1. If, for some reason, the user attempts to use a JWT to authenticate this request, steps 2-4 of the non-login pipeline list below are performed. 1. The `ApiController` base class inspects the request. - At this point, if the `ApiHeaders` (MINUS the `Authorization` header) cannot be properly parsed, HTTP 400 with an `ErrorMessageResponse` is returned. @@ -51,7 +51,7 @@ ## For all other authenticated requests -1. An attempt to parse the `ApiHeaders` is made. If they were valid. The API version check is performed. If it fails, HTTP 426 with an `ErrorMessageResponse` will be returned. +1. An attempt to parse the `ApiHeaders` is made. If they were valid. The API version check is performed. If it fails, HTTP 400 with an `ErrorMessageResponse` will be returned. 1. The JWT, if present, is validated. If it is, the scope's [AuthenticationContextFactory](./AuthenticationContextFactory.cs) has `SetTokenNbf` called. If not, HTTP 401 will be returned. - Inside ASP.NET Core, this initializes the calling user's identity principal and sets the "sub" claim to the TGS user ID parsed out of the JWT. - We know it's the user ID because we set it up like that in the [TokenFactory](./TokenFactory.cs) diff --git a/tests/Tgstation.Server.Tests/Live/RawRequestTests.cs b/tests/Tgstation.Server.Tests/Live/RawRequestTests.cs index fbf5fe44e5..bf0d409e61 100644 --- a/tests/Tgstation.Server.Tests/Live/RawRequestTests.cs +++ b/tests/Tgstation.Server.Tests/Live/RawRequestTests.cs @@ -87,7 +87,7 @@ namespace Tgstation.Server.Tests.Live request.Headers.Add(ApiHeaders.ApiVersionHeader, "Tgstation.Server.Api/6.0.0"); request.Headers.Authorization = new AuthenticationHeaderValue(ApiHeaders.BearerAuthenticationScheme, token); using var response = await httpClient.SendAsync(request, cancellationToken); - Assert.AreEqual(HttpStatusCode.UpgradeRequired, response.StatusCode); + Assert.AreEqual(HttpStatusCode.BadRequest, response.StatusCode); var content = await response.Content.ReadAsStringAsync(cancellationToken); var message = JsonConvert.DeserializeObject(content); Assert.AreEqual(ErrorCode.ApiMismatch, message.ErrorCode); @@ -101,7 +101,7 @@ namespace Tgstation.Server.Tests.Live request.Headers.Add(ApiHeaders.ApiVersionHeader, "Tgstation.Server.Api/6.0.0"); request.Headers.Authorization = new AuthenticationHeaderValue(ApiHeaders.BearerAuthenticationScheme, token); using var response = await httpClient.SendAsync(request, cancellationToken); - Assert.AreEqual(HttpStatusCode.UpgradeRequired, response.StatusCode); + Assert.AreEqual(HttpStatusCode.BadRequest, response.StatusCode); var content = await response.Content.ReadAsStringAsync(cancellationToken); var message = JsonConvert.DeserializeObject(content); Assert.AreEqual(ErrorCode.ApiMismatch, message.ErrorCode);