|Summary:||Add defensive initialization of iframe sandbox flags|
|Product:||WebKit||Reporter:||Patrik Persson <patrik.j.persson>|
|Version:||528+ (Nightly build)|
Description Patrik Persson 2009-12-10 01:50:38 PST
This is a followup to bug 21288, which concerned the implementation of the HTML5 iframe sandbox attribute. I'm curious whether it would be possible to use more defensive initial values for the sandbox flags than the current default of "SandboxNone" (indicating no sandboxing at all). This defensive technique is, to me, primarily about finding current bugs and preventing future ones. It is a design challenge rather than a functional bug, so I don't have a test case to share. Three classes contain such sandbox flags: HTMLFrameOwnerElement, FrameLoader and SecurityOrigin. I imagine three steps: 1. On object instantiation, the sandbox flags are set to a defensive default value (SandboxAll, possibly including a special flag indicating an illegal value). 2. At some point between 1 and 3, we know of a better value for the sandbox flags, and assign that value to the flags. 3. We now start making decisions based on the sandbox flags. To ensure that step 2 actually happened, we could ASSERT that the flag for an illegal value is not set. The design challenge is to ensure step 2 always happens before step 3. It was straight-forward for the FrameLoader (that part was included in the landed patch for bug 21288), but not for HTMLFrameOwnerElement and SecurityOrigin. I wrote down some observations on this in the thread for bug 21288, comment #58: https://bugs.webkit.org/show_bug.cgi?id=21288#c58
Comment 1 Darin Adler 2009-12-10 17:25:55 PST
I think for HTMLFrameOwnerElement this issue probably doesn't apply. Using the DOM you can create a frame element a step at a time, and you can start using it at any point, so there is no real "illegal value" time for the DOM elements. For SecurityOrigin I think we might be able to make it happen, though.
Comment 4 Darin Adler 2010-01-10 16:11:32 PST
Comment on attachment 46242 [details] Patch > if (shouldTreatURLSchemeAsNoAccess(m_protocol)) > m_isUnique = true; > > - // If this ASSERT becomes false in the future, please consider the impact > - // of m_sandoxFlags on m_isUnique. > - ASSERT(m_sandboxFlags == SandboxNone); > + if (isSandboxed(SandboxOrigin)) > + m_isUnique = true; I suggest initializing m_unique to the correct value from the outset. m_isUnique(shouldTreatURLSchemeAsNoAccess(m_protocol) || isSandboxed(SandboxOrigin)) To some the if statements may be slightly clearer, so it seems OK to leave it this way if you think that way. > + static PassRefPtr<SecurityOrigin> create(const KURL&, SandboxFlags sandboxFlags = SandboxNone); The argument name isn't needed here because the type speaks for itself.
Comment 5 Adam Barth 2010-01-10 16:13:38 PST
> > + static PassRefPtr<SecurityOrigin> create(const KURL&, SandboxFlags sandboxFlags = SandboxNone); > > The argument name isn't needed here because the type speaks for itself. Oh, I didn't know you could omit the argument name and provide a default value. Thanks for the review.