2238 document unsafe-inline exception #2941

jtimpe · 2024-04-12T15:23:09Z

Summary of Changes

Pull request closes #2238
Pull request closes #2843

Upon further investigation, the following low priority findings no longer show up in our latest Webinspect scans

HTML5: CORS Prolonged Caching of Preflight Response
HTML5: CORS Unsafe Methods Allowed

I have removed the changes associated with those findings and instead only addressed the final one

HTML5: Misconfigured Content Security Policy

Since our Kibana implementation requires being served behind a proxy, the unsafe-eval and unsafe-inline directives mentioned by the scan are required. I've included some documentation in ADR 16 to reflect this.

How to Test

List the steps to test the PR
These steps are generic, please adjust as necessary.

cd tdrs-frontend && docker-compose -f docker-compose.yml -f docker-compose.local.yml up -d
cd tdrs-backend && docker-compose -f docker-compose.yml -f docker-compose.local.yml up -d

Open http://localhost:3000/ and sign in.
Proceed with functional tests as described herein.
Test steps should be captured in the demo GIF(s) and/or screenshots below.

Demo GIF(s) and screenshots for testing procedure

Deliverables

More details on how deliverables herein are assessed included here.

Deliverable 1: Accepted Features

Checklist of ACs:

ADR 16 updated to capture new scenarios where unsafe-* policies are in-use
ADR 16 updated to capture security scanning tools that are flagging unsafe-* policies
lfrohlich and/or adpennington confirmed that ACs are met.

Deliverable 2: Tested Code

Are all areas of code introduced in this PR meaningfully tested?
- If this PR introduces backend code changes, are they meaningfully tested?
- If this PR introduces frontend code changes, are they meaningfully tested?
Are code coverage minimums met?
- Frontend coverage: [insert coverage %] (see CodeCov Report comment in PR)
- Backend coverage: [insert coverage %] (see CodeCov Report comment in PR)

Deliverable 3: Properly Styled Code

Are backend code style checks passing on CircleCI?
Are frontend code style checks passing on CircleCI?
Are code maintainability principles being followed?

Deliverable 4: Accessible

Does this PR complete the epic?
Are links included to any other gov-approved PRs associated with epic?
Does PR include documentation for Raft's a11y review?
Did automated and manual testing with iamjolly and ttran-hub using Accessibility Insights reveal any errors introduced in this PR?

Deliverable 5: Deployed

Was the code successfully deployed via automated CircleCI process to development on Cloud.gov?

Deliverable 6: Documented

Does this PR provide background for why coding decisions were made?
If this PR introduces backend code, is that code easy to understand and sufficiently documented, both inline and overall?
If this PR introduces frontend code, is that code easy to understand and sufficiently documented, both inline and overall?
If this PR introduces dependencies, are their licenses documented?
Can reviewer explain and take ownership of these elements presented in this code review?

Deliverable 7: Secure

Does the OWASP Scan pass on CircleCI?
Do manual code review and manual testing detect any new security issues?
If new issues detected, is investigation and/or remediation plan documented?

Deliverable 8: User Research

Research product(s) clearly articulate(s):

the purpose of the research
methods used to conduct the research
who participated in the research
what was tested and how
impact of research on TDP
(if applicable) final design mockups produced for TDP development

codecov · 2024-04-12T15:34:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.11%. Comparing base (229c32c) to head (8b1b6a2).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #2941   +/-   ##
========================================
  Coverage    93.11%   93.11%           
========================================
  Files          272      272           
  Lines         7055     7055           
  Branches       591      591           
========================================
  Hits          6569     6569           
  Misses         392      392           
  Partials        94       94

Flag	Coverage Δ
dev-backend	`93.19% <ø> (ø)`
dev-frontend	`92.62% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2729026...8b1b6a2. Read the comment docs.

docs/Technical-Documentation/Architecture-Decision-Record/016-zap-waiver.md

raftmsohani

need to move the unsafe-inline header to location instead of server global to limit the security implication

raftmsohani · 2024-04-17T19:38:15Z

changes needed:

need to make the frontend to load without unsafe-inline: see this npm run build without any minification facebook/create-react-app#6643 (comment)

raftmsohani · 2024-04-29T16:09:55Z

UPDATE Apr 9th:
To be able to have separate CSP header for the website and for Kibana, we will need to move the CSP header assignment to /location directive. The headers that are set in .conf file cannot be overwritten in /location directive and we can only add headers. There are two separate issues with this change:

TDRS website security: currently the frontend needs unsafe-inline directive in the CSP header both for style CSS files and for running .js file and finally for making axios requests. (there are a couple of solutions to this problem, but one is to separate css file into a static add and then add a token to other .js requests. This needs more investigation and work
For Kibana: there is no workaround to needed unsafe-inline tag. To reduce the risk, we can move this tag to the /kibana in the location file and remove the CSP header from .conf file.

This reverts commit e98cf41.

ADPennington · 2024-05-13T13:46:05Z

per async on 5/10 with @jtimpe -- we agreed that this PR can also close #2843

ADPennington · 2024-05-20T12:21:48Z

docs/Technical-Documentation/Architecture-Decision-Record/016-zap-waiver.md

+set $CSP "${CSP}script-src-attr 'self' 'unsafe-inline' http://{{env "KIBANA_BASE_URL"}}:5601;";
+```
+
+Kibana performs ajax requests from its frontend to scripts hosted on the server in order to provide styling and javascript functionality - in order to serve Kibana from behind a proxy, which is a requirement with cloud.gov, these `unsafe-inline` and `unsafe-eval` directives are required. The CSP exceptions are fairly low risk as they are limited to artifacts within the Kibana domain.


@jtimpe please include the links in #2843 as references. a comment from cloud.gov staff is included in this issue

ADPennington · 2024-05-20T12:23:12Z

docs/Technical-Documentation/Architecture-Decision-Record/016-zap-waiver.md

@@ -22,6 +22,21 @@ When investigating, we sought out information on what other, modern ACF systems

 While we also investigated and developed technical solutions utilizing either React-app-rewired or Next.js, we decided that the departure from our existing stack was not a worthwhile investment and could lead to other complications down the road.

+### 2024 Update


@jtimpe can we rename this markdown to 016-security-scan-waiver.md?

ADPennington · 2024-05-20T13:10:38Z

docs/Technical-Documentation/Architecture-Decision-Record/016-security-scan-waiver.md

@@ -22,6 +22,26 @@ When investigating, we sought out information on what other, modern ACF systems

 While we also investigated and developed technical solutions utilizing either React-app-rewired or Next.js, we decided that the departure from our existing stack was not a worthwhile investment and could lead to other complications down the road.

+### 2024 Update


@jtimpe @raftmsohani are any of the CSP related findings on the latest nightly scan related to what is described below? if so, we may want acknowledge that this is not just being found in webinspect but zap as well.

https://app.circleci.com/pipelines/github/raft-tech/TANF-app/24238/workflows/3faab2a5-dab3-4f52-bf56-6e321dfb9b03/jobs/71676/artifacts

i do not believe any of those findings are related to this waiver

@ADPennington Jan is correct, they are not relevant.

On a separate note, I am creating a ticket to check for user auth, similar to what we do for Kibana auth, to prevent access to api without auth: https://tdp-frontend-develop.acf.hhs.gov/v1/users/id/

ADPennington · 2024-05-20T13:57:19Z

docs/Technical-Documentation/Architecture-Decision-Record/016-security-scan-waiver.md

+* [elastic/kibana #27047](https://github.com/elastic/kibana/issues/27047#issuecomment-799680263) in March 2021
+* an [elastic discussion post](https://discuss.elastic.co/t/does-kibana-need-the-autorisation-of-unsafe-inline-or-unsafe-eval-to-work-properly/234390/2) in May 2020
+
+The CSP exceptions are fairly low risk as they are limited to artifacts within the Kibana domain.


@jtimpe can you provide a couple of examples of what we mean by artifacts here? might also be worth explicitly stating there is no risk to data assets.

open to ideas on how we can convey that there's no risk to data assets - what assets are specifically of concern?

jtimpe · 2024-05-20T17:20:51Z

docs/Technical-Documentation/Architecture-Decision-Record/016-security-scan-waiver.md

+* Vega (when interpreting its visualization syntax)
+* Lodash (via the _.template function)
+
+Maliciously side-loading a script would require Kibana have an unresolved persistent-XSS vulnerability, as well as file-system access to our cloud.gov Kibana application to load a script from an allowed domain. The attack vector is further limited by our authentication implementation, which requires a user be approved and granted Kibana access before loading any Kibana-related pages at all. In these cases, an attacker would have gained access to the system already.


@elipe17 @raftmsohani i added the following note about mitigation. please double-check my logic here :)

Looks good to me!

ADPennington

thank you @jtimpe 🚀

elipe17 · 2024-05-20T18:16:13Z

docs/Technical-Documentation/Architecture-Decision-Record/016-security-scan-waiver.md

+```
+
+Kibana performs ajax requests from its frontend to scripts hosted on the server in order to provide styling and javascript functionality - in order to serve Kibana from behind a proxy, which is a requirement with cloud.gov, these `unsafe-inline` and `unsafe-eval` directives are required.
+We currently use Kibana version 7.4 [released October, 2019](https://www.elastic.co/blog/kibana-7-4-0-released). This requirement is present in our current version as mentioned by the Elastic team in


Cloud.gov is actually running elastic/kibana 7.10 now!

document unsafe-inline exception

ac2b8eb

jtimpe added the raft review This issue is ready for raft review label Apr 12, 2024

jtimpe requested review from raftmsohani, andrew-jameson and elipe17 April 12, 2024 15:23

raftmsohani reviewed Apr 15, 2024

View reviewed changes

docs/Technical-Documentation/Architecture-Decision-Record/016-zap-waiver.md Show resolved Hide resolved

raftmsohani requested changes Apr 15, 2024

View reviewed changes

jtimpe added 5 commits April 15, 2024 11:57

move unsafe inline directives to kibana location

e98cf41

Merge branch 'develop' into fix/2238-webinspect-low

57a384e

additional add_header

f724c45

move csp above locations

2ca3ede

move kibana csp

6798abd

raftmsohani assigned raftmsohani and jtimpe Apr 19, 2024

raftmsohani added WIP and removed raft review This issue is ready for raft review labels Apr 19, 2024

jtimpe removed request for andrew-jameson and elipe17 April 22, 2024 14:22

raftmsohani changed the title ~~document unsafe-inline exception~~ 2238 document unsafe-inline exception Apr 29, 2024

jtimpe and others added 3 commits May 6, 2024 10:01

Revert "move unsafe inline directives to kibana location"

d372e6c

This reverts commit e98cf41.

remove extra lines

b1e8f4c

Merge branch 'develop' into fix/2238-webinspect-low

1c06ba7

jtimpe added raft review This issue is ready for raft review and removed WIP labels May 6, 2024

jtimpe requested review from andrew-jameson, atrimpe and elipe17 May 6, 2024 17:59

jtimpe added the QASP Review label May 8, 2024

jtimpe requested review from ADPennington and removed request for atrimpe and elipe17 May 8, 2024 19:10

ADPennington added raft review This issue is ready for raft review and removed QASP Review labels May 13, 2024

raftmsohani added 2 commits May 15, 2024 12:21

Merge branch 'develop' into fix/2238-webinspect-low

cc58bd3

Merge branch 'develop' into fix/2238-webinspect-low

4866c48

raftmsohani added QASP Review and removed raft review This issue is ready for raft review labels May 20, 2024

ADPennington reviewed May 20, 2024

View reviewed changes

jtimpe added 2 commits May 20, 2024 08:45

update waiver to include discussion links

bf7c49f

rename security waiver

c37e055

ADPennington reviewed May 20, 2024

View reviewed changes

jtimpe requested a review from ADPennington May 20, 2024 13:56

ADPennington reviewed May 20, 2024

View reviewed changes

jtimpe added 2 commits May 20, 2024 10:43

update waiver language w/ deps

a4cd17d

add attack vector info

cb9fc83

jtimpe commented May 20, 2024

View reviewed changes

ADPennington approved these changes May 20, 2024

View reviewed changes

ADPennington added Ready to Merge and removed QASP Review labels May 20, 2024

elipe17 reviewed May 20, 2024

View reviewed changes

jtimpe and others added 2 commits May 20, 2024 15:39

update kibana version

86e2441

Merge branch 'develop' into fix/2238-webinspect-low

8b1b6a2

jtimpe merged commit f1ef9d1 into develop May 20, 2024
16 checks passed

jtimpe deleted the fix/2238-webinspect-low branch May 20, 2024 20:01

jtimpe mentioned this pull request May 21, 2024

Resolve WebInspect scan findings for Jan-Feb 2024 #2822

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2238 document unsafe-inline exception #2941

2238 document unsafe-inline exception #2941

jtimpe commented Apr 12, 2024 •

edited by ADPennington

Loading

codecov bot commented Apr 12, 2024 •

edited

Loading

raftmsohani left a comment

raftmsohani commented Apr 17, 2024

raftmsohani commented Apr 29, 2024

ADPennington commented May 13, 2024

ADPennington May 20, 2024

ADPennington May 20, 2024

ADPennington May 20, 2024

jtimpe May 20, 2024

raftmsohani May 20, 2024

ADPennington May 20, 2024

jtimpe May 20, 2024

jtimpe May 20, 2024

elipe17 May 20, 2024

ADPennington left a comment

elipe17 May 20, 2024

		@@ -22,6 +22,21 @@ When investigating, we sought out information on what other, modern ACF systems

		While we also investigated and developed technical solutions utilizing either React-app-rewired or Next.js, we decided that the departure from our existing stack was not a worthwhile investment and could lead to other complications down the road.

		### 2024 Update

		@@ -22,6 +22,26 @@ When investigating, we sought out information on what other, modern ACF systems

		While we also investigated and developed technical solutions utilizing either React-app-rewired or Next.js, we decided that the departure from our existing stack was not a worthwhile investment and could lead to other complications down the road.

		### 2024 Update

2238 document unsafe-inline exception #2941

2238 document unsafe-inline exception #2941

Conversation

jtimpe commented Apr 12, 2024 • edited by ADPennington Loading

Summary of Changes

How to Test

Deliverables

codecov bot commented Apr 12, 2024 • edited Loading

Codecov Report

raftmsohani left a comment

Choose a reason for hiding this comment

raftmsohani commented Apr 17, 2024

raftmsohani commented Apr 29, 2024

ADPennington commented May 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ADPennington left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtimpe commented Apr 12, 2024 •

edited by ADPennington

Loading

codecov bot commented Apr 12, 2024 •

edited

Loading