Closed Bug 974435 Opened 10 years ago Closed 9 years ago

Remove inline scripts and styles usage via JavaScript for b2g/chrome/content/shell.html and shell.js

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox38 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox38 --- fixed

People

(Reporter: freddy, Assigned: gioyik, Mentored)

References

Details

(Whiteboard: [lang=html][good first bug][lang=js])

Attachments

(1 file, 4 obsolete files)

974435-update.patch
3.84 KB, patch
fabrice
: review+
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug #972316 +++
> 
> With the current plan to harden the security of Firefox, we want to disallow 
> internal privileged pages to use inline JavaScript. Since their amount is
> fairly  limited, we start this by rewriting them bit by bit.
> 
> See also https://wiki.mozilla.org/Security/Inline_Scripts_and_Styles

I just noticed that shell.js sets some inline style for the system app frame, we might want to remove as well.
The next line also loads a data URI that contains inline style.

While the first can be easily moved to shell.css, the latter might be achieved with additional files.
Blocks: 972316
Assignee: nobody → gasolin
I'm not sure what this line means 

    systemAppFrame.setAttribute('src', "data:text/html;charset=utf-8,%3C!DOCTYPE html>%3Cbody style='background:black;");


If I interpret it well, it looks like writing some html to iframe

    <!DOCTYPE html><body style='background:black;

which missing a '>' end

and iframe src is not used this way in MDN doc
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe

Is there any other reference that can help me understand this syntax?
Flags: needinfo?(mgoodwin)
That's just a data: uri. We could add the '>' at the end and use a class. Note that we change the src back to the system app launch patch shortly afterward: https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#327 so maybe we don't even need the first .src
Attached patch shelljs.patch (obsolete) — — Splinter Review 
I've tested the patch in emulator and not see any difference if I remove the src attribute
Attachment #8379554 - Flags: feedback?(fabrice)
If it might flash to a white page for a short time on slower devices, we should keep this in. If we do, I'd prefer using a small file like "black.html" than a data URI.
Attached patch blankhtml.patch (obsolete) — — Splinter Review 
a version not just remove src attribute, but come with blank.html + blank.css
Attachment #8380509 - Flags: feedback?(fabrice)
Attachment #8380509 - Flags: feedback?(fabrice) → feedback+
Attachment #8379554 - Flags: feedback?(fabrice) → feedback+
Comment on attachment 8380509 [details] [diff] [review]
blankhtml.patch

tested the patch in emulator and works fine
Attachment #8380509 - Flags: review?(fabrice)
(In reply to Fred Lin [:gasolin] from comment #1)
> Is there any other reference that can help me understand this syntax?

Answering your specific question; yes: https://developer.mozilla.org/en/docs/data_URIs
Flags: needinfo?(mgoodwin)
(In reply to Fred Lin [:gasolin] from comment #6)
> Comment on attachment 8380509 [details] [diff] [review]
> blankhtml.patch
> 
> tested the patch in emulator and works fine

Can you also verify on device? that's what matter most.
I'll figure out how to flash the gecko on device...
Status: NEW → ASSIGNED
Comment on attachment 8380509 [details] [diff] [review]
blankhtml.patch

Review of attachment 8380509 [details] [diff] [review]:
-----------------------------------------------------------------

Fred, sorry to be so late to review. That looks fine, but I'd like to see a rebased patch. Thanks!
Attachment #8380509 - Flags: review?(fabrice)
Mentor: mgoodwin
Whiteboard: [mentor=mgoodwin@mozilla.com][lang=html][good first bug][lang=js] → [lang=html][good first bug][lang=js]
Could I make the rebased version of the patch?
Flags: needinfo?(gasolin)
Flags: needinfo?(fabrice)
(In reply to Giovanny Andres Gongora Granada from comment #11)
> Could I make the rebased version of the patch?

Sure, no need to ask ;)
Flags: needinfo?(fabrice)
Attached patch 974435.patch (obsolete) — — Splinter Review 
Patch rebased
Assignee: gasolin → gioyik
Attachment #8379554 - Attachment is obsolete: true
Attachment #8380509 - Attachment is obsolete: true
Flags: needinfo?(gasolin)
Attachment #8547877 - Flags: review?(fabrice)
Giovanny, thanks for help!
Comment on attachment 8547877 [details] [diff] [review]
974435.patch

Review of attachment 8547877 [details] [diff] [review]:
-----------------------------------------------------------------

Please fix the nits and attach the update patch. Thanks!

::: b2g/chrome/content/shell.css
@@ +18,5 @@
>  }
> +
> +iframe {
> +  overflow: hidden;
> +  height: 100%; 

on this line and others, please remove the trailing whitespace

::: b2g/chrome/content/shell.js
@@ +289,5 @@
>      systemAppFrame.setAttribute('id', 'systemapp');
>      systemAppFrame.setAttribute('mozbrowser', 'true');
>      systemAppFrame.setAttribute('mozapp', manifestURL);
>      systemAppFrame.setAttribute('allowfullscreen', 'true');
> +    systemAppFrame.setAttribute('src', "blank.html");

nit: use single quotes
Attachment #8547877 - Flags: review?(fabrice) → review+
Attached patch 974435.patch (obsolete) — — Splinter Review 

        Attachment #8549184 -
        Flags: review?(fabrice)

    
    

    
  
Nits fixed!

    
    

    
  
Comment on attachment 8549184 [details] [diff] [review]
974435.patch

Review of attachment 8549184 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/chrome/content/shell.css
@@ +16,5 @@
>    overflow: hidden;
>  }
> +iframe {
> +  overflow: hidden;
> +  height: 100%; 

You still have trailing whitespace!

        Attachment #8549184 -
        Flags: review?(fabrice)

    
    

    
  


  
Sorry Fabrice, now it't fixed!

        Attachment #8547877 -
        Attachment is obsolete: true

        Attachment #8549184 -
        Attachment is obsolete: true

        Attachment #8549291 -
        Flags: review?(fabrice)

        Attachment #8549291 -
        Flags: review?(fabrice) → review+

    
    

    
  
Thanks Giovanny, I'll take care of landing the patch.

    
    

    
  
Thank you Fabrice

    
    

    
  
Hi Fabrice, just a question, Is normal for this take +one month to land or it's not necessary any more?

I just ask because it's the first time that I am see a patch on queue to land for a long time.

Regards
Flags: needinfo?(fabrice)

    
    

    
  
Sorry it took so long. I forgot about this bug.
Flags: needinfo?(fabrice)

    
    

    
  
Don't worry Fabrice. Thank you

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/fa3b4b1223d7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago

          status-firefox38:
          --- → fixed
Resolution: --- → FIXED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: