search

activescott / serverless-aws-static-file-handler

Easily serve static files with the Serverless Framework on AWS Lambda.
MIT License
52 stars 14 forks source link

No Cors headers present. #37

Open BlueSeph28 opened 4 years ago

BlueSeph28 commented 4 years ago

Expected behavior:

Serve static Files with cors enabled. No cors headers present.

Actual behavior:

Cors error:

Access to XMLHttpRequest at 'https://[URL]' from origin 'http://localhost:8080' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource.

Steps to reproduce the problem:

Serve the static file:

event.path = file; Return fileHandler.get(event, context);

Bonus Workaround

Adding the headers it worked but I need to access to the response object.

event.path = file;
let response = await fileHandler.get(event, context)
response.headers = {
   'Content-Type': response.headers["Content-Type"],
   'Access-Control-Allow-Methods': '*',
   'Access-Control-Allow-Origin': '*'
}
return response;

Environment:

OS: Debian 10 Buster

Node Version: 10.15.3

serverless-aws-static-file-handler Version: 2.0.4

tomchiverton commented 3 years ago

@activescott why doesn't the plugin always add these ?

activescott commented 3 years ago

@tomchiverton It's a fair question. I don't have a great answer other than I never ran into it. I think I didn't pay much attention to this one honestly since there was a workaround. Now that a second person has inquired, I'm more keen to get something ironed out.

I don't see the harm in doing it by default so as long as it meets the following criteria:

  1. Responds with CORS headers only for GET or OPTIONS requests (as other requests could be state changing by the outer function - unlikely but certainly possible and I noticed the current code never actually checks request method, which kinda stinks now that I think about it).
  2. Access-Control-Allow-Methods only returns the values GET, OPTIONS
  3. There is a way for the calling function to opt out (e.g. another optional constructor param).

Am I missing anything or overthinking it? If not, you want to send a PR (with tests) or should I prepare one?

Although not technically an API-breaking change, I do wonder if it is a significant enough behavior change (especially since it arguably involves security) to be a MAJOR version bump to prevent inadvertent upgrades. What do you think?