Update CSP handler to only query and modify frame ancestors instead o…

…f all CSP directives (#6398) * only allow updating frame ancestors Signed-off-by: Tianle Huang <[email protected]> * refactor Signed-off-by: Tianle Huang <[email protected]> * add test Signed-off-by: Tianle Huang <[email protected]> * fix link Signed-off-by: Tianle Huang <[email protected]> * update key Signed-off-by: Tianle Huang <[email protected]> * rename Signed-off-by: Tianle Huang <[email protected]> * update unit tests Signed-off-by: Tianle Huang <[email protected]> * update tests Signed-off-by: Tianle Huang <[email protected]> * add change log Signed-off-by: Tianle Huang <[email protected]> * undo yml Signed-off-by: Tianle Huang <[email protected]> * update readme and variable Signed-off-by: Tianle Huang <[email protected]> * fix link Signed-off-by: Tianle Huang <[email protected]> * make code generic Signed-off-by: Tianle Huang <[email protected]> * revert yml Signed-off-by: Tianle Huang <[email protected]> * add more tests Signed-off-by: Tianle Huang <[email protected]> * update key name Signed-off-by: Tianle Huang <[email protected]> * reword change title Signed-off-by: Tianle Huang <[email protected]> * fix typo Signed-off-by: Tianle Huang <[email protected]> --------- Signed-off-by: Tianle Huang <[email protected]>
opensearch-project · Apr 15, 2024 · 36c25ae · 36c25ae
1 parent e785fd3
commit 36c25ae
Show file tree

Hide file tree

Showing 6 changed files with 272 additions and 47 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -89,6 +89,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - [Multiple Datasource] Refactor data source selector component to include placeholder and add tests ([#6372](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6372))
 - Replace control characters before logging ([#6402](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6402))
 - [Dynamic Configurations] Improve dynamic configurations by adding cache and simplifying client fetch ([#6364](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6364))
+- [CSP Handler] Update CSP handler to only query and modify frame ancestors instead of all CSP directives ([#6398](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6398))
 - [MD] Add OpenSearch cluster group label to top of single selectable dropdown  ([#6400](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6400))
 - [Workspace] Support workspace in saved objects client in server side. ([#6365](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6365))
 

diff --git a/src/plugins/csp_handler/README.md b/src/plugins/csp_handler/README.md
@@ -2,7 +2,7 @@
 
 A OpenSearch Dashboards plugin
 
-This plugin is to support updating Content Security Policy (CSP) rules dynamically without requiring a server restart. It registers a pre-response handler to `HttpServiceSetup` which can get CSP rules from a dependent plugin `applicationConfig` and then rewrite to CSP header. Users are able to call the API endpoint exposed by the `applicationConfig` plugin directly, e.g through CURL. Currently there is no new OSD page for ease of user interactions with the APIs. Updates to the CSP rules will take effect immediately. As a comparison, modifying CSP rules through the key `csp.rules` in OSD YAML file would require a server restart.
+This plugin is to support updating the `frame-ancestors` directive in Content Security Policy (CSP) rules dynamically without requiring a server restart. It registers a pre-response handler to `HttpServiceSetup` which can get the `frame-ancestors` directive from a dependent plugin `applicationConfig` and then rewrite to CSP header. It will not change other directives. Users are able to call the API endpoint exposed by the `applicationConfig` plugin directly, e.g through CURL. The configuration key is `csp.rules.frame-ancestors`. Currently there is no new OSD page for ease of user interactions with the APIs. Updates to the `frame-ancestors` directive will take effect immediately. As a comparison, modifying CSP rules through the key `csp.rules` in OSD YAML file would require a server restart.
 
 By default, this plugin is disabled. Once enabled, the plugin will first use what users have configured through `applicationConfig`. If not configured, it will check whatever CSP rules aggregated by the values of `csp.rules` from OSD YAML file and default values. If the aggregated CSP rules don't contain the CSP directive `frame-ancestors` which specifies valid parents that may embed OSD page, then the plugin will append `frame-ancestors 'self'` to prevent Clickjacking.
 
@@ -23,23 +23,23 @@ Since it has a required dependency `applicationConfig`, make sure that the depen
 application_config.enabled: true
 ```
 
-For OSD users who want to make changes to allow a new site to embed OSD pages, they can update CSP rules through CURL. (See the README of `applicationConfig` for more details about the APIs.) **Please note that use backslash as string wrapper for single quotes inside the `data-raw` parameter. E.g use `'\''` to represent `'`**
+For OSD users who want to make changes to allow a new site to embed OSD pages, they can update the `frame-ancestors` directive through CURL. (See the README of `applicationConfig` for more details about the APIs.) **Please note that use backslash as string wrapper for single quotes inside the `data-raw` parameter. E.g use `'\''` to represent `'`**
 
 ```
-curl '{osd endpoint}/api/appconfig/csp.rules' -X POST -H 'Accept: application/json' -H 'Content-Type: application/json' -H 'osd-xsrf: osd-fetch' -H 'Sec-Fetch-Dest: empty' --data-raw '{"newValue":"script-src '\''unsafe-eval'\'' '\''self'\''; worker-src blob: '\''self'\''; style-src '\''unsafe-inline'\'' '\''self'\''; frame-ancestors '\''self'\'' {new site}"}'
+curl '{osd endpoint}/api/appconfig/csp.rules.frame-ancestors' -X POST -H 'Accept: application/json' -H 'Content-Type: application/json' -H 'osd-xsrf: osd-fetch' -H 'Sec-Fetch-Dest: empty' --data-raw '{"newValue":"{new value}"}'
 
 ```
 
-Below is the CURL command to delete CSP rules.
+Below is the CURL command to delete the `frame-ancestors` directive.
 
 ```
-curl '{osd endpoint}/api/appconfig/csp.rules' -X DELETE -H 'osd-xsrf: osd-fetch' -H 'Sec-Fetch-Dest: empty'
+curl '{osd endpoint}/api/appconfig/csp.rules.frame-ancestors' -X DELETE -H 'osd-xsrf: osd-fetch' -H 'Sec-Fetch-Dest: empty'
 ```
 
-Below is the CURL command to get the CSP rules.
+Below is the CURL command to get the `frame-ancestors` directive.
 
 ```
-curl '{osd endpoint}/api/appconfig/csp.rules'
+curl '{osd endpoint}/api/appconfig/csp.rules.frame-ancestors'
 
 ```
 

diff --git a/src/plugins/csp_handler/server/csp_handlers.test.ts b/src/plugins/csp_handler/server/csp_handlers.test.ts
@@ -8,6 +8,7 @@ import { createCspRulesPreResponseHandler } from './csp_handlers';
 import { MockedLogger, loggerMock } from '@osd/logging/target/mocks';
 
 const ERROR_MESSAGE = 'Service unavailable';
+const CSP_RULES_FRAME_ANCESTORS_CONFIG_KEY = 'csp.rules.frame-ancestors';
 
 describe('CSP handlers', () => {
   let toolkit: ReturnType<typeof httpServerMock.createToolkit>;
@@ -18,13 +19,13 @@ describe('CSP handlers', () => {
     logger = loggerMock.create();
   });
 
-  it('adds the CSP headers provided by the client', async () => {
+  it('adds the frame-ancestors provided by the client', async () => {
     const coreSetup = coreMock.createSetup();
-    const cspRulesFromIndex = "frame-ancestors 'self'";
+    const frameAncestorsFromIndex = "'self' localsystem";
     const cspRulesFromYML = "script-src 'unsafe-eval' 'self'";
 
     const configurationClient = {
-      getEntityConfig: jest.fn().mockReturnValue(cspRulesFromIndex),
+      getEntityConfig: jest.fn().mockReturnValue(frameAncestorsFromIndex),
     };
 
     const getConfigurationClient = jest.fn().mockReturnValue(configurationClient);
@@ -50,21 +51,26 @@ describe('CSP handlers', () => {
 
     expect(toolkit.next).toHaveBeenCalledWith({
       headers: {
-        'content-security-policy': cspRulesFromIndex,
+        'content-security-policy':
+          "script-src 'unsafe-eval' 'self'; frame-ancestors 'self' localsystem",
       },
     });
 
     expect(configurationClient.getEntityConfig).toBeCalledTimes(1);
+    expect(configurationClient.getEntityConfig).toBeCalledWith(
+      CSP_RULES_FRAME_ANCESTORS_CONFIG_KEY,
+      { headers: { 'sec-fetch-dest': 'document' } }
+    );
     expect(getConfigurationClient).toBeCalledWith(request);
   });
 
-  it('do not add CSP headers when the client returns empty and CSP from YML already has frame-ancestors', async () => {
+  it('do not modify frame-ancestors when the client returns empty and CSP from YML already has frame-ancestors', async () => {
     const coreSetup = coreMock.createSetup();
-    const emptyCspRules = '';
-    const cspRulesFromYML = "script-src 'unsafe-eval' 'self'; frame-ancestors 'self'";
+    const emptyFrameAncestors = '';
+    const cspRulesFromYML = "script-src 'unsafe-eval' 'self'; frame-ancestors localsystem";
 
     const configurationClient = {
-      getEntityConfig: jest.fn().mockReturnValue(emptyCspRules),
+      getEntityConfig: jest.fn().mockReturnValue(emptyFrameAncestors),
     };
 
     const getConfigurationClient = jest.fn().mockReturnValue(configurationClient);
@@ -87,19 +93,29 @@ describe('CSP handlers', () => {
     expect(result).toEqual('next');
 
     expect(toolkit.next).toHaveBeenCalledTimes(1);
-    expect(toolkit.next).toHaveBeenCalledWith({});
+    expect(toolkit.next).toHaveBeenCalledWith({
+      headers: {
+        'content-security-policy': cspRulesFromYML,
+      },
+    });
 
     expect(configurationClient.getEntityConfig).toBeCalledTimes(1);
+
+    expect(configurationClient.getEntityConfig).toBeCalledWith(
+      CSP_RULES_FRAME_ANCESTORS_CONFIG_KEY,
+      { headers: { 'sec-fetch-dest': 'document' } }
+    );
+
     expect(getConfigurationClient).toBeCalledWith(request);
   });
 
-  it('add frame-ancestors CSP headers when the client returns empty and CSP from YML has no frame-ancestors', async () => {
+  it('add frame-ancestors when the client returns empty and CSP from YML has no frame-ancestors', async () => {
     const coreSetup = coreMock.createSetup();
-    const emptyCspRules = '';
+    const emptyFrameAncestors = '';
     const cspRulesFromYML = "script-src 'unsafe-eval' 'self'";
 
     const configurationClient = {
-      getEntityConfig: jest.fn().mockReturnValue(emptyCspRules),
+      getEntityConfig: jest.fn().mockReturnValue(emptyFrameAncestors),
     };
 
     const getConfigurationClient = jest.fn().mockReturnValue(configurationClient);
@@ -125,17 +141,23 @@ describe('CSP handlers', () => {
     expect(toolkit.next).toHaveBeenCalledTimes(1);
     expect(toolkit.next).toHaveBeenCalledWith({
       headers: {
-        'content-security-policy': "frame-ancestors 'self'; " + cspRulesFromYML,
+        'content-security-policy': "script-src 'unsafe-eval' 'self'; frame-ancestors 'self'",
       },
     });
 
     expect(configurationClient.getEntityConfig).toBeCalledTimes(1);
+
+    expect(configurationClient.getEntityConfig).toBeCalledWith(
+      CSP_RULES_FRAME_ANCESTORS_CONFIG_KEY,
+      { headers: { 'sec-fetch-dest': 'document' } }
+    );
+
     expect(getConfigurationClient).toBeCalledWith(request);
   });
 
-  it('do not add CSP headers when the configuration does not exist and CSP from YML already has frame-ancestors', async () => {
+  it('do not modify frame-ancestors when the configuration does not exist and CSP from YML already has frame-ancestors', async () => {
     const coreSetup = coreMock.createSetup();
-    const cspRulesFromYML = "script-src 'unsafe-eval' 'self'; frame-ancestors 'self'";
+    const cspRulesFromYML = "script-src 'unsafe-eval' 'self'; frame-ancestors localsystem";
 
     const configurationClient = {
       getEntityConfig: jest.fn().mockImplementation(() => {
@@ -164,13 +186,23 @@ describe('CSP handlers', () => {
     expect(result).toEqual('next');
 
     expect(toolkit.next).toBeCalledTimes(1);
-    expect(toolkit.next).toBeCalledWith({});
+    expect(toolkit.next).toBeCalledWith({
+      headers: {
+        'content-security-policy': cspRulesFromYML,
+      },
+    });
 
     expect(configurationClient.getEntityConfig).toBeCalledTimes(1);
+
+    expect(configurationClient.getEntityConfig).toBeCalledWith(
+      CSP_RULES_FRAME_ANCESTORS_CONFIG_KEY,
+      { headers: { 'sec-fetch-dest': 'document' } }
+    );
+
     expect(getConfigurationClient).toBeCalledWith(request);
   });
 
-  it('add frame-ancestors CSP headers when the configuration does not exist and CSP from YML has no frame-ancestors', async () => {
+  it('add frame-ancestors when the configuration does not exist and CSP from YML has no frame-ancestors', async () => {
     const coreSetup = coreMock.createSetup();
     const cspRulesFromYML = "script-src 'unsafe-eval' 'self'";
 
@@ -199,15 +231,21 @@ describe('CSP handlers', () => {
     expect(toolkit.next).toBeCalledTimes(1);
     expect(toolkit.next).toBeCalledWith({
       headers: {
-        'content-security-policy': "frame-ancestors 'self'; " + cspRulesFromYML,
+        'content-security-policy': "script-src 'unsafe-eval' 'self'; frame-ancestors 'self'",
       },
     });
 
     expect(configurationClient.getEntityConfig).toBeCalledTimes(1);
+
+    expect(configurationClient.getEntityConfig).toBeCalledWith(
+      CSP_RULES_FRAME_ANCESTORS_CONFIG_KEY,
+      { headers: { 'sec-fetch-dest': 'document' } }
+    );
+
     expect(getConfigurationClient).toBeCalledWith(request);
   });
 
-  it('do not add CSP headers when request dest exists and shall skip', async () => {
+  it('do not add frame-ancestors when request dest exists and shall skip', async () => {
     const coreSetup = coreMock.createSetup();
     const cspRulesFromYML = "script-src 'unsafe-eval' 'self'";
 
@@ -243,7 +281,7 @@ describe('CSP handlers', () => {
     expect(getConfigurationClient).toBeCalledTimes(0);
   });
 
-  it('do not add CSP headers when request dest does not exist', async () => {
+  it('do not add frame-ancestors when request dest does not exist', async () => {
     const coreSetup = coreMock.createSetup();
     const cspRulesFromYML = "script-src 'unsafe-eval' 'self'";
 
@@ -277,4 +315,76 @@ describe('CSP handlers', () => {
     expect(configurationClient.getEntityConfig).toBeCalledTimes(0);
     expect(getConfigurationClient).toBeCalledTimes(0);
   });
+
+  it('use default values when getting client throws error and CSP from YML has no frame-ancestors', async () => {
+    const coreSetup = coreMock.createSetup();
+    const cspRulesFromYML = "script-src 'unsafe-eval' 'self'";
+
+    const getConfigurationClient = jest.fn().mockImplementation(() => {
+      throw new Error(ERROR_MESSAGE);
+    });
+
+    const handler = createCspRulesPreResponseHandler(
+      coreSetup,
+      cspRulesFromYML,
+      getConfigurationClient,
+      logger
+    );
+
+    const request = {
+      method: 'get',
+      headers: { 'sec-fetch-dest': 'document' },
+    };
+
+    toolkit.next.mockReturnValue('next' as any);
+
+    const result = await handler(request, {} as any, toolkit);
+
+    expect(result).toEqual('next');
+
+    expect(toolkit.next).toHaveBeenCalledTimes(1);
+    expect(toolkit.next).toHaveBeenCalledWith({
+      headers: {
+        'content-security-policy': "script-src 'unsafe-eval' 'self'; frame-ancestors 'self'",
+      },
+    });
+
+    expect(getConfigurationClient).toBeCalledWith(request);
+  });
+
+  it('do not modify when getting client throws error and CSP from YML has frame-ancestors', async () => {
+    const coreSetup = coreMock.createSetup();
+    const cspRulesFromYML = "script-src 'unsafe-eval' 'self'; frame-ancestors localsystem";
+
+    const getConfigurationClient = jest.fn().mockImplementation(() => {
+      throw new Error(ERROR_MESSAGE);
+    });
+
+    const handler = createCspRulesPreResponseHandler(
+      coreSetup,
+      cspRulesFromYML,
+      getConfigurationClient,
+      logger
+    );
+
+    const request = {
+      method: 'get',
+      headers: { 'sec-fetch-dest': 'document' },
+    };
+
+    toolkit.next.mockReturnValue('next' as any);
+
+    const result = await handler(request, {} as any, toolkit);
+
+    expect(result).toEqual('next');
+
+    expect(toolkit.next).toHaveBeenCalledTimes(1);
+    expect(toolkit.next).toHaveBeenCalledWith({
+      headers: {
+        'content-security-policy': cspRulesFromYML,
+      },
+    });
+
+    expect(getConfigurationClient).toBeCalledWith(request);
+  });
 });