Open Bug 1474525 Opened 6 years ago Updated 2 years ago

Characters in tag names are escaped when they shouldn't be (& turns into &)

Categories

(Firefox :: Pocket, defect, P3)

61 Branch
defect

Tracking

()

REOPENED

People

(Reporter: ori, Unassigned, NeedInfo)

Details

Attachments

(1 file)

Tested in Firefox 61, but maybe happens in later versions.

Steps to reproduce.

1) Click on the "Add to pocket" button.
2) In the dialog that appears, enter the tag nam "&", and confirm.

Results:
Right before the dialog closes, "&" will turn into "&".

I can later go to Pocket's website and rename the tag to "&", so this is not a technical limitation of the Pocket service.
MozReview-Commit-ID: 3KZii6HubeI
Hi everyone, here is a patch I've came up with. 

I have found that Pocket do not escape &,' and " characters so we can also pass them with no escaping. Pocket also do not accept < and > signs completely so there is no need to escape them and we can just replace them with nothing. 

I always feel weird when I remove something in codebase I'm not familiar with, so let me know if I did something wrong. 

P.S.
I also wasn't able to push to MozReview and had to use `hg bzexport` which seems added my commit to MozReview anyway. Let me know if I messed up here as well.
Attachment #9002243 - Flags: review?(justin)
Hi Justin, 
excuse me if I shouldn't add you as a reviewer. System has suggested you and I don't really know who should be assign as a reviewer.
Assignee: nobody → furyinbox
Status: NEW → ASSIGNED
Comment on attachment 9002243 [details] [diff] [review]
change the way to escape tags

Review of attachment 9002243 [details] [diff] [review]:
-----------------------------------------------------------------

I don't feel comfortable making this change. We shouldn't be writing our own sanitization code and instead rely on a trusted and tested implementation. We should evaluate all code paths that interact with this and see why it is/was necessary. Strings should only be converted to HTML entities when they are written as HTML output, not when stored in the database.

We could do something that will convert more characters to HTML entities if we determine that this is necessary,
```js
return s.replace(/[\u00A0-\u9999<>\&\'\"]/gim, i => `&#${i.charCodeAt(0)};`);
```

In addition, there is a duplicate of this code in signup.js that will need to be updated or removed as well (it looks like it's unused).
Attachment #9002243 - Flags: review?(justin)
Attachment #9002243 - Flags: review?(jaws)
Attachment #9002243 - Flags: review-
Thanks a lot for the review Jared. Good catch about signup.js. I'll take a look what i can do here.

This bug has been closed due to inactivity and/or the potential for this bug to no longer be an issue with the new Discovery Stream-powered New Tab experience, and improvements to Pocket’s integration with Firefox.

Please help us triage by reopening if this issue still persists and should be addressed.

Thanks!

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → INACTIVE

Reopening as this bug still occurs in Firefox Nightly.

Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---

Thank you for reopening, Ori!

Priority: -- → P3

The bug assignee didn't login in Bugzilla in the last 7 months.
:thecount, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: furyinbox → nobody
Flags: needinfo?(sdowne)
Severity: minor → S4
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: