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

feat: add CORS filter #8649

Merged
merged 27 commits into from
Mar 30, 2024
Merged

feat: add CORS filter #8649

merged 27 commits into from
Mar 30, 2024

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Mar 22, 2024

Description

  • add Cors filter class
  • add HTTP\Cors class

Screenshot 2024-03-25 at 10-06-33 Cross-Origin Resource Sharing (CORS) — CodeIgniter 4 4 6 documentation

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis marked this pull request as draft March 22, 2024 08:41
@kenjis kenjis added 4.5 enhancement PRs that improve existing functionalities labels Mar 22, 2024
@kenjis kenjis force-pushed the feat-cors branch 6 times, most recently from 2a3cb63 to 4dfb2d0 Compare March 23, 2024 00:51
@kenjis kenjis changed the title feat: add Cors filter feat: add CORS filter Mar 23, 2024
@kenjis kenjis force-pushed the feat-cors branch 4 times, most recently from 3712869 to 86ee415 Compare March 24, 2024 05:12
@kenjis kenjis marked this pull request as ready for review March 24, 2024 09:40
@kenjis
Copy link
Member Author

kenjis commented Mar 24, 2024

Now the CORS filter always adds Vary: Origin header.
But when the following three requests come:

(1) Plain OPTIONS request (CORS filter does not handle)

OPTIONS /

(2) Invalid preflight request (CORS filter does not handle)

OPTIONS /
Origin: https://app.example.com/

(3) Normal preflight request (CORS filter does handle)

OPTIONS /
Origin: https://app.example.com/
Access-Control-Request-Method: PUT

(1) and (2) are cached (if there is an intermediate cache to cache them) and the cached (2) will be returned to the request (3). So the current implementation is not good.

(Added)
This was fixed. Now it always adds Vary: Access-Control-Request-Method header.

@kenjis kenjis marked this pull request as draft March 24, 2024 12:50
kenjis added a commit to kenjis/CodeIgniter4 that referenced this pull request Mar 25, 2024
kenjis added a commit to kenjis/CodeIgniter4 that referenced this pull request Mar 25, 2024
@kenjis kenjis marked this pull request as ready for review March 25, 2024 01:05
Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice feature - well documented.

*
* @see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Max-Age
*/
'maxAge' => 7200,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is your criteria for choosing this number by default? Laravel assumes 0.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the same as session expiration.
I think the CORS config is not often changed, so it may be better to be longer.

tests/system/Filters/CorsTest.php Outdated Show resolved Hide resolved
// ...
public array $filters = [
// ...
'cors' => [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to use a different configuration (exactly like ['filter' => 'cors:api'] )? If yes, please document.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a link. 3a7e880

Copy link
Contributor

@datamweb datamweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks.

@yasinpalizban, This PR may be interesting for you.

@kenjis kenjis merged commit 62bcfa8 into codeigniter4:4.5 Mar 30, 2024
41 checks passed
@kenjis kenjis deleted the feat-cors branch March 30, 2024 23:52
@krangadurai
Copy link

krangadurai commented Jun 17, 2024

hi , im reccently try cors thing , ,i cann't connect cors way i don't know why its happening,
Screenshot 2024-06-17 172123
Screenshot (5)
Screenshot 2024-06-17 172319

@krangadurai
Copy link

krangadurai commented Jun 17, 2024

Access-Control-Allow-Headers not working

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.5 enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants