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

Global cors and security options not respected in 404 #3792

Closed
erfanio opened this issue May 17, 2018 · 9 comments
Closed

Global cors and security options not respected in 404 #3792

erfanio opened this issue May 17, 2018 · 9 comments
Assignees
Labels
Milestone

Comments

@erfanio
Copy link

@erfanio erfanio commented May 17, 2018

What are you trying to achieve or the steps to reproduce?

From API docs:

If set to 'ignore', any incoming Origin header is ignored (present or not) and the 'Access-Control-Allow-Origin' header is set to '*'

I have this in my hapi server configs:

...
    routes: {
      cors: { origin: 'ignore' },
...

This works fine most of the time
image

But when a route doesn't exist, the 404 response doesn't include the cors headers.
image

What did you expect?

I expect to have access-control-allow-origin: * on the 404

Context

  • node version: 10
  • hapi version: 17.4
  • os: Linux
@vladikoff
Copy link

@vladikoff vladikoff commented Jun 19, 2018

I was about to file this bug, but saw yours.

This can be reproduced with this:

git clone https://github.com/vladikoff/issue-hapi-17-404-headers.git
cd issue-hapi-17-404-headers
node index.js

Navigate to http://localhost:3000

Existing route:

image

404 route: http://localhost:3000/404something

Response headers are missing security route options:

image

This seems like a regression in Hapi 17. In hapi 16 this worked fine, we have testa covering this behaviour.

@vladikoff
Copy link

@vladikoff vladikoff commented Jun 19, 2018

Sample code:

'use strict';

const Hapi = require('hapi');

const server = Hapi.server({
  port: 3000,
  host: 'localhost',
  routes: {
    cors: true,
    payload: {
      maxBytes: 16384
    },
    security: {
      hsts: {
        maxAge: 15552000,
        includeSubdomains: true
      },
      xframe: true,
      xss: true,
      noOpen: false,
      noSniff: true
    },
  }
});

server.route({
  method: 'GET',
  path: '/',
  handler: (request, h) => {

    return 'Hello, world!';
  }
});

server.route({
  method: 'GET',
  path: '/hello',
  handler: (request, h) => {

    return 'Hello, ' + encodeURIComponent(request.params.name) + '!';
  }
});

const init = async () => {

  await server.start();
  console.log(`Server running at: ${server.info.uri}`);
};

process.on('unhandledRejection', (err) => {

  console.log(err);
  process.exit(1);
});

init();

@vladikoff
Copy link

@vladikoff vladikoff commented Jun 20, 2018

Workaround mentioned here: #3658 (comment)

@bmgdev
Copy link

@bmgdev bmgdev commented Jul 25, 2018

@vladikoff have you seen any official fix for adding a global cors: true? I'm bewildered how its been this long without a fix. This seems like a giant use case/issue for most people.

@hueniverse hueniverse added the bug label Aug 1, 2018
@hueniverse hueniverse self-assigned this Aug 1, 2018
@hueniverse hueniverse added this to the 17.5.3 milestone Aug 1, 2018
@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Aug 1, 2018

This is actually two separate issues. v17 broke the security headers on 404 responses. CORS header never worked on v16 either. I am trying to decide if 404 should honor CORS headers settings and if it should, if this is a bug fix or a breaking change...

@nlf @kanongil thoughts?

@bmgdev
Copy link

@bmgdev bmgdev commented Aug 1, 2018

2018-08-01_00-32-18

complete oversight, but i was able to add this to my manifest. had no idea, so simple.

@kanongil
Copy link
Contributor

@kanongil kanongil commented Aug 1, 2018

Hmm, tricky. The docs, could both mean every defined route, or every route (including built-in). I would probably expect it to just apply to all responses (contrary to the current implementation).

Adding CORS headers to unrouted 404 responses will add a bit of extra processing, and some extra bytes to the responses. There is also a remote possibility that it can trigger a bug in a browser app, due to receiving the actual 404 response instead of a CORS fail.

For reference:

server.options.routes

Default value: none.

A route options object used as the default configuration for every route.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Aug 1, 2018

due to receiving the actual 404 response instead of a CORS fail

It will not change the 404 response, just add a few headers as configured for the other routes.

@hueniverse hueniverse changed the title global cors options not respected in 404 Global cors and security options not respected in 404 Aug 1, 2018
@hueniverse hueniverse closed this in 583ea0a Aug 1, 2018
@lock
Copy link

@lock lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants