Cors issue resolved #162
Cors issue resolved #162
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant R as Request
participant U as CookieUtil
participant S as Response
R->>U: Call addJwtTokenToCookie(token, response, request)
U->>U: Set SameSite value to "None"
U->>U: Set Secure flag to true
U->>S: Construct and add custom Set-Cookie header (Path, Max-Age, HttpOnly, SameSite)
S->>R: Return response with cookie header
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/utils/CookieUtil.java (1)
27-67: Add input validation and make cookie settings configurable.Consider these improvements for better maintainability and debugging:
- Add input validation
- Make cookie settings configurable
- Add logging for debugging CORS issues
Consider adding these enhancements:
+ @Value("${cookie.same-site:Lax}") + private String cookieSameSite; + + @Value("${cookie.max-age:86400}") // 24 hours in seconds + private int cookieMaxAge; + public void addJwtTokenToCookie(String Jwttoken, HttpServletResponse response, HttpServletRequest request) { + if (Jwttoken == null || Jwttoken.trim().isEmpty()) { + throw new IllegalArgumentException("JWT token cannot be null or empty"); + } + + log.debug("Setting JWT cookie with SameSite={}, Secure={}", cookieSameSite, + "https".equalsIgnoreCase(request.getScheme()));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/common/utils/CookieUtil.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: style-check / checkstyle
- GitHub Check: Analyze (java)
- GitHub Check: Package-test
- GitHub Check: Build
🔇 Additional comments (1)
src/main/java/com/iemr/common/utils/CookieUtil.java (1)
28-38: LGTM! Basic cookie configuration is secure and well-documented.The implementation correctly sets essential cookie attributes with appropriate security considerations:
- HttpOnly flag prevents JavaScript access
- Reasonable expiration time of 24 hours
- Root path configuration for application-wide access
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/iemr/common/utils/CookieUtil.java (1)
28-38: Consider extracting hardcoded values into constants.For better maintainability and reusability, consider moving hardcoded values to class-level constants:
public class CookieUtil { + private static final String JWT_COOKIE_NAME = "Jwttoken"; + private static final String COOKIE_PATH = "/"; + private static final int COOKIE_MAX_AGE_SECONDS = 60 * 60 * 24; // 1 day public void addJwtTokenToCookie(String Jwttoken, HttpServletResponse response, HttpServletRequest request) { - Cookie cookie = new Cookie("Jwttoken", Jwttoken); + Cookie cookie = new Cookie(JWT_COOKIE_NAME, Jwttoken); cookie.setHttpOnly(true); - cookie.setMaxAge(60 * 60 * 24); - cookie.setPath("/"); + cookie.setMaxAge(COOKIE_MAX_AGE_SECONDS); + cookie.setPath(COOKIE_PATH);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/common/utils/CookieUtil.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: sandipkarmakar3
PR: PSMRI/Common-API#162
File: src/main/java/com/iemr/common/utils/CookieUtil.java:52-66
Timestamp: 2025-02-21T07:43:03.828Z
Learning: In the Common-API project's CookieUtil class, the current implementation of addJwtTokenToCookie using both response.addCookie() and manual Set-Cookie header has been tested and confirmed to work as required for their use case. URL encoding of the JWT token is not necessary.
Learnt from: sandipkarmakar3
PR: PSMRI/Common-API#162
File: src/main/java/com/iemr/common/utils/CookieUtil.java:40-47
Timestamp: 2025-02-21T07:42:36.497Z
Learning: In the Common-API project's CookieUtil class, JWT cookies are configured with SameSite=None to support cross-origin requests, which is required for the project's CORS functionality.
src/main/java/com/iemr/common/utils/CookieUtil.java (2)
Learnt from: sandipkarmakar3
PR: PSMRI/Common-API#162
File: src/main/java/com/iemr/common/utils/CookieUtil.java:52-66
Timestamp: 2025-02-21T07:43:03.828Z
Learning: In the Common-API project's CookieUtil class, the current implementation of addJwtTokenToCookie using both response.addCookie() and manual Set-Cookie header has been tested and confirmed to work as required for their use case. URL encoding of the JWT token is not necessary.
Learnt from: sandipkarmakar3
PR: PSMRI/Common-API#162
File: src/main/java/com/iemr/common/utils/CookieUtil.java:40-47
Timestamp: 2025-02-21T07:42:36.497Z
Learning: In the Common-API project's CookieUtil class, JWT cookies are configured with SameSite=None to support cross-origin requests, which is required for the project's CORS functionality.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Analyze (java)
- GitHub Check: Package-test
🔇 Additional comments (1)
src/main/java/com/iemr/common/utils/CookieUtil.java (1)
49-66: Implementation looks good!The dual approach of using both
response.addCookie()and manualSet-Cookieheader has been tested and confirmed to work as required for your use case.Note: Based on the learnings from previous discussions, this implementation is intentional and necessary for proper cookie handling across different browsers.
|
📋 Description
JIRA ID:
Cors issue
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
SameSiteattribute in cookies for better security.Secureflag in cookies.