Conversation
Resolves #3296
|
Important Review skippedAuto reviews are disabled on this repository. To trigger a review, include You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/cookies.ts (1)
44-47: Add explicit return type annotation.Per coding guidelines, top-level module functions should declare their return types. Consider adding a type annotation for clarity and maintainability.
Proposed fix
-export const getDefaultAttributes = () => ({ +export const getDefaultAttributes = (): Cookies.CookieAttributes => ({ path: '/', secure: config.app.protocol === 'https', });As per coding guidelines: "When declaring functions on the top-level of a module, declare their return types."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/cookies.ts` around lines 44 - 47, Update the top-level function getDefaultAttributes to include an explicit return type annotation (e.g., : { path: string; secure: boolean }) instead of relying on type inference; modify the function signature for getDefaultAttributes to declare that it returns an object with path: string and secure: boolean so the intent and shape are explicit and adhere to the module-level typing guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nextjs/getServerSideProps/handlers.ts`:
- Line 45: The manual Set-Cookie construction at res.setHeader using
cookies.NAMES.UUID and config.app.protocol can emit trailing semicolons/spaces
and diverges from the middleware's getDefaultAttributes behavior; replace this
manual header build by using the shared cookie-attribute helper (either call
cookiesLib.getDefaultAttributes() or add a helper in lib/cookies.ts that returns
a Set-Cookie string for a name/value using the same defaults) and use that
helper to set the cookie for UUID so attributes (Secure, SameSite, Path, etc.)
are consistent and no trailing characters are produced.
In `@nextjs/middlewares/appProfile.ts`:
- Around line 16-18: The cookie is set using res.cookies.set with attributes
from cookiesLib.getDefaultAttributes() but deleted without attributes, which can
prevent proper removal; update the deletion to pass the same attributes so the
name and path match (i.e., call res.cookies.delete with
cookiesLib.NAMES.APP_PROFILE and cookiesLib.getDefaultAttributes()) and ensure
you reference the same attributes used when setting the cookie
(cookiesLib.getDefaultAttributes()) alongside the existing res.cookies.set and
PRIVATE_PROFILE_VALUE usage.
---
Nitpick comments:
In `@lib/cookies.ts`:
- Around line 44-47: Update the top-level function getDefaultAttributes to
include an explicit return type annotation (e.g., : { path: string; secure:
boolean }) instead of relying on type inference; modify the function signature
for getDefaultAttributes to declare that it returns an object with path: string
and secure: boolean so the intent and shape are explicit and adhere to the
module-level typing guideline.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
configs/app/app.tslib/cookies.tsnextjs/getServerSideProps/handlers.tsnextjs/middlewares/addressFormat.tsnextjs/middlewares/appProfile.tsnextjs/middlewares/colorTheme.tsnextjs/middlewares/poorReputationTokens.tsnextjs/middlewares/scamTokens.ts
Description and Related Issue(s)
Resolves #3296
Adds the
Securecookie attribute for all app cookies when the app is served over HTTPS, so cookies are only sent over secure connections and are not exposed on HTTP.Proposed Changes
lib/cookies.ts: IntroducedgetDefaultAttributes()returning{ path: '/', secure: config.app.protocol === 'https' }. Allset()andremove()calls now use these defaults (with optional overrides).configs/app/app.ts: Defaultprotocolto'https'whenNEXT_PUBLIC_APP_PROTOCOLis unset, so the Secure flag is used by default in production.{ path: '/' }withcookiesLib.getDefaultAttributes()inaddressFormat,appProfile,colorTheme,poorReputationTokens, andscamTokensmiddlewares. IngetServerSideProps/handlers.ts, the UUIDSet-Cookieheader now includesPath=/andSecurewhen protocol is HTTPS.No new or changed environment variables.
Breaking or Incompatible Changes
None.
Additional Information
N/A
Checklist for PR author