themesberg / flowbite-react

Official React components built for Flowbite and Tailwind CSS
https://flowbite-react.com
MIT License
1.77k stars 395 forks source link

Fix CSP violation in Carousel drag-scroll helper #1289

Closed rnicholus closed 3 months ago

rnicholus commented 4 months ago

Users of flowbite-react with a strict CSP will not permit inline styles. In order to whitelist inline styles in a CSP, you must add the unsafe-inline value to the style-src directive. Allowing inline styles opens the application up to cross-site scripting attacks (XSS).

The <Carousel> component in flowbite-react indirectly introduces inline styles via the react-indiana-drag-scroll library, which inlines all of its styles via the build process.

I considered updating react-indiana-drag-scroll's build config to avoid inlining the styles, but that would introduce a signifiant breaking change for all other users of the library. So, given the small size of the dependency's codebase (1 file) and the fact that the library is already stable (no changes for 2+ years), I felt it was best to simply hoist the code into react-flowbite and do away with the inline styles by leaning on TailwindCSS classes instead. react-indiana-drag-scroll's MIT license fully permits this move.

The 2 new dependencies were dependencies of react-indiana-drag-scroll. I attempted to limit changes to that code as much as possible, to avoid regressions. But some changes were needed to address improper use of TypeScript in that project.

I did some basic manual tests in the development environment on the carousel page, and everything appears to work as expected.

I did not add tests as the dependency itself also did not have any tests to speak of.

stackblitz[bot] commented 4 months ago

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

vercel[bot] commented 4 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
flowbite-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 11, 2024 1:02pm
codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 53.21337% with 182 lines in your changes are missing coverage. Please review.

Project coverage is 95.56%. Comparing base (7461173) to head (d069344). Report is 197 commits behind head on main.

Files Patch % Lines
src/helpers/drag-scroll/index.tsx 53.09% 182 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1289 +/- ## ========================================== - Coverage 99.54% 95.56% -3.98% ========================================== Files 163 217 +54 Lines 6621 9612 +2991 Branches 401 550 +149 ========================================== + Hits 6591 9186 +2595 - Misses 30 426 +396 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

SutuSebastian commented 4 months ago

Thanks for the digging!

I was thinking more like deprecating altogether react-indiana-drag-scroll, and start using something more new, stable and maintained.

@rluders @tulup-conner what do u guys think?

rluders commented 4 months ago

This is a hard question... I have no strong opinion about it. AFAIR, using the react-indiana-drag-scroll was 'cause it was very stable and pretty direct to solve the need that we had. I'm not against accepting this PR, nor searching a new library to replace the current dependency. I think that I would like to have some options to consider, some kind of cons and pros are always helpful in deciding what we want to do.

Also, let me point out that, IMHO, I would not care that much about introducing breaking changes before we achieve the 1.0.0 release. I guess that we still at some point can experiment and do these kinds of things, however, I do understand that the pre-release version is aiming to leave the current library and API as stable as possible until we arrive at the official release.

@SutuSebastian do you have any library in mind? What would be the benefits if compared with the proposal on this PR?

SutuSebastian commented 4 months ago

For any swipe/scroll gestures, I'd definitely use either https://swiperjs.com/ or https://dndkit.com/.

SutuSebastian commented 4 months ago

Or why not, since this is about carousel, https://www.embla-carousel.com/.

rnicholus commented 4 months ago

Hey everyone, thanks for the quick responses.

The change I have here fixes an immediate issue for users of your library, and merging this doesn’t prevent you from coming back later and replacing the carousel component or the drag-scroll logic if you see a need.

Just my 2 cents.

rluders commented 4 months ago

@rnicholus agreed. I think that we can get this one merged, and open a new issue to investigate the replacement (it there is any benefit).