Skip to content

new crate feature: allow_all_protocols_in_img #165

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

Merged
merged 11 commits into from
Feb 28, 2025

Conversation

lovasoa
Copy link
Contributor

@lovasoa lovasoa commented Feb 26, 2025

test "unsafe" protocols on image sources here: https://codepen.io/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx-/pen/wBvzLmv

This is a follow-up on #164 (comment)

@wooorm
Copy link
Owner

wooorm commented Feb 26, 2025

I appreciate where this is going! Thank you for your work!

Most things here are options here. What’s the reason you wrapped this in a feature? 🤔 I have used features here because this is a no_std + alloc crate by default.

Alternatively, for an option name, how about an option name: allow_any_src?

For the docs, then, for § Security, I am thinking of something like:

You should be able to set `allow_any_src` safely.
The default is to allow only `http:`, `https:`, and relative images,
which is what GitHub does.
But it should be safe to allow any value on `src`.
The [HTML specification][whatwg-html-image] prohibits dangerous scripts in
images and all modern browsers respect this and are thus safe.
Opera 12 (from 2012) is a notable browser that did not respect this.

…

[whatwg-html-image]: https://html.spec.whatwg.org/multipage/images.html#images-processing-model

I tried to show why things are the way they are, and why, as you show, it should be safe to deviate from those defaults.

@lovasoa
Copy link
Contributor Author

lovasoa commented Feb 26, 2025

agreed, an option is better than a crate feature (which I thought you wanted given your previous comments).

Can we name it allow_any_img_src, since it's only about img tags ?

@wooorm
Copy link
Owner

wooorm commented Feb 27, 2025

Sorry, I did not mean to imply a crate feature!

allow_any_img_src sounds great 👍 to me!

@lovasoa
Copy link
Contributor Author

lovasoa commented Feb 27, 2025

I changed it to a configuration option instead of a crate feature.

I also updated the readme. I put the section about Opera above, since it's not related to the new allow_any_img_src option (opera executed scripts from images served over http as well).

You may also want to add a separate subsection about external images, warning about user tracking and CSRF attacks. Something like

Allowing external images still allows an attacker to perform an HTTP GET request
using the user's stored cookies, IP address, user agent, referer, etc.
This allows them to track the user, or perform CSRF attacks.
Depending on your threat model, you may want to store images on your own server.

Copy link
Owner

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

Thank you!

I think the Security section is not yet ready.

The rest of my suggestions are just some rephrasing and line wrapping.

The code looks 👍

@lovasoa lovasoa requested a review from wooorm February 27, 2025 13:02
@wooorm wooorm merged commit e923a3c into wooorm:main Feb 28, 2025
3 checks passed
@lovasoa lovasoa deleted the allow_all_protocols_in_img branch February 28, 2025 10:26
@lovasoa
Copy link
Contributor Author

lovasoa commented Feb 28, 2025

Great, thank you ! Can you make a new release so that I can use it in my crate ?

@wooorm
Copy link
Owner

wooorm commented Feb 28, 2025

Thank you! Released in 1.0.0-alpha.23!

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.

2 participants