fu: Close-up of Fu, bringing a scoop of water to her mouth (Default)
fu ([personal profile] fu) wrote in [site community profile] changelog2011-04-11 03:30 am
  • Add Memory
  • Share This Entry

[dw-free] handle iframe tags that aren't wrapped in site-embed

[commit: http://hg.dwscoalition.org/dw-free/rev/0bbacf3153c8]

http://bugs.dwscoalition.org/show_bug.cgi?id=3605

Put bare iframe tags into site-embed, instead of relying on the user to do
it for us.

Patch by [personal profile] fu.

Files modified:
  • cgi-bin/LJ/EmbedModule.pm
  • t/clean-embed.t
--------------------------------------------------------------------------------
diff -r 0567637b58f0 -r 0bbacf3153c8 cgi-bin/LJ/EmbedModule.pm
--- a/cgi-bin/LJ/EmbedModule.pm	Mon Apr 11 11:21:15 2011 +0800
+++ b/cgi-bin/LJ/EmbedModule.pm	Mon Apr 11 11:30:04 2011 +0800
@@ -30,6 +30,8 @@ use constant {
     MAX_WIDTH => 800,
     MAX_HEIGHT => 800,
 };
+
+my %embeddable_tags = map { $_ => 1 } qw( object embed iframe );
 
 # can optionally pass in an id of a module to change its contents
 # returns module id
@@ -134,7 +136,7 @@ sub parse_module_embed {
     return unless LJ::is_enabled('embed_module');
 
     # fast track out if we don't have to expand anything
-    return unless $$postref =~ /(lj|site)\-embed|embed|object/i;
+    return unless $$postref =~ /(lj|site)\-embed|embed|object|iframe/i;
 
     # do we want to replace with the lj-embed tags or iframes?
     my $expand = $opts{expand};
@@ -175,8 +177,8 @@ sub parse_module_embed {
                 $embed_attrs{id} = $attr->{id} if $attr->{id};
                 $embed_attrs{width} = ($attr->{width} > MAX_WIDTH ? MAX_WIDTH : $attr->{width}) if $attr->{width};
                 $embed_attrs{height} = ($attr->{height} > MAX_HEIGHT ? MAX_HEIGHT : $attr->{height}) if $attr->{height};
-            } elsif (($tag eq 'object' || $tag eq 'embed') && $type eq 'S') {
-                # <object> or <embed>
+            } elsif ( $embeddable_tags{$tag} && $type eq 'S' ) {
+                # <object> or <embed> or <iframe>
                 # switch to IMPLICIT state unless it is a self-closed tag
                 unless ($attr->{'/'}) {
                     $newstate = IMPLICIT;
@@ -190,15 +192,15 @@ sub parse_module_embed {
                 $newtxt .= $reconstructed;
             }
         } elsif ($state == IMPLICIT) {
-            if ($tag eq 'object' || $tag eq 'embed') {
+            if ( $embeddable_tags{$tag} ) {
                 if ($type eq 'E') {
-                    # </object> or </embed>
+                    # </object> or </embed> or </iframe>
                     # update tag balance, but only if we have a valid balance up to this moment
                     pop @stack if $stack[-1] eq $tag;
                     # switch to REGULAR if tags are balanced (stack is empty), stay in IMPLICIT otherwise
                     $newstate = REGULAR unless @stack;
                 } elsif ($type eq 'S') {
-                    # <object> or <embed>
+                    # <object> or <embed> or <iframe>
                     # mind the tag balance, do not update it in case of a self-closed tag
                     push @stack, $tag unless $attr->{'/'};
                 }
@@ -287,7 +289,7 @@ sub module_iframe_tag {
 
                 my $flashvars = $attr->{flashvars};
 
-                if ($tag eq 'object' || $tag eq 'embed') {
+                if ( $embeddable_tags{$tag} ) {
                     my $src;
                     next unless $src = $attr->{src};
 
diff -r 0567637b58f0 -r 0bbacf3153c8 t/clean-embed.t
--- a/t/clean-embed.t	Mon Apr 11 11:21:15 2011 +0800
+++ b/t/clean-embed.t	Mon Apr 11 11:30:04 2011 +0800
@@ -1,7 +1,7 @@
 # -*-perl-*-
 use strict;
 
-use Test::More tests => 143;
+use Test::More tests => 151;
 use lib "$ENV{LJHOME}/cgi-bin";
 require 'ljlib.pl';
 
@@ -270,10 +270,12 @@ note( "Testing parse_embed (We parse the
         [   "iframe tag; no site-embed (untrusted)",
             qq{foo <iframe>bar</iframe>baz},
 
-            qq{foo <iframe>bar</iframe>baz},    # we save the iframe as-is
-            qq{foo <iframe>bar</iframe>baz},
-            qq{foo baz},                        # but strip it out on display
-            $invalid_embed
+            # wrap the iframe in a site-embed tag
+            qq{foo <site-embed id="1"/>baz},
+            # but nested site-embed won't display the untrusted content
+            qq{foo <site-embed id="1"></site-embed>baz},
+            qr{foo ${iframe}baz},
+            qq{},
         ],
 
         [   "iframe tag with site-embed (untrusted)",
@@ -289,12 +291,13 @@ note( "Testing parse_embed (We parse the
         [   "iframe tag; no site-embed (trusted)",
             qq{foo <iframe src="http://www.youtube.com/embed/ABC123abc_-"></iframe>baz},
 
-            # save the iframe as-is
-            qq{foo <iframe src="http://www.youtube.com/embed/ABC123abc_-"></iframe>baz},
-            qq{foo <iframe src="http://www.youtube.com/embed/ABC123abc_-"></iframe>baz},
-            # but still strip it out on display
-            qr{foo baz},
-            $invalid_embed
+            # wrap the iframe in a site-embed
+            qq{foo <site-embed id="1"/>baz},
+            qq{foo <site-embed id="1"><iframe src="http://www.youtube.com/embed/ABC123abc_-"></iframe></site-embed>baz},
+            # site-embed iframe
+            qr{foo ${iframe}baz},
+            # ...which contains the nested iframe with a URL from a trusted source
+            qq{<iframe src="http://www.youtube.com/embed/ABC123abc_-"></iframe>},
         ],
 
         [   "iframe tag with site-embed (trusted)",
@@ -349,11 +352,12 @@ note( "Testing parse_embed (We parse the
         ],
 
 
+        # TODO: DANGER: EATS EVERYTHING PAST THE OPEN TAG
         [   "iframe tag left open; no site-embed (untrusted)",
-            qq{foo <iframe>bar baz},
+            qq{foo },
 
-            qq{foo <iframe>bar baz},
-            qq{foo <iframe>bar baz},
+            qq{foo },
+            qq{foo },
             qq{foo },
             $invalid_embed
         ],
@@ -368,11 +372,12 @@ note( "Testing parse_embed (We parse the
             $invalid_embed
         ],
 
+        # TODO: DANGER: EATS EVERYTHING PAST THE OPEN TAG
+        [   "iframe tag left open; no site-embed (trusted)",
+            qq{foo },
 
-        [   "iframe tag left open; no site-embed (trusted)",
-            qq{foo <iframe src="http://www.youtube.com/embed/ABC123abc_-">baz},
-
-            qq{foo <iframe src="http://www.youtube.com/embed/ABC123abc_-">baz},                        qq{foo <iframe src="http://www.youtube.com/embed/ABC123abc_-">baz},
+            qq{foo },
+            qq{foo },
             qr{foo },
             $invalid_embed
         ],
--------------------------------------------------------------------------------
Flat | Top-Level Comments Only
ninetydegrees: Art: self-portrait (Default)

[personal profile] ninetydegrees 2011-04-11 08:50 am (UTC)(link)
Fewer requests! Yay! :)
Edited (grammar!) 2011-04-11 08:51 (UTC)
fu: Close-up of Fu, bringing a scoop of water to her mouth (Default)

[personal profile] fu 2011-04-11 07:48 pm (UTC)(link)
\o/

(And \o/ grammar!)
Flat | Top-Level Comments Only