Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use correct character encoding when parsing of MIME parameter value #66

Merged
merged 6 commits into from Nov 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -0,0 +1,70 @@
/****************************************************************
* Licensed to the Apache Software Foundation (ASF) under one *
* or more contributor license agreements. See the NOTICE file *
* distributed with this work for additional information *
* regarding copyright ownership. The ASF licenses this file *
* to you under the Apache License, Version 2.0 (the *
* "License"); you may not use this file except in compliance *
* with the License. You may obtain a copy of the License at *
* *
* http://www.apache.org/licenses/LICENSE-2.0 *
* *
* Unless required by applicable law or agreed to in writing, *
* software distributed under the License is distributed on an *
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
* KIND, either express or implied. See the License for the *
* specific language governing permissions and limitations *
* under the License. *
****************************************************************/

package org.apache.james.mime4j.util;

import java.io.UnsupportedEncodingException;
import java.util.HashMap;
import java.util.Map;

public class MimeParameterMapping {

private final Map<String, String> parameters = new HashMap<>();

public Map<String, String> getParameters() {
Map<String,String> result = new HashMap<>();
for (Map.Entry<String, String > entry : parameters.entrySet()) {
result.put(entry.getKey(), decodeParameterValue(entry.getValue()) );
}
return result;
}

public void addParameter(String name, String value) {
String key = removeSectionFromName(name).toLowerCase();
if (parameters.containsKey(key)) {
parameters.put(key, parameters.get(key) + value);
} else {
parameters.put(key, value);
}
}

private String decodeParameterValue(String value) {
if (value == null) {
return null;
}
int charsetEnd = value.indexOf("'");
t4nmoy marked this conversation as resolved.
Show resolved Hide resolved
int languageEnd = value.indexOf("'", charsetEnd + 1);
if (charsetEnd < 0 || languageEnd < 0) {
return MimeUtil.unscrambleHeaderValue(value);
}
String charset = value.substring(0, charsetEnd);
String fileName = value.substring(languageEnd + 1);
try {
return java.net.URLDecoder.decode(fileName, charset);
}
catch (UnsupportedEncodingException ignore) {
}
return MimeUtil.unscrambleHeaderValue(value);
}

private String removeSectionFromName(String parameterName) {
int position = parameterName.indexOf('*');
return parameterName.substring(0, position < 0 ? parameterName.length() : position);
}
}
Expand Up @@ -23,7 +23,6 @@
import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;

Expand Down Expand Up @@ -231,20 +230,8 @@ private void parse() {

if (dispositionType != null) {
this.dispositionType = dispositionType.toLowerCase(Locale.US);

List<String> paramNames = parser.getParamNames();
List<String> paramValues = parser.getParamValues();

if (paramNames != null && paramValues != null) {
final int len = Math.min(paramNames.size(), paramValues.size());
for (int i = 0; i < len; i++) {
String paramName = paramNames.get(i).toLowerCase(Locale.US);
String paramValue = paramValues.get(i);
parameters.put(paramName, paramValue);
}
}
this.parameters.putAll(parser.getParameters());
}

parsed = true;
}

Expand Down
Expand Up @@ -39,6 +39,7 @@
import org.apache.james.mime4j.stream.RawBody;
import org.apache.james.mime4j.stream.RawField;
import org.apache.james.mime4j.stream.RawFieldParser;
import org.apache.james.mime4j.util.MimeParameterMapping;

/**
* Represents a <code>Content-Disposition</code> field.
Expand Down Expand Up @@ -168,9 +169,12 @@ private void parse() {
dispositionType = null;
}
parameters.clear();
for (NameValuePair nmp: body.getParams()) {
String name = nmp.getName().toLowerCase(Locale.US);
parameters.put(name, nmp.getValue());
MimeParameterMapping mapping = new MimeParameterMapping();
for (NameValuePair pair : body.getParams()) {
mapping.addParameter(pair.getName(), pair.getValue());
}
for (Map.Entry<String, String> entry : mapping.getParameters().entrySet()) {
parameters.put(entry.getKey(), entry.getValue());
}
}

Expand Down
Expand Up @@ -52,25 +52,21 @@ PARSER_BEGIN(ContentDispositionParser)
****************************************************************/
package org.apache.james.mime4j.field.contentdisposition.parser;

import java.util.List;
import java.util.ArrayList;
import org.apache.james.mime4j.util.MimeParameterMapping;
import java.util.Map;

public class ContentDispositionParser {

private String dispositionType;
private List<String> paramNames = new ArrayList<String>();
private List<String> paramValues = new ArrayList<String>();

private final MimeParameterMapping mapping = new MimeParameterMapping();

public String getDispositionType() {
return dispositionType;
}

public List<String> getParamNames() {
return paramNames;
}

public List<String> getParamValues() {
return paramValues;
public Map<String, String> getParameters() {
return mapping.getParameters();
}

public static void main(String args[]) throws ParseException {
Expand Down Expand Up @@ -121,8 +117,7 @@ void parameter() :
{
attrib=<ATOKEN> "=" val=value()
{
paramNames.add(attrib.image);
paramValues.add(val);
mapping.addParameter(attrib.image, val);
}
}

Expand Down
Expand Up @@ -92,6 +92,13 @@ public void testIsDispositionType() throws Exception {
assertFalse(f.isAttachment());
}

private static String getMessage(ContentDispositionField field) {
if( field.getParseException() == null) {
return "";
}
return field.getParseException().getMessage() + " when parsing " + field.getRaw();
}

public void testGetFilename() throws Exception {
ContentDispositionField f = parse("Content-Disposition: inline; filename=yada.txt");
assertEquals("yada.txt", f.getFilename());
Expand All @@ -106,6 +113,48 @@ public void testGetFilename() throws Exception {
assertNull(f.getFilename());
}

public void testNonAsciiFilename() throws MimeException {
ContentDispositionField f =
parse("Content-Disposition: attachment;"
+ "\nfilename*0=\"=?UTF-8?Q?3-2_FORPROSJEKT_2-Sheet_-_XXX_A_2_40_?="
+ "\n=?UTF-8?Q?\";"
+ "\nfilename*1=\"201_-_Fasader_nord=C3=B8st_og_nordvest.dwg?=\"");

assertEquals(getMessage(f),
"3-2 FORPROSJEKT 2-Sheet - XXX A 2 40 201 - Fasader nordøst og nordvest.dwg",
f.getFilename());
}
Comment on lines +116 to +126
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get this test too for the lenient version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure thing, but are suggesting to move this test to LenientContentDispositionFieldTest or just replicate the same test in LenientContentDispositionFieldTest?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replicate in my opinion.

The best way to go in the future is to have a common "contract" for both the lenient and non lenient implementations but this clearly looks like another topic to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I agree with you


public void testExtendedNotation() throws MimeException {
ContentDispositionField f = parse("Content-Disposition: attachment;\n" +
" filename*=utf-8''%D8%AF%D9%8A%D9%86%D8%A7%D8%B5%D9%88%D8%B1%2E%6F%64%74");
String name = f.getFilename();
assertEquals(getMessage(f), "ديناصور.odt", name);
}

public void testExtendedNotationWithEmptyCharsetShouldNotCrash() throws MimeException {
String fileName = "''%D8%AF%D9%8A%D9%86%D8%A7%D8%B5%D9%88%D8%B1%2E%6F%64%74";
String fileNameString = String.format("\"Content-Disposition: attachment;\n" +
" \nfilename*=%s\"", fileName);
ContentDispositionField f = parse(fileNameString);
String name = f.getFilename();
assertEquals(getMessage(f), fileName, name);
}

public void testFileNameWithInitialSection() throws MimeException {
ContentDispositionField f = parse("Content-Disposition: attachment;"
+ "\nfilename*0*=filename.txt");
assertEquals(getMessage(f),"filename.txt", f.getFilename());
}

public void testFileNameWithMultipleSections() throws MimeException {
ContentDispositionField f = parse("Content-Disposition: attachment;"
+ "\nfilename*0=\"first part \"; filename*1=\"of long filename.txt\"");
f.getFilename();
assertEquals(getMessage(f),"first part of long filename.txt", f.getFilename());
}


public void testGetCreationDate() throws Exception {
ContentDispositionField f = parse("Content-Disposition: inline; "
+ "creation-date=\"Tue, 01 Jan 1970 00:00:00 +0000\"");
Expand Down
Expand Up @@ -174,4 +174,27 @@ public void testGetSize() throws Exception {
Assert.assertEquals(12, f.getSize());
}

@Test
public void testMultipartFileName() throws Exception {
ContentDispositionField f = parse("Content-Disposition: attachment;\n" +
" filename*0=\"looooooooooooooooooooooooooooooooooooooooooooooooooooooooooo\";\n" +
" filename*1=\"oooooooooooooooooooooooooooooooooooooong_fiiiiiiiiiiiiiiiiii\";\n" +
" filename*2=\"iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiileeeeeeeeeeeeeeeeeee\";\n" +
" filename*3=\"eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee.txt\"");
Assert.assertEquals("loooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo" +
"ooooooooooooooong_fiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiileeeeeeeeeeeeeeeeeeeeeeeeeeeeee" +
"eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee.txt", f.getFilename());
}
chibenwa marked this conversation as resolved.
Show resolved Hide resolved

@Test
public void testNonAsciiFilename() throws MimeException {
ContentDispositionField f = parse("Content-Disposition: attachment;"
+ "\nfilename*0=\"=?UTF-8?Q?3-2_FORPROSJEKT_2-Sheet_-_XXX_A_2_40_?="
+ "\n=?UTF-8?Q?\";"
+ "\nfilename*1=\"201_-_Fasader_nord=C3=B8st_og_nordvest.dwg?=\"");

Assert.assertEquals("3-2 FORPROSJEKT 2-Sheet - XXX A 2 40 201 - Fasader nordøst og nordvest.dwg",
f.getFilename());
}

}