registry: don't trust the clipboard content type

[andyholmes]

The clipboard content type may be a filename represented by a variety of mime-types, so double-check it is supported type before loading the contents.

closes #447 closes #453

[andyholmes]

I think this is a reasonable fix, but you're the clipboard expert now :upside_down_face:

[ken-kuro]

Sorry to bring this up again. But since #460 was merged, I think we should discuss more about this, cause in the current code, if the contentType is unknown, we're loading its content which might lead to RCE in some case. Do you think some whitelisting like this would be better ? And could you please explain why are we load the content with contentType /text? @andyholmes

const allowed = ['/image', '/text']
if (contentType && allowed.some(type => contentType.startsWith(type))  {
    bytes = await new Promise((resolve, reject) => file.load_contents_async(null, (obj, res) => {
        let [success, contents] = obj.load_contents_finish(res);

        if (success) {
            resolve(contents);
        }
        else {
            reject(
                new Error('Clipboard Indicator: could not read image file from cache')
            );
        }
    }));
}
else {
    bytes = new TextEncoder().encode(jsonEntry.contents);
}

[andyholmes]

I can not explain that, sorry. The intention was to retain the same behavior, without relying on the advertised clipboard file being an image or text.

If the content type of the file is unknown, it is loading the literal contents of the clipboard; which is a file path.

[andyholmes]

Sorry, I didn't review your previous pull request, I only read your explanation. You're right that this line:

https://github.com/Tudmotu/gnome-shell-extension-clipboard-indicator/blob/72f779d974a1990eddafcd211f354e038a39e6be/registry.js#L240

Should instead be something like:

 if (!contentType?.startsWith('image/') && !contentType?.startsWith('text/')) {

There's a couple ways you can perform the same logical test, but the branch should be followed whenever the content type is neither text/* nor image/*.

[Tudmotu]

1. I agree using the optional chaining as Andy suggests is probably the way we should have done this. I pointed out the issue in the PR but didn't have time to make any changes yesterday and wanted to get the fix out
2. More broadly speaking I think the original issue is the fact I used the "content" field to save the file path. In hindsight that was the wrong decision. If the file path would have been saved in a different field (e.g. filePath) the original problem would not exist

[ken-kuro]

I see, at the end of the day, it's just how we want to deal with null or undefined content type, cause tbh, I'm not sure what exactly the case that it has those values , the docs just use unknown without a clear explanation. But if the content field contains the file path, I think we're fine with both approaches

[andyholmes]

Unknown in this context is pretty literal. There are only so many ways you can safely detect the content of file (i.e. magic numbers).