gmantele / vollt

Java libraries implementing the IVOA protocol: ADQL, UWS and TAP
http://cdsportal.u-strasbg.fr/taptuto/
29 stars 28 forks source link

DefaultOwnerGroupIdentifier.getOwnerGroup(JobOwner) throws Regex Error (Windows only) #87

Closed marcdexet-cnrs closed 5 years ago

marcdexet-cnrs commented 6 years ago

Description

When I use a JobOwner on a Windows plateform (nobody's perfect :) ) uws.service.file.DefaultOwnerGroupIdentifier.getOwnerGroup(JobOwner) throws a Regex Error java.util.regex.PatternSyntaxException because of File.separator use as pattern.

It's a well-known trap (cf; https://stackoverflow.com/questions/13175129/split-and-error )

owner.getID().trim().replaceAll(File.separator, "_") throws

  java.util.regex.PatternSyntaxException: Unexpected internal error near index 1
  \
 ^
    at java.util.regex.Pattern.error(Pattern.java:1955)
    at java.util.regex.Pattern.compile(Pattern.java:1702)
    at java.util.regex.Pattern.<init>(Pattern.java:1351)
    at java.util.regex.Pattern.compile(Pattern.java:1028)
    at java.lang.String.replaceAll(String.java:2223)
    at uws.service.file.DefaultOwnerGroupIdentifier.getOwnerGroup(DefaultOwnerGroupIdentifier.java:47)

You can test it with this code

import java.io.File;
public class MyClass {
    public static void main(String args[]) {
        String userId = "user-value";
        System.out.println(userId.trim().replaceAll("\\", "_"));
    }
}

Solution

Use java.util.regex.Pattern.Quote(String)

and turn the failing line in to

owner.getID().trim().replaceAll( Pattern.Quote(File.separator), "_")

marcdexet-cnrs commented 6 years ago

The same for uws.service.file.LocalUWSFileManager.getOwnerDirectory(JobOwner)

marcdexet-cnrs commented 6 years ago

sonarQube and SQUID say :

 Inappropriate regular expressions should not be used (squid:S2639)

BUG Bug MAJOR Major

Regular expressions are powerful but tricky, and even those long used to using them can make mistakes.

The following should not be used as regular expressions:
• . - matches any single character. Used in replaceAll, it matches everything 
• | - normally used as an option delimiter. Used stand-alone, it matches the space between characters 
• File.separator - matches the platform-specific file path delimiter. On Windows, this will be taken as an escape character 

Noncompliant Code Example
String str = "/File|Name.txt";

String clean = str.replaceAll(".",""); // Noncompliant; probably meant to remove only dot chars, but returns an empty string
String clean2 = str.replaceAll("|","_"); // Noncompliant; yields _/_F_i_l_e_|_N_a_m_e_._t_x_t_
String clean3 = str.replaceAll(File.separator,""); // Noncompliant; exception on Windows
gmantele commented 6 years ago

I agree. I used File.separator on purpose in order to be compatible with other OS than Unix-like, but I admit not having tested it on Windows....I should have....shame on me :( Tell me if you have any other issue with Windows.