Can't save configuration changes

Exact same thing for me.

same here

The problem seems to be that sharethis_form_submit is trying to get the values from $form_state['values'] instead of $form_state['input']

This changes $form_state['values'] to $form_state['input'] and removed some trailing spaces.

Yes i say every time dont change a running system.

And better makes useful features --> Implements hook_ds_fields_info() for display suite.

Remember ---> http://drupal.org/node/1239418

Greets Lennard

Having the same problem. No settings get changed!

This happens with the current "stable" release not dev. Don't know if dev also has this problem.

It does.

Patch worked for me. Thanks Jaime.

Using 'input' was what caused the security flaw as it didn't pass through Drupal's sanitization system.

But 'values' give the response of old settings thats not really useful to change settings.

A other way is settings automatic saves with drupal form without a seperate submit function.

The same for me after upgrading to the latest module version (7.x-2.3)

same.

Patch seems to work

I can't apply patches. Is someone working on this bug? I'd like to change my configuration.

Just roll back to 2.2 until they get it done.

Hello Guys,
I´m using Share This version no.: sharethis-7.x-2.3
Patch no .#5, working perfect.
Many thanks to jherencia

m.

Patch worked for me as well on the 2.3 version.

• Version 7.x-2.2 was tagged as a security exploit
• The 7.x-2.3 release disabled the ability to re-save the block or the display settings.
• The patch in #5 restores the ability to save settings, but also presents the same security issue

Is it possible for the maintainers of this module to remove the 2.3 version until the issue is resolved? Seems a shame all the people who "upgrade" this module will be faced with this issue.

Also note that "downgrading " to 7.x-2.2 did not work. It took down the site giving an error: PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'nameg.st_table' doesn't exist. Reinstalling 7.x-2.3 brought it back online.

Based on comment #11, this patch will merely flag another security warning as per the original ticket mentioned several times in this thread (#1506448: SA-CONTRIB-2012-049 - ShareThis - Multiple Vulnerablies). Therefore this is not RBTC and needs work.

Ok several things here...

#1. Nested forms.

The reason that the 'values' are not being passed in properly is probably due to form nesting... hook_menu defines

  $items['admin/config/services/sharethis'] = array(
    'title' => 'ShareThis',
    'description' => 'Choose the widget, button family, and services for using ShareThis to share content online.',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('sharethis_admin'),
    'access arguments' => array('administer sharethis')
  );
  return $items;

But when you get to function sharethis_admin() we see...

function sharethis_admin() {
  // First, setup variables we will need.
  // Get the path variables setup.
  $my_path = drupal_get_path('module', 'sharethis');
  // Load the css and js for our module's configuration.
  drupal_add_css($my_path . '/ShareThisForm.css');
  drupal_add_js('https://ajax.googleapis.com/ajax/libs/jquery/1.5.2/jquery.min.js', 'external'); // @TODO: Do we really need to load jQuery twice?
  drupal_add_js('http://w.sharethis.com/share5x/js/stcommon.js', 'external');  //This is ShareThis's common library - has a serviceList of all the objects that are currently supported.
  drupal_add_js($my_path . '/ShareThisForm.js');
  drupal_add_js($my_path . '/stlib_picker.js');
  drupal_add_css($my_path . '/stlib_picker.css');
 
  return drupal_get_form('sharethis_form'); 
}

If we simply take the add_css and add_js code and place it into sharethis_form... we can then change the reference from hook_menu to access that form directly and now the values pass. So the suggestion is to change "'page arguments' => array('sharethis_admin')," to " 'page arguments' => array('sharethis_form')," and then move all the js and css calls into sharethis_form.

#2 hook_form.

In addition to the changes above, the module uses 'hook_form', which is meant to be used for specifically for adding forms to nodes and not a more generic system setting.
http://api.drupal.org/api/drupal/modules%21node%21node.api.php/function/...

Therefore, I would suggest renaming the form to sharethis_admin_form instead of sharethis_form to avoid any unexpected behavior.

#3 system_settings_form

A lot of the work being done in the sharethis_form_submit function could be eliminated simply by using system_settings_form at the end of the form definition. This essentially gives you all the variable_set commands for this admin form and lets you get rid of the sharethis_form_submit function entirely.

I know that there are a lot of sanity checks going on in the _submit function right now... but they could be moved to a form_validate function and do all the processing, stripping, etc... and that would leave you with a lot less code to manage!

That said... I can perform the above and create a patch if the maintainer agrees with said advice and would accept said patch.

#22 posted by rickmanelius has my +1 (although I am not the maintainer of this module).

Thanks @mcfilms. The only reason I haven' submitted a patch yet is I've been burned before, and these changes are not hard... but will take a solid hour+. So until I hear back from rgon, I'll be anxiously waiting!

I understand. Nothing worse than that feeling of "there goes an hour of my life I'll never get back."

