[toolkit-parity] reorder suite "urlSecurityCheck" params

RESOLVED FIXED in seamonkey2.0a1

Status

SeaMonkey
Security
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Callek, Assigned: Callek)

Tracking

({addon-compat})

Trunk
seamonkey2.0a1
x86
Windows XP
addon-compat
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 years ago
 
(Assignee)

Comment 1

10 years ago
bah, mistyped an enter early.

We should fix our erroneous use of urlSecurityCheck params and match toolkit's use.  This so that we do not find ourselves having extensions (or even, in-app-code) accidentally hurt by the order.

Patch upcoming
Summary: [toolkit-parity] reorder suite "urlSecurityCheck → [toolkit-parity] reorder suite "urlSecurityCheck" params
(Assignee)

Updated

10 years ago
Target Milestone: --- → seamonkey2.0alpha
(Assignee)

Comment 2

10 years ago
Created attachment 307411 [details] [diff] [review]
rearrange params + unit test

this should do it, (took a bit more parity work than simply param order.

Also included the same test toolkit uses to keep us working here.
Assignee: nobody → bugspam.Callek
Status: NEW → ASSIGNED
Attachment #307411 - Flags: superreview?(neil)
Attachment #307411 - Flags: review?(neil)

Comment 3

10 years ago
(In reply to comment #1)
>We should fix our erroneous use of urlSecurityCheck params
You mean, we should match toolkit's erroneous use :-P

Maybe we should rename all our existing uses to checkLoadURIStrWithPrincipal?

Comment 4

10 years ago
Can we fix toolkit?

Comment 5

10 years ago
Or are they basically saying they disagree with the design of the security API (for that specific method) and find their order of arguments more intuitive (for themselves and extension developers)?

While we could rename our existing uses to checkLoadURIStrWithPrincipal, you'll still want the toolkit compat function, which apparently can take both a string and an nsIURI, so it seems like you'd end up duplicating quite a bit of code, or checkLoadURIStrWithPrincipal() just calls into urlSecurityCheck(), in which case why not just take the patch Callek wrote?
(In reply to comment #5)
> Or are they basically saying they disagree with the design of the security API
> (for that specific method) and find their order of arguments more intuitive
> (for themselves and extension developers)?

You'd have to ask Mano (and you can blame me for not noticing the disparity when reviewing bug 342485's patch). I wouldn't be opposed to fixing it, but we already took the extension compat hit once, I'm not sure that changing things again this late in the cycle would be a good idea.

Comment 7

10 years ago
I looked through about 100+ Firefox/Thunderbird extensions I have on my hard disk and none of them call "urlSecurityCheck". Unfortunately A.M.O. doesn't yet have LXR/MXR.
Well, there's bug 372304 for one - it's hard to know exactly how many extensions we have the potential of breaking. You could file a bug against AMO to request a manual grep of the extensions there (e.g. bug 421589). It may be that most extensions haven't yet been updated for Firefox 3, so maybe if we do it now we can get away with it.

Updated

10 years ago
Depends on: 421874

Updated

10 years ago
Keywords: late-compat

Comment 9

10 years ago
Yeah, I think we shouldn't break extensions this late in the game. Besides, I kinda like having the URL before the principal; it's the object you're interested in, and it matches the order of the "types" in the function names.

Comment 10

10 years ago
Comment on attachment 307411 [details] [diff] [review]
rearrange params + unit test

>+ *        or as a nsIURI object.
an nsIURI object

>+ *        The principal of the document from which aURL came.
Actually this should be the node, not the document
(although currently you get the same principal).

>+ *        Flags to be passed to checkLoadURIStr. If undefined,
...WithPrincipal

>+ *        nsIScriptSecurityManager.STANDARD will be passed.
Just say that nsIScriptSecurityManager.STANDARD is the default value.

>+  if (aFlags === undefined)
>+    aFlags = nsIScriptSecurityManager.STANDARD;
>+
Unnecessary. We left it out for a reason :-P

r=me with those fixed.
Attachment #307411 - Flags: superreview?(neil)
Attachment #307411 - Flags: superreview+
Attachment #307411 - Flags: review?(neil)
Attachment #307411 - Flags: review+
(Assignee)

Comment 11

10 years ago
Created attachment 309204 [details] [diff] [review]
Patch for checkin

I'll check this in later
Attachment #307411 - Attachment is obsolete: true
(Assignee)

Comment 12

10 years ago
Checking in browser/linkToolbarOverlay.js;
/cvsroot/mozilla/suite/browser/linkToolbarOverlay.js,v  <--  linkToolbarOverlay.js
new revision: 1.18; previous revision: 1.17
done
Checking in common/Makefile.in;
/cvsroot/mozilla/suite/common/Makefile.in,v  <--  Makefile.in
new revision: 1.5; previous revision: 1.4
done
Checking in common/contentAreaUtils.js;
/cvsroot/mozilla/suite/common/contentAreaUtils.js,v  <--  contentAreaUtils.js
new revision: 1.153; previous revision: 1.152
done
Checking in common/nsContextMenu.js;
/cvsroot/mozilla/suite/common/nsContextMenu.js,v  <--  nsContextMenu.js
new revision: 1.124; previous revision: 1.123
done
Checking in common/utilityOverlay.js;
/cvsroot/mozilla/suite/common/utilityOverlay.js,v  <--  utilityOverlay.js
new revision: 1.111; previous revision: 1.110
done
RCS file: /cvsroot/mozilla/suite/common/tests/Makefile.in,v
done
Checking in common/tests/Makefile.in;
/cvsroot/mozilla/suite/common/tests/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/suite/common/tests/unit/test_contentAreaUtils.js,v
done
Checking in common/tests/unit/test_contentAreaUtils.js;
/cvsroot/mozilla/suite/common/tests/unit/test_contentAreaUtils.js,v  <--  test_contentAreaUtils.js
initial revision: 1.1
done
Checking in makefiles.sh;
/cvsroot/mozilla/suite/makefiles.sh,v  <--  makefiles.sh
new revision: 1.9; previous revision: 1.8
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Depends on: 422872
You need to log in before you can comment on or make changes to this bug.