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

Node adapter ignores server.headers under astro preview #10161

Closed
1 task
nvzqz opened this issue Feb 19, 2024 · 5 comments · Fixed by #10208
Closed
1 task

Node adapter ignores server.headers under astro preview #10161

nvzqz opened this issue Feb 19, 2024 · 5 comments · Fixed by #10208
Assignees
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) pkg: node Related to Node adapter (scope)

Comments

@nvzqz
Copy link

nvzqz commented Feb 19, 2024

Astro Info

Astro                    v4.4.0
Node                     v21.5.0
System                   macOS (arm64)
Package Manager          pnpm
Output                   server
Adapter                  @astrojs/node
Integrations             none

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

When running astro dev, the headers set by astro.config.ts are received in responses. However, they are not received when running astro preview.

The headers I'm trying to use are:

{
  "Cross-Origin-Opener-Policy": "same-origin",
  "Cross-Origin-Embedder-Policy": "require-corp",
}

What's the expected result?

Receive configured headers when running astro preview.

Link to Minimal Reproducible Example

https://github.com/nvzqz/astro-missing-headers

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Feb 19, 2024
@bluwy bluwy added - P3: minor bug An edge case that only affects very specific usage (priority) pkg: node Related to Node adapter (scope) and removed needs triage Issue needs to be triaged labels Feb 20, 2024
@log101
Copy link
Contributor

log101 commented Feb 20, 2024

#5564 Add headers to Vite preview server, but in SSR mode Node integration uses the server defined in createServer as far as I understood. Could we also pass the headers to this server? I can work on this if my understanding is correct.

@bluwy
Copy link
Member

bluwy commented Feb 20, 2024

Yeah I think it also has to accept headers there. Likely we need to expand this first though:

export interface PreviewServerParams {
outDir: URL;
client: URL;
serverEntrypoint: URL;
host: string | undefined;
port: number;
base: string;
logger: AstroIntegrationLogger;
}

So that we can get the headers from:

const createPreviewServer: CreatePreviewServer = async function (preview) {

@log101
Copy link
Contributor

log101 commented Feb 20, 2024

Alright I'll be working on that, if you don't mind.

@log101
Copy link
Contributor

log101 commented Feb 22, 2024

In order to add headers to the response, I added a 'request' listener to the preview server and modified the response inside. It worked but now the headers are added even at 404 responses. I added a response status code check to avoid it but 404 responses still goes through.

https://github.com/log101/astro/blob/f5c2dd7686ba0a0fed64b48407a2f56793eff90f/packages/integrations/node/src/preview.ts#L39-L50

This listener is added after server is created so my listener should be called last and catch the 404 reponses but it is not. What I'm missing here?

@log101
Copy link
Contributor

log101 commented Feb 23, 2024

I couldn't solve the 404 problem, if user added custom headers at server.headers config they are present at all responses including 404. I'm not sure how could this problem be solved as 404 responses seems to be handled at the main app, which merges the responses. Also is this comment is relevant to the context?:

// If you're looking at here for possible bugs, it means that it's not a bug.
// With the middleware, users can meddle with headers, and we should pass to the 404/500.
// If users see something weird, it's because they are setting some headers they should not.
//
// Although, we don't want it to replace the content-type, because the error page must return `text/html`
headers: new Headers([
...Array.from(newResponse.headers),
...Array.from(originalResponse.headers),
]),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) pkg: node Related to Node adapter (scope)
Projects
None yet
3 participants