From 7324353d7450ddd534e1b8a1388639fba0473919 Mon Sep 17 00:00:00 2001 From: Sylvia van Os Date: Sat, 10 Jul 2021 18:33:54 +0200 Subject: [PATCH] Use location hash instead of query parameters in share URL for increased privacy --- CHANGELOG.md | 4 + .../protect/card_locker/ImportURIHelper.java | 85 ++++++++++++++----- .../card_locker/LoyaltyCardViewActivity.java | 8 +- .../protect/card_locker/MainActivity.java | 8 +- app/src/main/res/values/strings.xml | 1 + .../protect/card_locker/ImportURITest.java | 70 ++++++++------- 6 files changed, 122 insertions(+), 54 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d1e131181..a2ee07bc2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +Breaking changes: +- The backup format changed, see https://github.com/TheLastProject/Catima/wiki/Export-format +- The URL sharing format changed, see https://github.com/TheLastProject/Catima/wiki/Card-sharing-URL-format + Changes: - Add UPC-E support diff --git a/app/src/main/java/protect/card_locker/ImportURIHelper.java b/app/src/main/java/protect/card_locker/ImportURIHelper.java index 02a6328df..ff2d3fd9f 100644 --- a/app/src/main/java/protect/card_locker/ImportURIHelper.java +++ b/app/src/main/java/protect/card_locker/ImportURIHelper.java @@ -7,9 +7,14 @@ import android.net.Uri; import com.google.zxing.BarcodeFormat; import java.io.InvalidObjectException; +import java.io.UnsupportedEncodingException; import java.math.BigDecimal; +import java.net.URLDecoder; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; import java.util.Currency; import java.util.Date; +import java.util.HashMap; import java.util.List; public class ImportURIHelper { @@ -58,78 +63,114 @@ public class ImportURIHelper { Currency balanceType = null; Integer headerColor = null; - String store = uri.getQueryParameter(STORE); - String note = uri.getQueryParameter(NOTE); - String cardId = uri.getQueryParameter(CARD_ID); - String barcodeId = uri.getQueryParameter(BARCODE_ID); - if (store == null || note == null || cardId == null) throw new InvalidObjectException("Not a valid import URI"); + // Store everything in a simple key/value hashmap + HashMap kv = new HashMap<>(); - String unparsedBarcodeType = uri.getQueryParameter(BARCODE_TYPE); + // First, grab all query parameters (backwards compatibility) + for (String key : uri.getQueryParameterNames()) { + kv.put(key, uri.getQueryParameter(key)); + } + + // Then, parse the new and more private fragment part + // Overriding old format entries if they exist + String fragment = uri.getFragment(); + if (fragment != null) { + for (String fragmentPart : fragment.split("&")) { + String[] fragmentData = fragmentPart.split("=", 2); + kv.put(fragmentData[0], URLDecoder.decode(fragmentData[1], StandardCharsets.UTF_8.toString())); + } + } + + // Then use all values we care about + String store = kv.get(STORE); + String note = kv.get(NOTE); + String cardId = kv.get(CARD_ID); + String barcodeId = kv.get(BARCODE_ID); + if (store == null || note == null || cardId == null) throw new InvalidObjectException("Not a valid import URI: " + uri.toString()); + + String unparsedBarcodeType = kv.get(BARCODE_TYPE); if(unparsedBarcodeType != null && !unparsedBarcodeType.equals("")) { barcodeType = BarcodeFormat.valueOf(unparsedBarcodeType); } - String unparsedBalance = uri.getQueryParameter(BALANCE); + String unparsedBalance = kv.get(BALANCE); if(unparsedBalance != null && !unparsedBalance.equals("")) { balance = new BigDecimal(unparsedBalance); } - String unparsedBalanceType = uri.getQueryParameter(BALANCE_TYPE); + String unparsedBalanceType = kv.get(BALANCE_TYPE); if (unparsedBalanceType != null && !unparsedBalanceType.equals("")) { balanceType = Currency.getInstance(unparsedBalanceType); } - String unparsedExpiry = uri.getQueryParameter(EXPIRY); + String unparsedExpiry = kv.get(EXPIRY); if(unparsedExpiry != null && !unparsedExpiry.equals("")) { expiry = new Date(Long.parseLong(unparsedExpiry)); } - String unparsedHeaderColor = uri.getQueryParameter(HEADER_COLOR); + String unparsedHeaderColor = kv.get(HEADER_COLOR); if(unparsedHeaderColor != null) { headerColor = Integer.parseInt(unparsedHeaderColor); } return new LoyaltyCard(-1, store, note, expiry, balance, balanceType, cardId, barcodeId, barcodeType, headerColor, 0); - } catch (NullPointerException | NumberFormatException ex) { + } catch (NullPointerException | NumberFormatException | UnsupportedEncodingException ex) { throw new InvalidObjectException("Not a valid import URI"); } } + private StringBuilder appendFragment(StringBuilder fragment, String key, String value) throws UnsupportedEncodingException { + if (fragment.length() > 0) { + fragment.append("&"); + } + + // Double-encode the value to make sure it can't accidentally contain symbols that'll break the parser + fragment.append(key).append("=").append(URLEncoder.encode(value, StandardCharsets.UTF_8.toString())); + + return fragment; + } + // Protected for usage in tests - protected Uri toUri(LoyaltyCard loyaltyCard) { + protected Uri toUri(LoyaltyCard loyaltyCard) throws UnsupportedEncodingException { Uri.Builder uriBuilder = new Uri.Builder(); uriBuilder.scheme("https"); uriBuilder.authority(host); uriBuilder.path(path); - uriBuilder.appendQueryParameter(STORE, loyaltyCard.store); - uriBuilder.appendQueryParameter(NOTE, loyaltyCard.note); - uriBuilder.appendQueryParameter(BALANCE, loyaltyCard.balance.toString()); + + // Use fragment instead of QueryParameter to not leak this data to the server + StringBuilder fragment = new StringBuilder(); + + fragment = appendFragment(fragment, STORE, loyaltyCard.store); + fragment = appendFragment(fragment, NOTE, loyaltyCard.note); + fragment = appendFragment(fragment, BALANCE, loyaltyCard.balance.toString()); if (loyaltyCard.balanceType != null) { - uriBuilder.appendQueryParameter(BALANCE_TYPE, loyaltyCard.balanceType.getCurrencyCode()); + fragment = appendFragment(fragment, BALANCE_TYPE, loyaltyCard.balanceType.getCurrencyCode()); } if (loyaltyCard.expiry != null) { - uriBuilder.appendQueryParameter(EXPIRY, String.valueOf(loyaltyCard.expiry.getTime())); + fragment = appendFragment(fragment, EXPIRY, String.valueOf(loyaltyCard.expiry.getTime())); } - uriBuilder.appendQueryParameter(CARD_ID, loyaltyCard.cardId); + fragment = appendFragment(fragment, CARD_ID, loyaltyCard.cardId); if(loyaltyCard.barcodeId != null) { - uriBuilder.appendQueryParameter(BARCODE_ID, loyaltyCard.barcodeId); + fragment = appendFragment(fragment, BARCODE_ID, loyaltyCard.barcodeId); } if(loyaltyCard.barcodeType != null) { - uriBuilder.appendQueryParameter(BARCODE_TYPE, loyaltyCard.barcodeType.toString()); + fragment = appendFragment(fragment, BARCODE_TYPE, loyaltyCard.barcodeType.toString()); } if(loyaltyCard.headerColor != null) { - uriBuilder.appendQueryParameter(HEADER_COLOR, loyaltyCard.headerColor.toString()); + fragment = appendFragment(fragment, HEADER_COLOR, loyaltyCard.headerColor.toString()); } // Star status will not be exported // Front and back pictures are often too big to fit into a message in base64 nicely, not sharing either... + + uriBuilder.fragment(fragment.toString()); return uriBuilder.build(); } - public void startShareIntent(List loyaltyCards) { + public void startShareIntent(List loyaltyCards) throws UnsupportedEncodingException { int loyaltyCardCount = loyaltyCards.size(); StringBuilder text = new StringBuilder(); diff --git a/app/src/main/java/protect/card_locker/LoyaltyCardViewActivity.java b/app/src/main/java/protect/card_locker/LoyaltyCardViewActivity.java index 51b002cda..6e9438e76 100644 --- a/app/src/main/java/protect/card_locker/LoyaltyCardViewActivity.java +++ b/app/src/main/java/protect/card_locker/LoyaltyCardViewActivity.java @@ -29,6 +29,7 @@ import com.google.android.material.bottomsheet.BottomSheetBehavior; import com.google.android.material.floatingactionbutton.FloatingActionButton; import com.google.zxing.BarcodeFormat; +import java.io.UnsupportedEncodingException; import java.math.BigDecimal; import java.text.DateFormat; import java.util.Arrays; @@ -562,7 +563,12 @@ public class LoyaltyCardViewActivity extends AppCompatActivity break; case R.id.action_share: - importURIHelper.startShareIntent(Arrays.asList(loyaltyCard)); + try { + importURIHelper.startShareIntent(Arrays.asList(loyaltyCard)); + } catch (UnsupportedEncodingException e) { + Toast.makeText(LoyaltyCardViewActivity.this, R.string.failedGeneratingShareURL, Toast.LENGTH_LONG).show(); + e.printStackTrace(); + } return true; case R.id.action_lock_unlock: diff --git a/app/src/main/java/protect/card_locker/MainActivity.java b/app/src/main/java/protect/card_locker/MainActivity.java index 0b1f43990..5b1547627 100644 --- a/app/src/main/java/protect/card_locker/MainActivity.java +++ b/app/src/main/java/protect/card_locker/MainActivity.java @@ -22,6 +22,7 @@ import android.widget.Toast; import com.google.android.material.floatingactionbutton.FloatingActionButton; import com.google.android.material.tabs.TabLayout; +import java.io.UnsupportedEncodingException; import java.util.List; import androidx.appcompat.app.AppCompatActivity; @@ -97,7 +98,12 @@ public class MainActivity extends AppCompatActivity implements LoyaltyCardCursor else if (inputItem.getItemId() == R.id.action_share) { final ImportURIHelper importURIHelper = new ImportURIHelper(MainActivity.this); - importURIHelper.startShareIntent(mAdapter.getSelectedItems()); + try { + importURIHelper.startShareIntent(mAdapter.getSelectedItems()); + } catch (UnsupportedEncodingException e) { + Toast.makeText(MainActivity.this, R.string.failedGeneratingShareURL, Toast.LENGTH_LONG).show(); + e.printStackTrace(); + } inputMode.finish(); return true; } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 2822560ab..7d9749529 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -192,4 +192,5 @@ Yes No Please enter the password + Failed generating share URL. Please report this bug! diff --git a/app/src/test/java/protect/card_locker/ImportURITest.java b/app/src/test/java/protect/card_locker/ImportURITest.java index 7957f89ce..a2b6b75d9 100644 --- a/app/src/test/java/protect/card_locker/ImportURITest.java +++ b/app/src/test/java/protect/card_locker/ImportURITest.java @@ -14,6 +14,7 @@ import org.robolectric.RobolectricTestRunner; import org.robolectric.annotation.Config; import java.io.InvalidObjectException; +import java.io.UnsupportedEncodingException; import java.math.BigDecimal; import java.util.Currency; import java.util.Date; @@ -37,12 +38,11 @@ public class ImportURITest { } @Test - public void ensureNoDataLoss() throws InvalidObjectException - { + public void ensureNoDataLoss() throws InvalidObjectException, UnsupportedEncodingException { // Generate card Date date = new Date(); - db.insertLoyaltyCard("store", "note", date, new BigDecimal("100"), null, BarcodeFormat.UPC_E.toString(), BarcodeFormat.UPC_A.toString(), BarcodeFormat.QR_CODE, Color.BLACK, 1); + db.insertLoyaltyCard("store", "This note contains evil symbols like & and = that will break the parser if not escaped right $#!%()*+;:รก", date, new BigDecimal("100"), null, BarcodeFormat.UPC_E.toString(), BarcodeFormat.UPC_A.toString(), BarcodeFormat.QR_CODE, Color.BLACK, 1); // Get card LoyaltyCard card = db.getLoyaltyCard(1); @@ -68,8 +68,7 @@ public class ImportURITest { } @Test - public void ensureNoCrashOnMissingHeaderFields() throws InvalidObjectException - { + public void ensureNoCrashOnMissingHeaderFields() throws InvalidObjectException, UnsupportedEncodingException { // Generate card db.insertLoyaltyCard("store", "note", null, new BigDecimal("10.00"), Currency.getInstance("EUR"), BarcodeFormat.UPC_A.toString(), null, BarcodeFormat.QR_CODE, null, 0); @@ -110,35 +109,46 @@ public class ImportURITest { @Test public void failToParseBadData() { - try { - //"stare" instead of store - importURIHelper.parse(Uri.parse("https://brarcher.github.io/loyalty-card-locker/share?stare=store¬e=note&cardid=12345&barcodetype=ITF&headercolor=-416706")); - assertTrue(false); // Shouldn't get here - } catch(InvalidObjectException ex) { - // Desired behaviour + String[] urls = new String[2]; + urls[0] = "https://brarcher.github.io/loyalty-card-locker/share?stare=store¬e=note&cardid=12345&barcodetype=ITF&headercolor=-416706"; + urls[1] = "https://thelastproject.github.io/Catima/share#stare%3Dstore%26note%3Dnote%26balance%3D0%26cardid%3D12345%26barcodetype%3DITF%26headercolor%3D-416706"; + + for (String url : urls) { + try { + //"stare" instead of store + importURIHelper.parse(Uri.parse(url)); + assertTrue(false); // Shouldn't get here + } catch (InvalidObjectException ex) { + // Desired behaviour + } } } @Test - public void parseAdditionalUnforeseenData() - { - LoyaltyCard parsedCard = null; - try { - parsedCard = importURIHelper.parse(Uri.parse("https://brarcher.github.io/loyalty-card-locker/share?store=store¬e=note&cardid=12345&barcodetype=ITF&headercolor=-416706&headertextcolor=-1¬foreseen=no")); - } catch (InvalidObjectException e) { - e.printStackTrace(); - } + public void parseAdditionalUnforeseenData() { + String[] urls = new String[2]; + urls[0] = "https://brarcher.github.io/loyalty-card-locker/share?store=store¬e=note&cardid=12345&barcodetype=ITF&headercolor=-416706&headertextcolor=-1¬foreseen=no"; + urls[1] = "https://thelastproject.github.io/Catima/share#store%3Dstore%26note%3Dnote%26balance%3D0%26cardid%3D12345%26barcodetype%3DITF%26headercolor%3D-416706%26notforeseen%3Dno"; - // Compare everything - assertEquals("store", parsedCard.store); - assertEquals("note", parsedCard.note); - assertEquals(null, parsedCard.expiry); - assertEquals(new BigDecimal("0"), parsedCard.balance); - assertEquals(null, parsedCard.balanceType); - assertEquals("12345", parsedCard.cardId); - assertEquals(null, parsedCard.barcodeId); - assertEquals(BarcodeFormat.ITF, parsedCard.barcodeType); - assertEquals(Integer.valueOf(-416706), parsedCard.headerColor); - assertEquals(0, parsedCard.starStatus); + for (String url : urls) { + LoyaltyCard parsedCard = null; + try { + parsedCard = importURIHelper.parse(Uri.parse(url)); + } catch (InvalidObjectException e) { + e.printStackTrace(); + } + + // Compare everything + assertEquals("store", parsedCard.store); + assertEquals("note", parsedCard.note); + assertEquals(null, parsedCard.expiry); + assertEquals(new BigDecimal("0"), parsedCard.balance); + assertEquals(null, parsedCard.balanceType); + assertEquals("12345", parsedCard.cardId); + assertEquals(null, parsedCard.barcodeId); + assertEquals(BarcodeFormat.ITF, parsedCard.barcodeType); + assertEquals(Integer.valueOf(-416706), parsedCard.headerColor); + assertEquals(0, parsedCard.starStatus); + } } }