The default bug view has changed. See this FAQ.

Need to rework checking of URI so that it is not called multiple times

RESOLVED WORKSFORME

Status

()

Core
Security
RESOLVED WORKSFORME
14 years ago
9 months ago

People

(Reporter: Ian Neal, Assigned: Ian Neal)

Tracking

(Blocks: 2 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 obsolete attachments)

(Assignee)

Description

14 years ago
Currently, for certain clicks on links, urlSecurityCheck is called more than
once, this should not be necessary.
(Assignee)

Updated

14 years ago
Blocks: 84128
(Assignee)

Updated

14 years ago
Blocks: 193255
(Assignee)

Comment 1

14 years ago
Created attachment 132514 [details] [diff] [review]
Initial Patch v0.4d

This initial patch removes the call to urlSecurityCheck from the beginning of
handleLinkClick function and adds new calls futher within the function where
urlSecurityCheck would have no longer been called because of the removal.

I'm not sure that we do actually need a call to urlSecurityCheck for the right
click (event.button=2) but I have it in there for the moment.
(Assignee)

Comment 2

14 years ago
Created attachment 133087 [details] [diff] [review]
Patch v0.4e - removes duplicate calls to urlSecurityCheck

This patch removes the first call to urlSecurityCheck in handleLinkClick and
adds new calls where a call to an |openNew| function returns false, an
|openNew| function is not called and wrt saveModifier where handleLinkClick is
called with linkNode not being null.
(Assignee)

Updated

14 years ago
Attachment #132514 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #133087 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

14 years ago
Accepting
Status: NEW → ASSIGNED
Comment on attachment 133087 [details] [diff] [review]
Patch v0.4e - removes duplicate calls to urlSecurityCheck

This really needs review from caillon....

Why the "if (linkNode)" check, by the way?
Attachment #133087 - Flags: review?(bzbarsky) → review?(caillon)
(Assignee)

Comment 5

14 years ago
Well I'm pretty sure that I don't need the check when the linkNode is null but
I'm presuming that extensions could call this with linkNode not being null
otherwise why do we still take the linkNode argument in handleLinkClick? Mozilla
itself only calls it with a null for that argument.
My question is, why does it matter whether linkNode is null?  The URL you are
testing is not associated with linkNode at this point....

And yes, linkNode used to be used and then someone tweaked something, I guess.
Eek, why was I not cc'd?
Comment on attachment 133087 [details] [diff] [review]
Patch v0.4e - removes duplicate calls to urlSecurityCheck

There's just something about this patch that I don't like.  Maybe it's the fact
that now there are so many seemingly random security checks.  Some callers have
them, others don't, etc.

Would it make more sense to require the callers of anything that loads a URL to
do the check before calling the handleLinkClick/doOpenNewFoo methods?

Also note that the security check is relatively cheap, so it doesn't really
hurt anything here.  The performance impact of doing perhaps an extra security
check is negligible, and this bug may not be worth fixing.  It's better to do
too many checks than too few....
(Assignee)

Comment 9

14 years ago
Created attachment 133892 [details] [diff] [review]
Patch v0.4f - reworks contentAreaClick function removing duplicate calls to urlSecurityCheck

Patch reworks contentAreaClick function by inling handleLinkClick function and
moving call to urlSecurityCheck into openNewTabOrWindow function where it
hasn't previously been called from openNew* functions.

Need to fix this so that bug 84128 can be fixed as multiple calls to
urlSecurityCheck would cause multiple user alerts.
(Assignee)

Updated

14 years ago
Attachment #133087 - Attachment is obsolete: true
(Assignee)

Comment 10

14 years ago
Comment on attachment 133087 [details] [diff] [review]
Patch v0.4e - removes duplicate calls to urlSecurityCheck

Cancelling previous request
Attachment #133087 - Flags: review?(caillon)
(Assignee)

Updated

14 years ago
Attachment #133892 - Flags: review?(caillon)
(Assignee)

Updated

13 years ago
Attachment #133892 - Flags: review?(caillon)
(Assignee)

Comment 11

13 years ago
Created attachment 163169 [details] [diff] [review]
Patch v0.5 - makes sure urlSecurityCheck is called

This patch:
* Fixes the duplicate calls to urlSecurityCheck in handleLinkClick
  - instead of checking at start of function only checks if openNewWindowWith
    or openNewTabWith have not been called.
* Fixes the fact that urlSecurityCheck is not called before trying to load the
url in middleMousePaste function if ctrl key is not being pressed
Attachment #133892 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #163169 - Flags: review?(caillon)
(Assignee)

Comment 12

13 years ago
I guess firefox might need a similar middleMousePaste fix.
QA Contact: carosendahl → toolkit
Comment on attachment 163169 [details] [diff] [review]
Patch v0.5 - makes sure urlSecurityCheck is called

This is no longer anything like the current code.
Attachment #163169 - Attachment is obsolete: true
Attachment #163169 - Flags: review?(caillon)
This seems to have effectively been fixed when this function was rewritten at some point.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.