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

Set SameSite=Strict for improved security #355

Merged
merged 1 commit into from Apr 3, 2018

Conversation

@sholladay
Copy link
Contributor

sholladay commented Mar 24, 2018

This was set to false a long time ago, mainly due to a Chrome bug that prevented OAuth from working with Strict mode (see aspnet/Security#1231 for a good explanation of the infinite login loop).

That Chrome bug has since been fixed.

I have thoroughly tested that Strict works correctly now, across browsers. Chrome was the only browser to ever have an issue with this, though.

The Chrome bug that used to cause an infinite login loop when this was set has been fixed.
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Apr 2, 2018

Would this count as a breaking change?

@sholladay

This comment has been minimized.

Copy link
Contributor Author

sholladay commented Apr 2, 2018

IMO this is semver minor. I'm not aware of any use cases that would call for the bell cookie to be intentionally sent to another domain. It's only for the app's login route.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Apr 3, 2018

I'll let @AdriVanHoudt merge and publish...

@AdriVanHoudt AdriVanHoudt self-assigned this Apr 3, 2018
@AdriVanHoudt AdriVanHoudt merged commit 15059f9 into hapijs:master Apr 3, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Apr 3, 2018

@AdriVanHoudt Make sure to associate every issue and PR with a release and to queue the next release version when you close the published one.

@AdriVanHoudt AdriVanHoudt added this to the 9.3.0 milestone Apr 3, 2018
@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Apr 3, 2018

@hueniverse associate with a milestone right? And queue up the next release seems odd since you don't really know if it will be a patch/minor/major?

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Apr 3, 2018

Always start with a patch. And you edit it based on what you attach to it. So if you attach a bug, it stays a patch. If you attach a new feature, it's a minor, etc.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Apr 3, 2018

Ok cool, will do

@AVVS

This comment has been minimized.

Copy link

AVVS commented Apr 6, 2018

Which chrome version is this fixed? I'm running integration tests with headless chrome 64 and facebook oauth and cant seem to get this to work at all - cookies are not being sent, even after the refresh - any idea how to fix this except for setting this to lax ?

@AVVS

This comment has been minimized.

Copy link

AVVS commented Apr 6, 2018

The logs:

---> [http://ms-users.local:3000/users/oauth/facebook] headers - {"upgrade-insecure-requests":"1","user-agent":"Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) HeadlessChrome/64.0.3282.168 Safari/537.36","x-devtools-emulate-network-conditions-client-id":"(18E4231D578C666C88346977A6061B46)","accept":"text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8"}
<--- [http://ms-users.local:3000/users/oauth/facebook] 302 headers - {"set-cookie":"bfb=Fe26.2**702f261bf47cc0ef9d7667e3fb549a27e69801a6dacb7a29049a63cc7ed90e1c*Tjg7WWB3ZwvF9RCGlhQcVg*0Fdh62F1CyO5hLC2-7V6PJjjWCz6_yYBg-QkMdb7pU2dxqnWTKUqlNQiRNi9gStI**97723afa29c278df9d12a47d4c8059cc74d12d62090038c0a5d088eba641ed7c*LhZ_akT4q2w6yWs6imqB3Td-byQDDO5L5PWXJTy1E04; HttpOnly; SameSite=Strict; Path=/","location":"https://www.facebook.com/v2.9/dialog/oauth?client_id=836615033080462&response_type=code&redirect_uri=http%3A%2F%2Fms-users.local%3A3000%2Fusers%2Foauth%2Ffacebook&state=apGUp_k1wGCuCzPg6IG7pu&scope=email"}
---> [http://ms-users.local:3000/users/oauth/facebook?code=AQBr-E3unXN31yCv9EOpB6dcNAeB-NhVtLx7ssU_C8iazauZTmMR31ZgqxLWifLrLwt5Ew0aeElBVNcVU0EY3Pcvfnd4jfEX3jtHC-RZltup1HMlHgIA9BLVaHY4bwNRa0oNzA0JlgWuLEzmg82nGPkkalnERXEhzX6Iz3jISgn2KcHpiXlYRc2smYkj5ejBwxGJXYj0QlcAhnqq3FGPUxYy-VO1ag_zmud2JhChOW8T8WOkZZsy_14O1UjXhj3jEQnJcIgTObXTNSG7dx2ZfKFJIzPjzGQy2eNqXrCbtgnjBiIHfppVzhF3Ci1elF9C3A83idlUyOCqgvPvd1yhKFMy&state=apGUp_k1wGCuCzPg6IG7pu] headers - {"upgrade-insecure-requests":"1","user-agent":"Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) HeadlessChrome/64.0.3282.168 Safari/537.36","x-devtools-emulate-network-conditions-client-id":"(18E4231D578C666C88346977A6061B46)","accept":"text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8"}
<--- [http://ms-users.local:3000/users/oauth/facebook?code=AQBr-E3unXN31yCv9EOpB6dcNAeB-NhVtLx7ssU_C8iazauZTmMR31ZgqxLWifLrLwt5Ew0aeElBVNcVU0EY3Pcvfnd4jfEX3jtHC-RZltup1HMlHgIA9BLVaHY4bwNRa0oNzA0JlgWuLEzmg82nGPkkalnERXEhzX6Iz3jISgn2KcHpiXlYRc2smYkj5ejBwxGJXYj0QlcAhnqq3FGPUxYy-VO1ag_zmud2JhChOW8T8WOkZZsy_14O1UjXhj3jEQnJcIgTObXTNSG7dx2ZfKFJIzPjzGQy2eNqXrCbtgnjBiIHfppVzhF3Ci1elF9C3A83idlUyOCqgvPvd1yhKFMy&state=apGUp_k1wGCuCzPg6IG7pu] 200 headers - {}
---> [http://ms-users.local:3000/users/oauth/facebook?code=AQBr-E3unXN31yCv9EOpB6dcNAeB-NhVtLx7ssU_C8iazauZTmMR31ZgqxLWifLrLwt5Ew0aeElBVNcVU0EY3Pcvfnd4jfEX3jtHC-RZltup1HMlHgIA9BLVaHY4bwNRa0oNzA0JlgWuLEzmg82nGPkkalnERXEhzX6Iz3jISgn2KcHpiXlYRc2smYkj5ejBwxGJXYj0QlcAhnqq3FGPUxYy-VO1ag_zmud2JhChOW8T8WOkZZsy_14O1UjXhj3jEQnJcIgTObXTNSG7dx2ZfKFJIzPjzGQy2eNqXrCbtgnjBiIHfppVzhF3Ci1elF9C3A83idlUyOCqgvPvd1yhKFMy&state=apGUp_k1wGCuCzPg6IG7pu&refresh=1] headers - {"upgrade-insecure-requests":"1","user-agent":"Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) HeadlessChrome/64.0.3282.168 Safari/537.36","x-devtools-emulate-network-conditions-client-id":"(18E4231D578C666C88346977A6061B46)","accept":"text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8"}
<--- [http://ms-users.local:3000/users/oauth/facebook?code=AQBr-E3unXN31yCv9EOpB6dcNAeB-NhVtLx7ssU_C8iazauZTmMR31ZgqxLWifLrLwt5Ew0aeElBVNcVU0EY3Pcvfnd4jfEX3jtHC-RZltup1HMlHgIA9BLVaHY4bwNRa0oNzA0JlgWuLEzmg82nGPkkalnERXEhzX6Iz3jISgn2KcHpiXlYRc2smYkj5ejBwxGJXYj0QlcAhnqq3FGPUxYy-VO1ag_zmud2JhChOW8T8WOkZZsy_14O1UjXhj3jEQnJcIgTObXTNSG7dx2ZfKFJIzPjzGQy2eNqXrCbtgnjBiIHfppVzhF3Ci1elF9C3A83idlUyOCqgvPvd1yhKFMy&state=apGUp_k1wGCuCzPg6IG7pu&refresh=1] 401 headers - {}

validate existing cookies: [{"name":"bfb","value":"Fe26.2**702f261bf47cc0ef9d7667e3fb549a27e69801a6dacb7a29049a63cc7ed90e1c*Tjg7WWB3ZwvF9RCGlhQcVg*0Fdh62F1CyO5hLC2-7V6PJjjWCz6_yYBg-QkMdb7pU2dxqnWTKUqlNQiRNi9gStI**97723afa29c278df9d12a47d4c8059cc74d12d62090038c0a5d088eba641ed7c*LhZ_akT4q2w6yWs6imqB3Td-byQDDO5L5PWXJTy1E04","domain":"ms-users.local","path":"/","expires":-1,"size":273,"httpOnly":true,"secure":false,"session":true,"sameSite":"Strict"}]
@sholladay

This comment has been minimized.

Copy link
Contributor Author

sholladay commented Apr 6, 2018

Chrome 65. Looks like your tests are running on 64.

@AVVS

This comment has been minimized.

Copy link

AVVS commented Apr 6, 2018

yep, its 64 - 65 is still not available on some platform, so idk, could the setting be dynamic based on the headers (ie user-agent)? Otherwise gotta do some "dirty" setting via hapi.states.cookies[name].isSameSite = 'Lax'

@sholladay

This comment has been minimized.

Copy link
Contributor Author

sholladay commented Apr 6, 2018

I don't have any strong feelings, but IMO a setting (either manual or automatic) would have very limited usefulness. User agent parsing is rife with problems. Chrome auto updates and 65 has been available for a month now, so this isn't really a concern for the general bell user. All operating systems have Chrome 65. I even checked Sauce Labs and BrowserStack, which are usually a bit behind due to their WebDriver integration, and they both have Chrome 65.

I think a better solution, if you are in a niche case where you can't update your testing environment, would be to pin your bell dependency temporarily. You can do that by specifying "bell": "~9.2.0" in your package.json.

@AVVS

This comment has been minimized.

Copy link

AVVS commented Apr 6, 2018

I'm using alpine linux and it comes with 64 only for now. Anyway - that doesnt seem to be a big problem, mostly bugs with chrome itself. For instance, there is the refresh=1 thing where the page reloads itself (I think it was introduced in bell exactly due to this bug) and it works on one of the repos with e2e testing, but, on another one it doesnt. I thought there was some issue with hosts and that cookies are not being set, but checks with

page.cookies(page.url()) reveals that there are cookies with correct settings, but headers are not sent (doesn't even matter if I issue a new request to the same page with the same URL)

Either way - I'll just see how it goes in a month with alpine updates - wonder if its really just a bug there

@sholladay

This comment has been minimized.

Copy link
Contributor Author

sholladay commented Apr 6, 2018

Good to know about Alpine. I'm sure it will receive the update soon.

doesn't even matter if I issue a new request to the same page with the same URL

That sounds like an unrelated issue, possibly a bug in your app. I've always witnessed page refreshes working after login, on any Chrome version. In fact, I used to send <script>location.reload()</script> as a workaround to the Chrome bug.

You should double check that any fetch() calls in the app that need cookies are using { credentials : 'same-origin' }, since fetch() doesn't send them by default.

@AVVS

This comment has been minimized.

Copy link

AVVS commented Apr 6, 2018

I use puppeteer with headless chrome, so its an actual page load (not a fetch) - hence that shouldn't really apply.

The following code literally works in one set of tests and doesn't in the other. The only difference is NODE_ENV=test/production

I did think it could be related to secure cookies, but inspection of page.cookies revealed that set-cookie was identical in both cases

exports.getFacebookData = async (opts = {}) => {
  const {
    onlyToken = true,
    jwt,
    confirmed = false,
  } = opts;

  let token;
  try {
    const executeLink = `${hostUrl}/users/oauth/facebook`;
    const link = jwt ? `${executeLink}?jwt=${jwt}` : executeLink;
    console.info('open %s', link);
    await page.goto(link, { waitUntil: 'networkidle2' });
    await page.waitForSelector('input#email', { visible: true });
    await page.type('input#email', cache.testUser.email, { delay: 100 });
    await page.waitForSelector('input#pass');
    await page.type('input#pass', cache.testUser.password, { delay: 100 });
    await page.click('button[name=login]', { delay: 100 });

    // only needed for initial confirm
    if (confirmed === false) {
      await Promise.delay(1000);
      await page.waitForSelector('button[name=__CONFIRM__]', { visible: true });
      await Promise.all([
        page.waitForNavigation({ waitUntil: 'networkidle0' }),
        page.click('button[name=__CONFIRM__]'),
      ]);
    }

    await page.waitForSelector('.no-js > body > script');
    token = await (onlyToken ? extractToken() : extractBody());
  } catch (e) {
    console.info('page url - %s', page.url());
    await page.screenshot({ fullPage: true, path: `./ss/crash-${Date.now()}.png` });
    throw e;
  }

  return token;
};
@sholladay

This comment has been minimized.

Copy link
Contributor Author

sholladay commented Apr 6, 2018

To be clear, my suggestion above relates to the application code, not the test code. It's just a guess, but if you have fetch() calls that aren't explicitly sending cookies, then it doesn't matter if the cookie is set, it still won't be sent regardless of the value of page.cookies() in the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.