diff options
author | Kristóf Marussy <kristof@marussy.com> | 2022-04-26 16:55:57 +0200 |
---|---|---|
committer | Kristóf Marussy <kristof@marussy.com> | 2022-05-16 00:55:02 +0200 |
commit | b971de0717a66ae6085670fe5d3a469e236a9446 (patch) | |
tree | c64352908648512bad713fc1e91dd5f07a66968c /packages/main/src/reactions | |
parent | refactor: remove json5 dependency (diff) | |
download | sophie-b971de0717a66ae6085670fe5d3a469e236a9446.tar.gz sophie-b971de0717a66ae6085670fe5d3a469e236a9446.tar.zst sophie-b971de0717a66ae6085670fe5d3a469e236a9446.zip |
refactor: config file saving and debugging
Reduce the number of dependencies and the amount of code running in a
security sensitive context.
Instead of a deep comparison, we just compare the serialized versions
of the config files.
Signed-off-by: Kristóf Marussy <kristof@marussy.com>
Diffstat (limited to 'packages/main/src/reactions')
-rw-r--r-- | packages/main/src/reactions/__tests__/synchronizeConfig.spec.ts | 56 | ||||
-rw-r--r-- | packages/main/src/reactions/synchronizeConfig.ts | 40 |
2 files changed, 53 insertions, 43 deletions
diff --git a/packages/main/src/reactions/__tests__/synchronizeConfig.spec.ts b/packages/main/src/reactions/__tests__/synchronizeConfig.spec.ts index b5013ea..d3338d0 100644 --- a/packages/main/src/reactions/__tests__/synchronizeConfig.spec.ts +++ b/packages/main/src/reactions/__tests__/synchronizeConfig.spec.ts | |||
@@ -25,7 +25,7 @@ import type ConfigRepository from '../../infrastructure/config/ConfigRepository' | |||
25 | import SharedStore from '../../stores/SharedStore'; | 25 | import SharedStore from '../../stores/SharedStore'; |
26 | import type Disposer from '../../utils/Disposer'; | 26 | import type Disposer from '../../utils/Disposer'; |
27 | import { silenceLogger } from '../../utils/log'; | 27 | import { silenceLogger } from '../../utils/log'; |
28 | import synchronizeConfig from '../synchronizeConfig'; | 28 | import synchronizeConfig, { serializeConfig } from '../synchronizeConfig'; |
29 | 29 | ||
30 | let store: SharedStore; | 30 | let store: SharedStore; |
31 | const repository: ConfigRepository = { | 31 | const repository: ConfigRepository = { |
@@ -70,11 +70,11 @@ describe('when synchronizeializing', () => { | |||
70 | beforeEach(() => { | 70 | beforeEach(() => { |
71 | mocked(repository.readConfig).mockResolvedValueOnce({ | 71 | mocked(repository.readConfig).mockResolvedValueOnce({ |
72 | found: true, | 72 | found: true, |
73 | data: { | 73 | contents: serializeConfig({ |
74 | // Use a default empty config file to not trigger config rewrite. | 74 | // Use a default empty config file to not trigger config rewrite. |
75 | ...store.config, | 75 | ...store.config, |
76 | themeSource: 'dark', | 76 | themeSource: 'dark', |
77 | }, | 77 | }), |
78 | }); | 78 | }); |
79 | }); | 79 | }); |
80 | 80 | ||
@@ -97,12 +97,15 @@ describe('when synchronizeializing', () => { | |||
97 | it('should update the config file if new details are added during read', async () => { | 97 | it('should update the config file if new details are added during read', async () => { |
98 | mocked(repository.readConfig).mockResolvedValueOnce({ | 98 | mocked(repository.readConfig).mockResolvedValueOnce({ |
99 | found: true, | 99 | found: true, |
100 | data: { | 100 | contents: `{ |
101 | themeSource: 'light', | 101 | "themeSource": "light", |
102 | profile: { | 102 | "profiles": [ |
103 | name: 'Test profile', | 103 | { |
104 | }, | 104 | "name": "Test profile" |
105 | }, | 105 | } |
106 | ] | ||
107 | } | ||
108 | `, | ||
106 | }); | 109 | }); |
107 | await synchronizeConfig(store, repository); | 110 | await synchronizeConfig(store, repository); |
108 | expect(repository.writeConfig).toHaveBeenCalledTimes(1); | 111 | expect(repository.writeConfig).toHaveBeenCalledTimes(1); |
@@ -111,9 +114,10 @@ describe('when synchronizeializing', () => { | |||
111 | it('should not apply an invalid config file but should not overwrite it', async () => { | 114 | it('should not apply an invalid config file but should not overwrite it', async () => { |
112 | mocked(repository.readConfig).mockResolvedValueOnce({ | 115 | mocked(repository.readConfig).mockResolvedValueOnce({ |
113 | found: true, | 116 | found: true, |
114 | data: { | 117 | contents: `{ |
115 | themeSource: -1, | 118 | "themeSource": -1 |
116 | }, | 119 | } |
120 | `, | ||
117 | }); | 121 | }); |
118 | await synchronizeConfig(store, repository); | 122 | await synchronizeConfig(store, repository); |
119 | expect(store.settings.themeSource).not.toBe(-1); | 123 | expect(store.settings.themeSource).not.toBe(-1); |
@@ -136,7 +140,7 @@ describe('when it has loaded the config', () => { | |||
136 | beforeEach(async () => { | 140 | beforeEach(async () => { |
137 | mocked(repository.readConfig).mockResolvedValueOnce({ | 141 | mocked(repository.readConfig).mockResolvedValueOnce({ |
138 | found: true, | 142 | found: true, |
139 | data: store.config, | 143 | contents: serializeConfig(store.config), |
140 | }); | 144 | }); |
141 | mocked(repository.watchConfig).mockReturnValueOnce(watcherDisposer); | 145 | mocked(repository.watchConfig).mockReturnValueOnce(watcherDisposer); |
142 | sutDisposer = await synchronizeConfig(store, repository, throttleMs); | 146 | sutDisposer = await synchronizeConfig(store, repository, throttleMs); |
@@ -163,11 +167,11 @@ describe('when it has loaded the config', () => { | |||
163 | it('should read the config file when it has changed', async () => { | 167 | it('should read the config file when it has changed', async () => { |
164 | mocked(repository.readConfig).mockResolvedValueOnce({ | 168 | mocked(repository.readConfig).mockResolvedValueOnce({ |
165 | found: true, | 169 | found: true, |
166 | data: { | 170 | contents: serializeConfig({ |
167 | // Use a default empty config file to not trigger config rewrite. | 171 | // Use a default empty config file to not trigger config rewrite. |
168 | ...store.config, | 172 | ...store.config, |
169 | themeSource: 'dark', | 173 | themeSource: 'dark', |
170 | }, | 174 | }), |
171 | }); | 175 | }); |
172 | await configChangedCallback(); | 176 | await configChangedCallback(); |
173 | // Do not write back the changes we have just read. | 177 | // Do not write back the changes we have just read. |
@@ -178,12 +182,15 @@ describe('when it has loaded the config', () => { | |||
178 | it('should update the config file if new details are added', async () => { | 182 | it('should update the config file if new details are added', async () => { |
179 | mocked(repository.readConfig).mockResolvedValueOnce({ | 183 | mocked(repository.readConfig).mockResolvedValueOnce({ |
180 | found: true, | 184 | found: true, |
181 | data: { | 185 | contents: `{ |
182 | themeSource: 'light', | 186 | "themeSource": "light", |
183 | profile: { | 187 | "profiles": [ |
184 | name: 'Test profile', | 188 | { |
185 | }, | 189 | "name": "Test profile" |
186 | }, | 190 | } |
191 | ] | ||
192 | } | ||
193 | `, | ||
187 | }); | 194 | }); |
188 | await configChangedCallback(); | 195 | await configChangedCallback(); |
189 | expect(repository.writeConfig).toHaveBeenCalledTimes(1); | 196 | expect(repository.writeConfig).toHaveBeenCalledTimes(1); |
@@ -192,9 +199,10 @@ describe('when it has loaded the config', () => { | |||
192 | it('should not apply an invalid config file when it has changed but should not overwrite it', async () => { | 199 | it('should not apply an invalid config file when it has changed but should not overwrite it', async () => { |
193 | mocked(repository.readConfig).mockResolvedValueOnce({ | 200 | mocked(repository.readConfig).mockResolvedValueOnce({ |
194 | found: true, | 201 | found: true, |
195 | data: { | 202 | contents: `{ |
196 | themeSource: -1, | 203 | "themeSource": -1 |
197 | }, | 204 | } |
205 | `, | ||
198 | }); | 206 | }); |
199 | await configChangedCallback(); | 207 | await configChangedCallback(); |
200 | expect(store.settings.themeSource).not.toBe(-1); | 208 | expect(store.settings.themeSource).not.toBe(-1); |
diff --git a/packages/main/src/reactions/synchronizeConfig.ts b/packages/main/src/reactions/synchronizeConfig.ts index 7e366e2..247c2e2 100644 --- a/packages/main/src/reactions/synchronizeConfig.ts +++ b/packages/main/src/reactions/synchronizeConfig.ts | |||
@@ -18,7 +18,6 @@ | |||
18 | * SPDX-License-Identifier: AGPL-3.0-only | 18 | * SPDX-License-Identifier: AGPL-3.0-only |
19 | */ | 19 | */ |
20 | 20 | ||
21 | import deepEqual from 'deep-equal'; | ||
22 | import { debounce } from 'lodash-es'; | 21 | import { debounce } from 'lodash-es'; |
23 | import { reaction } from 'mobx'; | 22 | import { reaction } from 'mobx'; |
24 | 23 | ||
@@ -32,36 +31,38 @@ const DEFAULT_CONFIG_DEBOUNCE_TIME_MS = 1000; | |||
32 | 31 | ||
33 | const log = getLogger('synchronizeConfig'); | 32 | const log = getLogger('synchronizeConfig'); |
34 | 33 | ||
34 | export function serializeConfig(config: Config): string { | ||
35 | return `${JSON.stringify(config, undefined, 2)}\n`; | ||
36 | } | ||
37 | |||
35 | export default async function synchronizeConfig( | 38 | export default async function synchronizeConfig( |
36 | sharedStore: SharedStore, | 39 | sharedStore: SharedStore, |
37 | repository: ConfigRepository, | 40 | repository: ConfigRepository, |
38 | debounceTime: number = DEFAULT_CONFIG_DEBOUNCE_TIME_MS, | 41 | debounceTime: number = DEFAULT_CONFIG_DEBOUNCE_TIME_MS, |
39 | ): Promise<Disposer> { | 42 | ): Promise<Disposer> { |
40 | let lastConfigOnDisk: Config | undefined; | 43 | let lastConfigOnDisk: string | undefined; |
41 | 44 | ||
42 | async function writeConfig(): Promise<void> { | 45 | async function writeConfig(serializedConfig: string): Promise<void> { |
43 | const { config } = sharedStore; | 46 | await repository.writeConfig(serializedConfig); |
44 | await repository.writeConfig(config); | 47 | lastConfigOnDisk = serializedConfig; |
45 | lastConfigOnDisk = config; | ||
46 | } | 48 | } |
47 | 49 | ||
48 | async function readConfig(): Promise<boolean> { | 50 | async function readConfig(): Promise<boolean> { |
49 | const result = await repository.readConfig(); | 51 | const result = await repository.readConfig(); |
50 | if (result.found) { | 52 | if (result.found) { |
53 | const { contents } = result; | ||
51 | try { | 54 | try { |
52 | // This cast is unsound if the config file is invalid, | 55 | // This cast is unsound if the config file is invalid, |
53 | // but we'll throw an error in the end anyways. | 56 | // but we'll throw an error in the end anyways. |
54 | sharedStore.loadConfig(result.data as Config); | 57 | const data = JSON.parse(contents) as Config; |
58 | sharedStore.loadConfig(data); | ||
55 | } catch (error) { | 59 | } catch (error) { |
56 | log.error('Failed to apply config snapshot', result.data, error); | 60 | log.error('Failed to apply config snapshot', contents, error); |
57 | return true; | 61 | return true; |
58 | } | 62 | } |
59 | lastConfigOnDisk = sharedStore.config; | 63 | lastConfigOnDisk = serializeConfig(sharedStore.config); |
60 | // We can't use `comparer.structural` from `mobx`, because | 64 | if (contents !== lastConfigOnDisk) { |
61 | // it handles missing values and `undefined` values differently, | 65 | await writeConfig(lastConfigOnDisk); |
62 | // but JSON is unable to distinguish them. | ||
63 | if (!deepEqual(result.data, lastConfigOnDisk, { strict: true })) { | ||
64 | await writeConfig(); | ||
65 | } | 66 | } |
66 | } | 67 | } |
67 | return result.found; | 68 | return result.found; |
@@ -69,16 +70,17 @@ export default async function synchronizeConfig( | |||
69 | 70 | ||
70 | if (!(await readConfig())) { | 71 | if (!(await readConfig())) { |
71 | log.info('Config file was not found'); | 72 | log.info('Config file was not found'); |
72 | await writeConfig(); | 73 | const serializedConfig = serializeConfig(sharedStore.config); |
74 | await writeConfig(serializedConfig); | ||
73 | log.info('Created config file'); | 75 | log.info('Created config file'); |
74 | } | 76 | } |
75 | 77 | ||
76 | const disposeReaction = reaction( | 78 | const disposeReaction = reaction( |
77 | () => sharedStore.config, | 79 | () => sharedStore.config, |
78 | debounce((config) => { | 80 | debounce(() => { |
79 | // We can compare snapshots by reference, since it is only recreated on store changes. | 81 | const serializedConfig = serializeConfig(sharedStore.config); |
80 | if (!deepEqual(config, lastConfigOnDisk, { strict: true })) { | 82 | if (serializedConfig !== lastConfigOnDisk) { |
81 | writeConfig().catch((error) => { | 83 | writeConfig(serializedConfig).catch((error) => { |
82 | log.error('Failed to write config on config change', error); | 84 | log.error('Failed to write config on config change', error); |
83 | }); | 85 | }); |
84 | } | 86 | } |