Skip to content

Update table column affix helpers to support Htmlables#10649

Merged
danharrin merged 4 commits into
filamentphp:3.xfrom
emmadesilva:patch-2
Jan 8, 2024
Merged

Update table column affix helpers to support Htmlables#10649
danharrin merged 4 commits into
filamentphp:3.xfrom
emmadesilva:patch-2

Conversation

@emmadesilva
Copy link
Copy Markdown
Contributor

@emmadesilva emmadesilva commented Jan 4, 2024

Description

Updates the TableColumn ->prefix() and ->suffix() helpers support Htmlables like HtmlStrings.

Tables\Columns\TextColumn::make('name')
  ->suffix(function (Project $record): ?HtmlString {
      return $record->isActive()
        ? new HtmlString('<span class="text-green-500">Active</span>') 
        : null;
  })

Fixes #9729 and https://www.answeroverflow.com/m/1154022222118256700

  • Visual changes (if any) are shown using screenshots/recordings of before and after.

image

image

Code style

  • composer cs command has been run.

Testing

  • Changes have been tested.

Documentation

  • Documentation is up-to-date.

Comment thread packages/tables/src/Columns/Concerns/CanFormatState.php
Fixes filamentphp#9729 and https://www.answeroverflow.com/m/1154022222118256700 by making so that the `->prefix()` and  `->suffix()` helpers support Htmlables like HtmlStrings.
@emmadesilva emmadesilva marked this pull request as ready for review January 4, 2024 21:22
@emmadesilva
Copy link
Copy Markdown
Contributor Author

I'm not super well versed in PHPStan. Are the test failures my fault?

@danharrin danharrin added the enhancement New feature or request label Jan 5, 2024
@danharrin danharrin added this to the v3 milestone Jan 5, 2024
Copy link
Copy Markdown
Member

@danharrin danharrin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with this is that if the prefix/suffix are Htmlable, but the main content isn't, the main content is still open to XSS attacks as if it was

So when you set isHtml, you will also need to htmlencode the state if it is not Htmlable.

@emmadesilva
Copy link
Copy Markdown
Contributor Author

The problem with this is that if the prefix/suffix are Htmlable, but the main content isn't, the main content is still open to XSS attacks as if it was

So when you set isHtml, you will also need to htmlencode the state if it is not Htmlable.

Okay, how should we handle this? I think that the main content state should also support Htmlable, since the TextColumn state also supports it.

@danharrin
Copy link
Copy Markdown
Member

the main content state is the TextColumn state I was referring to

@emmadesilva
Copy link
Copy Markdown
Contributor Author

the main content state is the TextColumn state I was referring to

The TextColumn state is Htmlable, it's where I got the code for this PR from.

For example, here's the workaround I'm currently using to get a HTML suffix:

Tables\Columns\TextColumn::make('name')
    ->formatStateUsing(function (string $state, Project $record): HtmlString {
        $suffix = $record->isActive() ? view('components.filament.support.active-project-badge')->render() : '';

        return new HtmlString('<span class="flex">'.$state.$suffix.'</span>');
    })

@danharrin
Copy link
Copy Markdown
Member

I know, but using an Htmlable prefix/suffix should not also make the main content accept HTML. Its a security issue

@emmadesilva
Copy link
Copy Markdown
Contributor Author

I know, but using an Htmlable prefix/suffix should not also make the main content accept HTML. Its a security issue

Ah, now I get what you mean. My implementation has a side effect. You're more well versed in how these internals work, what do you think is a better way of doing this?

@danharrin
Copy link
Copy Markdown
Member

When you set $isHtml = true for the prefix/suffix, you will also need to e() the main content state.

@danharrin danharrin self-assigned this Jan 8, 2024
@danharrin danharrin merged commit caca2ea into filamentphp:3.x Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants