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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove x-forwarded-host headers in admin panel and set default security options for development #3822

Merged
merged 15 commits into from Sep 24, 2019

Conversation

@alexandrebodin
Copy link
Collaborator

alexandrebodin commented Aug 22, 2019

Description of what you did:

Wait for groups and repeatable to be merged before merging this branch

My PR is a:

  • 馃挜 Breaking change
  • 馃悰 Bug fix
  • 馃拝 Enhancement
  • 馃殌 New feature

Main update on the:

  • Admin
  • Documentation
  • Framework
  • Plugin

Manual testing done on the following databases:

  • Not applicable
  • MongoDB
  • MySQL
  • Postgres
  • SQLite
@alexandrebodin alexandrebodin force-pushed the chore/routing-x-forwarded branch 3 times, most recently from 965e3cb to 688b3d3 Aug 22, 2019
@alexandrebodin alexandrebodin force-pushed the chore/routing-x-forwarded branch from 612acf7 to 2668915 Sep 2, 2019
@alexandrebodin alexandrebodin marked this pull request as ready for review Sep 2, 2019
@alexandrebodin alexandrebodin requested a review from lauriejim Sep 2, 2019
@alexandrebodin

This comment has been minimized.

Copy link
Collaborator Author

alexandrebodin commented Sep 3, 2019

Test if we add private fields to plugins models

@lauriejim

This comment has been minimized.

Copy link
Member

lauriejim commented Sep 4, 2019

In this PR you removed csrf option.
Can you also remove it from the Settings Manager please.
Here is the object to remove.

- set security defaults for development mode that are standard
- refactor error messages to work without ctx.request.admin
- remove mask middleware and add a sanitization layer to the core-api to
hide private fileds
@alexandrebodin alexandrebodin force-pushed the chore/routing-x-forwarded branch from 2668915 to 705b8a5 Sep 6, 2019
@alexandrebodin alexandrebodin changed the base branch from develop to master Sep 9, 2019
@alexandrebodin alexandrebodin changed the title Remove x-forwarded-host headers in admin panel and set default security options for development [WIP] Remove x-forwarded-host headers in admin panel and set default security options for development Sep 9, 2019
@lauriejim lauriejim removed their request for review Sep 12, 2019
@alexandrebodin alexandrebodin changed the title [WIP] Remove x-forwarded-host headers in admin panel and set default security options for development Remove x-forwarded-host headers in admin panel and set default security options for development Sep 12, 2019
Copy link
Contributor

derrickmehaffy left a comment

LGTM

@alexandrebodin alexandrebodin requested a review from derrickmehaffy Sep 18, 2019
Copy link
Member

lauriejim left a comment

LGTM!
We will have to create a migration guide for this one.
For entry.toJSON() that is no longer needed and will break applications.

@alexandrebodin

This comment has been minimized.

Copy link
Collaborator Author

alexandrebodin commented Sep 20, 2019

LGTM!
We will have to create a migration guide for this one.
For entry.toJSON() that is no longer needed and will break applications.

Going to support it in the sanitizeFunction so it doesn't break compat

@alexandrebodin alexandrebodin added this to In progress in PRs via automation Sep 20, 2019
@alexandrebodin alexandrebodin moved this from In progress to Review in progress in PRs Sep 20, 2019
@alexandrebodin alexandrebodin moved this from Review in progress to Reviewer approved in PRs Sep 20, 2019
PRs automation moved this from Reviewer approved to Review in progress Sep 20, 2019
@alexandrebodin alexandrebodin requested a review from lauriejim Sep 20, 2019
Copy link
Contributor

derrickmehaffy left a comment

So far so good, just some AdminUI bugs. Forcing P3P true is fine but if there is no default value the user will have to set one or disable P3P to update any other settings.

},
"p3p": {
"enabled": false,
"enabled": true,
"value": ""

This comment has been minimized.

Copy link
@derrickmehaffy

derrickmehaffy Sep 20, 2019

Contributor

P3P on AdminUI states the value is required:

image

This comment has been minimized.

Copy link
@alexandrebodin

alexandrebodin Sep 20, 2019

Author Collaborator

OK I'll have to find a good default value then

This comment has been minimized.

Copy link
@derrickmehaffy

derrickmehaffy Sep 20, 2019

Contributor

TBH P3P is a pretty outdated security thing to begin with 馃槤

This comment has been minimized.

Copy link
@alexandrebodin

alexandrebodin Sep 20, 2019

Author Collaborator

I guess I can just disable it for now then

This comment has been minimized.

Copy link
@derrickmehaffy

derrickmehaffy Sep 20, 2019

Contributor

馃憤

},
"p3p": {
"enabled": false,
"enabled": true,

This comment has been minimized.

Copy link
@derrickmehaffy

derrickmehaffy Sep 20, 2019

Contributor

Same here, P3P on adminUI states value is required

@alexandrebodin alexandrebodin requested a review from derrickmehaffy Sep 20, 2019
Copy link
Member

lauriejim left a comment

LGTM!

@alexandrebodin alexandrebodin merged commit 6ed183d into master Sep 24, 2019
PRs automation moved this from Review in progress to Done Sep 24, 2019
@lauriejim lauriejim deleted the chore/routing-x-forwarded branch Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
PRs
  
Done
3 participants
You can鈥檛 perform that action at this time.