Open
Bug 371075
Opened 18 years ago
Updated 2 years ago
should reftest use CheckLoadURI?
Categories
(Testing :: Reftest, defect)
Testing
Reftest
Tracking
(Not tracked)
NEW
People
(Reporter: dbaron, Unassigned)
Details
Attachments
(1 file)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•18 years ago
|
||
Boris, thoughts? See previous comment.
Attachment #255838 -
Flags: review?(bzbarsky)
Comment 2•18 years ago
|
||
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.
Reporter | ||
Comment 3•18 years ago
|
||
This is less about making anything work than about making evil things not work.
Comment 4•18 years ago
|
||
What evil things? I doubt anyone will manually run reftest with a remote untrusted manifest, let alone that anyone will create an evil manifest.
Reporter | ||
Comment 5•18 years ago
|
||
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.
Comment 6•18 years ago
|
||
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.
Comment 7•18 years ago
|
||
The patch looks OK to me. I think we should make getCodebasePrincipal() scriptable; then this code could use checkLoadURIWithPrincipal without too much trouble.
Comment 8•18 years ago
|
||
David, do you want to go ahead and just remove the [noscript] on getCodebasePrincipal and switch to using that per comment 7?
Comment 9•17 years ago
|
||
Comment on attachment 255838 [details] [diff] [review]
patch
This is fine if we want to do it.
Attachment #255838 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 10•17 years ago
|
||
I checked the above patch into the trunk -- may get a chance to look at the other bit at some point...
Reporter | ||
Updated•16 years ago
|
Component: Testing → Reftest
Product: Core → Testing
Version: Trunk → unspecified
Updated•16 years ago
|
QA Contact: testing → reftest
Reporter | ||
Updated•4 years ago
|
Assignee: dbaron → nobody
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•