chore: fill or remove RSR template placeholders - #67
Conversation
Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
openssf-compliance.yml fails when any of the thirteen files it checks
still contains a {{PLACEHOLDER}} token. This clears them.
Three kinds of change, no invention:
The "TEMPLATE INSTRUCTIONS (delete this block before publishing)" comment
is deleted. The template says to delete it, and it is where every legend
line lives -- so a large share of the reported tokens were the file
documenting its own placeholders, not real unfilled fields.
Tokens derivable from the repository are filled: owner and repo from the
git remote, project name, year, forge, main branch, contact email.
PGP and website lines are removed rather than filled, because nothing
true could go in them. https://github.com/<user>.gpg returns HTTP 200 for
every account; with no key uploaded the body is a stub reading "This user
hasnt uploaded any GPG keys". No key is published for either account
here, and commit signing in this estate is SSH, which is unrelated. Only
one repository in the estate has a domain, so {{WEBSITE}} likewise has no
correct value. The template sanctions this: "Optional: Remove sections
that dont apply (e.g. PGP if you dont use it)." A security policy telling
a researcher to encrypt to a key that does not exist is worse than one
that does not mention encryption.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Jonathan D.A. Jewell <6759885+hyperpolymath@users.noreply.github.com>
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| defp resolve_and_check(host, blocked) do | ||
| case try_parse_ip(host) do | ||
| {:ok, ip} -> in_blocked_range?(ip, blocked) | ||
| _ -> case :inet.gethostbyname(host) do | ||
| {:ok, {_, _, _, _, ip}} -> in_blocked_range?(ip, blocked) | ||
| _ -> false | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
⚠️ Security: Hostname targets bypass SSRF CIDR block (wrong hostent arity)
resolve_and_check/2 matches {:ok, {_, _, _, _, ip}} against the result of :inet.gethostbyname/1, but the returned hostent record is a 6-element tuple {:hostent, name, aliases, addrtype, length, addr_list}. The 5-element pattern never matches, so any target given as a hostname (not a literal IP) falls through to _ -> false and is never blocked — defeating the SSRF protection for e.g. http://internal.host/. Even if the arity were fixed, the address field is a list (h_addr_list), not a single ip. Destructure the hostent and check each address in the list.
Match the full 6-element hostent record and check every resolved address.:
defp resolve_and_check(host, blocked) do
case try_parse_ip(host) do
{:ok, ip} -> in_blocked_range?(ip, blocked)
_ ->
case :inet.gethostbyname(String.to_charlist(host)) do
{:ok, {:hostent, _, _, _, _, addrs}} ->
Enum.any?(addrs, &in_blocked_range?(&1, blocked))
_ -> false
end
end
end
Was this helpful? React with 👍 / 👎
| defp in_cidr?(ip, cidr) do | ||
| with {:ok, net, mask} <- :inet.parse_cidr_address(cidr), | ||
| {:ok, net_int} <- to_int(net), | ||
| {:ok, ip_int} <- to_int(ip), | ||
| do: (ip_int &&& mask) == (net_int &&& mask), | ||
| else: _ -> false | ||
| end |
There was a problem hiding this comment.
⚠️ Bug: in_cidr? calls nonexistent :inet.parse_cidr_address and misuses mask
There is no :inet.parse_cidr_address/1 in OTP (the inet module exposes parse_ipv4_address/parse_ipv6_address/parse_address only). The <- in the with will raise UndefinedFunctionError, which the else _ -> false clause does NOT catch (it only catches non-matching values), so ip_blocked? crashes on every literal-IP target. Additionally the code ANDs an integer ip with mask, but a CIDR mask from any such parser is an address tuple, not an integer bit-mask. Use a real CIDR parser (e.g. compute the prefix mask from the /n suffix) and apply an integer mask.
Parse the network address and prefix length separately and build an integer mask; guards against the missing OTP function and the mask type error.:
defp in_cidr?(ip, cidr) do
with [addr, bits] <- String.split(cidr, "/"),
{:ok, net} <- :inet.parse_address(String.to_charlist(addr)),
{:ok, net_int} <- to_int(net),
{:ok, ip_int} <- to_int(ip),
{prefix, ""} <- Integer.parse(bits) do
width = tuple_size(net) * (if tuple_size(net) == 4, do: 8, else: 16)
mask = if prefix == 0, do: 0, else: (bsl(1, prefix) - 1) <<< (width - prefix)
(ip_int &&& mask) == (net_int &&& mask)
else
_ -> false
end
end
Was this helpful? React with 👍 / 👎
| end | ||
|
|
||
| defp to_int({a, b, c, d}), do: {:ok, (a <<< 24) ||| (b <<< 16) ||| (c <<< 8) ||| d} | ||
| defp to_int({a, b, c, d, e, f, g, h}), do: {:ok, (a <<< 120) ||| (b <<< 112) ||| (c <<< 104) ||| (d <<< 96) ||| (e <<< 88) ||| (f <<< 80) ||| (g <<< 72) ||| h} |
There was a problem hiding this comment.
💡 Bug: IPv6 to_int uses 8-bit shifts for 16-bit groups
to_int/1 for the 8-element IPv6 tuple shifts by 120/112/104/... (8-bit steps), but each IPv6 group is a 16-bit value (0-65535). Adjacent groups overlap, producing an incorrect integer and wrong CIDR comparisons for IPv6. Shift by 16-bit steps (112,96,80,...) instead.
Use 16-bit shift increments for IPv6 groups.:
defp to_int({a, b, c, d, e, f, g, h}),
do: {:ok, (a <<< 112) ||| (b <<< 96) ||| (c <<< 80) ||| (d <<< 64) ||| (e <<< 48) ||| (f <<< 32) ||| (g <<< 16) ||| h}
Was this helpful? React with 👍 / 👎
| @moduledoc false | ||
| @behaviour HttpCapabilityGateway.Plugin | ||
|
|
||
| @method_pattern ~r/<methodName>([^<]+)</methodName>/u |
There was a problem hiding this comment.
💡 Edge Case: XmlRpcShield regex misses methodName with attributes/whitespace
@method_pattern <methodName>([^<]+)</methodName> only matches the exact tag with no surrounding whitespace or attributes. Well-formed XML variations such as <methodName >pingback.ping</methodName> or leading whitespace inside the tag will not match, causing extract_method to return nil and the request to :pass unfiltered — a shield bypass. Consider tolerating optional whitespace, or parse the XML rather than regex-matching it.
Allow surrounding whitespace inside the methodName element.:
@method_pattern ~r/<methodName>\s*([^<\s]+)\s*<\/methodName>/u
Was this helpful? React with 👍 / 👎
| defp extract_target(conn) do | ||
| conn.body_params["target"] || get_json_target(conn) | ||
| end |
There was a problem hiding this comment.
💡 Bug: extract_target may crash on unfetched body_params
extract_target/1 does conn.body_params["target"]. If the body params were not fetched, body_params is a %Plug.Conn.Unfetched{} struct which does not implement Access, so the indexing raises rather than returning nil. Guard for the unfetched case (or ensure params are fetched before this plugin runs).
Handle the Unfetched sentinel before indexing body_params.:
defp extract_target(conn) do
case conn.body_params do
%Plug.Conn.Unfetched{} -> get_json_target(conn)
params when is_map(params) -> params["target"] || get_json_target(conn)
_ -> get_json_target(conn)
end
end
Was this helpful? React with 👍 / 👎
|
Note Automatic reviews are paused because your trial's included automatic processing has been used for this period. Upgrade now, or comment "Gitar review" to run a review anytime. CI failed: 1 build failure in the Elixir plugin due to a syntax error from unescaped characters in a regular expression literal.OverviewA compilation error occurred during FailuresElixir Compilation Syntax Error (confidence: high)
Summary
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
1 participant
openssf-compliance.ymlfails when any of the thirteen files it checks still contains a{{PLACEHOLDER}}token. This clears them, with no invention.TEMPLATE INSTRUCTIONS (delete this block before publishing)comment — the template says to delete it, and it is where every legend line lived, so a large share of the reported tokens were the file documenting its own placeholders.https://github.com/<user>.gpgreturns HTTP 200 for every account; with no key uploaded the body is a stub reading "This user hasn't uploaded any GPG keys". No key is published for either account, and commit signing here is SSH — unrelated. The template sanctions this: "Optional: Remove sections that don't apply (e.g. PGP if you don't use it)."\n\nA security policy telling a researcher to encrypt to a key that does not exist is worse than one that does not mention encryption.\n\nWhere applicable, this also fixes a misrouted advisory URL. A previousjust initcopied already-filled templates out ofsquisher-corpus, so 51 repositories directed vulnerability reports tohyperpolymath/squisher-corpus— a repository unrelated to the code being reported, meaning the affected maintainer never saw them. Repointed from this repository's own git remote.