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

Added CSP to HTML code in oauth.service.ts #1036

Merged

Conversation

TantumErgo
Copy link
Contributor

General:

HTML code is passed on lines 31-56. This code was without a CSP,
leaving it vulnerable. A strict CSP was added on lines 35-38.

Notes:

  1. VS Code documentation, as well as other recommendations on CSP's,
    use default-src 'none'. However, after reviewing the CSP's added in
    Pull Request '#'1020, I followed the convention of
    default-src vscode-resource. Make sure this is the level of security
    wanted, rather than the more secure 'none'.

  2. form-action and frame-ancestors are included because these two
    directives do not automatically use the default-src permission of
    the CSP and must be explicitly set.

  3. img-src includes https: as recommended by VS Code documentation for
    CSP's. I also included vscode-resource, but I did not include data:
    because I don't understand why it's used in Pull Request '#'1020.
    If you would like me to include data: let me know, and I will add it to
    the CSP.

  4. script-src and style-src have 'self' included, but if this is
    redundant after specifying vscode-resource, let me know and I will
    remove the 'self' value. By including 'self' it whitelists all scripts
    and styles originating from the URL of the project website.

  5. script-src and style-src also have 'unsafe-inline' included, as they
    were included in the CSP's in Pull Request '#'1020, but it is
    recommended that SHA-256 be used when possible instead. script-src
    can use a SHA-256 hash of scripts, but I'm not sure if style-src can
    as well. Regardless, I continued using 'unsafe-inline' to match the
    other CSP's in the project, and because I didn't want to break any of
    the project's current functions by using too strict of a CSP.

Short description of what this resolves:

See above General and Notes

Changes proposed in this pull request:

See above General and Notes

Fixes: #1010

How Has This Been Tested?

Wasn't sure how to test this, nor if any of the tests in the tests folder are applicable. If you'd like me to test this, just tell me how. I read the contributing guidelines, but those only detail how to set up the testing environment, and not how to actually complete any tests.

Screenshots (if appropriate):

Checklist:

  • [x ] I have read the contribution guidelines.
  • My change requires a change to the documentation and GitHub Wiki.
  • I have updated the documentation and Wiki accordingly.

General:

HTML code is passed on lines 31-56. This code was without a CSP,
leaving it vulnerable. A strict CSP was added on lines 35-38.

Notes:

1. VS Code documentation, as well as other recommendations on CSP's,
use default-src 'none'. However, after reviewing the CSP's added in
Pull Request '#'1020, I followed the convention of
default-src vscode-resource. Make sure this is the level of security
wanted, rather than the more secure 'none'.

2. form-action and frame-ancestors are included because these two
directives do not automatically use the default-src permission of
the CSP and must be explicitly set.

3. img-src includes https: as recommended by VS Code documentation for
CSP's. I also included vscode-resource, but I did not include data:
because I don't understand why it's used in Pull Request '#'1020.
If you would like me to include data: let me know, and I will add it to
the CSP.

4. script-src and style-src have 'self' included, but if this is
redundant after specifying vscode-resource, let me know and I will
remove the 'self' value. By including 'self' it whitelists all scripts
and styles originating from the URL of the project website.

5. script-src and style-src also have 'unsafe-inline' included, as they
were included in the CSP's in Pull Request '#'1020, but it is
recommended that SHA-256 be used when possible instead. script-src
can use a SHA-256 hash of scripts, but I'm not sure if style-src can
as well. Regardless, I continued using 'unsafe-inline' to match the
other CSP's in the project, and because I didn't want to break any of
the project's current functions by using too strict of a CSP.
@shanalikhan shanalikhan added this to the v3.4.3 milestone Sep 9, 2019
@shanalikhan
Copy link
Owner

img-src includes https: as recommended by VS Code documentation for CSP's. I also included vscode-resource, but I did not include data: because I don't understand why it's used in Pull Request '#'1020. If you would like me to include data: let me know, and I will add it to the CSP.

@ParkourKarthik can assist us on it.

script-src and style-src have 'self' included, but if this is redundant after specifying vscode-resource, let me know and I will remove the 'self' value. By including 'self' it whitelists all scripts and styles originating from the URL of the project website.

Lets keep this as of now, we can always improve it later.

script-src and style-src also have 'unsafe-inline' included, as they were included in the CSP's in Pull Request '#'1020, but it is recommended that SHA-256 be used when possible instead. script-src
can use a SHA-256 hash of scripts, but I'm not sure if style-src can as well.

We need a proper testing after using SHA-256 where possible. Currently there is no testing thing defined targeting CSP. Do you have any ideas for testing CSP by looking into other extensions or projects then feel free to add testing files for CSP and include SHA-256

@ParkourKarthik
Copy link
Contributor

img-src includes https: as recommended by VS Code documentation for
CSP's. I also included vscode-resource, but I did not include data:
because I don't understand why it's used in Pull Request '#'1020.
If you would like me to include data: let me know, and I will add it to
the CSP.

There were some images (don't exactly remember) loaded in several webviews that depended on data: to be mentioned in CSP.

If there are no console errors without mentioning data:, then there is no need for it. Also, make sure the styles are loaded properly in this html view.

Based on the html content, I guess it only needs default-src set to none and style-src set to unsafe-inline (since there is only style script in the html and no other external resources referred).

Thanks for finding this embedded html and trying to fix it 👍

@shanalikhan shanalikhan merged commit a424071 into shanalikhan:v3.4.3 Sep 12, 2019
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.

3 participants