Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2167573003:
Verify intent signatures. (Closed)
DescriptionVerify intent signatures.
This patch adds one step to verify the signature in intent
for chrome while the intent has a scheme for the app.
To test, start a website which contains a scheme intent in
chrome, one intent case contains official app signature,
it can start activity in app, and one another intent case
contains fake app signature, chrome won't start activity of
the fake app.
For example, original intent:
intent://platformapi/startapp?appId=20000001&_t=1468848794586#Intent;scheme=alipays;package=com.eg.oandroid.AlipayGphone;end
More secure intent (additional sha256):
intent://platformapi/startapp?appId=20000001&_t=1468848794586#Intent;scheme=alipays;sha256=389B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A;package=com.eg.android.AlipayGphone;end
sha256 can contain more than one signature. Concatenation
by "," as same package name and "|" as intended target app.
For example:
sha256=389B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A,123449F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A|5B9B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF499F
https://developer.chrome.com/multidevice/android/intents
BUG=629713
Patch Set 1 #
Total comments: 7
Patch Set 2 : modify codestyle #
Total comments: 11
Patch Set 3 : fix error on presubmit #Patch Set 4 : edit parsing uri for sha256 and pkgname to using IntentUtils.safeGetStringExtra #Patch Set 5 : fix error on presubmit #
Total comments: 7
Patch Set 6 : refine code for review #
Total comments: 9
Patch Set 7 : try to make code match Chrome style, remove unnessary try catch block, add snippet avoiding ArrayIn… #
Total comments: 8
Patch Set 8 : continue to make the code match Chrome code style and rename some variables #
Total comments: 1
Patch Set 9 : change return to continue in for loop #
Messages
Total messages: 28 (9 generated)
Description was changed from ========== ntent signatures. This patch adds one step to verify the signature in intent for chrome while the intent has a scheme for the app. To test, start a website which contains a scheme intent in chrome, one intent case contains official app signature, it can start activity in app, and one another intent case contains fake app signature, chrome won't start activity of the fake app. For example, original intent: intent://platformapi/startapp?appId=20000001&_t=1468848794586#Intent;scheme=alipays;package=com.eg.oandroid.AlipayGphone;end More secure intent (additional sha256): intent://platformapi/startapp?appId=20000001&_t=1468848794586#Intent;scheme=alipays;sha256=389B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A;package=com.eg.android.AlipayGphone;end And we now extend this feature to support apps contain more than one signature. sha256 param in intent scheme can concat by "," as same package name and "|" as intended target app. such as sha256=389B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A,123449F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A|5B9B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF499F； BUG=629713 D D D D C C C C B B B A BUG=629713 ========== to ========== Verify intent signatures. This patch adds one step to verify the signature in intent for chrome while the intent has a scheme for the app. To test, start a website which contains a scheme intent in chrome, one intent case contains official app signature, it can start activity in app, and one another intent case contains fake app signature, chrome won't start activity of the fake app. For example, original intent: intent://platformapi/startapp?appId=20000001&_t=1468848794586#Intent;scheme=alipays;package=com.eg.oandroid.AlipayGphone;end More secure intent (additional sha256): intent://platformapi/startapp?appId=20000001&_t=1468848794586#Intent;scheme=alipays;sha256=389B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A;package=com.eg.android.AlipayGphone;end And we now extend this feature to support apps contain more than one signature. sha256 param in intent scheme can concat by "," as same package name and "|" as intended target app. such as sha256=389B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A,123449F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A|5B9B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF499F； BUG=629713 ==========
jiajia.lijj@alipay.com changed reviewers: + esprehn@chromium.org, rouslan@chromium.org
Description was changed from ========== Verify intent signatures. This patch adds one step to verify the signature in intent for chrome while the intent has a scheme for the app. To test, start a website which contains a scheme intent in chrome, one intent case contains official app signature, it can start activity in app, and one another intent case contains fake app signature, chrome won't start activity of the fake app. For example, original intent: intent://platformapi/startapp?appId=20000001&_t=1468848794586#Intent;scheme=alipays;package=com.eg.oandroid.AlipayGphone;end More secure intent (additional sha256): intent://platformapi/startapp?appId=20000001&_t=1468848794586#Intent;scheme=alipays;sha256=389B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A;package=com.eg.android.AlipayGphone;end And we now extend this feature to support apps contain more than one signature. sha256 param in intent scheme can concat by "," as same package name and "|" as intended target app. such as sha256=389B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A,123449F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A|5B9B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF499F； BUG=629713 ========== to ========== Verify intent signatures. This patch adds one step to verify the signature in intent for chrome while the intent has a scheme for the app. To test, start a website which contains a scheme intent in chrome, one intent case contains official app signature, it can start activity in app, and one another intent case contains fake app signature, chrome won't start activity of the fake app. For example, original intent: intent://platformapi/startapp?appId=20000001&_t=1468848794586#Intent;scheme=alipays;package=com.eg.oandroid.AlipayGphone;end More secure intent (additional sha256): intent://platformapi/startapp?appId=20000001&_t=1468848794586#Intent;scheme=alipays;sha256=389B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A;package=com.eg.android.AlipayGphone;end sha256 can contain more than one signature. Concatenation by "," as same package name and "|" as intended target app. For example: sha256=389B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A,123449F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A|5B9B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF499F BUG=629713 ==========
palmer@chromium.org changed reviewers: + palmer@chromium.org
https://codereview.chromium.org/2167573003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java (right): https://codereview.chromium.org/2167573003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:127: String[] part = null; Declare this where it's used (line 146). https://codereview.chromium.org/2167573003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:129: String fingerPrint256 = ""; Declare this where it's used (line 148). https://codereview.chromium.org/2167573003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:166: String fingerPrint = ""; Declare this where it's used (line 169). https://codereview.chromium.org/2167573003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:181: //ignore the check sign miss Why? https://codereview.chromium.org/2167573003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:598: private static String computeNormalizedSha256Fingerprint(byte[] signature) { Although this is for Android package signatures, it'd be better to name this method's argument more generally. Also you can make the method name more concise: getSha256(byte[] bytes) https://codereview.chromium.org/2167573003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:614: private static String byteArrayToHexString(byte[] array) { Is there not already a function that does this somewhere in Java, Android, or Chrome? https://codereview.chromium.org/2167573003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:619: for (byte anArray : array) { Use more concise names: for (byte b : bytes)
On 2016/07/20 18:53:48, palmer (OOO through 21 July) wrote: > https://codereview.chromium.org/2167573003/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java > (right): > > https://codereview.chromium.org/2167573003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:127: > String[] part = null; > Declare this where it's used (line 146). > > https://codereview.chromium.org/2167573003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:129: > String fingerPrint256 = ""; > Declare this where it's used (line 148). > > https://codereview.chromium.org/2167573003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:166: > String fingerPrint = ""; > Declare this where it's used (line 169). > > https://codereview.chromium.org/2167573003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:181: > //ignore the check sign miss > Why? > > https://codereview.chromium.org/2167573003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:598: > private static String computeNormalizedSha256Fingerprint(byte[] signature) { > Although this is for Android package signatures, it'd be better to name this > method's argument more generally. Also you can make the method name more > concise: > > getSha256(byte[] bytes) > > https://codereview.chromium.org/2167573003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:614: > private static String byteArrayToHexString(byte[] array) { > Is there not already a function that does this somewhere in Java, Android, or > Chrome? > > https://codereview.chromium.org/2167573003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:619: > for (byte anArray : array) { > Use more concise names: > > for (byte b : bytes) hi, thanks for your comment, I have modifed code as you suggested. expect your review again.
Thank you for the patch. I've added a few stylistic comments and, more importantly, the suggestions to use IntentUtils and add getPackageSHA256Fingerprints() function. This should simplify your code significantly and will be easier to review and get accepted. https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java (right): https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:121: try { A lot of your string parsing can be replaced with IntentUtils.safeGetStringExtra(intent, "sha256") and IntentUtils.safeGetStringExtra(intent, "package"), similar to browserFallbackUrl on line 112. (Place the "sha256" and "package" Strings into static strings at the top of the file, similar to EXTRA_BROWSER_FALLBACK_URL.) https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:149: for (String v : fingerPrint256DifGroups) { Range loops create inefficient iterator objects. Use index loop instead: for (int i = 0; i < ingerPrint256DifGroups.length; i++) { ... } https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:170: } The established pattern is to place more functionality into the delegate. For example, ExternalNavigationDelegateImpl already imports and uses PackageManager. Therefore, you should add a new method in the delegate: getPackageSHA256Fingerprints(pkgName), which returns Set<String>. Then call this method instead of getAssociatedActivityContext(). https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:600: * compute normalized sha256fingerprint from signatures "Compute normalized sha256 fingerprint from signature." Explain what the word "normalized" means here. https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:601: * @param signature The signature to process. https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:602: * @return hexString of the fingerprint @return The hex string for the fingerprint. https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:609: throw new AssertionError("No SHA-256 implementation found."); Throwing assertions is inefficient. Instead, you should return null and check for null in the calling function. https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:616: * convert bytes to String "Convert bytes to a hex string." https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:617: * @param bytes The bytes to convert into a string. https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:618: * @return hexString @return The hex string for the bytes. https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:630: } Place the helper functions immediately below the functions where they are used.
thanks a lot！------------------------------------------------------------------发件人：<ro... 02:32:10收件人：李佳佳(善攻)<jiajia.lijj@alipay.com>; <esprehn@chromium.org>; <palmer@chromium.org>抄 送：<chromium-reviews@chromium.org>主 题：Re: Verify intent signatures. (issue 2167573003 by jiajia.lijj@alipay.com) { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "name": "Open CL", "url": "https://codereview.chromium.org/2167573003/" }, "description": "" } Thank you for the patch. I've added a few stylistic comments and, moreimportantly, the suggestions to use IntentUtils and addgetPackageSHA256Fingerprints() function. This should simplify your codesignificantly and will be easier to review and get accepted.https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.javaFilechrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java(right):https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java#newcode121chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:121:try {A lot of your string parsing can be replaced withIntentUtils.safeGetStringExtra(intent, "sha256") andIntentUtils.safeGetStringExtra(intent, "package"), similar tobrowserFallbackUrl on line 112.(Place the "sha256" and "package" Strings into static strings at the topof the file, similar to EXTRA_BROWSER_FALLBACK_URL.)https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java#newcode149chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:149:for (String v : fingerPrint256DifGroups) {Range loops create inefficient iterator objects. Use index loop instead:for (int i = 0; i < ingerPrint256DifGroups.length; i++) { ...}https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java#newcode170chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:170:}The established pattern is to place more functionality into thedelegate. For example, ExternalNavigationDelegateImpl already importsand uses PackageManager.Therefore, you should add a new method in the delegate:getPackageSHA256Fingerprints(pkgName), which returns Set<String>. Thencall this method instead of getAssociatedActivityContext().https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java#newcode600chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:600:* compute normalized sha256fingerprint from signatures"Compute normalized sha256 fingerprint from signature."Explain what the word "normalized" means here.https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java#newcode601chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:601:*@param signature The signature to process.https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java#newcode602chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:602:* @return hexString of the fingerprint@return The hex string for the fingerprint.https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java#newcode609chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:609:throw new AssertionError("No SHA-256 implementation found.");Throwing assertions is inefficient. Instead, you should return null andcheck for null in the calling function.https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java#newcode616chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:616:* convert bytes to String"Convert bytes to a hex string."https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java#newcode617chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:617:*@param bytes The bytes to convert into a string.https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java#newcode618chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:618:* @return hexString@return The hex string for the bytes.https://codereview.chromium.org/2167573003/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java#newcode630chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:630:}Place the helper functions immediately below the functions where theyare used.https://codereview.chromium.org/2167573003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi, Thank you for your reviewing , I have modified the code as you pointed out. But one thing is that, the function of "IntentUtils.safeGetStringExtra" usage, in my code situation, this method won't work to match my espect , 'cause the params I want to use later is not in mExtraData of intent, so I'm afraid that I have to manually split the intent string to get the params after "#" . Thanks again, looking forward to your review again.
Description was changed from ========== Verify intent signatures. This patch adds one step to verify the signature in intent for chrome while the intent has a scheme for the app. To test, start a website which contains a scheme intent in chrome, one intent case contains official app signature, it can start activity in app, and one another intent case contains fake app signature, chrome won't start activity of the fake app. For example, original intent: intent://platformapi/startapp?appId=20000001&_t=1468848794586#Intent;scheme=alipays;package=com.eg.oandroid.AlipayGphone;end More secure intent (additional sha256): intent://platformapi/startapp?appId=20000001&_t=1468848794586#Intent;scheme=alipays;sha256=389B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A;package=com.eg.android.AlipayGphone;end sha256 can contain more than one signature. Concatenation by "," as same package name and "|" as intended target app. For example: sha256=389B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A,123449F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A|5B9B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF499F BUG=629713 ========== to ========== Verify intent signatures. This patch adds one step to verify the signature in intent for chrome while the intent has a scheme for the app. To test, start a website which contains a scheme intent in chrome, one intent case contains official app signature, it can start activity in app, and one another intent case contains fake app signature, chrome won't start activity of the fake app. For example, original intent: intent://platformapi/startapp?appId=20000001&_t=1468848794586#Intent;scheme=alipays;package=com.eg.oandroid.AlipayGphone;end More secure intent (additional sha256): intent://platformapi/startapp?appId=20000001&_t=1468848794586#Intent;scheme=alipays;sha256=389B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A;package=com.eg.android.AlipayGphone;end sha256 can contain more than one signature. Concatenation by "," as same package name and "|" as intended target app. For example: sha256=389B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A,123449F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF498A|5B9B49F7832F53E9017923220AA85E14DFAA4886ECD7428818BF339543CF499F https://developer.chrome.com/multidevice/android/intents BUG=629713 ==========
On 2016/07/23 06:48:51, jiajia wrote: > Hi, > Thank you for your reviewing , I have modified the code as you pointed out. > But one thing is that, the function of "IntentUtils.safeGetStringExtra" usage, > in my code situation, this method won't work to match my espect , > 'cause the params I want to use later is not in mExtraData of intent, > so I'm afraid that I have to manually split the intent string to get the params > after > "#" . > Thanks again, looking forward to your review again. There's some push-back from Chrome security in the mailing list. So your time at this point is best spent understanding their arguments and coming to common conclusion. Without their approval on the mailing list. This code will be hard to get into Chrome. As for IntentUtils.safeGetStringExtra() not working: perhaps that's because you need to use "S.sha256" in the URL, instead of only "sha256"? I see on https://developer.chrome.com/multidevice/android/intents that "S.browser_fallback_url" is used, but in ExternalNavigationDelegateImpl.java the string is "browser_fallback_url".
tkanks a lot. i will do it latter.------------------------------------------------------------------发件人：... 06:12:12收件人：李佳佳(善攻)<jiajia.lijj@alipay.com>; <esprehn@chromium.org>; <palmer@chromium.org>抄 送：<chromium-reviews@chromium.org>主 题：Re: Verify intent signatures. (issue 2167573003 by jiajia.lijj@alipay.com) { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "name": "Open CL", "url": "https://codereview.chromium.org/2167573003/" }, "description": "" } On 2016/07/23 06:48:51, jiajia wrote:> Hi,> Thank you for your reviewing , I have modified the code as you pointed out.> But one thing is that, the function of "IntentUtils.safeGetStringExtra" usage,> in my code situation, this method won't work to match my espect , > 'cause the params I want to use later is not in mExtraData of intent, > so I'm afraid that I have to manually split the intent string to get theparams> after> "#" .> Thanks again, looking forward to your review again.There's some push-back from Chrome security in the mailing list. So your time atthis point is best spent understanding their arguments and coming to commonconclusion. Without their approval on the mailing list. This code will be hardto get into Chrome.As for IntentUtils.safeGetStringExtra() not working: perhaps that's because youneed to use "S.sha256" in the URL, instead of only "sha256"? I see onhttps://developer.chrome.com/multidevice/android/intents that"S.browser_fallback_url" is used, but in ExternalNavigationDelegateImpl.java thestring is "browser_fallback_url".https://codereview.chromium.org/2167573003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/07/23 22:12:12, rouslan wrote: > On 2016/07/23 06:48:51, jiajia wrote: > > Hi, > > Thank you for your reviewing , I have modified the code as you pointed out. > > But one thing is that, the function of "IntentUtils.safeGetStringExtra" usage, > > in my code situation, this method won't work to match my espect , > > 'cause the params I want to use later is not in mExtraData of intent, > > so I'm afraid that I have to manually split the intent string to get the > params > > after > > "#" . > > Thanks again, looking forward to your review again. > > There's some push-back from Chrome security in the mailing list. So your time at > this point is best spent understanding their arguments and coming to common > conclusion. Without their approval on the mailing list. This code will be hard > to get into Chrome. > > As for IntentUtils.safeGetStringExtra() not working: perhaps that's because you > need to use "S.sha256" in the URL, instead of only "sha256"? I see on > https://developer.chrome.com/multidevice/android/intents that > "S.browser_fallback_url" is used, but in ExternalNavigationDelegateImpl.java the > string is "browser_fallback_url". Thanks. I have edited the code and updated.
On 2016/07/23 22:12:12, rouslan wrote: > On 2016/07/23 06:48:51, jiajia wrote: > > Hi, > > Thank you for your reviewing , I have modified the code as you pointed out. > > But one thing is that, the function of "IntentUtils.safeGetStringExtra" usage, > > in my code situation, this method won't work to match my espect , > > 'cause the params I want to use later is not in mExtraData of intent, > > so I'm afraid that I have to manually split the intent string to get the > params > > after > > "#" . > > Thanks again, looking forward to your review again. > > There's some push-back from Chrome security in the mailing list. So your time at > this point is best spent understanding their arguments and coming to common > conclusion. Without their approval on the mailing list. This code will be hard > to get into Chrome. > > As for IntentUtils.safeGetStringExtra() not working: perhaps that's because you > need to use "S.sha256" in the URL, instead of only "sha256"? I see on > https://developer.chrome.com/multidevice/android/intents that > "S.browser_fallback_url" is used, but in ExternalNavigationDelegateImpl.java the > string is "browser_fallback_url". Hi Rouslan, I edited the code again, I'm afraid I have to reset back to last commit, because using IntentUtils.safeGetStringExtra needs the format of scheme url . But new format by turning sha256 into S.sha256,and turning package into S.package wouldn't work on other browser. So would it be OK to split url string to look up for sha256 ? Thanks for your understanding.
In securty sensitive code like this, it's important to (1) be cautious with your input data and (2) be clear in what your code does. My comments should start helping with these aspects. More changes may be necessary still. In the meantime, you should make sure that people on the mailing list agree with your proposed change. It was not clear to me that any of them agreed to include this change yet. https://codereview.chromium.org/2167573003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java (right): https://codereview.chromium.org/2167573003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:118: try { Put this block into a separate function, for example: if (!packageSignatureMatches()) { return OverrideLoadingResult.NO_OVERRIDE; } https://codereview.chromium.org/2167573003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:120: String scheme = intent.getData().getScheme(); You don't use "scheme" anywhere except to check that it's present. Add a comment about its purpose. https://codereview.chromium.org/2167573003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:122: if (!TextUtils.isEmpty(scheme) && null != fragment && fragment.contains(";")) { Chrome style is to put null after variable: fragment != null https://codereview.chromium.org/2167573003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:138: */ Chrome style is to use // style comments when inside of a function. https://codereview.chromium.org/2167573003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:143: String[] part = each.split("="); Integer loops are more efficient on Android. for (int i - 0; i < parts.length; i++) { ... } https://codereview.chromium.org/2167573003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:144: if (part[0].equals("sha256")) { This will crash if "each" does not contain "=". https://codereview.chromium.org/2167573003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:145: fingerPrint256 = part[1]; This will crash if "=" is the last character of "each".
The CQ bit was checked by jiajia.lijj@alipay.com
The CQ bit was unchecked by jiajia.lijj@alipay.com
On 2016/07/27 08:29:02, rouslan wrote: > In securty sensitive code like this, it's important to (1) be cautious with your > input data and (2) be clear in what your code does. My comments should start > helping with these aspects. More changes may be necessary still. > > In the meantime, you should make sure that people on the mailing list agree with > your proposed change. It was not clear to me that any of them agreed to include > this change yet. > > https://codereview.chromium.org/2167573003/diff/80001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java > (right): > > https://codereview.chromium.org/2167573003/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:118: > try { > Put this block into a separate function, for example: > > if (!packageSignatureMatches()) { > return OverrideLoadingResult.NO_OVERRIDE; > } > > https://codereview.chromium.org/2167573003/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:120: > String scheme = intent.getData().getScheme(); > You don't use "scheme" anywhere except to check that it's present. Add a comment > about its purpose. > > https://codereview.chromium.org/2167573003/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:122: > if (!TextUtils.isEmpty(scheme) && null != fragment && fragment.contains(";")) { > Chrome style is to put null after variable: > > fragment != null > > https://codereview.chromium.org/2167573003/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:138: > */ > Chrome style is to use // style comments when inside of a function. > > https://codereview.chromium.org/2167573003/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:143: > String[] part = each.split("="); > Integer loops are more efficient on Android. > > for (int i - 0; i < parts.length; i++) { > ... > } > > https://codereview.chromium.org/2167573003/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:144: > if (part[0].equals("sha256")) { > This will crash if "each" does not contain "=". > > https://codereview.chromium.org/2167573003/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:145: > fingerPrint256 = part[1]; > This will crash if "=" is the last character of "each". Thanks so much for your advice. I have edited most code, but as for two potential crashes, the method "splt" string will not lead crash. And I tested for specific test cases, there is no crash too. So I keep it the same as before. Would it be OK?
On 2016/07/27 13:00:44, jiajia wrote: > And I tested for specific test cases, there is no crash too. > So I keep it the same as before. > Would it be OK? In general, Chrome tries to avoid using exceptions in production code, because exceptions are slow. Below are several comments aimed to follow this rule. Now you will find that crashes do indeed happen in your code, but you can prevent them by checking the length of the arrays before using them. https://codereview.chromium.org/2167573003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegate.java (right): https://codereview.chromium.org/2167573003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegate.java:27: * Get all sha256 fingerprints of signature from pkgName. Get all sha256 fingerprints of signature from pkgName or null on failure. https://codereview.chromium.org/2167573003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegate.java:30: throws PackageManager.NameNotFoundException; Don't throw exceptions. https://codereview.chromium.org/2167573003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java (right): https://codereview.chromium.org/2167573003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java:557: PackageManager.GET_SIGNATURES).signatures; Surround lines 556-557 with try-catch block for NameNotFoundException. Return null if the exception is thrown. https://codereview.chromium.org/2167573003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java:561: String fingerPrint = getSha256(each.toByteArray()); if (fingerPrint == null) return null; Use null to indicate failure. https://codereview.chromium.org/2167573003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java:572: * @return The hex string for the fingerprint. @return The hex string for the fingerprint or null on failure. https://codereview.chromium.org/2167573003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java:579: return ""; return null; Use null to indicate failure. https://codereview.chromium.org/2167573003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java (right): https://codereview.chromium.org/2167573003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:148: //This new feature is based on custom scheme, string "scheme" used to check whether 1) "//" should always be followed by " " before the actual comment begins. See Line 100 for example. 2) Remove the word "new". This feature will be old soon enough. https://codereview.chromium.org/2167573003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:189: } catch (PackageManager.NameNotFoundException e) { Modify getPackageSHA256FingerPrints() to return null on failure instead of throwing an exception. Then check fingerPrint256Set for null before using it. https://codereview.chromium.org/2167573003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:191: } catch (Throwable ignore) { Don't surround large blocks of code with try-catch blocks, especially with such a generic exception type as Throwable. This will easily hide bugs in your code.
Thanks, I have updated the code as you commented, removed try catch block and etc.
https://codereview.chromium.org/2167573003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegate.java (left): https://codereview.chromium.org/2167573003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegate.java:6: Don't remove this newline. https://codereview.chromium.org/2167573003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java (right): https://codereview.chromium.org/2167573003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java:557: PackageManager.GET_SIGNATURES).signatures; Move the "catch" to this line. This is the only method that throws NameNotFoundException. https://codereview.chromium.org/2167573003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java:558: HashSet<String> fingerPrint256Set = new HashSet<String>(); Set<String> fingerprints = new HashSet<>(); https://codereview.chromium.org/2167573003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java:559: if (signatures.length > 0) { No need to check the length of "signatures" here. If it's empty, then the loop will not do anything anyway. https://codereview.chromium.org/2167573003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java:560: for (Signature each : signatures) { Range loops are inefficient. Use integer loops instead. for (int i = 0; i < signatures.length; i++) { Signature each = signatures[i]; ... } https://codereview.chromium.org/2167573003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java (right): https://codereview.chromium.org/2167573003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:144: */ /** * Check if the signature specified in the intent matches the signatures of * the package to launch. * * @param intent The intent to launch. * @return True if OK to launch the intent based on the signature. */ https://codereview.chromium.org/2167573003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:147: //the intent url has an extra custom scheme, if not, the feature won't work. Add a space after "//". https://codereview.chromium.org/2167573003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:160: //or "...;sha256=sha256_1,sha256_2|sha256_3;..." Add a space after "//".
Thanks so much for your time and help. I have updated the code and done some mofifying work.
rouslan@chromium.org changed reviewers: + tedchoc@chromium.org
palmer, esprehn, tedchoc: ptal. https://codereview.chromium.org/2167573003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java (right): https://codereview.chromium.org/2167573003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:165: return false; This should be "continue;" instead, because it does not tell you anything about signatures, so you should skip it.
tavatchaina444@gmail.com changed reviewers: + tavatchaina444@gmail.com
OK. Done.
lgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
