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.

Sign up for GitHub

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

Jump to bottom

[FEATURE] Add Server Option to Send SAP's Target CSPs by default #179

Merged
merged 4 commits into from
Apr 25, 2019
Merged

[FEATURE] Add Server Option to Send SAP's Target CSPs by default #179

merged 4 commits into from
Apr 25, 2019

Conversation

codeworrior
Copy link
Member

When option cspDefaults:true is given, the server now sends two policies
for *.html files, both in report-only mode:

  • sap-target-level-1, which forbids inline scripts and only allows
    sources from self
  • sap-target-level-2, which additionally forbids 'eval'

Each policy is sent with its own 'Content-Security-Policy-Report-Only'
header. This might look uncommon, but simplifies automated validation of
the violation reports that are sent by the browser. Browsers don't
consistently report blocked-uri or source-file, but the original-policy
is reported consistently.

middleware/csp.js:

  • allow to define and send a 2nd default policy
  • skip execution for file types other than *.html and for HTTP methods
    other than POST and GET
  • use native capabilities of the express request object instead of
    parsing URLs with NodeJS means
  • when using the URL parameter, the shorter suffix ":ro" can now be
    used to activate the report-only mode

server.js

  • add boolean server option 'cspDefaults' (default false)
  • enrich csp middleware configuration accordingly when option is set

test/

  • enhance for the new features

Thank you for your contribution! 🙌

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

Pull Request Checklist

@codeworrior
Add Server Option to Send SAP's Target CSPs by default
9c0a37d 
When option cspDefaults:true is given, the server now sends two policies
for *.html files, both in report-only mode:
- sap-target-level-1, which forbids inline scripts and only allows
  sources from self
- sap-target-level-2, which additionally forbids 'eval'

Each policy is sent with its own 'Content-Security-Policy-Report-Only'
header. This might look uncommon, but simplifies automated validation of
the violation reports that are sent by the browser. Browsers don't
consistently report blocked-uri or source-file, but the original-policy
is reported consistently.

middleware/csp.js:
- allow to define and send a 2nd default policy
- skip execution for file types other than *.html and for HTTP methods
  other than POST and GET
- use native capabilities of the express request object instead of
  parsing URLs with NodeJS means
- when using the URL parameter, the shorter suffix ":ro" can now be
  used to activate the report-only mode

server.js
- add boolean server option 'cspDefaults' (default false)
- enrich csp middleware configuration accordingly when option is set

test/
- enhance for the new features
@CLAassistant
Copy link

CLAassistant commented Apr 14, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Apr 14, 2019 •
edited
Loading

Coverage Status

Coverage increased (+2.5%) to 93.156% when pulling 10a8a74 on add-csp-defaults into aa57198 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+2.5%) to 93.156% when pulling 9c0a37d on add-csp-defaults into aa57198 on master.

@codeworrior codeworrior mentioned this pull request Apr 14, 2019
Merged
3 tasks
svbender
svbender previously approved these changes Apr 15, 2019
View reviewed changes
matz3
matz3 requested changes Apr 16, 2019
View reviewed changes
lib/server.js Outdated Show resolved Hide resolved
@codeworrior
Copy link
Member Author

Renamed to sendSapTargetCSPs. Or should it be 'SAP'?

@matz3
Copy link
Member

matz3 commented Apr 17, 2019

I don't mind. But should we really use CSPs instead of just CSP?
I guess your idea is that we currently send two headers with the two target levels, right?

@RandomByte
Copy link
Member

It should definitely be camelcased: Csp/Csps. I would also favor Csp. Imho, abstractly speaking, a policy may contain multiple policies.

@codeworrior
Copy link
Member Author

@RandomByte why 'definitely' ? Our documentation guidelines state that "Consider the correct spelling of capitalized abbreviations like ID, URL, JSON etc. At the beginning of UI5 development, we decided for the same reg. control properties and APIs.

And others give the same guidance, e.g. https://url.spec.whatwg.org/#url-apis-elsewhere :

If a standard decides to use a variant of the name "URL" for a feature it defines, it should name such a feature "url" (i.e., lowercase and with an "l" at the end). Names such as "URL", "URI", and "IRI" should not be used. However, if the name is a compound, "URL" (i.e., uppercase) is preferred, e.g., "newURL" and "oldURL".

@RandomByte
Copy link
Member

Hm, I see. So far we tried to apply Googles Java Script guidelines for such cases: https://google.github.io/styleguide/jsguide.html#naming-camel-case-defined

From my experience, I find this convention rather easy to apply and consume as it is very consistent with abbreviations of all kinds.
E.g. in your example Sap is camel cased while CSP is not. Google simply says camel case everything.

Anyways, I could not find a public API of the tooling that applies this convention in that way so we can still decide and document it in https://github.com/SAP/ui5-tooling/blob/master/docs/Guidelines.md 👍

@RandomByte
Copy link
Member

But this detail should not keep us from merging this. So from my side all uppercase or camel cased is fine and totally up to you in this case.

Just the Travis/ESLint still has something to mock about:

/home/travis/build/SAP/ui5-server/lib/server.js
   94:2    error  @param "sendSapTargetCSPs" does not match an existing function parameter  jsdoc/check-param-names
  107:108  error  Trailing spaces not allowed                                               no-trailing-spaces
  108:76   error  Trailing spaces not allowed                                               no-trailing-spaces

@codeworrior
Copy link
Member Author

"Your proposal is acceptable" :-)

@codeworrior codeworrior requested a review from matz3 April 18, 2019 14:13
codeworrior added a commit to SAP/ui5-cli that referenced this pull request Apr 18, 2019
@codeworrior
Adapt to current state of PR SAP/ui5-server#179
091de00
RandomByte pushed a commit to SAP/ui5-cli that referenced this pull request Apr 25, 2019
@codeworrior @RandomByte
Adapt to current state of PR SAP/ui5-server#179
cbd9370
@svbender svbender self-requested a review April 25, 2019 10:56
@svbender svbender self-requested a review April 25, 2019 11:38
@svbender svbender self-requested a review April 25, 2019 11:40
@RandomByte RandomByte changed the title Add Server Option to Send SAP's Target CSPs by default [FEATURE] Add Server Option to Send SAP's Target CSPs by default Apr 25, 2019
@RandomByte
Copy link
Member

Please use Squash and Merge!

@matz3 I think your review comment is resolved?

@RandomByte RandomByte merged commit 4f05967 into master Apr 25, 2019
@RandomByte RandomByte deleted the add-csp-defaults branch April 25, 2019 14:51
RandomByte pushed a commit to SAP/ui5-cli that referenced this pull request Apr 26, 2019
@codeworrior @RandomByte
[FEATURE] Add 'sap-csp-policies' option to 'serve' command. (#188)
57d5567 
CLI counterpart for SAP/ui5-server#179
@codeworrior codeworrior mentioned this pull request Nov 20, 2020
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matz3 matz3 matz3 approved these changes

@domianer domianer Awaiting requested review from domianer

@RandomByte RandomByte Awaiting requested review from RandomByte

@svbender svbender Awaiting requested review from svbender

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@codeworrior @CLAassistant @coveralls @matz3 @RandomByte @svbender