Skip to content

[TT-17218] Add a default branch mechanism in test-controller#128

Merged
konrad-sol merged 1 commit into
mainfrom
TT-17218_default_branches_in_test_controller
May 19, 2026
Merged

[TT-17218] Add a default branch mechanism in test-controller#128
konrad-sol merged 1 commit into
mainfrom
TT-17218_default_branches_in_test_controller

Conversation

@konrad-sol

Copy link
Copy Markdown
Collaborator

When a pipeline is triggered on a branch not explicitly configured in the tui.yml file in gromit, the API returns 404, causing the step and whole pipeline to fail.

This change introduces a two-attempt fallback mechanism:
Fetch config for the exact base_ref — capturing the HTTP status without failing
On 404, emit a warning annotation and retry against default_branch (defaults to master)
On any other non-200, fail as before.
A new optional input default_branch is added so callers can override the fallback target per-workflow if needed.

@probelabs

probelabs Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

This PR introduces a fallback mechanism to the test-controller GitHub Action to prevent CI pipeline failures when a test configuration for a specific branch is not found.

Previously, if the configuration API returned a 404 error for a given branch, the workflow would fail. This change modifies the action to first attempt fetching the configuration for the exact branch. If a 404 error occurs, it now logs a warning and retries the request using a default branch (master by default), making the pipeline more resilient. A new optional default_branch input has been added to allow customization of the fallback branch.

Files Changed Analysis

  • .github/actions/tests/test-controller/action.yaml: The core logic of the action's run step was updated. The simple curl command was replaced with a more robust shell script that captures the HTTP status code. This allows for conditional logic to handle a 404 status by falling back to the default branch, while failing on other non-200 statuses with more explicit error messages.

Architecture & Impact Assessment

  • Accomplishment: Increases the resilience of CI pipelines that use the test-controller action by gracefully handling missing branch-specific test configurations.
  • Technical Changes: Introduces a new default_branch input and replaces a single curl command with a multi-step script that checks HTTP status codes to implement a fallback-on-404-error logic.
  • Affected Components: This change affects the .github/actions/tests/test-controller action and all CI workflows that utilize it. The change is backward-compatible.

Flow Diagram

graph TD
    A[Start test-controller action] --> B{Construct API URL with `BASE_REF`};
    B --> C{Request test config from TUI API};
    C --> D{Get HTTP Status Code};
    D --> E{Code == 200?};
    E -- Yes --> F[Use response as config];
    F --> G[End];
    E -- No --> H{Code == 404?};
    H -- Yes --> I[Log Warning & Construct URL with `DEFAULT_BRANCH`];
    I --> J{Retry request with fallback branch};
    J --> K{Request successful?};
    K -- Yes --> F;
    K -- No --> L[Fail with error];
    H -- No --> M[Log HTTP Code and Body];
    M --> L;
    L --> G;
Loading

Scope Discovery & Context Expansion

The change is localized to the test-controller action but impacts all workflows that rely on it for test configuration. By preventing failures for branches without specific configurations (e.g., feature branches), it streamlines the development and testing process. The action interacts with an internal service (http://tui.internal.dev.tyk.technology), and this PR modifies the interaction pattern to be more fault-tolerant. Any workflow file within the .github/workflows/ directory that calls uses: ./.github/actions/tests/test-controller will be affected by this improvement.

Metadata
  • Review Effort: 2 / 5
  • Primary Label: enhancement

Powered by Visor from Probelabs

Last updated: 2026-05-18T10:54:38.708Z | Triggered by: pr_opened | Commit: 04dc437

💡 TIP: You can chat with Visor using /visor ask <your question>

@probelabs

probelabs Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

Security Issues (3)

Severity Location Issue
🔴 Critical .github/actions/tests/test-controller/action.yaml:40-54
The script constructs a URL for `curl` by expanding shell variables that are derived from user-controllable GitHub Action inputs (`test_type`, `default_branch`, and `variation`). If an input contains shell command substitution syntax like `$(command)` or `` `command` ``, the shell will execute the command before `curl` is run. This can be exploited by a malicious actor submitting a pull request that modifies a workflow file to call this action with a malicious input, leading to arbitrary code execution on the runner.
💡 SuggestionSanitize all inputs that are used to construct the URL. A simple approach is to check for and reject inputs containing shell command substitution characters. Add a validation step at the beginning of the script for all environment variables that originate from `inputs`.
      run: |
        set -eo pipefail

        for var_name in VARIATION TEST_TYPE DEFAULT_BRANCH; do
          var_value=&#34;${!var_name}&#34;
          if [[ &#34;$var_value&#34; == *&#39;$(&#39;* || &#34;$var_value&#34; == *&#39;`&#39;* ]]; then
            echo &#34;::error::Malicious characters detected in &#39;$var_name&#39; input.&#34;
            exit 1
          fi
        done

        BASE_URL=&#34;http://tui.internal.dev.tyk.technology/v2/$VARIATION/$REPO_NAME&#34;
        # ... rest of the script
🟡 Warning .github/actions/tests/test-controller/action.yaml:40
The action communicates with `tui.internal.dev.tyk.technology` over unencrypted HTTP. This could allow an attacker on the same network to intercept or modify the test configuration fetched by the action (Man-in-the-Middle attack). This could lead to tests being bypassed or malicious configuration being executed by the test runner.
💡 SuggestionUse HTTPS for all communication with the internal API. Update the `BASE_URL` to use `https://` and ensure the service at `tui.internal.dev.tyk.technology` has TLS enabled.
🟡 Warning .github/actions/tests/test-controller/action.yaml:52
The script prints the full body of a failed API response to the action logs using `cat /tmp/tui_response.txt >&2`. If the internal API `tui.internal.dev.tyk.technology` returns sensitive information in its error responses (e.g., stack traces, internal file paths, configuration details), this information will be exposed in GitHub Actions logs. This could provide attackers with valuable information about the internal infrastructure.
💡 SuggestionAvoid logging the entire response body on failure. If debugging information is needed, consider logging only specific, safe parts of the response, or log a generic error and provide a correlation ID to look up the full error in a secure logging system. If the full body must be logged, ensure the internal API does not leak sensitive details in error cases.

Security Issues (3)

Severity Location Issue
🔴 Critical .github/actions/tests/test-controller/action.yaml:40-54
The script constructs a URL for `curl` by expanding shell variables that are derived from user-controllable GitHub Action inputs (`test_type`, `default_branch`, and `variation`). If an input contains shell command substitution syntax like `$(command)` or `` `command` ``, the shell will execute the command before `curl` is run. This can be exploited by a malicious actor submitting a pull request that modifies a workflow file to call this action with a malicious input, leading to arbitrary code execution on the runner.
💡 SuggestionSanitize all inputs that are used to construct the URL. A simple approach is to check for and reject inputs containing shell command substitution characters. Add a validation step at the beginning of the script for all environment variables that originate from `inputs`.
      run: |
        set -eo pipefail

        for var_name in VARIATION TEST_TYPE DEFAULT_BRANCH; do
          var_value=&#34;${!var_name}&#34;
          if [[ &#34;$var_value&#34; == *&#39;$(&#39;* || &#34;$var_value&#34; == *&#39;`&#39;* ]]; then
            echo &#34;::error::Malicious characters detected in &#39;$var_name&#39; input.&#34;
            exit 1
          fi
        done

        BASE_URL=&#34;http://tui.internal.dev.tyk.technology/v2/$VARIATION/$REPO_NAME&#34;
        # ... rest of the script
🟡 Warning .github/actions/tests/test-controller/action.yaml:40
The action communicates with `tui.internal.dev.tyk.technology` over unencrypted HTTP. This could allow an attacker on the same network to intercept or modify the test configuration fetched by the action (Man-in-the-Middle attack). This could lead to tests being bypassed or malicious configuration being executed by the test runner.
💡 SuggestionUse HTTPS for all communication with the internal API. Update the `BASE_URL` to use `https://` and ensure the service at `tui.internal.dev.tyk.technology` has TLS enabled.
🟡 Warning .github/actions/tests/test-controller/action.yaml:52
The script prints the full body of a failed API response to the action logs using `cat /tmp/tui_response.txt >&2`. If the internal API `tui.internal.dev.tyk.technology` returns sensitive information in its error responses (e.g., stack traces, internal file paths, configuration details), this information will be exposed in GitHub Actions logs. This could provide attackers with valuable information about the internal infrastructure.
💡 SuggestionAvoid logging the entire response body on failure. If debugging information is needed, consider logging only specific, safe parts of the response, or log a generic error and provide a correlation ID to look up the full error in a secure logging system. If the full body must be logged, ensure the internal API does not leak sensitive details in error cases.
\n\n ### Architecture Issues (3)
Severity Location Issue
🟡 Warning .github/actions/tests/test-controller/action.yaml:41
The inline shell script has grown in complexity to handle the fallback logic. It now involves multiple API calls, conditional logic based on HTTP status codes, and temporary file management. While functional, complex inline scripts can be difficult to read, maintain, and test.
💡 SuggestionConsider moving this logic into a separate script file (e.g., `run.sh`) and calling it from the `run` step. This would improve modularity and readability, and make the script easier to test in isolation.
🟡 Warning .github/actions/tests/test-controller/action.yaml:46
The number of `curl` retries has been reduced from 5 to 2 for all API calls. The original implementation used `--retry 5`, while the new implementation uses `--retry 2`. This reduces the action's resilience to transient network issues and could lead to more frequent pipeline failures on a flaky network. This change is not documented in the pull request description.
💡 SuggestionIf this change is intentional, document the reason. Otherwise, consider restoring the retry count to 5 to maintain the previous level of resilience against transient network errors.
🟡 Warning .github/actions/tests/test-controller/action.yaml:18
The `default_branch` input defaults to `'master'`. While this is a common default, many projects have migrated to using `main` as the default branch. This hardcoded default might not be suitable for all repositories and could lead to unexpected behavior if not explicitly overridden.
💡 SuggestionConsider making the `default_branch` input required by setting `required: true` and removing the default. This would force workflow authors to be explicit about their desired fallback branch, preventing potential mismatches. Alternatively, if the remote service is expected to align with the git repository, you could dynamically set the default to `github.repository.default_branch`.

Performance Issues (2)

Severity Location Issue
🟡 Warning .github/actions/tests/test-controller/action.yaml:55
The 'cat' process is unnecessary when piping to 'tee'. The input file can be redirected to 'tee's standard input directly.
💡 SuggestionReplace 'cat /tmp/tui_response.txt | tee -a "$GITHUB_OUTPUT"' with 'tee -a "$GITHUB_OUTPUT" < /tmp/tui_response.txt'. This avoids spawning an extra 'cat' process, which is a shell scripting best practice for performance and resource usage.
🟡 Warning .github/actions/tests/test-controller/action.yaml:52
The 'cat' process is unnecessary. The file content can be redirected to standard error directly.
💡 SuggestionReplace 'cat /tmp/tui_response.txt >&2' with '< /tmp/tui_response.txt >&2'. This avoids spawning an extra 'cat' process for a simple redirection.

Powered by Visor from Probelabs

Last updated: 2026-05-18T10:54:34.727Z | Triggered by: pr_opened | Commit: 04dc437

💡 TIP: You can chat with Visor using /visor ask <your question>

@konrad-sol
konrad-sol merged commit 1894903 into main May 19, 2026
6 of 8 checks passed
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.

2 participants