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

New: Rule SRI #875

Closed

molant wants to merge 3 commits into webhintio:master from molant:rule-sri

Member

molant commented Mar 11, 2018

Pull request checklist

Make sure you:

Signed the Contributor License Agreement
Followed the commit message guidelines

For non-trivial changes, please make sure you also:

Added/Updated related documentation.
Added/Updated related tests.

Short description of the change(s)

Close #26

molant requested review from alrra, austinkelleher and sarvaje as code owners

March 11, 2018 22:17

Member Author

molant commented Mar 11, 2018 •

edited

Loading

The spec talks about integrity for script and link but all the examples are for styles so I'm not sure if it applies also for manifests, favicon, etc. Also CSP values are only for scripts and styles.
For now I'm limiting to styles and scripts and ask around.

alrra reviewed

View reviewed changes

packages/rule-sri/README.md Outdated

		@@ -0,0 +1,145 @@
		# Require scripts and styles to use Subresource integrity (`sri`)

Contributor

alrra Mar 12, 2018

Subresource => subresource

packages/rule-sri/README.md Outdated

+              ## Why is this important?
+              Nowadays it's very common to use third party resources from CDNs or different
+              services (analytics, ads, etc.) and thus increasing the risk surface of your

Contributor

alrra Mar 12, 2018

, and thus,

packages/rule-sri/README.md

+              Subresource integrity [is a standard][sri spec] that mitigates this by ensuring
+              that an exact representation of a resource, and only that representation, loads
+              and executes.

Contributor

alrra Mar 12, 2018

Maybe we should add a warning that this is only guaranteed over HTTPS?

Member Author

molant Mar 13, 2018

I put a comment in the latest comment in the next paragraph

packages/rule-sri/README.md Outdated

+              * When using a cross-origin resource (e.g.: using a script hosted in a third
+                party CDN), the `<script>` tag needs to have a valid
+                [`crossorigin` attribute][crossorigin].
+              * The resource needs to be served on a [secure context][secure context]

Contributor

alrra Mar 12, 2018

The resource is served...

Member Author

molant Mar 13, 2018

Done

packages/rule-sri/README.md Outdated

+                (i.e.: HTTPS)
+              * The hash from the `integrity` attribute needs to be the same as the one
+                calculated using the response's body.
+              * If multiple hashes are provided, at least one needs to be valid

Contributor

alrra Mar 12, 2018

valid.

Member Author

molant Mar 13, 2018

Done

packages/rule-sri/README.md

+              Same-origin resource with hash function less secure than `sha384`:
+              ```html
+              <script src="/script.js" integrity="sha256-validHashHere">

Contributor

alrra Mar 12, 2018

Maybe to be consistent we should use a random hash?

Member Author

molant Mar 12, 2018

I was thinking about that, but I though it was clearer this way so users reading about SRI for the first time know they need to calculate the hash and replace it.

Contributor

alrra Mar 12, 2018

ok.

packages/rule-sri/README.md Outdated

+              ```json
+              "sri": ["warning", {
+                  "baseline": "sha512"
+              }

Contributor

alrra Mar 12, 2018

}]

Member Author

molant Mar 13, 2018

Done

alrra reviewed

View reviewed changes

packages/rule-sri/package.json Outdated

+                },
+                "repository": "sonarwhal/sonarwhal",
+                "scripts": {
+                  "build": "npm run clean && npm-run-all build:*",

Contributor

alrra Mar 12, 2018

Add "build-release": "npm run clean && npm run build:assets && tsc --inlineSourceMap false --removeComments true",

Member Author

molant Mar 13, 2018

Done

packages/rule-sri/package.json Outdated

+                  "extends": "../../../../.nycrc"
+                },
+                "peerDependencies": {
+                  "sonarwhal": "^1.0.0"

Contributor

alrra Mar 12, 2018

1.0.1

Member Author

molant Mar 13, 2018

Actually 1.0.3, things move fast!

packages/rule-sri/package.json Outdated

+                  "watch:test": "ava --watch",
+                  "watch:ts": "npm run build:ts -- --watch"
+                },
+                "version": "0.1.0"

Contributor

alrra Mar 12, 2018

Add "private": true

Member Author

molant Mar 13, 2018

Done

packages/rule-sri/package.json Outdated

		@@ -0,0 +1,69 @@
		{
		"author": "",

Contributor

alrra Mar 12, 2018

Remove this.

Member Author

molant Mar 13, 2018

Done

packages/rule-sri/package.json Outdated

+                  "watch:test": "ava --watch",
+                  "watch:ts": "npm run build:ts -- --watch"
+                },
+                "version": "0.1.0"

Contributor

alrra Mar 12, 2018

1.0.0

Member Author

molant Mar 13, 2018

Done

alrra reviewed

View reviewed changes

packages/rule-sri/package.json Outdated

+                  "eslint-plugin-markdown": "^1.0.0-beta.7",
+                  "eslint-plugin-typescript": "^0.10.0",
+                  "markdownlint-cli": "^0.7.0",
+                  "npm-link-check": "^2.0.0",

Contributor

alrra Mar 12, 2018 •

edited

Loading

I think this ca be removed?

Member Author

molant Mar 13, 2018

Done

alrra reviewed

View reviewed changes

packages/rule-sri/src/index.ts Outdated

+               * @fileoverview Require scripts and styles to use Subresource Integrity
+               */
+              import * as sri from './sri';

Contributor

alrra Mar 12, 2018

Maybe we should just make a convention that if there is a single rule in a package we use rule.ts for the name of the file?

Member Author

molant Mar 13, 2018

Done

packages/rule-sri/src/sri.ts Outdated

+              import { Category } from 'sonarwhal/dist/src/lib/enums/category';
+              import { RuleContext } from 'sonarwhal/dist/src/lib/rule-context';
+              // The list of types depends on the events you want to capture.

Contributor

alrra Mar 12, 2018

Remove this line.

Member Author

molant Mar 13, 2018

Done

packages/rule-sri/src/sri.ts Outdated

+                      debug('Is eligible for integrity validation?');
+                      const { element, resource } = evt;
+                      const resourceOrigin: string = new URL(resource).origin;

Contributor

alrra Mar 12, 2018

Maybe use same-origin as things may be more complex then this?

Member Author

molant Mar 12, 2018

I actually think this code is better than the code in that package. It's using the old url.parse and doing some magic, and we get for free the origin if we use new URL. Maybe we could add a method in misc so we can remove that dependency from the project.

Contributor

alrra Mar 12, 2018

Maybe we could add a method in misc so we can remove that dependency from the project.

👍

Member Author

molant Mar 13, 2018

I've created #879 for this

packages/rule-sri/src/sri.ts Outdated

+                      const { element, resource } = evt;
+                      const resourceOrigin: string = new URL(resource).origin;
+                      // CORS validation only applies to scripts, styles are OK

Contributor

alrra Mar 12, 2018

Why are styles ok? Maybe provide a link?

Member Author

molant Mar 12, 2018

That's the link that's just above in the method description

Contributor

alrra Mar 12, 2018

OK.

packages/rule-sri/src/sri.ts Outdated

+                      debug('Is integrity attribute valid?');
+                      const { element, resource } = evt;
+                      const integrity = element.getAttribute('integrity');
+                      const integrityRegExp = /^sha(256|384|512)-/;

Contributor

alrra Mar 12, 2018

What if I have multiple integrity attributes?

Member Author

molant Mar 13, 2018

I've added another test. Only the first instance is accessible via getAttribute. Additionally, it looks like Chrome tries to download the file twice if the first hash is invalid, but only the first attribute is accessible. I've left the code of the test commented with an explanation.

alrra reviewed

View reviewed changes

packages/rule-sri/src/sri.ts Outdated

+                      }
+                      // cross-origin scripts need to be loaded with a valid "crossorigin" attribute (ie.: anonymous or use-credentials)
+                      const crossorigin = element.getAttribute('crossorigin');

Contributor

alrra Mar 12, 2018

Use normalizeString?

Member Author

molant Mar 13, 2018

Done

Member Author

molant commented Mar 13, 2018

@alrra all your feedback should be included.

@sonarwhal/core this is ready for another round.

Contributor

alrra commented Mar 13, 2018

Also, maybe add this to configuration-web-recommended?

Member Author

molant commented Mar 13, 2018 via email

👍

________________________________ From: Cătălin Mariș <notifications@github.com> Sent: Monday, March 12, 2018 9:36:06 PM To: sonarwhal/sonarwhal Cc: Anton Molleda; Author Subject: Re: [sonarwhal/sonarwhal] New: Rule SRI (#875) Also, maybe add this to configuration-web-recommended? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#875 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAlBgoVmT3Wi25XdZBHoncLGW4McqPsOks5td0y2gaJpZM4Sl4p7>.

molant added 2 commits

March 13, 2018 09:57


          New: Rule SRI

3715b15

Close #26


          New: Add sri to configuration-web-recommended

bcb6b6d

alrra reviewed

View reviewed changes

packages/rule-sri/LICENSE.txt

		@@ -0,0 +1,201 @@
		Apache License

Contributor

alrra Mar 13, 2018

Maybe we should make the release script add this file automatically for new packages?

Member Author

molant Mar 13, 2018

It's taken care by the generators. I wouldn't change anything TBH.

Contributor

alrra Mar 13, 2018

OK.

packages/rule-sri/README.md Outdated

		@@ -0,0 +1,145 @@
		# Require scripts and styles to use subresource integrity (`sri`)

Contributor

alrra Mar 13, 2018

sri => @sonarwhal/rule-sri

packages/rule-sri/README.md Outdated

		@@ -0,0 +1,145 @@
		# Require scripts and styles to use subresource integrity (`sri`)

		`sri` warns about requesting scripts or styles without using Subresource

Contributor

alrra Mar 13, 2018

styles => stylesheets

Contributor

alrra Mar 13, 2018

Subresource => subresource

packages/rule-sri/README.md

+              ## Why is this important?
+              Nowadays it's very common to use third party resources from CDNs or different
+              services (analytics, ads, etc.), and thus, increasing the risk surface of your

Contributor

alrra Mar 13, 2018

increase

Member Author

molant Mar 13, 2018

I'm pretty sure it's increasing

packages/rule-sri/README.md Outdated

+              Nowadays it's very common to use third party resources from CDNs or different
+              services (analytics, ads, etc.), and thus, increasing the risk surface of your
+              web application.

Contributor

alrra Mar 13, 2018

web site/app. ?

packages/rule-sri/README.md Outdated

+              * [The `integrity` attribute has to be valid][sri format]. I.e.: it should
+                contain something in the form of `sha(256|384|512)-HASH`, where `HASH` is
+                the hashed value of the downlaoded body's response using the previous
+                algorithm (`sha256`, `sha384`, or `sha512`).

Contributor

alrra Mar 13, 2018

previously specified algorithm?

packages/rule-sri/README.md Outdated

+                contain something in the form of `sha(256|384|512)-HASH`, where `HASH` is
+                the hashed value of the downlaoded body's response using the previous
+                algorithm (`sha256`, `sha384`, or `sha512`).
+              * The minium cryptographic hash function used has to be [`sha384`][collisions].

Contributor

alrra Mar 13, 2018

for consistency with the others:

used has to be => used is

packages/rule-sri/README.md Outdated

+                the hashed value of the downlaoded body's response using the previous
+                algorithm (`sha256`, `sha384`, or `sha512`).
+              * The minium cryptographic hash function used has to be [`sha384`][collisions].
+                If multiple are provided, the highest one will be used to determine if the

Contributor

alrra Mar 13, 2018

multiple ones

packages/rule-sri/src/rule.ts Outdated

+                   * * base64
+                   * * `sha384-hash`
+                   */
+                  private calculateHash(content: string, sha): string {

Contributor

alrra Mar 13, 2018

sha: string

packages/rule-sri/src/rule.ts Outdated

+                       * No need to report anything, but we can stop processing things right away.
+                       */
+                      const isScript: boolean = element.nodeName === 'SCRIPT' && !!element.getAttribute('src');
+                      const isStyle: boolean = element.nodeName === 'LINK' && element.getAttribute('rel') === 'stylesheet';

Contributor

alrra Mar 13, 2018

normalizeString(element.getAttribute('rel')) === 'stylesheet';


          Fix: PR Feedback

9e7ac6d

Member Author

molant commented Mar 13, 2018

Feedback is pushed

alrra closed this in

3fb2777

molant deleted the rule-sri branch

March 15, 2018 22:06

molant mentioned this pull request

Remove package same-origin with new method in misc #879

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment