Refactor X-Hub-Signature validation

[nwf]

Describe the feature

webhooks.net/src/Octokit.Webhooks.AspNetCore/GitHubWebhookExtensions.cs

Line 66 in 95e10f9

private static async Task<bool> VerifySignatureAsync(HttpContext context, string secret, string body)

requires a HttpContext object only for its header and for generating a response in the case of failure. Unfortunately. HttpContexts are sometimes hard to come by; for example, Azure Functions, at least as of runner version 4, for example, provide only HttpRequests and not the full HttpContext.

Therefore, it would be quite nice and DRY if this method could be split into a worker and wrapper, with the worker taking the already-extracted signature header and returning a HttpStatusCode and String for the body, and the wrapper adapting for the ASP.Net HttpContext object.

[JamieMagee]

Okay, so to clarify you'd like an interface that looks something like this:

public static class GitHubWebhookExtensionsWorker
{
    public static VerifyResult VerifyContentType(HttpRequest request, string expectedContentType);

    private static async Task<string> GetBodyAsync(HttpRequest request);

    private static async Task<VerifyResult> VerifySignatureAsync(HttpRequest request, string secret, string body)
}

where VerifyResult is something like:

public sealed record VerifyResult
{
    public bool IsSuccess { get; init; } = null!;

    public int? ResponseCode { get; init; }

    public string? ResponseMessage { get; init; }
}

and MapGitHubWebhooks will call GitHubWebhookExtensionsWorker methods checking IsSuccess?

[nwf]

That seems plausible, though maybe GitHubWebhookUtilities rather than ...ExtensionsWorker?

Is it possible to make VerifyResult parametric on its success case? Then VerifySignatureAsync can return VerifyResult<bool> and MapGitHubWebhooks could also be split into the preflight checks (fairly reusable) and flight (ASP.Net specific) bits? That is, pull

webhooks.net/src/Octokit.Webhooks.AspNetCore/GitHubWebhookExtensions.cs

Lines 20 to 33 in 95e10f9

	// Verify content type
	if (!VerifyContentType(context, MediaTypeNames.Application.Json))
	{
	return;
	}
	// Get body
	var body = await GetBodyAsync(context).ConfigureAwait(false);
	// Verify signature
	if (!await VerifySignatureAsync(context, secret, body).ConfigureAwait(false))
	{
	return;
	}

out to their own method (adjusted to take a HttpRequest and to return a Task<VerifyResult<string>> capturing the body of a verified message) in the worker class?

[nwf]

Ah, one more thought re: MapGitHubWebhooks, while in the neighborhood: should there be a limit on the content length done before pulling the entire body into memory?

[github-actions]

👋 Hey Friends, this issue has been automatically marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please add the Status: Pinned label if you feel that this issue needs to remain open/active. Thank you for your contributions and help in keeping things tidy!

[github-actions]

👋 Hey Friends, this issue has been automatically marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please add the Status: Pinned label if you feel that this issue needs to remain open/active. Thank you for your contributions and help in keeping things tidy!