PoC HWB #2298
PoC HWB #2298
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
|
There was a problem hiding this comment.
For this PoC, this is fine, just wanted to make sure we make this a route handler and whitelist the queries and mutations via a route handler instead. This should all be working within canary...spike/gql-enablelist#diff-1e657e3f6573e741ec9de194cf6f83b13cda623925400917c480315d2cc4d905 for both queries and mutations cc. @davidchin
There was a problem hiding this comment.
Looking at the whitelist POC, I believe it's sufficient to whitelist based on the name of the query or mutation. Otherwise, the filtering logic seems overly complicated as we need to traverse the query.
I recall we previously discussed delegating the filtering logic to the server by passing a header. The distinction between browser and server client already exists on the API side. We already limit what a browser client can do, so it would be ideal if we can leverage that existing logic instead of introducing a separate filtering layer on the Catalyst side. WDYT?
There was a problem hiding this comment.
We want to prevent other scripts/integrations from utilizing this endpoint by passing different query information. For example, you can send this query with the enabled query name:
const query = graphql(`
query PaymentWalletWithInitializationDataQuery {
site {
settings {
storeName
}
}
}
`);Passing the header will only prevent sensitive data from coming through, which we do want, but we also don't want this API handler to be open to all queries, just payment wallet specific queries.
There was a problem hiding this comment.
Yes, I suppose my point of view is that, from a security standpoint, enforcement should really happen at the SF API level. The SF API already distinguishes between browser and server-side clients and limits the fields that browser clients can access. What’s currently missing is the ability to recognise another type of client - those that use a server-side proxy to access the API via the browser.
If that distinction is made, then the filter we apply at the proxy layer isn’t really for security purposes, but rather to further narrow what queries/mutations are exposed through Catalyst. Therefore, it doesn’t need to be very granular. It’s sufficient to specify just the top-level nodes like cart, checkout, currency, and payment etc…
That said, you’re right in saying that if there are no restrictions at the SF API level for proxy clients, then we do need to be more granular to avoid accidental leakage of sensitive data. We probably don’t need to group the enabled list by operation names to avoid repetition, since what really matters are the query and mutation field names.
There was a problem hiding this comment.
Only reason why I would leave the operation name is so we could have a consistent set of names we can use for identifying queries (e.g. through otel and our other logging systems) since this is a proxy. If we don't enforce an operation name, you can provide an anonymous query and we won't have any sort of insight what the query is. For example:
query {
site {
settings {
storeName
}
}
}I don't mind if we remove it, but I do see it as a nice-to-have to enforce consistency + a narrower enablement of what is proxied.
There was a problem hiding this comment.
I see, it's mainly to enforce best practices. For that, we just need to ensure that clients provide an operation name, not specific operation names, as we're not aiming to enforce a particular naming convention.
Ideally, we would enforce this validation at the SF GQL API layer, but I suppose we can’t do that anymore since it would be a backward incompatible change.
That said, I still believe the more restrictions we impose at the proxy layer, the harder it will be to roll out changes in the future. For example, if the client code (Checkout SDK) requires a new field in a few months, merchants would need to manually update the Catalyst codebase themselves to modify the whitelist.
We really have two options here to get around this problem:
- Option 1: Use the header approach and delegate the filtering on the server side, minimising the possible changes we have to do on the proxy side in the future.
- Option 2: Go with the library approach, where we ask the merchants to periodically pull updates for the client code and the server code.
core/lib/wallet-buttons/index.ts
Outdated
| private async initCheckoutButtonInitializer() { | ||
| if (!window.checkoutKitLoader) { | ||
| throw new InitializationError(); | ||
| } | ||
|
|
||
| const checkoutButtonModule = await window.checkoutKitLoader.load('headless-checkout-wallet'); | ||
|
|
||
| return checkoutButtonModule.createHeadlessCheckoutWalletInitializer(); | ||
| } |
There was a problem hiding this comment.
One thing that @davidchin and I chatted about was being able to pass in the endpoint location into checkout-sdk in case other headless solutions need to change the path, but keep the logic consistent. We should add that as part of the final solution. For example:
window.checkoutKitLoader.setWalletGqlEndpoint("/wallets/graphql")| const walletButtons = await getPaymentWallets({ | ||
| filters: { | ||
| cartEntityId: cartId, | ||
| }, | ||
| }); | ||
|
|
||
| const walletButtonsInitOptions = await createWalletButtonsInitOptions(walletButtons, cart); |
There was a problem hiding this comment.
Instead of these being waterfall requests, can you see about using our Streamable pattern here?
⚡️🏠 Lighthouse reportLighthouse ran against https://catalyst-canary-884y6zdky-bigcommerce-platform.vercel.app 🖥️ DesktopWe ran Lighthouse against the changes on a desktop and produced this report. Here's the summary:
📱 MobileWe ran Lighthouse against the changes on a mobile and produced this report. Here's the summary:
|
| currencyCode: string; | ||
| }, | ||
| ) => { | ||
| const currencyData = await getCurrencyData(cart.currencyCode); |
There was a problem hiding this comment.
It seems this request can be made concurrently, as the requests below don't depend on the result of this request.
|
|
||
| const ENABLE_LIST = [ | ||
| { | ||
| name: 'PaymentWalletWithInitializationDataQuery', |
There was a problem hiding this comment.
As mentioned earlier (#2298 (comment)), I think we should not enforce specific operation names. Otherwise, the exact operation names become a part of the API contract and we're not free to change in the future. Also, ideally I want to move the filtering logic out of Catalyst to the API layer so the clients won't be coupled to a specific version of Catalyst.
What/Why?
PoC of Headless Wallet Buttons feature
PoC related to checkout-sdk part: bigcommerce/checkout-sdk-js#2855