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

Stop same-site cookies from being wiped when printing in Internet Explorer 11 #2045

Merged
merged 4 commits into from
Jan 21, 2021

Conversation

gunjam
Copy link
Contributor

@gunjam gunjam commented Nov 27, 2020

tl;dr this fixes an issue where a service with SameSite: Strict set on its session cookies will have them wiped in IE when opening the print dialog if they serve a page on the root of their domain.

We were experiencing an odd issue in our service where some users were unexpectantly getting sent back to the beginning of the service from the Check Your Answers page. After some research with users this appeared to only happen after they attempted to print their answers. A lot of head scratching later I figured out what was going on: when a user in IE11 (and presumably lower) opens the print dialog, the browser re-requests all uncached assets and in doing will make a GET request for xlink:href value in the crown SVG image tag, even though it's blank. It'll resolve this to the root of the domain which for us is the first page of our service. The problem is that if you have SameSite: Strict set on your cookies, IE won't send them when making the image request to / so our service will act as if this is a new session and send a new session cookie with the response, which IE will then propagate back into the page resetting the session cookie. On submission, the user is now trying to access a page at the end of the service with a blank session, so gets sent back to the start.

A pretty odd issue, typical of IE, but will affect any service with a session based page being served on /, unless you set SameSite to something other than Strict, which we don't really want to do.

The simplest solution to this I've come up with, is to set the empty xlink:href value to a blank data URI so that the browser will not initiate a network request. I've also set the SVG display attribute to none so browsers do no render a small blank SVG in place.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-2045 November 27, 2020 10:36 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-fronte-master-swfht2n2zv November 30, 2020 13:42 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-fronte-master-pn1l8vsccg November 30, 2020 13:51 Inactive
@36degrees
Copy link
Contributor

Thanks for contributing this @gunjam – really amazing investigative work getting to the bottom of that!

I've been able to verify the issue by tracking network requests in ngrok.

When the 'check your answers' page in our review app is first loaded in IE11 (using BrowserStack), the following requests are made:

before-pre-print

When pressing 'Print' in the browser several additional network requests are made, including a request for the fallback PNG but also an unexpected request to /full-page-examples/ which as you've identified appears to be caused by the empty xlink:href.

before-post-print

With this branch checked out, the request to /full-page-examples/ does not happen:

after-post-print

I've also set the SVG display attribute to none so browsers do no render a small blank SVG in place.

This fixes a tiny bordered square in IE10 and IE11 that I hadn't noticed before.

Browser Before After
IE 9 IE9-before IE9-after
IE 10 IE10-before IE10-after

We do need to rebase this to pick up the latest changes to our CI setup, which I can do for you now. I'll also add an entry to the changelog.

Finally, this will need another review from another member of the team and then we can get it merged.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-fronte-master-pn1l8vsccg January 8, 2021 13:57 Inactive
@36degrees 36degrees changed the title Replace blank xlink:href attribute in header image tag with empty data URI Stop same-site cookies from being wiped when printing in Internet Explorer 11 Jan 8, 2021
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-fronte-master-pn1l8vsccg January 8, 2021 15:16 Inactive
@36degrees
Copy link
Contributor

I've rebased this, added a test for the display attribute (as I think someone could easily try and remove it, not understanding what it does), and added an entry to the changelog.

@gunjam are you happy with the changelog entry and the updated PR title? I've tried to focus it around the issue it fixes, rather than the change that was made.

@36degrees 36degrees added this to the [NEXT] milestone Jan 8, 2021
Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

I'm happy with this, pending another review from the team.

@gunjam
Copy link
Contributor Author

gunjam commented Jan 8, 2021

More than happy, thanks @36degrees .

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-fronte-master-pn1l8vsccg January 8, 2021 16:24 Inactive
@hannalaakso hannalaakso self-requested a review January 12, 2021 10:09
Copy link
Member

@hannalaakso hannalaakso left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this @gunjam 🕵️

@@ -39,7 +39,7 @@

Copy link
Member

Choose a reason for hiding this comment

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

It would be good to correct the above comment to say

an empty data URI xlink:href

instead of

empty xlink:href attribute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @hannalaakso I've made the changes you suggested. Thanks!

gunjam and others added 4 commits January 21, 2021 10:23
This prevents SVG supporting versions of IE from making a network
request to `/` when the print dialog is opened, which can cause session
cookies to be cleared if SameSite is set to 'Strict'.
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-fronte-master-pn1l8vsccg January 21, 2021 10:31 Inactive
@36degrees 36degrees merged commit b547324 into alphagov:master Jan 21, 2021
@hannalaakso hannalaakso mentioned this pull request Feb 8, 2021
@calvin-lau-sig7
Copy link
Contributor

calvin-lau-sig7 commented Feb 8, 2021

Hi @gunjam, thanks again for your contribution.

We'd like to add your name to a list of contributors in some of our release comms. Can you let me know your full name, where you work and your Slack handle on 'UK Government Digital'?

Thanks kindly!

@vanitabarrett vanitabarrett mentioned this pull request Apr 9, 2021
36degrees added a commit that referenced this pull request May 17, 2021
The current fallback uses an `<image>` element, which is interpreted differently depending on whether it’s parsed in the context of SVG or HTML.

We include an `xlink:href` attribute on the `<image>` tag to prevent versions of IE which support SVG from downloading the fallback image when they don't need to.

However, this approach has proved problematic – IE11 still requests the fallback PNG when printing, which can cause issues with sessions (#2045) and an attempted fix for that issue by using a blank data uri has now caused further issues with Content Security Policies (#2203).

Switch to the simpler approach of using conditional comments for the header fallback PNG, targeting Internet Explorer 8 specifically. We can then use a good old-fashioned `<img>` tag.

This does mean that browsers that do not support SVG other than Internet Explorer 8 will not be able to see the crown emblem.

According to Can I Use, the other main browser that does not support SVG that we may need to consider is Android 2.1 - 2.3 [1].

Between 2020-05-14 and 2021-04-30 analytics for GOV.UK shows 6,792 (0.00041%) sessions from 4,343 (0.00119%) users using Android 2.x, out of 577,929,096 sessions from 77,144,107 users.

By comparison, in the same period we saw 36,075 sessions (0.00217%) from 28,963 users (0.00791%) using IE8.

It appears that Android 2.x browsers just omit the SVG entirely – there’s no ‘broken image’ displayed – these users just see ‘GOV.UK’ without the crown.

Given the very low volume of traffic from these browsers, this seems like an acceptable thing to do.

We could have removed the fallback PNG entirely, however our browser support explicitly states support for Internet Explorer 8 (‘although components may not look perfect’) and we know that some service teams do have a disproprtionate number of IE8 users – which is unlikely to be the case for Android 2.

[1]:https://caniuse.com/svg
36degrees added a commit that referenced this pull request May 17, 2021
The current fallback uses an `<image>` element, which is interpreted differently depending on whether it’s parsed in the context of SVG or HTML.

We include an `xlink:href` attribute on the `<image>` tag to prevent versions of IE which support SVG from downloading the fallback image when they don't need to.

However, this approach has proved problematic – IE11 still requests the fallback PNG when printing, which can cause issues with sessions (#2045) and an attempted fix for that issue by using a blank data uri has now caused further issues with Content Security Policies (#2203).

Switch to the simpler approach of using conditional comments for the header fallback PNG, targeting Internet Explorer 8 specifically. We can then use a good old-fashioned `<img>` tag.

This does mean that browsers that do not support SVG other than Internet Explorer 8 will not be able to see the crown emblem.

According to Can I Use, the other main browser that does not support SVG that we may need to consider is Android 2.1 - 2.3 [1].

Between 2020-05-14 and 2021-04-30 analytics for GOV.UK shows 6,792 (0.00041%) sessions from 4,343 (0.00119%) users using Android 2.x, out of 577,929,096 sessions from 77,144,107 users.

By comparison, in the same period we saw 36,075 sessions (0.00217%) from 28,963 users (0.00791%) using IE8.

It appears that Android 2.x browsers just omit the SVG entirely – there’s no ‘broken image’ displayed – these users just see ‘GOV.UK’ without the crown.

Given the very low volume of traffic from these browsers, this seems like an acceptable thing to do.

We could have removed the fallback PNG entirely, however our browser support explicitly states support for Internet Explorer 8 (‘although components may not look perfect’) and we know that some service teams do have a disproprtionate number of IE8 users – which is unlikely to be the case for Android 2.

[1]:https://caniuse.com/svg
36degrees added a commit that referenced this pull request May 17, 2021
The current fallback uses an `<image>` element, which is interpreted differently depending on whether it’s parsed in the context of SVG or HTML.

We include an `xlink:href` attribute on the `<image>` tag to prevent versions of IE which support SVG from downloading the fallback image when they don't need to.

However, this approach has proved problematic – IE11 still requests the fallback PNG when printing, which can cause issues with sessions (#2045) and an attempted fix for that issue by using a blank data uri has now caused further issues with Content Security Policies (#2203).

Switch to the simpler approach of using conditional comments for the header fallback PNG, targeting Internet Explorer 8 specifically. We can then use a good old-fashioned `<img>` tag.

This does mean that browsers that do not support SVG other than Internet Explorer 8 will not be able to see the crown emblem.

According to Can I Use, the other main browser that does not support SVG that we may need to consider is Android 2.1 - 2.3 [1].

Between 2020-05-14 and 2021-04-30 analytics for GOV.UK shows 6,792 (0.00041%) sessions from 4,343 (0.00119%) users using Android 2.x, out of 577,929,096 sessions from 77,144,107 users.

By comparison, in the same period we saw 36,075 sessions (0.00217%) from 28,963 users (0.00791%) using IE8.

It appears that Android 2.x browsers just omit the SVG entirely – there’s no ‘broken image’ displayed – these users just see ‘GOV.UK’ without the crown.

Given the very low volume of traffic from these browsers, this seems like an acceptable thing to do.

We could have removed the fallback PNG entirely, however our browser support explicitly states support for Internet Explorer 8 (‘although components may not look perfect’) and we know that some service teams do have a disproprtionate number of IE8 users – which is unlikely to be the case for Android 2.

[1]:https://caniuse.com/svg
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

5 participants