Summary: | Add support for registering noAccess URL schemes | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Fisher (:fishd, Google) <fishd> | ||||
Component: | Frames | Assignee: | Darin Fisher (:fishd, Google) <fishd> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, sam | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Darin Fisher (:fishd, Google)
2009-03-06 15:53:12 PST
Created attachment 28377 [details]
v1 patch
I also added a bit of cleanup to this patch as well:
- moved localSchemes to the top of the file
- renamed LocalSchemesMap to URLSchemesMap
- renamed shouldTreatSchemeAsLocal to shouldTreatURLSchemeAsLocal for consistency
Comment on attachment 28377 [details] v1 patch >+ // Grant the inspector the ability to script the inspected page. >+ m_page->mainFrame()->document()->securityOrigin()->grantUniversalAccess(); The mainFrame() in this chain is kind of scary. If I can get the inspector to load in an iframe, I can get universal access. I wouldn't block this patch on this issue because we're already screwed in that case because of: > // FIXME: This should be cleaned up. API Mix-up. > JSGlobalObject* globalObject = m_page->mainFrame()->script()->globalObject(); I should probably file a bug and investigate. >+bool FrameLoader::shouldTreatURLSchemeAsNoAccess(const String& scheme) >+{ >+ return noAccessSchemes().contains(scheme); >+} I wonder if we need to fast case HTTP here like we do in FrameLoader::shouldTreatURLSchemeAsLocal. I suspect it doesn't matter since this is only called once per document creation. It's kind of weird that this state is kept in FrameLoader. It might make more sense to put these in SecurityOrigin and have FrameLoader ask SecurityOrigin about the security policies for URL schemes. > The mainFrame() in this chain is kind of scary. If I can get the inspector to > load in an iframe, I can get universal access. I wouldn't block this patch on Good observation. I wonder if there is a way for the InspectorController to get the Frame that it is loaded in. > >+bool FrameLoader::shouldTreatURLSchemeAsNoAccess(const String& scheme) > >+{ > >+ return noAccessSchemes().contains(scheme); > >+} > > I wonder if we need to fast case HTTP here like we do in > FrameLoader::shouldTreatURLSchemeAsLocal. I suspect it doesn't matter since > this is only called once per document creation. I thought about adding a fast path for HTTP, but I came to the same conclusion you did. Once per document is not enough to worry about. > It's kind of weird that this state is kept in FrameLoader. It might make more > sense to put these in SecurityOrigin and have FrameLoader ask SecurityOrigin > about the security policies for URL schemes. Good point. I just put it on FrameLoader for consistency. I could certainly move both lists. Hey Sam, it looks like you have reviewed changes to SecurityOrigin in the past. Could you please comment on this patch? Thanks! Comment on attachment 28377 [details]
v1 patch
r=me. But I agree that we should move this logic into SecurityOrigin code at some point.
Thanks. I'm happy to make that change. Landed as http://trac.webkit.org/changeset/41555 I decided to do the cleanup to a followup patch, mainly because I have some question about the merits of the shouldTreatURLAsLocal function and the "optimizations" to return early if the scheme is "http" or "file". I suspect those are premature optimizations since these functions are called ~ once per resource on a page, which is typically not frequently enough to warrant such optimizations. |