Code

Opened 6 years ago

Closed 3 years ago

#7025 closed New feature (wontfix)

SafeUnicode.split() should return a list of SafeUnicode objects

Reported by: guettli Owned by: nobody
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

Patch is attached.

Attachments (1)

7025.diff (3.3 KB) - added by guettli 6 years ago.
Improved patch: Overwrite splitlines() and not split()

Download all attachments as: .zip

Change History (8)

comment:1 Changed 6 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

If I do something like this

SmartUnicode(u"<").split("l")

the first component is no longer safe, since it contains an unescaped ampersand.

You might be able to come up with some extra logic to work around all those possibilities, but it might also end up making the resulting function a lot slower. Probably worth it if you can work around those cases without too much penalty, though.

See what you can come up with.

comment:2 Changed 6 years ago by guettli

Yes, Mallcolm, you are right. I updated my patch. It now overwrites splitlines() (that's what I intended with split()).
The patch overwrites strip, rstrip and lstrip, too. And it includes a unittest.

There is still a rare condition where splitlines() can fail: Inside a CDATA section. But I think it is safe to ignore this.

Do you still think this patch needs improvement?

Changed 6 years ago by guettli

Improved patch: Overwrite splitlines() and not split()

comment:3 Changed 6 years ago by guettli

  • Cc hv@… removed
  • Resolution set to wontfix
  • Status changed from new to closed

comment:4 Changed 6 years ago by kmtracey

  • Patch needs improvement unset
  • Resolution wontfix deleted
  • Status changed from closed to reopened

It is unclear why this was closed wontfix. Malcolm accepted the ticket and asked for an improved patch, which was provided. Even if the original submitter is no longer able to pursue the problem, the improved patch should be considered to fix the reported problem (since accepting implies the problem is real), correct?

comment:5 Changed 3 years ago by lukeplant

  • Patch needs improvement set

If there are cases where splitlines can fail to behave correctly with the proposed change, then we shouldn't do it, since it is security related (i.e. XSS attacks). The author didn't give a reason why we can safely ignore those circumstances. (Those circumstances being 'rare' isn't good enough, it needs to be 'never'). The other improvements (strip methods) are welcome though. Marking as 'needs improvement' on that basis.

comment:6 Changed 3 years ago by baumer1122

  • Severity set to Normal
  • Type set to New feature

comment:7 Changed 3 years ago by guettli

  • Resolution set to wontfix
  • Status changed from reopened to closed

I (the original author of this ticket) will close this again: safestring.splitlines() ist not safe! Your content could include
CDATA sections with newlines. Please leave this closed.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.