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

[FEATURE] Add 'sap-csp-policies' option to 'serve' command. #188

Merged
merged 6 commits into from
Apr 26, 2019

Conversation

codeworrior
Copy link
Member

Thank you for your contribution! 🙌

To get it merged faster, kindly review the checklist below:

Pull Request Checklist

@CLAassistant
Copy link

CLAassistant commented Apr 14, 2019

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Apr 14, 2019

Coverage Status

Coverage remained the same at 82.051% when pulling 77a35fa on add-csp-defaults into b4747fb on master.

@codeworrior
Copy link
Member Author

Note: this PR is a counterpart to SAP/ui5-server#179.

svbender
svbender previously approved these changes Apr 15, 2019
@codeworrior
Copy link
Member Author

There's an open discussion about the name of the new option (--csp-defaults). At least in the tests, this looks a bit confusing. Other ideas had been --csp-presets, or --csp-sap-presets etc.

Until we come to a conclusion, I won't submit neither this PR nor SAP/ui5-server#179.

@RandomByte
Copy link
Member

Why not simply --csp?

@codeworrior
Copy link
Member Author

To me, --csp sounds as if this option would be mandatory to enable CSP at all. But that's not true. Even without any option, selection and definition of policies is already possible (via URL parameter --sap-ui-xx-csp-policy).

The purpose of the new option is to enable a set of policies that SAP want's to use in-house (for UI5 DIST layer development). As we don't have means to activate configuration for such a limited scope, having a new option seemed to be the 2nd best approach to me. But the term 'defaults' somehow conflicts with the fact that there are already (other) defaults even without the new option.

@RandomByte
Copy link
Member

Since CLI flags have defaults themselves, I would refrain from including the string "default" in any flags name since it refers to another level of defaults. Which I find very confusing.

From a user perspective the URL parameters are mostly unknown. Even when known, I would see them as a separate feature. I.e. a standard functionality of the server.

This new feature of having a certain set of CSP headers being sent from the server for every request can imho be referred to by a simple "do CSP" toggle. However, since it seems to be an in-house feature, maybe --sap-csp or --sap-csp-policies (to relate the flag name from the URL parameter name).

I don't see a reason for separating this feature from the existing URL parameters from a wording perspective. They are both controlled in different ways anyways.

Maybe read it like this:

When setting the flag --sap-csp-policies, the server always sends CSP headers sap-target-level-1 and sap-target-level-2 in report only mode

@codeworrior
Copy link
Member Author

Since CLI flags have defaults themselves, I would refrain from including the string "default" in any flags name since it refers to another level of defaults. Which I find very confusing.

That was exactly my point why I wasn't happy with the name of the option. To me it became obvious in the test code where I was tempted to name two tests "CSP defaults", one for each level of defaults.

I doubt that 'from a user perspective, the URL parameters are mostly unknown'. At least internally, developers have been asked to use these parameters to test their code against CSPs. Our QUnit integration even supports a UI (drop down) for them.

But it doesn't really matter as I'm totally fine with --sap-csp-policies. I'll even use

always send CSP headers sap-target-level-1 and sap-target-level-2 in report only mode

for the CLI help.

lib/cli/commands/serve.js Outdated Show resolved Hide resolved
lib/cli/commands/serve.js Show resolved Hide resolved
@RandomByte
Copy link
Member

@svbender for final approval.

Please use Squash and merge since the commit messages do not align with our convention. The squash commit should have a [FEATURE] prefix

@RandomByte RandomByte changed the title Add 'csp-defaults' option to 'serve' command. [FEATURE] Add 'csp-defaults' option to 'serve' command. Apr 25, 2019
codeworrior and others added 6 commits April 25, 2019 12:02
The duplication of the 'p' is accepted as 'csp' and 'policies' both help
to understand the purpose of the option and
'--sap-content-security-policies' would be a monster, not an option.
@RandomByte RandomByte changed the title [FEATURE] Add 'csp-defaults' option to 'serve' command. [FEATURE] Add 'sap-csp-policies' option to 'serve' command. Apr 26, 2019
@RandomByte RandomByte merged commit 57d5567 into master Apr 26, 2019
@RandomByte RandomByte deleted the add-csp-defaults branch April 26, 2019 08:19
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