Recognize certain jQuery methods in validator



8 years ago
3 years ago


(Reporter: kmag, Unassigned)



(Whiteboard: [ReviewTeam:P1])



8 years ago
User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:6.0a1) Gecko/20110419 Firefox/6.0a1
Build Identifier: 

As much as I hate to admit it, jQuery is fantastically popular among add-on authors. I think it would be a good idea to recognize certain jQuery methods and process them in the same manner as the equivalent DOM methods. The two that come to mind at the moment are .html() which should be processed the same way as the .innerHTML property, and .attr() which should be processed roughly the same as setAttribute (although when given an object literal, that might be a bit difficult). I'm sure that there are others that would be of interest as well.

It's probably best to just process these as universal method calls. It seems to me that it would be more trouble than it's worth to try to track which objects are jQuery proxies.

Reproducible: Always
Assignee: nobody → mbasta
Ever confirmed: true
Priority: -- → P3
Whiteboard: [required amo-editors]
Target Milestone: --- → Q2 2011
That would probably be easiest. I could just use alises in instanceactions and instanceproperties. I think.
I'd imagine that we should also support .prop() for jQuery 1.6. Just using this as a note to myself...

Comment 3

8 years ago
After a cursory look over the API docs, these are the only other methods that leap out as especially dangerous (and they're probably rather more dangerous than the above):

Comment 4

8 years ago
Ah, alas, I've just run into an add-on which uses the following. Apparently they accept HTML strings as well as elements:

And I recall that you can do things like $("<html>...</html>"), jQuery("<html>...</html>"). I don't expect that there are sane ways to handle all of these. They'll probably just have to be ignored, but I guess they're worth mentioning nonetheless.
I just added the first three. With regard to the last three, we can just run the input through a filter similar to what we have for innerHTML. For $ as a function, we'd do the same thing but test the inside with a Regex for whether it's an HTML string or not.
Target Milestone: Q2 2011 → Q3 2011
This is pretty important, as it can lead to dangerous code execution. 

Here's an update from the mailing list. We need to detect the above, plus 'jQuery.get,, or jQuery.ajax with a data type containing "script" or "jsonp"'.
Severity: normal → major
Priority: P3 → P2
jQuery will automatically switch to JSONP mode if there is a question mark in the URL of an AJAX request after the GET parameter list has been started. I.e.:

We should test for this, too, as it's much more common with jQuery developers.

Comment 8

8 years ago
Is it? Ugh... I dislike jQuery more every day. I gave up trying to make sense of the code that handles XHRs after about five minutes. I wouldn't be surprised if there were dozens of ways for it to execute code that I don't know about.
I feel like this is going to be one hell of a patch.

Comment 10

8 years ago
These are the preliminary runtime tests that I'm experimenting with:

They might be marginally useful to you, but I suspect that the checks will be much more difficult statically...


8 years ago
Assignee: mattbasta → nobody
Component: Admin/Editor Tools → Add-on Validation
QA Contact: admin-tools → add-on-validation


8 years ago
Assignee: nobody → mattbasta
Target Milestone: Q3 2011 → Q1 2012
Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...
Whiteboard: [required amo-editors] → [ReviewTeam]
Resetting a bunch of missed milestones. Sorry for the bugspam.
Target Milestone: Q1 2012 → ---
Assignee: mattbasta → kmaglione+bmo


5 years ago
Whiteboard: [ReviewTeam] → [ReviewTeam:P1]

Comment 13

4 years ago
While Jquery is unique in its de facto across the web and add-ons, if another Library is used such as one from the following incomplete list: Wouldn't the main concern and problems still persist? 

LivePipe UI

Product: → Graveyard


3 years ago
Assignee: kmaglione+bmo → nobody
You need to log in before you can comment on or make changes to this bug.