Skip to content

refactor(csp): expose strict and loose rule sets on CspConfig#11971

Open
sumukhswamy wants to merge 2 commits into
opensearch-project:mainfrom
sumukhswamy:feat/csp-expose-strict-loose-rules
Open

refactor(csp): expose strict and loose rule sets on CspConfig#11971
sumukhswamy wants to merge 2 commits into
opensearch-project:mainfrom
sumukhswamy:feat/csp-expose-strict-loose-rules

Conversation

@sumukhswamy
Copy link
Copy Markdown
Collaborator

Description

Always compute both strict and loose rule sets in the constructor and expose them as strictRules, looseRules, looseHeader, and buildStrictHeaderWithNonce(nonce). Startup-time behavior of rules and header is preserved.

This enables consumers to choose between strict and loose CSP at request time without depending on the constructor-time enable/strict decision.

Issues Resolved

Screenshot

Testing the changes

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

Always compute both strict and loose rule sets in the constructor and
expose them as strictRules, looseRules, looseHeader, and
buildStrictHeaderWithNonce(nonce). Startup-time behavior of rules and
header is preserved.

This enables consumers to choose between strict and loose CSP at
request time without depending on the constructor-time enable/strict
decision.

Signed-off-by: sumukhswamy <sumukhhs@amazon.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 15, 2026

PR Reviewer Guide 🔍

(Review updated until commit d032aea)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Possible Issue

The applyLoosenCspRules method is called with this.rules as a reference before this.rules is assigned to this.looseRules. The comment states that applyLoosenCspRules references this.rules to look up the "original" loose directive, but at the time this.strictRules is computed, this.rules points to source.rules (the loose rules). This works only because the assignment happens in the correct order. If the order changes or if applyLoosenCspRules is refactored to use a different property, this could break. The dependency on assignment order is fragile and not enforced by the code structure.

this.looseRules = source.rules;
this.rules = source.rules;

this.strictRules = this.applyLoosenCspRules(
  this.applyAllowedSources(STRICT_CSP_RULES_DEFAULT_VALUE, source)
);

@github-actions
Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Clarify redundant assignment purpose

The applyLoosenCspRules method references this.rules to look up the original loose
directive when a rule is loosened. However, this.rules is assigned twice before
calling applyLoosenCspRules, which may cause confusion. Consider removing the
redundant assignment or adding a comment to clarify why both assignments are
necessary.

src/core/server/csp/csp_config.ts [189-194]

 this.looseRules = source.rules;
+// applyLoosenCspRules references this.rules to look up loose directives
 this.rules = source.rules;
 
 this.strictRules = this.applyLoosenCspRules(
   this.applyAllowedSources(STRICT_CSP_RULES_DEFAULT_VALUE, source)
 );
Suggestion importance[1-10]: 3

__

Why: The suggestion correctly identifies that this.rules is assigned twice, which could be confusing. However, the PR already includes a comment explaining why this is necessary (lines 184-188), making the suggested comment redundant. The suggestion offers minimal improvement since the code is already documented.

Low

@github-actions
Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit d032aea

@github-actions
Copy link
Copy Markdown
Contributor

🔗 Workflow run · commit 17ff006ca4094036dd393d5234ace1066af69a9b

❌ 4 Jest Test Failure(s)

📄 junit-jest-integration-Windows/TEST-Jest Integration Tests.xml

❌ Auth attach security header to a successful response (0.014s)

Jest Integration Tests.src\core\server\http\integration_tests

Error: expected 200 "OK", got 400 "Bad Request"
    at Object.<anonymous> (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\src\plugins\workspace\server\saved_objects\integration_tests\saved_objects_wrapper_for_check_workspace_conflict.test.ts:137:10)
    at Object.asyncJestTest (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-jasmine2\build\jasmineAsyncInstall.js:173:37)
    at D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-jasmine2\build\queueRunner.js:89:12
    at new Promise (<anonymous>)
    at mapper (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-jasmine2\build\queueRunner.js:72:19)
    at D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-jasmine2\build\queueRunner.js:119:41
    at processTicksAndRejections (node:internal/process/task_queues:105:5)
----
    at Test._assertStatus (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\supertest\lib\test.js:252:14)
  … (11 more lines)

