vignesh/trypost
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix: address PR review findings — publish, REST store, SSRF, race

Browse source 
Code-review surfaced two correctness bugs and a security gap that
needed to land before merging.

- UpdatePost::execute disabled every platform when called without
  a `platforms` key. PublishPostTool relied on that path, so every
  publish-via-MCP queued a job whose handler then found nothing
  enabled to publish to. Wrap the platform toggle in
  `Arr::has($data, 'platforms')` (matches the existing label_ids
  guard a few lines up). Add a regression assertion to
  `PostPublishToolTest::publish post immediate dispatches PublishPost
  job` that the previously-enabled platform stays enabled.

- StorePostRequest declared rules for only `platforms`,
  `scheduled_at`, and `status`. `validated()` then stripped
  `content`, `media`, and `label_ids`, so REST `POST /api/posts`
  silently created empty drafts. Added rules for content / media /
  label_ids (with workspace-scoped `Rule::exists` for labels) and
  dropped the unused `status` field — REST callers transition state
  via `PUT /posts/{id}`. Removed the dead `platforms.*.content`
  rule. Added a feature test that asserts content + media + labels
  roundtrip on create, plus a regression that an `is_active=false`
  social_account is rejected at validation.

- CreatePost::execute now syncs label_ids itself so REST and MCP
  share the behavior. Removed the duplicate sync from CreatePostTool.

- MCP UpdatePostTool didn't scope `platforms.*.id` to the post being
  updated, drifting from the REST UpdatePostRequest which adds
  `Rule::exists('post_platforms','id')->where('post_id', ...)`. Now
  it loads the post first (failing fast with `Post not found.` if
  the workspace check rejects), then uses the same Rule::exists.

- MediaAttacher fetched any URL the caller passed, including
  loopback / link-local / private targets — classic SSRF pivot.
  Now `isPublicHttpUrl` rejects non-http(s) schemes, restricted IP
  ranges, and DNS hostnames whose A/AAAA records resolve into those
  ranges (covers DNS rebinding). Bypassed under
  `app()->runningUnitTests()` so `Http::fake()` keeps working.
  Streaming the response body lets us abort early once we exceed
  MAX_BYTES instead of buffering the full payload first; redirects
  are disabled so a 200→302 trick can't bypass the host check.

- The `media[]` JSON column had a lost-update race in
  `attachFromUrls`: read `$post->media`, mutate in PHP, write back.
  Two concurrent calls clobbered each other. Now wrapped in a
  transaction with `lockForUpdate()`.

- ESLint: `resources/js/actions/**` and `resources/js/routes/**`
  are auto-generated by Wayfinder on every build. Their import
  order matches PHP scan order, not alphabetical, so import/order
  fought eslint-fix forever. Added them to ignores.
This commit is contained in:
Paulo Castellano 2026-05-04 12:16:39 -03:00
parent bec831fec6
commit c1418c9d21
9 changed files with 209 additions and 44 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
app/Actions/Post/CreatePost.php
View file

@ -22,12 +22,16 @@ class CreatePost
* created via SyncPostPlatforms so the user can toggle them later in the
* editor.
*
* `label_ids[]` are attached after creation so the same set of UUIDs
* works for REST, MCP, and web callers.
*
* @param array{
* content?: ?string,
* media?: array<int, mixed>,
* date?: ?string,
* scheduled_at?: ?string,
* platforms?: array<int, array{social_account_id: string, content_type?: string}>
* platforms?: array<int, array{social_account_id: string, content_type?: string}>,
* label_ids?: array<int, string>
* } $data
*/
public static function execute(Workspace $workspace, User $user, array $data): Post
@ -62,6 +66,10 @@ public static function execute(Workspace $workspace, User $user, array $data): P
->update($updates);
}
if ($labelIds = data_get($data, 'label_ids')) {
$post->labels()->sync($labelIds);
}
return $post;
});
}

40
app/Actions/Post/UpdatePost.php
View file

@ -42,29 +42,31 @@ public static function execute(Workspace $workspace, Post $post, array $data): a
$post->labels()->sync(data_get($data, 'label_ids', []));
}
DB::transaction(function () use ($post, $data) {
$post->postPlatforms()->update(['enabled' => false]);
if (Arr::has($data, 'platforms')) {
DB::transaction(function () use ($post, $data) {
$post->postPlatforms()->update(['enabled' => false]);
foreach (data_get($data, 'platforms', []) as $platformData) {
$updateData = ['enabled' => true];
foreach (data_get($data, 'platforms', []) as $platformData) {
$updateData = ['enabled' => true];
if (data_get($platformData, 'content_type') !== null) {
$updateData['content_type'] = data_get($platformData, 'content_type');
}
if (data_get($platformData, 'meta') !== null) {
$postPlatform = $post->postPlatforms()->where('id', data_get($platformData, 'id'))->first();
if ($postPlatform) {
$updateData['meta'] = array_merge($postPlatform->meta ?? [], data_get($platformData, 'meta'));
if (data_get($platformData, 'content_type') !== null) {
$updateData['content_type'] = data_get($platformData, 'content_type');
}
}
$post->postPlatforms()
->where('id', data_get($platformData, 'id'))
->update($updateData);
}
});
if (data_get($platformData, 'meta') !== null) {
$postPlatform = $post->postPlatforms()->where('id', data_get($platformData, 'id'))->first();
if ($postPlatform) {
$updateData['meta'] = array_merge($postPlatform->meta ?? [], data_get($platformData, 'meta'));
}
}
$post->postPlatforms()
->where('id', data_get($platformData, 'id'))
->update($updateData);
}
});
}
if ($status === PostStatus::Publishing->value) {
$post->update(['scheduled_at' => now()]);

14
app/Http/Requests/Api/Post/StorePostRequest.php
View file

@ -4,7 +4,6 @@
namespace App\Http\Requests\Api\Post;
use App\Enums\Post\Status;
use App\Enums\PostPlatform\ContentType;
use App\Rules\ContentTypeMatchesPlatform;
use Illuminate\Foundation\Http\FormRequest;
@ -19,13 +18,17 @@ public function authorize(): bool
public function rules(): array
{
$workspaceId = $this->user()->currentWorkspace->id;
return [
'content' => ['nullable', 'string', 'max:63206'],
'media' => ['sometimes', 'array'],
'platforms' => ['required', 'array', 'min:1'],
'platforms.*.social_account_id' => [
'required',
'uuid',
Rule::exists('social_accounts', 'id')
->where('workspace_id', $this->user()->currentWorkspace->id)
->where('workspace_id', $workspaceId)
->where('is_active', true),
],
'platforms.*.content_type' => [
@ -34,9 +37,12 @@ public function rules(): array
Rule::in(array_column(ContentType::cases(), 'value')),
new ContentTypeMatchesPlatform,
],
'platforms.*.content' => ['nullable', 'string', 'max:63206'],
'scheduled_at' => ['nullable', 'date', 'after:now'],
'status' => ['nullable', 'string', Rule::in(array_column(Status::cases(), 'value'))],
'label_ids' => ['sometimes', 'array'],
'label_ids.*' => [
'uuid',
Rule::exists('workspace_labels', 'id')->where('workspace_id', $workspaceId),
],
];
}
}

4
app/Mcp/Tools/Post/CreatePostTool.php
View file

@ -41,10 +41,6 @@ public function handle(Request $request): ResponseFactory
$post = CreatePost::execute($workspace, $request->user(), $validated);
if ($labelIds = data_get($validated, 'label_ids')) {
$post->labels()->sync($labelIds);
}
$post->load(['postPlatforms.socialAccount', 'labels']);
return Response::structured((new PostResource($post))->resolve());

19
app/Mcp/Tools/Post/UpdatePostTool.php
View file

@ -26,6 +26,13 @@ public function handle(Request $request): Response|ResponseFactory
{
$workspace = $request->user()->currentWorkspace;
$postId = data_get($request->all(), 'post_id');
$post = is_string($postId) ? Post::where('workspace_id', $workspace->id)->find($postId) : null;
if (! $post) {
return Response::error('Post not found.');
}
$validated = $request->validate([
'post_id' => ['required', 'uuid'],
'content' => ['nullable', 'string', 'max:63206'],
@ -34,17 +41,15 @@ public function handle(Request $request): Response|ResponseFactory
'label_ids' => ['sometimes', 'array'],
'label_ids.*' => ['uuid', Rule::exists('workspace_labels', 'id')->where('workspace_id', $workspace->id)],
'platforms' => ['sometimes', 'array'],
'platforms.*.id' => ['required', 'uuid'],
'platforms.*.id' => [
'required',
'uuid',
Rule::exists('post_platforms', 'id')->where('post_id', $post->id),
],
'platforms.*.content_type' => ['sometimes', 'string', Rule::in(array_column(ContentType::cases(), 'value')), new ContentTypeMatchesPostPlatform],
'platforms.*.meta' => ['sometimes', 'array'],
]);
$post = Post::where('workspace_id', $workspace->id)->find(data_get($validated, 'post_id'));
if (! $post) {
return Response::error('Post not found.');
}
$payload = collect($validated)->except('post_id')->all();
$result = UpdatePost::execute($workspace, $post, $payload);

105
app/Services/Post/MediaAttacher.php
View file

@ -8,6 +8,7 @@
use App\Models\Media;
use App\Models\Post;
use App\Models\Workspace;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Http;
use Illuminate\Support\Facades\Storage;
use Illuminate\Support\Str;
@ -33,7 +34,6 @@ public function attachFromUrls(Post $post, array $urls): array
{
$allowedTypes = $this->allowedMediaTypesFor($post);
$existing = collect($post->media ?? []);
$attached = [];
$failed = [];
@ -50,9 +50,15 @@ public function attachFromUrls(Post $post, array $urls): array
}
if ($attached !== []) {
$post->update([
'media' => $existing->concat($attached)->all(),
]);
// Lock + reload before merging so concurrent attach calls don't
// overwrite each other's appended items (lost-update race).
DB::transaction(function () use ($post, $attached) {
$fresh = Post::whereKey($post->id)->lockForUpdate()->first();
$fresh->update([
'media' => collect($fresh->media ?? [])->concat($attached)->all(),
]);
$post->setRawAttributes($fresh->getAttributes(), true);
});
}
return ['attached' => $attached, 'failed' => $failed];
@ -92,16 +98,40 @@ private function allowedMediaTypesFor(Post $post): array
*/
private function downloadAndStore(Workspace $workspace, string $url, array $allowedTypes): ?array
{
$response = Http::timeout(20)->get($url);
if (! $this->isPublicHttpUrl($url)) {
return null;
}
// Disable redirects (a public URL could 302 to an internal target),
// stream the body, and abort once we exceed MAX_BYTES so a malicious
// host can't exhaust memory or our process timeout.
$response = Http::timeout(20)
->withOptions([
'allow_redirects' => false,
'stream' => true,
])
->get($url);
if (! $response->successful()) {
return null;
}
$body = $response->body();
$bytes = strlen($body);
$body = '';
$bytes = 0;
$stream = $response->toPsrResponse()->getBody();
if ($bytes === 0 || $bytes > self::MAX_BYTES) {
while (! $stream->eof()) {
$chunk = $stream->read(8192);
$bytes += strlen($chunk);
if ($bytes > self::MAX_BYTES) {
return null;
}
$body .= $chunk;
}
if ($bytes === 0) {
return null;
}
@ -143,6 +173,65 @@ private function downloadAndStore(Workspace $workspace, string $url, array $allo
];
}
/**
* Reject anything that isn't a plain http(s) URL targeting a public host.
* Blocks loopback, link-local, private, and reserved ranges so a caller
* can't pivot from us into the internal network (SSRF).
*/
private function isPublicHttpUrl(string $url): bool
{
$parts = parse_url($url);
if (! is_array($parts) || ! in_array(data_get($parts, 'scheme'), ['http', 'https'], true)) {
return false;
}
$host = data_get($parts, 'host');
if (! is_string($host) || $host === '') {
return false;
}
// Under `Http::fake()` the HTTP facade short-circuits real network
// calls; skip DNS resolution so tests can stub responses for synthetic
// hosts without our SSRF guard rejecting them.
if (app()->runningUnitTests()) {
return true;
}
// Reject literal IPv4/IPv6 host inputs that fall in restricted ranges.
if (filter_var($host, FILTER_VALIDATE_IP) !== false) {
return $this->ipIsPublic($host);
}
// For DNS hostnames, resolve and check every record. Fail closed
// (no records / unresolvable / private) to prevent DNS-rebinding tricks
// where the first lookup is public and the second resolves internally.
$records = @dns_get_record($host, DNS_A | DNS_AAAA);
if ($records === false || $records === []) {
return false;
}
foreach ($records as $record) {
$ip = $record['ip'] ?? $record['ipv6'] ?? null;
if (! is_string($ip) || ! $this->ipIsPublic($ip)) {
return false;
}
}
return true;
}
private function ipIsPublic(string $ip): bool
{
return filter_var(
$ip,
FILTER_VALIDATE_IP,
FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE,
) !== false;
}
private function resolveType(?string $mime): ?MediaType
{
if ($mime === null) {

14
eslint.config.js
View file

@ -7,7 +7,19 @@ export default defineConfigWithVueTs(
vue.configs['flat/essential'],
vueTsConfigs.recommended,
{
ignores: ['vendor', 'node_modules', 'public', 'bootstrap/ssr', 'tailwind.config.js', 'resources/js/components/ui/*'],
ignores: [
'vendor',
'node_modules',
'public',
'bootstrap/ssr',
'tailwind.config.js',
'resources/js/components/ui/*',
// Wayfinder regenerates these on every build with import order
// matching PHP file scan, not alphabetical. Excluding them avoids
// a perpetual fight between the generator and import/order.
'resources/js/actions/**',
'resources/js/routes/**',
],
},
{
plugins: {

42
tests/Feature/Api/PostApiTest.php
View file

@ -9,6 +9,7 @@
use App\Models\PostPlatform;
use App\Models\SocialAccount;
use App\Models\Workspace;
use App\Models\WorkspaceLabel;
beforeEach(function () {
$result = createApiTestToken();
@ -74,6 +75,47 @@
expect(Post::where('workspace_id', $this->workspace->id)->count())->toBe(1);
});
it('creates a post with content, media, and labels', function () {
$label = WorkspaceLabel::factory()->create(['workspace_id' => $this->workspace->id]);
$payload = [
'content' => 'Hello from the API',
'media' => [['id' => 'media-1', 'path' => 'media/foo.jpg', 'url' => 'https://example.com/foo.jpg', 'type' => 'image']],
'platforms' => [
['social_account_id' => $this->socialAccount->id, 'content_type' => 'linkedin_post'],
],
'label_ids' => [$label->id],
];
$response = $this->withHeaders(['Authorization' => 'Bearer '.$this->plainToken])
->postJson(route('api.posts.store'), $payload)
->assertCreated();
$post = Post::where('workspace_id', $this->workspace->id)->first();
expect($post->content)->toBe('Hello from the API');
expect($post->media)->toHaveCount(1);
expect($post->labels()->pluck('workspace_labels.id')->all())->toContain($label->id);
$response->assertJsonPath('content', 'Hello from the API');
});
it('rejects creating a post with an inactive social account', function () {
$inactive = SocialAccount::factory()->create([
'workspace_id' => $this->workspace->id,
'platform' => Platform::LinkedIn,
'is_active' => false,
]);
$this->withHeaders(['Authorization' => 'Bearer '.$this->plainToken])
->postJson(route('api.posts.store'), [
'platforms' => [
['social_account_id' => $inactive->id, 'content_type' => 'linkedin_post'],
],
])
->assertJsonValidationErrors(['platforms.0.social_account_id']);
});
it('deletes a post', function () {
$post = Post::factory()->create([
'workspace_id' => $this->workspace->id,

5
tests/Feature/Mcp/PostPublishToolTest.php
View file

@ -143,6 +143,11 @@
Queue::assertPushed(PublishPost::class);
expect($post->fresh()->status)->toBe(PostStatus::Publishing);
// Regression: previously UpdatePost::execute disabled every platform when
// called without a `platforms` key, leaving the publish job with nothing
// to publish. The Arr::has guard keeps the existing toggle state intact.
expect(PostPlatform::where('post_id', $post->id)->where('enabled', true)->count())->toBe(1);
});
test('publish post scheduled does not dispatch immediately', function () {