powerman / perl-Mojolicious-Plugin-SecureCORS

Perl module: Mojolicious::Plugin::SecureCORS - Complete control over CORS
https://metacpan.org/release/Mojolicious-Plugin-SecureCORS
Other
0 stars 4 forks source link

A warning when `cors.headers` is undefined #12

Closed x-yuri closed 1 year ago

x-yuri commented 1 year ago

Consider the following app:

#!/usr/bin/env perl
use Mojolicious::Lite -signatures;
plugin 'SecureCORS';

get '/' => sub ($c) {
  $c->render;
} => 'index';

app->routes->cors('/preflight')->to('cors.origin' => '*');
del '/preflight' => {'cors.origin' => '*'} => sub ($c) {
  $c->render(text => '');
};

app->start;
__DATA__

@@ index.html.ep
<!doctype html>
<html>
<body>
  <script>
    fetch('http://b.example.com/preflight', {method: 'DELETE'})
      .then(r => console.log(r))
  </script>
</body>
</html>

When a preflight request is handled, it triggers a warning (here):

Use of uninitialized value $opt{"headers"} in split at /app/local/lib/perl5/Mojolicious/Plugin/SecureCORS.pm line 118.

I'm running perl-5.36.0, Mojolicious-9.31, Mojolicious::Plugin::SecureCORS-2.0.4.

To reproduce it you need to run it on two domains (e.g. a.example.com and b.example.com), and open http://a.example.com.

In case you decide to run it under docker, you can use the following gist (only the app slightly differs).

Also, the documentation is pretty much confusing. Check out e.g. the following Stack Overflow question.

I think you should describe how it works at the beginning. That after each request it checks if it's a CORS request (the Origin header is present), if cors.origin is provided, and they match. If that is so, it sets Access-Control-Allow-Origin, and optionally Access-Control-Allow-Credentials, Access-Control-Expose-Headers.

To make preflight requests (OPTIONS) work, one needs to define each such route explicitly with app->routes->cors(). That preflight route takes settings either from the preflight route itself (if cors.origin is set on it), or from the target route. That it checks if cors.origin is specified, if the Origin header matches it, and if Access-Control-Request-Headers matches cors.headers. If that is so, it sets Access-Control-Allow-Origin, Access-Control-Allow-Methods, and possibly Access-Control-Allow-Headers, Access-Control-Allow-Credentials, Access-Control-Max-Age.

And that options are taken from the route, the parent routes and the router. Particularly, when you set cors.origin only on the preflight route, you should do so on the target route as well. Or else, the preflight request will succeed, but the target request won't.

That's like describing the whole logic, but the logic is complex, and I think it's worth it.

And this phrase is totally unclear:

This route use "headers" condition, so you can add your own handler for OPTIONS method on same path after this one, to handle non-CORS OPTIONS requests on same path.

https://metacpan.org/pod/Mojolicious::Plugin::SecureCORS

powerman commented 1 year ago

There are several unrelated issues reported here.

Warning about undefined was an easy to fix (just released in v2.0.5).

As for documentation - yeah, writing good documentation is hard.

Detailed explanation of whole logic in most cases is considered a bad thing: it duplicates code (which result in docs became unsync with code with time and thus lying) and it is hard to read and thus ignored by users anyway.

Some details you mentioned are actually documented in CORS docs (I mean the spec https://www.w3.org/TR/2014/REC-cors-20140116/ mentioned in this module's docs). Duplicating it is probably a bad thing too. If there are some cool TL;DR article summarizing the spec in an easy to understand way - maybe it's worth to add a link to such an article to module docs.

Having said that I'm pretty sure there are a lot of ways to improve module docs (PRs are welcome).