472975: Mylyn escapes special characters before writing

repository properties

Change-Id: I8228411b427631b0e1ffd8f6bf6566e49af71098
Task-Url: https://bugs.eclipse.org/bugs/show_bug.cgi?id=472975
Signed-off-by: Jaxsun McCarthy Huggan <jaxsun.mccarthy@tasktop.com>

7 files changed, 493 insertions, 67 deletions

diff --git a/org.eclipse.mylyn.tasks.core.tests/src/org/eclipse/mylyn/internal/tasks/core/LegacySaxRepositoriesTest.java b/org.eclipse.mylyn.tasks.core.tests/src/org/eclipse/mylyn/internal/tasks/core/LegacySaxRepositoriesTest.java
new file mode 100644
index 000000000..ecd98525b
--- /dev/null
+++ b/org.eclipse.mylyn.tasks.core.tests/src/org/eclipse/mylyn/internal/tasks/core/LegacySaxRepositoriesTest.java
@@ -0,0 +1,139 @@
+/*******************************************************************************
+ * Copyright (c) 2016 Tasktop Technologies and others.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *     Tasktop Technologies - initial API and implementation
+ *******************************************************************************/
+
+package org.eclipse.mylyn.internal.tasks.core;
+
+import static org.junit.Assert.assertEquals;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.HashSet;
+import java.util.Set;
+
+import org.eclipse.mylyn.tasks.core.TaskRepository;
+import org.junit.Test;
+import org.xml.sax.Attributes;
+import org.xml.sax.InputSource;
+import org.xml.sax.SAXException;
+import org.xml.sax.XMLReader;
+import org.xml.sax.helpers.DefaultHandler;
+
+import com.google.common.collect.ImmutableSet;
+
+public class LegacySaxRepositoriesTest {
+
+	private class SaxRepositoriesContentHandlerVersion1 extends DefaultHandler {
+
+		private final Set<TaskRepository> taskRepositories = new HashSet<TaskRepository>();
+
+		@SuppressWarnings({ "deprecation", "restriction" })
+		@Override
+		public void startElement(String uri, String localName, String qName, Attributes attributes)
+				throws SAXException {
+			try {
+				if (localName.equals(TaskRepositoriesExternalizer.ELEMENT_TASK_REPOSITORY) && attributes != null) {
+					String kind = org.eclipse.mylyn.internal.commons.core.XmlStringConverter
+							.convertXmlToString(attributes.getValue(IRepositoryConstants.PROPERTY_CONNECTOR_KIND));
+					String url = org.eclipse.mylyn.internal.commons.core.XmlStringConverter
+							.convertXmlToString(attributes.getValue(IRepositoryConstants.PROPERTY_URL));
+					if (kind != null && kind.length() > 0 && url != null && url.length() > 0) {
+						TaskRepository repository = new TaskRepository(kind, url);
+						for (int index = 0; index < attributes.getLength(); index++) {
+							String key = org.eclipse.mylyn.internal.commons.core.XmlStringConverter
+									.convertXmlToString(attributes.getLocalName(index));
+							String value = org.eclipse.mylyn.internal.commons.core.XmlStringConverter
+									.convertXmlToString(attributes.getValue(index));
+							repository.setProperty(key, value);
+						}
+						taskRepositories.add(repository);
+					}
+				}
+			} catch (Exception e) {
+				e.printStackTrace();
+			}
+
+		}
+
+		public Set<TaskRepository> getRepositories() {
+			return taskRepositories;
+		}
+	}
+
+	private static final String kind = "connector.kind";
+
+	private static final String firstUrl = "http://first.url";
+
+	private static final String secondUrl = "http://second.url";
+
+	private static final String labelPropertyValue = "test repository";
+
+	private static final String labelPropertyValueAlternate = "test repository alternate";
+
+	private static final String labelPropertyKey = "label";
+
+	private final String repositoryXmlVersion2 = String.format("<?xml version=\"1.0\" encoding=\"UTF-8\"?>" //
+			+ "<TaskRepositories OutputVersion=\"2\">" //
+			+ "<TaskRepository url=\"%s\" kind=\"%s\" %s=\"%s\"><Property key=\"%s\" value=\"%s\"/></TaskRepository>" //
+			+ "<TaskRepository url=\"%s\" kind=\"%s\" %s=\"%s\"><Property key=\"%s\" value=\"%s\"/></TaskRepository>" //
+			+ "</TaskRepositories>", //
+			firstUrl, kind, labelPropertyKey, labelPropertyValue, labelPropertyKey, labelPropertyValueAlternate,
+			secondUrl, kind, labelPropertyKey, labelPropertyValue, labelPropertyKey, labelPropertyValueAlternate);
+
+	@Test
+	public void version1ReaderCanReadVersion2Xml() throws Exception {
+		Set<TaskRepository> expectedRepositories = ImmutableSet.of(
+				createTestRepository(kind, firstUrl, labelPropertyKey, labelPropertyValue),
+				createTestRepository(kind, secondUrl, labelPropertyKey, labelPropertyValue));
+		Set<TaskRepository> repositories = parseRepositoriesWithVersion1Parser(repositoryXmlVersion2);
+		assertEquals(expectedRepositories, repositories);
+		for (TaskRepository repository : repositories) {
+			assertEquals(labelPropertyValue, repository.getProperty(labelPropertyKey));
+		}
+	}
+
+	@Test
+	public void version1CanReadLatestOutput() throws Exception {
+		TaskRepository initialRepository = createTestRepository(kind, firstUrl, labelPropertyKey, labelPropertyValue);
+		String xml = writeToXmlWithCurrentVersion(ImmutableSet.of(initialRepository));
+		Set<TaskRepository> parsed = parseRepositoriesWithVersion1Parser(xml);
+		assertEquals(1, parsed.size());
+		TaskRepository parsedRepository = parsed.iterator().next();
+		assertEquals(initialRepository, parsedRepository);
+		assertEquals(labelPropertyValue, parsedRepository.getProperty(labelPropertyKey));
+
+	}
+
+	private String writeToXmlWithCurrentVersion(Set<TaskRepository> repositories) throws IOException {
+		ByteArrayOutputStream output = new ByteArrayOutputStream(1000 * repositories.size());
+		SaxRepositoriesWriter writer = new SaxRepositoriesWriter();
+		writer.setOutputStream(output);
+		writer.writeRepositoriesToStream(repositories);
+		return output.toString("UTF-8");
+	}
+
+	private Set<TaskRepository> parseRepositoriesWithVersion1Parser(String xml) throws Exception {
+		SaxRepositoriesContentHandlerVersion1 handler = new SaxRepositoriesContentHandlerVersion1();
+		XMLReader reader = XmlReaderUtil.createXmlReader();
+		reader.setContentHandler(handler);
+		reader.parse(new InputSource(new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8))));
+		return handler.getRepositories();
+	}
+
+	private TaskRepository createTestRepository(String kind, String url, String labelPropertyKey,
+			String labelProperty) {
+		TaskRepository repository = new TaskRepository(kind, url);
+		repository.setProperty(labelPropertyKey, labelProperty);
+		return repository;
+	}
+
+}
diff --git a/org.eclipse.mylyn.tasks.core.tests/src/org/eclipse/mylyn/internal/tasks/core/SaxRepositoriesTest.java b/org.eclipse.mylyn.tasks.core.tests/src/org/eclipse/mylyn/internal/tasks/core/SaxRepositoriesTest.java
new file mode 100644
index 000000000..de9d60df8
--- /dev/null
+++ b/org.eclipse.mylyn.tasks.core.tests/src/org/eclipse/mylyn/internal/tasks/core/SaxRepositoriesTest.java
@@ -0,0 +1,237 @@
+/*******************************************************************************
+ * Copyright (c) 2015 Tasktop Technologies and others.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *     Tasktop Technologies - initial API and implementation
+ *******************************************************************************/
+
+package org.eclipse.mylyn.internal.tasks.core;
+
+import static org.eclipse.mylyn.internal.commons.core.XmlStringConverter.convertToXmlString;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.List;
+import java.util.Set;
+
+import org.eclipse.mylyn.tasks.core.TaskRepository;
+import org.junit.Before;
+import org.junit.Test;
+import org.xml.sax.InputSource;
+import org.xml.sax.SAXException;
+import org.xml.sax.XMLReader;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
+
+public class SaxRepositoriesTest {
+
+	private static final String kind = "connector.kind";
+
+	private static final String kindCharacters = "connector.kind-`~!@#$%^&*()_+-=[{}]\\|'\";:/?.>,<";
+
+	private static final String firstUrl = "http://first.url";
+
+	private static final String secondUrl = "http://second.url";
+
+	private static final String urlCharacters = "http://some.url--`~!@#$%^&*()_+-=[{}]\\|'\";:/?.>,<";
+
+	private static final String labelPropertyValue = "test repository";
+
+	private static final String labelPropertyValueAlternate = "test repository alternate";
+
+	private static final String labelPropertyKey = "label";
+
+	private static final String labelPropertyCharacters = "`~!@#$%^&*()_+-=[{}]\\|'\";:/?.>,<";
+
+	private static final String labelPropertyKeyCharacters = "label-`~!@#$%^&*()_+-=[{}]\\|'\";:/?.>,<";
+
+	private SaxRepositoriesContentHandler handler;
+
+	private final String version1RepositoryXml = String.format("<?xml version=\"1.0\" encoding=\"UTF-8\"?>" //
+			+ "<TaskRepositories OutputVersion=\"1\">" //
+			+ "<TaskRepository url=\"%s\" kind=\"%s\" %s=\"%s\"/>" //
+			+ "</TaskRepositories>", firstUrl, kind, labelPropertyKey, labelPropertyValue);
+
+	private final String version1RepositoryXmlMultiple = String.format(
+			"<?xml version=\"1.0\" encoding=\"UTF-8\"?>" //
+					+ "<TaskRepositories OutputVersion=\"1\">" //
+					+ "<TaskRepository url=\"%s\" kind=\"%s\" %s=\"%s\"/>" //
+					+ "<TaskRepository url=\"%s\" kind=\"%s\" %s=\"%s\"/>" //
+					+ "</TaskRepositories>",
+			firstUrl, kind, labelPropertyKey, labelPropertyValue, secondUrl, kind, labelPropertyKey,
+			labelPropertyValue);
+
+	/**
+	 * The old xml is escaped twice: once by the xml library and once within mylyn
+	 */
+	private final String version1RepositoryXmlSpecialCharacters = String.format(
+			"<?xml version=\"1.0\" encoding=\"UTF-8\"?>" //
+					+ "<TaskRepositories OutputVersion=\"1\">" //
+					+ "<TaskRepository url=\"%s\" kind=\"%s\" %s=\"%s\"/>" //
+					+ "</TaskRepositories>",
+			escapeXml(escapeXml(urlCharacters)), escapeXml(escapeXml(kindCharacters)), labelPropertyKey,
+			escapeXml(escapeXml(labelPropertyCharacters)));
+
+	private final String version2RepositoryXml = String.format("<?xml version=\"1.0\" encoding=\"UTF-8\"?>" //
+			+ "<TaskRepositories OutputVersion=\"2\">" //
+			+ "<TaskRepository url=\"%s\" kind=\"%s\"><Property key=\"%s\" value=\"%s\"/></TaskRepository>" //
+			+ "</TaskRepositories>" //
+			, firstUrl, kind, labelPropertyKey, labelPropertyValue);
+
+	private final String version2RepositoryXmlMultiple = String.format(
+			"<?xml version=\"1.0\" encoding=\"UTF-8\"?>" //
+					+ "<TaskRepositories OutputVersion=\"2\">" //
+					+ "<TaskRepository url=\"%s\" kind=\"%s\"><Property key=\"%s\" value=\"%s\"/></TaskRepository>" //
+					+ "<TaskRepository url=\"%s\" kind=\"%s\"><Property key=\"%s\" value=\"%s\"/></TaskRepository>" //
+					+ "</TaskRepositories>",
+			firstUrl, kind, labelPropertyKey, labelPropertyValue, secondUrl, kind, labelPropertyKey,
+			labelPropertyValue);
+
+	private final String version2RepositoryXmlSpecialCharacters = String.format(
+			"<?xml version=\"1.0\" encoding=\"UTF-8\"?>" //
+					+ "<TaskRepositories OutputVersion=\"2\">" //
+					+ "<TaskRepository url=\"%s\" kind=\"%s\"><Property key=\"%s\" value=\"%s\"/></TaskRepository>" //
+					+ "</TaskRepositories>",
+			escapeXml(escapeXml(urlCharacters)), escapeXml(escapeXml(kindCharacters)),
+			escapeXml(labelPropertyKeyCharacters), escapeXml(labelPropertyCharacters));
+
+	private final String version1AndVersion2RepositoryXmlMultiple = String.format(
+			"<?xml version=\"1.0\" encoding=\"UTF-8\"?>" //
+					+ "<TaskRepositories OutputVersion=\"2\">" //
+					+ "<TaskRepository url=\"%s\" kind=\"%s\" %s=\"%s\"><Property key=\"%s\" value=\"%s\"/></TaskRepository>" //
+					+ "<TaskRepository url=\"%s\" kind=\"%s\" %s=\"%s\"><Property key=\"%s\" value=\"%s\"/></TaskRepository>" //
+					+ "</TaskRepositories>", //
+			firstUrl, kind, labelPropertyKey, labelPropertyValueAlternate, labelPropertyKey, labelPropertyValue,
+			secondUrl, kind, labelPropertyKey, labelPropertyValueAlternate, labelPropertyKey, labelPropertyValue);
+
+	@Before
+	public void setUp() {
+		handler = new SaxRepositoriesContentHandler();
+	}
+
+	@Test
+	public void readVersion1() throws SAXException, IOException {
+		assertRead(version1RepositoryXml, Sets.newHashSet(createTestRepository(firstUrl)));
+	}
+
+	@Test
+	public void readVersion1Multiple() throws SAXException, IOException {
+		assertRead(version1RepositoryXmlMultiple,
+				Sets.newHashSet(createTestRepository(firstUrl), createTestRepository(secondUrl)));
+	}
+
+	@Test
+	public void readVersion1Characters() throws SAXException, IOException {
+		assertRead(version1RepositoryXmlSpecialCharacters,
+				Sets.newHashSet(
+						createTestRepository(kindCharacters, urlCharacters, labelPropertyKey, labelPropertyCharacters)),
+				labelPropertyKey, labelPropertyCharacters);
+	}
+
+	@Test
+	public void readVersion2() throws SAXException, IOException {
+		assertRead(version2RepositoryXml, Sets.newHashSet(createTestRepository(firstUrl)));
+	}
+
+	@Test
+	public void readVersion2Multiple() throws SAXException, IOException {
+		assertRead(version2RepositoryXmlMultiple,
+				Sets.newHashSet(createTestRepository(firstUrl), createTestRepository(secondUrl)));
+	}
+
+	@Test
+	public void readVersion2Characters() throws SAXException, IOException {
+		assertRead(
+				version2RepositoryXmlSpecialCharacters, Sets.newHashSet(createTestRepository(kindCharacters,
+						urlCharacters, labelPropertyKeyCharacters, labelPropertyCharacters)),
+				labelPropertyKeyCharacters, labelPropertyCharacters);
+	}
+
+	@Test
+	public void write() throws SAXException, IOException {
+		OutputStream output = new ByteArrayOutputStream();
+		write(output, ImmutableList.of(createTestRepository(firstUrl)));
+		assertRead(output.toString(), Sets.newHashSet(createTestRepository(firstUrl)));
+	}
+
+	@Test
+	public void writeMultiple() throws SAXException, IOException {
+		OutputStream output = new ByteArrayOutputStream();
+		write(output, ImmutableList.of(createTestRepository(firstUrl), createTestRepository(secondUrl)));
+		assertRead(output.toString(), Sets.newHashSet(createTestRepository(firstUrl), createTestRepository(secondUrl)));
+	}
+
+	@Test
+	public void readMixed() throws Exception {
+		assertRead(version1AndVersion2RepositoryXmlMultiple,
+				Sets.newHashSet(createTestRepository(firstUrl), createTestRepository(secondUrl)));
+	}
+
+	@Test
+	public void writeMixedBadCharacters() throws Exception {
+		OutputStream output = new ByteArrayOutputStream();
+		write(output, Lists
+				.newArrayList(createTestRepository(kind, firstUrl, labelPropertyKeyCharacters, labelPropertyValue)));
+		String serialized = output.toString();
+		// the Mylyn escaping utility turns ' into apos, but this is not done during the actual serialization
+		assertTrue(
+				serialized.contains(("key=\"" + escapeXml(labelPropertyKeyCharacters) + "\"").replace("&apos;", "'")));
+		assertTrue(serialized.contains("value=\"" + escapeXml(labelPropertyValue) + "\""));
+		// the property should have only been written once
+		assertEquals(serialized.lastIndexOf(labelPropertyValue), serialized.indexOf(labelPropertyValue));
+	}
+
+	private void assertRead(String xml, Set<TaskRepository> expectedRepositories) throws SAXException, IOException {
+		assertRead(xml, expectedRepositories, labelPropertyKey, labelPropertyValue);
+	}
+
+	private void assertRead(String xml, Set<TaskRepository> expectedRepositories, String labelPropertyKey,
+			String labelPropertyValue) throws SAXException, IOException {
+		parse(xml);
+		Set<TaskRepository> repositories = handler.getRepositories();
+		assertEquals(expectedRepositories, repositories);
+		for (TaskRepository repository : repositories) {
+			assertEquals(labelPropertyValue, repository.getProperty(labelPropertyKey));
+		}
+	}
+
+	private void parse(String xml) throws SAXException, IOException {
+		XMLReader reader = XmlReaderUtil.createXmlReader();
+		reader.setContentHandler(handler);
+		reader.parse(new InputSource(new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8))));
+	}
+
+	private void write(OutputStream output, List<TaskRepository> repositories) throws IOException {
+		SaxRepositoriesWriter writer = new SaxRepositoriesWriter();
+		writer.setOutputStream(output);
+		writer.writeRepositoriesToStream(repositories);
+	}
+
+	private TaskRepository createTestRepository(String url) {
+		return createTestRepository(kind, url, labelPropertyKey, labelPropertyValue);
+	}
+
+	private TaskRepository createTestRepository(String kind, String url, String labelPropertyKey,
+			String labelProperty) {
+		TaskRepository repository = new TaskRepository(kind, url);
+		repository.setProperty(labelPropertyKey, labelProperty);
+		return repository;
+	}
+
+	@SuppressWarnings({ "restriction", "deprecation" })
+	private String escapeXml(String stringToEscape) {
+		return convertToXmlString(stringToEscape);
+	}
+
+}
diff --git a/org.eclipse.mylyn.tasks.core/META-INF/MANIFEST.MF b/org.eclipse.mylyn.tasks.core/META-INF/MANIFEST.MF
index 9c3b4eff7..2a4b7fbde 100644
--- a/org.eclipse.mylyn.tasks.core/META-INF/MANIFEST.MF
+++ b/org.eclipse.mylyn.tasks.core/META-INF/MANIFEST.MF
@@ -12,7 +12,7 @@ Require-Bundle: org.eclipse.core.runtime,
  org.eclipse.mylyn.commons.net;bundle-version="3.8.0",
  org.eclipse.mylyn.commons.repositories.core;bundle-version="1.0.0",
  com.google.guava;bundle-version="15.0.0",
- org.apache.xerces;bundle-version="2.9.0";resolution:=optional
+ org.apache.xerces;bundle-version="2.9.0"
 Export-Package: org.eclipse.mylyn.internal.tasks.core;x-friends:="org.eclipse.mylyn.tasks.ui,org.eclipse.mylyn.tasks.bugs",
  org.eclipse.mylyn.internal.tasks.core.activity;x-friends:="org.eclipse.mylyn.tasks.ui,org.eclipse.mylyn.tasks.bugs",
  org.eclipse.mylyn.internal.tasks.core.context;x-friends:="org.eclipse.mylyn.tasks.ui,org.eclipse.mylyn.tasks.bugs",
diff --git a/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/SaxRepositoriesContentHandler.java b/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/SaxRepositoriesContentHandler.java
index 7f0aca415..3f2b108ce 100644
--- a/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/SaxRepositoriesContentHandler.java
+++ b/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/SaxRepositoriesContentHandler.java
@@ -11,17 +11,24 @@
 
 package org.eclipse.mylyn.internal.tasks.core;
 
+import static org.eclipse.mylyn.internal.commons.core.XmlStringConverter.convertXmlToString;
+
 import java.util.HashSet;
 import java.util.Set;
 
+import org.eclipse.core.runtime.IStatus;
+import org.eclipse.core.runtime.Status;
+import org.eclipse.mylyn.commons.core.StatusHandler;
 import org.eclipse.mylyn.tasks.core.TaskRepository;
 import org.xml.sax.Attributes;
 import org.xml.sax.SAXException;
 import org.xml.sax.helpers.DefaultHandler;
 
+import com.google.common.base.Strings;
+
 /**
  * Adapted from SaxContextContentHandler
- * 
+ *
  * @author Rob Elves
  */
 public class SaxRepositoriesContentHandler extends DefaultHandler {
@@ -30,27 +37,52 @@ public class SaxRepositoriesContentHandler extends DefaultHandler {
 
 	private final Set<TaskRepository> taskRepositories = new HashSet<TaskRepository>();
 
-	@SuppressWarnings({ "deprecation", "restriction" })
+	private TaskRepository currentRepository;
+
 	@Override
 	public void startElement(String uri, String localName, String qName, Attributes attributes) throws SAXException {
 		try {
-			if (localName.equals(TaskRepositoriesExternalizer.ELEMENT_TASK_REPOSITORY) && attributes != null) {
-				String kind = org.eclipse.mylyn.internal.commons.core.XmlStringConverter.convertXmlToString(attributes.getValue(IRepositoryConstants.PROPERTY_CONNECTOR_KIND));
-				String url = org.eclipse.mylyn.internal.commons.core.XmlStringConverter.convertXmlToString(attributes.getValue(IRepositoryConstants.PROPERTY_URL));
-				if (kind != null && kind.length() > 0 && url != null && url.length() > 0) {
-					TaskRepository repository = new TaskRepository(kind, url);
-					for (int index = 0; index < attributes.getLength(); index++) {
-						String key = org.eclipse.mylyn.internal.commons.core.XmlStringConverter.convertXmlToString(attributes.getLocalName(index));
-						String value = org.eclipse.mylyn.internal.commons.core.XmlStringConverter.convertXmlToString(attributes.getValue(index));
-						repository.setProperty(key, value);
-					}
-					taskRepositories.add(repository);
-				}
+			if (localName.equals(TaskRepositoriesExternalizer.ELEMENT_TASK_REPOSITORY)) {
+				handleRepositoryElement(attributes);
+			} else if (localName.equals(TaskRepositoriesExternalizer.ELEMENT_PROPERTY) && currentRepository != null) {
+				// properties are stored as attributes on the repository node as well as children property nodes
+				handleProperty(attributes);
 			}
 		} catch (Exception e) {
-			e.printStackTrace();
+			StatusHandler.log(new Status(IStatus.ERROR, ITasksCoreConstants.ID_PLUGIN, "Could not read repositories" //$NON-NLS-1$
+					, e));
+		}
+	}
+
+	@Override
+	public void endElement(String uri, String localName, String qName) {
+		if (currentRepository != null && localName.equals(TaskRepositoriesExternalizer.ELEMENT_TASK_REPOSITORY)) {
+			taskRepositories.add(currentRepository);
+			currentRepository = null;
+		}
+	}
+
+	@SuppressWarnings({ "deprecation", "restriction" })
+	private void handleRepositoryElement(Attributes attributes) throws SAXException {
+		String kind = convertXmlToString(attributes.getValue(IRepositoryConstants.PROPERTY_CONNECTOR_KIND));
+		String url = convertXmlToString(attributes.getValue(IRepositoryConstants.PROPERTY_URL));
+		if (!Strings.isNullOrEmpty(kind) && !Strings.isNullOrEmpty(url)) {
+			currentRepository = new TaskRepository(kind, url);
+			// properties are stored as attributes on the repository node as well as children property nodes
+			for (int index = 0; index < attributes.getLength(); index++) {
+				String key = convertXmlToString(attributes.getLocalName(index));
+				String value = convertXmlToString(attributes.getValue(index));
+				currentRepository.setProperty(key, value);
+			}
 		}
+	}
 
+	private void handleProperty(Attributes attributes) throws SAXException {
+		String key = attributes.getValue(TaskRepositoriesExternalizer.PROPERTY_KEY);
+		String value = attributes.getValue(TaskRepositoriesExternalizer.PROPERTY_VALUE);
+		if (key != null && value != null) {
+			currentRepository.setProperty(key, value);
+		}
 	}
 
 	public Set<TaskRepository> getRepositories() {
diff --git a/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/SaxRepositoriesWriter.java b/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/SaxRepositoriesWriter.java
index a0b669707..11e5368a8 100644
--- a/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/SaxRepositoriesWriter.java
+++ b/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/SaxRepositoriesWriter.java
@@ -11,6 +11,8 @@
 
 package org.eclipse.mylyn.internal.tasks.core;
 
+import static org.eclipse.mylyn.internal.commons.core.XmlStringConverter.convertToXmlString;
+
 import java.io.IOException;
 import java.io.OutputStream;
 import java.util.ArrayList;
@@ -22,6 +24,7 @@ import javax.xml.transform.TransformerFactory;
 import javax.xml.transform.sax.SAXSource;
 import javax.xml.transform.stream.StreamResult;
 
+import org.apache.xerces.util.XMLChar;
 import org.eclipse.core.runtime.IStatus;
 import org.eclipse.core.runtime.Status;
 import org.eclipse.mylyn.commons.core.StatusHandler;
@@ -39,7 +42,7 @@ import org.xml.sax.helpers.AttributesImpl;
 
 /**
  * Adapted from SaxContextWriter
- * 
+ *
  * @author Rob Elves
  */
 public class SaxRepositoriesWriter {
@@ -84,16 +87,6 @@ public class SaxRepositoriesWriter {
 
 	private static class RepositoriesWriter implements XMLReader {
 
-//		private static final String ELEMENT_TASK_REPOSITORIES = "TaskRepositories";
-//
-//		public static final String ELEMENT_TASK_REPOSITORY = "TaskRepository";
-//
-//		private static final String ATTRIBUTE_VERSION = "xmlVersion";
-
-//		private static final String ATTRIBUTE_URL = "Url";
-//
-//		private static final String ATTRIBUTE_KIND = "Kind";
-
 		private ContentHandler handler;
 
 		private ErrorHandler errorHandler;
@@ -145,7 +138,6 @@ public class SaxRepositoriesWriter {
 			return errorHandler;
 		}
 
-		@SuppressWarnings({ "deprecation", "restriction" })
 		public void parse(InputSource input) throws IOException, SAXException {
 			if (!(input instanceof TaskRepositoriesInputSource)) {
 				throw new SAXException("Can only parse writable input sources"); //$NON-NLS-1$
@@ -154,40 +146,76 @@ public class SaxRepositoriesWriter {
 			Collection<TaskRepository> repositories = ((TaskRepositoriesInputSource) input).getRepositories();
 
 			handler.startDocument();
+			writeRepositories(repositories);
+			handler.endDocument();
+		}
+
+		private void writeRepositories(Collection<TaskRepository> repositories) throws IOException, SAXException {
 			AttributesImpl rootAttributes = new AttributesImpl();
 			rootAttributes.addAttribute("", TaskRepositoriesExternalizer.ATTRIBUTE_VERSION, //$NON-NLS-1$
-					TaskRepositoriesExternalizer.ATTRIBUTE_VERSION, "", "1"); //$NON-NLS-1$ //$NON-NLS-2$
+					TaskRepositoriesExternalizer.ATTRIBUTE_VERSION, "", "2"); //$NON-NLS-1$ //$NON-NLS-2$
 
 			handler.startElement("", TaskRepositoriesExternalizer.ELEMENT_TASK_REPOSITORIES, //$NON-NLS-1$
 					TaskRepositoriesExternalizer.ELEMENT_TASK_REPOSITORIES, rootAttributes);
 
 			for (TaskRepository repository : new ArrayList<TaskRepository>(repositories)) {
+				writeRepository(repository);
+			}
+
+			handler.endElement("", TaskRepositoriesExternalizer.ELEMENT_TASK_REPOSITORIES, //$NON-NLS-1$
+					TaskRepositoriesExternalizer.ELEMENT_TASK_REPOSITORIES);
+		}
 
-				AttributesImpl ieAttributes = new AttributesImpl();
-				for (String key : repository.getProperties().keySet()) {
-					ieAttributes.addAttribute(
-							"", //$NON-NLS-1$
-							key,
-							key,
-							"", //$NON-NLS-1$
-							org.eclipse.mylyn.internal.commons.core.XmlStringConverter.convertToXmlString(repository.getProperties()
-									.get(key)));
+		@SuppressWarnings({ "deprecation", "restriction" })
+		private void writeRepository(TaskRepository repository) throws SAXException {
+			// write properties as attributes to support reading by older versions
+			AttributesImpl repositoryPropertyAttributes = new AttributesImpl();
+			for (String key : repository.getProperties().keySet()) {
+				// avoid emitting XML we cannnot read
+				if (XMLChar.isValidName(key)) {
+					repositoryPropertyAttributes.addAttribute("", //$NON-NLS-1$
+							key, key, "", //$NON-NLS-1$
+							convertToXmlString(repository.getProperties().get(key)));
 				}
+			}
+
+			handler.startElement("", TaskRepositoriesExternalizer.ELEMENT_TASK_REPOSITORY, //$NON-NLS-1$
+					TaskRepositoriesExternalizer.ELEMENT_TASK_REPOSITORY, repositoryPropertyAttributes);
 
-				handler.startElement("", TaskRepositoriesExternalizer.ELEMENT_TASK_REPOSITORY, //$NON-NLS-1$
-						TaskRepositoriesExternalizer.ELEMENT_TASK_REPOSITORY, ieAttributes);
-				handler.endElement("", TaskRepositoriesExternalizer.ELEMENT_TASK_REPOSITORY, //$NON-NLS-1$
-						TaskRepositoriesExternalizer.ELEMENT_TASK_REPOSITORY);
+			// write properties as child nodes to support attributes with special characters in their names
+			for (String key : repository.getProperties().keySet()) {
+				writeProperty(repository, key);
 			}
-			handler.endElement("", TaskRepositoriesExternalizer.ELEMENT_TASK_REPOSITORIES, //$NON-NLS-1$
-					TaskRepositoriesExternalizer.ELEMENT_TASK_REPOSITORIES);
 
-			handler.endDocument();
+			handler.endElement("", TaskRepositoriesExternalizer.ELEMENT_TASK_REPOSITORY, //$NON-NLS-1$
+					TaskRepositoriesExternalizer.ELEMENT_TASK_REPOSITORY);
+		}
+
+		private void writeProperty(TaskRepository repository, String key) throws SAXException {
+			if (!(key.equals(IRepositoryConstants.PROPERTY_CONNECTOR_KIND)
+					|| key.equals(IRepositoryConstants.PROPERTY_URL))) {
+				AttributesImpl propertiesAttributes = new AttributesImpl();
+				addAttribute(propertiesAttributes, TaskRepositoriesExternalizer.PROPERTY_VALUE,
+						repository.getProperties().get(key));
+				addAttribute(propertiesAttributes, TaskRepositoriesExternalizer.PROPERTY_KEY, key);
+
+				handler.startElement("", //$NON-NLS-1$
+						TaskRepositoriesExternalizer.ELEMENT_PROPERTY, TaskRepositoriesExternalizer.ELEMENT_PROPERTY,
+						propertiesAttributes);
+				handler.endElement("", //$NON-NLS-1$
+						TaskRepositoriesExternalizer.ELEMENT_PROPERTY, TaskRepositoriesExternalizer.ELEMENT_PROPERTY);
+			}
 		}
 
 		public void parse(String systemId) throws IOException, SAXException {
 			throw new SAXException("Can only parse writable input sources"); //$NON-NLS-1$
 		}
 
+		private void addAttribute(AttributesImpl attribute, String key, String value) {
+			attribute.addAttribute("", //$NON-NLS-1$
+					key, key, "", //$NON-NLS-1$
+					value);
+		}
+
 	}
 }
diff --git a/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/TaskRepositoriesExternalizer.java b/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/TaskRepositoriesExternalizer.java
index 393aa1e83..655eae07c 100644
--- a/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/TaskRepositoriesExternalizer.java
+++ b/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/TaskRepositoriesExternalizer.java
@@ -44,6 +44,12 @@ public class TaskRepositoriesExternalizer {
 
 	public static final String ATTRIBUTE_VERSION = "OutputVersion"; //$NON-NLS-1$
 
+	public static final String ELEMENT_PROPERTY = "Property"; //$NON-NLS-1$
+
+	public static final String PROPERTY_KEY = "key"; //$NON-NLS-1$
+
+	public static final String PROPERTY_VALUE = "value"; //$NON-NLS-1$
+
 	public void writeRepositoriesToXML(Collection<TaskRepository> repositories, File file) {
 		ZipOutputStream outputStream = null;
 		try {
@@ -117,8 +123,8 @@ public class TaskRepositoriesExternalizer {
 			return contentHandler.getRepositories();
 		} catch (Throwable e) {
 			file.renameTo(new File(file.getAbsolutePath() + "-save")); //$NON-NLS-1$
-			StatusHandler.log(new Status(IStatus.ERROR, ITasksCoreConstants.ID_PLUGIN,
-					"Error reading task repositories", e)); //$NON-NLS-1$
+			StatusHandler.log(
+					new Status(IStatus.ERROR, ITasksCoreConstants.ID_PLUGIN, "Error reading task repositories", e)); //$NON-NLS-1$
 			return null;
 		} finally {
 			if (inputStream != null) {
diff --git a/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/XmlReaderUtil.java b/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/XmlReaderUtil.java
index 482d017fc..1aa8866f5 100644
--- a/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/XmlReaderUtil.java
+++ b/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/XmlReaderUtil.java
@@ -11,36 +11,20 @@
 
 package org.eclipse.mylyn.internal.tasks.core;
 
-import javax.xml.parsers.ParserConfigurationException;
-import javax.xml.parsers.SAXParser;
-import javax.xml.parsers.SAXParserFactory;
-
+import org.apache.xerces.parsers.SAXParser;
 import org.xml.sax.SAXException;
 import org.xml.sax.XMLReader;
 
 /**
  * Utility to create {@link XMLReader} instances. Uses Xerces if available to ensure XML 1.1 parsing works correctly.
- * 
+ *
  * @author Steffen Pingel
  */
 public class XmlReaderUtil {
 
 	public static XMLReader createXmlReader() throws SAXException {
-		try {
-			// use Xerces to ensure XML 1.1 is handled correctly
-			Class<?> clazz = Class.forName("org.apache.xerces.parsers.SAXParser"); //$NON-NLS-1$
-			return (XMLReader) clazz.newInstance();
-		} catch (Throwable e) {
-			SAXParser saxParser;
-			try {
-				SAXParserFactory saxParserFactory = SAXParserFactory.newInstance();
-				saxParserFactory.setNamespaceAware(true);
-				saxParser = saxParserFactory.newSAXParser();
-			} catch (ParserConfigurationException e2) {
-				throw new SAXException(e2);
-			}
-			return saxParser.getXMLReader();
-		}
+		// use Xerces to ensure XML 1.1 is handled correctly
+		return new SAXParser();
 	}
 
 	private XmlReaderUtil() {