Skip to content

Feature/cross subdomain auth cookies #371

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
diegoMCampos wants to merge 30 commits into
base: master
Choose a base branch
from
Open

Feature/cross subdomain auth cookies #371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
09a06a9
feature/cross-subdomain-auth-cookies
diegoMCampos May 7, 2026
c78df3b
fix: improve cookie removal logic in removeTokenAsync function
diegoMCampos May 9, 2026
d0d25ec
Update dependencies because of vulnerability scan (#376)
Ronan-Fernandes May 25, 2026
6b12660
hotfix: update deps (#379)
anicioalexandre Jun 2, 2026
f126532
Merge branch 'master' of github.com:silverlogic/baseapp-frontend into…
marcelopessini Jun 15, 2026
ebb7c6e
Fix vulnerabilities
marcelopessini Jun 15, 2026
2915bd2
Update versions to prevent vulnerabilities
marcelopessini Jun 17, 2026
2188433
Fix vulnerabilities
marcelopessini Jun 17, 2026
9a9d09e
update axios from 1.16.0 to 1.18.0
marcelopessini Jul 21, 2026
c999df9
update libraries to prevent vulnerabilities
marcelopessini Jul 21, 2026
0fb5409
update libraries
marcelopessini Jul 21, 2026
1ac8e77
fix vulnerabilities
marcelopessini Jul 22, 2026
1e43467
Update next
marcelopessini Jul 23, 2026
c4315ba
update libraries
marcelopessini Jul 27, 2026
588b391
updade libraries
marcelopessini Jul 27, 2026
a251279
fix vulnerabilities
marcelopessini Aug 4, 2026
d884d65
Fix vulnerabilities on js-yaml
marcelopessini Aug 7, 2026
a4618a3
Fix vulnerabilities
marcelopessini Aug 11, 2026
6b1fe47
Fix vulnerabilities
marcelopessini Aug 14, 2026
a58547b
Auto redirect when click on itens with sub items on nav bar
marcelopessini Aug 20, 2026
716432f
update decode-uri-component to prevent vulnerabilities
marcelopessini Sep 1, 2026
2d1ddbe
fix vulnerabilities
marcelopessini Sep 4, 2026
867efd9
Fix vulnerabilities
marcelopessini Sep 4, 2026
aeadae0
Fix vulnerabilities
marcelopessini Sep 9, 2026
32654d1
fix vulnerabilities
marcelopessini Sep 30, 2026
1da7914
update axios
marcelopessini Oct 1, 2026
1792200
Update libraries
marcelopessini Oct 6, 2026
8c0bf6e
update libraries
marcelopessini Oct 6, 2026
ca066b6
update next
marcelopessini Oct 8, 2026
a1cac6b
update libraries
marcelopessini Oct 15, 2026
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
24 changes: 19 additions & 5 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,26 @@
},
"pnpm": {
"overrides": {
"@xmldom/xmldom": ">=0.9.10",
"brace-expansion@^5.0.0": ">=5.0.6",
"postcss": ">=8.5.10",
"shell-quote": ">=1.8.4",
"@xmldom/xmldom": ">=0.9.12",
"brace-expansion@^5.0.0": ">=5.0.12",
"brace-expansion@^1.1.0": ">=1.1.21 <2",
"postcss": ">=8.5.18",
"shell-quote": ">=1.11.0",
"uuid": ">=14.0.0",
"ws@^8.0.0": ">=8.20.1"
"ws@^8.0.0": ">=8.21.0",
"ws@^7.0.0": ">=7.5.11 <8",
"esbuild": ">=0.28.1",
"form-data": "4.0.6",
"js-yaml": "4.3.2",
"sharp": "0.35.5",
"nanoid": "3.3.18",
"decode-uri-component": "0.5.0",
"browserslist": "4.28.7",
"qs": ">=6.16.0",
"axios": "1.20.0",
"source-map-js": "1.2.2",
"compression": "1.8.2",
"image-size": "2.0.3"
},
"patchedDependencies": {
"@cypress/request@3.0.10": ".patches/@cypress+request@3.0.10.patch"
Expand Down
10 changes: 10 additions & 0 deletions packages/components/modules/navigations/web/NavMini/NavSectionMini/NavList/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,21 @@ const NavList: FC<NavListProps> = ({ data, depth, slotProps }) => {
}
}, [data.children])

// Children are revealed on hover here, so there is no open transition to gate on: a
// parent click just navigates. Leaves are left to their own <Link>.
const handleClick = useCallback(() => {
if (!data.children) {
return
}
data.onClick?.()
}, [data])

return (
<>
<NavItem
ref={navRef}
open={openMenu}
onClick={handleClick}
onMouseEnter={handleOpenMenu}
onMouseLeave={handleCloseMenu}
itemData={data}
Expand Down
12 changes: 9 additions & 3 deletions .../components/modules/navigations/web/__shared__/NavSectionVertical/Group/NavList/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,16 @@ const NavList: FC<NavListProps> = ({ data, depth, slotProps }) => {
}, [pathname])

const handleToggleMenu = useCallback(() => {
if (data.children) {
setOpenMenu((prev) => !prev)
if (!data.children) {
return
}
}, [data.children])
// Navigate on the open transition only: collapsing a group must not pull the user off
// the child page they are currently on. Leaves are left to their own <Link>.
if (!openMenu) {
data.onClick?.()
}
setOpenMenu((prev) => !prev)
}, [data, openMenu])

return (
<>
Expand Down
1 change: 1 addition & 0 deletions packages/components/modules/navigations/web/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export type NavItemBaseProps = {
disabled?: boolean
externalLink?: boolean
children?: NavItemChildren[]
onClick?: () => void
}

export type NavItemProps = ListItemButtonProps &
Expand Down
8 changes: 6 additions & 2 deletions packages/utils/functions/cookie/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,19 @@
ClientCookies.set(key, formattedValue, config)
dispatchCookieChange({ type: 'set', key, value: formattedValue })
} catch (error) {
console.error(error)

Check warning on line 41 in packages/utils/functions/cookie/index.ts

View workflow job for this annotation

GitHub Actions / Build and Lint Packages 

Unexpected console statement
}
}

export const removeCookie = (key: string) => {
export const removeCookie = (key: string, options?: SetCookieOptions) => {
try {
ClientCookies.remove(key)
if (options) {
ClientCookies.remove(key, options)
} else {
ClientCookies.remove(key)
}
dispatchCookieChange({ type: 'remove', key })
} catch (error) {
console.error(error)

Check warning on line 54 in packages/utils/functions/cookie/index.ts

View workflow job for this annotation

GitHub Actions / Build and Lint Packages 

Unexpected console statement
}
}
7 changes: 6 additions & 1 deletion packages/utils/functions/token/removeTokenAsync/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,14 @@
// logged-out state propagates immediately on mobile.
emitCookieChange({ type: 'remove', key })
} else {
removeCookie(key)
const cookieDomain = process.env.NEXT_PUBLIC_AUTH_COOKIE_DOMAIN
if (cookieDomain) {
removeCookie(key, { domain: cookieDomain })
} else {
removeCookie(key)
}
Comment on lines +17 to +22

@coderabbitai coderabbitai Bot May 11, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

🧩 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 -10

Repository: 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/null

Repository: 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.

@diegoMCampos diegoMCampos May 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

@coderabbitai coderabbitai Bot May 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@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.

Comment on lines +17 to +22

@coderabbitai coderabbitai Bot May 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

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 match

Per 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:

  1. Documenting that custom domains in setTokenAsync are unsupported when the env var is set
  2. Accepting an optional config parameter in removeTokenAsync to mirror setTokenAsync (would need to store or pass the domain somehow)
  3. Warning/erroring in setTokenAsync when 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.

}
} catch (error) {
console.error(`Failed to set token for ${key}:`, error)

Check warning on line 25 in packages/utils/functions/token/removeTokenAsync/index.ts

View workflow job for this annotation

GitHub Actions / Build and Lint Packages 

Unexpected console statement

@coderabbitai coderabbitai Bot May 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

Suggested change
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.

}
}
5 changes: 4 additions & 1 deletion packages/utils/functions/token/setTokenAsync/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,12 @@
// like `useJWTUser` see the new token without waiting for an app restart.
emitCookieChange({ type: 'set', key, value })
} else {
setCookie(key, value, config)
const cookieDomain = process.env.NEXT_PUBLIC_AUTH_COOKIE_DOMAIN
const mergedConfig =
cookieDomain && !config?.domain ? { ...config, domain: cookieDomain } : config
setCookie(key, value, mergedConfig)
}
} catch (error) {
console.error(`Failed to set token for ${key}:`, error)

Check warning on line 63 in packages/utils/functions/token/setTokenAsync/index.ts

View workflow job for this annotation

GitHub Actions / Build and Lint Packages 

Unexpected console statement
}
}
Loading
Loading