event listeners added using addEventListener() listen only trusted events

RESOLVED FIXED

Status

()

Core
DOM: Events
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

({fixed-aviary1.0.5, fixed1.7.9})

Trunk
x86
All
fixed-aviary1.0.5, fixed1.7.9
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.9 +
blocking-aviary1.0.5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: need branch landing)

Attachments

(2 attachments, 2 obsolete attachments)

This is a regression from Bug 289940.
Patch coming.
Created attachment 182281 [details] [diff] [review]
v.1

This fixes few things.

The size of the nsListenerStruct.mFlags is changed from 8 to 16 bits because
NS_PRIV_EVENT_UNTRUSTED_PERMITTED is 0x0800. (I could have changed the event
flags macros too, of course)

(ls->mFlags & ~NS_PRIV_EVENT_UNTRUSTED_PERMITTED) == aFlags is added so that
RemoveEventListener actually does something.

AddEventListener methods are changed so that in chrome they add listeners for
trusted events, otherwise also other events are handled.
Attachment #182281 - Flags: review?(jst)
Comment on attachment 182281 [details] [diff] [review]
v.1

Good catch, I'm surprised the compilers didn't whine about the mFlags problem.

- In nsDocument::AddEventListener():

+  nsIURI *docUri;
+  if ((docUri = NS_STATIC_CAST(nsIDocument*, this)->GetDocumentURI())) {
+    PRBool isChrome = PR_TRUE;
+    nsresult rv = docUri->SchemeIs("chrome", &isChrome);

Just use mDocumentURI here, no need for a local variable and a call to the
inline getter.

The rest looks fine to me. r=jst
Attachment #182281 - Flags: superreview?(peterv)
Attachment #182281 - Flags: review?(jst)
Attachment #182281 - Flags: review+
Flags: blocking1.8b2?
Status: NEW → ASSIGNED
Attachment #182281 - Flags: superreview?(peterv)
Created attachment 182341 [details] [diff] [review]
v1 using mDocumentURI
Attachment #182281 - Attachment is obsolete: true
Attachment #182341 - Flags: superreview?(peterv)
Attachment #182341 - Flags: superreview?(peterv) → superreview+
Attachment #182341 - Flags: approval1.8b2?
Is it possible for chrome documents to have other URI schemes (e.g., jar, file)?
 Should this instead be a principal check of some sort?
(In reply to comment #4)
> Is it possible for chrome documents to have other URI schemes (e.g., jar, file)?
>  Should this instead be a principal check of some sort?

Events are set trusted if the UniversalBrowserWrite is enabled.
http://lxr.mozilla.org/seamonkey/source/content/events/src/nsEventStateManager.cpp#4341

But for example XUL script event listeners are checking "chrome" scheme
http://lxr.mozilla.org/seamonkey/source/content/xul/content/src/nsXULElement.cpp#665

Which one should be used?

Comment 6

13 years ago
Comment on attachment 182341 [details] [diff] [review]
v1 using mDocumentURI

a=chofmann
Attachment #182341 - Flags: approval1.8b2? → approval1.8b2+
checked in

Checking in content/base/src/nsDocument.cpp;
/cvsroot/mozilla/content/base/src/nsDocument.cpp,v  <--  nsDocument.cpp
new revision: 3.549; previous revision: 3.548
done
Checking in content/base/src/nsGenericElement.cpp;
/cvsroot/mozilla/content/base/src/nsGenericElement.cpp,v  <--  nsGenericElement.cpp
new revision: 3.385; previous revision: 3.384
done
Checking in content/events/src/nsEventListenerManager.cpp;
/cvsroot/mozilla/content/events/src/nsEventListenerManager.cpp,v  <-- 
nsEventListenerManager.cpp
new revision: 1.200; previous revision: 1.199
done
Checking in content/events/src/nsEventListenerManager.h;
/cvsroot/mozilla/content/events/src/nsEventListenerManager.h,v  <-- 
nsEventListenerManager.h
new revision: 1.75; previous revision: 1.74
done
Checking in dom/src/base/nsGlobalWindow.cpp;
/cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v  <--  nsGlobalWindow.cpp
new revision: 1.733; previous revision: 1.732
done

If a principal check is wanted instead, please reopen.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Flags: blocking1.8b2?
Yeah, I do think we want that, but that's not critical for 1.8b2. Patch coming.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 182447 [details] [diff] [review]
Use the document's principals to check whether it's chrome or not.
Attachment #182447 - Flags: superreview?(peterv)
Attachment #182447 - Flags: review?(smaug)
Comment on attachment 182447 [details] [diff] [review]
Use the document's principals to check whether it's chrome or not.

jst, you added the IsScheme("chrome") check also elsewhere, at least:
http://lxr.mozilla.org/seamonkey/source/content/svg/content/src/nsSVGElement.cp
p#233
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsGenericElement.cpp#3
375
Those should be fixed too.
Attachment #182447 - Flags: review?(smaug) → review-
Created attachment 182542 [details] [diff] [review]
v2: Use the document's principals to check whether it's chrome or not.

Yeah, duh. This should get all places in the code where this is needed.
Attachment #182542 - Flags: superreview?(peterv)
Attachment #182542 - Flags: review?(smaug)

Updated

13 years ago
Attachment #182447 - Attachment is obsolete: true
Attachment #182447 - Flags: superreview?(peterv)
Attachment #182542 - Flags: review?(smaug) → review+
Attachment #182542 - Flags: superreview?(peterv) → superreview+
Blocks: 289940

Updated

13 years ago
Attachment #182542 - Flags: approval1.8b3?
Comment on attachment 182542 [details] [diff] [review]
v2: Use the document's principals to check whether it's chrome or not.

a=shaver
Attachment #182542 - Flags: approval1.8b3? → approval1.8b3+
Fixed.
Status: REOPENED → ASSIGNED
So is there something left to do here?  Or should this be resolved?
Flags: blocking1.8b3?
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED
Flags: blocking1.8b3?
it seems likely that this caused bug 296764 (arrow keys do not move cursor in
input fields with JS disabled). 
Depends on: 296764
Flags: blocking1.7.9+
Flags: blocking-aviary1.0.5+

Comment 16

13 years ago
Comment on attachment 182542 [details] [diff] [review]
v2: Use the document's principals to check whether it's chrome or not.

Please check this in on the Aviary branch for 1.0.5. a=jay
Attachment #182542 - Flags: approval-aviary1.0.5+

Updated

13 years ago
Whiteboard: need branch landing
merged into aviary port of patch for bug 289940
Fix checked into mozilla 1.7 and aviary1.0.1 branches
Keywords: fixed-aviary1.0.5, fixed1.7.9
You need to log in before you can comment on or make changes to this bug.