Skip to content
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.

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

Go: Rename UntrustedFlowSource to RemoteFlowSource to match other language libraries #16250

Merged
merged 13 commits into from
Apr 24, 2024

Conversation

owen-mc
Copy link
Contributor

@owen-mc owen-mc commented Apr 18, 2024

Go was the the only language library which wasn't using the name RemoteFlowSource, so this seemed worth fixing for the sake of consistency.

Related renamings have also been done. Some classes which should have been private have been made private while we are deprecating the old name anyway. Appropriate instructions have been put in the deprecation comments.

owen-mc added 11 commits April 17, 2024 21:27
Only the whole word. Skipped one instance in an old change note.
Relaxed whole word requirement. Again skipped one instance in an old
change note.
Relaxed match case requirement. Again skipped one instance in an old
change note.
Not matching case but preserving original case.
Preserve case, allow for "a `Remote" etc.
Including renaming some files (in the experimental folder).
`UntrustedFlowAsSource` should have been private. Since we are deprecating them anyway
we may as well make the replacement private (and make it use `instanceof`). The deprecation
comments have been updated.
@owen-mc owen-mc requested a review from a team as a code owner April 18, 2024 11:59
@owen-mc owen-mc force-pushed the go/rename-untrusted-flow-source branch from 57be161 to dc985c2 Compare April 18, 2024 12:02
@owen-mc owen-mc requested a review from smowton April 22, 2024 15:11
Copy link
Member

@mbg mbg left a comment

Choose a reason for hiding this comment

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

Thanks for doing this to bring Go in line with other languages here! This generally all looks good. I just have some minor comments and a recurring question about a bunch of the comments on the now deprecated classes, which suggests using a now private replacement. Feel free to answer one and ignore the others if that's fine as is.

Some classes which should have been private have been made private while we are deprecating the old name anyway.

Why should they be private?

/** A source of untrusted data, considered as a taint source for command injection. */
class UntrustedFlowAsSource extends Source instanceof UntrustedFlowSource { }
private class RemoteFlowAsSource extends Source instanceof RemoteFlowSource { }
Copy link
Member

Choose a reason for hiding this comment

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

Why private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It just doesn't make much sense to make this public. It isn't really of any use for external code to reference it, and will probably just lead to confusion if it is done by mistake.

go/ql/src/experimental/CWE-918/SSRF.qll Show resolved Hide resolved
Co-authored-by: Michael B. Gale <[email protected]>
@owen-mc owen-mc merged commit f828f8e into github:main Apr 24, 2024
18 checks passed
@owen-mc owen-mc deleted the go/rename-untrusted-flow-source branch April 24, 2024 10:37
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.

2 participants