Desktop: Fixes #9985: Filter Sync Target Info Logs #10014

criticic · 2024-02-27T11:29:05Z

Fixes #9985 .

Removes the Content & Checksum parameters from master keys
Truncates public key to first 40 characters
Truncates provate key to this format: "first20chars...last20chars"
Added testing to check this behaviour

github-actions · 2024-02-27T11:29:21Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

criticic · 2024-02-27T11:30:16Z

I have read the CLA Document and I hereby sign the CLA

criticic · 2024-02-27T12:10:18Z

The test fails due to a issue with cspell in the unit test, should I just disable it for this test?

criticic · 2024-02-27T12:45:29Z

@PackElend label me please

Introduction URL: https://discourse.joplinapp.org/t/introducing-criticic/36240

packages/lib/services/synchronizer/syncInfoUtils.test.ts

laurent22 · 2024-02-27T16:07:28Z

packages/lib/services/synchronizer/syncInfoUtils.ts

+				if (mk.content) delete mk.content;
+				if (mk.checksum) delete mk.checksum;


Why this check?

I remember some tests failing due to this, though they run fine now without it also. Not sure what caused the issue then.

Ok because I think with real data mk.checksum is an empty string so you wouldn't delete it in this case. And this check is unnecessary anyway

laurent22 · 2024-02-27T16:27:09Z

packages/lib/services/synchronizer/syncInfoUtils.test.ts

+			ppk_: {
+				value: {
+					// Public Key is truncated to 40 characters
+					publicKey: 'longstringverylongstringlongstringverylo',


Would you mind adding "..." to this string too to show it's truncated?

And please remove all the comments. They are not necessary and are likely to become outdated.

laurent22 · 2024-02-27T16:28:32Z

packages/lib/services/synchronizer/syncInfoUtils.test.ts

+			activeMasterKeyId_: originalSyncInfo.activeMasterKeyId,
+			appMinVersion_: originalSyncInfo.appMinVersion,
+			e2ee_: originalSyncInfo.e2ee,
+			version_: originalSyncInfo.version,


When you create a test you should not use dynamic data, especially not data that comes from the data you're supposed to check. So all these should be static strings/numbers.

laurent22 · 2024-02-27T16:28:47Z

packages/lib/services/synchronizer/syncInfoUtils.test.ts

+					createdTime: 0,
+					keySize: 0,
+				},
+				updatedTime: originalSyncInfo.ppk.updatedTime,


laurent22 · 2024-02-27T16:29:31Z

packages/lib/services/synchronizer/syncInfoUtils.test.ts

+			createdTime: 0,
+			keySize: 0,


Could you try the app and use more realistic data? Checking that the time or size is 0 is not a good test - it might be 0 because of a bug.

laurent22 · 2024-02-27T16:33:08Z

Please use this as test data:

{
	"version": 3,
	"e2ee": {
		"value": true,
		"updatedTime": 0
	},
	"activeMasterKeyId": {
		"value": "400227d2222c4d3bb7346514861c643b",
		"updatedTime": 0
	},
	"masterKeys": [
		{
			"id": "400227d8a77c4d3bb7346514861c643b",
			"created_time": 1515008161362,
			"updated_time": 1708103706234,
			"source_application": "net.cozic.joplin-desktop",
			"encryption_method": 4,
			"checksum": "",
			"content": "{\"iv\":\"M1uezlW1Pu1g3dwrCTqcHg==\",\"v\":1,\"iter\":10000,\"ks\":256,\"ts\":64,\"mode\":\"ccm\",\"adata\":\"\",\"cipher\":\"aes\",\"salt\":\"0dqWvU/PUVQ=\",\"ct\":\"wHXN5pk1s7qKX+2Y9puEGZGkojI1Pvc+TvZUKC6QCfwxtMK6C1Hmgvm53vAaeCMcCXPvGVLo9JwqINFhEgb0ux+KUFcCqgT1pNO2Sf/hJsH8PjaUvl0kwpC511zdnvY7Hk3WIpgXVKUevsQt9TkMK5e8y1JMsuuTD3fW7bEiv/ehe4CBSQ9eH1tWjr1qQ==\"}",
			"hasBeenUsed": true
		}
	],
	"ppk": {
		"value": {
			"id": "SNQ5ZCs61KDVUW2qqqqHd3",
			"keySize": 2048,
			"privateKey": {
				"encryptionMethod": 4,
				"ciphertext": "{\"iv\":\"Z2y11b4nCYvpmQ9gvxELug==\",\"v\":1,\"iter\":10000,\"ks\":256,\"ts\":64,\"mode\":\"ccm\",\"adata\":\"\",\"cipher\":\"aes\",\"salt\":\"0dqWvU/PUVQ=\",\"ct\":\"8CvjYayXMpLsrAMwtu18liRfewKfZVpRlC0D0I2FYziyFhRf4Cjqi2+Uy8kIC8au7oBSBUnNU6jd04ooNozneKv2MzkhbGlXo3izxqCMVHboqa2vkPWbBAxGlvUYQUg213xG61FjZ19ZJdpti+AQy7qpQU7/h5kyC0iJ2aXG5TIGcBuDq3lbGAVfG/RlL/abMKLYb8KouFYAJe+0bUajUXa1KJsey+eD+2hFVc+nAxKOLe1UoZysB77Lq43DRTBFGH2gTSC1zOXxuZeSbFPPN0Cj+FvV7D5pF9LhUSLPDsIiRwF/q+E506YgDjirSZAvW1Y2EEM22F2Mh0I0pbVPFXhhBafqPLRwXmUGULCnA64gkGwctK5mEs985VVSrpQ0nMvf/drg2vUQrJ3exgl43ddVSOCjeJuF7F06IBL5FQ34iAujsOheRNvlWtG9xm008Vc19NxvhtzIl1RO7XLXrrTBzbFHDrcHjda/xNWNEKwU/LZrH0xPgwEcwBmLItvy/NojI/JKNeck8R431QWooFb7cTplO4qsgCQNL9MJ9avpmNSXJAUQx8VnifKVbzcY4T7X7TmJWSrpvBWV8MLfi3TOF4kahR75vg47kCrMbthFMw5bvrjvMmGOtyKxheqbS5IlSnSSz5x7wIVz0g3vzMbcbb5rF5MuzNhU97wNiz3L1Aonjmnu8r3vCyXTB/4GSiwYH7KfixwYM68T4crqJ0VneNy+owznCdJQXnG4cmjxek1wmJMEmurQ1JtANNv/m43gzoqd62V6Dq05vLJF+n7CS9HgJ3FTqYVCZLGGYrSilIYnEjhdaBpkcnFrCitbfYj+IpNC6eN6qg2hpGAbmKId7RLOGwJyda0jkuNP9mTqWOF+6eYn8Q+Y3YIY\"}"
			},
			"publicKey": "-----BEGIN RSA PUBLIC KEY-----\nMIIBCgKCAQEAiSTY5wBscae/WmU3PfVP5FYQiuTi5V7BjPcge/6pXvgF3zwe43uy\nTWdzO2YgK/a8f3H507clcGlZN4e0e1jZ/rh4lMfaN\nugfNo0RAvuwn8Yniqfb69reygJywbFBIauxbBpVKbc21MLuCbPkVFjKG7qGNYdF4\nc17mQ8nQsbFPZcuvxsZvgvvbza1q0rqVETdDUClyIrY8plAjMgTKCRwq2gafP6eX\nWpkENAyIbOFxSKXjWy0yFidvZfYLz4mIRwIDAQAB\n-----END RSA PUBLIC KEY-----",
			"createdTime": 1633274368892
		},
		"updatedTime": 1633274368892
	},
	"appMinVersion": "0.0.0"
}

But really you should have looked for this data yourself or asked around. Testing with fake data with 0 values and empty strings is not something we can rely on.

criticic · 2024-02-27T16:40:49Z

In this case I had first made it all static data (from the running Joplin App), but then decided to write it this way. (seemed cleaner to my brain) but yeah I agree it might not be suited for a test

criticic · 2024-02-27T17:07:45Z

I think now this is in line with the feedback you provided

packages/lib/services/synchronizer/syncInfoUtils.test.ts

laurent22 · 2024-02-27T17:13:00Z

packages/lib/services/synchronizer/syncInfoUtils.ts

+		// Truncate the private key and public key
+		if (filtered.ppk_.value) {
+			filtered.ppk_.value.privateKey.ciphertext = `${filtered.ppk_.value.privateKey.ciphertext.substr(0, 20)}...${filtered.ppk_.value.privateKey.ciphertext.substr(-20)}`;
+			filtered.ppk_.value.publicKey = `${filtered.ppk_.value.publicKey.substr(0, 20)}...${filtered.ppk_.value.publicKey.substr(-20)}`;


Not what was asked. The point of including the first 40 characters is to show the start of the public key data. With your change all we get is "-----BEGIN RSA PUBLI... RSA PUBLIC KEY-----" which is useless.

sorry for the misunderstanding, have corrected it

Desktop: Fixes laurent22#9985: Filter Sync Target Info Logs

054b988

github-actions bot added a commit that referenced this pull request Feb 27, 2024

@criticic has signed the CLA in #10014

f254255

criticic closed this Feb 27, 2024

criticic reopened this Feb 27, 2024

Only Filter if values are present

b319fdd

Disable Spellcheck for test

0e79f4f

laurent22 reviewed Feb 27, 2024

View reviewed changes

packages/lib/services/synchronizer/syncInfoUtils.test.ts Outdated Show resolved Hide resolved

Compare with the complete object

441b85e

laurent22 reviewed Feb 27, 2024

View reviewed changes

Remove unnecessary check

612b705

laurent22 reviewed Feb 27, 2024

View reviewed changes

Update publicKey truncation & change test to use static data

320a8cc

laurent22 reviewed Feb 27, 2024

View reviewed changes

Make the filter consistent

32d6dfa

PackElend added the gsoc-2024 label Feb 28, 2024

laurent22 merged commit 9a2a251 into laurent22:dev Mar 2, 2024
10 checks passed

darcy168 mentioned this pull request May 13, 2024

[Snyk] Fix for 2 vulnerabilities darcy168/joplin#195

Open

datatheorem-phillip mentioned this pull request May 13, 2024

[Snyk] Fix for 2 vulnerabilities datatheorem-phillip/joplin#202

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Desktop: Fixes #9985: Filter Sync Target Info Logs #10014

Desktop: Fixes #9985: Filter Sync Target Info Logs #10014

criticic commented Feb 27, 2024

github-actions bot commented Feb 27, 2024 •

edited

Loading

criticic commented Feb 27, 2024

criticic commented Feb 27, 2024

criticic commented Feb 27, 2024

laurent22 Feb 27, 2024

criticic Feb 27, 2024

laurent22 Feb 27, 2024

laurent22 Feb 27, 2024

laurent22 Feb 27, 2024

laurent22 Feb 27, 2024

laurent22 Feb 27, 2024

laurent22 Feb 27, 2024

laurent22 commented Feb 27, 2024

criticic commented Feb 27, 2024

criticic commented Feb 27, 2024

laurent22 Feb 27, 2024

criticic Feb 27, 2024

		if (mk.content) delete mk.content;
		if (mk.checksum) delete mk.checksum;

Desktop: Fixes #9985: Filter Sync Target Info Logs #10014

Desktop: Fixes #9985: Filter Sync Target Info Logs #10014

Conversation

criticic commented Feb 27, 2024

github-actions bot commented Feb 27, 2024 • edited Loading

criticic commented Feb 27, 2024

criticic commented Feb 27, 2024

criticic commented Feb 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurent22 commented Feb 27, 2024

criticic commented Feb 27, 2024

criticic commented Feb 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 27, 2024 •

edited

Loading