Export copy to clipboard functionality #50

weizman · 2024-07-12T22:55:20Z

close #46 by providing a new LavaDome method copy which copies the secret to clipboard without leaking it to any other entity.

@weizman Don't forget to address TODO in code and update docs

packages/core/src/lavadome.mjs

legobeat · 2024-10-01T06:21:53Z

packages/core/src/lavadome.mjs

@@ -41,21 +46,25 @@ export function LavaDome(host, opts) {
    const child = hardened();
    appendChild(shadow, child);

-    function text(text) {
-        if (typeof text !== 'string') {
+    let secret = '';


It doesn't feel great to have the secret hang out in memory in this long-lived scope... Not that I have a proposal on how it could be achieved differently, though...

I see your point, but essentially, it's only natural that in order to provide service where the input is somehow manipulated we must have access to it

legobeat · 2024-10-01T06:24:59Z

packages/core/src/lavadome.mjs

-    defineProperties(this, {text: {value: text}});
+    defineProperties(this, {
+        text: {value: text},
+        copy: {value: copy},


Re https://github.com/LavaMoat/LavaDome/pull/50/files#r1782176796

Could setting the value of copy (including its declaration and instantiation) be moved to the inner scope of text? If so, maybe the need to move secret out of scope is no longer here?

Or would that conflict with the "make exported API tamper-proof" part? If so I think I'm still leaning towards that as the lesser bad. WDYT?

How would we export copy if it's inside text?

If we assign it inside the handler of text, is there a need to?

AFAIU, if you move the copy function into the text function, you closure the secret within the text scope instead of the LavaDome scope, which doesn't solve the problem, just shifts it elsewhere - the secret still must be scoped somewhere

The secret is already part of the text scope (by virtue of being an argument).

However, moving/copying it to the outer scope widens the exposure (and lifetime, but not sure to what extent we can actually limit that if introducing the copy-to-clipboard functionality as intended).

For example, consider a scenario where text() is called a second time with invalid input on an existing LD instance:

ld.text('actualsecret'); // at some point later... ld.text();

This will throw an Error where 'actualsecret' is still in scope. I don't have a PoC or anything but does seem like it could more easily leak to a malicious extension or devtool/debugger integration.

Vs if it's contained in the text function, separate text() invocations are not able to access each others' secrets, and subsequent calls would make new copy instances with isolated scopes wrt secret.

One way to at least prevent one aspect of scope-creep of secret itself without compromising on the tamper-proofing could be to freeze a property accessor and reassign copy itself instead:

// instead of `let secret` let _copy = () => Promise.resolve(); async function copy() { await _copy(); } // [...] function text(input) { _copy = async () = { // `_copy` can now access `input` from `text` directly } }

sold c8c9cd7

I actually just removed c8c9cd7 because:

it turns out the way i did it dropped a commit coming from main by accident which could have been a terrible mistake

a younger comment made me realize the implementation isn't right, and that also I'm again not convinced this can be solved without storing the secret in some upper scope

For example, I'm not sure how #50 (comment) demonstrates this otherwise, how scoping a function that has access to the secret instead of the secret itself is far better?

A demonstrating PR here could very much help me understand 🙏

legobeat · 2024-10-02T01:39:36Z

packages/core/src/lavadome.mjs

            throw new Error(
-                `LavaDomeCore: first argument must be a string, instead got ${stringify(text)}`);
+                `LavaDomeCore: first argument must be a string, instead got ${stringify(input)}`);


Suggested change

`LavaDomeCore: first argument must be a string, instead got ${stringify(input)}`);

`LavaDomeCore: first argument must be a string, instead got ${typeof input}`);

Type should be enough, no need to template secret in error message

also 38ef179

legobeat · 2024-10-02T01:40:48Z

packages/core/src/lavadome.mjs

+        const type = 'text/plain';
+        const blob = new Blob([secret], {type});
+        const data = [new ClipboardItem({[type]: blob})];
+        await write(clipboard, data);


Maybe swallow errors on write?

Suggested change

const type = 'text/plain';

const blob = new Blob([secret], {type});

const data = [new ClipboardItem({[type]: blob})];

await write(clipboard, data);

try {

const type = 'text/plain';

const blob = new Blob([secret], {type});

const data = [new ClipboardItem({[type]: blob})];

// do we also want to return the result of the await here?

await write(clipboard, data);

} finally {}

To reduce the likelihood of an error containing a reference to the secret?

We shouldn't swallow errors that have nothing to do with us, the app should become aware of such errors being thrown. The error will never leak the secret, because if it did, it would have been a major info leak in the design of the native API, which should remain outside of LavaDome's jurisdiction.

boneskull

mostly just questions. I'm not familiar with this (or React)

boneskull · 2024-10-07T21:30:47Z

packages/core/src/lavadome.mjs

        // add a distraction against side channel leaks attack attempts
        appendChild(child, distraction());
    }
 }
+
+export function LavaDome(host, opts) {


Is this a constructor? A React component? if the former, why not use class?

It's the former.
Because there's no way to make methods of a class (truly) private and inaccessible.

not even with #private? but wait, why do we need to worry about that at all in this use case? I don't see any methods.

not even with #private

text and the new copy are methods.
They're not assigned to the prototype as methods for the same privacy reason, but they are effectively methods of the instance.

packages/core/src/native.mjs

boneskull · 2024-10-07T21:32:45Z

packages/react/src/lavadome.jsx

    }

    if (!hasOwn(textToTokenMap, text)) {
        const token = create(null);
        textToTokenMap[text] = token;
        set(tokenToTextMap, token, text);
+        set(tokenToCopyInvokerMap, token, () => {});


why is there a noop function here?

I'm just generally in favor of having default values correspond to expected values, so since the values of this map are the copy method which is a function, I figured the default value should also be a function.

Am open to suggestions.

boneskull · 2024-10-07T21:33:46Z

packages/react/src/lavadome.jsx

    }

-    return textToTokenMap[text];
+    const token = textToTokenMap[text];
+    const copy = () => get(tokenToCopyInvokerMap, token)();


should this be a bound function or does it not matter?

the copy function stores what it needs within the closure that was originally constructed for it, which is the alternative to bounding in this case

boneskull · 2024-10-07T21:33:56Z

packages/react/src/lavadome.jsx

