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

fix Cookie processing problem related to applications running on 'localhost' with non-default port (close #1491) #1563

Merged
merged 11 commits into from
Apr 20, 2018

Conversation

Farfurix
Copy link
Contributor

@Farfurix Farfurix commented Apr 10, 2018

Example

#1491

Changes:

  1. Refactor default port omitting (the default port is omitted in Chrome, Firefox, Edge, Internet Explorer browsers)
  2. localhost/127.0.0.1 (http/https) with default port (80/443) case. If the cookie domain and the current url hostname are equal to localhost/127.0.0.1, we should remove Domain=... form cookie (see tough-cookie: getPublicSuffix('localhost') check)
  3. Non-default localhost/127.0.0.1 (http/https) case. Host name check in _isValidCookie() instead of same origin check

Reference:

#659

@Farfurix Farfurix requested a review from miherlosev April 10, 2018 14:00
@testcafe-build-bot
Copy link
Collaborator

✅ Tests for the commit fd561c4 have passed. See details:

@Farfurix Farfurix requested a review from LavrovArtem April 11, 2018 05:37
src/utils/url.js Outdated
@@ -20,6 +20,8 @@ export const REQUEST_DESCRIPTOR_VALUES_SEPARATOR = '!';
export const TRAILING_SLASH_RE = /\/$/;
export const SPECIAL_PAGES = ['about:blank', 'about:error'];

export const DEFAULT_PORT = ':80';
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't duplicate constants.
Create a shared function omitDefaultPort () and use it on client and server.
Use implementation from server side -

_omitDefaultPort (dest) {
.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for http and https protocol values.

@@ -31,6 +31,9 @@ export function sameOriginCheck (location, checkedUrl, rejectForSubdomains) {
if (checkedUrl)
checkedUrl = resolveUrl(checkedUrl);

// NOTE: exclude the default port from 'same origin check'
location = location.replace(sharedUrlUtils.DEFAULT_PORT, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

We should omit default port inside sameOriginCheck function.

@testcafe-build-bot
Copy link
Collaborator

✅ Tests for the commit b10589e have passed. See details:

@testcafe-build-bot
Copy link
Collaborator

✅ Tests for the commit 277ea66 have passed. See details:

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit acb0a72 have failed. See details:

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit d111874 have failed. See details:

@Farfurix
Copy link
Contributor Author

@testcafe-build-bot \retest

@testcafe-build-bot
Copy link
Collaborator

✅ Tests for the commit d111874 have passed. See details:

@testcafe-build-bot
Copy link
Collaborator

✅ Tests for the commit 3e9900e have passed. See details:

@Farfurix Farfurix changed the title [WIP] fix Cookie processing problem related to applications running on 'localhost' with non-default port (close #1491) fix Cookie processing problem related to applications running on 'localhost' with non-default port (close #1491) Apr 17, 2018
@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit e4dce4b have failed. See details:

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit b86b5a6 have failed. See details:

@Farfurix
Copy link
Contributor Author

@testcafe-build-bot \retest

@testcafe-build-bot
Copy link
Collaborator

✅ Tests for the commit b86b5a6 have passed. See details:

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 9066365 have failed. See details:

@Farfurix
Copy link
Contributor Author

@testcafe-build-bot \retest

@testcafe-build-bot
Copy link
Collaborator

✅ Tests for the commit 9066365 have passed. See details:

@testcafe-build-bot
Copy link
Collaborator

✅ Tests for the commit 4b11e7f have passed. See details:

@testcafe-build-bot
Copy link
Collaborator

✅ Tests for the commit b62488c have passed. See details:

Copy link
Contributor

@miherlosev miherlosev left a comment

Choose a reason for hiding this comment

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

Minor changes

import BYTES_PER_COOKIE_LIMIT from './cookie-limit';
import { castArray } from 'lodash';
import { parseUrl } from '../utils/url';

const LOCALHOST_NAME = 'localhost';
Copy link
Contributor

Choose a reason for hiding this comment

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

const LOCALHOST_DOMAIN = ...

// NOTE: If cookie.domain and url hostname are equal to localhost/127.0.0.1,
// we should remove 'Domain=...' form cookieStr (GH-1491)
if (cookie && cookie.domain) {
const isCookieDomainLocalhost = cookie.domain === LOCALHOST_NAME || cookie.domain === LOCALHOST_IP;
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to separate function _hasLocalhostDomain (cookie)

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 6cb421c have failed. See details:

@Farfurix
Copy link
Contributor Author

@testcafe-build-bot \retest

@testcafe-build-bot
Copy link
Collaborator

✅ Tests for the commit 6cb421c have passed. See details:

@miherlosev miherlosev merged commit e6235bc into DevExpress:master Apr 20, 2018
@Farfurix Farfurix deleted the i1491 branch April 26, 2018 06:37
AndreyBelym pushed a commit to AndreyBelym/testcafe-hammerhead that referenced this pull request Feb 28, 2019
…calhost' with non-default port` (close DevExpress#1491) (DevExpress#1563)

* fix `Cookie processing problem related to applications running on 'localhost' with non-default port`

* refactor default port omitting

* fix omitDefaultPort

* localhost/127.0.0.1 case

* non-default localhost/127.0.0.1 (http) case

* add location test for default port omitting

* requested changes

* rename test function

* add tests

* rename tests

* requested changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants