Page MenuHomePhabricator

[keyserver] Capture `siwe-nonce` header and make available to `SIWE` component
ClosedPublic

Authored by atul on Dec 21 2022, 11:58 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 7:34 AM
Unknown Object (File)
Fri, Dec 20, 7:16 AM
Unknown Object (File)
Fri, Dec 20, 7:01 AM
Unknown Object (File)
Fri, Dec 20, 6:59 AM
Unknown Object (File)
Fri, Dec 20, 6:50 AM
Unknown Object (File)
Fri, Dec 20, 6:45 AM
Unknown Object (File)
Fri, Dec 20, 1:52 AM
Unknown Object (File)
Wed, Dec 18, 6:19 PM
Subscribers
None

Details

Summary

Capture the siwe-nonce header (which will be passed via the WebView component on native) from the req object and make available to SIWE component in landing via SIWENonceContext.Provider.


Depends on D5975

Test Plan

Able to retrieve value in SIWE as expected:

3b63ab.png (78×970 px, 45 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul published this revision for review.Dec 21 2022, 11:58 AM
atul edited the test plan for this revision. (Show Details)
ashoat requested changes to this revision.Dec 21 2022, 12:06 PM
ashoat added inline comments.
keyserver/src/responders/landing-handler.js
106 ↗(On Diff #19974)

You should handle this like line 174, where we declare a var that is passed down

Then in landing/root.js, have a declare var, and then render a SIWENonceContext.Provider around Landing

This revision now requires changes to proceed.Dec 21 2022, 12:06 PM

fix SSR when siweNonce header missing (we want siweNonce to be null not empty string)

keyserver/src/responders/landing-handler.js
175 ↗(On Diff #19979)

Considered using nullish coalescing operator (??) but decided against. If I understand properly this line doesn't go through any sort of transpiler so there might be browser compatibility issues?

Also tried nested string interpolation but wasn't able to get it to work.

Open to any suggestions here.

Please address comment before landing

keyserver/src/responders/landing-handler.js
175 ↗(On Diff #19979)

Yeah def don't use nullish coalescing here, but there are better options. You can JSON.stringify it, although then maybe there would be some sneaky attack if somebody set the siwe-nonce header in some special way (Node always returns a string, right?)

If that doesn't work you can just define const siweNonceString = siweNonce ?? "null"; on line 171

This revision is now accepted and ready to land.Dec 21 2022, 5:31 PM

Given we'll need to support this on web as well I wonder if a header will be harder in that context than a query param

Although reading through D5978 it seems like web and mobile will need to be fairly different anyways

keyserver/src/responders/landing-handler.js
175 ↗(On Diff #19979)

Encounter the following using JSON.stringify when siweNonce is null (when we navigate to the page directly in desktop browser):

08b317.png (46×500 px, 14 KB)

Similarly we can't define const siweNonceString = siweNonce ?? "null";

I think what we'd need to do is

const siweNonceString = `"${siweNonce}"` ?? "null";

Where if siweNonce is null then the string gets computed as "" (empty string) which is falsey so we end up with "null" which ends up as null once in the html` string. (The way I thought about it is when we include siweNonceString in html` string the outer quote effectively gets "dropped")

keyserver/src/responders/landing-handler.js
175 ↗(On Diff #19979)

I think what we'd need to do is

Disregard everything past this... it's incorrect.

What we'd really have is "" (two double quotes) NOT an empty string.

keyserver/src/responders/landing-handler.js
175 ↗(On Diff #19979)

More verbose but I think the cleanest option here is:

const siweNonceString = siweNonce ? `"${siweNonce}"` : "null";

I don't think there's a way to do this with nullish coalescing even "from the outside"

keyserver/src/responders/landing-handler.js
175 ↗(On Diff #19979)

More verbose but I think the cleanest option here is:

const siweNonceString = siweNonce ? `"${siweNonce}"` : "null";

I don't think there's a way to do this with nullish coalescing even "from the outside"

Yeah this is correct.

Elements tab:

8503a7.png (2×3 px, 4 MB)

Console tab:

08f769.png (2×3 px, 4 MB)

Although reading through D5978 it seems like web and mobile will need to be fairly different anyways

Yeah I think things will be pretty different

This revision was landed with ongoing or failed builds.Dec 21 2022, 11:38 PM
This revision was automatically updated to reflect the committed changes.
keyserver/src/responders/landing-handler.js
172

This seems dangerous... you're letting somebody inject JavaScript into the site with a header. Eg. what if `siweNonce was set to something like:

const siweNonce = '" : null; do something bad code; const otherVar = true ? "';

Can you create a task to address?

keyserver/src/responders/landing-handler.js
172

I'm not fully understanding the scenario where code can be injected

keyserver/src/responders/landing-handler.js
172

08bb5b.png (756×1 px, 143 KB)

I believe the double quotes around ${siweNonce} ensure that siweNonceString is a "quoted data value."

keyserver/src/responders/landing-handler.js
172

Reminder:

Can you create a task to address?

keyserver/src/responders/landing-handler.js
172