Skip to content

Conversation

@Janpot
Copy link
Member

@Janpot Janpot commented Sep 15, 2025

Avoid injecting unsanitized html from third party ad networks.

Credit goes to @Gyde04, thank you for reporting!

Preview https://deploy-preview-46936--material-ui.netlify.app/material-ui/react-button/

@mui-bot
Copy link

mui-bot commented Sep 15, 2025

Netlify deploy preview

https://deploy-preview-46936--material-ui.netlify.app/

Bundle size report

Bundle Parsed size Gzip size
@mui/material 0B(0.00%) 0B(0.00%)
@mui/lab 0B(0.00%) 0B(0.00%)
@mui/system 0B(0.00%) 0B(0.00%)
@mui/utils 0B(0.00%) 0B(0.00%)

Details of bundle changes

Generated by 🚫 dangerJS against 1c72895

@Janpot Janpot added security Pull requests that address a security vulnerability. scope: code-infra Involves the code-infra product (https://www.notion.so/mui-org/5562c14178aa42af97bc1fa5114000cd). labels Sep 15, 2025
@Janpot Janpot changed the title [code-infra] Remove dangerouslySetInnerHTML for ad description [code-infra] Remove dangerouslySetInnerHTML for ad description Sep 15, 2025
@Janpot Janpot marked this pull request as ready for review September 15, 2025 12:07
@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation. label Sep 15, 2025
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I don't expect regressions from those changes; it seems OK.

On the security part, this seems far-fetched, we grant all permissions to https://cdn.carbonads.com/carbon.js by loading this as a script, which uses .innerHTML to inject the description, so they already have to sanitize the ad description, so this should make no difference.

However, forcing strings enforces consistency in the ad layout, which seems like a positive UI change.

@oliviertassinari oliviertassinari changed the title [code-infra] Remove dangerouslySetInnerHTML for ad description [docs-infra] Remove dangerouslySetInnerHTML for ad description Sep 15, 2025
@oliviertassinari oliviertassinari added scope: docs-infra Involves the docs-infra product (https://www.notion.so/mui-org/b9f676062eb94747b6768209f7751305). and removed scope: code-infra Involves the code-infra product (https://www.notion.so/mui-org/5562c14178aa42af97bc1fa5114000cd). labels Sep 15, 2025
@Janpot
Copy link
Member Author

Janpot commented Sep 16, 2025

On the security part, this seems far-fetched, we grant all permissions to https://cdn.carbonads.com/carbon.js by loading this as a script, which uses .innerHTML to inject the description, so they already have to sanitize the ad description, so this should make no difference.

We're loading these ads from https://srv.buysellads.com/. I think this makes some difference, we halved the amount of ad domains that can inject code in our pages.

response = await fetch('https://srv.buysellads.com/ads/CE7DC23W.json');

@Janpot Janpot merged commit e9183d0 into mui:master Sep 16, 2025
22 of 23 checks passed
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 16, 2025

We're loading these ads from https://srv.buysellads.com/

For context, srv.buysellads.com and cdn.carbonads.com are operated by the same product.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Improvements or additions to the documentation. scope: docs-infra Involves the docs-infra product (https://www.notion.so/mui-org/b9f676062eb94747b6768209f7751305). security Pull requests that address a security vulnerability.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants