Strip cookies for "/chat" #124

jackbot · 2024-11-19T16:42:22Z

https://trello.com/c/eTyOLA1r/1958

We currently strip cookies for requests which lack a relevant session cookie
for any URLs starting "/chat/" (note the trailing slash).

We want to extend the same functionality to the "/chat" URL as well.

It's pretty important that we match only "/chat" (and not, for
example, "/chat-with-someone").

Here's a Fastly Fiddle that'll pass any relevant URLs and return a 503 for any
URLs we don't want to match: https://fiddle.fastly.dev/fiddle/53922eb7

kevindew

Thanks for this. I think there's a few extra areas (because of course it's splattered around)

We've also got:

govuk-fastly/www/www.vcl.tftpl

Lines 372 to 374 in 18fbbae

    
           if (req.url !~ "^/(apply-for-a-licence|email|sign-in/callback|chat/)") { 
        
             unset req.http.Cookie; 
        
           }

that would strip cookies earlier

We've also got stripping of cookies in responses:

govuk-fastly/www/www.vcl.tftpl

Lines 518 to 521 in 18fbbae

    
           # We don't want to cache any /chat/* responses that set a cookie 
        
           if (req.url ~ "^/chat/" && beresp.http.Set-Cookie) { 
        
             return (pass); 
        
           }

to change to match /chat (to let homepage set a cookie)

As an aside I notice there is a bug with

govuk-fastly/www/www.vcl.tftpl

Lines 378 to 380 in 18fbbae

    
           if (req.url ~ "^/chat/") { 
        
             error 503 "Service unavailable"; 
        
           }

where it wouldn't match the chat homepage.

kevindew · 2024-11-19T17:02:52Z

www/www.vcl.tftpl

  # otherwise pass through
