Skip to content

[snack-sdk][website] Move to cookie-based auth#685

Merged
byronkarlen merged 2 commits into
mainfrom
byron/_snack-sdk_website_move_to_cookie-based_auth
May 20, 2026
Merged

[snack-sdk][website] Move to cookie-based auth#685
byronkarlen merged 2 commits into
mainfrom
byron/_snack-sdk_website_move_to_cookie-based_auth

Conversation

@byronkarlen
Copy link
Copy Markdown
Contributor

@byronkarlen byronkarlen commented May 8, 2026

Why

Remove client-side reads of the session cookie. Rely on credentials: 'include' instead.

How

snack-sdk

  • added a useCookieAuth option that defaults to false for existing consumers

website

  • sets snack-sdk's useCookieAuth to true
  • refactored authManager to get rid of legacy auth code, and use credentials: 'include' with direct fetches to website

To make these changes, I had claude audit all of website's requests (either direct or through snack-sdk) to the API server. Then, I had it determine which of these endpoints needed potentially authenticated contexts and added support for credentials: 'include'.

Test Plan

Tested out auth flow locally against local instance of website and staging api. Not super familiar with snack so I'm not confident my QA was comprehensive.

Copy link
Copy Markdown
Contributor Author

byronkarlen commented May 8, 2026

@byronkarlen byronkarlen changed the title [snack-sdk] Move to cookie-based auth [snack-sdk][website] Move to cookie-based auth May 8, 2026
@byronkarlen byronkarlen force-pushed the byron/_snack-sdk_website_snackager_migrate_api_server_hostname branch from d6e1968 to add7f33 Compare May 8, 2026 23:00
@byronkarlen byronkarlen force-pushed the byron/_snack-sdk_website_move_to_cookie-based_auth branch 2 times, most recently from 74a8bd9 to f6cb001 Compare May 11, 2026 00:45
@byronkarlen byronkarlen force-pushed the byron/_snack-sdk_website_snackager_migrate_api_server_hostname branch from a4eba5b to 87fd82b Compare May 11, 2026 15:40
@byronkarlen byronkarlen force-pushed the byron/_snack-sdk_website_move_to_cookie-based_auth branch from f6cb001 to 94be0a5 Compare May 11, 2026 15:40
@byronkarlen byronkarlen force-pushed the byron/_snack-sdk_website_snackager_migrate_api_server_hostname branch from 87fd82b to 0d0006d Compare May 11, 2026 16:27
@byronkarlen byronkarlen force-pushed the byron/_snack-sdk_website_move_to_cookie-based_auth branch from 94be0a5 to fed083a Compare May 11, 2026 16:27
@byronkarlen byronkarlen force-pushed the byron/_snack-sdk_website_snackager_migrate_api_server_hostname branch from 0d0006d to dd8021d Compare May 11, 2026 16:35
@byronkarlen byronkarlen force-pushed the byron/_snack-sdk_website_move_to_cookie-based_auth branch 2 times, most recently from 7985a81 to ea4bde3 Compare May 11, 2026 17:26
@byronkarlen byronkarlen force-pushed the byron/_snack-sdk_website_snackager_migrate_api_server_hostname branch from dd8021d to 716cdc7 Compare May 11, 2026 17:26
@byronkarlen byronkarlen force-pushed the byron/_snack-sdk_website_move_to_cookie-based_auth branch 2 times, most recently from 1e0ddfd to 9695d45 Compare May 11, 2026 18:48
@byronkarlen byronkarlen marked this pull request as ready for review May 11, 2026 18:56
@byronkarlen byronkarlen requested a review from byCedric as a code owner May 11, 2026 18:56
@byronkarlen byronkarlen requested a review from ide May 11, 2026 18:57
Comment thread packages/snack-sdk/src/DevSession.ts Outdated
Comment on lines +38 to +39
const isCloseUser = this.useCookieAuth
? prevState.online && (!state.online || state.url !== prevState.url)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure this is right - would state.online be set in the anonymous scenario? Looking at the this.close() call below, we don't send the user anyway when using cookies.

I think the current code works but it ends up being harder to read because of the history of how we ended up here.

Logically it's like we want:

if (useCookieAuth) {
  // cookie-specific logic here...
  this.close(...)
} else {
  // old logic here...
  this.close(...)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes state.online could be set in the anonymous scenario. Agreed confusing. I just changed it to be clearer. The logic is the same though.

@byronkarlen byronkarlen force-pushed the byron/_snack-sdk_website_move_to_cookie-based_auth branch from 9695d45 to 9b3d500 Compare May 11, 2026 23:08
@byronkarlen byronkarlen requested a review from ide May 11, 2026 23:10
@byronkarlen byronkarlen force-pushed the byron/_snack-sdk_website_snackager_migrate_api_server_hostname branch from 716cdc7 to 065b7a7 Compare May 12, 2026 18:43
@byronkarlen byronkarlen force-pushed the byron/_snack-sdk_website_move_to_cookie-based_auth branch 2 times, most recently from 8a144da to 6d5e6b9 Compare May 18, 2026 19:00
@byronkarlen byronkarlen force-pushed the byron/_snack-sdk_website_snackager_migrate_api_server_hostname branch from 065b7a7 to 9a2aa15 Compare May 18, 2026 20:19
@byronkarlen byronkarlen force-pushed the byron/_snack-sdk_website_move_to_cookie-based_auth branch from 6d5e6b9 to 73ac58a Compare May 18, 2026 20:19
@byronkarlen byronkarlen changed the base branch from byron/_snack-sdk_website_snackager_migrate_api_server_hostname to graphite-base/685 May 20, 2026 18:37
@byronkarlen byronkarlen force-pushed the byron/_snack-sdk_website_move_to_cookie-based_auth branch from 73ac58a to 3ab9305 Compare May 20, 2026 18:38
@graphite-app graphite-app Bot changed the base branch from graphite-base/685 to main May 20, 2026 18:38
@byronkarlen byronkarlen force-pushed the byron/_snack-sdk_website_move_to_cookie-based_auth branch from 3ab9305 to 9c00c29 Compare May 20, 2026 18:38
@byronkarlen byronkarlen merged commit c941cf4 into main May 20, 2026
23 checks passed
@byronkarlen byronkarlen deleted the byron/_snack-sdk_website_move_to_cookie-based_auth branch May 20, 2026 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants