Skip to content

Conversation

@PopoviciMarian
Copy link
Contributor

No description provided.

root and others added 14 commits September 10, 2025 17:04
* updated path traversal, shell injection, SQL injection, and SSRF checks to utilize TrimInvisible for input sanitization

* Update

* Update

* Add test

---------

Co-authored-by: root <[email protected]>
* implement URL normalization function and enhance SSRF detection

* add error checking in URL parsing

* refactor

* add default ports for HTTP and HTTPS schemes

* update test

* adding user info removal and improving backslash handling in request processing

* fix test cases for URL normalization

* enhance URL normalization and add test cases for SSRF detection

* refactor SSRF test

* update

* update

* add comment to clarify URL parsing error handling

* test

---------

Co-authored-by: root <[email protected]>
* Add GitHub Actions workflow for building PHP extension images
}

func isTrimRune(r rune) bool {
// 1) All Unicxde whitespace
Copy link
Contributor

@aikido-pr-checks aikido-pr-checks bot Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment contains typo 'Unicxde' instead of 'Unicode' which makes documentation incorrect. More info

}
// If the port is not present, we need to add it based on the scheme
if parsedURL.Port() == "" {
port := 0
Copy link
Contributor

@aikido-pr-checks aikido-pr-checks bot Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default port logic sets port to 0 for non-http/https schemes, resulting in invalid URLs like 'example.com:0'. More info

@PopoviciMarian PopoviciMarian changed the title Add QA testing workflow [TEST] - Add QA testing workflow Sep 15, 2025
func removeCTLByte(urlStr string) string {
for i := 0; i < len(urlStr); i++ {
if urlStr[i] <= ' ' || urlStr[i] == 0x7f {
urlStr = urlStr[:i] + urlStr[i+1:]
Copy link
Contributor

@aikido-pr-checks aikido-pr-checks bot Sep 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function parameter urlStr is reassigned, making it harder to track the original input value. More info

return false
}
// if hostname contains : we need to add the [ and ] to the hostname (ipv6)
if strings.Contains(hostname, ":") {
Copy link
Contributor

@aikido-pr-checks aikido-pr-checks bot Sep 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameter 'hostname' is reassigned, making original value unclear. More info


userInput = helpers.ExtractResourceOrOriginal(userInput)
variants := []string{userInput, "http://" + userInput, "https://" + userInput}
userInput = helpers.NormalizeRawUrl(userInput)
Copy link
Contributor

@aikido-pr-checks aikido-pr-checks bot Sep 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameter 'userInput' is reassigned, making original value unclear. More info

}
}

// if the response is 302, we need to check CURLINFO_REDIRECT_URL
Copy link
Contributor

@aikido-pr-checks aikido-pr-checks bot Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment restates obvious code mechanics without explaining why 302 responses need special redirect URL checking. More info

return Context.Callback(C.OUTGOING_REQUEST_RESPONSE_CODE)
}

func GetOutgoingRequestRedirectUrl() string {
Copy link
Contributor

@aikido-pr-checks aikido-pr-checks bot Sep 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function name GetOutgoingRequestRedirectUrl suggests it returns a URL but actually returns only the hostname portion. More info

Comment on lines +451 to +484
runs-on: ubuntu-latest
needs: [ build_deb ]
steps:

- name: Checkout zen-demo-php
uses: actions/checkout@v4
with:
repository: Aikido-demo-apps/zen-demo-php
path: zen-demo-php
ref: dev-testing
submodules: recursive

- name: Get Arch
run: echo "ARCH=$(uname -m)" >> $GITHUB_ENV

- name: Download artifacts
uses: actions/download-artifact@v4
with:
name: aikido-php-firewall.${{ env.ARCH }}.deb
path: ./zen-demo-php

- name: Overwrite aikido.sh install script
run: |
echo "dpkg -i -E \"/var/www/html/aikido-php-firewall.\$(uname -i).deb\"" > ./zen-demo-php/.fly/scripts/aikido.sh
- name: Run Firewall QA Tests
uses: AikidoSec/firewall-tester-action@releases/v1
with:
dockerfile_path: ./zen-demo-php/Dockerfile
extra_args: '--env-file=./zen-demo-php/.env.example -e APP_KEY=base64:W2v6u6VR4lURkxuMT9xZ6pdhXSt5rxsmWTbd1HGqlIM='
sleep_before_test: 20
skip_tests: test_wave_attack,test_rate_limiting_group_id_1_minute
max_parallel_tests: 7
ignore_failures: true

Check warning

Code scanning / CodeQL

Workflow does not contain permissions Medium

Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {contents: read}

Copilot Autofix

AI about 1 month ago

To fix this problem, we should explicitly set the permissions for the affected job to the minimal necessary privileges. The most conservative default is contents: read, which allows the job to checkout code and read repository contents, but does not allow write operations. This should be added to the test_php_qa_action job definition, immediately after the runs-on: line (line 451, in context), or optionally also at the workflow root for global enforcement. Since only this job is flagged, we will add:

permissions:
  contents: read

after runs-on: ubuntu-latest (line 451).


Suggested changeset 1
.github/workflows/build.yml

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -449,6 +449,8 @@
   
   test_php_qa_action:
     runs-on: ubuntu-latest
+    permissions:
+      contents: read
     needs: [ build_deb ]
     steps:
 
EOF
@@ -449,6 +449,8 @@

test_php_qa_action:
runs-on: ubuntu-latest
permissions:
contents: read
needs: [ build_deb ]
steps:

Copilot is powered by AI and may make mistakes. Always verify output.
return ""
}

parts := strings.Split(value, ",")
Copy link
Contributor

@aikido-pr-checks aikido-pr-checks bot Sep 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function has 6+ levels of nested control flow that could be simplified with early returns or helper functions. More info

* Add GetMethod function to Server class for handling HTTP method overrides

- Implemented GetMethod to retrieve the request method, supporting X-HTTP-METHOD-OVERRIDE and _method query parameter for frameworks like Symfony and Laravel.
- Added utility function ToUppercase for string manipulation.
- Updated GoContextCallback to use the new GetMethod function.

* test

* Add getMethodFromQuery function to Server class

* Refactor getMethodFromQuery to improve query method retrieval

* Refactor Symfony request class check in Server.cpp

* update

* fix

* fix

---------

Co-authored-by: root <[email protected]>
// Return the method from the query param _method (_GET["_method"])
std::string Server::GetMethodFromQuery() {
zval *get_array;
get_array = zend_hash_str_find(&EG(symbol_table), "_GET", sizeof("_GET") - 1);
Copy link
Contributor

@aikido-pr-checks aikido-pr-checks bot Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pointer get_array is dereferenced without null check after zend_hash_str_find call. More info

std::string Server::GetMethod() {
std::string method = ToUppercase(this->GetVar("REQUEST_METHOD"));

// TODO: Add a check here to see if the request is from Symfony or Laravel
Copy link
Contributor

@aikido-pr-checks aikido-pr-checks bot Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO comment indicates incomplete work that should be tracked properly or implemented. More info

* Propagate disk logs env (#274)

* Propage disk logs

* Remove SQL query logging

---------

Co-authored-by: Tudor Timcu <[email protected]>
Co-authored-by: root <[email protected]>
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