❌ saved_objects_wrapper_for_check_workspace_conflict integration test workspace related CRUD create-with-override with unexisting object id [Attempt #2] (0.020s)

Jest Integration Tests.src\plugins\workspace\server\saved_objects\integration_tests

Error: expected 200 "OK", got 400 "Bad Request"
    at Object.<anonymous> (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\src\plugins\workspace\server\saved_objects\integration_tests\saved_objects_wrapper_for_check_workspace_conflict.test.ts:153:10)
    at Object.asyncJestTest (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-jasmine2\build\jasmineAsyncInstall.js:173:37)
    at D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-jasmine2\build\queueRunner.js:89:12
    at new Promise (<anonymous>)
    at mapper (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-jasmine2\build\queueRunner.js:72:19)
    at D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-jasmine2\build\queueRunner.js:119:41
    at processTicksAndRejections (node:internal/process/task_queues:105:5)
----
    at Test._assertStatus (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\supertest\lib\test.js:252:14)
  … (11 more lines)

❌ saved_objects_wrapper_for_check_workspace_conflict integration test workspace related CRUD create-with-override [Attempt #2] (0.020s)

Jest Integration Tests.src\plugins\workspace\server\saved_objects\integration_tests

Error: expected 200 "OK", got 400 "Bad Request"
    at Object.<anonymous> (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\src\plugins\workspace\server\saved_objects\integration_tests\saved_objects_wrapper_for_check_workspace_conflict.test.ts:171:10)
    at Object.asyncJestTest (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-jasmine2\build\jasmineAsyncInstall.js:173:37)
    at D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-jasmine2\build\queueRunner.js:89:12
    at new Promise (<anonymous>)
    at mapper (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-jasmine2\build\queueRunner.js:72:19)
    at D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\jest-jasmine2\build\queueRunner.js:119:41
    at processTicksAndRejections (node:internal/process/task_queues:105:5)
----
    at Test._assertStatus (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\supertest\lib\test.js:252:14)
  … (11 more lines)

❌ saved_objects_wrapper_for_check_workspace_conflict integration test workspace related CRUD create disallowed types within workspace [Attempt #2] (0.055s)

Jest Integration Tests.src\plugins\workspace\server\saved_objects\integration_tests

Error: expect(received).toMatchInlineSnapshot(snapshot)

Snapshot name: `saved_objects_wrapper_for_check_workspace_conflict integration test workspace related CRUD create disallowed types within workspace 1`

- Snapshot  - 1
+ Received  + 1

  Object {
    "error": "Bad Request",
-   "message": "Unsupported type in workspace: 'data-source' is not allowed to be created in workspace.",
  … (5 more lines)
Error: expect(received).toMatchInlineSnapshot(snapshot)

Snapshot name: `saved_objects_wrapper_for_check_workspace_conflict integration test workspace related CRUD create disallowed types within workspace 2`

- Snapshot  - 1
+ Received  + 1

  Object {
    "error": "Bad Request",
-   "message": "Unsupported type in workspace: 'data-connection' is not allowed to be created in workspace.",
  … (5 more lines)
Error: expect(received).toMatchInlineSnapshot(snapshot)

Snapshot name: `saved_objects_wrapper_for_check_workspace_conflict integration test workspace related CRUD create disallowed types within workspace 3`

- Snapshot  - 1
+ Received  + 1

  Object {
    "error": "Bad Request",
-   "message": "Unsupported type in workspace: 'config' is not allowed to be created in workspace.",
  … (5 more lines)
Error: expect(received).toMatchInlineSnapshot(snapshot)

Snapshot name: `saved_objects_wrapper_for_check_workspace_conflict integration test workspace related CRUD create disallowed types within workspace 4`

- Snapshot  - 1
+ Received  + 1

  Object {
    "error": "Bad Request",
-   "message": "Unsupported type in workspace: 'data-source' is not allowed to be created in workspace.",
  … (5 more lines)
Error: expect(received).toMatchInlineSnapshot(snapshot)

Snapshot name: `saved_objects_wrapper_for_check_workspace_conflict integration test workspace related CRUD create disallowed types within workspace 5`

- Snapshot  - 1
+ Received  + 1

  Object {
    "error": "Bad Request",
-   "message": "Unsupported type in workspace: 'data-connection' is not allowed to be created in workspace.",
  … (5 more lines)
Error: expect(received).toMatchInlineSnapshot(snapshot)

Snapshot name: `saved_objects_wrapper_for_check_workspace_conflict integration test workspace related CRUD create disallowed types within workspace 6`

- Snapshot  - 1
+ Received  + 1

  Object {
    "error": "Bad Request",
-   "message": "Unsupported type in workspace: 'config' is not allowed to be created in workspace.",
  … (5 more lines)

4 failure(s) across 1 suite(s). Full XML reports are in the junit-jest-* artifacts.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant