Closed Bug 292464 Opened 19 years ago Closed 19 years ago

event listeners added using addEventListener() listen only trusted events

Categories

(Core :: DOM: Events, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

Details

(Keywords: fixed-aviary1.0.5, fixed1.7.9, Whiteboard: need branch landing)

Attachments

(2 files, 2 obsolete files)

This is a regression from Bug 289940.
Patch coming.
Attached patch v.1 (obsolete) — Splinter Review
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)
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 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
Closed: 19 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 → ---
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-
Yeah, duh. This should get all places in the code where this is needed.
Attachment #182542 - Flags: superreview?(peterv)
Attachment #182542 - Flags: review?(smaug)
Attachment #182447 - Attachment is obsolete: true
Attachment #182447 - Flags: superreview?(peterv)
Attachment #182542 - Flags: review?(smaug) → review+
Attachment #182542 - Flags: superreview?(peterv) → superreview+
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
Closed: 19 years ago19 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 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+
Whiteboard: need branch landing
merged into aviary port of patch for bug 289940
Fix checked into mozilla 1.7 and aviary1.0.1 branches
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: