Fix: default cookies to HttpOnly, matching the CookieData JSDoc#3239
Open
morgan-coded wants to merge 2 commits into
Open
Fix: default cookies to HttpOnly, matching the CookieData JSDoc#3239morgan-coded wants to merge 2 commits into
morgan-coded wants to merge 2 commits into
Conversation
…opify#3207) Cookies.set (and therefore setAndSign) now applies httpOnly: true by default, matching the long-standing CookieData.httpOnly JSDoc which already documented true as the default. Previously the default was never applied, so the OAuth shopify_app_state cookie (and the non-embedded session cookie) were emitted without the HttpOnly flag, which an OWASP ZAP baseline scan flags. Callers that need client-side JS access can still opt out by explicitly passing httpOnly: false. Adds assertions to the existing OAuth begin/callback tests verifying the state cookie, its signature companion, and the non-embedded session cookie all carry HttpOnly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
cla-needed
devtools-gardener
Author
|
I have signed the CLA! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WHY are these changes introduced?
Fixes #3207
CookieData.httpOnlyis documented as "true by default", butCookies.setnever applies that default — it only spreads the caller's options. No caller in
lib/auth/oauth/oauth.tspasseshttpOnly, so the OAuthshopify_app_statecookie (and the non-embedded session cookie) are emitted without the
HttpOnlyflag. An OWASP ZAP baseline scan flags this as "Cookie No HttpOnlyFlag". These cookies have no legitimate need for client-side JavaScript access.
WHAT is this pull request doing?
Cookies.set(and thereforeCookies.setAndSign) now defaultshttpOnlytotrue, matching the existingCookieData.httpOnlyJSDoc. The default isapplied before spreading caller options, so callers can still opt out by
explicitly passing
httpOnly: false. This fixes every call site at once,including the non-embedded session cookie.
cookie, its
.sigcompanion, and the non-embedded session cookie all carryHttpOnly.Cookies.setdefault, explicit opt-out, andpreservation of other caller options.
No behavior change other than the added flag; existing callers that relied on
the absence of
HttpOnlycan passhttpOnly: false.How was this verified?
jest --selectProjects library --testPathPatterns "oauth\\.test"inpackages/apps/shopify-api: 23/23 pass.(
httpOnlyisundefined); with it applied they pass.eslint .andtsc --noEmit: both clean.Type of change
Checklist
pnpm changesetto create a draft changelog entry