Go: Add tests for SafeUrlFlow, and fix a latent bug #20556
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive test coverage for the SafeUrlFlow library and fixes a bug where an unsafe field read sanitizer was incorrectly specified. The fix prevents some URLs from being incorrectly considered safe, which may result in more security findings.
Key changes:
- Added comprehensive test files for SafeUrlFlow functionality including various sources, sinks, and sanitizers
- Fixed bug in field read sanitizer specification by moving and correcting the implementation
- Updated imports to make SafeUrlFlow dependencies private where appropriate
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| SafeUrlFlow.qlref | Test query reference file to run SafeUrlFlow tests |
| SafeUrlFlow.ql | Main test query that traces safe URL flows from sources to sinks |
| SafeUrlFlow.go | Comprehensive test cases covering various flow scenarios, sanitizers, and edge cases |
| SafeUrlFlow.expected | Expected test results showing flow paths and sanitization behavior |
| SafeUrlFlowCustomizations.qll | Added UnsafeFieldReadSanitizer class with corrected field name specification |
| RequestForgeryCustomizations.qll | Removed duplicate UnsafeFieldReadSanitizer implementation and made import private |
| OpenUrlRedirectCustomizations.qll | Removed duplicate UnsafeFieldReadSanitizer implementation and made import private |
owen-mc
force-pushed
the
go/test/safeurlflow
branch
from
233b351 to
a111be4
Compare
|
I just force-pushed to fix the tests. I left something in that didn't work because of def-use flow - I now test for it in a separate function. |
owen-mc
force-pushed
the
go/test/safeurlflow
branch
from
a111be4 to
dd3f754
Compare
|
I have started DCA. I also just force-pushed to improve the wording of the change note. |
| fragment := baseURL.Fragment // should preserve flow | ||
| user := baseURL.User // should preserve flow (but unsafe field) | ||
|
|
||
| // These should still have flow (not sanitized) |
There was a problem hiding this comment.
All the wording about flow and alerts in this file leaves me confused. We're testing flow of things known to be safe, right? I.e. stuff that's subtracted from the actual query results? But the wording in a bunch of these comments seem to mix things up - e.g. by talking about a sanitizer that blocks flow, which then means that we don't get "safe" flow, so the sanitizer makes things unsafe? Or what's going on?
There was a problem hiding this comment.
Hmm, yes the terminology is confusing, since we're so used to a flow config tracking things that are unsafe, but in this case it is tracking something which is safe. I've removed uses of "unsafe" and tried to stick to flow terminology like "source", "sink", "propagate" and "barrier". Also, the tests were written by copilot, which tends to add lots of comments. I had already removed many redundant ones, but I've removed some more where they don't add any clarity. (And one or two were wrong.)
There was a problem hiding this comment.
I still find the comments confusing. Perhaps we could name all the sources something like safeURL. And then instead of talking about "preserving flow" we should say that something "remains safe" or similar. And instead of talking about "not preserving flow" or "barriers" then we should call out that now the thing is no longer guaranteed to be safe (and possibly why). For instance, it's somewhat counterintuitive that a string can be safe, but taking a substring makes it not safe.
aschackmull
left a comment
aschackmull
left a comment
There was a problem hiding this comment.
Besides the test file, which I don't quite get, then the bugfix and other QL changes LGTM.
owen-mc
force-pushed
the
go/test/safeurlflow
branch
from
493cd79 to
c93852d
Compare
aschackmull
left a comment
aschackmull
left a comment
There was a problem hiding this comment.
Thanks for indulging me. LGTM now.
2 participants
This didn't have any tests, so I generated some using copilot. While fixing up the tests I noticed a latent bug - a sanitizer had been incorrectly specified. This means that some URLs were considered safe when they shouldn't have been, so we may find more results now that it is fixed.