Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
fabpot merged 1 commit intosymfony:4.4fromSeldaek:patch-18
Jan 7, 2022

Conversation

@Seldaek
Copy link
Member

@SeldaekSeldaek commentedJan 5, 2022
edited
Loading

QA
Branch?4.4
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix #...
LicenseMIT
Doc PRsymfony/symfony-docs#...

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.

  • escape() now escapes> as well as<
  • URLs containing escaped<,> are now rendered correctly
  • user-provided URLs should now be safe to use (as in they cannot break the formatting) as long as they're piped throughescape()
  • possibly also resolves issues if you were trying to use user-provided colors i.e.'<'.OutputFormatter::escape($color).'>' where as in current released code it would not help you at all here. I haven't checked that yet

I 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 changeescape() in this way.

The rest of the changes I think are absolutely safe to merge and make sense regardless.

@carsonbot
Copy link

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

stof commentedJan 5, 2022

I would say that this makes sense (but it indeed needs tests covering it)

@SeldaekSeldaek marked this pull request as ready for reviewJanuary 5, 2022 11:45
@carsonbotcarsonbot added this to the4.4 milestoneJan 5, 2022
@Seldaek
Copy link
MemberAuthor

OK added tests to cover the new functionality and fixed existing ones 👍🏻

@SeldaekSeldaekforce-pushed thepatch-18 branch 2 times, most recently from08dc2bd to61f06faCompareJanuary 7, 2022 08:38
…<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
Copy link
Member

Thank you@Seldaek.

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

Reviewers

@fabpotfabpotfabpot approved these changes

@chalasrchalasrAwaiting requested review from chalasrchalasr is a code owner

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

4 participants

@Seldaek@carsonbot@stof@fabpot

[8]ページ先頭

©2009-2025 Movatter.jp