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 security header #2341

Merged
merged 3 commits into from
Mar 14, 2022
Merged

feat: add security header #2341

merged 3 commits into from
Mar 14, 2022

Conversation

zaunist
Copy link
Contributor

@zaunist zaunist commented Feb 24, 2022

Please answer these questions before submitting a pull request, or your PR will get closed.

Why submit this pull request?

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches

What changes will this PR take into?

Please update this section with detailed description.

Related issues

fix/ #2340

Checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

@codecov-commenter
Copy link

codecov-commenter commented Feb 24, 2022

Codecov Report

Merging #2341 (2b59686) into master (fd44364) will decrease coverage by 19.07%.
The diff coverage is 0.00%.

❗ Current head 2b59686 differs from pull request most recent head 9a1dab9. Consider uploading reports for the commit 9a1dab9 to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #2341       +/-   ##
===========================================
- Coverage   68.17%   49.10%   -19.08%     
===========================================
  Files         127       41       -86     
  Lines        3375     3179      -196     
  Branches      830        0      -830     
===========================================
- Hits         2301     1561      -740     
- Misses       1074     1419      +345     
- Partials        0      199      +199     
Flag Coverage Δ
backend-unit-test 49.10% <0.00%> (?)
frontend-e2e-test ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
api/internal/filter/cors.go 0.00% <0.00%> (ø)
web/src/components/Plugin/UI/limit-count.tsx
web/src/components/Plugin/data.tsx
web/src/components/Upstream/UpstreamForm.tsx
...omponents/Upstream/components/UpstreamSelector.tsx
web/src/helpers.tsx
web/src/pages/Consumer/Create.tsx
web/src/pages/Consumer/List.tsx
web/src/pages/Consumer/components/Step1.tsx
web/src/pages/PluginTemplate/List.tsx
... and 159 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbecd55...9a1dab9. Read the comment docs.

@zaunist
Copy link
Contributor Author

zaunist commented Mar 9, 2022

@nic-chen @bzp2010 PTAL, thanks.

@zaunist
Copy link
Contributor Author

zaunist commented Mar 10, 2022

@starsz Please take a look, thanks.

@@ -60,6 +60,14 @@ conf:
# such as absolute path on Windows: winfile:///C:\access.log
# log example: 2020-12-09T16:38:09.039+0800 INFO filter/logging.go:46 /apisix/admin/routes/r1 {"status": 401, "host": "127.0.0.1:9000", "query": "asdfsafd=adf&a=a", "requestId": "3d50ecb8-758c-46d1-af5b-cd9d1c820156", "latency": 0, "remoteIP": "127.0.0.1", "method": "PUT", "errs": []}
max_cpu: 0 # supports tweaking with the number of OS threads are going to be used for parallelism. Default value: 0 [will use max number of available cpu cores considering hyperthreading (if any)]. If the value is negative, is will not touch the existing parallelism profile.
# security:
# access_control_allow_origin: "http:httpbin.org"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# access_control_allow_origin: "http:httpbin.org"
# access_control_allow_origin: "http://httpbin.org"

@@ -60,6 +60,14 @@ conf:
# such as absolute path on Windows: winfile:///C:\access.log
# log example: 2020-12-09T16:38:09.039+0800 INFO filter/logging.go:46 /apisix/admin/routes/r1 {"status": 401, "host": "127.0.0.1:9000", "query": "asdfsafd=adf&a=a", "requestId": "3d50ecb8-758c-46d1-af5b-cd9d1c820156", "latency": 0, "remoteIP": "127.0.0.1", "method": "PUT", "errs": []}
max_cpu: 0 # supports tweaking with the number of OS threads are going to be used for parallelism. Default value: 0 [will use max number of available cpu cores considering hyperthreading (if any)]. If the value is negative, is will not touch the existing parallelism profile.
# security:
# access_control_allow_origin: "http:httpbin.org"
# access_control_allow_credentials: true # support user custom cors configration
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# access_control_allow_credentials: true # support user custom cors configration
# access_control_allow_credentials: true # support using custom cors configration

@@ -16,14 +16,36 @@
*/
package filter

import "github.com/gin-gonic/gin"
import (
"github.com/apisix/manager-api/internal/conf"
Copy link
Contributor

Choose a reason for hiding this comment

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

Code style.

SecurityConf = Security{
XFrameOptions: "deny",
ContentSecurityPolicy: "default-src 'self'; script-src 'self' 'unsafe-eval'; style-src 'self' 'unsafe-inline'",
}
Copy link
Contributor

Choose a reason for hiding this comment

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

So we don't have the default value of AllowCredentials AllowOrigin AllowMethods AllowHeaders

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need default value for AllowCredentialsAlloOriginAllowMethodsAllowHeaders. These configuration are left to the user to customize.

@zaunist zaunist requested a review from starsz March 11, 2022 01:34
@bzp2010 bzp2010 merged commit edca223 into apache:master Mar 14, 2022
@zaunist zaunist deleted the feat/add_security_conf branch March 14, 2022 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants