Sanitize input and HTML output better (#430)

* Set jinja2 autoescape. Closes #424 * Sanitize app name and app version read from user * Add validation methods to data structures read from JSON or instantiated locally * Better URLs to UBOS Gears while I see it. * Sanitize HTML output better --------- Co-authored-by: Johannes Ernst <[email protected]>
fediverse-devnet · Dec 16, 2024 · 5993cc0 · 5993cc0
1 parent 681b6b3
commit 5993cc0
Show file tree

Hide file tree

Showing 26 changed files with 171 additions and 43 deletions.
diff --git a/src/feditest/cli/utils.py b/src/feditest/cli/utils.py
@@ -29,6 +29,7 @@ def create_plan_from_session_and_constellations(args: Namespace) -> TestPlan | N
     constellations = create_constellations(args)
 
     plan = TestPlan(session, constellations, args.name)
+    plan.properties_validate()
     plan.simplify()
     return plan
 

diff --git a/src/feditest/nodedrivers/ubos.py b/src/feditest/nodedrivers/ubos.py
@@ -1,5 +1,5 @@
 """
-Nodes managed via UBOS Gears https://ubos.net/
+Nodes managed via UBOS Gears https://ubos.net/docs/gears/
 """
 from abc import abstractmethod
 import hashlib
@@ -440,10 +440,10 @@ def _provision_node(self, rolename: str, config: NodeConfiguration, account_mana
 
         if config._rshcmd:
             if self._exec_shell('which ubos-admin', config._rshcmd).returncode:
-                raise OSError(f'{ type(self).__name__ } with an rshcmd requires UBOS Gears on the remote system (see ubos.net).')
+                raise OSError(f'{ type(self).__name__ } with an rshcmd requires UBOS Gears on the remote system (see https://feditest.org/glossary/ubosgears/).')
         else:
             if not shutil.which('ubos-admin'):
-                raise OSError(f'{ type(self).__name__ } without an rshcmd requires a local system running UBOS Gears (see ubos.net).')
+                raise OSError(f'{ type(self).__name__ } without an rshcmd requires a local system running UBOS Gears (see https://feditest.org/glossary/ubosgears/).')
 
         if account_manager is None:
             raise RuntimeError(f'No AccountManager set for rolename { rolename } with UbosNodeDriver { self }')

diff --git a/src/feditest/testplan.py b/src/feditest/testplan.py
@@ -10,8 +10,7 @@
 import msgspec
 
 import feditest
-from feditest.utils import hostname_validate, FEDITEST_VERSION
-
+from feditest.utils import hostname_validate, symbolic_name_validate, FEDITEST_VERSION
 
 class InvalidAccountSpecificationException(Exception):
     """
@@ -143,6 +142,43 @@ class TestPlanConstellationNode(msgspec.Struct):
     non_existing_accounts: list[dict[str, str | None]] | None = None
 
 
+    def properties_validate(self) -> None:
+        """
+        Validate properties for correctness and safety.
+        """
+        # Unclear whether we can validate the values of the dicts; currently not done.
+
+        if self.parameters:
+            for par_name, _ in self.parameters.items():
+                if not par_name:
+                    raise ValueError('Invalid TestPlanConstellationNode parameter name: cannot be empty')
+
+                if not symbolic_name_validate(par_name):
+                    raise ValueError(f'Invalid TestPlanConstellationNode parameter name: { par_name }')
+
+        if self.accounts:
+            for account in self.accounts:
+                if 'role' not in account:
+                    raise ValueError('No role name given in account')
+
+                if not account['role']:
+                    raise ValueError('Invalid TestPlanConstellationNode account role name: cannot be empty')
+
+                if not symbolic_name_validate(account['role']):
+                    raise ValueError(f'Invalid role name in account: "{ account["role" ]}')
+
+        if self.non_existing_accounts:
+            for non_existing_account in self.non_existing_accounts:
+                if 'role' not in non_existing_account:
+                    raise ValueError('No role name given in non_existing_account')
+
+                if not non_existing_account['role']:
+                    raise ValueError('Invalid TestPlanConstellationNode non_existing_account role name: cannot be empty')
+
+                if not symbolic_name_validate(non_existing_account['role']):
+                    raise ValueError(f'Invalid role name in non_existing_account: "{ non_existing_account["role" ]}')
+
+
     @staticmethod
     def load(filename: str) -> 'TestPlanConstellationNode':
         """
@@ -224,6 +260,18 @@ class TestPlanConstellation(msgspec.Struct):
     roles : dict[str,TestPlanConstellationNode | None] # can be None if used as template
     name: str | None = None
 
+    def properties_validate(self) -> None:
+        """
+        Validate properties for correctness and safety.
+        """
+        for role_name, role_value in self.roles.items():
+            if not symbolic_name_validate(role_name):
+                raise ValueError(f'Invalid TestPlanConstellation role name: { role_name }')
+            if role_value:
+                role_value.properties_validate()
+
+        # Not checking self.name: can be any human-readable name
+
 
     @staticmethod
     def load(filename: str) -> 'TestPlanConstellation':
@@ -288,6 +336,21 @@ class TestPlanTestSpec(msgspec.Struct):
     skip: str | None = None # if a string is given, it's a reason message why this test spec should be skipped
 
 
+    def properties_validate(self) -> None:
+        """
+        Validate properties for correctness and safety.
+        """
+        # Not checking self.name: can be any human-readable name
+
+        if self.rolemapping:
+            for role_mapping_key, role_mapping_value in self.rolemapping.items():
+                if not symbolic_name_validate(role_mapping_key):
+                    raise ValueError(f'Invalid TestPlanTestSpec role mapping key: { role_mapping_key }')
+
+                if not symbolic_name_validate(role_mapping_value):
+                    raise ValueError(f'Invalid TestPlanTestSpec role mapping value: { role_mapping_value }')
+
+
     def __str__(self):
         return self.name
 
@@ -348,6 +411,16 @@ class TestPlanSessionTemplate(msgspec.Struct):
     name: str | None = None
 
 
+    def properties_validate(self) -> None:
+        """
+        Validate properties for correctness and safety.
+        """
+        for test in self.tests:
+            test.properties_validate()
+
+        # Not checking self.name: can be any human-readable name
+
+
     @staticmethod
     def load(filename: str) -> 'TestPlanSessionTemplate':
         """
@@ -412,6 +485,25 @@ class TestPlan(msgspec.Struct):
     feditest_version: str = FEDITEST_VERSION
 
 
+    def properties_validate(self) -> None:
+        """
+        Validate properties for correctness and safety.
+        """
+        if self.session_template is None:
+            raise ValueError('No session_template in TestPlan')
+        self.session_template.properties_validate()
+
+        for constellation in self.constellations:
+            constellation.properties_validate()
+
+        # Not checking self.name: can be any human-readable name
+        if self.type != 'feditest-testplan':
+            raise ValueError(f'TestPlan type is not feditest-testplan: "{ self.type }".')
+
+        if not symbolic_name_validate(self.feditest_version):
+            raise ValueError(f'Invalid TestPlan FediTest version: "{ self.feditest_version }".')
+
+
     def simplify(self) -> None:
         """
         If possible, simplify this test plan.

diff --git a/src/feditest/testruntranscriptserializer/html.py b/src/feditest/testruntranscriptserializer/html.py
@@ -89,11 +89,10 @@ def write(self, transcript: TestRunTranscript, dest: str | None):
             permit_line_breaks_in_identifier=lambda s: re.sub(
                 r"(\.|::)", r"<wbr>\1", s
             ),
-            local_name_with_tooltip=lambda n: f'<span title="{ n }">{ n.split(".")[-1] }</span>',
+            local_name_with_tooltip=lambda n: f'<span title="{ html.escape(n) }">{ n.split(".")[-1] }</span>',
             format_timestamp=lambda ts: ts.strftime("%Y:%m:%d-%H:%M:%S.%fZ") if ts else "",
             format_duration=lambda s: str(s), # makes it easier to change in the future
-            len=len,
-            html_escape=lambda s: html.escape(str(s))
+            len=len
         )
 
         try:

diff --git a/src/feditest/testruntranscriptserializer/templates/testplantranscript_default/matrix.jinja2 b/src/feditest/testruntranscriptserializer/templates/testplantranscript_default/matrix.jinja2
@@ -2,11 +2,11 @@
 <html lang="en">
  <head>
 {%  include "partials/shared/head.jinja2" %}
-  <title>{{ transcript.plan.name }} | Feditest</title>
+  <title>{{ transcript.plan.name | e }} | Feditest</title>
  </head>
  <body>
   <header class="feditest title">
-   <h1><span class="prefix"><a href="https://feditest.org/">Feditest</a> Summary Report:</span> {{ transcript.plan.name }}</h1>
+   <h1><span class="prefix"><a href="https://feditest.org/">Feditest</a> Summary Report:</span> {{ transcript.plan.name | e }}</h1>
    <p class="id">{{ transcript.id }}</p>
   </header>
 {%  include "partials/shared/mobile.jinja2" %}

diff --git a/...untranscriptserializer/templates/testplantranscript_default/partials/matrix/matrix.jinja2 b/...untranscriptserializer/templates/testplantranscript_default/partials/matrix/matrix.jinja2
@@ -1,15 +1,15 @@
 {%  macro column_headings(first) %}
        <tr>
-        <th>{{ first }}</th>
+        <th>{{ first | e }}</th>
 {%-     for run_session in transcript.sessions %}
 {%-         set constellation = run_session.constellation %}
         <th>
          <div class="title">
           <a href="{{ session_file_path(run_session) }}">
            <dl class="roles">
 {%-         for role, node in constellation.nodes.items() %}
-             <dt>{{ role }}</dt>
-             <dd>{{ node.node_driver }}</dd>
+             <dt>{{ role | e }}</dt>
+             <dd>{{ node.node_driver | e }}</dd>
 {%-         endfor %}
            </dl>
           </a>
@@ -34,9 +34,9 @@
 {%- for test_index, ( _, test_meta ) in enumerate(sorted(transcript.test_meta.items())) %}
        <tr>
         <td class="namedesc">
-         <span class="name">{{ permit_line_breaks_in_identifier(test_meta.name) }}</span>
+         <span class="name">{{ permit_line_breaks_in_identifier(test_meta.name) | e }}</span>
 {%-     if test_meta.description %}
-         <span class="description">{{ test_meta.description }}</span>
+         <span class="description">{{ test_meta.description | e }}</span>
 {%-     endif %}
         </td>
 {%-     for session_index, run_session in enumerate(transcript.sessions) %}

diff --git a/...transcriptserializer/templates/testplantranscript_default/partials/matrix/metadata.jinja2 b/...transcriptserializer/templates/testplantranscript_default/partials/matrix/metadata.jinja2
@@ -15,14 +15,14 @@
 {%- for key in ['username', 'hostname', 'platform'] -%}
 {%-     if getattr(transcript,key) %}
      <tr>
-      <th>{{ key.capitalize() }}</th>
-      <td>{{ getattr(transcript, key) }}</td>
+      <th>{{ key.capitalize() | e }}</th>
+      <td>{{ getattr(transcript, key) | e }}</td>
      </tr>
 {%-     endif %}
 {%- endfor %}
      <tr>
       <th>Feditest&nbsp;version</th>
-      <td>{{ getattr(transcript, 'feditest_version') }}</td>
+      <td>{{ getattr(transcript, 'feditest_version') | e }}</td>
      </tr>
     </tbody>
    </table>

diff --git a/...anscriptserializer/templates/testplantranscript_default/partials/matrix/testresult.jinja2 b/...anscriptserializer/templates/testplantranscript_default/partials/matrix/testresult.jinja2
@@ -1,6 +1,6 @@
 {%- if result %}
 <td class="status {{ result.css_classes() }} moreinfo" id="test-{{ session_index }}-{{ test_index }}">
-    <div>{{ result.short_title() }}</div>
+    <div>{{ result.short_title() | e }}</div>
 </td>
 {%- else %}
 <td class="status passed" id="test-{{ session_index }}-{{ test_index }}">

diff --git a/...ntranscriptserializer/templates/testplantranscript_default/partials/shared/summary.jinja2 b/...ntranscriptserializer/templates/testplantranscript_default/partials/shared/summary.jinja2
@@ -54,7 +54,7 @@
       </tr>
 {%-     for spec_level in [ feditest.SpecLevel.SHOULD, feditest.SpecLevel.IMPLIED, feditest.SpecLevel.UNSPECIFIED ] %}
       <tr>
-       <th>{{ spec_level.formatted_name }}</th>
+       <th>{{ spec_level.formatted_name | e }}</th>
 {%-         for interop_level in feditest.InteropLevel %}
        <td class="status {{ spec_level.name.lower() }} {{ interop_level.name.lower() }} moreinfo">
         <div>

diff --git a/...ptserializer/templates/testplantranscript_default/partials/shared_session/metadata.jinja2 b/...ptserializer/templates/testplantranscript_default/partials/shared_session/metadata.jinja2
@@ -16,7 +16,7 @@
 {%-     if getattr(transcript,key) %}
      <tr>
       <th>{{ key.capitalize() }}</th>
-      <td>{{ getattr(transcript, key) }}</td>
+      <td>{{ getattr(transcript, key) | e }}</td>
      </tr>
 {%-     endif %}
 {%- endfor %}

diff --git a/...iptserializer/templates/testplantranscript_default/partials/shared_session/results.jinja2 b/...iptserializer/templates/testplantranscript_default/partials/shared_session/results.jinja2
@@ -2,10 +2,10 @@
 <div class="roles">
 {%- for role_name, node in run_session.constellation.nodes.items() %}
  <div class="role">
-  <h3 class="name">{{ role_name }}</h3>
+  <h3 class="name">{{ role_name | e }}</h3>
   <div class="driver">{{ local_name_with_tooltip(node.node_driver) }}</div>
-  <div class="app">{{ node.appdata['app'] }}</div>
-  <div class="appversion">{{ node.appdata['app_version'] or '?'}}</div>
+  <div class="app">{{ node.appdata['app'] | e }}</div>
+  <div class="appversion">{{ node.appdata['app_version'] or '?' | e }}</div>
 {%-     if node.parameters %}
   <table class="parameters">
    <thead>
@@ -16,8 +16,8 @@
    <tbody>
 {%-         for key, value in node.parameters.items() %}
     <tr>
-     <td class="key">{{ key }}</td>
-     <td class="value">{{ value }}</td>
+     <td class="key">{{ key | e }}</td>
+     <td class="value">{{ value | e }}</td>
     </tr>
 {%-         endfor %}
    </tbody>
@@ -33,9 +33,9 @@
 {%-     set plan_test_spec = transcript.plan.session_template.tests[run_test.plan_test_index] %}
 {%-     set test_meta = transcript.test_meta[plan_test_spec.name] %}
  <div class="test" id="test-{{ test_index }}">
-  <h4><span class="prefix">Test:</span> {{ test_meta.name }}</h4>
+  <h4><span class="prefix">Test:</span> {{ test_meta.name | e }}</h4>
 {%-     if test_meta.description %}
-  <div class="description">{{ test_meta.description}}</div>
+  <div class="description">{{ test_meta.description | e }}</div>
 {%-     endif %}
   <p class="when">Started {{ format_timestamp(run_test.started) }}, ended {{ format_timestamp(run_test.ended) }} (duration: {{ format_duration(run_test.ended - run_test.started) }})</p>
 {%-     with result=run_test.worst_result %}
@@ -44,9 +44,9 @@
 {%-     for test_step_index, run_step in enumerate(run_test.run_steps or []) %}
   <div class="step">
 {%          set test_step_meta = test_meta.steps[run_step.plan_step_index] %}
-   <h5><span class="prefix">Test step:</span> {{ test_step_meta.name }}</h5>
+   <h5><span class="prefix">Test step:</span> {{ test_step_meta.name | e }}</h5>
 {%-         if test_step_meta.description %}
-   <div class="description">{{ test_step_meta.description}}</div>
+   <div class="description">{{ test_step_meta.description | e }}</div>
 {%-         endif %}
    <p class="when">Started {{ format_timestamp(run_test.started) }}, ended {{ format_timestamp(run_test.ended) }} (duration: {{ format_duration(run_test.ended - run_test.started) }})</p>
 {%-         with result=run_step.result, idmod='step' %}

diff --git a/...serializer/templates/testplantranscript_default/partials/shared_session/testresult.jinja2 b/...serializer/templates/testplantranscript_default/partials/shared_session/testresult.jinja2
@@ -1,8 +1,8 @@
 {%- if result %}
 <div class="status {{ result.css_classes() }} moreinfo">
-	<label for="details-{{ result.id() }}{{ idmod }}">{{ result.short_title() }}</label>
+	<label for="details-{{ result.id() }}{{ idmod }}">{{ result.short_title() | e }}</label>
     <input id="details-{{ result.id() }}{{ idmod }}" type="checkbox">
-    <pre class="stacktrace">{{ html_escape(result) }}</pre>
+    <pre class="stacktrace">{{ result | e }}</pre>
 </div>
 {%- else %}
 <div class="status passed">

diff --git a/...st/testruntranscriptserializer/templates/testplantranscript_default/session_single.jinja2 b/...st/testruntranscriptserializer/templates/testplantranscript_default/session_single.jinja2
@@ -2,11 +2,11 @@
 <html lang="en">
  <head>
 {%  include "partials/shared/head.jinja2" %}
-  <title>{{ transcript.plan.name }} | Feditest</title>
+  <title>{{ transcript.plan.name | e }} | Feditest</title>
  </head>
  <body>
   <header class="feditest title">
-   <h1><span class="prefix"><a href="https://feditest.org/">Feditest</a> Report:</span> {{ transcript.plan.name }}</h1>
+   <h1><span class="prefix"><a href="https://feditest.org/">Feditest</a> Report:</span> {{ transcript.plan.name | e }}</h1>
    <p class="id">{{ transcript.id }}</p>
   </header>
 {%  include "partials/shared/mobile.jinja2" %}

diff --git a/...struntranscriptserializer/templates/testplantranscript_default/session_with_matrix.jinja2 b/...struntranscriptserializer/templates/testplantranscript_default/session_with_matrix.jinja2
@@ -2,11 +2,11 @@
 <html lang="en">
  <head>
 {%  include "partials/shared/head.jinja2" %}
-  <title>{{ run_session.name }} | Feditest</title>
+  <title>{{ run_session.name | e }} | Feditest</title>
  </head>
  <body>
     <header class="feditest title" style="text-align: center">
-      <h1><span class="prefix"><a href="https://feditest.org/">Feditest</a> Session Report:</span> {{ run_session }}</h1>
+      <h1><span class="prefix"><a href="https://feditest.org/">Feditest</a> Session Report:</span> {{ run_session | e }}</h1>
       <p class="id">{{ transcript.id }} [<a href="{{ matrix_file_path }}">Summary</a>]</p>
     </header>