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

Better errors for when response is already sent #6719

Merged
merged 4 commits into from
Mar 31, 2023
Merged
Show file tree
Hide file tree
Changes from 3 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
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
12 changes: 12 additions & 0 deletions packages/astro/src/core/errors/errors-data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -900,6 +900,18 @@ See https://docs.astro.build/en/guides/server-side-rendering/ for more informati
hint: 'See https://docs.astro.build/en/guides/content-collections/ for more on the `slug` field.',
},

/**
* @docs
* @message The response cannot be altered after it has already been sent.
matthewp marked this conversation as resolved.
Show resolved Hide resolved
* @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: 9004,
matthewp marked this conversation as resolved.
Show resolved Hide resolved
message: 'The response has already been sent to the browser and cannot be altered.',
Copy link
Member

Choose a reason for hiding this comment

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

Beautiful message. Docs is happy!

},

// Generic catch-all - Only use this in extreme cases, like if there was a cosmic ray bit flip
UnknownError: {
title: 'Unknown Error.',
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(9004);
}
});
});
});