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

Add Security Module (Lombiq Technologies: OCORE-91) #11538

Merged
merged 108 commits into from
May 20, 2022

Conversation

hishamco
Copy link
Member

@hishamco hishamco commented Apr 14, 2022

Fixes #854

  • Strict-Transport-Security
  • Content-Security-Policy
  • X-Frame-Options (Obsoleted by frame-ancestor in CSP)
  • X-Content-Type-Options
  • Referrer-Policy
  • Permissions-Policy

@hishamco
Copy link
Member Author

@Piedone I'm almost finish ReferrerPolicy header, I just need to do a last review and polish the APIs if it's possible, if you have any comment please let me know. Coz I will follow the same pattern to implement the other security headers

@hishamco
Copy link
Member Author

PermissionsPolicy

@jtkech
Copy link
Member

jtkech commented May 17, 2022

@hishamco

Okay I could repro

Yes the dictionaries fixed the duplicates when using multiple config sources, and it will help other features to collaborate.

Then, the mixing was working when only updating settings defined from the configuration.

But you're right, I could repro when changing settings that are not defined from the config.

Need to leave, will be easy to fix tomorrow I think as we have a good repro ;)

@jtkech
Copy link
Member

jtkech commented May 17, 2022

@hishamco

Okay the dictionaries fixed the duplicates but their are still merging values in ConfigureSecuritySettings(), so we need to clear them as here ConfigureSecuritySettings() don't collaborate with the site settings but want to replace the whole collections

Then, as it is done, other features will be able to collaborate but by also using a later postConfigure, or use one of the alternatives to just have to use Configure, will see.

I will commit.

@hishamco
Copy link
Member Author

Can you demo the module if you attend the OC standup, to hear the feedback or you could refer to my first video which I demo it

@hishamco
Copy link
Member Author

@jtkech anything else or shall I merge?

@jtkech
Copy link
Member

jtkech commented May 20, 2022

@Piedone @hishamco

@jtkech anything else or shall I merge?

As you want, I don't want to block anything, @hishamco I assume that you re-tried different use cases as I didn't take the time to do it. Maybe consider #11538 (comment) but was only suggestions.

Yes, there are so many ways to mix config/settings, e.g. for tenant specific settings we pre-fill values coming from the config stack, then through the editor we can override those for which we provide a value, notice that here tenant specific settings is another config source itself ;)

The other thought was, because all is done at the tenant/module level, why having an helper to be used from the app, but yes maybe useful to override settings from config for all tenants in one line.

@hishamco
Copy link
Member Author

Thanks a lot @jtkech let us merge this, then we could think about different things to improve. Regarding your above comment, according to what I understand from Zoltan the newly introduced extension ConfigureSecuritySettings() is a pattern we might apply on different modules to override the admin settings. I will point to your comment in the original issue later

Thanks again

@hishamco hishamco merged commit bbe1b14 into main May 20, 2022
@hishamco hishamco deleted the hishamco/security-module branch May 20, 2022 06:10
@Skrypt
Copy link
Contributor

Skrypt commented May 20, 2022

image

UI is broken

@hishamco
Copy link
Member Author

What?!! why this happening?

Is there an errors in the console?

<div class="custom-control custom-checkbox">
<input type="checkbox" class="custom-control-input" asp-for="EnableSandbox" />
<label class="custom-control-label" asp-for="EnableSandbox">@T["Enable Sandbox"]</label>
<span class="hint">@T["Enables a sandbox for the requested resource similar to the <iframe> sandbox attribute."]</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Here is why it fails. I tries to render an '<iframe>'. Also, use proper Bootstrap 5 styles for checkboxes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I removed the <> and now it works.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, use proper Bootstrap 5 styles for checkboxes.

I started this PR before bootstrap 5 stuff

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the <> and now it works.

How's it working before?

@jtkech
Copy link
Member

jtkech commented May 20, 2022

@hishamco

Maybe only little tweaks to do related to the new bootstrap 5

We should have merged dev in the PR before merging it

@Skrypt
Copy link
Contributor

Skrypt commented May 20, 2022

Also here second issue.

image

After hitting the save button I get a blank page.

@hishamco
Copy link
Member Author

Maybe only little tweaks to do related to the new bootstrap 5

Seems you are right, @Skrypt let me check after finalizing the PR that i'm working on. One more this it's possible that the bootstrap PR breaks the UI at whole?

@Skrypt
Copy link
Contributor

Skrypt commented May 20, 2022

Very unlikely. Only cosmetical changes.

@hishamco
Copy link
Member Author

I will submit a PR as quick fix for bootstrap issue ASAP

@Skrypt
Copy link
Contributor

Skrypt commented May 20, 2022

I will test it later tonight. The only issue that bothers me right now is the fact that I get an "almost blank" page after submitting the settings form.

@hishamco
Copy link
Member Author

It works fine with me, I'm trying to push my PR now ..

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.

Security Headers
6 participants