Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] Allow OutputFormatter::escape() to be used for escaping URLs used in <href>#44912
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedJan 5, 2022
Hey! To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done? Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review. Cheers! Carsonbot |
stof commentedJan 5, 2022
I would say that this makes sense (but it indeed needs tests covering it) |
Seldaek commentedJan 6, 2022
OK added tests to cover the new functionality and fixed existing ones 👍🏻 |
Uh oh!
There was an error while loading.Please reload this page.
08dc2bd to61f06faCompare…<href>- escape() now escapes `>` as well as `<`- URLs containing escaped `<` and `>` are rendered correctly as is- user-provided URLs should now be safe to use (as in they cannot break the formatting) as long as they're piped through `escape()`
fabpot commentedJan 7, 2022
Thank you@Seldaek. |
Uh oh!
There was an error while loading.Please reload this page.
I was trying to use escape() to make user-provided URLs safe in
<href=...>but I realized it was really only good for avoid starting tags, and not for escaping the content of a tag.>as well as<<,>are now rendered correctlyescape()'<'.OutputFormatter::escape($color).'>'where as in current released code it would not help you at all here. I haven't checked that yetI am happy to spend time adding tests but would like to first get feedback on the changes to know if it's reasonable or not to change
escape()in this way.The rest of the changes I think are absolutely safe to merge and make sense regardless.