Swift: Convert .qlref security query tests to inline expectation tests#21973
Conversation
|
@copilot In |
Fixed in the latest commit. For each of the three folders:
|
This was stopping trailing comments, as in `// $ Alert // some comment`, from working.
c9a0410 to
85e1ae2
Compare
85e1ae2 to
b10abb6
Compare
There was a problem hiding this comment.
Pull request overview
This PR migrates Swift security query tests from .expected-file-driven assertions to inline expectation markers (// $ Alert, // $ Source, // $ Sink) in the Swift test sources, using utils/test/InlineExpectationsTestQuery.ql as a postprocessor to validate results.
Changes:
- Updated multiple Swift security
.qlreftests to addpostprocess: utils/test/InlineExpectationsTestQuery.ql. - Added inline expectation annotations across Swift test sources to represent expected alerts/sources/sinks directly in code.
- Tweaked Swift’s inline-expectations comment extraction to trim comment contents.
Show a summary per file
| File | Description |
|---|---|
| swift/ql/test/query-tests/Security/CWE-916/test.swift | Adds inline $ Source/$ Alert markers for insufficient hash-iteration test cases. |
| swift/ql/test/query-tests/Security/CWE-916/InsufficientHashIterations.qlref | Adds inline-expectations postprocessor. |
| swift/ql/test/query-tests/Security/CWE-760/test.swift | Adds inline markers for constant-salt test cases. |
| swift/ql/test/query-tests/Security/CWE-760/rncryptor.swift | Adds inline markers for constant-salt test cases and tracks known missing alerts. |
| swift/ql/test/query-tests/Security/CWE-760/ConstantSalt.qlref | Adds inline-expectations postprocessor. |
| swift/ql/test/query-tests/Security/CWE-730/tests.swift | Adds inline markers for regex-injection alerts/sources and known spurious results. |
| swift/ql/test/query-tests/Security/CWE-730/RegexInjection.qlref | Adds inline-expectations postprocessor. |
| swift/ql/test/query-tests/Security/CWE-730/RegexInjection.expected | Regenerated/rewritten expected output layout (now includes #select at top). |
| swift/ql/test/query-tests/Security/CWE-328/WeakSensitiveDataHashing.qlref | Adds inline-expectations postprocessor. |
| swift/ql/test/query-tests/Security/CWE-328/WeakPasswordHashing.qlref | Adds inline-expectations postprocessor. |
| swift/ql/test/query-tests/Security/CWE-328/testCryptoSwift.swift | Adds query-id-qualified inline $ Alert[...] markers to disambiguate overlapping weak-hash queries. |
| swift/ql/test/query-tests/Security/CWE-328/testCryptoKit.swift | Adds query-id-qualified inline $ Alert[...] and $ Source[...] markers; retains known missing alerts. |
| swift/ql/test/query-tests/Security/CWE-327/test.swift | Adds inline $ Source/$ Alert markers for ECB encryption cases. |
| swift/ql/test/query-tests/Security/CWE-327/ECBEncryption.qlref | Adds inline-expectations postprocessor. |
| swift/ql/test/query-tests/Security/CWE-312/testUserDefaults.swift | Adds query-id-qualified inline markers for cleartext storage (preferences) cases. |
| swift/ql/test/query-tests/Security/CWE-312/testNSUbiquitousKeyValueStore.swift | Adds query-id-qualified inline markers for iCloud key-value storage cases. |
| swift/ql/test/query-tests/Security/CWE-312/CleartextStoragePreferences.qlref | Adds inline-expectations postprocessor. |
| swift/ql/test/query-tests/Security/CWE-312/CleartextStoragePreferences.expected | Regenerated/rewritten expected output layout (now includes #select at top). |
| swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift | Adds query-id-qualified inline markers for cleartext logging cases; retains known missing alerts. |
| swift/ql/test/query-tests/Security/CWE-311/testURL.swift | Adds query-id-qualified inline markers for cleartext transmission in URL construction; retains known missing alerts. |
| swift/ql/test/query-tests/Security/CWE-311/testSwiftUI.swift | Adds a query-id-qualified inline missing-alert marker in a SwiftUI-specific case. |
| swift/ql/test/query-tests/Security/CWE-311/testSend.swift | Adds query-id-qualified inline markers for cleartext transmission via NWConnection.send. |
| swift/ql/test/query-tests/Security/CWE-311/testRealm2.swift | Adds query-id-qualified inline markers for cleartext database storage via Realm; retains known missing alerts. |
| swift/ql/test/query-tests/Security/CWE-311/testRealm.swift | Adds query-id-qualified inline markers for cleartext database storage via Realm. |
| swift/ql/test/query-tests/Security/CWE-311/testGRDB.swift | Adds query-id-qualified inline markers for cleartext database storage via GRDB APIs. |
| swift/ql/test/query-tests/Security/CWE-311/testCoreData2.swift | Adds query-id-qualified inline markers for CoreData persistence cases; includes spurious markers. |
| swift/ql/test/query-tests/Security/CWE-311/testCoreData.swift | Adds query-id-qualified inline markers for CoreData persistence cases; includes missing/spurious markers. |
| swift/ql/test/query-tests/Security/CWE-311/testAlamofire.swift | Adds query-id-qualified inline markers for cleartext transmission via Alamofire; retains known missing alerts. |
| swift/ql/test/query-tests/Security/CWE-311/sqlite3_c_api.swift | Adds query-id-qualified inline markers for cleartext database storage via sqlite3 C APIs. |
| swift/ql/test/query-tests/Security/CWE-311/SQLite.swift | Adds query-id-qualified inline markers for cleartext database storage via SQLite.swift APIs. |
| swift/ql/test/query-tests/Security/CWE-311/CleartextTransmission.qlref | Adds inline-expectations postprocessor. |
| swift/ql/test/query-tests/Security/CWE-311/CleartextStorageDatabase.qlref | Adds inline-expectations postprocessor. |
| swift/ql/test/query-tests/Security/CWE-259/test.swift | Adds inline markers for constant-password test cases. |
| swift/ql/test/query-tests/Security/CWE-259/rncryptor.swift | Adds inline $ Source/$ Alert markers for constant-password flows. |
| swift/ql/test/query-tests/Security/CWE-259/ConstantPassword.qlref | Adds inline-expectations postprocessor. |
| swift/ql/test/query-tests/Security/CWE-134/UncontrolledFormatString.swift | Adds inline $ Source/$ Alert markers for uncontrolled format-string cases; retains known missing alerts. |
| swift/ql/test/query-tests/Security/CWE-134/UncontrolledFormatString.qlref | Adds inline-expectations postprocessor. |
| swift/ql/test/query-tests/Security/CWE-1333/ReDoS.swift | Adds inline $ Alert markers for ReDoS cases, preserving explanatory comments. |
| swift/ql/test/query-tests/Security/CWE-1333/ReDoS.qlref | Adds inline-expectations postprocessor. |
| swift/ql/test/query-tests/Security/CWE-1204/test.swift | Adds inline $ Source/$ Alert markers for static IV usage. |
| swift/ql/test/query-tests/Security/CWE-1204/StaticInitializationVector.qlref | Adds inline-expectations postprocessor. |
| swift/ql/test/query-tests/Security/CWE-1204/rncryptor.swift | Adds inline $ Source/$ Alert markers for static IV usage in RNEncryptor flows. |
| swift/ql/test/query-tests/Security/CWE-116/test.swift | Adds inline $ Alert markers for bad tag-filter regex variants. |
| swift/ql/test/query-tests/Security/CWE-116/BadTagFilter.qlref | Adds inline-expectations postprocessor. |
| swift/ql/test/query-tests/Security/CWE-089/sqlite3_c_api.swift | Adds inline $ Source/$ Alert markers for SQL injection via sqlite3 APIs. |
| swift/ql/test/query-tests/Security/CWE-089/SQLite.swift | Adds inline $ Source/$ Alert markers for SQL injection via SQLite.swift APIs. |
| swift/ql/test/query-tests/Security/CWE-089/SqlInjection.qlref | Adds inline-expectations postprocessor. |
| swift/ql/test/query-tests/Security/CWE-089/other.swift | Adds inline $ Source/$ Alert markers for heuristic SQL injection stubs; retains known missing alerts. |
| swift/ql/test/query-tests/Security/CWE-089/GRDB.swift | Adds inline $ Source/$ Alert markers for SQL injection via GRDB APIs. |
| swift/ql/test/query-tests/Security/CWE-079/UnsafeWebViewFetch.swift | Adds inline $ Source/$ Alert markers for unsafe WebView fetch flows. |
| swift/ql/test/query-tests/Security/CWE-079/UnsafeWebViewFetch.qlref | Adds inline-expectations postprocessor. |
| swift/ql/test/query-tests/Security/CWE-079/UnsafeWebViewFetch.expected | Regenerated/rewritten expected output layout (now includes #select at top). |
| swift/ql/test/query-tests/Security/CWE-022/UnsafeUnpack/UnsafeUnpack.swift | Adds inline $ Source/$ Alert markers for unsafe unzip/unpack flows. |
| swift/ql/test/query-tests/Security/CWE-022/UnsafeUnpack/UnsafeUnpack.qlref | Adds inline-expectations postprocessor. |
| swift/ql/test/query-tests/Security/CWE-020/UnanchoredUrlRegex.swift | Adds query-id-qualified inline markers for missing-regexp-anchor cases, including missing/spurious annotations. |
| swift/ql/test/query-tests/Security/CWE-020/test.swift | Adds query-id-qualified inline markers for incomplete-hostname/missing-anchor regex cases. |
| swift/ql/test/query-tests/Security/CWE-020/SemiAnchoredRegex.swift | Adds query-id-qualified inline markers for semi-anchored regex cases. |
| swift/ql/test/query-tests/Security/CWE-020/MissingRegexAnchor.qlref | Adds inline-expectations postprocessor. |
| swift/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegex.qlref | Adds inline-expectations postprocessor. |
| swift/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll | Adjusts Swift comment content extraction used by the inline expectations framework. |
Copilot's findings
- Files reviewed: 62/62 changed files
- Comments generated: 3
| db.query(sqlStatement: remoteString) // BAD [NOT DETECTED] | ||
| db.query(sqliteStatement: remoteString) // BAD [NOT DETECTED] | ||
| db.query(sql: remoteString) // $ Alert | ||
| db.query(sqlLiteral: remoteString) // $ MISSING: Alert // BAD [NOT DETECTED] |
There was a problem hiding this comment.
In cases like these, where we just had // BAD [NOT DETECTED] with no additional text, should we just drop the // BAD [NOT DETECTED]?
There was a problem hiding this comment.
My policy has been to just keep all the original comments, as there are too many to make decisions on all of them. You are very welcome to either push a commit to this PR clearing them up or do it as a follow-up.
| } | ||
| set { | ||
| setValue(newValue, forKey: "myKey") // [additional result reported here] | ||
| setValue(newValue, forKey: "myKey") // $ Alert[swift/cleartext-storage-database] // [additional result reported here] |
There was a problem hiding this comment.
Should this be marked SPURIOUS
There was a problem hiding this comment.
It isn't totally clear to me from the comment. I added SPURIOUS and MISSING as a best-effort based on searching for comments like "not detected", and I don't consider it a blocker to this PR to get those all correct. Gopefully someone with more language knowledge can interpret what this comment means.
There was a problem hiding this comment.
Looking at 2622de9, not having a SPURIOUS here is seems the right thing.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Converts 19 Swift security query
.qlreftests from.expected-file-based testing to inline expectation tests by adding// $ Alert,// $ Source, and// $ Sinkcomments directly in the Swift source files.What changed
.qlreffiles updated to addpostprocess: utils/test/InlineExpectationsTestQuery.ql(compact single-line form).expectedfiles:@kind problemqueries:// $ Alertat each result's end-line@kind path-problemqueries:// $ Alertat alert location,// $ Sourceat source (when on different line),// $ Sinkat sink (when on different line from alert).expectedfiles were modifiedBefore/after example (
CWE-760/ConstantSalt):Lines with pre-existing
// BAD, descriptive textcomments were restructured to// $ Alert // BAD, descriptive textso the$appears first in the comment content (required by the inline expectation parser).Skipped tests (12)
PrintAst.qlreflibrary-tests/ast/Errors.qlref@kindmetadataAlertSuppression.qlref@kind alert-suppressionDiagnostics/ExtractedFiles.qlref@kind diagnosticDiagnostics/SuccessfullyExtractedLines.qlref@kind metricCWE-757/InsecureTLS.qlreffile://synthetic alert locations in#select(9 rows)CWE-321/HardcodedEncryptionKey.qlreffile://synthetic alert locationCWE-078/CommandInjection.qlreffile://synthetic alert locationsCWE-135/StringLengthConflation.qlreffile://synthetic source locations (13 rows)Original prompt
Follow these instructions for Swift.
qlref conversion instructions
Here’s the repeatable recipe for converting a CodeQL language’s
.qlreftests to inline expectation tests.Overall Workflow
cdinto the language folder.Run
grep -rL "InlineExpectationsTestQuery" --include="*.qlref".Ignore generated
.actualfiles,.testprojcopies, and other generated artifacts.For each
.qlref, resolve the referenced query and read its metadata.Classify each test before editing:
-
convert nowfor standard@kind problemand@kind path-problem-
skipfor anything else, with a short concrete reason.expectedfile before converting:- If it is empty and the query is a standard
problemorpath-problemquery, usually convert by adding the postprocessor only. No source comments are needed.- If it is non-empty, add inline expectation comments to the source files at the expected result locations, then add the postprocessor to the
.qlref.Keep the existing
.expectedfile. Do not edit generated expected files by hand, even when converting.Edit only source files and
.qlreffiles.Maintain a skip list for tests not converted, with exact paths and short reasons.
Do not make a PR.
.qlrefFormattingFor a single postprocessor, use the compact form:
If the
.qlrefalready has other postprocessors, use list form:If the original
.qlrefwas a plain single-line query reference, convert it toquery:form only when addingpostprocess:.Always preserve existing postprocessors.
Expectation Comments
Use the language’s supported inline comment syntax from its inline expectation implementation. Typical tags are:
For example, in Java or Kotlin:
For
problemqueries, addAlertat each expected result location.For
path-problemqueries, add:Alertat the alert location.Sourceat the source location, unless the source is on the same line as the alert.Sinkat the sink location when it is a distinct relevant location.Multi-line result locations should be annotated on the end line.
If more than one result is expected on the same line for the same query, just use a single tag in the comment, for example:
Only add comments to real source files that belong to the test. Do not annotate generated copies, shared vendored fixtures, or shared stubs unless that is clearly the right ownership boundary for the test.
Empty
.expectedFilesIf a
problemorpath-problemtest already has an empty.expectedfile, the default conversion is:Leave
.expecteduntouched.Add
postprocess: utils/test/InlineExpectationsTestQuery.qlto the.qlref.Add no source comments unless the test actually has expected results that are currently represented elsewhere.
This is common in zero-result tests.
Query Ids
Do not add
[query-id]by default.Only qualify tags when needed, usually when more than one inline expectation test in the same effective test folder or database can see the same source comments. Then use the query’s metadata id:
Apply this to
Alert,Source, andSinkonly for the comments that need disambiguation.If you add query ids in a shared folder, validate the owner queries for those comments as well, not just the query you were editing.
Good Skip Reasons
Skip, or revert to non-inline, when conversion would make the test misleading, brittle, or noisy. Common reasons:
The query is not
@kind problemor@kind path-problem.The query is a telemetry, metrics, table, definitions, diagnostic, extractor-information, stub-generation, or tool-specific query.
The test is an AST dump or similar structural output test, such as
PrintAst.The selected result text is inside a string literal, raw string, heredoc, text block, XML literal, or multiline literal and adding a comment changes the selected text.
The expected location is inside documentation syntax where adding a comment changes what the query reports, such as Javadoc tags.
The
.expectedoutput is nonstandard enough that mechanical conversion is risky.Results are in shared stubs, generated copies, vendored files, or synthetic/test-project paths where inline comments would pollute shared fixtures.
The alert, source, or sink locations are too dense or ambiguous to make the source understandable.
The language’s inline expectation implementation does not support the source file or comment style needed.
The query uses a special result kind or framework behavior, such as alert-...
Created from VS Code.