+1 to Rickmanelius idea in #22. I agree that using system_settings_form would make more sense. I think the module recently moved away from that, though I didn't dig in to try to see what issue that was tracked in and why that was the case.

@Rickmanelius, if you spend the hour on it that is very unlikely to go to waste. It will help show at least one way of solving it which is better than having no ways to solve it ;)

@greggles.

No worries. I just know a lot of people got burned on the views_datasource module because the maintainer essentially went away and all those patches got left in limbo. But it won't take me too long to get this one done and I'll be happy to provide one (seeing as this is a highly used module and this issue is kindof a deal breaker in using it!).

Here is the first stab at the changes proposed in #22. Basically:

- Renamed the sharethis_form to sharethis_configuration_form
- Removed the recursive drupal_get_form calls
- Used system_settings_form in the form declaration
- Renamed the form parameters so they match the drupal system variables being retrieved and stored.
- Changed _submit to _validate
- Removed a lot of redundant code in _validate.
- Moved _add_css and _add_js code from sharethis_admin into the sharethis_configuration_form.
- Removed the sharethis_admin function.

Questions still outstanding:
- whether all the functions in _validate are actually necessary. After all, do we really need to sanitize these strings? Are they not already sanitized as textfields?

Now I just need someone to review this to make sure all the variables are connecting up.

+++ b/sharethis.moduleundefined
@@ -44,35 +44,24 @@ function sharethis_permission() {
+  ¶

Trailing whitespace, here and on many other lines.

+++ b/sharethis.moduleundefined
@@ -217,80 +206,39 @@ function sharethis_form($form, &$form_state) {
+  //$form['submit'] = array(
+  //  '#type' => 'submit', ¶
+  //  '#value' => t('Submit')
+  //);

Just remove it, no need to comment it out.

+++ b/sharethis.moduleundefined
@@ -217,80 +206,39 @@ function sharethis_form($form, &$form_state) {
+ * This is the validation function for sharethis_configuration_form

See http://drupal.org/node/1354#forms

+++ b/sharethis.moduleundefined
@@ -217,80 +206,39 @@ function sharethis_form($form, &$form_state) {
+  //Additional filters for the option extras input

Missing space after // and missing trailing full stop, here and other lines.

+++ b/sharethis.moduleundefined
@@ -217,80 +206,39 @@ function sharethis_form($form, &$form_state) {
+  // Sanitize the publisher ID option.  Since it's a text field, remove anything that resembles code

Two spaces between sentences should be one.

+++ b/sharethis.moduleundefined
@@ -349,7 +297,7 @@ function sharethis_node_view($node, $view_mode, $langcode) {
-    $path_module .= drupal_lookup_path('alias',"node/".$node->nid);
+    $path_module .= drupal_lookup_path('alias', "node/" . $node->nid);

This patch would be easier to review without extra changes like this.

Hi tim.plunkett.

I guess I hadn't planned on doing a complete overhaul of all the formatting on the patch. But you're right... since I already tweaked some settings, I should have just completed the entire thing.

Anyway, I ran this through the textmate PHP Drupal bundle after having run this through Coder Review to get as much of the legwork done for me. Then I manually went through as per your helpful suggestions and instructions in #29. If there is anything missing, please let me know. And any testing/feedback on the functionality would be appreciated!

Oh no, that is the OPPOSITE of what I meant. Now the patch has doubled in size, and makes completely unrelated changes.
The diff should only show what needs to change to fix this bug.
The rest needs to be in a follow-up.

I only meant that proper coding style should be applied to the additions made in the patch. Like, if you're writing docs of the function you fix, they might as well be correct.

Hi tim.plunkett.

That's why I was confused. You went on to note and suggest changes but then ended by saying it would have been easier without. I wish you had been more clear!

I'm happy to submit the change specific to the issue at hand and then leave it to the maintaiers/another ticket to do the rest.

Yeah, sorry about that. Didn't mean to waste your time. It was just those specific things were additions made by the patch.

Not a problem my friend! Communicating in the issue queues is just one more aspect of the DRupal learning curve :)

Working on some client items right now, but I can get get back to this in an hour or two.

Ok. I've minimized the size of the patch to just the changes required for it to now work plus a few very short comments to carry over the intent of the remaining pieces... which is important given that a lot of the original form_submit functionality is removed.

Hopefully now it's at a place where it can be reviewed on the technical merits and (ultimately) fix this bug!

I begin to use 1507684-sharethis-configurations-35.patch work fine. I only need to uninstall the same version of the module with out the patch after install again i can save change like add content types, etc.

This looks fine. The patch still adds trailing whitespace, and the docs need to be fixed, but that should likely just happen in a follow-up.

Still can't change the value for Choose Your Service. Fancy slider thing isn't saving values. The rest of it is good though.

Hmm. Looks like it might be an issue with the javascript not loading properly (which may be why they did the recursive get_form calls). I can't look at this yet, but any suggestions in the meantime?

Wow... looks like there might be some considerable javascript issues to work out.

First, drupal_add_js is pulling another version of jquery (which is now included in D7 and therefore not necessary)
Second, there are a couple areas where javascript is being injected in via the .module file (including within the $form).
Third, I see a 2 $(document).ready() calls in ShareThisForm.js. We probably want something more in line with this. http://drupal.org/node/171213

Unless someone out there (who is better at Drupal specific javascript than I am), it's looking like there is some more work to do...

A followup item. I tried to just use a simple $( to jQuery( conversion in the hopes that I could just do a brute force conversion without introducing too many changes in the document. But no dice. It looks like the changes will be more extensive that I'd like.

Anyone else have anything to add? If I get some pointers, I'll gladly go after it. But in this scenario, I'm not sure what the most Drupal friendly approach would be. I know we'd want to yank the javascript out of the form directly. But in terms of creating behaviors, etc... I'll cede to someone else's opinion/experience on the best solution.

This is the working module file

I've tested and it's working, let me know... if someone wants to build the patch is welcome;)

i forgot the file

@cavax, here it is.

The patches in #44/#45 restore the 'input' keys in the form, which is exactly what tripped the security advisory here #1506448: SA-CONTRIB-2012-049 - ShareThis - Multiple Vulnerablies. Please look at comments 39-42 to see what is left from here to get this patched while working properly with the form API!

sorry i've just tried out the patches and it didnt work.
cause i got an error there where an undeclared function.

Hi cavax.
What function is undeclared? Can you refresh your cache and report back?

And yes, #36 doesn't work yet because there are some javascript issues to still work on from here as per comments #40-42.

Ok I figured it out. What happened was the addition of the 'sharethis_' prefix for all the $form entries was messing up the jquery selectors (e.g. $("#edit-sharethis-button-option-stbc-large") instead of $("#edit-button-option-stbc-large")).

In addition, we had the duplicate drupal_add_js call for jquery.

And finally, I don't believe the jquery calls are properly namespaced to use the $, particularly the calls in the .module file. Rather than rewriting huge swaths of code, I did a simple $( to jQuery( and $. to jQuery. conversion.

So this patch does 3 things on top of #36.
1. $ -> jQuery substitution where appropriate.
2. Removed drupal_add_js call to additional jquery 1.5.2
3. Changes jquery selectors to match the changes in the new $form array values.

I tested this and now the toggling issues from #39 are now gone (e.g. changing "Choose a widget type:" and "Choose a button style: *" now work). There is still an issue with the "Choose Your Services. *" toggling, which I can't get to at this very moment. But I wanted to leave this updated patch as a placeholder before adding/connecting the final piece to knock this issue out (finally).

Not setting to 'needs review' until this final bit is done... but if you want to take a peek and try it out thus far, be my guest.

Found it! I was missing the jQuery("#edit-sharethis-service-option") selector. That's now fixed.

Summarizing: This patch is the #36 (which was reviewed by #38) and then adds the changes in #49 to account for the javascript behaviors that are missing (see #39). I then added a final change (see the line above) to get a fully working version.

This patch is now ready for a review...

Had to clear caches after applying the patch, but now I can save settings.

Ping...

This is so close to RBTC it's not even funny... and 6,664 D7 sites need this fixed :)

Thank you guys so much! This looks great! Committed with some minor whitespace fixes: http://drupalcode.org/project/sharethis.git/commit/0c8e4f2

Awesome... thanks @Rob Loach for your speedy reply! I love a working module... :)

Thanks to @greggles for pinging me on it!!! And you for fixing up the patch!!!

Thanks, Rob for the patch. It works for me! However, there was one minor problem -the following link does not work when I tried to access it--gives me a corrupted content error.

http://drupalcode.org/project/sharethis.git/blob_plain/0c8e4f2:/sharethi...

Instead I copied your code for sharethis.module on git and manually erased the line numbers.

Thanks again for the fix.

Wait what? Something's broken? If you use
git clone --branch 7.x-2.x http://git.drupal.org/project/sharethis.git

You should get the latest code.

How do you implement the patch? I'm new to this.
Thank you!

@bjjy - no need to implement the patch. Just wait ~6 hours and when the 7.x-2.x-dev tarball has been rebuilt you can download that.

That said, http://drupal.org/patch/apply has the docs if you really want them.

It would be great if we could get a new point release with this fix in it as the module is basically unusable without it, but going back to 2.2 brings back the security issue. I understand I can just use the dev version, or even patch the latest release, but unfortunately there are annoying "rules" you have to follow on some projects, like not using dev or patched modules. So for myself and other with similar rules, a point release (even just containing this one fix) would be wonderful.

ShareThis 7.x-2.4 should be up soon. Please do testing off that and open up a new issue if you have troubles :-) .

Awesome! Thanks Rob!