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.

Sign up for GitHub

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: don't respond when headers already sent #2278

Merged
merged 1 commit into from
Nov 17, 2023
Merged

fix: don't respond when headers already sent #2278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 17 additions & 3 deletions src/server/HelixServer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
async handleProxyModeRequest(req, res) {
const sendFile = promisify(res.sendFile).bind(res);
const ctx = new RequestContext(req, this._project);
const { id } = ctx;
const { log } = this;
const proxyUrl = new URL(this._project.proxyUrl);

Expand All @@ -56,10 +57,19 @@
if (liveReload) {
liveReload.startRequest(ctx.requestId, ctx.path);
}
const pfx = log.level === 'silly' ? `[${id}] ` : '';

if (log.level === 'silly') {
log.trace(`${pfx}>>>--------------------------`);
log.trace(`${pfx}> ${req.method} ${req.url}`);
Object.entries(req.headers).forEach(([name, value]) => {
log.trace(`${pfx}> ${name}: ${value}`);
});
log.trace(`[${id}]`);
}

// try to serve static
try {
log.debug('trying to serve local file', filePath);
await sendFile(filePath, {
dotfiles: 'allow',
headers: {
Expand All @@ -69,9 +79,13 @@
if (liveReload) {
liveReload.registerFile(ctx.requestId, filePath);
}
log.debug(`${pfx}served local file ${filePath}`);
return;
} catch (e) {
log.debug(`Error while delivering resource ${ctx.path} - ${e.stack || e}`);
log.debug(`${pfx}unable to deliver local file ${ctx.path} - ${e.stack || e}`);
if (res.headersSent) {
return;
}

Check warning on line 88 in src/server/HelixServer.js

View check run for this annotation

Codecov / codecov/patch

src/server/HelixServer.js#L87-L88 

Added lines #L87 - L88 were not covered by tests
Comment on lines +86 to +88
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the actual fix: if serving a local file failed due to a communication error, the response was already initiated but it tried later to serve the proxied resource as well.

} finally {
if (liveReload) {
liveReload.endRequest(ctx.requestId);
Expand All @@ -92,7 +106,7 @@
file404html: this._project.file404html,
});
} catch (err) {
log.error(`Failed to proxy AEM request ${ctx.path}: ${err.message}`);
log.error(`${pfx}failed to proxy AEM request ${ctx.path}: ${err.message}`);

Check warning on line 109 in src/server/HelixServer.js

View check run for this annotation

Codecov / codecov/patch

src/server/HelixServer.js#L109 

Added line #L109 was not covered by tests
res.status(502).send(`Failed to proxy AEM request: ${err.message}`);
}

Expand Down
12 changes: 12 additions & 0 deletions src/server/RequestContext.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
import { parse } from 'url';
import utils from './utils.js';

let id = 0;

/**
* Context that is used during request handling.
*
Expand All @@ -26,6 +28,8 @@ export default class RequestContext {
const { url } = req;
this._cfg = cfg || {};
this._url = url;
// eslint-disable-next-line no-plusplus
this._id = id++;
const purl = parse(url);
this._path = purl.pathname || '/';
this._queryString = purl.search || '';
Expand Down Expand Up @@ -74,6 +78,14 @@ export default class RequestContext {
this._resourcePath = relPath;
}

/**
* returns the request id / counter
* @return {*|number}
*/
get id() {
return this._id;
}

/**
* the original request url
*/
Expand Down
55 changes: 23 additions & 32 deletions src/server/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,8 @@ window.LiveReloadOptions = {
* @return {Promise} A promise that resolves when the stream is done.
*/
async proxyRequest(ctx, url, req, res, opts = {}) {
ctx.log.debug(`Proxy ${req.method} request to ${url}`);
const { id } = ctx;
ctx.log.debug(`[${id}] Proxy ${req.method} request to ${url}`);

if (opts.cacheDirectory) {
const cached = await utils.getFromCache(url, opts.cacheDirectory, ctx.log);
Expand Down Expand Up @@ -263,19 +264,20 @@ window.LiveReloadOptions = {
});
const contentType = ret.headers.get('content-type') || 'text/plain';
const level = utils.status2level(ret.status, true);
ctx.log[level](`Proxy ${req.method} request to ${url}: ${ret.status} (${contentType})`);
ctx.log[level](`[${id}] Proxy ${req.method} request to ${url}: ${ret.status} (${contentType})`);

// because fetch decodes the response, we need to reset content encoding and length
const respHeaders = Object.fromEntries(ret.headers.entries());
delete respHeaders['content-encoding'];
delete respHeaders['content-length'];
delete respHeaders['x-frame-options'];
delete respHeaders['content-security-policy'];
delete respHeaders.connection;
respHeaders['access-control-allow-origin'] = '*';
respHeaders.via = `${ret.httpVersion ?? '1.0'} ${new URL(url).hostname}`;

if (ret.status === 404 && contentType.indexOf('text/html') === 0 && opts.file404html) {
ctx.log.debug('serve local 404.html', opts.file404html);
ctx.log.debug(`[${id}] serve local 404.html ${opts.file404html}`);
let textBody = await fs.readFile(opts.file404html, 'utf-8');
if (opts.injectLiveReload) {
textBody = utils.injectLiveReloadScript(textBody, ctx.config.server);
Expand All @@ -292,34 +294,19 @@ window.LiveReloadOptions = {
const replaceHead = isHTML && opts.headHtml;
const doIndex = isHTML && opts.indexer && url.indexOf('.plain.html') < 0;

if (ctx.log.level === 'silly') {
ctx.log.trace(`[${id}] -----------------------------`);
ctx.log.trace(`[${id}] < http/${ret.httpVersion} ${ret.status} ${ret.statusText}`);
Object.entries(ret.headers.raw()).forEach(([name, value]) => {
ctx.log.trace(`[${id}] < ${name}: ${value}`);
});
}

if (isHTML) {
let respBody;
let textBody;
if (contentType.startsWith('text/')) {
textBody = await ret.text();
} else {
respBody = await ret.buffer();
}
const lines = ['----------------------------->'];
let textBody = await ret.text();
if (ctx.log.level === 'silly') {
lines.push(`${req.method} ${url}`);
Object.entries(headers).forEach(([name, value]) => {
lines.push(`${name}: ${value}`);
});
lines.push('');
lines.push('<-----------------------------');
lines.push('');
lines.push(`http/${ret.httpVersion} ${ret.status} ${ret.statusText}`);
ret.headers.forEach((name, value) => {
lines.push(`${name}: ${value}`);
});
lines.push('');
if (respBody) {
lines.push(`<binary ${respBody.length} bytes>`);
} else {
lines.push(textBody);
}
ctx.log.trace(lines.join('\n'));
ctx.log.trace(textBody);
ctx.log.trace(`[${id}] <<<--------------------------`);
}
if (replaceHead) {
await opts.headHtml.setCookie(req.headers.cookie);
Expand All @@ -344,7 +331,7 @@ window.LiveReloadOptions = {
url,
opts.cacheDirectory,
{
body: respBody || textBody,
body: textBody,
headers: respHeaders,
status: ret.status,
},
Expand All @@ -353,12 +340,16 @@ window.LiveReloadOptions = {
}

res
.status(ret.status)
.set(respHeaders)
.send(respBody || textBody);
.status(ret.status)
.send(textBody);
return;
}

if (ctx.log.level === 'silly') {
ctx.log.trace(`[${id}] <<<--------------------------`);
}

if (opts.cacheDirectory) {
const buffer = await ret.buffer();
await utils.writeToCache(
Expand Down
Loading