Skip to content
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.

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

Add Support for Setting HttpOnly and Secure Flags on NEXT_LOCALE Cookie #1268

Closed
Michael-Grupp opened this issue Aug 17, 2024 · 7 comments · Fixed by #1391
Closed

Add Support for Setting HttpOnly and Secure Flags on NEXT_LOCALE Cookie #1268

Michael-Grupp opened this issue Aug 17, 2024 · 7 comments · Fixed by #1391

Comments

@Michael-Grupp
Copy link

Is your feature request related to a problem? Please describe.

Yes, the issue is that the cookie set by next-intl does not have the HttpOnly flag. This can present a security risk because cookies without this flag are accessible via JavaScript, which could potentially be exploited by cross-site scripting (XSS) attacks.

Additionally, when running security tests, such as with Mozilla Observatory, the cookie set by next-intl is flagged because it does not have the Secure flag. While transmission over HTTP is prevented by HSTS, adding the Secure flag would ensure that the cookie is only sent over HTTPS, providing an additional layer of security.

Describe the solution you'd like

I would like the next-intl library to support setting the HttpOnly and Secure flags on the cookies it creates. Specifically:

  • The HttpOnly flag should be added to prevent client-side scripts from accessing the cookies, reducing the risk of XSS attacks.
  • The Secure flag should be set on cookies to ensure they are only sent over HTTPS, even when HSTS is enabled.

The implementation could involve adding options to the existing cookie configuration, allowing developers to easily enable these flags as needed.

Describe alternatives you've considered

One alternative is to manually set these flags on the cookies after they are created by next-intl, but this approach is less desirable because:

  • It adds additional complexity to the codebase, requiring developers to remember to manually secure cookies.
  • It increases the risk of inconsistencies or errors, particularly in larger projects where this might be overlooked.
@amannn
Copy link
Owner

amannn commented Aug 20, 2024

Hey @Michael-Grupp, thank you for your thoughtful report!

Secure: That's a good point, I've just added #1272 to add this. Do you by chance know if this could be a breaking change for any consumers? As far as I can tell only, in case someone would host a Next.js app without HTTPS, which seems very unlikely to me. Would you agree?

HttpOnly: This is currently not set, because next-intl does in fact change the cookie on the client side (see #790). I understand that ideally we'd not have to do this, but due to the way Next.js works, this is currently necessary as far as I can tell.

One alternative is to manually set these flags on the cookies after they are created by next-intl, but this approach is less desirable

I agree, ideally next-intl would work out-of-the-box without any changes necessary.

@Michael-Grupp
Copy link
Author

Hey @amannn, thank you for the quick response and for addressing the Secure flag issue with #1272. I agree that hosting a Next.js app without HTTPS is uncommon, so I don't foresee this being a breaking change for most users. However, it might be worth mentioning in the release notes that this change is being introduced, just in case there are any edge cases.

Regarding the HttpOnly flag, I understand the current limitations due to client-side cookie manipulation and the way Next.js works. While it's not ideal from a security standpoint, I appreciate the challenge it presents.

Out of curiosity, do you know if there's an issue on Next.js that specifically tracks the behavior of the Router Cache in relation to cookies? It would be helpful to monitor any progress on this front so I can revisit the possibility of setting the HttpOnly flag in the future.

Thanks again for your work on this and for considering these enhancements to next-intl.

@amannn
Copy link
Owner

amannn commented Aug 22, 2024

Thinking this through again, I think there could be two cases where users might use HTTP:

  1. Local development: If you use a host different than localhost (Secure only has an exception for localhost). So either if you're for example accessing an IP address from your local network for testing, or also if the user potentially has multiple hosts set up via hosts (e.g. to test domain-based routing) this could be an issue.
  2. Internal communication: If you're running a web server behind a reverse proxy, a user might connect via HTTPS, but if the origin server running the Next.js app communicates over HTTP with the proxy, this would break. I'm honestly not sure how common this is, but I've seen users using rather legacy deployment infrastructure.

I also did some research, and found an article on the usage of the Secure flag from Jaspersoft. They specifically don't use the Secure flag for the language and timezone preference of the user:

Jaspersoft does not set the secure flag on these cookies because we do not want to force you to use secure connections.

This makes me reconsider if setting the Secure flag is really a good idea for the locale cookie. It's definitely a requirement for user sessions etc., but I'm unsure if this should be something we enforce for a user preference like the locale.

Potentially, we could instead offer an opt-in configuration option (related to #454 (comment)). I made a note about this in #779 to consider for the next major version.

For the time being, you can consider modifying the cookie after a response has been generated if this is important to you with something like this:

import {NextRequest} from 'next/server';
import createMiddleware from 'next-intl/middleware';

const handleI18nRouting = createMiddleware(/* ... */);

export default function middleware(request: NextRequest) {
  const response = handleI18nRouting(request);

  if (response.cookies.get('NEXT_LOCALE')) {
    // Set the `secure` flag
    response.cookies.set(
      'NEXT_LOCALE',
      response.cookies.get('NEXT_LOCALE').value,
      {
        ...response.cookies.get('NEXT_LOCALE'),
        secure: true
      }
    );
  }

  return response;
}

(not tested)

Does this make sense to you?

Out of curiosity, do you know if there's an issue on Next.js that specifically tracks the behavior of the Router Cache in relation to cookies? It would be helpful to monitor any progress on this front so I can revisit the possibility of setting the HttpOnly flag in the future.

They're making a lot of changes to caching in Next.js 15, might be worth keeping an eye on the release notes!

@Michael-Grupp
Copy link
Author

Thank you for the thoughtful response and for providing additional context.

I appreciate the detailed considerations regarding the Secure flag. Your points about local development and internal communication are well taken, and I understand how enforcing the Secure flag could present challenges in these scenarios. The comparison with Jaspersoft's approach to non-sensitive cookies also makes sense, especially for user preferences like locale.

The idea of offering an opt-in configuration option for the Secure flag sounds like a practical solution. This would provide flexibility and allow developers to tailor the security settings according to their specific use cases.

In the meantime, I’ll consider modifying the cookie after the response if necessary to meet our security requirements. I look forward to any updates related to this in future versions of next-intl.

Thank you again for your time and for considering these aspects.

@amannn
Copy link
Owner

amannn commented Oct 10, 2024

Update: A new localeCookie option for the middleware has been added in #1414 that now provides opt-in support for setting the secure attribute. The feature is available in the latest canary release.

Edit: I noticed there's a bug in the current implementation of the localeCookie feature, need to have another look in #1417.

@amannn
Copy link
Owner

amannn commented Oct 16, 2024

Re #454 (comment): Ok, a new canary is out that addresses the bug I've noticed. A small API change was necessary: the localeCookie option was moved from the second argument of the middleware to defineRouting (or just the first parameter of the middleware). However, the previous API is still supported, but now deprecated.

The proposed docs have been updated accordingly.

@ETOPS7
Copy link

ETOPS7 commented Oct 23, 2024

Thanks for fixing this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants