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

allow any type for customResponseHeaders config #66689

Merged
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/core/server/http/cookie_session_storage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ configService.atPath.mockReturnValue(
disableProtection: true,
whitelist: [],
},
customResponseHeaders: {},
} as any)
);

Expand Down
47 changes: 46 additions & 1 deletion src/core/server/http/http_config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
*/

import uuid from 'uuid';
import { config } from '.';
import { config, HttpConfig } from './http_config';
import { CspConfig } from '../csp';

const validHostnames = ['www.example.com', '8.8.8.8', '::1', 'localhost'];
const invalidHostname = 'asdf$%^';
Expand Down Expand Up @@ -107,6 +108,23 @@ test('throws if xsrf.whitelist element does not start with a slash', () => {
);
});

test('accepts any type of objects for custom headers', () => {
const httpSchema = config.schema;
const obj = {
customResponseHeaders: {
string: 'string',
bool: true,
number: 12,
array: [1, 2, 3],
nested: {
foo: 1,
bar: 'dolly',
},
},
};
expect(() => httpSchema.validate(obj)).not.toThrow();
});

describe('with TLS', () => {
test('throws if TLS is enabled but `redirectHttpFromPort` is equal to `port`', () => {
const httpSchema = config.schema;
Expand Down Expand Up @@ -173,3 +191,30 @@ describe('with compression', () => {
expect(() => httpSchema.validate(obj)).toThrowErrorMatchingSnapshot();
});
});

describe('HttpConfig', () => {
it('converts customResponseHeaders to strings or arrays of strings', () => {
const httpSchema = config.schema;
const rawConfig = httpSchema.validate({
customResponseHeaders: {
string: 'string',
bool: true,
number: 12,
array: [1, 2, 3],
nested: {
foo: 1,
bar: 'dolly',
},
},
});
const httpConfig = new HttpConfig(rawConfig, CspConfig.DEFAULT);

expect(httpConfig.customResponseHeaders).toEqual({
string: 'string',
bool: 'true',
number: '12',
array: ['1', '2', '3'],
nested: '{"foo":1,"bar":"dolly"}',
});
});
});
18 changes: 15 additions & 3 deletions src/core/server/http/http_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export const config = {
),
schema.boolean({ defaultValue: false })
),
customResponseHeaders: schema.recordOf(schema.string(), schema.string(), {
customResponseHeaders: schema.recordOf(schema.string(), schema.any(), {
defaultValue: {},
}),
host: schema.string({
Expand Down Expand Up @@ -136,7 +136,7 @@ export class HttpConfig {
public socketTimeout: number;
public port: number;
public cors: boolean | { origin: string[] };
public customResponseHeaders: Record<string, string>;
public customResponseHeaders: Record<string, string | string[]>;
public maxPayload: ByteSizeValue;
public basePath?: string;
public rewriteBasePath: boolean;
Expand All @@ -153,7 +153,15 @@ export class HttpConfig {
this.host = rawHttpConfig.host;
this.port = rawHttpConfig.port;
this.cors = rawHttpConfig.cors;
this.customResponseHeaders = rawHttpConfig.customResponseHeaders;
this.customResponseHeaders = Object.entries(rawHttpConfig.customResponseHeaders ?? {}).reduce(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lot of tests are using partials of the config. I tried to fix them, but adding customResponseHeaders: {} in some was breaking snapshots or even assertions about the received config, so I choose to add this ?? {} to avoid breaking tests.

(headers, [key, value]) => {
return {
...headers,
[key]: Array.isArray(value) ? value.map(e => convertHeader(e)) : convertHeader(value),
};
},
{}
);
this.maxPayload = rawHttpConfig.maxPayload;
this.name = rawHttpConfig.name;
this.basePath = rawHttpConfig.basePath;
Expand All @@ -166,3 +174,7 @@ export class HttpConfig {
this.xsrf = rawHttpConfig.xsrf;
}
}

const convertHeader = (entry: any): string => {
return typeof entry === 'object' ? JSON.stringify(entry) : String(entry);
};
Comment on lines +178 to +180
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To get exactly the previous behavior back, while keeping the headers conversion logic inside core instead of delegating it to hapi as it was done in legacy (where any type of value were sent as headers, letting hapi handle the conversion), I had to use this mixed JSON.stringify + String(of) approach.

Tell me if this can be improved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to avoid breaking the previous behavior.

1 change: 1 addition & 0 deletions src/core/server/http/test_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ configService.atPath.mockReturnValue(
disableProtection: true,
whitelist: [],
},
customResponseHeaders: {},
} as any)
);

Expand Down