Skip to content

fix: include missing optionalKeys when building config from env - #51

Open
DrunkOldDog wants to merge 2 commits into
workos:mainfrom
DrunkOldDog:fix/workos-cookie-domain-env
Open

fix: include missing optionalKeys when building config from env#51
DrunkOldDog wants to merge 2 commits into
workos:mainfrom
DrunkOldDog:fix/workos-cookie-domain-env

Conversation

@DrunkOldDog

@DrunkOldDog DrunkOldDog commented Jul 23, 2026

Copy link
Copy Markdown

Summary

Closes the config gap reported in workos/authkit-tanstack-start#114. Setting WORKOS_COOKIE_DOMAIN (and the related optional env vars) had no effect unless callers also passed the value through configure().

getConfig() built its key set from:

  • keys already present on the default config object (cookieName, apiHttps, cookieMaxAge, apiHostname)
  • requiredKeys (clientId, apiKey, redirectUri, cookiePassword)

Optional keys with no defaultscookieDomain, cookieSameSite, apiPort — were never included, so their WORKOS_* env vars were never probed. Calling configure({ cookieDomain }) hid the bug by putting the key on this.config via updateConfig.

This is why @workos-inc/authkit-nextjs was unaffected (it reads env vars through its own layer), while adapters that rely on @workos/authkit-session (TanStack Start and others) ignored WORKOS_COOKIE_DOMAIN and wrote session cookies without a Domain attribute.

Changes

  • Add optionalKeys (apiPort, cookieSameSite, cookieDomain) and include them in the getConfig() key set
  • Document WORKOS_COOKIE_DOMAIN / WORKOS_COOKIE_SAME_SITE on the corresponding AuthKitConfig fields
  • Add a regression test that sets those values only via the env value source (no configure() for those keys), so updateConfig cannot mask the failure

Test plan

  • pnpm test src/core/config/ConfigurationProvider.spec.ts — optional env vars are read without configure()
  • Optional keys remain absent from getConfig() when unset
  • Existing required-key / default behavior unchanged
  • Manual: set WORKOS_COOKIE_DOMAIN in a TanStack Start app, sign in, confirm Set-Cookie includes Domain=

@DrunkOldDog
DrunkOldDog marked this pull request as ready for review July 23, 2026 03:19
@greptile-apps

greptile-apps Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a configuration gap where optional AuthKitConfig keys with no defaults (cookieDomain, cookieSameSite, apiPort) were never included in the key set iterated by getConfig(), causing their WORKOS_* environment variables to be silently ignored in adapters that rely on authkit-session (e.g. TanStack Start).

  • Adds an optionalKeys array to ConfigurationProvider and spreads it into the allKeys Set in getConfig(), ensuring the three previously-invisible env vars are now probed on every config build.
  • Adds a targeted regression test that supplies all three optional values exclusively via the value source (no configure() call), preventing updateConfig from masking the failure again.
  • Documents WORKOS_COOKIE_SAME_SITE and WORKOS_COOKIE_DOMAIN in the corresponding JSDoc on AuthKitConfig.

Confidence Score: 4/5

Safe to merge; the fix is small, correct, and backed by a good regression test — the only note is that WORKOS_COOKIE_SAME_SITE values coming from the environment are not validated against the allowed union at runtime.

The core change is a two-line addition that unblocks a real, reproducible bug. The regression test is well-constructed and directly targets the failure mode. The only gap is that convertValueType does not validate cookieSameSite env-var strings against the union type, so a misconfigured env var would propagate silently — a quality concern rather than a breaking defect.

src/core/config/ConfigurationProvider.ts — specifically the convertValueType method, which could benefit from a runtime guard on the cookieSameSite branch now that this value is read from the environment.

Important Files Changed

Filename Overview
src/core/config/ConfigurationProvider.ts Adds optionalKeys array and includes it in getConfig()'s key set — correct, targeted fix; minor: cookieSameSite env var value has no runtime validation against its union type
src/core/config/ConfigurationProvider.spec.ts Adds regression test that reads all three optional env vars solely via the value source (no configure() call), and adds env-var-name assertions for the two new keys
src/core/config/types.ts Adds 'Equivalent to WORKOS_COOKIE_SAME_SITE / WORKOS_COOKIE_DOMAIN' JSDoc on the two newly-activated env vars

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[getConfig called] --> B[Build allKeys Set]
    B --> C[Object.keys this.config\ncookieName, apiHttps, cookieMaxAge, apiHostname]
    B --> D[requiredKeys\nclientId, apiKey, redirectUri, cookiePassword]
    B --> E[optionalKeys NEW\napiPort, cookieSameSite, cookieDomain]
    C & D & E --> F[Iterate allKeys]
    F --> G[getValue key]
    G --> H{Env var set?}
    H -- yes --> I[convertValueType]
    H -- no --> J{config key set?}
    J -- yes --> I
    J -- no --> K{Required key?}
    K -- yes --> L[throw Error]
    K -- no --> M[return undefined\nskip key in fullConfig]
    I --> N[Set on fullConfig]
    N & M --> O[Return fullConfig]
Loading

Comments Outside Diff (1)

  1. src/core/config/ConfigurationProvider.ts, line 114-134 (link)

    P2 No runtime validation for cookieSameSite env var value

    convertValueType doesn't validate that WORKOS_COOKIE_SAME_SITE is one of 'strict' | 'lax' | 'none'. Before this PR the env var was silently ignored; now it takes effect, meaning an invalid value like "Strict" or "invalid" will be set on the config object and propagated to the cookie without error. The type guard is purely compile-time and provides no protection when the value comes from the environment.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: src/core/config/ConfigurationProvider.ts
    Line: 114-134
    
    Comment:
    **No runtime validation for `cookieSameSite` env var value**
    
    `convertValueType` doesn't validate that `WORKOS_COOKIE_SAME_SITE` is one of `'strict' | 'lax' | 'none'`. Before this PR the env var was silently ignored; now it takes effect, meaning an invalid value like `"Strict"` or `"invalid"` will be set on the config object and propagated to the cookie without error. The type guard is purely compile-time and provides no protection when the value comes from the environment.
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
src/core/config/ConfigurationProvider.ts:114-134
**No runtime validation for `cookieSameSite` env var value**

`convertValueType` doesn't validate that `WORKOS_COOKIE_SAME_SITE` is one of `'strict' | 'lax' | 'none'`. Before this PR the env var was silently ignored; now it takes effect, meaning an invalid value like `"Strict"` or `"invalid"` will be set on the config object and propagated to the cookie without error. The type guard is purely compile-time and provides no protection when the value comes from the environment.

Reviews (1): Last reviewed commit: "test: add optional env vars test" | Re-trigger Greptile

@nicknisi nicknisi self-assigned this Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants