Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CSP violation in Carousel drag-scroll helper #1289

Merged
merged 3 commits into from
Mar 12, 2024

Conversation

rnicholus
Copy link
Contributor

@rnicholus rnicholus commented Mar 8, 2024

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.

Copy link

stackblitz bot commented Mar 8, 2024

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

Copy link

vercel bot commented Mar 8, 2024

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

Copy link

codecov bot commented Mar 8, 2024

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 ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SutuSebastian
Copy link
Collaborator

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
Copy link
Collaborator

rluders commented Mar 8, 2024

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
Copy link
Collaborator

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

@SutuSebastian
Copy link
Collaborator

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

@rnicholus rnicholus changed the title Fix CSP violation is Carousel drag-scroll helper Fix CSP violation in Carousel drag-scroll helper Mar 8, 2024
@rnicholus
Copy link
Contributor Author

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
Copy link
Collaborator

rluders commented Mar 8, 2024

@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).

styles/globals.css Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants