Feature Proposal: URLPARAM: Don't encode newline parameter substition

Motivation

Be able to use macros as newline, i.e. %BR%

Description and Documentation

We setup a new CommentPlugin template to print comments in an enumeration list like this:

   * %DATE%: %URLPARAM{"comment" newline="%BR%"}% -- %WIKIUSERNAME%

Unfortunately the newline doesn't work because newline translation happens before the encoding.

Reversing this could solve our problem. But I see security and compatibility aspects have to be considered.

Maybe these can be discussed here.

Examples

Impact

WhatDoesItAffect:

%WHATDOESITAFFECT%

edit

Implementation

-- Contributors: StefanH - 18 Oct 2017

Discussion

Since the newline= option comes from the TML macro and not the user's input, it seems safe to me to first encode the url input, and then do newline substitution. But that will most likely break compatibility for anyone expecting the newline= to be encoded. Though I can't see how that is useful.

The change would be in Foswiki::Macros::URLPARAM subroutine _handleURLPARAMValue relocating the line $value =~ s/\r?\n/$newLine/g if ( defined $newLine );, to after the encoding happens.

-- GeorgeClark - 18 Oct 2017

The only place newline= is used in our shippped code is in the two comment templates intended for table prepend / append. Newline is encoded to <br> but encoding is disabled for signed in users. In these examples I don't see any negative impact of relocating the newline substitution.

-- GeorgeClark - 18 Oct 2017

Patch that would implement this (Not Tested!) is:

diff --git a/core/lib/Foswiki/Macros/URLPARAM.pm b/core/lib/Foswiki/Macros/URLPARAM.pm
index 4ceb429..e5bf0a5 100644
--- a/core/lib/Foswiki/Macros/URLPARAM.pm
+++ b/core/lib/Foswiki/Macros/URLPARAM.pm
@@ -65,7 +65,6 @@ sub _handleURLPARAMValue {
     my ( $value, $newLine, $encode, $default ) = @_;
 
     if ( defined $value ) {
-        $value =~ s/\r?\n/$newLine/g if ( defined $newLine );
         foreach my $e ( split( /\s*,\s*/, $encode ) ) {
             if ( $e =~ m/entit(y|ies)/i ) {
                 $value = entityEncode($value);
@@ -86,6 +85,7 @@ sub _handleURLPARAMValue {
                 $value =~ s/([<>%'"])/'&#'.ord($1).';'/ge;
             }
         }
+        $value =~ s/\r?\n/$newLine/g if ( defined $newLine );
     }
     unless ( defined $value ) {
         $value = $default;

-- GeorgeClark - 18 Oct 2017

The TWiki solution to this is convoluted enough to hurt.

        if( defined $newLine ) {
            $newLine =~ s/\$br\b/\0-br-\0/go;
            $newLine =~ s/\$n\b/\0-n-\0/go;
            $value =~ s/\r?\n/$newLine/go;
            $value = _encode( $encode, $value );
            $value =~ s/\0-br-\0/<br \/>/go;
            $value =~ s/\0-n-\0/\n/go;
        } else {
            $value = _encode( $encode, $value );
        }

To me, just changing the order as shown in my suggested patch accomplishes the same thing.

-- GeorgeClark - 18 Oct 2017

Discussed in the foswiki-security email list. This does seem safe and is more of a bugfix than a significant feature change. Implementing for Foswiki 2.2

-- GeorgeClark - 18 Jan 2018

Commenting is disabled while not logged in