Skip to content

Conversation

owen-mc
Copy link
Contributor

@owen-mc owen-mc commented Oct 9, 2025

Should fix this issue.

@Copilot Copilot AI review requested due to automatic review settings October 9, 2025 11:25
@owen-mc owen-mc requested a review from a team as a code owner October 9, 2025 11:25
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the CodeQL Go request forgery analysis to sanitize simple types (numeric and boolean types) that are unlikely to carry taint. It also introduces a shared sanitizer class and deprecates an existing SQL injection sanitizer.

Key changes:

  • Creates a new shared SimpleTypeSanitizer class for simple types like numbers and booleans
  • Applies this sanitizer to the request forgery detection to reduce false positives
  • Deprecates the existing NumericOrBooleanSanitizer in SQL injection analysis in favor of the shared sanitizer

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
go/ql/lib/semmle/go/security/Sanitizers.qll New shared sanitizer module with SimpleTypeSanitizer class
go/ql/lib/semmle/go/security/RequestForgeryCustomizations.qll Adds simple type sanitization to request forgery analysis
go/ql/lib/semmle/go/security/SqlInjectionCustomizations.qll Deprecates existing sanitizer in favor of shared implementation
go/ql/test/query-tests/Security/CWE-918/tst.go Adds test case for simple type sanitization
go/ql/test/query-tests/Security/CWE-918/RequestForgery.ext.yml Test configuration for the new test case
go/ql/test/query-tests/Security/CWE-918/RequestForgery.expected Updated expected test results
go/ql/lib/change-notes/ Documentation for the changes

@owen-mc owen-mc force-pushed the go/sanitize-simpletypes-request-forgery branch from fc69a65 to 3715179 Compare October 9, 2025 11:26
Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

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

LGTM

@owen-mc
Copy link
Contributor Author

owen-mc commented Oct 9, 2025

I ran multi-repo analyses before and after. One repo out of the 1000 had alert changes: we go from 26 to 21 results on valyala/fasthttp, which all go through this function which examines a string byte by byte and escapes certain characters. This isn't what I thought the sanitizer would do, but it is good because this is an escaping function.

@owen-mc owen-mc merged commit 87f32dc into github:main Oct 10, 2025
17 checks passed
@owen-mc owen-mc deleted the go/sanitize-simpletypes-request-forgery branch October 10, 2025 08:15
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.

False positive - uncontrolled data used in network request golang int64
2 participants