1
0
mirror of https://github.com/bitwarden/server synced 2025-12-19 17:53:44 +00:00
Files
server/src/Api/Dirt/Controllers/HibpController.cs
Alex aa3172e24f [PM-6979] correct REST semantics (#6661)
* fix: Return 200 OK with empty array for HIBP breach endpoint when no breaches found

Changes the HIBP breach check endpoint to return HTTP 200 OK with an empty
JSON array `[]` instead of 404 Not Found when no breaches are found. This
follows proper REST API semantics where 404 should indicate the endpoint
doesn't exist, not that a query returned no results.

Changes:
- src/Api/Dirt/Controllers/HibpController.cs: Lines 67-71
- Changed: return new NotFoundResult(); → return Content("[]", "application/json");

Backward Compatible:
- Clients handle both 200 with [] (new) and 404 (old)
- No breaking changes
- Safe to deploy independently

API Response Changes:
- Before: GET /api/hibp/breach?username=safe@example.com → 404 Not Found
- After:  GET /api/hibp/breach?username=safe@example.com → 200 OK, Body: []

Impact:
- No user-facing changes
- Correct REST semantics
- Industry-standard API response pattern

* Address PR feedback: enhance comment and add comprehensive unit tests

Addresses feedback from PR #6661:

1. Enhanced comment per @prograhamming's feedback (lines 69-71):
   - Added date stamp (12/1/2025)
   - Explained HIBP API behavior: returns 404 when no breaches found
   - Clarified HIBP API specification about 404 meaning
   - Maintained REST semantics justification

2. Created comprehensive unit tests per Claude bot's Finding 1:
   - New file: test/Api.Test/Dirt/HibpControllerTests.cs
   - 9 test cases covering all critical scenarios:
     * Missing API key validation
     * No breaches found (404 → 200 with []) - KEY TEST FOR PR CHANGE
     * Breaches found (200 with data)
     * Rate limiting with retry logic
     * Server error handling (500, 400)
     * URL encoding of special characters
     * Required headers validation
     * Self-hosted vs cloud User-Agent differences

Test Coverage:
- Before: 0% coverage for HibpController
- After: ~90% coverage (all public methods and major paths)
- Uses xUnit, NSubstitute, BitAutoData patterns
- Matches existing Dirt controller test conventions

Changes:
- src/Api/Dirt/Controllers/HibpController.cs: Enhanced comment (+3 lines)
- test/Api.Test/Dirt/HibpControllerTests.cs: New test file (327 lines, 9 tests)

Addresses:
- @prograhamming's comment about enhancing the code comment
- Claude bot's Finding 1: Missing unit tests for HibpController

Related: PM-6979

* fix test/formating errors
2025-12-01 12:37:31 -08:00

104 lines
3.7 KiB
C#

// FIXME: Update this file to be null safe and then delete the line below
#nullable disable
using System.Net;
using System.Security.Cryptography;
using Bit.Core.Context;
using Bit.Core.Exceptions;
using Bit.Core.Services;
using Bit.Core.Settings;
using Bit.Core.Utilities;
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Mvc;
namespace Bit.Api.Dirt.Controllers;
[Route("hibp")]
[Authorize("Application")]
public class HibpController : Controller
{
private const string HibpBreachApi = "https://haveibeenpwned.com/api/v3/breachedaccount/{0}" +
"?truncateResponse=false&includeUnverified=false";
private static HttpClient _httpClient;
private readonly IUserService _userService;
private readonly ICurrentContext _currentContext;
private readonly GlobalSettings _globalSettings;
private readonly string _userAgent;
static HibpController()
{
_httpClient = new HttpClient();
}
public HibpController(
IUserService userService,
ICurrentContext currentContext,
GlobalSettings globalSettings)
{
_userService = userService;
_currentContext = currentContext;
_globalSettings = globalSettings;
_userAgent = _globalSettings.SelfHosted ? "Bitwarden Self-Hosted" : "Bitwarden";
}
[HttpGet("breach")]
public async Task<IActionResult> Get(string username)
{
return await SendAsync(WebUtility.UrlEncode(username), true);
}
private async Task<IActionResult> SendAsync(string username, bool retry)
{
if (!CoreHelpers.SettingHasValue(_globalSettings.HibpApiKey))
{
throw new BadRequestException("HaveIBeenPwned API key not set.");
}
var request = new HttpRequestMessage(HttpMethod.Get, string.Format(HibpBreachApi, username));
request.Headers.Add("hibp-api-key", _globalSettings.HibpApiKey);
request.Headers.Add("hibp-client-id", GetClientId());
request.Headers.Add("User-Agent", _userAgent);
var response = await _httpClient.SendAsync(request);
if (response.IsSuccessStatusCode)
{
var data = await response.Content.ReadAsStringAsync();
return Content(data, "application/json");
}
else if (response.StatusCode == HttpStatusCode.NotFound)
{
/* 12/1/2025 - Per the HIBP API, If the domain does not have any email addresses in any breaches,
an HTTP 404 response will be returned. API also specifies that "404 Not found is the account could
not be found and has therefore not been pwned". Per REST semantics we will return 200 OK with empty array. */
return Content("[]", "application/json");
}
else if (response.StatusCode == HttpStatusCode.TooManyRequests && retry)
{
var delay = 2000;
if (response.Headers.Contains("retry-after"))
{
var vals = response.Headers.GetValues("retry-after");
if (vals.Any() && int.TryParse(vals.FirstOrDefault(), out var secDelay))
{
delay = (secDelay * 1000) + 200;
}
}
await Task.Delay(delay);
return await SendAsync(username, false);
}
else
{
throw new BadRequestException("Request failed. Status code: " + response.StatusCode);
}
}
private string GetClientId()
{
var userId = _userService.GetProperUserId(User).Value;
using (var sha256 = SHA256.Create())
{
var hash = sha256.ComputeHash(userId.ToByteArray());
return Convert.ToBase64String(hash);
}
}
}