Skip to content

Commit

Permalink
Better errors for when response is already sent (#6719)
Browse files Browse the repository at this point in the history
* Better errors for when response is already sent

* Changeset

* Casing

* Change to a 300x error
  • Loading branch information
matthewp authored Mar 31, 2023
1 parent 7f8ccbe commit d54cbe4
Show file tree
Hide file tree
Showing 12 changed files with 90 additions and 3 deletions.
10 changes: 10 additions & 0 deletions .changeset/red-parents-knock.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
'astro': patch
---

Better errors for when response is already sent

This adds clearer error messaging when a Response has already been sent to the browser and the developer attempts to use:

- Astro.cookies.set
- Astro.redirect
2 changes: 2 additions & 0 deletions packages/astro/src/core/app/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export { deserializeManifest } from './common.js';

export const pagesVirtualModuleId = '@astrojs-pages-virtual-entry';
export const resolvedPagesVirtualModuleId = '\0' + pagesVirtualModuleId;
const responseSentSymbol = Symbol.for('astro.responseSent');

export interface MatchOptions {
matchNotFound?: boolean | undefined;
Expand Down Expand Up @@ -201,6 +202,7 @@ export class App {
});

const response = await renderPage(mod, ctx, this.#env);
Reflect.set(request, responseSentSymbol, true)
return response;
} catch (err: any) {
error(this.#logging, 'ssr', err.stack || err.message || String(err));
Expand Down
8 changes: 8 additions & 0 deletions packages/astro/src/core/cookies/cookies.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type { CookieSerializeOptions } from 'cookie';
import { parse, serialize } from 'cookie';
import { AstroError, AstroErrorData } from '../errors/index.js';

interface AstroCookieSetOptions {
domain?: string;
Expand Down Expand Up @@ -33,6 +34,7 @@ interface AstroCookiesInterface {

const DELETED_EXPIRATION = new Date(0);
const DELETED_VALUE = 'deleted';
const responseSentSymbol = Symbol.for('astro.responseSent');

class AstroCookie implements AstroCookieInterface {
constructor(public value: string | undefined) {}
Expand Down Expand Up @@ -160,6 +162,12 @@ class AstroCookies implements AstroCookiesInterface {
serialize(key, serializedValue, serializeOptions),
true,
]);

if((this.#request as any)[responseSentSymbol]) {
throw new AstroError({
...AstroErrorData.ResponseSentError,
});
}
}

/**
Expand Down
10 changes: 10 additions & 0 deletions packages/astro/src/core/errors/errors-data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,16 @@ See https://docs.astro.build/en/guides/server-side-rendering/ for more informati
}`,
hint: 'This is often caused by a typo in the image path. Please make sure the file exists, and is spelled correctly.',
},
/**
* @docs
* @description
* Making changes to the response, such as setting headers, cookies, and the status code cannot be done outside of page components.
*/
ResponseSentError: {
title: 'Unable to set response',
code: 3030,
message: 'The response has already been sent to the browser and cannot be altered.',
},
// No headings here, that way Vite errors are merged with Astro ones in the docs, which makes more sense to users.
// Vite Errors - 4xxx
/**
Expand Down
8 changes: 8 additions & 0 deletions packages/astro/src/core/render/result.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { AstroError, AstroErrorData } from '../errors/index.js';
import { warn, type LogOptions } from '../logger/core.js';

const clientAddressSymbol = Symbol.for('astro.clientAddress');
const responseSentSymbol = Symbol.for('astro.responseSent');

function onlyAvailableInSSR(name: 'Astro.redirect') {
return function _onlyAvailableInSSR() {
Expand Down Expand Up @@ -197,6 +198,13 @@ export function createResult(args: CreateResultArgs): SSRResult {
url,
redirect: args.ssr
? (path, status) => {
// If the response is already sent, error as we cannot proceed with the redirect.
if((request as any)[responseSentSymbol]) {
throw new AstroError({
...AstroErrorData.ResponseSentError,
});
}

return new Response(null, {
status: status || 302,
headers: {
Expand Down
2 changes: 1 addition & 1 deletion packages/astro/src/runtime/server/render/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export const decoder = new TextDecoder();
// Rendering produces either marked strings of HTML or instructions for hydration.
// These directive instructions bubble all the way up to renderPage so that we
// can ensure they are added only once, and as soon as possible.
export function stringifyChunk(result: SSRResult, chunk: string | SlotString | RenderInstruction) {
export function stringifyChunk(result: SSRResult, chunk: string | SlotString | RenderInstruction): string {
if (typeof (chunk as any).type === 'string') {
const instruction = chunk as RenderInstruction;
switch (instruction.type) {
Expand Down
3 changes: 2 additions & 1 deletion packages/astro/src/vite-plugin-astro-server/response.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ export async function writeWebResponse(res: http.ServerResponse, webResponse: Re
res.end();
}

export async function writeSSRResult(webResponse: Response, res: http.ServerResponse) {
export async function writeSSRResult(webRequest: Request, webResponse: Response, res: http.ServerResponse) {
Reflect.set(webRequest, Symbol.for('astro.responseSent'), true);
return writeWebResponse(res, webResponse);
}
2 changes: 1 addition & 1 deletion packages/astro/src/vite-plugin-astro-server/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,6 @@ export async function handleRoute(
} else {
const result = await renderPage(options);
throwIfRedirectNotAllowed(result, config);
return await writeSSRResult(result, res);
return await writeSSRResult(request, result, res);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
---
return Astro.redirect('/login');
---
12 changes: 12 additions & 0 deletions packages/astro/test/fixtures/ssr-redirect/src/pages/late.astro
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
import Redirect from '../components/redirect.astro';
---
<html>
<head>
<title>Testing</title>
</head>
<body>
<h1>Testing</h1>
<Redirect />
</body>
</html>
12 changes: 12 additions & 0 deletions packages/astro/test/ssr-redirect.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,16 @@ describe('Astro.redirect', () => {
expect(response.status).to.equal(302);
expect(response.headers.get('location')).to.equal('/login');
});

it('Warns when used inside a component', async () => {
const app = await fixture.loadTestAdapterApp();
const request = new Request('http://example.com/late');
const response = await app.render(request);
try {
const text = await response.text();
expect(false).to.equal(true);
} catch(e) {
expect(e.message).to.equal('The response has already been sent to the browser and cannot be altered.');
}
});
});
21 changes: 21 additions & 0 deletions packages/astro/test/units/cookies/error.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { expect } from 'chai';
import { AstroCookies } from '../../../dist/core/cookies/index.js';
import { apply as applyPolyfill } from '../../../dist/core/polyfill.js';

applyPolyfill();

describe('astro/src/core/cookies', () => {
describe('errors', () => {
it('Produces an error if the response is already sent', () => {
const req = new Request('http://example.com/', {});
const cookies = new AstroCookies(req);
req[Symbol.for('astro.responseSent')] = true;
try {
cookies.set('foo', 'bar');
expect(false).to.equal(true);
} catch(err) {
expect(err.errorCode).to.equal(3030);
}
});
});
});

0 comments on commit d54cbe4

Please sign in to comment.