Skip to content
This repository has been archived by the owner on Apr 11, 2024. It is now read-only.

Allows setting a domain-wide cookie in the oauth flow #905

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Allows setting a domain-wide cookie in the oauth flow #905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3462b36
allows setting a cookie domain for the oauth flow and fixes some cook…
cmelendez Jun 6, 2023
7b2eea3
runs changeset
cmelendez Jun 6, 2023
b39c832
Merge branch 'main' into cookie_paths
cmelendez Jun 12, 2023
be6fd2d
Merge branch 'main' into cookie_paths
cmelendez Jun 15, 2023
0d8385b
Merge branch 'main' into cookie_paths
cmelendez Jul 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/five-peaches-brake.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/shopify-api': patch
---

Allows to set an optional cookieDomain param when calling shopifyApi
7 changes: 7 additions & 0 deletions docs/reference/shopifyApi.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const shopify = shopifyApi({
userAgentPrefix: 'Custom prefix',
privateAppStorefrontAccessToken: 'PrivateAccessToken',
customShopDomains: ['*.my-custom-domain.io'],
cookieDomain: '.example.com',
billing: {
'My plan': {
amount: 5.0,
Expand Down Expand Up @@ -105,6 +106,12 @@ Fixed Storefront API access token for private apps.

Use this if you need to allow values other than `myshopify.com`.

### cookieDomain

`string` | Defaults to `undefined`

Use this if you need to set a domain-wide cookie for the oauth flow.

### billing

`BillingConfig` | Defaults to `undefined`
Expand Down
2 changes: 2 additions & 0 deletions lib/__tests__/config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ describe('Config object', () => {
apiVersion: ApiVersion.Unstable,
isEmbeddedApp: true,
isCustomStoreApp: false,
cookieDomain: '.example.com',
logger: {
log: jest.fn(),
level: LogSeverity.Debug,
Expand All @@ -33,6 +34,7 @@ describe('Config object', () => {
expect(config.apiSecretKey).toEqual(validParams.apiSecretKey);
expect(config.scopes.equals(validParams.scopes)).toBeTruthy();
expect(config.hostName).toEqual(validParams.hostName);
expect(config.cookieDomain).toEqual(validParams.cookieDomain);
});

it("can't initialize with empty values", () => {
Expand Down
8 changes: 5 additions & 3 deletions lib/auth/oauth/oauth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,10 @@ export function begin(config: ConfigInterface) {

await cookies.setAndSign(STATE_COOKIE_NAME, state, {
expires: new Date(Date.now() + 60000),
sameSite: 'lax',
sameSite: 'none',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this required for the domain to work? OAuth should only rely on top-level redirects, and as per the MDN docs:

Lax
Means that the cookie is not sent on cross-site requests, such as on requests to load images or frames, but is sent when a user is navigating to the origin site from an external site (for example, when following a link).

I believe it should still work if we use lax. I ask because using none carries some risk for CSRF. If it is strictly necessary, I think we should only set it to none if cookieDomain is set to help mitigate that risk.

secure: true,
path: callbackPath,
path: '/',
domain: config.cookieDomain,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to fully skip domain if the setting isn't present, otherwise we'll end up with this Set-Cookie string:

Set-Cookie:
shopify_app_state=<state>;sameSite=lax; secure=true; path=/auth/callback; domain=undefined;

});

const query = {
Expand Down Expand Up @@ -197,9 +198,10 @@ export function callback(config: ConfigInterface) {
if (!config.isEmbeddedApp) {
await cookies.setAndSign(SESSION_COOKIE_NAME, session.id, {
expires: session.expires,
sameSite: 'lax',
sameSite: 'none',
secure: true,
path: '/',
domain: config.cookieDomain,
});
}

Expand Down
1 change: 1 addition & 0 deletions lib/base-types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export interface ConfigParams<T extends ShopifyRestResources = any> {
customShopDomains?: (RegExp | string)[];
billing?: BillingConfig;
restResources?: T;
cookieDomain?: string;
logger?: {
log?: LogFunction;
level?: LogSeverity;
Expand Down