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 with secure attribute are not attached to the request on localhost(closes #2715) #2761

Merged
merged 4 commits into from Apr 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/request-pipeline/context.ts
Expand Up @@ -30,7 +30,7 @@ import getMockResponse from '../request-pipeline/request-hooks/response-mock/get
import { Http2Response } from './destination-request/http2';
import RequestEvent from '../session/events/request-event';

interface DestInfo {
export interface DestInfo {
url: string;
protocol: string;
host: string;
Expand Down
4 changes: 2 additions & 2 deletions src/request-pipeline/header-transforms/transforms.ts
Expand Up @@ -108,7 +108,7 @@ export const requestTransforms = {

export const forcedRequestTransforms = {
[BUILTIN_HEADERS.cookie]: (_src: string, ctx: RequestPipelineContext) =>
shouldOmitCredentials(ctx) ? void 0 : ctx.session.cookies.getHeader(ctx.dest.url) || void 0,
shouldOmitCredentials(ctx) ? void 0 : ctx.session.cookies.getHeader(ctx.dest) || void 0,
};

// Response headers
Expand Down Expand Up @@ -160,7 +160,7 @@ export const responseTransforms = {
src = src.replace('ALLOW-FROM', '').trim();

const isCrossDomain = ctx.isIframe && !urlUtils.sameOriginCheck(ctx.dest.url, src);
const proxiedUrl = ctx.toProxyUrl(src, isCrossDomain, ctx.contentInfo.contentTypeUrlToken);
const proxiedUrl = ctx.toProxyUrl(src, isCrossDomain, ctx.contentInfo.contentTypeUrlToken);

return 'ALLOW-FROM ' + proxiedUrl;
},
Expand Down
26 changes: 18 additions & 8 deletions src/session/cookies.ts
Expand Up @@ -4,6 +4,7 @@ import { castArray, flattenDeep } from 'lodash';
import { parseUrl } from '../utils/url';
import { parse as parseJSON, stringify as stringifyJSON } from '../utils/json';
import { URL } from 'url';
import { DestInfo } from '../request-pipeline/context';

const LOCALHOST_DOMAIN = 'localhost';
const LOCALHOST_IP = '127.0.0.1';
Expand Down Expand Up @@ -46,11 +47,11 @@ export default class Cookies {
this._pendingSyncCookies = [];
}

_syncWrap(method: string) {
_syncWrap (method: string) {
return (...args) => {
let syncErr, syncResult;
this._cookieJar.store[method](...args, (err, result) => {
syncErr = err;
syncErr = err;
syncResult = result;
});

Expand All @@ -61,9 +62,11 @@ export default class Cookies {
};
}

static _isLocalHostDomain = (domain): boolean => domain === LOCALHOST_DOMAIN || domain === LOCALHOST_IP;

static _hasLocalhostDomain (cookie): boolean {
if (cookie)
return cookie.domain === LOCALHOST_DOMAIN || cookie.domain === LOCALHOST_IP;
return this._isLocalHostDomain(cookie.domain);

return false;
}
Expand Down Expand Up @@ -144,8 +147,8 @@ export default class Cookies {
private _findCookiesByApi (urls: Url[], key?: string): (Cookie | Cookie[])[] {
return urls.map(({ domain, path }) => {
const cookies = key
? this._findCookieSync(domain, path, key)
: this._findCookiesSync(domain, path);
? this._findCookieSync(domain, path, key)
: this._findCookiesSync(domain, path);

return cookies || [];
});
Expand Down Expand Up @@ -244,7 +247,7 @@ export default class Cookies {

for (const deletedCookie of deletedCookies) {
if (deletedCookie.domain && deletedCookie.path && deletedCookie.key) {
this._removeCookieSync(deletedCookie.domain, deletedCookie.path, deletedCookie.key);
this._removeCookieSync(deletedCookie.domain, deletedCookie.path, deletedCookie.key);

deletedCookie.expires = new Date(0);
this._pendingSyncCookies.push(deletedCookie);
Expand Down Expand Up @@ -278,7 +281,14 @@ export default class Cookies {
return this._cookieJar.getCookieStringSync(url, { http: false });
}

getHeader (url: string): string | null {
return this._cookieJar.getCookieStringSync(url, { http: true }) || null;
getHeader ({ url, hostname }: DestInfo): string | null {
// NOTE: https://github.com/DevExpress/testcafe-hammerhead/issues/2715
// Cookies with the secure attribute should be passed to the localhost server(without ssl) or any other server(with ssl).
// CookieJar only checks the protocol, but not a hostname. That is why we should to add secure attribute in case of localhost.
const cookieJarOpts = Cookies._isLocalHostDomain(hostname) ?
{ http: true, secure: true } :
{ http: true };

return this._cookieJar.getCookieStringSync(url, cookieJarOpts) || null;
}
}
43 changes: 43 additions & 0 deletions test/server/cookies-test.js
Expand Up @@ -380,6 +380,49 @@ describe('Cookies', () => {
expect(expectedCookies).eql(cookies);
});
});
describe('Attach secure cookies to request', () => {
beforeEach(() => {
cookieJar.setCookies([
{ name: 'apiCookie1', value: 'value1', domain: 'domain1.com', path: '/', secure: true },
{ name: 'apiCookie2', value: 'value2', domain: 'localhost', path: '/', secure: true },
{ name: 'apiCookie3', value: 'value3', domain: '127.0.0.1', path: '/', secure: true },
]);
});

afterEach(() => {
cookieJar.deleteCookies();
cookieJar._pendingSyncCookies = [];
});

it('Should get secure cookies string from ssl domain', () => {
const expectedCookieString = 'apiCookie1=value1';
const destInfo = { url: 'https://domain1.com/', hostname: 'domain1.com' };
const cookieStr = cookieJar.getHeader(destInfo);

expect(cookieStr).eql(expectedCookieString);
});

it('Should get secure cookies string from localhost', () => {
const expectedCookieString1 = 'apiCookie2=value2';
const expectedCookieString2 = 'apiCookie3=value3';

const destInfo1 = { url: 'http://localhost:3006', hostname: 'localhost' };
const destInfo2 = { url: 'http://127.0.0.1:3001', hostname: '127.0.0.1' };
const cookieStr1 = cookieJar.getHeader(destInfo1);
const cookieStr2 = cookieJar.getHeader(destInfo2);

expect(cookieStr1).eql(expectedCookieString1);
expect(cookieStr2).eql(expectedCookieString2);
});

it('Should return null if domain is not HTTPS', () => {
const expectedCookieString = null;
const destInfo1 = { url: 'http://domain1.com', hostname: 'domain1.com' };
const cookieStr1 = cookieJar.getHeader(destInfo1);

expect(cookieStr1).eql(expectedCookieString);
});
});

describe('Set cookies', () => {
afterEach(() => {
Expand Down