Skip to content

Conversation

@LawnGnome
Copy link
Contributor

PostgreSQL has an unusual definition of UTF8 that doesn't allow for null bytes, even though they are technically valid UTF-8 by any other definition (including Rust's str type). Sending a crate search request that includes a null byte will eventually result in a database error anyway, but let's save Sentry the trouble and just 400 out early.

I did a quick audit of other places that we use query parameters and don't see any other code paths where we use a string directly as a query parameter, but I won't pretend this was a super detailed survey. (Fun fact: I can actually see the power usage for my laptop increase on my fancy new charger when I hit "show references" on the query method in RequestUtils.)

Fixes #9444.

[PostgreSQL has an unusual definition of `UTF8` that doesn't allow for
null bytes][null-bytes], even though they are technically valid UTF-8 by
any other definition (including Rust's `str` type). Sending a crate
search request that includes a null byte will eventually result in a
database error anyway, but let's save Sentry the trouble and just 400
out early.

Fixes rust-lang#9444.

[null-bytes]: https://www.commandprompt.com/blog/null-characters-workarounds-arent-good-enough/
@LawnGnome LawnGnome added C-bug 🐞 Category: unintended, undesired behavior A-backend ⚙️ labels Sep 19, 2024
@LawnGnome LawnGnome requested a review from a team September 19, 2024 19:57
@LawnGnome LawnGnome self-assigned this Sep 19, 2024
@codecov
Copy link

codecov bot commented Sep 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.10%. Comparing base (2baaf0b) to head (110e569).
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9473      +/-   ##
==========================================
+ Coverage   89.09%   89.10%   +0.01%     
==========================================
  Files         286      286              
  Lines       29035    29039       +4     
==========================================
+ Hits        25868    25875       +7     
+ Misses       3167     3164       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +58 to +64
let option_param = |s| match params.get(s).map(|v| v.as_str()) {
Some(v) if v.contains('\0') => Err(bad_request(format!(
"parameter {s} cannot contain a null byte"
))),
Some(v) => Ok(Some(v)),
None => Ok(None),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it okay to simply write it as:

Suggested change
let option_param = |s| match params.get(s).map(|v| v.as_str()) {
Some(v) if v.contains('\0') => Err(bad_request(format!(
"parameter {s} cannot contain a null byte"
))),
Some(v) => Ok(Some(v)),
None => Ok(None),
};
let option_param = |s| match params.get(s).map(|v| v.as_str()) {
Some(v) if v.contains('\0') => Err(bad_request(format!(
"parameter {s} cannot contain a null byte"
))),
v @ _ => Ok(v), // or just v => Ok(v)
};

Or will it be more difficult to read in this way?

Copy link
Member

Choose a reason for hiding this comment

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

I'll treat this comment as non-blocking. up to Adam whether he wants to pull this into a follow-up PR :)

@Turbo87 Turbo87 merged commit 4814999 into rust-lang:main Sep 20, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-backend ⚙️ C-bug 🐞 Category: unintended, undesired behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DatabaseError: invalid byte sequence for encoding "UTF8": 0x00

3 participants