Closed Bug 421024 Opened 16 years ago Closed 16 years ago

[toolkit-parity] reorder suite "urlSecurityCheck" params

Categories

(SeaMonkey :: Security, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0a1

People

(Reporter: Callek, Assigned: Callek)

References

()

Details

(Keywords: addon-compat)

Attachments

(1 file, 1 obsolete file)

 
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
Target Milestone: --- → seamonkey2.0alpha
Attached patch rearrange params + unit test (obsolete) — Splinter Review
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)
(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?
Can we fix toolkit?
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.
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.
Depends on: 421874
Keywords: late-compat
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 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+
I'll check this in later
Attachment #307411 - Attachment is obsolete: true
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
Closed: 16 years ago
Resolution: --- → FIXED
Depends on: 422872
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: