diff --git a/KNOWLEDGE_BASE.md b/KNOWLEDGE_BASE.md index 8516a18c6..e7598a0b2 100644 --- a/KNOWLEDGE_BASE.md +++ b/KNOWLEDGE_BASE.md @@ -514,7 +514,7 @@ Empty issue drafts show a short Markdown template: steps and expected versus act Successful issue creation is limited to **ten issues per day per user**. Rejected drafts and failed requests do not use that allowance. If GitHub or the connection fails, the draft stays available. Check upstream before retrying an uncertain submission to avoid duplicates. Use a commit's Report control instead when reporting abusive content. -Sources: [issue command UI](resources/js/components/issue-command.tsx), [issue validation](app/Http/Requests/CreateIssueRequest.php), [issue controller](app/Http/Controllers/IssueController.php). +Sources: [issue command UI](resources/js/components/issue-command.tsx), [issue validation](app/Http/Requests/CreateIssueRequest.php), [issue controller](app/Http/Controllers/IssueController.php), [issue limit](app/Http/Middleware/ThrottleIssues.php). ### Exporting your data diff --git a/app/Enums/ApiErrorCode.php b/app/Enums/ApiErrorCode.php index dd604800e..e9c71c96b 100644 --- a/app/Enums/ApiErrorCode.php +++ b/app/Enums/ApiErrorCode.php @@ -20,6 +20,7 @@ enum ApiErrorCode: string case PasteisExhausted = 'pasteis_exhausted'; case RateLimited = 'rate_limited'; case GitHubSyncFailed = 'github_sync_failed'; + case GitHubIssueFailed = 'github_issue_failed'; case TimedOut = 'timed_out'; case Banned = 'banned'; case CommitRulesRequired = 'commit_rules_required'; @@ -33,7 +34,7 @@ public function status(): int self::CommitUnavailable => 404, self::PasteisExhausted => 422, self::RateLimited => 429, - self::GitHubSyncFailed => 503, + self::GitHubSyncFailed, self::GitHubIssueFailed => 503, }; } } diff --git a/app/Http/Controllers/Api/IssueController.php b/app/Http/Controllers/Api/IssueController.php new file mode 100644 index 000000000..89520df71 --- /dev/null +++ b/app/Http/Controllers/Api/IssueController.php @@ -0,0 +1,35 @@ +handle($user, $request->string('command')->value(), $request->string('body')->value(), $request->environment()); + } catch (ConnectionException) { + return new ApiException(ApiErrorCode::GitHubIssueFailed, __('GitHub did not respond. Check the upstream issues before trying again. Your draft is safe.'))->render(); + } catch (RequestException) { + return new ApiException(ApiErrorCode::GitHubIssueFailed, __('GitHub did not create the issue. Your draft is safe.'))->render(); + } + + return response()->json($issue, 201); + } +} diff --git a/app/Http/Controllers/IssueController.php b/app/Http/Controllers/IssueController.php index 08cab82ec..5b5a89271 100644 --- a/app/Http/Controllers/IssueController.php +++ b/app/Http/Controllers/IssueController.php @@ -11,7 +11,6 @@ use Illuminate\Http\Client\ConnectionException; use Illuminate\Http\Client\RequestException; use Illuminate\Http\JsonResponse; -use Illuminate\Support\Facades\RateLimiter; final readonly class IssueController { diff --git a/app/Http/Middleware/ThrottleIssues.php b/app/Http/Middleware/ThrottleIssues.php new file mode 100644 index 000000000..e9dde971c --- /dev/null +++ b/app/Http/Middleware/ThrottleIssues.php @@ -0,0 +1,50 @@ +user(); + assert($user instanceof User); + + $key = "issues:{$user->id}"; + + if (RateLimiter::tooManyAttempts($key, self::PER_HOUR)) { + $message = 'Issue limit reached. Try again in an hour.'; + $headers = ['Retry-After' => RateLimiter::availableIn($key)]; + + // The mobile app reads JSON with a code... + return $request->routeIs('api-issue.store') + ? new ApiException(ApiErrorCode::RateLimited, $message, $headers)->render() + : response()->json(['message' => $message], 429, $headers); + } + + $response = $next($request); + + if ($response->getStatusCode() === 201) { + RateLimiter::hit($key, 3600); + } + + return $response; + } +} diff --git a/app/Http/Requests/CreateApiIssueRequest.php b/app/Http/Requests/CreateApiIssueRequest.php new file mode 100644 index 000000000..cadd19444 --- /dev/null +++ b/app/Http/Requests/CreateApiIssueRequest.php @@ -0,0 +1,53 @@ +> + */ + public function rules(): array + { + return [ + 'command' => ['required', Rule::in(['/feature', '/bug'])], + 'body' => ['required', 'string', 'min:'.CreateIssueRequest::MIN_BODY_LENGTH, 'max:'.Commit::MAX_BODY_LENGTH], + 'environment' => ['nullable', 'array:'.implode(',', CreateIssueRequest::ENVIRONMENT_FIELDS)], + 'environment.*' => ['nullable', 'string', 'max:100'], + ]; + } + + /** + * @return array + */ + public function environment(): array + { + /** @var array $environment */ + $environment = $this->array('environment'); + + return array_filter($environment, fn (?string $value): bool => $value !== null && $value !== ''); + } +} diff --git a/routes/api.php b/routes/api.php index aaf14381c..ed76a1fbe 100644 --- a/routes/api.php +++ b/routes/api.php @@ -22,6 +22,7 @@ use App\Http\Controllers\Api\ConversationTypingController; use App\Http\Controllers\Api\CurrentUserController; use App\Http\Controllers\Api\DeviceController; +use App\Http\Controllers\Api\IssueController; use App\Http\Controllers\Api\LogController; use App\Http\Controllers\Api\MacAppController; use App\Http\Controllers\Api\MessageImageController; @@ -60,6 +61,7 @@ use App\Http\Middleware\PreventPostingInTimeout; use App\Http\Middleware\RequireCommitRules; use App\Http\Middleware\ThrottleCommits; +use App\Http\Middleware\ThrottleIssues; use Illuminate\Http\JsonResponse; use Illuminate\Support\Facades\Route; diff --git a/routes/web.php b/routes/web.php index 7b6e9e094..8e0fb5b40 100644 --- a/routes/web.php +++ b/routes/web.php @@ -128,6 +128,7 @@ use App\Http\Middleware\SerializePushSessions; use App\Http\Middleware\SerializePushSubscriptions; use App\Http\Middleware\ThrottleCommits; +use App\Http\Middleware\ThrottleIssues; use Illuminate\Foundation\Http\Middleware\PreventRequestForgery; use Illuminate\Support\Facades\Route; use Inertia\Inertia; @@ -265,7 +266,7 @@ Route::middleware('auth')->group(function (): void { Route::post('issues', [IssueController::class, 'store']) - ->middleware('throttle:10,1') + ->middleware(['throttle:10,1', ThrottleIssues::class]) ->name('issue.store'); // Session... diff --git a/tests/Feature/Controllers/Api/IssueControllerTest.php b/tests/Feature/Controllers/Api/IssueControllerTest.php new file mode 100644 index 000000000..beb8e922b --- /dev/null +++ b/tests/Feature/Controllers/Api/IssueControllerTest.php @@ -0,0 +1,143 @@ + 2048, 'private_key_type' => OPENSSL_KEYTYPE_RSA]); + assert($key !== false); + openssl_pkey_export($key, $privateKey); + + config(['services.github_app' => ['id' => '12345', 'private_key' => str_replace("\n", '\n', $privateKey)]]); + + $this->user = User::factory()->create(['username' => 'taylor']); + $this->keyPair = apiKeyPairFor($this->user); +}); + +it('opens the issue as the app, crediting the reporter, with the label of the command, as the web does', function (string $command, string $label): void { + fakeGitHubApp(); + $body = "Search loses its filters\n\nSteps: select PHP, then navigate back."; + + signedApiRequest($this->keyPair, 'POST', route('api-issue.store'), ['command' => $command, 'body' => $body]) + ->assertCreated() + ->assertExactJson(['number' => 123, 'url' => 'https://github.com/nunomaduro/dev-null.sh/issues/123']); + + Http::assertSent(fn (Request $request): bool => $request->url() === 'https://api.github.com/repos/nunomaduro/dev-null.sh/issues' + && $request->hasHeader('Authorization', 'Bearer installation-token') + && $request['title'] === 'Search loses its filters' + && $request['body'] === $body."\n\n---\n\nReported by @taylor via [/dev/null](".route('user.show', $this->user).').' + && $request['labels'] === [$label]); + $this->assertDatabaseCount('commits', 0); +})->with([ + 'feature' => ['/feature', 'enhancement'], + 'bug' => ['/bug', 'bug'], +]); + +it('collapses blank lines before measuring and sending the body', function (): void { + fakeGitHubApp(); + + signedApiRequest($this->keyPair, 'POST', route('api-issue.store'), ['command' => '/feature', 'body' => "Dark mode\n\n\n\nplease"]) + ->assertCreated(); + + Http::assertSent(fn (Request $request): bool => str_ends_with($request->url(), '/issues') + && str_starts_with((string) $request['body'], "Dark mode\n\nplease\n\n---")); +}); + +it('folds the environment the app sends into the body, as the web does', function (): void { + fakeGitHubApp(); + + signedApiRequest($this->keyPair, 'POST', route('api-issue.store'), [ + 'command' => '/bug', + 'body' => 'Broken.', + 'environment' => ['browser' => 'devnull app', 'os' => 'iOS 27.0', 'language' => ''], + ])->assertCreated(); + + Http::assertSent(fn (Request $request): bool => str_ends_with($request->url(), '/issues') + && str_starts_with((string) $request['body'], "Broken.\n\n
\nEnvironment\n\n| | |\n|---|---|\n| Browser | `devnull app` |\n| Os | `iOS 27.0` |\n\n
\n\n---")); +}); + +it('keeps the draft when GitHub rejects the issue or does not respond, with a code', function (Closure $response, string $message): void { + Http::fake([ + 'api.github.com/repos/nunomaduro/dev-null.sh/installation' => Http::response(['id' => 99]), + 'api.github.com/app/installations/99/access_tokens' => Http::response(['token' => 'installation-token'], 201), + 'api.github.com/repos/nunomaduro/dev-null.sh/issues' => $response, + ]); + + signedApiRequest($this->keyPair, 'POST', route('api-issue.store'), ['command' => '/bug', 'body' => 'Broken.']) + ->assertServiceUnavailable() + ->assertExactJson(['message' => $message, 'code' => 'github_issue_failed']); +})->with([ + 'rejected' => [fn () => Http::response(['message' => 'Validation Failed'], 422), 'GitHub did not create the issue. Your draft is safe.'], + 'unavailable' => [fn () => throw new ConnectionException('timeout'), 'GitHub did not respond. Check the upstream issues before trying again. Your draft is safe.'], +]); + +it('validates the command and the body as the web does', function (array $payload, string $field): void { + Http::fake(); + + signedApiRequest($this->keyPair, 'POST', route('api-issue.store'), $payload) + ->assertUnprocessable() + ->assertOnlyJsonValidationErrors($field); + + Http::assertNothingSent(); +})->with([ + 'unknown command' => [['command' => '/deploy', 'body' => 'Ship it.'], 'command'], + 'no command' => [['body' => 'Ship it.'], 'command'], + 'missing body' => [['command' => '/bug'], 'body'], + 'blank lines only' => [['command' => '/bug', 'body' => "a\n\n\n\n"], 'body'], + 'too short for an issue' => [['command' => '/bug', 'body' => 'ab'], 'body'], + 'too long' => [['command' => '/bug', 'body' => str_repeat('a', 4001)], 'body'], + 'unknown environment field' => [['command' => '/bug', 'body' => 'Broken.', 'environment' => ['cookie' => 'secret']], 'environment'], +]); + +it('shares the web limit of five opened issues an hour, answering with a code', function (): void { + fakeGitHubApp(); + + foreach (range(1, 3) as $attempt) { + $this->actingAs($this->user)->postJson(route('issue.store'), ['command' => '/bug', 'body' => "Broken {$attempt}."])->assertCreated(); + } + + foreach (range(4, 5) as $attempt) { + signedApiRequest($this->keyPair, 'POST', route('api-issue.store'), ['command' => '/bug', 'body' => "Broken {$attempt}."])->assertCreated(); + } + + signedApiRequest($this->keyPair, 'POST', route('api-issue.store'), ['command' => '/bug', 'body' => 'Broken again.']) + ->assertTooManyRequests() + ->assertHeader('Retry-After', 3600) + ->assertExactJson(['message' => 'Issue limit reached. Try again in an hour.', 'code' => 'rate_limited']); + + $this->travel(1)->hours(); + + signedApiRequest($this->keyPair, 'POST', route('api-issue.store'), ['command' => '/bug', 'body' => 'Broken later.'])->assertCreated(); +}); + +it('does not count rejected drafts or failed attempts towards the limit', function (): void { + Http::fake([ + 'api.github.com/repos/nunomaduro/dev-null.sh/installation' => Http::response(['id' => 99]), + 'api.github.com/app/installations/99/access_tokens' => Http::response(['token' => 'installation-token'], 201), + 'api.github.com/repos/nunomaduro/dev-null.sh/issues' => Http::sequence() + ->push([], 500)->push([], 500)->push([], 500) + ->push([], 500)->push([], 500)->push([], 500) + ->push(['number' => 1, 'html_url' => 'https://github.com/nunomaduro/dev-null.sh/issues/1'], 201), + ]); + + signedApiRequest($this->keyPair, 'POST', route('api-issue.store'), ['command' => '/deploy', 'body' => 'Nope.'])->assertUnprocessable(); + + foreach (range(1, 6) as $attempt) { + signedApiRequest($this->keyPair, 'POST', route('api-issue.store'), ['command' => '/bug', 'body' => 'Broken.'])->assertServiceUnavailable(); + $this->travel(11)->seconds(); + } + + signedApiRequest($this->keyPair, 'POST', route('api-issue.store'), ['command' => '/bug', 'body' => 'Broken.'])->assertCreated(); +}); + +it('rejects requests that are not signed', function (): void { + Http::fake(); + + $this->actingAs($this->user)->postJson(route('api-issue.store'), ['command' => '/bug', 'body' => 'Broken.'])->assertUnauthorized(); + + Http::assertNothingSent(); +}); diff --git a/tests/Pest.php b/tests/Pest.php index c6c5dc6a4..ba04cc6eb 100644 --- a/tests/Pest.php +++ b/tests/Pest.php @@ -202,6 +202,24 @@ function fakeGitHubSponsorsListing(bool $hasSponsorsListing = false): void ]); } +/** + * The GitHub App opening issue #123 upstream, as the /feature and /bug commands do. + * + * @param array $issue + */ +function fakeGitHubApp(array $issue = []): void +{ + Http::fake([ + 'api.github.com/repos/nunomaduro/dev-null.sh/installation' => Http::response(['id' => 99]), + 'api.github.com/app/installations/99/access_tokens' => Http::response(['token' => 'installation-token'], 201), + 'api.github.com/repos/nunomaduro/dev-null.sh/issues' => Http::response([ + 'number' => 123, + 'html_url' => 'https://github.com/nunomaduro/dev-null.sh/issues/123', + ...$issue, + ], 201), + ]); +} + /** * @param list $imageHashes * @return array{payload: string, signature: string}