grpc / grpc/grpc-rust

tonic-prost-build: URLs in service method comments are not escaped for proper Rust doc formats

Open
#2,741 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
12.5k
Forks
1.3k
Avg merge
4d 7h
Merged PRs (30d)
24

Description

## Bug Report

### Version

tonic v0.14.6
tonic-build v0.14.6
tonic-prost v0.14.6
tonic-prost-build v0.14.6
prost-build v0.14.4

This also happens on the current grpc-rust master branch.

### Platform

Linux x86_64 on GitHub Actions. Also reproduced on macOS arm64.

### Crates

tonic-prost-build and tonic-build

### Description

tonic-prost-build copies bare URLs from RPC method comments into Rust doc comments. This makes cargo doc fail when rustdoc::bare_urls is denied. The default cleanup-markdown feature does not fix it.

Here is a rough sketch for a minimal reproducer:

```proto
syntax = "proto3";

package repro;

service Reproducer {
// https://example.com/
rpc Call(Request) returns (Response) {}
}

message Request {}
message Response {}
```

The build script is:

```rust
fn main() {
tonic_prost_build::configure()
.build_server(false)
.build_transport(false)
.compile_protos(&["proto/repro.proto"], &["proto"])
.unwrap();
}
```

The library is:

```rust
#![deny(rustdoc::bare_urls)]

tonic::include_proto!("repro");
```

Running cargo doc fails because the generated file contains a bare link in the doc-comment on that method on the generated service, like so:

```rust
/// https://example.com/
pub async fn call(/* ... */) {
// ...
}
```

I expected the generated comment to contain an automatic Markdown escaped URL instead, so that it passes Rust doc style checks.

```rust
///
pub async fn call(/* ... */) {
// ...
}
```

# Diagnostics

From what I can work out, prost-build already does this cleanup for message comments via prost's built in sanitization and formatting. Where this goes wrong is that tonic registers/implements its own the [Generator](https://github.com/grpc/grpc-rust/blob/0858621ef6eda0f19928-prost-build/src/lib.rs#L366) for generating its own services, and then that handler [feeds](https://github.com/grpc/grpc-rust/blob/0858621ef6eda0f19928b3972181719f954f5bf2/tonic-build/src/lib.rs#L205) that doc-comment without formatting/sanitization straight into the Rust docs.

I can imagine and did implement a very rough test fix, right in the generator:

```diff
diff --git a/tonic-prost-build/Cargo.toml b/tonic-prost-build/Cargo.toml
index fd1efc2..9ced3e8 100644
--- a/tonic-prost-build/Cargo.toml
+++ b/tonic-prost-build/Cargo.toml
@@ -24,6 +24,7 @@ prost-types = { version = "0.14" }
prettyplease = { version = "0.2" }
proc-macro2 = "1.0"
quote = "1.0"
+regex = "1"
syn = "2.0"
tempfile = "3.0"

diff --git a/tonic-prost-build/src/lib.rs b/tonic-prost-build/src/lib.rs
index 055b511..da25762 100644
--- a/tonic-prost-build/src/lib.rs
+++ b/tonic-prost-build/src/lib.rs
@@ -363,8 +363,38 @@ impl ServiceGenerator {
}
}

+/// Sanitizes comment lines for rustdoc the same way prost-build does for
+/// message and field comments (`Comments::sanitize_line`, which is private):
+/// - escape bare URLs as ``
+/// - escape `[` & `]` if not already escaped and not part of a markdown link
+fn sanitize_comments(comments: &mut prost_build::Comments) {
+ use regex::Regex;
+ use std::sync::LazyLock;
+
+ static RULE_URL: LazyLock = LazyLock::new(|| Regex::new(r"https?://[^\s)]+").unwrap());
+ static RULE_BRACKETS: LazyLock =
+ LazyLock::new(|| Regex::new(r"(^|[^\]\\])\[(([^\]]*[^\\])?)\]([^(\[]|$)").unwrap());
+
+ let sanitize_line = |line: &mut String| {
+ let mut s = RULE_URL.replace_all(line, r"<$0>").to_string();
+ s = RULE_BRACKETS.replace_all(&s, r"$1\[$2\]$4").to_string();
+ *line = s;
+ };
+
+ for block in &mut comments.leading_detached {
+ block.iter_mut().for_each(sanitize_line);
+ }
+ comments.leading.iter_mut().for_each(sanitize_line);
+ comments.trailing.iter_mut().for_each(sanitize_line);
+}
+
impl prost_build::ServiceGenerator for ServiceGenerator {
- fn generate(&mut self, service: Service, buf: &mut String) {
+ fn generate(&mut self, mut service: Service, buf: &mut String) {
+ sanitize_comments(&mut service.comments);
+ for method in &mut service.methods {
+ sanitize_comments(&mut method.comments);
+ }
+
let tonic_service = TonicBuildService::new(service, self.codec_path.clone());

let mut builder = CodeGenBuilder::new();
```

This resolves the problem, but I'm not sure how naturally it lends itself to a production fix. The real fix is probably to adjust the visibility of sanitize_line in prost_build, maybe with some ownership model tweaks, and then adjust how we actually return the comments to be sanitized?

Contributor guide

Open the contributing guide

Research direction

Start in tonic-prost-build/src/lib.rs at ServiceGenerator::generate, then read prost-build's Comments sanitization mentioned in the report. Reproduce the issue with the minimal proto and cargo doc using deny(rustdoc::bare_urls). Done means generated RPC method comments escape bare URLs and cargo doc passes the rustdoc check.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
documentation
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
62/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.