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

feat: adds more standard compliant request body handling #2260

Merged
merged 1 commit into from
Apr 12, 2023
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
7 changes: 4 additions & 3 deletions packages/http-server/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,10 @@ function parseRequestBody(request: IncomingMessage) {
if (
// If the body size is null, it means the body itself is null so the promise can resolve with a null value
request.headers['content-length'] === '0' ||
(request.headers['content-type'] === undefined &&
request.headers['transfer-encoding'] === undefined &&
request.headers['content-length'] === undefined)
// Per HTTP 1.1 - these 2 headers are the valid way to indicate that a body exists:
// > The presence of a message body in a request is signaled by a Content-Length or Transfer-Encoding header field.
// https://httpwg.org/specs/rfc9112.html#message.body
(request.headers['transfer-encoding'] === undefined && request.headers['content-length'] === undefined)
) {
return Promise.resolve(null);
}
Expand Down
7 changes: 7 additions & 0 deletions packages/http/src/forwarder/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,10 @@ export const UPSTREAM_NOT_IMPLEMENTED: Omit<ProblemJson, 'detail'> = {
title: 'The server does not support the functionality required to fulfill the request',
status: 501,
};

export const PROXY_UNSUPPORTED_REQUEST_BODY: Omit<ProblemJson, 'detail'> = {
type: 'PROXY_UNSUPPORTED_REQUEST_BODY',
title:
'The Prism proxy does not support sending a GET/HEAD request with a message body to an upstream server. See: https://github.com/stoplightio/prism/issues/2259',
status: 501,
};
26 changes: 18 additions & 8 deletions packages/http/src/forwarder/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { parseResponse } from '../utils/parseResponse';
import { hopByHopHeaders } from './resources';
import { createUnauthorisedResponse, createUnprocessableEntityResponse } from '../mocker';
import { ProblemJsonError } from '../types';
import { UPSTREAM_NOT_IMPLEMENTED } from './errors';
import { PROXY_UNSUPPORTED_REQUEST_BODY, UPSTREAM_NOT_IMPLEMENTED } from './errors';
import * as createHttpProxyAgent from 'http-proxy-agent';
import * as createHttpsProxyAgent from 'https-proxy-agent';
import { toURLSearchParams } from '../utils/url';
Expand Down Expand Up @@ -44,7 +44,7 @@ const forward: IPrismComponents<IHttpOperation, IHttpRequest, IHttpResponse, IHt
: createUnprocessableEntityResponse(failedValidations);
}),
TE.swap,
TE.chainEitherK(() => serializeBody(input.body)),
TE.chainEitherK(() => serializeBodyForFetch(input, logger)),
TE.chain(body =>
TE.tryCatch(async () => {
const partialUrl = new URL(baseUrl);
Expand Down Expand Up @@ -98,13 +98,23 @@ const forward: IPrismComponents<IHttpOperation, IHttpRequest, IHttpResponse, IHt

export default forward;

function serializeBodyForFetch(input: IHttpRequest, logger: Logger): E.Either<Error, string | undefined> {
const upperMethod = input.method.toUpperCase();
if (['GET', 'HEAD'].includes(upperMethod) && ![null, undefined].includes(input.body as any)) {
logger.warn(`Upstream ${upperMethod} call to ${input.url.path} has request body`);
return E.left(ProblemJsonError.fromTemplate(PROXY_UNSUPPORTED_REQUEST_BODY));
}

return serializeBody(input.body);
}

export function serializeBody(body: unknown): E.Either<Error, string | undefined> {
if (typeof body === 'string') {
return E.right(body);
}

if (body) return pipe(J.stringify(body), E.mapLeft(E.toError));

return E.right(undefined);
}

Expand All @@ -113,21 +123,21 @@ function logForwardRequest({ logger, url, request }: { logger: Logger; url: stri
logger.info(`${prefix}Forwarding "${request.method}" request to ${url}...`);
logRequest({ logger, prefix, ...pick(request, 'body', 'headers') });
}

function forwardResponseLogger(logger: Logger) {
return (response: Response) => {
const prefix = chalk.grey('< ');

logger.info(`${prefix}Received forward response`);

const { status: statusCode } = response;

logResponse({
logger,
statusCode,
...pick(response, 'body', 'headers'),
prefix,
});
});

return response;
};
Expand Down
18 changes: 18 additions & 0 deletions test-harness/specs/proxy/proxy.get-null-body.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
====test====
Making a GET request with a Content-Type header through the proxy server ignores null body
====spec====
swagger: "2.0"
paths:
/status/204:
get:
produces:
- text/plain
responses:
204:
description: No Content
====server====
proxy -p 4010 ${document} http://httpbin.org
====command====
curl -i http://localhost:4010/status/204 -X GET --header 'Content-Type: application/json'
====expect====
HTTP/1.1 204 No Content
20 changes: 20 additions & 0 deletions test-harness/specs/proxy/proxy.get-with-body.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
====test====
Making a GET request with request body through the proxy server rejects with 501
====spec====
swagger: "2.0"
paths:
/status/204:
get:
produces:
- text/plain
responses:
204:
description: No Content
====server====
proxy -p 4010 ${document} http://httpbin.org
====command====
curl -i http://localhost:4010/status/204 -X GET --header 'Content-Type: application/json' --data '{}'
Copy link
Contributor

Choose a reason for hiding this comment

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

I've never tried to do this before. Does '{}' get serialized to a zero-length request body?

Copy link
Author

Choose a reason for hiding this comment

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

yea - its a verbatim string here - so the request body is just {}

====expect====
HTTP/1.1 501 Not Implemented

{"type":"https://stoplight.io/prism/errors#PROXY_UNSUPPORTED_REQUEST_BODY","title":"The Prism proxy does not support sending a GET/HEAD request with a message body to an upstream server. See: https://github.com/stoplightio/prism/issues/2259","status":501,"detail":""}