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

Header not added inside error handling middleware #109

Closed
rodrigoreis22 opened this issue May 14, 2019 · 6 comments
Closed

Header not added inside error handling middleware #109

rodrigoreis22 opened this issue May 14, 2019 · 6 comments

Comments

@rodrigoreis22
Copy link

@rodrigoreis22 rodrigoreis22 commented May 14, 2019

lambda-api version: 0.10.1

I have a middleware function to handle errors:

function errorHandler(err, req, res, next) {
  res.header('Access-Control-Allow-Origin', '*');
  console.error(err);
  if (err && err.message && err.message.toUpperCase() === 'UNAUTHORIZED') {
    return res.error(401, 'Unauthorized');
  }
  next();
}

I want this CORS headers to be added on every response, including the error ones.
In fact I already have another middleware that was supposed to be doing this, but it doesn't work:

function addCorsHeaders(req, res, next) {
  res.header('Access-Control-Allow-Origin', '*');
  next();
}

My api routing file:

const api = require('lambda-api')({ base: 'v2' })
  , middleware = require('../utils/middleware')
  , OEmbedApi = require('../apis/oEmbedApi');

api.use(middleware.addCorsHeaders);
api.use(middleware.populateCommonHeaders);
api.use(middleware.logRequest);
api.use(middleware.errorHandler);

api.get('/oembed/videoproxy', async (req, res) => {
  let videoUrl = await OEmbedApi.videoProxy(req.query.url);
  return res.redirect(307, videoUrl);
});

api.get('/oembed/imageproxy', async (req, res) => {
  let imageUrl = await OEmbedApi.imageProxy(req.query.url);
  return res.redirect(307, imageUrl);
});

api.post('/oembed/fetch', async (req, res) => {
  let response = await OEmbedApi.oEmbed(req.body.url, req.body.nativeCard);
  return res.json(response);
});

module.exports = api;

When the request fails with 401 - Unauthorized the header Access-Control-Allow-Origin is not added.

This is my configuration on serverless.yml for this api:

oembed:
    handler: handler_oembed.oembed
    timeout: 20
    memorySize: 2536
    events:
      - http:
          path: oembed/{proxy+}
          method: ANY
          cors:
            origin: '*'
            headers:
              - Content-Type
              - X-Amz-Date
              - Authorization
              - X-Api-Key
              - X-Amz-Security-Token
              - X-Amz-User-Agent
              - Preferred-Language
          cacheControl: 'max-age=3600, s-maxage=3600, proxy-revalidate'
@rodrigoreis22

This comment has been minimized.

Copy link
Author

@rodrigoreis22 rodrigoreis22 commented May 14, 2019

is it related to this line?

// Strip the headers (TODO: find a better way to handle this)

@jeremydaly

This comment has been minimized.

Copy link
Owner

@jeremydaly jeremydaly commented May 14, 2019

Hi @rodrigoreis22,

Yes, the headers get stripped out when there is an error. As you can see from the code, it is a less than ideal solution.

Any thoughts on how to handle this? Should there be a whitelist of headers for errors handlers? Or, should error middleware be required to generate their own headers? Other ideas?

Thanks,
Jeremy

@rodrigoreis22

This comment has been minimized.

Copy link
Author

@rodrigoreis22 rodrigoreis22 commented May 14, 2019

Hi @jeremydaly ,

Why are they stripped out when there is an error?

I think at least the CORS headers should always be included. Maybe implementing a whitelist would make it more flexible, but in this solution the CORS headers would be on the whitelist by default.

@jeremydaly

This comment has been minimized.

Copy link
Owner

@jeremydaly jeremydaly commented May 23, 2019

I’m not sure the whitelist is the best way to handle it. I need to do some more research on this to make sure that if follows proper guidelines. I suppose we could publish a patch that removes the header stripping, but I don’t want it to cause compatibility issues.

@rodrigoreis22

This comment has been minimized.

Copy link
Author

@rodrigoreis22 rodrigoreis22 commented May 23, 2019

Maybe an option that can be set to false, meaning headers will not be stripped out on error responses. Default is true, keeping the same behavior as is today, to maintain compatibility.

hussfelt added a commit to proxyco/lambda-api that referenced this issue Sep 2, 2019
hussfelt added a commit to proxyco/lambda-api that referenced this issue Oct 28, 2019
jeremydaly added a commit that referenced this issue Oct 29, 2019
#109 Implement a whitelist for headers as an option to LambdaAPI
@rodrigoreis22

This comment has been minimized.

Copy link
Author

@rodrigoreis22 rodrigoreis22 commented Jan 18, 2020

Just saw that this was fixed on https://github.com/jeremydaly/lambda-api/releases/tag/v0.10.3 . Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.