You are here: Foswiki>Tasks Web>Item13477 (17 Jul 2015, MichaelDaum)Edit Attach

Item13477: Using TopicInteractionPlugin to Upload an attachment produces an Internal Server Error

Priority: Normal
Current State: Closed
Released In: n/a
Target Release: n/a

Applies To: Extension
Component: TopicInteractionPlugin
Branches: master

Reported By: PascalSchuppli
Waiting For:
Last Change By: MichaelDaum

I've installed a Vanilla Foswiki 1.2.0_Beta_2 and NatSkin (from /bin/configure).

When I try to attach a file to a topic using PatternSkin, everything works fine. Doing the same with NatSkin enabled results in a Internal Server Error response from rest call to bin/rest/TopicInteractionPlugin/upload. The cause for this seems to be the call to UTF82SiteCharSet in TopicInteractionPlugin/Core.pm's urlDecode, which doesn't seem to exist any more:


sub urlDecode {
  my $value = shift;
  return $value;
  $value =~ s/%([\da-f]{2})/chr(hex($1))/gei;
  my $session = $Foswiki::Plugins::SESSION;
  my $downgradedValue = $session->UTF82SiteCharSet($value);
  $value = $downgradedValue if defined $downgradedValue;  
  $value =~ s/^\s+//g;
  $value =~ s/\s+$//g;
  return $value;
}

Unfortunately I don't know how to fix this myself. Taking out the call will get rid of the Internal Server Error, but produce another error message with a wrongly decoded topic name if the topic name contains special characters like Umlauts. It seems to me that multiple changes will be necessary to get it to work; the above snippet is just responsible for the error, but fixing urlDecode won't be enough.

-- PascalSchuppli - 26 Jun 2015

Confirmed. In addition, TopicInteractionPlugin processes the cgi params directly, and need processing by Foswiki::decode_utf8. utf8 topic names seem to be accessible, but I've been getting corruption of utf8 attachment names. I've been getting confused because the Core

-- GeorgeClark - 26 Jun 2015

I looked at it some more.

When I comment out the UTF8CharSet call and enable tracing, I get the following debug output for the topic parameter:

- TopicInteractionPlugin - param topic=Main.D%C3%BCnenR%C3%B6cke

Notice the topic name is still UTF8 encoded. When I look at the actual request that goes over the wire, This is how it's encoded in the multipart content of the file upload.

Now, if I try to delete an attachment (which doesn't work either...), I have the following debug output for the topic parameter:

- TopicInteractionPlugin - param topic=Main.DünenRöcke

Note how the topic was correctly decoded from UTF-8 encoding to (I guess) perl's internal Unicode representation. The actual hex values for the bytes are 4d 61 69 6e 2e 44 fc 6e 65 6e 52 f6 63 6b 65 (fc and f6 are the Unicode points für ü and ö). However, in the actual request, this is encoded in UTF-8 just like the topic name in the multipart upload request:

validation_key=%3F1cafd5a7f598e834109d99649d294958&id=delete&filename=new.txt&topic=Main.D%C3%BCnenR%C3%B6cke

So I'm at a loss. The same UTF-8 encoded string is NOT correctly decoded in the multipart message body of the upload POST but it's decoded correctly in the simple POST message body for the rest call to delete an attachment.

As a side note: When I add a manual decoding step in urlDecode via Foswiki::decode_utf8, this is a no-op (which seems to indicate that the string I want to decode already has the Unicode flag set). So I would have to use Encode::decode("utf8", ...) to get the correctly decoded topic name in the upload REST call, but this will also decode the already decoded strings in the delete REST call, which will trash the topic name, so that's not a solution. I'm in way over my head, I think I'll leave this to the experts.

-- PascalSchuppli - 26 Jun 2015

Here is a fix: thanks to help from CDot

diff --git a/lib/Foswiki/Plugins/TopicInteractionPlugin/Core.pm b/lib/Foswiki/Plugins/TopicInteractionPlugin/Core.pm
index dd84983..34dfc2c 100644
--- a/lib/Foswiki/Plugins/TopicInteractionPlugin/Core.pm
+++ b/lib/Foswiki/Plugins/TopicInteractionPlugin/Core.pm
@@ -197,6 +197,7 @@ sub prepareAction {
 
   # read parameters 
   my $topic = $params->{topic} || $session->{webTopic};
+  $topic = Foswiki::decode_utf8($topic) if ( $Foswiki::UNICODE );
   my $web = $session->{webName};
 
   ($web, $topic) = Foswiki::Func::normalizeWebTopicName($web, $topic);
@@ -273,8 +274,10 @@ sub urlDecode {
 
   $value =~ s/%([\da-f]{2})/chr(hex($1))/gei;
   my $session = $Foswiki::Plugins::SESSION;
-  my $downgradedValue = $session->UTF82SiteCharSet($value);
-  $value = $downgradedValue if defined $downgradedValue;
+  unless ($Foswiki::UNICODE) {
+      my $downgradedValue = $session->UTF82SiteCharSet($value);
+      $value = $downgradedValue if defined $downgradedValue;
+  }
  
   $value =~ s/^\s+//g;
   $value =~ s/\s+$//g;
@@ -319,7 +322,7 @@ sub printJSONRPC {
   }
 
   $message = JSON::to_json($message, {pretty=>1});
-  $response->print($message);
+  $response->body($message);
 }
 
 ##############################################################################

Also need the following:

diff --git a/lib/Foswiki/Plugins/TopicInteractionPlugin/Attachments.pm b/lib/Foswiki/Plugins/TopicInteractionPlugin/Attachments.pm
index 37fd4fd..4391147 100644
--- a/lib/Foswiki/Plugins/TopicInteractionPlugin/Attachments.pm
+++ b/lib/Foswiki/Plugins/TopicInteractionPlugin/Attachments.pm
@@ -277,6 +277,7 @@ sub urlEncode {
   my ($infoOrText, $property) = @_;
 
   my $text = defined($property)?$infoOrText->{$property}:$infoOrText;
+  $text = Foswiki::encode_utf8($text) if ( $Foswiki::UNICODE );
   $text =~ s/([^0-9a-zA-Z-_.:~!*\/])/'%'.sprintf('%02X',ord($1))/ge;
 
   return $text;

-- GeorgeClark - 26 Jun 2015

Something is still not right here. If I change an attachment comment on a utf-8 topic name, then there is a Wide character in print error reported in Engine::CGI. Changing comments on utf-8 attachment names on a non-utf-8 topic works fine.

Crawford also pointed out that the plugin does it's own local parsing of QUERY_STRING. But it seems to only happen when used in GET, not POST operations.

-- GeorgeClark - 26 Jun 2015

The problem is in the way parameters are handled, specifically in the function getRequestParams.

Some key general points (some may need to be captured in Utf8MigrationConsiderations):

CGI parameters (the result of $request->param) can come from two places; the POST data in the request (if it's a POST), and/or they may be encoded in the URL.
If you have the same parameter in POST data as in the query string, which takes precedence?
1. RFC7231 suggests that processors of POST requests ignore the query string. However this is not a requirement.
2. CPAN:CGI takes the view that POST data is more relevant than the query string, so when param names conflict the POSTed value will be the one returned by param().
3. Foswiki::Engine is compatible with CPAN::CGI, so also takes the view that POST data takes precedence over the query string.
The QUERY_STRING is automatically URL-decoded when the parameter values are extracted. No URL decoding is performed on POSTed data. Thus URL-decoding of params should never be required in client code.
In Foswiki 1.1.9 and earlier, param names and values are passed back to the caller as utf8-encoded byte strings, which need to be decoded to Perl characters and re-encoded in the {Site}{CharSet} before use.
In Foswiki 1.2.0, param names and values are already decoded to Perl characters in the engine. Re-encoding should never be required when dealing with param values.

So much for the standard. Here's how TopicInteractionPlugin= (before recent changes) behaves:

All param values (POST data as well as query params) are explicitly URL-decoded
1. for QUERY_STRING params this will be a NOP, as they are already URL-decoded in CGI
2. params coming from POST data will be URL-decoded (this is very odd, and not done anywhere else AFAIK)
the QUERY_STRING is separately parsed and values defined there allowed to override those in POST data (the opposite of Foswiki::Engine)
All incoming params are decoded from utf8 and re-encoded in the {Site}{CharSet} using Foswiki::UTF8ToSiteCharSet

You have a choice; either

fix the current code by poking in the right decode steps in the right places, or
change the plugin to observe the standard

I'd strongly recommend (2), but it requires MichaelDaum to review why:

Param values in the QUERY_STRING override those in the POST data
POST params are URL-decoded

-- Main.CrawfordCurrie - 27 Jun 2015 - 06:58

Crawford, I still think that there might be something else going on here.

The QUERY_STRING override does not come into play. Debug print statements show that it is never triggered during upload or properties change events.
- I commented it out anyway, no effect.
I eliminated the urlDecode, and still have issues. The topic name in the POST request is sometimes decoded, and sometimes not decoded
- POST an UPLOAD, name is not decoded. Litterbox.TestT%C3%B6pic
- POST a comment change, name IS decoded. Litterbox.TestT\x{f6}pic

I have no idea where to go from here.

-- GeorgeClark - 27 Jun 2015

Debugging a bit further, I added a Data::Dumper call to print the output of the Engine::CGI prepareBody and prepareUploads functions. Abbreviated here.

POST an upload:

                   'param' => {
                                'hidefile' => [
                                              'off'
                                            ],
                                'topic' => [
                                           'Litterbox.TestT%C3%B6pic'
                                         ],
                                'file' => [
                                          "A\x{15b}\x{10d}\x{c1}\x{160}\x{164}\x{15b}\x{11b}\x{17e}.txt"
                                        ],
                                'autostart' => [
                                               'off'
                                             ],
                                'id' => [
                                        '1435416098295'
                                      ],
                                'filecomment' => [
                                                 ''
                                               ],
                                'name' => [
                                          "A\x{15b}\x{10d}\x{c1}\x{160}\x{164}\x{15b}\x{11b}\x{17e}.txt"
                                        ],
                                'createlink' => [
                                                'off'
                                              ]
                              },
                   'action' => 'rest',
                   'secure' => 0
                 }, 'Foswiki::Request' );

POST a comment change:

                   'param' => {
                                'id' => [
                                        'save'
                                      ],
                                'origfilename' => [
                                                  "TestT\x{f6}pic.txt"
                                                ],
                                'filename' => [
                                              "TestT\x{f6}pic.txt"
                                            ],
                                'filecomment' => [
                                                 'asdfqerqewr'
                                               ],
                                'topic' => [
                                           "Litterbox.TestT\x{f6}pic"
                                         ],
                                'validation_key' => [
                                                    '?5b96670c8959261a3dcb9d2c2f6d77ce'
                                                  ]
                              },
                   'start_time' => [
                                     1435416182,
                                     243809
                                   ],
                   'method' => 'POST',
                 }, 'Foswiki::Request' );

-- GeorgeClark - 27 Jun 2015

I think what's happening is the UPLOAD parameters are not being URL decoded first before the decode uft8. So the literal %C3%B6 in the topic name is ignored in the utf8 decode in the CGI engine. Changing the Engine to call urlDecode instead of decode_utf8 seems to fix the upload, but then foswiki crashes with a wide print following the successful attach. This isn't the fix, but might give a bit more information as to what's going on.

-- GeorgeClark - 27 Jun 2015

Changing the printJSONRPC subroutine to add the utf8 flag fixes the wide character in print.

$message = JSON::to_json($message, {utf8 => 1, pretty=>1});

So that, plus a core change to urlDecode each parameter in the Engine::CGI seems to fix everything.

-- GeorgeClark - 27 Jun 2015

You are being confused by the way the TopicInteractionPlugin (TIP) works.

As I observed above, the TIP code url-decodes POSTDATA parameters, despite this not being part of the standard. The reason is that the Javascript jquery.uploader.uncompressed.js:297 encodes the postdata that it sends (no idea why, it doesn't need to).

The core is fine. The Engine must not urlDecode POSTDATA - this is an artifact of the TIP only.

You can either:

URL-decode all parameters read in the getRequestParams method, and leave MichaelDaum to untangle the mess
Untangle it for him:
1. get rid of the encodeURI calls in jquery.uploader.uncompressed.js:297
2. get rid of the urlDecode calls in TopicInteractionPlugin/Core.pm
3. review the JS for any other cases of unnecessary encodeURI calls

Here's the patch for (2)

diff --git a/lib/Foswiki/Plugins/TopicInteractionPlugin/Core.pm b/lib/Foswiki/Plugins/TopicInteractionPlugin/Core.pm
index 34dfc2c..1d9bd33 100644
--- a/lib/Foswiki/Plugins/TopicInteractionPlugin/Core.pm
+++ b/lib/Foswiki/Plugins/TopicInteractionPlugin/Core.pm
@@ -197,7 +197,6 @@ sub prepareAction {
 
   # read parameters 
   my $topic = $params->{topic} || $session->{webTopic};
-  $topic = Foswiki::decode_utf8($topic) if ( $Foswiki::UNICODE );
   my $web = $session->{webName};
 
   ($web, $topic) = Foswiki::Func::normalizeWebTopicName($web, $topic);
@@ -241,52 +240,19 @@ sub getRequestParams {
   foreach my $key ($request->param()) {
     if ($key eq 'filename') { #SMELL: hard coded multi-val
       my @val = $request->multi_param($key);
-      $params{$key} = urlDecode($val[0]);
-      $params{$key."s"} = [map {urlDecode($_)} @val];
+      $params{$key} = $val[0];
+      $params{$key."s"} = [ @val ];
     } else {
       my $val = $request->param($key);
-      $params{$key} = urlDecode($val) if defined $val;
+      $params{$key} = $val if defined $val;
       writeDebug("param $key=$val") unless $key eq 'POSTDATA';
     }
   }
 
-  my $queryString = $ENV{QUERY_STRING} || '';
-
-  foreach my $item (split(/[&;]/, $queryString)) {
-    if ($item =~ /^(.+?)=(.*)$/ && !defined($params{$1})) {
-      my $key = $1;
-      my $val = $2;
-      $params{$key} = urlDecode($val);
-      if ($key eq 'filename') { #SMELL: hard coded multi-val
-        $params{$key."s"} = [map {urlDecode($_)} split(/\s*,\s*/, $val)];
-      }
-      writeDebug("param $key=$params{$key}");
-    }
-  }
-
   return \%params;
 }
 
 ##############################################################################
-# this one handles url params that are url-encoded and/or utf8 encoded
-sub urlDecode {
-  my $value = shift;
-
-  $value =~ s/%([\da-f]{2})/chr(hex($1))/gei;
-  my $session = $Foswiki::Plugins::SESSION;
-  unless ($Foswiki::UNICODE) {
-      my $downgradedValue = $session->UTF82SiteCharSet($value);
-      $value = $downgradedValue if defined $downgradedValue;
-  }
- 
-  $value =~ s/^\s+//g;
-  $value =~ s/\s+$//g;
-
-  return $value;
-}
-
-
-##############################################################################
 sub writeDebug {
   print STDERR "- TopicInteractionPlugin - $_[0]\n" if TRACE;
 }
@@ -322,7 +288,7 @@ sub printJSONRPC {
   }
 
   $message = JSON::to_json($message, {pretty=>1});
-  $response->body($message);
+  $response->print($message);
 }
 
 ##############################################################################
diff --git a/pub/System/TopicInteractionPlugin/jquery.uploader.uncompressed.js b/pub/System/TopicInteractionPlugin/jquery.uploader.uncompressed.js
index a23649d..ed87e13 100644
--- a/pub/System/TopicInteractionPlugin/jquery.uploader.uncompressed.js
+++ b/pub/System/TopicInteractionPlugin/jquery.uploader.uncompressed.js
@@ -290,11 +290,11 @@
           }
 
           if (typeof(name) !== 'undefined' && typeof(val) !== 'undefined') {
-            params[name] = encodeURI(val);
+            params[name] = val;
           }
         });
 
