Add title attribute to iframe (7.x)

That sounds sensible, thanks! Someone should tell Google that too, because the iframe embedding code was originally taken from what they provide when you go to an address, click the link icon, and choose the "embed" option.

Anyway, if someone wants to make a patch, that would be good... Probably the correct thing to do in the context of this module would be to make the title text configurable, and let the text contain [address] as a token to insert the address.

Suggested field help text for the configuration:

Title of embedded iframe for screen readers

(or something similar)?

I think that's incorrect. The title text would be part of the field configuration, and field configuration values are (at least in principle) translatable, even in Drupal 7 (you might need to use a contributed module to make them translatable).

So the idea would be that in English, you might make ti so that the text is "Map of [address]" and in Spanish that part of the field configuration could be translated to "Mapa de [address]".

Right?

A teeny, tiny bit of security review :-)

1. +++ b/simple_gmap.module
   @@ -135,6 +143,7 @@ function simple_gmap_field_formatter_view($entity_type, $entity, $field, $instan
   +      '#iframe_title' => $iframe_title,
   

   This should go through check_plain() to prevent XSS

2. +++ b/simple_gmap.module
   @@ -277,6 +286,13 @@ function simple_gmap_field_formatter_settings_form($field, $instance, $view_mode
   +    '#default_value' => check_plain($settings['iframe_title']),
   

   This doesn't need check_plain() - the Form API will handle this. I think this could actually lead to double encoding of certain characters

Thanks for the review @dsnopek. In the first case, check_plain is run on $iframe_title before the str_replace, and it's also run on the $address_value before it's substituted in. So the $iframe_title should have been fully checked before it's added to the array.

Ah, ok, you're right! It's best practice to do filtering as late as possible, so I would have done it when building the render array, but this appears to be totally secure (I just tested it too), so that's fine.

In the 2nd case, all of the other settings that are strings are also being run through check_plain. I tried putting & and < into the address text, and nothing was double-encoded in the page source.

This isn't about the address text, but the "Iframe title" formatter setting. I actually just tested this, and it does get double encoded!

To test in Panopoly, here's some steps to reproduce:

1. Go to /admin/structure/fieldable-panels-panes/manage/map/display (if not on Panopoly, you can go the "Manage display" page for an entity using the field formatter)
2. Click the gear in the far right column in the row for "Address"
3. Put <script>alert("xss");</script> in the "Iframe title" field
4. Click the "Update" button at the end of formatter settings
5. Click the gear to re-open the formatter settings again
6. Notice that the "Iframe title" field now contains Google Map of [address] <script>alert("blah")</script>
7. If you keep pressing "Update" and the "Gear" to save and re-open the same settings it'll keep double encoding it more and more -- the expected behavior is for the same data saved to be shown again

You're right that there's existing code that passing values to '#default_value' through check_plain(), but those are wrong too. :-) Since that's pre-existing, and not insecure (just buggy), maybe fixing that could be handled in another issue, though?

Good catch dsnopek -- let's move that to another issue to fix the other settings, and only make sure the new setting works right on this issue. Thanks!

Sounds like this patch is Needs Work based on #10.

The latest patch looks great and addresses my review! Marking as RTBC :-)

This patch looks pretty good to me! Thanks for the patches and reviews.

But there were a couple of minor things to fix:

+++ b/simple_gmap.module
@@ -277,6 +286,13 @@ function simple_gmap_field_formatter_settings_form($field, $instance, $view_mode
+    '#title' => t('Iframe title'),
+    '#type' => 'textfield',
+    '#default_value' => $settings['iframe_title'],
+    '#description' => t("Title of embedded iframe for screen readers. Use [address] to insert the field's address text."),
+  );

a) I wasn't sure about the title/description of the settings field here. In the other settings related to the iframe, we don't use the "iframe" technical term -- iframe_width says "Width of embedded map" etc.

Also I wasn't sure if "Iframe" can be capitalized?

So...

Maybe here we should say something like:

title => 'Title of iframe for embedded map'

description => 'The embedded map is in an iframe HTML tag, which should have a title for screen readers (not shown on the page). Use [address] to insert the address text in the title.'

b) I think this field should go into the settings form nearer the iframe_width and iframe_height settings instead of at the end of the settings.

c) It also needs to be added to the settings summary function.

The new patch looks good (at least, the interdiff). I will need to give it a quick test (we have a test script) before committing. Thanks!

For the double-encoding issue mentioned on #10, I added a separate issue #2913846: Repeated submit of settings form causes problems.

Sorry I haven't tested this yet...

Someone just filed this duplicate issue but leaving them both as the other is Drupal 8

With D7 officially EOL believe this is out dated, if still valid for 4.0.x please reopen