-  if (req.url ~ "^/chat/") {
+  if (req.url.path ~ "^/chat(/.*)?$") {


Nice one, did you get a chance to verify this? Though it does look good to me 👍

I tested it in the Fastly Fiddle in the PR description against a few URLs:

/chat

/chat/admin

/chat/

/chat-with-me

/chat?foo=bar

/chatttt

And everything seemed good. I'll deploy it to integration though just to make sure. (I assume it's the same process to deploy this to integration as it is for the chat app?)

We currently strip cookies for requests which lack a relevant session cookie for any URLs starting "/chat/" (note the trailing slash). We want to extend the same functionality to the "/chat" URL as well. It's pretty important that we match _only_ "/chat" (and not, for example, "/chat-with-someone").

The /chat page now relies on cookies to determine its content, so we don't want to cache that URL. This changes the conditional to look at `req.url.path` instead of `req.url` as we're not interested in matching any query params.

jackbot · 2024-11-20T08:43:09Z

Thanks Kev. I've updated the bits I missed.

www/www.vcl.tftpl

This rule is in place to return a 503 if the `disable_chat` variable is `true` and the request URL is for any GOV.UK Chat page. However due to the trailing slash, it doesn't apply on the chat homepage (www.gov.uk/chat). This updates the logic so we inspect just the path and will kick in if the URL is "/chat" or "/chat/*".

We do some stuff with cookies on requests coming to GOV.UK Chat, but the ordering is currently wrong and so sometimes cookies are being stripped before we get to these rules.

kevindew

Looks almost there just a bit of a variable misunderstanding to work out

kevindew · 2024-11-21T09:26:53Z

www/service.tf

@@ -20,6 +20,7 @@ locals {
      ssl_ciphers          = "ECDHE-RSA-AES256-GCM-SHA384"
      basic_authentication = null
      disable_chat         = false
+      chat_path_regex      = "^/chat(/.*)?$" # matches /chat and /chat/*


I imagine we need to do something janky to have this be a string inside quotes - my comments on the next bits will explain the need for that.

Gotcha. I've amended that commit to wrap the variable in quotes in the template file.

kevindew · 2024-11-21T09:28:18Z

www/www.vcl.tftpl

@@ -363,37 +363,37 @@ sub vcl_recv {

  ${indent(2, ab_tests_rendered)}

+  # Strip cookies for requests to GOV.UK Chat URLs that lack a session cookie,
+  # otherwise pass through
+  if (req.url.path ~ chat_path_regex) {


We've treated this as a variable within the VCL but it's actually a variable in the terraform template that needs to be printed.

Think of it as we're using the terraftorm template language (tftpl) to create a VCL script file.

Example print:

govuk-fastly/www/www.vcl.tftpl

Line 235 in 4204163

${private_extra_vcl_recv}

This regex is repeated multiple times so it makes sense to move it to a variable.

kevindew

Great, looks good to me. We should probably brace ourselves for some trial and error if it doesn’t quite work out.

jackbot force-pushed the strip-non-chat-cookies-for-chat branch from 45cbce4 to d008c27 Compare November 19, 2024 16:43

kevindew reviewed Nov 19, 2024

View reviewed changes

jackbot added 2 commits November 20, 2024 08:32

Prevent caching on /chat URL

5c49970

The /chat page now relies on cookies to determine its content, so we don't want to cache that URL. This changes the conditional to look at `req.url.path` instead of `req.url` as we're not interested in matching any query params.

jackbot force-pushed the strip-non-chat-cookies-for-chat branch from d008c27 to a5a3cf6 Compare November 20, 2024 08:39

kevindew reviewed Nov 20, 2024

View reviewed changes

www/www.vcl.tftpl Show resolved Hide resolved

www/www.vcl.tftpl Show resolved Hide resolved

www/www.vcl.tftpl Outdated Show resolved Hide resolved

jackbot added 2 commits November 21, 2024 09:08

Update ordering of GOV.UK Chat matching rules

35f196b

We do some stuff with cookies on requests coming to GOV.UK Chat, but the ordering is currently wrong and so sometimes cookies are being stripped before we get to these rules.

jackbot force-pushed the strip-non-chat-cookies-for-chat branch from a5a3cf6 to 4204163 Compare November 21, 2024 09:09

kevindew reviewed Nov 21, 2024

View reviewed changes

Move GOV.UK Chat URL regex to a variable

61939af

This regex is repeated multiple times so it makes sense to move it to a variable.

jackbot force-pushed the strip-non-chat-cookies-for-chat branch from 4204163 to 61939af Compare November 21, 2024 09:48

kevindew approved these changes Nov 21, 2024

View reviewed changes

jackbot merged commit 4ba766e into main Nov 21, 2024
9 checks passed

jackbot deleted the strip-non-chat-cookies-for-chat branch November 21, 2024 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strip cookies for "/chat" #124

Strip cookies for "/chat" #124

jackbot commented Nov 19, 2024 •

edited

Loading

kevindew left a comment

kevindew Nov 19, 2024

jackbot Nov 20, 2024

jackbot commented Nov 20, 2024

kevindew left a comment

kevindew Nov 21, 2024

jackbot Nov 21, 2024

kevindew Nov 21, 2024

kevindew Nov 21, 2024

kevindew left a comment

	if (req.url !~ "^/(apply-for-a-licence\|email\|sign-in/callback\|chat/)") {
	unset req.http.Cookie;
	}

	# We don't want to cache any /chat/* responses that set a cookie
	if (req.url ~ "^/chat/" && beresp.http.Set-Cookie) {
	return (pass);
	}

Strip cookies for "/chat" #124

Strip cookies for "/chat" #124

Conversation

jackbot commented Nov 19, 2024 • edited Loading

kevindew left a comment

Choose a reason for hiding this comment

kevindew Nov 19, 2024

Choose a reason for hiding this comment

jackbot Nov 20, 2024

Choose a reason for hiding this comment

jackbot commented Nov 20, 2024

kevindew left a comment

Choose a reason for hiding this comment

kevindew Nov 21, 2024

Choose a reason for hiding this comment

jackbot Nov 21, 2024

Choose a reason for hiding this comment

kevindew Nov 21, 2024

Choose a reason for hiding this comment

kevindew Nov 21, 2024

Choose a reason for hiding this comment

kevindew left a comment

Choose a reason for hiding this comment

jackbot commented Nov 19, 2024 •

edited

Loading