Skip to content

Commit 699f977

Browse files
jason-ojisanjason-ojisan
committed
Fix Rikaitan API not able to export dictionary CSS on Firefox
* Use CSSOM as fallback for sanitization * Allow data-permissions-setting to trigger transforms * Add frontend for rikaitanApiAllowCssSanitizationBypass * Add rikaitanApiAllowCssSanitizationBypass option * Allow css to be passed through without sanitization * Move newline stripping outside sanitization method * Log css when sanitization is bypassed * Only show sanitization bypass for firefox * Fix typo * Improve sanitization bypass description <rikaitan.link>OWU0NDQzN2RjNzE2N2U4NGM1Nzc2MmE5OTJkNDkzYWRlZDE0OGQ3ZQo=</rikaitan.link>
1 parent 177b3df commit 699f977

File tree

7 files changed

+121
-27
lines changed

7 files changed

+121
-27
lines changed

‎ext/data/schemas/options-schema.json‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,8 @@
129129
"fontSize",
130130
"lineHeight",
131131
"enableRikaitanApi",
132-
"rikaitanApiServer"
132+
"rikaitanApiServer",
133+
"rikaitanApiAllowCssSanitizationBypass"
133134
],
134135
"properties": {
135136
"enable": {
@@ -346,6 +347,10 @@
346347
"rikaitanApiServer": {
347348
"type": "string",
348349
"default": "http://127.0.0.1:19633"
350+
},
351+
"rikaitanApiAllowCssSanitizationBypass": {
352+
"type": "boolean",
353+
"default": false
349354
}
350355
}
351356
},

‎ext/js/comm/rikaitan-api.js‎

Lines changed: 41 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,7 @@ export class RikaitanApi {
213213

214214
const dictionaryMedia = includeMedia ? await this._fetchDictionaryMedia(dictionaryEntries) : [];
215215
const audioMedia = includeAudioMedia ? await this._fetchAudio(dictionaryEntries, profileOptions) : [];
216-
const commonDatas = await this._createCommonDatas(text, dictionaryEntries, dictionaryMedia, audioMedia, profileOptions);
216+
const commonDatas = await this._createCommonDatas(text, dictionaryEntries, dictionaryMedia, audioMedia, profileOptions, domlessDocument);
217217
const ankiTemplateRenderer = new AnkiTemplateRenderer(domlessDocument, domlessWindow);
218218
await ankiTemplateRenderer.prepare();
219219
const templateRenderer = ankiTemplateRenderer.templateRenderer;
@@ -378,9 +378,10 @@ export class RikaitanApi {
378378
* @param {import('rikaitan-api.js').apiDictionaryMediaDetails[]} dictionaryMediaDetails
379379
* @param {import('rikaitan-api.js').apiAudioMediaDetails[]} audioMediaDetails
380380
* @param {import('settings').ProfileOptions} options
381+
* @param {Document} domlessDocument
381382
* @returns {Promise<import('anki-note-builder.js').CommonData[]>}
382383
*/
383-
async _createCommonDatas(text, dictionaryEntries, dictionaryMediaDetails, audioMediaDetails, options) {
384+
async _createCommonDatas(text, dictionaryEntries, dictionaryMediaDetails, audioMediaDetails, options, domlessDocument) {
384385
/** @type {import('anki-note-builder.js').CommonData[]} */
385386
const commonDatas = [];
386387
for (const dictionaryEntry of dictionaryEntries) {
@@ -451,43 +452,71 @@ export class RikaitanApi {
451452
}],
452453
dictionaryMedia: dictionaryMedia,
453454
},
454-
dictionaryStylesMap: await this._getDictionaryStylesMapDomless(options.dictionaries),
455+
dictionaryStylesMap: await this._getDictionaryStylesMapDomless(options, domlessDocument),
455456
});
456457
}
457458
return commonDatas;
458459
}
459460

460461
/**
461-
* @param {import('settings').DictionariesOptions} dictionaries
462+
* @param {import('settings').ProfileOptions} options
463+
* @param {Document} domlessDocument
462464
* @returns {Promise<Map<string, string>>}
463465
*/
464-
async _getDictionaryStylesMapDomless(dictionaries) {
466+
async _getDictionaryStylesMapDomless(options, domlessDocument) {
465467
const styleMap = new Map();
466-
for (const dictionary of dictionaries) {
468+
for (const dictionary of options.dictionaries) {
467469
const {name, styles} = dictionary;
468470
if (typeof styles === 'string') {
469-
styleMap.set(name, await this._sanitizeCSSOffscreen(styles));
471+
// newlines and returns do not get converted into json well, are not required in css, and cause invalid css if not parsed for by the api consumer, just do the work for them
472+
const sanitizedCSS = (await this._sanitizeCSSOffscreen(options, styles, domlessDocument)).replaceAll(/(\r|\n)/g, ' ');
473+
styleMap.set(name, sanitizedCSS);
470474
}
471475
}
472476
return styleMap;
473477
}
474478

475479
/**
480+
* @param {import('settings').ProfileOptions} options
476481
* @param {string} css
482+
* @param {Document} domlessDocument
477483
* @returns {Promise<string>}
478484
*/
479-
async _sanitizeCSSOffscreen(css) {
485+
async _sanitizeCSSOffscreen(options, css, domlessDocument) {
480486
if (css.length === 0) { return ''; }
481487
try {
488+
if (!this._offscreen) {
489+
throw new Error('Offscreen page not available');
490+
}
482491
const sanitizedCSS = this._offscreen ? await this._offscreen.sendMessagePromise({action: 'sanitizeCSSOffscreen', params: {css}}) : '';
483492
if (sanitizedCSS.length === 0 && css.length > 0) {
484-
throw new Error('Failed to sanitize css');
493+
throw new Error('CSS parsing failed');
485494
}
486-
// newlines and returns do not get converted into json well, are not required in css, and cause invalid css if not parsed for by the api consumer, just do the work for them
487-
return sanitizedCSS.replaceAll(/(\r|\n)/g, ' ');
495+
return sanitizedCSS;
488496
} catch (e) {
489-
log.log('Failed to sanitize css: ' + css.replaceAll(/(\r|\n)/g, ' ') + ', ' + toError(e).message);
497+
log.log('Offscreen CSS sanitizer failed: ' + toError(e).message);
490498
}
499+
500+
try {
501+
const style = domlessDocument.createElement('style');
502+
// eslint-disable-next-line no-unsanitized/property
503+
style.innerHTML = css;
504+
domlessDocument.appendChild(style);
505+
const styleSheet = style.sheet;
506+
if (!styleSheet) {
507+
throw new Error('CSS parsing failed');
508+
}
509+
return [...styleSheet.cssRules].map((rule) => rule.cssText || '').join('\n');
510+
} catch (e) {
511+
log.log('CSSOM CSS sanitizer failed: ' + toError(e).message);
512+
}
513+
514+
if (options.general.rikaitanApiAllowCssSanitizationBypass) {
515+
log.log('Failed to sanitize CSS. Sanitization bypass is enabled, passing through CSS without sanitization: ' + css.replaceAll(/(\r|\n)/g, ' '));
516+
return css;
517+
}
518+
519+
log.log('Failed to sanitize CSS: ' + css.replaceAll(/(\r|\n)/g, ' '));
491520
return '';
492521
}
493522

‎ext/js/dom/dom-data-binder.js‎

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -25,16 +25,16 @@ import {SelectorObserver} from './selector-observer.js';
2525
*/
2626
export class DOMDataBinder {
2727
/**
28-
* @param {string} selector
28+
* @param {string[]} selectors
2929
* @param {import('dom-data-binder').CreateElementMetadataCallback<T>} createElementMetadata
3030
* @param {import('dom-data-binder').CompareElementMetadataCallback<T>} compareElementMetadata
3131
* @param {import('dom-data-binder').GetValuesCallback<T>} getValues
3232
* @param {import('dom-data-binder').SetValuesCallback<T>} setValues
3333
* @param {import('dom-data-binder').OnErrorCallback<T>|null} [onError]
3434
*/
35-
constructor(selector, createElementMetadata, compareElementMetadata, getValues, setValues, onError = null) {
36-
/** @type {string} */
37-
this._selector = selector;
35+
constructor(selectors, createElementMetadata, compareElementMetadata, getValues, setValues, onError = null) {
36+
/** @type {string[]} */
37+
this._selectors = selectors;
3838
/** @type {import('dom-data-binder').CreateElementMetadataCallback<T>} */
3939
this._createElementMetadata = createElementMetadata;
4040
/** @type {import('dom-data-binder').CompareElementMetadataCallback<T>} */
@@ -49,27 +49,31 @@ export class DOMDataBinder {
4949
this._updateTasks = new TaskAccumulator(this._onBulkUpdate.bind(this));
5050
/** @type {TaskAccumulator<import('dom-data-binder').ElementObserver<T>, import('dom-data-binder').AssignTaskValue>} */
5151
this._assignTasks = new TaskAccumulator(this._onBulkAssign.bind(this));
52-
/** @type {SelectorObserver<import('dom-data-binder').ElementObserver<T>>} */
53-
this._selectorObserver = new SelectorObserver({
52+
/** @type {SelectorObserver<import('dom-data-binder').ElementObserver<T>>[]} */
53+
this._selectorObservers = selectors.map((selector) => new SelectorObserver({
5454
selector,
5555
ignoreSelector: null,
5656
onAdded: this._createObserver.bind(this),
5757
onRemoved: this._removeObserver.bind(this),
5858
onChildrenUpdated: this._onObserverChildrenUpdated.bind(this),
5959
isStale: this._isObserverStale.bind(this),
60-
});
60+
}));
6161
}
6262

6363
/**
6464
* @param {Element} element
6565
*/
6666
observe(element) {
67-
this._selectorObserver.observe(element, true);
67+
for (const selectorObserver of this._selectorObservers) {
68+
selectorObserver.observe(element, true);
69+
}
6870
}
6971

7072
/** */
7173
disconnect() {
72-
this._selectorObserver.disconnect();
74+
for (const selectorObserver of this._selectorObservers) {
75+
selectorObserver.disconnect();
76+
}
7377
}
7478

7579
/** */
@@ -98,8 +102,10 @@ export class DOMDataBinder {
98102
}
99103
if (all) {
100104
targets.length = 0;
101-
for (const observer of this._selectorObserver.datas()) {
102-
targets.push([observer, null]);
105+
for (const selectorObserver of this._selectorObservers) {
106+
for (const observer of selectorObserver.datas()) {
107+
targets.push([observer, null]);
108+
}
103109
}
104110
}
105111

‎ext/js/pages/settings/generic-setting-controller.js‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ export class GenericSettingController {
3232
this._defaultScope = 'profile';
3333
/** @type {DOMDataBinder<import('generic-setting-controller').ElementMetadata>} */
3434
this._dataBinder = new DOMDataBinder(
35-
'[data-setting]',
35+
['[data-setting]', '[data-permissions-setting]'],
3636
this._createElementMetadata.bind(this),
3737
this._compareElementMetadata.bind(this),
3838
this._getValues.bind(this),
@@ -75,7 +75,8 @@ export class GenericSettingController {
7575
*/
7676
_createElementMetadata(element) {
7777
if (!(element instanceof HTMLElement)) { return void 0; }
78-
const {setting: path, scope, transform: transformRaw} = element.dataset;
78+
const {scope, transform: transformRaw} = element.dataset;
79+
const path = element.dataset.setting ?? element.dataset.permissionsSetting;
7980
if (typeof path !== 'string') { return void 0; }
8081
const scope2 = this._normalizeScope(scope);
8182
return {

‎ext/settings.html‎

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,13 @@ <h1>Rikaitan Settings</h1>
188188
</div>
189189
</div>
190190
<div class="settings-item-right">
191-
<label class="toggle"><input type="checkbox" class="permissions-toggle" data-permissions-setting="general.enableRikaitanApi" data-required-permissions="nativeMessaging"><span class="toggle-body"><span class="toggle-track"></span><span class="toggle-knob"></span></span></label>
191+
<label class="toggle"><input type="checkbox" class="permissions-toggle" data-permissions-setting="general.enableRikaitanApi" data-required-permissions="nativeMessaging"
192+
data-transform='{
193+
"type": "setVisibility",
194+
"selector": "#rikaitan-api-additional-options",
195+
"condition": {"op": "===", "value": true}
196+
}'
197+
><span class="toggle-body"><span class="toggle-track"></span><span class="toggle-knob"></span></span></label>
192198
</div>
193199
</div>
194200
<div class="settings-item-children more" hidden>
@@ -205,6 +211,51 @@ <h1>Rikaitan Settings</h1>
205211
<a tabindex="0" class="more-toggle" data-parent-distance="3">Less&hellip;</a>
206212
</p>
207213
</div>
214+
<div class="settings-item-children settings-item-children-group" id="rikaitan-api-additional-options" hidden>
215+
<div class="settings-item">
216+
<div class="settings-item-inner settings-item-inner-wrappable">
217+
<div class="settings-item-left">
218+
<div class="settings-item-label">
219+
Allow bypassing css sanitization
220+
</div>
221+
<div class="settings-item-description">
222+
<p class="warning-text">
223+
Only enable this option if you trust the authors of your dictionaries or will perform sanitization outside of Rikaitan.
224+
Malicious dictionaries can send malformed CSS to allow Javascript execution if injected into a webview or browser.
225+
<a tabindex="0" class="more-toggle more-only" data-parent-distance="4">More&hellip;</a>
226+
</p>
227+
</div>
228+
<div class="settings-item-description more" hidden>
229+
<p>
230+
A malicious dictionary which contains malicious CSS may try to execute Javascript.
231+
For example, if the output of the Rikaitan API is used in Anki, then the resulting Anki card may execute unintended Javascript.
232+
The risk in this case is similar to downloading a malicious Anki deck.
233+
Namely, in both cases, you are relying purely on Anki's sandbox to protect your machine and information from the malicious code.
234+
If the Anki sandbox works, card contents could be leaked (a minor confidential issue).
235+
If the sandbox fails, there is much more to worry about (actions being taken on your machine such as stealing credentials and sensitive information, or ransomware taking over your computer).
236+
</p>
237+
<p>
238+
If you choose to disable the CSS sanitization, we recommend thinking end-to-end about where the Rikaitan API output is used, and whether that destination is resilient to potentially malicious Javascript code being included, otherwise you are potentially risking the data and integrity of your system.
239+
</p>
240+
<p>
241+
Sanitization bypass will allow CSS that cannot be sanitized to pass through the sanitizer unchanged.
242+
This ONLY applies to the Rikaitan API, it will not make any other areas of Rikaitan potentially vulnerable.
243+
</p>
244+
<p>
245+
This can be required to retrieve CSS on some browsers which do not support the offscreen page necessary for full CSS sanitization in the Rikaitan API on the backend.
246+
Rikaitan will attempt to fall back on basic sanitization but this will fail for dictionaries making use of complex CSS.
247+
</p>
248+
<p class="margin-above">
249+
<a tabindex="0" class="more-toggle" data-parent-distance="3">Less&hellip;</a>
250+
</p>
251+
</div>
252+
</div>
253+
<div class="settings-item-right">
254+
<label class="toggle"><input type="checkbox" data-setting="general.rikaitanApiAllowCssSanitizationBypass"><span class="toggle-body"><span class="toggle-track"></span><span class="toggle-knob"></span></span></label>
255+
</div>
256+
</div>
257+
</div>
258+
</div>
208259
</div>
209260
</div>
210261

‎test/options-util.test.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,7 @@ function createProfileOptionsUpdatedTestData1() {
314314
stickySearchHeader: false,
315315
enableRikaitanApi: false,
316316
rikaitanApiServer: 'http://127.0.0.1:19633',
317+
rikaitanApiAllowCssSanitizationBypass: false,
317318
},
318319
audio: {
319320
enabled: true,

‎types/ext/settings.d.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,7 @@ export type GeneralOptions = {
153153
sortFrequencyDictionaryOrder: SortFrequencyDictionaryOrder;
154154
stickySearchHeader: boolean;
155155
enableRikaitanApi: boolean;
156+
rikaitanApiAllowCssSanitizationBypass: boolean;
156157
};
157158

158159
export type PopupWindowOptions = {

0 commit comments

Comments
 (0)