-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add ability to disable cookie processing altogether by setting cookie… #123
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -166,7 +166,7 @@ describe( `context (${version} over ${proto.replace( ":", "" )})`, ( ) => | |
{ | ||
it( "should be able to specify custom cookie jar", async ( ) => | ||
{ | ||
const { server, port } = await makeServer( ); | ||
const { server, port, receivedCookies: serverReceivedCookies } = await makeServer( ); | ||
|
||
const cookieJar = new CookieJar( ); | ||
|
||
|
@@ -181,11 +181,14 @@ describe( `context (${version} over ${proto.replace( ":", "" )})`, ( ) => | |
userAgent: "foobar", | ||
} ); | ||
|
||
await fetch( `${proto}//localhost:${port}/set-cookie`, { | ||
const response1 = await fetch( `${proto}//localhost:${port}/set-cookie`, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [minor]: It's batter to avoid numbers in a variable name. It seems like a code smell. If you realised that you need more then one response, so just describe in the name of these responses what you expect. |
||
json: [ "a=b" , "c=d" ], | ||
method: "POST", | ||
allowForbiddenHeaders: true, | ||
} ); | ||
|
||
expect(serverReceivedCookies).toHaveLength(0); | ||
expect(response1.headers.get('set-cookie')).not.toBeNull(); | ||
const cookies = | ||
await cookieJar.getCookies( `${proto}//localhost:${port}/` ); | ||
|
||
|
@@ -198,6 +201,8 @@ describe( `context (${version} over ${proto.replace( ":", "" )})`, ( ) => | |
// Next request should maintain cookies | ||
|
||
await fetch( `${proto}//localhost:${port}/echo` ); | ||
expect(serverReceivedCookies.length).toBe(1); | ||
expect(serverReceivedCookies).toEqual(["a=b; c=d"]); | ||
|
||
const cookies2 = | ||
await cookieJar.getCookies( `${proto}//localhost:${port}/` ); | ||
|
@@ -207,10 +212,13 @@ describe( `context (${version} over ${proto.replace( ":", "" )})`, ( ) => | |
// If we manually clear the cookie jar, subsequent requests | ||
// shouldn't have any cookies | ||
|
||
serverReceivedCookies.splice(0); | ||
cookieJar.reset( ); | ||
|
||
await fetch( `${proto}//localhost:${port}/echo` ); | ||
|
||
expect(serverReceivedCookies.length).toBe(0); | ||
|
||
const cookies3 = | ||
await cookieJar.getCookies( `${proto}//localhost:${port}/` ); | ||
|
||
|
@@ -221,7 +229,74 @@ describe( `context (${version} over ${proto.replace( ":", "" )})`, ( ) => | |
await server.shutdown( ); | ||
} ); | ||
|
||
it( "shouldn't be able to read cookie headers be default", async ( ) => | ||
it( "should be able to specify undefined cookie jar to indicate not to manage cookies", async ( ) => | ||
{ | ||
const { server, port, receivedCookies: serverReceivedCookies } = await makeServer( ); | ||
|
||
const { disconnectAll, fetch } = context( { | ||
...cycleOpts, | ||
cookieJar: undefined, | ||
overwriteUserAgent: true, | ||
userAgent: "foobar", | ||
} ); | ||
|
||
await fetch( `${proto}//localhost:${port}/set-cookie`, { | ||
json: [ "a=b" , "c=d" ], | ||
method: "POST", | ||
} ); | ||
|
||
expect(serverReceivedCookies).toHaveLength(0); | ||
|
||
// Next request should not return cookies | ||
await fetch( `${proto}//localhost:${port}/set-cookie`, { | ||
json: [ "x=y" , "y=z" ], | ||
method: "POST", | ||
} ); | ||
|
||
expect(serverReceivedCookies).toHaveLength(0); | ||
|
||
await fetch( `${proto}//localhost:${port}/echo` ); | ||
|
||
expect(serverReceivedCookies).toHaveLength(0); | ||
|
||
disconnectAll( ); | ||
|
||
await server.shutdown( ); | ||
} ); | ||
|
||
it( "should be able to not specify cookie jar and have its setup by default, cookies working", async ( ) => | ||
{ | ||
const { server, port, receivedCookies: serverReceivedCookies } = await makeServer( ); | ||
|
||
|
||
const { disconnectAll, fetch } = context( { | ||
...cycleOpts, | ||
overwriteUserAgent: true, | ||
userAgent: "foobar", | ||
} ); | ||
|
||
const response1 = await fetch( `${proto}//localhost:${port}/set-cookie`, { | ||
json: [ "a=b" , "c=d" ], | ||
method: "POST", | ||
allowForbiddenHeaders: true, | ||
} ); | ||
|
||
expect(serverReceivedCookies).toHaveLength(0); | ||
expect(response1.headers.get('set-cookie')).not.toBeNull(); | ||
|
||
// Next request should maintain cookies | ||
|
||
await fetch( `${proto}//localhost:${port}/echo` ); | ||
expect(serverReceivedCookies.length).toBe(1); | ||
expect(serverReceivedCookies).toEqual(["a=b; c=d"]); | ||
|
||
disconnectAll( ); | ||
|
||
await server.shutdown( ); | ||
} ); | ||
|
||
|
||
it( "shouldn't be able to read cookie headers by default", async ( ) => | ||
{ | ||
const { server, port } = await makeServer( ); | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -41,10 +41,10 @@ export interface ServerOptions | |||||
export abstract class Server | ||||||
{ | ||||||
public port: number | null = null; | ||||||
public receivedCookies: Array<string> = new Array<string>(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [minor]: It is not a good idea to use constructors as types.
Suggested change
|
||||||
protected _opts: ServerOptions = { }; | ||||||
protected _server: HttpServer | HttpsServer | Http2Server = < any >void 0; | ||||||
|
||||||
|
||||||
public async listen( port: number | undefined = void 0 ): Promise< number > | ||||||
{ | ||||||
return new Promise< void >( ( resolve, _reject ) => | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -95,6 +95,10 @@ export class ServerHttp1 extends TypedServer< HttpServer | HttpsServer > | |||||
if ( path == null ) | ||||||
throw new Error( "Internal test error" ); | ||||||
|
||||||
if (request.headers.cookie) { | ||||||
this.receivedCookies.push(request.headers.cookie); | ||||||
} | ||||||
|
||||||
const sendHeaders = ( headers: RawHeaders ) => | ||||||
{ | ||||||
const { ":status": status = 200, ...rest } = { ...headers }; | ||||||
|
@@ -310,11 +314,11 @@ export class ServerHttp1 extends TypedServer< HttpServer | HttpsServer > | |||||
} | ||||||
|
||||||
export async function makeServer( opts: ServerOptions = { } ) | ||||||
: Promise< { server: Server; port: number | null; } > | ||||||
: Promise< { server: Server; port: number | null; receivedCookies: Array<string> } > | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [minor]: As i described before use constructors as types is not a good idea.
Suggested change
|
||||||
{ | ||||||
opts = opts || { }; | ||||||
|
||||||
const server = new ServerHttp1( opts ); | ||||||
await server.listen( opts.port ); | ||||||
return { server, port: server.port }; | ||||||
return { server, port: server.port, receivedCookies: server.receivedCookies }; | ||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -92,6 +92,10 @@ export class ServerHttp2 extends TypedServer< Http2Server > | |||||
const path = headers[ HTTP2_HEADER_PATH ] as string; | ||||||
let m; | ||||||
|
||||||
if (headers.cookie) { | ||||||
this.receivedCookies.push(headers.cookie); | ||||||
} | ||||||
|
||||||
if ( path === "/headers" ) | ||||||
{ | ||||||
stream.respond( { | ||||||
|
@@ -129,7 +133,6 @@ export class ServerHttp2 extends TypedServer< Http2Server > | |||||
( < any >responseHeaders[ HTTP2_HEADER_SET_COOKIE ] ) | ||||||
.push( cookie ); | ||||||
} ); | ||||||
|
||||||
stream.respond( responseHeaders ); | ||||||
stream.end( ); | ||||||
} | ||||||
|
@@ -360,11 +363,11 @@ export class ServerHttp2 extends TypedServer< Http2Server > | |||||
} | ||||||
|
||||||
export async function makeServer( opts: ServerOptions = { } ) | ||||||
: Promise< { server: Server; port: number | null; } > | ||||||
: Promise< { server: Server; port: number | null; receivedCookies: Array<string> } > | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [minor]: As i described before use constructors as types is not a good idea.
Suggested change
|
||||||
{ | ||||||
opts = opts || { }; | ||||||
|
||||||
const server = new ServerHttp2( opts ); | ||||||
await server.listen( opts.port ); | ||||||
return { server, port: server.port }; | ||||||
return { server, port: server.port, receivedCookies: server.receivedCookies }; | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor]: since you need to use the same type in another place, so I would suggest that you make this an alias.