-        params.topic = encodeURI(foswiki.getPreference("WEB")) + "." + encodeURI(foswiki.getPreference("TOPIC"));
+        params.topic = foswiki.getPreference("WEB") + "." + foswiki.getPreference("TOPIC");
         params.id = (new Date()).getTime();
 
         if (uploader.features.multipart && uploader.settings.multipart) {

-- Main.CrawfordCurrie - 28 Jun 2015 - 08:32

Just a bit more needed, and it all seems to be working. Now need to put back backwards compatibility.

iff --git a/lib/Foswiki/Plugins/TopicInteractionPlugin/Attachments.pm b/lib/Foswiki/Plugins/TopicInteractionPlugin/Attachments.pm
index 37fd4fd..cbf18d1 100644
--- a/lib/Foswiki/Plugins/TopicInteractionPlugin/Attachments.pm
+++ b/lib/Foswiki/Plugins/TopicInteractionPlugin/Attachments.pm
@@ -183,7 +183,7 @@ sub handle {
     my $iconUrl = '%ICONURL{"' . $info->{name} . '" alt="else"}%';
     my $icon = '%ICON{"' . $info->{name} . '" alt="else"}%';
 
-    my $encName = urlEncode($info, 'name');
+    my $encName = getInfoOrText($info, 'name');
 
     # actions
     my $thisWebDavUrl = $webDavUrl;
@@ -251,7 +251,7 @@ sub handle {
     $text =~ s/\$oldversions\b/$oldVersions/g;
     $text =~ s/\$web\b/$thisWeb/g;
     $text =~ s/\$topic\b/$thisTopic/g;
-    $text =~ s/\$encode\((.*?)\)/urlEncode($info, $1)/ges;
+    $text =~ s/\$encode\((.*?)\)/getInfoOrText($info, $1)/ges;
 
     push @result, $text if $text;
   }
@@ -273,11 +273,10 @@ sub handle {
 }
 
 ##############################################################################
-sub urlEncode {
+sub getInfoOrText {
   my ($infoOrText, $property) = @_;
 
   my $text = defined($property)?$infoOrText->{$property}:$infoOrText;
-  $text =~ s/([^0-9a-zA-Z-_.:~!*\/])/'%'.sprintf('%02X',ord($1))/ge;
 
   return $text;
 }

-- GeorgeClark - 28 Jun 2015

Well done for sticking to this one!

-- Main.CrawfordCurrie - 28 Jun 2015 - 20:48

Michael, Patch for TIP, backwards compatible on Foswiki 1.1.x . (Tested on 1.1.9 with Site Charset set to utf-8.)

TIP-patch: TopicInteractionPlugin Foswiki 1.2 patch (backwards compatible)

I reverted the last patch, as it wasn't right.

Note that the jquery uploader no longer encodes the parameters. I was still able to upload utf8 topics, so not sure when that would be required. So this needs some careful review.

-- GeorgeClark - 29 Jun 2015

Thanks everyone for looking into this issue.

A bit of feedback: I applied the two patches (George's and Crawforwds). Uploading to a Topic with an Umlaut in its name seems to work. Moving the attachment to another page and back works. Deleting the attachment works. Changing a comment works. But the thumbnail for the attachment isn't shown on a topic with an Umlaut in its name (but the thumbnail works on a topic without special characters in its name).

The thumbnail display fails because the browser can't load the image resource - it's trying to load /pub/Main/TestT%f6pic/igp_2ff1b6383d3701f5ccfc3574703a6dd7_wiki-web.png, which fails even though the thumbnail image file itself exists.

I think the topic name should be URL encoded, not UTF-8 encoded.

-- PascalSchuppli - 29 Jun 2015

Removing url-encoding breaks interaction with attachments that quotes in their comments. These things have to be encoded to be on the safe side.

-- MichaelDaum - 14 Jul 2015

Fixed in 4.00

-- MichaelDaum - 17 Jul 2015

ItemTemplate edit

Summary	Using TopicInteractionPlugin to Upload an attachment produces an Internal Server Error
ReportedBy	PascalSchuppli
Codebase	1.2.0 beta2
SVN Range
AppliesTo	Extension
Component	TopicInteractionPlugin
Priority	Normal
CurrentState	Closed
WaitingFor
Checkins	TopicInteractionPlugin:52ea28141842 TopicInteractionPlugin:aec7ecb09b2d TopicInteractionPlugin:d51acdb4c726
TargetRelease	n/a
ReleasedIn	n/a
CheckinsOnBranches	master
trunkCheckins
masterCheckins	TopicInteractionPlugin:52ea28141842 TopicInteractionPlugin:aec7ecb09b2d TopicInteractionPlugin:d51acdb4c726
ItemBranchCheckins
Release01x01Checkins