Open Bug 371075 Opened 18 years ago Updated 2 years ago

should reftest use CheckLoadURI?

Categories

(Testing :: Reftest, defect)

defect

Tracking

(Not tracked)

People

(Reporter: dbaron, Unassigned)

Details

Attachments

(1 file)

I have a patch that makes reftest use CheckLoadURI. This might make remote reftest manifests reasonably sane (although I'd still recommend against them), although it's possible there could be further problems. However, my patch uses a deprecated method. I'm not sure I could, or should, make it use checkLoadURIWithPrincipal, or if this really makes sense at all.
Attached patch patch (deleted) — Splinter Review
Boris, thoughts? See previous comment.
Attachment #255838 - Flags: review?(bzbarsky)
What's the point in making remote loading with untrusted manifests work again? Reftest supports the functionality for which it's used (using a single in-tree manifest which can include other in-tree manifests). If you need server-side support for HTTP headers and other such things, adding support for CGI-like functionality to the HTTP server is high on my priority list, although I can't see that being ready for at least a couple weeks due to my schedule.
This is less about making anything work than about making evil things not work.
What evil things? I doubt anyone will manually run reftest with a remote untrusted manifest, let alone that anyone will create an evil manifest.
Being the default browser means somebody might be able to pass arguments to the browser giving a chosen reftest manifest, perhaps due to some other (otherwise minor?) security hole.
Does reftest does anything insecure though? It loads the documents and draws them on a canvas. Also, I don't think anyone will bother writing an exploit that can possibly affect only several thousands of users.
The patch looks OK to me. I think we should make getCodebasePrincipal() scriptable; then this code could use checkLoadURIWithPrincipal without too much trouble.
David, do you want to go ahead and just remove the [noscript] on getCodebasePrincipal and switch to using that per comment 7?
Comment on attachment 255838 [details] [diff] [review] patch This is fine if we want to do it.
Attachment #255838 - Flags: review?(bzbarsky) → review+
I checked the above patch into the trunk -- may get a chance to look at the other bit at some point...
Component: Testing → Reftest
Product: Core → Testing
Version: Trunk → unspecified
QA Contact: testing → reftest
Assignee: dbaron → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: