Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
konrad-sol merged 1 commit into from
May 19, 2026
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions .github/actions/tests/test-controller/action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@
test_type:
description: 'Type of test, e.g. api or ui'
required: true
default_branch:
description: 'Branch to fall back to when base_ref has no config in the API'
required: false
default: 'master'
outputs:

Check warning on line 18 in .github/actions/tests/test-controller/action.yaml

View check run for this annotation

probelabs / Visor: architecture

architecture Issue 

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.
Raw output 
Consider 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`.
envfiles:
description: 'Environment files for the test'
value: ${{ steps.params.outputs.envfiles }}
Expand All @@ -33,10 +37,29 @@
REPO_NAME: ${{ github.event.repository.name }}
TEST_TYPE: ${{ inputs.test_type }}
TRIGGER: ${{ github.event_name }}
DEFAULT_BRANCH: ${{ inputs.default_branch }}

Check warning on line 40 in .github/actions/tests/test-controller/action.yaml

View check run for this annotation

probelabs / Visor: security

security Issue 

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.
Raw output 
Use 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.
run: |

Check warning on line 41 in .github/actions/tests/test-controller/action.yaml

View check run for this annotation

probelabs / Visor: architecture

architecture Issue 

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.
Raw output 
Consider 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.
set -eo pipefail
curl -s --retry 5 --retry-delay 10 --fail-with-body "http://tui.internal.dev.tyk.technology/v2/$VARIATION/$REPO_NAME/$BASE_REF/$TRIGGER/$TEST_TYPE.gho" | tee -a "$GITHUB_OUTPUT"
if ! [[ $VARIATION =~ prod ]] ;then
BASE_URL="http://tui.internal.dev.tyk.technology/v2/$VARIATION/$REPO_NAME"
ENDPOINT="$TRIGGER/$TEST_TYPE.gho"

HTTP_CODE=$(curl -s --retry 2 --retry-delay 10 \

Check warning on line 46 in .github/actions/tests/test-controller/action.yaml

View check run for this annotation

probelabs / Visor: architecture

logic Issue 

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.
Raw output 
If 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.
-w "%{http_code}" -o /tmp/tui_response.txt \

Check warning on line 47 in .github/actions/tests/test-controller/action.yaml

View check run for this annotation

probelabs / Visor: quality

architecture Issue 

The number of retries for the `curl` command has been reduced from 5 (in the original implementation) to 2. This could make the action more susceptible to failures from transient network or service issues, potentially increasing pipeline flakiness.
Raw output 
Consider restoring the retry count to 5 to maintain the previous level of resilience against transient errors, unless the reduction was a deliberate choice to fail faster.
"$BASE_URL/$BASE_REF/$ENDPOINT")

if [ "$HTTP_CODE" = "404" ]; then
echo "::warning::No test config for branch '$BASE_REF', falling back to '$DEFAULT_BRANCH'"
curl -s --retry 2 --retry-delay 10 --fail-with-body \

Check warning on line 52 in .github/actions/tests/test-controller/action.yaml

View check run for this annotation

probelabs / Visor: performance

performance Issue 

The 'cat' process is unnecessary. The file content can be redirected to standard error directly.
Raw output 
Replace 'cat /tmp/tui_response.txt >&2' with '< /tmp/tui_response.txt >&2'. This avoids spawning an extra 'cat' process for a simple redirection.

Check failure on line 52 in .github/actions/tests/test-controller/action.yaml

View check run for this annotation

probelabs / Visor: quality

logic Issue 

The fallback `curl` command pipes output directly to `GITHUB_OUTPUT`. If this request fails (e.g., returns a 500 error), `curl --fail-with-body` writes the error response body to the output file, which can corrupt it and cause downstream steps to fail. The script then exits due to `set -e`, but the corrupted output file remains.
Raw output 
The fallback request should be handled with the same care as the primary one. Capture the HTTP status code and response body separately. Only write the body to `GITHUB_OUTPUT` if the status code is 200. Otherwise, print an informative error message and exit with a failure status. Refactoring the logic to avoid code duplication is also recommended.

Check warning on line 52 in .github/actions/tests/test-controller/action.yaml

View check run for this annotation

probelabs / Visor: security

security Issue 

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.
Raw output 
Avoid 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.
"$BASE_URL/$DEFAULT_BRANCH/$ENDPOINT" | tee -a "$GITHUB_OUTPUT"
elif [ "$HTTP_CODE" != "200" ]; then

Check failure on line 54 in .github/actions/tests/test-controller/action.yaml

View check run for this annotation

probelabs / Visor: security

security Issue 

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.
Raw output 
Sanitize 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`.

```yaml
      run: |
        set -eo pipefail

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

        BASE_URL="http://tui.internal.dev.tyk.technology/v2/$VARIATION/$REPO_NAME"
        # ... rest of the script
```
echo "API returned HTTP $HTTP_CODE for branch '$BASE_REF'" >&2

Check warning on line 55 in .github/actions/tests/test-controller/action.yaml

View check run for this annotation

probelabs / Visor: performance

performance Issue 

The 'cat' process is unnecessary when piping to 'tee'. The input file can be redirected to 'tee's standard input directly.
Raw output 
Replace '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.
cat /tmp/tui_response.txt >&2
exit 1
else
cat /tmp/tui_response.txt | tee -a "$GITHUB_OUTPUT"
fi

if ! [[ $VARIATION =~ prod ]] ;then
echo "::warning file=.github/workflows/release.yml,line=24,col=1,endColumn=8::Using non-prod variation"
echo "### :warning: You are using VARIATION=${VARIATION} in test-controller-{{ .test }}" >> $GITHUB_STEP_SUMMARY
fi
Loading