Skip to content

Conversation

@pabzm
Copy link
Member

@pabzm pabzm commented Oct 7, 2024

The CSP gets adapted to remote objects being allowed or not. It can be configured or disabled via the config option content_security_policy (and
content_security_policy_add_allow_remote).

The default is pretty weak in order to not introduce a breaking change, but still not useless (e.g. it adds another layer of defence against loading remote objects unless allowed.)

Closes #9638

alecpl and others added 30 commits April 21, 2024 11:33
…dcube#9422)

* \n\s+'file' => __FILE__,

* \n\s+'line' => __LINE__,

* 'line' => __LINE__, 'file' => __FILE__,

* 'file' => __FILE__, 'line' => __LINE__,

* rest

* more

* improve cs

* more cs

* revert rcube_utils::preg_error changes

* impl file/line from backtrace

* Revert "revert rcube_utils::preg_error changes"
As stated in roundcube#5574 (comment) redis is supported, adding it to supported backed

[skip ci]
The eslint package "eslint-plugin-unicorn" introduced a new rule in
version 53.0.0 that suggests to use `String.raw` in order to avoid the
need for escaping backslashes.

While this certainly is a good idea we cannot follow that rule as long
as we support Internet Explorer 11+, which doesn't support template
literals (and thus `String.raw`).
Fix Code Spell check by disabling `String.raw` rule
[skip ci]
… Windows

Reported by Huy Nguyễn Phạm Nhật.
… from user preferences

Reported by Huy Nguyễn Phạm Nhật.
…attributes

Reported by Valentin T. and Lutz Wolf of CrowdStrike.
[skip ci]
[skip ci]
Added JetBrains PhpStorm project folder (.idea)
[skip ci]
ShE3py and others added 20 commits January 26, 2025 14:57
The CSP gets adapted to remote objects being allowed or not.
It can be configured or disabled via the config option
`content_security_policy` (and
`content_security_policy_add_allow_remote`).
If people write a lax default CSP they might set the additional config
option to the blank string, or false. Then the CSP header should not
contain that value.
This way the code always has values it can work with, no matter how good
or broken the
configuration is.
Previously the code also treated `'false'` (the string) as invalid, but
that's a very specific check against a specific edge case, which
wouldn't even break the code (but will only make the browser complain),
so I'm dropping that check.
The config value might be something else than a string.
In the previous way useless semicolons could have happened.
We still support PHP v7, which doesn't support union types.
phpstan complains that `assertIsArray($config)` will always fail because
it doesn't know about the side effects of `require`ing the default
config.
A "lax" CSP (aka using the config option
`content_security_policy_add_allow_remote`) is required to allow using remote
ressources like image URLs in the HTML editor.

We can't depend this on the intial content being HTML or not because the
user might want to change the editor after loading the page, and then
add remote ressources.
@pabzm
Copy link
Member Author

pabzm commented Feb 3, 2025

I removed the wrong colons and I think I fixed the usage of the cases you mentioned, thank you for those hints!

I found another problem: If a mailvelope user wants to automatically lookup missing openpgp keys (to encrypt a message to), the HTTP request is sent from JavaScript directly to the configured keyserver. This would be blocked by a strict CSP.

I see two options:

  • Include the configured keyserver as allowed source in the strict CSP, or
  • Proxy the search requests through the server and keep the strict CSP as it is.

The latter would keep the security advantage of a stricter CSP, and also improve the user's privacy slightly because it won't allow for profiling based on the client IP address.
This would also be beneficial for a future feature, to lookup keys in a WKD, for which hiding client IP addresses would be nice, too.

@alecpl Do you have an opinion on this?

@alecpl
Copy link
Member

alecpl commented Feb 3, 2025

That reminds me of a key search in the enigma plugin, which may have the same issue, I suppose.

I'm not sure we can fix the HTML composer this way, though. When you compose a reply/forward you deal with the original mail content that should get secured, don't you think? Marking the content safe does not look right.

As I said in the beginning this becomes complicated and problematic.

@pabzm
Copy link
Member Author

pabzm commented Feb 3, 2025

Enigma calls the same JavaScript-function as the Mailvelope-integration.

Good point about the replies!

I've changed the code to not use the safe mode for triggering the lax CSP but a dedicated variable. (Actually I had this change set in place once before but discarded it to save a variable and jump onto safemode… should've trusted my initial idea!)

(I'll happily clean up the code before it would be merged, just don't want to stir around in the commits as long as they're continuously reviewed.)

@pabzm pabzm marked this pull request as draft February 20, 2025 11:18
@pabzm pabzm moved this from 🏗️ In progress to 📄 To do in 💌 📅 👥 Groupware team Jun 18, 2025
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.

Document a working Content-Security-Policy