Collection description not properly escaped; less-than "<" and characters after it are not saved

VERIFIED FIXED in 5.1

Status

defect
P3
normal
VERIFIED FIXED
10 years ago
3 years ago

People

(Reporter: davemgarrett, Assigned: cesar)

Tracking

unspecified

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

10 years ago
Less-than ("<") characters are not saved in collection descriptions. Greater-than (">") characters are saved, however not if they are preceded by their counterpart. i.e. "<<>>>" saves as ">". Clearly something isn't being escaped correctly here.
Reporter

Comment 1

10 years ago
In fact, do something like "<asdgasgd>" and none of it will be saved.

See also bug 498080 & bug 507834.
Reporter

Updated

10 years ago
Summary: Collection description not properly escaped; cannot add a less-than "<" to description → Collection description not properly escaped; less-than "<" and characters after it are not saved

Comment 2

10 years ago
This is not only for collection descriptions, but also for many other user-provided data on AMO.  See bug 493247 comment 2.

Comment 3

10 years ago
(In reply to comment #0)
> Less-than ("<") characters are not saved in collection descriptions.
> Greater-than (">") characters are saved, however not if they are preceded by
> their counterpart. i.e. "<<>>>" saves as ">". Clearly something isn't being
> escaped correctly here.

This might be merely a difference of terminology, but I do not think this is a matter of escaping, but this is a matter of so-called “sanitizing.”

It seems that in many cases, AMO “sanitizes” input assuming incorrectly that the input is meant to be HTML; that is, it assumes that “<...>” in the input is meant to be an HTML tag and removes it.  This prevents a user from entering angle brackets even when they are meant to be simply angle brackets, not an HTML tag.

A better way is to treat input as plaintext and escape “<” to “&lt;” and so on when generating HTML (that is, when storing it to the database if the values in the database are HTML, or when producing the output for the user if the values in the database are plaintext).

Comment 4

10 years ago
I noticed bug 343573.  Guessing from that bug, maybe collection descriptions (as well as the content of many of the other text fields) are meant to be HTML, and the support for tags has merely not been implemented yet.  If this is the case, removing “<<>>” might be correct because it is not correct as HTML.  In that case, we should be able to write “&lt;&lt;&gt;&gt;” to represent that string, but unfortunately, this has not been implemented, either.
Reporter

Updated

10 years ago
Duplicate of this bug: 510704
Target Milestone: --- → 5.1
Priority: -- → P3
Assignee

Comment 6

10 years ago
Posted patch don't strip the tags (obsolete) — Splinter Review
The reason is that the Amo->clean() method calls php's strip_tag, which removes anything that is even remotely possible to being a HTML/PHP tag. We already sanitize the data when we output. So I think it's safe to tell the clean() method to skip calling strip_tags, and just save (possible) HTML in the DB.
Assignee: nobody → cdolivei.bugzilla
Status: NEW → ASSIGNED
Attachment #399665 - Flags: review?(rdoherty)
Reporter

Comment 7

10 years ago
Cesar: You think you could also apply the same quick fix for bug 493247? There's probably some other fields that do this too.
Attachment #399665 - Flags: review?(rdoherty) → review-
Comment on attachment 399665 [details] [diff] [review]
don't strip the tags

This works when I edit a collection, but not when I'm creating a new one. Characters are stripped on creation.
Assignee

Comment 9

10 years ago
Posted patch oopsSplinter Review
oops. my bad. thanks for catching that!
Attachment #399665 - Attachment is obsolete: true
Attachment #400411 - Flags: review?(rdoherty)
Comment on attachment 400411 [details] [diff] [review]
oops

Looks good!
Attachment #400411 - Flags: review?(rdoherty) → review+
Today is code freeze, not waiting for a commit anymore. :) r51811

Thanks cesar/ryan.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Verified FIXED on https://preview.addons.mozilla.org/en-US/firefox/collections/edit/8de4c068-13d0-94ed-ae52-fc9550c6bcce; I tested both creating and editing a collection with < and > in its description.
Status: RESOLVED → VERIFIED
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.