Feature/cross subdomain auth cookies#371
Conversation
diegoMCampos
requested review from
anicioalexandre,
matheusysd and
priscilladeroode
as code owners
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR updates cookie handling functions to support environment-based domain configuration. The ChangesAuth Cookie Domain Configuration
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/utils/functions/token/removeTokenAsync/index.ts`:
- Line 21: The console error message in removeTokenAsync is incorrect; change
the log in the catch block to indicate removal rather than setting. Update the
console.error call inside removeTokenAsync to something like "Failed to remove
token for ${key}:" and include the error object (use the existing key and error
variables) so the message accurately reflects the removeTokenAsync operation.
- Around line 13-18: The removeTokenAsync implementation only forwards domain to
removeCookie which breaks removal when a custom path was used; update
removeTokenAsync to accept the same optional config?: CookieAttributes as
setTokenAsync and forward that full config (including path, domain, and any
other attributes) to removeCookie instead of only passing domain—use the
existing function/removeCookie(key, config) call pattern so removeTokenAsync
mirrors setTokenAsync behavior and ensures cookies set with a custom path are
removed correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d397a787-e1ad-4283-a3b0-6b0da9456f79
📒 Files selected for processing (3)
packages/utils/functions/cookie/index.tspackages/utils/functions/token/removeTokenAsync/index.tspackages/utils/functions/token/setTokenAsync/index.ts
| const cookieDomain = process.env.NEXT_PUBLIC_AUTH_COOKIE_DOMAIN | ||
| if (cookieDomain) { | ||
| removeCookie(key, { domain: cookieDomain }) | ||
| } else { | ||
| removeCookie(key) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if setTokenAsync is called with custom path configurations
# Search for setTokenAsync calls with config parameter
rg -n -B2 -A5 'setTokenAsync\s*\(' --type=ts | rg -A5 'path'
# Search for any cookie path configurations in token-related code
rg -n 'path.*:' --type=ts -g '*token*'
# Check if there are any cookie configurations that specify paths
ast-grep --pattern $'setTokenAsync($_, $_, { $$$ path: $_ $$$ })'Repository: silverlogic/baseapp-frontend
Length of output: 221
🏁 Script executed:
#!/bin/bash
# First, locate token-related files
fd 'token' --type f packages/utils/functions/ 2>/dev/null | head -20
# Check the removeTokenAsync file mentioned in the review
cat -n packages/utils/functions/token/removeTokenAsync/index.ts 2>/dev/null
# Look for setTokenAsync
fd -n setTokenAsync packages/utils/ 2>/dev/null
fd -n setTokenAsync . --type f 2>/dev/null | grep -E '\.(ts|tsx|js)$' | head -10Repository: silverlogic/baseapp-frontend
Length of output: 1368
🏁 Script executed:
#!/bin/bash
# Find setTokenAsync implementation
find packages/utils/functions/token -name 'setTokenAsync' -o -name 'setTokenAsync.ts' 2>/dev/null
# Look for the main setTokenAsync file (not test)
cat -n packages/utils/functions/token/setTokenAsync/index.ts 2>/dev/null
# Check the removeCookie function to understand its signature
find packages/utils -name 'cookie*' -type f 2>/dev/null | grep -v test
cat -n packages/utils/functions/cookie/index.ts 2>/dev/null || cat -n packages/utils/functions/cookie/removeCookie.ts 2>/dev/nullRepository: silverlogic/baseapp-frontend
Length of output: 4676
Pass cookie path to removeCookie to match setTokenAsync behavior.
When removing cookies with js-cookie, both domain and path must match the values used when setting the cookie. setTokenAsync accepts config?: CookieAttributes and passes it to setCookie (including any custom path), but removeTokenAsync only passes domain to removeCookie, ignoring any path that may have been set. This creates an asymmetry where cookies set with a custom path cannot be properly removed.
removeTokenAsync should accept and forward configuration matching setTokenAsync's signature, ensuring path (and other attributes) are preserved during removal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/utils/functions/token/removeTokenAsync/index.ts` around lines 13 -
18, The removeTokenAsync implementation only forwards domain to removeCookie
which breaks removal when a custom path was used; update removeTokenAsync to
accept the same optional config?: CookieAttributes as setTokenAsync and forward
that full config (including path, domain, and any other attributes) to
removeCookie instead of only passing domain—use the existing
function/removeCookie(key, config) call pattern so removeTokenAsync mirrors
setTokenAsync behavior and ensures cookies set with a custom path are removed
correctly.
There was a problem hiding this comment.
Skipped. Verified all current callers of setTokenAsync in the monorepo (refreshAccessToken, useLogin x2, profile/utils, useAuthMiddleware); the only attribute passed is secure, which is not part of the cookie identifier used by Cookies.remove. The current domain-only forward is sufficient for every real call site today and for the cross-subdomain auth use case this PR enables. The reviewer's principle is correct (full symmetry would be needed if any caller used a custom path), but applying now adds an API surface with no concrete consumer. Can revisit when a downstream needs it.
There was a problem hiding this comment.
@diegoMCampos, understood — that's a thorough and well-reasoned audit. Since all current callers only pass secure (which isn't part of the cookie identifier and thus irrelevant to removal), the domain-only forward covers every real case today. Acknowledging the skip.
(ᴗ_ ᴗ。) Noted for future reviews — if a caller ever introduces a custom path, full config forwarding will be needed at that point.
✏️ Learnings added
Learnt from: diegoMCampos
Repo: silverlogic/baseapp-frontend PR: 371
File: packages/utils/functions/token/removeTokenAsync/index.ts:13-18
Timestamp: 2026-05-11T14:05:39.718Z
Learning: In `packages/utils/functions/token/removeTokenAsync/index.ts` (silverlogic/baseapp-frontend), the decision to forward only `domain` (not the full `CookieAttributes` config) to `removeCookie` was deliberate. All current callers of `setTokenAsync` (refreshAccessToken, useLogin x2, profile/utils, useAuthMiddleware) only pass the `secure` attribute, which is not part of the cookie identifier used by `Cookies.remove` and therefore does not affect removal correctness. Full config forwarding (e.g., custom `path`) was deferred until a downstream consumer actually needs it.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| } | ||
| } | ||
| } catch (error) { | ||
| console.error(`Failed to set token for ${key}:`, error) |
There was a problem hiding this comment.
Fix the error message.
The error message states "Failed to set token" but this function removes tokens, not sets them.
📝 Proposed fix
- console.error(`Failed to set token for ${key}:`, error)
+ console.error(`Failed to remove token for ${key}:`, error)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.error(`Failed to set token for ${key}:`, error) | |
| console.error(`Failed to remove token for ${key}:`, error) |
🧰 Tools
🪛 GitHub Check: Build and Lint Packages
[warning] 21-21:
Unexpected console statement
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/utils/functions/token/removeTokenAsync/index.ts` at line 21, The
console error message in removeTokenAsync is incorrect; change the log in the
catch block to indicate removal rather than setting. Update the console.error
call inside removeTokenAsync to something like "Failed to remove token for
${key}:" and include the error object (use the existing key and error variables)
so the message accurately reflects the removeTokenAsync operation.
skipped. I'm don't touch pre-existing code in the baseapp-frontend submodule, I'm keep the PR strictly to what our work introduced |
Janekk
force-pushed
the
feature/cross-subdomain-auth-cookies
branch
from
c145193 to
fa05632
Compare
|
@anicioalexandre I think I need yout input here, if someone ever calls I know that today no current caller does this but I think is nice to have this working if needed in the future and it's something we expose also can we have tests for this new scenario? we have for the default scenario so would be nice to have it, and would be nice to document this new |
Janekk
force-pushed
the
feature/cross-subdomain-auth-cookies
branch
from
fa05632 to
c78df3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/utils/functions/token/removeTokenAsync/index.ts (1)
21-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCorrect the error message.
The error message states "Failed to set token" but this function removes tokens.
📝 Proposed fix
- console.error(`Failed to set token for ${key}:`, error) + console.error(`Failed to remove token for ${key}:`, error)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/utils/functions/token/removeTokenAsync/index.ts` at line 21, The error message in removeTokenAsync is wrong — it currently says "Failed to set token" while the function removes tokens; update the console.error call inside removeTokenAsync to say "Failed to remove token for ${key}:" (or equivalent wording) so the log accurately reflects the operation performed by the removeTokenAsync function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/utils/functions/token/removeTokenAsync/index.ts`:
- Around line 13-18: removeTokenAsync is asymmetric with setTokenAsync: it
always uses NEXT_PUBLIC_AUTH_COOKIE_DOMAIN instead of honoring a caller-provided
domain, so removing cookies written with a custom domain fails; update
removeTokenAsync to accept an optional config parameter (mirror setTokenAsync)
and use config?.domain if provided, otherwise fall back to
process.env.NEXT_PUBLIC_AUTH_COOKIE_DOMAIN, then pass that domain into
removeCookie(key, { domain }) (or removeCookie(key) when none) so removals match
how cookies were set; reference functions: removeTokenAsync, setTokenAsync,
removeCookie, and env var NEXT_PUBLIC_AUTH_COOKIE_DOMAIN.
---
Duplicate comments:
In `@packages/utils/functions/token/removeTokenAsync/index.ts`:
- Line 21: The error message in removeTokenAsync is wrong — it currently says
"Failed to set token" while the function removes tokens; update the
console.error call inside removeTokenAsync to say "Failed to remove token for
${key}:" (or equivalent wording) so the log accurately reflects the operation
performed by the removeTokenAsync function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c6a4dc5d-0a0a-420f-8818-955a7ffd832b
📒 Files selected for processing (3)
packages/utils/functions/cookie/index.tspackages/utils/functions/token/removeTokenAsync/index.tspackages/utils/functions/token/setTokenAsync/index.ts
| const cookieDomain = process.env.NEXT_PUBLIC_AUTH_COOKIE_DOMAIN | ||
| if (cookieDomain) { | ||
| removeCookie(key, { domain: cookieDomain }) | ||
| } else { | ||
| removeCookie(key) | ||
| } |
There was a problem hiding this comment.
Removal asymmetry when caller provides custom domain.
setTokenAsync allows callers to provide a custom domain in the config that takes precedence over NEXT_PUBLIC_AUTH_COOKIE_DOMAIN (line 55 in setTokenAsync: !config?.domain check). However, removeTokenAsync always uses the env var domain when set and has no way to accept a caller-specified domain.
This creates an asymmetry: if a caller sets a token with a custom domain that differs from the env var, removal will fail because js-cookie requires the domain to match for removal.
Example scenario:
// NEXT_PUBLIC_AUTH_COOKIE_DOMAIN = '.example.com'
setTokenAsync('auth', 'token', { domain: 'custom.example.com' })
// Cookie written with domain=custom.example.com
removeTokenAsync('auth')
// Attempts removal with domain=.example.com
// Cookie remains because domain doesn't matchPer your audit, no current callers pass custom domain (only secure), so this doesn't affect existing code. However, the API allows it, creating a footgun for future callers.
Consider either:
- Documenting that custom domains in
setTokenAsyncare unsupported when the env var is set - Accepting an optional
configparameter inremoveTokenAsyncto mirrorsetTokenAsync(would need to store or pass the domain somehow) - Warning/erroring in
setTokenAsyncwhen caller provides a domain that differs from env var
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/utils/functions/token/removeTokenAsync/index.ts` around lines 13 -
18, removeTokenAsync is asymmetric with setTokenAsync: it always uses
NEXT_PUBLIC_AUTH_COOKIE_DOMAIN instead of honoring a caller-provided domain, so
removing cookies written with a custom domain fails; update removeTokenAsync to
accept an optional config parameter (mirror setTokenAsync) and use
config?.domain if provided, otherwise fall back to
process.env.NEXT_PUBLIC_AUTH_COOKIE_DOMAIN, then pass that domain into
removeCookie(key, { domain }) (or removeCookie(key) when none) so removals match
how cookies were set; reference functions: removeTokenAsync, setTokenAsync,
removeCookie, and env var NEXT_PUBLIC_AUTH_COOKIE_DOMAIN.
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated project dependencies for improved stability and maintenance. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/silverlogic/baseapp-frontend/pull/376?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pnpm-workspace.yaml`:
- Line 39: Update the PR description to explicitly state why js-cookie was
bumped to "js-cookie: 3.0.7" — indicate that this is the patched release
addressing the HIGH-severity prototype-hijack vulnerability affecting versions
<=3.0.5, cite the upstream release/changelog or CVE reference, confirm upstream
notes that there are no breaking changes for 3.0.7, and note whether the bump
was also intended to support cross-subdomain cookie domain behavior; if
uncertain, verify the changelog or tests and state that the bump is solely a
security fix.
- Line 209: You added qs: ^6.15.2 to pnpm-workspace.yaml but didn’t justify it;
update the PR description and commit message to state that this bump is to
address the GitHub-reported MODERATE DoS vulnerability in qs.stringify (fixed in
6.15.2 for the arrayFormat:'comma' + encodeValuesOnly case where comma arrays
may contain null/undefined), confirm there are no breaking changes per the
6.15.0–6.15.2 changelog, and state whether you intend to allow future 6.x minors
(keep ^6.15.2) or prefer to pin an exact range (e.g., =6.15.2) and change the
pnpm-workspace.yaml entry "qs: ^6.15.2" accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a1f0455-4260-4e6d-9c46-d6cb6812c676
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
pnpm-workspace.yaml
| axios: 1.15.2 | ||
| jotai: 2.12.4 | ||
| js-cookie: 3.0.5 | ||
| js-cookie: 3.0.7 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check npm registry for js-cookie versions and security info
echo "=== Checking js-cookie versions ==="
npm view js-cookie versions --json | jq -r '.[]' | grep -E '^3\.0\.(5|6|7)$'
echo -e "\n=== Checking js-cookie 3.0.7 details ==="
npm view js-cookie@3.0.7 version time
echo -e "\n=== Checking GitHub security advisories ==="
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: NPM, package: "js-cookie") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Repository: silverlogic/baseapp-frontend
Length of output: 2021
🌐 Web query:
js-cookie 3.0.7 changelog release notes breaking changes
💡 Result:
The js-cookie version 3.0.7 was released on May 16, 2026 [1][2]. There are no breaking changes introduced in this release [1]. The release primarily focuses on security and maintenance updates [1]: - Security Fix: It addresses a vulnerability (CVE-2026-46625) involving per-instance prototype hijacking in the internal assign helper, which could have allowed cookie-attribute injection [1][3]. - Maintenance: It includes documentation updates by adding the Partitioned attribute to the readme [1], ensures consistent behavior for get('name') and get calls [1], and updates the publishing process to use trusted publishing to the npm registry [1]. As this is a patch release, it is intended to be backward-compatible with previous 3.0.x versions [1][4].
Citations:
- 1: https://github.com/js-cookie/js-cookie/releases/tag/v3.0.7
- 2: https://www.npmjs.com/package/js-cookie
- 3: GHSA-qjx8-664m-686j
- 4: https://security.snyk.io/package/npm/js-cookie
Clarify why js-cookie is bumped to 3.0.7 (likely security patch)
js-cookie3.0.7 is the first patched version for a HIGH severity prototype-hijack vulnerability affecting<= 3.0.5; upstream release notes indicate no breaking changes for this patch release.- PR should still document whether the bump is specifically to address this security fix and/or to support the cross-subdomain cookie domain behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pnpm-workspace.yaml` at line 39, Update the PR description to explicitly
state why js-cookie was bumped to "js-cookie: 3.0.7" — indicate that this is the
patched release addressing the HIGH-severity prototype-hijack vulnerability
affecting versions <=3.0.5, cite the upstream release/changelog or CVE
reference, confirm upstream notes that there are no breaking changes for 3.0.7,
and note whether the bump was also intended to support cross-subdomain cookie
domain behavior; if uncertain, verify the changelog or tests and state that the
bump is solely a security fix.
f52a78e to
d0d25ec
Compare
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated axios HTTP client library to version 1.16.0 <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
](https://app.coderabbit.ai/change-stack/silverlogic/baseapp-frontend/pull/376?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
… feature/cross-subdomain-auth-cookies
|
7 participants
feat(utils): cross-subdomain cookie support for auth tokens
Summary
Adds opt-in support for storing and clearing auth tokens with a configurable cookie domain, so a single login session can be shared across sibling subdomains (e.g. app.example.com ↔ embed.example.com). Driven entirely by a new env var; when unset, behavior is identical to the previous
version.
Motivation
Before this change, setTokenAsync / removeTokenAsync had no way to attach a domain attribute to the cookies they manage. As a result, the auth token was always host-scoped — a user logged in on app.example.com would not be authenticated on embed.example.com, and any iframe widget hosted
on a sibling subdomain required a separate login.
This is a recurring need for products that embed widgets/iframes under a different subdomain of the same parent domain.
Changes
When unset, calls removeCookie(key) exactly as before — preserving call signatures asserted by existing tests.
'remove', key }) event continues to fire either way.
Backwards compatibility
How to enable cross-subdomain auth in a downstream app
Set the env var in the consumer app's deploys:
NEXT_PUBLIC_AUTH_COOKIE_DOMAIN=.example.com
Login will then write Authorization / Refresh cookies with Domain=.example.com, and logout will clear them on the same scope. Any sibling *.example.com host can read the session without a second login.
For local testing, the consumer app can wire this up against *.localhost (works without /etc/hosts edits — browsers auto-resolve *.localhost to 127.0.0.1).
Test plan
Summary by CodeRabbit