-    // variable @text is named that way only for visibility - in reality it's a lavadome token
-    const token = text, host = useRef(null);
+export const LavaDome = ({ token, unsafeOpenModeShadow }) => {
+    const host = useRef(null);


what does this do?

to be clear, I'm asking about L60 not L59

useRef is this React feature that allows you to create a reference to some value that should not trigger rerendering of a React component (since other types of variables definition in React do trigger a rerender).

It's useful here, because React allows you to make it point to a non React declarative DOM node, so that it's accessible when passed on to another component (which is what we do with the span host here).

boneskull · 2024-10-07T21:34:11Z

packages/react/src/lavadome.jsx


    return (
        // form a span to act as the LavaDome host
        <span ref={host}>
            <LavaDomeShadow
-                host={host} token={token}
+                host={host}
+                // accept both formats (token, or [token, copy])


Originally, so that users can ignore the copy option and just do const token = toLavaDomeCapabilities(secret);, but I think to cancel that would mean they'd have to do const [token] = toLavaDomeCapabilities(secret); which is fine, so i need to see if that's the only reason which makes this a wrong choice or not

would prefer to do [token]/[token, copy] if there's no great reason just for simplicity sake. I assume this is a React thing that means you can't just use an object and instead need to deal with arrays

c9f2ec4 (you're right about the object part too)

boneskull · 2024-10-07T21:34:43Z

packages/core/src/lavadome.mjs

-    replaceChildren(shadow);
+function defineCopy(instance, input) {
+    return async function copy() {
+        const type = 'text/plain';


how can we support other mimetypes?

The whole project currently focuses on plain text only because of how hard it is to safely present more complex types.

Generally, this is a question for another day, but i might be able to answer your question if you have an example in mind for "other mimetypes".

a PNG. say it's a QR code for initializing TOTP that we don't want to expose

Very interesting, perhaps sometime far in the future

boneskull · 2024-10-07T21:35:14Z

packages/core/src/lavadome.mjs

+    return function text(input) {
+        const type = typeof input;
+        if (type !== 'string') {
+            throw new Error(


Suggested change

throw new Error(

throw new TypeError(

look, an actual suggestion!

damn right fb55997

legobeat · 2024-10-07T22:26:20Z

packages/core/src/lavadome.mjs

            const span = createElement(document, 'span');
            // mark as internal LavaDome instance
            opts[OPTIONS.isInnerInstance] = true;
            new LavaDome(span, opts).text(char);
            appendChild(child, span);
        });

+        // make exported API tamper-proof
+        defineProperties(instance, {
+            copy: {value: defineCopy(instance, input)},


Won't this error with Cannot redefine property: copy if text is called twice on the same instance? Could we add testing coverage for what happens in that scenario?

outdated, continue @ #50 (comment)

support issue 46

63633a6

weizman marked this pull request as draft July 12, 2024 23:01

weizman added 2 commits July 16, 2024 14:30

support ld react

81297dc

update comment

d165398

weizman marked this pull request as ready for review July 16, 2024 11:38

weizman added 2 commits July 16, 2024 14:52

use safe native

2e9046f

button to anchor

3f371ae

weizman self-assigned this Jul 25, 2024

weizman added 2 commits September 4, 2024 15:33

address navigation bypass

cebd9cb

Merge branch 'main' into issue-46

ea7f8e5

legobeat reviewed Sep 10, 2024

View reviewed changes

packages/core/src/lavadome.mjs Outdated Show resolved Hide resolved

weizman force-pushed the main branch 2 times, most recently from 7958a69 to fa4643f Compare September 18, 2024 08:04

weizman added 2 commits September 18, 2024 11:27

Merge remote-tracking branch 'origin/main' into issue-46

1b57716

change var name

fb25541

weizman requested review from legobeat and naugtur September 18, 2024 08:28

legobeat reviewed Oct 1, 2024

View reviewed changes

legobeat reviewed Oct 2, 2024

View reviewed changes

weizman added 2 commits October 4, 2024 13:08

Merge branch 'main' into issue-46

ac1636d

safer

5265dd4

weizman requested a review from legobeat October 7, 2024 13:51

boneskull previously approved these changes Oct 7, 2024

View reviewed changes

legobeat reviewed Oct 7, 2024

View reviewed changes

weizman dismissed boneskull’s stale review via fb55997 October 8, 2024 15:14

weizman added 3 commits October 9, 2024 14:07

improve

4f01fac

error to typerror

19ed8a9

change api

1b73fe6

weizman force-pushed the issue-46 branch from c9f2ec4 to 1b73fe6 Compare October 9, 2024 11:11

	`LavaDomeCore: first argument must be a string, instead got ${stringify(input)}`);
	`LavaDomeCore: first argument must be a string, instead got ${typeof input}`);

Export copy to clipboard functionality #50

Are you sure you want to change the base?

Export copy to clipboard functionality #50

Conversation

weizman commented Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legobeat Oct 1, 2024 • edited Loading

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

legobeat Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

legobeat Oct 2, 2024 • edited Loading

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

legobeat Oct 2, 2024 • edited Loading

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

boneskull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boneskull Oct 8, 2024 • edited Loading

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

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

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

weizman commented Jul 12, 2024 •

edited

Loading

legobeat Oct 1, 2024 •

edited

Loading

legobeat Oct 1, 2024 •

edited

Loading

legobeat Oct 2, 2024 •

edited

Loading

legobeat Oct 2, 2024 •

edited

Loading

boneskull Oct 8, 2024 •

edited

Loading